feat: add strict mode identity filter, profile management and credential extension#252
Merged
liangshuo-1 merged 33 commits intomainfrom Apr 7, 2026
Merged
feat: add strict mode identity filter, profile management and credential extension#252liangshuo-1 merged 33 commits intomainfrom
liangshuo-1 merged 33 commits intomainfrom
Conversation
…ial extension Port changes from feat/strict-mode-identity-filter_3 branch: - Add strict mode for identity filtering and configuration - Add profile management commands (add/list/remove/rename/use) - Add credential extension framework (registry, env provider) - Add VFS abstraction layer - Refactor factory default and client options - Update shortcuts to use new credential and validation patterns Change-Id: I8c104c6b147e1901d94aefcefe35a174932c742b Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
domain/base
|
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:
📝 WalkthroughWalkthroughAdds a credential provider chain (env + registry), multi-profile config and strict-mode enforcement, a VFS abstraction replacing direct os.* usage, credential-driven token resolution and streaming API client support, global/profile bootstrap flags and profile commands, shortcut input/file I/O enhancements, and forbidigo linter rules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI (root)
participant Bootstrap as BootstrapInvocationContext
participant Factory as cmdutil.Factory
participant CredProv as CredentialProvider
participant ExtEnv as extcred/env
participant Config as core.MultiAppConfig
CLI->>Bootstrap: parse args (e.g. --profile)
Bootstrap-->>CLI: InvocationContext{Profile}
CLI->>Factory: NewDefault(InvocationContext)
Factory->>CredProv: build chain / ResolveAccount(ctx)
CredProv->>ExtEnv: ResolveAccount()
alt env provides account
ExtEnv-->>CredProv: Account + SupportedIdentities
else
CredProv->>Config: LoadMultiAppConfig()
Config-->>CredProv: AppConfig for profile
end
CredProv-->>Factory: Resolved Account
Factory-->>CLI: ready (Config(), ResolveStrictMode())
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Change-Id: I0f610ccea6bc874248e84c24770944a3071dcc57 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5583909787eebab535f7711836f6c5c8258bf4d8🧩 Skill updatenpx skills add larksuite/cli#refactor/plugin -y -g |
- Remove unused TAT stub registrations in api and service tests (CredentialProvider manages tokens, SDK no longer calls TAT endpoint) - Update strict mode integration test: +chat-create now supports user identity, so it should succeed under strict mode user Change-Id: Iab51c2e12a97995e0b95dcd71df212d2d1f76570 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace direct os.Stat/Open/MkdirAll/OpenFile/Remove/ReadDir/UserHomeDir with vfs equivalents in shortcuts/minutes, shortcuts/drive, and internal/keychain. Add ReadDir to the vfs interface and OsFs implementation. Change-Id: I8f97e5fb3e1731b4684d276644fcb10fae823067
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
shortcuts/im/im_messages_resources_download.go (1)
145-156:⚠️ Potential issue | 🔴 CriticalVerify that
DoStreamconverts non-2xx HTTP responses to errors, or add local status code validation.The code relies entirely on the undocumented contract that
ac.DoStream()(from larkcore SDK) returns an error for non-2xx responses. However, none of the fourDoAPIStreamcallers (this file, drive_download, doc_media_download, sheets_sheet_export) validate the response status code before writing to file. This pattern is inconsistent with other downloads in the same codebase—minutes_download.goanddrive_export_common.goexplicitly checkStatusCode. Without verification thatDoStreamconverts failures to errors, an HTTP error response body could be written as a downloaded file.Either confirm the
DoStreamcontract with tests, or add local validation:if downloadResp.StatusCode >= 400 { return "", 0, ... }after the DoAPIStream call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_messages_resources_download.go` around lines 145 - 156, After calling runtime.DoAPIStream you must validate the HTTP status before treating the response as a successful download: check downloadResp.StatusCode (the value returned by runtime.DoAPIStream) and if it's >= 400 return an error (include status and any returned body/message) instead of proceeding to write the file; update the code around the runtime.DoAPIStream call (referencing downloadResp, runtime.DoAPIStream and defaultIMResourceDownloadTimeout) to perform this local check and return early on error.internal/validate/path.go (1)
63-88:⚠️ Potential issue | 🟠 MajorDon't mix
vfsprobes withfilepath.EvalSymlinks.Lines 63-75 and 102-108 now check existence through
vfs, but canonicalization still goes throughfilepath.EvalSymlinks. That means a non-default VFS can report a path as present and then immediately fail resolving the same path against the host OS, which breaks the new abstraction and its tests.Suggested direction
- resolved, err = filepath.EvalSymlinks(resolved) + resolved, err = vfs.EvalSymlinks(resolved) ... - canonicalCwd, _ := filepath.EvalSymlinks(cwd) + canonicalCwd, _ := vfs.EvalSymlinks(cwd) ... - real, err := filepath.EvalSymlinks(cur) + real, err := vfs.EvalSymlinks(cur)If
internal/vfsdoes not expose this yet, I'd add a small wrapper there first so the whole validation flow stays on one filesystem boundary.Also applies to: 102-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/validate/path.go` around lines 63 - 88, The code mixes host FS calls (filepath.EvalSymlinks) with vfs probes (vfs.Getwd/vfs.Lstat), which breaks non-default VFS implementations; replace the direct calls to filepath.EvalSymlinks (both the EvalSymlinks(resolved) and EvalSymlinks(cwd) usages) with a vfs-level symlink-resolution API (e.g., add and call vfs.EvalSymlinks or equivalent) and ensure resolveNearestAncestor and isUnderDir operate entirely via the vfs API so all existence checks and canonicalization stay on the same filesystem boundary.shortcuts/sheets/sheet_export.go (1)
115-143:⚠️ Potential issue | 🟡 MinorMissing early return when outputPath is empty causes unnecessary download attempt.
When
outputPathis empty, the code outputsfile_tokenandticket(lines 116-119) but then continues to attempt the download (line 123). This download will fail at line 132-134 whenSafeOutputPath("")returns a validation error.The original intent appears to be: if no output path is specified, just return the export task result without downloading. Add an early return after outputting the tokens.
Proposed fix
if outputPath == "" { runtime.Out(map[string]interface{}{ "file_token": fileToken, "ticket": ticket, }, nil) + return nil } // Download🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_export.go` around lines 115 - 143, When outputPath is empty the code calls runtime.Out(...) but does not return, so the subsequent download and calls to validate.SafeOutputPath and validate.AtomicWriteFromReader run and fail; fix by adding an early return immediately after the runtime.Out(...) call in the block that checks if outputPath == "" so the function exits successfully (no download attempted) when only file_token/ticket are being emitted; locate the check for outputPath, the runtime.Out invocation, and add the return there to skip the DoAPIStream, SafeOutputPath, vfs.MkdirAll and AtomicWriteFromReader logic.shortcuts/common/runner.go (1)
465-472:⚠️ Potential issue | 🔴 CriticalValidate strict mode before
ResolveAscan coerce the shortcut identity.If
ResolveAs()already applies strict-mode/default overrides,CheckStrictMode(as)no longer sees the shortcut’s original/default identity. That lets a disallowed shortcut execute under the coerced identity instead of returning the strict-mode envelope, which matches the failingTestIntegration_StrictModeUser_ProfileOverride_DirectBotShortcutReturnsEnvelopein CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/runner.go` around lines 465 - 472, Call CheckStrictMode before letting ResolveAs coerce the identity: extract the raw flag value (asFlag) and call f.CheckStrictMode(core.Identity(asFlag)) first, returning any error; only after strict-mode passes, call f.ResolveAs(cmd, core.Identity(asFlag)) to apply defaults/auto-detection and continue using the resolved identity. Update resolveShortcutIdentity (and use f.CheckStrictMode and f.ResolveAs references) so strict-mode validation runs against the original input before coercion.
🧹 Nitpick comments (15)
cmd/profile/list.go (1)
17-25: Consider usingomitemptyfor optional JSON fields.The
UserandTokenStatusfields are only populated when applicable, but withoutomitempty, they will appear as empty strings in the JSON output for profiles without logged-in users. This is minor but could make the output cleaner.🔧 Suggested improvement
type profileListItem struct { Name string `json:"name"` AppID string `json:"appId"` Brand core.LarkBrand `json:"brand"` Active bool `json:"active"` - User string `json:"user"` - TokenStatus string `json:"tokenStatus"` + User string `json:"user,omitempty"` + TokenStatus string `json:"tokenStatus,omitempty"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/profile/list.go` around lines 17 - 25, The profileListItem struct currently marshals User and TokenStatus as empty strings for profiles without values; update the struct tags for the User and TokenStatus fields on profileListItem to use the `omitempty` json option so those keys are omitted when empty (i.e., change the `json:"user"` and `json:"tokenStatus"` tags to include `omitempty`), leaving other fields unchanged.cmd/config/init.go (1)
255-283: The "existing app with unchanged secret" branch has grown complex.This section handles four distinct cases: profile mode with existing profile, profile mode without existing profile, non-profile mode with current app, and non-profile mode without config. Consider extracting this into a helper function for readability, though it's not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` around lines 255 - 283, The existing "result.Mode == 'existing' && result.AppID != ''" branch mixes four paths and should be extracted into a helper to simplify logic: create a function (e.g., updateExistingAppConfig(existing, result, opts) or core.UpdateExistingApp) that encapsulates the profile vs non-profile branching, uses existing.FindAppIndex(opts.ProfileName) to update Apps[idx].AppId/Brand/Lang when profile mode and existing, returns a validation error when profile name not found, and otherwise uses existing.CurrentAppConfig("") to update AppId/Brand/Lang or return the validation error when nil; ensure the caller only handles calling this helper and a single core.SaveMultiAppConfig(existing) invocation and returns any error from those calls (keeping references to result.Mode, result.AppID, opts.ProfileName, existing.FindAppIndex, existing.CurrentAppConfig, and core.SaveMultiAppConfig).internal/core/config.go (1)
132-152: Consider adding newline characters to the forbidden character check.The validation rejects control characters and shell-problematic characters, but explicitly listing
\nand\rin the switch statement (alongside the control character check) would make the intent clearer and match the pattern used elsewhere in the codebase for input validation.That said, the current implementation already catches them via the control character check (
r <= 0x1F), so this is just a clarity suggestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/config.go` around lines 132 - 152, The ValidateProfileName function currently rejects control characters via r <= 0x1F but does not explicitly list newline chars; update the switch in ValidateProfileName to include '\n' and '\r' alongside the other forbidden characters to make the intent explicit and consistent with other validators (leave the control-character check intact so behavior is unchanged).internal/core/types.go (1)
16-23: Consider case-insensitive brand matching.The
ParseBrandfunction uses case-sensitive comparison (value == "lark"), which means inputs like"Lark"or"LARK"will default toBrandFeishu. If this is intentional for strict matching, the current implementation is fine. Otherwise, consider usingstrings.EqualFoldfor case-insensitive matching.💡 Optional: Case-insensitive matching
func ParseBrand(value string) LarkBrand { - if value == "lark" { + if strings.EqualFold(value, "lark") { return BrandLark } return BrandFeishu }This would require adding
"strings"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/types.go` around lines 16 - 23, ParseBrand currently does a case-sensitive comparison so inputs like "Lark" or "LARK" will fall back to BrandFeishu; update ParseBrand to perform a case-insensitive match (use strings.EqualFold(value, "lark")) and add the "strings" import so inputs with different casing map to BrandLark while still returning BrandFeishu for unrecognized values; keep the function signature and return values (BrandLark, BrandFeishu) unchanged.shortcuts/mail/mail_watch_test.go (1)
99-100: Re-add HTTP method assertion for the profile dry-run call.At Line 99, the test only validates URL; a method regression would now pass silently. Consider asserting
GETas well.Proposed test hardening
+ 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 currently only asserts the URL for the profile dry-run call (checking apis[1].URL against mailboxPath("me", "profile")) but misses the HTTP method; update the assertion to also check the method (e.g., assert apis[1].Method == "GET" and call t.Fatalf with a clear message if it differs) so a regression that changes the request verb will fail; modify the test near the existing URL check referencing the apis slice and mailboxPath("me","profile") to include this additional method assertion.internal/credential/default_provider_test.go (1)
7-13: Add a behavioral test for provider selection.Lines 7-13 only prove the types satisfy the interfaces, so a broken dispatch/fallback chain would still compile cleanly. Given the new credential-extension path is core behavior, I'd add at least one runtime test that exercises provider lookup through
DefaultTokenProviderandDefaultAccountProvider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/credential/default_provider_test.go` around lines 7 - 13, Add a runtime behavioral test that exercises provider selection and fallback through DefaultTokenProvider and DefaultAccountProvider instead of only asserting interface conformance; specifically, register at least two mock providers (one that returns a valid token/account and one that errors or is lower priority), ensure the DefaultTokenProvider and DefaultAccountProvider lookup pipelines prefer the higher-priority provider, and verify that when the preferred provider fails the fallback provider is used. Use the same public constructors/types (DefaultTokenProvider, DefaultAccountProvider) and any registry or resolver entry points your code exposes to add/remove test providers so the test sets up providers, calls the resolve methods on DefaultTokenProvider/DefaultAccountProvider, asserts the returned token/account and errors, and then cleans up the test registry state.shortcuts/drive/drive_download.go (1)
73-80: Consider using a more accurate error code for filesystem errors.Lines 74 and 79 use
"api_error"as the error code for local filesystem operations (directory creation and file writing). A code like"filesystem_error"or"io_error"would be more accurate for debugging and monitoring purposes.Suggested improvement
if err := vfs.MkdirAll(filepath.Dir(safePath), 0700); err != nil { - return output.Errorf(output.ExitInternal, "api_error", "cannot create parent directory: %s", err) + return output.Errorf(output.ExitInternal, "io_error", "cannot create parent directory: %s", err) } sizeBytes, err := validate.AtomicWriteFromReader(safePath, resp.Body, 0600) if err != nil { - return output.Errorf(output.ExitInternal, "api_error", "cannot create file: %s", err) + return output.Errorf(output.ExitInternal, "io_error", "cannot create file: %s", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_download.go` around lines 73 - 80, The returns for filesystem failures use the generic "api_error" code; update the two error returns that call vfs.MkdirAll(filepath.Dir(safePath), ...) and validate.AtomicWriteFromReader(safePath, resp.Body, ...) to use a more accurate code (e.g., "filesystem_error" or "io_error") instead of "api_error" so filesystem/create-file failures are correctly categorized in output.Errorf calls while keeping the existing error messages intact.shortcuts/sheets/sheet_export.go (1)
136-142: Same suggestion: use accurate error codes for filesystem errors.As noted for
drive_download.go, the"api_error"code at lines 137 and 142 is misleading for local filesystem errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/sheets/sheet_export.go` around lines 136 - 142, The filesystem error handling in the block that creates parent directories and writes the file (calls to vfs.MkdirAll and validate.AtomicWriteFromReader using safePath and resp.Body) incorrectly uses the "api_error" error code; change the output.Errorf calls to use a filesystem-specific error code (e.g. "fs_error" or "filesystem_error") and keep the same messages and exit code (output.ExitInternal) so logs accurately reflect local filesystem failures instead of API errors.cmd/config/strict_mode_test.go (2)
73-89: Consider checking errors in multi-step tests.In
TestStrictMode_SetOff_Profile, line 78 ignores the error from the firstcmd.Execute(). If setting "bot" fails, the test continues with incorrect state. Similarly inTestStrictMode_Resetat line 110.Suggested improvement
func TestStrictMode_SetOff_Profile(t *testing.T) { setupStrictModeTestConfig(t) f, _, _, _ := cmdutil.TestFactory(t, &core.CliConfig{AppID: "test-app", AppSecret: "secret"}) cmd := NewCmdConfigStrictMode(f) cmd.SetArgs([]string{"bot"}) - cmd.Execute() + if err := cmd.Execute(); err != nil { + t.Fatalf("setup: setting bot failed: %v", err) + } cmd = NewCmdConfigStrictMode(f)🤖 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 73 - 89, The test TestStrictMode_SetOff_Profile (and similarly TestStrictMode_Reset) ignores the result of the first cmd.Execute() call (the invocation that sets "bot"), which can leave the test in a bad state; update the test to check the error returned by that first cmd.Execute() and fail the test if non-nil (e.g., use t.Fatal or t.Fatalf) before continuing, locating the calls to cmd.Execute() in TestStrictMode_SetOff_Profile and TestStrictMode_Reset to add the error check.
51-52: Consider checking errors fromLoadMultiAppConfig.Multiple tests ignore the error return from
core.LoadMultiAppConfig()(lines 51, 66, 84, 99, 116). While unlikely to fail in test context, checking errors would make test failures more diagnosable.Example fix
- multi, _ := core.LoadMultiAppConfig() + multi, err := core.LoadMultiAppConfig() + if err != nil { + t.Fatalf("LoadMultiAppConfig() error: %v", err) + }Also applies to: 66-67, 84-85, 99-100, 116-117
🤖 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 51 - 52, Tests currently ignore the error return from core.LoadMultiAppConfig() (called in strict_mode_test.go and elsewhere), which hides failures; update each test that does "multi, _ := core.LoadMultiAppConfig()" to capture the error (e.g., "multi, err := core.LoadMultiAppConfig()") and assert/handle it (t.Fatalf or require.NoError) before calling multi.CurrentAppConfig("") so failures are reported clearly; apply the same pattern for all occurrences at the lines noted that call LoadMultiAppConfig.internal/cmdutil/factory_default.go (1)
68-69: Misleading comment: AppSecret is not a placeholder.The comment says "placeholder AppSecret" but line 129 passes
acct.AppSecretwhich is the actual secret from the resolved account.📝 Fix misleading comment
- // Phase 4: LarkClient from Credential (placeholder AppSecret) + // Phase 4: LarkClient from Credential f.LarkClient = cachedLarkClientFunc(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/factory_default.go` around lines 68 - 69, The comment above the Lark client initialization is misleading — it says "placeholder AppSecret" even though the real account secret (acct.AppSecret) is passed into cachedLarkClientFunc; update the comment near f.LarkClient = cachedLarkClientFunc(f) to remove or replace "placeholder AppSecret" with wording that indicates the real account AppSecret is used (reference symbols: f.LarkClient, cachedLarkClientFunc, acct.AppSecret).cmd/service/service.go (1)
258-262: Silently swallowing token resolution errors may mask misconfigurations.When
cred.ResolveTokenfails or returns nil/empty scopes, the function silently returns nil, allowing the API call to proceed without scope validation. This could mask credential configuration issues.Consider logging a debug/warning message when token resolution fails to aid troubleshooting, while still allowing the command to proceed.
♻️ Suggested improvement
func checkServiceScopes(ctx context.Context, cred *credential.CredentialProvider, identity core.Identity, config *core.CliConfig, method map[string]interface{}, scopes []interface{}) error { result, err := cred.ResolveToken(ctx, credential.NewTokenSpec(identity, config.AppID)) - if err != nil || result == nil || result.Scopes == "" { + if err != nil { + // Token resolution failed; skip scope check (API may still succeed) + return nil + } + if result == nil || result.Scopes == "" { + // No scopes available; skip pre-check return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/service/service.go` around lines 258 - 262, In checkServiceScopes, do not silently swallow errors from cred.ResolveToken: when cred.ResolveToken(ctx, credential.NewTokenSpec(identity, config.AppID)) returns an error, a nil result, or an empty result.Scopes, emit a debug/warning log (e.g., via the existing logger in config or a context-aware logger) that includes the error message and identity/AppID so operators can troubleshoot, then continue to return nil to preserve the current behavior; update handling around the ResolveToken call and use the function name checkServiceScopes and symbols cred.ResolveToken, credential.NewTokenSpec, result and result.Scopes to locate where to add the log.shortcuts/im/helpers.go (1)
313-323: Stream plain file URLs instead of buffering them.This branch always builds a
mediaBufferformediaKindFile, even whens.withDurationis false. A normal--fileURL now gets fully copied into memory before upload for no functional gain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/helpers.go` around lines 313 - 323, The code always constructs a media buffer via newMediaBuffer even when s.withDuration is false, causing full in-memory copies for plain file URLs; change the flow so newMediaBuffer is only called when s.withDuration is true (to obtain mb.Duration()), and when s.withDuration is false create or use a streaming reader for s.value and call uploadFileFromReader with that streaming reader and the file metadata (avoiding mb.Reader()/mb.FileName()/mb.FileType()). Keep the existing call-site uploadFileFromReader and only switch the source/metadata to the streaming path when not using mb so the buffer is only allocated when duration is needed.internal/core/config_strict_mode_test.go (1)
17-19: Prefer structural assertions over exact JSON string equality.Line 17 couples the test to exact byte ordering/format. Asserting parsed keys/values is more stable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/config_strict_mode_test.go` around lines 17 - 19, Replace the exact JSON string equality with structural checks: unmarshal data into a map[string]interface{} (or a small struct), assert that "apps" is a slice of length 1, and that the first app object has appId == "a", appSecret == "s", brand == "feishu"; also assert that the "users" field is either absent or empty as required by strict mode (check absence in the map or that len(users)==0). Use json.Unmarshal and t.Fatalf/t.Errorf for failures; reference the existing variable data and the apps entry when making assertions.internal/credential/credential_provider.go (1)
84-117: Consider logging non-BlockError failures for debugging.When
ResolveTokenreturns an error that isn't aBlockError(line 98), the error is silently swallowed as the loop continues. While this is acceptable for a non-fatal enrichment path, logging these errors would help diagnose provider issues.🔧 Optional: Add debug logging for swallowed errors
if err != nil { var blockErr *extcred.BlockError if errors.As(err, &blockErr) { return nil // provider explicitly blocks UAT; skip enrichment } + // Log non-blocking errors for debugging (optional) + // log.Printf("enrichUserInfo: provider %T returned error: %v", prov, err) continue }🤖 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 84 - 117, In enrichUserInfo, when prov.ResolveToken returns an error that is not a BlockError (the branch after errors.As in the providers loop), add a debug/info log that records the error and identifies the provider (e.g., provider ID/name or the prov variable) before continuing; keep the existing behavior of skipping BlockError (return nil) and continuing on other errors, but emit the log so non-BlockError failures are visible for debugging (use the existing logger on CredentialProvider, e.g., p.logger, or the standard log package if no logger field exists).
🤖 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 317-329: When multi.FindApp(config.ProfileName) returns nil the
code currently silently skips updating Users; change this so the login does not
pretend success: if app == nil return a visible error (use output.Errorf with a
clear message like "profile %s not found" and an internal exit code) instead of
proceeding, and keep the existing logic that prunes tokens via
larkauth.RemoveStoredToken, sets app.Users = []core.AppUser{{UserOpenId: openId,
UserName: userName}}, and calls core.SaveMultiAppConfig(multi) when app exists;
apply the same nil-profile handling to the other identical block that mirrors
this logic.
In `@cmd/config/show.go`:
- Around line 48-51: The command currently prints "No active profile found."
when config.CurrentAppConfig(f.Invocation.Profile) returns nil but still returns
nil; change this to return a non-zero error instead: construct and return an
error (e.g. via fmt.Errorf) that includes the requested profile name when
f.Invocation.Profile is set, and preserve the existing ErrOut print or replace
it with the error message; update the block around app :=
config.CurrentAppConfig(f.Invocation.Profile) in cmd/config/show.go (the code
handling nil app) to return that error so the command exits non-zero on missing
profile.
In `@cmd/config/strict_mode.go`:
- Around line 44-47: The current check immediately returns if app == nil, which
blocks commands using the --global flag; change the validation so the nil-app
early return only runs for non-global operations. Concretely, in the block where
app := multi.CurrentAppConfig(f.Invocation.Profile) and the subsequent if app ==
nil return statement, gate that return behind a check for the global flag (i.e.,
only enforce app != nil when the command is not running with --global). Keep the
rest of the flow unchanged so global set/show commands bypass the
profile-required validation.
In `@cmd/profile/add.go`:
- Around line 72-75: The current code in cmd/profile/add.go calls
core.LoadMultiAppConfig() and always replaces errors by creating a new
&core.MultiAppConfig{}, which risks overwriting a corrupt/unreadable config;
change the logic in the LoadMultiAppConfig() caller so that after calling
core.LoadMultiAppConfig() you only create a new MultiAppConfig when the error
indicates a missing file (e.g. os.IsNotExist or a specific "not found" error
from core), but for other errors (parse/permission) return or propagate the
error instead of silently continuing; reference the call site
(core.LoadMultiAppConfig and the local variable multi) and add an explicit
conditional that distinguishes not-found from other failures and handles each
appropriately.
In `@cmd/profile/remove.go`:
- Around line 52-71: The code currently removes secrets/tokens before persisting
the updated multi-app config which can leave the system in a partially
destructive state if core.SaveMultiAppConfig fails; change the order so you
first remove the app from multi.Apps (the slice append operation) and update
multi.CurrentApp/multi.PreviousApp as currently done, then call
core.SaveMultiAppConfig(multi) and only after that succeeds call
core.RemoveSecretStore(app.AppSecret, f.Keychain) and
larkauth.RemoveStoredToken(app.AppId, user.UserOpenId) for each user; keep the
same functions and slice manipulation (multi.Apps = append(...)), but move the
persistence step (core.SaveMultiAppConfig) before the calls to
core.RemoveSecretStore and larkauth.RemoveStoredToken so secrets are only
deleted after a successful save.
In `@cmd/profile/use.go`:
- Around line 34-37: The current handler for core.LoadMultiAppConfig() hides
non-missing errors by always returning output.ErrWithHint(... "not configured"),
so change the error handling in the use command to only map the specific “config
missing” sentinel/error to output.ErrWithHint(output.ExitValidation, "config",
"not configured", "run: lark-cli config init") and for any other error
(parse/permission/IO) return the original err unchanged; locate the call to
core.LoadMultiAppConfig() and the surrounding if err { ... } block and implement
a conditional that inspects/compares err against the missing-config sentinel (or
error type) before wrapping it with output.ErrWithHint, otherwise propagate err.
In `@cmd/prune.go`:
- Around line 69-73: Summary: the current pruning logic force-unhides group
commands by setting child.Hidden = false when child.HasAvailableSubCommands() is
true; instead preserve the original Hidden state. Fix: in the switch handling
child commands (use child.HasAvailableSubCommands() and child.Commands()),
remove the assignment child.Hidden = false so we do not unhide groups during
pruning; keep the branch that sets child.Hidden = true when
len(child.Commands()) > 0 so empty groups can be hidden, but otherwise leave
child.Hidden untouched to preserve its pre-prune value.
In `@extension/credential/env/env_test.go`:
- Around line 31-36: TestResolveAccount_NeitherSet calls
(&Provider{}).ResolveAccount and expects nil,nil but can be flaky if LARK_*
environment variables are set on the host; before invoking ResolveAccount clear
or unset all relevant env vars (all LARK_* keys) in the test setup so the
provider runs in a clean environment and then restore them after the test; apply
the same pattern to the other tests in this file that assume unset environment
(the tests mentioned around the later ranges) so each test explicitly clears
LARK_* before exercising Provider.ResolveAccount or related code paths.
In `@extension/credential/env/env.go`:
- Around line 37-54: The current switch on LARKSUITE_CLI_STRICT_MODE falls
through unknown values to token inference; change it to normalize
(strings.ToLower) and validate the env var: if LARKSUITE_CLI_STRICT_MODE is
non-empty and equals "bot" set acct.SupportedIdentities =
credential.SupportsBot, if "user" set credential.SupportsUser, if "off" set
credential.SupportsAll; if the env var is non-empty but none of these values,
return/propagate an error (fail closed) instead of inferring; only perform the
hasUAT/hasTAT inference when LARKSUITE_CLI_STRICT_MODE is unset/empty so
acct.SupportedIdentities is not widened on typos.
In `@internal/client/client.go`:
- Around line 81-90: The code calls c.Credential.ResolveToken(ctx,
credential.NewTokenSpec(as, c.Config.AppID)) and immediately uses result.Token
(in branches that set req.SupportedAccessTokenTypes and append
larkcore.WithTenantAccessToken/WithUserAccessToken), but TokenProvider can
return (nil, nil); add a nil check for result after ResolveToken in the
DoSDKRequest and DoStream flows (and the other similar sites referenced) and if
result is nil return a clear authentication error (or wrap/convert to the
existing auth error type) instead of dereferencing result.Token; ensure the
check covers both bot and user branches so the code doesn’t panic when
ResolveToken returns (nil, nil).
- Around line 125-133: The code sets httpClient.Timeout (via httpClient :=
*c.HTTP; httpClient.Timeout = cfg.timeout) which will abort response body reads
and can cut off healthy streaming responses; remove the assignment to
httpClient.Timeout and instead rely solely on the request context deadline
created by context.WithTimeout when cfg.timeout > 0 (using requestCtx and
cancel) so streaming responses are not prematurely terminated; keep the copy of
c.HTTP (httpClient := *c.HTTP) if needed for other per-request changes but do
not set httpClient.Timeout = cfg.timeout.
In `@internal/core/config_strict_mode_test.go`:
- Around line 16-26: The test currently ignores errors from
json.Marshal/json.Unmarshal which can hide failures; update all
Marshal/Unmarshal calls in this test (the marshaling of m after setting
StrictMode and the subsequent Unmarshal into parsed, as well as the other
Marshal/Unmarshal occurrences noted) to capture the error and fail the test on
error (use t.Fatalf or t.Fatal with the error), e.g., check err :=
json.Marshal(m) and err := json.Unmarshal(data, &parsed) and assert err == nil
before proceeding; this applies to the Marshal/Unmarshal around m.StrictMode,
the parsed map.Unmarshal, and the other instances in the same file.
In `@internal/credential/credential_provider_test.go`:
- Around line 95-99: The test for AccountCached is currently ignoring errors
from cp.ResolveAccount so a1 == a2 can be true when both are nil; change the
test to capture and assert both errors are nil (e.g., err1, err2 from
cp.ResolveAccount) before comparing pointers, or explicitly fail if either err
is non-nil, then assert that a1 and a2 are the same pointer; locate the calls to
cp.ResolveAccount and variables a1/a2 in the test and add the nil-error checks
and appropriate test failures before the pointer equality assertion.
In `@internal/credential/integration_test.go`:
- Around line 49-67: The test TestFullChain_Fallthrough (and the other tests
around lines 78-112) relies on the env provider but doesn't isolate environment
variables; update the tests to clear and restore all env vars the envprovider
reads (e.g., token and strict-mode vars and any provider-specific env keys)
before creating ep so the provider always falls through deterministically — in
Go, prefer using t.Setenv("VAR_NAME", "") or explicitly unset/restore around the
test; ensure the same isolation change is applied to the other tests referenced
so external env cannot alter provider-chain behavior.
In `@shortcuts/common/runner.go`:
- Around line 91-95: AccessToken currently dereferences result.Token even when
Credential.ResolveToken can return (nil, nil); update it to handle the
unresolved-token path by checking the ResolveToken return before accessing
Token: call ctx.Factory.Credential.ResolveToken(...), if err != nil return
output.ErrAuth(...), then if result == nil (or result.Token is empty) return an
auth error (e.g., output.ErrAuth("no access token resolved")) instead of
dereferencing; keep the same function names (AccessToken, ResolveToken,
credential.NewTokenSpec) so the guard is placed immediately after the
ResolveToken call.
In `@shortcuts/im/helpers.go`:
- Around line 327-339: The resolveLocalMedia function calls parseMediaDuration
on s.value before uploadFileToIM can run validate.SafeInputPath, allowing an
unsafe filesystem open for audio/video files; fix by validating the input path
first: in resolveLocalMedia call validate.SafeInputPath (or the equivalent path
validation helper used by uploadFileToIM) on s.value before calling
detectIMFileType or parseMediaDuration so any local file is checked/normalized
first, then proceed to parseMediaDuration and finally call uploadFileToIM (keep
the existing uploadImageToIM branch unchanged).
---
Outside diff comments:
In `@internal/validate/path.go`:
- Around line 63-88: The code mixes host FS calls (filepath.EvalSymlinks) with
vfs probes (vfs.Getwd/vfs.Lstat), which breaks non-default VFS implementations;
replace the direct calls to filepath.EvalSymlinks (both the
EvalSymlinks(resolved) and EvalSymlinks(cwd) usages) with a vfs-level
symlink-resolution API (e.g., add and call vfs.EvalSymlinks or equivalent) and
ensure resolveNearestAncestor and isUnderDir operate entirely via the vfs API so
all existence checks and canonicalization stay on the same filesystem boundary.
In `@shortcuts/common/runner.go`:
- Around line 465-472: Call CheckStrictMode before letting ResolveAs coerce the
identity: extract the raw flag value (asFlag) and call
f.CheckStrictMode(core.Identity(asFlag)) first, returning any error; only after
strict-mode passes, call f.ResolveAs(cmd, core.Identity(asFlag)) to apply
defaults/auto-detection and continue using the resolved identity. Update
resolveShortcutIdentity (and use f.CheckStrictMode and f.ResolveAs references)
so strict-mode validation runs against the original input before coercion.
In `@shortcuts/im/im_messages_resources_download.go`:
- Around line 145-156: After calling runtime.DoAPIStream you must validate the
HTTP status before treating the response as a successful download: check
downloadResp.StatusCode (the value returned by runtime.DoAPIStream) and if it's
>= 400 return an error (include status and any returned body/message) instead of
proceeding to write the file; update the code around the runtime.DoAPIStream
call (referencing downloadResp, runtime.DoAPIStream and
defaultIMResourceDownloadTimeout) to perform this local check and return early
on error.
In `@shortcuts/sheets/sheet_export.go`:
- Around line 115-143: When outputPath is empty the code calls runtime.Out(...)
but does not return, so the subsequent download and calls to
validate.SafeOutputPath and validate.AtomicWriteFromReader run and fail; fix by
adding an early return immediately after the runtime.Out(...) call in the block
that checks if outputPath == "" so the function exits successfully (no download
attempted) when only file_token/ticket are being emitted; locate the check for
outputPath, the runtime.Out invocation, and add the return there to skip the
DoAPIStream, SafeOutputPath, vfs.MkdirAll and AtomicWriteFromReader logic.
---
Nitpick comments:
In `@cmd/config/init.go`:
- Around line 255-283: The existing "result.Mode == 'existing' && result.AppID
!= ''" branch mixes four paths and should be extracted into a helper to simplify
logic: create a function (e.g., updateExistingAppConfig(existing, result, opts)
or core.UpdateExistingApp) that encapsulates the profile vs non-profile
branching, uses existing.FindAppIndex(opts.ProfileName) to update
Apps[idx].AppId/Brand/Lang when profile mode and existing, returns a validation
error when profile name not found, and otherwise uses
existing.CurrentAppConfig("") to update AppId/Brand/Lang or return the
validation error when nil; ensure the caller only handles calling this helper
and a single core.SaveMultiAppConfig(existing) invocation and returns any error
from those calls (keeping references to result.Mode, result.AppID,
opts.ProfileName, existing.FindAppIndex, existing.CurrentAppConfig, and
core.SaveMultiAppConfig).
In `@cmd/config/strict_mode_test.go`:
- Around line 73-89: The test TestStrictMode_SetOff_Profile (and similarly
TestStrictMode_Reset) ignores the result of the first cmd.Execute() call (the
invocation that sets "bot"), which can leave the test in a bad state; update the
test to check the error returned by that first cmd.Execute() and fail the test
if non-nil (e.g., use t.Fatal or t.Fatalf) before continuing, locating the calls
to cmd.Execute() in TestStrictMode_SetOff_Profile and TestStrictMode_Reset to
add the error check.
- Around line 51-52: Tests currently ignore the error return from
core.LoadMultiAppConfig() (called in strict_mode_test.go and elsewhere), which
hides failures; update each test that does "multi, _ :=
core.LoadMultiAppConfig()" to capture the error (e.g., "multi, err :=
core.LoadMultiAppConfig()") and assert/handle it (t.Fatalf or require.NoError)
before calling multi.CurrentAppConfig("") so failures are reported clearly;
apply the same pattern for all occurrences at the lines noted that call
LoadMultiAppConfig.
In `@cmd/profile/list.go`:
- Around line 17-25: The profileListItem struct currently marshals User and
TokenStatus as empty strings for profiles without values; update the struct tags
for the User and TokenStatus fields on profileListItem to use the `omitempty`
json option so those keys are omitted when empty (i.e., change the `json:"user"`
and `json:"tokenStatus"` tags to include `omitempty`), leaving other fields
unchanged.
In `@cmd/service/service.go`:
- Around line 258-262: In checkServiceScopes, do not silently swallow errors
from cred.ResolveToken: when cred.ResolveToken(ctx,
credential.NewTokenSpec(identity, config.AppID)) returns an error, a nil result,
or an empty result.Scopes, emit a debug/warning log (e.g., via the existing
logger in config or a context-aware logger) that includes the error message and
identity/AppID so operators can troubleshoot, then continue to return nil to
preserve the current behavior; update handling around the ResolveToken call and
use the function name checkServiceScopes and symbols cred.ResolveToken,
credential.NewTokenSpec, result and result.Scopes to locate where to add the
log.
In `@internal/cmdutil/factory_default.go`:
- Around line 68-69: The comment above the Lark client initialization is
misleading — it says "placeholder AppSecret" even though the real account secret
(acct.AppSecret) is passed into cachedLarkClientFunc; update the comment near
f.LarkClient = cachedLarkClientFunc(f) to remove or replace "placeholder
AppSecret" with wording that indicates the real account AppSecret is used
(reference symbols: f.LarkClient, cachedLarkClientFunc, acct.AppSecret).
In `@internal/core/config_strict_mode_test.go`:
- Around line 17-19: Replace the exact JSON string equality with structural
checks: unmarshal data into a map[string]interface{} (or a small struct), assert
that "apps" is a slice of length 1, and that the first app object has appId ==
"a", appSecret == "s", brand == "feishu"; also assert that the "users" field is
either absent or empty as required by strict mode (check absence in the map or
that len(users)==0). Use json.Unmarshal and t.Fatalf/t.Errorf for failures;
reference the existing variable data and the apps entry when making assertions.
In `@internal/core/config.go`:
- Around line 132-152: The ValidateProfileName function currently rejects
control characters via r <= 0x1F but does not explicitly list newline chars;
update the switch in ValidateProfileName to include '\n' and '\r' alongside the
other forbidden characters to make the intent explicit and consistent with other
validators (leave the control-character check intact so behavior is unchanged).
In `@internal/core/types.go`:
- Around line 16-23: ParseBrand currently does a case-sensitive comparison so
inputs like "Lark" or "LARK" will fall back to BrandFeishu; update ParseBrand to
perform a case-insensitive match (use strings.EqualFold(value, "lark")) and add
the "strings" import so inputs with different casing map to BrandLark while
still returning BrandFeishu for unrecognized values; keep the function signature
and return values (BrandLark, BrandFeishu) unchanged.
In `@internal/credential/credential_provider.go`:
- Around line 84-117: In enrichUserInfo, when prov.ResolveToken returns an error
that is not a BlockError (the branch after errors.As in the providers loop), add
a debug/info log that records the error and identifies the provider (e.g.,
provider ID/name or the prov variable) before continuing; keep the existing
behavior of skipping BlockError (return nil) and continuing on other errors, but
emit the log so non-BlockError failures are visible for debugging (use the
existing logger on CredentialProvider, e.g., p.logger, or the standard log
package if no logger field exists).
In `@internal/credential/default_provider_test.go`:
- Around line 7-13: Add a runtime behavioral test that exercises provider
selection and fallback through DefaultTokenProvider and DefaultAccountProvider
instead of only asserting interface conformance; specifically, register at least
two mock providers (one that returns a valid token/account and one that errors
or is lower priority), ensure the DefaultTokenProvider and
DefaultAccountProvider lookup pipelines prefer the higher-priority provider, and
verify that when the preferred provider fails the fallback provider is used. Use
the same public constructors/types (DefaultTokenProvider,
DefaultAccountProvider) and any registry or resolver entry points your code
exposes to add/remove test providers so the test sets up providers, calls the
resolve methods on DefaultTokenProvider/DefaultAccountProvider, asserts the
returned token/account and errors, and then cleans up the test registry state.
In `@shortcuts/drive/drive_download.go`:
- Around line 73-80: The returns for filesystem failures use the generic
"api_error" code; update the two error returns that call
vfs.MkdirAll(filepath.Dir(safePath), ...) and
validate.AtomicWriteFromReader(safePath, resp.Body, ...) to use a more accurate
code (e.g., "filesystem_error" or "io_error") instead of "api_error" so
filesystem/create-file failures are correctly categorized in output.Errorf calls
while keeping the existing error messages intact.
In `@shortcuts/im/helpers.go`:
- Around line 313-323: The code always constructs a media buffer via
newMediaBuffer even when s.withDuration is false, causing full in-memory copies
for plain file URLs; change the flow so newMediaBuffer is only called when
s.withDuration is true (to obtain mb.Duration()), and when s.withDuration is
false create or use a streaming reader for s.value and call uploadFileFromReader
with that streaming reader and the file metadata (avoiding
mb.Reader()/mb.FileName()/mb.FileType()). Keep the existing call-site
uploadFileFromReader and only switch the source/metadata to the streaming path
when not using mb so the buffer is only allocated when duration is needed.
In `@shortcuts/mail/mail_watch_test.go`:
- Around line 99-100: The test currently only asserts the URL for the profile
dry-run call (checking apis[1].URL against mailboxPath("me", "profile")) but
misses the HTTP method; update the assertion to also check the method (e.g.,
assert apis[1].Method == "GET" and call t.Fatalf with a clear message if it
differs) so a regression that changes the request verb will fail; modify the
test near the existing URL check referencing the apis slice and
mailboxPath("me","profile") to include this additional method assertion.
In `@shortcuts/sheets/sheet_export.go`:
- Around line 136-142: The filesystem error handling in the block that creates
parent directories and writes the file (calls to vfs.MkdirAll and
validate.AtomicWriteFromReader using safePath and resp.Body) incorrectly uses
the "api_error" error code; change the output.Errorf calls to use a
filesystem-specific error code (e.g. "fs_error" or "filesystem_error") and keep
the same messages and exit code (output.ExitInternal) so logs accurately reflect
local filesystem failures instead of API errors.
🪄 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: 14d486bc-8c74-4e89-a5d2-df5d325f72c1
📒 Files selected for processing (116)
.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/list.gocmd/auth/login.gocmd/auth/login_strict_test.gocmd/auth/logout.gocmd/bootstrap.gocmd/bootstrap_test.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/global_flags.gocmd/profile/add.gocmd/profile/list.gocmd/profile/profile.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/prune.gocmd/prune_test.gocmd/root.gocmd/root_e2e_test.gocmd/root_integration_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.goextension/credential/types_test.gointernal/auth/uat_client.gointernal/client/client.gointernal/client/client_test.gointernal/client/option.gointernal/client/response.gointernal/cmdutil/annotations.gointernal/cmdutil/annotations_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/factory_test.gointernal/cmdutil/secheader.gointernal/cmdutil/testing.gointernal/core/config.gointernal/core/config_strict_mode_test.gointernal/core/config_test.gointernal/core/secret_resolve.gointernal/core/strict_mode.gointernal/core/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.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_other.gointernal/lockfile/lockfile.gointernal/registry/remote.gointernal/update/update.gointernal/validate/atomicwrite.gointernal/validate/path.gointernal/vfs/default.gointernal/vfs/fs.gointernal/vfs/osfs.gointernal/vfs/osfs_test.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/base_shortcut_helpers.goshortcuts/base/record_upload_attachment.goshortcuts/base/workflow_execute_test.goshortcuts/calendar/calendar_test.goshortcuts/common/helpers.goshortcuts/common/runner.goshortcuts/common/validate.goshortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_test.goshortcuts/doc/doc_media_upload.goshortcuts/drive/drive_download.goshortcuts/drive/drive_export_common.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_upload.goshortcuts/event/pipeline.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.goshortcuts/im/helpers_network_test.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_resources_download.goshortcuts/im/im_messages_search_execute_test.goshortcuts/mail/draft/patch.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/helpers.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.goshortcuts/sheets/sheet_export.goshortcuts/vc/vc_notes.goshortcuts/vc/vc_notes_test.go
💤 Files with no reviewable changes (16)
- cmd/api/api_test.go
- cmd/service/service_test.go
- shortcuts/base/base_advperm_test.go
- shortcuts/base/base_dashboard_execute_test.go
- shortcuts/base/base_execute_test.go
- shortcuts/base/workflow_execute_test.go
- shortcuts/calendar/calendar_test.go
- shortcuts/im/convert_lib/helpers_test.go
- shortcuts/im/convert_lib/thread_test.go
- shortcuts/im/convert_lib/merge_test.go
- shortcuts/base/base_form_execute_test.go
- shortcuts/im/im_messages_search_execute_test.go
- shortcuts/vc/vc_notes_test.go
- shortcuts/doc/doc_media_test.go
- cmd/root_e2e_test.go
- shortcuts/base/base_role_test.go
Change-Id: If61578631f5698f7ca2d9a946ca59753651463fb
Greptile SummaryThis PR introduces strict-mode identity filtering, multi-profile management ( Confidence Score: 5/5Safe to merge; all prior P1 concerns are resolved and the only remaining findings are P2 style notes No new P0 or P1 issues found. The three previously flagged concerns (brand default in env provider, missing AppID in TokenSpec, double timeout in DoStream) are all addressed in the current implementation. The two inline P2 comments describe dead code and an inconsistent lookup function — neither affects correctness or security. internal/credential/credential_provider.go (P2 only: unreachable fallback in ResolveToken); cmd/auth/login.go (P2 only: findProfileByName vs FindApp divergence) Important Files Changed
Sequence DiagramsequenceDiagram
participant main
participant Factory
participant CredentialProvider
participant EnvProvider
participant DefaultAccountProvider
participant enrichUserInfo
participant Command
main->>Factory: NewDefault(inv)
note over Factory: Phase 1: HttpClient
Factory->>CredentialProvider: buildCredentialProvider(deps)
note over Factory: Phase 2: Credential (sole data source)
Factory->>Factory: ResolveStrictMode (root.go pruning)
Factory->>CredentialProvider: ResolveAccount(ctx) [sync.Once]
CredentialProvider->>EnvProvider: ResolveAccount(ctx)
alt env vars present
EnvProvider-->>CredentialProvider: *Account{AppID, Brand, SupportedIdentities}
CredentialProvider->>enrichUserInfo: TryResolveToken(UAT, acct.AppID)
enrichUserInfo->>enrichUserInfo: fetchUserInfo via HTTP
enrichUserInfo-->>CredentialProvider: OpenID, Name
else env vars absent
EnvProvider-->>CredentialProvider: nil, nil
CredentialProvider->>DefaultAccountProvider: ResolveAccount(ctx)
DefaultAccountProvider-->>CredentialProvider: *Account from config.json + strictModeToIdentitySupport
end
CredentialProvider-->>Factory: SupportedIdentities → StrictMode
Factory->>Factory: pruneForStrictMode(rootCmd, mode)
note over Factory: Phase 3: Config from Credential
note over Factory: Phase 4: LarkClient from Credential
Factory->>Command: runs
Command->>CredentialProvider: ResolveToken(ctx, spec)
CredentialProvider->>CredentialProvider: selectedCredentialSource (cached)
CredentialProvider-->>Command: *TokenResult{Token, Scopes}
Reviews (12): Last reviewed commit: "fix: remove file/stdin input support fro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Add framework-level support for reading flag values from files (@path) or stdin (-), solving the fundamental problem of passing complex text (markdown, multi-line content) via CLI arguments where shell escaping breaks content. Closes #239, fixes #163. - Add File/Stdin constants and Input field to Flag struct - Add resolveInputFlags() in runner pipeline (pre-Validate) - Support @@ escape for literal @ prefix - Guard against multiple stdin consumers - Auto-append "(supports @file, - for stdin)" to help text - Apply to: docs +create/+update --markdown, im +messages-send/+reply --text/--markdown/--content, task +comment --content, drive +add-comment --content Change-Id: I305a326d972417542aeadd70f37b74ea456461ef Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- task/minutes: remove unused tenant_access_token httpmock stubs (TestFactory's testDefaultToken provides tokens directly, so the HTTP stub was never consumed and failed verification) - registry: fix hasEmbeddedData() to check for actual services instead of just byte length (meta_data_default.json has empty services array) Change-Id: Ic7b5fc7f9de09137a7254fe1ddf47d24ade40587 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/registry/remote_test.go (1)
32-41: Clarify helper semantics to avoid ambiguous test gating.
hasEmbeddedData()now effectively checks for non-empty services, not just embedded payload presence/parseability. That diverges from production embedded-load criteria and may skip/route tests differently when embedded JSON is valid butservicesis empty/null. Consider renaming this helper (e.g.,hasEmbeddedServices) or splitting into two helpers (hasEmbeddedJSONvshasEmbeddedServices) and using each explicitly per test intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/remote_test.go` around lines 32 - 41, The current helper hasEmbeddedData() conflates presence/parseability of embeddedMetaJSON with having non-empty reg.Services; update tests by either renaming this function to hasEmbeddedServices() to reflect its true check (non-empty Services) or split it into two helpers—hasEmbeddedJSON() that returns true if embeddedMetaJSON unmarshal succeeds and hasEmbeddedServices() that checks len(reg.Services)>0—then update test usages to call the appropriate helper depending on whether the test is gating on parseable embedded JSON (use hasEmbeddedJSON) or on actual embedded service data (use hasEmbeddedServices); refer to the existing function name hasEmbeddedData, the variable embeddedMetaJSON, and the MergedRegistry type to locate and implement the change.shortcuts/common/runner.go (1)
523-530: Fail fast whenInputis attached to a non-string flag.
resolveInputFlags()always goes throughGetString/Set, soInputonbool,int, orstring_arrayflags will currently no-op or swallow the type mismatch until later. A small guard here would make the new metadata much harder to misuse.♻️ Proposed change
- raw, _ := rctx.Cmd.Flags().GetString(fl.Name) + raw, err := rctx.Cmd.Flags().GetString(fl.Name) + if err != nil { + return FlagErrorf("--%s: Input is only supported for string flags", fl.Name) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/runner.go` around lines 523 - 530, resolveInputFlags currently always calls GetString/Set on flags with Input metadata which hides type mismatches; update resolveInputFlags to lookup each flag (use rctx.Cmd.Flags().Lookup(fl.Name)) and fail fast if the flag's Value.Type() is not "string" (or otherwise not compatible with GetString/Set), returning a clear error for invalid Input usage instead of silently no-oping; this should be applied inside resolveInputFlags before calling GetString/Set so Input on bool/int/string_array is rejected early.
🤖 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/common/runner.go`:
- Around line 534-549: The current branch treats raw == "-" as always meaning
"read from stdin", preventing passing a literal "-" to flags (e.g., --text);
change the resolver to accept an explicit escape for a literal dash such as "\-"
(or another chosen sentinel) by first checking if raw equals the escape sequence
and, if so, set the flag value to "-" instead of reading stdin; otherwise
preserve the existing behavior where raw == "-" consumes rctx.IO().In, using the
same symbols fl.Name, Stdin, stdinUsed and rctx.IO().In, and update the flag
help/docs to document the escape.
---
Nitpick comments:
In `@internal/registry/remote_test.go`:
- Around line 32-41: The current helper hasEmbeddedData() conflates
presence/parseability of embeddedMetaJSON with having non-empty reg.Services;
update tests by either renaming this function to hasEmbeddedServices() to
reflect its true check (non-empty Services) or split it into two
helpers—hasEmbeddedJSON() that returns true if embeddedMetaJSON unmarshal
succeeds and hasEmbeddedServices() that checks len(reg.Services)>0—then update
test usages to call the appropriate helper depending on whether the test is
gating on parseable embedded JSON (use hasEmbeddedJSON) or on actual embedded
service data (use hasEmbeddedServices); refer to the existing function name
hasEmbeddedData, the variable embeddedMetaJSON, and the MergedRegistry type to
locate and implement the change.
In `@shortcuts/common/runner.go`:
- Around line 523-530: resolveInputFlags currently always calls GetString/Set on
flags with Input metadata which hides type mismatches; update resolveInputFlags
to lookup each flag (use rctx.Cmd.Flags().Lookup(fl.Name)) and fail fast if the
flag's Value.Type() is not "string" (or otherwise not compatible with
GetString/Set), returning a clear error for invalid Input usage instead of
silently no-oping; this should be applied inside resolveInputFlags before
calling GetString/Set so Input on bool/int/string_array is rejected early.
🪄 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: 01f9834f-5999-4554-96e6-e0e01827262c
📒 Files selected for processing (12)
internal/registry/remote_test.goshortcuts/common/runner.goshortcuts/common/runner_input_test.goshortcuts/common/types.goshortcuts/doc/docs_create.goshortcuts/doc/docs_update.goshortcuts/drive/drive_add_comment.goshortcuts/im/im_messages_reply.goshortcuts/im/im_messages_send.goshortcuts/minutes/minutes_download_test.goshortcuts/task/task_comment.goshortcuts/task/task_shortcut_test.go
💤 Files with no reviewable changes (2)
- shortcuts/task/task_shortcut_test.go
- shortcuts/minutes/minutes_download_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/im/im_messages_reply.go
Both cases intentionally return nil on error for graceful degradation: - profile list: show friendly message when config is not initialized - service: skip scope check when token resolution fails Change-Id: I7285c37277c9b0361a421ab00359244c2cd150b3 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/profile/list.go (1)
17-25: Consider addingomitemptyfor optional fields.When a profile has no configured users,
UserandTokenStatuswill output as empty strings ("user": "", "tokenStatus": ""). Addingomitemptywould produce cleaner JSON by omitting these fields when empty.Suggested change
type profileListItem struct { Name string `json:"name"` AppID string `json:"appId"` Brand core.LarkBrand `json:"brand"` Active bool `json:"active"` - User string `json:"user"` - TokenStatus string `json:"tokenStatus"` + User string `json:"user,omitempty"` + TokenStatus string `json:"tokenStatus,omitempty"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/profile/list.go` around lines 17 - 25, The profileListItem struct currently always emits empty strings for unset fields; update the json tags for the optional fields User and TokenStatus in the profileListItem type to include ",omitempty" (i.e., change the tags for User and TokenStatus to `json:"user,omitempty"` and `json:"tokenStatus,omitempty"`) so those fields are omitted from JSON when empty; run tests or marshal a sample profile to verify output.cmd/service/service.go (1)
258-262: Consider distinguishing context cancellation from other token resolution errors.The graceful degradation pattern is reasonable for credential provider failures, but swallowing
ctx.Err()(context cancellation/timeout) means the user won't see why the request didn't complete as expected. Context errors typically indicate user-initiated cancellation or deadline exceeded, which are worth surfacing.💡 Optional: Surface context errors
func checkServiceScopes(ctx context.Context, cred *credential.CredentialProvider, identity core.Identity, config *core.CliConfig, method map[string]interface{}, scopes []interface{}) error { result, err := cred.ResolveToken(ctx, credential.NewTokenSpec(identity, config.AppID)) + if ctx.Err() != nil { + return ctx.Err() + } if err != nil || result == nil || result.Scopes == "" { return nil //nolint:nilerr // skip scope check when token resolution fails or has no scopes }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/service/service.go` around lines 258 - 262, The current checkServiceScopes function swallows all ResolveToken errors (and returns nil) which hides context cancellation/timeouts; change the error handling after calling cred.ResolveToken(ctx, credential.NewTokenSpec(identity, config.AppID)) to detect if ctx.Err() is non-nil and return that context error (or wrap/propagate it) instead of returning nil, while preserving the existing graceful fallback for non-context token resolution failures; update the cred.ResolveToken error branch and the result==nil || result.Scopes=="" branch to only suppress non-context errors and ensure ctx.Err() is returned to surface cancellations/timeouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/profile/list.go`:
- Around line 17-25: The profileListItem struct currently always emits empty
strings for unset fields; update the json tags for the optional fields User and
TokenStatus in the profileListItem type to include ",omitempty" (i.e., change
the tags for User and TokenStatus to `json:"user,omitempty"` and
`json:"tokenStatus,omitempty"`) so those fields are omitted from JSON when
empty; run tests or marshal a sample profile to verify output.
In `@cmd/service/service.go`:
- Around line 258-262: The current checkServiceScopes function swallows all
ResolveToken errors (and returns nil) which hides context cancellation/timeouts;
change the error handling after calling cred.ResolveToken(ctx,
credential.NewTokenSpec(identity, config.AppID)) to detect if ctx.Err() is
non-nil and return that context error (or wrap/propagate it) instead of
returning nil, while preserving the existing graceful fallback for non-context
token resolution failures; update the cred.ResolveToken error branch and the
result==nil || result.Scopes=="" branch to only suppress non-context errors and
ensure ctx.Err() is returned to surface cancellations/timeouts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a9a17b3-ed1d-41c1-9cf5-08d5d4d04265
📒 Files selected for processing (2)
cmd/profile/list.gocmd/service/service.go
- runner.go: fail fast when Input is used on non-string flags - remote_test.go: rename hasEmbeddedData → hasEmbeddedServices - profile/list.go: add omitempty to optional JSON fields - service.go: surface context cancellation errors in scope check Change-Id: I7072d41f8c711b4b37c542e32dfd8150f42b13c0 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
shortcuts/common/runner.go (1)
537-558:⚠️ Potential issue | 🟡 MinorLiteral
"-"is still unreachable for input-backed flags.Any input-enabled string flag still treats raw
-as stdin, and the help text only documents that mode. That still leaves flags like--text/--markdownwith no way to send a literal dash. Please add and document an escape, parallel to@@for literal@.Also applies to: 627-636
🧹 Nitpick comments (1)
cmd/profile/list.go (1)
41-43: Consider distinguishing "not configured" from "config corrupted" errors.Currently all
LoadMultiAppConfig()errors produce the same "Not configured yet" message. If a user has a corrupted config file (malformed JSON, permission issues), this message could be misleading.That said, the
nolint:nilerrcomment indicates this is an intentional design choice for a friendlier UX in the common case.🔧 Optional: More nuanced error handling
func profileListRun(f *cmdutil.Factory) error { multi, err := core.LoadMultiAppConfig() if err != nil { - fmt.Fprintln(f.IOStreams.ErrOut, "Not configured yet. Run `lark-cli config init` to initialize.") - return nil //nolint:nilerr // graceful fallback: show friendly message instead of raw error + if os.IsNotExist(err) { + fmt.Fprintln(f.IOStreams.ErrOut, "Not configured yet. Run `lark-cli config init` to initialize.") + return nil + } + return fmt.Errorf("failed to load config: %w", err) }This would require importing
osand checking whetherLoadMultiAppConfigreturns or wrapsos.ErrNotExist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/profile/list.go` around lines 41 - 43, Replace the blanket "Not configured yet" fallback in the LoadMultiAppConfig error path by distinguishing a missing config from other failures: call LoadMultiAppConfig (the function currently returning err) and if errors.Is(err, os.ErrNotExist) or os.IsNotExist(errors.Unwrap(err)) print the friendly "Not configured yet..." to f.IOStreams.ErrOut and return nil, otherwise print a concise diagnostic including err (to f.IOStreams.ErrOut) and return the error; import "os" and "errors" as needed and update the nolint usage accordingly so only the not-exist case is treated as a graceful nil return while corrupted/permission errors propagate.
🤖 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/service/service.go`:
- Around line 259-264: After calling cred.ResolveToken(ctx,
credential.NewTokenSpec(identity, config.AppID)) re-check the context for
cancellation and return ctx.Err() (or propagate
context.Canceled/context.DeadlineExceeded) before taking the "skip scope check"
path; specifically, in the code around ResolveToken and the subsequent if that
currently returns nil, add a guard like if ctx.Err() != nil { return ctx.Err() }
so that cancellation during ResolveToken is honored, then keep the existing
behavior for non-cancellation resolution failures.
In `@internal/registry/remote_test.go`:
- Around line 33-41: The helper hasEmbeddedServices currently swallows JSON
parse errors (json.Unmarshal of embeddedMetaJSON into MergedRegistry) and
returns false, which hides malformed embedded payloads; change the error path to
fail fast: when json.Unmarshal returns an error, either panic or propagate the
error so tests fail immediately (e.g., replace the silent return false on
unmarshal error with a panic/log.Fatalf including the error and context) and if
you choose to propagate, update callers to handle the returned error; reference
functions/vars: hasEmbeddedServices, embeddedMetaJSON, MergedRegistry,
json.Unmarshal.
In `@shortcuts/common/runner.go`:
- Around line 350-355: Change checkScopePrereqs to return ([]string, error)
instead of just []string: when f.Credential.ResolveToken returns an error or
result is nil/empty, return nil and that error (if err==nil but ctx
canceled/expired, return ctx.Err()). Keep returning
auth.MissingScopes(result.Scopes, required) on success. Update callers (e.g.,
checkShortcutScopes and runShortcut) to handle the new error return: propagate
ctx.Err() up (or return the error) instead of treating nil as “no scopes”, and
ensure runShortcut aborts when the error indicates canceled/timeout. Also apply
the same signature and error propagation to the other similar helper used around
the later region referenced in the review.
- Around line 234-245: RuntimeContext.DoAPIStream must guard against a missing
token before calling APIClient.DoStream: call the token resolver from the auth
context (use ctx.As() -> TokenProvider.ResolveToken) and check the returned
token/result.Token for nil or empty; if no token is resolved either (a)
remove/avoid adding an empty Authorization header from the base options you pass
into ac.DoStream, or (b) return a clear error instead of delegating, to prevent
APIClient.DoStream from reading a nil/empty token and producing "Authorization:
Bearer " or panicking. Locate DoAPIStream, ctx.As(), TokenProvider.ResolveToken
and APIClient.DoStream to implement this guard.
---
Nitpick comments:
In `@cmd/profile/list.go`:
- Around line 41-43: Replace the blanket "Not configured yet" fallback in the
LoadMultiAppConfig error path by distinguishing a missing config from other
failures: call LoadMultiAppConfig (the function currently returning err) and if
errors.Is(err, os.ErrNotExist) or os.IsNotExist(errors.Unwrap(err)) print the
friendly "Not configured yet..." to f.IOStreams.ErrOut and return nil, otherwise
print a concise diagnostic including err (to f.IOStreams.ErrOut) and return the
error; import "os" and "errors" as needed and update the nolint usage
accordingly so only the not-exist case is treated as a graceful nil return while
corrupted/permission errors propagate.
🪄 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: 8d8166ca-f4b6-45a8-8612-d6b2b5029cce
📒 Files selected for processing (4)
cmd/profile/list.gocmd/service/service.gointernal/registry/remote_test.goshortcuts/common/runner.go
Change-Id: I83f6d424540eab9b1708944b9b6e26e8477cc60d
Change-Id: I38d5f98160b92adb62dc929ae73697ae5b3d64f8
Change-Id: Ia86d9bd19add9010176339ec4cc89deb033f5b4f
Change-Id: I40b2ffedc5c1db5e08e86b9472ea2b84fa02bb29
Change-Id: I5663a53e147577f0f1f533f67d12bea504e6b839
Change-Id: I289027dfd364c92205012feef6f05037066c035b
- im: fix double SafeInputPath in resolveLocalMedia → uploadImageToIM/ uploadFileToIM chain that rejected all local image/file uploads - credential: stop writing plain-text warnings to stderr, preserving JSON envelope contract for AI agent consumers - profile add: reject duplicate app-id to prevent keychain credential collisions across profiles - profile rename: exclude self when checking name uniqueness so renaming to own appId works correctly - config: replace bare fmt.Errorf with output.Errorf in save-failure paths (default_as, strict_mode ×2, profile add) - factory: remove unused resolveDefaultAs method (lint) Change-Id: I6aa0d064414016f367f1edb08dd0604adf7bf13d Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test triggers a data race: resetInit() writes package globals while a background goroutine from a previous test may still be reading them. The embedded-data path is covered by other tests. Change-Id: I7a0c3bf85a9fb337b9279c9053697f40a0c0a0d4 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Keep main's TestDriveUploadLargeFileUsesMultipart (replaces old reject test), deduplicate registerDriveBotTokenStub, and add missing os import. Change-Id: Iea18e9112c70a5c590af2f3deb686456310a03b0 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace raw string fields with typed enums for compile-time safety: - extension/credential: add Brand and Identity named types - internal/core: AppConfig.DefaultAs and CliConfig.DefaultAs → Identity - internal/credential: Account.DefaultAs and IdentityHint.DefaultAs → core.Identity The full data flow is now typed end-to-end: extcred.Brand → core.LarkBrand (named-type cast) extcred.Identity → core.Identity (named-type cast) No string intermediaries, no implicit conversions. Change-Id: I715b3b3f033fcb624010f1af9619e3562740ef08 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change-Id: Ibfac0703a5a28f3c6ba4a47bf40696028d0f3b90 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change-Id: If49704ca4612465a23bd30b755d6e72a35fc2349 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
autoDetectIdentity() is only called from tests, never from production code. Remove it along with its 3 test cases to reduce surface area before the upcoming ctx propagation refactor. Change-Id: I35a188860f17656f3e1fe9874f87f284985ae196
Private method resolveIdentityHint now accepts context.Context and passes it to CredentialProvider.ResolveIdentityHint instead of using context.Background(). The caller (ResolveAs) still uses context.Background() temporarily until its own signature is updated. Change-Id: I14634a4e0dc1d657d56936ba61a7b7a206da8ac4
ResolveStrictMode now accepts context.Context and passes it to CredentialProvider.ResolveAccount instead of using context.Background(). Callers in cobra RunE pass cmd.Context(); callers outside RunE (cmd/root.go startup, tests) use context.Background() explicitly. Change-Id: I31be48e548ac5ac5640a65f3bfdde4a53ed1dc7e
CheckStrictMode now accepts context.Context and forwards it to ResolveStrictMode. Callers pass cmd.Context() (cobra RunE) or opts.Ctx (APIOptions/ServiceMethodOptions). Change-Id: I47888519d4cae8c94054771c32aff075565a8cdc
ResolveAs now accepts context.Context as first parameter and forwards it to ResolveStrictMode and resolveIdentityHint. This completes the ctx propagation chain: all Factory methods that call CredentialProvider now receive ctx from cobra cmd.Context(). No more context.Background() calls remain in factory.go for credential provider operations. Change-Id: I6d10b6350e3b149470660de3e7855614314e8b29
JackZhao10086
previously approved these changes
Apr 7, 2026
Change-Id: I4a87d5a815b959f14cc4371b73dee4aae106932f
liuxinyanglxy
previously approved these changes
Apr 7, 2026
…ment The Input (file/stdin) feature is not yet ready for these flags: - im send/reply: --content, --text, --markdown - drive add-comment: --content Retained only in doc create/update where markdown from file is essential. Change-Id: I582b6349528fccb639ad9edc84650cca3b68535c Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tuxedomm
approved these changes
Apr 7, 2026
This was referenced Apr 7, 2026
This was referenced Apr 15, 2026
Merged
6 tasks
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
4 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
profilesubcommands (add/list/remove/rename/use) for managing multiple Lark app profilesTest plan
cmd/config/strict_mode_test.go,internal/core/strict_mode_test.go)extension/credential/*_test.go,internal/credential/*_test.go)cmd/bootstrap_test.go,cmd/prune_test.go)cmd/root_integration_test.go)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests