feat(base): require delete approval auth code for destructive shortcuts#1816
Closed
dengfanxin wants to merge 1 commit into
Closed
feat(base): require delete approval auth code for destructive shortcuts#1816dengfanxin wants to merge 1 commit into
dengfanxin wants to merge 1 commit into
Conversation
1. add shared delete approval helper and prepare-approval/auth-code flags for base delete shortcuts
2. validate auth code before delete execution for table field view record and form paths
3. update shortcut tests to stub validate-auth-code and pass auth-code on destructive flows
说明：
- local run-test pipeline lookup is unavailable in this repo config,
but scoped go vet and the earlier shortcuts/base test rerun both passed
```ai-signature
改动范围: shortcuts/base/delete_approval.go、各 delete shortcut/ops 文件以及 base_execute_test.go/base_form_execute_test.go
思考过程: 把审批校验收口成一个共享 helper，避免在每个 delete shortcut 重复拼 request/validate 逻辑；真正执行删除前统一验码，测试只补最小 stub 以覆盖新契约。
改动原因: 需求要求 CLI 删除命令先走 prepare approval，再显式传一次性 auth code 才能执行危险删除；本次需要把该约束接到所有 base delete shortcut。
Break Change: 否
```
Co-authored-by: BASE Infra Harness <ai@base-infra-harness.noreply.local>
AI-SHA256: c1fc8965c8f94e5458ea67f552733422e67b26d20123ff72e9aaac4f2ef2c188
github-actions
Bot
added
domain/base
📝 WalkthroughWalkthroughIntroduces a delete-approval module ( ChangesDelete Approval Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant handleDeleteApproval
participant ApprovalAPI
participant BaseAPI
CLI->>handleDeleteApproval: deleteApprovalSpec(action, resource)
alt prepare-approval set
handleDeleteApproval->>ApprovalAPI: POST action/token/resource
ApprovalAPI-->>handleDeleteApproval: approval_url, expires_at, request_digest
handleDeleteApproval-->>CLI: print approval info, stop=true
CLI-->>CLI: return without deleting
else auth-code provided
handleDeleteApproval->>ApprovalAPI: POST auth-code, request_digest
ApprovalAPI-->>handleDeleteApproval: error_code or success
handleDeleteApproval-->>CLI: stop=false or return error
CLI->>BaseAPI: DELETE resource
BaseAPI-->>CLI: deletion result
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/base/base_form_execute_test.go (1)
227-241: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd tests for the new delete-approval branches In
shortcuts/base/base_form_execute_test.go, add coverage for--prepare-approvaland the missing--auth-codepath; the latter should asserterrs.ProblemOf/errors.Asmetadata, not just the message.🤖 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/base/base_form_execute_test.go` around lines 227 - 241, Add test coverage in TestBaseFormExecuteDelete and related base form execution tests for the new delete-approval branches: add a case for BaseFormDelete with --prepare-approval and a separate case for the missing --auth-code path. Use the existing BaseFormDelete, runShortcut, and registerDeleteApprovalValidationStub setup to locate the flow, and for the missing auth-code case assert the returned error with errs.ProblemOf via errors.As so the metadata is verified instead of only matching the message.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/base/delete_approval.go`:
- Around line 1-96: Add targeted unit tests around deleteApproval helpers to
cover the untested branches in handleDeleteApproval, prepareDeleteApproval,
validateDeleteApproval, and deleteApprovalDigest. Verify --prepare-approval
returns the approval request path output, missing --auth-code triggers the
expected baseFlagErrorf branch, validateDeleteApproval surfaces an
error_code/error_message response as a baseValidationErrorf, and
deleteApprovalDigest is stable for the same deleteApprovalSpec inputs. Use the
existing deleteApprovalSpec and RuntimeContext behavior to locate and exercise
these branches directly.
In `@shortcuts/base/record_ops.go`:
- Around line 344-355: The deduplication helper is using the wrong
RuntimeContext accessor, which breaks compilation because record-ids is
registered as a string-array flag and only StrArray is available. Update
deduplicateRecordIDs to read the input with runtime.StrArray("record-ids")
instead of runtime.StrSlice("record-ids"), keeping the rest of the deduplication
logic unchanged.
---
Outside diff comments:
In `@shortcuts/base/base_form_execute_test.go`:
- Around line 227-241: Add test coverage in TestBaseFormExecuteDelete and
related base form execution tests for the new delete-approval branches: add a
case for BaseFormDelete with --prepare-approval and a separate case for the
missing --auth-code path. Use the existing BaseFormDelete, runShortcut, and
registerDeleteApprovalValidationStub setup to locate the flow, and for the
missing auth-code case assert the returned error with errs.ProblemOf via
errors.As so the metadata is verified instead of only matching the message.
🪄 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: f6a0e729-f724-4c8b-85de-b51edb98f98f
📒 Files selected for processing (12)
shortcuts/base/base_execute_test.goshortcuts/base/base_form_delete.goshortcuts/base/base_form_execute_test.goshortcuts/base/delete_approval.goshortcuts/base/field_delete.goshortcuts/base/field_ops.goshortcuts/base/record_delete.goshortcuts/base/record_ops.goshortcuts/base/table_delete.goshortcuts/base/table_ops.goshortcuts/base/view_delete.goshortcuts/base/view_ops.go
Comment on lines
+1
to
+96
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package base | ||
|
|
||
| import ( | ||
| "crypto/sha256" | ||
| "encoding/hex" | ||
| "fmt" | ||
| "net/http" | ||
| "strings" | ||
|
|
||
| "github.com/larksuite/cli/shortcuts/common" | ||
| ) | ||
|
|
||
| const deleteApprovalValidatePath = "/space/api/base/delete/validate-auth-code" | ||
| const deleteApprovalRequestPath = "/space/api/base/delete/requests" | ||
|
|
||
| type deleteApprovalSpec struct { | ||
| Action string | ||
| BaseToken string | ||
| ResourceType string | ||
| ResourceID string | ||
| } | ||
|
|
||
| func deleteApprovalFlags() []common.Flag { | ||
| return []common.Flag{ | ||
| {Name: "auth-code", Desc: "one-time delete authorization code returned by the Base delete approval page"}, | ||
| {Name: "prepare-approval", Type: "bool", Desc: "create a delete approval request instead of executing the delete"}, | ||
| } | ||
| } | ||
|
|
||
| func appendDeleteApprovalFlags(flags ...common.Flag) []common.Flag { | ||
| return append(flags, deleteApprovalFlags()...) | ||
| } | ||
|
|
||
| func handleDeleteApproval(runtime *common.RuntimeContext, spec deleteApprovalSpec) (bool, error) { | ||
| if runtime.Bool("prepare-approval") { | ||
| return true, prepareDeleteApproval(runtime, spec) | ||
| } | ||
| if strings.TrimSpace(runtime.Str("auth-code")) == "" { | ||
| return false, baseFlagErrorf("--auth-code is required for %s; run with --prepare-approval first to create an approval URL", spec.Action) | ||
| } | ||
| return false, validateDeleteApproval(runtime, spec) | ||
| } | ||
|
|
||
| func prepareDeleteApproval(runtime *common.RuntimeContext, spec deleteApprovalSpec) error { | ||
| data, err := runtime.CallAPITyped(http.MethodPost, deleteApprovalRequestPath, nil, map[string]interface{}{ | ||
| "action": spec.Action, | ||
| "base_token": spec.BaseToken, | ||
| "resource_type": spec.ResourceType, | ||
| "resource_id": spec.ResourceID, | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| runtime.Out(map[string]interface{}{ | ||
| "approval_required": true, | ||
| "request_id": common.GetString(data, "request_id"), | ||
| "approval_url": common.GetString(data, "approval_url"), | ||
| "expires_at": data["expires_at"], | ||
| "request_digest": common.GetString(data, "request_digest"), | ||
| "action": spec.Action, | ||
| "base_token": spec.BaseToken, | ||
| "resource_type": spec.ResourceType, | ||
| "resource_id": spec.ResourceID, | ||
| }, nil) | ||
| return nil | ||
| } | ||
|
|
||
| func validateDeleteApproval(runtime *common.RuntimeContext, spec deleteApprovalSpec) error { | ||
| data, err := runtime.CallAPITyped(http.MethodPost, deleteApprovalValidatePath, nil, map[string]interface{}{ | ||
| "auth_code": strings.TrimSpace(runtime.Str("auth-code")), | ||
| "action": spec.Action, | ||
| "base_token": spec.BaseToken, | ||
| "resource_type": spec.ResourceType, | ||
| "resource_id": spec.ResourceID, | ||
| "request_digest": deleteApprovalDigest(spec), | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if code := common.GetString(data, "error_code"); code != "" { | ||
| msg := common.GetString(data, "error_message") | ||
| if msg == "" { | ||
| msg = code | ||
| } | ||
| return baseValidationErrorf("delete authorization failed: %s", msg) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func deleteApprovalDigest(spec deleteApprovalSpec) string { | ||
| sum := sha256.Sum256([]byte(fmt.Sprintf("%s|%s|%s|%s", spec.Action, spec.BaseToken, spec.ResourceType, spec.ResourceID))) | ||
| return hex.EncodeToString(sum[:]) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for test files covering delete_approval functions
rg -l 'handleDeleteApproval|prepareDeleteApproval|validateDeleteApproval|deleteApprovalDigest' shortcuts/base/ --type=go -g '*_test.go'
# Check for direct tests of delete_approval functions
rg -n 'func Test.*[Dd]elete[Aa]pproval|func Test.*HandleDeleteApproval|func Test.*DeleteApprovalDigest' shortcuts/base/ --type=goRepository: larksuite/cli
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List base shortcut files and tests
git ls-files shortcuts/base | sed -n '1,200p'
printf '\n--- deleteApproval references ---\n'
rg -n 'deleteApproval|prepare-approval|auth-code|request_digest|delete/validate-auth-code|delete/requests' shortcuts/base --type=go
printf '\n--- test files in shortcuts/base ---\n'
rg -n '^func Test' shortcuts/base --type=go -g '*_test.go'Repository: larksuite/cli
Length of output: 22113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '*_test.go' shortcuts/base | sed -n '1,200p'Repository: larksuite/cli
Length of output: 440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect delete-approval-related tests and the validation stub
sed -n '1,180p' shortcuts/base/base_execute_test.go
printf '\n--- table/record/view delete test regions ---\n'
sed -n '1160,1285p' shortcuts/base/base_execute_test.go
printf '\n---\n'
sed -n '2880,2975p' shortcuts/base/base_execute_test.go
printf '\n---\n'
sed -n '2020,2085p' shortcuts/base/base_execute_test.go
printf '\n--- delete approval consumers ---\n'
sed -n '160,235p' shortcuts/base/table_ops.go
sed -n '170,215p' shortcuts/base/view_ops.go
sed -n '490,535p' shortcuts/base/record_ops.go
sed -n '165,205p' shortcuts/base/field_ops.goRepository: larksuite/cli
Length of output: 24431
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('shortcuts/base/base_execute_test.go')
text = path.read_text()
targets = [
'func TestBaseTableExecuteReadAndDelete',
'func TestBaseRecordExecuteReadCreateDelete',
'func TestBaseViewExecuteReadCreateDeleteAndFilter',
'func TestBaseFieldExecuteCRUD',
'func registerDeleteApprovalValidationStub',
]
for target in targets:
idx = text.find(target)
print(f'\n=== {target} ===')
if idx == -1:
print('NOT FOUND')
continue
start = max(0, idx - 200)
end = min(len(text), idx + 2200)
print(text[start:end])
PYRepository: larksuite/cli
Length of output: 12405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- prepare-approval occurrences ---'
rg -n 'prepare-approval|delete/requests|delete/validate-auth-code|request_digest|auth-code' shortcuts/base --type=go
printf '\n%s\n' '--- delete approval helper tests elsewhere ---'
rg -n 'handleDeleteApproval|prepareDeleteApproval|validateDeleteApproval|deleteApprovalDigest|registerDeleteApprovalValidationStub' shortcuts --type=goRepository: larksuite/cli
Length of output: 6173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --fixed-strings '--auth-code is required' shortcuts/base --type=go
printf '\n---\n'
rg -n --fixed-strings '--prepare-approval' shortcuts/base --type=go
printf '\n---\n'
rg -n 'delete approval|approval URL|request_digest|error_code' shortcuts/base/*_test.go --type=goRepository: larksuite/cli
Length of output: 577
Add targeted tests for the delete-approval helper branches.
base_execute_test.go covers the delete commands’ happy paths, but delete_approval.go still needs direct coverage for --prepare-approval, missing --auth-code, validation error_code responses, and deleteApprovalDigest.
🤖 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/base/delete_approval.go` around lines 1 - 96, Add targeted unit
tests around deleteApproval helpers to cover the untested branches in
handleDeleteApproval, prepareDeleteApproval, validateDeleteApproval, and
deleteApprovalDigest. Verify --prepare-approval returns the approval request
path output, missing --auth-code triggers the expected baseFlagErrorf branch,
validateDeleteApproval surfaces an error_code/error_message response as a
baseValidationErrorf, and deleteApprovalDigest is stable for the same
deleteApprovalSpec inputs. Use the existing deleteApprovalSpec and
RuntimeContext behavior to locate and exercise these branches directly.
Source: Coding guidelines
Comment on lines
+344
to
+355
| func deduplicateRecordIDs(runtime *common.RuntimeContext) []string { | ||
| raw := runtime.StrSlice("record-ids") | ||
| seen := make(map[string]bool, len(raw)) | ||
| result := make([]string, 0, len(raw)) | ||
| for _, id := range raw { | ||
| if id != "" && !seen[id] { | ||
| seen[id] = true | ||
| result = append(result, id) | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm StrSlice accessor and record-ids flag registration
echo "== StrSlice / StrArray definitions on RuntimeContext =="
rg -nP '\bfunc\s*\(\w+\s*\*?RuntimeContext\)\s*Str(Slice|Array)\s*\(' shortcuts/common
echo "== StrSlice call sites =="
rg -nP '\bStrSlice\s*\(' shortcuts
echo "== record-ids flag registration (share-batch) =="
rg -nP 'record-ids' shortcuts/baseRepository: larksuite/cli
Length of output: 769
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== RuntimeContext definition and string accessors =="
rg -n -A3 -B3 'type RuntimeContext|func \(ctx \*RuntimeContext\) Str(Array|Slice)\(' shortcuts/common/runner.go shortcuts/common
echo "== Any StrSlice symbol anywhere in repo =="
rg -n '\bStrSlice\b' shortcuts
echo "== record-ids / record-id around share-batch block =="
sed -n '300,360p' shortcuts/base/record_ops.goRepository: larksuite/cli
Length of output: 4201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== record-ids flag registration sites =="
rg -n -A3 -B3 'record-ids|StringArray\(|StringSlice\(' shortcuts/base shortcuts/common
echo "== surrounding command definition for share-batch =="
rg -n -A40 -B20 'share-batch|record-ids' shortcuts/base/record_ops.goRepository: larksuite/cli
Length of output: 5414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Any StrSlice method/identifier anywhere in repo =="
rg -n '\bStrSlice\b' .
echo "== RuntimeContext helper methods =="
sed -n '100,160p' shortcuts/common/runner.goRepository: larksuite/cli
Length of output: 2341
Use runtime.StrArray("record-ids") here — RuntimeContext only exposes StrArray, and record-ids is registered as a string-array flag. runtime.StrSlice(...) is undefined here and will block the build.
🤖 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/base/record_ops.go` around lines 344 - 355, The deduplication
helper is using the wrong RuntimeContext accessor, which breaks compilation
because record-ids is registered as a string-array flag and only StrArray is
available. Update deduplicateRecordIDs to read the input with
runtime.StrArray("record-ids") instead of runtime.StrSlice("record-ids"),
keeping the rest of the deduplication logic unchanged.
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
2 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.
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes