A sign-in event core can raise without knowing who cares - #2180
Merged
Conversation
The Store wants per-user tile convergence at sign-in, and asked me to post UserSignedIn to its root hub, probe-and-delegate, tolerating NotFound where the Store is absent. That works — and it puts "the Store exists, and this is where it lives" into core, which is the knowledge I had just argued core must not hold. So the event is here and the addressing is inverted: subscribers register themselves. builder.AddSignInNotificationTarget(storeAddress); Empty by default, so a portal where nothing subscribes posts nothing: no address to probe, no NotFound to tolerate, no latency to explain. It also generalises for free — the second thing that wants a sign-in hook adds a line at its own registration instead of another address hard-coded in core. Fire-and-forget, deliberately. Each target gets a Post; nothing is observed and no response type exists. Sign-in is on the user's critical path, and the moment it can be held up by another partition's health, a Store outage becomes a login outage. A throw from Post logs at Debug, not Warning — on a portal whose subscriber partition is absent, Warning would be a line on every sign-in. EveryLogon, and run-once would be a contradiction: subscribers exist to react to each sign-in, and a ledger entry would silence every one after the first. There is nothing to make idempotent because core writes nothing; what a subscriber does is its own business, including deciding it has already done it. Ordered last, so a subscriber sees core's own per-user work settled rather than mid-repair. Not correctness — the event carries no state — it just removes an ordering question the subscriber would otherwise have to reason about. The event carries a user PATH and nothing else, with a test asserting it stays that way: an event that grows fields grows coupling, and a subscriber should read the mesh rather than the message. Full MeshWeaver.Graph.Test: 1577/1577. -c Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The doc said 'register from the subscriber's own configuration', and the Store session correctly pointed out that a node-native module has no MeshBuilder — only a per-type HubConfiguration lambda. So the seam's premise was unimplementable for exactly the subscriber it was built for. The deeper reason is worse than API surface, and is now written down: the registry is a mesh-scoped singleton, so it is per-PROCESS, and the announcement is posted by whichever process handles the sign-in. A module's hub configuration runs only when that node's hub ACTIVATES — one silo, some time, possibly never — so a module registering itself there would populate the registry in a process that may not be the announcing one, and the event would silently go nowhere. It would look self-contained and work on a monolith. That is the worst combination available. So deployment configuration is not a compromise on module self-containment, it is the only placement that runs in every process. A module that genuinely must declare its own subscription wants DATA the announcer reads, for the same reason logon actions are data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in, fire-and-forget UserSignedIn event seam for subscribers to react to sign-ins without coupling core to application-specific details.
Changes:
- Defines the single-field sign-in event contract.
- Adds notification target registration and ordered dispatch.
- Registers the announcement as an
EveryLogonaction with structural tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Summary | Findings |
|---|---|---|
test/MeshWeaver.Graph.Test/SignInAnnouncementTest.cs |
Tests event shape, registration, ordering, and logon mode. | No final comments. |
src/MeshWeaver.Mesh.Contract/UserSignedIn.cs |
Defines the sign-in event. | Moderate (2 votes): Mark the response-less event with [CanBeIgnored]. |
src/MeshWeaver.Graph/Logon/SignInNotificationTargets.cs |
Stores registered notification targets. | No final comments. |
src/MeshWeaver.Graph/Logon/AnnounceSignInLogonAction.cs |
Posts events during sign-in. | Moderate (2 votes): Carry or re-enter the delivery identity explicitly when posting. |
src/MeshWeaver.Graph/Configuration/LogonActionNodeType.cs |
Registers the action and target API. | Critical (1 vote): Register UserSignedIn in the shared mesh type registry. Critical (1 vote): Avoid duplicate singleton registration and ensure registration is order-independent. |
Suppressed comments (3)
src/MeshWeaver.Graph/Logon/AnnounceSignInLogonAction.cs:40
- The action is registered together with
SignInNotificationTargetsbyAddLogonActionType, so resolving this dependency as optional turns a registration/configuration error into a successfulNothingoutcome and silently skips every announcement. UseGetRequiredService<SignInNotificationTargets>()here; the logger can remain optional if that is intentional.
var registry = context.Hub.ServiceProvider.GetService<SignInNotificationTargets>();
src/MeshWeaver.Graph/Logon/AnnounceSignInLogonAction.cs:52
- The added tests only inspect the target list, record shape, and action metadata; none executes the initialization callback or delivers a
UserSignedInthrough a real hub. Consequently a wrong target, missing route/handler registration, missing access context, or incorrect missing-target behavior can leave the suite green. Add an integration test with a registered target handler and assert bothUserPathand the deliveredAccessContextidentity.
context.Hub.Post(announcement, o => o.WithTarget(target));
test/MeshWeaver.Graph.Test/SignInAnnouncementTest.cs:66
- These tests only inspect the action metadata; they never subscribe to
AnnounceSignInLogonAction.Runwith a configured target and receiving handler. The core behavior added here—hub-initialization registration, posting to each target, and carrying the user's delivery identity—could silently be broken while this suite remains green. Add a real-hub integration test that observes a target handler and its receivedAccessContext.
public void It_announces_on_every_sign_in()
{
// Run-once would be a contradiction: subscribers exist to react to EACH sign-in, and a
// ledger entry would silence every one after the first.
new AnnounceSignInLogonAction().Mode.Should().Be(LogonActionMode.EveryLogon);
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+63
to
+64
| .AddSingleton<SignInNotificationTargets>() | ||
| .AddSingleton<ILogonAction, AnnounceSignInLogonAction>() |
Comment on lines
+88
to
+92
| builder.ConfigureServices(services => | ||
| { | ||
| services.AddSingleton<SignInNotificationTargets>(); | ||
| return services; | ||
| }); |
| { | ||
| try | ||
| { | ||
| context.Hub.Post(announcement, o => o.WithTarget(target)); |
| /// with and nothing worth replaying.</para> | ||
| /// </summary> | ||
| /// <param name="UserPath">The signing-in user's node path — the partition their own nodes live in.</param> | ||
| public record UserSignedIn(string UserPath); |
Test Results (shard 3) 9 files 9 suites 4m 49s ⏱️ Results for commit 7441770. |
Test Results (shard 5) 10 files 10 suites 7m 11s ⏱️ Results for commit 7441770. |
Test Results (shard 0)1 287 tests 1 287 ✅ 11m 2s ⏱️ Results for commit 7441770. |
Test Results (shard 4)2 140 tests 1 841 ✅ 9m 24s ⏱️ Results for commit 7441770. |
Test Results (shard 2)3 343 tests 3 343 ✅ 8m 26s ⏱️ Results for commit 7441770. |
Test Results (shard 1)1 769 tests 1 769 ✅ 9m 2s ⏱️ Results for commit 7441770. |
Test Results 55 files 55 suites 49m 58s ⏱️ Results for commit 7441770. |
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
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 Store wants per-user tile convergence at sign-in (MeshWeaver.Plugins#624) and the "user never visits /Store" gap is real. This is core's half: the trigger, and nothing else.
The addressing is inverted from what was proposed
The suggested shape was for core to post
UserSignedInto the Store's root hub, probe-and-delegate, tolerating a NotFound where the Store is absent. That works — and it puts "the Store exists, and this is where it lives" into core, which is exactly the knowledge core deliberately does not hold anywhere else about apps. (AppRecordSpecscovers the platform defaults only for the same reason.)So subscribers register themselves:
Empty by default. A portal where nothing subscribes posts nothing — no address to probe, no NotFound to tolerate, no latency to explain. And it generalises for free: the second thing that wants a sign-in hook adds a line at its own registration rather than another address hard-coded in core.
The contract
UserSignedIn(string UserPath)— a path, not a session or a token. The handler runs on the subscriber's hub under the delivery's identity, so it acts as the signing-in user. A test asserts the record stays single-field: an event that grows fields grows coupling, and a subscriber should read the mesh rather than the message.Fire-and-forget, deliberately. Each target gets a
Post; nothing is observed and no response type exists. Sign-in is on the user's critical path — the moment it can be held up by another partition's health, a Store outage becomes a login outage. A throw fromPostlogs at Debug rather than Warning, because on a portal whose subscriber partition is absent, Warning would be a line on every single sign-in.EveryLogon. Run-once would be a contradiction: subscribers exist to react to each sign-in, and a ledger entry would silence every one after the first. Nothing here needs to be idempotent because core writes nothing.
Ordered last, so a subscriber sees core's own per-user work settled rather than mid-repair. Not a correctness requirement — the event carries no state — it just removes an ordering question the subscriber would otherwise have to reason about.
Why an event and not an interface
Convergence needs to know a package's current artwork, what "stale" means for an install, and what a viewer's escape hatch from convergence is. That is Store knowledge. Core holding an interface to call would put it back in core wearing a different shape; core owns exactly one fact — a user signed in — and says it out loud.
Full
MeshWeaver.Graph.Test: 1577/1577. Release-warnaserrorclean.Companion: the Store-side handler lands in MeshWeaver.Plugins.