feat: add FileIO extension for file transfer abstraction#314
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a FileIO abstraction and global provider registry, implements a local filesystem Provider with atomic-write and path-safety utilities, wires provider injection into Factory and RuntimeContext, and delegates previous validate/atomic-write logic to the new internal/vfs/localfileio package. (50 words) Changes
Sequence DiagramsequenceDiagram
participant Init as init
participant Registry as ProviderRegistry
participant Factory as Factory
participant Provider as LocalProvider
participant LocalIO as LocalFileIO
participant VFS as VFS
Init->>Registry: Register(provider)
Registry->>Registry: store provider (mutex)
App->>Factory: NewDefault()
Factory->>Registry: GetProvider()
Registry-->>Factory: Provider
App->>Factory: ResolveFileIO(ctx)
Factory->>Provider: ResolveFileIO(ctx)
Provider-->>Factory: LocalFileIO
App->>LocalIO: Save(path, opts, body)
LocalIO->>VFS: SafeOutputPath / MkdirAll
VFS-->>LocalIO: resolved path / mkdir ok
LocalIO->>VFS: AtomicWriteFromReader(temp -> rename)
VFS-->>LocalIO: success / bytes written
LocalIO-->>App: SaveResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
internal/vfs/localfileio/atomicwrite.go (1)
45-51: Double-close of temp file when rename fails.If the rename on line 65 fails, the deferred cleanup will call
tmp.Close()on a file that was already closed on line 62. While this is generally safe in Go (returns an error but doesn't panic), it's cleaner to track the closed state.Proposed fix to avoid double-close
success := false + closed := false defer func() { if !success { - tmp.Close() + if !closed { + tmp.Close() + } vfs.Remove(tmpName) } }() if err := tmp.Chmod(perm); err != nil { return err } if err := writeFn(tmp); err != nil { return err } if err := tmp.Sync(); err != nil { return err } if err := tmp.Close(); err != nil { return err } + closed = true if err := vfs.Rename(tmpName, path); err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/atomicwrite.go` around lines 45 - 51, The defer cleanup currently always calls tmp.Close(), which can double-close the file if it was already closed before the rename failure; add a boolean flag (e.g., closed := false) scoped with success and tmpName, set closed = true immediately after the explicit tmp.Close() call that happens before rename, and change the deferred func to only call tmp.Close() when closed == false (still call vfs.Remove(tmpName) when !success). Update references to the flag in the deferred closure so the temp file is closed exactly once while still removing the temp file on failure.internal/cmdutil/testing.go (1)
73-73:FileIOProvidermay be nil iflocalfileiois not imported by the test.
fileio.GetProvider()returnsniluntil thelocalfileiopackage'sinit()runs. Tests that useTestFactoryand rely onFileIOProviderwill receive a nil provider unless they (or their dependencies) importinternal/vfs/localfileio.Consider either:
- Adding a blank import
_ "github.com/larksuite/cli/internal/vfs/localfileio"in this file to ensure registration, or- Documenting this requirement clearly in
TestFactory's doc comment.Option 1: Register provider via blank import
import ( "bytes" "context" "net/http" "os" "testing" lark "github.com/larksuite/oapi-sdk-go/v3" larkcore "github.com/larksuite/oapi-sdk-go/v3/core" "github.com/larksuite/cli/extension/fileio" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/credential" "github.com/larksuite/cli/internal/httpmock" + _ "github.com/larksuite/cli/internal/vfs/localfileio" // register default FileIO provider )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/testing.go` at line 73, FileIOProvider is set from fileio.GetProvider() which can be nil unless the localfileio package's init runs; update TestFactory initialization to ensure the local file provider is registered by adding a blank import of internal/vfs/localfileio (e.g., `_ "github.com/larksuite/cli/internal/vfs/localfileio"`) in this package so fileio.GetProvider() returns the provider, or alternatively add a clear doc comment on TestFactory stating tests must import internal/vfs/localfileio to register the provider.
🤖 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/vfs/localfileio/localfileio.go`:
- Line 69: Replace the direct call to os.MkdirAll with the vfs wrapper: where
the code calls os.MkdirAll(filepath.Dir(safePath), 0700) (the mkdir-for-safePath
call), change it to use vfs.MkdirAll with the same directory argument and
permissions so all filesystem operations go through the VFS layer; ensure you
import/reference the vfs package and keep the permission mode (0700) unchanged.
- Line 48: Replace the direct os.Stat call with the vfs wrapper: change the
return os.Stat(safePath) invocation to use vfs.Stat(safePath) and ensure the
package imports the appropriate vfs package (add the vfs import if missing) so
all filesystem access in localfileio.go uses vfs.* per guidelines.
- Line 39: Replace the direct os.Open call with the VFS helper: change the call
site that currently does os.Open(safePath) (in the localfileio.Open
implementation using variable safePath) to vfs.Open(safePath), remove the unused
"os" import and add the appropriate vfs package import, and ensure the returned
value and error handling remain identical so callers of the Open function see no
behavioral change.
- Around line 6-13: Replace the "os" import with the vfs package in the import
block and update imports list to include "vfs" instead of "os"; ensure any
former os.* usage in localfileio.go now calls vfs.* (e.g., calls referenced in
functions like Open/Stat/Remove) so the "os" import can be removed and "vfs"
added.
In `@internal/vfs/localfileio/path.go`:
- Around line 66-69: The EvalSymlinks call's error is being ignored which can
make canonicalCwd invalid and break the containment check in isUnderDir; modify
the code around filepath.EvalSymlinks(cwd) (variable canonicalCwd) to capture
and handle its error: if EvalSymlinks returns an error, propagate a descriptive
error (including the original error) back to the caller instead of continuing,
so the subsequent isUnderDir(resolved, canonicalCwd) uses a guaranteed valid
canonicalCwd or the function returns the error.
---
Nitpick comments:
In `@internal/cmdutil/testing.go`:
- Line 73: FileIOProvider is set from fileio.GetProvider() which can be nil
unless the localfileio package's init runs; update TestFactory initialization to
ensure the local file provider is registered by adding a blank import of
internal/vfs/localfileio (e.g., `_
"github.com/larksuite/cli/internal/vfs/localfileio"`) in this package so
fileio.GetProvider() returns the provider, or alternatively add a clear doc
comment on TestFactory stating tests must import internal/vfs/localfileio to
register the provider.
In `@internal/vfs/localfileio/atomicwrite.go`:
- Around line 45-51: The defer cleanup currently always calls tmp.Close(), which
can double-close the file if it was already closed before the rename failure;
add a boolean flag (e.g., closed := false) scoped with success and tmpName, set
closed = true immediately after the explicit tmp.Close() call that happens
before rename, and change the deferred func to only call tmp.Close() when closed
== false (still call vfs.Remove(tmpName) when !success). Update references to
the flag in the deferred closure so the temp file is closed exactly once while
still removing the temp file on failure.
🪄 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: d6fe55e5-634c-422e-867e-6a03f971e8af
📒 Files selected for processing (11)
extension/fileio/registry.goextension/fileio/types.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/testing.gointernal/validate/atomicwrite.gointernal/validate/path.gointernal/vfs/localfileio/atomicwrite.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/path.goshortcuts/common/runner.go
tuxedomm
force-pushed
the
feat/fileio-extension-interface
branch
2 times, most recently
from
fa35aff to
5c27da0
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@0cdc668561ad510ffb4c010825d45bd606117ba1🧩 Skill updatenpx skills add larksuite/cli#feat/fileio-extension-interface -y -g |
Greptile SummaryThis PR introduces an Confidence Score: 5/5Safe to merge; all remaining findings are P2 suggestions that do not block the primary file I/O path. The architecture is sound and follows established patterns. The one new finding (dangling-symlink gap in ValidatePath) is P2: downstream Save/Open calls would still reject the path, so no data loss or security risk results. Previously-flagged concerns are tracked in the existing thread. shortcuts/common/runner.go (ValidatePath dangling-symlink gap) and internal/vfs/localfileio/path.go (EvalSymlinks cwd fallback, per previous thread).
|
| Filename | Overview |
|---|---|
| shortcuts/common/runner.go | Added FileIO(), ResolveSavePath(), ValidatePath() helpers to RuntimeContext; ValidatePath has a dangling-symlink gap where os.IsNotExist filtering incorrectly accepts symlink-resolution failures. |
| internal/vfs/localfileio/path.go | Path validation logic (safePath, SafeInputPath, SafeOutputPath, SafeLocalFlagPath); silently discarded EvalSymlinks error on cwd and SafeLocalFlagPath return-value inconsistency noted in previous threads. |
| internal/vfs/localfileio/atomicwrite.go | Atomic write via temp-file + rename; correct cleanup on error (defer removes temp when success=false), Chmod before write, fsync before close. |
| internal/vfs/localfileio/localfileio.go | LocalFileIO implements fileio.FileIO; correctly delegates path validation to SafeInputPath/SafeOutputPath and uses atomic writes for Save. |
| extension/fileio/types.go | New interface definitions (Provider, FileIO, FileInfo, File, SaveResult, SaveOptions); clean, minimal surface area, *os.File satisfies File without adaptation. |
| extension/fileio/registry.go | New global registry for FileIO Provider using sync.Mutex; correct last-write-wins semantics, mirrors credential registry pattern. |
| internal/cmdutil/factory.go | Added FileIOProvider field and ResolveFileIO method; nil-guards are correct, defers resolution to call time as intended. |
| internal/cmdutil/factory_default.go | Wires FileIOProvider via blank import of localfileio and fileio.GetProvider(); initialization order (Phase 0) is correct and dependency-free. |
| internal/cmdutil/testing.go | Added FileIOProvider to TestFactory and TestChdir helper; correctly notes TestChdir is not safe with t.Parallel() since os.Chdir is process-wide. |
| internal/charcheck/charcheck.go | New package centralizing dangerous-Unicode and control-char detection; clean single source of truth extracted from both validate and localfileio paths. |
| internal/validate/path.go | Thin delegation layer forwarding SafeOutputPath, SafeInputPath, SafeEnvDirPath, SafeLocalFlagPath to localfileio; preserves backward compat for existing callers. |
| internal/validate/sanitize.go | SanitizeForTerminal strips ANSI escapes, C0 control chars, and dangerous Unicode; regex covers both CSI and OSC sequences correctly. |
Sequence Diagram
sequenceDiagram
participant CMD as Command
participant RC as RuntimeContext
participant F as Factory
participant REG as fileio.Registry
participant LFI as LocalFileIO
participant VFS as vfs (OS)
Note over CMD,VFS: Initialization (factory_default.go)
REG->>REG: init() registers &Provider{}
F->>REG: GetProvider()
REG-->>F: *localfileio.Provider
F->>F: FileIOProvider = provider
Note over CMD,VFS: Runtime execution (runner.go)
CMD->>RC: FileIO()
RC->>F: ResolveFileIO(ctx)
F->>LFI: Provider.ResolveFileIO(ctx)
LFI-->>RC: &LocalFileIO{}
CMD->>RC: ResolveSavePath(path)
RC->>LFI: ResolvePath(path)
LFI->>LFI: SafeOutputPath(path)
LFI-->>RC: canonicalAbsPath
CMD->>RC: FileIO().Save(path, opts, body)
RC->>LFI: Save(path, opts, body)
LFI->>LFI: SafeOutputPath(path)
LFI->>VFS: MkdirAll(dir, 0700)
LFI->>VFS: CreateTemp → Chmod → Write → Sync → Close → Rename
LFI-->>RC: SaveResult{size}
Reviews (5): Last reviewed commit: "fix: address PR #314 review issues" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/vfs/localfileio/path.go (1)
57-66: Clarify handling of empty input and cross-platform path behavior insafePath.
filepath.Clean("")collapses to"."(current directory), andfilepath.IsAbsis host-OS dependent. On Unix,filepath.IsAbs("C:/foo")andfilepath.IsAbs(C:\foo)return false, so Windows-style paths are not caught by the absolute-path guard. While this is semantically correct on Unix (those strings are relative paths in Unix semantics), rejecting empty input explicitly beforefilepath.Cleanmay be warranted if the intent is to prevent silent resolution to the current directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/path.go` around lines 57 - 66, The safePath function should explicitly reject empty input before calling filepath.Clean to avoid silently accepting "" (which Clean turns into "."); add a check at the start of safePath (after rejectControlChars) that returns an error if raw == "" (or strings.TrimSpace(raw) == "" if you want to treat all-whitespace as empty) with a clear message referencing the flagName; leave the existing filepath.Clean and filepath.IsAbs logic as-is so platform-specific absolute-path semantics (e.g., Windows-style paths on Unix) remain unchanged.
🤖 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.go`:
- Line 21: The file has formatting drift; run gofmt on
internal/cmdutil/factory_default.go to fix whitespace/formatting at the import
line that registers the default FileIO provider (the blank import _
"github.com/larksuite/cli/internal/vfs/localfileio"), then stage the updated
file so CI passes; ensure the file is saved after running `gofmt -w` (or
equivalent) so the import line and surrounding formatting match `gofmt`
expectations.
In `@internal/vfs/localfileio/atomicwrite.go`:
- Around line 37-65: atomicWrite currently calls tmp.Chmod, tmp.Sync, and
tmp.Close directly on the *os.File returned by vfs.CreateTemp, bypassing the vfs
abstraction; update the vfs API to expose a file handle abstraction (e.g., an
interface with Chmod, Sync, Close, Name methods) or add vfs.Chmod/Sync/Close
helpers, change vfs.CreateTemp to return that vfs file type, and then modify
atomicWrite to call the vfs file methods (or vfs helpers) instead of
tmp.Chmod/tmp.Sync/tmp.Close while keeping existing logic around vfs.CreateTemp,
vfs.Remove and vfs.Rename and preserving the tmpName/success defer behavior.
- Around line 59-66: The rename currently flushes the temp file (tmp.Sync,
tmp.Close) and calls vfs.Rename(tmpName, path) but omits syncing the containing
directory; after calling vfs.Rename(tmpName, path) in atomic write
(atomicwrite.go) add a call to syncDir(filepath.Dir(path)) and return any error
so the directory entry for the rename is fsynced and durable; ensure you
reference the existing tmp.Sync/ tmp.Close and vfs.Rename calls and handle
errors from syncDir the same way as other errors in this function.
In `@internal/vfs/localfileio/path.go`:
- Around line 68-86: Add an EvalSymlinks(ctx string) (string, error) method to
the internal/vfs.FS interface and implement it on OsFs, then expose a
package-level wrapper function vfs.EvalSymlinks(path string) that forwards to
vfs.DefaultFS.EvalSymlinks; update internal/vfs/localfileio/path.go to call
vfs.EvalSymlinks instead of filepath.EvalSymlinks (both where symlinks are
resolved and for canonicalCwd) and update
internal/vfs/localfileio/localfileio.go to use vfs.Open and vfs.Stat instead of
os.Open/os.Stat so all symlink resolution and file ops go through the vfs
abstraction and tests can swap DefaultFS.
---
Nitpick comments:
In `@internal/vfs/localfileio/path.go`:
- Around line 57-66: The safePath function should explicitly reject empty input
before calling filepath.Clean to avoid silently accepting "" (which Clean turns
into "."); add a check at the start of safePath (after rejectControlChars) that
returns an error if raw == "" (or strings.TrimSpace(raw) == "" if you want to
treat all-whitespace as empty) with a clear message referencing the flagName;
leave the existing filepath.Clean and filepath.IsAbs logic as-is so
platform-specific absolute-path semantics (e.g., Windows-style paths on Unix)
remain unchanged.
🪄 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: 10a762e4-a8d9-4466-95e0-2316a47f2b6d
📒 Files selected for processing (11)
extension/fileio/registry.goextension/fileio/types.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/testing.gointernal/validate/atomicwrite.gointernal/validate/path.gointernal/vfs/localfileio/atomicwrite.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/path.goshortcuts/common/runner.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/validate/atomicwrite.go
- shortcuts/common/runner.go
- extension/fileio/registry.go
- extension/fileio/types.go
- internal/vfs/localfileio/localfileio.go
tuxedomm
force-pushed
the
feat/fileio-extension-interface
branch
3 times, most recently
from
0e11fe3 to
09573d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/vfs/localfileio/path.go (1)
74-86:⚠️ Potential issue | 🟠 MajorKeep symlink resolution inside
vfs, and fail whencwdcan't be canonicalized.Lines 75 and 99 still use
filepath.EvalSymlinks, so a swappedvfs.DefaultFSwill not cover the full validator. Line 86 also ignores the error, which makes the containment check proceed with an invalidcanonicalCwdinstead of returning the real failure.As per coding guidelines: "Use
vfs.*instead ofos.*for all filesystem access".Also applies to: 98-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/path.go` around lines 74 - 86, Replace direct calls to filepath.EvalSymlinks with the VFS equivalent and propagate errors: when resolving `resolved` use `vfs.EvalSymlinks` (or the VFS API used elsewhere) instead of filepath.EvalSymlinks and keep the existing error handling around `resolveNearestAncestor`; for the working directory canonicalization replace `filepath.EvalSymlinks(cwd)` with the VFS canonicalization call and do not ignore its error—return a wrapped error (e.g., "cannot canonicalize cwd: %w") if it fails; apply the same change to the other occurrence around the containment check (the block referencing canonicalCwd at lines ~98-102) so all symlink and cwd resolution uses `vfs.*` and errors are surfaced (references: `vfs.Lstat`, `resolveNearestAncestor`, and the canonicalization of `cwd`).internal/vfs/localfileio/atomicwrite.go (2)
53-63: 🛠️ Refactor suggestion | 🟠 MajorKeep temp-file handle operations behind
vfs.This helper already routes create/remove/rename through
vfs, but Lines 53-63 still callChmod,Sync, andCloseon the concrete*os.File. That leaves the abstraction half-open and makes the atomic writer impossible to swap end-to-end in tests.As per coding guidelines: "Use
vfs.*instead ofos.*for all filesystem access".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/atomicwrite.go` around lines 53 - 63, Replace direct method calls on the concrete tmp *os.File (tmp.Chmod, tmp.Sync, tmp.Close) with the VFS abstraction equivalents so all file-handle operations go through vfs; specifically, after writeFn(tmp) succeeds call vfs.Chmod using the temp file's name (instead of tmp.Chmod), call the vfs-provided sync method for the temp file (instead of tmp.Sync), and close the temp file via vfs.Close (instead of tmp.Close) so the atomic writer uses vfs for Chmod/Sync/Close just like create/remove/rename already do.
59-66:⚠️ Potential issue | 🟠 MajorSync the containing directory after the rename.
tmp.Sync()only flushes file contents. If this atomic-write helper is expected to survive a crash or power loss, the parent directory also needs an fsync after Line 65; otherwise the rename entry itself can still be lost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/atomicwrite.go` around lines 59 - 66, The atomic write currently fsyncs the temp file (tmp.Sync) and closes it, then renames (vfs.Rename) but does not fsync the parent directory; after calling vfs.Rename(tmpName, path) in the atomic write helper, open the parent directory (use filepath.Dir(path)), call Sync on that directory file and close it, and return any error from that Sync so the rename metadata is persisted; reference tmp.Sync, tmp.Close, vfs.Rename and the atomic write function when making the change.
🤖 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/testing.go`:
- Around line 95-105: The TestChdir helper currently mixes vfs.Getwd() with
os.Chdir and ignores restore errors; change it to use vfs for changing
directories (replace os.Chdir(dir) with vfs.Chdir(dir)) so tests using the vfs
abstraction see a consistent cwd, and in the t.Cleanup restore callback call
vfs.Chdir(orig) and check its error (calling t.Fatalf or t.Errorf) instead of
discarding it to ensure failed restores are surfaced; keep vfs.Getwd() as the
original retrieval and retain t.Helper().
In `@internal/vfs/localfileio/path.go`:
- Around line 26-33: SafeLocalFlagPath currently validates via SafeInputPath
which hardcodes the caller flag as "--file", causing errors like "--cover:
--file ..."; change the validation to call safePath(value, flagName) (or the
internal helper that accepts the caller flag) instead of SafeInputPath, discard
the resolved path returned, and propagate the error so the message contains the
original flagName; update SafeLocalFlagPath to call safePath(value, flagName)
and return fmt.Errorf("%s: %v", flagName, err) on error.
---
Duplicate comments:
In `@internal/vfs/localfileio/atomicwrite.go`:
- Around line 53-63: Replace direct method calls on the concrete tmp *os.File
(tmp.Chmod, tmp.Sync, tmp.Close) with the VFS abstraction equivalents so all
file-handle operations go through vfs; specifically, after writeFn(tmp) succeeds
call vfs.Chmod using the temp file's name (instead of tmp.Chmod), call the
vfs-provided sync method for the temp file (instead of tmp.Sync), and close the
temp file via vfs.Close (instead of tmp.Close) so the atomic writer uses vfs for
Chmod/Sync/Close just like create/remove/rename already do.
- Around line 59-66: The atomic write currently fsyncs the temp file (tmp.Sync)
and closes it, then renames (vfs.Rename) but does not fsync the parent
directory; after calling vfs.Rename(tmpName, path) in the atomic write helper,
open the parent directory (use filepath.Dir(path)), call Sync on that directory
file and close it, and return any error from that Sync so the rename metadata is
persisted; reference tmp.Sync, tmp.Close, vfs.Rename and the atomic write
function when making the change.
In `@internal/vfs/localfileio/path.go`:
- Around line 74-86: Replace direct calls to filepath.EvalSymlinks with the VFS
equivalent and propagate errors: when resolving `resolved` use
`vfs.EvalSymlinks` (or the VFS API used elsewhere) instead of
filepath.EvalSymlinks and keep the existing error handling around
`resolveNearestAncestor`; for the working directory canonicalization replace
`filepath.EvalSymlinks(cwd)` with the VFS canonicalization call and do not
ignore its error—return a wrapped error (e.g., "cannot canonicalize cwd: %w") if
it fails; apply the same change to the other occurrence around the containment
check (the block referencing canonicalCwd at lines ~98-102) so all symlink and
cwd resolution uses `vfs.*` and errors are surfaced (references: `vfs.Lstat`,
`resolveNearestAncestor`, and the canonicalization of `cwd`).
🪄 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: 5eba3364-9a3f-47a1-b5e8-5ac08e5543eb
📒 Files selected for processing (11)
extension/fileio/registry.goextension/fileio/types.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/testing.gointernal/validate/atomicwrite.gointernal/validate/path.gointernal/vfs/localfileio/atomicwrite.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/path.goshortcuts/common/runner.go
✅ Files skipped from review due to trivial changes (1)
- extension/fileio/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- extension/fileio/registry.go
- internal/cmdutil/factory_default.go
- internal/validate/atomicwrite.go
- shortcuts/common/runner.go
There was a problem hiding this comment.
♻️ Duplicate comments (5)
internal/vfs/localfileio/path.go (3)
30-31:⚠️ Potential issue | 🟡 MinorPreserve the caller’s flag name in
SafeLocalFlagPatherrors.
SafeLocalFlagPathstill validates throughSafeInputPath, so invalid non---fileflags can surface nested--filemessaging. Validate withsafePath(value, flagName)directly.Proposed fix
func SafeLocalFlagPath(flagName, value string) (string, error) { if value == "" || strings.HasPrefix(value, "http://") || strings.HasPrefix(value, "https://") { return value, nil } - if _, err := SafeInputPath(value); err != nil { - return "", fmt.Errorf("%s: %v", flagName, err) + if _, err := safePath(value, flagName); err != nil { + return "", err } return value, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/path.go` around lines 30 - 31, SafeLocalFlagPath currently calls SafeInputPath which can emit generic "--file" messaging; change SafeLocalFlagPath to validate via safePath(value, flagName) instead of SafeInputPath so the caller's flagName is preserved in error messages. Locate the SafeLocalFlagPath function and replace the SafeInputPath-based validation with a direct call to safePath(value, flagName), returning any error wrapped or forwarded exactly as before to keep flagName context; keep SafeInputPath usage elsewhere unchanged.
86-86:⚠️ Potential issue | 🟡 MinorHandle
EvalSymlinks(cwd)errors before the containment check.Line 86 drops the error from
filepath.EvalSymlinks(cwd). If cwd resolution fails, the subsequentisUnderDirdecision can be made on invalid data.Proposed fix
- canonicalCwd, _ := filepath.EvalSymlinks(cwd) + canonicalCwd, err := filepath.EvalSymlinks(cwd) + if err != nil { + return "", fmt.Errorf("cannot resolve working directory: %w", err) + } if !isUnderDir(resolved, canonicalCwd) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/path.go` at line 86, The code drops the error from filepath.EvalSymlinks(cwd) when assigning canonicalCwd; instead, check and handle that error before using canonicalCwd in the containment check (isUnderDir). Modify the logic around the EvalSymlinks call so that if err != nil you return or propagate the error (or handle it per function contract) rather than proceeding, and only call isUnderDir(canonicalCwd, ...) after successful resolution; update any callers of the enclosing function to handle the propagated error if needed.
75-76:⚠️ Potential issue | 🟠 MajorRoute symlink resolution through
vfsconsistently.
safePath/resolveNearestAncestorusefilepath.EvalSymlinksdirectly, which bypasses the filesystem abstraction used elsewhere in this package.#!/bin/bash # Verify direct symlink-resolution calls in localfileio. rg -n --type=go 'filepath\.EvalSymlinks|vfs\.EvalSymlinks' internal/vfs/localfileio/path.go internal/vfs # Inspect FS abstraction surface for EvalSymlinks support. rg -n --type=go 'type FS interface|func EvalSymlinks|EvalSymlinks\(' internal/vfs -A15 -B5As per coding guidelines: "Use
vfs.*instead ofos.*for all filesystem access".Also applies to: 86-86, 99-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/path.go` around lines 75 - 76, safePath and resolveNearestAncestor call filepath.EvalSymlinks directly, bypassing the package's filesystem abstraction; replace those calls with the VFS wrapper (use vfs.EvalSymlinks or the receiver's fs.EvalSymlinks where appropriate) so symlink resolution goes through the localfileio vfs layer, update imports to include the vfs package if needed, and keep existing error handling/semantics unchanged.internal/vfs/localfileio/atomicwrite.go (2)
65-67:⚠️ Potential issue | 🟠 MajorSync the parent directory after
Renameto make the rename durable.The file data is synced, but the directory entry update from
Renameis not explicitly flushed. A crash immediately after rename can still lose the new name on some filesystems.Proposed change
if err := vfs.Rename(tmpName, path); err != nil { return err } + if err := syncDir(filepath.Dir(path)); err != nil { + return err + } success = true return nil#!/bin/bash # Verify absence of directory fsync step after rename. rg -n --type=go 'vfs\.Rename|syncDir|\.Sync\(' internal/vfs/localfileio/atomicwrite.go -A5 -B5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/atomicwrite.go` around lines 65 - 67, After calling vfs.Rename(tmpName, path) ensure the parent directory is fsynced so the directory entry is durable: locate the atomic commit code that calls vfs.Rename(tmpName, path) and immediately after a successful rename open the parent directory (using the directory of path), call Sync() on that opened directory handle and close it (or call an existing helper like syncDir if available), and return any Sync error; this guarantees the rename's directory entry is flushed to disk.
37-63:⚠️ Potential issue | 🟠 MajorMove temp-file handle operations behind the
vfsabstraction.
tmp.Chmod,tmp.Sync, andtmp.Closeare invoked directly on*os.File, so this flow still bypassesvfsfor part of filesystem I/O.#!/bin/bash # Verify direct temp-file method usage in atomicwrite. rg -n --type=go 'tmp\.(Chmod|Sync|Close)\(' internal/vfs/localfileio/atomicwrite.go # Inspect current vfs abstraction surface related to file operations. rg -n --type=go 'type FS interface|CreateTemp|Open|Close|Sync|Chmod' internal/vfs -A20 -B5As per coding guidelines: "Use
vfs.*instead ofos.*for all filesystem access".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/atomicwrite.go` around lines 37 - 63, The atomicWrite function currently calls tmp.Chmod, tmp.Sync and tmp.Close directly on *os.File (tmp) which bypasses the vfs layer; change these to use the vfs abstraction instead (e.g., call vfs.Chmod(tmpName, perm) or the vfs-provided file interface methods such as vfs.Sync(tmp) and vfs.Close(tmp) depending on the vfs API) and ensure any error handling and the defer cleanup logic still uses vfs.Remove(tmpName) and the vfs-close variant; update references in atomicWrite and keep using vfs.CreateTemp and vfs.Remove as before so no direct os.* file operations remain.
🤖 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/vfs/localfileio/atomicwrite.go`:
- Around line 65-67: After calling vfs.Rename(tmpName, path) ensure the parent
directory is fsynced so the directory entry is durable: locate the atomic commit
code that calls vfs.Rename(tmpName, path) and immediately after a successful
rename open the parent directory (using the directory of path), call Sync() on
that opened directory handle and close it (or call an existing helper like
syncDir if available), and return any Sync error; this guarantees the rename's
directory entry is flushed to disk.
- Around line 37-63: The atomicWrite function currently calls tmp.Chmod,
tmp.Sync and tmp.Close directly on *os.File (tmp) which bypasses the vfs layer;
change these to use the vfs abstraction instead (e.g., call vfs.Chmod(tmpName,
perm) or the vfs-provided file interface methods such as vfs.Sync(tmp) and
vfs.Close(tmp) depending on the vfs API) and ensure any error handling and the
defer cleanup logic still uses vfs.Remove(tmpName) and the vfs-close variant;
update references in atomicWrite and keep using vfs.CreateTemp and vfs.Remove as
before so no direct os.* file operations remain.
In `@internal/vfs/localfileio/path.go`:
- Around line 30-31: SafeLocalFlagPath currently calls SafeInputPath which can
emit generic "--file" messaging; change SafeLocalFlagPath to validate via
safePath(value, flagName) instead of SafeInputPath so the caller's flagName is
preserved in error messages. Locate the SafeLocalFlagPath function and replace
the SafeInputPath-based validation with a direct call to safePath(value,
flagName), returning any error wrapped or forwarded exactly as before to keep
flagName context; keep SafeInputPath usage elsewhere unchanged.
- Line 86: The code drops the error from filepath.EvalSymlinks(cwd) when
assigning canonicalCwd; instead, check and handle that error before using
canonicalCwd in the containment check (isUnderDir). Modify the logic around the
EvalSymlinks call so that if err != nil you return or propagate the error (or
handle it per function contract) rather than proceeding, and only call
isUnderDir(canonicalCwd, ...) after successful resolution; update any callers of
the enclosing function to handle the propagated error if needed.
- Around line 75-76: safePath and resolveNearestAncestor call
filepath.EvalSymlinks directly, bypassing the package's filesystem abstraction;
replace those calls with the VFS wrapper (use vfs.EvalSymlinks or the receiver's
fs.EvalSymlinks where appropriate) so symlink resolution goes through the
localfileio vfs layer, update imports to include the vfs package if needed, and
keep existing error handling/semantics unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 562814f2-cf7d-4c45-b000-034f7b28a2b0
📒 Files selected for processing (11)
extension/fileio/registry.goextension/fileio/types.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/testing.gointernal/validate/atomicwrite.gointernal/validate/path.gointernal/vfs/localfileio/atomicwrite.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/path.goshortcuts/common/runner.go
✅ Files skipped from review due to trivial changes (2)
- extension/fileio/registry.go
- internal/validate/path.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/cmdutil/factory_default.go
- internal/cmdutil/testing.go
- internal/cmdutil/factory.go
- internal/validate/atomicwrite.go
- shortcuts/common/runner.go
tuxedomm
force-pushed
the
feat/fileio-extension-interface
branch
from
09573d4 to
e2648fa
Compare
|
|
Introduce extension/fileio package with Provider/FileIO/File interfaces and a global registry, following the same pattern as extension/credential. - Add LocalFileIO default implementation with path validation and atomic writes - Wire FileIOProvider into Factory and resolve at runtime via RuntimeContext.FileIO() - Factory holds Provider (not resolved instance), deferring resolution to execution time Change-Id: I6404ac68cbd468ada426db12e2d8dcef90fd9d93
tuxedomm
force-pushed
the
feat/fileio-extension-interface
branch
from
e2648fa to
e21c5ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/vfs/localfileio/path.go (3)
86-86:⚠️ Potential issue | 🟡 MinorDon't continue with an unresolved
canonicalCwd.Line 86 still ignores the canonicalization error. If
cwdcannot be resolved,isUnderDiris comparing against an invalid parent and can make the wrong containment decision.Proposed fix
- canonicalCwd, _ := filepath.EvalSymlinks(cwd) + canonicalCwd, err := filepath.EvalSymlinks(cwd) + if err != nil { + return "", fmt.Errorf("cannot resolve working directory: %w", err) + } if !isUnderDir(resolved, canonicalCwd) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/path.go` at line 86, The code currently ignores errors from filepath.EvalSymlinks when computing canonicalCwd; update path canonicalization around canonicalCwd (result of filepath.EvalSymlinks) to check the returned error and handle it instead of continuing — e.g., if EvalSymlinks returns an error return that error (or wrap it) so isUnderDir uses a valid canonical parent; locate the call to filepath.EvalSymlinks and the isUnderDir logic and add an error check that prevents proceeding with an unresolved canonicalCwd.
74-75: 🛠️ Refactor suggestion | 🟠 MajorResolve symlinks through
vfs, notfilepath.
internal/vfs/default.go:1-30makesvfs.DefaultFSswappable, but these calls always hit the host filesystem. That letsGetwd/Lstatobserve one FS while symlink resolution observes another, which breaks end-to-end coverage forLocalFileIOand any non-defaultvfs.FSimplementation. AddEvalSymlinkstointernal/vfs.FSand use avfs.EvalSymlinkswrapper here.Also applies to: 86-86, 98-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/path.go` around lines 74 - 75, The code calls filepath.EvalSymlinks directly after vfs.Lstat (e.g., in the LocalFileIO path resolution logic), causing symlink resolution to bypass the swappable vfs implementation; add an EvalSymlinks method to the internal/vfs.FS interface and implement a vfs.EvalSymlinks wrapper, then replace uses of filepath.EvalSymlinks (the calls following vfs.Lstat in functions handling path resolution and Getwd-related logic) with vfs.EvalSymlinks so symlink resolution goes through the configured FS consistently.
26-33:⚠️ Potential issue | 🟡 MinorPreserve the caller's flag name in the validation error.
This still validates through
SafeInputPath, so an invalid--covervalue will surface as--cover: --file .... CallsafePath(value, flagName)directly and discard the resolved path instead.Proposed fix
func SafeLocalFlagPath(flagName, value string) (string, error) { if value == "" || strings.HasPrefix(value, "http://") || strings.HasPrefix(value, "https://") { return value, nil } - if _, err := SafeInputPath(value); err != nil { - return "", fmt.Errorf("%s: %v", flagName, err) + if _, err := safePath(value, flagName); err != nil { + return "", err } return value, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/localfileio/path.go` around lines 26 - 33, SafeLocalFlagPath currently validates via SafeInputPath which wraps errors without the original flag name; instead call safePath(value, flagName) directly inside SafeLocalFlagPath (keeping the empty and http(s) early-return behavior), discard the resolved path it returns and return any error from safePath so the error message preserves the caller's flagName; ensure you still return value,nil on success. Use function names SafeLocalFlagPath and safePath to locate the change.
🤖 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/vfs/localfileio/path.go`:
- Around line 72-91: The current safePath-style string validation (using
resolveNearestAncestor, isUnderDir and returning a pathname) has a TOCTOU
symlink race; instead, stop returning a string and perform the sensitive
open/write while holding a trusted directory file descriptor: add/replace with a
function like OpenSafePath/OpenOrCreateSafe that walks path components using
fd-based syscalls (openat) from a cwd dirfd, uses O_NOFOLLOW (and O_DIRECTORY
for intermediate components), and only performs create/open of the final target
via openat on the verified ancestor fd; reuse resolveNearestAncestor only to
find the nearest existing ancestor to obtain its dirfd, then use
openat/unix.Openat with appropriate flags to prevent following symlinks for
intermediates and the final open/create. Ensure errors are wrapped with the same
messages where applicable and remove the validate-then-use string return to
eliminate the race.
---
Duplicate comments:
In `@internal/vfs/localfileio/path.go`:
- Line 86: The code currently ignores errors from filepath.EvalSymlinks when
computing canonicalCwd; update path canonicalization around canonicalCwd (result
of filepath.EvalSymlinks) to check the returned error and handle it instead of
continuing — e.g., if EvalSymlinks returns an error return that error (or wrap
it) so isUnderDir uses a valid canonical parent; locate the call to
filepath.EvalSymlinks and the isUnderDir logic and add an error check that
prevents proceeding with an unresolved canonicalCwd.
- Around line 74-75: The code calls filepath.EvalSymlinks directly after
vfs.Lstat (e.g., in the LocalFileIO path resolution logic), causing symlink
resolution to bypass the swappable vfs implementation; add an EvalSymlinks
method to the internal/vfs.FS interface and implement a vfs.EvalSymlinks
wrapper, then replace uses of filepath.EvalSymlinks (the calls following
vfs.Lstat in functions handling path resolution and Getwd-related logic) with
vfs.EvalSymlinks so symlink resolution goes through the configured FS
consistently.
- Around line 26-33: SafeLocalFlagPath currently validates via SafeInputPath
which wraps errors without the original flag name; instead call safePath(value,
flagName) directly inside SafeLocalFlagPath (keeping the empty and http(s)
early-return behavior), discard the resolved path it returns and return any
error from safePath so the error message preserves the caller's flagName; ensure
you still return value,nil on success. Use function names SafeLocalFlagPath and
safePath to locate the change.
🪄 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: 921a4990-0d95-4054-b905-a899a467beb0
📒 Files selected for processing (11)
extension/fileio/registry.goextension/fileio/types.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/testing.gointernal/validate/atomicwrite.gointernal/validate/path.gointernal/vfs/localfileio/atomicwrite.gointernal/vfs/localfileio/localfileio.gointernal/vfs/localfileio/path.goshortcuts/common/runner.go
✅ Files skipped from review due to trivial changes (2)
- extension/fileio/types.go
- internal/validate/atomicwrite.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/cmdutil/factory_default.go
- extension/fileio/registry.go
- internal/cmdutil/testing.go
- internal/validate/path.go
- internal/vfs/localfileio/atomicwrite.go
- internal/cmdutil/factory.go
- internal/vfs/localfileio/localfileio.go
- shortcuts/common/runner.go
- ResolveSavePath returns (string, error) instead of silently falling back to unvalidated path - Extract isDangerousUnicode/RejectControlChars into internal/charcheck to eliminate security-logic duplication between localfileio and validate - Fix atomicWrite double-close by adding closed flag - Clarify ValidatePath doc: input-path only, use ResolveSavePath for output - Document FileIO registry override-vs-chain design in registry.go Change-Id: I1eae9459218fd1c590f4d58d6d331c2d99d81a39
liangshuo-1
approved these changes
Apr 8, 2026
2 tasks
This was referenced Apr 8, 2026
This was referenced Apr 9, 2026
This was referenced Apr 10, 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
3 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.
Introduce extension/fileio package with Provider/FileIO/File interfaces and a global registry, following the same pattern as extension/credential.
Change-Id: I6404ac68cbd468ada426db12e2d8dcef90fd9d93
Summary
Changes
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit