Kill the MSSP crawler; surface MSSP in the client (F5 ▸ i) by HarryCordewener · Pull Request #13 · SharpMUSH/SharpMUTerm

HarryCordewener · 2026-07-31T01:15:01Z

Two commits, in this order because they touch overlapping Core code and one pass keeps the diff honest.

1. Drop the crawler

"let's drop the crawler aspect. instead, focus on the mssp info and build that in. kill the crawler"

src/SharpMUTerm.Crawler/ and tests/SharpMUTerm.Crawler.Tests/ are gone, with their SharpMUTerm.slnx entries and the Test — Crawler CI step. The if: ${{ !cancelled() }} on the remaining steps stays — it is independent of the crawler and earned its place.

src/SharpMUTerm.Core/Telnet/Mssp/, three files and three fates:


`MsspData.cs`	kept — the report the INFO screen renders, and already a projection of the library's reader rather than a parser. `Referrals` and `CrawlDelay` go with `MsspHost`.
`MsspVariables.cs`	kept — the names `MsspData`'s domain accessors read by, and the labels the screen uses. `REFERRAL` and `CRAWL DELAY` drop out with their accessors.
`MsspHost.cs`	deleted — referral parsing, scope classification and `IsCrawlable` are crawler-shaped end to end. `REFERRAL`'s raw values are still one indexer away.

MsspParsingTests moved to the Core suite rather than dying with the crawler: its subject was never the crawler but what TelnetNegotiationCore's MSSP reader does with arrays, booleans, spaced names and unknown variables — which is exactly the supply this feature is built on. It now drives a real TelnetSession.

TelnetSessionOptions.RequestOptions/MsspOption would otherwise have become dead. Deleting them was the wrong reading: the client wants what the crawler wanted, so WorldSession's session factory now sends IAC DO MSSP. The library opens with IAC WILL NAWS and nothing else, so a server that supports MSSP but waits to be asked is never asked — and an INFO screen reporting that as "publishes no MSSP" would be a claim about the server made out of our own silence.

Also swept: CLAUDE.md (six suites → five; the stale 2.6.5 pin; the two upstream MSSP defects 2.7.0 fixed), the design doc's crawler boundary, and a Directory.Packages.props console sink declared for one project that no longer exists.

2. The MSSP info screen

MSSP was parsed and thrown away — WorldSession.MsspReceived was raised and nothing in SharpMUTerm.Tui subscribed. It is now captured per server, persisted, and read from F5 ▸ i on the selected world.

Three design decisions, argued in the code:

Where it lives — not config.json. That file is what the user asked for and is hand-edited; a write per connect has no business landing there. A sibling mssp.json, versioned on its own, the shape SecretsStore and the restore log already have. Fourth of the save:/logRoot:/restore: family with one difference: the field is never null, because a MsspCache with no path is memory-only by construction — a structural guarantee rather than a null check at each use site.
Per host:port, not per world — MSSP describes a server; a world name is a user-editable label two entries may share and a rename changes out from under a report that has not. The one case that reads oddly (editing a port) is right: those are two endpoints, and one is usually the TLS one.
A second report replaces the first — MSSP is not a delta protocol. A merge keeps variables a server has stopped publishing for ever, and leaves a report that is a snapshot of no moment that ever existed.

Three states, not two. ConnectedAt is written on the Connected transition, ObservedAt only when a report arrives, so never dialled, dialled and this server publishes none, and here is the report, as of 3 days ago are three different screens. The middle one is the ordinary case on a MUSH and is worded as optional-and-normally-absent, with the world's own host, port and transport beside it.

Everything the server sent is shown — official and unofficial, told apart by a reserved mark column with a legend — and multi-valued variables (PORT, CODEBASE, REFERRAL) render as the lists they are, one value per row, because "least to most relevant" is meaning and the first value is a server's least preferred.

