feat: add strict mode identity filter, profile management and credential extension#252
Open
liangshuo-1 wants to merge 7 commits intomainfrom
Open
feat: add strict mode identity filter, profile management and credential extension#252liangshuo-1 wants to merge 7 commits intomainfrom
liangshuo-1 wants to merge 7 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
📝 WalkthroughWalkthroughAdds a credential-provider system with an env provider, multi-profile config and strict-mode enforcement, a virtual filesystem abstraction (vfs), global/profile bootstrap flags, profile management commands, credential-driven token resolution for API clients/shortcuts, streaming I/O, and widespread migrations from direct os.* usage to vfs.*. 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 reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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@0b26e33327d0d83db47f4707ecdaf3e5afa566d2🧩 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
Comment on lines
+317
to
329
| if multi != nil { | ||
| app := multi.FindApp(config.ProfileName) | ||
| if app != nil { | ||
| for _, oldUser := range app.Users { | ||
| if oldUser.UserOpenId != openId { | ||
| larkauth.RemoveStoredToken(config.AppID, oldUser.UserOpenId) | ||
| } | ||
| } | ||
| app.Users = []core.AppUser{{UserOpenId: openId, UserName: userName}} | ||
| if err := core.SaveMultiAppConfig(multi); err != nil { | ||
| return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) | ||
| } | ||
| } | ||
| app.Users = []core.AppUser{{UserOpenId: openId, UserName: userName}} | ||
| if err := core.SaveMultiAppConfig(multi); err != nil { | ||
| return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) | ||
| } |
There was a problem hiding this comment.
Don't silently skip the profile update when the selected profile can't be found.
The token is already persisted before this block runs. If multi.FindApp(config.ProfileName) returns nil, both paths still return success, but Users is never updated and stale user tokens are never pruned, leaving login only partially applied.
Also applies to: 399-412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 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.
Comment on lines
+48
to
+51
| app := config.CurrentAppConfig(f.Invocation.Profile) | ||
| if app == nil { | ||
| fmt.Fprintln(f.IOStreams.ErrOut, "No active profile found.") | ||
| return nil |
There was a problem hiding this comment.
Return a non-zero error when profile resolution fails.
Lines 49-51 currently print an error and still return nil. If --profile points to a missing profile, the command exits 0 and automation can't distinguish that from a successful config show. Please return an error here and include the requested profile name when one was supplied.
Suggested fix
app := config.CurrentAppConfig(f.Invocation.Profile)
if app == nil {
- fmt.Fprintln(f.IOStreams.ErrOut, "No active profile found.")
- return nil
+ if f.Invocation.Profile != "" {
+ return fmt.Errorf("profile %q not found", f.Invocation.Profile)
+ }
+ return fmt.Errorf("no active profile found")
}📝 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
| app := config.CurrentAppConfig(f.Invocation.Profile) | |
| if app == nil { | |
| fmt.Fprintln(f.IOStreams.ErrOut, "No active profile found.") | |
| return nil | |
| app := config.CurrentAppConfig(f.Invocation.Profile) | |
| if app == nil { | |
| if f.Invocation.Profile != "" { | |
| return fmt.Errorf("profile %q not found", f.Invocation.Profile) | |
| } | |
| return fmt.Errorf("no active profile found") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/show.go` around lines 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.
Comment on lines
+44
to
+47
| app := multi.CurrentAppConfig(f.Invocation.Profile) | ||
| if app == nil { | ||
| return output.ErrWithHint(output.ExitValidation, "config", "no active profile", "run: lark-cli config init") | ||
| } |
There was a problem hiding this comment.
Global strict-mode operations are incorrectly blocked by active-profile requirement.
The early app == nil validation prevents --global set/show flows from running, even though they don’t need a profile app.
✅ Suggested fix
app := multi.CurrentAppConfig(f.Invocation.Profile)
- if app == nil {
+ if !global && app == nil {
return output.ErrWithHint(output.ExitValidation, "config", "no active profile", "run: lark-cli config init")
}📝 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
| app := multi.CurrentAppConfig(f.Invocation.Profile) | |
| if app == nil { | |
| return output.ErrWithHint(output.ExitValidation, "config", "no active profile", "run: lark-cli config init") | |
| } | |
| app := multi.CurrentAppConfig(f.Invocation.Profile) | |
| if !global && app == nil { | |
| return output.ErrWithHint(output.ExitValidation, "config", "no active profile", "run: lark-cli config init") | |
| } |
🤖 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 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.
Comment on lines
+72
to
+75
| multi, err := core.LoadMultiAppConfig() | ||
| if err != nil { | ||
| multi = &core.MultiAppConfig{} | ||
| } |
There was a problem hiding this comment.
Don't collapse every config-load failure into "start a new config".
This treats parse/permission errors the same as "config not found". If the existing multi-profile file is corrupt or unreadable, profile add will build a fresh MultiAppConfig here and can overwrite the user's current profiles on save.
🛡️ Suggested fix
+import (
+ "errors"
+ "os"
+)
+
// Load or create config
multi, err := core.LoadMultiAppConfig()
if err != nil {
- multi = &core.MultiAppConfig{}
+ if !errors.Is(err, os.ErrNotExist) {
+ return output.Errorf(output.ExitInternal, "internal", "failed to load config: %v", err)
+ }
+ multi = &core.MultiAppConfig{}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/profile/add.go` around lines 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.
Comment on lines
+52
to
+71
| // Cleanup keychain: app secret + user tokens | ||
| core.RemoveSecretStore(app.AppSecret, f.Keychain) | ||
| for _, user := range app.Users { | ||
| larkauth.RemoveStoredToken(app.AppId, user.UserOpenId) | ||
| } | ||
|
|
||
| // Remove from slice | ||
| multi.Apps = append(multi.Apps[:idx], multi.Apps[idx+1:]...) | ||
|
|
||
| // Fix currentApp / previousApp references | ||
| if multi.CurrentApp == removedName { | ||
| multi.CurrentApp = multi.Apps[0].ProfileName() | ||
| } | ||
| if multi.PreviousApp == removedName { | ||
| multi.PreviousApp = "" | ||
| } | ||
|
|
||
| if err := core.SaveMultiAppConfig(multi); err != nil { | ||
| return fmt.Errorf("failed to save config: %w", err) | ||
| } |
There was a problem hiding this comment.
Order of operations can cause partial destructive failure.
Credentials are deleted before config persistence. If Line 69 save fails, profile config remains but its secrets/tokens are already gone.
✅ Suggested fix (save first, then cleanup)
- // Cleanup keychain: app secret + user tokens
- core.RemoveSecretStore(app.AppSecret, f.Keychain)
- for _, user := range app.Users {
- larkauth.RemoveStoredToken(app.AppId, user.UserOpenId)
- }
-
// Remove from slice
multi.Apps = append(multi.Apps[:idx], multi.Apps[idx+1:]...)
@@
if err := core.SaveMultiAppConfig(multi); err != nil {
return fmt.Errorf("failed to save config: %w", err)
}
+
+ // Best-effort credential cleanup after config commit
+ core.RemoveSecretStore(app.AppSecret, f.Keychain)
+ for _, user := range app.Users {
+ larkauth.RemoveStoredToken(app.AppId, user.UserOpenId)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/profile/remove.go` around lines 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.
Comment on lines
+16
to
+26
| data, _ := json.Marshal(m) | ||
| if string(data) != `{"apps":[{"appId":"a","appSecret":"s","brand":"feishu","users":[]}]}` { | ||
| t.Errorf("StrictMode empty should be omitted, got: %s", data) | ||
| } | ||
|
|
||
| // StrictMode="bot" should be present | ||
| m.StrictMode = StrictModeBot | ||
| data, _ = json.Marshal(m) | ||
| var parsed map[string]interface{} | ||
| json.Unmarshal(data, &parsed) | ||
| if parsed["strictMode"] != "bot" { |
There was a problem hiding this comment.
Handle JSON (un)marshal errors explicitly in tests.
Right now these calls ignore error, which can mask serialization failures and produce noisy assertions.
✅ Suggested fix
- data, _ := json.Marshal(m)
+ data, err := json.Marshal(m)
+ if err != nil {
+ t.Fatalf("marshal failed: %v", err)
+ }
@@
- json.Unmarshal(data, &parsed)
+ if err := json.Unmarshal(data, &parsed); err != nil {
+ t.Fatalf("unmarshal failed: %v", err)
+ }
@@
- data, _ := json.Marshal(app)
+ data, err := json.Marshal(app)
+ if err != nil {
+ t.Fatalf("marshal failed: %v", err)
+ }
@@
- json.Unmarshal(data, &parsed)
+ if err := json.Unmarshal(data, &parsed); err != nil {
+ t.Fatalf("unmarshal failed: %v", err)
+ }Also applies to: 34-36, 44-45, 53-54
🤖 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 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.
Comment on lines
+95
to
+99
| a1, _ := cp.ResolveAccount(context.Background()) | ||
| a2, _ := cp.ResolveAccount(context.Background()) | ||
| if a1 != a2 { | ||
| t.Error("expected same pointer (cached)") | ||
| } |
There was a problem hiding this comment.
AccountCached can pass incorrectly when resolution fails.
Lines 95-96 ignore errors, so a1 == a2 can be true for two nil values.
✅ Suggested fix
- a1, _ := cp.ResolveAccount(context.Background())
- a2, _ := cp.ResolveAccount(context.Background())
+ a1, err := cp.ResolveAccount(context.Background())
+ if err != nil {
+ t.Fatal(err)
+ }
+ a2, err := cp.ResolveAccount(context.Background())
+ if err != nil {
+ t.Fatal(err)
+ }
+ if a1 == nil || a2 == nil {
+ t.Fatal("expected non-nil accounts")
+ }
if a1 != a2 {
t.Error("expected same pointer (cached)")
}📝 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
| a1, _ := cp.ResolveAccount(context.Background()) | |
| a2, _ := cp.ResolveAccount(context.Background()) | |
| if a1 != a2 { | |
| t.Error("expected same pointer (cached)") | |
| } | |
| a1, err := cp.ResolveAccount(context.Background()) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| a2, err := cp.ResolveAccount(context.Background()) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if a1 == nil || a2 == nil { | |
| t.Fatal("expected non-nil accounts") | |
| } | |
| if a1 != a2 { | |
| t.Error("expected same pointer (cached)") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/credential/credential_provider_test.go` around lines 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.
Comment on lines
+49
to
+67
| 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, 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.
Integration tests should fully isolate env state for deterministic provider-chain behavior.
These tests depend on env provider fallthrough/config path but do not clear all related env inputs (e.g., token and strict-mode vars), so external env can change outcomes.
Also applies to: 78-112
🤖 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 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.
Comment on lines
+327
to
+339
| func resolveLocalMedia(ctx context.Context, runtime *common.RuntimeContext, s mediaSpec) (string, error) { | ||
| fmt.Fprintf(runtime.IO().ErrOut, "uploading %s: %s\n", s.mediaType, filepath.Base(s.value)) | ||
|
|
||
| if s.kind == mediaKindImage { | ||
| return uploadImageToIM(ctx, runtime, s.value, "message") | ||
| } | ||
|
|
||
| ft := detectIMFileType(s.value) | ||
| dur := "" | ||
| if s.withDuration { | ||
| dur = parseMediaDuration(s.value, ft) | ||
| } | ||
| return uploadFileToIM(ctx, runtime, s.value, ft, dur) |
There was a problem hiding this comment.
Validate the local path before parsing duration.
For --audio and --video, parseMediaDuration opens s.value before uploadFileToIM gets a chance to run validate.SafeInputPath. That reintroduces a raw filesystem read on untrusted input for every local path that maps to mp4/opus.
🔒 Suggested fix
func resolveLocalMedia(ctx context.Context, runtime *common.RuntimeContext, s mediaSpec) (string, error) {
- fmt.Fprintf(runtime.IO().ErrOut, "uploading %s: %s\n", s.mediaType, filepath.Base(s.value))
+ safePath, err := validate.SafeInputPath(s.value)
+ if err != nil {
+ return "", err
+ }
+ fmt.Fprintf(runtime.IO().ErrOut, "uploading %s: %s\n", s.mediaType, filepath.Base(safePath))
if s.kind == mediaKindImage {
- return uploadImageToIM(ctx, runtime, s.value, "message")
+ return uploadImageToIM(ctx, runtime, safePath, "message")
}
- ft := detectIMFileType(s.value)
+ ft := detectIMFileType(safePath)
dur := ""
if s.withDuration {
- dur = parseMediaDuration(s.value, ft)
+ dur = parseMediaDuration(safePath, ft)
}
- return uploadFileToIM(ctx, runtime, s.value, ft, dur)
+ return uploadFileToIM(ctx, runtime, safePath, ft, dur)
}📝 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 resolveLocalMedia(ctx context.Context, runtime *common.RuntimeContext, s mediaSpec) (string, error) { | |
| fmt.Fprintf(runtime.IO().ErrOut, "uploading %s: %s\n", s.mediaType, filepath.Base(s.value)) | |
| if s.kind == mediaKindImage { | |
| return uploadImageToIM(ctx, runtime, s.value, "message") | |
| } | |
| ft := detectIMFileType(s.value) | |
| dur := "" | |
| if s.withDuration { | |
| dur = parseMediaDuration(s.value, ft) | |
| } | |
| return uploadFileToIM(ctx, runtime, s.value, ft, dur) | |
| func resolveLocalMedia(ctx context.Context, runtime *common.RuntimeContext, s mediaSpec) (string, error) { | |
| safePath, err := validate.SafeInputPath(s.value) | |
| if err != nil { | |
| return "", err | |
| } | |
| fmt.Fprintf(runtime.IO().ErrOut, "uploading %s: %s\n", s.mediaType, filepath.Base(safePath)) | |
| if s.kind == mediaKindImage { | |
| return uploadImageToIM(ctx, runtime, safePath, "message") | |
| } | |
| ft := detectIMFileType(safePath) | |
| dur := "" | |
| if s.withDuration { | |
| dur = parseMediaDuration(safePath, ft) | |
| } | |
| return uploadFileToIM(ctx, runtime, safePath, ft, dur) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/im/helpers.go` around lines 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).
Change-Id: If61578631f5698f7ca2d9a946ca59753651463fb
Greptile SummaryThis large PR introduces profile management (add/list/use/remove/rename), strict-mode identity filtering, a credential extension framework with an env provider, and a VFS abstraction for testability. The architecture is well-designed: credential resolution is a clean chain (extension providers → default), strict mode pruning is composable via Cobra command replacement, and the sync.Once caching pattern is appropriate for a CLI's single-invocation lifecycle.
Confidence Score: 4/5Safe to merge with the rename bug fixed; all other open issues are P2 style or carry-over from prior review threads. One P1 logic bug in cmd/profile/rename.go (false conflict blocks valid rename). Three previously-flagged P1 issues (env brand default, proxy bypass, AppID in TokenSpec) remain open. If those are intentional or deferred, score would rise; the rename bug is the only new blocker. cmd/profile/rename.go (P1 rename conflict bug), internal/credential/credential_provider.go (P2 unnecessary UAT enrichment for bot-only accounts) Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as lark-cli Execute()
participant Boot as BootstrapInvocationContext
participant Cred as CredentialProvider
participant Ext as ExtensionProviders
participant Def as DefaultAccountProvider
participant Cfg as config.json
participant Prune as pruneForStrictMode
participant Cmd as Command.RunE
CLI->>Boot: parse --profile flag
Boot-->>CLI: InvocationContext{Profile}
CLI->>Cred: NewCredentialProvider(ext, default)
CLI->>Cred: ResolveStrictMode() → ResolveAccount()
Cred->>Ext: ResolveAccount() [env provider]
alt LARK_APP_ID set
Ext-->>Cred: &Account{SupportedIdentities}
else
Ext-->>Cred: nil, nil
Cred->>Def: ResolveAccount()
Def->>Cfg: LoadMultiAppConfig()
Cfg-->>Def: MultiAppConfig
Def-->>Cred: &Account{SupportedIdentities from StrictMode}
end
Cred-->>CLI: StrictMode (bot|user|off)
CLI->>Prune: pruneForStrictMode(rootCmd, mode)
Note over Prune: replaces incompatible commands with hidden stubs
CLI->>Cmd: Execute command
Cmd->>Cred: ResolveToken(TokenSpec)
Cred-->>Cmd: TokenResult{Token}
Cmd->>CLI: response
Greploops — Automatically fix all review issues by running Reviews (2): Last reviewed commit: "fix: fix pre-existing test failures in t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+33
to
+36
| } | ||
| acct := &credential.Account{AppID: appID, AppSecret: appSecret, Brand: brand} | ||
|
|
||
| // Explicit strict mode policy takes priority |
There was a problem hiding this comment.
The env provider silently defaults to "lark" when LARK_BRAND is unset, but cmd/profile/add.go defaults to "feishu" and core.ParseBrand treats unrecognized values as BrandFeishu. A user who relies on env-var credentials without setting LARK_BRAND will be routed to Lark endpoints while every other config path defaults to Feishu — a silent misconfiguration that can produce confusing 401/404 errors.
Suggested change
| } | |
| acct := &credential.Account{AppID: appID, AppSecret: appSecret, Brand: brand} | |
| // Explicit strict mode policy takes priority | |
| brand := os.Getenv("LARK_BRAND") | |
| if brand == "" { | |
| brand = "feishu" | |
| } |
Comment on lines
+115
to
116
| lark.WithEnableTokenCache(false), | ||
| lark.WithLogLevel(larkcore.LogLevelError), |
There was a problem hiding this comment.
LARK_CLI_NO_PROXY no longer respected for Lark SDK transport
Replacing util.NewBaseTransport() with http.DefaultTransport drops the LARK_CLI_NO_PROXY proxy-bypass behaviour. The warning via WarnIfProxied still fires, but requests will continue to transit the proxy even when the user has set LARK_CLI_NO_PROXY=1. util.NewBaseTransport() clones DefaultTransport and conditionally zeroes out the Proxy field — that intent is now lost.
Suggested change
| lark.WithEnableTokenCache(false), | |
| lark.WithLogLevel(larkcore.LogLevelError), | |
| var sdkTransport http.RoundTripper = util.NewBaseTransport() |
Comment on lines
+103
to
+110
| // Have UAT — must verify and resolve identity | ||
| hc, err := p.httpClient() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get HTTP client for user_info: %w", err) | ||
| } | ||
| info, err := fetchUserInfo(ctx, hc, acct.Brand, tok.Value) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to verify user identity: %w", err) |
There was a problem hiding this comment.
AppID missing from TokenSpec in enrichUserInfo
ResolveToken is called with extcred.TokenSpec{Type: extcred.TokenTypeUAT} — AppID is not populated. Any extension provider that uses AppID to discriminate (e.g. a multi-tenant vault backend keyed by app ID) will receive an empty string and may skip resolution or return a token for the wrong app. The resolved acct.AppID is available here and should be passed.
Suggested change
| // Have UAT — must verify and resolve identity | |
| hc, err := p.httpClient() | |
| if err != nil { | |
| return fmt.Errorf("failed to get HTTP client for user_info: %w", err) | |
| } | |
| info, err := fetchUserInfo(ctx, hc, acct.Brand, tok.Value) | |
| if err != nil { | |
| return fmt.Errorf("failed to verify user identity: %w", err) | |
| tok, err := prov.ResolveToken(ctx, extcred.TokenSpec{Type: extcred.TokenTypeUAT, AppID: acct.AppID}) |
Comment on lines
+107
to
+116
| // Resolve auth | ||
| result, err := c.Credential.ResolveToken(ctx, credential.NewTokenSpec(as, c.Config.AppID)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Build URL | ||
| requestURL, err := buildStreamURL(c.Config.Brand, req) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
DoStream when context has no deadline
When cfg.timeout > 0 and the incoming context has no deadline, both httpClient.Timeout and context.WithTimeout are set to the same cfg.timeout duration. The HTTP client timeout fires on the full round-trip (connect + read), while context.WithTimeout fires from request start; both use the same value so one is redundant. Consider setting only the context timeout and leaving httpClient.Timeout at zero, or setting only httpClient.Timeout and skipping the context derivative:
if cfg.timeout > 0 {
if _, hasDeadline := ctx.Deadline(); !hasDeadline {
requestCtx, cancel = context.WithTimeout(ctx, cfg.timeout)
}
}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
Comment on lines
+534
to
+549
| // stdin: - | ||
| if raw == "-" { | ||
| if !slices.Contains(fl.Input, Stdin) { | ||
| return FlagErrorf("--%s does not support stdin (-)", fl.Name) | ||
| } | ||
| if stdinUsed { | ||
| return FlagErrorf("--%s: stdin (-) can only be used by one flag", fl.Name) | ||
| } | ||
| stdinUsed = true | ||
| data, err := io.ReadAll(rctx.IO().In) | ||
| if err != nil { | ||
| return FlagErrorf("--%s: failed to read from stdin: %v", fl.Name, err) | ||
| } | ||
| rctx.Cmd.Flags().Set(fl.Name, string(data)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Keep a way to pass a literal "-" value.
For any flag that enables Stdin, this branch makes "-" impossible to send as ordinary content because the resolver always consumes stdin instead. That is a real regression for the newly enabled plain-text flags in this PR, like --text and --markdown. Please add a documented escape or bypass for literal dash values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/common/runner.go` around lines 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.
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
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