refactor: migrate common/client/im to FileIO and add localfileio tests#322
Merged
refactor: migrate common/client/im to FileIO and add localfileio tests#322
Conversation
- runner resolveInputFlags: replace validate.SafeInputPath + vfs.ReadFile with FileIO.Open + io.ReadAll - SaveResponse: delegate to FileIO.Save + ResolvePath - cmd/api, cmd/service: pass FileIO to ResponseOptions - im: replace validate.SafeLocalFlagPath with RuntimeContext.ValidatePath, migrate download/upload to FileIO.Save/Open/Stat - Add path_test.go and atomicwrite_test.go for localfileio - Add validate_media_test.go for im media flag validation - Adapt test mocks to fileio.FileInfo interface Change-Id: I1cacaf36a07af6b011e275680ac20192422406e7
…write - Remove ErrMkdir sentinel, replace with MkdirError type - Add WriteError type to wrap file write failures - Error types use transparent Error() (no prefix) to avoid breakchange - SaveResponse keeps switch for backward-compatible error messages - WrapSaveError uses errors.As for type-based matching Change-Id: Iaeba30b82cf5ebaa1868d6efbb471968094d405c
github-actions
Bot
added
domain/im
📝 WalkthroughWalkthroughThreads a FileIO provider through response handling and shortcut flows, replaces direct path validation/atomic writes with FileIO.Save/ResolvePath, introduces typed file I/O errors ( Changes
Sequence Diagram(s)sequenceDiagram
actor CLI
participant Handler as HandleResponse
participant Factory as FileIOProvider
participant FileIO as FileIO (resolved)
participant LocalFS as LocalFileIO
CLI->>Handler: HandleResponse(resp, opts{FileIO?})
opt opts.FileIO == nil
Handler->>Factory: ResolveFileIO(ctx)
Factory-->>Handler: FileIO
end
Handler->>FileIO: ResolvePath(outputPath) / Save(resp.Body, options)
FileIO->>LocalFS: Save implementation (mkdir, atomic write)
LocalFS-->>FileIO: SaveResult{path,size} or Error (PathValidation/Mkdir/Write)
FileIO-->>Handler: result or wrapped error
Handler->>CLI: return metadata or mapped user-facing error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8636b7aa12dc4fc363a8ee5f07062317b4306140🧩 Skill updatenpx skills add larksuite/cli#feat/fileio-migrate-common-im -y -g |
Greptile SummaryThis PR migrates Confidence Score: 5/5Safe to merge; the single finding is a style inconsistency with no current functional impact. All remaining findings are P2. The only inline comment is about using %s vs %w in SaveResponse — the current callers never inspect the error chain, so there is no runtime breakage. The core FileIO migration, path validation, and atomic-write logic are correct and well-tested. internal/client/response.go — error wrapping uses %s instead of %w, inconsistent with WrapSaveError.
|
| Filename | Overview |
|---|---|
| internal/vfs/localfileio/localfileio.go | New LocalFileIO implementation: Open/Stat delegate to SafeInputPath + vfs, Save uses SafeOutputPath + AtomicWriteFromReader. Clean, correct implementation. |
| shortcuts/common/runner.go | Adds FileIO(), ResolveSavePath(), ValidatePath(), WrapSaveError(), WrapOpenError() helpers. ResolveSavePath and ValidatePath guard against nil FileIO; resolveInputFlags at line 667 does not (flagged in previous review). |
| internal/client/response.go | SaveResponse migrated to FileIO.Save; error classification uses %s instead of %w (breaks error chain), inconsistent with WrapSaveError in runner.go which uses %w. |
| shortcuts/im/im_messages_resources_download.go | Download migrated to FileIO.Save; ResolveSavePath called after Save succeeds to get absolute path for display. FileIO() called without nil guard on line 174. |
| extension/fileio/errors.go | New typed errors: PathValidationError (multi-unwrap for ErrPathValidation sentinel), MkdirError, WriteError. Well-designed for errors.Is/As matching. |
| shortcuts/im/helpers.go | validateMediaFlagPath added; receives fio from runtime.FileIO() without nil check in callers (send/reply shortcuts). |
| internal/vfs/localfileio/localfileio_test.go | Good coverage: Open/Stat/Save/ResolvePath happy paths and rejections; error message content test couples to path.go internals but correctly documents the expected flag-name conventions. |
| internal/client/response_test.go | New tests for SaveResponse, HandleResponse binary/JSON/error paths using real LocalFileIO; good coverage of non-JSON error handling and path traversal rejection. |
Sequence Diagram
sequenceDiagram
participant CLI as CLI User
participant Validate as Shortcut.Validate
participant Execute as Shortcut.Execute
participant FileIO as LocalFileIO
participant Path as SafeOutputPath
participant Atomic as AtomicWriteFromReader
CLI->>Validate: run +messages-resources-download
Validate->>Path: normalizeDownloadOutputPath()
Validate->>FileIO: ResolveSavePath(relPath)
FileIO->>Path: SafeOutputPath(relPath)
Path-->>FileIO: absolutePath or error
FileIO-->>Validate: validated (or ErrValidation)
CLI->>Execute: (after Validate passes)
Execute->>Execute: normalizeDownloadOutputPath() again
Execute->>Execute: DoAPIStream() → downloadResp
Execute->>Execute: auto-detect ext → finalPath
Execute->>FileIO: Save(finalPath, opts, body)
FileIO->>Path: SafeOutputPath(finalPath)
Path-->>FileIO: safePath
FileIO->>Atomic: AtomicWriteFromReader(safePath, body)
Atomic-->>FileIO: SaveResult{size}
FileIO-->>Execute: SaveResult
Execute->>FileIO: ResolveSavePath(finalPath)
FileIO->>Path: SafeOutputPath(finalPath)
Path-->>FileIO: absolutePath
FileIO-->>Execute: absolutePath
Execute-->>CLI: {saved_path, size_bytes}
Reviews (2): Last reviewed commit: "style: fix gofmt formatting in test file..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
shortcuts/im/helpers.go (2)
553-576:⚠️ Potential issue | 🟡 MinorMissing nil guard on
runtime.FileIO()calls.The
parseMediaDurationfunction callsruntime.FileIO().Statandruntime.FileIO().Openwithout checking for nil. Based oninternal/cmdutil/factory.go:48-55,ResolveFileIOcan return nil whenFileIOProvideris nil.While the current code silently returns
""on errors (which is acceptable for duration parsing), a nilFileIOwould cause a panic.🛡️ Suggested defensive check
func parseMediaDuration(runtime *common.RuntimeContext, filePath, fileType string) string { if fileType != "opus" && fileType != "mp4" { return "" } + fio := runtime.FileIO() + if fio == nil { + return "" + } - info, err := runtime.FileIO().Stat(filePath) + info, err := fio.Stat(filePath) if err != nil || info.Size() == 0 { return "" } - f, err := runtime.FileIO().Open(filePath) + f, err := fio.Open(filePath) if err != nil { return "" } + defer f.Close()Also note: the file handle
fopened at line 561 is never closed, which is a resource leak.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/helpers.go` around lines 553 - 576, In parseMediaDuration, guard against a nil FileIO provider and close the opened file: first assign the result of runtime.FileIO() to a local (e.g., fi := runtime.FileIO()), return "" immediately if fi == nil, then use fi.Stat and fi.Open instead of calling runtime.FileIO() repeatedly; after successfully opening f, defer f.Close() to avoid the resource leak; keep existing error returns ("" on failure) otherwise.
999-1008:⚠️ Potential issue | 🟡 MinorPotential nil panic and missing file close in
uploadImageToIM.Similar to
parseMediaDuration,runtime.FileIO()could return nil causing a panic. The function does properly close the file viadefer f.Close()at line 1008, which is good.🛡️ Suggested nil guard
func uploadImageToIM(ctx context.Context, runtime *common.RuntimeContext, filePath, imageType string) (string, error) { + fio := runtime.FileIO() + if fio == nil { + return "", fmt.Errorf("file I/O not available") + } - if info, err := runtime.FileIO().Stat(filePath); err == nil && info.Size() > maxImageUploadSize { + if info, err := fio.Stat(filePath); err == nil && info.Size() > maxImageUploadSize { return "", fmt.Errorf("image size %s exceeds limit (max 5MB)", common.FormatSize(info.Size())) } - f, err := runtime.FileIO().Open(filePath) + f, err := fio.Open(filePath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/helpers.go` around lines 999 - 1008, In uploadImageToIM, runtime.FileIO() is used without a nil check which can panic if it returns nil; obtain the FileIO once (e.g., fio := runtime.FileIO()), guard if fio == nil and return a clear error, then call fio.Stat(...) and fio.Open(...) instead of runtime.FileIO(); keep the existing defer f.Close() after successful open to ensure the file is closed.cmd/service/service.go (1)
247-256:⚠️ Potential issue | 🟠 MajorFileIO is correctly threaded but lacks nil safety in SaveResponse.
The addition of
FileIO: f.ResolveFileIO(opts.Ctx)properly wires the file I/O abstraction. However, a nil pointer dereference risk exists:ResolveFileIOcan return nil whenFileIOProvideris nil (perfactory.go:48-55), butSaveResponse(line 123 ofresponse.go) immediately callsfio.Save()without nil checking, violating the contract stated in its own comment: "fio must not be nil."The auto-save path at line 82 calls
SaveResponse(opts.FileIO, ...)without guarding against nil FileIO. Add a nil check before calling SaveResponse or ensure ResolveFileIO never returns nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/service/service.go` around lines 247 - 256, The auto-save path calls SaveResponse(opts.FileIO, ...) but ResolveFileIO(opts.Ctx) can return nil when FileIOProvider is nil, so add a nil-safety check before invoking SaveResponse: where the code calls SaveResponse(opts.FileIO, ...) (auto-save path), guard with if opts.FileIO != nil { SaveResponse(opts.FileIO, resp, ...) } else { skip/save fallback (e.g., log or return nil) }; alternatively ensure ResolveFileIO never returns nil by returning a no-op FileIO implementation from ResolveFileIO, but the simplest change is to check opts.FileIO for nil before calling SaveResponse to avoid the nil dereference in SaveResponse (function SaveResponse and method ResolveFileIO are the relevant symbols).
🧹 Nitpick comments (5)
shortcuts/common/runner_jq_test.go (1)
165-166: Please switch this helper tocmdutil.TestFactory.Having to append
FileIOProviderhere is another sign that this hand-built factory helper is drifting from the shared test setup. ConvertingnewTestFactoryto usecmdutil.TestFactorywill keep these dependencies in one place and isolate config state at the same time.As per coding guidelines,
**/*_test.go: Usecmdutil.TestFactory(t, config)for creating test factories in Go tests;**/*_test.go: 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/common/runner_jq_test.go` around lines 165 - 166, The current test helper newTestFactory constructs a manual factory and sets IOStreams/FileIOProvider directly; change newTestFactory to call cmdutil.TestFactory(t, config) instead and remove manual FileIOProvider/IOStreams configuration so the shared test helper manages those concerns. Update tests that call newTestFactory to accept a *testing.T and ensure they call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before creating the factory; keep references to newTestFactory and replace its internals to return the cmdutil.TestFactory-produced factory while preserving any config argument handling.internal/vfs/localfileio/localfileio_test.go (2)
52-73: Check error fromos.WriteFilein test setup.Ignoring the error from
os.WriteFileon line 57 could mask test environment issues. While unlikely to fail, checking the error improves test reliability.Suggested change
content := []byte("hello world") - os.WriteFile("test.txt", content, 0644) + if err := os.WriteFile("test.txt", content, 0644); err != nil { + t.Fatalf("WriteFile: %v", err) + }The same pattern should be applied to line 114 in
TestLocalFileIO_Stat_ValidFile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/localfileio_test.go` around lines 52 - 73, The test helpers TestLocalFileIO_Open_ValidFile and TestLocalFileIO_Stat_ValidFile currently call os.WriteFile without checking its return; update both tests (the os.WriteFile calls in TestLocalFileIO_Open_ValidFile and TestLocalFileIO_Stat_ValidFile) to capture the error and fail the test on error (e.g., if err := os.WriteFile(...); err != nil { t.Fatalf("os.WriteFile failed: %v", err) }) so any setup failures are reported immediately.
16-28: Consider reusingcmdutil.TestChdirinstead of duplicating it.This
testChdirhelper is functionally identical tocmdutil.TestChdir. Reusing the existing helper would reduce duplication and ensure consistent behavior if the canonical implementation changes.Suggested change
-// testChdir temporarily changes the working directory for a test. -// Not compatible with t.Parallel(). -func testChdir(t *testing.T, dir string) { - t.Helper() - orig, err := os.Getwd() - if err != nil { - t.Fatal(err) - } - if err := os.Chdir(dir); err != nil { - t.Fatal(err) - } - t.Cleanup(func() { os.Chdir(orig) }) -}Then import
cmdutiland replace calls totestChdir(t, dir)withcmdutil.TestChdir(t, dir).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/localfileio_test.go` around lines 16 - 28, The local helper function testChdir in this file duplicates existing behavior; remove the testChdir function and replace all calls to testChdir(t, dir) with cmdutil.TestChdir(t, dir), adding an import for the cmdutil package; ensure any reference to testChdir (function name) is updated and that the new cmdutil import is added to the test file's import block so tests compile and use the canonical helper.internal/vfs/localfileio/path_test.go (1)
66-85: Consider extracting the repeated chdir setup into a helper.The pattern of
dir, _ = filepath.EvalSymlinks(dir)+origDir, _ := os.Getwd()+defer os.Chdir(origDir)+os.Chdir(dir)is repeated in multiple tests. A helper similar totestChdirinlocalfileio_test.go(or reusingcmdutil.TestChdir) would reduce boilerplate.🤖 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 66 - 85, The test TestSafeOutputPath_ReturnsCanonicalAbsolutePath repeats setup code for changing to a temp dir (EvalSymlinks + os.Getwd + defer os.Chdir(origDir) + os.Chdir(dir)); refactor by extracting that sequence into a helper (e.g., testChdir used in localfileio_test.go or reuse cmdutil.TestChdir) and call it at the top of TestSafeOutputPath_ReturnsCanonicalAbsolutePath before invoking SafeOutputPath so the test body only contains the WHEN/THEN assertions.shortcuts/common/runner.go (1)
667-679: Consider usingdeferforf.Close()to ensure cleanup on all paths.The current pattern calls
f.Close()afterio.ReadAll, which works but is fragile. Usingdeferimmediately after a successfulOpenis the idiomatic Go pattern and ensures the file is closed even if future code changes introduce early returns.Suggested change
f, err := rctx.FileIO().Open(path) if err != nil { if errors.Is(err, fileio.ErrPathValidation) { return FlagErrorf("--%s: invalid file path %q: %v", fl.Name, path, err) } return FlagErrorf("--%s: cannot read file %q: %v", fl.Name, path, err) } + defer f.Close() 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 667 - 679, After successfully opening the file with rctx.FileIO().Open(path), defer f.Close() immediately (right after the Open success) instead of calling f.Close() after io.ReadAll; remove the explicit f.Close() call, and if you want to surface close failures, check the deferred Close error where appropriate and return a FlagErrorf (same style as existing errors) so file handles are always released even on early returns; references: rctx.FileIO().Open, io.ReadAll, rctx.Cmd.Flags().Set, FlagErrorf.
🤖 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_test.go`:
- Around line 360-366: Update the test TestSaveResponse_RejectsAbsolutePath to
use a platform-agnostic absolute path instead of the hardcoded "/tmp/evil.txt":
detect the OS (runtime.GOOS) or use filepath utilities to construct an absolute
path appropriate for Windows (e.g. "C:\\evil.txt" style) vs Unix
("/tmp/evil.txt"), then pass that absPath into
SaveResponse(&localfileio.LocalFileIO{}, resp, absPath); keep the rest of the
test (resp from newApiResp and the err assertion) unchanged.
In `@internal/client/response.go`:
- Around line 31-34: HandleResponse currently forwards opts.FileIO into
SaveResponse without ensuring it's non-nil, which can cause panics in Save or
ResolvePath when ResolveFileIO returned nil; update HandleResponse to check if
opts.FileIO is nil and either construct/use a sensible default FileIO
implementation (an os-based fallback) before calling SaveResponse or return a
clear error stating FileIO must be provided; reference the FileIO field,
HandleResponse, SaveResponse, and ResolveFileIO/ResolvePath/Save symbols when
making the change so callers are safe when factory wiring is incomplete.
In `@internal/vfs/localfileio/atomicwrite_test.go`:
- Around line 127-145: The test currently ignores errors from concurrent
AtomicWrite calls and only checks the file is non-empty; modify the test in
atomicwrite_test.go so each goroutine captures and reports the AtomicWrite error
(e.g. send errors on a channel or store them in a slice protected by a mutex)
and assert that every AtomicWrite returned nil before proceeding; additionally,
strengthen the final verification by validating the file content (e.g. parse the
JSON or check it matches one of the expected `{"n":...}` values) to ensure
writes were not corrupted. Ensure references to AtomicWrite, the goroutine
closure, and the final ReadFile/assertion are updated accordingly.
- Around line 87-93: parentDir is computed incorrectly and the os.ReadDir error
is ignored, so the loop runs on an empty slice; change parentDir to
filepath.Dir(path) (not filepath.Dir(filepath.Dir(path))) and check the error
return from os.ReadDir(parentDir), calling t.Fatalf or t.Errorf on non-nil err
before iterating over entries to ensure a real read occurs and the test will
fail if the directory can't be inspected.
In `@shortcuts/im/helpers.go`:
- Around line 1036-1045: The uploadFileToIM function lacks a nil guard for the
file handle returned by runtime.FileIO().Open; replicate the pattern used in
uploadImageToIM by checking that f is non-nil after the Open call (and before
deferring f.Close or using f), returning an explicit error if f == nil, and only
deferring f.Close when f != nil; reference function uploadFileToIM and the
runtime.FileIO().Open / f.Close symbols when making the change.
In `@shortcuts/im/im_messages_resources_download.go`:
- Around line 155-160: The code currently classifies any HTTP error from
downloadResp as output.ErrNetwork; change it to distinguish 4xx (API/user) vs
5xx (network/server) errors: read the body as before from downloadResp.Body
(using io.LimitReader) and if status is 400–499 return output.ErrAPI with the
same formatted message and body, while keeping output.ErrNetwork for 500+
responses; ensure you still return the empty string and 0 as before and reuse
the same message format ("download failed: HTTP %d: %s") when body is present
and the shorter form when not.
In `@shortcuts/im/im_messages_send.go`:
- Around line 214-222: The validateMediaFlagPath function currently ignores
os.IsNotExist errors, allowing non-existent paths to pass; update
validateMediaFlagPath to explicitly check the result of fio.Stat(value): if Stat
returns no error, return nil; if Stat returns an os.IsNotExist error, return a
validation error indicating the file does not exist for the given flagName; for
any other Stat error (e.g., PathValidationError) return output.ErrValidation
with the error details; keep the existing early-return behavior for empty
values, http/https prefixes, and isMediaKey(value).
In `@shortcuts/im/validate_media_test.go`:
- Around line 15-19: The test currently ignores errors from os.Getwd, os.Chdir
and os.WriteFile which can mask failures; replace the manual chdir pattern with
cmdutil.TestChdir(t, dir) (or call cmdutil.TestChdir to change dirs and ensure
deferred restore) and check all error returns from os.Getwd and os.WriteFile
(use t.Fatalf or t.Fatalf on error) so the fixture creation (creating
"photo.jpg" via WriteFile) and working-directory changes fail the test
explicitly instead of causing false negatives in the "valid local file" case.
- Around line 36-37: The test uses a hard-coded "/etc/passwd" which is
Unix-only; update the "absolute path rejected" case to construct a
platform-native absolute path (e.g. use the existing temp dir or variable dir or
call filepath.Abs on a relative filename) so the test exercises the
absolute-path branch on Windows as well; modify the test table entry in
validate_media_test.go (the case with the description "absolute path rejected")
to build the absolute path via filepath.Abs(...) or by joining to dir and ensure
the expected boolean remains true.
---
Outside diff comments:
In `@cmd/service/service.go`:
- Around line 247-256: The auto-save path calls SaveResponse(opts.FileIO, ...)
but ResolveFileIO(opts.Ctx) can return nil when FileIOProvider is nil, so add a
nil-safety check before invoking SaveResponse: where the code calls
SaveResponse(opts.FileIO, ...) (auto-save path), guard with if opts.FileIO !=
nil { SaveResponse(opts.FileIO, resp, ...) } else { skip/save fallback (e.g.,
log or return nil) }; alternatively ensure ResolveFileIO never returns nil by
returning a no-op FileIO implementation from ResolveFileIO, but the simplest
change is to check opts.FileIO for nil before calling SaveResponse to avoid the
nil dereference in SaveResponse (function SaveResponse and method ResolveFileIO
are the relevant symbols).
In `@shortcuts/im/helpers.go`:
- Around line 553-576: In parseMediaDuration, guard against a nil FileIO
provider and close the opened file: first assign the result of runtime.FileIO()
to a local (e.g., fi := runtime.FileIO()), return "" immediately if fi == nil,
then use fi.Stat and fi.Open instead of calling runtime.FileIO() repeatedly;
after successfully opening f, defer f.Close() to avoid the resource leak; keep
existing error returns ("" on failure) otherwise.
- Around line 999-1008: In uploadImageToIM, runtime.FileIO() is used without a
nil check which can panic if it returns nil; obtain the FileIO once (e.g., fio
:= runtime.FileIO()), guard if fio == nil and return a clear error, then call
fio.Stat(...) and fio.Open(...) instead of runtime.FileIO(); keep the existing
defer f.Close() after successful open to ensure the file is closed.
---
Nitpick comments:
In `@internal/vfs/localfileio/localfileio_test.go`:
- Around line 52-73: The test helpers TestLocalFileIO_Open_ValidFile and
TestLocalFileIO_Stat_ValidFile currently call os.WriteFile without checking its
return; update both tests (the os.WriteFile calls in
TestLocalFileIO_Open_ValidFile and TestLocalFileIO_Stat_ValidFile) to capture
the error and fail the test on error (e.g., if err := os.WriteFile(...); err !=
nil { t.Fatalf("os.WriteFile failed: %v", err) }) so any setup failures are
reported immediately.
- Around line 16-28: The local helper function testChdir in this file duplicates
existing behavior; remove the testChdir function and replace all calls to
testChdir(t, dir) with cmdutil.TestChdir(t, dir), adding an import for the
cmdutil package; ensure any reference to testChdir (function name) is updated
and that the new cmdutil import is added to the test file's import block so
tests compile and use the canonical helper.
In `@internal/vfs/localfileio/path_test.go`:
- Around line 66-85: The test TestSafeOutputPath_ReturnsCanonicalAbsolutePath
repeats setup code for changing to a temp dir (EvalSymlinks + os.Getwd + defer
os.Chdir(origDir) + os.Chdir(dir)); refactor by extracting that sequence into a
helper (e.g., testChdir used in localfileio_test.go or reuse cmdutil.TestChdir)
and call it at the top of TestSafeOutputPath_ReturnsCanonicalAbsolutePath before
invoking SafeOutputPath so the test body only contains the WHEN/THEN assertions.
In `@shortcuts/common/runner_jq_test.go`:
- Around line 165-166: The current test helper newTestFactory constructs a
manual factory and sets IOStreams/FileIOProvider directly; change newTestFactory
to call cmdutil.TestFactory(t, config) instead and remove manual
FileIOProvider/IOStreams configuration so the shared test helper manages those
concerns. Update tests that call newTestFactory to accept a *testing.T and
ensure they call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before
creating the factory; keep references to newTestFactory and replace its
internals to return the cmdutil.TestFactory-produced factory while preserving
any config argument handling.
In `@shortcuts/common/runner.go`:
- Around line 667-679: After successfully opening the file with
rctx.FileIO().Open(path), defer f.Close() immediately (right after the Open
success) instead of calling f.Close() after io.ReadAll; remove the explicit
f.Close() call, and if you want to surface close failures, check the deferred
Close error where appropriate and return a FlagErrorf (same style as existing
errors) so file handles are always released even on early returns; references:
rctx.FileIO().Open, io.ReadAll, rctx.Cmd.Flags().Set, FlagErrorf.
🪄 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: bffcd10f-59f7-46cc-97f4-0407bedc6e58
📒 Files selected for processing (21)
cmd/api/api.gocmd/service/service.goextension/fileio/errors.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/factory_default_test.gointernal/vfs/localfileio/atomicwrite_test.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/localfileio_test.gointernal/vfs/localfileio/path_test.goshortcuts/common/runner.goshortcuts/common/runner_input_test.goshortcuts/common/runner_jq_test.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.go
There was a problem hiding this comment.
🧹 Nitpick comments (5)
internal/vfs/localfileio/localfileio_test.go (5)
114-114: Same issue: check error fromos.WriteFile.Proposed fix
- os.WriteFile("stat.txt", []byte("12345"), 0644) + if err := os.WriteFile("stat.txt", []byte("12345"), 0644); err != nil { + t.Fatalf("setup: WriteFile failed: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/localfileio_test.go` at line 114, The test currently ignores the error returned by os.WriteFile("stat.txt", []byte("12345"), 0644); update the test in localfileio_test.go to capture the returned error (e.g., err := os.WriteFile(...)) and fail the test if non-nil (use t.Fatalf/t.Fatal or your test helper like require.NoError) so file write failures are detected; ensure the error variable name matches surrounding test conventions.
27-27: Consider logging or failing on cleanup error.If the original working directory was deleted during the test,
os.Chdir(orig)will fail silently, leaving subsequent tests in an undefined working directory.Proposed fix
- t.Cleanup(func() { os.Chdir(orig) }) + t.Cleanup(func() { + if err := os.Chdir(orig); err != nil { + t.Logf("warning: failed to restore working directory: %v", err) + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/localfileio_test.go` at line 27, The cleanup currently calls os.Chdir(orig) without checking its error; update the t.Cleanup call that wraps os.Chdir(orig) to check the returned error and report it (e.g., if err := os.Chdir(orig); err != nil { t.Fatalf("failed to restore cwd: %v", err) } or t.Logf depending on desired strictness) so tests fail or log when restoring the original working directory fails.
56-58: Check error fromos.WriteFileto avoid misleading test failures.If the write fails unexpectedly, the test will fail later with a confusing "file not found" error rather than pointing to the real issue.
Proposed fix
content := []byte("hello world") - os.WriteFile("test.txt", content, 0644) + if err := os.WriteFile("test.txt", content, 0644); err != nil { + t.Fatalf("setup: WriteFile failed: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/localfileio_test.go` around lines 56 - 58, The test currently calls os.WriteFile("test.txt", content, 0644) without checking its error; update the test in localfileio_test.go to capture and check the returned error from os.WriteFile and fail the test immediately (e.g., t.Fatalf or t.Fatal) if it is non-nil so a write failure produces a clear, immediate error instead of a downstream "file not found" failure.
173-176: Check error fromos.ReadFileto catch actual failures.If
Savesilently fails to create the file, ignoring the read error will cause a confusing string comparison failure instead of a clear "file not found" message.Proposed fix
- got, _ := os.ReadFile(filepath.Join(dir, "output.bin")) + got, err := os.ReadFile(filepath.Join(dir, "output.bin")) + if err != nil { + t.Fatalf("ReadFile failed: %v", err) + } if string(got) != "saved content" { t.Errorf("file content = %q, want %q", got, "saved content") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/localfileio_test.go` around lines 173 - 176, The test currently ignores the error from os.ReadFile which hides file-not-found or read failures; update the test around the os.ReadFile call (the line using os.ReadFile(filepath.Join(dir, "output.bin")) and the got variable) to check the returned error and fail the test immediately (e.g., t.Fatalf or t.Fatalf-like assertion) when err != nil, then proceed to compare string(got) to "saved content".
190-193: Same issue: check error fromos.ReadFile.Proposed fix
- got, _ := os.ReadFile(filepath.Join(dir, "a", "b", "c.txt")) + got, err := os.ReadFile(filepath.Join(dir, "a", "b", "c.txt")) + if err != nil { + t.Fatalf("ReadFile failed: %v", err) + } if string(got) != "nested" { t.Errorf("file content = %q, want %q", got, "nested") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/localfileio_test.go` around lines 190 - 193, The test ignores the error returned by os.ReadFile; update the call to capture the error (e.g., got, err := os.ReadFile(filepath.Join(dir, "a", "b", "c.txt"))) and immediately fail the test if err != nil (use t.Fatalf or t.Fatalf("ReadFile failed: %v", err)). Keep the subsequent content check using string(got) as before (and ensure the t.Errorf/t.Fatalf prints string(got) when reporting content).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/vfs/localfileio/localfileio_test.go`:
- Line 114: The test currently ignores the error returned by
os.WriteFile("stat.txt", []byte("12345"), 0644); update the test in
localfileio_test.go to capture the returned error (e.g., err :=
os.WriteFile(...)) and fail the test if non-nil (use t.Fatalf/t.Fatal or your
test helper like require.NoError) so file write failures are detected; ensure
the error variable name matches surrounding test conventions.
- Line 27: The cleanup currently calls os.Chdir(orig) without checking its
error; update the t.Cleanup call that wraps os.Chdir(orig) to check the returned
error and report it (e.g., if err := os.Chdir(orig); err != nil {
t.Fatalf("failed to restore cwd: %v", err) } or t.Logf depending on desired
strictness) so tests fail or log when restoring the original working directory
fails.
- Around line 56-58: The test currently calls os.WriteFile("test.txt", content,
0644) without checking its error; update the test in localfileio_test.go to
capture and check the returned error from os.WriteFile and fail the test
immediately (e.g., t.Fatalf or t.Fatal) if it is non-nil so a write failure
produces a clear, immediate error instead of a downstream "file not found"
failure.
- Around line 173-176: The test currently ignores the error from os.ReadFile
which hides file-not-found or read failures; update the test around the
os.ReadFile call (the line using os.ReadFile(filepath.Join(dir, "output.bin"))
and the got variable) to check the returned error and fail the test immediately
(e.g., t.Fatalf or t.Fatalf-like assertion) when err != nil, then proceed to
compare string(got) to "saved content".
- Around line 190-193: The test ignores the error returned by os.ReadFile;
update the call to capture the error (e.g., got, err :=
os.ReadFile(filepath.Join(dir, "a", "b", "c.txt"))) and immediately fail the
test if err != nil (use t.Fatalf or t.Fatalf("ReadFile failed: %v", err)). Keep
the subsequent content check using string(got) as before (and ensure the
t.Errorf/t.Fatalf prints string(got) when reporting content).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 094bcd71-39ff-4ad7-86de-d64641041160
📒 Files selected for processing (2)
internal/cmdutil/factory_default_test.gointernal/vfs/localfileio/localfileio_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cmdutil/factory_default_test.go
YangJunzhou-01
previously approved these changes
Apr 8, 2026
Change-Id: Ibe275d3a67555e8cbfcf285c016d57d51e27b142
tuxedomm
force-pushed
the
feat/fileio-migrate-common-im
branch
from
cab596d to
8636b7a
Compare
jackie3927
approved these changes
Apr 8, 2026
liangshuo-1
approved these changes
Apr 8, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/client/response.go (1)
31-34:⚠️ Potential issue | 🟡 MinorFileIO nil dereference risk persists.
The comment states
FileIOis "required when saving files," butHandleResponsepassesopts.FileIOtosaveAndPrintandSaveResponse(lines 64, 78, 82) without any nil check. If a caller fails to initializeFileIO, the code will panic onfio.Save()orfio.ResolvePath().Consider adding a defensive nil check in
HandleResponsebefore the first use, returning a clear error if FileIO is required but missing:Suggested guard
// At the start of the file-saving paths in HandleResponse: if opts.OutputPath != "" || !IsJSONContentType(ct) { if opts.FileIO == nil { return output.ErrValidation("FileIO is required for saving files but was not provided") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/response.go` around lines 31 - 34, Handle the nil FileIO risk by adding a defensive guard in HandleResponse before any file-saving path: when opts.OutputPath != "" or when the response content-type is non-JSON (use IsJSONContentType(ct)), verify opts.FileIO is non-nil and return a clear validation error (e.g., via output.ErrValidation) if it is nil; this ensures saveAndPrint and SaveResponse (which use opts.FileIO and call fio.Save()/fio.ResolvePath()) never receive a nil FileIO.
🧹 Nitpick comments (1)
internal/client/response.go (1)
128-141: Consider simplifying redundant error case.The
WriteErrorcase (lines 136-137) anddefaultcase (lines 138-139) produce identical error messages. If this distinction isn't needed for future differentiation or logging, consider combining them:Suggested simplification
switch { case errors.Is(err, fileio.ErrPathValidation): return nil, fmt.Errorf("unsafe output path: %s", err) case errors.As(err, &me): return nil, fmt.Errorf("create directory: %s", err) - case errors.As(err, &we): - return nil, fmt.Errorf("cannot write file: %s", err) default: return nil, fmt.Errorf("cannot write file: %s", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/response.go` around lines 128 - 141, The switch in the error handling block for WriteOutput in response.go repeats the same message for fileio.WriteError and the default branch; simplify by removing the redundant case for fileio.WriteError (the errors.As check with &we) and let the default branch handle both, or merge them into a single branch that returns fmt.Errorf("cannot write file: %s", err); keep the existing errors.Is(err, fileio.ErrPathValidation) and errors.As(err, &me) branches unchanged so only the WriteError duplication is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/client/response.go`:
- Around line 31-34: Handle the nil FileIO risk by adding a defensive guard in
HandleResponse before any file-saving path: when opts.OutputPath != "" or when
the response content-type is non-JSON (use IsJSONContentType(ct)), verify
opts.FileIO is non-nil and return a clear validation error (e.g., via
output.ErrValidation) if it is nil; this ensures saveAndPrint and SaveResponse
(which use opts.FileIO and call fio.Save()/fio.ResolvePath()) never receive a
nil FileIO.
---
Nitpick comments:
In `@internal/client/response.go`:
- Around line 128-141: The switch in the error handling block for WriteOutput in
response.go repeats the same message for fileio.WriteError and the default
branch; simplify by removing the redundant case for fileio.WriteError (the
errors.As check with &we) and let the default branch handle both, or merge them
into a single branch that returns fmt.Errorf("cannot write file: %s", err); keep
the existing errors.Is(err, fileio.ErrPathValidation) and errors.As(err, &me)
branches unchanged so only the WriteError duplication is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c7d259b-514b-4df9-9ebc-64241f2597bc
📒 Files selected for processing (3)
internal/client/response.gointernal/cmdutil/factory_default_test.gointernal/vfs/localfileio/localfileio_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/cmdutil/factory_default_test.go
- internal/vfs/localfileio/localfileio_test.go
This was referenced Apr 8, 2026
This was referenced Apr 9, 2026
This was referenced Apr 10, 2026
This was referenced Apr 22, 2026
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
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
New Features
Bug Fixes
Other