fix(sheets): mark +delete-dimension high-risk-write#1533
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)
📝 WalkthroughWalkthrough
ChangesSheetDeleteDimension confirmation gate
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 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: 1
🤖 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/sheets/backward/lark_sheets_sheet_dimension_test.go`:
- Around line 980-982: The error validation at lines 980-982 only checks if the
error is non-nil but does not validate typed error metadata, allowing
regressions in error types to go undetected. Replace the nil check with proper
typed error assertion using errs.ProblemOf to validate the error category and
subtype metadata, and assert the error cause chain where applicable. Apply this
same pattern to the additional locations mentioned at lines 1018-1020 as well.
🪄 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: db73caeb-d6ef-46dc-9cff-58bb285b31bb
📒 Files selected for processing (2)
shortcuts/sheets/backward/lark_sheets_row_column_management.goshortcuts/sheets/backward/lark_sheets_sheet_dimension_test.go
|
Thanks @coderabbitai — addressed in fe84b95. The two error-path tests now assert typed metadata via
The success and |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh api repos/larksuite/cli/pulls/1533/commits --jq '.[].sha' | head -5
gh api repos/larksuite/cli/commits/fe84b955 --jq '.files[] | select(.filename | contains("dimension_test")) | {filename: .filename, patch: .patch}'Length of output: 2060
The improvement over nil/substring checks is meaningful — these will catch regressions in both the error type and its structured metadata, not just the message text. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
recheck |
Deleting rows/columns is an irreversible structural change, but the deprecated +delete-dimension alias was annotated Risk: "write" and so never triggered the confirmation gate. Its modern replacement +dim-delete and the sibling +delete-sheet are both high-risk-write. Align +delete-dimension with them: without --yes it now returns a structured confirmation_required error (exit 10); --yes executes; --dry-run still previews without confirmation. - Lock the risk level with a metadata test. - Add gate-behavior tests (without --yes blocked / with --yes executes / --dry-run bypasses), and pass --yes in the existing execute tests. Closes larksuite#1532 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…aths Address CodeRabbit review: error-path tests must assert typed metadata via errs.ProblemOf, not nil/message-substring checks. - confirmation path: assert Category=confirmation / Subtype=confirmation_required - API-error path: assert Category=api / Code=90001 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hehanlin1996
force-pushed
the
fix/sheets-delete-dimension-high-risk
branch
from
fe84b95 to
68a4a85
Compare
|
recheck |
2 participants
Summary
sheets +delete-dimensiondeletes entire rows/columns (irreversible) but was annotatedRisk: "write", so it never triggered the high-risk confirmation gate. Its modern replacement+dim-deleteand the sibling+delete-sheetare bothhigh-risk-write. This aligns the deprecated alias with them.Changes
SheetDeleteDimension.Risk:"write"→"high-risk-write", so without--yesit returns a structuredconfirmation_requirederror (exit 10);--yesexecutes;--dry-runstill previews without confirmation.--yesblocked + API not called / with--yesexecutes /--dry-runbypasses.--yesunder the gate.Test Plan
go test ./shortcuts/sheets/...lark-cli sheets +delete-dimensionflow works as expected:--helpshowsRisk: high-risk-writeand--yes--yes→ exit 10,confirmation_required, no network call--dry-run→ previews the DELETE, no--yesneeded--yes→ passes the gate and reaches the APIRelated Issues
Summary by CodeRabbit
+delete-dimensioncommand now uses a higher-risk safety level and requires an explicit--yesconfirmation to proceed, since row/column deletion is irreversible.--dry-runto preview the DELETE request and response output without triggering the confirmation gate.--yes, successful execution with--yes, and confirmation bypass during--dry-run.