[AI-2156] kcap setup creates the flow and polls it to completion - #640
Open
George-Payne wants to merge 1 commit into
Open
[AI-2156] kcap setup creates the flow and polls it to completion#640George-Payne wants to merge 1 commit into
George-Payne wants to merge 1 commit into
Conversation
The server's two rendezvous routes shipped with no caller at all, so nothing
generated a flow id and the browser's claim-on-arrival was what established
ownership - which is where it sat under the retired pairing, and the one
property of the design the server half could not realise alone.
The leg runs after login, since both routes are authenticated. It generates a
128-bit base64url id, creates the flow, opens {server}/setup?s=<id>, and polls
until every step it knows has settled.
- Refusals are handled apart: 404/401/403/405 on the create mean the tenant
does not serve the flow, and say nothing; 429 reports the server's own
Retry-After rather than sleeping through it; 409 retries with a fresh id,
since it means the id is taken rather than the credentials wrong.
- The poll's decision is extracted and unit-tested per branch. 410 is a dead
link, 404 a flow that will never be ours, 401 a re-login rather than a new
link, and 5xx or a transport blip is another tick.
- Outcomes, never instructions. Step and status strings map onto closed local
sets and an unrecognised member is dropped, because kcap setup writes Claude
Code hooks and a hook entry is a command string Claude Code runs. Which steps
are gates stays the server's to say, via can_finish.
- The setup URL is composed locally, so unlike the pairing there is no
server-supplied URL reaching a shell-executed open to validate.
- Any key ends the wait. The 30-minute budget is the backstop for a terminal
nobody is sitting at; a closed tab should not cost half an hour of dots.
- Headless is deliberately not a skip - the link is printed as well as opened,
which is what keeps the screens available to the device-path population.
The leg reports and configures nothing: the screens that would push
configuration are their own tickets, and the terminal steps remain what wires
the machine up.
PR Summary by QodoCreate and poll browser first-run setup flow during
AI Description
Diagram
High-Level Assessment
Files changed (18)
|
Code Review by Qodo
1. Cancellation swallowed in client
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+8
to
+11
| /// <para><b>Create-then-redirect, and that order is the whole point.</b> The browser then arrives at a | ||
| /// flow that already has an owner, so the server's ownership check has something to check from the | ||
| /// first request rather than from whenever a browser happens to turn up. Reversed, the first browser | ||
| /// to open the link owns the flow — which is where it sat under the retired pairing, and is the one |
There was a problem hiding this comment.
1. Verbose docblocks in firstrun 📘 Rule violation ⚙ Maintainability
Several newly-added comment blocks are overly long and include historical narrative (e.g., retired pairing/spec references) that reduces readability and exceeds the “minimal, non-obvious rationale” standard. This increases maintenance cost by burying the intent in multi-paragraph prose instead of concise constraints.
Agent Prompt
## Issue description
Newly added comments are overly verbose and include historical/narrative detail rather than minimal, non-obvious rationale.
## Issue Context
Rule requires comments to be short and focused on important rationale/constraints, not long narration.
## Fix Focus Areas
- src/Capacitor.Cli.Core/FirstRun/BrowserFirstRunFlow.cs[5-18]
- src/Capacitor.Cli/Commands/SetupCommand.cs[920-933]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+88
to
+89
| static bool IsTransient(Exception e) => | ||
| e is HttpRequestException or OperationCanceledException or JsonException or NotSupportedException; |
There was a problem hiding this comment.
2. Cancellation swallowed in client 🐞 Bug ☼ Reliability
FirstRunFlowClient treats OperationCanceledException as a transient transport blip and converts it to StatusCode=0, which prevents CancellationToken cancellation (e.g., Ctrl+C / host shutdown) from aborting browser setup polling. This can leave setup stuck polling until the 30-minute PollBudget instead of stopping promptly on cancellation.
Agent Prompt
## Issue description
`FirstRunFlowClient` currently classifies `OperationCanceledException` as transient and degrades it to `StatusCode = 0`. This swallows legitimate caller cancellation (`ct.IsCancellationRequested == true`), which means higher-level cancellation cannot stop the browser setup flow promptly.
## Issue Context
Elsewhere in the codebase, cancellation is explicitly preserved (either rethrown or only treated as transient when the *caller token* is not canceled).
## Fix Focus Areas
- src/Capacitor.Cli.Core/FirstRun/FirstRunFlowClient.cs[39-74]
- src/Capacitor.Cli.Core/FirstRun/FirstRunFlowClient.cs[76-90]
### Implementation notes
- Update `CreateAsync` / `PollAsync` / `ReadAsync` to **rethrow** `OperationCanceledException` when `ct.IsCancellationRequested` is true.
- Only degrade `OperationCanceledException` to status 0 when it represents a timeout or other non-caller cancellation (i.e., `!ct.IsCancellationRequested`).
- One simple pattern:
- `catch (OperationCanceledException) when (!ct.IsCancellationRequested) { return new(0, null); }`
- `catch (HttpRequestException) { ... }` etc.
- Remove `OperationCanceledException` from the generic `IsTransient(Exception e)` helper, or replace the helper with overloads that can examine `ct`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+52
to
+55
| if (!resp.IsSuccessStatusCode) | ||
| return new((int)resp.StatusCode, null, resp.Headers.RetryAfter?.Delta); | ||
|
|
||
| return new((int)resp.StatusCode, await ReadAsync(resp, ct)); |
There was a problem hiding this comment.
3. Retry-after date ignored 🐞 Bug ≡ Correctness
FirstRunFlowClient only reads Retry-After as a delta, so servers that send Retry-After as an HTTP date will be treated as having no Retry-After and will fall back to the hardcoded 10-minute default. This can misreport when browser setup will be available again.
Agent Prompt
## Issue description
`FirstRunFlowClient.CreateAsync` only uses `resp.Headers.RetryAfter?.Delta`. If the server uses the date form (`RetryAfter.Date`), the value is ignored and the caller will fall back to a default (10 minutes), producing misleading messaging.
## Issue Context
There is already repo precedent for correctly parsing both delta and date forms.
## Fix Focus Areas
- src/Capacitor.Cli.Core/FirstRun/FirstRunFlowClient.cs[39-58]
### Implementation notes
- When building `FirstRunCreateOutcome` for non-success responses, compute RetryAfter roughly like:
- `var retryAfter = resp.Headers.RetryAfter?.Delta;
if (retryAfter is null && resp.Headers.RetryAfter?.Date is { } date) {
var v = date - DateTimeOffset.UtcNow;
retryAfter = v > TimeSpan.Zero ? v : null;
}`
- Keep `RetryAfter` null when absent/unparseable so the higher layer’s fallback remains intact.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gives the server's two rendezvous routes a caller. They shipped with none, so nothing generated a flow id and the browser's claim-on-arrival was what established ownership - which is where it sat under the retired pairing, and is the one property of the design the server half could not realise alone.
The leg runs after login, since both routes are authenticated. It generates a 128-bit base64url id, creates the flow, opens
{server}/setup?s=<id>, and polls until every step it knows has settled.FirstRunFlowId- 16 CSPRNG bytes as base64url, 22 characters. The server's floor is what makes that the only shape that fits; it can check length and alphabet but never entropy, so the guarantee is the generator's alone.FirstRunFlowClient- the two routes, degrading rather than throwing. Refusals are handled apart: 404/401/403/405 on the create mean the tenant does not serve the flow and say nothing to the user; 429 reports the server's ownRetry-Afterrather than sleeping through ten minutes of it; 409 retries with a fresh id, since it means the id is taken rather than the credentials wrong.FirstRunFlowPoll- the poll's decision, extracted so every branch is tested without a socket. 410 is a dead link, 404 a flow that will never be ours, 401 a re-login rather than a new link, and 5xx or a transport blip is another tick.FirstRunFlowOutcomes- outcomes, never instructions. Step and status strings map onto closed local sets and an unrecognised member is dropped, becausekcap setupwrites Claude Code hooks and a hook entry is a command string Claude Code runs. Which steps are gates stays the server's to say, throughcan_finish, rather than being restated here where an old CLI could get it wrong.BrowserFirstRunFlow- create, then open, then poll. The setup URL is composed locally, so unlike the pairing there is no server-supplied URL reaching a shell-executed open to validate.SetupCommand- an unnumbered leg after login. Skipped on--no-promptand theNoneprovider. Headless deliberately is not a skip: the link is printed as well as opened, which is what keeps the screens available to the device-path population rather than designing it out of them.Two things worth flagging for review. 401/403 on the create are read as "no flow here" even though the route is authenticated - a gateway answering them on a path it does not know is indistinguishable from the feature being off, and a login succeeded seconds earlier, so guessing wrong here silently skips an additive leg while guessing the other way prints an alarming auth failure on every tenant that has the flow off. And the leg reports, configuring nothing: the screens that would push configuration are their own tickets, so the terminal steps remain what wires the machine up, and which of the two renders a given step is a decision that belongs to neither.
Unblocks two things the server half deferred: refusing a flow no CLI created (and with it metering the claim path, which is unlimited today), and
sbeing single-consumer.Capacitor.Cli.Core.Tests.UnitandCapacitor.Cli.Tests.Unitare green apart fromWriteAndBootstrap_writes_the_unit_and_bootstraps_without_a_leading_bootout, which fails identically on an unmodified tree - it refuses a group-writable temp directory, which is a devcontainer artefact rather than anything here. AOT publish is clean of IL2026/IL3050.AI-2156