Make scoped WebMCP registration cancellable - #7
Conversation
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. |
ffb6656 to
deba781
Compare
![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: ffb665639d
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
WebMcpRetryPolicy invariants and exposedTo origin validation have edge cases that can allow invalid configurations/origins through and should be tightened before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens WebMCP tool registration and execution by making registration lifecycle-aware (bounded retries, incremental scope synchronization, duplicate-name protection) and by adding reactive cancellation support that yields stable, safe agent-facing results.
Changes:
- Added bounded registration retry behavior with lifecycle wakeups and incremental tool synchronization in
WebMcpToolScope. - Introduced global duplicate tool-name coordination and aggregated registration-status reporting for UI/diagnostics.
- Added reactive cancellation (
context.cancelled,throwIfCancelled, stable cancelled result envelope) and expanded regression coverage.
File summaries
| File | Description |
|---|---|
| test/webmcp_tool_scope_test.dart | Adds coverage for incremental scope sync, retries, duplicate-name handling, status snapshots, and disposal races. |
| test/webmcp_test.dart | Adds tests for stricter exposedTo validation and execution cancellation semantics/envelopes. |
| test/webmcp_browser_test.dart | Adds browser coverage for modelContext late availability retries, lifecycle wakeups, and AbortSignal-driven cancellation behavior. |
| README.md | Documents incremental scope synchronization, retries/status reporting, cancellation, and tightened exposedTo rules. |
| lib/webmcp.dart | Exports the new registration status types as part of the public API. |
| lib/src/webmcp.dart | Adds supportChanges, retry-policy plumbing, duplicate-name protection, and stricter exposedTo validation. |
| lib/src/webmcp_tool.dart | Extends execution context with reactive cancellation (cancelled) and stable cancellation exception (throwIfCancelled). |
| lib/src/webmcp_result.dart | Adds WebMcpResult.cancelled() as the stable cancelled error envelope. |
| lib/src/webmcp_registration.dart | Introduces WebMcpRetryPolicy used by platform and widget registration retries. |
| lib/src/webmcp_registration_status.dart | Adds immutable registration status snapshots and error metadata for UI/diagnostics. |
| lib/src/webmcp_name_registry.dart | Adds a registry to coordinate unique tool names across imperative and scoped registrations. |
| lib/src/webmcp_exception.dart | Adds WebMcpDuplicateToolNameException and WebMcpCancellationException. |
| lib/src/webmcp_annotations.dart | Minor formatting change to the annotations constructor. |
| lib/src/platform/platform.dart | Extends the platform interface with supportChanges and retryPolicy support. |
| lib/src/platform/platform_web.dart | Implements lifecycle-aware supportChanges, modelContext availability retries, and AbortSignal-based reactive cancellation + cleanup. |
| lib/src/platform/platform_stub.dart | Implements supportChanges and retryPolicy args for unsupported platforms. |
| lib/src/flutter/webmcp_tool_scope.dart | Reworks scope synchronization to be incremental, retry-aware, status-emitting, and duplicate-name resilient; adds aggregated controller. |
| CHANGELOG.md | Documents the new lifecycle/retry/status/cancellation/validation behavior. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a996e2d32
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
The current WebMcpToolScope support-change reset logic can repeatedly re-report duplicate-name errors on every lifecycle event, creating noisy/undesired error churn.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 2
- Review effort level: Lite
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed correctness issues in the updated tool-scope sync logic (a null-safety compile error and an after-dispose error-reporting path) that must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
lib/src/platform/platform_web.dart:226
- If
addEventListener('abort', ...)throws,_BrowserCancellationreturns without re-checkingsignal.aborted. That creates a small race where the signal can become aborted after the initial check but before listener attachment, causingcontext.cancelledto never complete even thoughabortedis true. Re-check (and complete) before returning from the catch.
lib/src/flutter/webmcp_tool_scope.dart:456
previousis nullable (_errors[name]may be absent), but_recordErrorunconditionally callsprevious.runtimeType/previous.toString(), which is a null-safety compile error. Guardprevious == nullbefore comparing so the scope builds on sound null safety.
_emitStatus();
if (previous.runtimeType != error.runtimeType ||
previous.toString() != error.toString()) {
_reportError(error, stackTrace);
}
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5759e1fe6
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8a8bb2675
ℹ️ 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".
There was a problem hiding this comment.
🔵 Needs a closer look
Two correctness issues remain in the new lifecycle/status logic (aggregated support depends on update order, and zero-delay retries can spin without yielding), which should be addressed before approval.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
lib/src/flutter/webmcp_tool_scope.dart:59
WebMcpRegistrationStatusController._rebuild()overwritessupportwith the last-updated scope’s value (support = status.support). That makes the aggregatedsupportdepend on_sourcesiteration/update order, which can vary across rebuilds (especially when different scopes use differentsupportCheckimplementations). Consider making aggregation deterministic—e.g., keepsupportas the globalWebMcp.supportvalue (since platform support isn’t scope-specific), or explicitly combine statuses in a stable way.
lib/src/platform/platform_web.dart:137_waitForDelayOrLifecycle()returns immediately whendelay == Duration.zero, so_waitForModelContext()will run all remaining attempts in a tight loop without yielding back to the event loop. WithmaxAttempts > 1this can make retries effectively instantaneous and missdocument.modelContextappearing on the next tick. Consider yielding once (even for zero delay) to keep retries lifecycle-/task-aware and avoid a synchronous spin.
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8694a28926
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
The tool-name lease is released before async unregister() completes in WebMcpToolScope, which can allow duplicate-name registrations to overlap and defeats the intended duplicate-name protection.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
lib/src/flutter/webmcp_tool_scope.dart:559
- During
dispose(), each active tool’s name lease is released immediately after schedulingunregister()(which may complete later). This can transiently free the name while the old registration is still active, allowing a duplicate registration to start in another scope.
Release the name only after _unregisterObsolete completes.
final active = Map<String, _ActiveTool>.of(_active);
_active.clear();
for (final current in active.values) {
unawaited(_unregisterObsolete(current.registration));
_releaseName(current.nameLease);
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ef9cc6aee
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8462f7243b
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
The platform interface introduces a null-safety compile error (non-nullable optional named parameter without a default) that must be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
8462f72 to
09ea4c2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09ea4c292f
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
WebMcpRegistrationAttempt.cancel() can leave a previously returned WebMcpRegistration reporting an incorrect isRegistered state after cancellation, and the README wording may misstate when “unrelated tool names stay registered.”
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
09ea4c2 to
5b59d32
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b59d32826
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
WebMcpToolScope currently treats “unchanged configuration” as object-identity for WebMcpTool, which will cause unintended cancel/re-register churn on common rebuild patterns and conflicts with the documented behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
README.md:204
- The README claims that "slots whose configuration did not change stay registered", but
WebMcpToolScopecurrently decides “unchanged” using object identity for parts of the tool definition (notablyexecute). If the intent is value-based stability, align the docs to the actual comparison semantics (or tighten the code to compare tool fields instead of requiring the same instance).
`WebMcpToolScope` uses this form internally. When a tool changes, its old
attempt is aborted before the replacement starts; slots whose configuration
did not change stay registered.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
5b59d32 to
5e78832
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e78832b45
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
There are documentation and cleanup issues (README example wording mismatch and unused _ToolSlotState state) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
README.md:191
- The paragraph says this example is for cancelling while the browser is still registering, but the snippet awaits
attempt.readybefore callingcancel(), which describes unregistering after registration instead. Adjust the wording so it matches the example (or adjust the example to demonstrate cancelling beforereadycompletes).
To cancel while the browser is still registering the tool, start an attempt
and keep it for the whole lifecycle:
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
5e78832 to
ae23793
Compare
|
@codex review Please perform the final review of ae23793. The lifecycle remains canonical: a pending adapter must honor cancellation; the browser adapter does so with AbortController. Post-disposal errors remain observable, dead slot state is removed, and the docs show cancellation may precede readiness. |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
There was a problem hiding this comment.
🟡 Changes recommended
The new WebMcpRegistrationAttempt.cancel() can leak a successfully-completed registration when cancellation happens before ready is observed, which can leave tools registered after disposal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
ae23793 to
1db9b3a
Compare
Summary
This is a standalone WebMCP tool package. It has no Foldboard-specific code or integration.
Scope
This PR intentionally does not add retry/status APIs, a process-wide name registry, detached cleanup, or execution-lifecycle changes. The old async registrar override is replaced by a cancellable registration starter; backward compatibility is not required.
Verification