Skip to content

fix(sheets): correct scope name from "sheets:spreadsheet:read" to "sheets:spreadsheet:readonly"#902

Open
EvanYao826 wants to merge 3 commits into
larksuite:mainfrom
EvanYao826:fix/sheets-scope-readonly
Open

fix(sheets): correct scope name from "sheets:spreadsheet:read" to "sheets:spreadsheet:readonly"#902
EvanYao826 wants to merge 3 commits into
larksuite:mainfrom
EvanYao826:fix/sheets-scope-readonly

Conversation

@EvanYao826
Copy link
Copy Markdown

@EvanYao826 EvanYao826 commented May 15, 2026

Summary

Fixes #838
The Feishu Open Platform uses sheets:spreadsheet:readonly as the actual scope name for read access to spreadsheets. The incorrect scope sheets:spreadsheet:read was hardcoded in all sheets shortcut commands, causing the local precheck to reject tokens that had the correct scopes (:readonly, :write_only, :create).

Changes

  • Replaced all 36 occurrences of "sheets:spreadsheet:read" with "sheets:spreadsheet:readonly" across 9 sheets shortcut files
  • Updated internal/registry/registry_test.go to use the correct scope name
  • Updated skills/lark-sheets/SKILL.md documentation table

Files modified

  • shortcuts/sheets/lark_sheets_cell_data.go
  • shortcuts/sheets/lark_sheets_cell_images.go
  • shortcuts/sheets/lark_sheets_cell_style_and_merge.go
  • shortcuts/sheets/lark_sheets_dropdown.go
  • shortcuts/sheets/lark_sheets_filter_views.go
  • shortcuts/sheets/lark_sheets_float_images.go
  • shortcuts/sheets/lark_sheets_row_column_management.go
  • shortcuts/sheets/lark_sheets_sheet_management.go
  • shortcuts/sheets/lark_sheets_spreadsheet_management.go
  • internal/registry/registry_test.go
  • skills/lark-sheets/SKILL.md

Testing

Verified that scope_overrides.json already contains sheets:spreadsheet:readonly in both the priority overrides and the recommend allow list, confirming this is the correct scope name.

Summary by CodeRabbit

  • Chores
    • Updated OAuth scope declarations for Sheets operations to use sheets:spreadsheet:readonly (and retain sheets:spreadsheet:write_only where applicable).
  • Tests
    • Adjusted tests to expect the revised Sheets OAuth scopes.
  • Documentation
    • Updated Sheets permission reference entries to reflect the new readonly scope.

Review Change Stack

…eets:spreadsheet:readonly'

The Feishu Open Platform uses 'sheets:spreadsheet:readonly' as the
actual scope name for read access to spreadsheets. The incorrect scope
'sheets:spreadsheet:read' was hardcoded in all sheets shortcut commands,
causing the local precheck to reject tokens that had the correct scopes.

Fixed all 36 occurrences across 9 shortcut files, updated the registry
test, and corrected the SKILL.md documentation table.

Fixes larksuite#838
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@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 May 15, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

📝 Walkthrough

Walkthrough

Replaces all occurrences of the incorrect OAuth scope sheets:spreadsheet:read with the valid sheets:spreadsheet:readonly across Sheets shortcut declarations, corresponding registry tests, and the Sheets skill permission table.

Changes

Sheets OAuth Scope Correction

Layer / File(s) Summary
Registry and test scope expectations
internal/registry/registry_test.go
Precheck tests for auto-approve now expect sheets:spreadsheet:readonly in allow-list assertions and filtered-scope inputs.
Shortcut OAuth scope declarations
shortcuts/sheets/lark_sheets_cell_data.go, shortcuts/sheets/lark_sheets_cell_images.go, shortcuts/sheets/lark_sheets_cell_style_and_merge.go, shortcuts/sheets/lark_sheets_dropdown.go, shortcuts/sheets/lark_sheets_filter_views.go, shortcuts/sheets/lark_sheets_float_images.go, shortcuts/sheets/lark_sheets_row_column_management.go, shortcuts/sheets/lark_sheets_sheet_management.go, shortcuts/sheets/lark_sheets_spreadsheet_management.go
Exported Sheets shortcut variables updated to use sheets:spreadsheet:readonly for read operations; write-capable shortcuts retain sheets:spreadsheet:write_only alongside readonly where applicable.
Documentation scope table updates
skills/lark-sheets/SKILL.md
Permission table entries updated from sheets:spreadsheet:read to sheets:spreadsheet:readonly for listed read operations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • larksuite/cli#722: Updates OAuth scope strings for sheet-management shortcuts (overlapping scope changes).
  • larksuite/cli#412: Adds cell-operation shortcuts that are receiving corrected scope declarations here.
  • larksuite/cli#494: Introduced float-image shortcuts which have their scope strings updated in this PR.

Suggested reviewers

  • fangshuyu-768

Poem

🐰 I nudged through code at break of day,

read< turned to >readonly< along the way,
tokens now pass the precheck door,
shortcuts hum and errors no more —
a tiny hop, a tidy play.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: correcting the scope name from 'sheets:spreadsheet:read' to 'sheets:spreadsheet:readonly' across the sheets shortcuts.
Linked Issues check ✅ Passed The PR fully addresses issue #838 by replacing all 36 occurrences of the non-existent scope 'sheets:spreadsheet:read' with the correct 'sheets:spreadsheet:readonly' across 9 shortcuts files, test files, and documentation.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the scope naming issue in #838. Updates to 9 shortcuts files, registry test, and documentation are all within scope of the fix.
Description check ✅ Passed The PR description covers all required template sections: Summary (with issue reference), Changes (with specific scope replacements), Files modified list, and Testing details.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

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

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

🤖 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 `@internal/registry/registry_test.go`:
- Around line 240-241: The test assertion message is stale: when checking
aaSet["sheets:spreadsheet:readonly"] in registry_test.go (inside the test that
verifies auto-approve sets), update the t.Error message to mention
"sheets:spreadsheet:readonly" instead of the old "sheets:spreadsheet:read" so
the failure message matches the checked scope (refer to aaSet and the
"sheets:spreadsheet:readonly" key to locate the assertion).

In `@shortcuts/sheets/lark_sheets_sheet_management.go`:
- Line 500: The Scopes slice incorrectly includes the non-existent scope
"sheets:spreadsheet:write_only"; replace that value with the official write
scope "sheets:spreadsheet" wherever it's used (e.g., the Scopes: []string{...}
declarations in this file) so that write permissions use "sheets:spreadsheet"
and read-only remains "sheets:spreadsheet:readonly"; update all occurrences
noted in the review (the other Scopes declarations around the same code paths)
to avoid using the custom "write_only" scope.
🪄 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: 5cc3906e-8b97-494f-9e56-4143b559c203

📥 Commits

Reviewing files that changed from the base of the PR and between f03138b and 7c53824.

📒 Files selected for processing (11)
  • internal/registry/registry_test.go
  • shortcuts/sheets/lark_sheets_cell_data.go
  • shortcuts/sheets/lark_sheets_cell_images.go
  • shortcuts/sheets/lark_sheets_cell_style_and_merge.go
  • shortcuts/sheets/lark_sheets_dropdown.go
  • shortcuts/sheets/lark_sheets_filter_views.go
  • shortcuts/sheets/lark_sheets_float_images.go
  • shortcuts/sheets/lark_sheets_row_column_management.go
  • shortcuts/sheets/lark_sheets_sheet_management.go
  • shortcuts/sheets/lark_sheets_spreadsheet_management.go
  • skills/lark-sheets/SKILL.md

Comment thread internal/registry/registry_test.go Outdated
Comment thread shortcuts/sheets/lark_sheets_sheet_management.go
Address CodeRabbit review: the error message in
TestLoadAutoApproveSet still referenced the old scope name.
@EvanYao826
Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@EvanYao826
Copy link
Copy Markdown
Author

@CLAassistant recheck please

@EvanYao826
Copy link
Copy Markdown
Author

@CLAassistant recheck

@EvanYao826
Copy link
Copy Markdown
Author

@CLAassistant recheck

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.

[bug] sheets shortcut precheck rejects valid token — hardcoded "sheets:spreadsheet:read" instead of "sheets:spreadsheet:readonly"

2 participants