Skip to content

feat(cli): polymorfa doctor - #23

Merged
purpshell merged 4 commits into
devfrom
feat/doctor
Sep 20, 2026
Merged

purpshell merged 4 commits into
devfrom
feat/doctor

Conversation

@purpshell

@purpshell purpshell commented Sep 19, 2026 •

Copy link
Copy Markdown
Member

Summary

Adds polymorfa doctor. The keyring error message on dev already tells users to "Run polymorfa doctor", but the command did not exist until now.

Check Behavior
node Fails below Node.js 22 (engines).
config Loads and validates the profile config; reports the active profile.
keyring Reads the selected profile's keyring entry (or probes an unused entry when no profile is selected). A locked or unavailable keyring fails, and a missing credential fails. Nothing is written.
api Unauthenticated SystemClient.ping() (/ping) against the resolved base URL, without retries.
credential For server API keys: Client.projects.list() is the authenticated ping. Authentication error → fail; authorization error → warn (valid key, missing sessions:read). Skipped when the API is unreachable. Project and client tokens are reported as present but not validated remotely.

The command exits 1 if any check fails, and --json prints {checks, ok}. Credential values are registered for redaction and never appear in output. Environment credentials take precedence over the profile, matching the other commands.

Independent of #19 (project context): it does not report directory links yet, so the two PRs don't have to land in order.

Five-part assessment

  • API — unaffected. Uses existing /ping and GET /platform/projects.
  • SDK — unaffected. SystemClient.ping, Client.projects.list. The SDK has no credential-introspection ("whoami") method, so the project list stands in as the authenticated ping, and project/client tokens cannot be validated generically.
  • CLI — updated. New doctor command with help and examples, plus a README section.
  • Docs — unaffected. No public CLI page exists in the Mintlify site.
  • Feature releases — unaffected. No flags.

Verification

  • npm run verify passes. test/services/doctor.test.ts (8 tests) covers the all-green path, old Node, keyring failure, invalid config, unreachable API skipping the key check, 401 vs 403, env precedence, and client tokens. It also asserts that the key never appears in the report.
  • Manual run of the built binary: exit code 1 and a readable table against an unreachable base URL. From this machine https://api.polymorfa.com/ping was also unreachable, so a real successful remote ping and key validation were not exercised.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.

@purpshell

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

  • Ask an admin to enable usage-based reviews

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Next included review available in 15 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 89 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

The included review limit has been reached and this organization has disabled usage-based review continuation. Wait for reviews to reset or ask a billing admin to change After included review limits.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: dcf920e1-a439-414e-919e-ee05b73d9d0d

📥 Commits

Reviewing files that changed from the base of the PR and between 6fec968 and cd18911.

📒 Files selected for processing (4)
  • README.md
  • src/commands/doctor.ts
  • src/services/doctor.ts
  • test/services/doctor.test.ts

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Sep 19, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@purpshell

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 20, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@purpshell

Copy link
Copy Markdown
Member Author

Reviewed locally because CodeRabbit is rate-limited (it answered @coderabbitai review with "Review rate limited" on the current head). Ran the CodeRabbit CLI against dev (0 findings) plus a separate local code review.

Fixed one finding:

  • doctor failed the run for a stale activeProfile even when environment credentials applied. The no-resolvable-profile branch hardcoded status: 'fail', so a config.activeProfile naming a profile that no longer exists made polymorfa doctor exit non-zero while the env credential was the one actually being validated downstream. The sibling branch deliberately downgrades exactly this case to warn; the two paths now agree. Test added.

Nothing left unfixed.

@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@purpshell Thanks for the update. The corrected behavior is consistent with environment credential precedence. A stale activeProfile must not fail doctor when an environment credential is the effective credential.


✏️ Learnings added
Learnt from: purpshell
URL: https://github.com/polymorfa/cli/pull/23

Timestamp: 2026-09-20T07:05:41.192Z
Learning: In the CLI `doctor` command, if `config.activeProfile` refers to a profile that cannot be resolved and an environment credential is effective, report the stale active profile as `warn`, not `fail`. Environment credentials take precedence over profile credentials, so this condition must not make `polymorfa doctor` exit non-zero. Tests cover this case.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

@purpshell

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 20, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@purpshell

Copy link
Copy Markdown
Member Author

Rebased onto dev after #18 and #19 merged (README conflict resolved by keeping both the Project context and Doctor sections), then re-reviewed. CodeRabbit is still rate-limited under its fair-usage policy, so this was reviewed locally with the CodeRabbit CLI plus a local code review.

Two further fixes:

  • The catch around the keyring block still hardcoded fail, bypassing the environment downgrade the two branches above apply — the same bug class as the previous round, one branch over. In a CI container with POLYMORFA_API_KEY set and no secret service, doctor exited 1 while every real command worked, because resolveCommandEnvironment resolves the environment credential before touching the keyring. Now warns when environment credentials apply. Test added.
  • The suggested remediation was not runnable. polymorfa auth login --profile <name> exits 2, because auth login requires exactly one of --with-api-key/--with-project-token. Both messages now name a runnable command.

README's keyring row updated to match.

Deliberately left unfixed, as design questions larger than this PR:

  • doctor's environment-credential model is broader than resolveCommandEnvironment's: it honours POLYMORFA_PROJECT_TOKEN, and it prefers the API key when both a key and a client token are set, where command resolution raises "credentials are ambiguous". Aligning the two is worth doing, but it changes which credential doctor reports on rather than fixing a check.
  • A skip status keeps ok === true, so a project- or client-token credential exits 0 even though messaging and platform commands reject it.
  • CodeRabbit also suggested renaming "server API key" to "organization API key". Declined: "server API key" is the established term across the CLI (10+ files, including every sip-trunk command and sdk/client.ts), so the rename would make doctor the inconsistent one. It belongs in a repo-wide rename, not here.

Checks Node.js version, profile config, keyring access, API reachability
via /ping, and server API key validity via an authenticated project list.
…dentials

The missing-profile branch hardcoded fail, so a stale activeProfile made
doctor exit non-zero even when the environment credential was the one
being validated. Downgrade to warn, matching the sibling branch.
…dentials

The catch around the keyring block hardcoded fail, bypassing the env
downgrade the branches above apply, so doctor exited 1 in CI containers
without a secret service even though commands resolve the environment
credential first. Also point the remediation at a runnable auth login
invocation; it requires a credential-source flag.
@purpshell
purpshell merged commit 89a2754 into dev Sep 20, 2026
3 checks passed
@purpshell
purpshell deleted the feat/doctor branch September 20, 2026 14:31
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