feat: add session-level profile selection and identity diagnostics#1776
Open
luozhixiong01 wants to merge 42 commits into
Open
feat: add session-level profile selection and identity diagnostics#1776luozhixiong01 wants to merge 42 commits into
luozhixiong01 wants to merge 42 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 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.66% 74.90% +0.23%
==========================================
Files 877 887 +10
Lines 91731 92959 +1228
==========================================
+ Hits 68494 69629 +1135
- Misses 17926 17957 +31
- Partials 5311 5373 +62 ☔ 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@588c27e79b7b822efffc2d1aac8cc2e066c044d4🧩 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
July 13, 2026 06:22
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
July 14, 2026 08:55
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.
…n errors Four external-review findings on the arbitration's error contract: - A typed resolver failure on the profile route (e.g. keychain key out of sync) now passes through with its own subtype and repair hint instead of being flattened into the generic profile_secret_invalid. Untyped failures and not_configured stay masked: their content is not guaranteed secret-free. - A config that exists but cannot be read (permission denied, I/O error) now loads as invalid_config with the real cause preserved, so it can no longer degrade into profile_not_found / no_active_profile and misdirect the repair. - Invalid LARKSUITE_CLI_DEFAULT_AS / LARKSUITE_CLI_STRICT_MODE values are classified at the env provider (BlockReasonInvalidPolicy) and translated into a typed validation error carrying param and a repair hint (exit 2), instead of falling through to the internal-error envelope (exit 5). - execute now refuses a profile account whose app_id differs from the arbitration snapshot, closing the read-twice race window with a deterministic error instead of silently using unchecked credentials. Also documents that identityInputs.directKeys describes only the builtin process-env credential surface, and makes the AccountKind switch exhaustive.
- config show now resolves the saved default profile unconditionally: its own help (and the skill routing) promise "saved config, not current usage", but the output still followed --profile / LARKSUITE_CLI_PROFILE. A behavior test locks the boundary the help text claims. - profile list dual-publishes the deprecated `active` field as an alias of `default` for one deprecation cycle, so external consumers reading `.active` keep working; it still marks the saved default, never the currently effective identity.
- buildCredentialProvider no longer records a phantom env profile source when no profile is selected. - ERROR_CONTRACT.md documents the credential/identity-selection extension fields (missing_keys, required_any_of, profile, app_id, credential_source, profile_app_id/env_app_id). - profile --help names the full auth status --json --verify form. - Drop a stale out-of-repo spec reference from the CredentialSource comment.
The out-of-sync check in ResolveConfigFromMulti returned subtype not_configured, which both the profile route and the config-default route deliberately mask behind generic errors — so the precise diagnosis (message naming the mismatch, hint naming the expected keychain key and the fix command) never reached the user. This was the review's F1: the typed-passthrough added earlier could not fire because the real production error carried the excluded subtype, and the test that claimed to cover it used a stub with a different subtype. Classify the inconsistency as invalid_config at the source: the config exists but is internally broken. Both routes now pass it through, which matches what main surfaced before the profile feature. The regression tests now drive the REAL DefaultAccountProvider against an out-of-sync keychain ref on both routes and assert the precise subtype, message, and hint; the stub-based test remains only as the generic any-typed-error contract check with neutral wording. Test fixtures also switch to recognized credential placeholders (the previous literal tripped the deterministic gate).
The account-level consistency check closed only half of the read-twice race (review F2): DefaultTokenProvider re-reads the config for both UAT and TAT, so a profile edit between account resolution and token resolution could still hand back a token minted for a different app, and TokenSpec.AppID was never consulted. Validate the resolved app against TokenSpec.AppID on both token paths. The TAT check runs on every call including cache hits, so a cached token can never serve a request that resolved a different app. The regression test drives the real DefaultAccountProvider + DefaultTokenProvider with a config swap in between; the stubbed HTTP client proves the check runs before any token work.
… the external gate - The engagement probe no longer counts an invalid_policy block as an external credential takeover (review F4): auth/config commands were answering "credentials are provided externally" for a mistyped LARKSUITE_CLI_DEFAULT_AS/STRICT_MODE, while whoami correctly said invalid_argument. The probe skips such blocks; the command's own resolution then surfaces the precise typed error. - config show overrides the parent gate entirely: it inspects the SAVED config only (its help promises "saved config, not current usage"), so the currently effective credential source must not decide whether it runs (review delivery finding). The blocked-commands pin drops show and a command-level test locks the bypass. - AccountDirect is now an enforced contract, not a comment (review F5): only the builtin env provider may declare it, and gather fails fast otherwise instead of producing self-contradictory diagnostics. The SPI documents the reservation.
Drop the dual-published deprecated `active` alias after review discussion: a compat field with no removal owner becomes permanent, it always mirrors `default` (inviting readers to hunt for a nonexistent difference), and its historic name keeps misleading agents into reading it as the currently effective identity — which is whoami's job. The rename is declared as a breaking change in the PR body; no in-repo consumer exists and no external consumer was identified.
The TAT path validated TokenSpec.AppID only AFTER doResolveTAT had re-read the config, minted a token with the (possibly different) app's secret, and cached it — so an A→B config race still produced a network mint, quota use, and audit trail for the wrong app before the result was refused (review F1). The account is now resolved and checked against the request before the sync.Once mint; doResolveTAT receives the already-checked account instead of re-reading; and the cached result stays re-checked on every hit. The regression test asserts the HTTP client is never even constructed for a mismatched request. TokenSpec.AppID is now REQUIRED on the default token paths (review F3): an empty value silently disabled the consistency guarantee. Every production caller already passes it via NewTokenSpec.
…with arbitration - The AccountDirect reservation is enforced by concrete type (*envprovider.Provider), not Name() == "env" (review F2): the registry reserves neither names nor uniqueness, so any provider could previously impersonate the builtin env provider with a name string. A regression test proves a name-forging AccountDirect provider is rejected. - The engagement probe now translates an invalid_policy block into the same typed validation error as formal arbitration (review F4), instead of skipping it and letting a later provider be reported as an external takeover. A multi-provider test pins probe/arbitration producing identical errors.
First resolve mints over HTTP, second is served from the sync.Once cache with exactly one HTTP call total.
… evidence
Two skill-eval failures on the final acceptance run, both fixed at the
smallest possible surface:
- A pure standing instruction ("run everything as tenant A from now
on") triggers no tool call, so only the frontmatter description is
visible — and the main description has no profile/tenant trigger
words, leaving the body's routing table unloaded (eval case 2, agent
answered with an unactionable promise). Add one 14-word clause:
pinning/clearing a profile/tenant identity for a task or session.
This partially revisits the earlier description revert, now backed by
a concrete discovery failure rather than speculation.
- The body routing table only routed the SET direction; add the reverse
entry: clearing the session identity -> unset LARKSUITE_CLI_PROFILE
(session-scoped, idempotent; never profile use / profile remove)
(eval case 4).
Skill eval rerun: 7/7 PASS.
- The canned TAT test token tripped the deterministic gate's public-content rule; switch to the recognized your-access-token placeholder (no assertion semantics change). - Drop the implementation mechanisms from the lark-shared frontmatter description (WHAT only; the body owns HOW) and tighten the clear-session-identity routing entry.
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
Adds session-level profile selection and identity diagnostics to lark-cli: a profile chosen via
--profile(one command) orLARKSUITE_CLI_PROFILE(shell/agent session) arbitrates against direct env credentials, and every identity question has exactly one authoritative command.Behavior
Selection order (all pinned by the pure-function decision table in
decide_test.go):credentialSource: "extension:<name>").app_id(even APP_ID-only env) → the profile supplies credentials AND tokens;app_id→profile_app_credential_conflict(exit 2), naming both app ids and the exact env vars to unset;app_id→app_credential_incompletewith the provider's real reason,missing_keys/required_any_of, and a context-aware hint.credentialSource: "env:LARKSUITE_CLI_APP_ID").config:currentApp→config:firstApp).Error contract (documented in
errs/ERROR_CONTRACT.md):BlockError.Code); the arbitration layer never re-reads env vars or parses messages to guess error types.app_credential_incompletecarriesmissing_keys(all required) orrequired_any_of(any one completes), names only — never values.LARKSUITE_CLI_DEFAULT_AS/LARKSUITE_CLI_STRICT_MODEare typed validation errors (exit 2,param+ repair hint) wherever identity resolution runs, including the auth/config gate probe. They are validated before credential completeness, so they can never be masked.invalid_configwith the real cause and repair hint; only a genuinely absent config degrades toprofile_not_found/no_active_profile.profile_secret_invalid(secret-hygiene guarantee, pinned by unwrap-chain leak tests).Command boundaries:
whoami --json— the app/profile lark-cli is using NOW (addscredentialSource,explicit,directCredentialEnv).auth status --json --verify— OAuth login/token state.config show/profile list— SAVED config only; both ignore the session profile, andconfig showbypasses the external-credential gate entirely.profile,config show, andprofile liststates these boundaries. Inskills/lark-shared/SKILL.md, the frontmatter routes task/session profile identity intents (WHAT); the body carries the operational commands (HOW).Consistency guarantees:
app_id; token resolution requiresTokenSpec.AppIDand checks it BEFORE any token work (no mint, no cache, no HTTP for a mismatched app), including TAT cache hits.AccountDirectis reserved for the builtin env provider, enforced by concrete type (unforgeable byName()).Breaking changes
profile list --jsonrenamesactivetodefault. The field marks the saved default profile, never the currently effective identity — usewhoami --jsonfor that. No in-repo consumer; no external consumer identified.LARKSUITE_CLI_DEFAULT_AS/LARKSUITE_CLI_STRICT_MODEnow exit 2 (validation/invalid_argument) instead of 5 (internal).whoamicredentialSourcegainsextension:<name>for managed extension providers;""now only means "not resolved".Test evidence (HEAD
588c27e7)docs/superpowers/verify_results/acceptance_review.md.docs/superpowers/verify_results/skillave_waiver.md).