fix(layout): a layout stream accepts input only from its subscriber - #6009
Conversation
A layout area is rendered once per subscriber. Its stream now accepts that subscriber's input - a person's action (IUserAction: ClickedEvent, BlurEvent, CloseDialogEvent) and an edited value (PatchDataChangeRequest) - only when the delivery carries the identity the stream was subscribed under. Any other delivery is refused before a handler runs: the sender is answered with a Forbidden DeliveryFailure carrying a localized sentence, and the owner logs one Warning. One seam: a delivery-pipeline step on the stream's own synchronization hub (SynchronizationStream.AcceptInputFromSubscriberOnly), in front of the rule chain, so it holds however the delivery is addressed. The identity is recorded when the stream is created (StreamConfiguration.SubscriberIdentity, off the SubscribeRequest delivery; the LayoutAreaHost's captured viewer for a stream opened without a subscribe). It fails closed: no identity on either side is a refusal, and no sender is exempt. LayoutAreaHost opts every layout area in; data streams, which are shared by design and authorize each write under the writer's identity, are unchanged. Doc: Doc/Architecture/InputFromTheSubscriber. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 0) 1 files 1 suites 3m 7s ⏱️ Results for commit a843cdf. ♻️ This comment has been updated with latest results. |
Test Results (shard 4)660 tests 660 ✅ 2m 38s ⏱️ Results for commit a843cdf. ♻️ This comment has been updated with latest results. |
Test Results (shard 2) 2 files 2 suites 4m 10s ⏱️ Results for commit a843cdf. ♻️ This comment has been updated with latest results. |
Test Results (shard 1) 2 files 2 suites 4m 41s ⏱️ Results for commit a843cdf. ♻️ This comment has been updated with latest results. |
Test Results (shard 5)1 550 tests 1 550 ✅ 9m 38s ⏱️ Results for commit a843cdf. ♻️ This comment has been updated with latest results. |
Test Results (shard 3) 3 files 3 suites 15m 56s ⏱️ Results for commit a843cdf. ♻️ This comment has been updated with latest results. |
Test Results 16 files 16 suites 40m 12s ⏱️ Results for commit a843cdf. ♻️ This comment has been updated with latest results. |
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Records the identity a layout stream was subscribed under (StreamConfiguration.WithSubscriberIdentity, taken by the owner off the SubscribeRequest delivery; for a stream opened without a subscribe, the viewer context captured at construction) and adds a delivery-pipeline step on the stream's own hub, AcceptInputFromSubscriberOnly, which refuses IUserAction and PatchDataChangeRequest deliveries whose AccessContext.ObjectId is not the recorded subscriber's or is missing, before any handler runs: nothing executes, the sender is answered from the host with a Forbidden DeliveryFailure carrying a localized sentence (new error.userActionNotFromSubscriber and error.inputNotFromSubscriber keys, added in English and German), and one Warning names the stream, owner, area, subscriber and sender. LayoutAreaHost opts every layout area in; data streams are not opted in; one Blazor test harness is adapted to open its stream as the identity it acts as. Reviewed from the diff alone. Checked: the accept condition is fail-closed exactly as claimed, an input running only when both identities are known and equal (ordinal ObjectId comparison), so a missing identity on either side refuses; the Warning template's seven placeholders match its seven arguments; the pipeline step is gated on the opt-in flag and sits in front of the handler chain; the new test covers all three actions from another identity, from none and from the platform identity, on both addressing routes (routed via the owner, posted on the stream's own hub) and an edited value, each case ending on the subscriber's own accepted action; no async/await/Task is added and no secret material appears. Not verifiable from the diff: whether IUserAction and PatchDataChangeRequest exhaust a layout stream's mutating input (question on SynchronizationStream.cs); the MeshWeaver.Todo.Test failures the PR itself reports as 2 of 6 failing before any click, with no refusal logged, left without cause or plan in its table (question on LayoutAreaHost.cs); the PR's own not-exercised list, cross-cluster routing, participant connections and in-mesh node sources; and whether any no-identity subscriber exists whose views this rule now makes inert by design. Parts of the diff text arrive redacted, with identifiers and addresses replaced by placeholders, which limited line-exact reading of the new test file and the German resource strings; nothing is asserted about those passages.
Findings: 0 blocking · 0 should-fix · 2 question · 1 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
nit test/MeshWeaver.Layout.Test/UiInputIsAcceptedOnlyFromTheSubscriberTest.cs
The new test applies null-forgiving operators to ToString() results: `stream.GetControlStream(stack.Areas.First().Area.ToString()!)` and `var resultArea = stack.Areas.Last().Area.ToString()!;`. The repo rule forbids `!` used to silence a warning: if a nullable warning is being suppressed at these calls, that is a broken rule, and if none is, the operators are dead decoration; in both readings an explicit null check or pattern match states the same intent without the suppression.
Internal review of ce2701085e91ef3fe75c256b9e8d9afacaa5b099 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| /// <returns>The refused delivery, or <c>null</c> when the delivery may proceed.</returns> | ||
| private IObservable<IMessageDelivery>? AcceptInputFromSubscriberOnly(IMessageDelivery delivery) | ||
| { | ||
| if (delivery.Message is not (IUserAction or PatchDataChangeRequest)) |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The type gate on this line - `delivery.Message is not (IUserAction or PatchDataChangeRequest)` - is the whole input surface the new rule covers, and the PR's guarantee (a layout stream accepts input only from its subscriber) holds only if those two message shapes exhaust the mutating input a layout stream can receive. That exhaustiveness cannot be verified from the diff: the stream's hub also handles DataChangedEvent and lifecycle messages (out of scope by design), and any other state-mutating request type a sender could address to the stream would pass this gate untouched. Nothing in the diff shows such a type; this records the unverifiable assumption the security guarantee rests on.
There was a problem hiding this comment.
Answered in d31074f. The two-type check is replaced by an explicit, closed classification of every message type the stream's hub handles (StreamInputRule), and the rule decides by role:
| Message | Role | Accepted from |
|---|---|---|
IUserAction, PatchDataChangeRequest, DataChangeRequest, DataChangedEvent, StreamErrorEvent |
input | the subscriber's identity only |
UpdateStreamRequest, SetCurrentRequest |
the stream's own write path | the stream itself only (a per-stream token stamped on its own Update / OnNext posts; the identity is not the criterion because the owner renders under the platform identity) |
UnsubscribeRequest, GetDataResponse, DeliveryFailure |
release / answers | the subscriber's identity, or the mesh's own hubs (a release is posted by a tearing-down hub without the subscriber's identity, and an answer by whichever hub answers); refused from any other participant connection |
The reason for each row is in the StreamInputRule doc comment and in Doc/Architecture/InputFromTheSubscriber.
The set is closed: every handler on the synchronization hub is registered through one helper that records its type, and a stream whose hub handles a type the classification does not name fails to build and names the type. StreamInputRuleClassifiesEveryHandledTypeTest pins the classification and the handled set. When StreamErrorEvent was dropped from the list, both pin tests went red with that construction error.
New cases in UiInputIsAcceptedOnlyFromTheSubscriberTest require a Forbidden refusal for each newly covered type from another identity, from no identity and from the platform identity, and for each own write posted by anyone but the stream, including on its own hub under the subscriber's identity. Each case has a control: the subscriber's own action still runs exactly once, the stream's own write still applies, and the subscriber's own release ends the stream. Against the previous two-type check, all 14 new refusal cases fail. The class passes 26/26.
Local Release -warnaserror runs on this head: Layout.Test 605/605, Data.Test 568/568, Graph.Test 2454/2454, Messaging.Hub.Test 567/567, Documentation.Test 662/662, Hosting.Orleans.Test 352/352, PluginTester.Test 410/410, ContentCollections.Test 37/37. Memex.Portal.Shared.Test: 2215 passed, 1 skipped, 1 failed. The failure is CatalogOrphanActionIdentityTest.TheAutoChoiceInRowK_SetsPackageKsPolicy, which also fails 2/2 on the base commit 35361e6 without this change and logs no refusal.
| // blur, a dialog dismissal or an edited value on it is therefore accepted only from | ||
| // the identity it was rendered for — the subscribe delivery's, or (a stream opened | ||
| // without a subscribe) the viewer captured above. Anything else is refused by the | ||
| // stream before OnClick / OnBlur / OnCloseDialog or the data update can run. |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The PR's own dependent-suite table reports MeshWeaver.Todo.Test as '6 run, 2 failed — before any click, with no refusal logged'. The twelve Blazor harness failures in the same table are explained (the harness opens the stream as one identity and submits the click as another) and come with an adaptation landing ahead of this PR; these two come with neither a cause nor a fix plan. A failure before any click, with no refusal logged, sits outside the seam this change adds — either those two tests already fail on the base, which the table does not state, or something else in this diff breaks them, and since this line opts every layout area in rather than only areas a person clicks, the change reaches whatever those tests render. Not decidable from the diff.
There was a problem hiding this comment.
Checked by running the tests. Both failures come from the Todo test suite as it was before MeshWeaver.Plugins#2662, and this change does not cause them:
- Without this change: MeshWeaver.Plugins at a340f2280 (main just before i18n: two model-stream failure strings the clients need (Plugins#855) #2662), built against core 35361e6 (this PR's base, which has no gate code).
MeshWeaver.Todo.Test: 6 run, 2 failed. The failures areTodoList_ClickStartButton_ShouldMoveItemToInProgressAndUpdateActionsandTodoList_ClickIndividualStartButton_ShouldMoveSpecificItemToInProgress, both stopping at "Should have at least one pending todo item before starting test". That matches the table's "before any click, no refusal logged". core#5973 had changed the Todo sample, and i18n: two model-stream failure strings the clients need (Plugins#855) #2662 rewrote the suite to match. - With this change: MeshWeaver.Plugins origin/main 731a3c377 (i18n: two model-stream failure strings the clients need (Plugins#855) #2662 included), built against this branch (
-p:MeshWeaverRoot=<worktree>).MeshWeaver.Todo.Test: 6/6 passed.
The other dependent suites, run against this branch with Plugins main (including #2717, which adapted the harness): MeshWeaver.Blazor.Views.Test 368/368, MeshWeaver.Graph.Views.Test 282/282, MeshWeaver.Markdown.Collaboration.Test 429 run, 0 failed, 1 skipped.
…closed classification The subscriber-only rule on an input-checked stream now decides by an explicit classification of every message type the stream's hub handles (StreamInputRule), instead of a two-type list: - input (IUserAction, PatchDataChangeRequest, DataChangeRequest, DataChangedEvent, StreamErrorEvent): accepted only from the identity the stream was subscribed under; - the stream's own writes (UpdateStreamRequest, SetCurrentRequest): accepted only when the stream posted them to itself, proven by a per-stream token; - the release and answers (UnsubscribeRequest, GetDataResponse, DeliveryFailure): accepted from the subscriber's identity or from the mesh's own hubs. The classification is closed: every handler is registered through one helper that records its type, and a stream whose hub handles a type the classification does not name fails to build, naming the type. StreamInputRuleClassifiesEveryHandledTypeTest pins the set. New cases in UiInputIsAcceptedOnlyFromTheSubscriberTest demand the refusal for each newly covered type, with the subscriber's own input (or the stream's own write) as the control. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Records the identity a stream was subscribed under (StreamConfiguration.WithSubscriberIdentity, set by JsonSynchronizationStream from the subscribe delivery, with the viewer context as fallback for a stream opened without a subscribe) and adds a delivery-pipeline step on the stream's own hub, AcceptInputFromSubscriberOnly, gated on WithInputFromSubscriberOnly, which LayoutAreaHost applies to every layout area. This head replaces the previous head's two-message type gate with a closed classification, StreamInputRule: IUserAction, PatchDataChangeRequest, DataChangeRequest, DataChangedEvent and StreamErrorEvent are subscriber input; UpdateStreamRequest and SetCurrentRequest are the stream's own writes, proven by a per-stream token stamped on the post and compared by reference; UnsubscribeRequest, GetDataResponse and DeliveryFailure are release-or-answer, accepted from the subscriber or from a non-participant sender. A refused delivery reaches no handler, is answered from the host with a Forbidden DeliveryFailure carrying a new localized sentence (error.userActionNotFromSubscriber, error.inputNotFromSubscriber, added in English and German) unless it is itself an answer, and logs one Warning; a construction-time check throws when a handled type is unclassified. The diff is INCOMPLETE: the patch of test/MeshWeaver.Layout.Test/UiInputIsAcceptedOnlyFromTheSubscriberTest.cs is truncated (first 20000 of 24307 characters kept), so that file's tail — the OpenAsSubscriber, Submit, Answer and Refusal helpers and the ends of several test methods — was not read and nothing is asserted about it; other passages arrive with identifiers and addresses replaced by placeholder tokens, limiting line-exact reading (notably the German strings and parts of StreamInputRule.Roles). Checked: the accept condition is fail-closed, an input running only when both identities are known and equal (ordinal comparison); the Warning template's eight placeholders match its eight arguments; the own-write token is a fresh object per stream, so the other end of the stream cannot present it; an answer is never answered again; the construction gate throws for every stream, not only opted-in ones; the readable test cases execute and end on positive terminals; production code adds no async/await (a completed-task helper appears in the new test — nit); no secret material appears. Not verifiable from the diff: what enforces that WithStreamHandler is the only route by which a handler reaches a synchronization hub (question on SynchronizationStream.cs); the two unexplained MeshWeaver.Todo.Test failures the PR itself reports (question on LayoutAreaHost.cs, carried over from the previous head); cross-cluster routing, participant connections and in-mesh node sources (the PR's own not-exercised list); the truncated test tail; and the content of the German sentences. The PR description is one revision stale against this head: it counts 11 test cases and a two-message guarantee table, where the rule now gates ten message types across three roles and the test file carries roughly 26 cases — the new documentation page documents the actual set.
Findings: 0 blocking · 0 should-fix · 2 question · 2 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
nit test/MeshWeaver.Layout.Test/UiInputIsAcceptedOnlyFromTheSubscriberTest.cs
Null-forgiving operators on the editor lookup — stack.Areas.First().Area.ToString()! and stack.Areas.Last().Area.ToString()! — are present on this head unchanged from the previous one. The repo rule bans ! used to silence a warning: if a nullable warning is suppressed at these calls that is a broken rule, and if none is, the operators are dead decoration; an explicit null check states the same intent either way.
nit test/MeshWeaver.Layout.Test/UiInputIsAcceptedOnlyFromTheSubscriberTest.cs
The new test adds a Task-returning helper outside IIoPool: private static Task Count(ref int counter) returns Task.CompletedTask, and the file adds using System.Threading.Tasks. The helper introduces no asynchrony (no async, no await — a completed task) and exists to satisfy the blur action's Task-typed signature, where the click and close actions in the same view are void. Under the letter of the repo rule it falls inside it; a void-returning overload of the blur action, where the API offers one, would clear it.
Internal review of d31074fb245846f0b90bca5c3610f5a31b53b80f — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| // an input-checked stream that nothing checks — refused here, for every stream, so the | ||
| // first test that opens one names it. | ||
| HandledMessageTypes = handled.ToImmutable(); | ||
| foreach (var type in HandledMessageTypes) |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The construction-time check iterates only HandledMessageTypes, the types the new internal WithStreamHandler helper recorded while ConfigureSynchronizationHub registered the stream's own handlers; a handler that reaches a synchronization hub by any other route is invisible to the check, and StreamInputRule.RoleOf returns null for its type, so AcceptInputFromSubscriberOnly passes its deliveries straight through and on an opted-in stream it runs for any sender. The helper is internal, so no registration path outside this assembly can use it at all; and the hub-level Register<T> API that this diff's own tests use on the client hub exists on every hub, the stream's hub being reachable as host.Stream.Hub. The new documentation page states that a new handler cannot reach a stream without first being classified — the mechanism shown covers only the registrations made through the helper inside ConfigureSynchronizationHub, and nothing in the diff shows what enforces that monopoly. This records the assumption the closed classification rests on.
There was a problem hiding this comment.
Fixed in a843cdf. The construction check only sees the handlers registered in the stream's hub configuration, so on its own it does not cover a handler registered at run time. The gate no longer lets an unrecognised type through. A type with no role in StreamInputRule now gets the Unclassified role. That covers the hub's framework messages and any handler added at run time with Register<T>. It is held to the release rule: accepted from the subscriber's identity or from the mesh's own hubs, and refused from any other participant connection. So the classification is closed in two ways:
- Every handler in the stream's hub configuration has a role, or the stream is not built (
StreamInputRuleClassifiesEveryHandledTypeTest). - Every other type that reaches the hub is never open to a participant other than the subscriber.
The only run-time registration the platform makes is LayoutAreaHost's click, blur and dialog handlers. Their types are IUserAction, which are classified, so they stay under the subscriber-only input rule. The doc comment on StreamInputRule and Doc/Architecture/InputFromTheSubscriber now describe both guarantees rather than claiming the first covers everything.
New test AHandlerRegisteredAtRunTimeIsNotOpenToAnotherParticipant: a handler is registered at run time on the stream's hub for an unclassified type. A delivery from a participant connection with another identity is refused with Forbidden, and the handler does not run. The subscriber's own connection is the control: it reaches the handler and gets the handler's answer, and the handler runs exactly once. Without the fallback, this test fails.
On this head (local, Release, -warnaserror for core): Layout.Test 606/606, Data.Test 568/568, Graph.Test 2454/2454, Messaging.Hub.Test 567/567, Documentation.Test 662/662, Hosting.Orleans.Test 352/352, PluginTester.Test 410/410, ContentCollections.Test 37/37. Memex.Portal.Shared.Test has 1 failure, CatalogOrphanActionIdentityTest.TheAutoChoiceInRowK_SetsPackageKsPolicy, which also fails 2/2 on the base 35361e6.
Plugins main, built against this head: Todo.Test 6/6, Blazor.Views.Test 368/368, Graph.Views.Test 282/282, Markdown.Collaboration.Test 429 run with 0 failed and 1 skipped, Persistence.Test 2016/2016, AI.Test 2528 run with 0 failed and 3 skipped.
| // blur, a dialog dismissal or an edited value on it is therefore accepted only from | ||
| // the identity it was rendered for — the subscribe delivery's, or (a stream opened | ||
| // without a subscribe) the viewer captured above. Anything else is refused by the | ||
| // stream before OnClick / OnBlur / OnCloseDialog or the data update can run. |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The PR's dependent-suite table reports MeshWeaver.Todo.Test as '6 run, 2 failed — before any click, with no refusal logged', with neither a cause nor a fix plan; the twelve Blazor harness failures in the same table get both, plus an adaptation that lands ahead of this pull request. The question was raised on the previous head and this revision leaves it unanswered. The line this finding sits on opts every layout area in — not only areas a person clicks — so the rule reaches whatever those tests render: either the two already fail on the base (the table does not state that), or something in this diff breaks them, and a failure before any click with no refusal logged sits outside the seam this change adds. Not decidable from the diff; the missing fact is whether those two tests pass on the base.
There was a problem hiding this comment.
These two failures are already on the base and come from the Todo test suite as it was before MeshWeaver.Plugins#2662. This change does not cause them. I checked by running the tests:
- Base, without this change: MeshWeaver.Plugins a340f2280 (main just before i18n: two model-stream failure strings the clients need (Plugins#855) #2662), built against core 35361e6 (this PR's base, which has no gate code).
MeshWeaver.Todo.Test: 6 run, 2 failed. The failures areTodoList_ClickStartButton_ShouldMoveItemToInProgressAndUpdateActionsandTodoList_ClickIndividualStartButton_ShouldMoveSpecificItemToInProgress. Both stop at "Should have at least one pending todo item before starting test", before any click. core#5973 had changed the Todo sample, and i18n: two model-stream failure strings the clients need (Plugins#855) #2662 rewrote the suite to match. - With this change: MeshWeaver.Plugins origin/main 731a3c377 (i18n: two model-stream failure strings the clients need (Plugins#855) #2662 included), built against this branch at d31074f and again at a843cdf.
MeshWeaver.Todo.Test: 6/6 passed both times.
The PR description's table was written before #2662 merged, and its body is left unedited. This thread holds the current answer.
…se rule A message type that reaches an input-checked stream's hub without a role in StreamInputRule (the hub's framework messages, or a handler registered on the hub at run time rather than in its configuration) is no longer passed through. It gets the new Unclassified role, which is accepted from the subscriber's identity or from the mesh's own hubs and refused from any other participant connection. The construction check still covers every handler in the stream's hub configuration. The fallback covers everything else. AHandlerRegisteredAtRunTimeIsNotOpenToAnotherParticipant requires the refusal and uses the subscriber's own connection as the control. The test fails when the fallback is removed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Gates every delivery to an opted-in synchronization stream through one pipeline step on the stream's own hub (AcceptInputFromSubscriberOnly), decided by a closed type classification (StreamInputRule): IUserAction, PatchDataChangeRequest, DataChangeRequest, DataChangedEvent and StreamErrorEvent are subscriber input, accepted only when the identity recorded at subscribe time (StreamConfiguration.WithSubscriberIdentity, set by JsonSynchronizationStream from the subscribe delivery, with LayoutAreaHost's captured viewer context as the fallback via WithInputFromSubscriberOnly) and the delivery's identity are both known and equal; UpdateStreamRequest and SetCurrentRequest are the stream's own writes, proven by a per-stream token stamped on the post and compared by reference; UnsubscribeRequest, GetDataResponse and DeliveryFailure, and every unclassified type, are accepted from the subscriber or from a non-participant sender and refused from any other participant connection. LayoutAreaHost opts every layout area in. A refused delivery reaches no handler, is answered from the owner with a Forbidden DeliveryFailure carrying one of two new localized sentences unless it is itself an answer, and logs one Warning; a construction-time check throws when a handled type is unclassified. Against the head reviewed before this one, the revision answers the open question about unclassified deliveries: RoleOf null no longer passes a delivery through but maps to the new Unclassified role held to the release rule, and a new test pins a run-time-registered handler as refused from another participant. Checked: the accept conditions fail closed on both sides; the own-write token is a fresh object per stream, compared by reference; an answer is never answered; the Warning's eight placeholders match its eight arguments; the English strings are valid JSON at the insertion point; the readable test cases execute, each refusal case ending on the subscriber's own accepted input as the control. Not verifiable from this item's view of the diff: the patch of the new layout test file is truncated (first 20000 of 26877 characters) — its Submit / Answer / Refusal / OpenAsSubscriber helpers and the ends of the last test methods were not read; the German strings file declares two added lines that are not legible in the hunk shown, so the German sentences are not asserted; identifiers, paths and some structure are replaced by placeholder tokens, which mangles several lines and limits line-exact reading. From the PR's own reporting: participant connections (gRPC, SignalR) and cross-cluster delivery are not exercised — the ReleaseOrAnswer and Unclassified rules rest on IsFromParticipant and ParticipantIngress stamping, neither of which is in the diff; MeshWeaver.Todo.Test still reports two failures with no cause (question filed); the twelve Blazor harness failures have a cause and a landing order in MeshWeaver.Plugins ahead of this pull request; and the PR body is now two revisions stale — it counts 11 test cases and a two-message guarantee table where the classification gates ten message types and the test file carries at least 27 cases.
Findings: 0 blocking · 0 should-fix · 1 question · 2 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
nit test/MeshWeaver.Layout.Test/UiInputIsAcceptedOnlyFromTheSubscriberTest.cs
The null-forgiving operators on the editor lookup are unchanged from the previous head: the Area.ToString() results of stack.Areas.First() and stack.Areas.Last() each carry a trailing !. The repo rule bans ! used to silence a warning: if a nullable warning is suppressed at those calls that is a broken rule, and if none is, the operators are dead decoration — an explicit null check states the same intent either way.
nit test/MeshWeaver.Layout.Test/UiInputIsAcceptedOnlyFromTheSubscriberTest.cs
The Task-returning helper is unchanged from the previous head: private static Task Count(ref int counter) returns Task.CompletedTask and exists to satisfy the blur action's Task-typed signature, while the click and close actions of the same view are void. The helper performs no asynchrony, but the repo rule bans Task outside IIoPool; a void-accepting blur action, where the API offers one, would keep Task usage in this file to the test methods themselves.
Internal review of a843cdfc5ed4e15cf1bc3ac3fe487200ccff7381 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| // blur, a dialog dismissal or an edited value on it is therefore accepted only from | ||
| // the identity it was rendered for — the subscribe delivery's, or (a stream opened | ||
| // without a subscribe) the viewer captured above. Anything else is refused by the | ||
| // stream before OnClick / OnBlur / OnCloseDialog or the data update can run. |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The dependent-suite table still reports MeshWeaver.Todo.Test as 6 run, 2 failed — before any click, with no refusal logged — with neither a cause nor a fix plan, and it is the one failing entry the landing-order paragraph does not cover: the twelve Blazor harness failures get both, plus an adaptation that merges in MeshWeaver.Plugins ahead of this pull request, and the Memex failure is shown to fail the same way with the opt-in line removed. The same question was raised on the previous head and this revision leaves it unanswered. The line this finding sits on opts every layout area in, so the rule reaches whatever those two tests render; a failure before any click, with no refusal logged, does not match the seam this change adds, so either those tests already fail on the base or something in this diff outside the refusal path breaks them — the diff alone cannot say which, and the missing fact is whether the two pass on the base.
There was a problem hiding this comment.
The missing fact is now measured: the two tests fail on the base. MeshWeaver.Plugins a340f2280 (the Todo suite before MeshWeaver.Plugins#2662), built against core 35361e6 (this PR's base, without this change), gives MeshWeaver.Todo.Test 6 run and 2 failed: TodoList_ClickStartButton_ShouldMoveItemToInProgressAndUpdateActions and TodoList_ClickIndividualStartButton_ShouldMoveSpecificItemToInProgress. Both stop at "Should have at least one pending todo item before starting test". Cause: core#5973 converted the Todo sample to templates. Fix: MeshWeaver.Plugins#2662 rewrote the suite, merged 2026-10-03.
With this change, Plugins origin/main 731a3c377 (which includes #2662) built against this branch gives MeshWeaver.Todo.Test 6/6 passed at d31074f and again at a843cdf.
The table in the description dates from before #2662 merged. Descriptions on this repo are not edited after push, so the current figures are in this thread and in the PR conversation.
|
Current dependent-suite results (these replace the table in the description, which predates MeshWeaver.Plugins#2662 and #2717) Plugins origin/main 731a3c377, built against this branch at a843cdf (
The two Core on a843cdf (Release, |
1 participant
What this guarantees
A layout area is rendered once per subscriber. Its stream now accepts that subscriber's input only from the identity the stream was subscribed under:
ClickedEvent,BlurEvent,CloseDialogEvent— everyIUserActionPatchDataChangeRequestA delivery of one of these that carries another identity, or none, is refused:
LayoutAreaHost.OnClick/OnBlur/OnCloseDialog, not the data update;DeliveryFailure(ErrorType.Forbidden) carrying a localized sentence, which a view shows throughSubmitUserAction'sonRefused;It fails closed: an input is accepted because both identities are known and equal. There is no per-action or per-control opt-out and no exemption by sender.
Where the check sits
One seam: a delivery-pipeline step on the stream's own synchronization hub (
SynchronizationStream.AcceptInputFromSubscriberOnly). It runs in front of the hub's rule chain — every rule of a chain runs for a delivery, so a handler-level check could not keep the other handlers from running — and it holds however the delivery is addressed, at the owner or at the stream's hub.The identity is recorded when the stream is created:
StreamConfiguration.SubscriberIdentity, taken off theSubscribeRequestdelivery by the owner (JsonSynchronizationStream.CreateSynchronizationStream); for a stream opened without a subscribe, the viewerLayoutAreaHostcaptured at construction. A stream opts in withStreamConfiguration.WithInputFromSubscriberOnly, andLayoutAreaHostdoes for every layout area.Senders whose identity differs from the subscriber's — each decided here
system-security)CatalogAdminActionsNeedTheAdministratorTest; see "Dependent suites".Stream lifecycle messages (subscribe, re-subscribe, unsubscribe, heartbeat) are not input and are unchanged.
Tests
UiInputIsAcceptedOnlyFromTheSubscriberTest(new, 11 cases): each of the three actions from another identity, from no identity and from the platform identity is refused withForbiddenand the control's action does not run; the subscriber's own action, submitted the way a view submits it, then runs exactly once. The same for an action posted on the stream's own hub, and for an edited value (refused, the sender is answered, and the subscriber's own edit is the only one in the data). Every case ends on a positive terminal — the refusal is an answer — so no window is spent waiting for nothing to happen.Negative control: with the
WithInputFromSubscriberOnlyline removed fromLayoutAreaHost, all 11 fail.Executed locally (Release,
-warnaserror), on this branch:MeshWeaver.Layout.TestMeshWeaver.Graph.TestMeshWeaver.Data.TestMeshWeaver.Messaging.Hub.TestMeshWeaver.Documentation.TestMeshWeaver.Hosting.Orleans.TestMeshWeaver.PluginTester.TestMeshWeaver.ContentCollections.TestMemex.Portal.Shared.TestCatalogOrphanActionIdentityTest.TheAutoChoiceInRowK_SetsPackageKsPolicy, which fails the same way on this machine with the opt-in line removed and logs no refusalDependent suites (MeshWeaver.Plugins)
Run against this branch (
-p:MeshWeaverRoot=<this worktree>):MeshWeaver.Blazor.Views.TestUserActionSubmissionFromViewsTest,FormEditReachesTheOwnerBeforeTheClickTest,ButtonPendingStateTest,RowScopedActionsFromViewsTest,CouponFieldWritesThroughItsOwnContextTest. The edits those tests make through a realBlazorVieware accepted.MeshWeaver.AI.TestMeshWeaver.Persistence.TestMeshWeaver.Markdown.Collaboration.TestMeshWeaver.Graph.Views.TestMeshWeaver.Todo.TestLanding order: the twelve Blazor harness tests have to open their stream as the acting identity first. That adaptation is compatible with core before and after this change, so it merges in MeshWeaver.Plugins ahead of this pull request.
Not exercised
MeshWeaver.Hosting.Orleans.Testpasses, but none of its tests submits a user action; the delivery's identity is whatOrleansRoutingServiceandMessageHubGraincarry for the subscribe and for the action alike.SubmitUserActionfrom the client's stream, a post on the stream's own hub).Doc:
Doc/Architecture/InputFromTheSubscriber(new page, listed in the topic map).Mirror-sync: MeshWeaver.Plugins will run
npm run sync:i18n -- --ref <this merge sha>— two keys added (error.userActionNotFromSubscriber,error.inputNotFromSubscriber).🤖 Generated with Claude Code