Repository navigation
Fall back to legacy registration on the real missing-method answer - #1935
Conversation
…1858) A JSON-protocol server cannot bind a method it lacks, so it answers "Method does not exist." rather than "Unknown hub method" Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedThe included review limit has been reached and this organization has disabled usage-based review continuation. Wait for reviews to reset or ask a billing admin to change After included review limits. Next included review available in 7 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Method-specific detection and invocation src/Capacitor.Cli.Daemon/Services/ServerConnection.cs |
The classifier recognizes the wrapped “Method does not exist” error and the “Unknown hub method” prefix for the invoked method. Registration and parking pass their method names to the classifier and use constants for raw invocations. |
Fallback behavior validation test/Capacitor.Cli.Daemon.Tests.Unit/Services/ServerConnectionAgentRegisteredTests.cs, test/Capacitor.Cli.Daemon.Tests.Unit/Services/ServerConnectionParkProxyTests.cs, test/Capacitor.Cli.Daemon.Tests.Unit/Services/ServerConnectionLegacyHubWireTests.cs |
Tests cover supported and rejected error cases, parking rejection, and successful registration against a SignalR hub with only the legacy method. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- kurrent-io/kcap-cli#1893: Updates the registration payload passed through the same acknowledged-to-legacy registration path.
- kurrent-io/kcap-cli#1870: Adds registration fields to the payload used by the same registration and fallback path.
- kurrent-io/kcap-cli#1874: Addresses missing SignalR hub methods in another daemon fallback path.
Suggested reviewers: alexeyzimarev, inok
Merge Risk: 
4e3d6
-
The change is ready to merge after normal checks; no unresolved fallback issue was established.
Security architecture risk: 
4e3d6
-
The fallbacks remain within the existing server connection, with method-specific matching and preserved handling of refusals and uncertain outcomes. No introduced security bypass was established. Server-side authorization and the provenance of matching error messages remain unverified.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — A registration capability decision can affect subsequent agent registrations on the same connection through the existing memo. The parking decision affects the specified reviewer's cleanup path. These consequences warrant lifecycle review even though no new transport or privilege boundary is introduced.
Trust Boundaries and Controls
- inferred — The classifier consumes server-provided exception type and text, not independently authenticated dispatcher provenance. A client-visible exception with identical accepted text would be indistinguishable at this boundary. The inspected evidence does not establish that an unauthorized caller can produce that response or bypass the legacy handler's authorization.
Resilience and Maintainability Implications
- observed — The negative test cases distinguish ordinary registration refusal, another or similarly named method, non-hub exceptions, and cancellation from missing-method responses. These cases support containment of fallback decisions but do not prove server-side error provenance.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Linked Issues check The implementation satisfies the main #1858behavior.IsUnknownHubMethodmatches both dispatcher responses only for the invoked method and only forHubException. Registration fallback memoization,…Add a focused automated test that starts an ordinary hosted session through the legacy-only SignalR hub and verifies that the session remains active after registration. Keep the existing exact-error, fallback, reconnect, refusal, null, and … Docstring Coverage Docstring coverage is 46.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the main change: fallback to legacy registration when the server returns the real missing-method response. Out of Scope Changes check All changes are connected to #1858. The classifier update, the two caller updates, unit cases, park regression cases, and the real legacy-hub wire test implement or validate the requested fallback and…Full details: Out of Scope Changes check
-
Explanation
All changes are connected to
#1858. The classifier update, the two caller updates, unit cases, park regression cases, and the real legacy-hub wire test implement or validate the requested fallback and shared-classifier behavior. No unrelated production behavior or unrelated test area was added.
-
Full details: Linked Issues check
-
Explanation
The implementation satisfies the main
#1858behavior.IsUnknownHubMethodmatches both dispatcher responses only for the invoked method and only forHubException. Registration fallback memoization, reconnect re-probing, refusals, null outcomes, and unrelated failures remain covered inServerConnectionAgentRegisteredTests.cs.ServerConnectionLegacyHubWireTests.csuses a real SignalR hub withoutAgentRegisteredAcked. The issue also requires verification that an ordinary hosted session survives registration. The added wire test verifies registration only and does not verify a hosted session.Resolution
Add a focused automated test that starts an ordinary hosted session through the legacy-only SignalR hub and verifies that the session remains active after registration. Keep the existing exact-error, fallback, reconnect, refusal, null, and unrelated-failure coverage.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
PR Summary by QodoRecognize legacy servers’ missing-method SignalR response
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closes #1858 — AI-4268
What & why
Against a server without
AgentRegisteredAcked, launches died after spawning. The legacy fallback only recognisedUnknown hub method, but a JSON-protocol server cannot bind the arguments of a method it lacks. Its dispatcher answersFailed to invoke 'AgentRegisteredAcked' due to an error on the server. HubException: Method does not exist.instead.IsUnknownHubMethodnow takes the invoked method's name and accepts either dispatcher answer only for that method, only as aHubException. A handler's refusal, another method's absence, an authorization failure, a cancellation or a non-hub exception carrying the same text never reads as a missing method. The per-connection memo, the re-probe after reconnect, explicit refusals and null outcomes are unchangedWhere to look
ReportParticipantParkedAsyncshares the classifier and had the same gap: a server withoutReportParticipantParkedanswered with the binding wording, which read as ambiguous and retried the park. Both answers now map toRejected, so the reviewer falls back to the normal reap as intendedVerification
ServerConnectionLegacyHubWireTests(real Kestrel SignalR hub with onlyAgentRegistered, detailed errors off), beforeHubExceptionAgentRegisteredand reads as acceptedServerConnectionAgentRegisteredTests+ServerConnectionParkProxyTests+ the wire testdotnet publish -c Release🤖 Generated with Claude Code
Summary by CodeRabbit