Skip to content

feat: add sheets +write-image shortcut#343

Merged
fangshuyu-768 merged 1 commit intomainfrom
feat/sheet_image
Apr 8, 2026
Merged

feat: add sheets +write-image shortcut#343
fangshuyu-768 merged 1 commit intomainfrom
feat/sheet_image

Conversation

@caojie0621
Copy link
Copy Markdown
Collaborator

@caojie0621 caojie0621 commented Apr 8, 2026

Summary

Add a new sheets +write-image shortcut that writes a local image file into
a spreadsheet cell via the Sheets v2 values_image API. Supports PNG, JPEG,
GIF, BMP, TIFF, HEIC and other common image formats.

Changes

  • Add SheetWriteImage shortcut in shortcuts/sheets/sheet_write_image.go
    with validate, dry-run, and execute handlers
  • Support --url / --spreadsheet-token, --range, --sheet-id,
    --image (required), and --name flags
  • Register the shortcut in shortcuts/sheets/shortcuts.go
  • Add skill reference doc skills/lark-sheets/references/lark-sheets-write-image.md
  • Update skills/lark-sheets/SKILL.md shortcut table

Test Plan

  • make unit-test passed
  • Added TestSheetWriteImage* covering validate, dry-run, and execute paths:
    • Validate: requires token via --url or --spreadsheet-token
    • DryRun: range normalization, default/custom name derivation
    • Execute: verifies JSON body with image bytes, rejects nonexistent file

Related Issues

N/A

Label: feature

Summary by CodeRabbit

  • New Features

    • Added a +write-image shortcut to write a local image into a single spreadsheet cell. Supports common image formats, accepts spreadsheet URL or token, requires a single-cell range (sheet-qualified or with --sheet-id), infers image name from filename, forbids absolute/unsafe paths, and enforces a 20MB file limit. Dry-run shows prepared JSON payload.
  • Documentation

    • Added a usage guide with examples, parameter rules, supported formats, and sample JSON output.
  • Tests

    • Added tests covering validation, dry-run normalization, execution, request payloads, and size/path/error cases.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 399d3f47-aa53-4f66-bff1-16350795b9cf

📥 Commits

Reviewing files that changed from the base of the PR and between ee7dc27 and 7fdf656.

📒 Files selected for processing (6)
  • shortcuts/sheets/helpers.go
  • shortcuts/sheets/sheet_write_image.go
  • shortcuts/sheets/sheet_write_image_test.go
  • shortcuts/sheets/shortcuts.go
  • skills/lark-sheets/SKILL.md
  • skills/lark-sheets/references/lark-sheets-write-image.md

📝 Walkthrough

Walkthrough

Adds a new sheets.SheetWriteImage shortcut (+write-image) to upload a local image into exactly one Google Sheets cell via the Sheets v2 values_image endpoint; includes validation, dry-run, execution, tests, a single‑cell range helper, and documentation.

Changes

Cohort / File(s) Summary
Core Implementation
shortcuts/sheets/sheet_write_image.go
New +write-image shortcut with flags (--url/--spreadsheet-token, --sheet-id, --range, --image, --name); Validate enforces credentials and single-cell range; DryRun builds JSON POST with placeholder binary image; Execute checks path safety, ensures regular file ≤20MB, reads bytes, infers name, and POSTs JSON to /open-apis/sheets/v2/spreadsheets/{token}/values_image.
Tests
shortcuts/sheets/sheet_write_image_test.go
End-to-end tests covering Validate, DryRun, and Execute: credential derivation, range normalization and single-cell enforcement, name inference/override, dry-run payload, execute request content/headers, stdout, and error cases (missing file, directory, oversized >20MB, unsafe absolute paths, API errors).
Helpers
shortcuts/sheets/helpers.go
Added validateSingleCellRange(input string) error to normalize separators, optionally strip <sheetId>!, accept empty input, and reject multi-cell spans unless both endpoints are identical (e.g., A1:A1).
Integration
shortcuts/sheets/shortcuts.go
Registered SheetWriteImage in the Shortcuts() return slice.
Documentation
skills/lark-sheets/SKILL.md, skills/lark-sheets/references/lark-sheets-write-image.md
Added +write-image entry and new reference doc describing usage, supported formats, parameter constraints (single-cell only, relative image paths), examples including --dry-run, and expected JSON output fields.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI Client
    participant Handler as SheetWriteImage Handler
    participant FS as File System
    participant API as Sheets v2 API

    CLI->>Handler: invoke +write-image (flags: --image, --range/--sheet-id, --url/--spreadsheet-token, --name?, --dry-run?)
    Handler->>Handler: resolve spreadsheet token, normalize/validate single-cell range
    alt validation error
        Handler-->>CLI: return validation error
    else continue
        Handler->>FS: validate path safety, existence, regular file, size ≤ 20MB
        FS-->>Handler: OK / Error
        alt FS OK
            Handler->>FS: read file bytes
            FS-->>Handler: image bytes
            alt dry-run
                Handler-->>CLI: emit dry-run JSON with placeholder image field
            else execute
                Handler->>API: POST /open-apis/sheets/v2/spreadsheets/{token}/values_image (JSON: range, image bytes, name)
                API-->>Handler: JSON response (spreadsheetToken, updateRange, revision)
                Handler-->>CLI: write response JSON to stdout
            end
        else FS error
            Handler-->>CLI: return FS error
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • liujinkun2025

Poem

🐰 I nibble bytes and tidy the trail,
I hop a token and polish a cell.
Names from filenames, ranges made neat,
A dry-run wink or a POST to meet.
Pixels snug in their single seat. 🎨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title directly and clearly describes the main change: adding a new sheets +write-image shortcut, which is the primary focus of the changeset.
Description check ✅ Passed The pull request description follows the template structure with Summary, Changes, Test Plan, and Related Issues sections. All required sections are present and populated with relevant details about the new feature.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/sheet_image

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added domain/ccm PR touches the ccm domain size/L Large or sensitive change across domains or core paths labels Apr 8, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 8, 2026

Greptile Summary

This PR adds a sheets +write-image shortcut that writes a local image file into a spreadsheet cell via the Lark Sheets v2 values_image API. The implementation includes validation, dry-run preview, and execute handlers, with file safety checks (path traversal rejection, 20 MB cap, regular-file guard) and a new validateSingleCellRange helper in helpers.go.

Confidence Score: 5/5

Safe to merge — all previously raised P1 concerns have been addressed and no new blocking issues were found.

The three P1 concerns from prior review rounds are resolved: the 20 MB size cap is in place, --range is declared Required (eliminating the bare-sheet-ID and DryRun-divergence scenarios), and the new validateSingleCellRange helper correctly gates multi-cell ranges. The only open item (Validate not checking file existence early) was already a pre-existing thread comment and is a UX nicety rather than a correctness defect. All remaining concerns are P2 or below.

shortcuts/sheets/sheet_write_image.go — minor: Validate does not pre-check image file existence, but this is an existing known concern and does not block merge.

Vulnerabilities

  • Path traversal: validate.SafeInputPath rejects absolute paths and .. sequences in --image; enforced in Execute but not in Validate (pre-existing thread concern, not new).
  • File read is gated behind a 20 MB size check before vfs.ReadFile, preventing memory exhaustion from large files.
  • No token or credential leakage in logs or dry-run output; the spreadsheet token appears only in the URL path via validate.EncodePathSegment.
  • --name is passed to the API without sanitization, but path-separator abuse there is a server-side concern.
  • No other injection, auth-bypass, or data-exposure issues identified.

Important Files Changed

Filename Overview
shortcuts/sheets/sheet_write_image.go Core shortcut implementation: Validate/DryRun/Execute handlers with path safety, 20 MB size check, and range normalization; --range is marked Required so the bare-sheet-ID and empty-range divergence concerns from earlier threads are addressed by design.
shortcuts/sheets/helpers.go Adds validateSingleCellRange to reject multi-cell spans; logic is correct and well-tested — A1:A1 (same-cell span) passes while A1:B2 is rejected.
shortcuts/sheets/sheet_write_image_test.go Comprehensive test suite covering validate, dry-run, execute happy paths, API error, oversized file, directory input, absolute path rejection, and custom name handling.
shortcuts/sheets/shortcuts.go Registers SheetWriteImage between SheetWrite and SheetAppend; one-line addition, no issues.
skills/lark-sheets/SKILL.md Adds +write-image row in the shortcut reference table; correctly positioned after +write.
skills/lark-sheets/references/lark-sheets-write-image.md New usage guide (Chinese) with examples, parameter table, output schema, and links to related skills; accurate and consistent with implementation.

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI
    participant Validate
    participant Execute
    participant LarkAPI

    User->>CLI: "sheets +write-image --token --range --image [--name] [--dry-run]"
    CLI->>Validate: "check token, validateSheetRangeInput, validateSingleCellRange"
    Validate-->>CLI: "ok / flag error"

    alt dry-run
        CLI-->>User: "JSON preview with binary placeholder"
    else execute
        CLI->>Execute: "SafeInputPath check"
        Execute->>Execute: "vfs.Stat: exists, regular file, size <= 20MB"
        Execute->>Execute: "vfs.ReadFile -> []byte (base64 in JSON)"
        Execute->>LarkAPI: "POST /open-apis/sheets/v2/spreadsheets/{token}/values_image"
        LarkAPI-->>Execute: "{spreadsheetToken, updateRange, revision}"
        Execute-->>User: "JSON output"
    end
