feat: add session-level profile selection and identity diagnostics#1776
Open
luozhixiong01 wants to merge 29 commits into
Open
feat: add session-level profile selection and identity diagnostics#1776luozhixiong01 wants to merge 29 commits into
luozhixiong01 wants to merge 29 commits into
Conversation
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 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.66% 74.82% +0.15%
==========================================
Files 877 879 +2
Lines 91731 92051 +320
==========================================
+ Hits 68494 68875 +381
+ Misses 17926 17856 -70
- Partials 5311 5320 +9 ☔ 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@00d8c07de9db0d873b0ec3cfce5de4d6f49589f9🧩 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
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
Collaborator
Author
Addressed in 94032da — the |
luozhixiong01
force-pushed
the
feat/session-profile-selection
branch
from
8b26c71 to
20571e0
Compare
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
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.
Match the rest of the skill document, which is in Chinese. Keep command and env-var tokens in English. Content unchanged.
The config-default (no-profile) path loaded config via LoadMultiAppConfig
and coerced any error into NotConfiguredError, so a malformed config was
misreported as no_active_profile ("run config init") — risking overwrite
of a recoverable-but-broken config. Route both the explicit-profile and
config-default paths through core.LoadOrNotConfigured so a malformed config
consistently surfaces as invalid_config (exit 3, cause preserved), while a
genuinely absent config keeps the friendly profile_not_found / no_active_profile.
luozhixiong01
force-pushed
the
feat/session-profile-selection
branch
from
20571e0 to
e6f8b8a
Compare
The description broadening to profile-selection triggers exceeded the routing fix's actual scope: the observed failure was command selection after the skill was already engaged, not skill discovery. Routing guidance stays in the body's profile-selection section and command help; the frontmatter matches main again.
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
Add session-level profile selection and make credential arbitration explicit and diagnosable. A selected profile can safely coexist with matching direct-credential environment variables, while incomplete credentials, conflicts, invalid policy values, missing profiles, and malformed configuration retain precise typed errors.
Changes
LARKSUITE_CLI_PROFILEselection with--profileprecedence incmd/bootstrap.goand expose the effective source throughwhoami --json.internal/credential/credential_provider.gointo explicit gather, decide, and execute stages so profile, direct environment credentials, managed providers, and config defaults use one decision table.extension/credential/types.go,errs/types.go, anderrs/subtypes.go, preserving safe causes without exposing secrets or tokens.LARKSUITE_CLI_DEFAULT_ASandLARKSUITE_CLI_STRICT_MODEbefore credential completeness checks inextension/credential/env/env.go.cmd/whoami,cmd/config, andcmd/profile;profile list --jsonnow reportsdefaultinstead ofactive.skills/lark-shared/SKILL.mdwith profile-selection routing guidance validated by the skill evaluation suite.Test Plan
make unit-test,go vet ./...,gofmt -l .,go mod tidy, andgolangci-lint run --new-from-rev=origin/mainpassedLARKSUITE_CLI_PROFILE=tenant_b ./lark-cli whoami --json --as botand the isolated matching/conflict/UAT-only matrix passed with a freshly built binaryRelated Issues
N/A