fix(identity): correct identity diagnosis under external credential providers#1693
Merged
Conversation
…roviders whoami and doctor judged token availability from the local keychain and app-secret heuristics via the shared identitydiag engine. Under an extension credential provider — which mints tokens on demand and blocks interactive auth — a signed-in user was reported as missing with a hint to run `auth login`, a command that is itself blocked there. Add an external-provider branch to identitydiag.Diagnose, reached only when an extension provider is active (all built-in paths stay byte-identical, so auth status is unaffected). Availability follows the identities the provider declares and is confirmed against the credential chain when verifying; unavailable identities get provider-appropriate guidance instead of keychain-only hints. whoami now renders the diagnosed hint instead of synthesizing its own, and validates the resolved identity exactly as a real API call does (CheckStrictMode then CheckIdentity) so it cannot preview an identity strict mode would refuse. doctor's identity_ready failure no longer points at `auth status` (blocked under an external provider); it defers to the per-identity hints.
whoami is consumed by agents, so make its JSON the single, clean contract: - Always emit JSON; drop the human-readable (pretty) output. The --json flag is kept hidden and ignored so existing `whoami --json` callers don't break. - Make the self-vs-delegated distinction explicit: `identity` is the signal (bot = the app itself; user = acting on behalf of a person). The user's openId/userName nest under `onBehalfOf`, present only once a user is resolved. - tokenStatus uses one vocab for both identities (ready / needs_refresh / missing / not_configured), so "ready" means usable for bot and user alike; `available` stays the canonical usable signal. - identitySource values are snake_case (auto_detect / strict_mode / default_as) to match the co-located enums. Keys stay camelCase, lark-cli's house style. BREAKING: whoami --json nests user under onBehalfOf (was flat openId/userName), changes user tokenStatus values (valid->ready, expired->missing) and identitySource values (kebab->snake), and removes the non-JSON output mode.
github-actions
Bot
added
the
size/L
📝 WalkthroughWalkthroughAdds external credential provider support to ChangesExternal Provider Identity Diagnosis and Command Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MaxHuang22
approved these changes
Jun 30, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1693 +/- ##
==========================================
- Coverage 74.73% 74.47% -0.27%
==========================================
Files 811 849 +38
Lines 81965 86631 +4666
==========================================
+ Hits 61258 64517 +3259
- Misses 16138 17172 +1034
- Partials 4569 4942 +373 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@91aee2389cc9f5a6fff03649de354a178bb26801🧩 Skill updatenpx skills add larksuite/cli#fix/identity-external-provider -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/identitydiag/diagnostics_test.go (1)
371-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the standard test factory helper here.
The new helper manually constructs
cmdutil.Factory; please build fromcmdutil.TestFactory(t, cfg), setLARKSUITE_CLI_CONFIG_DIRwitht.Setenv, then overrideCredential. As per coding guidelines, “Usecmdutil.TestFactory(t, config)for test factories” and “Isolate config state in tests witht.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/identitydiag/diagnostics_test.go` around lines 371 - 380, The externalFactory test helper is constructing cmdutil.Factory manually instead of using the standard test factory setup. Update externalFactory to build from cmdutil.TestFactory(t, cfg), set LARKSUITE_CLI_CONFIG_DIR with t.Setenv to a temporary dir, and then override the Credential field with the fake provider; use the existing externalFactory and cmdutil.TestFactory symbols to keep the test config isolated and consistent.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/doctor/doctor_test.go`:
- Line 7: The test setup is manually constructing a cmdutil.Factory instead of
using the shared cmdutil.TestFactory helper. Update the doctor tests to create
the factory via cmdutil.TestFactory(t, config), then override f.Credential only
for the external-provider case so the existing test behavior stays the same. Use
the doctor test setup and any related helper code in the same test file as the
reference point for the change.
In `@cmd/whoami/whoami_test.go`:
- Around line 250-262: The external-provider test fixture is manually
constructing cmdutil.Factory instead of using the standard test setup. Update
externalWhoamiFactory to accept t, initialize the factory with
cmdutil.TestFactory(t, cfg), and then override only the Credential field with
the fake ext provider so the helper matches the repository’s test-factory
pattern. Also update the other whoami test helpers at the referenced call sites
to follow the same cmdutil.TestFactory-based setup.
- Around line 227-234: The error-path test in whoami_test should assert the
typed metadata for the rejected --as case, not only that the returned error is a
*errs.ValidationError. Update the test around cmd.Execute() to validate
errs.ProblemOf(err) fields for category and subtype, and verify the
ValidationError.Param matches the rejected input, using the existing error
variable and the whoami command path to locate the assertion block.
In `@internal/identitydiag/diagnostics.go`:
- Around line 67-68: The external identity path in
activeExternalProvider/diagnoseExternal is dropping extcred.BlockError state and
can incorrectly report bot/user as ready. Carry the resolved account/block
status from ActiveExtensionProviderName into diagnoseExternal, and ensure
blocked account resolution is treated as unavailable with the external provider
hint instead of falling through to the ready result from cfg.
---
Nitpick comments:
In `@internal/identitydiag/diagnostics_test.go`:
- Around line 371-380: The externalFactory test helper is constructing
cmdutil.Factory manually instead of using the standard test factory setup.
Update externalFactory to build from cmdutil.TestFactory(t, cfg), set
LARKSUITE_CLI_CONFIG_DIR with t.Setenv to a temporary dir, and then override the
Credential field with the fake provider; use the existing externalFactory and
cmdutil.TestFactory symbols to keep the test config isolated and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5551f8fb-4bae-4fe1-a6ad-9dfe218efbe7
📒 Files selected for processing (7)
cmd/doctor/doctor.gocmd/doctor/doctor_test.gocmd/whoami/whoami.gocmd/whoami/whoami_test.gointernal/identitydiag/diagnostics.gointernal/identitydiag/diagnostics_test.goskills/lark-shared/SKILL.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Identity diagnosis (shared by
auth status,doctor, and the new top-levelidentity command) judged token availability from the local keychain and
app-secret heuristics. Under an extension credential provider — which mints
tokens on demand and blocks interactive auth — this mis-reports:
missing/unavailable, because the provider's tokenisn't in the local keychain;
auth login/auth status, both of which areblocked under an external provider (
RequireBuiltinCredentialProvider),so the agent is handed a next step that immediately errors.
auth statusis gated and never runs under an external provider, butdoctor(ungated) and the identity command both did.
Changes
internal/identitydiag(engine, root cause)Diagnosegains an external-provider branch, reached only when anextension provider is active. Availability follows the identities the provider
declares (and is confirmed against the credential chain when verifying);
unavailable identities get provider-appropriate guidance instead of
keychain-only hints.
auth statusoutput is unchanged;doctoris incidentally fixed.identity command
keychain-only ones.
(
CheckStrictModethenCheckIdentity), so it can't preview an identity thatstrict mode would refuse.
is explicit:
identityis the signal (a bot identity is the app acting asitself; a user identity acts on behalf of a person, nested under
onBehalfOf).tokenStatususes one vocab for both identities;availablestays the canonical usable signal. Enum values are snake_case; keys camelCase.
doctoridentity_readysummary no longer points atauth status(blocked underan external provider); the per-identity checks carry the right next step.
BREAKING (identity command
--json)onBehalfOf{userName,openId}(was flatopenId/userName);tokenStatusvalues change (valid→ready,expired→missing);identitySourcevalues change from kebab-case to snake_case;--jsonflag is kepthidden and ignored for backward compatibility).
Test plan
internal/identitydiag, theidentity command, and
doctor(including a regression that theidentity_readyhint never names a blocked command).go build ./...passes;internal/identitydiag,cmd/auth,cmd/doctor,the identity command,
internal/credential,internal/cmdutil, andcmdtest suites all green.
go vet ./...clean across the repo (apart from unrelated pre-existinguntracked files).
Summary by CodeRabbit
New Features
whoaminow always returns structured JSON and clearly shows delegated identity details when applicable.Bug Fixes
Documentation
whoamibehavior.