Loading

Reviews (5): Last reviewed commit: "feat: add sheets +write-image shortcut" | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 8, 2026

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@7fdf656ba8b2ff8027685b28877b45406a8182a9

🧩 Skill update

npx skills add larksuite/cli#feat/sheet_image -y -g

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/sheets/sheet_write_image_test.go`:
- Around line 154-156: Replace the direct os.WriteFile call in the test with the
project VFS API: call vfs.WriteFile (or the configured test filesystem
instance's WriteFile function) instead of os.WriteFile, e.g. vfs.WriteFile(fs,
"test.png", imgData, 0644) or fs.WriteFile("test.png", imgData, 0644);
add/import the vfs/test filesystem variable if needed and remove the os import
if it becomes unused so the test uses the filesystem abstraction rather than
os.* directly.
- Around line 43-218: Tests leak shared config because they don't set
LARKSUITE_CLI_CONFIG_DIR; fix by setting t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) at the start of each test (e.g., in
TestSheetWriteImageValidateRequiresToken, TestSheetWriteImageDryRun,
TestSheetWriteImageExecuteSendsJSON,
TestSheetWriteImageExecuteRejectsNonexistentFile, etc.) or centrally inside
helpers used by these tests (newSheetsTestRuntime and/or the setup path that
calls cmdutil.TestFactory/mountAndRunSheets) so each test gets an isolated
config directory.

In `@shortcuts/sheets/sheet_write_image.go`:
- Around line 73-83: The fallback sets pointRange to a sheet ID only (via
getFirstSheetID and the sheet-id flag) which after normalizePointRange can still
be non-cell, but values_image requires a concrete A1 range; change the logic
around pointRange (the code that assigns pointRange, calls getFirstSheetID, and
then normalizePointRange) so that if pointRange contains only a sheet identifier
(no '!' or no cell reference) you append a default cell (for example "!A1" or
another single-cell A1 range) before calling normalizePointRange, ensuring
values_image receives a concrete cell range.
- Around line 40-42: validateSheetRangeInput currently allows multi-cell ranges
so the sheet_write_image handler must enforce a single-cell constraint: after
the existing call to validateSheetRangeInput(validate runtime.Str("sheet-id"),
runtime.Str("range")) add a check that parses the provided range
(runtime.Str("range")) and ensures it represents exactly one cell (start == end
for row and column) or call/implement a helper like ensureSingleCellRange(range)
and return an error when the range spans multiple cells; update
sheet_write_image.go to perform this single-cell validation before proceeding
with image write logic.

In `@skills/lark-sheets/references/lark-sheets-write-image.md`:
- Line 49: The table row for the `--image <path>` option is missing the trailing
pipe which violates MD055; edit the markdown row that currently reads "|
`--image <path>` | 是 | 本地图片文件的**相对路径**(必须在当前目录下,如 `./logo.png`;不支持绝对路径)" and
append a final "|" so the row ends with "...不支持绝对路径) |"; this restores the
required pipe-style table formatting for the table in
lark-sheets-write-image.md.
🪄 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: c23ad3cc-43ea-4f19-91ce-78cb444fa8ca

📥 Commits

Reviewing files that changed from the base of the PR and between db9ca5c and 9371625.

📒 Files selected for processing (5)
  • shortcuts/sheets/sheet_write_image.go
  • shortcuts/sheets/sheet_write_image_test.go
  • shortcuts/sheets/shortcuts.go
  • skills/lark-sheets/SKILL.md
  • skills/lark-sheets/references/lark-sheets-write-image.md

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 8, 2026

CLA assistant check
All committers have signed the CLA.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 8, 2026

Tip:

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.

Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
shortcuts/sheets/sheet_write_image_test.go (1)

10-10: 🛠️ Refactor suggestion | 🟠 Major

Please stop using os.* in the test fixtures.

These setup paths write fixtures via os.WriteFile, os.Mkdir, and os.Create, which bypasses the same filesystem abstraction the shortcut is expected to use. Swapping them to vfs.* keeps the tests aligned with the production path.

As per coding guidelines **/*.go: Use vfs.* instead of os.* for all filesystem access.

Also applies to: 261-263, 334-336, 374-376, 414-416, 460-462, 526-528, 566-568, 589-596, 619-621, 655-657

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/sheets/sheet_write_image_test.go` at line 10, Tests in
sheet_write_image_test.go are using os.* (os.WriteFile, os.Mkdir, os.Create,
etc.) which bypasses the virtual FS; replace all os filesystem calls with the
vfs equivalents (vfs.WriteFile, vfs.MkdirAll/Mkdir, vfs.Create or vfs.OpenFile
as appropriate), update the import to remove "os" and add the vfs package, and
ensure any file handle handling (Close) and permission/flag arguments are
adapted to the vfs APIs; check the helper/test functions referenced in this file
(setup fixtures and any functions that create or write files) and apply the same
swap for the other occurrences noted in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/sheets/helpers.go`:
- Around line 125-132: The current single-cell validation only rejects
multi-cell spans with cellSpanRangePattern but still allows whole-row/column or
partial spans like "A:B", "1:2", or "A1:B"; update the check around
cellSpanRangePattern.MatchString(subRange) to split subRange into parts, and
validate that both parts match a strict single-cell regex (e.g., cellRefPattern
like ^[A-Za-z]+[0-9]+$) before accepting; if either side does not match the
single-cell pattern, return the existing common.FlagErrorf("--range %q must be a
single cell (e.g. A1 or A1:A1), got a multi-cell span", input). Ensure you
reference and reuse cellSpanRangePattern, subRange, parts (from strings.SplitN),
and common.FlagErrorf so the fix integrates with the surrounding code.

In `@skills/lark-sheets/references/lark-sheets-write-image.md`:
- Around line 47-51: Add a clear sentence to lark-sheets-write-image.md
describing the default target when `--range` is omitted: state exactly where the
image will be written (per the new tests) and how `--sheet-id` influences that
default (e.g., default sheet vs first sheet and default cell), so users know the
behavior when `--range` is left blank; reference the `--range`, `--sheet-id`,
`--image`, and `--name` options when clarifying this default.

---

Duplicate comments:
In `@shortcuts/sheets/sheet_write_image_test.go`:
- Line 10: Tests in sheet_write_image_test.go are using os.* (os.WriteFile,
os.Mkdir, os.Create, etc.) which bypasses the virtual FS; replace all os
filesystem calls with the vfs equivalents (vfs.WriteFile, vfs.MkdirAll/Mkdir,
vfs.Create or vfs.OpenFile as appropriate), update the import to remove "os" and
add the vfs package, and ensure any file handle handling (Close) and
permission/flag arguments are adapted to the vfs APIs; check the helper/test
functions referenced in this file (setup fixtures and any functions that create
or write files) and apply the same swap for the other occurrences noted in the
comment.
🪄 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: f72be6bb-7238-4754-a116-c5be14eefa24

📥 Commits

Reviewing files that changed from the base of the PR and between 9371625 and e0bf215.

📒 Files selected for processing (6)
  • shortcuts/sheets/helpers.go
  • shortcuts/sheets/sheet_write_image.go
  • shortcuts/sheets/sheet_write_image_test.go
  • shortcuts/sheets/shortcuts.go
  • skills/lark-sheets/SKILL.md
  • skills/lark-sheets/references/lark-sheets-write-image.md
✅ Files skipped from review due to trivial changes (2)
  • skills/lark-sheets/SKILL.md
  • shortcuts/sheets/shortcuts.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • shortcuts/sheets/sheet_write_image.go

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
shortcuts/sheets/sheet_write_image_test.go (1)

468-471: Consider checking error from fh.Close().

While not critical for test cleanup, explicitly checking or deferring the close with error handling is slightly more robust.

-	fh.Close()
+	if err := fh.Close(); err != nil {
+		t.Logf("Close() warning: %v", err)
+	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/sheets/sheet_write_image_test.go` around lines 468 - 471, The test
currently calls fh.Close() without checking its error; update the teardown to
handle Close errors by either deferring fh.Close() and checking the returned
error (e.g., `defer func(){ if err := fh.Close(); err != nil { t.Fatalf("Close()
error: %v", err) } }()`) or capture and assert the error after Truncate (replace
the bare `fh.Close()` with an error check), referencing the file handle `fh` and
the existing `Truncate` call to place the check immediately after that
operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@skills/lark-sheets/references/lark-sheets-write-image.md`:
- Around line 57-59: The docs list an output field named `updatedRange` but the
actual API/test response uses `updateRange` (no "d"); update the documentation
to match the API by replacing `updatedRange` with `updateRange` in the output
field descriptions for the Lark Sheets write-image response so examples and
field listings align with the real response.

---

Nitpick comments:
In `@shortcuts/sheets/sheet_write_image_test.go`:
- Around line 468-471: The test currently calls fh.Close() without checking its
error; update the teardown to handle Close errors by either deferring fh.Close()
and checking the returned error (e.g., `defer func(){ if err := fh.Close(); err
!= nil { t.Fatalf("Close() error: %v", err) } }()`) or capture and assert the
error after Truncate (replace the bare `fh.Close()` with an error check),
referencing the file handle `fh` and the existing `Truncate` call to place the
check immediately after that operation.
🪄 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: e12c5a33-d127-4284-bfc1-d2047bb388e1

📥 Commits

Reviewing files that changed from the base of the PR and between e0bf215 and 4db4c34.

📒 Files selected for processing (6)
  • shortcuts/sheets/helpers.go
  • shortcuts/sheets/sheet_write_image.go
  • shortcuts/sheets/sheet_write_image_test.go
  • shortcuts/sheets/shortcuts.go
  • skills/lark-sheets/SKILL.md
  • skills/lark-sheets/references/lark-sheets-write-image.md
✅ Files skipped from review due to trivial changes (2)
  • skills/lark-sheets/SKILL.md
  • shortcuts/sheets/shortcuts.go

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
shortcuts/sheets/sheet_write_image_test.go (1)

491-504: Use the temp file’s absolute path instead of /etc/passwd.

This keeps the test hermetic and still exercises the same validate.SafeInputPath rejection. It also avoids baking a Unix-specific path into a CLI test.

♻️ Proposed refactor
 import (
 	"bytes"
 	"context"
 	"encoding/json"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
@@
-	err := mountAndRunSheets(t, SheetWriteImage, []string{
+	absPath := filepath.Join(tmpDir, "abs.png")
+
+	err := mountAndRunSheets(t, SheetWriteImage, []string{
 		"+write-image",
 		"--spreadsheet-token", "shtTOKEN",
 		"--range", "sheet1!A1:A1",
-		"--image", "/etc/passwd",
+		"--image", absPath,
 		"--as", "user",
 	}, f, nil)

Based on learnings: absolute paths are intentionally unsupported by design, and validate.SafeInputPath is the intended rejection gate.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/sheets/sheet_write_image_test.go` around lines 491 - 504, The test
uses a hard-coded Unix path "/etc/passwd" which makes it non-hermetic; change
the mountAndRunSheets invocation for the SheetWriteImage test to pass the
absolute path of the temp file created earlier (the "abs.png" in the tmpDir)
instead of "/etc/passwd" so the test remains platform-independent yet still
exercises validate.SafeInputPath's rejection; locate the mountAndRunSheets call
in this test and replace the "--image" argument with the computed absolute path
for the temp file (use the temp file's path from the tmpDir/abs.png or
filepath.Abs on that filename) so the rest of the test and SheetWriteImage
behavior 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 `@skills/lark-sheets/references/lark-sheets-write-image.md`:
- Around line 47-50: Add a note to the `--image <path>` option line stating
there is a 20MB file size cap (rejects image files larger than 20MB) and that
the path must be a relative path in the current directory (no absolute paths);
this mirrors the runtime validation that rejects files above 20MB in the image
upload validation logic and will make the error predictable for users.

