feat: show build servers to the team in the navigator (Stage 5) - #219
feat: show build servers to the team in the navigator (Stage 5)#219TheMeinerLP wants to merge 7 commits into
Conversation
… see A navigator with fixed slots cannot hide anything. If the build servers owned slots of their own, a player without titan.navigator.buildserver would find those slots empty every single time, and a slot that is only ever empty for some players is exactly the information US-5.02 and NFR-005 want kept back. So no slot belongs to an entry. NavigatorEntry carries the permission it needs - null means public - and NavigatorLayout filters first and places afterwards: the visible entries become one uninterrupted, centred block, computed from the visible entries alone. A filtered-out entry never existed as far as the layout is concerned, which gives the property the tests assert: the menu of a player without the permission is identical to the menu of a lobby that has no build servers at all. Nothing to count, no hole to notice. Entries that exceed the row are dropped from the end rather than spilling into a second row, so the menu keeps the size every player sees.
US-5.03: the check that decides what is drawn is not a permission. A click arrives as a packet - it can name a slot the menu never had, it can arrive long after the menu was built, and the permission can have been withdrawn in between. GuardedDeliver therefore decides once more when the switch is requested, against the permission the player holds at that moment, and it sits in the Deliver chain rather than in the navigator because every path to another server goes through Deliver, including the ones that never involved a menu. Everything that is not a build server passes through untouched. BuildServerAccess answers what a build server is, and does so by name rather than by the list of servers that happen to be online: a destination is a build server because of the task it belongs to. Membership tied to the reachable list would let a request naming a stopped build server slip past the guard as an ordinary destination. CloudNet names a service <task>-<id>, so the task is recoverable from the service name with JDK types only. The task defaults to Build and can be overridden per deployment via titan.buildserver.task. BuildServerDirectory plus TitanBuildServerDirectory are the seam for the reachable list (US-5.04). CloudNet lives behind a classloader boundary, so the implementation is installed by the bridge extension and only a List<String> crosses over - the same rule ServerConnector and TitanPermissionBridge already follow. Until one is installed the directory reports nothing, which hides the build servers rather than guessing.
The navigator no longer writes items into hardcoded slots. It builds the list of entries for the player it is drawing for - the four game modes always, the reachable build servers only for a holder of titan.navigator.buildserver - and asks NavigatorLayout where they go (US-5.01, US-5.02). Both the permission and the list of reachable build servers are read while the menu is being drawn rather than when the helper is created, so a menu opened again reflects a stopped server or a withdrawn permission (US-5.04). A player without the permission is not a reason to ask CloudNet anything: their menu must not depend on the answer, so the lookup is skipped entirely for them. Titan wraps whatever DeliverProvider returned in GuardedDeliver, which makes the second permission check unavoidable for every switch the lobby performs, and hands the navigator the same audience and the TitanBuildServerDirectory holder. The build server icon names the service as plain text rather than through MiniMessage, so a service whose name contains tag-like characters cannot inject formatting into the menu. The tests open the real menu and read what the player sees. The unprivileged menu is asserted to be byte-identical to the one of a lobby with no build servers, slot by slot - absence, not a reserved gap.
US-5.04 asks for the servers that are reachable at the moment the menu opens, and CloudNet is the only source of that. :app deliberately does not depend on CloudNet, so the lookup lives here, where the driver is visible, and is installed into the TitanBuildServerDirectory holder; the application receives nothing but a List<String> of service names. A service counts as reachable when it is RUNNING and connected to its node - a started process the node has not seen connect is not somewhere to send a player. Anything that cannot be answered, a missing driver or a failed lookup, is reported as "no build servers" rather than as a stale list, because the navigator promises reachability.
US-5.01 to US-5.04 are in place, and the acceptance criterion they satisfy - a player without titan.navigator.buildserver neither sees the build servers nor reaches them through a tampered click - is ticked.
e3a54fa to
052256f
Compare
The navigator re-derived build-server membership from the service name after the bridge had already asked CloudNet for the services of the build task. That re-derivation assumed the name separator is always "-", but CloudNet configures it per task (ServiceTask/ServiceId.nameSplitter, default "-"): a Build task set to "_" produces Build_1, every entry was filtered away again, and a team member holding the permission saw an empty list with no explanation. The list now comes from servicesByTask unchanged, because that query is the authority on which services belong to the task. The name-based rule stays where it is needed and cannot be replaced: the guard has to recognise a build server that has stopped and is in no service list at all, otherwise a request naming it would look like an ordinary destination. That rule now follows CloudNet's own naming instead of guessing at it. A service is named <task><splitter><number>, so the splitter is configuration here as well (-Dtitan.buildserver.namesplitter) and the numeric tail is required - which also retires the latent false positive where a separate task named Build-Test would have had its services claimed by the Build rule. The bridge reads the task's real splitter once and warns when it disagrees with the configured one, so the drift cannot go unnoticed the way it did here. Also resolves driver services through InjectionLayer.ext() rather than boot(): ext is the layer CloudNet documents for external components such as extensions, and it is a child of boot, so every boot binding remains visible through it.
NavigationHelper bound a SimpleFeatureUser to the tick thread while the layout was computed and released it afterwards, without try/finally - any throw from FeatureGate or LuckPerms in between leaked the binding onto a thread that lives for the whole server. Wrapping it properly would have preserved nothing: FeatureGate.decide reads the FeatureState directly and takes the player id as a parameter, so it never asks Togglz's UserProvider anything. The binding is left over from the pre-gate implementation and is deleted rather than repaired.
Review findings fixed — 139 tests greenThe name-matching problem was one question doing two jobs
The menu list — CloudNet decides, and its answer stands. The guard stays name-based, so the stopped-server property is untouched: a request naming The name rule now follows CloudNet instead of guessing. Verified in the driver source: The splitter is deliberately not inferred from the name: The ThreadLocal was deleted, not wrappedWrapping in Injection layer: switched to
|
1 participant
Stacked on #216. Implements spec Stage 5 — US-5.01 … US-5.04 and NFR-005. 126 tests green.
The requirement that shaped the design
NFR-005 does not just say "hide the entries" — it says the absence must not reveal that something is hidden. A fixed layout cannot satisfy that, because a slot that is only ever empty for unprivileged players is the leak.
So the build servers were not given slots.
NavigatorLayout.plan(...)filters by permission first and derives positions from the survivors alone: one contiguous, centred block. Everything else is the same grey filler every player sees, the row is re-blanked on each draw, and surplus entries are truncated rather than growing the inventory — so the unprivileged view never varies with how many build servers exist.testUnprivilegedMenuHasNoEmptySlotasserts the strong form: the unprivileged menu is slot-by-slot identical to a lobby that has no build servers at all.Where the second check sits
GuardedDeliverwraps whateverDeliverProviderreturns, so the permission is re-checked in theDeliverchain rather than in the menu — every path to another server goes throughDeliver, including ones that never involved a menu. A click is a packet, not proof that the menu offered the destination.One design point:
BuildServerAccess.covers(...)decides "is this a build server" from the task name, not from the reachable list. Tying it to the live list would mean a request naming a stopped build server looks like an ordinary destination and slips past the guard. There is a test for exactly that.CloudNet boundary held
The service lookup lives in
:bridge(CloudServiceProvider→servicesByTask, filtered toRUNNING && connected()); only aList<String>crosses into the application. Same pattern asTitanServerConnector/TitanPermissionBridge.:appgained no CloudNet dependency. A missing driver reports "no build servers" rather than a stale list.Merged with Stage 3's gate
Both branches had rewritten
NavigationHelper. Resolved so that Stage 5's layout wins and Stage 3's gate becomes a second filter:PUBLIC_ENTRIESis now a list ofGatedEntry(TitanFeatures, NavigatorEntry), andentriesFor(...)keeps only entries the gate admits before the build-server permission narrows the list further. Letting the fixed slots win would have destroyed the anti-leak property above.The tests were checked for bite, not just for green
Mutation check: with
entriesForignoring the gate, 4 of 47:apptests fail — kill switch, internal stage hidden,litegroup, and flag-change-on-reopen. The two gate tests that survive assert an entry is shown, which a permanently-open gate cannot break; that is the expected shape.Merging the two suites also exposed a trap worth recording: the old "hidden" assertion compared a slot against the filler pane. Translated naively that becomes "filler is present" — true in every menu ever rendered. The rewritten helper excludes filler and air from the set, so the hidden case can only pass if the entry is genuinely gone, and a forgotten
env.tick()makes the positive assertions fail loudly instead of the negative ones passing vacuously.One caveat deliberately kept
Stage 3's kill switch is evaluated when the menu is drawn, so a player holding the navigator open sees the stale item until the next open.
GuardedDeliverre-checks the entry's permission, not the feature gate — verified against the code rather than assumed. The caveat therefore still stands and is recorded in the spec rather than quietly dropped when this branch ticked its own box.