feat(base): emit typed error envelopes across the base domain#1248
Merged
Conversation
github-actions
Bot
added
domain/base
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates shortcuts/base from legacy output-based errors to typed errs, expands lint scopes to include shortcuts/base, adds shared base error and API-envelope helpers, updates many shortcut validation and API-response paths to use the new helpers, and updates tests to assert typed problems and structured partial-failure output. ChangesError handling framework migration and linting scopes
Flag and validation error migration across shortcuts
API response handling refactor
Test assertion refactoring and helper integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@75533a59ffe778784bda27081578a3878823d7cb🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-base -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1248 +/- ##
==========================================
- Coverage 70.18% 70.17% -0.01%
==========================================
Files 671 671
Lines 65240 65329 +89
==========================================
+ Hits 45786 45847 +61
- Misses 15783 15794 +11
- Partials 3671 3688 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
shortcuts/base/helpers.go (1)
422-426:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReattach
x-tt-logidbefore returning the decoded Base envelope.After
runtime.ClassifyAPIResponse(resp)succeeds, this path throws away its parsed state and re-decodesresp.RawBody. If the server only sent the log ID in the header, the returned map no longer carries it, so laterhandleBaseAPIResultAny/baseAPIErrorFromResultcalls cannot surfacelog_idfor ordinary Base business errors.attachBaseErrorLogID()is still available just below and should run here before returning. Based on learnings: typed Base errors should preserve structured machine-readable fields likelog_idacross migration.Proposed fix
result, parseErr := decodeBaseV3Response(resp.RawBody) if parseErr != nil { return nil, parseErr } + attachBaseErrorLogID(result, baseResponseLogID(resp)) return result, nil }🤖 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/helpers.go` around lines 422 - 426, The decoded Base envelope from decodeBaseV3Response(resp.RawBody) loses the header-only x-tt-logid after runtime.ClassifyAPIResponse(resp) succeeds; call attachBaseErrorLogID(result, resp) (or the existing attachBaseErrorLogID helper) on the resulting map before returning so the structured result preserves log_id for downstream functions like handleBaseAPIResultAny and baseAPIErrorFromResult; ensure you run attachBaseErrorLogID immediately after parseErr check and before the final return.shortcuts/base/record_markdown.go (1)
32-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow
--jqwith explicit--format json.Line 33 keys off
runtime.Changed("format"), so--format json --jq ...now hits the markdown-only conflict even though JSON is the compatible fallback. Only reject when the explicit format ismarkdown; otherwise keep the JSONruntime.Out(...)path.Proposed fix
if runtime.JqExpr != "" { - if !runtime.Changed("format") { - runtime.Out(data, nil) - return nil - } - return baseValidationErrorf("--jq and --format markdown are mutually exclusive") + if runtime.Changed("format") && runtime.Str("format") == "markdown" { + return baseValidationErrorf("--jq and --format markdown are mutually exclusive") + } + runtime.Out(data, nil) + return nil }🤖 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_markdown.go` around lines 32 - 38, The current check rejects any use of runtime.JqExpr when runtime.Changed("format") is false, causing valid combinations like --format json --jq ... to be rejected; update the logic so that you only return baseValidationErrorf("--jq and --format markdown are mutually exclusive") when the user explicitly set the format to "markdown". Concretely, in the block referencing runtime.JqExpr and runtime.Changed("format"), change the condition to detect an explicit "markdown" choice (e.g. check the runtime's format field or accessor, such as runtime.Format or runtime.Get("format") depending on available API) and only error when that explicit value equals "markdown"; otherwise call runtime.Out(data, nil) and return nil as before.shortcuts/base/record_upload_attachment.go (1)
241-250:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve path-validation failures when checking
--output.This branch turns every
Staterror into “must be an existing directory”. Ifruntime.FileIO().Stat()rejects an unsafe path, the caller loses the real typed validation detail and gets a misleading directory message instead.💡 Proposed fix
if len(tokens) != 1 { info, statErr := runtime.FileIO().Stat(runtime.Str("output")) - if statErr != nil || !info.IsDir() { + if statErr != nil { + if errors.Is(statErr, fileio.ErrPathValidation) { + return baseValidationErrorf("unsafe output path: %s", statErr) + } + return baseFlagErrorf("--output must be an existing directory when downloading multiple attachments or when --file-token is omitted") + } + if !info.IsDir() { return baseFlagErrorf("--output must be an existing directory when downloading multiple attachments or when --file-token is omitted") } }Based on learnings: when checking whether a user-supplied
--outputpath is an existing directory, this repo distinguishesfs.ErrNotExistfromfileio.PathValidationErrorand other filesystem errors instead of collapsing them into one generic 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/record_upload_attachment.go` around lines 241 - 250, In validateRecordDownloadAttachment, preserve and propagate specific filesystem errors from runtime.FileIO().Stat instead of masking them; call runtime.FileIO().Stat(runtime.Str("output")) and if it returns an error check for fs.ErrNotExist and fileio.PathValidationError (or the repo's PathValidationError type) separately: if fs.ErrNotExist then return baseFlagErrorf("--output must be an existing directory when downloading multiple attachments or when --file-token is omitted"), if it's a PathValidationError return that error (or wrap/preserve it) so callers see validation failures, otherwise handle other unexpected errors by returning them rather than always returning the generic directory message; keep the existing branch that checks !info.IsDir() and returns the same baseFlagErrorf.shortcuts/base/record_ops.go (1)
332-353:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim
--record-idsentries before the empty-check.
validateRecordShareBatchnow rejects an empty slice, butdeduplicateRecordIDs()still treats" "as a valid ID because it only filters exact empty strings. That lets whitespace-only--record-idsbypass validation and reachshare_links/batchas invalid record IDs.💡 Proposed fix
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 { + id = strings.TrimSpace(id) if id != "" && !seen[id] { seen[id] = true result = append(result, id) } } return result }🤖 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 332 - 353, The deduplication currently treats whitespace-only entries as valid; modify deduplicateRecordIDs to trim each entry using strings.TrimSpace before checking emptiness and before using it as the map key/append value so that entries like " " are treated as empty and filtered out; ensure you import "strings" if necessary and keep the rest of the logic (seen map, result append) operating on the trimmedID.
🤖 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_errors.go`:
- Around line 175-180: The merge logic in the block using
errs.ProblemOf(enriched) and errs.ProblemOf(err) overwrites an existing
dst.LogID with an empty src.LogID; change the assignment so dst.LogID is only
set when src.LogID is non-empty (i.e., after confirming src.LogID != ""), while
still copying Message and Hint unconditionally; update the block that references
src, dst, dst.Message, dst.Hint, and dst.LogID to preserve an existing dst.LogID
when enriched provides no log id.
- Around line 38-42: Reject non-numeric "code" values before treating code==0 as
success: verify the value in resultMap["code"] is actually numeric (or that
util.ToFloat64 succeeded) and return an InvalidResponse/InternalError when
conversion fails or the value is not a number. Replace the current blind call
that lets util.ToFloat64 fall back to 0 by checking existence and conversion
result (e.g., capture any error/ok from util.ToFloat64 or use a type switch on
resultMap["code"]) and only then compare the numeric code; if conversion fails,
return the invalid-response error instead of proceeding with code==0 logic.
In `@shortcuts/base/base_form_submit.go`:
- Line 197: The current return uses baseValidationErrorf for a missing FileIO
provider which is a runtime/setup issue; change the return to an internal error
so callers can distinguish internal runtime failures from user validation
errors. Locate the check that returns baseValidationErrorf("file operations
require a FileIO provider (needed for attachments in --json)") (in
base_form_submit.go near the FileIO use) and replace it with the project’s
internal error constructor (e.g., internalErrorf or the InternalError type
factory used elsewhere) with a similar descriptive message so the error envelope
and exit code reflect an internal/runtime failure rather than validation.
In `@shortcuts/base/base_role_common.go`:
- Around line 21-24: The early return in handleRoleAPIResponse loses the action
context; change the error path so the error returned from
runtime.ClassifyAPIResponse is enriched via enrichBaseAPIErrorFromBody and then
wrapped/prefixed with baseRoleAPIError using the action string. Specifically, in
handleRoleAPIResponse replace the current return enrichBaseAPIErrorFromBody(err,
apiResp.RawBody, runtime.APIClassifyContext()) with a call that first enriches
the error (using enrichBaseAPIErrorFromBody) and then passes that result into
baseRoleAPIError(..., action) so the final error preserves the "create role
failed"/"get role failed" prefix.
In `@shortcuts/base/record_upload_attachment.go`:
- Around line 378-380: The error message for exceeding
baseAttachmentUploadMaxFileSize uses common.FormatSize(fileInfo.Size()) but
drops the offending file path; update the baseValidationErrorf call so it
includes the file path (use the variable that holds the uploaded file path/name
— e.g., filePath, filename, or file.Name()) along with the formatted size, by
changing the message to something like "file %s (%s) exceeds 2GB limit" and
passing the path and common.FormatSize(fileInfo.Size())); ensure you modify the
call at the check that references baseAttachmentUploadMaxFileSize and
common.FormatSize(fileInfo.Size()).
---
Outside diff comments:
In `@shortcuts/base/helpers.go`:
- Around line 422-426: The decoded Base envelope from
decodeBaseV3Response(resp.RawBody) loses the header-only x-tt-logid after
runtime.ClassifyAPIResponse(resp) succeeds; call attachBaseErrorLogID(result,
resp) (or the existing attachBaseErrorLogID helper) on the resulting map before
returning so the structured result preserves log_id for downstream functions
like handleBaseAPIResultAny and baseAPIErrorFromResult; ensure you run
attachBaseErrorLogID immediately after parseErr check and before the final
return.
In `@shortcuts/base/record_markdown.go`:
- Around line 32-38: The current check rejects any use of runtime.JqExpr when
runtime.Changed("format") is false, causing valid combinations like --format
json --jq ... to be rejected; update the logic so that you only return
baseValidationErrorf("--jq and --format markdown are mutually exclusive") when
the user explicitly set the format to "markdown". Concretely, in the block
referencing runtime.JqExpr and runtime.Changed("format"), change the condition
to detect an explicit "markdown" choice (e.g. check the runtime's format field
or accessor, such as runtime.Format or runtime.Get("format") depending on
available API) and only error when that explicit value equals "markdown";
otherwise call runtime.Out(data, nil) and return nil as before.
In `@shortcuts/base/record_ops.go`:
- Around line 332-353: The deduplication currently treats whitespace-only
entries as valid; modify deduplicateRecordIDs to trim each entry using
strings.TrimSpace before checking emptiness and before using it as the map
key/append value so that entries like " " are treated as empty and filtered
out; ensure you import "strings" if necessary and keep the rest of the logic
(seen map, result append) operating on the trimmedID.
In `@shortcuts/base/record_upload_attachment.go`:
- Around line 241-250: In validateRecordDownloadAttachment, preserve and
propagate specific filesystem errors from runtime.FileIO().Stat instead of
masking them; call runtime.FileIO().Stat(runtime.Str("output")) and if it
returns an error check for fs.ErrNotExist and fileio.PathValidationError (or the
repo's PathValidationError type) separately: if fs.ErrNotExist then return
baseFlagErrorf("--output must be an existing directory when downloading multiple
attachments or when --file-token is omitted"), if it's a PathValidationError
return that error (or wrap/preserve it) so callers see validation failures,
otherwise handle other unexpected errors by returning them rather than always
returning the generic directory message; keep the existing branch that checks
!info.IsDir() and returns the same baseFlagErrorf.
🪄 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: d253ee83-cceb-44c3-ba22-e69b14795bda
📒 Files selected for processing (36)
.golangci.ymllint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/base/base_advperm_disable.goshortcuts/base/base_advperm_enable.goshortcuts/base/base_advperm_test.goshortcuts/base/base_data_query.goshortcuts/base/base_errors.goshortcuts/base/base_errors_test.goshortcuts/base/base_execute_test.goshortcuts/base/base_form_questions_create.goshortcuts/base/base_form_questions_delete.goshortcuts/base/base_form_questions_update.goshortcuts/base/base_form_submit.goshortcuts/base/base_role_common.goshortcuts/base/base_role_create.goshortcuts/base/base_role_delete.goshortcuts/base/base_role_get.goshortcuts/base/base_role_list.goshortcuts/base/base_role_test.goshortcuts/base/base_role_update.goshortcuts/base/base_shortcut_helpers.goshortcuts/base/dashboard_block_create.goshortcuts/base/field_ops.goshortcuts/base/helpers.goshortcuts/base/record_markdown.goshortcuts/base/record_markdown_test.goshortcuts/base/record_ops.goshortcuts/base/record_query.goshortcuts/base/record_upload_attachment.goshortcuts/base/table_ops.goshortcuts/base/workflow_create.goshortcuts/base/workflow_disable.goshortcuts/base/workflow_enable.goshortcuts/base/workflow_get.goshortcuts/base/workflow_list.goshortcuts/base/workflow_update.go
evandance
force-pushed
the
feat/errs-migrate-base
branch
2 times, most recently
from
611b727 to
ae8417f
Compare
evandance
changed the title
evandance
force-pushed
the
feat/errs-migrate-base
branch
from
ae8417f to
306a8a7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/base/base_execute_test.go (1)
517-574: ⚡ Quick winCover the blank
--typevalidation branch too.This table misses the other changed
validateBaseBlockCreatepath:--typeblank now also routes throughbaseFlagErrorf. Adding a"create blank type"case would keep both modified branches inshortcuts/base/base_block_ops.gounder typed-error assertions.🤖 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 517 - 574, Add a new test case in TestBaseBlockValidationReturnsTypedErrors to cover the blank --type branch: add an entry with name "create blank type", shortcut BaseBaseBlockCreate, args []string{"+base-block-create", "--base-token", "app_x", "--type", " " , "--name", "doc"} (or similar required flags) and params []string{"--type"} so the test exercises validateBaseBlockCreate's --type blank path and asserts the same typed ValidationError checks already present in the loop.
🤖 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.
Nitpick comments:
In `@shortcuts/base/base_execute_test.go`:
- Around line 517-574: Add a new test case in
TestBaseBlockValidationReturnsTypedErrors to cover the blank --type branch: add
an entry with name "create blank type", shortcut BaseBaseBlockCreate, args
[]string{"+base-block-create", "--base-token", "app_x", "--type", " " ,
"--name", "doc"} (or similar required flags) and params []string{"--type"} so
the test exercises validateBaseBlockCreate's --type blank path and asserts the
same typed ValidationError checks already present in the loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 616269d7-f008-4476-8dc3-0ddf849be70a
📒 Files selected for processing (37)
.golangci.ymllint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/base/base_advperm_disable.goshortcuts/base/base_advperm_enable.goshortcuts/base/base_advperm_test.goshortcuts/base/base_block_ops.goshortcuts/base/base_data_query.goshortcuts/base/base_errors.goshortcuts/base/base_errors_test.goshortcuts/base/base_execute_test.goshortcuts/base/base_form_questions_create.goshortcuts/base/base_form_questions_delete.goshortcuts/base/base_form_questions_update.goshortcuts/base/base_form_submit.goshortcuts/base/base_role_common.goshortcuts/base/base_role_create.goshortcuts/base/base_role_delete.goshortcuts/base/base_role_get.goshortcuts/base/base_role_list.goshortcuts/base/base_role_test.goshortcuts/base/base_role_update.goshortcuts/base/base_shortcut_helpers.goshortcuts/base/dashboard_block_create.goshortcuts/base/field_ops.goshortcuts/base/helpers.goshortcuts/base/record_markdown.goshortcuts/base/record_markdown_test.goshortcuts/base/record_ops.goshortcuts/base/record_query.goshortcuts/base/record_upload_attachment.goshortcuts/base/table_ops.goshortcuts/base/workflow_create.goshortcuts/base/workflow_disable.goshortcuts/base/workflow_enable.goshortcuts/base/workflow_get.goshortcuts/base/workflow_list.goshortcuts/base/workflow_update.go
✅ Files skipped from review due to trivial changes (2)
- shortcuts/base/workflow_list.go
- shortcuts/base/field_ops.go
🚧 Files skipped from review as they are similar to previous changes (30)
- lint/errscontract/rule_no_legacy_envelope_literal.go
- shortcuts/base/base_form_questions_update.go
- shortcuts/base/workflow_get.go
- shortcuts/base/workflow_create.go
- shortcuts/base/base_form_questions_delete.go
- shortcuts/base/base_advperm_disable.go
- shortcuts/base/base_role_create.go
- shortcuts/base/base_role_list.go
- shortcuts/base/base_form_questions_create.go
- shortcuts/base/workflow_disable.go
- shortcuts/base/base_advperm_test.go
- shortcuts/base/base_role_delete.go
- .golangci.yml
- shortcuts/base/base_data_query.go
- shortcuts/base/table_ops.go
- shortcuts/base/base_shortcut_helpers.go
- shortcuts/base/record_markdown_test.go
- shortcuts/base/base_role_get.go
- shortcuts/base/base_role_update.go
- shortcuts/base/record_query.go
- shortcuts/base/record_ops.go
- shortcuts/base/base_form_submit.go
- shortcuts/base/base_role_common.go
- shortcuts/base/dashboard_block_create.go
- shortcuts/base/base_role_test.go
- shortcuts/base/base_errors.go
- shortcuts/base/record_markdown.go
- shortcuts/base/base_errors_test.go
- shortcuts/base/helpers.go
- shortcuts/base/record_upload_attachment.go
evandance
force-pushed
the
feat/errs-migrate-base
branch
2 times, most recently
from
e745454 to
8563b1f
Compare
evandance
force-pushed
the
feat/errs-migrate-base
branch
from
8563b1f to
75533a5
Compare
kongenpei
approved these changes
Jun 5, 2026
8 tasks
tuxedomm
pushed a commit
to zhumiaoxin/cli
that referenced
this pull request
Jun 6, 2026
This was referenced Jun 9, 2026
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
Migrate Base shortcut errors to typed errs.* envelopes so validation, API, network, internal, policy, and partial-failure paths expose structured machine-readable fields.
Changes
Test Plan
Related Issues
None