Send one line terminator, not two - #15
Conversation
A tab travelled the whole pipeline as one character, so the wrap, the pane's width and MarkupWidth all counted it as one cell while the terminal painted it as a jump to the next tab stop - the layout computed against a width the screen does not use, which is the same defect class as chrome that grows on wire data. Measured: 'a\tb' claimed three cells. TextSettings.TabWidth, default 4, clamped 0-16 at the point of use rather than in the setter, because config.json is hand-edited and a negative number would otherwise throw out of the read loop on an ordinary line. Fixed spaces, not tab stops. A real tab's width depends on the column it starts in; tracking that buys alignment nobody asked for on output where a tab is a crude separator. A test says so out loud so it is not later 'fixed' into stop-tracking. Applied beside StripIncomingColour and before the triggers, so a pattern matches the spaces the reader sees rather than a tab they cannot know is there, and so a width change takes effect on the next line rather than the next restart. A line with no tab is returned unrebuilt - this runs on every line from every session.
The setting had no way to reach it from the UI. It is a count rather than a checkbox and it is the only thing on this screen that changes how wide a line *is* rather than how it looks, so it gets its own WHITESPACE section instead of a fifth row under COLOUR. Floor 0, not 1: zero is a real answer, it deletes tabs. Three tests moved with it. All three addressed rows positionally - the F7 screen is four checkboxes, so emoji was navigable row 3 and HasEditableRow was false. Emoji is row 4 now and the screen has an editable row, which puts '⏎ edit' back in its header. The doc comment asserting the old shape was corrected rather than left to mislead.
TelnetSession.SendLineAsync appended \r\n to the text and then handed it to TelnetInterpreter.SendAsync, which is itself a line send and appends the terminator. Every line this client has ever sent went out as <text>\r\n\r\n: a spurious empty command after every real one - each typed command, each macro, each timer, and the login line. It was visible and had been read as the server's doing. A MU* connect screen treats the empty line as a command like any other, and a server that redisplays its banner for an unrecognised one did so on every connect, which is why the welcome screen appears twice in the session transcripts before a login that in fact succeeded. Measured, not reasoned about: LineTerminatorTests asserts on the bytes the transport received, at both seams - SendLineAsync directly, and the login line through WorldSession, which is the one send a user cannot see or retype. Both fail on main and pass here. Identical on TelnetNegotiationCore 2.6.5 and 2.7.0, so this predates the 2.7.0 bump rather than coming from it. Also fixes the two TUnitAssertions0005 warnings in TabExpansionTests: a const compared to a literal is folded away and pins nothing. The values are now read through TextSettings, which pins the fact that can drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
Comment |
1 participant
What this is — and what it is not
This does not fix the reported auto-connect failure. I could not reproduce one, and the evidence says there is no regression in the dial or login path. What it does fix is a real wire defect found while investigating, which is the visible half of what was reported: the doubled welcome banner on every connect.
What the symptom actually is
Of the four candidates, it is #3 — it connects, the login line is sent, and the server does not accept it — and it is character-specific. Two transcripts from the same launch (
22:27:31), same code path:logs/Convergence MUSH.Mannaz-…-222731.log—*** Connecting→*** Connected.→*** Sent the login line for Mannaz.→ banner →Last connect was from … Fri Jul 31 01:14:01 2026→ the OOC Nexus room withMannaz Byron … 0sidle. Mannaz auto-connected and auto-logged in.logs/Convergence MUSH.Seidre-Zwei-…-222731.log— same three***lines, same banner, then EOF at line 54. Never logged in. Same at the22:18launch.These are live transcripts written this evening, so the banner is not restored scrollback: the
*** Connecting/Connected/Sent the login linerows are the live session's ownPrintSystemoutput.The same code, in one launch, logged one character in and was refused for another. A patch-induced regression cannot be character-selective like that. This server has an account layer (
login/register/play, withconnectdocumented as "Legacy alternative to play"), so the likely cause is Seidre-Zwei's stored credential or account state, not the code.All four prime suspects killed
Reproduced headlessly against a real loopback server with the real
TcpTransport,TelnetSessionandDefaultSessionFactory(soRequestOptions = [MsspOption]is live), the user's configuration shape (three characters marked at start on one world, resumed workspace) and the user's own restore-log content copied out. All three dial, all three connect, each putsconnect <name> <password>on the wire.IAC DO MSSP— tested against a server that never answers and one that answersIAC WILL MSSPplus a real MSSP subnegotiation. The login still goes out, intact, after the option request. No effect on the dial.MsspCacheislock-guarded andPersist()swallows IO errors; in the live clientOnUi→EnqueueOnUIThread, so it runs insideDrainUIActionQueue, which catches. It cannot fault a dial.The bug this does fix
TelnetSession.SendLineAsyncappended\r\nand then handed the bytes toTelnetInterpreter.SendAsync, which is itself a line send and appends the terminator. Every line the client has ever sent went out as<text>\r\n\r\n— a spurious empty command after every real one, including the login line.On a MU* connect screen that empty line is a command like any other, and a server that redisplays its banner for an unrecognised one did so on every connect. The doubled welcome screen in the transcripts is ours, not the server's.
Identical on TelnetNegotiationCore 2.6.5 and 2.7.0, so it predates the 2.7.0 bump.
Tests
LineTerminatorTestsasserts on the bytes the transport received — not a return value, sinceSendLineAsyncreturns as soon as the send is handed off. Two seams:SendLineAsyncdirectly, and the login line throughWorldSession, which is the one send a user cannot see or retype.On
main:With the fix:
total: 2 failed: 0 succeeded: 2.Five suites, all green, build warning-free (0 warnings):
Core 806(804 + 2 new) ·Graphics 83·Scripting 42·Web 37·Tui 1337Also clears the two pre-existing
TUnitAssertions0005warnings inTabExpansionTests— aconstcompared to a literal is folded away and pins nothing; the values now read throughTextSettings.Two things to know
main(c6fcd18), which is two commits ahead oforigin/main. It therefore also carries0b11113andc6fcd18(the tab-width work), because theTabExpansionTestsfix cannot exist without them.SharpMUTerm.Tui.csprojprobes..\..\..\SharpConsoleUI\…. From the main checkout that resolves to/home/grave/RiderProjects/SharpConsoleUIand takes a ProjectReference to the clone; from.claude/worktrees/agent-*/it resolves to a path that does not exist and silently falls back to the NuGet package 2.5.14. The clone currently has two locally modified files not in any release (Window.State.cs,Layout/ControlBounds.cs). Your binary and any worktree agent's differ in the UI framework, and nothing says so at build time.Nothing outside the repo was written;
LiveLogDirectoryGuardpassed in the green Tui run.🤖 Generated with Claude Code
https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM