Repository navigation
fix(#3472): address the review findings, and pin the sweep-selector disagreement (#3511) - #3515
Conversation
…isagreement (#3511) Follow-up to #3509 (merged 00:14:57Z). Held unpushed while core CD is protected. 1. `MeshOperations.ResolveHubConfigForSchema` — the `.Where` still used `n?.Content is NodeTypeDefinition`, so an un-materialized node was filtered out BEFORE the `ContentAs` below it: the stream never emitted, the read hung out its whole 5 s budget, and the schema reported no configuration for a type that has one. #3509's comment claimed that was fixed; it was not, because nothing reached the fallback. Fixed in the filter, where it actually decides. 2. `NodeTypeLayoutAreas.Short` returned the English literal "(none)" INTO the localized `ui.compileForeignFrameworkBody`, so an absent identity rendered untranslated in the middle of a German sentence. Now an em dash — the house preference is a language-neutral glyph over a translated word. `NodeTypeBuildIdentity.Short` keeps "(none)": that one feeds log lines, which stay English by house rule. 3. `SweepSelectorReachesTheCompileStatusTest` + the doc section it belongs to. 🚨 The correction that matters: "the sweep is broken" was the WRONG summary, and #3509's doc carried it. The two query providers DISAGREE about which selectors exist, and nothing compares them (#3511). Measured, both halves: * in-memory / FileSystem (`QueryEvaluator`): the bare `compilationStatus:Error` resolves to null and matches NOTHING — reflection against `MeshNode`, which has no such property and no `Content` fallback. The dotted `content.compilationStatus` walks into Content and does reach it. * Postgres (`PostgreSqlSqlGenerator`, out of repo): the bare form DISCRIMINATES — a peer measured a live mesh at 5 Error / 195 Ok, disjoint. That is worse than a uniformly broken sweep: on the portal a deploy is gated on it works, while a rehearsal on a local or CI mesh is green by construction and proves nothing about it. The evaluator is pinned rather than changed — a `Content` fallback would alter the meaning of every selector in every query in the platform, and closing the gap properly is one test exercising BOTH providers, which is #3511's scope. This file is its first half and claims nothing about the provider it cannot execute. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Test Results (shard 1)491 tests 491 ✅ 44s ⏱️ Results for commit 5b58157. |
Test Results (shard 0)251 tests 251 ✅ 2m 3s ⏱️ Results for commit 5b58157. |
Test Results (shard 3)1 034 tests 1 034 ✅ 1m 18s ⏱️ Results for commit 5b58157. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, targeted, and include a new test plus documentation updates; the only remaining feedback is a non-blocking performance/clarity tweak.
Pull request overview
This PR follows up on #3509 to (1) fix a NodeType-definition filtering bug that prevented ContentAs<T> from ever being reached, (2) remove an English literal from a localized UI sentence by switching to a language-neutral glyph, and (3) add a test that pins the current “selector reachability” disagreement between query providers (scoped in #3511).
Changes:
- Fix
ResolveHubConfigForSchemato useContentAs<NodeTypeDefinition>(...)inside the stream filter so un-materialized content isn’t discarded before typing can occur. - Update
NodeTypeLayoutAreas.Short()to render missing identities as an em dash to avoid leaking English into localized UI. - Add
SweepSelectorReachesTheCompileStatusTestand update docs to explicitly describe the selector mismatch across query providers.
File summaries
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Test/SweepSelectorReachesTheCompileStatusTest.cs | Adds a focused regression test pinning bare vs dotted selector behavior in QueryEvaluator. |
| src/MeshWeaver.Mesh.Operations/MeshOperations.cs | Moves ContentAs<NodeTypeDefinition> into the .Where(...) filter so the fix is applied at the decision point. |
| src/MeshWeaver.Graph/NodeTypeLayoutAreas.cs | Replaces "(none)" with an em dash in a viewer-facing UI string substitution to avoid i18n leakage. |
| src/MeshWeaver.Documentation/Data/Architecture/BuildIdentityAdmission.md | Updates documentation to reflect the selector disagreement and points to the new pinning test. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .Where(n => n.ContentAs<NodeTypeDefinition>(hub.JsonSerializerOptions) | ||
| is { CompilationStatus: CompilationStatus.Ok } d | ||
| && !string.IsNullOrEmpty(d.LatestAssemblyCollection) | ||
| && !string.IsNullOrEmpty(d.LatestAssemblyPath)) | ||
| .Take(1) | ||
| .Timeout(TimeSpan.FromSeconds(5)) | ||
| .SelectMany(node => |
Test Results (shard 4)1 600 tests 1 406 ✅ 2m 8s ⏱️ Results for commit 5b58157. |
Test Results (shard 5) 5 files 5 suites 2m 1s ⏱️ Results for commit 5b58157. |
Test Results (shard 2)1 079 tests 1 079 ✅ 3m 27s ⏱️ Results for commit 5b58157. |
Test Results 16 files 16 suites 11m 44s ⏱️ Results for commit 5b58157. |
Follow-up to #3509 (#3472, merged as
6577052de). Two Copilot findings from that review — both real — plus a test that pins the selector disagreement #3511 is now scoped to.Pairs-with: none— removes nopublictype and nopublicmember fromsrc/; two behaviour fixes inside existing methods, one string, one new test.1 · The filter decided before the fix could
ResolveHubConfigForSchema's.Wherestill readn?.Content is NodeTypeDefinition. An un-materialized node was therefore discarded before reaching theContentAs<T>that #3509 added to handle exactly that case — so the read hung out its whole 5 s budget instead of resolving.🚨 This is the platform's own trap-door rule biting the fix meant to apply it:
is/ason anobjectpayload is correct only when the value already happens to be the CLR type, and yields a silent null for untyped JSON. The fix belongs in the filter, where the decision is actually made — putting it after the filter left the filter deciding.2 · An English literal inside a localized string — mine, introduced by #3509
Short()returned the literal"(none)"intoui.compileForeignFrameworkBody, so a German viewer got a German sentence with an English parenthetical inside it. Now an em dash — the house preference: a language-neutral glyph beats a translated word, and beats an untranslated one outright.The
"(none)"inNodeTypeBuildIdentity.Shortstays, correctly: that one feeds log lines, which are not viewer-facing and are deliberately not localized.3 ·
SweepSelectorReachesTheCompileStatusTest— and a correction to how #3511 was filedI filed #3511 claiming the documented sweep
search 'nodeType:NodeType compilationStatus:Error'"reaches nothing, on every backend". That was wrong in the consequential direction, and a peer's measurement on the live Postgres mesh disproved it: the bare form returns 5,content.compilationStatus:Errorthe same 5, andcompilationStatus:Okreturns 195 with none of the five among them — a discriminating predicate.What my code read actually established is narrower and still real:
QueryEvaluatorhas noContentfallback, soGetDirectPropertyValuereturnsnullforcompilationStatusagainst aMeshNode(the field lives onNodeTypeDefinition, insideContent). The bare form is blind on the in-memory / file-adapter / static-node path — tests included.So the real defect is two providers disagreeing about which selectors exist, with no test comparing them, and #3511 is re-scoped to that. This test is the first half: three rows pinned on the provider it can actually execute —
compilationStatus:Errornull→ matches nothingcontent.compilationStatus:ErrorOk, notError🚨 The control row is the point. Without it, a matcher that had stopped matching anything would score identically to one that works, which is the failure mode this whole family of issues is about. The test claims nothing about the Postgres path it cannot execute, and says so.
Verification
Re-run here rather than inherited — the agent that wrote this was killed by a rate limit before pushing, so its numbers were not trusted:
MeshWeaver.Mesh.OperationsRelease-warnaserror→ 0 Warning(s), 0 Error(s)MeshWeaver.Hosting.TestRelease-warnaserror→ 0 Warning(s), 0 Error(s)SweepSelectorReachesTheCompileStatusTest→ 3 passed, 0 failed, real.trx