[AI-2156] kcap setup creates the flow and polls it to completion - #640
Merged
George-Payne merged 5 commits intoAug 24, 2026
Merged
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.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
… flow client Qodo review findings on the first-run browser leg: - caller cancellation was swallowed as a transport blip, so Ctrl-C could not stop the poll before its 30-minute budget; rethrow OCE when the caller token is cancelled and degrade only HttpClient's own timeout (same exception type, token unsignalled) - Retry-After was read as delta-seconds only; a proxy rewriting it as an HTTP date was reported as no header at all. Read both forms, measured against the response's own Date header so server clock skew cannot turn the wait negative - trim the retired-pairing narrative from the new docblocks down to the non-obvious constraints, per the comment rule
George-Payne
requested review from
alexeyzimarev,
nortonandreev,
realtonyyoung and
rsergeenko
August 24, 2026 09:22
realtonyyoung
left a comment
realtonyyoung
left a comment
Collaborator
There was a problem hiding this comment.
Peer review via kcap code-review flow (reviewer vendor: claude)
Two rounds, reviewer signed off at round 2. The 8 findings below are those still open at sign-off — nothing fixed or superseded in round 1 is repeated here. Each is posted as an inline thread so it can be resolved individually.
Verification scope — please read before acting. The reviewer's worktree was on the merge-base (a5d92c4d), not the PR branch, so the inlined diff was authoritative for the new files. That means:
- Verified against the real checkout: every collaborator the leg leans on —
HttpClientExtensions,TokenStore,SystemBrowser,IKeyWatcher, andSetupCommand's existing login/ping legs. Findings 1, 2, 3, 5 and 8 are anchored in those, and I independently re-confirmed the citations behind 1, 2 and 3. - Not verified: the six test files, the README hunk, and
help-setup.txt's rendered result. Test-coverage remarks are flagged as unverified rather than asserted. - Could not be settled: the server contract for
/api/first-run/flows(absent from both this repo andkcap-server's main). Finding 4 is a contract risk, not a confirmed defect — it needs a check against a real payload.
Ranking: 1-3 are the ones worth acting on before merge. 4 is a should-confirm. 5-8 are cheap fixes; 6 and 8 are explicitly not blockers.
Explicitly not findings, for the record: the create-before-open ordering, composing the setup URL locally rather than taking it from the server, the closed-set outcome mapping, and treating the payload as outcomes-never-instructions all look right. Base64Url.EncodeToString(RandomNumberGenerator.GetBytes(16)) is a correct 22-char/128-bit id. No AOT/trimming issue found: both new models are registered on CapacitorJsonContext (Models.cs:962-963), every serialize/deserialize goes through a JsonTypeInfo, and Base64Url/Enum.ToString/the LINQ All are all AOT-safe.
Of the author's two flagged-for-review decisions: the outcomes-never-instructions boundary holds up. The 401/403-on-create trade does not fully — see Finding 1 for why the premise weakens once the token is read raw.
| /// gate blocks finishing, and a flow whose import failed is over, not stuck.</para> | ||
| /// </summary> | ||
| public static bool IsFinished(FirstRunFlowResponse view) => | ||
| view.CanFinish && KnownSteps.All(step => IsSettled(view, step)); |
Collaborator
There was a problem hiding this comment.
Finding 4 — IsFinished requires the Done step to carry an outcome; wire-name mapping is exact-case.
(Flagged unverified: the server routes are in neither this checkout nor /Users/tony/dev/kcap-server's main, so this is a contract risk the reviewer could not settle. Please confirm against a real payload.)
Done reads like the terminal value of the step field ("The step the browser is on"), not a gate with its own outcome. If the server's steps dictionary only carries the three gates, StatusOf(view, Done) falls to Pending forever, IsFinished is never true, and a browser flow that completed in ten seconds still burns the full 30-minute budget before reporting Abandoned -> "The browser didn't finish setup." — the exact opposite of what happened.
Same silent-never-finishes failure from a second cause: Step()/Outcome() switch on exact PascalCase wire strings, and StatusOf looks up step.ToString() in a default (ordinal, case-sensitive) Dictionary. A server serializing its step/outcome enums camelCase ("signIn", "completed") or snake_case makes every outcome unrecognised -> Pending -> same 30-minute hang. The "unknown members are dropped" boundary is right, but it means a naming mismatch degrades to a half-hour wait rather than to anything diagnosable.
Fix: (a) confirm against the real payload that Done is always present in steps once can_finish is true, and (b) add at least one test that feeds a recorded server response through IsFinished rather than a hand-built FirstRunFlowResponse — a hand-built fixture agrees with the CLI's own casing by construction and cannot catch this.
Member
Author
There was a problem hiding this comment.
Verified against the server half (the paired kcap-server change): FirstRunFlowView.From always sends all four steps — FirstRunSteps.InOrder (SignIn, Agents, Import, Done) — each with an outcome, keyed by enum.ToString(), so the exact-case names (Done) match this mapping. Done does settle: the payoff screen writes the completed event, and the poll endpoint own comment describes a CLI poll landing "just after the browser settled Done" — the exact state this IsFinished exists to detect. The only forward-compat caveat is the deliberate, documented closed-set trade (an unknown new-server step is dropped rather than stalling the poll). No change needed.
From Alexey's inline comments and Tony's kcap peer review: - the leg now builds its client through the ONE authenticated-client choke point: the bearer is resolved against this server (refreshing if expired, binding-checked) and a mid-poll 401 is recovered by refresh, so a short-lived WorkOS token cannot turn the back half of a thirty-minute wait into a dead sign-in. The token read moved inside the leg's guarded try (the "cannot crash setup" promise now covers it), and a non-Ok auth status gets one line telling the user to re-login - the poll verifies the echoed flow_id exactly as the create path does - the escape hatch stays responsive: the delay is slept in 200ms slices, a keypress during an in-flight poll is noticed right after it, and a keypress that preceded the wait is drained rather than taken as a dismiss - the poll backs off on every unhappy response (honouring the route's Retry-After) and snaps back to the 2s cadence on a good state - an unreadable 2xx create body is reported as unreadable, not as a rejection quoting the success status - the setup URL is reprinted every ~minute so the poll dots cannot scroll the one line a headless machine's user needs to read away - docs: the browser leg's skip list now includes auth provider None - fix the stale rationale on the poll 401 classification
alexeyzimarev
self-requested a review
August 24, 2026 13:33
alexeyzimarev
previously approved these changes
Aug 24, 2026
|
Code review by qodo was updated up to the latest commit e3a0ae7 |
- a server-provided Retry-After is honoured as-is, even beyond the 30s cap that still bounds the locally computed doubling; a rate-limited route that asks for 60s is not polled at 30s - the stale-input drain moved to before the "press any key" prompt renders: a key that preceded the leg is still drained, and a key pressed in response to the prompt is a real dismissal, not stale input - the poll loop re-checks the budget deadline after the interval wait, so a sleep crossing the deadline ends the wait instead of issuing one more poll - trim the choke-point comment in the leg down to the non-obvious why
realtonyyoung
requested changes
Aug 24, 2026
realtonyyoung
left a comment
realtonyyoung
left a comment
Collaborator
There was a problem hiding this comment.
Static review of the latest head found two actionable issues. The first allows a server-supplied delay to defeat the flow's stated 30-minute backstop; the second exposes raw Spectre tags in an error path. No build or tests were run, per request.
…ough Spectre From Tony's static review of the latest head: - a server Retry-After longer than what remains of the 30-minute budget no longer sleeps past the backstop: the interval is capped at deadline - now, so a route that asks for an hour cannot hold a keyboard-less host for one - the no-token skip line goes through AnsiConsole so its [dim] markup renders instead of printing literally
Collaborator
|
NO FINDINGS |
realtonyyoung
approved these changes
Aug 24, 2026
realtonyyoung
left a comment
realtonyyoung
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed the latest head (13c6f4b). The previously reported findings are addressed. No build or tests were run, per request.
George-Payne
deleted the
georgepayne/ai-2156-setup-creates-and-polls-flow
branch
August 24, 2026 14:39
Coldaine
pushed a commit
to MooseGooseConsulting/kcap-cli
that referenced
this pull request
Aug 25, 2026
…rent-io#640) * Create the first-run flow before opening the browser, and poll it 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. * Honour caller cancellation and date-form Retry-After in the first-run flow client Qodo review findings on the first-run browser leg: - caller cancellation was swallowed as a transport blip, so Ctrl-C could not stop the poll before its 30-minute budget; rethrow OCE when the caller token is cancelled and degrade only HttpClient's own timeout (same exception type, token unsignalled) - Retry-After was read as delta-seconds only; a proxy rewriting it as an HTTP date was reported as no header at all. Read both forms, measured against the response's own Date header so server clock skew cannot turn the wait negative - trim the retired-pairing narrative from the new docblocks down to the non-obvious constraints, per the comment rule * Address peer-review findings on the first-run browser leg From Alexey's inline comments and Tony's kcap peer review: - the leg now builds its client through the ONE authenticated-client choke point: the bearer is resolved against this server (refreshing if expired, binding-checked) and a mid-poll 401 is recovered by refresh, so a short-lived WorkOS token cannot turn the back half of a thirty-minute wait into a dead sign-in. The token read moved inside the leg's guarded try (the "cannot crash setup" promise now covers it), and a non-Ok auth status gets one line telling the user to re-login - the poll verifies the echoed flow_id exactly as the create path does - the escape hatch stays responsive: the delay is slept in 200ms slices, a keypress during an in-flight poll is noticed right after it, and a keypress that preceded the wait is drained rather than taken as a dismiss - the poll backs off on every unhappy response (honouring the route's Retry-After) and snaps back to the 2s cadence on a good state - an unreadable 2xx create body is reported as unreadable, not as a rejection quoting the success status - the setup URL is reprinted every ~minute so the poll dots cannot scroll the one line a headless machine's user needs to read away - docs: the browser leg's skip list now includes auth provider None - fix the stale rationale on the poll 401 classification * Address the follow-up qodo review on the first-run browser leg - a server-provided Retry-After is honoured as-is, even beyond the 30s cap that still bounds the locally computed doubling; a rate-limited route that asks for 60s is not polled at 30s - the stale-input drain moved to before the "press any key" prompt renders: a key that preceded the leg is still drained, and a key pressed in response to the prompt is a real dismissal, not stale input - the poll loop re-checks the budget deadline after the interval wait, so a sleep crossing the deadline ends the wait instead of issuing one more poll - trim the choke-point comment in the leg down to the non-obvious why * Bound the poll wait by the remaining budget; render the skip line through Spectre From Tony's static review of the latest head: - a server Retry-After longer than what remains of the 30-minute budget no longer sleeps past the backstop: the interval is capped at deadline - now, so a route that asks for an hour cannot hold a keyboard-less host for one - the no-token skip line goes through AnsiConsole so its [dim] markup renders instead of printing literally
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
3 participants
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