feat(sheets): guard +csv-put --csv against a path passed without @#1337
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a safety guard to the ChangesCSV file-path guard for +csv-put
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
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. Comment |
github-actions
Bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/common/runner_input_test.go`:
- Around line 275-388: Tests call newTestRuntimeWithStdin directly and don't
isolate config/env; replace those usages with cmdutil.TestFactory(t, config) and
set t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of each test,
then use the factory's runtime (and set factory's stdin payload where tests used
the second arg of newTestRuntimeWithStdin) before calling resolveInputFlags;
update TestResolveInputFlags_FilenameMistakenForContent,
TestResolveInputFlags_ContentNotMistakenForPath,
TestResolveInputFlags_PathLikeContentViaStdin, and
TestResolveInputFlags_PathCheckSkippedWithoutFileInput to use the factory
pattern and provide stdin via the factory instead of newTestRuntimeWithStdin so
tests respect the repo test-factory isolation rules.
In `@shortcuts/common/runner.go`:
- Around line 1229-1230: The comment in shortcuts/common/runner.go incorrectly
states that the escape-hatch "@@" preserves a bare value; update the comment to
reflect actual behavior: "@@" prefixes the payload with a single '@' rather than
preserving the original bare value, and the true bypass is using stdin (the
"--flag -" convention) as implemented earlier. Locate the comment around the
argument parsing/trigger description (near the logic that handles "@@" and the
stdin "-" case) and rewrite it to mention that "@@" yields a leading "@" in the
payload and that "--flag -" is the correct way to force-passthrough via stdin.
🪄 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: b158748c-2eae-465b-9245-7a18e9e7e0e0
📒 Files selected for processing (2)
shortcuts/common/runner.goshortcuts/common/runner_input_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ab71e2070df73d01e0ff40609e092ac972a61255🧩 Skill updatenpx skills add larksuite/cli#feat/input-filename-guard -y -g |
github-actions
Bot
added
domain/mail
xiongyuanwen-byted
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1337 +/- ##
==========================================
+ Coverage 71.47% 71.79% +0.31%
==========================================
Files 688 690 +2
Lines 65482 65596 +114
==========================================
+ Hits 46806 47094 +288
+ Misses 15031 14845 -186
- Partials 3645 3657 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
domain/ccm
xiongyuanwen-byted
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/sheets/csv_put_guard_test.go (2)
45-57: ⚡ Quick winAdd test case for directory to exercise
IsDir()guard branch.The guard at line 324 has an explicit
info.IsDir()check, but the test doesn't verify that a directory value passes through without error. Consider adding a directory to the passing test cases.🧪 Add directory test coverage
dir := t.TempDir() cmdutil.TestChdir(t, dir) + if err := os.Mkdir("testdir", 0755); err != nil { + t.Fatal(err) + } if err := os.WriteFile("data.csv", []byte("a,b\n1,2\n"), 0644); err != nil { t.Fatal(err) } // ... existing test ... // Content that is not a real file must pass through unchanged. for _, v := range []string{ "改完记得更新config.json", // prose ending in a filename — not a real file "remember to update data.csv", // mentions the real file but isn't its name "a,b\n1,2", // multi-cell CSV "hello world", "nope.csv", // path-shaped but no such file + "testdir", // directory — not a regular file "", } {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/sheets/csv_put_guard_test.go` around lines 45 - 57, Add a test case that ensures a directory value passes through the guard that checks info.IsDir(): in the existing test loop that calls guardCSVValueIsNotFilePath(newCSVGuardRuntime(v)), include a value representing a directory (e.g., a temp dir path created with os.MkdirTemp or t.TempDir()) so the IsDir() branch is exercised and the call returns no error; locate the test using the function name guardCSVValueIsNotFilePath and helper newCSVGuardRuntime to add this passing-case directory value.
17-23: ⚡ Quick winPrefer
common.TestNewRuntimeContextfor proper test setup.Per learnings, validate/dry-run unit tests should use
common.TestNewRuntimeContext(t, config)instead of manually constructing aRuntimeContext. This ensures proper initialization of all fields including FileIO.♻️ Refactor the helper to use the recommended pattern
-func newCSVGuardRuntime(csvVal string) *common.RuntimeContext { - cmd := &cobra.Command{Use: "test"} - cmd.Flags().String("csv", "", "") - cmd.ParseFlags(nil) - cmd.Flags().Set("csv", csvVal) - return &common.RuntimeContext{Cmd: cmd} +func newCSVGuardRuntime(t *testing.T, csvVal string) *common.RuntimeContext { + rt := common.TestNewRuntimeContext(t, nil) + rt.Cmd.Flags().String("csv", "", "") + rt.Cmd.ParseFlags(nil) + rt.Cmd.Flags().Set("csv", csvVal) + return rt }Then update the test calls to pass
t:- err := guardCSVValueIsNotFilePath(newCSVGuardRuntime("data.csv")) + err := guardCSVValueIsNotFilePath(newCSVGuardRuntime(t, "data.csv"))Based on learnings, validate/dry-run unit tests should use
common.TestNewRuntimeContextfor consistent test setup patterns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/sheets/csv_put_guard_test.go` around lines 17 - 23, The helper newCSVGuardRuntime currently constructs a RuntimeContext manually and misses proper initialization (e.g., FileIO); replace its implementation to call common.TestNewRuntimeContext(t, map[string]string{"csv": csvVal}) so the returned *common.RuntimeContext is fully initialized; rename or change the helper signature to accept testing.T (t) and update all test call sites to pass t when invoking newCSVGuardRuntime (or inline the common.TestNewRuntimeContext call in tests) and reference the csv flag via the config map instead of manually setting Cmd flags.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@shortcuts/sheets/csv_put_guard_test.go`:
- Around line 45-57: Add a test case that ensures a directory value passes
through the guard that checks info.IsDir(): in the existing test loop that calls
guardCSVValueIsNotFilePath(newCSVGuardRuntime(v)), include a value representing
a directory (e.g., a temp dir path created with os.MkdirTemp or t.TempDir()) so
the IsDir() branch is exercised and the call returns no error; locate the test
using the function name guardCSVValueIsNotFilePath and helper newCSVGuardRuntime
to add this passing-case directory value.
- Around line 17-23: The helper newCSVGuardRuntime currently constructs a
RuntimeContext manually and misses proper initialization (e.g., FileIO); replace
its implementation to call common.TestNewRuntimeContext(t,
map[string]string{"csv": csvVal}) so the returned *common.RuntimeContext is
fully initialized; rename or change the helper signature to accept testing.T (t)
and update all test call sites to pass t when invoking newCSVGuardRuntime (or
inline the common.TestNewRuntimeContext call in tests) and reference the csv
flag via the config map instead of manually setting Cmd flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53605484-c876-431f-a562-cebdede7e404
📒 Files selected for processing (2)
shortcuts/sheets/csv_put_guard_test.goshortcuts/sheets/lark_sheet_write_cells.go
+csv-put --csv data.csv (a forgotten @) was silently written as one-cell content, because any string parses as valid CSV — unlike malformed JSON it never errored, so the filename landed in the sheet instead of the file's contents. +csv-put's Validate now rejects a --csv value when it names a real file in the cwd subtree (guardCSVValueIsNotFilePath; fileIO.Stat, fail-open), hinting to use --csv @file or stdin (--csv -). Scoped to --csv only — no framework or other-flag change. Checking real existence (not name shape) lets inline content that merely ends in a filename pass through. Adds TestGuardCSVValueIsNotFilePath.
xiongyuanwen-byted
force-pushed
the
feat/input-filename-guard
branch
from
June 9, 2026 11:12
6b45e99 to
ab71e20
Compare
liangshuo-1
approved these changes
Jun 9, 2026
fangshuyu-768
approved these changes
Jun 9, 2026
fangshuyu-768
approved these changes
Jun 9, 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.
What
+csv-put --csv data.csv(a forgotten@prefix) was silently written as one-cell content — any string is valid CSV, so unlike malformed JSON it never surfaced as an error, and the filename landed in the sheet instead of the file's contents.How
+csv-put'sValidatenow rejects a--csvvalue when it names a real file in the cwd subtree — the strong signal for a forgotten@. It fails fast with a hint to use--csv @file, or to pipe the literal text via stdin (--csv -).--csvonly (guardCSVValueIsNotFilePath, in+csv-put's Validate). No framework change, no other flag/command affected.@file/ stdin values are already their contents (a real CSV blob, never a path) and never trip it — only a bare value is checked.fileIO.Stat, fail-open: any Stat error or a directory leaves the value untouched.Why existence, not a name-shape check. Prose that merely ends in or mentions a filename — e.g.
--csv "改完记得更新config.json"— is not a real file and passes through. A "looks like a path" heuristic would wrongly reject such content, and its comma guard never fired for CJK text (full-width commas aren't ASCII,). Checking actual existence sidesteps both.Test
TestGuardCSVValueIsNotFilePath: a bare value naming an existing file is rejected with a fix-it hint; prose, multi-cell CSV, plain text, and path-shaped-but-nonexistent values all pass through.go test ./shortcuts/sheets/green; gofmt clean;go build ./...OK; golangci-lint 0 issues on the package.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
--csvinput is now validated: if it references an existing file without the required prefix, the command fails with guidance to use the file-prefix form or provide CSV via stdin; directory paths are also handled.Tests