Give each connected character its own capture pane - #16
Merged
Conversation
Two connected characters whose triggers matched the same pattern shared one
capture pane. `Workspace.SpawnWindowId` was `$"spawn:{target}"` — no owner in
it — so the first session to match created the window *with its own session key
on it* and every later session's lines were appended to a pane somebody else
owned. That is worse than a mixed-up channel: the rail draws window rows for the
active character only, so the second character's own channel was filed under the
first and was invisible from the character it belonged to.
The id now carries the owner, length-prefixed:
`spawn:{owner.Length}:{owner}:{target}`, or `spawn:-:{target}` for a window
nobody owns. The length is what makes it unambiguous rather than merely usually
fine — a session key and a target are both user-controlled free text and either
may hold a colon, so `spawn:{owner}:{target}` spells ("a", "b:c") and ("a:b",
"c") identically, which is this very defect one shape rarer. Reading the digits,
taking exactly that many characters and consuming one more colon is total and
reversible, so distinct pairs cannot collide whatever is in them. It is an
encoding and not a digest because this string is a dictionary key, a value in
config.json and the stem of a RestoreLog file name, and all three want to stay
readable.
Windows with no owner keep sharing one bucket per target, deliberately: a live
WorldSession always has a key (its world's name when it has no character), so an
unowned spawn window cannot come from a connection at all, and the rail lists an
unowned window under *every* character — two of them for one target would draw
two identical rows everywhere with nothing able to tell them apart. Null is one
identity, "nobody", not an unknown.
Nothing on screen changes. Every surface that names a window reads
`WorkspaceWindow.Title`; the one place that derived a display name from an id
was a test helper, now asking the app for the title instead.
Migration, both halves, because the two stores are joined only by this id:
- `ConfigurationMigrator` v4 → v5 rewrites every saved spawn id in
`lastSession.windows` and in every pane's `tabs`, taking the owner from the
`sessionKey` that state has always persisted. Migrated rather than adopted or
dropped: adoption leaves a pane no id the client produces can ever reach, and
dropping throws away a pane the user had. The decision is made by schema
version and never by looking at the id, because a target is free text and can
be made to look like anything.
- `SharpMUTermApp.CarryLegacySpawnLogsOver` moves the restore log's content onto
the id its pane now has, at the launch that reads it, and drops the old file —
so it happens once and cannot replay the same lines twice. A log no pane
claims is left exactly where it was, as it always has been.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
…ds check `owner.Length < length + 1` passes for a declared length of int.MaxValue — the addition wraps negative — and the next line then indexes off the end of the string. `spawn:2147483647:abc:x` in a hand-edited config or a corrupt restore log file name threw IndexOutOfRangeException out of a reader whose whole contract is to answer false. Written as `owner.Length <= length`, which says the same thing and cannot wrap. 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: 34 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)
WalkthroughThe change introduces session-qualified spawn window IDs, updates routing and restore behavior, migrates persisted version 4 configurations and logs, and adds coverage for multi-session isolation. SharpConsoleUI now uses the pinned package only. ChangesSession-aware spawn windows
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CharacterSession
participant SharpMUTermApp
participant Workspace
participant RestoreLogFiles
CharacterSession->>SharpMUTermApp: capture target output
SharpMUTermApp->>Workspace: route by session key and target
Workspace-->>SharpMUTermApp: return session-qualified window
SharpMUTermApp->>RestoreLogFiles: append restored content
SharpMUTermApp-->>CharacterSession: display isolated capture pane
Possibly related PRs
🚥 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: 1
🤖 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 `@tests/SharpMUTerm.Tui.Tests/SpawnWindowPerSessionTests.cs`:
- Around line 133-143: Update the assertions in SpawnWindowPerSessionTests to
inspect only the Public window’s tab and rail row. Filter PaneTabTitles and
RailLines to the entries associated with Public before asserting the display
name, and remove the collection-wide DoesNotContain(Bob) checks; leave the
Public presence assertions intact.
🪄 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: 15a3d26f-2869-4d0a-9b6b-63f528664645
📒 Files selected for processing (26)
CLAUDE.mdsrc/SharpMUTerm.Core/Configuration/AppConfiguration.cssrc/SharpMUTerm.Core/Configuration/ConfigurationMigrator.cssrc/SharpMUTerm.Core/Workspace/Workspace.cssrc/SharpMUTerm.Tui/DemoScene.cssrc/SharpMUTerm.Tui/SharpMUTerm.Tui.csprojsrc/SharpMUTerm.Tui/SharpMUTermApp.cstests/SharpMUTerm.Core.Tests/Configuration/ConfigurationTests.cstests/SharpMUTerm.Core.Tests/Configuration/SpawnWindowIdMigrationTests.cstests/SharpMUTerm.Core.Tests/Workspace/SpawnWindowIdTests.cstests/SharpMUTerm.Core.Tests/Workspace/WorkspaceStateTests.cstests/SharpMUTerm.Core.Tests/Workspace/WorkspaceTests.cstests/SharpMUTerm.Tui.Tests/FocusIndicationTests.cstests/SharpMUTerm.Tui.Tests/HistorySearchEndToEndTests.cstests/SharpMUTerm.Tui.Tests/InputAreaEndToEndTests.cstests/SharpMUTerm.Tui.Tests/NawsPaneReportTests.cstests/SharpMUTerm.Tui.Tests/PanePrefixEndToEndTests.cstests/SharpMUTerm.Tui.Tests/PaneResizeEndToEndTests.cstests/SharpMUTerm.Tui.Tests/PaneTabCloseTests.cstests/SharpMUTerm.Tui.Tests/PrefixWhichKeyTests.cstests/SharpMUTerm.Tui.Tests/RailClickTests.cstests/SharpMUTerm.Tui.Tests/RailWindowRowTests.cstests/SharpMUTerm.Tui.Tests/RestoreLogEndToEndTests.cstests/SharpMUTerm.Tui.Tests/SessionStateAccountingTests.cstests/SharpMUTerm.Tui.Tests/SpawnWindowIdUpgradeTests.cstests/SharpMUTerm.Tui.Tests/SpawnWindowPerSessionTests.cs
#14 landed first, so this carries its rail rewrite, its window numbering and the removal of CapturePattern. The merge was textually clean and semantically not: SpawnWindowId takes an owner now, so #14's new tests called it with one argument, and its migration fixture asserted on a field #14 deleted. The review item on BothPanesAreStillCalledPublic was right that a frame-wide DoesNotContain is the wrong shape - it can pass for reasons unrelated to its claim - though not for the stated reason: the constant is the session key 'Convergence.Bob', not 'Bob', so the collections it warned about would not have matched it. Scoping the assertion found what the frame-wide one had been missing. A spawn tab reads 'Ann - Public (1)': owner label, name, unread count, in markup. So the test now reads that tab specifically, through StripMarkup, and pins the distinction that actually matters - the character's name is on the strip deliberately, so two characters capturing one target can be told apart, and the session key must never be. It still fails on the shared id: 4 of 5, including this one.
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.
The report
Reproduced first
Two characters on one world, both connected over their own transport, both assigned a trigger set that captures
^<Public>to aPublicwindow. Against9e6f242the workspace came back holding one window:One pane, owned by Ann, holding both characters' channel. And because
BuildRailWindowsfilters window rows to the active character, Bob's own channel was not merely mixed in — it was filed under Ann, so it was invisible from Bob and reachable only from the character it did not belong to.The cause
Workspace.SpawnWindowIdwas$"spawn:{target}". No owner in it, so one window per workspace per target: the first session to match created it with its own session key on it, and every later session's lines were appended to somebody else's pane.The id
spawn:{owner.Length}:{owner}:{target}, andspawn:-:{target}for a window nobody owns.Why the length prefix, and not a plain join. A session key and a target are both user-controlled free text and either may hold a colon — a world or character can be called
a:b, and a trigger'sSpawnTargetis whatever was typed on F2.spawn:{owner}:{target}spells("a", "b:c")and("a:b", "c")identically, which is this very defect one shape rarer. Reading the digits up to the first colon, taking exactly that many characters as the owner and consuming one more colon is a total, reversible encoding: distinct pairs cannot produce equal ids, whatever is in them.-is used for "no owner" rather than an empty length because it is not a decimal digit, so the two arms can never be confused. Pinned by an exhaustive cross-product over deliberately awkward names (SpawnWindowIdTests.DistinctPairsAlwaysGiveDistinctIds), not by a table of examples.Why an encoding and not a digest. This string is a dictionary key, a value in
config.json, and the filename stem of aRestoreLogfile. A hash would be unambiguous too and would make all three unreadable to whoever has to look at them, for no property a reversible encoding lacks. The file name's own collision handling is untouched:RestoreLogstores the full id in each file's header and refuses a file whose header names a different window, so a CRC-32 clash on the stem costs one window's log rather than mixing two.Windows with no owner keep sharing one bucket per target, deliberately. A live
WorldSessionalways has a key (its world's name when it has no character), so an unowned spawn window cannot come from a connection at all — the web view isAuxiliaryand never routed here. And the rail lists an unowned window under every character precisely because it belongs to none, so two of them for one target would draw two identical rows under everybody with nothing in the client able to tell them apart or route between them. Null is one identity, "nobody", not an unknown owner.Nothing on screen changed
Every surface that names a window reads
WorkspaceWindow.Title. The tab strip (TabTitles), the sidebar (RailWindowLabel) and the ⌃P catalog all do; the only thing anywhere deriving a display name from an id was a test helper (RailClickTests.SpawnTitle), now asking the app for the title instead.Checked against rendered frames rather than by reading the markup: twelve snapshot views involving the demo's Chat spawn window —
default,spawn,split,timestamps-toggled,connections,rail-long,move,drag,focus,collapsed,prefix-panel,scrollback— are byte-identical between9e6f242and this branch.Migration — both halves, because the two stores are joined only by this id
config.json(ConfigurationMigrator, v4 → v5). Every saved spawn id is rewritten, inlastSession.windowsand in every pane'stabsarray, taking the owner from thesessionKeythatWorkspaceWindowStatehas persisted all along — so the rewrite is a lookup, not a guess.Migrated rather than adopted or dropped. Adoption — leaving
spawn:Publicalone for whichever session claims the target — cannot work: nothing the running client now produces equals it, so the pane would come back and then sit there for ever with nothing writing to it while its channel filled a second pane beside it. That failure is reproduced in the falsification below (two:Chatwindows). Dropping avoids the orphan and throws away a pane the user had, plus — through the id the restore log is keyed by — the text in it.The decision is made by schema version and never by looking at the id. A target is free text and can be made to look like a v5 id, so classifying by shape would eventually mis-file one and produce exactly the orphan this step exists to prevent. Everything under a document that says version 4 is v4, by construction.
The restore log (
SharpMUTermApp.CarryLegacySpawnLogsOver). At the launch that reads it, a log filed under a pre-v5 id is copied onto the id its pane now has and the old file is dropped — so it happens once, and the next launch has nothing to map and cannot replay the same lines twice. Here shape is safe to judge by, and the test is all three of: the id does not parse as a current one, it names no window this workspace holds, and exactly one live spawn window claims its target. Anything short of that is left alone, which costs nothing: a log the workspace cannot place is buffered under its own id exactly as it always has been, so its pane refills if that channel ever speaks again.SpawnWindowIdUpgradeTestsdrives a genuine v4 document throughConfigurationStore.Deserializebeside a restore log written underspawn:Chat, and asserts the pane comes back in its pane, calledChat, owned by Corvid, holding the previous session's conversation — and that the live capture then writes to that same pane rather than a second one.Falsification
Three, each breaking one half and watching the right tests fail:
9e6f242: "Expected to containspawn:15:Convergence.Ann:Public, but the item was not found", and Ann's pane empty.MigrateV4ToV5returns immediately). 6 of 8 Core migration tests fail, and in the Tui suiteTheUpgradedPaneIsTheOneTheLiveCaptureWritesTofails with "Expected to be 1 but found 2" — the orphan pane beside the live one, exactly the failure mode this design rejects.CarryLegacySpawnLogsOverreturns empty). The pane comes back — and empty: "Expected to containRivane: anyone up for the crypt run?but received""". The upgrade would have cost the user their scrollback.Also fixed on the way
TryReadSpawnWindowId's bounds check was writtenowner.Length < length + 1, which wraps negative for a declared length ofint.MaxValueand then indexes off the end.spawn:2147483647:abc:xin a hand-edited config threwIndexOutOfRangeExceptionout of a reader whose whole contract is to answer false. Second commit, with the case pinned.Verification
dotnet build SharpMUTerm.slnxclean apart from the two knownTUnitAssertions0005warnings inTabExpansionTests, and all five suites green:XDG_CONFIG_HOMEwas redirected for every run andUserDirectoryGuardpassed;~/.config/SharpMUTerm/was not written by this work.Two notes for the reviewer
This branch carries
9e6f242("Take SharpConsoleUI from NuGet, always"), which is not onorigin/main. It wasmainlocally when this work started and it is still unpushed, so it appears here as a third commit. It is not mine.Expected conflicts with #14 (
feat/tab-jump-alt-digit). Both touchWorkspace.cs— #14 addsWorkspaceWindow.SequenceandWorkspace.PlacedWindows; this rewritesRouteSpawnand replacesSpawnWindowId. The two are in different members and the textual conflict should be small. The one to watch isRouteSpawn, which is where a new window is registered: if #14 assigns a window sequence there, both edits land on the same few lines.WorkspaceWindow.csis untouched here. The test-side edits are one-line call-site updates (SpawnWindowId("Chat")→DemoScene.ChatWindowIdorSpawnWindowId(owner, target)) across the rail/pane suites #14 also rewrites, so expect a handful of trivial conflicts there.🤖 Generated with Claude Code
https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM
Summary by CodeRabbit
New Features
Refactor
Chores