Correct the brief: the encoding seed does not reach MSSP - #11
Merged
Conversation
The request was "ALT-1 2 3 4 5, or CTRL-1 2 3 4 5 — go to those numbered panes and bring them to the forefront". Alt, and nine rather than five. Ctrl+digit is not a chord this terminal has, read off a pty with `kitten @ send-key` at a raw-mode reader rather than remembered: every Alt+digit is ESC + the digit, while Ctrl+1/9/0 are the bare digits, Ctrl+2 is NUL, Ctrl+3 is Escape, Ctrl+4–7 are 0x1C–0x1F and Ctrl+8 is Backspace. Three of those are keys this client cannot afford to bind over and three are indistinguishable from typing. MacroKeys.DigitBytes records it per digit, which is what F4 now prints instead of "Ctrl+digit is dropped". Nine because that is the range the framework's own Alt+digit handler claims. InputCoordinator.HandleAltInput selects among top-level windows by index; it is reached from ProcessInput's fall-through for any unhandled Alt chord and — unlike the move and resize handlers beside it — is not gated on IsMovable/IsResizable, so Movable(false) did not switch it off. All nine digits are claimed as application shortcuts, in range or not: an out-of-range ⌥7 reports and stops rather than falling through to it. ⌥0 is left free, and stays bindable as a macro. The number is the rail's number: Layout.Panes order, which is what the sidebar's hosting column spells `pane N`. PaneLabel — the move and drag overlays — called the first pane `main`, the spelling the rail abandoned because `▪ main main` is two meanings in one line; the same pane read `pane 1` in the sidebar and `main` under the cursor. Harmless until a chord had to land on the pane a label names. Bringing a pane forward is the full Activate, so the command line ends up talking to the character whose pane you landed on. A zoom is carried with the jump (WorkspaceLayout.CarryZoomToFocused): a zoomed workspace realises one pane, so a mover that left the zoom behind put the selection, the session and the caret on a pane that is not on the screen. ⌃O had that bug and gets the same fix; the directional movers cannot and refuse out loud. Advertised in --help, in ⌃P (one `Go to pane N` entry per pane that exists, only on a split), and in MacroKeys.AppShortcuts so F4 says which pane has each digit and a claim with no action fails at startup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
Alt+1-9 jump to a numbered pane, matching the numbering the rail already paints. Ctrl+digit is deliberately unbound: the terminal sends Escape for Ctrl+3, Backspace for Ctrl+8, NUL for Ctrl+2 and bare digits for Ctrl+1/9. Also carries an existing zoom through ordinal moves, fixing the same latent bug in Ctrl+O.
Move mode lettered the panes a-j while the prompt one line below named the target 'pane N' - one ordering spelt two ways, so using the feature meant translating 'B' into 'pane 2' in your head. The badge is now the pane's own ordinal: the number the sidebar's hosting column prints, the number the prompt names, and the number Alt+N jumps to. Capped at 9 rather than 10 because there is no tenth digit; a tenth pane keeps no badge and stays a mouse drop target. An out-of-range digit leaves the current target alone, as an unmapped letter did.
Carries the five fixes merged upstream today: GMCP/MSDP/TTABLE no longer truncate at 8192 bytes silently, bodyless and separator-less GMCP messages are delivered, MSDP-over-GMCP forwards the JSON body instead of the package name, and MSSP keeps every value a variable carries. Verified the package actually contains them rather than trusting the number: SubnegotiationBuffer, MSDPOverGMCPPackage, TTABLE_REJECTED, OnGMCPMessageTooLarge and MSSPConfig.Variables are all present in lib/net10.0. All six suites green on it.
…hannel Rewrite and respond have always run their template through match.Result; the route did not, so one rule could only ever feed one statically-named window. ^<(.+?)> routing to 'Channel $1' now opens a pane per channel, each created on that channel's first line. The resolved name is guarded where the other two templates are not, because it becomes a durable named object - a window id, a tab title, a sidebar row - built from whatever the server sent. A name that is empty, over 64 characters, carries a control character, or still holds a group reference the pattern cannot supply is refused, and the line simply is not routed. Regex.Result leaves an out-of-range $2 as literal text rather than throwing, which is why that last case needs a guard of its own. No ceiling on how many panes a rule may open: a pattern that captures more loosely than its author meant is theirs to tighten, and a client-imposed cap would silently drop the channel they cared about. The route now carries its rule's pattern (SpawnRoute) instead of the Tui looking the rule up by comparing SpawnTarget to the window's name - a lookup that finds nothing once the name is dynamic, which would have left every such pane without its capture header. The F2 editor's '$1..$9 insert captures' caption now sits over the route field too.
The byte-level MsspSubnegotiationParser existed for one reason — 2.6.0's
MSSP reader destroyed the protocol's own data inside the library, before
any consumer could see the bytes. PORT "80" "23" "4201" arrived as the
integer 80234201, REFERRAL (array-only, and what a crawler follows)
arrived null, booleans failed to bind from 1/0, CHARSET and every
invented name were dropped, CRAWL_DELAY/MINIMUM_AGE bound to nothing, and
a variable with no value wedged MSSP for the rest of the connection.
All six are fixed upstream (PR #56, shipped in 2.6.5, which this repo
already takes). MSSPConfig.Variables is now an ordered name → value-list
map, so:
- MsspSubnegotiationParser is deleted, along with TelnetSession's
parallel wire scan and the degraded MsspSource.Interpreter fallback
path it gated. The library's own callback is the one path again.
- MsspData is now a projection with no parsing in it, built from
MSSPConfig.Variables by MsspData.From. It survives because what it adds
is ours rather than the protocol's: REFERRAL read as crawlable
MsspHosts with normalising equality, CRAWL DELAY -1 read as "no
preference" rather than a negative interval, ports validated as ports,
and an immutable snapshot a report can be written from.
- MsspVariables keeps only the names our accessors read by. The
vocabulary rules — Canonicalise, IsOfficial, the 45-name official list
— are now TelnetNegotiationCore.Models.MSSPVariables', derived from the
same model that reads the wire. Two copies of a vocabulary drift.
- MsspSource is gone: there is one source now. ObservationLog no longer
writes a "source" field.
The cases that pinned the parser now run against a real telnet session
over the scripted server and assert on what MSSPConfig.Variables yields:
multi-value PORT, REFERRAL as a list, the underscore and stray-whitespace
spellings, unknown and unofficial variables, an empty value, a variable
with no value (and that the rest of the report survives it), a payload
split one byte per read, and another option's subnegotiation carrying the
MSSP bytes by coincidence. All equivalent.
Two are NOT equivalent, and are pinned by name as upstream defects rather
than quietly accepted:
- MSSPProtocol.FlushField decodes every field with Encoding.ASCII rather
than the session's CurrentEncoding, so a non-ASCII NAME/WEBSITE/CONTACT
comes back as question marks, one per byte ("Café" -> "Caf??"). The
deleted parser read each field with whatever CHARSET had settled on.
- IAC IAC inside a value loses the literal 0xFF: the state machine moves
EscapingMSSPVal -> EvaluatingMSSPVal on IAC but registers no capture
handler for that trigger. Only reachable on a payload MSSP forbids, so
it is a one-byte loss rather than a lost report.
Also: MSSP has no payload size cap upstream. SubnegotiationBuffer guards
GMCP, MSDP and CHARSET's TTABLE; the MSSP field buffer is unbounded, so
the parser's 64 KiB ceiling has no replacement and a hostile server can
decide how much a crawler allocates for it. Reported, not patched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
Panes were counted in tree order, so a pane's number was a function of where it sat: dropping a window on the left edge of pane 2 made that pane into pane 3, and Alt+2 stopped meaning what it meant while the user was doing something else. PaneNode now carries a per-workspace creation Sequence, persisted in LayoutNodeState, and WorkspaceLayout.Panes sorts by it — so a pane keeps its number for as long as it is open. The number is the index in that list rather than the sequence itself, which is what compacts the numbering on a close: shut pane 2 of three and the survivors are 1 and 2, not 1 and 3 with Alt+2 silently doing nothing. A layout restored without sequences (a config written before the field) is seeded from tree order, which is the numbering it was saved under, so an existing workspace comes back reading exactly as it was left. Ctrl+O cycles in the same order. It read tree order, which agreed with the numbering back when the numbering was tree order; the two ordinal movers have to count one sequence. Alt+N was already global — it indexes the workspace's one split tree, not the active character's windows — but only the active character's window rows were drawn, so the other panes' numbers were invisible from anywhere you could use them. Every character row now carries the pane its session is in. The owner filter stays: a window row under a character still means that window is theirs. It costs no width — across every multi-pane snapshot view the frame changes by exactly the six cells of the new column, and no pane rectangle moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
Delete the MSSP parser we wrote around TelnetNegotiationCore's bugs, now that 2.6.5 reads MSSP correctly. MsspData survives as a parsing-free projection: Referrals returns a crawler type, CrawlDelay reads -1 as no preference, and MSSPVariableCollection is mutable and cleared per report. Three upstream defects found and pinned rather than papered over: MSSP fields decode as hardcoded ASCII, IAC IAC in a value loses the byte, and MSSP has no payload size cap.
Number panes by creation order, persisted, so splitting a pane no longer renames the panes beside it. Closing one compacts, keeping Alt+N contiguous. Ctrl+O counts the same way, since the two ordinal movers disagreeing is worse than either ordering. Every character row in the rail now names the pane its session is in, so Alt+N across characters is discoverable without loosening the owner filter that makes a window row mean 'this window is theirs'.
⌃B < / > reordered the focused pane's tabs in the workspace model and then called RefreshTabTitles, which repaints each TabPage by its own Tag and leaves the pages in the order they were built in. TabControl has no way to move a page — TabPages is a copy and the only mutators add — so the strip on screen never reordered. That is not a cosmetic lag, because the refusal is read against the model. With three tabs and the middle one active, ⌃B > swapped it to the end in the model while the screen still showed it in the middle; the move looked like a no-op, and the next press refused with "the tab is already at that end of the strip" about a tab the user could see was not. Rebuilding the pane area is what every other ⌃B command that changes what the strip holds already does, and the kept viewports survive it (ScrollViewFor), so scroll positions do not move. The test is the whole 3 tabs × 2 directions × 3 positions grid, asserting the strip as well as the model: the model half passed throughout, which is why two tabs and a model-only assertion never caught this. The two existing reorder tests compared order with IsEquivalentTo, which is order-insensitive in TUnit and so asserted nothing about order at all; both now compare ordered strings and check the strip too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
The hosting column read 'pane 3' behind a two-space gap. The sidebar's width is taken out of the pane area and reported to every connected session over NAWS, so four cells on every row is four cells off every pane. It now reads '⌥3' behind one space: three cells where the column appears at all, down from seven. The sigil is not decoration. It names the key that goes there, which the words left the reader to infer, and it is what keeps the column apart from the unread badge that ends immediately before it - '2 3' is two numbers with nothing to tell them apart, and the word used to do that job. PaneLabel still says 'pane N' wherever the noun carries meaning: split pane 2 left, Go to pane 3, there is no pane 7. Same ordinal, shorter spelling, only in the one surface that pays for its width by the cell.
Moving a tab within its pane reordered the model and never repainted the strip, so the move looked like it did nothing - and the second press, judging a model the screen had stopped showing, refused with 'already at that end'. RefreshTabTitles repaints each page from its own Tag and cannot reorder pages; RebuildPaneArea, which every other strip-changing command already uses, can.
…session Restarting the client emptied every pane. The layout came back — LastSession persists the pane tree and the window registry — and the content did not, because nothing kept it: the file-backed spill purges its root on startup and says so in its own comment, and LastSession's doc explicitly excludes scrollback. The obvious fix is to persist WorldSession.Scrollback, and it is wrong. A spawn window's lines never reach it: ProcessOutputLine raises SpawnLine and the shell appends the result straight into that window's markup buffer, and the ordinary channel rule gags the line so it is not in the transcript at all. A session-keyed restore therefore refills the main windows and hands the reader an empty Chat pane, which is the failure this exists to remove. RestoreLogEndToEndTests pins that premise before anything is built on it. So the log is keyed by window id, the same ids LastSession already records, and it is fed from the shell at OnLine and OnSpawnLine — not from AppendWindowLine, which also carries the client's own chrome and the restore replay and would have each launch re-record its own history. The two halves are joined loosely: a window in the log the saved workspace forgot is buffered anyway and its pane opens holding its history when that channel next speaks; a saved window with no log starts empty. Neither throws. One 0600 file per window under restore/, beside config.json and secrets.json. Header, then length-prefixed CRC-32 records whose payload is StyledLineCodec — markup would have frozen the lines in one theme's colours and lost the palette indices, the rule colour and every span's interaction. Appends flush to the OS per line, so a crash loses nothing and a polite exit is not required; there is deliberately no fsync per line. The bound is 500 lines per window and is in lines, never bytes, because a byte-bounded ring hangs for ever making room for a block bigger than itself. Space is reclaimed by compaction: a byte-range copy of the newest records through an atomic rename, amortising to one extra record written per record appended. Damage degrades rather than throws, which matters because the only caller is startup: a record that fails its checksum is skipped, and reading stops only where the framing itself stops making sense, so a crash's torn tail costs the newest few lines and a bad sector costs one. Restored content is closed off by a single RestoreBarRenderer row naming how much came back and when; the lines themselves are left in the game's own colours, since recolouring them to prove they were restored would destroy the thing being restored. Restoring 3,000 lines costs ~18 ms before the first frame (~5.5 ms of app construction becomes ~23 ms), of which ~2.8 ms is reading and decoding. F9 gains a `restore` row per character; clearing it stops the writing and drops that character's stored content on the next launch. ⌃P gains "Purge the restore log", which deletes everything now and keeps working. `restore:` is the third member of the save:/logRoot: family and is null by default, so no test and no snapshot owns one. Also: the CRC-32 the spill kept privately is now shared with the restore log rather than copied, and PaneContentFor fills a newly created control from its window's buffer, which is what lets a forgotten window's history reappear.
Panes refill from the previous session on restart. Fed from OnLine and OnSpawnLine rather than the single AppendWindowLine seam: that seam also carries client chrome and the restore replay itself, so logging there would have re-recorded each launch's own history and doubled every pane. Spawn windows had to be logged separately because the ordinary channel rule gags its line, so a channel pane's content is not in the session transcript at all - a session-keyed restore would have returned every channel pane empty. 500 lines per window, bounded in lines so the ceiling cannot be reached by one oversized record. Append-as-you-go flushed to the OS but not fsynced: a crash loses nothing, and a device round-trip per line of output is not affordable.
The count was already there. Workspace.NoteActivity has always kept it, ActivateWindow and SetScrolledBack have always cleared it, the rail has always drawn it, and TabTitles has always appended "(n)" - OutputScrollbackTests even reads it off the tab strip. Two things were missing. The tab had no colour. TabTitles said tab titles were plain text and so a colour could not ride on the label; TabControl.Rendering parses every label with MarkupParser.Parse and measures it - the paint, the desired width, and the click hit test alike - with MarkupParser.StripLength. A tag therefore costs no cells and moves no hit test, which is what makes a tint affordable on a surface where a cell may not be spent. The name and the count are drawn in the app accent; the focus marker stays outside it, because focus and activity are independent and a tab can have both. The two cues are different channels: focus is said entirely in backgrounds from the theme's chrome family, activity in a foreground no plane is painted in. That claim also meant a title was never escaped, so a window called [Chat] - or a web view titled from the page it loaded - had that eaten as a tag by the parser and by the hit test. Titles are escaped now. And the count was uncapped where the sidebar's was capped, so one number had two spellings: 99+ in the rail beside (4127) on the tab. Cap, field width and colour move into UnreadBadge, which both surfaces format through; the rail's hardcoded #00f5b7 becomes the palette's accent. The strip is deliberately not given the rail's reserved-width treatment. A rail row's width sizes the sidebar's column and the pane area is what is left over, so a badge appearing there narrows every pane; a tab strip is a TabControl arranged Fill+Stretch inside the pane it already fills, and the framework pads the header out to the pane's edge, so a longer label only shifts the tabs beside it. Reserving three cells per tab would cost width on every strip for ever to prevent a reflow this layout cannot produce. ActivityMovesNoPaneRectangle proves it across 0 -> 1 -> 9 -> 10 -> 99 -> 100 -> 150 at four terminal sizes, and ActivityTellsNoServerANewSize proves it on the bytes. Writing that test found the real one. The status row's scrollback segment counts lines below the viewport - a number that grows unbidden from the wire - and wrote it out raw. Its own comment warned that a wordier phrasing had once wrapped the row and that a status line growing a row takes one off the workspace; the number was never guarded. At 80 columns the 99 -> 100 step took the row from 80 cells to 81, it wrapped, every pane lost a row, and per-pane NAWS re-announced the new size to every connected server. It is now in a reserved field capped the same way. Clearing is untouched and stays untouched: both surfaces render from WorkspaceWindow.Unread, so picking the tab of a scrolled-back window clears neither badge and returning to the live tail clears both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
The tab title already carried the unread count. What was missing was the tint and the cap. TabTitles claimed SharpConsoleUI renders tab titles as plain text; it does not - TabControl parses them as markup and measures with StripLength on all three paths, so a colour tag costs no cells and moves no hit test. That stale claim also left titles unescaped, so a window called [Chat] had its name swallowed as a tag by the parser and the click test alike. Width is deliberately not reserved on a tab, unlike the rail: a strip is arranged Fill+Stretch inside the pane it already fills, so a longer label shifts its neighbours rather than resizing anything. Proving that turned up a live bug in the status row, which did reserve nothing for the scrollback distance: at 80 columns the 99 to 100 step took the row to 81 cells, it wrapped, every pane lost a row, and per-pane NAWS re-announced the size to every connected server.
…es not CLAUDE.md said the seeded CurrentEncoding is used for GMCP, MSDP and MSSP. It is right about the first two -- GMCPProtocol calls context.CurrentEncoding.GetString throughout and MSDPServerHandler passes telnet.CurrentEncoding -- and wrong about the third. MSSPProtocol.FlushField decodes every name and value with a hardcoded Encoding.ASCII, so a game named Cafe Noir with an accent reports one question mark per byte regardless of what CHARSET settled on or what we seed. The bytes are gone before any callback sees them. Recorded rather than worked around, with the caveat that it is arguably conformant: RFC 2066 scopes CHARSET to text and not commands, and a subnegotiation is a command. The complaint that survives is narrower -- it is lossy where it need not be, and Encoding.Latin1 would round-trip all 256 values at no cost. That is the upstream PR. Two details a reader would otherwise rediscover the hard way: the plaintext MSSP-REQUEST fallback does not go through MSSPProtocol at all, so the same server read the two ways disagrees byte for byte; and at 2.6.0 the same call is inlined at four sites which also ToUpper() names with the current culture, a Turkish-I hazard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughChangesThe change adds crash-tolerant per-window restore logs, stable creation-order pane numbering with numeric shortcuts, structured trigger spawn routes, updated unread/tab rendering, and TelnetNegotiationCore-based MSSP handling. Extensive core, crawler, and TUI tests cover persistence, navigation, rendering, and protocol behavior. Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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: 9
🤖 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 @.gitignore:
- Around line 63-75: Update the ignore rules and comments near config.json to
include secrets.json, ensuring plaintext credentials cannot be staged in Git.
Correct the restore log description from per-pane to per-window while preserving
the existing restore/ and *.log ignore entries.
In `@src/SharpMUTerm.Core/Telnet/Mssp/MsspData.cs`:
- Around line 204-229: Update MsspData.From to tolerate null variable names and
value lists from deserialized file data: guard the input before calling
MSSPVariables.Canonicalize, and skip entries whose canonical name is empty or
whose list is null before AddRange. Preserve aggregation and ordering for valid
entries.
In `@src/SharpMUTerm.Core/Text/RestoreLog.cs`:
- Around line 800-822: Update the compaction bookkeeping after File.Move
succeeds so _offsets is rebased or rebuilt to match the newly compacted file
before any stream reopening can fail. Ensure the catch path’s append-mode
recovery cannot retain offsets from the pre-compaction file, and preserve valid
indexing for subsequent Append and compaction operations.
In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs`:
- Around line 4320-4325: Update RailPaneLabel so panes with ordinals beyond
CommandIds.PaneJumpDigits use a non-chord label instead of generating an
unsupported ⌥10-style value; preserve the existing ⌥ordinal format for ordinals
through the supported single-digit range and align the fallback with the
established pane-label conventions.
In `@tests/SharpMUTerm.Core.Tests/Text/RestoreLogTests.cs`:
- Around line 307-314: Update the damage setup in the restore-log test to locate
the third record’s encoded text within its payload rather than using
Array.IndexOf over the entire file. Anchor the search to the known record text
and mutate a byte in that text, preserving the length prefix and framing so only
the record checksum fails.
- Around line 50-56: Update DefaultRoot_SitsBesideTheConfigurationAndItsSecrets
to construct the configuration input from a rooted temporary path rather than
the Unix-specific "/somewhere" literal, while preserving the expected directory
relationship and RestoreLogOptions.DirectoryName assertion across platforms.
In `@tests/SharpMUTerm.Crawler.Tests/MsspParsingTests.cs`:
- Around line 139-165: Remove the exact MSSPVariables.Official.Count == 45
assertion from the test, keeping the existing membership assertions as the
contract; if a size check is required, change it to a lower-bound assertion
instead.
In `@tests/SharpMUTerm.Tui.Tests/RestoreBarRendererTests.cs`:
- Around line 48-62: The test comment for ItCarriesOnlyTheTagsItBuilt claims an
unusual culture is supplied, but the test does not set one. Either configure
CultureInfo.CurrentCulture to a deliberately unusual culture for the assertion
and restore it afterward, or remove the unsupported culture-related sentence
from the documentation; keep the tag-count assertions unchanged.
In `@tests/SharpMUTerm.Tui.Tests/RestoreLogEndToEndTests.cs`:
- Around line 146-153: Update the boundary assertion for bar in the restore log
test to require bar > 0 before accessing lines[bar - 1], ensuring the test
reports a failed assertion when no restored line precedes the restore boundary.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f583593-45e7-4fa5-820f-c9f8f9a4da7f
📒 Files selected for processing (59)
.gitignoreCLAUDE.mdDirectory.Packages.propsdocs/design/README.mddocs/design/SharpMUTerm-TUI-v3.dc.htmlsrc/SharpMUTerm.Core/Automation/TriggerEngine.cssrc/SharpMUTerm.Core/Commands/CommandCatalog.cssrc/SharpMUTerm.Core/Commands/CommandIds.cssrc/SharpMUTerm.Core/Configuration/AppConfiguration.cssrc/SharpMUTerm.Core/Configuration/CharacterDefinition.cssrc/SharpMUTerm.Core/Configuration/WorldDefinition.cssrc/SharpMUTerm.Core/Session/SessionEvents.cssrc/SharpMUTerm.Core/Session/WorldSession.cssrc/SharpMUTerm.Core/Telnet/Mssp/MsspData.cssrc/SharpMUTerm.Core/Telnet/Mssp/MsspSubnegotiationParser.cssrc/SharpMUTerm.Core/Telnet/Mssp/MsspVariables.cssrc/SharpMUTerm.Core/Telnet/TelnetSession.cssrc/SharpMUTerm.Core/Text/Crc32.cssrc/SharpMUTerm.Core/Text/FileScrollbackSpill.cssrc/SharpMUTerm.Core/Text/RestoreLog.cssrc/SharpMUTerm.Core/Text/RestoreLogOptions.cssrc/SharpMUTerm.Core/Workspace/LayoutNode.cssrc/SharpMUTerm.Core/Workspace/WorkspaceLayout.cssrc/SharpMUTerm.Core/Workspace/WorkspaceState.cssrc/SharpMUTerm.Core/Workspaces/RailModel.cssrc/SharpMUTerm.Crawler/Output/ObservationLog.cssrc/SharpMUTerm.Tui/Glyphs.cssrc/SharpMUTerm.Tui/MacroKeys.cssrc/SharpMUTerm.Tui/Program.cssrc/SharpMUTerm.Tui/RailRenderer.cssrc/SharpMUTerm.Tui/RestoreBarRenderer.cssrc/SharpMUTerm.Tui/SharpMUTermApp.cssrc/SharpMUTerm.Tui/TabTitles.cssrc/SharpMUTerm.Tui/TriggersScreenRenderer.cssrc/SharpMUTerm.Tui/UnreadBadge.cssrc/SharpMUTerm.Tui/WorldsScreenRenderer.cstests/SharpMUTerm.Core.Tests/Automation/TriggerEngineTests.cstests/SharpMUTerm.Core.Tests/Commands/CommandCatalogTests.cstests/SharpMUTerm.Core.Tests/Text/RestoreLogTests.cstests/SharpMUTerm.Core.Tests/Workspace/PaneNumberingTests.cstests/SharpMUTerm.Core.Tests/Workspace/WorkspaceLayoutTests.cstests/SharpMUTerm.Crawler.Tests/BackoffTests.cstests/SharpMUTerm.Crawler.Tests/MsspParsingTests.cstests/SharpMUTerm.Crawler.Tests/PersistenceTests.cstests/SharpMUTerm.Crawler.Tests/ProbeTests.cstests/SharpMUTerm.Crawler.Tests/ReferralTests.cstests/SharpMUTerm.Crawler.Tests/Support/FakeProbe.cstests/SharpMUTerm.Crawler.Tests/Support/MsspWire.cstests/SharpMUTerm.Crawler.Tests/Support/ScriptedTransport.cstests/SharpMUTerm.Tui.Tests/MacroKeyCaptureTests.cstests/SharpMUTerm.Tui.Tests/PaneJumpTests.cstests/SharpMUTerm.Tui.Tests/PaneNumberingRailTests.cstests/SharpMUTerm.Tui.Tests/PanePrefixEndToEndTests.cstests/SharpMUTerm.Tui.Tests/RailWindowRowTests.cstests/SharpMUTerm.Tui.Tests/RestoreBarRendererTests.cstests/SharpMUTerm.Tui.Tests/RestoreLogEndToEndTests.cstests/SharpMUTerm.Tui.Tests/ScreenModelTests.cstests/SharpMUTerm.Tui.Tests/TabActivityIndicatorTests.cstests/SharpMUTerm.Tui.Tests/TabTitlesTests.cs
💤 Files with no reviewable changes (2)
- src/SharpMUTerm.Core/Telnet/Mssp/MsspSubnegotiationParser.cs
- src/SharpMUTerm.Core/Text/FileScrollbackSpill.cs
Comment on lines
+63
to
+75
| # logging settings point, the scrollback spill is an ephemeral cache under XDG_CACHE_HOME, and | ||
| # the restore log is a `restore/` directory beside config.json holding one bounded file per pane. | ||
| # They are listed because a character's password is persisted in config.json in plaintext, so | ||
| # the one realistic route into git is a human copying a real config in to reproduce something. | ||
| config.json | ||
| client-diagnostics-*.log | ||
| *.log | ||
|
|
||
| # The restore log's own directory. Its files are already covered by the `*.log` above — that is why | ||
| # they carry that extension — but the directory is named too, because "why is this ignored" should be | ||
| # answerable by reading this file rather than by knowing which extension the format happened to pick. | ||
| restore/ | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Ignore secrets.json and correct the storage description.
The current configuration stores plaintext passwords in secrets.json and only a reference in config.json (src/SharpMUTerm.Core/Configuration/CharacterDefinition.cs, src/SharpMUTerm.Tui/Program.cs). This block omits secrets.json, so a normal save can leave credentials unignored and stage them in Git. It also describes restore files as per-pane, while the restore contract is per-window.
Suggested fix
# logging settings point, the scrollback spill is an ephemeral cache under XDG_CACHE_HOME, and
-# the restore log is a `restore/` directory beside config.json holding one bounded file per pane.
-# They are listed because a character's password is persisted in config.json in plaintext, so
-# the one realistic route into git is a human copying a real config in to reproduce something.
+# the restore log is a `restore/` directory beside config.json holding one bounded file per window.
+# config.json contains password references; plaintext credentials live in secrets.json, so both
+# files are ignored to keep a copied configuration from putting credentials into git.
config.json
+secrets.json📝 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
| # logging settings point, the scrollback spill is an ephemeral cache under XDG_CACHE_HOME, and | |
| # the restore log is a `restore/` directory beside config.json holding one bounded file per pane. | |
| # They are listed because a character's password is persisted in config.json in plaintext, so | |
| # the one realistic route into git is a human copying a real config in to reproduce something. | |
| config.json | |
| client-diagnostics-*.log | |
| *.log | |
| # The restore log's own directory. Its files are already covered by the `*.log` above — that is why | |
| # they carry that extension — but the directory is named too, because "why is this ignored" should be | |
| # answerable by reading this file rather than by knowing which extension the format happened to pick. | |
| restore/ | |
| # logging settings point, the scrollback spill is an ephemeral cache under XDG_CACHE_HOME, and | |
| # the restore log is a `restore/` directory beside config.json holding one bounded file per window. | |
| # config.json contains password references; plaintext credentials live in secrets.json, so both | |
| # files are ignored to keep a copied configuration from putting credentials into git. | |
| config.json | |
| secrets.json | |
| client-diagnostics-*.log | |
| *.log | |
| # The restore log's own directory. Its files are already covered by the `*.log` above — that is why | |
| # they carry that extension — but the directory is named too, because "why is this ignored" should be | |
| # answerable by reading this file rather than by knowing which extension the format happened to pick. | |
| restore/ |
🤖 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 @.gitignore around lines 63 - 75, Update the ignore rules and comments near
config.json to include secrets.json, ensuring plaintext credentials cannot be
staged in Git. Correct the restore log description from per-pane to per-window
while preserving the existing restore/ and *.log ignore entries.
Comment on lines
+204
to
+229
| public static MsspData From(IEnumerable<KeyValuePair<string, IReadOnlyList<string>>> variables) | ||
| { | ||
| private readonly Dictionary<string, List<string>> _values = []; | ||
| private readonly List<string> _order = []; | ||
|
|
||
| /// <summary> | ||
| /// Records one value of one variable. Repeating a variable appends to its list rather than | ||
| /// replacing it, which is what makes the two ways MSSP spells an array — repeated variables and | ||
| /// repeated values — end up in one place and keep their order. | ||
| /// </summary> | ||
| public Builder Add(string variable, string value) | ||
| { | ||
| List(variable)?.Add(value); | ||
| return this; | ||
| } | ||
| ArgumentNullException.ThrowIfNull(variables); | ||
|
|
||
| /// <summary> | ||
| /// Records that a variable was sent without recording a value for it. A variable with no | ||
| /// <c>MSSP_VAL</c> at all is malformed, but "the server mentioned this and said nothing" is a | ||
| /// different fact from "the server never mentioned it", and inventing an empty value to carry | ||
| /// the first would erase the difference. | ||
| /// </summary> | ||
| public Builder Declare(string variable) | ||
| { | ||
| List(variable); | ||
| return this; | ||
| } | ||
| var values = new Dictionary<string, List<string>>(StringComparer.Ordinal); | ||
| var order = new List<string>(); | ||
|
|
||
| private List<string>? List(string variable) | ||
| foreach (var (variable, list) in variables) | ||
| { | ||
| var name = MsspVariables.Canonicalise(variable); | ||
| var name = MSSPVariables.Canonicalize(variable); | ||
| if (name.Length == 0) | ||
| { | ||
| return null; | ||
| continue; | ||
| } | ||
|
|
||
| if (!_values.TryGetValue(name, out var list)) | ||
| if (!values.TryGetValue(name, out var accumulated)) | ||
| { | ||
| list = []; | ||
| _values[name] = list; | ||
| _order.Add(name); | ||
| accumulated = []; | ||
| values[name] = accumulated; | ||
| order.Add(name); | ||
| } | ||
|
|
||
| return list; | ||
| accumulated.AddRange(list); | ||
| } | ||
|
|
||
| public bool IsEmpty => _order.Count == 0; | ||
|
|
||
| public MsspData Build() => | ||
| new(_values.ToDictionary(kv => kv.Key, kv => (IReadOnlyList<string>)kv.Value), [.. _order], source); | ||
| return new MsspData(values.ToDictionary(kv => kv.Key, kv => (IReadOnlyList<string>)kv.Value), order); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
From will NRE on a null value list — the doc explicitly names the file-read path where that can happen.
The XML doc says the other source is "a flat dictionary read back from a file". A JSON record whose variables entry has a null array (an ObservationLog line hand-edited, or written by a build where the property was null) deserializes to KeyValuePair<string, IReadOnlyList<string>> with a null value, and accumulated.AddRange(list) dereferences it. MSSPVariables.Canonicalize(variable) with a null name is the same shape of risk.
🛡️ Proposed guard
foreach (var (variable, list) in variables)
{
- var name = MSSPVariables.Canonicalize(variable);
+ var name = MSSPVariables.Canonicalize(variable ?? string.Empty);
if (name.Length == 0)
{
continue;
}
if (!values.TryGetValue(name, out var accumulated))
{
accumulated = [];
values[name] = accumulated;
order.Add(name);
}
- accumulated.AddRange(list);
+ if (list is not null)
+ {
+ accumulated.AddRange(list);
+ }
}📝 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
| public static MsspData From(IEnumerable<KeyValuePair<string, IReadOnlyList<string>>> variables) | |
| { | |
| private readonly Dictionary<string, List<string>> _values = []; | |
| private readonly List<string> _order = []; | |
| /// <summary> | |
| /// Records one value of one variable. Repeating a variable appends to its list rather than | |
| /// replacing it, which is what makes the two ways MSSP spells an array — repeated variables and | |
| /// repeated values — end up in one place and keep their order. | |
| /// </summary> | |
| public Builder Add(string variable, string value) | |
| { | |
| List(variable)?.Add(value); | |
| return this; | |
| } | |
| ArgumentNullException.ThrowIfNull(variables); | |
| /// <summary> | |
| /// Records that a variable was sent without recording a value for it. A variable with no | |
| /// <c>MSSP_VAL</c> at all is malformed, but "the server mentioned this and said nothing" is a | |
| /// different fact from "the server never mentioned it", and inventing an empty value to carry | |
| /// the first would erase the difference. | |
| /// </summary> | |
| public Builder Declare(string variable) | |
| { | |
| List(variable); | |
| return this; | |
| } | |
| var values = new Dictionary<string, List<string>>(StringComparer.Ordinal); | |
| var order = new List<string>(); | |
| private List<string>? List(string variable) | |
| foreach (var (variable, list) in variables) | |
| { | |
| var name = MsspVariables.Canonicalise(variable); | |
| var name = MSSPVariables.Canonicalize(variable); | |
| if (name.Length == 0) | |
| { | |
| return null; | |
| continue; | |
| } | |
| if (!_values.TryGetValue(name, out var list)) | |
| if (!values.TryGetValue(name, out var accumulated)) | |
| { | |
| list = []; | |
| _values[name] = list; | |
| _order.Add(name); | |
| accumulated = []; | |
| values[name] = accumulated; | |
| order.Add(name); | |
| } | |
| return list; | |
| accumulated.AddRange(list); | |
| } | |
| public bool IsEmpty => _order.Count == 0; | |
| public MsspData Build() => | |
| new(_values.ToDictionary(kv => kv.Key, kv => (IReadOnlyList<string>)kv.Value), [.. _order], source); | |
| return new MsspData(values.ToDictionary(kv => kv.Key, kv => (IReadOnlyList<string>)kv.Value), order); | |
| public static MsspData From(IEnumerable<KeyValuePair<string, IReadOnlyList<string>>> variables) | |
| { | |
| ArgumentNullException.ThrowIfNull(variables); | |
| var values = new Dictionary<string, List<string>>(StringComparer.Ordinal); | |
| var order = new List<string>(); | |
| foreach (var (variable, list) in variables) | |
| { | |
| var name = MSSPVariables.Canonicalize(variable ?? string.Empty); | |
| if (name.Length == 0) | |
| { | |
| continue; | |
| } | |
| if (!values.TryGetValue(name, out var accumulated)) | |
| { | |
| accumulated = []; | |
| values[name] = accumulated; | |
| order.Add(name); | |
| } | |
| if (list is not null) | |
| { | |
| accumulated.AddRange(list); | |
| } | |
| } | |
| return new MsspData(values.ToDictionary(kv => kv.Key, kv => (IReadOnlyList<string>)kv.Value), order); |
🤖 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/SharpMUTerm.Core/Telnet/Mssp/MsspData.cs` around lines 204 - 229, Update
MsspData.From to tolerate null variable names and value lists from deserialized
file data: guard the input before calling MSSPVariables.Canonicalize, and skip
entries whose canonical name is empty or whose list is null before AddRange.
Preserve aggregation and ordering for valid entries.
Comment on lines
+800
to
+822
|
|
||
| _stream.Dispose(); | ||
| File.Move(temporary, _path, overwrite: true); | ||
|
|
||
| _stream = new FileStream(_path, FileMode.Open, FileAccess.Write, FileShare.Read); | ||
| _stream.Seek(0, SeekOrigin.End); | ||
|
|
||
| var shift = keepFrom - _dataStart; | ||
| var kept = _offsets.Skip(_offsets.Count - maxLines).Select(o => o - shift).ToList(); | ||
| _offsets.Clear(); | ||
| _offsets.AddRange(kept); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // Compaction failing is not worth losing the log over: the file simply keeps growing | ||
| // until the next attempt succeeds, and the reader takes the newest `maxLines` anyway. | ||
| _logger.LogDebug(ex, "Compacting the restore log for window {WindowId} failed", _windowId); | ||
| TryRemove(temporary); | ||
| if (!_stream.CanWrite) | ||
| { | ||
| _stream = new FileStream(_path, FileMode.Append, FileAccess.Write, FileShare.Read); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Compaction that fails after the rename leaves _offsets describing the old file.
If File.Move (Line 802) succeeds but reopening the stream (Line 804) throws, the catch path reopens in append mode yet _offsets still holds pre-compaction offsets. The list stays above 2 * maxLines, so the next Append compacts again with keepFrom = _offsets[^maxLines] pointing past the end of the now-shorter file — source.Seek beyond length copies nothing and the window's whole log is reduced to a header. Rebase the offsets as soon as the rename succeeds (or clear them and re-index) so the in-memory index can never outlive the file it describes.
🛠️ Suggested restructuring of the post-rename bookkeeping
_stream.Dispose();
File.Move(temporary, _path, overwrite: true);
+ // Rebase before anything else can throw: from here on the file is the compacted one,
+ // so an index describing the old one must not survive a failure below.
+ var shift = keepFrom - _dataStart;
+ var kept = _offsets.Skip(_offsets.Count - maxLines).Select(o => o - shift).ToList();
+ _offsets.Clear();
+ _offsets.AddRange(kept);
+
_stream = new FileStream(_path, FileMode.Open, FileAccess.Write, FileShare.Read);
_stream.Seek(0, SeekOrigin.End);
-
- var shift = keepFrom - _dataStart;
- var kept = _offsets.Skip(_offsets.Count - maxLines).Select(o => o - shift).ToList();
- _offsets.Clear();
- _offsets.AddRange(kept);📝 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
| _stream.Dispose(); | |
| File.Move(temporary, _path, overwrite: true); | |
| _stream = new FileStream(_path, FileMode.Open, FileAccess.Write, FileShare.Read); | |
| _stream.Seek(0, SeekOrigin.End); | |
| var shift = keepFrom - _dataStart; | |
| var kept = _offsets.Skip(_offsets.Count - maxLines).Select(o => o - shift).ToList(); | |
| _offsets.Clear(); | |
| _offsets.AddRange(kept); | |
| } | |
| catch (Exception ex) | |
| { | |
| // Compaction failing is not worth losing the log over: the file simply keeps growing | |
| // until the next attempt succeeds, and the reader takes the newest `maxLines` anyway. | |
| _logger.LogDebug(ex, "Compacting the restore log for window {WindowId} failed", _windowId); | |
| TryRemove(temporary); | |
| if (!_stream.CanWrite) | |
| { | |
| _stream = new FileStream(_path, FileMode.Append, FileAccess.Write, FileShare.Read); | |
| } | |
| } | |
| _stream.Dispose(); | |
| File.Move(temporary, _path, overwrite: true); | |
| // Rebase before anything else can throw: from here on the file is the compacted one, | |
| // so an index describing the old one must not survive a failure below. | |
| var shift = keepFrom - _dataStart; | |
| var kept = _offsets.Skip(_offsets.Count - maxLines).Select(o => o - shift).ToList(); | |
| _offsets.Clear(); | |
| _offsets.AddRange(kept); | |
| _stream = new FileStream(_path, FileMode.Open, FileAccess.Write, FileShare.Read); | |
| _stream.Seek(0, SeekOrigin.End); | |
| } | |
| catch (Exception ex) | |
| { | |
| // Compaction failing is not worth losing the log over: the file simply keeps growing | |
| // until the next attempt succeeds, and the reader takes the newest `maxLines` anyway. | |
| _logger.LogDebug(ex, "Compacting the restore log for window {WindowId} failed", _windowId); | |
| TryRemove(temporary); | |
| if (!_stream.CanWrite) | |
| { | |
| _stream = new FileStream(_path, FileMode.Append, FileAccess.Write, FileShare.Read); | |
| } | |
| } |
🤖 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/SharpMUTerm.Core/Text/RestoreLog.cs` around lines 800 - 822, Update the
compaction bookkeeping after File.Move succeeds so _offsets is rebased or
rebuilt to match the newly compacted file before any stream reopening can fail.
Ensure the catch path’s append-mode recovery cannot retain offsets from the
pre-compaction file, and preserve valid indexing for subsequent Append and
compaction operations.
Comment on lines
+4320
to
+4325
| /// The sigil is also what keeps the column legible beside the unread badge. A bare <c>3</c> after a | ||
| /// count of <c>2</c> is <c>2 3</c>, two numbers with nothing to tell them apart; the word used to do | ||
| /// that work, and something has to. | ||
| /// </para> | ||
| /// </summary> | ||
| private static string RailPaneLabel(int ordinal) => $"⌥{ordinal}"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Past nine panes the sidebar names a chord that does not exist.
RailPaneLabel is called for every pane (line 4206), so a tenth pane's rows read ⌥10 — a key nothing registers and the terminal cannot deliver. CommandCatalog already withholds the subtitle past CommandIds.PaneJumpDigits, and _moveOrdinals' own doc states the rule ("a badge whose key does not exist is worse than no badge"), so this is the one surface still claiming it.
🐛 Fall back to a non-chord spelling past the ninth pane
- private static string RailPaneLabel(int ordinal) => $"⌥{ordinal}";
+ private static string RailPaneLabel(int ordinal) =>
+ ordinal <= CommandIds.PaneJumpDigits ? $"⌥{ordinal}" : $"p{ordinal}";📝 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
| /// The sigil is also what keeps the column legible beside the unread badge. A bare <c>3</c> after a | |
| /// count of <c>2</c> is <c>2 3</c>, two numbers with nothing to tell them apart; the word used to do | |
| /// that work, and something has to. | |
| /// </para> | |
| /// </summary> | |
| private static string RailPaneLabel(int ordinal) => $"⌥{ordinal}"; | |
| /// The sigil is also what keeps the column legible beside the unread badge. A bare <c>3</c> after a | |
| /// count of <c>2</c> is <c>2 3</c>, two numbers with nothing to tell them apart; the word used to do | |
| /// that work, and something has to. | |
| /// </para> | |
| /// </summary> | |
| private static string RailPaneLabel(int ordinal) => | |
| ordinal <= CommandIds.PaneJumpDigits ? $"⌥{ordinal}" : $"p{ordinal}"; |
🤖 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/SharpMUTerm.Tui/SharpMUTermApp.cs` around lines 4320 - 4325, Update
RailPaneLabel so panes with ordinals beyond CommandIds.PaneJumpDigits use a
non-chord label instead of generating an unsupported ⌥10-style value; preserve
the existing ⌥ordinal format for ordinals through the supported single-digit
range and align the fallback with the established pane-label conventions.
Comment on lines
+50
to
+56
| public async Task DefaultRoot_SitsBesideTheConfigurationAndItsSecrets() | ||
| { | ||
| // The config directory, not a cache one: unlike the spill these files are meant to survive. | ||
| var config = Path.Combine("/somewhere", "SharpMUTerm", "config.json"); | ||
| await Assert.That(RestoreLog.DefaultRoot(config)) | ||
| .IsEqualTo(Path.Combine("/somewhere", "SharpMUTerm", RestoreLogOptions.DirectoryName)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This assertion only holds on Unix. DefaultRoot runs the path through Path.GetFullPath, so on Windows "/somewhere/SharpMUTerm/config.json" resolves to the current drive (C:\somewhere\SharpMUTerm) while the expectation stays drive-less — the test fails there. Build the input from a rooted temp path instead of a literal /somewhere.
🛠️ Drive-agnostic fixture
- var config = Path.Combine("/somewhere", "SharpMUTerm", "config.json");
+ var directory = Path.Combine(Path.GetTempPath(), "SharpMUTerm");
+ var config = Path.Combine(directory, "config.json");
await Assert.That(RestoreLog.DefaultRoot(config))
- .IsEqualTo(Path.Combine("/somewhere", "SharpMUTerm", RestoreLogOptions.DirectoryName));
+ .IsEqualTo(Path.Combine(directory, RestoreLogOptions.DirectoryName));📝 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
| public async Task DefaultRoot_SitsBesideTheConfigurationAndItsSecrets() | |
| { | |
| // The config directory, not a cache one: unlike the spill these files are meant to survive. | |
| var config = Path.Combine("/somewhere", "SharpMUTerm", "config.json"); | |
| await Assert.That(RestoreLog.DefaultRoot(config)) | |
| .IsEqualTo(Path.Combine("/somewhere", "SharpMUTerm", RestoreLogOptions.DirectoryName)); | |
| } | |
| public async Task DefaultRoot_SitsBesideTheConfigurationAndItsSecrets() | |
| { | |
| // The config directory, not a cache one: unlike the spill these files are meant to survive. | |
| var directory = Path.Combine(Path.GetTempPath(), "SharpMUTerm"); | |
| var config = Path.Combine(directory, "config.json"); | |
| await Assert.That(RestoreLog.DefaultRoot(config)) | |
| .IsEqualTo(Path.Combine(directory, RestoreLogOptions.DirectoryName)); | |
| } |
🤖 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 `@tests/SharpMUTerm.Core.Tests/Text/RestoreLogTests.cs` around lines 50 - 56,
Update DefaultRoot_SitsBesideTheConfigurationAndItsSecrets to construct the
configuration input from a rooted temporary path rather than the Unix-specific
"/somewhere" literal, while preserving the expected directory relationship and
RestoreLogOptions.DirectoryName assertion across platforms.
Comment on lines
+307
to
+314
| // Flip a byte inside the third record's payload. Its length prefix is untouched, so the frames | ||
| // after it are still findable; only its own checksum fails. | ||
| var path = Directory.GetFiles(root.Path).Single(); | ||
| var bytes = File.ReadAllBytes(path); | ||
| var third = Array.IndexOf(bytes, (byte)'3'); | ||
| await Assert.That(third).IsGreaterThan(0); // the fixture found what it meant to damage | ||
| bytes[third] = (byte)'X'; | ||
| File.WriteAllBytes(path, bytes); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The damage fixture can land on a length prefix rather than the payload. Array.IndexOf(bytes, (byte)'3') matches the first 0x33 byte anywhere in the file, and a record's length prefix or codec field can legitimately be 51. Flipping that byte breaks framing instead of one record's checksum, which makes the test fail for a reason it is not about. Locate the encoded text instead.
🛠️ Anchor on the record's text
- var third = Array.IndexOf(bytes, (byte)'3');
+ var needle = System.Text.Encoding.UTF8.GetBytes("line 3");
+ var third = bytes.AsSpan().IndexOf(needle) + needle.Length - 1;
await Assert.That(third).IsGreaterThan(0); // the fixture found what it meant to damage🧰 Tools
🪛 OpenGrep (1.26.0)
[WARNING] 310-310: File operation with dynamic path can lead to path traversal. Validate and sanitize file paths against a safe base directory.
(coderabbit.path-traversal.csharp-file-read)
🤖 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 `@tests/SharpMUTerm.Core.Tests/Text/RestoreLogTests.cs` around lines 307 - 314,
Update the damage setup in the restore-log test to locate the third record’s
encoded text within its payload rather than using Array.IndexOf over the entire
file. Anchor the search to the known record text and mutate a byte in that text,
preserving the length prefix and framing so only the record checksum fails.
Comment on lines
+139
to
+165
| await Assert.That(MSSPVariables.Official.Count).IsEqualTo(45); | ||
|
|
||
| foreach (var required in new[] { "NAME", "PLAYERS", "UPTIME" }) | ||
| { | ||
| await Assert.That(MsspVariables.IsOfficial(required)).IsTrue(); | ||
| await Assert.That(MSSPVariables.IsOfficial(required)).IsTrue(); | ||
| } | ||
|
|
||
| foreach (var spaced in new[] { "CRAWL DELAY", "MINIMUM AGE", "XTERM 256 COLORS", "PAY TO PLAY", "HIRING CODERS" }) | ||
| { | ||
| await Assert.That(MsspVariables.IsOfficial(spaced)).IsTrue(); | ||
| await Assert.That(MsspVariables.IsOfficial(spaced.Replace(' ', '_'))).IsTrue(); | ||
| await Assert.That(MSSPVariables.IsOfficial(spaced)).IsTrue(); | ||
| await Assert.That(MSSPVariables.IsOfficial(spaced.Replace(' ', '_'))).IsTrue(); | ||
| } | ||
|
|
||
| // Unofficial but widely deployed: recognised, and correctly not claimed as official. | ||
| await Assert.That(MsspVariables.IsKnownUnofficial("PUEBLO")).IsTrue(); | ||
| await Assert.That(MsspVariables.IsOfficial("PUEBLO")).IsFalse(); | ||
| // The two the specification's tables omit and 2.6.5 added: CHARSET and DISCORD. | ||
| await Assert.That(MSSPVariables.IsOfficial(MsspVariables.Charset)).IsTrue(); | ||
| await Assert.That(MSSPVariables.IsOfficial("DISCORD")).IsTrue(); | ||
|
|
||
| // Wholly unknown: neither, and still kept by the parser. | ||
| await Assert.That(MsspVariables.IsOfficial("CORVID SPECIFIC")).IsFalse(); | ||
| await Assert.That(MsspVariables.IsKnownUnofficial("CORVID SPECIFIC")).IsFalse(); | ||
| // Unofficial but widely deployed: modelled, and correctly not claimed as official. | ||
| await Assert.That(MSSPVariables.IsKnown("PUEBLO")).IsTrue(); | ||
| await Assert.That(MSSPVariables.IsOfficial("PUEBLO")).IsFalse(); | ||
|
|
||
| // Wholly unknown: neither, and still kept in the report. | ||
| await Assert.That(MSSPVariables.IsOfficial("CORVID SPECIFIC")).IsFalse(); | ||
| await Assert.That(MSSPVariables.IsKnown("CORVID SPECIFIC")).IsFalse(); | ||
|
|
||
| // UTF-8 keeps its hyphen; nothing in the folding touches it. | ||
| await Assert.That(MsspVariables.Canonicalise("utf-8")).IsEqualTo("UTF-8"); | ||
| await Assert.That(MSSPVariables.Canonicalize("utf-8")).IsEqualTo("UTF-8"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP --type=cs 'MSSPVariables\.\w+' -o --no-filename | sort -uRepository: SharpMUSH/SharpMUTerm
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## files\n'
git ls-files | rg 'Mssp|MSSP|Variables|Crawler'
printf '\n## symbol usages\n'
rg -n --hidden --glob '!**/bin/**' --glob '!**/obj/**' 'MSSPVariables|MsspVariables|IsKnownUnofficial|Canonicali[sz]e' .
printf '\n## target test excerpt\n'
sed -n '120,180p' tests/SharpMUTerm.Crawler.Tests/MsspParsingTests.csRepository: SharpMUSH/SharpMUTerm
Length of output: 11503
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a -t f 'MSSP' .
fd -a -t f 'Mssp' .Repository: SharpMUSH/SharpMUTerm
Length of output: 626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## test file\n'
cat -n tests/SharpMUTerm.Crawler.Tests/MsspParsingTests.cs | sed -n '130,175p'
printf '\n## candidate source files\n'
fd -a -t f 'Mssp' . || true
fd -a -t f 'MSSP' . || true
printf '\n## repository-wide symbol hits\n'
rg -n --hidden --glob '!**/bin/**' --glob '!**/obj/**' 'MSSPVariables|MsspVariables|IsKnownUnofficial|Canonicalize|Canonicalise' .Repository: SharpMUSH/SharpMUTerm
Length of output: 10125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/SharpMUTerm.Core/Telnet/Mssp/MsspVariables.cs | sed -n '1,240p'Repository: SharpMUSH/SharpMUTerm
Length of output: 2514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/SharpMUTerm.Core/Telnet/Mssp/MsspData.cs | sed -n '1,260p'Repository: SharpMUSH/SharpMUTerm
Length of output: 12671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## MsspVariables.cs\n'
cat -n src/SharpMUTerm.Core/Telnet/Mssp/MsspVariables.cs | sed -n '1,240p'
printf '\n## MsspData.cs\n'
cat -n src/SharpMUTerm.Core/Telnet/Mssp/MsspData.cs | sed -n '1,260p'Repository: SharpMUSH/SharpMUTerm
Length of output: 15062
Avoid pinning the MSSP official list to an exact size
MSSPVariables.Official.Count == 45 will break on the next upstream MSSP addition even though the membership assertions already cover the intended contract. Use a lower bound or drop the count check entirely.
🤖 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 `@tests/SharpMUTerm.Crawler.Tests/MsspParsingTests.cs` around lines 139 - 165,
Remove the exact MSSPVariables.Official.Count == 45 assertion from the test,
keeping the existing membership assertions as the contract; if a size check is
required, change it to a lower-bound assertion instead.
Comment on lines
+48
to
+62
| /// <summary> | ||
| /// Nothing in it comes from a world, so nothing in it can carry markup — but the date is | ||
| /// culture-formatted and a locale's month abbreviation is not this code's to vouch for, so it goes | ||
| /// through the same escape every other composed row does. Asserted by feeding a culture whose date | ||
| /// format is unusual and checking the row still has exactly the tags it was built with. | ||
| /// </summary> | ||
| [Test] | ||
| public async Task ItCarriesOnlyTheTagsItBuilt() | ||
| { | ||
| var bar = RestoreBarRenderer.Bar(9, Whenever, Accent); | ||
|
|
||
| // Two opening tags (the accent and the dim) and two closes, and no stray bracket anywhere else. | ||
| await Assert.That(bar.Count(c => c == '[')).IsEqualTo(4); | ||
| await Assert.That(bar.Count(c => c == ']')).IsEqualTo(4); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The doc comment promises a culture the test never sets. ItCarriesOnlyTheTagsItBuilt runs under the ambient culture, so the "feeding a culture whose date format is unusual" claim is untrue and the escape path it describes is unexercised. Either set CultureInfo.CurrentCulture for the duration of the assertion or drop that sentence.
🤖 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 `@tests/SharpMUTerm.Tui.Tests/RestoreBarRendererTests.cs` around lines 48 - 62,
The test comment for ItCarriesOnlyTheTagsItBuilt claims an unusual culture is
supplied, but the test does not set one. Either configure
CultureInfo.CurrentCulture to a deliberately unusual culture for the assertion
and restore it afterward, or remove the unsupported culture-related sentence
from the documentation; keep the tag-count assertions unchanged.
Comment on lines
+146
to
+153
| var bar = lines.FindIndex(l => l.Contains(RestoreBarRenderer.Label, StringComparison.Ordinal)); | ||
| await Assert.That(bar).IsGreaterThanOrEqualTo(0); | ||
| await Assert.That(lines.Count(l => l.Contains(RestoreBarRenderer.Label, StringComparison.Ordinal))) | ||
| .IsEqualTo(1); | ||
|
|
||
| // Everything above it is the previous session and everything below it is this one — including | ||
| // the connect banner, which is this session announcing itself and belongs on its own side. | ||
| await Assert.That(lines[bar - 1]).Contains("The Grand Plaza"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
lines[bar - 1] throws when the bar is the first row. The guard only asserts bar >= 0, so a regression that restores nothing but still draws the boundary fails with an IndexOutOfRangeException instead of the assertion that describes the defect. Assert bar > 0 (there must be a restored line above it) before indexing.
🛠️ Tighten the guard
- await Assert.That(bar).IsGreaterThanOrEqualTo(0);
+ await Assert.That(bar).IsGreaterThan(0); // restored content sits above it📝 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
| var bar = lines.FindIndex(l => l.Contains(RestoreBarRenderer.Label, StringComparison.Ordinal)); | |
| await Assert.That(bar).IsGreaterThanOrEqualTo(0); | |
| await Assert.That(lines.Count(l => l.Contains(RestoreBarRenderer.Label, StringComparison.Ordinal))) | |
| .IsEqualTo(1); | |
| // Everything above it is the previous session and everything below it is this one — including | |
| // the connect banner, which is this session announcing itself and belongs on its own side. | |
| await Assert.That(lines[bar - 1]).Contains("The Grand Plaza"); | |
| var bar = lines.FindIndex(l => l.Contains(RestoreBarRenderer.Label, StringComparison.Ordinal)); | |
| await Assert.That(bar).IsGreaterThan(0); // restored content sits above it | |
| await Assert.That(lines.Count(l => l.Contains(RestoreBarRenderer.Label, StringComparison.Ordinal))) | |
| .IsEqualTo(1); | |
| // Everything above it is the previous session and everything below it is this one — including | |
| // the connect banner, which is this session announcing itself and belongs on its own side. | |
| await Assert.That(lines[bar - 1]).Contains("The Grand Plaza"); |
🤖 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 `@tests/SharpMUTerm.Tui.Tests/RestoreLogEndToEndTests.cs` around lines 146 -
153, Update the boundary assertion for bar in the restore log test to require
bar > 0 before accessing lines[bar - 1], ensuring the test reports a failed
assertion when no restored line precedes the restore boundary.
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
1 participant
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.
CLAUDE.mdclaimed the seededCurrentEncodingis used for GMCP, MSDP and MSSP. It is right about two of the three.CurrentEncodingGMCPProtocolcallscontext.CurrentEncoding.GetString(...)throughoutMSDPServerHandlerpassestelnet.CurrentEncodingtoMSDPLibrary.ReportMSSPProtocol.FlushFieldhardcodesEncoding.ASCII.GetString(...)So a game whose MSSP
NAMEisCafé NoirreportsCaf? Noirwhatever CHARSET settled on and whatever we seed — the bytes are gone beforeCallbackOnByteAsyncsees them.Documented rather than worked around, and deliberately not overstated: RFC 2066 scopes CHARSET to text, not commands, and a subnegotiation is a command, so this is arguably conformant. The narrower complaint is the defensible one — it is lossy where it needn't be, and
Encoding.Latin1would round-trip all 256 byte values at no cost. That is a good upstream PR against TelnetNegotiationCore.Two things a reader would otherwise rediscover the hard way are recorded with it: the plaintext
MSSP-REQUESTfallback never goes throughMSSPProtocol, so the same server read the two ways disagrees byte for byte; and at 2.6.0 the same call is inlined at four sites which alsoToUpper()variable names using the current culture — a Turkish-I hazard.Documentation only; no code change, no test change.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Alt+1–Alt+9, including matching move-mode and sidebar labels.Documentation