fix: address code review issues in credential provider system#223
Closed
liangshuo-1 wants to merge 6 commits intomainfrom
Closed
fix: address code review issues in credential provider system#223liangshuo-1 wants to merge 6 commits intomainfrom
liangshuo-1 wants to merge 6 commits intomainfrom
Conversation
Converge scattered UAT/TAT/AppID/AppSecret acquisition into a unified CredentialProvider with enterprise plugin support. Architecture: - Credential-first Factory init: HttpClient → Credential → Config/LarkClient - Public extension API (extension/credential/) following database/sql driver pattern - Enterprise plugins via wrapper main.go + blank import (Caddy/CoreDNS pattern) - SDK token cache disabled — Credential manages all tokens - Direct HTTP for TAT/user_info — no circular dependency with LarkClient Key design decisions: - Provider interface: ResolveAccount(ctx) + ResolveToken(ctx, spec) - Flow control: nil,nil=skip / &result,nil=handle / nil,&BlockError=block - Account = core.CliConfig type alias (zero field copying) - TokenSpec struct for token parameters (not functional options) - enrichUserInfo mandatory when UAT available (security) - ProfileOverride late-binding via closure New packages: - extension/credential/ — public Provider interface, registry, TokenSpec, BlockError - extension/credential/env/ — built-in EnvProvider - internal/credential/ — CredentialProvider, DefaultAccountProvider, DefaultTokenProvider Change-Id: I62c7628ffe229b0b665f40bfb81b550e542ef45d Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Critical fixes: - Fix drive_io_test.go compilation (missing registerDriveBotTokenStub) - Fix mail_watch_test.go DryRun assertions (3 APIs, not 2) - Handle BlockError correctly in enrichUserInfo (respect chain-stop) - Reject --reset with value arg in strict-mode command - Warn when --global on has profile-level overrides Important fixes: - Change internal TokenType from int (iota) to string (match extension) - Replace os.Setenv/Unsetenv with t.Setenv in tests - Use utf8.RuneCountInString for profile name length validation - Handle json.Marshal error in TAT request body - Clean up old keychain secret when profile AppId changes - Move ctx to first param in checkServiceScopes (Go convention) - Fix profile add --use setting PreviousApp to self - Add sync.Once context behavior documentation - Remove duplicate comment in NewAPIClientWithConfig - Add UAT non-caching rationale comment Change-Id: I0dae93d15b6abf97a3dde5157d7bd6d0c6568700 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a pluggable credential system (extension providers + registry + defaults), profile-aware configuration and commands, strict-mode enforcement, credential-driven token resolution across CLI and SDK wiring, refactors media uploads to stream-based handling, and updates many tests to inject or rely on credential providers. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Factory
participant CredentialProvider
participant ExtProviders
participant DefaultProviders
participant HTTP
CLI->>Factory: start command (--profile, --as)
Factory->>Factory: ResolveAs (enforce strict-mode)
Factory->>CredentialProvider: ResolveAccount(ctx)
CredentialProvider->>ExtProviders: iterate ResolveAccount
alt ext provider returns account
ExtProviders-->>CredentialProvider: Account
else none
CredentialProvider->>DefaultProviders: ResolveAccount fallback
DefaultProviders-->>CredentialProvider: Account or error
end
CredentialProvider->>HTTP: maybe fetch user_info (needs UAT)
CredentialProvider->>ExtProviders: ResolveToken(spec for UAT)
alt ext provider returns UAT
ExtProviders-->>CredentialProvider: UAT + scopes
else
CredentialProvider->>DefaultProviders: ResolveToken(UAT)
DefaultProviders-->>CredentialProvider: UAT + scopes
end
CredentialProvider-->>Factory: CliConfig (account)
CLI->>CredentialProvider: ResolveToken(TokenSpec for API call)
CredentialProvider->>ExtProviders: iterate ResolveToken
alt ext provider returns token
ExtProviders-->>CredentialProvider: TokenResult
else
CredentialProvider->>DefaultProviders: ResolveToken fallback
DefaultProviders-->>CredentialProvider: TokenResult
end
CredentialProvider-->>CLI: TokenResult (value + scopes)
CLI->>HTTP: call API with token (Authorization)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
domain/base
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@0d69221233ef4152342309c7c64ace87bfbf7b28🧩 Skill updatenpx skills add larksuite/cli#fix/credential-provider-review-fixes -y -g |
Greptile SummaryThis PR addresses a comprehensive code review of the unified credential provider system introduced in Key improvements include:
One UX gap remains: Confidence Score: 5/5Safe to merge; all critical and important code-review issues from the original review have been addressed. All 6 Critical issues (drive test compilation, mail_watch assertions, BlockError in enrichUserInfo, strict-mode --reset conflict, --global on warning, json.Marshal error handling) are resolved. The remaining new finding — silent --as user override in strict mode — is a UX inconsistency (P2) that does not cause data loss or incorrect API calls. All other open items are P2 suggestions. internal/cmdutil/factory.go — the strict-mode override path in ResolveAs should emit a warning or error when an explicit --as flag is silently coerced to bot. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant Factory as cmdutil.Factory
participant CP as CredentialProvider
participant EnvP as env.Provider (extension)
participant DefAcct as DefaultAccountProvider
participant DefTok as DefaultTokenProvider
participant KC as Keychain/Config
CLI->>Factory: ResolveAs(cmd, flagAs)
Factory->>Factory: IsStrictMode()? → force AsBot if true
Factory-->>CLI: resolved identity
CLI->>Factory: Config()
Factory->>CP: ResolveAccount(ctx) [sync.Once]
CP->>EnvP: ResolveAccount(ctx)
alt LARK_APP_ID+SECRET set
EnvP-->>CP: &Account{...}
CP->>CP: enrichUserInfo() via LARK_USER_ACCESS_TOKEN
else env vars absent
EnvP-->>CP: nil, nil (skip)
CP->>DefAcct: ResolveAccount(ctx)
DefAcct->>KC: RequireConfigForProfile
KC-->>DefAcct: AppConfig
DefAcct-->>CP: *Account
end
CP-->>Factory: *Account (cached)
Factory-->>CLI: *core.CliConfig
CLI->>Factory: Credential.ResolveToken(ctx, TokenSpec{TAT|UAT})
Factory->>CP: ResolveToken(ctx, req)
CP->>EnvP: ResolveToken(ctx, spec)
alt env token set
EnvP-->>CP: &Token{Value}
CP-->>CLI: &TokenResult{Token, Scopes}
else
EnvP-->>CP: nil, nil (skip)
CP->>DefTok: ResolveToken(ctx, req)
alt TAT
DefTok->>DefTok: doResolveTAT [sync.Once] → HTTP POST
else UAT
DefTok->>DefTok: auth.GetValidAccessToken → keychain
end
DefTok-->>CP: *TokenResult
CP-->>CLI: *TokenResult
end
Reviews (4): Last reviewed commit: "fix: address bot review feedback (3 item..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+448
to
455
| // checkScopePrereqs performs a fast local check: does the token | ||
| // contain all scopes declared by the shortcut? Returns the missing ones. | ||
| // If scope data is unavailable, returns nil (let the API call handle it). | ||
| func checkScopePrereqs(f *cmdutil.Factory, ctx context.Context, appID string, identity core.Identity, required []string) []string { | ||
| result, err := f.Credential.ResolveToken(ctx, credential.NewTokenSpec(identity, appID)) | ||
| if err != nil || result == nil || result.Scopes == "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
checkScopePrereqs previously did a pure in-memory lookup (auth.GetStoredToken) with no side-effects. It is now replaced by a call to f.Credential.ResolveToken, which fully resolves the token — including making an HTTP request to the TAT endpoint for bot-mode or refreshing a UAT for user-mode on the very first invocation.
For bot mode, NewTokenSpec returns a TAT spec, which calls DefaultTokenProvider.resolveTAT → doResolveTAT, an HTTP POST to /open-apis/auth/v3/tenant_access_token/internal. The old code had an explicit as != core.AsUser guard in checkShortcutScopes that prevented any scope pre-check for bots; that guard was removed, so the TAT fetch now always fires for every shortcut invocation.
The TAT is cached via sync.Once so only the first invocation pays the cost, but the change still:
- Converts a zero-cost pre-flight check into a blocking network call on the first command run.
- Makes the scope check silently succeed (returns
nil) on any network error — so the check no longer does anything useful when the token endpoint is unreachable; previously it reliably checked the locally stored scopes.
Consider restoring the as != core.AsUser guard in checkShortcutScopes (bot tokens don't carry scope metadata, so the check is a no-op for bots) and keeping a fast-path for user mode that reads from auth.GetStoredToken before falling back to Credential.ResolveToken.
Comment on lines
+448
to
455
| // checkScopePrereqs performs a fast local check: does the token | ||
| // contain all scopes declared by the shortcut? Returns the missing ones. | ||
| // If scope data is unavailable, returns nil (let the API call handle it). | ||
| func checkScopePrereqs(f *cmdutil.Factory, ctx context.Context, appID string, identity core.Identity, required []string) []string { | ||
| result, err := f.Credential.ResolveToken(ctx, credential.NewTokenSpec(identity, appID)) | ||
| if err != nil || result == nil || result.Scopes == "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
ctx should be the first parameter per Go convention
Both checkScopePrereqs and checkShortcutScopes place ctx context.Context as the second parameter (after f *cmdutil.Factory). Go's official convention and the standard library consistently require context.Context to be the first parameter.
checkServiceScopes in cmd/service/service.go already follows the correct ordering (ctx context.Context first), creating an inconsistency between the two files.
Suggested change
| // checkScopePrereqs performs a fast local check: does the token | |
| // contain all scopes declared by the shortcut? Returns the missing ones. | |
| // If scope data is unavailable, returns nil (let the API call handle it). | |
| func checkScopePrereqs(f *cmdutil.Factory, ctx context.Context, appID string, identity core.Identity, required []string) []string { | |
| result, err := f.Credential.ResolveToken(ctx, credential.NewTokenSpec(identity, appID)) | |
| if err != nil || result == nil || result.Scopes == "" { | |
| return nil | |
| } | |
| func checkScopePrereqs(ctx context.Context, f *cmdutil.Factory, appID string, identity core.Identity, required []string) []string { |
The same fix applies to checkShortcutScopes:
// before
func checkShortcutScopes(f *cmdutil.Factory, ctx context.Context, as core.Identity, ...)
// after
func checkShortcutScopes(ctx context.Context, f *cmdutil.Factory, as core.Identity, ...)
Comment on lines
71
to
+75
| } | ||
| return f, stdoutBuf, stderrBuf, reg | ||
| } | ||
|
|
||
| type testDefaultAcct struct { |
There was a problem hiding this comment.
testDefaultToken returns empty Scopes for all types — may mask scope-related test failures
testDefaultToken.ResolveToken returns a TokenResult with an empty Scopes field unconditionally for both UAT and TAT. Because Scopes is always empty, every call to checkScopePrereqs / checkShortcutScopes / checkServiceScopes will short-circuit at the result.Scopes == "" guard and skip the scope check entirely.
Tests that exercise scope pre-checking logic will silently pass even when the scope-check code is broken, because the stub never populates scope data.
Consider extending the stub to accept configurable scope data so individual tests can inject realistic values when exercising scope guards:
type testDefaultToken struct {
scopes string // empty by default; set in scope-related tests
}This would make scope-guard tests self-contained and explicit about what they are testing.
Comment on lines
+116
to
+133
| func (p *CredentialProvider) ResolveToken(ctx context.Context, req TokenSpec) (*TokenResult, error) { | ||
| for _, prov := range p.providers { | ||
| tok, err := prov.ResolveToken(ctx, extcred.TokenSpec{ | ||
| Type: extcred.TokenType(req.Type), | ||
| Identity: string(req.Identity), | ||
| AppID: req.AppID, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if tok != nil { | ||
| return &TokenResult{Token: tok.Value, Scopes: tok.Scopes}, nil | ||
| } | ||
| } | ||
| if p.defaultToken != nil { | ||
| return p.defaultToken.ResolveToken(ctx, req) | ||
| } | ||
| return nil, fmt.Errorf("no credential provider returned a token for %s", req.Type) |
There was a problem hiding this comment.
BlockError handled differently in ResolveToken vs enrichUserInfo
In enrichUserInfo, a BlockError from a provider is explicitly treated as "skip enrichment and return success":
if errors.As(err, &blockErr) {
return nil // provider explicitly blocks UAT; skip enrichment
}In ResolveToken, any error (including BlockError) immediately propagates as a hard failure. This asymmetry is intentional per the design, but is undocumented at the ResolveToken call-site. A brief inline comment clarifying the intended semantics would reduce ambiguity for future provider authors:
if err != nil {
// BlockError propagates as a hard failure — it stops the chain
// and signals to the caller that the provider actively denied access.
return nil, err
}Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+74
to
+82
| } | ||
| boolVal := value == "on" | ||
|
|
||
| if global { | ||
| multi.StrictMode = boolVal | ||
| if boolVal { | ||
| for _, a := range multi.Apps { | ||
| if a.StrictMode != nil && !*a.StrictMode { | ||
| fmt.Fprintf(f.IOStreams.ErrOut, "Warning: profile %q has strict-mode explicitly set to off, which overrides the global setting. Use `lark-cli config strict-mode --reset` in that profile to inherit global.\n", a.ProfileName()) |
There was a problem hiding this comment.
--global off does not warn about profile-level on overrides
When setting --global on, the code correctly warns about profiles that have StrictMode explicitly set to off. However, the symmetric case is unhandled: setting --global off when some profiles have StrictMode explicitly set to on produces no warning, even though those profiles will remain in strict mode.
} else if !boolVal {
for _, a := range multi.Apps {
if a.StrictMode != nil && *a.StrictMode {
fmt.Fprintf(f.IOStreams.ErrOut,
"Warning: profile %q has strict-mode explicitly set to on, which overrides the global setting.\n",
a.ProfileName())
}
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (4)
shortcuts/mail/mail_watch_test.go (1)
99-100: Reinstate method assertion for the profile dry-run call.Line 99 now checks only URL, so a method regression on the profile endpoint would pass unnoticed. Please keep the explicit GET assertion for this API step as well.
Suggested patch
+ if apis[1].Method != "GET" { + t.Fatalf("unexpected profile method: %s", apis[1].Method) + } if apis[1].URL != mailboxPath("me", "profile") { t.Fatalf("unexpected profile url: %s", apis[1].URL) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_watch_test.go` around lines 99 - 100, The test removed the HTTP method check for the profile dry-run step; restore an explicit assertion that apis[1].Method == "GET" alongside the existing URL check so a method regression on the profile endpoint is caught. Locate the assertion that currently checks apis[1].URL against mailboxPath("me", "profile") and add a sibling assertion verifying the Method field equals "GET" (consistent with other API step checks in this test) so the profile dry-run call is validated for both URL and method.shortcuts/im/helpers_network_test.go (1)
29-33: Assert the requested credential spec in the stub.This resolver returns a token for any
credential.TokenSpec, so these tests stop detecting regressions where the IM runtime asks for the wrong identity or token type. Capture the incoming spec and fail on unexpected values to keep the new credential wiring covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/helpers_network_test.go` around lines 29 - 33, The staticShortcutTokenResolver stub currently returns the same token regardless of the incoming credential.TokenSpec, which hides regressions; update the stub (staticShortcutTokenResolver and its ResolveToken method) to capture the incoming credential.TokenSpec and assert it matches an expected spec (e.g., store an expected spec or validator function on the resolver struct), and return an error when it does not match; use a deterministic comparison (reflect.DeepEqual or explicit field checks) so tests fail when the IM runtime requests the wrong identity/token type before returning the TokenResult.internal/client/client_test.go (1)
37-42: Make the token stub verify the requested spec.Returning
"test-token"for everycredential.TokenSpecmeans these tests won't catch regressions where the client asks for the wrong identity or token flavor. Recording the incoming spec and asserting it in the helper would keep the credential-path coverage meaningful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/client_test.go` around lines 37 - 42, The staticTokenResolver::ResolveToken helper currently returns the same token for every credential.TokenSpec, so update staticTokenResolver to store an expected credential.TokenSpec (add a field like expectedSpec) and in ResolveToken compare the incoming spec against that expectedSpec (or record the received spec into a field like receivedSpec and return an error if it differs) before returning the token; update tests to construct staticTokenResolver with the expected spec and assert no mismatch occurred.internal/client/client.go (1)
34-40: Add a nil guard before dereferencingCredentialinDoSDKRequest().
Credentialis not initialized in the emptyAPIClient{}composite literal pattern. While the production factory properly populates it, adding an early nil check prevents panics on misuse and provides a clearer error message.Proposed fix
func (c *APIClient) DoSDKRequest(ctx context.Context, req *larkcore.ApiReq, as core.Identity, extraOpts ...larkcore.RequestOptionFunc) (*larkcore.ApiResp, error) { var opts []larkcore.RequestOptionFunc + if c.Credential == nil { + return nil, output.Errorf(output.ExitInternal, "internal", "credential provider is not configured") + } result, err := c.Credential.ResolveToken(ctx, credential.NewTokenSpec(as, c.Config.AppID)) if err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/client.go` around lines 34 - 40, DoSDKRequest currently dereferences APIClient.Credential without checking for nil; add a nil guard at the start of the APIClient.DoSDKRequest method to detect when c.Credential is nil and return a descriptive error instead of panicking. Specifically, inside the DoSDKRequest receiver (APIClient), check if c == nil or c.Credential == nil and return an error like "missing credential provider on APIClient" (or wrap with fmt.Errorf) before using c.Credential so callers receive a clear failure instead of a nil pointer panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/login.go`:
- Around line 108-111: The code currently assumes config.ProfileName resolves
via core.LoadMultiAppConfig() and multi.FindApp(config.ProfileName) and silently
proceeds when app == nil, skipping Users overwrite and token cleanup and
defaulting lang to "zh"; change this so you either (1) resolve the target app
with a robust fallback—try finding by AppID, fall back to the "current" app in
multi config, or use a sensible default profile—then proceed with Users
overwrite and old-token cleanup, or (2) if no fallback is appropriate,
return/propagate an explicit error instead of continuing; update the blocks
using core.LoadMultiAppConfig(), multi.FindApp(config.ProfileName), and any
logic that sets lang or updates Users so they only skip when a clear fallback is
applied or an error is returned (also apply same fix to the other occurrences
around the indicated ranges).
In `@cmd/config/init.go`:
- Around line 120-129: When updating an existing profile where the AppId changes
(in the multi.Apps[idx] update block), also reset stored user state to avoid
carrying users/tokens from the previous app: set multi.Apps[idx].Users to an
empty slice or nil after changing AppId/AppSecret (and ensure any secret cleanup
via core.RemoveSecretStore remains). Mirror this same Users reset in the other
profile-update branch that handles the unchanged-secret path so both update
paths clear orphaned user state when AppId changes.
In `@cmd/config/strict_mode_test.go`:
- Around line 90-107: The setup step in TestStrictMode_Reset is not validated:
capture and check the error from the initial
NewCmdConfigStrictMode(...).SetArgs([]string{"on"}).Execute() call and assert
that the profile override was applied (load the config via
core.LoadMultiAppConfig() and assert app.StrictMode != nil && *app.StrictMode ==
true) before running the reset; then run the reset command and keep the existing
post-reset assertion. Reference NewCmdConfigStrictMode, cmd.Execute,
core.LoadMultiAppConfig, and app.StrictMode to locate and change the test.
- Around line 14-29: The shared test setup function setupStrictModeTestConfig
should clear the LARKSUITE_CLI_STRICT_MODE environment so tests don't inherit
the runner's setting; update setupStrictModeTestConfig to call
t.Setenv("LARKSUITE_CLI_STRICT_MODE", "") (or otherwise unset it) at the start
of the helper, leaving explicit tests (e.g., the override test) to set the
variable themselves.
In `@cmd/config/strict_mode.go`:
- Around line 47-57: The branch handling reset currently allows a simultaneous
--global flag and silently ignores it; add an explicit check at the start of the
reset branch to fail fast if both flags are present (e.g. if reset && global)
and return a validation error (similar to existing output.ErrValidation) so no
profile-scoped write occurs. Update the reset handling in the same function that
references app.StrictMode, core.SaveMultiAppConfig and f.IOStreams.ErrOut to
perform this check before clearing app.StrictMode or saving multi, returning an
error message like "--reset cannot be used with --global".
In `@cmd/profile/add.go`:
- Around line 72-75: The current handling around core.LoadMultiAppConfig()
silently converts any load error into an empty core.MultiAppConfig, risking
overwriting real configs; change it so you only initialize multi =
&core.MultiAppConfig{} when the error explicitly indicates "config not found"
(e.g. errors.Is(err, os.ErrNotExist) or a package sentinel like
core.ErrNotFound), otherwise return or surface the error (propagate it up from
the command) so parse/read failures are not swallowed; update the logic around
the multi variable and error handling in the Add profile path (the call to
core.LoadMultiAppConfig) to use errors.Is checks and fail fast for non-missing
errors.
In `@cmd/profile/list.go`:
- Around line 40-43: The code in cmd/profile/list.go currently treats any error
from core.LoadMultiAppConfig() as "not configured"; change this to only
special-case the explicit missing/uninitialized sentinel (e.g.,
core.ErrNotConfigured or whatever sentinel the core package exposes) by using
errors.Is(err, core.ErrNotConfigured) or the core-provided check, print the init
hint in that case, and for any other error propagate or return the original err
(or print the real error to ErrOut) so corrupted/unreadable config errors are
not swallowed.
In `@cmd/profile/remove.go`:
- Around line 49-70: Move the config mutation and call to
core.SaveMultiAppConfig(multi) to occur before any credential/keychain deletion
so we never remove secrets unless the profile removal is persisted;
specifically, after removing the app from multi.Apps and updating
multi.CurrentApp/multi.PreviousApp, call core.SaveMultiAppConfig(multi) and only
if it returns nil proceed to call core.RemoveSecretStore(app.AppSecret,
f.Keychain) and larkauth.RemoveStoredToken(app.AppId, user.UserOpenId) for each
user. Ensure you still return the save error immediately if SaveMultiAppConfig
fails and do not delete credentials in that error path.
In `@extension/credential/env/env_test.go`:
- Around line 31-63: Add a small helper (e.g., clearLarkEnv(t *testing.T)) that
clears the relevant env vars before each test by calling t.Setenv("LARK_APP_ID",
"") , t.Setenv("LARK_APP_SECRET", "") and t.Setenv("LARK_BRAND", "") (or
os.Unsetenv if you prefer) and invoke it at the start of
TestResolveAccount_NeitherSet, TestResolveAccount_OnlyIDSet,
TestResolveAccount_OnlySecretSet, TestResolveAccount_DefaultBrand (and the other
test around lines 87-92) so ResolveAccount (Provider.ResolveAccount) runs with a
clean environment.
In `@internal/cmdutil/factory_default.go`:
- Around line 72-83: The http.Client created for the Lark SDK lacks a Timeout
and can hang indefinitely; update the client passed to lark.WithHttpClient to
include a timeout (e.g. Timeout: 30 * time.Second) so SDK-backed requests mirror
the CLI's 30s timeout, ensuring you import time and keep the existing Transport
(UserAgentTransport and auth.SecurityPolicyTransport) and CheckRedirect
(safeRedirectPolicy) configuration intact.
- Around line 131-135: The fallback token resolution calls
DefaultTokenProvider.resolveUAT/resolveTAT which use
p.defaultAcct.ResolveAccount() (only keychain/config), so change construction to
ensure token fallback uses the aggregated account chain: create the aggregated
CredentialProvider first (using extcred.Providers()), then pass that
CredentialProvider into the DefaultTokenProvider (or modify DefaultTokenProvider
to accept and call credentialProvider.ResolveAccount() rather than
p.defaultAcct.ResolveAccount()). Update buildCredentialProvider to construct
credential.NewCredentialProvider(...) before creating defaultToken (or adjust
parameter order) so ResolveToken falls back via
CredentialProvider.ResolveAccount() consistent with
CredentialProvider.ResolveAccount().
In `@internal/cmdutil/factory.go`:
- Around line 121-124: The IsStrictMode method currently treats any non-empty
env value that isn't exactly "true" or "1" as false, silently disabling strict
mode; update Factory.IsStrictMode to normalize the env value (trim whitespace
and toLower) and accept a small explicit set of truthy values (e.g., "true",
"1", "yes", "on") and falsy values (e.g., "false", "0", "no", "off"); if the env
is set but not in either set, DO NOT return false—fall back to the factory's
configured strict setting (the existing config field on Factory, e.g.,
f.config.StrictMode or equivalent) so invalid env values do not silently
downgrade the policy.
In `@internal/core/config.go`:
- Around line 66-84: CurrentAppConfig currently ignores the
LARKSUITE_CLI_PROFILE env while RequireConfigForProfile applies it separately;
centralize the effective-profile resolution so both policy checks and config
loading use the same value by updating CurrentAppConfig (or adding a shared
resolver like ResolveEffectiveProfile) to consult LARKSUITE_CLI_PROFILE first,
then profileOverride, then m.CurrentApp, then fallback to m.Apps[0]; adjust
callers (e.g., RequireConfigForProfile) to use this single resolver instead of
applying the env fallback themselves so strict-mode paths and config lookups
target the same profile.
In `@internal/credential/credential_provider_test.go`:
- Around line 11-24: TestCredentialProvider_AccountCached currently doesn't
validate caching because mockExtProvider.ResolveAccount always returns the same
pointer; update the test to either (a) add a call counter field on
mockExtProvider and increment it inside ResolveAccount then assert the counter
== 1 after two ResolveAccount calls, or (b) change ResolveAccount to return a
newly allocated *extcred.Account each invocation and assert the provider's
invocation count is 1; reference mockExtProvider and its ResolveAccount method
and the TestCredentialProvider_AccountCached test to implement the
counter/assertion change (also apply the same fix to the similar assertions
around lines 90-99).
In `@internal/credential/credential_provider.go`:
- Around line 87-95: The loop over p.providers incorrectly calls
prov.ResolveToken(ctx, extcred.TokenSpec{Type: extcred.TokenTypeUAT}) without
identifying which account/app/user to resolve and it swallows all non-BlockError
errors; change the ResolveToken call to populate TokenSpec with the
account/app/user context used for enrichment (so multi-account providers can
disambiguate), and change the error handling so that: if errors.As(err,
&extcred.BlockError{}) then continue (skip), if err != nil return err (surface
real provider failures), and if err==nil && tok==nil continue (provider
skipped). Update references in this block to p.providers, ResolveToken,
extcred.TokenSpec, TokenTypeUAT, and extcred.BlockError accordingly.
In `@internal/credential/integration_test.go`:
- Around line 43-61: TestFullChain_Fallthrough assumes envprovider.Provider
returns no creds but doesn't clear process env; ensure the test unsets or clears
LARK_APP_ID, LARK_APP_SECRET, and LARK_USER_ACCESS_TOKEN before creating ep :=
&envprovider.Provider{} so the env provider cannot win. Modify
TestFullChain_Fallthrough to call os.Unsetenv (or equivalent) for those three
env names at the start and optionally restore previous values after the test,
then proceed to construct credential.NewCredentialProvider and assert the mock
default token path.
In `@internal/credential/user_info.go`:
- Around line 52-55: The user_info handler currently treats API responses with
Code==0 as success even if result.Data.OpenID is empty; update the logic in the
function that constructs and returns &userInfo{OpenID: result.Data.OpenID, Name:
result.Data.Name} to validate result.Data.OpenID is non-empty and return an
error (e.g., fmt.Errorf("user_info missing open_id")) when it is empty so
malformed payloads are rejected before updating accounts; reference the result,
result.Data.OpenID and userInfo construction in your change.
In `@shortcuts/common/runner.go`:
- Around line 92-96: The code calls ctx.Factory.Credential.ResolveToken(...) and
immediately dereferences result.Token, which can panic if ResolveToken returns
(nil, nil); add a nil-check after the call (e.g., if result == nil) and return a
proper auth error via output.ErrAuth with a clear message (including context
like AppID) instead of dereferencing; update the return path in the function
that contains this call so it safely handles a nil result from ResolveToken
before accessing result.Token.
---
Nitpick comments:
In `@internal/client/client_test.go`:
- Around line 37-42: The staticTokenResolver::ResolveToken helper currently
returns the same token for every credential.TokenSpec, so update
staticTokenResolver to store an expected credential.TokenSpec (add a field like
expectedSpec) and in ResolveToken compare the incoming spec against that
expectedSpec (or record the received spec into a field like receivedSpec and
return an error if it differs) before returning the token; update tests to
construct staticTokenResolver with the expected spec and assert no mismatch
occurred.
In `@internal/client/client.go`:
- Around line 34-40: DoSDKRequest currently dereferences APIClient.Credential
without checking for nil; add a nil guard at the start of the
APIClient.DoSDKRequest method to detect when c.Credential is nil and return a
descriptive error instead of panicking. Specifically, inside the DoSDKRequest
receiver (APIClient), check if c == nil or c.Credential == nil and return an
error like "missing credential provider on APIClient" (or wrap with fmt.Errorf)
before using c.Credential so callers receive a clear failure instead of a nil
pointer panic.
In `@shortcuts/im/helpers_network_test.go`:
- Around line 29-33: The staticShortcutTokenResolver stub currently returns the
same token regardless of the incoming credential.TokenSpec, which hides
regressions; update the stub (staticShortcutTokenResolver and its ResolveToken
method) to capture the incoming credential.TokenSpec and assert it matches an
expected spec (e.g., store an expected spec or validator function on the
resolver struct), and return an error when it does not match; use a
deterministic comparison (reflect.DeepEqual or explicit field checks) so tests
fail when the IM runtime requests the wrong identity/token type before returning
the TokenResult.
In `@shortcuts/mail/mail_watch_test.go`:
- Around line 99-100: The test removed the HTTP method check for the profile
dry-run step; restore an explicit assertion that apis[1].Method == "GET"
alongside the existing URL check so a method regression on the profile endpoint
is caught. Locate the assertion that currently checks apis[1].URL against
mailboxPath("me", "profile") and add a sibling assertion verifying the Method
field equals "GET" (consistent with other API step checks in this test) so the
profile dry-run call is validated for both URL and method.
🪄 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: 30f71a27-ce1c-4479-bfca-26897298bdfc
📒 Files selected for processing (66)
cmd/api/api.gocmd/api/api_test.gocmd/auth/list.gocmd/auth/login.gocmd/auth/login_strict_test.gocmd/auth/logout.gocmd/config/config.gocmd/config/default_as.gocmd/config/init.gocmd/config/init_interactive.gocmd/config/show.gocmd/config/strict_mode.gocmd/config/strict_mode_test.gocmd/profile/add.gocmd/profile/list.gocmd/profile/profile.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/root.gocmd/root_e2e_test.gocmd/service/service.gocmd/service/service_test.goextension/credential/env/env.goextension/credential/env/env_test.goextension/credential/registry.goextension/credential/registry_test.goextension/credential/types.gointernal/client/client.gointernal/client/client_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_test.gointernal/cmdutil/testing.gointernal/core/config.gointernal/core/config_strict_mode_test.gointernal/core/types.gointernal/credential/credential_provider.gointernal/credential/credential_provider_test.gointernal/credential/default_provider.gointernal/credential/default_provider_test.gointernal/credential/integration_test.gointernal/credential/types.gointernal/credential/types_test.gointernal/credential/user_info.gomain.goshortcuts/base/base_advperm_test.goshortcuts/base/base_dashboard_execute_test.goshortcuts/base/base_execute_test.goshortcuts/base/base_form_execute_test.goshortcuts/base/base_role_test.goshortcuts/base/workflow_execute_test.goshortcuts/calendar/calendar_test.goshortcuts/common/runner.goshortcuts/doc/doc_media_test.goshortcuts/drive/drive_io_test.goshortcuts/im/convert_lib/helpers_test.goshortcuts/im/convert_lib/merge_test.goshortcuts/im/convert_lib/runtime_test.goshortcuts/im/convert_lib/thread_test.goshortcuts/im/coverage_additional_test.goshortcuts/im/helpers_network_test.goshortcuts/im/im_messages_search_execute_test.goshortcuts/mail/mail_watch_test.goshortcuts/vc/vc_notes.goshortcuts/vc/vc_notes_test.go
💤 Files with no reviewable changes (17)
- shortcuts/vc/vc_notes_test.go
- shortcuts/doc/doc_media_test.go
- shortcuts/im/convert_lib/helpers_test.go
- shortcuts/base/base_dashboard_execute_test.go
- shortcuts/im/convert_lib/thread_test.go
- shortcuts/im/convert_lib/merge_test.go
- shortcuts/base/base_advperm_test.go
- shortcuts/im/coverage_additional_test.go
- cmd/api/api_test.go
- shortcuts/base/base_role_test.go
- shortcuts/base/base_form_execute_test.go
- shortcuts/calendar/calendar_test.go
- shortcuts/im/im_messages_search_execute_test.go
- shortcuts/base/base_execute_test.go
- cmd/root_e2e_test.go
- cmd/service/service_test.go
- shortcuts/base/workflow_execute_test.go
Comment on lines
+108
to
+111
| if multi, _ := core.LoadMultiAppConfig(); multi != nil { | ||
| if app := multi.FindApp(config.ProfileName); app != nil { | ||
| lang = app.Lang | ||
| } |
There was a problem hiding this comment.
Don't silently skip profile bookkeeping when config.ProfileName can't be resolved.
These new lookups assume the profile name is always populated and still present in the config. If it is blank or stale, login still stores the token but skips Users overwrite and old-token cleanup while reporting success; the same lookup also silently falls back to "zh" for UI copy. Please resolve the target app with a fallback (for example, current app or AppID match) or return an error instead of continuing on app == nil.
Also applies to: 314-325, 396-407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 108 - 111, The code currently assumes
config.ProfileName resolves via core.LoadMultiAppConfig() and
multi.FindApp(config.ProfileName) and silently proceeds when app == nil,
skipping Users overwrite and token cleanup and defaulting lang to "zh"; change
this so you either (1) resolve the target app with a robust fallback—try finding
by AppID, fall back to the "current" app in multi config, or use a sensible
default profile—then proceed with Users overwrite and old-token cleanup, or (2)
if no fallback is appropriate, return/propagate an explicit error instead of
continuing; update the blocks using core.LoadMultiAppConfig(),
multi.FindApp(config.ProfileName), and any logic that sets lang or updates Users
so they only skip when a clear fallback is applied or an error is returned (also
apply same fix to the other occurrences around the indicated ranges).
Comment on lines
+90
to
+107
| // First set it on | ||
| cmd := NewCmdConfigStrictMode(f) | ||
| cmd.SetArgs([]string{"on"}) | ||
| cmd.Execute() | ||
|
|
||
| // Then reset | ||
| cmd = NewCmdConfigStrictMode(f) | ||
| cmd.SetArgs([]string{"--reset"}) | ||
| err := cmd.Execute() | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| multi, _ := core.LoadMultiAppConfig() | ||
| app := multi.CurrentAppConfig("") | ||
| if app.StrictMode != nil { | ||
| t.Errorf("expected nil StrictMode after reset, got %v", *app.StrictMode) | ||
| } |
There was a problem hiding this comment.
TestStrictMode_Reset can pass without proving reset works.
The initial "on" command ignores its error and there is no intermediate assertion, so a broken setup step or a no-op reset still leaves StrictMode == nil and this test passes. Assert the profile override became true before running --reset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/strict_mode_test.go` around lines 90 - 107, The setup step in
TestStrictMode_Reset is not validated: capture and check the error from the
initial NewCmdConfigStrictMode(...).SetArgs([]string{"on"}).Execute() call and
assert that the profile override was applied (load the config via
core.LoadMultiAppConfig() and assert app.StrictMode != nil && *app.StrictMode ==
true) before running the reset; then run the reset command and keep the existing
post-reset assertion. Reference NewCmdConfigStrictMode, cmd.Execute,
core.LoadMultiAppConfig, and app.StrictMode to locate and change the test.
Comment on lines
+87
to
+95
| for _, prov := range p.providers { | ||
| tok, err := prov.ResolveToken(ctx, extcred.TokenSpec{Type: extcred.TokenTypeUAT}) | ||
| if err != nil { | ||
| var blockErr *extcred.BlockError | ||
| if errors.As(err, &blockErr) { | ||
| return nil // provider explicitly blocks UAT; skip enrichment | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
Don't suppress real token-provider failures during account enrichment.
Line 88 asks each provider for a UAT using only Type, so multi-account providers cannot tell which app/user to resolve. Then Lines 89-95 swallow every non-BlockError, even though nil, nil is already the provider contract for “skip”. That can bind the wrong user to the resolved account or mask a broken provider.
Suggested fix
- tok, err := prov.ResolveToken(ctx, extcred.TokenSpec{Type: extcred.TokenTypeUAT})
+ tok, err := prov.ResolveToken(ctx, extcred.TokenSpec{
+ Type: extcred.TokenTypeUAT,
+ Identity: string(core.AsUser),
+ AppID: acct.AppID,
+ })
if err != nil {
var blockErr *extcred.BlockError
if errors.As(err, &blockErr) {
return nil // provider explicitly blocks UAT; skip enrichment
}
- continue
+ return fmt.Errorf("%s failed to resolve UAT for user enrichment: %w", prov.Name(), err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/credential/credential_provider.go` around lines 87 - 95, The loop
over p.providers incorrectly calls prov.ResolveToken(ctx,
extcred.TokenSpec{Type: extcred.TokenTypeUAT}) without identifying which
account/app/user to resolve and it swallows all non-BlockError errors; change
the ResolveToken call to populate TokenSpec with the account/app/user context
used for enrichment (so multi-account providers can disambiguate), and change
the error handling so that: if errors.As(err, &extcred.BlockError{}) then
continue (skip), if err != nil return err (surface real provider failures), and
if err==nil && tok==nil continue (provider skipped). Update references in this
block to p.providers, ResolveToken, extcred.TokenSpec, TokenTypeUAT, and
extcred.BlockError accordingly.
Comment on lines
+43
to
+61
| func TestFullChain_Fallthrough(t *testing.T) { | ||
| // env provider returns nil (no env vars set), falls through to default token | ||
| ep := &envprovider.Provider{} | ||
| mock := &mockDefaultTokenProvider{token: "mock_tok", scopes: "drive:read"} | ||
|
|
||
| cp := credential.NewCredentialProvider( | ||
| []extcred.Provider{ep}, | ||
| nil, mock, nil, | ||
| ) | ||
| result, err := cp.ResolveToken(context.Background(), credential.TokenSpec{ | ||
| Type: credential.TokenTypeUAT, Identity: core.AsUser, AppID: "app1", | ||
| }) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if result.Token != "mock_tok" || result.Scopes != "drive:read" { | ||
| t.Errorf("unexpected: %+v", result) | ||
| } | ||
| } |
There was a problem hiding this comment.
Clear credential env vars before the fallthrough case.
TestFullChain_Fallthrough assumes the env provider returns nothing, but it never unsets LARK_APP_ID, LARK_APP_SECRET, or LARK_USER_ACCESS_TOKEN. If those are present in the runner environment, the env provider wins and this test stops exercising the default resolver.
🧪 Proposed fix
func TestFullChain_Fallthrough(t *testing.T) {
+ t.Setenv("LARK_APP_ID", "")
+ t.Setenv("LARK_APP_SECRET", "")
+ t.Setenv("LARK_USER_ACCESS_TOKEN", "")
+
// env provider returns nil (no env vars set), falls through to default token
ep := &envprovider.Provider{}
mock := &mockDefaultTokenProvider{token: "mock_tok", scopes: "drive:read"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestFullChain_Fallthrough(t *testing.T) { | |
| // env provider returns nil (no env vars set), falls through to default token | |
| ep := &envprovider.Provider{} | |
| mock := &mockDefaultTokenProvider{token: "mock_tok", scopes: "drive:read"} | |
| cp := credential.NewCredentialProvider( | |
| []extcred.Provider{ep}, | |
| nil, mock, nil, | |
| ) | |
| result, err := cp.ResolveToken(context.Background(), credential.TokenSpec{ | |
| Type: credential.TokenTypeUAT, Identity: core.AsUser, AppID: "app1", | |
| }) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if result.Token != "mock_tok" || result.Scopes != "drive:read" { | |
| t.Errorf("unexpected: %+v", result) | |
| } | |
| } | |
| func TestFullChain_Fallthrough(t *testing.T) { | |
| t.Setenv("LARK_APP_ID", "") | |
| t.Setenv("LARK_APP_SECRET", "") | |
| t.Setenv("LARK_USER_ACCESS_TOKEN", "") | |
| // env provider returns nil (no env vars set), falls through to default token | |
| ep := &envprovider.Provider{} | |
| mock := &mockDefaultTokenProvider{token: "mock_tok", scopes: "drive:read"} | |
| cp := credential.NewCredentialProvider( | |
| []extcred.Provider{ep}, | |
| nil, mock, nil, | |
| ) | |
| result, err := cp.ResolveToken(context.Background(), credential.TokenSpec{ | |
| Type: credential.TokenTypeUAT, Identity: core.AsUser, AppID: "app1", | |
| }) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if result.Token != "mock_tok" || result.Scopes != "drive:read" { | |
| t.Errorf("unexpected: %+v", result) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/credential/integration_test.go` around lines 43 - 61,
TestFullChain_Fallthrough assumes envprovider.Provider returns no creds but
doesn't clear process env; ensure the test unsets or clears LARK_APP_ID,
LARK_APP_SECRET, and LARK_USER_ACCESS_TOKEN before creating ep :=
&envprovider.Provider{} so the env provider cannot win. Modify
TestFullChain_Fallthrough to call os.Unsetenv (or equivalent) for those three
env names at the start and optionally restore previous values after the test,
then proceed to construct credential.NewCredentialProvider and assert the mock
default token path.
Comment on lines
+52
to
+55
| if result.Code != 0 { | ||
| return nil, fmt.Errorf("user_info API error: [%d] %s", result.Code, result.Msg) | ||
| } | ||
| return &userInfo{OpenID: result.Data.OpenID, Name: result.Data.Name}, nil |
There was a problem hiding this comment.
Reject user_info successes that omit open_id.
open_id is the stable identifier this enrichment path needs. Returning success with OpenID == "" lets a malformed payload look valid and can leave the upstream account update in a bad state.
Suggested guard
if result.Code != 0 {
return nil, fmt.Errorf("user_info API error: [%d] %s", result.Code, result.Msg)
}
+ if result.Data.OpenID == "" {
+ return nil, fmt.Errorf("user_info API returned empty open_id")
+ }
return &userInfo{OpenID: result.Data.OpenID, Name: result.Data.Name}, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if result.Code != 0 { | |
| return nil, fmt.Errorf("user_info API error: [%d] %s", result.Code, result.Msg) | |
| } | |
| return &userInfo{OpenID: result.Data.OpenID, Name: result.Data.Name}, nil | |
| if result.Code != 0 { | |
| return nil, fmt.Errorf("user_info API error: [%d] %s", result.Code, result.Msg) | |
| } | |
| if result.Data.OpenID == "" { | |
| return nil, fmt.Errorf("user_info API returned empty open_id") | |
| } | |
| return &userInfo{OpenID: result.Data.OpenID, Name: result.Data.Name}, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/credential/user_info.go` around lines 52 - 55, The user_info handler
currently treats API responses with Code==0 as success even if
result.Data.OpenID is empty; update the logic in the function that constructs
and returns &userInfo{OpenID: result.Data.OpenID, Name: result.Data.Name} to
validate result.Data.OpenID is non-empty and return an error (e.g.,
fmt.Errorf("user_info missing open_id")) when it is empty so malformed payloads
are rejected before updating accounts; reference the result, result.Data.OpenID
and userInfo construction in your change.
Comment on lines
+92
to
+96
| result, err := ctx.Factory.Credential.ResolveToken(ctx.ctx, credential.NewTokenSpec(ctx.As(), ctx.Config.AppID)) | ||
| if err != nil { | ||
| return "", output.ErrAuth("failed to get access token: %s", err) | ||
| } | ||
| return token, nil | ||
| return result.Token, nil |
There was a problem hiding this comment.
Handle a missing token result before dereferencing it.
ResolveToken is already treated as nullable elsewhere in this PR. If it returns (nil, nil) here, result.Token panics instead of returning an auth error.
Suggested fix
result, err := ctx.Factory.Credential.ResolveToken(ctx.ctx, credential.NewTokenSpec(ctx.As(), ctx.Config.AppID))
if err != nil {
return "", output.ErrAuth("failed to get access token: %s", err)
}
+ if result == nil || result.Token == "" {
+ return "", output.ErrAuth("failed to get access token: no token resolved for %s", ctx.As())
+ }
return result.Token, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| result, err := ctx.Factory.Credential.ResolveToken(ctx.ctx, credential.NewTokenSpec(ctx.As(), ctx.Config.AppID)) | |
| if err != nil { | |
| return "", output.ErrAuth("failed to get access token: %s", err) | |
| } | |
| return token, nil | |
| return result.Token, nil | |
| result, err := ctx.Factory.Credential.ResolveToken(ctx.ctx, credential.NewTokenSpec(ctx.As(), ctx.Config.AppID)) | |
| if err != nil { | |
| return "", output.ErrAuth("failed to get access token: %s", err) | |
| } | |
| if result == nil || result.Token == "" { | |
| return "", output.ErrAuth("failed to get access token: no token resolved for %s", ctx.As()) | |
| } | |
| return result.Token, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/common/runner.go` around lines 92 - 96, The code calls
ctx.Factory.Credential.ResolveToken(...) and immediately dereferences
result.Token, which can panic if ResolveToken returns (nil, nil); add a
nil-check after the call (e.g., if result == nil) and return a proper auth error
via output.ErrAuth with a clear message (including context like AppID) instead
of dereferencing; update the return path in the function that contains this call
so it safely handles a nil result from ResolveToken before accessing
result.Token.
Comment on lines
+447
to
453
| result, err := runtime.Factory.Credential.ResolveToken(ctx, credential.NewTokenSpec(runtime.As(), runtime.Config.AppID)) | ||
| if err == nil && result != nil && result.Scopes != "" { | ||
| if missing := auth.MissingScopes(result.Scopes, required); len(missing) > 0 { | ||
| return output.ErrWithHint(output.ExitAuth, "missing_scope", | ||
| fmt.Sprintf("missing required scope(s): %s", strings.Join(missing, ", ")), | ||
| fmt.Sprintf("run `lark-cli auth login --scope \"%s\"` in the background. It blocks and outputs a verification URL — retrieve the URL and open it in a browser to complete login.", strings.Join(missing, " "))) | ||
| } |
There was a problem hiding this comment.
Keep scope validation side-effect free.
ResolveToken is an active provider operation, not just a metadata lookup. Calling it from Validate makes argument validation depend on live credential resolution, and the err == nil gate silently disables the scope check whenever the provider errors. This should read cached scope metadata only, or skip the extra check when that metadata is unavailable.
Replace downloadURLToTempFile with two new paths: - Image uploads: stream HTTP response body directly as io.Reader - File/video/audio uploads: buffer in memory via mediaBuffer struct, which provides Duration() for seek-like access and Reader() for upload SDK Formdata.content() already reads everything into memory, so temp files were redundant disk I/O. Local file upload paths unchanged. Change-Id: I264bf114155e56bee32a9729e6673c6bae43d355 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract mediaSpec struct and resolve helpers to eliminate repetitive if/else branches. Image/file/audio follow a uniform pattern via resolveOneMedia; video stays separate due to its dual-key output. Change-Id: Id27b92ef44c23f3a143994a3d1aee3baba8be112 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/im/helpers.go`:
- Around line 348-359: The code uploads the video (resolveOneMedia(videoSpec))
before verifying the cover, wasting work if the cover is invalid; swap the order
so you construct coverSpec and call resolveOneMedia(ctx, runtime, coverSpec)
first (handling its error with the same fmt.Errorf("cover image upload failed:
%w", err) or mediaFallbackOrError as appropriate), then call resolveOneMedia for
videoSpec (fKey) only after coverKey succeeds; update any variables (videoSpec,
videoVal, videoCoverVal, mediaKindImage, maxImageUploadSize) usage to match the
new flow and keep existing error-return semantics.
- Around line 879-881: The log prints the raw download error which can leak
sensitive URL data; add a helper (e.g., redactDownloadError) that strips URL
details by detecting *url.Error via errors.As and returning only the operation
and inner error (or err.Error() otherwise), then change the download failure
branch in downloadURLToReader's caller (the code around
sanitizeURLForDisplay(imgURL) in the block where rc, _, err :=
downloadURLToReader(...) is handled) to log the sanitized URL and
redactDownloadError(err) instead of the raw err (replace the %v usage with the
redacted string).
- Around line 190-191: The code currently infers extension from the original
rawURL (filepath.Ext(fileNameFromURL(rawURL))) which can be empty if redirects
changed the final resource; change it to use the final response URL from
resp.Request (e.g., resp.Request.URL) to compute the extension instead. Update
the code around the ext computation to check resp and resp.Request for nil,
derive finalURL := resp.Request.URL.String() when available, call
fileNameFromURL(finalURL) and then filepath.Ext(...) so mediaBuffer.FileType()
gets the correct extension (fall back to rawURL only if resp.Request is nil).
🪄 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: dce63b7e-34d9-47f6-b2af-c2885eef615a
📒 Files selected for processing (2)
shortcuts/im/coverage_additional_test.goshortcuts/im/helpers.go
Comment on lines
190
to
+191
| ext := filepath.Ext(fileNameFromURL(rawURL)) | ||
| tmpFile, err := os.CreateTemp("", "lark-media-*"+ext) | ||
| if err != nil { | ||
| return "", fmt.Errorf("create temp file: %w", err) | ||
| } | ||
| return resp, ext, nil |
There was a problem hiding this comment.
Infer the media extension from the final response URL.
http.Client may already have followed redirects by Line 190. If a short/signed URL without an extension lands on .mp4 or .ogg, filepath.Ext(fileNameFromURL(rawURL)) stays empty, so mediaBuffer.FileType() falls back to stream and duration parsing is skipped for --audio / --video.
💡 Suggested fix
- ext := filepath.Ext(fileNameFromURL(rawURL))
+ finalURL := rawURL
+ if resp.Request != nil && resp.Request.URL != nil {
+ finalURL = resp.Request.URL.String()
+ }
+ ext := filepath.Ext(fileNameFromURL(finalURL))
return resp, ext, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ext := filepath.Ext(fileNameFromURL(rawURL)) | |
| tmpFile, err := os.CreateTemp("", "lark-media-*"+ext) | |
| if err != nil { | |
| return "", fmt.Errorf("create temp file: %w", err) | |
| } | |
| return resp, ext, nil | |
| finalURL := rawURL | |
| if resp.Request != nil && resp.Request.URL != nil { | |
| finalURL = resp.Request.URL.String() | |
| } | |
| ext := filepath.Ext(fileNameFromURL(finalURL)) | |
| return resp, ext, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/im/helpers.go` around lines 190 - 191, The code currently infers
extension from the original rawURL (filepath.Ext(fileNameFromURL(rawURL))) which
can be empty if redirects changed the final resource; change it to use the final
response URL from resp.Request (e.g., resp.Request.URL) to compute the extension
instead. Update the code around the ext computation to check resp and
resp.Request for nil, derive finalURL := resp.Request.URL.String() when
available, call fileNameFromURL(finalURL) and then filepath.Ext(...) so
mediaBuffer.FileType() gets the correct extension (fall back to rawURL only if
resp.Request is nil).
Comment on lines
+348
to
+359
| fKey, err := resolveOneMedia(ctx, runtime, videoSpec) | ||
| if err != nil { | ||
| return mediaFallbackOrError(videoVal, "video", err) | ||
| } | ||
|
|
||
| coverSpec := mediaSpec{ | ||
| value: videoCoverVal, flagName: "--video-cover", mediaType: "cover image", | ||
| kind: mediaKindImage, maxSize: maxImageUploadSize, resultKey: "image_key", | ||
| } | ||
| coverKey, err := resolveOneMedia(ctx, runtime, coverSpec) | ||
| if err != nil { | ||
| return "", "", fmt.Errorf("cover image upload failed: %w", err) |
There was a problem hiding this comment.
Resolve --video-cover before uploading the video.
Line 348 performs the expensive remote file upload before Line 357 proves the required cover can be resolved. If the cover path/URL is bad, the command fails only after a wasted write that the user will usually retry.
💡 Suggested fix
- fKey, err := resolveOneMedia(ctx, runtime, videoSpec)
- if err != nil {
- return mediaFallbackOrError(videoVal, "video", err)
- }
-
coverSpec := mediaSpec{
value: videoCoverVal, flagName: "--video-cover", mediaType: "cover image",
kind: mediaKindImage, maxSize: maxImageUploadSize, resultKey: "image_key",
}
coverKey, err := resolveOneMedia(ctx, runtime, coverSpec)
if err != nil {
return "", "", fmt.Errorf("cover image upload failed: %w", err)
}
+
+ fKey, err := resolveOneMedia(ctx, runtime, videoSpec)
+ if err != nil {
+ return mediaFallbackOrError(videoVal, "video", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fKey, err := resolveOneMedia(ctx, runtime, videoSpec) | |
| if err != nil { | |
| return mediaFallbackOrError(videoVal, "video", err) | |
| } | |
| coverSpec := mediaSpec{ | |
| value: videoCoverVal, flagName: "--video-cover", mediaType: "cover image", | |
| kind: mediaKindImage, maxSize: maxImageUploadSize, resultKey: "image_key", | |
| } | |
| coverKey, err := resolveOneMedia(ctx, runtime, coverSpec) | |
| if err != nil { | |
| return "", "", fmt.Errorf("cover image upload failed: %w", err) | |
| coverSpec := mediaSpec{ | |
| value: videoCoverVal, flagName: "--video-cover", mediaType: "cover image", | |
| kind: mediaKindImage, maxSize: maxImageUploadSize, resultKey: "image_key", | |
| } | |
| coverKey, err := resolveOneMedia(ctx, runtime, coverSpec) | |
| if err != nil { | |
| return "", "", fmt.Errorf("cover image upload failed: %w", err) | |
| } | |
| fKey, err := resolveOneMedia(ctx, runtime, videoSpec) | |
| if err != nil { | |
| return mediaFallbackOrError(videoVal, "video", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/im/helpers.go` around lines 348 - 359, The code uploads the video
(resolveOneMedia(videoSpec)) before verifying the cover, wasting work if the
cover is invalid; swap the order so you construct coverSpec and call
resolveOneMedia(ctx, runtime, coverSpec) first (handling its error with the same
fmt.Errorf("cover image upload failed: %w", err) or mediaFallbackOrError as
appropriate), then call resolveOneMedia for videoSpec (fKey) only after coverKey
succeeds; update any variables (videoSpec, videoVal, videoCoverVal,
mediaKindImage, maxImageUploadSize) usage to match the new flow and keep
existing error-return semantics.
Comment on lines
+879
to
881
| rc, _, err := downloadURLToReader(ctx, runtime, imgURL, maxImageUploadSize) | ||
| if err != nil { | ||
| fmt.Fprintf(runtime.IO().ErrOut, "warning: failed to download image %s: %v\n", sanitizeURLForDisplay(imgURL), err) |
There was a problem hiding this comment.
Redact the underlying download error before logging it.
This branch already sanitizes imgURL, but %v on err can still include the full source URL from url.Error or URL-parse errors. Failed markdown-image downloads would then leak signed query parameters into stderr / CI logs.
💡 Suggested fix
- fmt.Fprintf(runtime.IO().ErrOut, "warning: failed to download image %s: %v\n", sanitizeURLForDisplay(imgURL), err)
+ fmt.Fprintf(runtime.IO().ErrOut, "warning: failed to download image %s: %s\n", sanitizeURLForDisplay(imgURL), redactDownloadError(err))func redactDownloadError(err error) string {
var urlErr *url.Error
if errors.As(err, &urlErr) {
return fmt.Sprintf("%s: %v", urlErr.Op, urlErr.Err)
}
return err.Error()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/im/helpers.go` around lines 879 - 881, The log prints the raw
download error which can leak sensitive URL data; add a helper (e.g.,
redactDownloadError) that strips URL details by detecting *url.Error via
errors.As and returning only the operation and inner error (or err.Error()
otherwise), then change the download failure branch in downloadURLToReader's
caller (the code around sanitizeURLForDisplay(imgURL) in the block where rc, _,
err := downloadURLToReader(...) is handled) to log the sanitized URL and
redactDownloadError(err) instead of the raw err (replace the %v usage with the
redacted string).
Add forbidigo lint rules scoped to shortcuts/ (excluding tests) to prevent temp file intermediary patterns and direct process exits. Only checks incremental code (--new-from-rev), existing code unaffected. Change-Id: Ie7f2e79193bb1298e28bf1613961de470042dd16 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.golangci.yml (1)
62-67:os\.Remove\bexcludesos.RemoveAll— consider clarifying intent.The word boundary
\bin the pattern meansos.RemoveAllwill not be flagged. Whileos.RemoveAllis not currently used inshortcuts/, it's worth being explicit about whether both functions should be forbidden or just one. If both should be forbidden, remove the\bor add a separate pattern foros.RemoveAll.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 62 - 67, The golangci-lint rule currently targets the regex pattern `os\.Remove\b` which excludes `os.RemoveAll`; update the rule for clarity and coverage: decide if both `os.Remove` and `os.RemoveAll` should be forbidden in `shortcuts/` and then either remove the `\b` from the pattern or add an explicit second pattern for `os.RemoveAll`, and update the rule message to mention both `os.Remove` and `os.RemoveAll` and reference `shortcuts/im/helpers.go` as the recommended alternative.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 54-73: Move the per-pattern exclude-godoc-examples settings into
the forbidigo top-level: under the forbidigo block add exclude-godoc-examples:
true once, then keep the forbid list (patterns os\.CreateTemp, os\.Remove\b,
os\.Exit and their msg entries) unchanged beneath it; retain analyze-types: true
at the same level so the option applies globally instead of being ignored
per-pattern.
- Line 30: Pre-existing use of os.Exit in the shortcuts mail watcher will break
the new forbidigo linter; fix it by either adding a localized suppression
comment "//nolint:forbidigo" on the specific os.Exit(...) invocation (include a
short TODO referencing a tracking issue/PR for refactor) or update CI lint
invocation to pass "--new-from-rev=origin/main" so pre-existing violations are
ignored; target the os.Exit call in the mail watcher code path and apply one of
these two mitigations.
---
Nitpick comments:
In @.golangci.yml:
- Around line 62-67: The golangci-lint rule currently targets the regex pattern
`os\.Remove\b` which excludes `os.RemoveAll`; update the rule for clarity and
coverage: decide if both `os.Remove` and `os.RemoveAll` should be forbidden in
`shortcuts/` and then either remove the `\b` from the pattern or add an explicit
second pattern for `os.RemoveAll`, and update the rule message to mention both
`os.Remove` and `os.RemoveAll` and reference `shortcuts/im/helpers.go` as the
recommended alternative.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| - reassign # checks that package variables are not reassigned | ||
| - unconvert # removes unnecessary type conversions | ||
| - unused # checks for unused constants, variables, functions and types | ||
| - forbidigo # forbids specific function calls |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all os.Exit usages in shortcuts/ non-test files that would violate the new rule
rg -n --type=go 'os\.Exit' shortcuts/ -g '!*_test.go'Repository: larksuite/cli
Length of output: 103
🏁 Script executed:
cat -n .golangci.yml | sed -n '20,80p'Repository: larksuite/cli
Length of output: 2894
🏁 Script executed:
sed -n '440,450p' shortcuts/mail/mail_watch.goRepository: larksuite/cli
Length of output: 382
Address pre-existing os.Exit(0) violation at shortcuts/mail/mail_watch.go:444.
The forbidigo rule being added forbids os.Exit in the shortcuts/ directory. Existing code at line 444 violates this rule and will fail linting unless mitigated. Either add //nolint:forbidigo to that line with a tracking issue for refactoring, or ensure CI consistently runs with --new-from-rev=origin/main to skip linting pre-existing violations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml at line 30, Pre-existing use of os.Exit in the shortcuts mail
watcher will break the new forbidigo linter; fix it by either adding a localized
suppression comment "//nolint:forbidigo" on the specific os.Exit(...) invocation
(include a short TODO referencing a tracking issue/PR for refactor) or update CI
lint invocation to pass "--new-from-rev=origin/main" so pre-existing violations
are ignored; target the os.Exit call in the mail watcher code path and apply one
of these two mitigations.
Comment on lines
+54
to
+73
| forbidigo: | ||
| forbid: | ||
| - pattern: os\.CreateTemp | ||
| msg: >- | ||
| Do not use os.CreateTemp in shortcuts/ for temp file intermediary logic. | ||
| Use downloadURLToReader (streaming) or mediaBuffer (in-memory) instead. | ||
| See shortcuts/im/helpers.go for reference. | ||
| exclude-godoc-examples: true | ||
| - pattern: os\.Remove\b | ||
| msg: >- | ||
| Do not use os.Remove in shortcuts/ for temp file cleanup. | ||
| Avoid temp files entirely: use io.Reader streaming or in-memory buffers. | ||
| See shortcuts/im/helpers.go for reference. | ||
| exclude-godoc-examples: true | ||
| - pattern: os\.Exit | ||
| msg: >- | ||
| Do not use os.Exit in shortcuts/. Return an error instead and let | ||
| the caller (cmd layer) decide how to terminate. | ||
| exclude-godoc-examples: true | ||
| analyze-types: true |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does golangci-lint forbidigo support exclude-godoc-examples per pattern or only at the top level?
💡 Result:
No, golangci-lint forbidigo does not support exclude-godoc-examples per pattern. It is a top-level global setting that applies to all forbid rules. The official golangci-lint documentation shows exclude-godoc-examples as a top-level option under forbidigo settings, separate from individual pattern definitions: linters: settings: forbidigo: exclude-godoc-examples: false Complex patterns can specify fields like pattern, pkg, and msg, but exclude-godoc-examples is not included per pattern. It is passed once to forbidigo.OptionExcludeGodocExamples in the linter implementation, applying globally. The .golangci.reference.yml confirms this structure, listing exclude-godoc-examples at the forbidigo level, not nested under individual forbid entries. The forbidigo library itself provides OptionExcludeGodocExamples as a single global option, not per-rule.
Citations:
- 1: https://golangci-lint.run/docs/linters/configuration/
- 2: https://github.com/golangci/golangci-lint/blob/ab192139/.golangci.reference.yml
Move exclude-godoc-examples: true to the top level of forbidigo settings.
The exclude-godoc-examples option is not supported per-pattern in forbidigo; it must be placed at the forbidigo top level to apply globally to all patterns. As currently configured, the per-pattern settings will be ignored and godoc examples may still be flagged.
Corrected configuration
forbidigo:
exclude-godoc-examples: true
forbid:
- pattern: os\.CreateTemp
msg: >-
Do not use os.CreateTemp in shortcuts/ for temp file intermediary logic.
Use downloadURLToReader (streaming) or mediaBuffer (in-memory) instead.
See shortcuts/im/helpers.go for reference.
- pattern: os\.Remove\b
msg: >-
Do not use os.Remove in shortcuts/ for temp file cleanup.
Avoid temp files entirely: use io.Reader streaming or in-memory buffers.
See shortcuts/im/helpers.go for reference.
- pattern: os\.Exit
msg: >-
Do not use os.Exit in shortcuts/. Return an error instead and let
the caller (cmd layer) decide how to terminate.
analyze-types: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml around lines 54 - 73, Move the per-pattern
exclude-godoc-examples settings into the forbidigo top-level: under the
forbidigo block add exclude-godoc-examples: true once, then keep the forbid list
(patterns os\.CreateTemp, os\.Remove\b, os\.Exit and their msg entries)
unchanged beneath it; retain analyze-types: true at the same level so the option
applies globally instead of being ignored per-pattern.
- Reject --reset --global together in strict-mode command - Clear Users and UAT tokens when saveAsProfile detects AppId change - Clear LARKSUITE_CLI_STRICT_MODE in test setup to avoid CI pollution Change-Id: I1b6f68984a837a5534b30942c8f0f492c5d6c9fa Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/config/init.go (1)
257-277:⚠️ Potential issue | 🟠 MajorMirror AppId-change cleanup in unchanged-secret interactive updates.
The unchanged-secret flow still rewrites
AppIdwithout clearingUsersand stored tokens, so stale user state can be carried into a different app context. The same cleanup logic already exists insaveAsProfile, but is missing here.Suggested fix
} else if result.Mode == "existing" && result.AppID != "" { // Existing app with unchanged secret — update app ID and brand only if opts.ProfileName != "" && existing != nil { // Profile mode: update named profile in-place if idx := existing.FindAppIndex(opts.ProfileName); idx >= 0 { + if existing.Apps[idx].AppId != result.AppID { + for _, user := range existing.Apps[idx].Users { + auth.RemoveStoredToken(existing.Apps[idx].AppId, user.UserOpenId) + } + existing.Apps[idx].Users = []core.AppUser{} + } existing.Apps[idx].AppId = result.AppID existing.Apps[idx].Brand = result.Brand existing.Apps[idx].Lang = opts.Lang } else { return output.ErrValidation("App Secret cannot be empty for new profile") } if err := core.SaveMultiAppConfig(existing); err != nil { return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) } } else if existing != nil { app := existing.CurrentAppConfig("") if app != nil { + if app.AppId != result.AppID { + for _, user := range app.Users { + auth.RemoveStoredToken(app.AppId, user.UserOpenId) + } + app.Users = []core.AppUser{} + } app.AppId = result.AppID app.Brand = result.Brand app.Lang = opts.Lang if err := core.SaveMultiAppConfig(existing); err != nil { return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` around lines 257 - 277, The interactive update path that updates existing.CurrentAppConfig("") (inside the else-if existing != nil block) only overwrites AppId/Brand/Lang and fails to clear per-app user state and tokens, causing stale Users/tokens to persist; mirror the cleanup used in saveAsProfile by clearing app.Users and any stored tokens/session state for that app before calling core.SaveMultiAppConfig(existing), ensuring you perform the same resets you already apply in saveAsProfile when result.AppID changes.
🧹 Nitpick comments (1)
cmd/config/strict_mode.go (1)
111-120: Consider extracting shared logic to avoid drift withFactory.IsStrictMode().This function duplicates the priority resolution logic from
Factory.IsStrictMode()ininternal/cmdutil/factory.go:119-134. If either implementation is updated independently, the display command may report a different effective value than what runtime actually enforces.Extract to a shared helper (e.g., in
internal/core) that both can call:// In internal/core/strict_mode.go func ResolveStrictMode(multi *MultiAppConfig, app *AppConfig) (effective bool, source string) { if v := os.Getenv("LARKSUITE_CLI_STRICT_MODE"); v != "" { return v == "true" || v == "1", "env LARKSUITE_CLI_STRICT_MODE" } if app != nil && app.StrictMode != nil { return *app.StrictMode, fmt.Sprintf("profile %q", app.ProfileName()) } return multi.StrictMode, "global" }Then
Factory.IsStrictMode()can call this and discard the source string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/strict_mode.go` around lines 111 - 120, The priority/resolution logic in resolveStrictModeStatus duplicates Factory.IsStrictMode; extract it into a single helper in internal/core (e.g., func ResolveStrictMode(multi *core.MultiAppConfig, app *core.AppConfig) (bool, string)) implementing the env -> app profile -> multi fallback, then have resolveStrictModeStatus and Factory.IsStrictMode call that helper (Factory.IsStrictMode can ignore the returned source string). Update imports/usages accordingly so both use the shared ResolveStrictMode implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/config/init.go`:
- Around line 257-277: The interactive update path that updates
existing.CurrentAppConfig("") (inside the else-if existing != nil block) only
overwrites AppId/Brand/Lang and fails to clear per-app user state and tokens,
causing stale Users/tokens to persist; mirror the cleanup used in saveAsProfile
by clearing app.Users and any stored tokens/session state for that app before
calling core.SaveMultiAppConfig(existing), ensuring you perform the same resets
you already apply in saveAsProfile when result.AppID changes.
---
Nitpick comments:
In `@cmd/config/strict_mode.go`:
- Around line 111-120: The priority/resolution logic in resolveStrictModeStatus
duplicates Factory.IsStrictMode; extract it into a single helper in
internal/core (e.g., func ResolveStrictMode(multi *core.MultiAppConfig, app
*core.AppConfig) (bool, string)) implementing the env -> app profile -> multi
fallback, then have resolveStrictModeStatus and Factory.IsStrictMode call that
helper (Factory.IsStrictMode can ignore the returned source string). Update
imports/usages accordingly so both use the shared ResolveStrictMode
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4142c7f7-a172-4b03-aeb6-3bad5fce29a8
📒 Files selected for processing (3)
cmd/config/init.gocmd/config/strict_mode.gocmd/config/strict_mode_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/config/strict_mode_test.go
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.
Summary
enrichUserInfo, strict-mode--resetarg conflict,--global onoverride warningTokenTypefrominttostring(eliminate mapping),t.Setenvmigration,utf8.RuneCountInStringfor profile name validation,json.Marshalerror handling, keychain cleanup on profile AppId change,ctxparam ordering,profile add --usePreviousApp fixsync.Once+ context behavior, UAT non-caching rationaleContext
Code review of
2e342fd(unified credential provider system, written by Codex) identified 6 Critical, 18 Important, and 17 Minor issues across 4 review domains (core packages, integration layer, command layer, shortcuts/tests). This PR addresses all Critical and key Important issues.Test plan
go build ./...passesshortcuts/drivetests pass (were failing to compile before)internal/credential,internal/cmdutil,internal/core,cmd/config,cmd/auth,cmd/serviceall pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements