feat: add session-level profile selection and identity diagnostics#1776
Open
luozhixiong01 wants to merge 25 commits into
Open
feat: add session-level profile selection and identity diagnostics#1776luozhixiong01 wants to merge 25 commits into
luozhixiong01 wants to merge 25 commits into
Conversation
Add LARKSUITE_CLI_PROFILE env var and make BootstrapInvocationContext fall back to it when --profile is empty, so downstream credential resolution sees the correct profile. Also track whether the resolved profile came from the flag or the env fallback via a new InvocationContext.ProfileFromFlag field, needed by a later task to report the correct credential source.
Declares the 5 stable error subtypes (4 config + 1 validation) and the ConfigError/ValidationError extension fields the profile-selection credential core (Task 4) will produce, plus builder-chain and wire-pin tests pinning their shape.
…env provider Mirror the env-incomplete block-path guard on the success-account path so a non-env extension provider (e.g. sidecar, Priority 0) that returns an account wins outright instead of being misreported as a direct-credential env account. This restores pre-diff behavior for such providers: no profile arbitration, no spurious profile_app_credential_conflict, and DirectCredentialEnv.Present stays false when no direct env vars are set. Env matrix states are unchanged. Add TestSelection_NonEnvExtensionProviderWinsOverProfile as a regression guard.
…cause Add a case where the underlying account-resolution error itself contains a secret marker, proving doResolveAccount's drop-the-cause design (§5.1) holds beyond the existing noop-keychain (empty-error) test, including across the full errors.Unwrap chain.
whoami reports facts about the effective identity; it should not proactively push profile-switching guidance at agents. That guidance lives in `profile --help` / the lark-shared skill, and failure recovery already lives in error hints. Remove the now-unused Suggestion field from IdentitySelection and its only setter/consumer.
…ofile_secret_invalid for broken default secret
…n-env persistence guidance
luozhixiong01
added
size/M
github-actions
Bot
added
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds profile-aware credential selection: bootstrap resolves the active profile from a flag or ChangesProfile-aware credential selection
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1776 +/- ##
==========================================
+ Coverage 74.43% 74.53% +0.09%
==========================================
Files 854 861 +7
Lines 88490 89390 +900
==========================================
+ Hits 65867 66624 +757
- Misses 17554 17631 +77
- Partials 5069 5135 +66 ☔ 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@94032da8623d02b76580a767cf24fa4bd17a03e5🧩 Skill updatenpx skills add larksuite/cli#feat/session-profile-selection -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: 2
🧹 Nitpick comments (2)
cmd/whoami/whoami_test.go (1)
338-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
cmdutil.TestFactoryhere.profileSelectionFactorycan take the shared factory setup, then overridef.Credentialwith the custom profile-aware provider. That keeps the test aligned with the rest of the suite and avoids duplicating boilerplate in this helper.🤖 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 `@cmd/whoami/whoami_test.go` around lines 338 - 374, `profileSelectionFactory` is hand-building a `cmdutil.Factory` instead of using the shared test helper. Switch this helper to start from `cmdutil.TestFactory` and then override the returned factory’s `Credential` with the custom `credential.NewCredentialProvider`/`WithProfile` setup, while keeping the existing config and IO stream tweaks. This keeps the test consistent with the rest of the suite and removes duplicated factory boilerplate.Source: Path instructions
internal/credential/credential_provider.go (1)
233-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate enrichment/warning logic — extract a helper.
The account-enrichment + warn-and-clear-on-failure block here (Lines 245-254) is duplicated almost verbatim at Lines 320-329 for the env-direct path. Consider extracting a shared helper (e.g.
p.enrichAndClearOnFailure(ctx, acct, source)) to avoid future divergence.♻️ Proposed refactor
+func (p *CredentialProvider) enrichOrClear(ctx context.Context, acct *Account, source credentialSource) { + if err := p.enrichUserInfo(ctx, acct, source); err != nil { + if p.warnOut != nil { + _, _ = fmt.Fprintf(p.warnOut, "warning: unable to verify user identity from credential source %q: %v\n", source.Name(), err) + } + acct.UserOpenId = "" + acct.UserName = "" + } +}Then replace both call sites (Lines 245-254 and 320-329) with
p.enrichOrClear(ctx, internal, source)/p.enrichOrClear(ctx, envAcct, envSource).🤖 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/credential/credential_provider.go` around lines 233 - 262, The account enrichment and warning/clear-on-failure logic in credential_provider.go is duplicated between the non-env provider path and the env-direct path. Extract that shared behavior from the credential selection flow in CredentialProvider into a helper (for example, on p as enrichOrClear or enrichAndClearOnFailure) that takes ctx, the converted account, and the source, calls enrichUserInfo, logs the warning through warnOut, and clears UserOpenId/UserName on failure. Then replace both existing call sites in the provider-selection logic with the helper so the two paths stay consistent.
🤖 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/bootstrap.go`:
- Around line 31-39: The profile bootstrap logic is using globals.Profile != ""
to detect whether --profile was provided, which incorrectly treats an explicit
empty value the same as no flag. Update the bootstrap path in the command setup
to use fs.Changed("profile") to determine whether the user passed the flag, and
only fall back to envvars.CliProfile when the flag was not set. Keep the
InvocationContext construction in sync so ProfileFromFlag reflects the explicit
flag state rather than the resolved value.
In `@internal/credential/credential_provider.go`:
- Around line 264-277: The explicit-profile branch in credential_provider.go is
swallowing the error from core.LoadMultiAppConfig and always turning it into
profile_not_found, which masks real config-loading failures. Update the logic
around p.profile, LoadMultiAppConfig, and the app == nil check so config load
errors are preserved and propagated instead of reported as a missing profile;
only return SubtypeProfileNotFound when the config loads successfully but
FindApp(p.profile) returns nil. Follow the pattern used in the later no-profile
branch by inspecting errs.ProblemOf(loadErr) when applicable and attaching the
original error with .WithCause(loadErr) so errors.Is and errors.Unwrap keep
working.
---
Nitpick comments:
In `@cmd/whoami/whoami_test.go`:
- Around line 338-374: `profileSelectionFactory` is hand-building a
`cmdutil.Factory` instead of using the shared test helper. Switch this helper to
start from `cmdutil.TestFactory` and then override the returned factory’s
`Credential` with the custom `credential.NewCredentialProvider`/`WithProfile`
setup, while keeping the existing config and IO stream tweaks. This keeps the
test consistent with the rest of the suite and removes duplicated factory
boilerplate.
In `@internal/credential/credential_provider.go`:
- Around line 233-262: The account enrichment and warning/clear-on-failure logic
in credential_provider.go is duplicated between the non-env provider path and
the env-direct path. Extract that shared behavior from the credential selection
flow in CredentialProvider into a helper (for example, on p as enrichOrClear or
enrichAndClearOnFailure) that takes ctx, the converted account, and the source,
calls enrichUserInfo, logs the warning through warnOut, and clears
UserOpenId/UserName on failure. Then replace both existing call sites in the
provider-selection logic with the helper so the two paths stay 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: e5965ff7-ac70-4f0e-ae37-05d2790a35c5
📒 Files selected for processing (20)
cmd/auth/status.gocmd/auth/status_test.gocmd/bootstrap.gocmd/bootstrap_test.gocmd/profile/profile.gocmd/profile/profile_test.gocmd/whoami/whoami.gocmd/whoami/whoami_test.goerrs/marshal_test.goerrs/subtypes.goerrs/types.goerrs/types_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/credential/credential_provider.gointernal/credential/credential_provider_selection_test.gointernal/credential/identity_selection.gointernal/credential/identity_selection_test.gointernal/envvars/envvars.goskills/lark-shared/SKILL.md
Replace credential-shaped literals in whoami and selection tests with placeholder values recognized by the public-content quality gate (test-secret / your-secret / your-password / your-access-token), so the deterministic public-content scan does not flag test fixtures as generic credentials. No behavioral change; the fixtures are only compared for non-leakage and identity arbitration.
Clarify that --profile and LARKSUITE_CLI_PROFILE accept either a profile name or an app_id, keep the effective-identity vs OAuth-token boundary (whoami vs auth status --json --verify), and note not to set direct app-credential env vars unless direct credentials are provided.
When an explicit profile was requested and LoadMultiAppConfig failed, the error was discarded and every failure reported as profile_not_found, masking a real config problem (e.g. malformed file) behind a misleading "run profile list" hint. Propagate the underlying error when it is a malformed-config failure (errors.Is ErrMalformedConfig) so errors.Is / errors.Unwrap keep working, mirroring the no-profile branch. An absent config is not malformed and still yields the friendly profile_not_found.
profile list / config show only report the saved default profile, not the app/profile a specific invocation actually resolves to (especially under --profile or LARKSUITE_CLI_PROFILE). Rename the misleading profile list JSON field active -> default (it is the configured default, not the one in effect), and point config show / profile list / profile help at lark-cli whoami --json for the identity actually used now. Restructure the lark-shared skill profile guidance as an intent -> command table.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
skills/lark-shared/SKILL.md (1)
129-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLanguage inconsistency in new "Profile 选择" section.
The section header is Chinese but the content on Line 131 is entirely English, unlike every other section in this document (auth table, update-check, security rules) which is written in Chinese. Consider translating this content for consistency with the rest of the skill guidance.
🤖 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 `@skills/lark-shared/SKILL.md` around lines 129 - 132, The new “Profile 选择” section is inconsistent because its guidance text is written in English while the surrounding skill documentation uses Chinese. Update the content in this section to Chinese for consistency, keeping the same meaning for profile selection, CLI flags, environment variables, saved config, default profile handling, and the ambiguity/credential note; use the existing “Profile 选择” heading and nearby sections as the style reference.
🤖 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.
Nitpick comments:
In `@skills/lark-shared/SKILL.md`:
- Around line 129-132: The new “Profile 选择” section is inconsistent because its
guidance text is written in English while the surrounding skill documentation
uses Chinese. Update the content in this section to Chinese for consistency,
keeping the same meaning for profile selection, CLI flags, environment
variables, saved config, default profile handling, and the ambiguity/credential
note; use the existing “Profile 选择” heading and nearby sections as the style
reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a654e00-7609-4bc0-9a4f-ac3211765632
📒 Files selected for processing (6)
cmd/config/config_test.gocmd/config/show.gocmd/profile/list.gocmd/profile/profile.gocmd/profile/profile_test.goskills/lark-shared/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- cmd/config/show.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/profile/profile.go
Match the rest of the skill document, which is in Chinese. Keep command and env-var tokens in English. Content unchanged.
Collaborator
Author
Addressed in 94032da — the |
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
1 participant
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.
Summary
Session-level profile selection for lark-cli. All identity inputs — the
--profileflag, the newLARKSUITE_CLI_PROFILEenv var, direct app-credential env (LARKSUITE_CLI_APP_ID/LARKSUITE_CLI_APP_SECRET), and the configured default — now resolve through one explainable path with app-id conflict detection and stable, machine-readable error codes. It also sharpens App/profile observability:whoami --jsonreports the identity a specific invocation actually resolves to, whileconfig show/profile listreport the saved default. This is an incremental change on top of the existing--profileflag,whoami, and profile commands.Changes
LARKSUITE_CLI_PROFILEenv var.BootstrapInvocationContextfalls back to it when--profileis absent, and records whether the active profile came from the flag or the env.CredentialProvider.doResolveAccount: precedence--profile>LARKSUITE_CLI_PROFILE> direct app env / configured default. When a profile and direct app-credential env are both present, compare app_id: matching app uses the profile; a mismatch fails instead of silently picking one. Actively-specified identities never fall back on failure. The resolution result is cached as an explainableIdentitySelection(no secret).profile_not_found,no_active_profile,app_credential_incomplete(config, exit 3),profile_app_credential_conflict(validation, exit 2), andprofile_secret_invalid. Secrets never appear in diagnostics or error envelopes.profile_not_foundwhen an explicit profile is requested (only a genuinely absent config keeps the friendlyprofile_not_found).whoami --jsonnow surfacescredentialSource,explicit, anddirectCredentialEnv.profile list --jsonrenames theactivefield todefault— it reports the saved default profile, not the identity currently in effect. Usewhoami --jsonfor the effective identity.whoami,auth status,profile,config show, andprofile listthatwhoami --jsonreports the effective app/profile for an invocation,auth status --json --verifyreports OAuth login/token state, andconfig show/profile listreport the saved config (not current usage).whoami; OAuth/token →auth status; per-command →--profile; same-shell script →LARKSUITE_CLI_PROFILE; saved config →config show/profile list; long-term default →profile use).Test Plan
make unit-test,go vet ./...,gofmt -l .(no output),go mod tidy(no change), andgolangci-lint run --new-from-rev=origin/main(0 issues).whoamidiagnostic fields, the malformed-config passthrough, and secret non-leakage (a broken-secret path asserts the raw secret never reaches the error envelope).cmd/configandcmd/profiletests updated for theactive→defaultfield rename and the clarified help.whoamioutput and error envelopes across--profile,LARKSUITE_CLI_PROFILE, direct app env, and configured-default sources.Related Issues
None.
Summary by CodeRabbit
whoami --jsonnow includes credential-selection context (credentialSource) plus whether direct app-credential environment values are present/matched.profile list --jsonnow usesdefault(replacing legacyactive) to indicate the saved default profile.auth status,profile,whoami, andconfig showhelp with clearer guidance; updated shared skill “profile-selection” scenarios and examples.