Skip to content

fix: support CLI login on fresh Linux hosts#17

Merged
ryanmcmillan merged 1 commit intomainfrom
fix/cli-login-fallback-and-exit-codes
Mar 27, 2026
Merged

fix: support CLI login on fresh Linux hosts#17
ryanmcmillan merged 1 commit intomainfrom
fix/cli-login-fallback-and-exit-codes

Conversation

@ryanmcmillan
Copy link
Copy Markdown
Member

Summary

  • fall back to ~/.delega/config.json when no secure credential store is available
  • keep the secure-store path unchanged where it exists
  • make unknown commands exit non-zero after printing help

Why

On a fresh Linux controller with no keyring backend, delega login could validate the key but then failed to store it at all. The CLI also returned exit code 0 for unknown commands, which is wrong for scripts and automation.

Validation

  • cd /Users/openclaw/projects/delega-cli && npm run build
  • fresh controller repro: printf "$DELEGA_HOSTED_KEY " | delega login now succeeds and stores config in /root/.delega/config.json
  • fresh controller repro: delega totally-fake-command now exits 1
  • reran /root/work/delega-tests/cli/cli-hosted.sh on the fresh controller -> 20/20 pass

@ryanmcmillan ryanmcmillan merged commit 9a95ec9 into main Mar 27, 2026
2 checks passed
@ryanmcmillan ryanmcmillan deleted the fix/cli-login-fallback-and-exit-codes branch March 27, 2026 02:40
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR fixes two independent regressions on fresh Linux hosts: delega login now succeeds when no keyring backend is available by falling back to storing the credential in ~/.delega/config.json, and unknown commands now exit with code 1 instead of 0.

Key changes:

  • persistApiKey (src/config.ts) now returns an ApiKeyStorageResult { location, secure } object instead of throwing when no system keyring is found. The fallback path returns { location: config path, secure: false }, letting callers decide how to persist the credential.
  • Both login.ts and init.ts were updated to write the credential into config.json when secure === false, and to clear it from config.json when secure === true (keyring path is unchanged).
  • The unknown-command handler in src/index.ts was updated from program.help() (which exits 0) to program.outputHelp() + process.exit(1), which is the correct behavior for scripts and automation.
  • The secure flag design is clean and ensures existing users on macOS/Windows/Linux-with-keyring see no behavior change.
  • One minor observation: saveConfig is now inside the same try block as persistApiKey in login.ts. If the config write fails, the error surfaces as "Unable to store API key" rather than a more specific config-write message — a small UX clarity gap but not a functional issue.

Confidence Score: 4/5

Safe to merge — the fallback logic is correct, existing secure-store paths are unaffected, and the exit-code fix is straightforward.

The core logic is sound: persistApiKey now correctly falls back instead of throwing, callers propagate the secure flag to decide where to write the credential, and the config is updated correctly in both paths. The only open item is a minor UX improvement to the error message in login.ts when saveConfig fails, which does not affect correctness or the primary user flow.

No files require special attention; src/commands/login.ts has the small error-message improvement opportunity noted above.

Important Files Changed

Filename Overview
src/config.ts Adds ApiKeyStorageResult interface and changes persistApiKey to return { location, secure } instead of throwing when no keyring is available; clean and correct fallback logic
src/commands/login.ts Correctly uses ApiKeyStorageResult to conditionally write key to config.json on insecure fallback; saveConfig moved into the try block, which means a config-write failure is caught but reported with a slightly misleading "Unable to store API key" message
src/commands/init.ts Updated storeApiKey wrapper and saveApiConfig to carry the secure flag through to the config write; logic is correct for both secure and insecure paths
src/index.ts Switches unknown-command handler from program.help() (exits 0) to program.outputHelp() + process.exit(1); correct fix with a descriptive error message printed to stderr

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["delega login / delega init"] --> B["persistApiKey\nsrc/config.ts"]
    B --> C{secret-store\nreturns a label?}
    C -- "yes — keyring available" --> D["Return\nlocation: keyring label\nsecure: true"]
    C -- "no — no keyring backend" --> E["Return\nlocation: config.json path\nsecure: false"]
    D --> F{secure flag?}
    E --> F
    F -- "true" --> G["Remove credential field\nfrom config\nsaveConfig\nkey lives only in keyring"]
    F -- "false" --> H["Write credential field\ninto config\nsaveConfig\nkey lives in config.json"]
    G --> I["Log: saved to keyring label"]
    H --> I
Loading

Reviews (1): Last reviewed commit: "fix: support cli login fallback on fresh..." | Re-trigger Greptile

Comment on lines 168 to 171
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
console.error(`Unable to store API key securely: ${msg}`);
console.error(`Unable to store API key: ${msg}`);
process.exit(1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Mixed error message if config write fails

saveConfig is now inside the catch-all try block, which is the right change. But if saveConfig throws (e.g. a permission error writing the config directory), the catch clause will surface it as Unable to store API key, implying the credential-store step failed rather than the file write.

For clarity, consider wrapping saveConfig in its own try/catch with a distinct message like Unable to save config, consistent with the wording already used in saveApiConfig inside init.ts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant