Repository navigation
fix(layout): the Catalog guard reads the registry its render uses (#5065) - #5161
Conversation
) `DomainLayoutAreas.Catalog` resolved its `ITypeDefinition` from the hub's `ITypeRegistry` and then handed it to a render that streams `Workspace.GetStream(new CollectionReference(...))` — it checked one registry and read another. The two are not the same set and are not meant to be: the type registry is the hub's SERIALIZATION registry, which every content type a NodeType declares lands in, while `DataContext.DataSourcesByCollection` holds only the collections a data source MAPS. A type in the first and not the second walked past the guard and died three frames deeper as a bare `ArgumentException: Collections Harness are not mapped to any source` — an unhandled throw out of a render, so `LayoutAreaHost` logged it at Error (the level that auto-files an incident) and put that framework sentence in the viewer's face for what is an authoring mistake. The guard now asks `DataContext.GetTypeSource(collection)`, which is the same map keyed the same way AND the set `AddTypesCatalogs` enumerates to OFFER catalog links — so the accept set and the offer set are one set. Both of the area's declines are now localized (`catalog.needsType`, `catalog.notAWorkspaceCollection`); the first was a hard-coded English literal. Control: `TestCatalogOfATypeThatIsNotAWorkspaceCollection` sits opposite the existing `TestCatalog` across exactly one condition. Reverting the guard alone reproduces the production reading in-process — `There were unmapped collections: UnmappedRecord`, `Rendering failed for area Catalog` at Error, `WorkspaceStreams.cs:38` — and fails the new test; 1 failed / 2 succeeded against 3 succeeded. Two diagnostics in the same family, both of which named nothing a reader could follow up: * `ObjectAsExtensions.LogRecoveryFailure` dropped the `what` its callers already pass and its two sibling report sites already print, so `As<MarkdownContent> could not recover value: JsonException` had no referent — not the node, not the partition, not which writer sent it. That is #4597's remaining half, and its review recorded the missing subject as the blocker. Content and exception text stay withheld, which is the load-bearing part of that contract. `ObjectAsPrivatePayloadTest` passed the private text AS the subject and asserted its absence, which reads as "content is not published" but measures "the caller's subject is dropped" — the two strings are now separate and the test asserts both halves. * `JsonSynchronizationStream`'s reducer refusal printed the same type parameter twice where the reduced type belonged, and named neither the hub that refused nor the subscriber that asked (#5120). It now names all four. The condition is real and reachable: a hub with `AddData()` has a workspace and reaches the lookup, but only `AddMeshDataSource` registers `MeshNodeReference -> MeshNode`, so the mesh's infrastructure hubs — which own no node on purpose — accept the subscribe and have nothing to reduce with. Doc: `Doc/Architecture/ThreeRegistriesAReadCanMiss`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes consistently align guard behavior with the actual workspace registry used by the render path, add localized viewer-facing copy, and include targeted regression coverage and clearer diagnostics without introducing async/task patterns in src/.
Review effort: Lite
Findings: None
What changed in this PR
This PR fixes a production-facing failure mode in the Domain @@Catalog layout area by ensuring its precondition guard interrogates the same registry that the render path actually reads (workspace-mapped collections), preventing unmapped-but-serializable types (e.g., NodeType content records) from reaching Workspace.GetStream(CollectionReference) and throwing during render.
Changes:
- Align
DomainLayoutAreas.Catalogguard with the workspaceDataContextcollection/type-source map and return localized, actionable cautions instead of throwing for unmapped collections. - Strengthen diagnostics: include the caller-supplied subject in
ObjectAsExtensionsrecovery-failure logs, and expand reducer-missing errors inJsonSynchronizationStreamto name hub/subscriber/types/stream. - Add regression tests and an architecture doc page explaining the three distinct “not registered” registries involved in these failures.
| File | Description |
|---|---|
| test/MeshWeaver.Messaging.Hub.Test/ObjectAsPrivatePayloadTest.cs | Updates test to distinguish “private content” from diagnostic subject and assert both privacy + diagnosability. |
| test/MeshWeaver.Layout.Test/DomainLayoutServiceTest.cs | Adds a negative control covering “type is in TypeRegistry but not a workspace collection” behavior. |
| src/MeshWeaver.Messaging.Hub/Localization/strings.en.json | Adds localized catalog error/help strings (English). |
| src/MeshWeaver.Messaging.Hub/Localization/strings.de.json | Adds localized catalog error/help strings (German). |
| src/MeshWeaver.Mesh.Contract/ObjectAsExtensions.cs | Ensures recovery-failure logs include the subject (what) while still withholding content/exception messages. |
| src/MeshWeaver.Layout/Domain/DomainLayoutAreas.cs | Fixes Catalog guard to use workspace DataContext.GetTypeSource and return localized cautions instead of throwing. |
| src/MeshWeaver.Documentation/Data/Architecture/ThreeRegistriesAReadCanMiss.md | Adds a new architecture page documenting the three registries and the failure modes. |
| src/MeshWeaver.Documentation/Data/Architecture.md | Links the new architecture page from the Architecture topic map. |
| src/MeshWeaver.Data/Serialization/JsonSynchronizationStream.cs | Improves “no reducer defined” refusal message to include hub/subscriber/types/stream context. |
| src/MeshWeaver.Compiler.Pipeline/NodeTypeCompilationHelpers.cs | Updates commentary to reflect the improved reducer-missing diagnostic wording. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CI is red on one shard, and the failure is UNATTRIBUTED — this diff cannot reach it. Not merging, and not re-running.
Everything the test is ABOUT passed; only
|
| Change | Reachability |
|---|---|
DomainLayoutAreas.Catalog guard |
only when a Catalog layout area renders; this test renders none |
ObjectAsExtensions.LogRecoveryFailure |
one log message gains a format argument. Same level, same call count, identical return value and control flow |
JsonSynchronizationStream refusal |
message text. Same condition, same throw |
NodeTypeCompilationHelpers.cs |
a comment — the only file touched in the compile area, verified with git diff; no code changed |
| two localization keys, two test files | additive |
Nothing changes control flow anywhere near adoption, compilation, bundles or provenance.
Controls
- The same test, locally, Release, same filter:
total: 1, succeeded: 1. Passes on this branch. mainis green — the last eightMeshWeaver Build and Testruns arecompleted/success,
includinge3509fcfat 22:00Z, which is this branch's merge base.- Not in
.github/known-flakes.json, so the merge-queue steward will not re-queue it — it would leave
the PR out withqueue-rejected. That is the right behaviour and I am not going to route around it. - No open issue mentions both
AdoptedVerifiedandCompiled.
What I am NOT doing, and why
- Not re-running the shard. Re-running to see whether it passes the second time is exactly what hides
a race, and this repo's rule is explicit about it. - Not adding a flake-catalogue entry. That file is for "a TEMPORARY, EVIDENCE-BEARING allowance for
one known race while its root cause is fixed — never a permanent tolerance". I have not root-caused
this, so an entry would be the tolerance the file forbids. - Not merging. CI is red.
Filed, with the evidence
The race is filed into bug triage on the control instance (a Feedback/Feedback submission, handed over
to the triage agent). What I did not establish is in the filing: which writer set Compiled, how
often it races, and whether it is CI-specific — one local pass is not a negative result.
What this PR needs: either that race fixed, or a decision by someone who owns the adoption path that
it is a known flake worth cataloguing. The change set itself is complete and verified.
The handle for whoever picks this up: the assertion is a POINT READ, and it is stricter than its own stated reasonTwo details from // line 146 — a WAIT. At this point the provenance genuinely IS AdoptedVerified.
var adopted = await DefinitionWhen(d => d.BuildProvenance is BuildProvenance.AdoptedVerified, ct);
// … the held reimport at commit B …
// line 182 — a POINT READ of whatever the record says right now, with nothing to filter on.
var stillAdopted = await Definition(ct);
// line 184
stillAdopted.BuildProvenance.Should().Be(BuildProvenance.AdoptedVerified,
"the type goes AdoptedVerified → AdoptedVerified, never through StaleAdopted");
Both candidate fixes are wrong if the other reading is true: widening the assertion to admit Everything else on this PR is green, including |
…d-reads-the-registry-the-render-uses
…d-reads-the-registry-the-render-uses
🚨 Correction: the adoption race did NOT reproduce on the new base. The second red was an artifact 403, and no test ran at all.I merged
|
The third red is the SAME artifact 403, now on UPLOAD — and re-running it is the named exception, not a flake-hunt
Content uploaded, digest computed, finalize refused. Paired with the earlier red on Why re-running is allowed here and re-running the shard was not. The rule bans re-running a test to What I could not establish: whether this is fleet-wide. My run is the only one in this repository Standing tally on this PR, for whoever reads it next:
Shard 5 passed on |
Closes #5065.
The root: a guard that interrogated a different registry from the read
DomainLayoutAreas.Catalogresolved itsITypeDefinitionfrom the hub'sITypeRegistry, then handedit to
DomainCatalogLayoutArea.RenderCatalog, which streamsWorkspace.GetStream(new CollectionReference(collectionName))—DataContext.DataSourcesByCollection.Two different tables. The type registry is the hub's SERIALIZATION registry:
DataContext.Initializeregisters every mapped type source into it,
PolymorphicTypeInfoResolveradds any type the hub merelyserialises, and every content type a NodeType declares lands there.
DataSourcesByCollectionholds onlywhat a data source MAPS.
For the mapped types the two agree, which is why this passed review and every test. For
Harness— aNodeType content record with no data source — they do not, and the guard let it through to die three
frames deeper:
An unhandled throw out of a render reaches
LayoutAreaHost's top-levelCatch, which logs at Error— the level that auto-files an incident — and renders the exception's own text. So an AUTHORING mistake
(
@@Catalog/Harnessnaming a type that is not a workspace collection) was reported as an engineeringdefect and the viewer was shown a sentence about the framework's collection map.
AreaErrorClassifieralready demotes exactly this class (
IsAccessDenied,IsNodeGoneNotFound→ Warning + viewer copy); anunmapped collection belongs in that company.
The fix asks the registry the read uses —
area.Workspace.DataContext.GetTypeSource(collection)—which is the same map keyed the same way (
DataContext.InitializebuildsTypeSourcesandDataSourcesByCollectionin one pass) and the setAddTypesCatalogsenumerates to OFFER cataloglinks. The accept set and the offer set become one set, which is what stops the guard drifting from the
read a second time. Both of the area's declines are now localized; the missing-Id one was a hard-coded
English literal.
The control
TestCatalogOfATypeThatIsNotAWorkspaceCollectionsits opposite the existingTestCatalogacrossexactly one condition (
DataRecordis mapped,UnmappedRecordis not; both are in the sameTypeRegistry, which is why the old guard could not tell them apart). Reverting the guard alone
reproduces the production reading in-process:
DomainLayoutServiceTestThe assertion is on the CONTROL TYPE, deliberately: before the fix the area still produced a control
(the host's
Catchrenders an error panel), so "the area produced something" proves nothing.Two diagnostics in the same family
Both named nothing a reader could follow up, which is the reason each issue is open rather than fixed.
#4597's remaining half.
ObjectAsExtensions.LogRecoveryFailuredropped thewhatits callersalready pass and its two sibling report sites already print, so
As<MarkdownContent> could not recover value: JsonExceptionhad no referent — not the node, not thepartition, not which of the many writers of markdown content sent it. The incident's own review recorded
that as the blocker: "LogRecoveryFailure deliberately withholds content AND carries no node path, so
there is nothing to look up." It now prints
for {What}. Content and exception text stay withheld,which is the load-bearing half of that contract; a node path is already in this file's other two log
lines and
ContentAs<T>is documented as "this same conversion with the node's path in the diagnostics",so no new class of information is exported.
🚨 And that is a finding about the test.
ObjectAsPrivatePayloadTestpassed the private text AS thewhatargument and asserted the log did not contain it — which reads as "content is not published" andactually measures "the caller-supplied subject is dropped". Those are different claims and only the
first is the accessor's contract. The two strings are now separate, and the test asserts both halves:
the content is absent AND the subject is present.
#5120's refusal.
JsonSynchronizationStream.CreateSynchronizationStreamprinted"No reducer defined for MeshNodeReference from MeshNodeReference"— the same type parameter twicewhere the reduced type belonged, doubled space and all, and neither the hub that refused nor the
subscriber that asked. It now names all four. The condition is real and reachable, not impossible: a hub
configured with
AddData()HAS a workspace, soWorkspace.SubscribeToClientruns and reaches thereducer lookup, but only
AddMeshDataSourceregistersMeshNodeReference → MeshNode— so the mesh'sinfrastructure hubs (
portal/nodeops-{meshId},portal/reads-{meshId}), which carry a workspace anddeliberately own no mesh node of their own, accept the subscribe and have nothing to reduce with.
Registering a reducer there would be the wrong fix (a
CreateNodePermissionAttributeanchors at theRECEIVING hub's path — that is why
WithNodeOperationExecutionmust never land on a per-node hub), sowhat the refusal has to make answerable is who asked an address with no node for its node. That is the
subscriber, and the subscriber is now in the message. #5120 stays open until the next occurrence
names its caller.
Verification
dotnet build -c Release -warnaserror— whole solution,0 Warning(s) 0 Error(s), 66 assemblies,MeshWeaver.Hosting.dllrewritten inside the build window (so the dependents recompiled against thechanged
DataandMesh.Contract).DomainLayoutServiceTest3/3 ·ObjectAs*+LocalizationTest80/80 ·NodeUpdateContentTypeChangeTest(NodeUpdatePipeline logs "As<MarkdownContent> could not recover value" at Error for stored content with no $type (EmailContent half fixed by #4679; remainder fixed in #5926) #4597's own control suite) 6/6 ·DocumentationLinkIntegrityTest1/1 — which caught six bare sibling links in the new page first.signature reaching another repo.
LogRecoveryFailureisprivate.area compiled in the mesh is changed by this diff.
Mirror-sync:
npm run sync:i18n -- --ref <merged core sha>in MeshWeaver.Plugins for the two added keyscatalog.needsTypeandcatalog.notAWorkspaceCollection. Both are NEW keys with no value drift, so theplugins drift guard is green either way — this is the debt-declaration, not a red.
Doc:
Doc/Architecture/ThreeRegistriesAReadCanMiss, linked from the Architecture topic map.🤖 Generated with Claude Code