---

Nitpick comments:
In `@shortcuts/sheets/sheet_write_image_test.go`:
- Around line 491-504: The test uses a hard-coded Unix path "/etc/passwd" which
makes it non-hermetic; change the mountAndRunSheets invocation for the
SheetWriteImage test to pass the absolute path of the temp file created earlier
(the "abs.png" in the tmpDir) instead of "/etc/passwd" so the test remains
platform-independent yet still exercises validate.SafeInputPath's rejection;
locate the mountAndRunSheets call in this test and replace the "--image"
argument with the computed absolute path for the temp file (use the temp file's
path from the tmpDir/abs.png or filepath.Abs on that filename) so the rest of
the test and SheetWriteImage behavior 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: 650844f3-2436-4fb5-b219-167e1a5a2d68

📥 Commits

Reviewing files that changed from the base of the PR and between 4db4c34 and ee7dc27.

📒 Files selected for processing (6)
  • shortcuts/sheets/helpers.go
  • shortcuts/sheets/sheet_write_image.go
  • shortcuts/sheets/sheet_write_image_test.go
  • shortcuts/sheets/shortcuts.go
  • skills/lark-sheets/SKILL.md
  • skills/lark-sheets/references/lark-sheets-write-image.md
✅ Files skipped from review due to trivial changes (1)
  • skills/lark-sheets/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • shortcuts/sheets/shortcuts.go
  • shortcuts/sheets/helpers.go

