Repository navigation
Conversation
The cli-sign-in mode (`boxel keys sign-in`) rendered the shared sign-in screen with its default heading and the Google subtitle offering to create an account, so a user signing the CLI in wasn't told that's what they were doing. It now passes the screen "Authorize Boxel CLI" as its title and an intro saying signing in lets the CLI manage the user's keys, as the cli-auth mode does for `boxel profile add`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review traced what the CLI receives from each secure-origin CLI sign-in mode and checked the new copy and the PR description against that. It also checked the test, the build record (lint:build in check mode), and the Playwright specs that open these modes. It did not look at visual design beyond the screenshots.
No blocking issues. The new title and intro are right for cli-sign-in, and the test fails without the change. The PR description misstates what cli-auth hands over, and that wrong statement is what the new intro's wording is contrasted against.
- The description says
cli-authgrants less than it does (description accuracy, non-blocking; thecli-authcopy is a pre-existing follow-up). The description sayscli-authgives the CLI a single-use login token, and that the new intro "names what this particular sign-in grants", as opposed to the profile sign-in. ButbuildSecureCliAuthUrlinpackages/boxel-cli/src/lib/sso-login.tsalways addssession=secure, sohandProfileSignIninpackages/secure-origin/src/security/cli-handoff.jsgivesboxel profile addthe same seven-day secure session thatboxel keys sign-ingets, along with the login token.profile addthen saves it as the profile's keys session and prints "Signed in to manage keys". Socli-authgrants keys management too, yet its intro ("…access to your workspaces") doesn't say so. Please correct the description. Separately, decide whethercli-auth's intro should also mention keys: this PR argues that a CLI sign-in page should say what it lets the CLI do, andcli-authis the page where that is still untrue. That copy fix can be a follow-up, not this PR.
Red checks: none. A few checks are still running.
`boxel profile add` asks for the secure session alongside the login token (`session=secure`), so signing in there lets the CLI manage the user's keys as well as reach their workspaces. The cli-auth page said only the latter. It now names keys whenever the link asks for the secure session, in both the signed-out intro and the confirm-your-password line, and keeps the workspaces-only copy for a link that doesn't. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
[Claude Code 🤖] On the review's finding that
|
Host Test Results 1 files ± 0 1 suites ±0 1h 38m 39s ⏱️ + 1h 31m 48s Results for commit 229c746. ± Comparison against earlier commit 91c8f0b. Realm Server Test Results 1 files ± 0 366 suites +1 2h 10m 13s ⏱️ + 7m 35s Results for commit 229c746. ± Comparison against earlier commit 91c8f0b. |
…cure-origins-cli-sign-in-for-the-cli-not-as-the # Conflicts: # packages/secure-origin/build-record.json
Linear: CS-14314
The secure origin is the separate origin (
secure.boxel.ai,https://localhost:4208/securein development) where Boxel signs people in, so that card code running in the app can never read a typed password. Its page is one shell document with several modes, picked by themodequery parameter. Two of those modes sign in for the Boxel CLI instead of for the app:cli-sign-inis whatboxel keys sign-inopens. Once signed in, the window hands the CLI a secure session, which is the only session that can change the user's keys. The CLI uses it forboxel keysand nothing else.cli-authis whatboxel profile addopens. Once signed in, the window hands the CLI a single-use Matrix login token, which the CLI swaps for a Matrix device of its own (its access to the user's workspaces). The CLI's link always carriessession=secure, so the window hands it the secure session in the same step too. A new profile can therefore manage keys without a separateboxel keys sign-in. Acli-authlink withoutsession=securegets the login token alone. That's the link a CLI that can't hold a secure session reaches, by way of the app's/cli-authpage.Both modes draw the same
SignInScreencomponent the app's own sign-in uses. That screen takes an@titleand anintroblock so a page that signs in for something other than the app can say so. Theintrotakes the place of the line inviting a new user to start with Google.Before this PR, neither page said fully what the user was agreeing to:
cli-sign-inpassed neither. Someone runningboxel keys sign-inlanded on "Sign in to your Boxel Account", under a subtitle offering to create an account with Google. Nothing said they were signing in their terminal, or what that sign-in would let it do.cli-authwas titled "Authorize Boxel CLI", but its intro mentioned only access to the user's workspaces. It said nothing about keys, even though the CLI walks away with a session that can manage them.This PR fixes both:
cli-sign-inpassesSignInForm(which forwards both toSignInScreen) the title "Authorize Boxel CLI" and the intro "Signing in lets the Boxel CLI running on this computer manage your keys." The title matches the profile sign-in, since in both cases the user is authorizing the CLI. The intro's paragraph setsmargin: 0in a scoped style, ascli-auth's does, because the screen's.introwrapper already sets the spacing around it.cli-authnames keys whenever the link asks for the secure session (callback.profile.secureSession, which the page already reads to decide what to hand over). Signed out, it reads "Signing in gives the Boxel CLI running on this computer access to your workspaces, and lets it manage your keys." When the window is already signed in, it asks that account to confirm its password: "…access to the workspaces of @user:server, and to let it manage their keys." A link withoutsession=securekeeps the workspaces-only copy, because that's all it hands over.Nothing else changes: the fields, Google button, links and hand-off to the CLI's loopback listener stay the same.
build-record.jsonis regenerated. The secure-origin build checks that every module it bundles is recorded, and thecli-sign-inpage now has its own scoped CSS file.Screenshots
Both pages are rendered from this package's build,
origin/mainfor before and this branch for after, against the development stack's realm server, with Google offered so the subtitle thecli-sign-inintro replaces is visible.cli-sign-in(boxel keys sign-in)cli-auth(boxel profile add, whose link carriessession=secure)Tests
These are component tests in
packages/secure-origin/tests/integration/sign-in-test.gts:cli-sign-in: a new test renders the page with Google offered. It checks that the page is titled "Authorize Boxel CLI", that it shows the keys intro, and that the Google subtitle is gone.cli-auth, link withsession=secure: the existing intro assertion now expects the keys clause. The signed-in test now also asserts the full confirm-your-password line, keys included.cli-auth, link withoutsession=secure: two new tests check that the intro and the confirm line both keep the workspaces-only copy.pnpm lintandpnpm test(unit and component) pass inpackages/secure-origin. None of the Playwright specs assert either page's intro, orcli-sign-in's heading on a valid link, so they don't move.🤖 Generated with Claude Code