Repository navigation
Add hosted WebUI transport tests and proxy method controls - #7
Conversation
625862b to
bdaf0e6
Compare
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdaf0e6f69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const reported = Array.isArray(proxy?.backends) ? proxy.backends : []; | ||
| const backends = reported.map(backend => nodeIndex.get(backend.backendId)).filter(Boolean); |
There was a problem hiding this comment.
When the nodes endpoint sets backendItemsTruncated—for example, when a proxy reports more than the 256-per-node summary limit—proxy.backends contains only a prefix, but this code treats that prefix as the complete network. If every returned backend is capable, the method buttons become enabled and the apply targets only the proxy and that prefix, leaving omitted backends on the old method after the proxy switches. Fetch the complete topology or disable switching whenever backend summaries are truncated.
Useful? React with 👍 / 👎.
| const backends = reported.map(backend => nodeIndex.get(backend.backendId)).filter(Boolean); | ||
| const unavailable = reported.filter(backend => { | ||
| const node = nodeIndex.get(backend.backendId); | ||
| return !node || !node.online || !node.acceptedCapabilities.includes('config.proxy-method.v1'); |
There was a problem hiding this comment.
For networks spanning multiple 100-node pages, nodeIndex contains only the current page, so any backend reported by the selected proxy but listed on another page resolves to undefined and is marked unavailable. Paging cannot fix this because the proxy and all of its backends may never appear on the same page, making the new method controls permanently unusable despite every node being enrolled, online, and capable. Resolve reported backend IDs through an unpaginated or targeted lookup.
Useful? React with 👍 / 👎.
| transportTestBackend.replaceChildren(backendPlaceholder, ...backends.map(backend => { | ||
| const option = text(document.createElement('option'), `${backend.displayName} · ${backend.backendId}`); | ||
| option.value = backend.displayName; | ||
| return option; |
There was a problem hiding this comment.
When a backend's display name differs from its ID (which the protocol permits, and existing fixtures model as Lobby versus lobby), the diagnostic sends the presentation label in options.server rather than the unique configured backend identity. Display names are not required to be unique, so two entries can also produce the same request value and cannot be targeted independently. Store and send backend.backendId, using displayName only in the option label.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49ec9e9652
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const preview = await startConfigurationOperation('/api/v1/configuration/preview', { | ||
| nodeIds: network.nodeIds, | ||
| configuration: {domain: 'quick-setup', preset: 'proxy-method', options: {method}} |
There was a problem hiding this comment.
When the selected proxy reports 100 or more capable backends, network.nodeIds contains at least 101 entries, so this preview request is always rejected by ConfigurationOperations.validateTargets, which permits at most 100 node IDs. A complete topology can still reach this path without backendItemsTruncated when the proxy sorts onto a sparsely populated final registry page, leaving the UI showing the network as ready even though no method can be selected successfully; disable the control with an explicit capacity message or add supported batching/atomic coordination.
Useful? React with 👍 / 👎.
| if (preview.state !== 'SUCCEEDED' || !preview.approvalToken) return; | ||
| if (proxyMethodProxyId !== network.proxy.nodeId) { | ||
| text(proxyMethodStatus, 'The selected proxy changed while preflighting. Choose the method again.'); |
There was a problem hiding this comment.
If the proxy adds or removes a reported backend while the preview is running, this guard still passes because it compares only the proxy ID, and the subsequent apply uses the stale preview target set. The newly added backend can therefore remain on the old method after the proxy and earlier backends switch, potentially breaking transport; fresh evidence in the current code is that the post-preview guard does not refetch or compare backend IDs, truncation, or capabilities before approval.
Useful? React with 👍 / 👎.
💡 Codex ReviewVotingPlugin-Control/src/main/resources/web/app.js Lines 1328 to 1329 in fbbf0b7 Cap VoteSites targets before previewing
When more than 100 online backends advertise ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
💡 Codex ReviewVotingPlugin-Control/src/main/resources/web/app.js Lines 1434 to 1436 in 86a9abe Include proxy availability in the refreshed network check
If the selected proxy goes offline or loses Enforce the operation limit for VoteSites targets
When more than 100 sync-capable backends are connected, the initial target selection includes all of them and ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5ea6ab3b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| NodeStatus target = registry.find(id); | ||
| return target != null && "BUKKIT".equalsIgnoreCase(target.platform()); | ||
| }).toList(); | ||
| if (backends.stream().anyMatch(id -> !"COMPLETE".equals(operation.states.get(id)))) return true; |
There was a problem hiding this comment.
When one backend completes the apply and another goes offline before claiming its task, the latter remains QUEUED because capability loss is processed only when that backend polls. Fresh evidence in the current sequencing change is that this condition now defers the proxy solely until every backend state is COMPLETE; the operation therefore runs until its 15-minute expiry while the successful backend remains on the new method and the proxy remains on the old one. Detect unavailable queued backends centrally and cancel or roll back the partially applied switch.
Useful? React with 👍 / 👎.
| if (backends.stream().noneMatch(id -> operation.results.get(id) == null | ||
| || !operation.results.get(id).success())) return false; |
There was a problem hiding this comment.
When an authenticated API client previews proxy-method with only a proxy ID, the request passes target and capability validation, and this empty-backend noneMatch evaluates true, releasing the proxy apply immediately. The documented configuration API can therefore bypass the WebUI's topology checks and switch a proxy without any of its reported backends; validate that the target set exactly contains the proxy's complete reported backend network before issuing the approval.
Useful? React with 👍 / 👎.
|
@codex review |
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea07b02ad3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| List<String> backends = operation.states.keySet().stream().filter(id -> { | ||
| NodeStatus target = registry.find(id); | ||
| return target != null && "BUKKIT".equalsIgnoreCase(target.platform()); | ||
| }).toList(); |
There was a problem hiding this comment.
When an offline backend is removed by InMemoryNodeRegistry.reclaimOfflineTopology, this filter drops its still-queued operation target because registry.find(id) returns null; an empty or otherwise completed backends list then lets the proxy claim and apply the new method while the evicted backend remains unchanged and the operation remains permanently queued. Fresh evidence beyond the earlier offline-backend report is that the current fix derives dependencies from the mutable registry rather than the operation's validated target roles, so registry eviction bypasses the new cancellation path entirely.
Useful? React with 👍 / 👎.
| if ("COMPLETE".equals(operation.states.get(backendId))) continue; | ||
| NodeStatus backend = registry.find(backendId); | ||
| if (backend != null && backend.online() | ||
| && backend.acceptedCapabilities().contains(operation.configuration.capability())) continue; | ||
| audit.append("TASK_CANCELLED", operation.id, backendId, "CAPABILITY_LOST"); | ||
| operation.results.put(backendId, boundedResult(operation, new ConfigurationTaskResult(sessionId(backend), false, |
There was a problem hiding this comment.
When a backend has already claimed its apply but temporarily becomes online == false before its two-minute lease expires—particularly when the configurable offline timeout is shorter—this branch overwrites its IN_PROGRESS task with a synthetic failure and cancels the proxy. The backend still owns the task and can finish switching afterward, but completeCurrentSession ignores its result because the state is already COMPLETE, leaving that backend on the new method while the proxy stays on the old one; only unclaimed queued tasks should be cancelled this way.
Useful? React with 👍 / 👎.
# Conflicts: # src/main/resources/web/app.js
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e88d3cdf0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| String state = operation.states.get(backendId); | ||
| if ("COMPLETE".equals(state) || !"QUEUED".equals(state)) continue; |
There was a problem hiding this comment.
When a backend claims the apply and then remains offline beyond the two-minute lease, its state stays IN_PROGRESS, so this condition skips it forever rather than converting the expired task to a dependency failure. Each proxy poll then returns at line 184 because that backend is not complete, leaving any already-switched backends on the new method and the proxy on the old method until the entire operation is pruned after 15 minutes; preserve active leases, but cancel or retry them once they expire.
Useful? React with 👍 / 👎.
| if (backends.stream().anyMatch(id -> !"COMPLETE".equals(operation.states.get(id)))) return true; | ||
| if (backends.stream().noneMatch(id -> operation.results.get(id) == null | ||
| || !operation.results.get(id).success())) return false; |
There was a problem hiding this comment.
If the proxy adds a reported backend after createApply validates the topology but while the stored backend tasks are running, this success check considers only the original targetPlatforms dependencies. Once those original backends finish, the proxy is released and switches methods even though the newly reported backend received no task and may remain on the old transport; compare the proxy's current reported backend IDs with the stored dependency set immediately before returning false.
Useful? React with 👍 / 👎.
|
Follow-up validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d492ed6d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!"APPLY".equals(operation.type) | ||
| || !ManagedConfiguration.QUICK_SETUP.equals(operation.configuration.domain()) | ||
| || !ManagedConfiguration.PROXY_METHOD.equals(operation.configuration.preset()) | ||
| || "BUKKIT".equalsIgnoreCase(node.platform())) return false; |
There was a problem hiding this comment.
When a target is evicted and re-registers under the same node ID with a different platform during an apply, this check uses the replacement node's current role instead of the role retained in operation.targetPlatforms. A proxy target re-registered as BUKKIT therefore bypasses deferral and can claim its task before the validated backends finish, while a backend re-registered as a proxy can defer itself indefinitely; use the stored target role and reject incompatible replacements.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31899a364e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| StoredOperation apply = store("APPLY", targets, preview.configuration, | ||
| null, revisions, preview.id); |
There was a problem hiding this comment.
When another authenticated client starts a second proxy-method workflow after the first operation's backends finish but before its proxy claims, this creates another apply for the same network without detecting the pending one. The backends can apply the second method, after which the first operation still releases its proxy based solely on its earlier successful backend results, leaving the proxy on the first method and the backends on the second; revision checks then prevent the second proxy task from repairing the split. Fresh evidence beyond the earlier WebUI locking report is that proxyMethodWorkflowInFlight is client-local while this server-side apply path has no conflict gate for another active proxy-method operation.
Useful? React with 👍 / 👎.
|
Final follow-up validation:
AI-assisted implementation; changes and test coverage were reviewed before publication. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cdb722a2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .map(Map.Entry::getKey).toList(); | ||
| for (String backendId : backends) { | ||
| String state = operation.states.get(backendId); | ||
| if ("COMPLETE".equals(state)) continue; |
There was a problem hiding this comment.
When a backend completes successfully and then is evicted or re-registers under the same ID with a non-Bukkit role before the proxy claims, this early continue prevents its stored role from being compared with the current registry entry. The successful result and unchanged proxy backend-ID set then allow the proxy task to be released even though the replacement node never applied the method, splitting the network. Fresh evidence beyond the earlier role-drift case is that completed dependencies bypass cancelChangedProxyMethodRole; revalidate their current role before releasing the proxy.
Useful? React with 👍 / 👎.
|
Current-head follow-up:
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
UX and safety
Checks
Depends on #6. Paired VotingPlugin implementation: BenCodez/VotingPlugin#1589.