Send image bytes as JSON body (not multipart/form-data) to match
the v2 values_image API contract. Use CallAPI for consistent
response envelope handling. Includes skill reference doc and
unit tests for validate, dry-run, and execute paths.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
skills/lark-sheets/references/lark-sheets-write-image.md (1)

49-49: ⚠️ Potential issue | 🟡 Minor

Document the 20MB cap on --image.

The shortcut rejects files above 20MB at shortcuts/sheets/sheet_write_image.go Line 90-Line 93, but this row only documents the path restriction. Please add the size limit here so users can predict that validation failure.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@skills/lark-sheets/references/lark-sheets-write-image.md` at line 49, Update
the table row for the `--image <path>` flag to also document the 20MB file-size
limit (e.g., append "且不大于20MB" or "最大 20MB") so users know the CLI will reject
images larger than 20MB; this mirrors the validation implemented in
sheet_write_image.go that rejects files above 20MB and prevents surprise
failures when using `--image`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/sheets/sheet_write_image.go`:
- Around line 32-46: The validation for the --image path currently lives in
Execute and allows DryRun to accept paths the real command will reject; move the
non-I/O safety check into the Validate function by invoking
validate.SafeInputPath on the runtime string for the image flag (e.g.,
runtime.Str("image") or the flag name used in this file) and return the error if
it fails, while leaving file existence/size/read checks in Execute; ensure you
reference the Validate, Execute and DryRun functions and use
validate.SafeInputPath for the pre-I/O path validation.

