feat: profile selection from environment with source-aware errors - #2198
Merged
Conversation
A profile selector that matches no configured entry was answered by the generic "no active profile" — and config default-as / strict-mode steered agents toward `config init --new`, a destructive OAuth setup flow, while intact profiles sat in config.json. The environment variable form is the worst case: nothing in what the user just typed points at LARKSUITE_CLI_PROFILE. MultiAppConfig.RequireAppConfig now folds lookup + typed error into one seam. The error names the requested profile, carries the selector as the envelope field (--profile / LARKSUITE_CLI_PROFILE / currentApp), and scopes the hint to the actual fix; config init remains only for a genuinely empty config. auth list / auth logout stop disguising the input error as not_logged_in (exit 0 -> exit 3), the credential chain threads the selector source through ResolveConfigFromMulti, and doctor warns when an external credential provider silently ignores the selector.
profile list marked only the persisted default as active, profile use compared only persisted state, and config show reported only the resolved name — with a session-level LARKSUITE_CLI_PROFILE in play the three commands gave contradictory answers to "which profile am I on", and a successful profile use looked like it took effect while the environment kept overriding every subsequent command. Both dimensions are now visible wherever they can disagree, computed by one definition (MultiAppConfig.EffectiveProfile): profile list adds effective/effectiveSource beside the unchanged active field and warns on a dangling selector instead of silently marking nothing, profile use discloses on both its branches when the environment shadows the switch, and config show adds profileSource (config | flag | environment).
github-actions
Bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughProfile selection now follows flag, environment, and configuration precedence. The selected source flows through invocation, configuration, credentials, and commands. Missing profiles return typed errors. Profile and doctor commands report environment overrides and ignored selectors. ChangesProfile source resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BootstrapInvocationContext
participant Build
participant MultiAppConfig
participant DefaultAccountProvider
BootstrapInvocationContext->>BootstrapInvocationContext: Apply flag, environment, or config precedence
BootstrapInvocationContext->>Build: Pass InvocationContext with ProfileSource
Build->>MultiAppConfig: RequireAppConfig(profile, ProfileSource)
MultiAppConfig-->>DefaultAccountProvider: Resolve selected AppConfig
DefaultAccountProvider-->>Build: Return account provider
Build-->>BootstrapInvocationContext: Continue or return typed validation error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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`:
- Around line 275-280: Update cmd/doctor/doctor_test.go at lines 275-280 and
316-321 to initialize both test factories with cmdutil.TestFactory(t, config)
instead of direct cmdutil.Factory literals. Override only the dependencies
needed for each credential-provider scenario, and preserve isolation of
LARKSUITE_CLI_CONFIG_DIR.
In `@cmd/doctor/doctor.go`:
- Around line 138-145: The profile-selector warning in cmd/doctor/doctor.go must
only run when Invocation.ProfileSource is ProfileFromFlag or
ProfileFromEnvironment; update the condition around the profile_selector check
while preserving the existing selector-specific messaging. In
cmd/doctor/doctor_test.go, add a regression case for a persisted
currentApp/profile source with external credentials and assert that no
profile_selector check is produced.
In `@cmd/presentation_test.go`:
- Around line 673-681: Replace string-only stderr checks in
cmd/presentation_test.go lines 673-681 by decoding the JSON error envelope and
asserting its category, subtype, and parameter fields directly. In
cmd/config/config_test.go lines 135-148, use errs.ProblemOf to verify the
expected category and subtype while preserving the existing ConfigError.Field
assertion for currentApp.
In `@cmd/profile_env_integration_test.go`:
- Around line 119-135: Strengthen the profile-resolution error test around the
error-producing resolver by decoding the returned error envelope and asserting
errs.CategoryValidation and errs.SubtypeInvalidArgument via errs.ProblemOf, then
use errors.As to verify the *errs.ValidationError Param value and preserve any
wrapped lower-layer cause. Keep the existing integration assertions for rendered
guidance, but rely on typed error assertions rather than serialized substrings
for the contract.
In `@cmd/profile/profile_test.go`:
- Around line 687-722: Add a new `profileListRun` coverage case alongside
`TestProfileListRun_MarksEffectiveOverride` that exercises
`cmdutil.InvocationContext.ProfileSource` set to `core.ProfileFromFlag` with the
target profile selected by flag. Assert the same persisted/default vs override
split as the environment case: the default entry remains `Active` but not
`Effective`, the target entry is `Effective`, and `EffectiveSource` is "flag".
Reuse the existing `setupProfileConfigDir`, `core.SaveMultiAppConfig`, and
`profileListRun` flow so the test directly covers the `ProfileFromFlag` mapping.
In `@internal/core/notconfigured_test.go`:
- Around line 249-272: Update the error assertions in the RequireAppConfig test
cases around the existing errors.As checks to call errs.ProblemOf(err) and
verify the expected category and SubtypeNotConfigured subtype, while also
asserting the expected param where applicable. Preserve the existing
*errs.ConfigError checks for Field, message, and recovery-hint behavior,
including the prohibition on suggesting config init.
🪄 Autofix
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 Plus
Run ID: 7ff720a2-e570-4132-a73d-c91c68e36a2a
📒 Files selected for processing (27)
cmd/auth/list.gocmd/auth/logout.gocmd/bootstrap.gocmd/bootstrap_test.gocmd/build.gocmd/config/config_test.gocmd/config/default_as.gocmd/config/show.gocmd/config/strict_mode.gocmd/doctor/doctor.gocmd/doctor/doctor_test.gocmd/flag_gate.gocmd/presentation_test.gocmd/profile/list.gocmd/profile/profile_test.gocmd/profile/use.gocmd/profile_env_integration_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/core/config.gointernal/core/config_test.gointernal/core/notconfigured.gointernal/core/notconfigured_test.gointernal/core/types.gointernal/credential/default_provider.gointernal/credential/integration_test.gointernal/envvars/envvars.go
Comment on lines
+138
to
+145
| if f.Invocation.Profile != "" && cfg.ProfileName == "" { | ||
| selector := "--profile" | ||
| if f.Invocation.ProfileSource == core.ProfileFromEnvironment { | ||
| selector = envvars.CliProfile | ||
| } | ||
| checks = append(checks, warn("profile_selector", | ||
| fmt.Sprintf("%s=%q is ignored: credentials are provided externally", selector, f.Invocation.Profile), | ||
| fmt.Sprintf("unset %s, or remove the external credential variables to select accounts by profile", selector))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only warn for explicit selector sources.
Invocation.Profile also contains the persisted currentApp. With an external provider, this condition emits profile_selector and says --profile was ignored even when the user supplied no flag or environment selector.
Require ProfileFromFlag or ProfileFromEnvironment before adding this warning. Add a persisted-default regression case that asserts no profile_selector check.
cmd/doctor/doctor.go#L138-L145: gate the warning onProfileFromFlagorProfileFromEnvironment.cmd/doctor/doctor_test.go#L257-L302: add coverage for a persisted profile source with external credentials.
📍 Affects 2 files
cmd/doctor/doctor.go#L138-L145(this comment)cmd/doctor/doctor_test.go#L257-L302
🤖 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/doctor/doctor.go` around lines 138 - 145, The profile-selector warning in
cmd/doctor/doctor.go must only run when Invocation.ProfileSource is
ProfileFromFlag or ProfileFromEnvironment; update the condition around the
profile_selector check while preserving the existing selector-specific
messaging. In cmd/doctor/doctor_test.go, add a regression case for a persisted
currentApp/profile source with external credentials and assert that no
profile_selector check is produced.
Collaborator
Author
There was a problem hiding this comment.
The premise doesn't hold: BootstrapInvocationContext only populates Invocation.Profile for an explicit selector — the persisted-default path returns Profile == "" with ProfileFromConfig (cmd/bootstrap.go), so this warning cannot fire without a flag or environment selector. Added TestDoctorRun_NoSelectorWarningForPersistedDefault in 22c4e51 to pin that contract against future bootstrap changes.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline cmd/bootstrap.go --items all --type function --match 'BootstrapInvocationContext'
ast-grep outline cmd/doctor/doctor_test.go --items all --type function --match 'TestDoctorRun_NoSelectorWarningForPersistedDefault|TestDoctorRun_WarnsWhenExternalProviderIgnoresProfileSelector'
printf '\n--- Bootstrap implementation ---\n'
rg -n -A90 -B8 'func BootstrapInvocationContext\s*\(' cmd/bootstrap.go
printf '\n--- Persisted-default doctor regression test ---\n'
rg -n -A100 -B8 'func TestDoctorRun_NoSelectorWarningForPersistedDefault\s*\(' cmd/doctor/doctor_test.go
printf '\n--- Doctor warning condition ---\n'
rg -n -A22 -B7 'profile_selector' cmd/doctor/doctor.goLength of output: 6247
@liangshuo-1, verification confirms your explanation.
BootstrapInvocationContext returns an empty Profile with ProfileFromConfig. The warning requires a non-empty Profile. Therefore, a persisted default cannot produce profile_selector.
TestDoctorRun_NoSelectorWarningForPersistedDefault correctly protects this contract.
🧠 Learnings used
Learnt from: JayYoung2021
Repo: larksuite/cli PR: 410
File: cmd/auth/login_scope_cache.go:47-54
Timestamp: 2026-04-11T23:06:23.250Z
Learning: In larksuite/cli Go code, use `validate.SafeInputPath` only for user-supplied file path inputs (e.g., `--file` flags). Do NOT require `validate.SafeInputPath` for internally constructed, application-managed filesystem paths when: (1) their root directory is already validated via `validate.SafeEnvDirPath()` inside helpers like `appdir.CacheDir()`, `appdir.StateDir()`, or `appdir.LogDir()`, and (2) the filename/path components are sanitized by helpers such as `sanitizeLoginScopeCacheKey()`. In code reviews, missing `SafeInputPath` calls should not be flagged for these app-managed paths if the above safeguards are already in place.
Learnt from: JayYoung2021
Repo: larksuite/cli PR: 410
File: internal/update/update.go:147-147
Timestamp: 2026-04-11T23:06:28.717Z
Learning: In this repo’s Go code (larksuite/cli), treat `validate.SafeInputPath` as a guard for **user-supplied local file paths only**. When you build a path by joining a previously validated base directory (e.g., from `appdir.StateDir()`, `appdir.CacheDir()`, `appdir.ConfigDir()` which already validate env overrides via `validate.SafeEnvDirPath()`) with a compile-time constant filename (e.g., `"update-state.json"`, `"config.json"`), do **not** require an additional `validate.SafeInputPath` call before using `vfs.*` or `validate.AtomicWrite`. Reviewers should avoid flagging/adding redundant `validate.SafeInputPath` in these computed-path cases; only require it when the path ultimately comes from user input.
Learnt from: ViperCai
Repo: larksuite/cli PR: 450
File: shortcuts/slides/slides_create.go:50-60
Timestamp: 2026-04-14T03:13:09.459Z
Learning: When reviewing this Go codebase, do not require explicit `validate.SafeInputPath` / `validate.SafeOutputPath` calls for user-supplied paths if the code uses the `runtime.FileIO()` abstraction methods (`Stat`, `Open`, `Save`, `ResolvePath`). `runtime.FileIO()` is backed by `LocalFileIO` (in `localfileio.go`), which enforces `SafeInputPath`/`SafeOutputPath` before any filesystem access and reports violations as `fileio.PathValidationError`. Only require explicit `validate.SafeInputPath` when bypassing `runtime.FileIO()` and calling `vfs.*` or `os.*` directly with user-influenced paths.
Learnt from: evandance
Repo: larksuite/cli PR: 984
File: internal/lintcheck/scan_test.go:0-0
Timestamp: 2026-05-20T08:44:00.596Z
Learning: In Go, the "declared and not used" compiler error is triggered only for variables that are never referenced after declaration. A statement like `x++` counts as a use, so a variable that is only incremented (e.g., `rejectCount++`) will still compile even if it is never read. When reviewing Go code, don’t assume `x++`-only variables would fail to compile—treat them as potentially semantically dead code instead (remove/avoid unless it’s required for later reads, logging, metrics, or other side effects).
Learnt from: evandance
Repo: larksuite/cli PR: 1135
File: cmd/config/default_as.go:44-50
Timestamp: 2026-05-27T11:31:47.366Z
Learning: In migrated command files (the ones covered by the `errs-typed-only` forbidigo rule in `.golangci.yml`) where `RunE` is expected to use typed errors, `RunE` must return typed `errs.NewXxxError(...)` values directly (return the error value), and must not use `output.Errorf(...)` or `output.ErrWithHint(...)`. The root dispatcher (`cmd/root.go` → `handleRootError`) will route `errs.*` typed errors through `output.WriteTypedErrorEnvelope` and `output.ExitCodeOf`, so direct `errs.*` returns are the correct post-migration stderr JSON contract shape. Only non-migrated/legacy paths should continue to use `output.Errorf` / `output.ErrWithHint`. In migrated command files, do not flag direct `errs.*` returns from `RunE` as stderr JSON contract violations.
Learnt from: evandance
Repo: larksuite/cli PR: 1135
File: cmd/config/keychain_downgrade_other.go:23-25
Timestamp: 2026-05-27T11:31:49.884Z
Learning: For larksuite/cli command files under cmd/**/*.go (starting with migrated commands / PR `#1135` onward), ensure `RunE` returns typed errors directly: return values created by `errs.NewXxxError(...)` (e.g., `errs.NewValidationError`, `errs.NewInternalError`, `errs.NewConfigError`, etc.). Do not return `output.ErrXxx` or `output.ErrWithHint` from `RunE`. The command-layer error dispatcher (`cmd/root.go`’s `handleRootError`) should route `errs.TypedError` / `errs.ProblemOf` through `output.WriteTypedErrorEnvelope` and `output.ExitCodeOf` to produce the typed stderr JSON envelope. Treat direct `errs.NewXxxError` returns in `RunE` as compliant with the new stderr contract (do not flag them); the older pre-migration rule requiring `output.Errorf` / `output.ErrWithHint` no longer applies. This is enforced by the `.golangci.yml` `errs-typed-only` forbidigo rule for migrated paths.
Learnt from: evandance
Repo: larksuite/cli PR: 1135
File: cmd/config/keychain_downgrade.go:57-60
Timestamp: 2026-05-27T11:31:54.717Z
Learning: In larksuite/cli, for migrated code paths, RunE handlers should return typed errors directly (e.g., errs.NewInternalError / errs.NewValidationError / errs.NewAuthenticationError) rather than using the old output.Errorf / output.ErrWithHint pattern. This is the intended post-migration behavior introduced in PR `#1135` (typed envelope contract for auth-domain errors): cmd/root.go’s handleRootError detects errs.* typed errors and dispatches them through output.WriteTypedErrorEnvelope to produce the canonical stderr JSON envelope. Do not flag typed errs.* returns as violations of the old guideline; that old output.Errorf/ErrWithHint guideline applies only to un-migrated paths. The .golangci.yml forbidigo rule errs-typed-only defines which paths are migrated and must use typed errors exclusively—follow that rule when reviewing RunE handlers.
Learnt from: evandance
Repo: larksuite/cli PR: 1449
File: cmd/profile/rename.go:70-71
Timestamp: 2026-06-13T11:20:19.566Z
Learning: In larksuite/cli Go code, when wrapping errors returned by `core.SaveMultiAppConfig` and other local config persistence/save operations, use `errs.NewInternalError(errs.SubtypeStorage, ...)` instead of `errs.SubtypeFileIO`. In this codebase, `errs.SubtypeStorage` is the canonical subtype for “config file save”/local persistence failures, while `errs.SubtypeFileIO` is reserved for general file I/O operations. Do not treat `errs.SubtypeStorage` as an incorrect subtype for config-save paths during code review.
Learnt from: evandance
Repo: larksuite/cli PR: 1449
File: cmd/profile/list.go:49-49
Timestamp: 2026-06-13T11:20:17.330Z
Learning: In the larksuite/cli codebase, when migrating error handling for calls to `core.LoadMultiAppConfig`, don’t add typed passthrough guards like `errs.ProblemOf` / `errs.IsTyped` just to preserve/forward typed errors—`core.LoadMultiAppConfig` only returns raw (untyped) errors, and there’s no typed `errs.*` error to pass through. Also, if the failure-path subtype classification (e.g., `SubtypeFailedPrecondition` vs `SubtypeFileIO`) is already inconsistent as a pre-existing issue elsewhere, don’t block a migration-only PR on that mismatch; keep the PR focused on the migration.
Learnt from: evandance
Repo: larksuite/cli PR: 1449
File: cmd/profile/use.go:70-71
Timestamp: 2026-06-13T11:20:25.527Z
Learning: In larksuite/cli, when an error subtype represents internal persistence of configuration/state (e.g., saving multi-app config via core.SaveMultiAppConfig or similar “config file save” operations), use errs.SubtypeStorage rather than errs.SubtypeFileIO. errs.SubtypeStorage is intended for local persistence failures (config file save is the canonical case). Reserve errs.SubtypeFileIO for user-facing file read/write operations (e.g., reading a user-supplied --file path). Therefore, do not flag errs.SubtypeStorage usage when it occurs on config/state save paths—this is intentional and correct.
Learnt from: wanghm25
Repo: larksuite/cli PR: 2122
File: skills/lark-base/references/lark-base-dashboard-block-get-data.md:66-66
Timestamp: 2026-07-30T13:20:18.339Z
Learning: When reviewing Go code related to lark “base shortcuts” dashboard URL resolution and the `+dashboard-block-get-data` command, do not infer/require a `dsh*` prefix for dashboard IDs from mocked test fixtures. A dashboard should be treated as a top-level `Base` block: `shortcuts/base/base_resolve.go` assigns the selected block ID directly to `dashboard_id` for dashboard URLs. Also ensure `+dashboard-block-get-data` accepts `--dashboard-id` only as a compatibility argument (not as the source of truth for fetching chart data), and that chart data is read using `base_token + block_id`. Documentation/examples should use `blk_xxx` placeholders for `--dashboard-id`.
You are interacting with an AI system.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@22c4e5170c916bc287f2e1285ce700ffa6a10cca🧩 Skill updatenpx skills add larksuite/cli#feat/profile-provenance -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2198 +/- ##
==========================================
+ Coverage 76.00% 76.02% +0.02%
==========================================
Files 966 966
Lines 102541 102644 +103
==========================================
+ Hits 77933 78036 +103
+ Misses 18704 18702 -2
- Partials 5904 5906 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review follow-ups: build doctor test factories with cmdutil.TestFactory; decode stderr envelopes and assert category/subtype/param instead of matching serialized substrings; assert ProblemOf metadata at the profile resolver; cover the flag-selected effective profile in profile list; pin that a persisted default never triggers the external-provider selector warning.
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.
What
Profile selection can now come from the environment (
LARKSUITE_CLI_PROFILE), and every place that resolves a profile now knows which channel selected it — flag, environment, or the persisted default — so failures name the actual selector instead of guessing.Four commits, one functional unit:
feat: support profile selection from environment— resolve the env var at the process boundary (singleos.Getenvconsumption point in bootstrap), with precedence--profileflag > non-empty env > persistedcurrentApp. Restricted builds reject an environment-origin profile through the same distribution gate that already rejects the flag, before hooks or lifecycle events can observe the invocation.refactor: carry profile selection source in the invocation context—InvocationContextcarriesProfileSourcealongsideProfile; no behavior change on its own.fix: name the profile selector in resolution failures— when the selected profile doesn't resolve, errors state which selector chose it and list the available profiles, instead of the previous generic (and sometimes misleading) messages.feat: surface the effective profile beside the persisted default— status output distinguishes "what is persisted" from "what is in effect for this invocation".Contract changes (intentional, reviewer-relevant)
auth list/auth logoutwith a dangling persisted selector (currentApppointing at a profile that no longer exists): previously returned success-shaped output ("ok": true, empty users) at exit 0; now fail with a named config error at exit 3. The old behavior actively masked a broken default.config showadds aprofileSourcefield;profile listaddseffective/effectiveSourcefields. Purely additive — with no env var and no flag,config showoutput differs from main by exactly the one added field, andauth list/doctorare byte-identical.fieldkey naming the selector (existing envelope key, newly populated here).config init --newwhile valid profiles existed).Verification
go vet, and-race -count=1acrosscmd/internal/shortcuts/extensiongreen;golangci-lint run --new-from-rev=origin/mainclean.mainbinary: 824 help paths, fullschemaoutput (42k lines), and shell completions byte-identical; 62 populated-config scenarios acrossconfig/profile/auth/doctor/whoamiand 36 error paths audited — every difference traces to the contract changes listed above, none outside them.Summary by CodeRabbit