feat(cmdutil): support @file for --params/--data (issue #705)#715
feat(cmdutil): support @file for --params/--data (issue #705)#715liangshuo-1 wants to merge 2 commits intomainfrom
Conversation
Inline JSON values for --params/--data are mangled by Windows PowerShell 5's CommandLineToArgvW. Stdin (-) was the only escape hatch but supports just one flag at a time. Extend ResolveInput to accept @<path> (read JSON from a file) and @@... (escape for a literal @-prefixed value), mirroring the shortcuts framework's resolveInputFlags semantics. With this, both --params and --data can be sourced from files in the same call, sidestepping shell quoting on every platform. - internal/cmdutil/resolve.go: add @path / @@ handling, trim file content like stdin does, error on empty path or empty file - internal/cmdutil/resolve_test.go: cover file read, whitespace trim, missing file, empty path, empty content, @@ escape, plus ParseJSONMap / ParseOptionalBody integration through @file - cmd/api/api.go, cmd/service/service.go: update --params/--data help text to mention @file Change-Id: I366aa0f5783fbec6f05403f7f542505098a98c82
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/resolve.go`:
- Around line 54-57: Replace the direct os.ReadFile usage with input validation
and the vfs abstraction: call validate.SafeInputPath(path) and return its error
if any, then use vfs.ReadFile (or the repository's vfs equivalent) instead of
os.ReadFile to load the file into data, preserving the existing error wrapping
(e.g., fmt.Errorf("cannot read file %q: %w", path, err)). Update the code around
the existing data, err := os.ReadFile(path) site in resolve.go to perform these
two steps (validate.SafeInputPath and vfs.ReadFile) using the same path variable
and error-handling style.
🪄 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: 75b90dee-97b2-44f1-b10d-df9b19605d63
📒 Files selected for processing (4)
cmd/api/api.gocmd/service/service.gointernal/cmdutil/resolve.gointernal/cmdutil/resolve_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #715 +/- ##
==========================================
+ Coverage 64.14% 64.15% +0.01%
==========================================
Files 504 504
Lines 44285 44308 +23
==========================================
+ Hits 28406 28427 +21
- Misses 13411 13412 +1
- Partials 2468 2469 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@60e19d76b3f2b71f97951697efa1fd809c076583🧩 Skill updatenpx skills add larksuite/cli#agent/claude/ed72ff9f -y -g |
The first cut of @file support called os.ReadFile directly inside ResolveInput, bypassing the codebase's fileio.FileIO abstraction (SafeInputPath validation, pluggable provider). That diverged from how every other file-reading path works: BuildFormdata for --file uploads and the shortcuts framework's resolveInputFlags both go through fileio.FileIO.Open with explicit fileio.ErrPathValidation handling. Re-route @file through the same path: - ResolveInput, ParseJSONMap, ParseOptionalBody now take a fileio.FileIO; @path uses fileIO.Open which goes through SafeInputPath (control-char rejection, abs-path rejection, symlink-escape check) — same security posture as --file - cmd/api and cmd/service callsites pass Factory.ResolveFileIO(ctx); the upload path now reuses the resolved fileIO instead of resolving twice - Path-validation errors surface as `--params: invalid file path "...": ...` distinct from `--params: cannot read file "...": ...` for genuine I/O errors - Nil fileIO with an @path returns a clear "file input (@path) is not available" error - Tests use localfileio.LocalFileIO with TestChdir(t, dir), matching the existing fileupload_test.go pattern; absolute-path rejection and nil-fileIO are covered This makes the feature behave identically under any FileIO provider (including server mode) instead of being silently bound to the local filesystem. Change-Id: I878c4e8fb03f43f1f19afad75ec3af9cdab7a7f9
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cmdutil/json_test.go (1)
26-26: Add one parser-level case with non-nilfileIOto lock in@filewiring.These updates keep compilation green, but all current cases still pass
nilforfileIO. Adding at least one@filecase throughParseOptionalBody/ParseJSONMapwould protect this new wiring from regression.As per coding guidelines
**/*.go: Every behavior change must have an accompanying test.Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/json_test.go` at line 26, Add a parser-level test case that passes a non-nil fileIO into ParseOptionalBody (and exercises ParseJSONMap via an `@file` input) so the `@file` wiring is exercised; locate the test harness around the call to ParseOptionalBody in json_test.go (the table-driven cases feeding into got, err := ParseOptionalBody(...)) and add one case where method/data include an "@file" reference and the fileIO argument is a non-nil implementation (e.g., a small in-memory/mock that returns expected file contents), then assert the parsed result matches the expected map and no error is returned.
🤖 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/cmdutil/json_test.go`:
- Line 26: Add a parser-level test case that passes a non-nil fileIO into
ParseOptionalBody (and exercises ParseJSONMap via an `@file` input) so the `@file`
wiring is exercised; locate the test harness around the call to
ParseOptionalBody in json_test.go (the table-driven cases feeding into got, err
:= ParseOptionalBody(...)) and add one case where method/data include an "@file"
reference and the fileIO argument is a non-nil implementation (e.g., a small
in-memory/mock that returns expected file contents), then assert the parsed
result matches the expected map and no error is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 794a85d3-a043-463d-8634-268f20492668
📒 Files selected for processing (6)
cmd/api/api.gocmd/service/service.gointernal/cmdutil/json.gointernal/cmdutil/json_test.gointernal/cmdutil/resolve.gointernal/cmdutil/resolve_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/api/api.go
- cmd/service/service.go
- internal/cmdutil/resolve_test.go
- internal/cmdutil/resolve.go
Summary
cmdutil.ResolveInputwith@<path>(read JSON from a file) and@@...(escape for a literal@-prefixed value), mirroring the shortcuts framework'sresolveInputFlagssemantics.--paramsand--datanow support@fileeverywhere they're used (lark-cli api ...viacmd/api, declarative service commands viacmd/service).@file.@@escape, and integration throughParseJSONMap/ParseOptionalBody.Fixes the Windows PowerShell 5
CommandLineToArgvWquote-mangling bug reported in #705 — users can now write JSON to a file and pass--params @params.jsonon any shell, no.batworkaround needed. Both flags can come from files in a single invocation, which stdin (-) couldn't support.Behaviour
-@<path>@@foo@→@foo, no file read (new)'…'Backward compatibility
The only behaviour change is
@…values are now resolved as files instead of being passed through as-is. This was previously undocumented; the@@escape covers anyone relying on a literal@-prefixed JSON value.Test plan
go test ./internal/cmdutil/...— all green, including new@filecasesgo test ./cmd/api/... ./cmd/service/...— greengo build ./...— greenlark-cli api GET /open-apis/drive/v1/files --params @params.json --dry-runresolves the file and serialises the params correctly--params: cannot read file "...": no such file or directory(supports - for stdin, @file for file input)Closes #705
Summary by CodeRabbit
New Features
Documentation