---

Duplicate comments:
In `@skills/lark-sheets/references/lark-sheets-write-image.md`:
- Line 49: Update the table row for the `--image <path>` flag to also document
the 20MB file-size limit (e.g., append "且不大于20MB" or "最大 20MB") so users know
the CLI will reject images larger than 20MB; this mirrors the validation
implemented in sheet_write_image.go that rejects files above 20MB and prevents
surprise failures when using `--image`.
🪄 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: 8661c5de-c223-4b59-abdf-bb7c188abcdd

📥 Commits

Reviewing files that changed from the base of the PR and between ee7dc27 and 7fdf656.

📒 Files selected for processing (6)
  • shortcuts/sheets/helpers.go
  • shortcuts/sheets/sheet_write_image.go
  • shortcuts/sheets/sheet_write_image_test.go
  • shortcuts/sheets/shortcuts.go
  • skills/lark-sheets/SKILL.md
  • skills/lark-sheets/references/lark-sheets-write-image.md
✅ Files skipped from review due to trivial changes (1)
  • skills/lark-sheets/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • shortcuts/sheets/helpers.go
  • shortcuts/sheets/sheet_write_image_test.go
  • shortcuts/sheets/shortcuts.go

@fangshuyu-768 fangshuyu-768 merged commit 15bd134 into main Apr 8, 2026
17 checks passed
@fangshuyu-768 fangshuyu-768 deleted the feat/sheet_image branch April 8, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/ccm PR touches the ccm domain size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants