From 414050935ea8fbd18f2e6e60d51d1e13f0a3fdac Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:48:02 -0500 Subject: [PATCH] fix(galaxyrepository): scoped browse surfaces ancestors as navigation-only nodes (scadaproj#42) A browse-subtree scope such as Plant/LineA/* filtered out Plant and Plant/LineA, so scoped lazy browse from the root was empty and DiscoverHierarchy returned orphans. Out-of-scope ancestors of an allowed object are now returned as navigation-only containers: attributes stripped, child_has_children = true, and only their children on the path to an allowed object are listed (siblings stay hidden). DiscoverHierarchy reports them in GalaxyHierarchyQueryResult.NavigationAncestorCount; WatchDeployEvents subtracts it so its scoped counts stay allowed-objects-only. - tests: ScopedAncestorBrowseTests; deploy-event scope test updated - IGalaxyBrowseScopeProvider documents the semantics; CLAUDE.md 0.2.1 notes - code-reviews: GalaxyRepository-002 Resolved; README regenerated Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- ZB.MOM.WW.GalaxyRepository/CLAUDE.md | 6 + .../GalaxyBrowseProjector.cs | 23 ++- .../GalaxyHierarchyProjector.cs | 84 +++++++++-- .../GalaxyHierarchyQueryResult.cs | 10 +- .../Grpc/GalaxyRepositoryGrpcService.cs | 4 +- .../Grpc/IGalaxyBrowseScopeProvider.cs | 17 +++ .../GalaxyRepositoryGrpcServiceScopeTests.cs | 13 +- .../ScopedAncestorBrowseTests.cs | 131 ++++++++++++++++++ code-reviews/GalaxyRepository/findings.md | 6 +- code-reviews/README.md | 11 +- 10 files changed, 275 insertions(+), 30 deletions(-) create mode 100644 ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ScopedAncestorBrowseTests.cs diff --git a/ZB.MOM.WW.GalaxyRepository/CLAUDE.md b/ZB.MOM.WW.GalaxyRepository/CLAUDE.md index 6a03cd8..ecaf6c3 100644 --- a/ZB.MOM.WW.GalaxyRepository/CLAUDE.md +++ b/ZB.MOM.WW.GalaxyRepository/CLAUDE.md @@ -7,6 +7,12 @@ Shared Galaxy object-hierarchy SQL browse + `galaxy_repository.v1` gRPC service Version bumped in `Directory.Build.props`, not packed or pushed; consumers stay on `0.2.0`. - GalaxyRepository-001 (scadaproj#41): the per-cache-entry filter memos of both projectors are now `ProjectionMemo`, capped at 64 signatures (FIFO; an evicted signature is recomputed, same answer). +- GalaxyRepository-002 (scadaproj#42): a browse-subtree scope (`Plant/LineA/*`) no longer hides the + allowed subtree's ancestors. `BrowseChildren` and `DiscoverHierarchy` return them as + **navigation-only** nodes — attributes stripped, only the path to an allowed object listed, siblings + still hidden. **Behaviour change for consumers:** a scoped `DiscoverHierarchy` now includes those + ancestors (`GalaxyHierarchyQueryResult.NavigationAncestorCount` says how many); `WatchDeployEvents` + still counts allowed objects only. Semantics documented on `IGalaxyBrowseScopeProvider`. ## Index history (migrated from the scadaproj root CLAUDE.md) diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs index 1ec3298..9560113 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs @@ -60,7 +60,9 @@ public static class GalaxyBrowseProjector List hasChildren = new(Math.Max(0, end - offset)); for (int index = offset; index < end; index++) { - page.Add(CloneObject(filtered.Children[index].Object, includeAttributes)); + // A navigation-only ancestor of the browse scope never carries attributes: it is in + // the reply only so the caller can reach the allowed subtree (GalaxyRepository-002). + page.Add(CloneObject(filtered.Children[index].Object, includeAttributes && !filtered.NavigationOnly[index])); hasChildren.Add(filtered.HasMatchingDescendant[index]); } @@ -134,10 +136,22 @@ public static class GalaxyBrowseProjector List matched = []; List hasMatching = []; + List navigationOnly = []; foreach (GalaxyObjectView view in directChildren) { if (!MatchesBrowseSubtrees(view, state.BrowseSubtreeGlobs)) { + // Out of scope itself, but an ancestor of an allowed object (scope + // `Plant/LineA/*` → `Plant`, `Plant/LineA`): surface it as a navigation- + // only container, or scoped lazy browse from the root is empty + // (GalaxyRepository-002). Its other children stay hidden because the same + // rule applies one level down when the caller expands it. + if (HasMatchingDescendant(view, state.Entry.Index, state.Request, state.BrowseSubtreeGlobs)) + { + matched.Add(view); + hasMatching.Add(true); + navigationOnly.Add(true); + } continue; } if (!MatchesFilters(view.Object, state.Request)) @@ -150,14 +164,16 @@ public static class GalaxyBrowseProjector { matched.Add(view); hasMatching.Add(true); + navigationOnly.Add(false); } continue; } matched.Add(view); hasMatching.Add(HasMatchingDescendant(view, state.Entry.Index, state.Request, state.BrowseSubtreeGlobs)); + navigationOnly.Add(false); } - return new FilteredChildren(matched, hasMatching); + return new FilteredChildren(matched, hasMatching, navigationOnly); }, (Entry: entry, ParentId: parentId, Request: request, BrowseSubtreeGlobs: browseSubtreeGlobs)); } @@ -282,5 +298,6 @@ public static class GalaxyBrowseProjector private sealed record FilteredChildren( IReadOnlyList Children, - IReadOnlyList HasMatchingDescendant); + IReadOnlyList HasMatchingDescendant, + IReadOnlyList NavigationOnly); } diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs index 67d6449..53ed6e8 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs @@ -26,7 +26,7 @@ public static class GalaxyHierarchyProjector /// signatures, because the signature is /// client-controlled (GalaxyRepository-001). /// - private static readonly ConditionalWeakTable>> FilteredViewCache = new(); + private static readonly ConditionalWeakTable> FilteredViewCache = new(); /// Number of filter signatures currently memoized for (tests). internal static int MemoCount(GalaxyHierarchyCacheEntry entry) => @@ -83,28 +83,33 @@ public static class GalaxyHierarchyProjector } string filterSignature = ComputeFilterSignature(request, browseSubtreeGlobs); - IReadOnlyList matchedViews = GetFilteredViews( + FilteredViews filtered = GetFilteredViews( entry, request, browseSubtreeGlobs, maxDepth, filterSignature); + IReadOnlyList matchedViews = filtered.Views; bool includeAttributes = IncludeAttributes(request); List page = new(Math.Min(pageSize, Math.Max(0, matchedViews.Count - offset))); int end = (int)Math.Min((long)offset + pageSize, matchedViews.Count); for (int index = offset; index < end; index++) { - page.Add(CloneObject(matchedViews[index].Object, includeAttributes)); + // Navigation-only scope ancestors never carry attributes (GalaxyRepository-002). + page.Add(CloneObject(matchedViews[index].Object, includeAttributes && !filtered.NavigationOnly[index])); } return new GalaxyHierarchyQueryResult( page, matchedViews.Count, - filterSignature); + filterSignature) + { + NavigationAncestorCount = filtered.NavigationAncestorCount, + }; } - private static IReadOnlyList GetFilteredViews( + private static FilteredViews GetFilteredViews( GalaxyHierarchyCacheEntry entry, DiscoverHierarchyRequest request, IReadOnlyList? browseSubtreeGlobs, @@ -116,29 +121,86 @@ public static class GalaxyHierarchyProjector IReadOnlyList views = entry.Index.ObjectViews; GalaxyObjectView? root = ResolveRoot(request, entry.Index); - ProjectionMemo> memo = - FilteredViewCache.GetValue(entry, static _ => new ProjectionMemo>()); + ProjectionMemo memo = + FilteredViewCache.GetValue(entry, static _ => new ProjectionMemo()); return memo.GetOrAdd( filterSignature, static (_, state) => { - List matched = []; + HashSet inScope = []; foreach (GalaxyObjectView view in state.Views) { if (MatchesRoot(view, state.Root, state.MaxDepth) && MatchesBrowseSubtrees(view, state.BrowseSubtreeGlobs) && MatchesFilters(view.Object, state.Request)) { - matched.Add(view); + inScope.Add(view.Object.GobjectId); } } - return matched; + // A scope such as `Plant/LineA/*` excludes `Plant` and `Plant/LineA` themselves, + // leaving the allowed objects with no parent in the reply (GalaxyRepository-002). + // Ancestors of an allowed object are returned as navigation-only nodes — + // attribute-stripped — when the request would otherwise have returned them + // (inside root/depth, passing the request filters). + HashSet ancestors = state.BrowseSubtreeGlobs is { Count: > 0 } + ? CollectAncestors(inScope, state.Index) + : []; + + List matched = []; + List navigationOnly = []; + int navigationCount = 0; + foreach (GalaxyObjectView view in state.Views) + { + int id = view.Object.GobjectId; + if (inScope.Contains(id)) + { + matched.Add(view); + navigationOnly.Add(false); + } + else if (ancestors.Contains(id) + && MatchesRoot(view, state.Root, state.MaxDepth) + && MatchesFilters(view.Object, state.Request)) + { + matched.Add(view); + navigationOnly.Add(true); + navigationCount++; + } + } + + return new FilteredViews(matched, navigationOnly, navigationCount); }, - (Views: views, Root: root, MaxDepth: maxDepth, BrowseSubtreeGlobs: browseSubtreeGlobs, Request: request)); + (Views: views, Index: entry.Index, Root: root, MaxDepth: maxDepth, BrowseSubtreeGlobs: browseSubtreeGlobs, Request: request)); } + /// + /// The out-of-scope ancestors of every in-scope object. Cycle-safe: a walk stops at any id + /// already seen, which also stops it re-walking a chain another object already covered. + /// + private static HashSet CollectAncestors(HashSet inScope, GalaxyHierarchyIndex index) + { + HashSet ancestors = []; + foreach (int id in inScope) + { + int parentId = index.ObjectViewsById.TryGetValue(id, out GalaxyObjectView? view) ? view.Object.ParentGobjectId : 0; + while (parentId != 0 + && !inScope.Contains(parentId) + && ancestors.Add(parentId) + && index.ObjectViewsById.TryGetValue(parentId, out GalaxyObjectView? parent)) + { + parentId = parent.Object.ParentGobjectId; + } + } + + return ancestors; + } + + private sealed record FilteredViews( + IReadOnlyList Views, + IReadOnlyList NavigationOnly, + int NavigationAncestorCount); + /// Finds an object in the hierarchy by its tag address. /// The Galaxy hierarchy cache entry. /// The tag address to search for. diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyQueryResult.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyQueryResult.cs index 1f2de7f..b8470e0 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyQueryResult.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyQueryResult.cs @@ -13,4 +13,12 @@ namespace ZB.MOM.WW.GalaxyRepository; public sealed record GalaxyHierarchyQueryResult( IReadOnlyList Objects, int TotalObjectCount, - string FilterSignature); + string FilterSignature) +{ + /// + /// How many of the objects are navigation-only ancestors + /// of a browse-subtree scope: returned (attribute-stripped) only so a client can attach the + /// allowed objects to a tree, not because they are themselves in scope. Always 0 unscoped. + /// + public int NavigationAncestorCount { get; init; } +} diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Grpc/GalaxyRepositoryGrpcService.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Grpc/GalaxyRepositoryGrpcService.cs index b705629..22829a8 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Grpc/GalaxyRepositoryGrpcService.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Grpc/GalaxyRepositoryGrpcService.cs @@ -246,7 +246,9 @@ public sealed class GalaxyRepositoryGrpcService( cache.Current, new DiscoverHierarchyRequest(), browseSubtrees); - objectCount = scoped.TotalObjectCount; + // Count the objects the scope allows, not the navigation-only ancestors that the + // projection adds so a tree can be built (GalaxyRepository-002). + objectCount = scoped.TotalObjectCount - scoped.NavigationAncestorCount; attributeCount = scoped.Objects.Sum(obj => obj.Attributes.Count); } diff --git a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Grpc/IGalaxyBrowseScopeProvider.cs b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Grpc/IGalaxyBrowseScopeProvider.cs index fab290c..080abc6 100644 --- a/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Grpc/IGalaxyBrowseScopeProvider.cs +++ b/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Grpc/IGalaxyBrowseScopeProvider.cs @@ -8,6 +8,23 @@ namespace ZB.MOM.WW.GalaxyRepository.Grpc; /// identity without the library knowing the host's authorization model. The default /// applies no scoping (full hierarchy). /// +/// +/// +/// Each glob is matched (anchored, case-insensitive, */?) against an +/// object's slash-separated contained path. An object whose path matches any glob is +/// allowed and is returned in full. +/// +/// +/// Ancestors of an allowed object are navigation-only. With Plant/LineA/*, +/// neither Plant nor Plant/LineA matches, but both are returned so a client +/// can reach the allowed subtree: by BrowseChildren (with +/// child_has_children = true) and by DiscoverHierarchy. They are returned +/// with their attributes stripped, and only their children on the path to an allowed +/// object are listed — siblings such as Plant/LineB stay hidden. An ancestor is +/// surfaced only when an allowed descendant also passes the request's own filters. +/// WatchDeployEvents counts allowed objects only. +/// +/// public interface IGalaxyBrowseScopeProvider { /// diff --git a/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/GalaxyRepositoryGrpcServiceScopeTests.cs b/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/GalaxyRepositoryGrpcServiceScopeTests.cs index 8d91f0b..e2b097a 100644 --- a/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/GalaxyRepositoryGrpcServiceScopeTests.cs +++ b/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/GalaxyRepositoryGrpcServiceScopeTests.cs @@ -80,14 +80,17 @@ public sealed class GalaxyRepositoryGrpcServiceScopeTests Assert.Equal(6, full.TotalObjectCount); Assert.Equal(4, full.Objects.Sum(obj => obj.Attributes.Count)); - // The glob selects only LineA's two leaf objects (Pump01, Valve01), each with one - // attribute. That scoped projection (2 objects / 2 attributes) is a non-empty subset - // distinct from both the full count and the raw notifier values below. + // The glob allows only LineA's two leaf objects (Pump01, Valve01), each with one + // attribute. The projection also returns their two ancestors (PlantArea, LineA) as + // attribute-stripped navigation nodes (GalaxyRepository-002); the deploy event must count + // only the 2 allowed objects / 2 attributes — a non-empty subset distinct from both the + // full count and the raw notifier values below. GalaxyHierarchyQueryResult scopedProjection = GalaxyHierarchyProjector.Project( entry, new DiscoverHierarchyRequest(), browseSubtreeGlobs: ["PlantArea/LineA/*"]); - Assert.Equal(2, scopedProjection.TotalObjectCount); + Assert.Equal(4, scopedProjection.TotalObjectCount); + Assert.Equal(2, scopedProjection.NavigationAncestorCount); Assert.Equal(2, scopedProjection.Objects.Sum(obj => obj.Attributes.Count)); // Publish a deploy event whose RAW counts differ from both full and scoped, so an @@ -114,7 +117,7 @@ public sealed class GalaxyRepositoryGrpcServiceScopeTests new TestServerCallContext()); DeployEvent emitted = Assert.Single(responseStream.Written); - Assert.Equal(scopedProjection.TotalObjectCount, emitted.ObjectCount); + Assert.Equal(2, emitted.ObjectCount); Assert.Equal(2, emitted.AttributeCount); // The raw notifier values were overridden by the scoped re-projection. Assert.NotEqual(999, emitted.ObjectCount); diff --git a/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ScopedAncestorBrowseTests.cs b/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ScopedAncestorBrowseTests.cs new file mode 100644 index 0000000..a797b60 --- /dev/null +++ b/ZB.MOM.WW.GalaxyRepository/tests/ZB.MOM.WW.GalaxyRepository.Tests/ScopedAncestorBrowseTests.cs @@ -0,0 +1,131 @@ +using ZB.MOM.WW.GalaxyRepository; +using ZB.MOM.WW.GalaxyRepository.Grpc; + +namespace ZB.MOM.WW.GalaxyRepository.Tests; + +/// +/// GalaxyRepository-002 (scadaproj#42): a browse-subtree scope such as Plant/LineA/* +/// hid the ancestors of the allowed subtree, so scoped lazy browse from the root was empty. +/// Ancestors must now surface as attribute-stripped navigation containers, while their +/// non-allowed children stay hidden. +/// +public sealed class ScopedAncestorBrowseTests +{ + private static readonly string[] Scope = ["Plant/LineA/*"]; + + [Fact] + public void Root_browse_surfaces_the_ancestor_as_a_navigation_container() + { + GalaxyBrowseChildrenResult result = Browse(new BrowseChildrenRequest()); + + GalaxyObject plant = Assert.Single(result.Children); + Assert.Equal("Plant", plant.TagName); + Assert.True(result.ChildHasChildren[0]); + Assert.Empty(plant.Attributes); + Assert.Equal(1, result.TotalChildCount); + } + + [Fact] + public void Ancestor_browse_shows_only_the_path_to_the_allowed_subtree() + { + GalaxyBrowseChildrenResult result = Browse(new BrowseChildrenRequest { ParentContainedPath = "Plant" }); + + GalaxyObject lineA = Assert.Single(result.Children); + Assert.Equal("Plant.LineA", lineA.TagName); + Assert.True(result.ChildHasChildren[0]); + Assert.Empty(lineA.Attributes); + } + + [Fact] + public void Allowed_subtree_is_returned_with_its_attributes() + { + GalaxyBrowseChildrenResult result = Browse(new BrowseChildrenRequest { ParentContainedPath = "Plant/LineA" }); + + GalaxyObject pump = Assert.Single(result.Children); + Assert.Equal("Plant.LineA.Pump01", pump.TagName); + Assert.Single(pump.Attributes); + } + + [Fact] + public void Non_allowed_sibling_subtree_does_not_leak() + { + GalaxyBrowseChildrenResult result = Browse(new BrowseChildrenRequest { ParentContainedPath = "Plant/LineB" }); + + Assert.Empty(result.Children); + Assert.Equal(0, result.TotalChildCount); + } + + [Fact] + public void Ancestor_is_not_surfaced_when_no_allowed_descendant_matches_the_request_filter() + { + GalaxyBrowseChildrenResult result = Browse(new BrowseChildrenRequest { TagNameGlob = "*Nothing*" }); + + Assert.Empty(result.Children); + } + + [Fact] + public void DiscoverHierarchy_includes_stripped_ancestors_and_no_other_objects() + { + GalaxyHierarchyQueryResult result = GalaxyHierarchyProjector.Project( + CreateEntry(), new DiscoverHierarchyRequest(), Scope); + + Assert.Equal( + new[] { "Plant", "Plant.LineA", "Plant.LineA.Pump01" }, + result.Objects.Select(o => o.TagName).Order(StringComparer.Ordinal)); + Assert.Empty(result.Objects.Single(o => o.TagName == "Plant").Attributes); + Assert.Empty(result.Objects.Single(o => o.TagName == "Plant.LineA").Attributes); + Assert.Single(result.Objects.Single(o => o.TagName == "Plant.LineA.Pump01").Attributes); + Assert.Equal(2, result.NavigationAncestorCount); + } + + [Fact] + public void Unscoped_DiscoverHierarchy_is_unchanged() + { + GalaxyHierarchyQueryResult result = GalaxyHierarchyProjector.Project(CreateEntry(), new DiscoverHierarchyRequest()); + + Assert.Equal(6, result.TotalObjectCount); + Assert.Equal(0, result.NavigationAncestorCount); + Assert.Single(result.Objects.Single(o => o.TagName == "Plant").Attributes); + } + + private static GalaxyBrowseChildrenResult Browse(BrowseChildrenRequest request) => + GalaxyBrowseProjector.ProjectChildren(CreateEntry(), request, Scope, 0, 100); + + private static GalaxyHierarchyCacheEntry CreateEntry() + { + GalaxyObject[] objects = + [ + Obj(1, 0, "Plant", "Plant", isArea: true), + Obj(2, 1, "LineA", "Plant.LineA", isArea: true), + Obj(3, 2, "Pump01", "Plant.LineA.Pump01"), + Obj(4, 1, "LineB", "Plant.LineB", isArea: true), + Obj(5, 4, "Pump02", "Plant.LineB.Pump02"), + Obj(6, 1, "Loose", "Plant.Loose"), + ]; + return GalaxyHierarchyCacheEntry.Empty with + { + Status = GalaxyCacheStatus.Healthy, + Sequence = 1, + LastSuccessAt = DateTimeOffset.UtcNow, + Objects = objects, + Index = GalaxyHierarchyIndex.Build(objects), + ObjectCount = objects.Length, + }; + } + + private static GalaxyObject Obj(int id, int parent, string name, string tag, bool isArea = false) + { + GalaxyObject obj = new() + { + GobjectId = id, + ParentGobjectId = parent, + IsArea = isArea, + ContainedName = name, + BrowseName = name, + TagName = tag, + }; + // Every object, ancestors included, carries an attribute so stripping is observable. + obj.Attributes.Add(new GalaxyAttribute { AttributeName = "Secret", FullTagReference = tag + ".Secret" }); + return obj; + } +} diff --git a/code-reviews/GalaxyRepository/findings.md b/code-reviews/GalaxyRepository/findings.md index 52ffe90..d0960cc 100644 --- a/code-reviews/GalaxyRepository/findings.md +++ b/code-reviews/GalaxyRepository/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 | | Reviewer | Claude | | Commit reviewed | `91fb2b4` | -| Open findings | 7 | +| Open findings | 6 | ## Summary @@ -117,7 +117,7 @@ Resolved 2026-11-09 in the commit tagged `(scadaproj#41)` (GalaxyRepository `0.2 | Severity | Medium | | Priority | P2 | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs:134` (also `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs:233`) | **Description** @@ -162,7 +162,7 @@ a deep scope glob. **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#42)` (GalaxyRepository `0.2.1`, unpublished) — an out-of-scope child that has an allowed descendant (also passing the request filters, via the existing `HasMatchingDescendant`) is now returned by `BrowseChildren` as a navigation-only container: `child_has_children = true`, attributes stripped; its other children stay hidden because the same rule applies when it is expanded. `DiscoverHierarchy` likewise returns the out-of-scope ancestors of allowed objects (cycle-safe parent walk; only when inside root/depth and passing the request filters), attribute-stripped, and reports them in the new `GalaxyHierarchyQueryResult.NavigationAncestorCount` init property, which `WatchDeployEvents` subtracts so its scoped counts remain allowed-objects-only. Semantics documented on `IGalaxyBrowseScopeProvider`. Regression tests in `ScopedAncestorBrowseTests` (root browse, ancestor browse, allowed subtree, sibling `LineB` does not leak, filter-gated ancestor, scoped and unscoped `DiscoverHierarchy`); root/ancestor/`DiscoverHierarchy` tests fail without the fix. `GalaxyRepositoryGrpcServiceScopeTests.WatchDeployEvents_ScopedProvider_EmitsFilteredCounts` updated (projection 4 objects incl. 2 ancestors; event still 2). Behaviour change for consumers: a scoped `DiscoverHierarchy` now includes the ancestor objects. ### GalaxyRepository-003 — Objects in a parent cycle disappear from `BrowseChildren` diff --git a/code-reviews/README.md b/code-reviews/README.md index 933a948..18a2112 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 45 pending findings across all libraries. +10 of 10 libraries reviewed. 44 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -50,9 +50,9 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 12 | +| Medium | 11 | | Low | 33 | -| **Total** | **45** | +| **Total** | **44** | ## Library Status @@ -61,7 +61,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | [Audit](Audit/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 | | [Auth](Auth/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/3 | 4 | 10 | | [Configuration](Configuration/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 4 | -| [GalaxyRepository](GalaxyRepository/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/6 | 7 | 8 | +| [GalaxyRepository](GalaxyRepository/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/6 | 6 | 8 | | [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/1 | 1 | 7 | | [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/4/3 | 7 | 7 | | [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/3 | 3 | 5 | @@ -84,12 +84,11 @@ _None open._ _None open._ -### Medium (12) +### Medium (11) | ID | Library | Title | |----|---------|-------| | Auth-007 | [Auth](Auth/findings.md) | User-bind mode never checks that the entry whose groups it returns is the account that bound | -| GalaxyRepository-002 | [GalaxyRepository](GalaxyRepository/findings.md) | Browse-subtree scoping hides the ancestors of an allowed subtree, so scoped lazy browse from the root is empty | | LocalDb-001 | [LocalDb](LocalDb/findings.md) | Sync endpoint is unauthenticated by default, and `ApiKey` is documented as "expected from the peer" | | LocalDb-002 | [LocalDb](LocalDb/findings.md) | Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good | | LocalDb-003 | [LocalDb](LocalDb/findings.md) | `INSERT OR REPLACE` apply silently deletes a different row on a secondary `UNIQUE` conflict, so the pair diverges |