Repository navigation
Report the workspaces discovery finds, without choosing one - #1005
Conversation
The workspaces an account belongs to only exist on the far side of a sign-in, and setup learns them mid-run and carries on to completion. A tool that has to ask someone which workspace to use has nowhere to stand: the choice has to be made before the only command that could inform it. `kcap setup --discover [--json]` signs in, reports what it found, and stops. It changes nothing, which is the point rather than a detail: the picker declines, a decline is strictly pre-boundary, and the boundary is where profiles, tenant activation and tokens are published together. No provisioner is supplied, so the route cannot create a workspace either. Publishing nothing means the token is not published, so the run that follows signs in again. That is the trade for a report that cannot leave a machine half-configured. Naming a server alongside --discover is refused rather than ignored: it already answers the question discovery exists to ask.
PR Summary by QodoAdd non-mutating workspace discovery to setup
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
The first cut asked the normal discovery flow for the rows and supplied a picker that declined, on the reasoning that a decline is pre-boundary and therefore publishes nothing. It publishes nothing only when the picker is asked. A sole workspace is auto-selected before any picker is consulted, so that route configured the machine for exactly the case a report is most needed and then reported no workspaces; an account with none failed instead of answering. `DiscoverOnlyAsync` goes at the rows directly: sign in, list through the proxy, return. Publishing is a separate step this route never reaches, and no provisioner is passed, so nothing is written and nothing is created. `can_create` is now the lane's answer rather than a count of an empty list. Only the hosted lane provisions; a GitHub-App account gets a workspace by having the app installed on an org, so offering it creation is a dead end, and the human hint for that lane says so. The progress sink takes the stream to write to. Its notices, browser lines, device code and poll ticks all went to stdout, which would have put human text in front of the document -- the sign-in still narrates itself under --json, on stderr.
…etup-discover # Conflicts: # docs/CHANGES.md
|
Code review by qodo was updated up to the latest commit f9d8f37 |
The guard read args[1] only, which is where the tenant argument normally goes - but --discover commonly occupies that slot, and anything after it escaped. `setup --discover acme` signed in and discovered instead of refusing the workspace it had been handed. The check now scans every bare token after the verb, less the values of the flags that take one, so --default-visibility private is not read as a workspace called private. Naming one through --server-url, --org or --slug counts the same way.
Four things the rework got wrong, all of them a message to a person. A rejected token borrowed GitHub's wording on the WorkOS lane, which sends that user to the wrong sign-in; the error now names the lane it came from, and the unreachable-service message names neither, because it is not either provider's doing. A GitHub-App row is identified by origin and carries no slug, so closing with `setup <slug>` handed those users a command they cannot run. The hint now offers the URL form when no row has a slug. A cancelled request comes back from the proxy looking like an unreachable one, so cancelling discovery reported an outage and pointed the reader at a service that is fine. And the README described a --discover that setup never had - the flag was documented and unimplemented. It exists now, so the sentence says what it does.
| if (GetArg(args, "--slug") is not null) return true; | ||
|
|
||
| string[] valued = ["--server-url", "--org", "--slug", "--default-visibility", "--daemon-name", "--profile"]; | ||
| var values = valued.Select(flag => GetArg(args, flag)).Where(v => v is not null).ToHashSet(); |
There was a problem hiding this comment.
The values are excluded by text, not by their argument positions. For example, kcap setup --discover --profile acme acme treats both acme tokens as the --profile value, so the extra positional workspace is silently accepted and discovery starts instead of rejecting the invalid pairing. Skip each valued flag and only its following token while scanning, and cover a positional token that equals a flag value.
There was a problem hiding this comment.
Fixed in 167dfa4, by removing the value list rather than making it positional. --discover now takes a closed set (--json, --github, --device, --no-prompt, plus the process-wide --no-update-check), and the scan refuses the first argument outside it, so there is no set of values for a positional to hide in.
Your example is refused at --profile and discovery never starts. That is pinned by A_workspace_repeating_an_earlier_value_does_not_start_discovery (a245c52), and the mirrored order, --discover acme --daemon-name acme, by A_workspace_is_still_caught_when_a_later_value_repeats_it.
Closed set rather than skipping each valued flag's following token, because a skip list has to know every flag that takes a value. The one here already missed --plugin-scope and --use-provider-api-key, so --discover --plugin-scope user was refused as a workspace called "user". The cost: options --discover used to ignore, such as --daemon-name and --default-visibility, are now refused by name.
Run against the published AOT binary: kcap setup --discover acme, kcap setup acme --discover --json and kcap setup --discover --server-url <url> each exit 1 with the workspace refusal, and kcap setup --discover --plugin-scope user exits 1 naming the flag, all before any sign-in.
realtonyyoung
left a comment
realtonyyoung
left a comment
There was a problem hiding this comment.
One argument-validation issue remains in --discover: a positional workspace can be missed when it matches another option’s value. I left a focused inline comment. This was a source review only; I did not build or run tests.
The scripts answer the org switch and the token exchange, and a control runs the same one through the choosing route, so the tests fail if this route ever reaches either. A cancel thrown by the sign-in escaped the facade while its siblings return one; it now comes back as a cancelled report. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A list of what to refuse has to know every flag that takes a value, or it reads --plugin-scope user as a workspace called user. kcap login --discover keeps its own meaning, so each help text now points at the other. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
alexeyzimarev
changed the title
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…etup-discover # Conflicts: # docs/CHANGES.md
3 participants
AI-2935 — no GitHub issue exists for this work
What & why
kcap setup --discover [--json]signs in, reports the workspaces the account can reach, and stops. That list only exists on the far side of a sign-in, andsetuplearns it in the middle of a run it then carries to completion, so a tool that has to ask someone which workspace to use had no command to ask with.It reaches the rows through the proxy directly rather than through the choosing flow, which selects a sole workspace before any picker is asked. Nothing is published — no profile, no activation, no token — so the run that follows signs in again; on a device-code flow that is a second approval.
Where to look
can_createmeans no workspace was found on the one lane that can create one. It is not a promise: a workspace already being made for the account is only learned by the create call.--discovertakes a closed set (--json,--github,--device,--no-prompt); a workspace argument or any other option is refused by name.kcap login --discoverkeeps its own meaning: pick a workspace and save it.Verification
OnboardingFacadeDiscoverOnlyTests: one, several and zero workspaces leave the config root empty, beside a control that publishes the same script through the choosing route.kcap setup --discover acmeandkcap setup --discover --plugin-scope userexit 1 with their refusal before any sign-in; the publish shows no IL2026/IL3050.--discoverrun against a live tenant, which needs an approval in a browser.🤖 Generated with Claude Code