Wire data is bounded twice. Control characters replaced, raw text truncated before escaping (truncating escaped markup splits a [[), and column widths that are functions of the terminal and never of the data — at 80 columns a fixed 58-cell value column overruns by three. Capture is bounded at the door as well: a value only the renderer trimmed would still be full size on disk and in memory on every later launch.

The key is a key, not a chip. ScreenButtonKind gains Detail, Stops() trims it with Remove, and the WORLDS pane reads list → [+ world] → i info … → Del removes …. An INFO chip reached with ↑↓ would drag the selection to the last world and could only ever have reported on that one — the documented "only the last world can be deleted" bug, one feature later. i runs outside ScreenEdits: opening a screen is navigation, and routing it through the edit log would write config.json and re-periodise every running timer each time somebody looked at a world.

Snapshot views mssp / mssp-none / mssp-never drive the real i into a real F5; the demo report goes in through SharpMUTermApp.CaptureMssp, the same writer the wire uses, so a frame and a connection cannot file under different endpoints.

Verification

dotnet build SharpMUTerm.slnx — 0 warnings, 0 errors. All five suites green:

suite	before	after
Core	744	780
Graphics	83	83
Scripting	42	42
Web	37	37
Tui	1312	1335
~~Crawler~~	~~103~~	—

UserDirectoryGuard passed; no mssp.json in the real ~/.config/SharpMUTerm/.

Rendered frames read cell by cell at 80×24, 100×30, 120×32 and 160×48 — nothing overruns, and the F5 header at 80 columns is byte-identical to main (the i info hint is appended after Del remove so the destructive key survives truncation).

🤖 Generated with Claude Code

https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM

Summary by CodeRabbit

New Features
- Added MSSP server information capture, persistent caching, and connection/report status tracking.
- Added read-only server details accessible from the Worlds screen with the i key.
- Added support for viewing multi-valued, unofficial, and unavailable server data with scrolling and terminal-size-aware rendering.
Documentation
- Documented MSSP behavior, caching, server-info states, and the information screen.
Refactor
- Removed the standalone crawler and related workflow coverage.

The maintainer's call: "let's drop the crawler aspect. instead, focus on the mssp info and build that in. kill the crawler." Deleted src/SharpMUTerm.Crawler and tests/SharpMUTerm.Crawler.Tests, their solution entries, and the CI step. The 'if: !cancelled()' on the remaining steps stays — it is independent of the crawler and earned its place. Three files in Core/Telnet/Mssp and three fates: MsspData kept. It is the report the INFO screen renders, and it was already a projection of the library's reader rather than a parser. Referrals and CrawlDelay go with MsspHost. MsspVariables kept. Its names are what MsspData's domain accessors read by; REFERRAL and CRAWL DELAY drop out with their accessors. MsspHost deleted. Referral parsing, scope classification and IsCrawlable are crawler shape end to end; nothing else reads them, and REFERRAL's raw values are still one indexer away for anything that wants to audit them. MsspParsingTests moved to the Core suite rather than dying with the crawler. Its subject was never the crawler — it pins what TelnetNegotiationCore's MSSP reader does with arrays, booleans, spaced names and unknown variables, which is exactly the supply the INFO screen is built on. It now drives a real TelnetSession instead of the probe; MsspWire and ScriptedTransport came with it. TelnetSessionOptions.RequestOptions/MsspOption would otherwise have become dead, and deleting them would have been the wrong reading: the client wants what the crawler wanted. WorldSession's session factory now sends IAC DO MSSP. The library opens with IAC WILL NAWS and nothing else, so a server that supports MSSP but waits to be asked is never asked — and an INFO screen reporting that as "publishes no MSSP" would be making a claim about the server out of our own silence. Also swept: CLAUDE.md (six suites to five, the stale 2.6.5 pin, the two upstream MSSP defects that 2.7.0 fixed), the design doc's crawler boundary, and Directory.Packages.props' console sink — declared for one project, which is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM

"someone who has created a World area to look at the MSSP information in a nice UI. So an INFO button that goes to an MSSP read screen." MSSP was parsed and thrown away — WorldSession.MsspReceived was raised and nothing in the Tui subscribed. It is now captured per server, kept between launches, and read from the F5 Worlds & Characters screen. Three decisions, argued in the code: Where it lives. Not config.json. That file is what the user asked for and is hand-edited; a write per connect has no business landing there. It is a sibling cache, mssp.json, versioned on its own so nothing here can force an AppConfiguration migration — the shape SecretsStore and the restore log already have. Fourth of the save:/logRoot:/restore: family with one difference: the field is never null. A MsspCache with no path is memory-only *by construction*, so "a snapshot writes nothing" is a property of the object rather than a check at each use site, and the screen needs no "is there a cache" branch. Per host:port, not per world. MSSP describes a server; a world name is a user-editable label two entries may share, and a rename must not lose a report. Two worlds pointed at one server read one report. The one case that reads oddly — editing a world's port makes its report "vanish" — is right: those are two endpoints, and one of them is usually the TLS one. A second report replaces the first. MSSP is not a delta protocol; a server sends its whole table once per connection. A merge would keep variables it has stopped publishing for ever — accumulating room exits in a different costume — and would leave a report that is a snapshot of no moment that ever existed, which is exactly what a dated report exists not to be. Three states, not two. ConnectedAt is written on the Connected transition and ObservedAt only when a report arrives, so "never dialled", "dialled and this server publishes none" and "here is the report, as of 3 days ago" are three different screens. The middle one is the ordinary case on a MUSH and is worded as optional-and-normally-absent, with the world's own host, port and transport beside it; saying "no data" there is what makes a client look broken. Everything the server sent is shown, official and unofficial, told apart by a reserved mark column with a legend. Multi-valued variables (PORT, CODEBASE, REFERRAL) are drawn as the lists they are — one value per row, the name printed once — because "least to most relevant" is meaning and the first value is a server's *least* preferred. The values are a stranger's. Control characters are replaced, the raw text is truncated before it is escaped (truncating escaped markup splits a [[), and the column widths are functions of the terminal and never of the data — at 80 columns a fixed 58-cell value column overruns by three. Capture is bounded at the door as well as at the renderer: a value only the screen trimmed would still be full size on disk and in memory on every later launch. The key is a key, not a chip. ScreenButtonKind gains Detail, Stops() trims it with Remove, and the WORLDS pane reads list → [+ world] → i info … → Del removes …. An INFO chip reached with ↑↓ would drag the selection to the last world and could only ever have reported on that one — the documented "only the last world can be deleted" bug, one feature later. `i` runs outside ScreenEdits: opening a screen is navigation, and routing it through the edit log would write config.json and re-periodise every running timer each time somebody looked at a world. SettingsOverlay swaps content over a one-deep stack rather than stacking a second modal, so Esc goes back to the world you were on and the screen stays drivable headlessly. Views mssp / mssp-none / mssp-never drive the real `i` into a real F5, and the demo report goes in through SharpMUTermApp.CaptureMssp — the same writer the wire uses — so the frame and the connection cannot file under different endpoints. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM

coderabbitai · 2026-07-31T01:15:35Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 27 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2e4fd57d-6602-49d3-aec6-af86c701311d

📥 Commits

Reviewing files that changed from the base of the PR and between cd22fd8 and fd70845.

📒 Files selected for processing (10)

CLAUDE.md
README.md
docs/superpowers/specs/2026-07-30-structured-server-data-design.md
src/SharpMUTerm.Core/Telnet/Mssp/MsspCache.cs
src/SharpMUTerm.Tui/MsspScreenRenderer.cs
src/SharpMUTerm.Tui/MsspScreenView.cs
src/SharpMUTerm.Tui/ScreenModel.cs
src/SharpMUTerm.Tui/WorldsScreenRenderer.cs
tests/SharpMUTerm.Core.Tests/Telnet/MsspCacheTests.cs
tests/SharpMUTerm.Tui.Tests/MsspScreenTests.cs

Walkthrough

The change adds MSSP negotiation, bounded persistent caching, and read-only server-information screens with three connection/report states. It removes crawler projects and crawler-specific MSSP projections, updates Telnet dependency documentation, and adds Core and TUI coverage.

Changes

MSSP client and presentation

Layer / File(s)	Summary
MSSP observation and cache contract `src/SharpMUTerm.Core/Telnet/Mssp/`, `src/SharpMUTerm.Core/Session/WorldSession.cs`, `tests/SharpMUTerm.Core.Tests/Telnet/`	MSSP requests, endpoint observations, bounded replacement semantics, persistence, tolerant loading, and raw report parsing are implemented and tested.
MSSP screen and world navigation `src/SharpMUTerm.Tui/MsspScreen`, `src/SharpMUTerm.Tui/Screen`, `src/SharpMUTerm.Tui/Settings`, `src/SharpMUTerm.Tui/WorldsScreen`, `src/SharpMUTerm.Tui/SharpMUTermApp.cs`, `tests/SharpMUTerm.Tui.Tests/MsspScreenTests.cs`	The TUI captures MSSP observations, adds world INFO actions, opens read-only reports, restores the underlying screen, and renders populated, unavailable, and never-connected states.
Demo and startup integration `src/SharpMUTerm.Tui/DemoScene.cs`, `src/SharpMUTerm.Tui/Program.cs`	Demo MSSP data is added, and startup creates a persistent cache beside configuration while reporting cache diagnostics and help text.

Crawler removal and repository alignment

Layer / File(s)	Summary
Project, dependency, and documentation alignment `.github/workflows/ci.yml`, `CLAUDE.md`, `Directory.Packages.props`, `README.md`, `SharpMUTerm.slnx`, `docs/superpowers/specs/*`	Crawler projects and workflow references are removed, obsolete logging package management is deleted, and MSSP behavior and dependency documentation is updated.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

SharpMUSH/SharpMUTerm#7: Both changes modify TUI settings-screen infrastructure and detail-action wiring.
SharpMUSH/SharpMUTerm#11: Both changes concern MSSP handling, MsspData, and Telnet negotiation behavior.

Suggested reviewers: claude

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 51.91% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main changes: removing the MSSP crawler and exposing MSSP information in the client.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

_{Comment @coderabbitai help to get the list of available commands.}

Two things a --no-incremental build turned up that an incremental one hid. TUnit's HasCount() is [Obsolete] in favour of Count(); five of the new assertions used it, and this repository's primary signal is a build with no warnings in it. An incremental build does not recompile a test project whose sources have not changed since, which is exactly how they went unseen. MsspData.UnofficialNames allocates and filters on every read, and the mark column asked it once per drawn row — a hundred-variable report walked it a hundred times. Built once per render instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM

Closing from over a report has to review the *screen's* deletions. A report carries an edit log of its own and it is always empty, so reading the top of the stack silently dropped a world someone had just taken out — which is the one class of edit this project asks about precisely because its subject cannot be retyped. The line was already right; nothing pinned it. Now something does, and breaking it fails ADeletionMadeBeforeOpeningAReportIsStillReviewedOnTheWayOut rather than being noticed by a user. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM

The design doc left "its own full-screen overlay or an overlay over F5" open. It is an overlay over F5 — one window, content swapped over a one-deep stack — for a reason the doc could not have known: two modal windows with two PreviewKeyPressed handlers cannot be driven headlessly, which would have put this screen outside every snapshot and every test in the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM

coderabbitai

Actionable comments posted: 11

🤖 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 `@CLAUDE.md`:
- Line 577: Update the TelnetSession architecture overview reference in
CLAUDE.md from TelnetNegotiationCore 2.6.5 to 2.7.0, while preserving the
separate historical MSSP references that correctly say “since 2.6.5.”

In `@docs/superpowers/specs/2026-07-30-structured-server-data-design.md`:
- Around line 948-959: Update the remainder of §8.3 after the supersession
notice to clearly mark obsolete design details as historical or revise them to
match the shipped implementation centered on MsspData and MSSPConfig.Variables.
Remove or qualify references to MsspReport, the old Protocols/Mssp location,
crawler consumption, and custom MSSP parsing; also update the Stage 3 roadmap to
remove the obsolete “upstream fix or own parse” plan.

In `@README.md`:
- Around line 43-49: Update the MSSP description in the README to qualify that
retained server descriptions are bounded and sanitized before persistence,
replacing the unqualified “everything else” claim while preserving the existing
details about capture timing and unavailable MSSP responses.

In `@src/SharpMUTerm.Core/Telnet/Mssp/MsspCache.cs`:
- Around line 341-363: Update the loading loop in MsspCache.Load to normalize
each endpoint’s host component through Key before storing it in into, ensuring
loaded keys match those produced by Find. Enforce MaxEndpoints while iterating
servers by stopping once the limit is reached, and preserve skipped-entry
handling without materializing beyond the configured budget.
- Around line 341-421: Update the cache parsing in Load and Read to use
JsonValue.TryGetValue<T> for every typed JSON field, including version,
connectedAt, observedAt, variable names, and values, so incompatible JSON types
are treated as unreadable data rather than throwing InvalidOperationException.
Preserve the existing skip and fallback behavior, and add regression coverage
confirming malformed field types do not abort startup.

In `@src/SharpMUTerm.Tui/MsspScreenRenderer.cs`:
- Around line 374-376: Update the Row method so label padding is based on the
displayed label width rather than the escaped string length: fit and pad the
label before passing it to Escape, or use a visible-width-aware padding routine.
Preserve the existing NameWidth constraint and ensure labels containing markup
characters keep the value column aligned.

In `@src/SharpMUTerm.Tui/MsspScreenView.cs`:
- Around line 29-51: Reuse the first result of MsspScreenRenderer.Render in the
MsspScreenView layout flow: store the rendered content before constructing body,
then use that same value for the row-height calculation instead of calling
Render again. Keep the existing body construction and sizing behavior unchanged.

In `@src/SharpMUTerm.Tui/ScreenModel.cs`:
- Around line 137-167: Rehome the XML documentation in ScreenModel.cs#L137-L167
by moving DetailKeyLabel and Detail below Remove, restoring the existing summary
and describe parameter documentation to Remove. In
WorldsScreenRenderer.cs#L654-L663, delete the superseded WorldButtons summary
and fold its note that deletion occurs only when a world is selected into the
new summary, leaving WorldButtons with one complete documentation block.

In `@src/SharpMUTerm.Tui/WorldsScreenRenderer.cs`:
- Around line 654-663: Remove the pre-existing contiguous XML summary above
WorldButtons and incorporate its delete-only-when-selected note into the
remaining summary for WorldButtons. Preserve the existing button-order and
cursor-stop documentation while keeping a single summary block.

In `@tests/SharpMUTerm.Core.Tests/Telnet/MsspCacheTests.cs`:
- Line 73: Replace the obsolete HasCount() assertion with Count() in both
affected assertions in the relevant cache tests, preserving the existing
expected count of 1.
- Around line 226-236: Update ACacheWithNoPathWritesNothingAnywhere so the
assertion targets the cache’s would-be persistent storage path rather than the
unrelated TempRoot path. Derive or expose that expected path using the same path
construction used by MsspCache, then assert it does not exist after RecordReport
while preserving the IsPersistent assertion.

🪄 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: b893a1fe-5278-4343-a435-f4d38af11c68

📥 Commits

Reviewing files that changed from the base of the PR and between f67db6f and cd22fd8.

📒 Files selected for processing (52)

.github/workflows/ci.yml
CLAUDE.md
Directory.Packages.props
README.md
SharpMUTerm.slnx
docs/superpowers/specs/2026-07-30-structured-server-data-design.md
src/SharpMUTerm.Core/Session/WorldSession.cs
src/SharpMUTerm.Core/Telnet/Mssp/MsspCache.cs
src/SharpMUTerm.Core/Telnet/Mssp/MsspData.cs
src/SharpMUTerm.Core/Telnet/Mssp/MsspHost.cs
src/SharpMUTerm.Core/Telnet/Mssp/MsspObservation.cs
src/SharpMUTerm.Core/Telnet/Mssp/MsspVariables.cs
src/SharpMUTerm.Core/Telnet/TelnetSession.cs
src/SharpMUTerm.Crawler/CommandLine.cs
src/SharpMUTerm.Crawler/CrawlOptions.cs
src/SharpMUTerm.Crawler/Model/CrawlModel.cs
src/SharpMUTerm.Crawler/MsspCrawler.cs
src/SharpMUTerm.Crawler/Output/CrawlReport.cs
src/SharpMUTerm.Crawler/Output/ObservationLog.cs
src/SharpMUTerm.Crawler/Probing/TelnetMsspProbe.cs
src/SharpMUTerm.Crawler/Program.cs
src/SharpMUTerm.Crawler/Scheduling/CrawlFrontier.cs
src/SharpMUTerm.Crawler/Scheduling/CrawlRateLimiter.cs
src/SharpMUTerm.Crawler/SeedList.cs
src/SharpMUTerm.Crawler/SharpMUTerm.Crawler.csproj
src/SharpMUTerm.Crawler/Storage/CrawlStore.cs
src/SharpMUTerm.Tui/DemoScene.cs
src/SharpMUTerm.Tui/MsspScreenRenderer.cs
src/SharpMUTerm.Tui/MsspScreenView.cs
src/SharpMUTerm.Tui/Program.cs
src/SharpMUTerm.Tui/ScreenChrome.cs
src/SharpMUTerm.Tui/ScreenModel.cs
src/SharpMUTerm.Tui/SettingsOverlay.cs
src/SharpMUTerm.Tui/SettingsSession.cs
src/SharpMUTerm.Tui/SharpMUTermApp.cs
src/SharpMUTerm.Tui/WorldsScreenRenderer.cs
src/SharpMUTerm.Tui/WorldsScreenView.cs
tests/SharpMUTerm.Core.Tests/Telnet/MsspCacheTests.cs
tests/SharpMUTerm.Core.Tests/Telnet/MsspParsingTests.cs
tests/SharpMUTerm.Core.Tests/Telnet/MsspWire.cs
tests/SharpMUTerm.Core.Tests/Telnet/ScriptedTransport.cs
tests/SharpMUTerm.Crawler.Tests/BackoffTests.cs
tests/SharpMUTerm.Crawler.Tests/CrawlLoopTests.cs
tests/SharpMUTerm.Crawler.Tests/PersistenceTests.cs
tests/SharpMUTerm.Crawler.Tests/ProbeTests.cs
tests/SharpMUTerm.Crawler.Tests/RateLimitTests.cs
tests/SharpMUTerm.Crawler.Tests/ReferralTests.cs
tests/SharpMUTerm.Crawler.Tests/SeedTests.cs
tests/SharpMUTerm.Crawler.Tests/SharpMUTerm.Crawler.Tests.csproj
tests/SharpMUTerm.Crawler.Tests/Support/FakeProbe.cs
tests/SharpMUTerm.Crawler.Tests/Support/ManualTimeProvider.cs
tests/SharpMUTerm.Tui.Tests/MsspScreenTests.cs

💤 Files with no reviewable changes (27)

tests/SharpMUTerm.Crawler.Tests/SharpMUTerm.Crawler.Tests.csproj
src/SharpMUTerm.Crawler/SharpMUTerm.Crawler.csproj
src/SharpMUTerm.Crawler/Program.cs
src/SharpMUTerm.Core/Telnet/Mssp/MsspVariables.cs
src/SharpMUTerm.Crawler/Model/CrawlModel.cs
src/SharpMUTerm.Crawler/Probing/TelnetMsspProbe.cs
src/SharpMUTerm.Crawler/SeedList.cs
tests/SharpMUTerm.Crawler.Tests/RateLimitTests.cs
src/SharpMUTerm.Crawler/CommandLine.cs
tests/SharpMUTerm.Crawler.Tests/Support/ManualTimeProvider.cs
SharpMUTerm.slnx
src/SharpMUTerm.Crawler/Output/CrawlReport.cs
tests/SharpMUTerm.Crawler.Tests/SeedTests.cs
src/SharpMUTerm.Crawler/CrawlOptions.cs
tests/SharpMUTerm.Crawler.Tests/CrawlLoopTests.cs
tests/SharpMUTerm.Crawler.Tests/PersistenceTests.cs
tests/SharpMUTerm.Crawler.Tests/ProbeTests.cs
src/SharpMUTerm.Crawler/MsspCrawler.cs
tests/SharpMUTerm.Crawler.Tests/BackoffTests.cs
tests/SharpMUTerm.Crawler.Tests/Support/FakeProbe.cs
Directory.Packages.props
src/SharpMUTerm.Crawler/Output/ObservationLog.cs
src/SharpMUTerm.Core/Telnet/Mssp/MsspHost.cs
tests/SharpMUTerm.Crawler.Tests/ReferralTests.cs
src/SharpMUTerm.Crawler/Scheduling/CrawlRateLimiter.cs
src/SharpMUTerm.Crawler/Storage/CrawlStore.cs
src/SharpMUTerm.Crawler/Scheduling/CrawlFrontier.cs

coderabbitai · 2026-07-31T01:20:54Z

+    public async Task ACacheWithNoPathWritesNothingAnywhere()
+    {
+        // This is the guarantee a snapshot and every test in the suite runs on, and it is structural:
+        // there is no file to write to, rather than a check at each use site that could be forgotten.
+        using var temp = new TempRoot();
+        var cache = new MsspCache();
+        cache.RecordReport("mud.example.org", 4000, Report(("NAME", ["Corvid"])), Noon);
+
+        await Assert.That(cache.IsPersistent).IsFalse();
+        await Assert.That(Directory.Exists(temp.Root)).IsFalse();
+    }


📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

This case does not actually pin "writes nothing".

temp is never handed to the cache, so the assertion would hold even if a memory-only cache wrote to some other location. Pointing the assertion at the path a persistent cache would have used makes the guarantee real.

♻️ Proposed refactor

- await Assert.That(cache.IsPersistent).IsFalse(); - await Assert.That(Directory.Exists(temp.Root)).IsFalse(); + await Assert.That(cache.IsPersistent).IsFalse(); + await Assert.That(File.Exists(temp.CachePath)).IsFalse(); + await Assert.That(Directory.Exists(temp.Root)).IsFalse();

📝 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 ACacheWithNoPathWritesNothingAnywhere()

{

// This is the guarantee a snapshot and every test in the suite runs on, and it is structural:

// there is no file to write to, rather than a check at each use site that could be forgotten.

using var temp = new TempRoot();

var cache = new MsspCache();

cache.RecordReport("mud.example.org", 4000, Report(("NAME", ["Corvid"])), Noon);

await Assert.That(cache.IsPersistent).IsFalse();

await Assert.That(Directory.Exists(temp.Root)).IsFalse();

}

public async Task ACacheWithNoPathWritesNothingAnywhere()

{

// This is the guarantee a snapshot and every test in the suite runs on, and it is structural:

// there is no file to write to, rather than a check at each use site that could be forgotten.

using var temp = new TempRoot();

var cache = new MsspCache();

cache.RecordReport("mud.example.org", 4000, Report(("NAME", ["Corvid"])), Noon);

await Assert.That(cache.IsPersistent).IsFalse();

await Assert.That(File.Exists(temp.CachePath)).IsFalse();

await Assert.That(Directory.Exists(temp.Root)).IsFalse();

}

🤖 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/Telnet/MsspCacheTests.cs` around lines 226 - 236, Update ACacheWithNoPathWritesNothingAnywhere so the assertion targets the cache’s would-be persistent storage path rather than the unrelated TempRoot path. Derive or expose that expected path using the same path construction used by MsspCache, then assert it does not exist after RecordReport while preserving the IsPersistent assertion.

JsonNode.GetValue<string>() throws on a number or a bool, and mssp.json sits beside config.json — a file people hand-edit. A mistyped entry would have thrown out of MsspCache's constructor, on the startup path, over a cache whose entire failure mode is supposed to be 'start empty and say so'. Both readers are total now, and a file whose every field is the wrong kind comes back as skipped rows rather than as a crash. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM

CodeRabbit found eight things worth acting on. Verified each against the code rather than applied on sight; two were genuine defects, three were doc damage this branch had done, one was already fixed, and the rest were text. **The label was padded after it was escaped.** Under EVERYTHING ELSE the label *is* the variable name the server sent, so it is exactly as hostile as a value — and Escape doubles every bracket, so a name containing `[` padded to NameWidth *characters* is short of NameWidth *visible cells* and the value column steps left on that row alone. The hostile-value tests only ever put brackets in a value; they now put them in a name too. Sanitised as well, for the same reason: MSSP says names are upper-case letters and spaces, and a server is not obliged to be truthful. **The cache trusted the endpoint keys in its own file.** Persist writes normalised keys so a self-written file was always fine, but mssp.json sits beside config.json: a hand-written `MUD.Example.ORG:4201` was filed under a key Key() can never produce — unreachable through Find for ever while still spending the endpoint budget. Re-keyed through Key() on the way in. And MaxEndpoints was enforced in Persist alone, so a bloated file was fully materialised at startup and trimmed only if something later wrote, which is exactly the launch where the bound was wanted. Three doc regressions, all introduced by this branch: ScreenModel Remove<T>'s summary and its `describe` param were left above the inserted Detail members, so they documented DetailKeyLabel. Rehomed. WorldsScreenRenderer A second <summary> was added above the existing one on WorldButtons. Folded into one. CLAUDE.md The architecture line still said the session wraps 2.6.5, and Verification still said six suites — the sweep caught the dependency note and the Building section and missed these two. MsspScreenView rendered the body twice, the second time only to read .Count for the row budget — wasted work per layout pass, and a way for the content and the height it was sized for to disagree the moment the renderer stopped being pure. Declined: nothing. The wrong-JSON-kind finding was already fixed in e72f0cc; the README and design-doc wording is qualified here rather than as suggested, because "a bounded, sanitized copy" buries what the feature does under how it is defended. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpL7Ht6sLBsSEtVNsYcXMM

HarryCordewener and others added 2 commits July 30, 2026 19:47

HarryCordewener and others added 3 commits July 30, 2026 20:17

coderabbitai Bot reviewed Jul 31, 2026

View reviewed changes

HarryCordewener and others added 2 commits July 30, 2026 20:22

HarryCordewener merged commit e06cb58 into main Jul 31, 2026
3 checks passed

HarryCordewener deleted the feat/mssp-info-screen branch July 31, 2026 01:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Kill the MSSP crawler; surface MSSP in the client (F5 ▸ i) - #13

Kill the MSSP crawler; surface MSSP in the client (F5 ▸ i)#13
HarryCordewener merged 7 commits into
mainfrom
feat/mssp-info-screen

HarryCordewener commented Jul 31, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading

Review limit reached

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot Jul 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

HarryCordewener commented Jul 31, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Drop the crawler

2. The MSSP info screen

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

HarryCordewener commented Jul 31, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading