refactor: introduce FileIO extension to abstract file transfer operations#297
Closed
refactor: introduce FileIO extension to abstract file transfer operations#297
Conversation
📝 WalkthroughWalkthroughIntroduces a file I/O abstraction layer ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces a substantial abstraction refactor across 50+ files, with new interface definitions, a concrete localfileio implementation, and systematic replacement of vfs/validate calls with FileIO abstractions throughout shortcuts and commands. Although most changes follow a repetitive injection pattern (reducing relative complexity), the breadth, logic density in atomic write/path validation, and number of interdependent touchpoints warrant careful review to ensure correct FileIO threading and no regressions in path safety or error handling. Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
domain/base
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@7625b8004a100a33cc38f5f099bd8a84050b74bf🧩 Skill updatenpx skills add larksuite/cli#feat/fileio-extension-refactor-plugin -y -g |
Greptile SummaryThis PR introduces a
Confidence Score: 4/5Safe to merge after addressing the nil-FileIO panic in HandleResponse/SaveResponse; all other findings are P2 or pre-existing. One new P1: ResponseOptions.FileIO is documented as nil-safe but SaveResponse calls fio.Save() without any nil guard, causing a panic on any binary API response when the provider is absent. All other findings are P2 style issues or were already flagged in prior review rounds. internal/client/response.go — contradictory nil-safety guarantee and missing nil guard in SaveResponse/HandleResponse
|
| Filename | Overview |
|---|---|
| extension/fileio/types.go | New interfaces (Provider, FileIO, File, SaveResult, SaveOptions) — well-scoped, no issues. |
| extension/fileio/registry.go | Thread-safe global registry using sync.Mutex; later registrations override earlier ones as documented. |
| internal/vfs/localfileio/localfileio.go | LocalFileIO implements FileIO correctly; Save uses atomic write and MkdirAll with secure permissions (0700/0600). |
| internal/vfs/localfileio/path.go | Path validation logic (TOCTOU-safe symlink resolution, CWD containment) migrated correctly from validate/path.go. |
| internal/client/response.go | ResponseOptions.FileIO comment says "nil falls back to direct os calls" but SaveResponse panics on nil fio — contradictory contracts and a latent panic. |
| shortcuts/common/runner.go | FileIO(), ValidatePath(), ResolveSavePath() added; ValidatePath doc-comment bleeds into ResolveSavePath; otherwise correct. |
| shortcuts/drive/drive_download.go | Migrated to FileIO.Save; overwrite check now uses FileIO.Stat (safe); saved_path returns raw user input instead of resolved absolute path (pre-flagged). |
| shortcuts/drive/drive_upload.go | Multipart upload migrated to FileIO.Open/SectionReader; per-block file handle opened in loop but never closed (pre-existing leak). |
| internal/cmdutil/factory_default.go | Blank-imports localfileio to trigger init() registration; wires FileIOProvider from global registry — correct bootstrap. |
| shortcuts/base/helpers.go | parseStringList refactored to avoid nil-fio panic by using a simple comma splitter; loadJSONInput now takes explicit fio parameter. |
| .golangci.yml | New depguard rule denies internal/vfs imports in shortcuts/**; two existing production files (shortcuts/event/pipeline.go, shortcuts/mail/mail_watch.go) use vfs functions not yet available via FileIO, causing new lint failures. |
| shortcuts/drive/drive_export_common.go | saveContentToOutputDir migrated to FileIO.Save; overwrite check uses Stat; returned resolvedPath falls back to raw target if ResolvePath fails. |
| shortcuts/vc/vc_notes.go | Fixes pre-existing transcriptPath variable shadowing; migrates overwrite check and write to FileIO correctly. |
| internal/validate/path.go | Thinned to a one-line delegate to localfileio.SafeOutputPath; backward-compatible shim for existing callers. |
Sequence Diagram
sequenceDiagram
participant CLI as CLI Command
participant Factory as Factory
participant Registry as fileio.Registry
participant RCtx as RuntimeContext
participant FIO as FileIO (LocalFileIO)
participant FS as Local Filesystem
Note over Factory,Registry: Startup (NewDefault)
Factory->>Registry: GetProvider() [via blank import init()]
Registry-->>Factory: localfileio.Provider
Factory->>Factory: FileIOProvider = provider
Note over CLI,FS: Download / Save flow
CLI->>RCtx: runtime.FileIO()
RCtx->>Factory: ResolveFileIO(ctx)
Factory-->>RCtx: LocalFileIO{}
CLI->>RCtx: FileIO().Stat(path)
RCtx->>FIO: Stat(path)
FIO->>FIO: safeInputPath(path)
FIO->>FS: os.Stat(safePath)
FS-->>FIO: FileInfo / ErrNotExist
CLI->>RCtx: FileIO().Save(path, opts, body)
RCtx->>FIO: Save(path, opts, body)
FIO->>FIO: SafeOutputPath(path)
FIO->>FS: MkdirAll + atomicWrite
FS-->>FIO: n bytes
FIO-->>CLI: SaveResult{size}
CLI->>RCtx: ResolveSavePath(path)
RCtx->>FIO: ResolvePath(path)
FIO-->>RCtx: absolute path
RCtx-->>CLI: absolute path (or raw fallback)
Reviews (4): Last reviewed commit: "fix: gofmt runner_jq_test.go" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
tuxedomm
force-pushed
the
feat/fileio-extension-refactor-plugin
branch
from
ee0bc07 to
a4e0fcf
Compare
tuxedomm
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/sheets/sheet_export.go (1)
114-141:⚠️ Potential issue | 🟠 MajorReturn after the no-download branch.
--output-pathis optional, but this still falls through intoSave(outputPath, ...)after emitting{file_token, ticket}. With an empty path that turns into a bogus save attempt, and the command can write a success JSON to stdout before returning an error.🐛 Proposed fix
if outputPath == "" { runtime.Out(map[string]interface{}{ "file_token": fileToken, "ticket": ticket, }, nil) + return nil } // Download resp, err := runtime.DoAPIStream(ctx, &larkcore.ApiReq{🤖 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 114 - 141, When outputPath is empty the code writes the file_token/ticket via runtime.Out but then continues and attempts to call runtime.FileIO().Save with an empty path; change the branch handling outputPath == "" (the block that calls runtime.Out with file_token and ticket) to return immediately after emitting the JSON so execution does not fall through to the download and Save logic—ensure you return the appropriate nil/error value from the surrounding function so no further download or success JSON (saved_path/size_bytes) is produced.internal/vfs/localfileio/path_test.go (1)
223-235:⚠️ Potential issue | 🟡 MinorDon’t codify
--fileas the shared input-path label.
safeInputPathnow sits underneath genericFileIOreads, and this expectation hard-codes--fileinto any caller that forwards the raw error. The newly migrated--patch-fileand draft patchpathflows do exactly that, so users will get guidance for the wrong flag/op. Make the validator return a neutral path error or accept a caller-supplied label instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/path_test.go` around lines 223 - 235, The test fails because safeInputPath currently returns an error that hard-codes the flag name "--file", which is incorrect for callers like --patch-file or other flows; change safeInputPath (and any helper it uses) to return a neutral path validation error message (no specific flag name) or refactor safeInputPath to accept a caller-provided label argument (e.g., safeInputPath(path string, label string)) and use that label in the error; update Test_safeInputPath_ErrorMessageContainsCorrectFlagName to assert for the neutral/error type or the provided label behavior instead of checking for "--file".
🧹 Nitpick comments (9)
.golangci.yml (1)
92-116: Duplicateforbidigopatterns foros.Stdin/Stdout/Stderrandos.Exit.The patterns at lines 92-98 (
os\.Std(in|out|err)andos\.Exit) duplicate the individual patterns at lines 106-116. This redundancy may cause duplicate lint warnings or confusion.Consider removing the duplicate individual patterns (lines 106-116) since the grouped regex patterns already cover them.
♻️ Remove duplicate patterns
- pattern: os\.Exit\b msg: >- Do not use os.Exit in shortcuts/. Return an error instead and let the caller (cmd layer) decide how to terminate. # ── filepath: functions that access the filesystem ── - pattern: filepath\.(EvalSymlinks|Walk|WalkDir|Glob|Abs)\b msg: >- These filepath functions access the filesystem directly. internal/: use vfs helpers or localfileio path validation. shortcuts/: use runtime.ValidatePath() or runtime.FileIO(). - # ── IO streams: use IOStreams from cmdutil instead ── - - pattern: os\.Stdin\b - msg: "use IOStreams.In instead of os.Stdin" - - pattern: os\.Stdout\b - msg: "use IOStreams.Out instead of os.Stdout" - - pattern: os\.Stderr\b - msg: "use IOStreams.ErrOut instead of os.Stderr" - # ── Process-level rules ── - - pattern: os\.Exit\b - msg: >- - Do not use os.Exit in shortcuts/. Return an error instead and let - the caller (cmd layer) decide how to terminate. analyze-types: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 92 - 116, Remove the duplicated forbidigo rules: keep the grouped regex entries (pattern: os\.Std(in|out|err)\b and pattern: os\.Exit\b) and delete the redundant individual rules (pattern: os\.Stdin\b, pattern: os\.Stdout\b, pattern: os\.Stderr\b and the second pattern: os\.Exit\b) so each prohibition appears only once while preserving the original msg text for the retained patterns.shortcuts/drive/drive_upload.go (1)
177-193: Resource leak: file handle not closed on upload error beforepartFile.Close().If
runtime.DoAPIat line 188 returns an error,partFile.Close()at line 193 is still called. However, iflarkcore.NewFormdata()orfd.AddFilefails internally before DoAPI, or if there's a panic, the file might not be closed properly.Consider using
defer partFile.Close()immediately after opening, or restructuring to ensure cleanup.♻️ Suggested restructure using defer
partFile, err := runtime.FileIO().Open(filePath) if err != nil { return "", output.ErrValidation("cannot open file: %v", err) } + defer partFile.Close() fd := larkcore.NewFormdata() fd.AddField("upload_id", uploadID) fd.AddField("seq", fmt.Sprintf("%d", seq)) fd.AddField("size", fmt.Sprintf("%d", partSize)) fd.AddFile("file", io.NewSectionReader(partFile, offset, partSize)) apiResp, err := runtime.DoAPI(&larkcore.ApiReq{ HttpMethod: http.MethodPost, ApiPath: "/open-apis/drive/v1/files/upload_part", Body: fd, }, larkcore.WithFileUpload()) - partFile.Close() if err != nil {Note: Using
deferinside a loop creates a closure per iteration. If the loop has many iterations and files are large, this could delay cleanup until function return. The current explicitClose()is acceptable if you prefer immediate cleanup, but ensure all error paths are covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_upload.go` around lines 177 - 193, The opened file handle partFile (from runtime.FileIO().Open) can leak if NewFormdata, fd.AddFile, runtime.DoAPI or other intermediate operations return an error or panic before the explicit partFile.Close() call; ensure partFile is always closed by calling defer partFile.Close() immediately after a successful Open (or restructure to close on every error path) so that fd.AddFile, larkcore.NewFormdata and runtime.DoAPI error paths cannot leak the handle.shortcuts/drive/drive_import_test.go (1)
15-16: Prefer the standard test bootstrap over a blank import here.This package-level side effect wires
localfileiodifferently from the real CLI bootstrap, so these tests can still pass even if provider registration drifts elsewhere. Using the normal test factory plus an isolated config dir would keep the FileIO path closer to production.As per coding guidelines, "Use
cmdutil.TestFactory(t, config)for creating test factories in Go tests" and "Isolate config state in Go tests by usingt.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_import_test.go` around lines 15 - 16, Remove the package-level blank import of "github.com/larksuite/cli/internal/vfs/localfileio" and replace the implicit provider wiring by creating an explicit test factory using cmdutil.TestFactory(t, config) in the test setup; also isolate config state by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before initializing the factory so FileIO registration follows the normal CLI bootstrap path instead of package side-effects (refer to symbols localfileio, cmdutil.TestFactory, and the env var LARKSUITE_CLI_CONFIG_DIR).shortcuts/common/runner_jq_test.go (1)
155-167: Prefercmdutil.TestFactory(t, config)for test factories.
newTestFactorystill buildscmdutil.Factorymanually; using the standard helper will keep test setup consistent with repo conventions.As per coding guidelines:
**/*_test.go: Usecmdutil.TestFactory(t, config)for creating test factories in Go tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/runner_jq_test.go` around lines 155 - 167, The test helper newTestFactory manually constructs a cmdutil.Factory; replace it with the repo-standard cmdutil.TestFactory(t, config) call: remove newTestFactory and in tests call cmdutil.TestFactory(t, func() *cmdutil.Factory { return &cmdutil.Factory{ Config: func() (*core.CliConfig, error){ return &core.CliConfig{AppID:"test",AppSecret:"test",Brand:core.BrandFeishu}, nil }, LarkClient: func() (*lark.Client, error){ return lark.NewClient("test","test"), nil }, IOStreams: &cmdutil.IOStreams{Out: &bytes.Buffer{}, ErrOut: &bytes.Buffer{}}, FileIOProvider: fileio.GetProvider(), } }) so the test uses TestFactory(t, ...) instead of the manual newTestFactory construction and follows repository test conventions.extension/fileio/registry.go (1)
22-27: Consider usingsync.RWMutexfor better read concurrency.
GetProvider()is likely called frequently at runtime, whileRegister()is typically called once during init. UsingRWMutexwithRLock()for reads would allow concurrentGetProvider()calls without blocking each other.♻️ Optional: Use RWMutex for better read performance
var ( - mu sync.Mutex + mu sync.RWMutex provider Provider ) func Register(p Provider) { mu.Lock() defer mu.Unlock() provider = p } func GetProvider() Provider { - mu.Lock() - defer mu.Unlock() + mu.RLock() + defer mu.RUnlock() return provider }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/fileio/registry.go` around lines 22 - 27, GetProvider currently uses mu.Lock() which serializes reads; change the mutex type from sync.Mutex to sync.RWMutex (symbol mu) and update GetProvider to use mu.RLock() / mu.RUnlock() so concurrent reads don't block each other; also update Register (the provider registration function) to use mu.Lock() / mu.Unlock() for writes and ensure the import of "sync" is preserved/updated accordingly while keeping the provider variable usage the same.shortcuts/base/base_shortcuts_test.go (2)
21-48: Use the standard test factory in this helper.This helper now wires
Factorymanually, which bypasses the repo’s test-factory defaults and still leaves no place to isolate config state. Please threadt *testing.TintonewBaseTestRuntimeand build the runtime withcmdutil.TestFactory(t, config)plus a tempLARKSUITE_CLI_CONFIG_DIR.As per coding guidelines,
**/*_test.go: Usecmdutil.TestFactory(t, config)for creating test factories in Go tests, and isolate config state viat.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/base_shortcuts_test.go` around lines 21 - 48, Update newBaseTestRuntime to accept t *testing.T, set isolated config dir with t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()), and build the runtime using cmdutil.TestFactory(t, config) instead of manually wiring Factory; ensure the returned RuntimeContext uses Factory from cmdutil.TestFactory and keep other flag setup the same so tests use the standard test factory and isolated config state.
77-96: Add a file-backed case for the newFileIObranch.These updates only adapt the signature by passing
nil, so this test still never exercises the new@pathloading path that motivated the extra parameter. A regression inloadJSONInput/parseObjectListwould slip through unnoticed.shortcuts/im/validate_media_test.go (1)
14-19: Usecmdutil.TestChdirfor consistency with other tests.Other tests in this PR (e.g.,
helpers_network_test.go) usecmdutil.TestChdir(t, t.TempDir())instead of manualos.Getwd/os.Chdir/deferbookkeeping. This helper handles cleanup more robustly.Proposed fix
func TestValidateMediaFlagPath(t *testing.T) { - dir := t.TempDir() - orig, _ := os.Getwd() - defer os.Chdir(orig) - os.Chdir(dir) - os.WriteFile(filepath.Join(dir, "photo.jpg"), []byte("img"), 0644) + cmdutil.TestChdir(t, t.TempDir()) + os.WriteFile("photo.jpg", []byte("img"), 0644)This requires adding the import:
import ( "os" "testing" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/vfs/localfileio" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/validate_media_test.go` around lines 14 - 19, Replace the manual working-directory bookkeeping in TestValidateMediaFlagPath with the test helper cmdutil.TestChdir: call cmdutil.TestChdir(t, t.TempDir()) at the start of TestValidateMediaFlagPath and remove the orig := os.Getwd(), defer os.Chdir(orig) and os.Chdir(dir) lines; also add the import for the cmdutil package (e.g., import "github.com/larksuite/cli/internal/cmdutil") so the test compiles.internal/cmdutil/factory_default_test.go (1)
211-230: Missing config directory isolation.Other tests in this file use
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())to isolate config state. This test should follow the same pattern to prevent interference with machine-local configuration.Proposed fix
func TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + prev := fileio.GetProvider() provider := &countingFileIOProvider{} fileio.Register(provider)As per coding guidelines: "Isolate config state in Go tests by using
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/factory_default_test.go` around lines 211 - 230, TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization fails to isolate config state; update the test to set the config dir to a temporary directory by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of the test (before calling NewDefault/InvocationContext) so machine-local config cannot affect NewDefault/ResolveFileIO behavior; ensure this change is applied inside the TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/client/response.go`:
- Around line 82-83: When handling errors returned from FileIO.Save (fio.Save)
where currently code does return output.Errorf(output.ExitInternal,
"file_error", "%s", err) and also uses fmt.Errorf("cannot write file: %s", err),
change the logic to detect typed validation errors from fio.Save (use errors.Is
/ errors.As against the validation error type returned by the FileIO layer) and,
when matched, return output.Errorf(output.ErrValidation, "file_error", "%v",
err) (preserving the original error rather than wrapping with fmt.Errorf); for
non-validation failures keep the ExitInternal mapping. Apply the same change to
the other similar return sites that currently remap all failures to ExitInternal
(the other occurrences in this file).
- Line 30: The code unconditionally calls fio.Save(...) which will panic when
ResponseOptions.FileIO is nil; update SaveResponse and the other save paths
(where fio.Save is used) to guard against nil by resolving a fallback OS-backed
FileIO (e.g., create or use a fileio.NewOSFileIO()/fileio.Default
implementation) before calling Save, or return a clear error if no FileIO is
available; locate the ResponseOptions struct and the SaveResponse (and related
save) functions and insert a small nil-check: if ro.FileIO == nil { ro.FileIO =
fileio.NewOSFileIO() } (or assign a local fio := ro.FileIO; if nil { fio =
fileio.NewOSFileIO() }) then call fio.Save(...) to avoid the panic.
In `@internal/cmdutil/testing.go`:
- Line 10: In TestChdir (and the related helper code around lines 96-104)
replace direct os package calls with the repository VFS wrappers: use the VFS
method to get the current working dir instead of os.Getwd and use the VFS
chdir-equivalent to change directories instead of os.Chdir; update any error
handling to reflect the VFS method signatures and ensure the function restores
the original VFS working directory on return (same semantics as before) so all
filesystem access goes through the vfs layer.
In `@internal/validate/path.go`:
- Around line 6-11: Update the stale docs/comments that still instruct callers
to use validate.SafeInputPath: change AGENTS.md (around the previous line 72)
and the comment in shortcuts/mail/draft/limits.go (previously referencing
"Callers must validate the file path via validate.SafeInputPath") to instead
state that path validation is performed by FileIO operations and callers should
use FileIO methods such as fio.Stat(), fio.Open(), fio.Read(), etc.; reference
the existing example in shortcuts/drive/drive_import.go (around the usage at
line 144) to show the new pattern and remove any mention of SafeInputPath or
validate.SafeInputPath.
In `@internal/vfs/localfileio/localfileio.go`:
- Around line 34-49: The Open and Stat methods in LocalFileIO (and the other
method around MkdirAll at lines ~59-67) call os.Open, os.Stat, and os.MkdirAll
directly which bypasses the repo VFS wrapper; change these to call the vfs
package equivalents (e.g., vfs.Open, vfs.Stat, vfs.MkdirAll or the project’s
vfs.FS interface methods used elsewhere) after validating the path with
safeInputPath so all filesystem access consistently goes through internal/vfs;
update imports to include the vfs package and ensure error propagation stays the
same.
In `@internal/vfs/localfileio/path.go`:
- Around line 21-23: The SafeOutputPath implementation only validates the path
string but does not prevent TOCTOU races; update the code so that subsequent I/O
operations (LocalFileIO.Open, LocalFileIO.Stat, LocalFileIO.Save and any code
paths referenced in the same file regions) are anchored to trusted directory
file descriptors using openat/mkdirat and O_NOFOLLOW semantics rather than
relying on a vetted path string. Concretely: change callers to walk and open
each path component relative to a secured dirfd (using openat with
O_DIRECTORY|O_NOFOLLOW or mkdirat when creating tails), avoid following symlinks
on the final component, and propagate the dirfd or opened file descriptor
instead of re-resolving the string path; ensure all usages referenced
(SafeOutputPath and the LocalFileIO Open/Stat/Save code paths) adopt this
dirfd-based approach so the TOCTOU window is closed.
In `@shortcuts/common/runner.go`:
- Around line 299-316: ValidatePath and resolveInputFlags currently call
ctx.FileIO() and dereference it without checking for nil; add defensive nil
checks where FileIO() is used (in the ValidatePath and resolveInputFlags
functions) and return a clear error (or fallback behavior) if ctx.FileIO() ==
nil. Specifically, in ValidatePath and in resolveInputFlags locate calls like
ctx.FileIO().Stat/Read/Open and guard them with fio := ctx.FileIO(); if fio ==
nil { return fmt.Errorf("file IO provider not available") } (or appropriate
error handling), ensuring RuntimeContext.FileIO() is safely used even if
Factory/FileIOProvider or global provider are absent; alternatively, if you
prefer contract documentation, add a comment on RuntimeContext.FileIO/Execute
guaranteeing non-nil in production and add a runtime check in Execute to panic
or return an error if Factory/FileIOProvider is unset.
In `@shortcuts/doc/doc_media_insert.go`:
- Around line 115-119: The current branch that handles errors from
runtime.FileIO().Stat(filePath) hides the original error by always returning
output.ErrValidation("file not found: %s", filePath); change it to preserve and
return the underlying error (or wrap it) so path/permission errors from
runtime.FileIO().Stat are conveyed. Locate the Stat call in doc_media_insert.go
(runtime.FileIO().Stat(filePath)), and replace the unconditional "file not
found" return with a return that includes err (e.g., wrap err into
output.ErrValidation or return err directly) while keeping the filePath context.
In `@shortcuts/drive/drive_download.go`:
- Line 10: The code currently treats any fileio.FileIO.Stat error as an
os.IsNotExist miss and labels it an "unsafe output path"; instead, validate the
path up-front and treat Stat errors generically: call validate.SafeInputPath (or
runtime.ValidatePath where used in the codebase) on the path before invoking
fileio.FileIO.Stat, remove any os.IsNotExist-based branching, and on Stat errors
return or propagate the original error (or a clear not-found error) rather than
assuming os.IsNotExist; update the logic around the fileio.FileIO.Stat call in
drive_download.go to use validate.SafeInputPath/runtime.ValidatePath and handle
Stat errors without os.IsNotExist checks.
In `@shortcuts/im/im_messages_send.go`:
- Around line 214-221: validateMediaFlagPath currently ignores missing local
files because it only returns an error when Stat fails with something other than
os.IsNotExist; update validateMediaFlagPath to reject non-empty, non-URL,
non-media-key paths that do not exist by checking fio.Stat(value) and returning
output.ErrValidation("%s: %v", flagName, err) whenever err != nil (including
os.IsNotExist), while preserving the early return for URLs/media keys/empty
values — change the conditional around fio.Stat in validateMediaFlagPath to
return a validation error on any Stat error instead of skipping when
os.IsNotExist(err).
In `@shortcuts/mail/emlbuilder/builder.go`:
- Around line 62-70: The readFile method can panic if Builder.fio is nil (when
WithFileIO was not called); update Builder.readFile to check if b.fio == nil and
return a clear error (e.g., "file IO not initialized: call WithFileIO") instead
of calling b.fio.Open, and reference the Builder.readFile and WithFileIO symbols
so the check is added near the start of that method; ensure the error message is
wrapped/returned consistently with existing fmt.Errorf usage.
---
Outside diff comments:
In `@internal/vfs/localfileio/path_test.go`:
- Around line 223-235: The test fails because safeInputPath currently returns an
error that hard-codes the flag name "--file", which is incorrect for callers
like --patch-file or other flows; change safeInputPath (and any helper it uses)
to return a neutral path validation error message (no specific flag name) or
refactor safeInputPath to accept a caller-provided label argument (e.g.,
safeInputPath(path string, label string)) and use that label in the error;
update Test_safeInputPath_ErrorMessageContainsCorrectFlagName to assert for the
neutral/error type or the provided label behavior instead of checking for
"--file".
In `@shortcuts/sheets/sheet_export.go`:
- Around line 114-141: When outputPath is empty the code writes the
file_token/ticket via runtime.Out but then continues and attempts to call
runtime.FileIO().Save with an empty path; change the branch handling outputPath
== "" (the block that calls runtime.Out with file_token and ticket) to return
immediately after emitting the JSON so execution does not fall through to the
download and Save logic—ensure you return the appropriate nil/error value from
the surrounding function so no further download or success JSON
(saved_path/size_bytes) is produced.
---
Nitpick comments:
In @.golangci.yml:
- Around line 92-116: Remove the duplicated forbidigo rules: keep the grouped
regex entries (pattern: os\.Std(in|out|err)\b and pattern: os\.Exit\b) and
delete the redundant individual rules (pattern: os\.Stdin\b, pattern:
os\.Stdout\b, pattern: os\.Stderr\b and the second pattern: os\.Exit\b) so each
prohibition appears only once while preserving the original msg text for the
retained patterns.
In `@extension/fileio/registry.go`:
- Around line 22-27: GetProvider currently uses mu.Lock() which serializes
reads; change the mutex type from sync.Mutex to sync.RWMutex (symbol mu) and
update GetProvider to use mu.RLock() / mu.RUnlock() so concurrent reads don't
block each other; also update Register (the provider registration function) to
use mu.Lock() / mu.Unlock() for writes and ensure the import of "sync" is
preserved/updated accordingly while keeping the provider variable usage the
same.
In `@internal/cmdutil/factory_default_test.go`:
- Around line 211-230:
TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization fails to isolate
config state; update the test to set the config dir to a temporary directory by
calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of the
test (before calling NewDefault/InvocationContext) so machine-local config
cannot affect NewDefault/ResolveFileIO behavior; ensure this change is applied
inside the TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization
function.
In `@shortcuts/base/base_shortcuts_test.go`:
- Around line 21-48: Update newBaseTestRuntime to accept t *testing.T, set
isolated config dir with t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()), and
build the runtime using cmdutil.TestFactory(t, config) instead of manually
wiring Factory; ensure the returned RuntimeContext uses Factory from
cmdutil.TestFactory and keep other flag setup the same so tests use the standard
test factory and isolated config state.
In `@shortcuts/common/runner_jq_test.go`:
- Around line 155-167: The test helper newTestFactory manually constructs a
cmdutil.Factory; replace it with the repo-standard cmdutil.TestFactory(t,
config) call: remove newTestFactory and in tests call cmdutil.TestFactory(t,
func() *cmdutil.Factory { return &cmdutil.Factory{ Config: func()
(*core.CliConfig, error){ return
&core.CliConfig{AppID:"test",AppSecret:"test",Brand:core.BrandFeishu}, nil },
LarkClient: func() (*lark.Client, error){ return lark.NewClient("test","test"),
nil }, IOStreams: &cmdutil.IOStreams{Out: &bytes.Buffer{}, ErrOut:
&bytes.Buffer{}}, FileIOProvider: fileio.GetProvider(), } }) so the test uses
TestFactory(t, ...) instead of the manual newTestFactory construction and
follows repository test conventions.
In `@shortcuts/drive/drive_import_test.go`:
- Around line 15-16: Remove the package-level blank import of
"github.com/larksuite/cli/internal/vfs/localfileio" and replace the implicit
provider wiring by creating an explicit test factory using
cmdutil.TestFactory(t, config) in the test setup; also isolate config state by
calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before initializing
the factory so FileIO registration follows the normal CLI bootstrap path instead
of package side-effects (refer to symbols localfileio, cmdutil.TestFactory, and
the env var LARKSUITE_CLI_CONFIG_DIR).
In `@shortcuts/drive/drive_upload.go`:
- Around line 177-193: The opened file handle partFile (from
runtime.FileIO().Open) can leak if NewFormdata, fd.AddFile, runtime.DoAPI or
other intermediate operations return an error or panic before the explicit
partFile.Close() call; ensure partFile is always closed by calling defer
partFile.Close() immediately after a successful Open (or restructure to close on
every error path) so that fd.AddFile, larkcore.NewFormdata and runtime.DoAPI
error paths cannot leak the handle.
In `@shortcuts/im/validate_media_test.go`:
- Around line 14-19: Replace the manual working-directory bookkeeping in
TestValidateMediaFlagPath with the test helper cmdutil.TestChdir: call
cmdutil.TestChdir(t, t.TempDir()) at the start of TestValidateMediaFlagPath and
remove the orig := os.Getwd(), defer os.Chdir(orig) and os.Chdir(dir) lines;
also add the import for the cmdutil package (e.g., import
"github.com/larksuite/cli/internal/cmdutil") so the test compiles.
🪄 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: a1810c48-aa8f-4f85-96be-94ceb14109c1
📒 Files selected for processing (81)
.golangci.ymlcmd/api/api.gocmd/service/service.goextension/fileio/registry.goextension/fileio/types.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/testing.gointernal/validate/atomicwrite.gointernal/validate/path.gointernal/vfs/localfileio/atomicwrite.gointernal/vfs/localfileio/atomicwrite_test.gointernal/vfs/localfileio/input.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/path.gointernal/vfs/localfileio/path_test.goshortcuts/base/base_shortcut_helpers.goshortcuts/base/base_shortcuts_test.goshortcuts/base/dashboard_block_create.goshortcuts/base/dashboard_block_update.goshortcuts/base/dashboard_ops.goshortcuts/base/field_ops.goshortcuts/base/helpers.goshortcuts/base/helpers_test.goshortcuts/base/record_ops.goshortcuts/base/record_upload_attachment.goshortcuts/base/table_ops.goshortcuts/base/view_ops.goshortcuts/base/workflow_create.goshortcuts/base/workflow_update.goshortcuts/common/common_test.goshortcuts/common/helpers.goshortcuts/common/runner.goshortcuts/common/runner_input_test.goshortcuts/common/runner_jq_test.goshortcuts/common/validate.goshortcuts/common/validate_test.goshortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_upload.goshortcuts/drive/drive_download.goshortcuts/drive/drive_export.goshortcuts/drive/drive_export_common.goshortcuts/drive/drive_export_test.goshortcuts/drive/drive_import.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_import_test.goshortcuts/drive/drive_upload.goshortcuts/im/coverage_additional_test.goshortcuts/im/helpers.goshortcuts/im/helpers_network_test.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_reply.goshortcuts/im/im_messages_resources_download.goshortcuts/im/im_messages_send.goshortcuts/im/validate_media_test.goshortcuts/mail/draft/acceptance_test.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_attachment_test.goshortcuts/mail/draft/patch_body_test.goshortcuts/mail/draft/patch_header_test.goshortcuts/mail/draft/patch_recipient_test.goshortcuts/mail/draft/patch_test.goshortcuts/mail/draft/serialize_golden_test.goshortcuts/mail/draft/serialize_test.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/minutes/minutes_download.goshortcuts/sheets/sheet_export.goshortcuts/vc/vc_notes.go
💤 Files with no reviewable changes (4)
- shortcuts/common/validate_test.go
- shortcuts/common/common_test.go
- shortcuts/common/helpers.go
- shortcuts/common/validate.go
| JqExpr string // if set, apply jq filter instead of Format | ||
| Out io.Writer // stdout | ||
| ErrOut io.Writer // stderr | ||
| FileIO fileio.FileIO // file transfer abstraction; nil falls back to direct os calls |
There was a problem hiding this comment.
Avoid a nil FileIO panic on save paths.
ResponseOptions still says nil falls back, but SaveResponse now unconditionally calls fio.Save(...). Any caller using the zero value will panic on --output or on the auto-save binary path instead of getting a normal error.
🩹 Minimal guard
func SaveResponse(fio fileio.FileIO, resp *larkcore.ApiResp, outputPath string) (map[string]interface{}, error) {
+ if fio == nil {
+ return nil, fmt.Errorf("file I/O not configured")
+ }
result, err := fio.Save(outputPath, fileio.SaveOptions{
ContentType: resp.Header.Get("Content-Type"),
ContentLength: int64(len(resp.RawBody)),
}, bytes.NewReader(resp.RawBody))Also applies to: 63-63, 77-81, 122-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/client/response.go` at line 30, The code unconditionally calls
fio.Save(...) which will panic when ResponseOptions.FileIO is nil; update
SaveResponse and the other save paths (where fio.Save is used) to guard against
nil by resolving a fallback OS-backed FileIO (e.g., create or use a
fileio.NewOSFileIO()/fileio.Default implementation) before calling Save, or
return a clear error if no FileIO is available; locate the ResponseOptions
struct and the SaveResponse (and related save) functions and insert a small
nil-check: if ro.FileIO == nil { ro.FileIO = fileio.NewOSFileIO() } (or assign a
local fio := ro.FileIO; if nil { fio = fileio.NewOSFileIO() }) then call
fio.Save(...) to avoid the panic.
Comment on lines
82
to
83
| if err != nil { | ||
| return output.Errorf(output.ExitInternal, "file_error", "%s", err) |
There was a problem hiding this comment.
Preserve validation failures from FileIO.Save.
fio.Save can now return user-input errors such as an invalid --output path, but both save paths remap every failure to file_error/ExitInternal. fmt.Errorf("cannot write file: %s", err) also strips the cause chain, so callers cannot recover the right exit code later. Please keep typed validation errors intact and map them to output.ErrValidation here.
Also applies to: 92-93, 123-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/client/response.go` around lines 82 - 83, When handling errors
returned from FileIO.Save (fio.Save) where currently code does return
output.Errorf(output.ExitInternal, "file_error", "%s", err) and also uses
fmt.Errorf("cannot write file: %s", err), change the logic to detect typed
validation errors from fio.Save (use errors.Is / errors.As against the
validation error type returned by the FileIO layer) and, when matched, return
output.Errorf(output.ErrValidation, "file_error", "%v", err) (preserving the
original error rather than wrapping with fmt.Errorf); for non-validation
failures keep the ExitInternal mapping. Apply the same change to the other
similar return sites that currently remap all failures to ExitInternal (the
other occurrences in this file).
| "bytes" | ||
| "context" | ||
| "net/http" | ||
| "os" |
There was a problem hiding this comment.
Use vfs.* wrappers instead of direct os.* in TestChdir.
TestChdir currently uses os.Getwd/os.Chdir, which breaks the repo filesystem abstraction rule. Please switch this helper to the repository VFS layer for consistency.
As per coding guidelines: Use vfs.* instead of os.* for all filesystem access.
Also applies to: 96-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmdutil/testing.go` at line 10, In TestChdir (and the related helper
code around lines 96-104) replace direct os package calls with the repository
VFS wrappers: use the VFS method to get the current working dir instead of
os.Getwd and use the VFS chdir-equivalent to change directories instead of
os.Chdir; update any error handling to reflect the VFS method signatures and
ensure the function restores the original VFS working directory on return (same
semantics as before) so all filesystem access goes through the vfs layer.
Comment on lines
115
to
119
| // Validate file | ||
| stat, err := vfs.Stat(filePath) | ||
| stat, err := runtime.FileIO().Stat(filePath) | ||
| if err != nil { | ||
| return output.ErrValidation("file not found: %s", filePath) | ||
| } |
There was a problem hiding this comment.
Preserve the original FileIO error instead of rewriting everything to "file not found".
Now that runtime.FileIO().Stat() owns path and permission checks, this branch turns unsafe-path and permission failures into the same missing-file message. That makes the error misleading and regresses the clearer validation feedback from the new FileIO layer.
Suggested fix
// Validate file
stat, err := runtime.FileIO().Stat(filePath)
if err != nil {
- return output.ErrValidation("file not found: %s", filePath)
+ var exitErr *output.ExitError
+ if errors.As(err, &exitErr) {
+ return err
+ }
+ return output.ErrValidation("cannot read file %q: %v", filePath, err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/doc/doc_media_insert.go` around lines 115 - 119, The current branch
that handles errors from runtime.FileIO().Stat(filePath) hides the original
error by always returning output.ErrValidation("file not found: %s", filePath);
change it to preserve and return the underlying error (or wrap it) so
path/permission errors from runtime.FileIO().Stat are conveyed. Locate the Stat
call in doc_media_insert.go (runtime.FileIO().Stat(filePath)), and replace the
unconditional "file not found" return with a return that includes err (e.g.,
wrap err into output.ErrValidation or return err directly) while keeping the
filePath context.
| "fmt" | ||
| "net/http" | ||
| "path/filepath" | ||
| "os" |
There was a problem hiding this comment.
Don’t derive FileIO semantics from os.IsNotExist.
This gate assumes every fileio.FileIO.Stat miss satisfies os.IsNotExist, which is not part of the FileIO contract. A wrapped or provider-specific miss will be reported here as unsafe output path, and this also bypasses the centralized runtime.ValidatePath(...) flow the rest of the refactor moved to.
Suggested adjustment
- "os"
@@
- // Early path validation + overwrite check via FileIO.Stat
- if _, statErr := runtime.FileIO().Stat(outputPath); statErr != nil && !os.IsNotExist(statErr) {
- return output.ErrValidation("unsafe output path: %s", statErr)
- } else if statErr == nil && !overwrite {
- return output.ErrValidation("output file already exists: %s (use --overwrite to replace)", outputPath)
- }
+ if err := runtime.ValidatePath(outputPath); err != nil {
+ return output.ErrValidation("unsafe output path: %s", err)
+ }
+ if !overwrite {
+ if _, statErr := runtime.FileIO().Stat(outputPath); statErr == nil {
+ return output.ErrValidation("output file already exists: %s (use --overwrite to replace)", outputPath)
+ }
+ }As per coding guidelines, "Validate paths using validate.SafeInputPath before any file I/O operations".
Also applies to: 55-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/drive/drive_download.go` at line 10, The code currently treats any
fileio.FileIO.Stat error as an os.IsNotExist miss and labels it an "unsafe
output path"; instead, validate the path up-front and treat Stat errors
generically: call validate.SafeInputPath (or runtime.ValidatePath where used in
the codebase) on the path before invoking fileio.FileIO.Stat, remove any
os.IsNotExist-based branching, and on Stat errors return or propagate the
original error (or a clear not-found error) rather than assuming os.IsNotExist;
update the logic around the fileio.FileIO.Stat call in drive_download.go to use
validate.SafeInputPath/runtime.ValidatePath and handle Stat errors without
os.IsNotExist checks.
Comment on lines
+214
to
+221
| func validateMediaFlagPath(fio fileio.FileIO, flagName, value string) error { | ||
| if value == "" || strings.HasPrefix(value, "http://") || strings.HasPrefix(value, "https://") || isMediaKey(value) { | ||
| return nil | ||
| } | ||
| if _, err := fio.Stat(value); err != nil && !os.IsNotExist(err) { | ||
| return output.ErrValidation("%s: %v", flagName, err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Reject missing local media files here.
fio.Stat failures are ignored when the file does not exist, so values like --image ./missing.png now pass both Validate and the early Execute preflight and only fail later in the upload path. That regresses the old “bad local path” feedback and makes user errors surface much later.
🐛 Proposed fix
-import (
- "context"
- "encoding/json"
- "net/http"
- "os"
- "strings"
+import (
+ "context"
+ "encoding/json"
+ "net/http"
+ "strings"
@@
func validateMediaFlagPath(fio fileio.FileIO, flagName, value string) error {
if value == "" || strings.HasPrefix(value, "http://") || strings.HasPrefix(value, "https://") || isMediaKey(value) {
return nil
}
- if _, err := fio.Stat(value); err != nil && !os.IsNotExist(err) {
+ if _, err := fio.Stat(value); err != nil {
return output.ErrValidation("%s: %v", flagName, err)
}
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/im/im_messages_send.go` around lines 214 - 221,
validateMediaFlagPath currently ignores missing local files because it only
returns an error when Stat fails with something other than os.IsNotExist; update
validateMediaFlagPath to reject non-empty, non-URL, non-media-key paths that do
not exist by checking fio.Stat(value) and returning output.ErrValidation("%s:
%v", flagName, err) whenever err != nil (including os.IsNotExist), while
preserving the early return for URLs/media keys/empty values — change the
conditional around fio.Stat in validateMediaFlagPath to return a validation
error on any Stat error instead of skipping when os.IsNotExist(err).
Comment on lines
62
to
70
| // readFile reads the named file and returns its contents via the Builder's FileIO. | ||
| func (b Builder) readFile(path string) ([]byte, error) { | ||
| f, err := b.fio.Open(path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("attachment %q: %w", path, err) | ||
| } | ||
| return vfs.ReadFile(safePath) | ||
| defer f.Close() | ||
| return io.ReadAll(f) | ||
| } |
There was a problem hiding this comment.
Potential nil pointer dereference if WithFileIO is not called.
The readFile method calls b.fio.Open(path) but fio could be nil if WithFileIO was never called on the builder. This would cause a panic.
Consider adding a nil check or documenting that WithFileIO must be called before using file-based methods.
🛡️ Proposed defensive check
func (b Builder) readFile(path string) ([]byte, error) {
+ if b.fio == nil {
+ return nil, fmt.Errorf("attachment %q: FileIO not configured (call WithFileIO first)", path)
+ }
f, err := b.fio.Open(path)
if err != nil {
return nil, fmt.Errorf("attachment %q: %w", path, err)
}
defer f.Close()
return io.ReadAll(f)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/mail/emlbuilder/builder.go` around lines 62 - 70, The readFile
method can panic if Builder.fio is nil (when WithFileIO was not called); update
Builder.readFile to check if b.fio == nil and return a clear error (e.g., "file
IO not initialized: call WithFileIO") instead of calling b.fio.Open, and
reference the Builder.readFile and WithFileIO symbols so the check is added near
the start of that method; ensure the error message is wrapped/returned
consistently with existing fmt.Errorf usage.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/common/runner_jq_test.go (1)
156-168:⚠️ Potential issue | 🟠 MajorUse the repo test factory helper and isolate config env in this test helper.
Line 156still buildscmdutil.Factorymanually, and this helper path does not isolateLARKSUITE_CLI_CONFIG_DIR. Please switch tocmdutil.TestFactory(t, config)and set per-test config dir viat.Setenv(...)to keep tests hermetic.As per coding guidelines,
**/*_test.go: Usecmdutil.TestFactory(t, config)for creating test factories in Go tests and isolate config state witht.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/runner_jq_test.go` around lines 156 - 168, The test helper newTestFactory() currently constructs a cmdutil.Factory manually and doesn't isolate config state; replace its usage with the repository test helper by switching to cmdutil.TestFactory(t, config) in tests and ensure you call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) so each test gets a hermetic per-test config dir; locate references to newTestFactory and update the test to create a cfg (core.CliConfig with AppID/AppSecret/Brand) passed into cmdutil.TestFactory(t, cfg) and remove the manual Factory construction and direct LarkClient/IOStreams setup.shortcuts/minutes/minutes_download.go (1)
264-287:⚠️ Potential issue | 🟠 MajorValidate the finalized output path before writing.
outputPathis only known after this block, but the code goes straight toStat/Save. An unsafe path currently comes back as a latecannot create fileafter the download request has already succeeded. Validate it once here and return a validation error before touching the body.As per coding guidelines, `Validate paths using validate.SafeInputPath before any file I/O operations`.Suggested change
if outputPath == "" { filename := resolveFilenameFromResponse(resp, minuteToken) // Deduplicate filenames in batch mode: prefix with token on collision. if opts.usedNames != nil { @@ } outputPath = filepath.Join(opts.outputDir, filename) } + if _, err := opts.fio.ResolvePath(outputPath); err != nil { + return nil, output.ErrValidation("unsafe output path: %s", err) + } if !opts.overwrite { if _, statErr := opts.fio.Stat(outputPath); statErr == nil { return nil, output.ErrValidation("output file already exists: %s (use --overwrite to replace)", outputPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/minutes/minutes_download.go` around lines 264 - 287, After computing outputPath (after the block that sets opts.outputPath / calls resolveFilenameFromResponse and deduplicates via opts.usedNames), validate the finalized path using validate.SafeInputPath(outputPath) and return an output.ErrValidation if validation fails; do this before any file I/O such as opts.fio.Stat or opts.fio.Save so you fail fast (before consuming resp.Body). Update the code paths around opts.overwrite/opts.fio.Stat and the subsequent opts.fio.Save call to assume a pre-validated outputPath, and reference the existing symbols resolveFilenameFromResponse, minuteToken, opts.usedNames, opts.outputDir, opts.overwrite, opts.fio.Stat, opts.fio.Save, and validate.SafeInputPath when making the change.
♻️ Duplicate comments (3)
internal/vfs/localfileio/localfileio.go (1)
34-39:⚠️ Potential issue | 🟠 MajorRoute these filesystem calls through
internal/vfs.
Open,Stat, andSavestill hitos.Open,os.Stat, andos.MkdirAlldirectly, so the new FileIO layer bypasses the repo’s VFS abstraction at its core filesystem boundaries.As per coding guidelines,
Use vfs.* instead of os.* for all filesystem access.Also applies to: 43-48, 64-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/localfileio.go` around lines 34 - 39, The LocalFileIO methods (Open, Stat, Save) call os.Open/os.Stat/os.MkdirAll directly and must instead route filesystem access through the repo VFS layer; update LocalFileIO.Open, LocalFileIO.Stat and LocalFileIO.Save (and any use of safeInputPath) to call the corresponding internal/vfs functions (e.g., vfs.Open, vfs.Stat, vfs.MkdirAll or their equivalents) and preserve existing error handling/return values so the FileIO implementation uses the VFS abstraction rather than the os package.shortcuts/drive/drive_download.go (1)
55-60:⚠️ Potential issue | 🟠 MajorDon’t key FileIO miss handling off
os.IsNotExist.
fileio.FileIO.Statdoes not promiseos.IsNotExistsemantics. A provider-specific validation/miss error is misclassified here, and the directruntime.FileIO().Stat(...)call still panics when no provider is registered. Reuseruntime.ValidatePath(outputPath)for the safety check, then only treatstatErr == nilas “already exists”. This also lets you drop theosimport.As per coding guidelines, `Validate paths using validate.SafeInputPath before any file I/O operations`.Suggested change
- // Early path validation + overwrite check via FileIO.Stat - if _, statErr := runtime.FileIO().Stat(outputPath); statErr != nil && !os.IsNotExist(statErr) { - return output.ErrValidation("unsafe output path: %s", statErr) - } else if statErr == nil && !overwrite { - return output.ErrValidation("output file already exists: %s (use --overwrite to replace)", outputPath) - } + if err := runtime.ValidatePath(outputPath); err != nil { + return output.ErrValidation("unsafe output path: %s", err) + } + if !overwrite { + if _, statErr := runtime.FileIO().Stat(outputPath); statErr == nil { + return output.ErrValidation("output file already exists: %s (use --overwrite to replace)", outputPath) + } + }🤖 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 55 - 60, Replace the runtime.FileIO().Stat-based validation with a call to runtime.ValidatePath(outputPath) (or validate.SafeInputPath per guidelines) to perform safety checks and avoid panics when no FileIO provider is registered, then only treat a subsequent runtime.FileIO().Stat(outputPath) result of statErr == nil as “already exists” (return the overwrite validation error when statErr == nil and !overwrite); ignore provider-specific/stat errors instead of checking os.IsNotExist and remove the os import. Ensure you update the code around the existing runtime.FileIO().Stat usage and the output.ErrValidation checks (referencing runtime.ValidatePath, runtime.FileIO().Stat, and overwrite).internal/cmdutil/testing.go (1)
95-104:⚠️ Potential issue | 🟠 MajorKeep
TestChdiron the VFS abstraction.This helper still uses process-wide
os.Chdirdirectly, and the cleanup path drops any restore failure. Please add/use a VFSChdirwrapper here so both the switch and restore stay inside the repo abstraction and can fail the test if cleanup breaks.As per coding guidelines,
Use vfs.* instead of os.* for all filesystem access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/testing.go` around lines 95 - 104, The TestChdir helper currently calls os.Chdir and ignores errors on restore; change it to use the VFS abstraction by calling vfs.Chdir(dir) for the switch and capture any error, and in the t.Cleanup closure call vfs.Chdir(orig) and fail the test (t.Fatalf) if that restore returns an error; keep the existing vfs.Getwd usage and preserve the nolint comments only if still applicable.
🧹 Nitpick comments (1)
shortcuts/base/helpers.go (1)
86-99: Extract the repeated comma-splitting logic into one helper.Line 90–Line 98 duplicates behavior already present in
parseStringListFlexible(Line 74–Line 82), which can drift over time.♻️ Proposed refactor
func parseStringListFlexible(fio fileio.FileIO, raw string, flagName string) ([]string, error) { @@ - parts := strings.Split(raw, ",") - result := make([]string, 0, len(parts)) - for _, part := range parts { - item := strings.TrimSpace(part) - if item != "" { - result = append(result, item) - } - } - return result, nil + return splitCommaList(raw), nil } func parseStringList(raw string) []string { raw = strings.TrimSpace(raw) if raw == "" { return nil } + return splitCommaList(raw) +} + +func splitCommaList(raw string) []string { parts := strings.Split(raw, ",") result := make([]string, 0, len(parts)) for _, part := range parts { item := strings.TrimSpace(part) if item != "" { result = append(result, item) } } return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/helpers.go` around lines 86 - 99, Extract the duplicated comma-splitting/trimming logic into a single helper (e.g., splitAndTrimCommaSeparated(raw string) []string) and replace the duplicated block in the current function (the block shown) and parseStringListFlexible to call this helper; the helper should TrimSpace(raw), return nil for empty input, Split by ",", TrimSpace each part, skip empty items, and return the resulting slice so nil-vs-empty semantics are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmdutil/factory_default_test.go`:
- Around line 211-231: The test
TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization can pick up
machine-local config because NewDefault follows normal initialization; to
isolate state, set the config dir to a temp directory at the start of the test
by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before invoking
NewDefault(InvocationContext{}), so the factory initialization (NewDefault) will
use an isolated config directory for the duration of the test.
In `@shortcuts/common/runner_jq_test.go`:
- Around line 109-114: The testResolvedFileIO stub methods (Open, Stat,
ResolvePath, Save) have formatting that causes gofmt drift; run gofmt (or gofmt
-w) on shortcuts/common/runner_jq_test.go to reformat these method declarations
so they follow Go formatting rules (ensuring spacing and line breaks for the
return statements match gofmt output) and commit the resulting changes.
In `@shortcuts/minutes/minutes_download.go`:
- Around line 81-82: The code dereferences runtime.FileIO() (used in the Stat
call and later saved into downloadOpts used by downloadMediaFile) without
checking for nil; add an explicit nil check after calling runtime.FileIO() and
return a CLI error (e.g., via output.ErrValidation or a suitable error helper)
if no provider is registered so the code fails fast instead of panicking. Update
the two occurrences (the Stat check around outputPath and the later assignment
into downloadOpts) to validate fileIO != nil before using or storing it, and
return a clear error message like "no FileIO provider registered" when nil.
---
Outside diff comments:
In `@shortcuts/common/runner_jq_test.go`:
- Around line 156-168: The test helper newTestFactory() currently constructs a
cmdutil.Factory manually and doesn't isolate config state; replace its usage
with the repository test helper by switching to cmdutil.TestFactory(t, config)
in tests and ensure you call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
so each test gets a hermetic per-test config dir; locate references to
newTestFactory and update the test to create a cfg (core.CliConfig with
AppID/AppSecret/Brand) passed into cmdutil.TestFactory(t, cfg) and remove the
manual Factory construction and direct LarkClient/IOStreams setup.
In `@shortcuts/minutes/minutes_download.go`:
- Around line 264-287: After computing outputPath (after the block that sets
opts.outputPath / calls resolveFilenameFromResponse and deduplicates via
opts.usedNames), validate the finalized path using
validate.SafeInputPath(outputPath) and return an output.ErrValidation if
validation fails; do this before any file I/O such as opts.fio.Stat or
opts.fio.Save so you fail fast (before consuming resp.Body). Update the code
paths around opts.overwrite/opts.fio.Stat and the subsequent opts.fio.Save call
to assume a pre-validated outputPath, and reference the existing symbols
resolveFilenameFromResponse, minuteToken, opts.usedNames, opts.outputDir,
opts.overwrite, opts.fio.Stat, opts.fio.Save, and validate.SafeInputPath when
making the change.
---
Duplicate comments:
In `@internal/cmdutil/testing.go`:
- Around line 95-104: The TestChdir helper currently calls os.Chdir and ignores
errors on restore; change it to use the VFS abstraction by calling
vfs.Chdir(dir) for the switch and capture any error, and in the t.Cleanup
closure call vfs.Chdir(orig) and fail the test (t.Fatalf) if that restore
returns an error; keep the existing vfs.Getwd usage and preserve the nolint
comments only if still applicable.
In `@internal/vfs/localfileio/localfileio.go`:
- Around line 34-39: The LocalFileIO methods (Open, Stat, Save) call
os.Open/os.Stat/os.MkdirAll directly and must instead route filesystem access
through the repo VFS layer; update LocalFileIO.Open, LocalFileIO.Stat and
LocalFileIO.Save (and any use of safeInputPath) to call the corresponding
internal/vfs functions (e.g., vfs.Open, vfs.Stat, vfs.MkdirAll or their
equivalents) and preserve existing error handling/return values so the FileIO
implementation uses the VFS abstraction rather than the os package.
In `@shortcuts/drive/drive_download.go`:
- Around line 55-60: Replace the runtime.FileIO().Stat-based validation with a
call to runtime.ValidatePath(outputPath) (or validate.SafeInputPath per
guidelines) to perform safety checks and avoid panics when no FileIO provider is
registered, then only treat a subsequent runtime.FileIO().Stat(outputPath)
result of statErr == nil as “already exists” (return the overwrite validation
error when statErr == nil and !overwrite); ignore provider-specific/stat errors
instead of checking os.IsNotExist and remove the os import. Ensure you update
the code around the existing runtime.FileIO().Stat usage and the
output.ErrValidation checks (referencing runtime.ValidatePath,
runtime.FileIO().Stat, and overwrite).
---
Nitpick comments:
In `@shortcuts/base/helpers.go`:
- Around line 86-99: Extract the duplicated comma-splitting/trimming logic into
a single helper (e.g., splitAndTrimCommaSeparated(raw string) []string) and
replace the duplicated block in the current function (the block shown) and
parseStringListFlexible to call this helper; the helper should TrimSpace(raw),
return nil for empty input, Split by ",", TrimSpace each part, skip empty items,
and return the resulting slice so nil-vs-empty semantics are preserved.
🪄 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: 5f0caf83-bf43-4d93-86b5-f17aa7b5ed34
📒 Files selected for processing (18)
extension/fileio/types.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/testing.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/path.gointernal/vfs/localfileio/path_test.goshortcuts/base/helpers.goshortcuts/common/runner.goshortcuts/common/runner_jq_test.goshortcuts/doc/doc_media_download.goshortcuts/drive/drive_download.goshortcuts/drive/drive_export_common.goshortcuts/drive/drive_export_test.goshortcuts/im/im_messages_resources_download.goshortcuts/minutes/minutes_download.goshortcuts/sheets/sheet_export.go
✅ Files skipped from review due to trivial changes (2)
- extension/fileio/types.go
- shortcuts/common/runner.go
🚧 Files skipped from review as they are similar to previous changes (8)
- shortcuts/drive/drive_export_test.go
- internal/client/response_test.go
- internal/vfs/localfileio/path_test.go
- internal/client/response.go
- internal/vfs/localfileio/path.go
- shortcuts/sheets/sheet_export.go
- shortcuts/im/im_messages_resources_download.go
- shortcuts/drive/drive_export_common.go
Comment on lines
+211
to
231
| func TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization(t *testing.T) { | ||
| prev := fileio.GetProvider() | ||
| provider := &countingFileIOProvider{} | ||
| fileio.Register(provider) | ||
| t.Cleanup(func() { fileio.Register(prev) }) | ||
|
|
||
| mid := &extensionMiddleware{Base: capturer, Ext: tamperIC} | ||
|
|
||
| origCtx := context.WithValue(context.Background(), testKey, "original") | ||
| req, _ := http.NewRequestWithContext(origCtx, "GET", srv.URL, nil) | ||
| resp, err := mid.RoundTrip(req) | ||
| if err != nil { | ||
| t.Fatalf("request failed: %v", err) | ||
| } | ||
| resp.Body.Close() | ||
|
|
||
| // Built-in chain should see original context, not tampered | ||
| if ctxValue != "original" { | ||
| t.Fatalf("built-in chain saw context value %q, want %q", ctxValue, "original") | ||
| } | ||
| } | ||
|
|
||
| // interceptorFunc adapts a function to exttransport.Interceptor. | ||
| type interceptorFunc func(*http.Request) func(*http.Response, error) | ||
|
|
||
| func (f interceptorFunc) PreRoundTrip(req *http.Request) func(*http.Response, error) { return f(req) } | ||
|
|
||
| func TestBuildSDKTransport_WithExtension(t *testing.T) { | ||
| exttransport.Register(&stubTransportProvider{}) | ||
| t.Cleanup(func() { exttransport.Register(nil) }) | ||
|
|
||
| transport := buildSDKTransport() | ||
|
|
||
| // Chain: extensionMiddleware → SecurityPolicy → UserAgent → Retry → Base | ||
| mid, ok := transport.(*extensionMiddleware) | ||
| if !ok { | ||
| t.Fatalf("outer transport type = %T, want *extensionMiddleware", transport) | ||
| } | ||
| sec, ok := mid.Base.(*internalauth.SecurityPolicyTransport) | ||
| if !ok { | ||
| t.Fatalf("transport type = %T, want *auth.SecurityPolicyTransport", mid.Base) | ||
| } | ||
| ua, ok := sec.Base.(*UserAgentTransport) | ||
| if !ok { | ||
| t.Fatalf("transport type = %T, want *UserAgentTransport", sec.Base) | ||
| f := NewDefault(InvocationContext{}) | ||
| if f.FileIOProvider != provider { | ||
| t.Fatalf("NewDefault() provider = %T, want %T", f.FileIOProvider, provider) | ||
| } | ||
| if _, ok := ua.Base.(*RetryTransport); !ok { | ||
| t.Fatalf("innermost transport type = %T, want *RetryTransport", ua.Base) | ||
| if provider.resolveCalls != 0 { | ||
| t.Fatalf("ResolveFileIO() calls after NewDefault() = %d, want 0", provider.resolveCalls) | ||
| } | ||
| } | ||
|
|
||
| func TestBuildSDKTransport_WithoutExtension(t *testing.T) { | ||
| exttransport.Register(nil) | ||
|
|
||
| transport := buildSDKTransport() | ||
|
|
||
| sec, ok := transport.(*internalauth.SecurityPolicyTransport) | ||
| if !ok { | ||
| t.Fatalf("outer transport type = %T, want *auth.SecurityPolicyTransport", transport) | ||
| } | ||
| ua, ok := sec.Base.(*UserAgentTransport) | ||
| if !ok { | ||
| t.Fatalf("middle transport type = %T, want *UserAgentTransport", sec.Base) | ||
| if got := f.ResolveFileIO(context.Background()); got == nil { | ||
| t.Fatal("ResolveFileIO() = nil, want non-nil") | ||
| } | ||
| if _, ok := ua.Base.(*RetryTransport); !ok { | ||
| t.Fatalf("inner transport type = %T, want *RetryTransport", ua.Base) | ||
| if provider.resolveCalls != 1 { | ||
| t.Fatalf("ResolveFileIO() calls after explicit resolve = %d, want 1", provider.resolveCalls) | ||
| } | ||
| } |
There was a problem hiding this comment.
Isolate config state in this test.
NewDefault still walks the normal factory initialization path, so this test can pick up machine-local config unless it sets a temp config dir first.
Suggested change
func TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
prev := fileio.GetProvider()
provider := &countingFileIOProvider{}
fileio.Register(provider)📝 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 TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization(t *testing.T) { | |
| prev := fileio.GetProvider() | |
| provider := &countingFileIOProvider{} | |
| fileio.Register(provider) | |
| t.Cleanup(func() { fileio.Register(prev) }) | |
| mid := &extensionMiddleware{Base: capturer, Ext: tamperIC} | |
| origCtx := context.WithValue(context.Background(), testKey, "original") | |
| req, _ := http.NewRequestWithContext(origCtx, "GET", srv.URL, nil) | |
| resp, err := mid.RoundTrip(req) | |
| if err != nil { | |
| t.Fatalf("request failed: %v", err) | |
| } | |
| resp.Body.Close() | |
| // Built-in chain should see original context, not tampered | |
| if ctxValue != "original" { | |
| t.Fatalf("built-in chain saw context value %q, want %q", ctxValue, "original") | |
| } | |
| } | |
| // interceptorFunc adapts a function to exttransport.Interceptor. | |
| type interceptorFunc func(*http.Request) func(*http.Response, error) | |
| func (f interceptorFunc) PreRoundTrip(req *http.Request) func(*http.Response, error) { return f(req) } | |
| func TestBuildSDKTransport_WithExtension(t *testing.T) { | |
| exttransport.Register(&stubTransportProvider{}) | |
| t.Cleanup(func() { exttransport.Register(nil) }) | |
| transport := buildSDKTransport() | |
| // Chain: extensionMiddleware → SecurityPolicy → UserAgent → Retry → Base | |
| mid, ok := transport.(*extensionMiddleware) | |
| if !ok { | |
| t.Fatalf("outer transport type = %T, want *extensionMiddleware", transport) | |
| } | |
| sec, ok := mid.Base.(*internalauth.SecurityPolicyTransport) | |
| if !ok { | |
| t.Fatalf("transport type = %T, want *auth.SecurityPolicyTransport", mid.Base) | |
| } | |
| ua, ok := sec.Base.(*UserAgentTransport) | |
| if !ok { | |
| t.Fatalf("transport type = %T, want *UserAgentTransport", sec.Base) | |
| f := NewDefault(InvocationContext{}) | |
| if f.FileIOProvider != provider { | |
| t.Fatalf("NewDefault() provider = %T, want %T", f.FileIOProvider, provider) | |
| } | |
| if _, ok := ua.Base.(*RetryTransport); !ok { | |
| t.Fatalf("innermost transport type = %T, want *RetryTransport", ua.Base) | |
| if provider.resolveCalls != 0 { | |
| t.Fatalf("ResolveFileIO() calls after NewDefault() = %d, want 0", provider.resolveCalls) | |
| } | |
| } | |
| func TestBuildSDKTransport_WithoutExtension(t *testing.T) { | |
| exttransport.Register(nil) | |
| transport := buildSDKTransport() | |
| sec, ok := transport.(*internalauth.SecurityPolicyTransport) | |
| if !ok { | |
| t.Fatalf("outer transport type = %T, want *auth.SecurityPolicyTransport", transport) | |
| } | |
| ua, ok := sec.Base.(*UserAgentTransport) | |
| if !ok { | |
| t.Fatalf("middle transport type = %T, want *UserAgentTransport", sec.Base) | |
| if got := f.ResolveFileIO(context.Background()); got == nil { | |
| t.Fatal("ResolveFileIO() = nil, want non-nil") | |
| } | |
| if _, ok := ua.Base.(*RetryTransport); !ok { | |
| t.Fatalf("inner transport type = %T, want *RetryTransport", ua.Base) | |
| if provider.resolveCalls != 1 { | |
| t.Fatalf("ResolveFileIO() calls after explicit resolve = %d, want 1", provider.resolveCalls) | |
| } | |
| } | |
| func TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization(t *testing.T) { | |
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | |
| prev := fileio.GetProvider() | |
| provider := &countingFileIOProvider{} | |
| fileio.Register(provider) | |
| t.Cleanup(func() { fileio.Register(prev) }) | |
| f := NewDefault(InvocationContext{}) | |
| if f.FileIOProvider != provider { | |
| t.Fatalf("NewDefault() provider = %T, want %T", f.FileIOProvider, provider) | |
| } | |
| if provider.resolveCalls != 0 { | |
| t.Fatalf("ResolveFileIO() calls after NewDefault() = %d, want 0", provider.resolveCalls) | |
| } | |
| if got := f.ResolveFileIO(context.Background()); got == nil { | |
| t.Fatal("ResolveFileIO() = nil, want non-nil") | |
| } | |
| if provider.resolveCalls != 1 { | |
| t.Fatalf("ResolveFileIO() calls after explicit resolve = %d, want 1", provider.resolveCalls) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmdutil/factory_default_test.go` around lines 211 - 231, The test
TestNewDefault_FileIOProviderDoesNotResolveDuringInitialization can pick up
machine-local config because NewDefault follows normal initialization; to
isolate state, set the config dir to a temp directory at the start of the test
by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before invoking
NewDefault(InvocationContext{}), so the factory initialization (NewDefault) will
use an isolated config directory for the duration of the test.
Comment on lines
+81
to
82
| if fi, err := runtime.FileIO().Stat(outputPath); err == nil && !fi.IsDir() { | ||
| return output.ErrValidation("--output %q is a file; batch mode expects a directory path", outputPath) |
There was a problem hiding this comment.
Fail fast when no FileIO provider is available.
runtime.FileIO() is dereferenced here and then stored into downloadOpts without a nil check. If a test or custom factory forgets to register a provider, downloadMediaFile will panic on Stat/Save instead of returning a normal CLI error.
Suggested change
errOut := runtime.IO().ErrOut
single := len(tokens) == 1
+ fio := runtime.FileIO()
+ if fio == nil {
+ return output.Errorf(output.ExitInternal, "io", "no file I/O provider registered")
+ }
// Batch mode: --output must be a directory, not an existing file.
if !single && outputPath != "" {
- if fi, err := runtime.FileIO().Stat(outputPath); err == nil && !fi.IsDir() {
+ if fi, err := fio.Stat(outputPath); err == nil && !fi.IsDir() {
return output.ErrValidation("--output %q is a file; batch mode expects a directory path", outputPath)
}
}
@@
- opts := downloadOpts{fio: runtime.FileIO(), overwrite: overwrite, usedNames: usedNames}
+ opts := downloadOpts{fio: fio, overwrite: overwrite, usedNames: usedNames}Also applies to: 164-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/minutes/minutes_download.go` around lines 81 - 82, The code
dereferences runtime.FileIO() (used in the Stat call and later saved into
downloadOpts used by downloadMediaFile) without checking for nil; add an
explicit nil check after calling runtime.FileIO() and return a CLI error (e.g.,
via output.ErrValidation or a suitable error helper) if no provider is
registered so the code fails fast instead of panicking. Update the two
occurrences (the Stat check around outputPath and the later assignment into
downloadOpts) to validate fileIO != nil before using or storing it, and return a
clear error message like "no FileIO provider registered" when nil.
6674484 to
e43f34d
Compare
| Format output.Format // output format for JSON responses | ||
| JqExpr string // if set, apply jq filter instead of Format | ||
| Out io.Writer // stdout | ||
| ErrOut io.Writer // stderr |
There was a problem hiding this comment.
HandleResponse
ResponseOptions.FileIO is documented as "nil falls back to direct os calls", but SaveResponse (called unconditionally from HandleResponse for any non-JSON binary response) says "fio must not be nil" and immediately calls fio.Save() — a nil interface dereference that panics at runtime.
cmd/api/api.go passes f.ResolveFileIO(opts.Ctx) which returns nil when f.FileIOProvider is unset. While the blank import of localfileio makes this unlikely in production today, the contradictory documentation is a correctness trap for any future caller who relies on the struct-field comment.
Either remove the "nil falls back to direct os calls" comment and add a nil guard in SaveResponse, or implement the advertised fallback:
// saveAndPrint / SaveResponse guard
if fio == nil {
return nil, fmt.Errorf("no FileIO provider available")
}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/drive/drive_upload.go (1)
63-67:⚠️ Potential issue | 🟡 MinorReject non-regular files during the initial stat.
This preflight only proves the path is stat-able. Directories, FIFOs, and device files still get past validation and then fail much later in the upload path with a less actionable error. Please mirror the
Mode().IsRegular()check you already have inpreflightDriveImportFile.🩹 Suggested fix
info, err := runtime.FileIO().Stat(filePath) if err != nil { return output.ErrValidation("cannot read file: %s", err) } + if !info.Mode().IsRegular() { + return output.ErrValidation("file must be a regular file: %s", filePath) + } fileSize := info.Size()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_upload.go` around lines 63 - 67, The initial stat in drive_upload.go uses runtime.FileIO().Stat(filePath) but doesn't reject non-regular files; add the same regular-file check used in preflightDriveImportFile by calling info.Mode().IsRegular() after retrieving info and returning output.ErrValidation("cannot read file: not a regular file") (or similar) when false so directories/FIFOs/devices fail early; reference the runtime.FileIO().Stat call, the returned info variable, the Mode().IsRegular() method, and output.ErrValidation to locate and implement the change.
♻️ Duplicate comments (5)
internal/client/response.go (2)
30-30:⚠️ Potential issue | 🔴 CriticalDon't panic when
FileIOis omitted.Line 30 still documents a nil fallback, but Lines 123-126 unconditionally call
fio.Save(...). Any zero-valueResponseOptionsor caller that trusts the comment will crash on a save path instead of getting a normal error.🩹 Minimal guard
type ResponseOptions struct { OutputPath string // --output flag; "" = auto-detect Format output.Format // output format for JSON responses JqExpr string // if set, apply jq filter instead of Format Out io.Writer // stdout ErrOut io.Writer // stderr - FileIO fileio.FileIO // file transfer abstraction; nil falls back to direct os calls + FileIO fileio.FileIO // file transfer abstraction; required when saving responses // CheckError is called on parsed JSON results. Nil defaults to CheckLarkResponse. CheckError func(interface{}) error } @@ func SaveResponse(fio fileio.FileIO, resp *larkcore.ApiResp, outputPath string) (map[string]interface{}, error) { + if fio == nil { + return nil, fmt.Errorf("file I/O not configured") + } result, err := fio.Save(outputPath, fileio.SaveOptions{ ContentType: resp.Header.Get("Content-Type"), ContentLength: int64(len(resp.RawBody)), }, bytes.NewReader(resp.RawBody))Also applies to: 122-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/response.go` at line 30, The code documents that ResponseOptions.FileIO may be nil but the code unconditionally calls fio.Save(...), which will panic for zero-value ResponseOptions; add a nil-guard that uses the fallback behavior when FileIO is nil: check ResponseOptions.FileIO (or local variable fio) before calling Save and if nil call the default/os-based save routine or return a normal error; update the path where fio.Save is invoked (the Save call site in the ResponseOptions handling) to perform this check and use the fallback implementation.
81-83:⚠️ Potential issue | 🟠 MajorPreserve
FileIO.Savevalidation errors.Lines 81-83 and 90-94 remap every save failure to
ExitInternal, and Line 128 flattens the originalfio.Saveerror with%s. Invalid--outputpaths will now look like internal failures instead of validation errors.💡 Suggested direction
- return nil, fmt.Errorf("cannot write file: %s", err) + return nil, fmt.Errorf("cannot write file: %w", err)Then let the two callers above pass validation-classified save errors through unchanged instead of forcing
ExitInternal.Also applies to: 90-94, 127-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/response.go` around lines 81 - 83, The call to SaveResponse (and underlying FileIO.Save) currently maps every error to output.Errorf with output.ExitInternal and flattens the original error; change SaveResponse error handling in response.go (calls to SaveResponse where you return output.Errorf) to detect validation-classified save errors from FileIO.Save (the specific validation error type/exit code your codebase uses) and return those errors unchanged instead of remapping to ExitInternal, and for non-validation errors continue wrapping as internal while preserving the original error (use error wrapping rather than replacing the message). Ensure this logic is applied to the SaveResponse call sites shown (the blocks using output.Errorf/output.ExitInternal) so validation errors pass through verbatim.shortcuts/minutes/minutes_download.go (1)
81-82:⚠️ Potential issue | 🟠 MajorFail fast when no
FileIOprovider is registered.These sites dereference
runtime.FileIO()without checking it. If a test or custom factory forgets to register a provider, the command panics before it can return a normal CLI error.🩹 Minimal guard
errOut := runtime.IO().ErrOut single := len(tokens) == 1 + fio := runtime.FileIO() + if fio == nil { + return output.Errorf(output.ExitInternal, "file_error", "no file I/O provider registered") + } // Batch mode: --output must be a directory, not an existing file. if !single && outputPath != "" { - if fi, err := runtime.FileIO().Stat(outputPath); err == nil && !fi.IsDir() { + if fi, err := fio.Stat(outputPath); err == nil && !fi.IsDir() { return output.ErrValidation("--output %q is a file; batch mode expects a directory path", outputPath) } } @@ - opts := downloadOpts{fio: runtime.FileIO(), overwrite: overwrite, usedNames: usedNames} + opts := downloadOpts{fio: fio, overwrite: overwrite, usedNames: usedNames}Also applies to: 164-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/minutes/minutes_download.go` around lines 81 - 82, The code dereferences runtime.FileIO() without nil-checks (e.g., the Stat call near the if using outputPath) which can panic if no FileIO provider is registered; add a guard that checks runtime.FileIO() != nil before calling Stat (and do the same for the other occurrence around lines referenced), and if nil return a sensible CLI error (via output.ErrValidation or another appropriate output error) indicating that no FileIO provider is registered so the command fails gracefully instead of panicking.shortcuts/drive/drive_download.go (1)
55-60:⚠️ Potential issue | 🟠 MajorValidate the destination before probing it.
Line 56 is using
Statas both a safety check and an existence probe, butos.IsNotExistis not guaranteed by theFileIOabstraction. A provider-specific miss will be reported asunsafe output path, and this bypasses the newruntime.ValidatePath(...)flow from the refactor.🐛 Proposed fix
import ( "context" "fmt" "net/http" - "os" larkcore "github.com/larksuite/oapi-sdk-go/v3/core" @@ - // Early path validation + overwrite check via FileIO.Stat - if _, statErr := runtime.FileIO().Stat(outputPath); statErr != nil && !os.IsNotExist(statErr) { - return output.ErrValidation("unsafe output path: %s", statErr) - } else if statErr == nil && !overwrite { - return output.ErrValidation("output file already exists: %s (use --overwrite to replace)", outputPath) + if err := runtime.ValidatePath(outputPath); err != nil { + return output.ErrValidation("unsafe output path: %s", err) + } + if !overwrite { + if _, statErr := runtime.FileIO().Stat(outputPath); statErr == nil { + return output.ErrValidation("output file already exists: %s (use --overwrite to replace)", outputPath) + } }Based on learnings,
Validate paths using validate.SafeInputPath before any file I/O operations.🤖 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 55 - 60, The code currently uses runtime.FileIO().Stat(outputPath) for both safety validation and existence checking, but FileIO.Stat may return provider-specific errors and os.IsNotExist isn't reliable; instead call the refactored path validation (runtime.ValidatePath or validate.SafeInputPath) on outputPath first to perform safety checks, then use runtime.FileIO().Stat only to determine whether the file exists and honor the overwrite flag (i.e., if Stat returns nil and overwrite is false return output.ErrValidation("output file already exists: %s", outputPath)); ensure you no longer treat non-OS-specific Stat errors as “unsafe output path” and surface them appropriately after the validate.SafeInputPath/runtime.ValidatePath step.shortcuts/im/im_messages_send.go (1)
214-221:⚠️ Potential issue | 🟠 MajorReject missing local media paths during validation.
Line 218 still ignores
os.IsNotExist, so values like--image ./missing.pngor--file ./missing.txtpassValidate/Executeand only fail later in the upload path. Return a validation error on anyStatfailure for non-URL, non-key inputs.🐛 Proposed fix
-import ( - "context" - "encoding/json" - "net/http" - "os" - "strings" +import ( + "context" + "encoding/json" + "net/http" + "strings" @@ func validateMediaFlagPath(fio fileio.FileIO, flagName, value string) error { if value == "" || strings.HasPrefix(value, "http://") || strings.HasPrefix(value, "https://") || isMediaKey(value) { return nil } - if _, err := fio.Stat(value); err != nil && !os.IsNotExist(err) { + if _, err := fio.Stat(value); err != nil { return output.ErrValidation("%s: %v", flagName, err) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_messages_send.go` around lines 214 - 221, The validateMediaFlagPath function currently treats os.IsNotExist errors as non-fatal, letting missing local files (e.g., --image ./missing.png) pass validation; change the Stat error handling so that for non-empty, non-URL, non-key values any error returned by fio.Stat(value) results in a validation error via output.ErrValidation, i.e., in validateMediaFlagPath replace the current conditional that ignores os.IsNotExist with logic that if fio.Stat(value) returns a non-nil err then return output.ErrValidation("%s: %v", flagName, err); keep the early return for empty/URL/key cases and only alter the Stat error branch.
🧹 Nitpick comments (2)
shortcuts/common/runner_jq_test.go (1)
156-168: Prefercmdutil.TestFactoryover a hand-rolled factory here.This helper already had to be updated for
FileIOProvider; that drift is exactly what the shared test factory is supposed to prevent. BuildingnewTestFactoryon top ofcmdutil.TestFactory(t, config)will keep futureFactoryadditions from silently diverging again.As per coding guidelines:
Use cmdutil.TestFactory(t, config) for creating test factories in Go tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/runner_jq_test.go` around lines 156 - 168, Replace the hand-rolled newTestFactory with the shared cmdutil.TestFactory to avoid drift: construct a core.CliConfig with AppID/AppSecret/Brand and any required Lark client stub, then call cmdutil.TestFactory(t, config) to produce the *cmdutil.Factory, and ensure the returned Factory has FileIOProvider set (or pass it via TestFactory options if available); update references to newTestFactory to use the TestFactory-backed factory instead of manually building cmdutil.Factory.shortcuts/im/helpers_network_test.go (1)
90-103: Prefercmdutil.TestFactoryfor these runtimes.Both blocks manually rebuild
cmdutil.Factory, which is why this file had to start copyingFileIOProviderplumbing in this PR.cmdutil.TestFactory(t, cfg)plus an isolated config dir will keep future factory defaults from drifting out of these tests.As per coding guidelines,
**/*_test.go: Usecmdutil.TestFactory(t, config)for creating test factories in Go testsandIsolate config state in Go tests by using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())`.Also applies to: 418-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/helpers_network_test.go` around lines 90 - 103, Replace the manual construction of cmdutil.Factory inside the runtime (the common.RuntimeContext runtime variable and its Factory field) with cmdutil.TestFactory(t, cfg) and ensure tests isolate config state by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before creating the factory; also remove the duplicated FileIOProvider/IOStreams plumbing since TestFactory provides the proper defaults for HttpClient, LarkClient, Credential and file I/O in test contexts.
🤖 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 623-631: The code dereferences rctx.FileIO() without checking for
nil, which can panic when no FileIO provider is registered; update the block
that calls rctx.FileIO().Open(path) to first get the provider into a local
(e.g., fio := rctx.FileIO()), return a FlagErrorf indicating missing file IO if
fio == nil, then call fio.Open(path) and proceed with io.ReadAll and f.Close()
as before; use the same FlagErrorf style as the other checks (referencing
rctx.FileIO(), Open, FlagErrorf, io.ReadAll, f.Close()) to keep error messages
consistent.
---
Outside diff comments:
In `@shortcuts/drive/drive_upload.go`:
- Around line 63-67: The initial stat in drive_upload.go uses
runtime.FileIO().Stat(filePath) but doesn't reject non-regular files; add the
same regular-file check used in preflightDriveImportFile by calling
info.Mode().IsRegular() after retrieving info and returning
output.ErrValidation("cannot read file: not a regular file") (or similar) when
false so directories/FIFOs/devices fail early; reference the
runtime.FileIO().Stat call, the returned info variable, the Mode().IsRegular()
method, and output.ErrValidation to locate and implement the change.
---
Duplicate comments:
In `@internal/client/response.go`:
- Line 30: The code documents that ResponseOptions.FileIO may be nil but the
code unconditionally calls fio.Save(...), which will panic for zero-value
ResponseOptions; add a nil-guard that uses the fallback behavior when FileIO is
nil: check ResponseOptions.FileIO (or local variable fio) before calling Save
and if nil call the default/os-based save routine or return a normal error;
update the path where fio.Save is invoked (the Save call site in the
ResponseOptions handling) to perform this check and use the fallback
implementation.
- Around line 81-83: The call to SaveResponse (and underlying FileIO.Save)
currently maps every error to output.Errorf with output.ExitInternal and
flattens the original error; change SaveResponse error handling in response.go
(calls to SaveResponse where you return output.Errorf) to detect
validation-classified save errors from FileIO.Save (the specific validation
error type/exit code your codebase uses) and return those errors unchanged
instead of remapping to ExitInternal, and for non-validation errors continue
wrapping as internal while preserving the original error (use error wrapping
rather than replacing the message). Ensure this logic is applied to the
SaveResponse call sites shown (the blocks using
output.Errorf/output.ExitInternal) so validation errors pass through verbatim.
In `@shortcuts/drive/drive_download.go`:
- Around line 55-60: The code currently uses runtime.FileIO().Stat(outputPath)
for both safety validation and existence checking, but FileIO.Stat may return
provider-specific errors and os.IsNotExist isn't reliable; instead call the
refactored path validation (runtime.ValidatePath or validate.SafeInputPath) on
outputPath first to perform safety checks, then use runtime.FileIO().Stat only
to determine whether the file exists and honor the overwrite flag (i.e., if Stat
returns nil and overwrite is false return output.ErrValidation("output file
already exists: %s", outputPath)); ensure you no longer treat non-OS-specific
Stat errors as “unsafe output path” and surface them appropriately after the
validate.SafeInputPath/runtime.ValidatePath step.
In `@shortcuts/im/im_messages_send.go`:
- Around line 214-221: The validateMediaFlagPath function currently treats
os.IsNotExist errors as non-fatal, letting missing local files (e.g., --image
./missing.png) pass validation; change the Stat error handling so that for
non-empty, non-URL, non-key values any error returned by fio.Stat(value) results
in a validation error via output.ErrValidation, i.e., in validateMediaFlagPath
replace the current conditional that ignores os.IsNotExist with logic that if
fio.Stat(value) returns a non-nil err then return output.ErrValidation("%s: %v",
flagName, err); keep the early return for empty/URL/key cases and only alter the
Stat error branch.
In `@shortcuts/minutes/minutes_download.go`:
- Around line 81-82: The code dereferences runtime.FileIO() without nil-checks
(e.g., the Stat call near the if using outputPath) which can panic if no FileIO
provider is registered; add a guard that checks runtime.FileIO() != nil before
calling Stat (and do the same for the other occurrence around lines referenced),
and if nil return a sensible CLI error (via output.ErrValidation or another
appropriate output error) indicating that no FileIO provider is registered so
the command fails gracefully instead of panicking.
---
Nitpick comments:
In `@shortcuts/common/runner_jq_test.go`:
- Around line 156-168: Replace the hand-rolled newTestFactory with the shared
cmdutil.TestFactory to avoid drift: construct a core.CliConfig with
AppID/AppSecret/Brand and any required Lark client stub, then call
cmdutil.TestFactory(t, config) to produce the *cmdutil.Factory, and ensure the
returned Factory has FileIOProvider set (or pass it via TestFactory options if
available); update references to newTestFactory to use the TestFactory-backed
factory instead of manually building cmdutil.Factory.
In `@shortcuts/im/helpers_network_test.go`:
- Around line 90-103: Replace the manual construction of cmdutil.Factory inside
the runtime (the common.RuntimeContext runtime variable and its Factory field)
with cmdutil.TestFactory(t, cfg) and ensure tests isolate config state by
calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before creating the
factory; also remove the duplicated FileIOProvider/IOStreams plumbing since
TestFactory provides the proper defaults for HttpClient, LarkClient, Credential
and file I/O in test contexts.
🪄 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: 8e11b384-1055-401f-9b28-197a09537935
📒 Files selected for processing (81)
.golangci.ymlcmd/api/api.gocmd/service/service.goextension/fileio/registry.goextension/fileio/types.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/testing.gointernal/validate/atomicwrite.gointernal/validate/path.gointernal/vfs/localfileio/atomicwrite.gointernal/vfs/localfileio/atomicwrite_test.gointernal/vfs/localfileio/input.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/path.gointernal/vfs/localfileio/path_test.goshortcuts/base/base_shortcut_helpers.goshortcuts/base/base_shortcuts_test.goshortcuts/base/dashboard_block_create.goshortcuts/base/dashboard_block_update.goshortcuts/base/dashboard_ops.goshortcuts/base/field_ops.goshortcuts/base/helpers.goshortcuts/base/helpers_test.goshortcuts/base/record_ops.goshortcuts/base/record_upload_attachment.goshortcuts/base/table_ops.goshortcuts/base/view_ops.goshortcuts/base/workflow_create.goshortcuts/base/workflow_update.goshortcuts/common/common_test.goshortcuts/common/helpers.goshortcuts/common/runner.goshortcuts/common/runner_input_test.goshortcuts/common/runner_jq_test.goshortcuts/common/validate.goshortcuts/common/validate_test.goshortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_upload.goshortcuts/drive/drive_download.goshortcuts/drive/drive_export.goshortcuts/drive/drive_export_common.goshortcuts/drive/drive_export_test.goshortcuts/drive/drive_import.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_import_test.goshortcuts/drive/drive_upload.goshortcuts/im/coverage_additional_test.goshortcuts/im/helpers.goshortcuts/im/helpers_network_test.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_reply.goshortcuts/im/im_messages_resources_download.goshortcuts/im/im_messages_send.goshortcuts/im/validate_media_test.goshortcuts/mail/draft/acceptance_test.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_attachment_test.goshortcuts/mail/draft/patch_body_test.goshortcuts/mail/draft/patch_header_test.goshortcuts/mail/draft/patch_recipient_test.goshortcuts/mail/draft/patch_test.goshortcuts/mail/draft/serialize_golden_test.goshortcuts/mail/draft/serialize_test.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/minutes/minutes_download.goshortcuts/sheets/sheet_export.goshortcuts/vc/vc_notes.go
💤 Files with no reviewable changes (4)
- shortcuts/common/common_test.go
- shortcuts/common/helpers.go
- shortcuts/common/validate.go
- shortcuts/common/validate_test.go
✅ Files skipped from review due to trivial changes (24)
- shortcuts/common/runner_input_test.go
- internal/vfs/localfileio/atomicwrite_test.go
- shortcuts/drive/drive_export_test.go
- shortcuts/base/dashboard_block_create.go
- shortcuts/doc/doc_media_insert.go
- shortcuts/doc/doc_media_upload.go
- cmd/service/service.go
- shortcuts/base/dashboard_ops.go
- shortcuts/mail/draft/serialize_golden_test.go
- shortcuts/base/field_ops.go
- shortcuts/mail/draft/serialize_test.go
- shortcuts/mail/emlbuilder/builder_test.go
- shortcuts/base/workflow_update.go
- shortcuts/mail/draft/patch_attachment_test.go
- shortcuts/mail/draft/patch_test.go
- shortcuts/base/base_shortcuts_test.go
- shortcuts/base/dashboard_block_update.go
- shortcuts/drive/drive_import_common.go
- internal/cmdutil/factory_default_test.go
- shortcuts/mail/draft/patch_recipient_test.go
- extension/fileio/types.go
- shortcuts/base/view_ops.go
- shortcuts/im/im_messages_resources_download.go
- shortcuts/base/base_shortcut_helpers.go
🚧 Files skipped from review as they are similar to previous changes (28)
- shortcuts/drive/drive_import_test.go
- shortcuts/base/table_ops.go
- shortcuts/drive/drive_export.go
- cmd/api/api.go
- shortcuts/mail/draft/acceptance_test.go
- shortcuts/mail/draft/patch_body_test.go
- shortcuts/im/helpers_test.go
- shortcuts/im/coverage_additional_test.go
- internal/cmdutil/factory.go
- shortcuts/base/record_upload_attachment.go
- shortcuts/mail/mail_reply_all.go
- shortcuts/drive/drive_export_common.go
- shortcuts/mail/mail_forward.go
- shortcuts/base/helpers_test.go
- shortcuts/vc/vc_notes.go
- shortcuts/base/helpers.go
- internal/cmdutil/factory_default.go
- shortcuts/mail/helpers.go
- internal/vfs/localfileio/input.go
- shortcuts/im/validate_media_test.go
- shortcuts/mail/emlbuilder/builder.go
- extension/fileio/registry.go
- shortcuts/mail/draft/patch.go
- shortcuts/im/im_messages_reply.go
- shortcuts/base/workflow_create.go
- shortcuts/sheets/sheet_export.go
- shortcuts/doc/doc_media_download.go
- shortcuts/base/record_ops.go
Comment on lines
+623
to
631
| f, err := rctx.FileIO().Open(path) | ||
| if err != nil { | ||
| return FlagErrorf("--%s: invalid file path %q: %v", fl.Name, path, err) | ||
| return FlagErrorf("--%s: cannot read file %q: %v", fl.Name, path, err) | ||
| } | ||
| data, err := vfs.ReadFile(safePath) | ||
| data, err := io.ReadAll(f) | ||
| f.Close() | ||
| if err != nil { | ||
| return FlagErrorf("--%s: cannot read file %q: %v", fl.Name, path, err) | ||
| } |
There was a problem hiding this comment.
Missing nil check for FileIO() — potential panic.
rctx.FileIO() can return nil (per lines 299-316), but this code dereferences it without checking. If no FileIO provider is registered, this will panic at runtime.
Unlike ValidatePath (which now has a nil check at line 334), this path was not updated.
🐛 Proposed fix
// file: `@path`
if strings.HasPrefix(raw, "@") {
if !slices.Contains(fl.Input, File) {
return FlagErrorf("--%s does not support file input (`@path`)", fl.Name)
}
path := strings.TrimSpace(raw[1:])
if path == "" {
return FlagErrorf("--%s: file path cannot be empty after @", fl.Name)
}
+ fio := rctx.FileIO()
+ if fio == nil {
+ return FlagErrorf("--%s: no file I/O provider available", fl.Name)
+ }
- f, err := rctx.FileIO().Open(path)
+ f, err := fio.Open(path)
if err != nil {
return FlagErrorf("--%s: cannot read file %q: %v", fl.Name, path, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| f, err := rctx.FileIO().Open(path) | |
| if err != nil { | |
| return FlagErrorf("--%s: invalid file path %q: %v", fl.Name, path, err) | |
| return FlagErrorf("--%s: cannot read file %q: %v", fl.Name, path, err) | |
| } | |
| data, err := vfs.ReadFile(safePath) | |
| data, err := io.ReadAll(f) | |
| f.Close() | |
| if err != nil { | |
| return FlagErrorf("--%s: cannot read file %q: %v", fl.Name, path, err) | |
| } | |
| path := strings.TrimSpace(raw[1:]) | |
| if path == "" { | |
| return FlagErrorf("--%s: file path cannot be empty after @", fl.Name) | |
| } | |
| fio := rctx.FileIO() | |
| if fio == nil { | |
| return FlagErrorf("--%s: no file I/O provider available", fl.Name) | |
| } | |
| f, err := fio.Open(path) | |
| if err != nil { | |
| return FlagErrorf("--%s: cannot read file %q: %v", fl.Name, path, err) | |
| } | |
| data, err := io.ReadAll(f) | |
| f.Close() | |
| if err != nil { | |
| return FlagErrorf("--%s: cannot read file %q: %v", fl.Name, path, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/common/runner.go` around lines 623 - 631, The code dereferences
rctx.FileIO() without checking for nil, which can panic when no FileIO provider
is registered; update the block that calls rctx.FileIO().Open(path) to first get
the provider into a local (e.g., fio := rctx.FileIO()), return a FlagErrorf
indicating missing file IO if fio == nil, then call fio.Open(path) and proceed
with io.ReadAll and f.Close() as before; use the same FlagErrorf style as the
other checks (referencing rctx.FileIO(), Open, FlagErrorf, io.ReadAll,
f.Close()) to keep error messages consistent.
2 tasks
Change-Id: I5d163fd8f5a50a2fb6c5406f7053af51cb813f7e
tuxedomm
force-pushed
the
feat/fileio-extension-refactor-plugin
branch
from
e43f34d to
7625b80
Compare
Collaborator
Author
|
Superseded/obsolete. Closing and wiping branch history. |
github-actions
Bot
added
size/L
github-actions
Bot
removed
domain/im
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
=======================================
Coverage 60.92% 60.92%
=======================================
Files 399 399
Lines 34089 34089
=======================================
Hits 20770 20770
Misses 11395 11395
Partials 1924 1924 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduce a FileIO extension layer that abstracts file transfer operations (Open/Stat/Save) in shortcuts behind a replaceable interface, eliminating direct
ospackage dependencies and providing an extension point for server mode and other scenarios.Changes
extension/fileiopackage with Provider/FileIO/File interfaces and registryLocalFileIOdefault implementation with built-in path validation and atomic writesFileIO.SaveFileIO.Open/FileIO.StatRuntimeContext.ValidatePathto deduplicate Stat-based path checksTest Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit