feat: nuget package distribution + sample-based ruleset extraction (PLAN-0006)#9
Merged
Merged
Conversation
…-0006 New SharpMud.Hosting package: SharpMudApplicationBuilder/SharpMudApplication wrapping HostApplicationBuilder/IHost, WorldContext/IWorldBuilder/ WorldLoaderHostedService (resolves the previously-open "world-builder registration point" question - a consumer-supplied IWorldBuilder loads a persisted world or builds+saves a fresh one during startup, before any transport starts accepting connections), GameLoopHostedService (wraps GameLoop as a BackgroundService rather than giving GameLoop itself a Hosting dependency, and auto-registers every DI-resolved ITickable), ShutdownSaveHostedService (the previously-unowned shutdown-time whole-world save), IPlayerFactory (lets LoginFlow/PlayerLogin stay ruleset-agnostic without blowing the 4-parameter limit - they're now constructor-injected services instead of static classes taking a 5th parameter), and AddSharpMudRuleset (registers BuiltinCommands before a consumer's ruleset callback runs). SessionLoop/LoginFlow/PlayerLogin converted from static classes taking World/parser/registry/repository as method parameters to constructor-injected service classes - SessionLoop alone was already past coding-standards.md's 4-parameter limit before this change. src/SharpMud.Host deleted. HostOptions trimmed to DbPath only (transport selection moves to the consumer's own Program.cs, not yet updated in this commit). HostRunner.cs moved into SharpMud.Adapters.Telnet as a starting point for AddSharpMudTelnetTransport (not yet rewritten). Ruleset.Classic + Program.cs/appsettings.json/HubWorldBuilder.cs moved into samples/SharpMud.Samples.Classic (csproj/Program.cs rewrite not yet done - next commit). Test coverage: SessionLoopTests/LoginFlowTests/HostOptionsTests/ PasswordHashingTests moved and updated for the new constructor-injected shape, plus new PlayerLoginTests. 16/16 passing. Per PLAN-0006 (docs/adr/0006-nuget-package-distribution.md). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…, finish sample rewrite
SharpMud.Persistence core drops its SQLite PackageReferences - genuinely
provider-agnostic now, GameDbContext already used DbContextOptions<T>
generically with no hardcoded provider. New SharpMud.Persistence.Sqlite
(AddSharpMudSqlitePersistence) and SharpMud.Persistence.DynamoDb
(AddSharpMudDynamoDbPersistence, EntityFrameworkCore.DynamoDb 10.0.0).
Resolved the TPH/discriminator open question from PLAN-0006 by reading the
DynamoDB provider's own docs directly: TPH is the supported (only
supported) inheritance strategy, discriminators auto-configured by
default - BehaviorConfiguration's HasDiscriminator<string>("BehaviorType")
usage is compatible in principle, though the exact fluent-API interop
isn't verified against a live table.
Multi-targeted every packaged src project to net10.0;net11.0 (verified
clean - no net11.0-only API usage anywhere), since Persistence.DynamoDb
needed net10.0 support to even reference Engine/Persistence, and the ADR
already called for this. Persistence.DynamoDb itself stays net10.0-only,
matching the provider's own target.
Added a new IStorageInitializer hook in SharpMud.Hosting and wired it into
WorldLoaderHostedService - SQLite's EnsureCreatedAsync schema step needs
to run before world loading, but hosted-service StartAsync ordering across
independently-registered Hosting/Persistence packages would be fragile to
rely on directly.
Found and fixed a real startup-ordering bug: the generic host constructs
every registered IHostedService (resolving IEnumerable<IHostedService>)
before calling any of their StartAsync methods - so GameLoopHostedService/
the transport BackgroundServices constructor-injecting ITickable/
LoginFlow/SessionLoop directly would eagerly resolve WorldContext-dependent
services before WorldLoaderHostedService.StartAsync had populated it.
Fixed by deferring resolution to ExecuteAsync via IServiceProvider - the
legitimate service-locator exception in coding-standards.md.
Renamed HostOptions to SharpMudHostOptions - collided with the real
Microsoft.Extensions.Hosting.HostOptions BCL type every consumer of this
package would also have in scope.
Finished samples/SharpMud.Samples.Classic/Program.cs against
SharpMud.Hosting's builder: ClassicWorldBuilder/ClassicPlayerFactory
implement Hosting's extension points, CombatManager registered as both
ICombatManager and ITickable, SHARPMUD_MODE/--telnet parsing preserved as
the sample's own composition-root decision. csproj gained OutputType=Exe
and the references/packages Program.cs needs.
Full solution builds clean (net10.0 + net11.0) and all 107 tests pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Manual verification caught a real behavior regression: under the old procedural Program.cs, the CLI branch's SessionLoop.RunAsync returning meant the whole process fell through to shutdown. Under IHost, CliTransportBackgroundService.ExecuteAsync returning doesn't stop anything else - the host just kept running GameLoop forever, waiting for a SIGINT/SIGTERM a local CLI user has no reason to send. CliTransportBackgroundService now calls IHostApplicationLifetime .StopApplication() once its single session ends. Telnet's BackgroundService deliberately does not do this - many concurrent sessions, one ending must not stop the host. Verified end-to-end: CLI mode (look/move/quit, persists across runs, exits cleanly on quit), Telnet mode (real TCP connection - IAC negotiation, username/password character creation, look/quit, host stays alive after the client disconnects), and graceful SIGTERM shutdown (process exits promptly, world data actually lands on disk - confirmed via direct sqlite3 query). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd subsystem doc corrections for PLAN-0006 Completes PLAN-0006: Directory.Build.props/LICENSE/icon/meta-package/ publish-preview+release workflows for the SharpMud.* NuGet packages; docsite/ (Zensical+uv) GitHub Pages skeleton with a real Getting Started walkthrough; every subsystem doc updated off the old SharpMud.Host/ Ruleset.Classic paths. Verified via dotnet pack + a real consumer project built against the packed nupkgs over both CLI and Telnet transports, which caught and fixed a doc bug (CLI is login-free, not the login flow the doc originally claimed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a3814f77e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ncipollina
changed the title
…, fix Codex-flagged getting-started sample - Sample no longer explicitly references/loads Microsoft.Extensions.Configuration.Json/ .EnvironmentVariables - Microsoft.Extensions.Hosting already pulls these in and Host.CreateApplicationBuilder wires them up by default. - Bump global.json, Directory.Packages.props' net11.0 group, Dockerfile, and docs/deployment.md from preview.5.26302.115 to preview.6.26359.118 (matching the installed SDK); bump Microsoft.SourceLink.GitHub to the matching preview.6 release. Left EntityFrameworkCore.DynamoDb on the stable 10.0.0 line - its only net11.0 build still depends on EF Core preview.5, one behind ours - and updated the comment to record why. - Fix Codex review feedback on PR #9: docsite/docs/getting-started.md's sample IWorldBuilder used ThingId.New() for its root id, which changes every process start and defeats reload-from-persistence; switch to a fixed Guid like the real HubWorldBuilder does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ncipollina
changed the title
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…cs site No SemVer compatibility guarantees yet and only prerelease packages are published so far - make that explicit before someone builds against it expecting stability. Also fixes getting-started.md's install commands, which were missing --prerelease (no stable release exists to resolve without it). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
…riptions, warning-clean build, DB path CLI arg, narrow meta-package scope - Fix shutdown-save stop-order race: GameLoopHostedService now registered after ShutdownSaveHostedService, so the generic host's reverse-order stop sequence quiesces the tick loop before the snapshot save runs, not concurrently with it. New regression test verifies this ordering directly and was confirmed to fail against the old ordering. - Add a real Description to every SharpMud.* package's csproj (only the meta-package had one before). - Pin NSubstitute back to 5.3.0 (AutoFixture.AutoNSubstitute 4.18.1's <6.0.0 constraint - only unreleased AutoFixture previews support 6.x), which also resolved two nullable warnings NSubstitute 6.0's newly-annotated CallInfo indexer introduced in SessionLoopTests.cs. Suppress the pre-existing xUnit1051 false-positive in TelnetListenerTests.cs via pragma - the token genuinely derives from TestContext.Current.CancellationToken through a linked source the analyzer can't trace. Solution now builds with 0 warnings. - Add --db-path CLI arg support (wins over SHARPMUD_DB_PATH, same precedence as --telnet), resolved in the sample's own Program.cs matching the transport-selection precedent, rather than just downgrading docs/persistence.md to describe a CLI-arg gap that didn't need to exist. - ADR-0007: narrow the SharpMud meta-package to Engine+Hosting+Persistence only, dropping the persistence-provider/adapter ProjectReferences ADR-0006 originally included. Every consumer was getting an unused DynamoDB SDK dependency and Telnet listener by default, and it made getting-started.md's "you still pick a provider/transport explicitly" framing false. Verified via a real dotnet pack - the meta-package's nuspec now only depends on SharpMud.Engine/.Hosting/.Persistence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 604d73bb65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…s, stale docs, IStorageInitializer dependency direction - Fix --telnet/--db-path CLI arg parsing: both were positional patterns requiring their own flag to be args[0], so combining them silently dropped whichever came second. Switched both to Array.IndexOf-based lookups so they work in any order/combination. Verified live with both orderings over real Telnet. - Move IStorageInitializer from SharpMud.Hosting to SharpMud.Engine.Core, alongside IThingRepository - it's a pure persistence-lifecycle abstraction with nothing Hosting-specific about it. This removes SharpMud.Persistence.Sqlite's reference to SharpMud.Hosting entirely, making the "Persistence.* -> Persistence -> Engine" dependency-direction rule in docs/architecture.md actually true instead of a documented exception. - Update docsite/docs/index.md's meta-package description (still said "all of the above", stale since ADR-0007 narrowed it last round). - Update docs/plans/0006-nuget-package-distribution.md's Open Questions section - 4 entries were still listed as unresolved (DynamoDB TPH support, IWorldBuilder registration shape, GameLoop hosting shape, docsite/ naming) despite all being decided/implemented. - Update docs/networking.md - still described Telnet's old "Name:"-prompt placeholder instead of the real LoginFlow username/password flow that's been live since earlier in this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5a10bd090
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…architecture.md Same class of gap as last round's docsite/docs/index.md fix - both Solution layout ASCII trees still said the meta-package "pulls in every SharpMud.* package", stale since ADR-0007 narrowed it to Engine+Hosting+ Persistence. Grepped the whole repo for the same phrasing to make sure these were the last two. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
ncipollina
commented
Jul 20, 2026
…ad of pinning below it AutoFixture.AutoNSubstitute 4.18.1 (latest stable, Nov 2023) caps NSubstitute below 6.0.0 - NuGet correctly flags NU1608 since we're outside that stated range, but the integration itself has been validated against 6.0.0 by this repo's own test suite (108/108). Suppress the warning directly rather than staying pinned to 5.3.0 - documented in Directory.Build.props with the reasoning and a removal condition (once AutoFixture publishes an updated dependency range). Also fixes the two real nullable warnings NSubstitute 6.0's newly-annotated CallInfo indexer surfaces in SessionLoopTests.cs - switched the manual (Thing)x[0] cast to x.ArgAt<Thing>(0), NSubstitute's own null-safe typed accessor, rather than suppressing those too. The NU1608 suppression had to go in Directory.Build.props's unconditional top-level PropertyGroup, not the existing IsTestProject-gated one - NU1608 is evaluated during restore, before IsTestProject (set by an early-imported test-package .props file) is available, the same restore-vs-project-body ordering gap already noted for IsPackable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…LAN-0006, and architecture.md - deployment.md: config table still described --telnet/port as positional args, stale since the order-independent Array.IndexOf fix last round. - SPEC.md: roadmap still named SharpMud.Ruleset.Classic, deleted in this PR (renamed to SharpMud.Samples.Classic) - left alone earlier as "historical roadmap text", but unlike an ADR, SPEC.md isn't an immutable record, and this pointed at a project name that no longer exists at all. - PLAN-0006: meta-package task entry still described the pre-ADR-0007 "every SharpMud.* project" scope. - architecture.md: solution layout was missing SharpMud.Adapters.Cli.Tests/.Telnet.Tests; dependency-direction rule implied Adapters.*/Persistence.* only reach Engine transitively through Hosting/Persistence, but both directly reference Engine too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
ncipollina
commented
Jul 21, 2026
ncipollina
commented
Jul 21, 2026
Contributor
Author
|
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ncipollina
commented
Jul 21, 2026
Contributor
Author
|
|
…e, rename test project off the deleted Ruleset.Classic namespace - Move the NU1608 (NSubstitute/AutoFixture.AutoNSubstitute) suppression out of the repo-root Directory.Build.props into a new tests/Directory.Build.props, per review: the repo-wide suppression would have hidden future package-range conflicts in src/ packages too. Directory-based discovery scopes this to tests/ without needing an IsTestProject condition at all - sidesteps the restore-vs-project-body evaluation-order gap that made the previous IsTestProject-gated attempt not actually apply at restore time. Verified empirically both ways before landing on this. - Drop SharpMud.Persistence.DynamoDb's stale "no reference to SharpMud.Hosting" rationale for not registering IStorageInitializer - stale since that interface moved to SharpMud.Engine last round, which this project already references. The real reason (doesn't create/migrate tables) stands alone. - Rename tests/SharpMud.Samples.Classic.Tests off the deleted SharpMud.Ruleset.Classic.Tests namespace (RootNamespace, global usings, and all 4 file namespace declarations) - a leftover from the original project move that never got renamed. Required adding explicit `using SharpMud.Ruleset.Classic;` to CombatManagerTests.cs/CombatResolverTests.cs, since they'd been implicitly resolving CombatManager/ICombatResolver/ CombatantBehavior via C#'s enclosing-namespace lookup (the old test namespace was nested under the production one) rather than an explicit using - swept the rest of the repo for the same pattern and confirmed no other spots need it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eset.Classic to SharpMud.Samples.Classic The project/folder was renamed to SharpMud.Samples.Classic back in PLAN-0006's implementation, but the C# namespace was deliberately left as SharpMud.Ruleset.Classic at the time (no drive-by rename). Now renamed for real across every file in samples/SharpMud.Samples.Classic and its Persistence.Tests/Samples.Classic.Tests consumers, closing that gap. Also fixes 7 comments in src/SharpMud.Engine and src/SharpMud.Persistence that referenced the sample by its old namespace name as an example consumer, stale now that the rename is real. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Implements PLAN-0006/ADR-0006: sharp-mud now ships as a set of
SharpMud.*NuGet packages (Engine,Hosting,Persistence+.Sqlite/.DynamoDbproviders,Adapters.Cli/.Telnet, plus aSharpMudmeta-package) rather than a single monolithic solution with one hardcoded ruleset.src/SharpMud.Hostsplit intoSharpMud.Hosting(ruleset-agnostic generic-host composition helpers:WorldContext,IWorldBuilder,IPlayerFactory,SessionLoop/LoginFlow,AddSharpMud*DI extensions) andsamples/SharpMud.Samples.Classic(the D&D-flavored reference consumer + composition root, merged fromsrc/SharpMud.Ruleset.Classic+src/SharpMud.Host).SharpMud.Persistencesplit into a provider-agnostic core plusSharpMud.Persistence.Sqlite/SharpMud.Persistence.DynamoDbprovider packages.Directory.Build.props,LICENSE,icon.png,NOTICE.md), theSharpMudmeta-package, andpublish-preview.yaml/publish-release.yamlCI workflows.docsite/— a Zensical + uv GitHub Pages skeleton with a real Getting Started walkthrough, matchingdynamodb-efcore-provider's established shape.README.md,SPEC.md, and theDockerfileupdated off the oldSharpMud.Host/SharpMud.Ruleset.Classicpaths.Test plan
dotnet build SharpMud.slnx— clean, 0 warnings, bothnet10.0/net11.0dotnet test SharpMud.slnx— 107/107 passingdotnet pack SharpMud.slnx— produces all 7 expected packages, correct meta-package nuspec dependencies, no sample/test projects packedProjectReference) built and ran cleanly over bothSharpMud.Adapters.Cli(login-free single-player) andSharpMud.Adapters.Telnet(real IAC negotiation + username/password login flow), including a cleanSIGTERMshutdown-savedocsite/builds clean viauv sync→zensical build, matching the CI workflow's own command🤖 Generated with Claude Code