feat(base): support auto-number field reformat updates#1613
feat(base): support auto-number field reformat updates#1613yballul-bytedance wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds auto-number field update reformat support with normalized payload handling, legacy bitable reformat calls, no-op handling, and dry-run/E2E coverage. Also adds record-list page-size alias validation and expands shortcut guidance for field, record, and view commands. ChangesAuto-number field update flow
Record list pagination
Field and view help guidance
Sequence Diagram(s)sequenceDiagram
participant executeFieldUpdate
participant baseV3Raw
participant bitableV1Call
participant executeFieldGet
executeFieldUpdate->>baseV3Raw: PUT normalized update body
baseV3Raw-->>executeFieldUpdate: update result or noop error
executeFieldUpdate->>bitableV1Call: PUT legacy auto_number reformat body
bitableV1Call-->>executeFieldUpdate: reformat result
executeFieldUpdate->>executeFieldGet: re-fetch field after reformat
executeFieldGet-->>executeFieldUpdate: updated field output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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/base
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
shortcuts/base/base_execute_test.go (1)
894-900: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert typed error metadata here, not just the message text.
Both negative tests only check for
"no operation produced", so they will still pass if the command regresses to the wrong category/subtype or drops the wrapped cause. Please assert the typed problem directly once the error shape is fixed.As per coding guidelines,
**/*_test.go: Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone.Also applies to: 929-935
🤖 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_execute_test.go` around lines 894 - 900, The negative test around runShortcut/BaseFieldUpdate currently only checks the error string, so it can miss regressions in the typed error shape. Update this test to assert the returned error with errs.ProblemOf using the expected category, subtype, and param, and also verify the wrapped cause is preserved instead of relying on strings.Contains on "no operation produced". Keep the existing runShortcut and BaseFieldUpdate setup, but change the assertions to validate the typed metadata directly.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/base_shortcuts_test.go`:
- Around line 1238-1244: The test for BaseRecordList.Validate currently only
checks the error text, so update it to assert the typed validation contract
returned by validateLimitPageSizeAlias instead. Use errs.ProblemOf to verify the
validation category/subtype, and use errors.As into a *errs.ValidationError to
assert Param is "--page-size" rather than relying on strings.Contains. This
keeps the test anchored to BaseRecordList.Validate and the typed error path
instead of message wording.
In `@shortcuts/base/field_ops.go`:
- Around line 239-247: The auto-number reformat path in field update logic is
re-resolving the target from the stale field ref after the PUT, which breaks the
“ID or name” flow when the field is renamed. Update the logic in field_ops.go
around resolveLegacyFieldUpdateField and buildAutoNumberReformatBody to derive
the post-update target from the PUT response’s returned id/name instead of
calling back into the old ref, so the reformat step uses the updated field
identity.
- Around line 551-565: The legacy ID resolution helpers still return
command-facing failures with output.ErrWithHint, which breaks the typed errors
contract and lint expectations. Update resolveLegacyFieldUpdateTableID and
resolveLegacyFieldUpdateField to use errs.NewValidationError with
errs.SubtypeFailedPrecondition instead, then attach the retry message using
WithHint and identify the bad input with WithParam. Keep the existing fallback
logic in these helpers, and apply the same errs.* pattern to any similar legacy
error construction in field_ops.go.
- Around line 162-170: The not-found hint enrichment in
enrichFieldGetNotFoundHint is checking the wrong error type; replace the
output.ExitError assertion with errs.APIError and add the missing errs import.
Update the errors.As handling in enrichFieldGetNotFoundHint so it inspects
*errs.APIError, then use its Detail field (as a string) when deciding whether to
enrich the hint, keeping the existing early-return behavior for non-matching
errors.
---
Nitpick comments:
In `@shortcuts/base/base_execute_test.go`:
- Around line 894-900: The negative test around runShortcut/BaseFieldUpdate
currently only checks the error string, so it can miss regressions in the typed
error shape. Update this test to assert the returned error with errs.ProblemOf
using the expected category, subtype, and param, and also verify the wrapped
cause is preserved instead of relying on strings.Contains on "no operation
produced". Keep the existing runShortcut and BaseFieldUpdate setup, but change
the assertions to validate the typed metadata directly.
🪄 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: 5fb6e073-c243-4982-954d-3ff2c08fa5a7
📒 Files selected for processing (16)
shortcuts/base/base_dryrun_ops_test.goshortcuts/base/base_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/field_get.goshortcuts/base/field_ops.goshortcuts/base/field_update.goshortcuts/base/helpers.goshortcuts/base/record_upsert.goshortcuts/base/view_list.goshortcuts/base/view_set_visible_fields.goskills/lark-base/SKILL.mdskills/lark-base/references/lark-base-field-json.mdskills/lark-base/references/lark-base-field-update.mdskills/lark-base/references/lark-base-record-upsert.mdtests/cli_e2e/base/base_field_update_dryrun_test.gotests/cli_e2e/base/coverage.md
| if err := BaseRecordList.Validate(ctx, newBaseTestRuntime( | ||
| map[string]string{"base-token": "b", "table-id": "tbl_1"}, | ||
| nil, | ||
| map[string]int{"limit": 1, "page-size": 1}, | ||
| )); err == nil || !strings.Contains(err.Error(), "--limit and --page-size are mutually exclusive") { | ||
| t.Fatalf("err=%v", err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the typed validation contract, not just the message.
This new error-path test only checks a substring of err.Error(), so it would still pass if the validation category/subtype or param regressed. Since validateLimitPageSizeAlias returns a typed validation error, please assert the typed problem metadata and Param == "--page-size" here instead of relying on message text alone. Based on learnings, errs.ProblemOf exposes only problem-level fields, so read Param via errors.As(err, &ve) on *errs.ValidationError.
🤖 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_shortcuts_test.go` around lines 1238 - 1244, The test for
BaseRecordList.Validate currently only checks the error text, so update it to
assert the typed validation contract returned by validateLimitPageSizeAlias
instead. Use errs.ProblemOf to verify the validation category/subtype, and use
errors.As into a *errs.ValidationError to assert Param is "--page-size" rather
than relying on strings.Contains. This keeps the test anchored to
BaseRecordList.Validate and the typed error path instead of message wording.
Sources: Coding guidelines, Learnings
| func enrichFieldGetNotFoundHint(fieldRef string, err error) error { | ||
| fieldRef = strings.TrimSpace(fieldRef) | ||
| if !strings.HasPrefix(fieldRef, "fld") { | ||
| return err | ||
| } | ||
|
|
||
| var exitErr *output.ExitError | ||
| if !errors.As(err, &exitErr) || exitErr == nil || exitErr.Detail == nil { | ||
| return err |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
fd -t f 'output.go' | head -5 | while read f; do echo "=== $f ==="; cat -n "$f"; doneRepository: larksuite/cli
Length of output: 4494
🏁 Script executed:
grep -r "type.*ExitError" --include="*.go" . | grep -v test | head -10Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
grep -r "type.*ExitError" --include="*.go" shortcuts/internal/output shortcuts/internal/common 2>/dev/null | head -20Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
fd -t f 'output.go' --glob 'shortcuts/**/output.go' | while read f; do echo "=== $f ==="; cat -n "$f"; done 2>/dev/null | head -100Repository: larksuite/cli
Length of output: 261
🏁 Script executed:
grep -n "import.*output" shortcuts/base/field_ops.goRepository: larksuite/cli
Length of output: 151
🏁 Script executed:
grep -n "output\\.ExitError" shortcuts/base/field_ops.goRepository: larksuite/cli
Length of output: 221
🏁 Script executed:
head -30 shortcuts/base/field_ops.go | grep -E "import|output"Repository: larksuite/cli
Length of output: 204
🏁 Script executed:
fd -l shortcuts 2>/dev/null | grep '\.go$' | head -20Repository: larksuite/cli
Length of output: 1820
🏁 Script executed:
ls -la shortcuts/ 2>/dev/nullRepository: larksuite/cli
Length of output: 1697
🏁 Script executed:
cat internal/output/output.go 2>/dev/null | head -100Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
grep -n "type ExitError" internal/output/output.goRepository: larksuite/cli
Length of output: 210
🏁 Script executed:
grep -n "ExitError" internal/output/*.goRepository: larksuite/cli
Length of output: 151
🏁 Script executed:
fd -t f 'output.go' 'internal/output' 2>/dev/nullRepository: larksuite/cli
Length of output: 151
🏁 Script executed:
ls -la internal/output/ 2>/dev/nullRepository: larksuite/cli
Length of output: 2229
🏁 Script executed:
find internal/output -name "*.go" 2>/dev/null | head -10Repository: larksuite/cli
Length of output: 425
🏁 Script executed:
grep -rn "ExitError" internal/output/ 2>/dev/null | head -20Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
cat internal/output/errors.goRepository: larksuite/cli
Length of output: 3799
🏁 Script executed:
grep -n "type.*Error" internal/output/errors.goRepository: larksuite/cli
Length of output: 404
🏁 Script executed:
cat internal/output/exitcode.goRepository: larksuite/cli
Length of output: 2310
🏁 Script executed:
cat internal/output/bare.goRepository: larksuite/cli
Length of output: 949
🏁 Script executed:
grep -n "type.*Error.*struct" errs/*.go | head -20Repository: larksuite/cli
Length of output: 635
🏁 Script executed:
grep -rn "Detail" errs/*.go | head -10Repository: larksuite/cli
Length of output: 802
🏁 Script executed:
cat errs/types.go | head -100Repository: larksuite/cli
Length of output: 4250
🏁 Script executed:
grep -A 10 "type APIError struct" errs/types.goRepository: larksuite/cli
Length of output: 353
🏁 Script executed:
grep -A 20 "type.*struct" errs/types.go | grep -B 2 -A 2 "Detail"Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
head -20 shortcuts/base/field_ops.goRepository: larksuite/cli
Length of output: 553
Replace *output.ExitError with *errs.APIError and add the required import.
output.ExitError is not defined in the output package. The code attempts to access a Detail field to enrich a "not found" hint, which matches the errs.APIError type (embedding errs.Problem).
Add the import and update the type assertion:
import (
// ...
"github.com/larksuite/cli/errs"
// ...
)// Line 168 (and 263)
var exitErr *errs.APIError
if !errors.As(err, &exitErr) || exitErr == nil || exitErr.Detail == "" {
return err
}
// Use exitErr.Detail to enrich the hint🧰 Tools
🪛 GitHub Actions: CI / 10_security.txt
[error] 168-168: govulncheck failed: undefined identifier 'output.ExitError' (field_ops.go:168:22).
🪛 GitHub Actions: CI / 9_fast-gate.txt
[error] 168-168: go build failed: undefined: output.ExitError
🪛 GitHub Actions: CI / fast-gate
[error] 168-168: go build failed: undefined: output.ExitError
🪛 GitHub Actions: CI / security
[error] 168-168: govulncheck: loading packages failed due to compile error: undefined: output.ExitError
🪛 GitHub Actions: PR Preview Package / 0_publish.txt
[error] 168-168: Go build failed: undefined: output.ExitError
🪛 GitHub Actions: PR Preview Package / publish
[error] 168-168: Go build failed: undefined: output.ExitError
🪛 GitHub Check: fast-gate
[failure] 168-168:
undefined: output.ExitError
🪛 GitHub Check: publish
[failure] 168-168:
undefined: output.ExitError
🪛 GitHub Check: security
[failure] 168-168:
undefined: output.ExitError
🤖 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/field_ops.go` around lines 162 - 170, The not-found hint
enrichment in enrichFieldGetNotFoundHint is checking the wrong error type;
replace the output.ExitError assertion with errs.APIError and add the missing
errs import. Update the errors.As handling in enrichFieldGetNotFoundHint so it
inspects *errs.APIError, then use its Detail field (as a string) when deciding
whether to enrich the hint, keeping the existing early-return behavior for
non-matching errors.
Source: Linters/SAST tools
| tableIDResolved, err := resolveLegacyFieldUpdateTableID(runtime, baseToken, tableIDValue) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fieldIDResolved, fieldNameResolved, err := resolveLegacyFieldUpdateField(runtime, baseToken, tableIDResolved, fieldRef) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| legacyBody, err := buildAutoNumberReformatBody(normalized, fieldNameResolved) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve the reformat target from the PUT response, not the stale ref.
When --field-id is a field name and the update also renames that auto-number field, Line 243 re-resolves it by the old name after the PUT succeeds. That makes the reformat step fail on a supported “ID or name” path even though the update response already has the new id/name.
Suggested fix
tableIDResolved, err := resolveLegacyFieldUpdateTableID(runtime, baseToken, tableIDValue)
if err != nil {
return err
}
- fieldIDResolved, fieldNameResolved, err := resolveLegacyFieldUpdateField(runtime, baseToken, tableIDResolved, fieldRef)
- if err != nil {
- return err
- }
+ fieldIDResolved := strings.TrimSpace(common.GetString(data, "id"))
+ fieldNameResolved := strings.TrimSpace(common.GetString(data, "name"))
+ if fieldIDResolved == "" || fieldNameResolved == "" {
+ fieldIDResolved, fieldNameResolved, err = resolveLegacyFieldUpdateField(runtime, baseToken, tableIDResolved, fieldRef)
+ if err != nil {
+ return err
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tableIDResolved, err := resolveLegacyFieldUpdateTableID(runtime, baseToken, tableIDValue) | |
| if err != nil { | |
| return err | |
| } | |
| fieldIDResolved, fieldNameResolved, err := resolveLegacyFieldUpdateField(runtime, baseToken, tableIDResolved, fieldRef) | |
| if err != nil { | |
| return err | |
| } | |
| legacyBody, err := buildAutoNumberReformatBody(normalized, fieldNameResolved) | |
| tableIDResolved, err := resolveLegacyFieldUpdateTableID(runtime, baseToken, tableIDValue) | |
| if err != nil { | |
| return err | |
| } | |
| fieldIDResolved := strings.TrimSpace(common.GetString(data, "id")) | |
| fieldNameResolved := strings.TrimSpace(common.GetString(data, "name")) | |
| if fieldIDResolved == "" || fieldNameResolved == "" { | |
| fieldIDResolved, fieldNameResolved, err = resolveLegacyFieldUpdateField(runtime, baseToken, tableIDResolved, fieldRef) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| legacyBody, err := buildAutoNumberReformatBody(normalized, fieldNameResolved) |
🤖 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/field_ops.go` around lines 239 - 247, The auto-number reformat
path in field update logic is re-resolving the target from the stale field ref
after the PUT, which breaks the “ID or name” flow when the field is renamed.
Update the logic in field_ops.go around resolveLegacyFieldUpdateField and
buildAutoNumberReformatBody to derive the post-update target from the PUT
response’s returned id/name instead of calling back into the old ref, so the
reformat step uses the updated field identity.
| func resolveLegacyFieldUpdateTableID(runtime *common.RuntimeContext, baseToken string, tableRef string) (string, error) { | ||
| if strings.HasPrefix(strings.TrimSpace(tableRef), "tbl") { | ||
| return tableRef, nil | ||
| } | ||
| table, err := baseV3Call(runtime, "GET", baseV3Path("bases", baseToken, "tables", tableRef), nil, nil) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if tableID := strings.TrimSpace(common.GetString(table, "id")); tableID != "" { | ||
| return tableID, nil | ||
| } | ||
| if tableID := strings.TrimSpace(common.GetString(table, "table_id")); tableID != "" { | ||
| return tableID, nil | ||
| } | ||
| return "", output.ErrWithHint(output.ExitAPI, "api_error", fmt.Sprintf("resolved table %q but the response did not include a table id", tableRef), "Retry with the table ID from +table-list or +table-get.") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg --type go -A 5 "errs\.(New|Build|Error)" shortcuts/ --type-add 'go:*.go' | head -100Repository: larksuite/cli
Length of output: 8464
🏁 Script executed:
ast-grep outline shortcuts/base/field_ops.goRepository: larksuite/cli
Length of output: 2753
🏁 Script executed:
rg "import.*errs" shortcuts/base/field_ops.go -A 10Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
fd --extension go shortcuts/ | xargs rg "output\.Err" | head -50Repository: larksuite/cli
Length of output: 5317
🏁 Script executed:
rg "errs\.SubtypeInvalidData" shortcuts/ --type-add 'go:*.go' -A 2 -B 2Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
rg "errs\.New.*Error.*Subtype" shortcuts/ --type-add 'go:*.go' | head -30Repository: larksuite/cli
Length of output: 4671
🏁 Script executed:
cat -n shortcuts/base/field_ops.go | sed -n '1,30p'Repository: larksuite/cli
Length of output: 1078
🏁 Script executed:
cat -n shortcuts/base/field_ops.go | sed -n '551-585p'Repository: larksuite/cli
Length of output: 203
🏁 Script executed:
rg "errs\.SubtypeInvalidResponse" --type-add 'go:*.go' -A 2 -B 2 | head -40Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
sed -n '551,585p' shortcuts/base/field_ops.goRepository: larksuite/cli
Length of output: 1819
🏁 Script executed:
rg "SubtypeInvalidResponse" --type-add 'go:*.go' -A 2 -B 2 | head -50Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
rg "errs\.New.*Error\(" --type-add 'go:*.go' -A 3 | grep -A 3 "NewInternalError\|NewValidationError" | head -60Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
rg "internal/errs\." --type-add 'go:*.go' -A 5 | head -50Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
find . -path ./vend* -prune -o -path ./third* -prune -o -name "errs*.go" -type f -print | head -20Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
ls -la errs/Repository: larksuite/cli
Length of output: 1360
🏁 Script executed:
cat errs/typed.go | head -100Repository: larksuite/cli
Length of output: 197
🏁 Script executed:
cat errs/subtypes.goRepository: larksuite/cli
Length of output: 5947
🏁 Script executed:
cat errs/types.go | head -150Repository: larksuite/cli
Length of output: 5435
🏁 Script executed:
rg "FailedPrecondition" --type-add 'go:*.go' -A 3 -B 3 | head -40Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
grep -r "did not include" shortcuts/ --include="*.go"Repository: larksuite/cli
Length of output: 994
Replace legacy output.Err* with typed errs.* errors in legacy ID resolution helpers
The resolveLegacyFieldUpdateTableID and resolveLegacyFieldUpdateField functions construct command-facing failures using output.ErrWithHint(...), which violates the shortcuts error contract. This downgrades typed metadata that callers and tests rely on and will fail the errs-typed-only lint rule.
Replace both occurrences with errs.NewValidationError(errs.SubtypeFailedPrecondition, ...) and attach retry guidance via .WithHint(...). Name the failed input parameter with .WithParam(...).
// Lines 563-565
return "", errs.NewValidationError(errs.SubtypeFailedPrecondition,
"resolved table %q but the response did not include a table id", tableRef).
WithHint("Retry with the table ID from +table-list or +table-get.").
WithParam("table-ref")// Lines 579-580
return "", "", errs.NewValidationError(errs.SubtypeFailedPrecondition,
"resolved field %q but the response did not include a field id", fieldRef).
WithHint("Retry with the field ID from +field-list or +field-get.").
WithParam("field-ref")Apply the same pattern to any similar legacy error constructions in this file.
🤖 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/field_ops.go` around lines 551 - 565, The legacy ID resolution
helpers still return command-facing failures with output.ErrWithHint, which
breaks the typed errors contract and lint expectations. Update
resolveLegacyFieldUpdateTableID and resolveLegacyFieldUpdateField to use
errs.NewValidationError with errs.SubtypeFailedPrecondition instead, then attach
the retry message using WithHint and identify the bad input with WithParam. Keep
the existing fallback logic in these helpers, and apply the same errs.* pattern
to any similar legacy error construction in field_ops.go.
Sources: Coding guidelines, Learnings
1 participant
Summary
Add Base field update support for auto-number reformat flows and improve agent guidance around field, record, and view operations.
Changes
--reformat-existing-recordshandling forbase +field-update, including normalized auto-number JSON and the required bitable v1 reformat request.Test Plan
git diff --checkRelated Issues
Auto research task: 01KVA60JY5PVDDC09WA5QEY6P9
Summary by CodeRabbit
New Features
--page-sizeand its alias behavior.Bug Fixes
--limitand--page-sizeare used together.