Conversation
Port profile management from standalone boxel-cli into monorepo. Adds ProfileManager class for CRUD operations on ~/.boxel-cli/profiles.json with subcommands: list, add, switch, remove, migrate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d9086f986
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| env === 'production' | ||
| ? 'https://matrix.boxel.ai' | ||
| : 'https://matrix-staging.stack.cards'; | ||
| const defaultRealmUrl = | ||
| env === 'production' |
There was a problem hiding this comment.
ProfileManager.addProfile() treats every non-boxel.ai Matrix ID as staging by default, so boxel profile add -u @alice:custom.domain silently stores https://matrix-staging.stack.cards and https://realms-staging.stack.cards/ instead of endpoints that match the supplied ID. In practice this creates profiles that look valid but always authenticate against the wrong server when the domain is mistyped or custom, which is hard to diagnose; unknown domains should be rejected or explicitly mapped instead of defaulting to staging.
Useful? React with 👍 / 👎.
| if (this.config.profiles[matrixId]) { | ||
| return matrixId; |
There was a problem hiding this comment.
migrateFromEnv() returns the profile ID even when the profile already exists, which makes the caller print Created profile and suggest removing .env credentials although no migration happened. Re-running boxel profile migrate against an existing profile therefore reports a false success; this should return a separate status (or null for no-op) so the command can message existing-profile cases accurately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Review: CS-10615 — Reimplement boxel profile command
Overall: Solid foundation for profile management. This is the right first step toward CS-10642's full auth lifecycle.
Key concerns (8 inline comments):
-
Realm server URL inference from matrix URL —
getActiveCredentials()guessesrealmServerUrlfrommatrixUrlhostname patterns when env vars are used. This is fragile and inconsistent withmigrateFromEnv()which correctly requiresREALM_SERVER_URLexplicitly. Recommend removing the inference. -
Duplicate helper functions —
getEnvironmentLabelandgetEnvironmentShortLabelare identical implementations. -
Singleton
configDirconfusion —getProfileManager(configDir?)caches the first instance and silently ignoresconfigDiron subsequent calls. -
migrateFromEnvno-op on existing profiles — Silently returns without updating credentials if profile already exists. -
Terminal cleanup in
promptPassword— Raw mode may not be restored on unexpected errors. -
Duplicated ANSI constants — Same color codes defined in both
profile.tsandprofile-manager.ts. -
I would like to see a happy path test against a real realm server. consider leveraging testing infra from the packages/realm-server/tests so that we can spin up a real realm server that we can test against and assert that auth works against an actual realm server.
Phase 2 alignment (CS-10642): The Profile interface and async method signatures are well-positioned for extension with server tokens, per-realm JWTs, and auto-refresh. The getActiveCredentials() method is the natural evolution point for the programmatic BoxelAuth API. Inline comment on the interface details what fields will be needed.
| default: | ||
| return 'unknown'; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get short environment label (uses domain) | ||
| */ | ||
| export function getEnvironmentShortLabel(env: Environment): string { | ||
| switch (env) { | ||
| case 'staging': | ||
| return 'stack.cards'; | ||
| case 'production': | ||
| return 'boxel.ai'; | ||
| default: | ||
| return 'unknown'; | ||
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
getEnvironmentLabel and getEnvironmentShortLabel are identical — they return the exact same values for all cases. Should one be removed, or were they intended to differ (e.g., short label returning abbreviated forms like "staging" / "prod")?
| matrixUrl: string; | ||
| realmServerUrl: string; | ||
| password: string; // Stored in plaintext - file should have restricted permissions | ||
| } |
There was a problem hiding this comment.
Plaintext password storage: the comment acknowledges this, but worth flagging for the CS-10642 follow-up. When ProfileManager becomes the canonical auth store for the full token lifecycle (realm server tokens, per-realm JWTs, auto-acquired tokens on realm creation), plaintext credentials in a JSON file become a bigger liability. Consider planning for OS keychain integration (e.g., keytar or @anthropic-ai/keychain) as a future step — the async signatures on getPassword/updatePassword are well-positioned for this.
| return { | ||
| matrixUrl: active.profile.matrixUrl, | ||
| username: getUsernameFromMatrixId(active.id), | ||
| password: active.profile.password, | ||
| realmServerUrl: active.profile.realmServerUrl, | ||
| profileId: active.id, | ||
| }; | ||
| } | ||
|
|
||
| const matrixUrl = process.env.MATRIX_URL; | ||
| const username = process.env.MATRIX_USERNAME; | ||
| const password = process.env.MATRIX_PASSWORD; | ||
| let realmServerUrl = process.env.REALM_SERVER_URL; | ||
|
|
||
| if (matrixUrl && username && password) { | ||
| if (!realmServerUrl) { | ||
| try { | ||
| const matrixUrlObj = new URL(matrixUrl); | ||
| if (matrixUrlObj.hostname.startsWith('matrix.')) { | ||
| realmServerUrl = `${matrixUrlObj.protocol}//app.${matrixUrlObj.hostname.slice(7)}/`; | ||
| } else if (matrixUrlObj.hostname.startsWith('matrix-staging.')) { |
There was a problem hiding this comment.
This block infers realmServerUrl from the matrixUrl hostname pattern (e.g., matrix. → app., matrix-staging. → realms-staging.). This is fragile — it breaks for non-standard deployments and encodes URL conventions that may change. The project has an established convention of never inferring realm server URL from other URLs.
Consider requiring REALM_SERVER_URL to be set explicitly when falling back to env vars (i.e., return null if it's missing rather than guessing). The migrateFromEnv() method already requires it explicitly — this fallback path should be consistent.
| let _instance: ProfileManager | null = null; | ||
|
|
||
| export function getProfileManager(configDir?: string): ProfileManager { | ||
| if (!_instance) { | ||
| _instance = new ProfileManager(configDir); | ||
| } | ||
| return _instance; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The singleton caches the first instance, but configDir is only used on the first call — subsequent calls silently ignore it. This could cause confusing behavior in tests or when the programmatic API (CS-10642's BoxelAuth) needs multiple managers for different contexts.
Consider either:
- Removing the
configDirparameter fromgetProfileManager()(force callers who need a custom dir to usenew ProfileManager(dir)directly) - Or validating that the
configDirmatches if an instance already exists
| if (active) { | ||
| console.log( | ||
| `\n${BOLD}Active Profile:${RESET} ${formatProfileBadge(active.id)}`, | ||
| ); |
There was a problem hiding this comment.
If a profile already exists for the migrated Matrix ID, this silently returns the existing ID without updating the password or other fields. A user running boxel profile migrate after changing their password in .env would expect the stored credentials to update. Consider either updating the existing profile or logging a message that tells the user to use boxel profile add --user ... --password ... to update.
| const FG_GREEN = '\x1b[32m'; | ||
| const FG_YELLOW = '\x1b[33m'; | ||
| const FG_CYAN = '\x1b[36m'; | ||
| const FG_MAGENTA = '\x1b[35m'; | ||
| const FG_RED = '\x1b[31m'; | ||
| const DIM = '\x1b[2m'; | ||
| const BOLD = '\x1b[1m'; |
There was a problem hiding this comment.
ANSI color constants are duplicated here and in profile-manager.ts. Consider extracting them to a shared utils/colors.ts or similar — other future commands will likely need them too.
| const mutableOutput = new Writable({ | ||
| write: (_chunk, _encoding, callback) => callback(), | ||
| }); | ||
| const rl = readline.createInterface({ | ||
| input: process.stdin, | ||
| output: mutableOutput, | ||
| terminal: true, | ||
| }); | ||
|
|
||
| return new Promise((resolve) => { | ||
| const stdin = process.stdin; | ||
| if (stdin.isTTY) { | ||
| stdin.setRawMode(true); | ||
| } | ||
|
|
||
| process.stdout.write(question); | ||
| let password = ''; | ||
|
|
||
| const onData = (char: Buffer) => { | ||
| const c = char.toString(); | ||
| if (c === '\n' || c === '\r') { | ||
| stdin.removeListener('data', onData); | ||
| if (stdin.isTTY) { | ||
| stdin.setRawMode(false); | ||
| } | ||
| process.stdout.write('\n'); | ||
| rl.close(); | ||
| resolve(password); | ||
| } else if (c === '\u0003') { | ||
| // Ctrl+C | ||
| process.exit(); | ||
| } else if (c === '\u007F' || c === '\b') { | ||
| // Backspace | ||
| if (password.length > 0) { | ||
| password = password.slice(0, -1); | ||
| process.stdout.write('\b \b'); | ||
| } | ||
| } else { | ||
| password += c; | ||
| process.stdout.write('*'); | ||
| } |
There was a problem hiding this comment.
If an error is thrown between stdin.setRawMode(true) (line 50) and the cleanup in the onData handler, the terminal will be left in raw mode. Consider wrapping in a try/finally, or adding a process exit handler that restores terminal state:
const cleanup = () => {
stdin.removeListener('data', onData);
if (stdin.isTTY) stdin.setRawMode(false);
};Also, stdin.resume() at line 76 may not be paired with a stdin.pause() — if stdin was already flowing (e.g., piped input), this is fine, but in a TTY context the rl.close() in the resolve path doesn't explicitly pause it.
| displayName: string; | ||
| matrixUrl: string; | ||
| realmServerUrl: string; | ||
| password: string; // Stored in plaintext - file should have restricted permissions | ||
| } | ||
|
|
||
| export interface ProfilesConfig { | ||
| profiles: Record<string, Profile>; |
There was a problem hiding this comment.
Phase 2 / CS-10642 alignment note: The Profile interface will need extension to support the two-tier token model. Fields like serverToken, serverTokenExpiry, realmTokens: Record<string, { token: string, expiry: number }> will eventually live here. The current shape is a good foundation — just flagging that getActiveCredentials() will evolve significantly when it needs to return (and auto-refresh) realm server tokens and per-realm JWTs.
The async signatures on credential methods are well-placed for this future — token refresh will require async operations.
There was a problem hiding this comment.
Pull request overview
Ports the boxel profile command into the monorepo’s @cardstack/boxel-cli, adding a local profile storage layer and CLI UX to manage Matrix/realm credentials across environments.
Changes:
- Added
ProfileManager+ helper utilities for reading/writing~/.boxel-cli/profiles.jsonwith restrictive permissions. - Implemented
boxel profilesubcommands (list/add/switch/remove/migrate) including interactive prompting. - Added unit tests for ProfileManager behavior and environment parsing helpers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/boxel-cli/src/lib/profile-manager.ts | Implements profile config persistence, credential selection, and migration from env vars. |
| packages/boxel-cli/src/commands/profile.ts | Adds CLI subcommand routing + interactive/non-interactive profile workflows. |
| packages/boxel-cli/src/index.ts | Wires the new profile command into the CLI entrypoint and loads dotenv. |
| packages/boxel-cli/tests/commands/profile.test.ts | Adds unit tests covering ProfileManager operations and helper functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (fs.existsSync(this.profilesFile)) { | ||
| try { | ||
| const data = fs.readFileSync(this.profilesFile, 'utf-8'); | ||
| return JSON.parse(data); | ||
| } catch { | ||
| // Corrupted file, start fresh | ||
| } | ||
| } | ||
| return { profiles: {}, activeProfile: null }; |
There was a problem hiding this comment.
loadConfig() returns the raw result of JSON.parse() without validating the expected shape. A profiles.json that is valid JSON but missing profiles/activeProfile (or with wrong types) will later cause runtime errors (e.g., Object.keys(this.config.profiles) when profiles is undefined/null). Consider normalizing/validating the parsed value and falling back to { profiles: {}, activeProfile: null } when the shape is not correct.
| if (fs.existsSync(this.profilesFile)) { | |
| try { | |
| const data = fs.readFileSync(this.profilesFile, 'utf-8'); | |
| return JSON.parse(data); | |
| } catch { | |
| // Corrupted file, start fresh | |
| } | |
| } | |
| return { profiles: {}, activeProfile: null }; | |
| const defaultConfig: ProfilesConfig = { profiles: {}, activeProfile: null }; | |
| if (fs.existsSync(this.profilesFile)) { | |
| try { | |
| const data = fs.readFileSync(this.profilesFile, 'utf-8'); | |
| const parsed: unknown = JSON.parse(data); | |
| if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { | |
| const candidate = parsed as { | |
| profiles?: unknown; | |
| activeProfile?: unknown; | |
| }; | |
| const profiles = | |
| candidate.profiles && | |
| typeof candidate.profiles === 'object' && | |
| !Array.isArray(candidate.profiles) | |
| ? candidate.profiles | |
| : null; | |
| const activeProfile = | |
| candidate.activeProfile === null || | |
| typeof candidate.activeProfile === 'string' | |
| ? candidate.activeProfile | |
| : null; | |
| if (profiles) { | |
| return { | |
| profiles: profiles as ProfilesConfig['profiles'], | |
| activeProfile, | |
| }; | |
| } | |
| } | |
| } catch { | |
| // Corrupted file, start fresh | |
| } | |
| } | |
| return defaultConfig; |
| const matrixUrl = process.env.MATRIX_URL; | ||
| const username = process.env.MATRIX_USERNAME; | ||
| const password = process.env.MATRIX_PASSWORD; | ||
|
|
||
| if (!matrixUrl || !username || !password) { | ||
| console.log( | ||
| `${FG_YELLOW}No complete credentials found in environment variables.${RESET}`, | ||
| ); | ||
| console.log( | ||
| `\nRequired variables: MATRIX_URL, MATRIX_USERNAME, MATRIX_PASSWORD, REALM_SERVER_URL`, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
migrateFromEnv() help text says REALM_SERVER_URL is required, but this precheck only validates MATRIX_URL/MATRIX_USERNAME/MATRIX_PASSWORD. When REALM_SERVER_URL is missing, the command falls through and later prints a generic "Migration failed" message. Consider including process.env.REALM_SERVER_URL in the early check so the user gets the correct actionable message.
| it('sets file permissions to 0600', async () => { | ||
| await manager.addProfile('@testuser:stack.cards', 'password123'); | ||
|
|
||
| const profilesFile = path.join(tmpDir, 'profiles.json'); | ||
| const stats = fs.statSync(profilesFile); | ||
| // Check owner-only permissions (0600 = 0o600 = 384 decimal) | ||
| const mode = stats.mode & 0o777; | ||
| expect(mode).toBe(0o600); | ||
| }); |
There was a problem hiding this comment.
This permission-mode assertion is likely to fail on Windows, where chmodSync is ignored/unsupported (and the implementation explicitly swallows chmod errors on Windows). Consider skipping this test on process.platform === 'win32' or asserting more loosely there.
| * Get environment emoji/label for display | ||
| */ | ||
| export function getEnvironmentLabel(env: Environment): string { | ||
| switch (env) { | ||
| case 'staging': | ||
| return 'stack.cards'; | ||
| case 'production': | ||
| return 'boxel.ai'; | ||
| default: | ||
| return 'unknown'; | ||
| } |
There was a problem hiding this comment.
The getEnvironmentLabel doc comment says "emoji/label" but the function returns only a string label and appears unused (and duplicates getEnvironmentShortLabel). Consider either removing it or updating the comment/behavior to match intended usage to avoid confusion.
| * Get environment emoji/label for display | |
| */ | |
| export function getEnvironmentLabel(env: Environment): string { | |
| switch (env) { | |
| case 'staging': | |
| return 'stack.cards'; | |
| case 'production': | |
| return 'boxel.ai'; | |
| default: | |
| return 'unknown'; | |
| } | |
| * Get environment label for display (uses domain) | |
| */ | |
| export function getEnvironmentLabel(env: Environment): string { | |
| return getEnvironmentShortLabel(env); |
| const matrixId = `@${username}:${domain}`; | ||
|
|
||
| if (this.config.profiles[matrixId]) { | ||
| return matrixId; |
There was a problem hiding this comment.
migrateFromEnv() returns matrixId even when the profile already exists (and no migration occurred). The command currently treats any non-null return value as "Created profile", which becomes misleading. Consider returning a richer result (e.g., { profileId, created }) or returning null when the profile already exists so callers can report the right outcome.
| return matrixId; | |
| return null; |
Summary
boxel profilecommand from standalone boxel-cli into monorepo atpackages/boxel-cliProfileManagerclass for CRUD operations on~/.boxel-cli/profiles.json(0600 perms)list,add(interactive wizard),switch(partial match),remove,migrate(from .env)Test plan
pnpm --filter @cardstack/boxel-cli test— 24 tests passpnpm --filter @cardstack/boxel-cli build— builds successfullypnpm --filter @cardstack/boxel-cli lint— no errors🤖 Generated with Claude Code