Repository navigation
Guard the normalised node: run the AccessAssignment scope check after MainNode normalisation - #708
Merged
Merged
Conversation
… MainNode normalisation main has been red since bbd2e96 (#702) — five MeshWeaver.Query.Test cases, across four merges, blocking the portal self-update. #702 added a structural guard refusing an AccessAssignment whose MainNode does not match the scope its path encodes. The invariant is right; its POSITION was not. The guard runs at the top of CreateNode, but the satellite MainNode normalisation it implicitly depends on runs ~80 lines later, inside the existingObs.Select. MeshNode.MainNode defaults to Path, so a satellite created the ordinary way — MeshNode.FromPath("{p}/_Access/a1"), no explicit MainNode — reached the guard carrying MainNode == its own path while the path encoded scope "{p}". Mismatch, refused, and CreateNode completed without emitting. So the guard was rejecting the framework's OWN documented default shape. The clearest evidence is the name of one of the tests it broke: SatelliteTypes_HaveMainNodeSetToOwningNode — it asserts precisely the auto-derivation the guard was pre-empting. Fix: normalise first, then guard, on BOTH write paths, via a shared NormalizeSatelliteMainNode helper. UpsertNode had no normalisation at all, so it gets the helper too. That closes a real asymmetry the guard's own comment already demands ("Both write paths, or neither"): an upserted satellite pointed MainNode at ITSELF — the exact shape block 1b documents as misread by every consumer (SatelliteAccessRule delegates permissions to it, rebuild_user_effective_permissions projects the grant at COALESCE(main_node, namespace), satellite scope filters match it as the attachment point). The security property is untouched. Normalisation only fires when MainNode == Path (i.e. never explicitly set), so the memex incident shape — an EMPTY MainNode under a non-empty path scope, 43 root-scoped accounts — still fails, as does any explicit mismatch. #702's AccessAssignmentGuardTest passes 29/29 unmodified, which is the check that matters here. Fixing the five test fixtures instead would have gone green while leaving every production caller that relies on the auto-derivation refused at create. Verified: MeshWeaver.Query.Test 10/10 (the 5 previously-failing cases, unmodified), MeshWeaver.Graph.Test AccessAssignmentGuardTest 29/29, Mesh.Contract builds clean under -c Release -warnaserror. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the main-breaking regression introduced by the AccessAssignment scope guard by ensuring satellite MainNode normalization runs before the guard on both write paths (create and upsert). This restores the framework’s documented default satellite shape (where MainNode initially defaults to Path) while keeping the mis-scoped/empty-scope AccessAssignment protections intact.
Changes:
- Normalize satellite
MainNodebefore runningAccessAssignmentGuard.IsScopeInvalid(...)inCreateNode. - Apply the same normalization prior to the guard in
CreateOrUpdateNode(upsert path), closing the prior asymmetry. - Factor normalization into a shared
NormalizeSatelliteMainNode(...)helper to keep create/upsert behavior consistent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Second half of unbreaking main. #690 (de86b03) made MeshWeaver.Hosting.Monolith.Test exceed CI's 6m wall-clock cap (exit=124) on every run since it landed. CI reports that cap as "likely fixture/init hang". It is not a hang — the suite completes; it just got slower. Measured on one box, CompileLeafStabilityTest: before #690 (cached query primary) 4s #690 as merged (probe, then cached) 52s ← 13× this change (true race) 4s full suite: 7m50s -> 3m13s (CI cap 6m) WHY IT GOT SLOWER. #690 chained the direct probe ahead of the synced query with Concat. Concat is SEQUENTIAL: the synced query is not even subscribed until the probe COMPLETES. So every compile paid the probe's full latency up front — and the probe cannot answer faster than SourceProbeQuietWindow, because each leg has to watch its chunked Initial go quiet before it may emit. That is a ~1s floor per compile that the previous code never paid: the probe was DelaySubscription'd and on a healthy mesh never ran at all. That floor lands on far more calls than "compiles". The disk cache is keyed by effectiveLastModified, which comes from DiscoverSourceMaxLastModified -> SnapshotSources — so source discovery runs BEFORE the cache can be consulted, and a cache HIT pays it too. In a suite that touches the same NodeTypes repeatedly the cache-hit path is the common case, which is why a 1s floor cost minutes. Merge starts both sources NOW and FirstAsync takes whichever answers first, so: * healthy synced query -> answers immediately, probe latency never on the path * stalled synced query -> probe answers in a read + quiet window, not 45s #690's production win is kept; neither source is privileged, so the fast path does not depend on winning a race (#690's stated objection to #682). Merging is safe precisely because of #690's own `.Where(count > 0)`: a probe that finds nothing stays SILENT and completes, so it can never beat the cached query with an empty set and compile against no sources. That filter is also the likelier reason #682's probe "never fired" in production — not the 3s delay alone. NOT done: shrinking SourceProbeQuietWindow to 50ms, which also takes 52s -> 7s. That window exists so a chunked Initial can settle; the code's own comment warns a partial source set "compiles WRONG (missing files -> phantom errors)". Nor raising the 6m cap — the cap did not change, the compile path got 13x slower. Verified: Hosting.Monolith.Test 419/421 in 3m13s (2 skipped, 0 failed), including CompileSourceSnapshotWedgeTest which covers the stalled-snapshot path #690 exists to fix; Graph builds clean under -c Release -warnaserror. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 8, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a red
main. It has been red sincebbd2e960a(#702) — fiveMeshWeaver.Query.Testcases, across four merges — which blocks the portal self-update (no image rolls forward while main is red).What's wrong
#702 added a structural guard refusing an
AccessAssignmentwhoseMainNodedoesn't match the scope its path encodes. The invariant is right; its position was not.The guard runs at the top of
CreateNode(MeshExtensions.cs:324), but the satelliteMainNodenormalisation it implicitly depends on runs ~80 lines later, inside theexistingObs.Select(...).MeshNode.MainNodedefaults toPath(MeshNode.cs:95), so a satellite created the ordinary way —— reached the guard carrying
MainNode == "{p}/_Access/a1"while its path encoded scope"{p}". Mismatch → refused →CreateNodecompleted without emitting, and the test's.Should().Emit()failed with "completed without one".So the guard was rejecting the framework's own documented default shape. The clearest evidence is the name of one of the tests it broke:
SatelliteTypes_HaveMainNodeSetToOwningNode— it asserts precisely the auto-derivation the guard was pre-empting.The fix
Normalise first, then guard, on both write paths, via a shared
NormalizeSatelliteMainNodehelper.UpsertNodehad no normalisation at all, so it gets the helper too. That closes a real asymmetry the guard's own comment already demands ("Both write paths, or neither"): an upserted satellite pointedMainNodeat itself — the exact shape block 1b documents as misread by every consumer (SatelliteAccessRuledelegates permissions to it,rebuild_user_effective_permissionsprojects the grant atCOALESCE(main_node, namespace), satellite scope filters match it as the attachment point).The security property is untouched
Normalisation only fires when
MainNode == Path— i.e. never explicitly set. So the memex incident shape (an emptyMainNodeunder a non-empty path scope; 43 root-scoped accounts) still fails, as does any explicit mismatch. #702'sAccessAssignmentGuardTestpasses 29/29 unmodified, which is the check that matters here.Why not just fix the fixtures
Setting
MainNodeexplicitly on the five test fixtures would have gone green while leaving every production caller that relies on the auto-derivation refused at create — green CI over a live defect.Verification
MeshWeaver.Query.Test— 10/10, including the 5 previously-failing cases, unmodifiedMeshWeaver.Graph.TestAccessAssignmentGuardTest— 29/29 (the security invariant)MeshWeaver.Mesh.Contractbuilds clean under-c Release -warnaserrorNo What's New entry: #702 never shipped (main went red, so no image was produced), making this a fix-forward on an unreleased change with no user-visible delta.
🤖 Generated with Claude Code