Skip to content

fix(write): refuse a content $type that names no type, and Create's silent member drop (Plugins#3042) - #6231

Merged
rbuergi merged 7 commits into
mainfrom
fix/create-refuses-unknown-content
Oct 7, 2026
Merged

rbuergi merged 7 commits into
mainfrom
fix/create-refuses-unknown-content

Conversation

@rbuergi

@rbuergi rbuergi commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Refs Systemorph/MeshWeaver.Plugins#3042 — closes after verification on memex.systemorph.com.

The defect

An agent created Feedback/Feedback nodes with content {"$type":"Feedback","status":…,"category":…,"description":…}. No type named Feedback exists; the NodeType binds FeedbackContent. Every write layer accepted it:

  1. src/MeshWeaver.Graph/Security/ContentDiscriminatorValidator.cs exempts every runtime-compiled NodeType (HasRuntimeCompiledConfiguration), because their content types live only on their own hub.
  2. src/MeshWeaver.Graph/Security/ContentSchemaValidator.cs (on main, Judge, the if (!ContentDiscriminator.Admits(content, declared)) return Valid() line) admitted any $type that names "a different record". It left that case to guard 1, which had already exempted it.
  3. src/MeshWeaver.Mesh.Operations/MeshOperations.cs ReadFromContentType looked up the bound content type with typeRegistry.TryGetType(nodeType), using the NodeType path as a $type name. WithContentType registers the CLR name and never the path. So the lookup only hits when the record has the same name as the NodeType (Story → Story, which is the shape every existing probe test uses). For Feedback/Feedback the probe returned nothing, and MCP create validated nothing. Create also never applied the unknown-member refusal that patch and update have. So the reverse shape (the right $type plus description) silently lost the member.

The node was stored. Every ContentAs<FeedbackContent> returned null, and the owning hub's hand-over watcher skipped it without logging anything. The three filings were dead letters for about 1.5 h.

The fix

  • ContentSchemaValidator.JudgeForeignDiscriminator runs on every Create and Update. It refuses a $type that contradicts the declared content type and resolves in none of these: this hub's $type registry (full name, then short name), the mesh-wide IMeshContentTypeRegistry, or the declared type's own assembly. Checking the declared type's assembly keeps a polymorphic subtype compiled beside an in-mesh content type legal. The error is the new localized content.schema.unknownDiscriminator (en + de). It names the discriminator, the declared type and the declared members. An Update that keeps the discriminator already stored on the node is still Valid, so an existing dead letter can still be repaired.
  • MeshOperations.Create:
    • It now refuses undeclared top-level content members, with the same rule and message as Patch and Update.
    • It runs ValidateCreatedContent on the probe hub. That does the same unresolvable-$type check and the undeclared-member check against the bound type, so it also covers in-mesh content, which is still a JsonElement on the facade hub.
    • ReadFromContentType now falls back to IMeshContentTypeRegistry.TryResolveByNodeType and tells the reader which lookup answered.
    • Scoping: the historical ValidateAgainst bind check keeps its old reach (exact-name lookups only). Without that, Update and Patch would start refusing the partial-content shapes that the write boundary admits on purpose (CD failed on main: run 35257430439 #4648). GetContentSchema and ValidateContentAgainstSchema are unchanged for the same reason.
  • Doc: Doc/Architecture/ContentSchemaOnWrite has a new section, "A $type that names nothing", and rule 3 of "Where it deliberately says nothing" is narrowed.

Tests (test/MeshWeaver.Graph.Test/ContentSchemaValidationTest.cs)

The new fixture NodeType SchemaCompiled is a static definition that carries a runtime-compile Configuration source, so ContentDiscriminatorValidator exempts it exactly as it exempts Feedback/Feedback. Its content record (SchemaGuardedContent) is named differently from the NodeType.

case asserts
Create_WhoseTypeDiscriminatorNamesNoType_IsRefused_NamingItAndTheDeclaredType {"$type":"Feedback","label":…,"description":…} is refused through IMeshService.CreateNode, naming 'Feedback' and SchemaGuardedContent
McpCreate_WhoseTypeDiscriminatorNamesNoType_IsRefusedBeforeTheWrite MCP create refuses it with its own Error: refused create …
McpCreate_WithAnUndeclaredContentMember_IsRefusedNamingIt description is named, label is not
control Create_WhoseTypeDiscriminatorNamesAnExistingOtherType_StillLands a foreign $type that EXISTS is still admitted, so the guard does not reshape content
control McpCreate_WithTheDeclaredShape_IsCreated the declared shape still answers Created:

Negative control (src reverted to main, tests kept): 3 failed / 10 passed. On main the MCP create of {"$type":"Feedback","label":"Bug"} answered Created: TestData/sg5e54c01e, which is the production defect reproduced. The undeclared-member create also answered Created:. Both controls passed on main and on the branch.

With the fix, all local runs used -c Release:

  • ContentSchemaValidationTest: 13/13 passed.
  • Full MeshWeaver.Graph.Test: 2543/2543 passed.
  • PatchUnknownContentMembersTest: 5/5 passed.
  • LocalizationTest: 66/66 passed.

Every touched project and test project builds clean with -warnaserror.

What I did NOT establish

  • I did not sweep Plugins or satellite tests for MCP create calls whose content carries members the bound type does not declare. Create now refuses those, as Patch and Update already do. The Plugins CI run will show any that exist.
  • Live mesh (memex.systemorph.com, read as rbuergi):
    • Feedback/_Submissions/* is not readable with this credential. get returns Not found for the three records named in the issue, and a partitions:all search returns only Feedback/Inbox in the feedback partition. So I cannot count the dead letters there.
    • In rbuergi/Feedback I found one more record with the same shape, validate-repos-path-collision-20261005 ($type: Feedback, status New, description). It was never moved into the Inbox.
    • 20260921-0930-iconcontrol-… holds MarkdownContent on a Feedback/Feedback node. Its $type names an existing type, so this rule does not refuse that shape. The Plugins half names it as a dead letter instead.
    • About 23 records in rbuergi/Feedback carry nodeType: "Feedback", a NodeType that does not exist. That is a different gap (Create accepting an unknown NodeType) and is not addressed here.
    • I modified no live nodes.

Pairs-with: none — no public type or member is removed. ValidateContentWithSchema(MeshNode) keeps its signature, the new overload is private, and the new UnknownContentMembersOfType is internal.
Mirror-sync: none — the new key content.schema.unknownDiscriminator is a server-side write refusal, returned as an error string by the write boundary and never rendered by the React/RN clients. The next routine npm run sync:i18n picks it up.

Paired with the Plugins half (loudness for existing dead letters), which follows this PR.

🤖 Generated with Claude Code

…ilent member drop

Plugins#3042: Feedback/Feedback nodes created as {"$type":"Feedback",...} (no such
type; the NodeType binds FeedbackContent) were stored and became dead letters.

- ContentSchemaValidator: a $type that contradicts the declared content type and
  resolves on none of this hub's registry, the mesh-wide content-type map, or the
  declared type's own assembly is refused (content.schema.unknownDiscriminator).
  ContentDiscriminatorValidator exempts runtime-compiled NodeTypes, so nothing
  refused it there before.
- MeshOperations.Create: refuses top-level content members the bound type does not
  declare (the Patch/Update rule) and the same unresolvable $type, judged on the
  probe hub. The probe resolved the bound type by the NodeType PATH as a $type name,
  which misses whenever the record is not named like the NodeType; it now falls back
  to IMeshContentTypeRegistry.TryResolveByNodeType (create-only checks; the historical
  ValidateAgainst keeps its reach).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 7, 2026 07:11
@meshweaver-cloud
meshweaver-cloud Bot enabled auto-merge October 7, 2026 07:11
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

    18 files   -   4      18 suites   - 4   48m 34s ⏱️ +14s
10 684 tests  - 280  10 684 ✅  - 87  0 💤  - 193  0 ❌ ±0 
10 697 runs   - 280  10 697 ✅  - 87  0 💤  - 193  0 ❌ ±0 

Results for commit a9fa0f8. ± Comparison against base commit cac0664.

This pull request removes 329 and adds 25 tests. Note that renamed tests count towards both.

   --- End of inner exception stack trace ---
   --- End of inner exception stack trace ---, expected: True)
   --- End of inner exception stack trace ---, isDenial: True)
 ---> (Inner Exception #1) MeshWeaver.Mesh.QueryProviderStalledException: Query provider(s) [pg] did not emit an Initial within the query fan-in's 16s bound for query 'nodeType:NodeType' (user 'system'). The merged Initial gates on EVERY provider, so this query has NO snapshot to answer with — it is reported as unavailable (retryable) rather than left hanging with no error. This is an availability failure, never a permission verdict: a consumer deciding access must fail CLOSED and say it could not establish the answer. Fix the stalled provider; never bump the consumer's timeout.<---
 ---> (Inner Exception #1) MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432<---
 ---> (Inner Exception #1) System.ArgumentException: Value does not fall within the expected range.<---
 ---> (Inner Exception #1) System.InvalidOperationException: boom<---
 ---> (Inner Exception #1) System.InvalidOperationException: source B is misconfigured<---
 ---> (Inner Exception #1) System.Net.Sockets.SocketException (0xFFFDFFFF): Name or service not known<---
…
Memex.Portal.Shared.Test.InstanceIdRulesMatchTheRegistryTest ‑ TheSetupHostAgreesWithTheRegistry(candidate: "997599f0-bd3b-4243-8bd4-2c5a47cfcd8c")
Memex.Portal.Shared.Test.SessionDenialIsAnAnswerTest ‑ OnlyAVerdictReadsAsADenial(shape: "the same verdict nested, as a late denial dispatch"···, failure: System.InvalidOperationException: write failed
 ---> System.UnauthorizedAccessException: Access denied
   --- End of inner exception stack trace ---, isDenial: True)
MeshWeaver.Compiler.Pipeline.Test.SourcesWatcherStallBackoffTest ‑ The_production_classifier_recognises_a_stall_through_every_wrapping(because: "a stall as an aggregate's FIRST member", fault: System.AggregateException: One or more errors occurred. (Query provider(s) [pg] did not emit an Initial within the query fan-in's 16s bound for query 'nodeType:NodeType' (user 'system'). The merged Initial gates on EVERY provider, so this query has NO snapshot to answer with — it is reported as unavailable (retryable) rather than left hanging with no error. This is an availability failure, never a permission verdict: a consumer deciding access must fail CLOSED and say it could not establish the answer. Fix the stalled provider; never bump the consumer's timeout.)
 ---> MeshWeaver.Mesh.QueryProviderStalledException: Query provider(s) [pg] did not emit an Initial within the query fan-in's 16s bound for query 'nodeType:NodeType' (user 'system'). The merged Initial gates on EVERY provider, so this query has NO snapshot to answer with — it is reported as unavailable (retryable) rather than left hanging with no error. This is an availability failure, never a permission verdict: a consumer deciding access must fail CLOSED and say it could not establish the answer. Fix the stalled provider; never bump the consumer's timeout.
   --- End of inner exception stack trace ---, expected: True)
MeshWeaver.Compiler.Pipeline.Test.SourcesWatcherStallBackoffTest ‑ The_production_classifier_recognises_a_stall_through_every_wrapping(because: "a stall as an aggregate's SECOND member", fault: System.AggregateException: One or more errors occurred. (The operation has timed out.) (Query provider(s) [pg] did not emit an Initial within the query fan-in's 16s bound for query 'nodeType:NodeType' (user 'system'). The merged Initial gates on EVERY provider, so this query has NO snapshot to answer with — it is reported as unavailable (retryable) rather than left hanging with no error. This is an availability failure, never a permission verdict: a consumer deciding access must fail CLOSED and say it could not establish the answer. Fix the stalled provider; never bump the consumer's timeout.)
 ---> System.TimeoutException: The operation has timed out.
   --- End of inner exception stack trace ---
 ---> (Inner Exception #1) MeshWeaver.Mesh.QueryProviderStalledException: Query provider(s) [pg] did not emit an Initial within the query fan-in's 16s bound for query 'nodeType:NodeType' (user 'system'). The merged Initial gates on EVERY provider, so this query has NO snapshot to answer with — it is reported as unavailable (retryable) rather than left hanging with no error. This is an availability failure, never a permission verdict: a consumer deciding access must fail CLOSED and say it could not establish the answer. Fix the stalled provider; never bump the consumer's timeout.<---
, expected: True)
MeshWeaver.Compiler.Pipeline.Test.SourcesWatcherStallBackoffTest ‑ The_production_classifier_recognises_a_stall_through_every_wrapping(because: "a stall nested in an aggregate inside a wrapper", fault: System.InvalidOperationException: outer
 ---> System.AggregateException: One or more errors occurred. (Value does not fall within the expected range.) (Query provider(s) [pg] did not emit an Initial within the query fan-in's 16s bound for query 'nodeType:NodeType' (user 'system'). The merged Initial gates on EVERY provider, so this query has NO snapshot to answer with — it is reported as unavailable (retryable) rather than left hanging with no error. This is an availability failure, never a permission verdict: a consumer deciding access must fail CLOSED and say it could not establish the answer. Fix the stalled provider; never bump the consumer's timeout.)
 ---> System.ArgumentException: Value does not fall within the expected range.
   --- End of inner exception stack trace ---
 ---> (Inner Exception #1) MeshWeaver.Mesh.QueryProviderStalledException: Query provider(s) [pg] did not emit an Initial within the query fan-in's 16s bound for query 'nodeType:NodeType' (user 'system'). The merged Initial gates on EVERY provider, so this query has NO snapshot to answer with — it is reported as unavailable (retryable) rather than left hanging with no error. This is an availability failure, never a permission verdict: a consumer deciding access must fail CLOSED and say it could not establish the answer. Fix the stalled provider; never bump the consumer's timeout.<---

   --- End of inner exception stack trace ---, expected: True)
MeshWeaver.Compiler.Pipeline.Test.SourcesWatcherStallBackoffTest ‑ The_production_classifier_recognises_a_stall_through_every_wrapping(because: "a stall wrapped as InnerException", fault: System.InvalidOperationException: outer
 ---> MeshWeaver.Mesh.QueryProviderStalledException: Query provider(s) [pg] did not emit an Initial within the query fan-in's 16s bound for query 'nodeType:NodeType' (user 'system'). The merged Initial gates on EVERY provider, so this query has NO snapshot to answer with — it is reported as unavailable (retryable) rather than left hanging with no error. This is an availability failure, never a permission verdict: a consumer deciding access must fail CLOSED and say it could not establish the answer. Fix the stalled provider; never bump the consumer's timeout.
   --- End of inner exception stack trace ---, expected: True)
MeshWeaver.Compiler.Pipeline.Test.SourcesWatcherStallBackoffTest ‑ The_production_classifier_recognises_a_stall_through_every_wrapping(because: "an aggregate of unrelated faults", fault: System.AggregateException: One or more errors occurred. (The operation has timed out.) (Value does not fall within the expected range.)
 ---> System.TimeoutException: The operation has timed out.
   --- End of inner exception stack trace ---
 ---> (Inner Exception #1) System.ArgumentException: Value does not fall within the expected range.<---
, expected: False)
MeshWeaver.Graph.Test.ContentSchemaValidationTest ‑ Create_AsJsonObject_WhoseTypeDiscriminatorNamesNoType_IsRefused
MeshWeaver.Graph.Test.ContentSchemaValidationTest ‑ Create_WhoseTypeDiscriminatorNamesAnExistingOtherType_StillLands
MeshWeaver.Graph.Test.ContentSchemaValidationTest ‑ Create_WhoseTypeDiscriminatorNamesNoType_IsRefused_NamingItAndTheDeclaredType
…

♻️ This comment has been updated with latest results.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

JsonObject writes can bypass validation, and MCP validation uses incomplete type resolution and incorrect serializer options.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)
What changed in this PR

Hardens mesh writes against invalid $type discriminators and silently dropped content members.

Changes:

  • Validates unresolved discriminators during Create and Update.
  • Adds MCP create validation and regression coverage.
  • Documents behavior and localizes refusal messages.
File Description
ContentSchemaValidationTest.cs Adds write-validation integration tests.
strings.en.json Adds English refusal text.
strings.de.json Adds German refusal text.
MeshOperations.cs Validates raw MCP create content.
ContentSchemaValidator.cs Rejects unresolved foreign discriminators.
ContentSchemaOnWrite.md Documents the new validation rule.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/MeshWeaver.Graph/Security/ContentSchemaValidator.cs
Comment thread src/MeshWeaver.Mesh.Operations/MeshOperations.cs Outdated
Comment thread src/MeshWeaver.Mesh.Operations/MeshOperations.cs
@systemorph-com systemorph-com Bot added the thread:pr-systemorph-meshweaver-6231-4892c1 https://memex.systemorph.com/Hosting/Triage/_Thread/pr-systemorph-meshweaver-6231 label Oct 7, 2026
@systemorph-com
systemorph-com Bot disabled auto-merge October 7, 2026 07:17
…es probe options and the mesh-wide registry

Review on #6231:
- ContentSchemaValidator normalizes the as-written JsonObject DOM to a JsonElement before judging,
  so a direct Create/Update carrying {"$type":"Feedback"} as a JsonObject is refused too (new test).
- MeshOperations.ValidateCreatedContent judges unknown members with the probe hub's serializer
  options, which own the bound type's contract.
- MeshOperations.DiscriminatorResolves consults IMeshContentTypeRegistry (full + short name), the same
  instruments as the write-boundary validator, so the verb never refuses what the boundary admits.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 7, 2026 07:20
@meshweaver-cloud
meshweaver-cloud Bot enabled auto-merge October 7, 2026 07:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Assembly-only type detection can still admit unreadable content, and new MCP errors bypass localization.

Review effort: Balanced
Findings: None

Resolved since last review (3)
Previously missed (6)

In code that hasn't changed since last review

Medium severity Assembly scan accepts unresolvable same-named CLR types

src/​MeshWeaver.Graph/​Security/​ContentSchemaValidator.cs:279

This assembly scan treats any same-named CLR type as resolvable, but the actual receive paths do not resolve arbitrary assembly types: WithContentType registers only the bound type (MeshDataSource.cs:2277-2298), and PolymorphicTypeInfoResolver.cs:189-219 explicitly warns that an unregistered same-assembly subtype drops on receipt. A payload can therefore use the name of an unrelated/unregistered helper type from this assembly, pass this guard, and still remain an untyped JsonElement, recreating the dead-letter behavior. The existing control uses RequiredMemberContent, which is already registered by another NodeType, so it does not cover this branch. Restrict acceptance to names the reader registries can actually resolve (or add a real reader resolution path) and add an assembly-only negative control.

Medium severity Typed-content Create refusal is hard-coded in English

src/​MeshWeaver.Mesh.Operations/​MeshOperations.cs:1790

This new Create refusal routes through UnknownContentMembersMessage, which is an English-only user-visible error. Consequently the German locale receives English for the typed-content Create path. Move this response into the localization catalog and resolve it from the caller's locale.

This issue also appears on line 3377 of the same file.

Medium severity MCP refusal bypasses caller locale localization

src/​MeshWeaver.Mesh.Operations/​MeshOperations.cs:3372

This newly returned MCP error is hard-coded English, so a German caller bypasses the localized content.schema.unknownDiscriminator message added by this PR. Localize the complete MCP refusal using the caller's captured locale (including the Error:/remedy text), rather than maintaining a parallel English-only explanation.

Medium severity Create accepts assembly-local discriminators unavailable to readers

src/​MeshWeaver.Mesh.Operations/​MeshOperations.cs:3408

This repeats the write-boundary false positive: merely finding a CLR type in the bound type's assembly does not make that discriminator resolvable by the receive pipeline. WithContentType registers only the bound content type, while PolymorphicTypeInfoResolver.cs:189-219 documents that unregistered same-assembly subtypes drop on receipt. MCP create can therefore approve an assembly-local helper name that consumers still cannot materialize. Keep this predicate aligned with the actual reader registries (or implement the missing reader resolution) and cover an assembly-only, unregistered name.

Low severity Assembly-local subtype legality conflicts with receive resolution

src/​MeshWeaver.Documentation/​Data/​Architecture/​ContentSchemaOnWrite.md:209

The statement that an assembly-local subtype stays legal is not true for the current receive pipeline: unregistered same-assembly subtypes are explicitly reported as dropping on receipt in PolymorphicTypeInfoResolver.cs:189-219. As written, the new validators can admit a discriminator solely because an unrelated CLR type has that name while consumers still cannot materialize it. Update this section after the resolution predicate is made consistent with the reader.

Low severity Add Update coverage for unknown discriminator transitions

src/​MeshWeaver.Graph/​Security/​ContentSchemaValidator.cs:244

The new Update behavior is not covered by the added tests: every unknown-discriminator case creates a node. Please pin both sides of this branch—changing an existing node to a new unknown $type must be refused, while reasserting the same already-stored unknown discriminator must remain allowed—so a future reorder cannot either reintroduce bad writes or make legacy rows irreparable.

@systemorph-com
systemorph-com Bot disabled auto-merge October 7, 2026 07:35
@rbuergi
rbuergi enabled auto-merge October 7, 2026 07:45
@systemorph-com
systemorph-com Bot disabled auto-merge October 7, 2026 07:51
Copilot AI balanced review requested due to automatic review settings October 7, 2026 07:56
@meshweaver-cloud
meshweaver-cloud Bot enabled auto-merge October 7, 2026 07:56
@systemorph-com
systemorph-com Bot disabled auto-merge October 7, 2026 07:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

MCP Create incorrectly rejects members of an otherwise admissible foreign content type, and Update behavior lacks coverage.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)

Comment on lines +241 to +244
if (context.Operation == NodeOperation.Update
&& ExistingDiscriminator(context.ExistingNode) is { } existing
&& string.Equals(existing, discriminator, StringComparison.Ordinal))
return NodeValidationResult.Valid();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 9083720. (1) Update_ToATypeDiscriminatorThatNamesNoType_IsRefused creates a well-formed SchemaCompiled node, then sends UpdateNode with $type "Feedback". The update is refused and the message names 'Feedback' and SchemaGuardedContent. (2) Update_KeepingTheStoredUnresolvableTypeDiscriminator_IsAdmitted_ChangingToItIsNot covers the exemption. The guarded write path can no longer store a dead letter, so this test calls ContentSchemaValidator directly with an Update context. When ExistingNode already carries $type "Feedback", the write is Valid. As a negative control, the same write against an ExistingNode with $type SchemaGuardedContent is refused. If the exemption is widened, the second assertion fails; if it disappears, the first one fails. ContentSchemaValidationTest: 17/17 green locally, built with -c Release -warnaserror.

Comment thread src/MeshWeaver.Mesh.Operations/MeshOperations.cs
…ite boundary; pin the Update discriminator cases

A $type naming a DIFFERENT real type carries that type's members, so the MCP
create verb no longer judges them against the declared type (it refused what
ContentSchemaValidator admits). Adds the MCP control, an Update-to-unknown-$type
refusal, and the keep-the-stored-$type exemption with its negative control.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 7, 2026 08:41
@meshweaver-cloud
meshweaver-cloud Bot enabled auto-merge October 7, 2026 08:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The new raw-content parse rejects comments and trailing commas that the existing create parser accepts.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Use matching JSON options for the second parse

src/​MeshWeaver.Mesh.Operations/​MeshOperations.cs:1761

This second parse uses JsonNode.Parse's strict defaults, while the preceding deserialization intentionally accepts comments and trailing commas via the hub options. A create payload containing either now deserializes successfully and then returns Invalid JSON here, regressing the existing input contract. Parse the raw DOM with matching document options.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

  4 files  +  1    4 suites  +1   7m 38s ⏱️ +13s
415 tests  - 156  415 ✅ +35  0 💤  - 191  0 ❌ ±0 
419 runs   - 156  419 ✅ +35  0 💤  - 191  0 ❌ ±0 

Results for commit a9fa0f8. ± Comparison against base commit cac0664.

This pull request removes 193 and adds 37 tests. Note that renamed tests count towards both.
MeshWeaver.Portal.E2E.AgentFilesToolingTest ‑ EveryTool_RunsForReal_AndTheFileIsARealMeshNode
MeshWeaver.Portal.E2E.AgentFilesToolingTest ‑ TheWorkingArea_IsScopedToItsOwnThread
MeshWeaver.Portal.E2E.BlazorVsNextCompareTest ‑ Compare_Blazor_Vs_Next_AcrossScreens
MeshWeaver.Portal.E2E.ChatAtAutocompleteTest ‑ TypingAt_SurfacesLocalPartitionNodes_AndSlashExpandsPartitions
MeshWeaver.Portal.E2E.ChatClearSkillNewComposerTest ‑ ClearSkill_InSidePanel_ReplacesThreadWithComposer_AndLeavesMainAlone
MeshWeaver.Portal.E2E.ChatComposerRefocusTest ‑ Composer_AcceptsTyping_AfterClickingAwayAndBack
MeshWeaver.Portal.E2E.ChatComposerSafariFocusReproTest ‑ Composer_AcceptsTyping_AfterFocusStealDuringStream
MeshWeaver.Portal.E2E.ChatComposerStreamingFocusTest ‑ Composer_KeepsFocus_WhileResponseStreams
MeshWeaver.Portal.E2E.ChatComposerSwitchSelectionTest ‑ SlashCommands_OpenPicker_AndSelectionUpdatesStatusRow
MeshWeaver.Portal.E2E.ChatDelegationTest ‑ Coordinator_DelegatesToWorker_SubThreadSpawns_ResultFlowsBack_AndRendersWhenOpened
…
MeshWeaver.ContentCollections.Test.ContentCollectionClaimApplyRaceTest ‑ TornIngestOvertakenBetweenClaimAndPost_CannotOverwriteTheLaterTriggeredArticle
MeshWeaver.ContentCollections.Test.ContentCollectionConnectionOwnedByHubTest ‑ AfterTheHubsTeardown_GetCollectionIsRefused_NotServedFromTheReplay
MeshWeaver.ContentCollections.Test.ContentCollectionDiesWithItsHubTest ‑ DisposingTheHub_DisposesTheCollectionsItsContentServiceCreated
MeshWeaver.ContentCollections.Test.ContentCollectionIngestOrderTest ‑ TornReadFromTheCreatedEvent_CannotOverwriteTheCompleteArticle
MeshWeaver.ContentCollections.Test.ContentCollectionWriteIngestTest ‑ SaveFile_ingests_the_article_without_the_watcher
MeshWeaver.ContentCollections.Test.ContentCollectionWriteReadRaceTest ‑ Write_Succeeds_While_A_Concurrent_Read_Holds_The_File
MeshWeaver.ContentCollections.Test.ContentDeleteNotificationTest ‑ A_collection_already_qualified_by_its_address_is_not_qualified_twice
MeshWeaver.ContentCollections.Test.ContentDeleteNotificationTest ‑ Deleting_a_file_notifies_observers_with_the_qualified_collection_path
MeshWeaver.ContentCollections.Test.ContentDeleteNotificationTest ‑ Deleting_a_folder_notifies_observers_once_per_contained_file
MeshWeaver.ContentCollections.Test.ContentServiceDelegationTest ‑ ContentService_ShouldDelegateToParent
…

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

2 064 tests  +658   2 064 ✅ +658   4m 37s ⏱️ -11s
    3 suites  -   2       0 💤 ±  0 
    3 files    -   2       0 ❌ ±  0 

Results for commit a9fa0f8. ± Comparison against base commit cac0664.

This pull request removes 26 and adds 684 tests. Note that renamed tests count towards both.
MeshWeaver.Cli.Test.GateHostCompositionTest ‑ ATesterReference_IsRecognisedAsTheWrongPlatformImage(image: "ghcr.io/systemorph/memex-portal-ai:main", isTester: False)
MeshWeaver.Cli.Test.GateHostCompositionTest ‑ ATesterReference_IsRecognisedAsTheWrongPlatformImage(image: "meshweaver.azurecr.io/MW-Plugin-Test@sha256:abc", isTester: True)
MeshWeaver.Cli.Test.GateHostCompositionTest ‑ ATesterReference_IsRecognisedAsTheWrongPlatformImage(image: "meshweaver.azurecr.io/memex-portal-ai@sha256:abc", isTester: False)
MeshWeaver.Cli.Test.GateHostCompositionTest ‑ ATesterReference_IsRecognisedAsTheWrongPlatformImage(image: "meshweaver.azurecr.io/mw-plugin-test:3.0.0-rc9.ci."···, isTester: True)
MeshWeaver.Cli.Test.GateHostCompositionTest ‑ DiscardTree_OnAnAbsentPath_IsSilent
MeshWeaver.Cli.Test.GateHostCompositionTest ‑ DiscardTree_RemovesTheTree
MeshWeaver.Cli.Test.GateHostCompositionTest ‑ DiscardTree_ThatCannotBeRemoved_ReportsAndDoesNotThrow
MeshWeaver.Cli.Test.GateHostCompositionTest ‑ EveryRun_StartsThePlatformImagesDotnet_OnTheComposedHostsTesterCli
MeshWeaver.Cli.Test.GateHostCompositionTest ‑ ExtractComposeScript_WritesTheScriptWhereTheCliRunsIt
MeshWeaver.Cli.Test.GateHostCompositionTest ‑ MountsAndEnvironment_LandBeforeTheImage
…
MeshWeaver.Documentation.Test.AWebhookIssueSnapshotKeepsItsCloseDecisionTest ‑ ADuplicateCloseSurvivesTheWebhook
MeshWeaver.Documentation.Test.AWebhookIssueSnapshotKeepsItsCloseDecisionTest ‑ AnOpenIssueCarriesNoCloseDecision
MeshWeaver.Documentation.Test.AWebhookIssueSnapshotKeepsItsCloseDecisionTest ‑ TheTwoMappersAgree(wireValue: "completed", expected: Completed)
MeshWeaver.Documentation.Test.AWebhookIssueSnapshotKeepsItsCloseDecisionTest ‑ TheTwoMappersAgree(wireValue: "duplicate", expected: Duplicate)
MeshWeaver.Documentation.Test.AWebhookIssueSnapshotKeepsItsCloseDecisionTest ‑ TheTwoMappersAgree(wireValue: "not_planned", expected: NotPlanned)
MeshWeaver.Documentation.Test.AgentsRuleAndSkillAgreementGuard ‑ BothDetectorsSeeTheDefectTheyWereWrittenFor
MeshWeaver.Documentation.Test.AgentsRuleAndSkillAgreementGuard ‑ EveryPairsRuleIsStillStatedInAgentsMd
MeshWeaver.Documentation.Test.AgentsRuleAndSkillAgreementGuard ‑ NoSkillCarriesASupersededRulesWording
MeshWeaver.Documentation.Test.AgentsRuleAndSkillAgreementGuard ‑ NoSkillMintsAFileUnderAWhatsNewPath
MeshWeaver.Documentation.Test.AmbientCultureRatchetGuard ‑ NoSourceFile_ReachesForAnAmbientCulture
…

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    2 files  ±0      2 suites  ±0   8m 30s ⏱️ -12s
1 119 tests ±0  1 119 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 120 runs  ±0  1 120 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit a9fa0f8. ± Comparison against base commit cac0664.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

    3 files   -     1      3 suites   - 1   8m 40s ⏱️ + 4m 12s
2 919 tests +1 261  2 919 ✅ +1 263  0 💤  - 2  0 ❌ ±0 
2 923 runs  +1 264  2 923 ✅ +1 266  0 💤  - 2  0 ❌ ±0 

Results for commit a9fa0f8. ± Comparison against base commit cac0664.

This pull request removes 60 and adds 1321 tests. Note that renamed tests count towards both.
MeshWeaver.ContentCollections.Test.ContentCollectionClaimApplyRaceTest ‑ TornIngestOvertakenBetweenClaimAndPost_CannotOverwriteTheLaterTriggeredArticle
MeshWeaver.ContentCollections.Test.ContentCollectionConnectionOwnedByHubTest ‑ AfterTheHubsTeardown_GetCollectionIsRefused_NotServedFromTheReplay
MeshWeaver.ContentCollections.Test.ContentCollectionDiesWithItsHubTest ‑ DisposingTheHub_DisposesTheCollectionsItsContentServiceCreated
MeshWeaver.ContentCollections.Test.ContentCollectionIngestOrderTest ‑ TornReadFromTheCreatedEvent_CannotOverwriteTheCompleteArticle
MeshWeaver.ContentCollections.Test.ContentCollectionWriteIngestTest ‑ SaveFile_ingests_the_article_without_the_watcher
MeshWeaver.ContentCollections.Test.ContentCollectionWriteReadRaceTest ‑ Write_Succeeds_While_A_Concurrent_Read_Holds_The_File
MeshWeaver.ContentCollections.Test.ContentDeleteNotificationTest ‑ A_collection_already_qualified_by_its_address_is_not_qualified_twice
MeshWeaver.ContentCollections.Test.ContentDeleteNotificationTest ‑ Deleting_a_file_notifies_observers_with_the_qualified_collection_path
MeshWeaver.ContentCollections.Test.ContentDeleteNotificationTest ‑ Deleting_a_folder_notifies_observers_once_per_contained_file
MeshWeaver.ContentCollections.Test.ContentServiceDelegationTest ‑ ContentService_ShouldDelegateToParent
…
MeshWeaver.Compiler.Pipeline.Test.AFailedCompileCarriesTheSetItConsumedTest ‑ TheFailureResult_FoldsTheConsumedSetUnderTheSameRuleAsTheLiveSnapshot
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ APrebuiltAdoption_RetiresThePreviousBuildsPendingRelease
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ AReleaseAtAnotherPath_IsNotAdopted_AndTheStampStands
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ ARelease_LandingAtTheStampedPath_IsAdopted
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ AReplayOfThePendingBuild_PreservesItsMarker
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ ASeedReturningThePendingCoordinates_KeepsTheLateReleaseWatch
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ Adopt_LeavesTheNodeAlone_ForAnyOtherPath(landed: "")
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ Adopt_LeavesTheNodeAlone_ForAnyOtherPath(landed: "T/Release/20260923055426-OtherByt")
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ Adopt_LeavesTheNodeAlone_ForAnyOtherPath(landed: "t/release/20260923055416-hd-ifsia")
MeshWeaver.Compiler.Pipeline.Test.ALateReleaseIsAdoptedWhenItLandsTest ‑ Adopt_LeavesTheNodeAlone_WhenALaterSettleClearedTheStamp
…

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

    4 files  ±    0      4 suites  ±0   13m 20s ⏱️ - 2m 49s
2 297 tests  - 1 307  2 297 ✅  - 1 307  0 💤 ±0  0 ❌ ±0 
2 297 runs   - 1 310  2 297 ✅  - 1 310  0 💤 ±0  0 ❌ ±0 

Results for commit a9fa0f8. ± Comparison against base commit cac0664.

This pull request removes 1346 and adds 15 tests. Note that renamed tests count towards both.

   --- End of inner exception stack trace ---
   --- End of inner exception stack trace ---, expected: True)
   --- End of inner exception stack trace ---, isDenial: True)
 ---> (Inner Exception #1) MeshWeaver.Mesh.QueryProviderStalledException: Query provider(s) [pg] did not emit an Initial within the query fan-in's 16s bound for query 'nodeType:NodeType' (user 'system'). The merged Initial gates on EVERY provider, so this query has NO snapshot to answer with — it is reported as unavailable (retryable) rather than left hanging with no error. This is an availability failure, never a permission verdict: a consumer deciding access must fail CLOSED and say it could not establish the answer. Fix the stalled provider; never bump the consumer's timeout.<---
 ---> (Inner Exception #1) MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432<---
 ---> (Inner Exception #1) System.ArgumentException: Value does not fall within the expected range.<---
 ---> (Inner Exception #1) System.InvalidOperationException: boom<---
 ---> (Inner Exception #1) System.InvalidOperationException: source B is misconfigured<---
 ---> (Inner Exception #1) System.Net.Sockets.SocketException (0xFFFDFFFF): Name or service not known<---
…
Memex.Portal.Shared.Test.SessionDenialIsAnAnswerTest ‑ OnlyAVerdictReadsAsADenial(shape: "the same verdict nested, as a late denial dispatch"···, failure: System.InvalidOperationException: write failed
 ---> System.UnauthorizedAccessException: Access denied
   --- End of inner exception stack trace ---, isDenial: True)
MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest ‑ Classifies(shape: "a MIXED aggregate — one transient branch, one genu"···, exception: System.AggregateException: One or more errors occurred. (Failed to connect to 10.42.18.4:5432) (source B is misconfigured)
 ---> MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432
   --- End of inner exception stack trace ---
 ---> (Inner Exception #1) System.InvalidOperationException: source B is misconfigured<---
, expected: False)
MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest ‑ Classifies(shape: "a nested aggregate with one genuine leaf", exception: System.AggregateException: One or more errors occurred. (One or more errors occurred. (Failed to connect to 10.42.18.4:5432) (boom)) (Failed to connect to 10.42.18.4:5432)
 ---> System.AggregateException: One or more errors occurred. (Failed to connect to 10.42.18.4:5432) (boom)
 ---> MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432
   --- End of inner exception stack trace ---
 ---> (Inner Exception #1) System.InvalidOperationException: boom<---

   --- End of inner exception stack trace ---
 ---> (Inner Exception #1) MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432<---
, expected: False)
MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest ‑ Classifies(shape: "a nested aggregate, all leaves transient", exception: System.AggregateException: One or more errors occurred. (One or more errors occurred. (Failed to connect to 10.42.18.4:5432)) (Failed to connect to 10.42.18.4:5432)
 ---> System.AggregateException: One or more errors occurred. (Failed to connect to 10.42.18.4:5432)
 ---> MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432
   --- End of inner exception stack trace ---
   --- End of inner exception stack trace ---
 ---> (Inner Exception #1) MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432<---
, expected: True)
MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest ‑ Classifies(shape: "a reflective wrapper around a transient cause", exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.Net.Sockets.SocketException (110): Connection timed out
   --- End of inner exception stack trace ---, expected: True)
MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest ‑ Classifies(shape: "a transient cause UNDER an aggregate branch's wrap"···, exception: System.AggregateException: One or more errors occurred. (wrapped)
 ---> System.InvalidOperationException: wrapped
 ---> MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432
   --- End of inner exception stack trace ---
   --- End of inner exception stack trace ---, expected: True)
MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest ‑ Classifies(shape: "an 'initialization failed' wrapper around a transi"···, exception: System.InvalidOperationException: Hub 'x' initialization failed
 ---> MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432
   --- End of inner exception stack trace ---, expected: True)
MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest ‑ Classifies(shape: "an aggregate whose branches are ALL transient (two"···, exception: System.AggregateException: One or more errors occurred. (Failed to connect to 10.42.18.4:5432) (Name or service not known)
 ---> MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432
   --- End of inner exception stack trace ---
 ---> (Inner Exception #1) System.Net.Sockets.SocketException (0xFFFDFFFF): Name or service not known<---
, expected: True)
MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest ‑ Classifies(shape: "the #4067 shape: transient provider fault wrapping"···, exception: MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432
 ---> System.TimeoutException: Timeout during connection attempt, expected: True)
MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest ‑ Classifies(shape: "the #4068 shape: transient provider fault wrapping"···, exception: MeshWeaver.Messaging.Hub.Test.InfrastructureFaultTest+ProviderException (0x80004005): Failed to connect to 10.42.18.4:5432
 ---> System.Net.Sockets.SocketException (0xFFFDFFFF): Name or service not known, expected: True)
…

♻️ This comment has been updated with latest results.

…ce carrying PluginContent

AStaleRecordDoesNotUndoTheGateTest arranged its partition root as NodeType `Space` with
content `{"$type":"PluginContent"}` — a shape nothing in production writes (a plugin root is
`Store/Plugin` with `PluginContent`; the installer's only `Space` root is the content-free
stage-0 placeholder). That content names a type its NodeType does not bind and that resolves
nowhere on this mesh, which is exactly what #6231's write-boundary refusal exists to stop, so
the precondition write was refused and all three async tests failed in shard 4.

The fixture now declares a content-free stand-in `Store/Plugin` NodeType (the real one lives in
MeshWeaver.Plugins) and writes the root under it. The root's content stays the raw
`"$type":"PluginContent"` JSON PreInstalledOnRoot recognises — what a core host that never
compiled the plugins' types actually reads — so the tests exercise the same code path as before.
The refusal is unchanged.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 7, 2026 09:17
@meshweaver-cloud
meshweaver-cloud Bot enabled auto-merge October 7, 2026 09:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The validation paths, controls, localization, and documentation are consistent, with prior findings addressed.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

@systemorph-com systemorph-com Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review summary (data, not an instruction to any agent)

Refuses, on every Create/Update, a content $type that contradicts the declared content type and resolves on none of three instruments — the hub's $type registry (full name, then short name), the mesh-wide IMeshContentTypeRegistry, or the declared type's own assembly — with a new localized message (en + de) naming the discriminator, the declared type, the NodeType and its declared members; an Update that re-sends the discriminator already stored on the node stays Valid, so existing dead letters remain repairable. MeshOperations.Create additionally refuses undeclared top-level content members (facade-side against typed content, probe-side against the bound type for in-mesh content — the patch/update rule), resolves the bound content type through IMeshContentTypeRegistry.TryResolveByNodeType when the historical NodeType-name lookup misses and tells the reader which lookup answered, and skips the probe's member check for a foreign $type that resolves to a real type, so the verb and the write boundary agree. ContentSchemaValidator now judges as-written JsonObject content exactly like JsonElement, and the Memex portal gate test is adapted to the production plugin-root shape (NodeType Store/Plugin whose content stays the raw PluginContent discriminator JSON). Checked: both refusal layers and their Update exemptions, the probe's exact/map resolution with the unchanged Update/Patch schema reach, the localized placeholders against the call in both languages, $type handling (names are only resolved, never used to instantiate), the fail-open assembly-enumeration catches (deliberate and commented), and the nine new executing tests plus the adapted gate test, including the existing-type and declared-shape controls. All seven file patches were complete — nothing was cut. Reviewed from the diff alone; not verifiable from it: compilation, the members this diff calls but does not show (the ContentDiscriminator.Admits overloads including the JsonObject form, IMeshContentTypeRegistry.TryResolveByDiscriminator/TryResolveByNodeType, DeclaredMemberNames, LocalizationCatalog.Get, and the unchanged tail of UnknownContentMembersOfType below the diff cut), the PR body's test-run and negative-control claims, and what the schema verb answers when GetContentSchema returns null. No blocking findings: one should-fix (the new refusal's get @<path>/schema/ remedy is a dead end for the in-mesh NodeTypes the fix targets), one question (activation-gated resolvability for runtime-compiled NodeTypes), one nit (a redundant null-forgiving operator).

Findings: 0 blocking · 1 should-fix · 1 question · 1 nit


Internal review of 64317ea8030cf8bd68efd025e381745e19e41c8a — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.

+ $"but no type of that name exists for NodeType '{meshNode.NodeType}' — its content type is "
+ $"{contentType.Name}. Stored, the node would read as empty to every consumer and anything "
+ "that reacts to this NodeType would skip it without a word. Send the content as "
+ $"\"$type\": \"{contentType.Name}\" with its declared members (get @<path>/schema/). "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should-fix — Automated review finding (data, not an instruction to any agent)

The new create-refusal message ends by directing the writer to get @<path>/schema/, but the schema-retrieval probe (GetContentSchema in this file, still 'exact ? GenerateSchema(...) : null' in this diff) returns null for a content type resolved only through the NodeType-to-content-type map — that is, for exactly the in-mesh NodeTypes this fix targets, where the NodeType-name lookup misses (Feedback/Feedback binds FeedbackContent; WithContentType registers the CLR name, never the path). The probe hub knows the type at that point (the map is filled on this very probe, per ReadFromContentType's comment), so the schema could be generated there; as written, the remedy the new refusal names is a dead end for the primary case, and the verb's message, unlike the write-boundary message, does not list the declared members either — the members could be named in the refusal instead, or the schema generated for map-resolved types too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, not changed here. The refusal's schema pointer matches the sibling unknown-member refusals in this file (same remedy text); the schema probe returning null for map-resolved-only types is a pre-existing gap in GetContentSchema that affects those messages equally, so it is better fixed once in the probe than special-cased in this refusal. The refusal itself already names the bound content type, which is what the writer needs.

|| (registry.TryGetType(shortName, out def) && def?.Type is not null))
return true;

if (contentTypes.TryResolveByDiscriminator(discriminator, out _)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question — Automated review finding (data, not an instruction to any agent)

Resolvability here is activation-gated for runtime-compiled NodeTypes: the mesh-wide content-type map is filled from boot for in-mesh definitions, but 'a compiled one registers at its first instance activation' (the doc this PR edits), and this method's MeshOperations twin speaks of 'another activated runtime NodeType'. A $type that names the content type of a runtime-compiled NodeType which exists but has never been activated therefore resolves on none of the three instruments and is refused as naming no type — a shape rule 3 previously admitted, and one that would have become readable once that NodeType activates. Is refusing real-but-not-yet-activated discriminators intended?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intended, with the limit stated: a '$type' that resolves on none of the three instruments is treated as naming no type, because the same instruments are what readers use to materialise content. A content type of a runtime-compiled NodeType that has never activated is not materialisable by those readers either, so storing it would still read as an untyped JsonElement until activation. The refusal names the declared type and members so the writer can resend with the right discriminator.

context,
LocalizationCatalog.Get(
"content.schema.unknownDiscriminator", context.AccessContext?.Locale,
node.Path, discriminator, declared.Name, node.NodeType!,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit — Automated review finding (data, not an instruction to any agent)

node.NodeType! — the null-forgiving operator is redundant: Judge has already returned when NodeType is null or empty, so the value passed here is non-null and the '!' silences nothing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged: the '!' is redundant (Judge returns earlier for a null/empty NodeType). Harmless and nullable-clean, left as is to avoid restarting review for a nit.

@systemorph-com

systemorph-com Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🚰 PR babysitter (build instance) is merging the base into this branch on head 64317ea8030c — once per head.

Why: stale: 'Test Results', 'Consolidate test results', 'Test Results (shard 3)', 'Run tests (shard 3)' red on a head that tested 'main' at d513d1e — green on the base's newest run, so a gate the base has fixed since, not the diff ('Test Results' concluded failure: 3 fail, 10 615 pass in 52m 11s) — the base 'main' moved from d513d1e (what the red run tested) to cac0664. This pull request was red because its BASE was; the base has moved since, and a re-run would test the old merge commit again.

Validated: Stale base, not the diff: run 37599549946 failed 'Test Results', 'Consolidate test results', 'Test Results (shard 3)' and 'Run tests (shard 3)' on a head that tested base 'main' at d513d1e, while the base's newest run is GREEN on those same gates and 'main' has since moved to cac0664 — a gate the base has fixed since, which the diff cannot reach. The proposal carries no log line naming a changed file (logTails empty), the same shape as the executed stale-base heals on this page. The measured remedy is to merge the fixed 'main' into the branch (update-branch), producing a fresh head w…

It does not merge the pull request, push anything else or dequeue. A red after this is left for the owner (rbuergi).

Copilot AI balanced review requested due to automatic review settings October 7, 2026 14:15
@meshweaver-cloud
meshweaver-cloud Bot enabled auto-merge October 7, 2026 14:15
@systemorph-com
systemorph-com Bot disabled auto-merge October 7, 2026 14:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Create validation still has parser-regression and false-positive type-resolution paths, plus an untranslated MCP error.

Review effort: Balanced
Findings: 4 Medium severity · 1 Low severity

Open (5)

Comment thread src/MeshWeaver.Graph/Security/ContentSchemaValidator.cs
Comment thread src/MeshWeaver.Mesh.Operations/MeshOperations.cs Outdated
Comment thread src/MeshWeaver.Mesh.Operations/MeshOperations.cs
Comment thread src/MeshWeaver.Mesh.Operations/MeshOperations.cs
…x options so validation does not narrow accepted input

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 7, 2026 14:35
@meshweaver-cloud
meshweaver-cloud Bot enabled auto-merge October 7, 2026 14:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The update exemption can still admit an unknown discriminator while retyping a node, preserving unreadable content under the destination type.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (4)
Previously missed (1)

In code that hasn't changed since last review

Low severity Unresolvable-type error misdiagnoses ambiguous discriminator collisions

src/​MeshWeaver.Graph/​Security/​ContentSchemaValidator.cs:274

TryResolveByDiscriminator returns false both when a name is absent and when multiple declarations claim it (see IMeshContentTypeRegistry.cs:151-185), but the new refusal reports that “no such type exists.” Common collisions such as package-local content names therefore produce a false diagnosis. Keep rejecting the unresolvable payload, but word the localized and MCP errors as “does not resolve uniquely” (or otherwise distinguish ambiguity from absence).

Comment on lines +241 to +244
if (context.Operation == NodeOperation.Update
&& ExistingDiscriminator(context.ExistingNode) is { } existing
&& string.Equals(existing, discriminator, StringComparison.Ordinal))
return NodeValidationResult.Valid();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, not changed. The exemption exists only so a node that already holds an unresolvable $type can still be repaired; it never admits a NEW dead letter. An in-place retype that keeps the same stored $type moves content that was already unreadable and leaves it exactly as unreadable under the new NodeType, so nothing becomes unreadable that was readable, which is the defect this guard targets (a discriminator naming no type written fresh). Judging a retype against the destination declaration is a stricter rule than this change set claims; it is not a behaviour break of the guard as shipped, so it is left for a follow-up rather than a second push here.

@systemorph-com
systemorph-com Bot disabled auto-merge October 7, 2026 14:50
@systemorph-com

systemorph-com Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🩹 PR babysitter (build instance) is re-running the failed jobs of run(s) 37637953470 on head a9fa0f8dbb0b — once per head, never again for this commit.

Why: 'Automatic review answered' is red with every thread answered — a stale verdict; its run is re-run to read the live state.

Validated: rule (no model): 'infra' — the platform's red, not the diff's: re-run the failed jobs once per head

It does not merge, push or dequeue. A red after this re-run is left for the owner (rbuergi).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests-before-review thread:pr-systemorph-meshweaver-6231-4892c1 https://memex.systemorph.com/Hosting/Triage/_Thread/pr-systemorph-meshweaver-6231

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants