Improve secure label error handling#1707
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 (1)
📝 WalkthroughWalkthroughThis PR expands Drive error-code metadata mappings and updates secure-label list/update commands to normalize ChangesDrive secure-label error handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
domain/ccm
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/drive/drive_secure_label_test.go (1)
153-273: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPlease cover the remaining changed paths.
The new tests lock down rejection and update-side API decoration, but this PR also changes success-path behavior by trimming
--label-idbefore DryRun/Execute and changes+secure-label-listto decorate API failures. A small request-body assertion for a whitespace-padded label ID, plus one list error case, would close the gaps introduced by this change set. As per coding guidelines, "Every behavior change needs a test alongside the change".🤖 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/drive/drive_secure_label_test.go` around lines 153 - 273, The current tests cover rejection and update error decoration, but they miss the new success-path trimming of --label-id and the API failure decoration in +secure-label-list. Add a request-body assertion around DriveSecureLabelUpdate to verify a whitespace-padded --label-id is trimmed before DryRun/Execute, and add one +secure-label-list error case that exercises the decorated API failure path using the existing DriveSecureLabelList command and errs API/validation types.Source: Coding guidelines
🤖 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/drive/drive_secure_label.go`:
- Around line 162-180: The decorateSecureLabelError helper is overwriting
existing Problem.Hint values and hard-coding update-only recovery text for codes
like 1063001, 1063002, and 9499, which is wrong for commands such as
+secure-label-list. Update decorateSecureLabelError to preserve any upstream
hint already set by errs.ProblemOf/classify logic, and make the added guidance
conditional on the invoking command or operation context rather than always
applying the same update-specific wording. Keep the symbol
decorateSecureLabelError as the place to adjust the hint-merging behavior and
command-aware branching.
---
Outside diff comments:
In `@shortcuts/drive/drive_secure_label_test.go`:
- Around line 153-273: The current tests cover rejection and update error
decoration, but they miss the new success-path trimming of --label-id and the
API failure decoration in +secure-label-list. Add a request-body assertion
around DriveSecureLabelUpdate to verify a whitespace-padded --label-id is
trimmed before DryRun/Execute, and add one +secure-label-list error case that
exercises the decorated API failure path using the existing DriveSecureLabelList
command and errs API/validation types.
🪄 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: 51a96c5c-da63-4131-b3dd-52dd1dffad65
📒 Files selected for processing (4)
internal/errclass/codemeta_drive.gointernal/errclass/codemeta_drive_test.goshortcuts/drive/drive_secure_label.goshortcuts/drive/drive_secure_label_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1707 +/- ##
========================================
Coverage 74.51% 74.51%
========================================
Files 850 851 +1
Lines 87070 87205 +135
========================================
+ Hits 64879 64983 +104
- Misses 17223 17244 +21
- Partials 4968 4978 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@85f738515e0d354f6776e681c5d6d00b858800cf🧩 Skill updatenpx skills add larksuite/cli#codex/secure-label-error-reliability -y -g |
|
Addressed the review feedback in 85f7385:\n\n- made secure-label error decoration operation-aware for list vs update\n- preserve existing upstream/classifier hints and append command-specific guidance instead of overwriting\n- added coverage for whitespace-padded --label-id trimming in dry-run/execute success paths\n- added +secure-label-list rate-limit test that preserves upstream detail and uses list-specific guidance\n\nValidation:\n- go test ./internal/errclass ./shortcuts/drive\n- go build ./...\n- git diff --check |
3 participants
Summary
Validation
Notes
Summary by CodeRabbit
New Features
--label-idusage tips and more contextual recovery hints.Bug Fixes
--label-idand enforce numeric, non-empty IDs.Tests