feat(apps): miaoda db (data/audit/env/recovery/quota) + file storage commands#1530
feat(apps): miaoda db (data/audit/env/recovery/quota) + file storage commands#1530chenxingyang1019 wants to merge 10 commits into
Conversation
…commands Add the second batch of apps db commands — data import/export, DDL changelog, row-level audit (status/enable/disable/list), dev→online env diff/migrate, point-in-time recovery diff/apply, and db quota — plus the full file-storage command set (list/get/sign/download/upload/delete/quota-get). Consolidate the apps skill references: one lark-apps-db.md for the whole db domain (db-execute kept separate) and one lark-apps-file.md for storage; drop the per-command db reference files and update SKILL.md routing. Change-Id: Ica42c0021332c3dbbf7eb20cab6fff18f5ef67ba
Run gofmt on the new apps test files, and mark the file-storage presigned-URL transfer (upload PUT / download GET) with //nolint:forbidigo — those bypass the Lark gateway and legitimately need raw HTTP, which the shortcuts-no-raw-http rule otherwise forbids. Change-Id: Ie2a635932f0434f54c66e2551a22b921b9a5a67a
|
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:
📝 WalkthroughWalkthroughAdds DB audit, changelog, import/export, environment migration, recovery, quota, and file storage shortcuts plus shared helpers, registry wiring, runtime spinner support, tests, and docs. ChangesInfrastructure: Terminal Output
DB Operations
File Storage Operations
Shortcut Registration and Documentation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 28
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (7)
shortcuts/apps/apps_db_changelog_list_test.go-52-59 (1)
52-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert typed error metadata, not message substrings.
Per coding guidelines, error-path tests must assert typed metadata (
category,subtype,param) viaerrs.ProblemOf, not substring matching. Substring checks can miss regressions when error types or param names change.🧪 Proposed fix: assert validation error type and param
func TestAppsDBChangelogList_RejectsBadSince(t *testing.T) { factory, stdout, _ := newAppsExecuteFactory(t) err := runAppsShortcut(t, AppsDBChangelogList, []string{"+db-changelog-list", "--app-id", "app_x", "--since", "notatime", "--as", "user"}, factory, stdout) - if err == nil || !strings.Contains(err.Error(), "since") { - t.Fatalf("expected --since validation, got %v", err) + if err == nil { + t.Fatal("expected validation error for invalid --since") + } + var ve *errs.ValidationError + if !errors.As(err, &ve) || ve.Param != "--since" { + t.Fatalf("expected ValidationError with param=--since, got %v", err) } }As per coding guidelines: "Error-path tests must assert typed metadata via
errs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone" (**/*_test.gosection).🤖 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/apps/apps_db_changelog_list_test.go` around lines 52 - 59, The TestAppsDBChangelogList_RejectsBadSince test currently validates errors by checking if the error message contains the substring "since" using strings.Contains, which can miss regressions when error types or parameter names change. Instead, replace the substring check with typed error metadata assertions using errs.ProblemOf to verify the error's category, subtype, and param fields (where param should be "since"). This ensures the test validates the actual error structure rather than relying on message text.Source: Coding guidelines
shortcuts/apps/apps_db_changelog_list_test.go-16-23 (1)
16-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert typed error metadata, not message substrings.
Per coding guidelines, error-path tests must assert typed metadata (
category,subtype,param) viaerrs.ProblemOf, not substring matching. Substring checks can miss regressions when error types or param names change.🧪 Proposed fix: assert validation error type and param
+ "errors" + + "github.com/larksuite/cli/internal/errs" "github.com/larksuite/cli/internal/httpmock" ) const dbChangelogURL = "/open-apis/spark/v1/apps/app_x/db/changelog_list" func TestAppsDBChangelogList_RequiresAppID(t *testing.T) { factory, stdout, _ := newAppsExecuteFactory(t) err := runAppsShortcut(t, AppsDBChangelogList, []string{"+db-changelog-list", "--app-id", " ", "--as", "user"}, factory, stdout) - if err == nil || !strings.Contains(err.Error(), "app-id") { - t.Fatalf("expected app-id error, got %v", err) + if err == nil { + t.Fatal("expected validation error for missing app-id") + } + var ve *errs.ValidationError + if !errors.As(err, &ve) || ve.Param != "--app-id" { + t.Fatalf("expected ValidationError with param=--app-id, got %v", err) } }As per coding guidelines: "Error-path tests must assert typed metadata via
errs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone" (**/*_test.gosection).🤖 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/apps/apps_db_changelog_list_test.go` around lines 16 - 23, The test TestAppsDBChangelogList_RequiresAppID currently uses substring matching (strings.Contains on err.Error()) to validate the error, but per coding guidelines it must assert typed error metadata instead. Replace the substring check with a call to errs.ProblemOf(err) to extract the error's typed metadata, then assert the specific category, subtype, and param fields rather than checking if "app-id" appears in the error message. This ensures the test validates the actual error type structure and catches regressions when error types or parameter names change.Source: Coding guidelines
shortcuts/apps/apps_db_recovery.go-131-131 (1)
131-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace legacy
output.ErrAPIwith typed error constructor.Same issue as in
apps_db_env_migrate.go. Per coding guidelines, use typederrs.*errors instead of legacyoutput.Err*helpers.🤖 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/apps/apps_db_recovery.go` at line 131, Replace the legacy output.ErrAPI call in the return statement with a typed error constructor from the errs package. The output.ErrAPI(0, msg, nil) call should be changed to use the appropriate typed errs.* error function that matches the error type being returned. Ensure the error message and context are preserved while using the modern typed error approach, maintaining the withAppsHint wrapper around the new typed error.Source: Coding guidelines
shortcuts/apps/apps_db_recovery.go-176-176 (1)
176-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace legacy
output.ErrAPIwith typed error constructor.Same issue as lines 118 and 131. Use typed
errs.*error instead ofoutput.ErrAPI.🤖 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/apps/apps_db_recovery.go` at line 176, Replace the legacy output.ErrAPI call in the return statement that uses withAppsHint and dbRecoveryHint with a typed errs.* error constructor instead. This is consistent with the refactoring done at lines 118 and 131 in the same file. Determine the appropriate typed error constructor based on the error context and ensure it properly captures the error message and details before passing it to withAppsHint.Source: Coding guidelines
shortcuts/apps/apps_db_env_migrate.go-118-118 (1)
118-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace legacy
output.ErrAPIwith typed error constructor.Per coding guidelines, command-facing failures must use typed
errs.*errors, never legacyoutput.Err*helpers. The lint ruleforbidigo [errs-typed-only]should flag this usage inshortcuts/.Replace with a typed error constructor such as
errs.NewAPIError(...)orerrs.NewInternalError(...)with appropriate subtype.🤖 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/apps/apps_db_env_migrate.go` at line 118, Replace the legacy output.ErrAPI call in the return statement (where migrateFailMsg(d, taskID) is passed) with a typed error constructor from the errs package such as errs.NewAPIError or errs.NewInternalError, choosing the appropriate subtype based on whether this is an API-level failure or internal error. Maintain the same call structure by keeping this typed error constructor wrapped within the withAppsHint call to preserve the existing error handling and hint attachment logic.Source: Coding guidelines
shortcuts/apps/apps_file_delete_test.go-16-22 (1)
16-22: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTests should assert typed error metadata once production code is migrated.
This test currently checks for error string content (
strings.Contains(err.Error(), "path")), but per coding guidelines, error-path tests must assert typed metadata viaerrs.ProblemOf. Once line 43 inapps_file_delete.gois fixed to useerrs.NewValidationError, update this test to assert the error structure.🤖 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/apps/apps_file_delete_test.go` around lines 16 - 22, In the TestAppsFileDelete_RequiresAppIDAndPath test function, replace the string-based error check using strings.Contains(err.Error(), "path") with a typed error assertion using errs.ProblemOf to validate the error structure instead of its string content. This change should be made once the production code at line 43 in apps_file_delete.go is updated to use errs.NewValidationError, ensuring the test asserts the proper error type and metadata rather than relying on error message string matching.Source: Coding guidelines
shortcuts/apps/apps_file_upload_test.go-19-25 (1)
19-25: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTests should assert typed error metadata once production code is migrated.
These tests currently check for error string content (
strings.Contains(err.Error(), ...)), but per coding guidelines, error-path tests must assert typed metadata viaerrs.ProblemOf(category,subtype,param) and cause preservation, not message substrings alone.Once the production code violations (lines 52, 56, 59, 62 in
apps_file_upload.go) are fixed to use typed errors, update these tests to assert the error structure, e.g.:prob, ok := errs.ProblemOf(err) if !ok || prob.Subtype != errs.SubtypeInvalidArgument { t.Fatalf("expected typed validation error, got %v", err) } // For param assertions, use errors.As with *errs.ValidationErrorAlso applies to: 27-43
🤖 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/apps/apps_file_upload_test.go` around lines 19 - 25, The test function TestAppsFileUpload_RequiresAppIDAndFile currently asserts errors by checking error message strings with strings.Contains instead of asserting typed error metadata. Replace the string-based error checking with typed error assertions using errs.ProblemOf to verify the error's Subtype matches errs.SubtypeInvalidArgument and other typed properties. Additionally, use errors.As to check for specific error types like *errs.ValidationError when validating parameter-specific errors. Apply this approach to all related test functions that currently use string matching for error validation.Source: Coding guidelines
🟡 Minor comments (1)
shortcuts/apps/apps_db_quota_get.go-51-51 (1)
51-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect error hint for quota retrieval.
dbEnvMigrateHintinstructs users to "ensure the app is multi-env and has pending dev changes; preview with +db-env-diff", which is unrelated to quota retrieval. This appears to be a copy-paste error from the migrate shortcuts.Either define a quota-specific hint (e.g., checking app permissions or quota provisioning status) or pass an empty string to
withAppsHint(err, "")if no specific recovery guidance applies.🤖 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/apps/apps_db_quota_get.go` at line 51, The withAppsHint function in the quota retrieval code path is using dbEnvMigrateHint, which provides guidance about multi-env and dev changes that is unrelated to quota operations. This appears to be a copy-paste error. Replace the dbEnvMigrateHint argument in the withAppsHint call with either a quota-specific hint that provides relevant recovery guidance (such as checking app permissions or quota provisioning status), or use an empty string if no specific recovery guidance is applicable to quota retrieval errors.
🧹 Nitpick comments (3)
shortcuts/apps/apps_db_env_recovery_quota_test.go (2)
208-223: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd typed error metadata assertions.
Same issue as the migrate test: Line 220 only checks error message substring. Add assertions for error type/category/subtype to verify proper typed error wrapping.
🤖 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/apps/apps_db_env_recovery_quota_test.go` around lines 208 - 223, In the TestAppsDBRecoveryDiff_PreviewFailed test function, enhance the error validation by adding typed error metadata assertions in addition to the existing substring check. After verifying the error message contains "snapshot expired", add assertions to check that the returned error has the correct typed error properties such as error type, category, and subtype to ensure proper error wrapping is applied throughout the call chain.Source: Coding guidelines
121-136: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd typed error metadata assertions.
Per coding guidelines, error-path tests should assert typed metadata via
errs.ProblemOf(category/subtype/param), not message substrings alone. Currently Line 133 only checks if the error message contains "lock timeout".Add assertions for error type, category, and subtype to verify the CLI properly wraps backend failures as typed errors.
🤖 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/apps/apps_db_env_recovery_quota_test.go` around lines 121 - 136, In the TestAppsDBEnvMigrate_PollFailedSurfacesError test function, replace the error assertion that uses strings.Contains(err.Error(), "lock timeout") with assertions using errs.ProblemOf to verify the typed metadata of the returned error. Assert the error's category, subtype, and relevant parameters to ensure the CLI properly wraps the backend failure as a typed error rather than only checking the raw error message string.Source: Coding guidelines
shortcuts/apps/apps_file_download_test.go (1)
21-27: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider asserting typed error metadata for the required-path validation.
Line 24 validates the error using
strings.Contains(err.Error(), "path"). Per coding guidelines, "Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param)". While substring matching may suffice for simple required-field checks, typed assertions improve test robustness.🧪 Optional improvement
+ "errors" + + "github.com/larksuite/cli/errs" + func TestAppsFileDownload_RequiresAppIDAndPath(t *testing.T) { factory, stdout, _ := newAppsExecuteFactory(t) - if err := runAppsShortcut(t, AppsFileDownload, - []string{"+file-download", "--app-id", "app_x", "--path", " ", "--as", "user"}, factory, stdout); err == nil || !strings.Contains(err.Error(), "path") { - t.Fatalf("expected path error, got %v", err) + err := runAppsShortcut(t, AppsFileDownload, + []string{"+file-download", "--app-id", "app_x", "--path", " ", "--as", "user"}, factory, stdout) + if err == nil { + t.Fatal("expected validation error for empty path") + } + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("expected ValidationError, got %T: %v", err, err) + } + if ve.Param != "--path" { + t.Errorf("expected param '--path', got %q", ve.Param) } }🤖 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/apps/apps_file_download_test.go` around lines 21 - 27, The test function TestAppsFileDownload_RequiresAppIDAndPath currently validates the error using string matching with strings.Contains(err.Error(), "path"), which is fragile. Replace this string-based assertion with typed error metadata assertion using errs.ProblemOf to check the error's category, subtype, and param fields, ensuring the validation specifically targets the "path" parameter. This provides more robust error validation aligned with coding guidelines for error-path tests.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/apps/apps_db_audit_list.go`:
- Line 50: Replace the `output.ErrValidation` call with
`errs.NewValidationError(errs.SubtypeInvalidArgument,
"message").WithParam("--table")` to comply with the shortcuts package coding
guidelines and pass the golangci-lint forbidigo rule. Additionally, add the
import statement `"github.com/larksuite/cli/internal/errs"` at the top of the
file to make the errs package available.
In `@shortcuts/apps/apps_db_data_export_test.go`:
- Around line 19-27: The error-path tests (TestAppsDBDataExport_RequiresTable
and the other error tests at lines 29-38, 40-47, and 161-178) are currently
asserting errors by checking message substrings, but per coding guidelines they
must assert typed error metadata instead. Replace the string-based error
validation with typed assertions using errs.ProblemOf to verify the category,
subtype, and param fields of the error, and also verify cause preservation. This
ensures the tests validate the structured error information rather than relying
on error message content.
In `@shortcuts/apps/apps_db_data_export.go`:
- Around line 112-114: The HTTP status code error handling in the conditional
block checking resp.StatusCode >= 400 uses the untyped output.ErrNetwork
function, which violates the typing guidelines for command-facing failures.
Replace the output.ErrNetwork call with errs.NewNetworkError using
errs.SubtypeNetworkTransport as the subtype parameter, and pass the same message
format with HTTP status code details to maintain the error context.
- Around line 103-105: In the error handling block where err is checked after
the export request (around the return statement with withAppsHint), replace the
output.ErrNetwork function call with errs.NewNetworkError using the
SubtypeNetworkTransport subtype, pass the descriptive error message as the first
argument after the subtype, and chain .WithCause(err) to preserve the underlying
error details. Ensure the entire errs.NewNetworkError result is still wrapped
with the withAppsHint function call as it currently is. Make sure to import the
errs package if it is not already imported in the file.
- Around line 116-118: Replace the output.ErrValidation call in the
dbDataExportMaxBytes size limit check with errs.NewValidationError instead. The
error message about the 1 MB limit should be the main error, and the recovery
guidance about filtering rows with +db-execute and exporting smaller subsets
should be moved to a .WithHint() method call. Since this is a server-side
resource limit being exceeded rather than invalid client input, use
SubtypeResourceExhausted as the subtype for the validation error.
- Around line 56-61: The validation error handling in this function uses the
legacy output.ErrValidation helper which violates the typed errors requirement.
Replace both output.ErrValidation calls on lines that validate the "table"
parameter and the "limit" parameter with appropriate typed errors from the errs
package instead. Ensure you import the necessary errs package at the top of the
file if not already present, and use the correct typed error constructor that
matches the validation failure being reported for each parameter check.
- Around line 124-126: The error handling block that wraps the result from
rctx.FileIO().Save() unconditionally wraps errors with output.ErrValidation
without first checking if the error is already a typed error from the FileIO
abstraction. Check the error type first: if it is already a typed error like
fileio.PathValidationError, pass it through unchanged; if it is not a typed
error, then wrap it with output.ErrValidation and use .WithCause(err) to chain
the original error for better context preservation.
In `@shortcuts/apps/apps_db_data_import_test.go`:
- Around line 29-38: Replace the string-based error assertions using
strings.Contains(err.Error(), ...) in the error-path tests
TestAppsDBDataImport_RequiresAppID and the other affected test functions (lines
40-49, 51-60, 62-72) with typed error assertions. Use errs.ProblemOf to extract
and assert the typed metadata (category, subtype, param) from the error instead
of checking message substrings. This aligns with the coding guidelines for
error-path testing and ensures the tests verify the actual error type structure
rather than relying on error message text.
In `@shortcuts/apps/apps_db_data_import.go`:
- Around line 60-62: Replace the output.ErrValidation call in the
importTableName validation block with errs.NewValidationError instead. The
validation error message should be passed to errs.NewValidationError, and the
recovery guidance text (about specifying --table) should be moved to a
.WithHint() method call chained on the error to follow the coding guidelines for
validation errors with recovery guidance.
- Around line 80-83: The error handling in the ReadInputFile call wraps the
error with output.ErrValidation, but according to coding guidelines, when a
lower layer already returns a typed error, it should be passed through
unchanged. Check if cmdutil.ReadInputFile already returns a typed error, and if
so, return it directly instead of wrapping it with output.ErrValidation. If
wrapping is necessary, use a typed error approach with .WithCause(err) for error
chaining instead of the output.Err* functions.
- Around line 53-55: Replace the legacy output.ErrValidation function call with
the typed validation error approach as per coding guidelines. In the validation
check where strings.TrimSpace(rctx.Str("file")) is tested, replace the return
statement that currently uses output.ErrValidation("--file is required") with
errs.NewValidationError("--file is required"). Ensure the errs package is
imported at the top of the file.
- Around line 101-103: In the error handling block where the DoAPI request
fails, replace the `output.ErrNetwork` call with
`errs.NewNetworkError(errs.SubtypeNetworkTransport, "import request failed")` to
use the typed network error as per coding guidelines, and chain
`.WithCause(err)` to preserve the underlying error details for debugging
purposes.
- Around line 84-86: In the validation check where len(content) exceeds
dbDataImportMaxBytes, replace the output.ErrValidation call with
errs.NewValidationError using errs.SubtypeResourceExhausted as the subtype. Move
the recovery guidance text (about splitting into chunks) from the main error
message into a .WithHint() method call chained to the NewValidationError call,
keeping only the core validation message about exceeding the 1 MB limit in the
main error constructor.
In `@shortcuts/apps/apps_file_delete.go`:
- Around line 38-46: In the Validate function within apps_file_delete.go,
replace the call to output.ErrValidation with the appropriate typed error from
the errs package. The validation check for the --path argument that currently
returns output.ErrValidation("--path is required (at least one remote path)")
must be changed to use a typed errs error instead to comply with the forbidigo
[errs-typed-only] lint rule for the shortcuts/ directory.
In `@shortcuts/apps/apps_file_download.go`:
- Around line 92-95: Replace the output.ErrNetwork call on line 94 with
errs.NewNetworkError to properly handle HTTP error responses. Change the error
handling in the status code check (if resp.StatusCode >= 400) to use
errs.NewNetworkError(errs.SubtypeNetworkTransport, "download failed: HTTP %d",
resp.StatusCode) instead of output.ErrNetwork("download failed: HTTP %d",
resp.StatusCode) to comply with the typed error handling guidelines for
network/transport failures.
- Around line 83-90: Replace the legacy output.ErrNetwork calls on lines 85 and
89 with the typed errs.NewNetworkError function. For both error handling blocks
(after http.NewRequestWithContext and after newFileTransferClient().Do), use
errs.NewNetworkError with errs.SubtypeNetworkTransport as the subtype parameter,
provide a descriptive message (such as "build download request" or "download
failed"), and chain the method WithCause(err) to preserve the underlying error
information rather than formatting it into the message string.
- Around line 96-102: The error handling after rctx.FileIO().Save()
indiscriminately wraps all errors as validation errors using
output.ErrValidation(). Instead, check the type of error returned and handle
appropriately: pass through typed errors like fileio.PathValidationError
unchanged or with .WithCause(), and classify I/O failures (disk space,
permissions) as errs.SubtypeFileIO instead of validation errors to preserve
proper error classification and underlying error details.
In `@shortcuts/apps/apps_file_list.go`:
- Around line 47-62: Replace the `output.ErrValidation("--%s: %v", f, err)` call
in the Validate function with the appropriate typed `errs.*` error from the errs
package. According to the coding guidelines and golangci-lint forbidigo rule,
command-facing failures must use typed errs.* errors instead of output.Err*
helpers. Ensure the replacement maintains the same error message format with the
flag name and error details.
In `@shortcuts/apps/apps_file_sign_test.go`:
- Around line 39-46: In the TestAppsFileSign_RejectsDurationOverMax function,
replace the current error validation using strings.Contains(err.Error(), "30d")
with proper typed error assertion using errs.ProblemOf to check the error's
category, subtype, and param metadata, and ensure cause preservation is verified
as required by the coding guidelines rather than asserting against error message
substrings.
In `@shortcuts/apps/apps_file_sign.go`:
- Around line 46-48: The validation error check in the expires-in validation
block uses the legacy output.ErrValidation helper instead of the typed
errs.NewValidationError required by coding guidelines. Replace the
output.ErrValidation call with errs.NewValidationError while preserving the
error message and fileSignMaxExpiresSeconds argument formatting.
In `@shortcuts/apps/apps_file_upload.go`:
- Around line 79-82: In the error handling block after calling
cmdutil.ReadInputFile in the file upload functionality, do not wrap the returned
error directly with output.ErrValidation. Instead, check if the error is already
a typed error such as fileio.PathValidationError or other errs.* types using
type assertion, and if it is, return it unchanged to preserve the error type
classification. Only wrap with output.ErrValidation if the error is not a typed
error. This ensures that errors from lower layers maintain their proper type
information for error handling downstream.
- Around line 46-65: Replace all four instances of output.ErrValidation calls in
the Validate function with typed errors using
errs.NewValidationError(errs.SubtypeInvalidArgument, ...). Specifically, update
the validation error returns at: the requireAppID check, the empty file
validation, the file stat error handling, the directory check, and the file size
limit check to use the typed error pattern instead of the legacy output.Err*
helpers, as required by the forbidigo linting rule for shortcuts code.
- Around line 130-150: Replace all three `output.ErrNetwork` calls in the
`putFileBytes` function with `errs.NewNetworkError(errs.SubtypeNetworkTransport,
...)` following the new coding guidelines. For the first two occurrences where
an `err` variable is available (in the http.NewRequestWithContext and client.Do
calls), chain the error using `.WithCause(err)` to preserve the underlying error
for proper error chain handling with errors.Is and errors.Unwrap. The third
occurrence (HTTP status code check) does not have an underlying error, so it
only needs the typed error call without WithCause.
In `@shortcuts/apps/db_common.go`:
- Line 44: Replace the legacy output.ErrNetwork call on line 44 with the typed
errs.NewNetworkError function. Change the error construction to use
errs.NewNetworkError with errs.SubtypeNetworkTransport as the subtype, include
the error message "cancelled while waiting" and preserve the underlying context
error by calling .WithCause(ctx.Err()) on the error returned from
errs.NewNetworkError to ensure proper error unwrapping and context preservation
for errors.Is and errors.Unwrap checks.
- Line 40: Replace the legacy output.ErrNetwork call on line 40 with the typed
errs.NewNetworkError function, passing errs.SubtypeNetworkTransport as the first
argument followed by the existing format string and maxWait argument.
Additionally, add the import statement "github.com/larksuite/cli/internal/errs"
to the imports section of the db_common.go file if it is not already present.
- Around line 179-181: The code uses output.ErrValidation() which violates the
coding guidelines requiring typed errs.* errors and will fail the golangci-lint
forbidigo [errs-typed-only] rule. Replace both output.ErrValidation() calls (the
ones with messages about unsupported data format for file extensions .csv,
.json, .sql) with errs.NewValidationError(errs.SubtypeInvalidArgument, ...)
while preserving the existing error message strings.
In `@shortcuts/apps/file_common.go`:
- Around line 176-190: In the normalizeTimeFlags function, replace the
output.ErrValidation call with a typed error from the errs package. Instead of
returning output.ErrValidation("--%s: %v", f, err), create and return an
appropriate typed error from the errs package that follows the typed error
patterns required by the forbidigo [errs-typed-only] linting rule. Ensure the
error message still includes the flag name and underlying error details for
proper debugging context.
- Around line 120-127: The requireFilePath function is currently using
output.ErrValidation which violates the coding guidelines requiring typed errs.*
errors for command-facing failures. Replace the output.ErrValidation call in the
requireFilePath function with the appropriate typed error from the errs package
that represents a validation failure, ensuring the error message "--path is
required" is preserved in the replacement.
---
Major comments:
In `@shortcuts/apps/apps_db_changelog_list_test.go`:
- Around line 52-59: The TestAppsDBChangelogList_RejectsBadSince test currently
validates errors by checking if the error message contains the substring "since"
using strings.Contains, which can miss regressions when error types or parameter
names change. Instead, replace the substring check with typed error metadata
assertions using errs.ProblemOf to verify the error's category, subtype, and
param fields (where param should be "since"). This ensures the test validates
the actual error structure rather than relying on message text.
- Around line 16-23: The test TestAppsDBChangelogList_RequiresAppID currently
uses substring matching (strings.Contains on err.Error()) to validate the error,
but per coding guidelines it must assert typed error metadata instead. Replace
the substring check with a call to errs.ProblemOf(err) to extract the error's
typed metadata, then assert the specific category, subtype, and param fields
rather than checking if "app-id" appears in the error message. This ensures the
test validates the actual error type structure and catches regressions when
error types or parameter names change.
In `@shortcuts/apps/apps_db_env_migrate.go`:
- Line 118: Replace the legacy output.ErrAPI call in the return statement (where
migrateFailMsg(d, taskID) is passed) with a typed error constructor from the
errs package such as errs.NewAPIError or errs.NewInternalError, choosing the
appropriate subtype based on whether this is an API-level failure or internal
error. Maintain the same call structure by keeping this typed error constructor
wrapped within the withAppsHint call to preserve the existing error handling and
hint attachment logic.
In `@shortcuts/apps/apps_db_recovery.go`:
- Line 131: Replace the legacy output.ErrAPI call in the return statement with a
typed error constructor from the errs package. The output.ErrAPI(0, msg, nil)
call should be changed to use the appropriate typed errs.* error function that
matches the error type being returned. Ensure the error message and context are
preserved while using the modern typed error approach, maintaining the
withAppsHint wrapper around the new typed error.
- Line 176: Replace the legacy output.ErrAPI call in the return statement that
uses withAppsHint and dbRecoveryHint with a typed errs.* error constructor
instead. This is consistent with the refactoring done at lines 118 and 131 in
the same file. Determine the appropriate typed error constructor based on the
error context and ensure it properly captures the error message and details
before passing it to withAppsHint.
In `@shortcuts/apps/apps_file_delete_test.go`:
- Around line 16-22: In the TestAppsFileDelete_RequiresAppIDAndPath test
function, replace the string-based error check using
strings.Contains(err.Error(), "path") with a typed error assertion using
errs.ProblemOf to validate the error structure instead of its string content.
This change should be made once the production code at line 43 in
apps_file_delete.go is updated to use errs.NewValidationError, ensuring the test
asserts the proper error type and metadata rather than relying on error message
string matching.
In `@shortcuts/apps/apps_file_upload_test.go`:
- Around line 19-25: The test function TestAppsFileUpload_RequiresAppIDAndFile
currently asserts errors by checking error message strings with strings.Contains
instead of asserting typed error metadata. Replace the string-based error
checking with typed error assertions using errs.ProblemOf to verify the error's
Subtype matches errs.SubtypeInvalidArgument and other typed properties.
Additionally, use errors.As to check for specific error types like
*errs.ValidationError when validating parameter-specific errors. Apply this
approach to all related test functions that currently use string matching for
error validation.
---
Minor comments:
In `@shortcuts/apps/apps_db_quota_get.go`:
- Line 51: The withAppsHint function in the quota retrieval code path is using
dbEnvMigrateHint, which provides guidance about multi-env and dev changes that
is unrelated to quota operations. This appears to be a copy-paste error. Replace
the dbEnvMigrateHint argument in the withAppsHint call with either a
quota-specific hint that provides relevant recovery guidance (such as checking
app permissions or quota provisioning status), or use an empty string if no
specific recovery guidance is applicable to quota retrieval errors.
---
Nitpick comments:
In `@shortcuts/apps/apps_db_env_recovery_quota_test.go`:
- Around line 208-223: In the TestAppsDBRecoveryDiff_PreviewFailed test
function, enhance the error validation by adding typed error metadata assertions
in addition to the existing substring check. After verifying the error message
contains "snapshot expired", add assertions to check that the returned error has
the correct typed error properties such as error type, category, and subtype to
ensure proper error wrapping is applied throughout the call chain.
- Around line 121-136: In the TestAppsDBEnvMigrate_PollFailedSurfacesError test
function, replace the error assertion that uses strings.Contains(err.Error(),
"lock timeout") with assertions using errs.ProblemOf to verify the typed
metadata of the returned error. Assert the error's category, subtype, and
relevant parameters to ensure the CLI properly wraps the backend failure as a
typed error rather than only checking the raw error message string.
In `@shortcuts/apps/apps_file_download_test.go`:
- Around line 21-27: The test function TestAppsFileDownload_RequiresAppIDAndPath
currently validates the error using string matching with
strings.Contains(err.Error(), "path"), which is fragile. Replace this
string-based assertion with typed error metadata assertion using errs.ProblemOf
to check the error's category, subtype, and param fields, ensuring the
validation specifically targets the "path" parameter. This provides more robust
error validation aligned with coding guidelines for error-path tests.
🪄 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: a5bfcc36-f0b6-4d2a-925c-d36f3c203928
📒 Files selected for processing (38)
shortcuts/apps/apps_db_audit_list.goshortcuts/apps/apps_db_audit_set.goshortcuts/apps/apps_db_audit_status.goshortcuts/apps/apps_db_audit_test.goshortcuts/apps/apps_db_changelog_list.goshortcuts/apps/apps_db_changelog_list_test.goshortcuts/apps/apps_db_data_export.goshortcuts/apps/apps_db_data_export_test.goshortcuts/apps/apps_db_data_import.goshortcuts/apps/apps_db_data_import_test.goshortcuts/apps/apps_db_env_migrate.goshortcuts/apps/apps_db_env_recovery_quota_test.goshortcuts/apps/apps_db_quota_get.goshortcuts/apps/apps_db_recovery.goshortcuts/apps/apps_file_delete.goshortcuts/apps/apps_file_delete_test.goshortcuts/apps/apps_file_download.goshortcuts/apps/apps_file_download_test.goshortcuts/apps/apps_file_get.goshortcuts/apps/apps_file_get_test.goshortcuts/apps/apps_file_list.goshortcuts/apps/apps_file_list_test.goshortcuts/apps/apps_file_quota_get.goshortcuts/apps/apps_file_quota_get_test.goshortcuts/apps/apps_file_sign.goshortcuts/apps/apps_file_sign_test.goshortcuts/apps/apps_file_upload.goshortcuts/apps/apps_file_upload_test.goshortcuts/apps/db_common.goshortcuts/apps/file_common.goshortcuts/apps/shortcuts.goshortcuts/apps/shortcuts_test.goskills/lark-apps/SKILL.mdskills/lark-apps/references/lark-apps-db-env-create.mdskills/lark-apps/references/lark-apps-db-table-get.mdskills/lark-apps/references/lark-apps-db-table-list.mdskills/lark-apps/references/lark-apps-db.mdskills/lark-apps/references/lark-apps-file.md
💤 Files with no reviewable changes (3)
- skills/lark-apps/references/lark-apps-db-table-get.md
- skills/lark-apps/references/lark-apps-db-env-create.md
- skills/lark-apps/references/lark-apps-db-table-list.md
…inner, review fixes
- Migrate apps db/file shortcuts from output.Err* to typed errs.* (validation/network/api) with WithParam/WithCause; error-path tests assert typed metadata via errors.As/ProblemOf.
- db-execute: shape JSON `data` per SQL type (SELECT -> row array, DML -> {command,rows_affected}, DDL -> {command}, multi -> element array) instead of passing through the raw backend result string; field filtering via --jq.
- db-execute: report accurate rolled_back by inferring from BEGIN/COMMIT in completed statements (was hardcoded false); failure inside an explicit transaction -> rolled_back=true, completed=[].
- Add stderr spinner for slow ops (env-diff/migrate, recovery-diff/apply, audit-enable); no-op unless stderr is a TTY (IOStreams.ErrIsTerminal).
- quota-get pretty: align labels with miaoda-cli (Storage/Tables/Views/Files).
- file commands: drop redundant generic hint on API errors.
- data-import: move 1 MB size guard to Validate (Stat-based, fail before reading the file).
- skills: document db-execute JSON shapes + import/export 1MB/5000 limits.
Change-Id: Iefacb8cd85c25707a69a1983d2d2148a6fa29882
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/apps/apps_db_execute_test.go`:
- Around line 719-735: The test validating the error path in the function
containing the statement_index, rolled_back, and completed field checks is using
substring matching on the hint text instead of typed metadata assertions.
Replace the strings.Contains call checking for "rolled back" in
exitErr.Detail.Hint with proper typed problem metadata validation using
errs.ProblemOf to extract and assert the appropriate category, subtype, and
param fields. Ensure the cause preservation is also validated through typed
assertions rather than message substring inspection.
In `@shortcuts/apps/apps_db_execute.go`:
- Around line 241-245: The code is using the legacy output.ErrAPI helper for a
command-facing error, which violates the shortcuts/ coding guidelines and lint
policy. Replace the output.ErrAPI call with an appropriate typed errs.* error
constructor that can convey the error code, message, and the additional context
fields (statement_index, completed, rolled_back). Refer to the errs package to
find the correct typed error constructor that matches the error classification
needed for this database execution failure.
🪄 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: 213331e6-30e9-400a-800c-4268d323cbc6
📒 Files selected for processing (37)
internal/cmdutil/iostreams.gointernal/output/spinner.gointernal/output/spinner_test.goshortcuts/apps/apps_db_audit_list.goshortcuts/apps/apps_db_audit_set.goshortcuts/apps/apps_db_audit_status.goshortcuts/apps/apps_db_audit_test.goshortcuts/apps/apps_db_changelog_list.goshortcuts/apps/apps_db_changelog_list_test.goshortcuts/apps/apps_db_data_export.goshortcuts/apps/apps_db_data_export_test.goshortcuts/apps/apps_db_data_import.goshortcuts/apps/apps_db_data_import_test.goshortcuts/apps/apps_db_env_migrate.goshortcuts/apps/apps_db_env_recovery_quota_test.goshortcuts/apps/apps_db_execute.goshortcuts/apps/apps_db_execute_test.goshortcuts/apps/apps_db_quota_get.goshortcuts/apps/apps_db_recovery.goshortcuts/apps/apps_file_delete.goshortcuts/apps/apps_file_delete_test.goshortcuts/apps/apps_file_download.goshortcuts/apps/apps_file_download_test.goshortcuts/apps/apps_file_get.goshortcuts/apps/apps_file_get_test.goshortcuts/apps/apps_file_list.goshortcuts/apps/apps_file_list_test.goshortcuts/apps/apps_file_quota_get.goshortcuts/apps/apps_file_sign.goshortcuts/apps/apps_file_sign_test.goshortcuts/apps/apps_file_upload.goshortcuts/apps/apps_file_upload_test.goshortcuts/apps/db_common.goshortcuts/apps/file_common.goshortcuts/common/runner.goskills/lark-apps/references/lark-apps-db-execute.mdskills/lark-apps/references/lark-apps-db.md
✅ Files skipped from review due to trivial changes (2)
- shortcuts/apps/apps_file_quota_get.go
- skills/lark-apps/references/lark-apps-db.md
🚧 Files skipped from review as they are similar to previous changes (27)
- shortcuts/apps/apps_file_sign.go
- shortcuts/apps/apps_file_sign_test.go
- shortcuts/apps/apps_file_delete_test.go
- shortcuts/apps/apps_file_delete.go
- shortcuts/apps/apps_file_upload_test.go
- shortcuts/apps/apps_db_quota_get.go
- shortcuts/apps/apps_db_audit_status.go
- shortcuts/apps/apps_db_recovery.go
- shortcuts/apps/apps_file_list.go
- shortcuts/apps/apps_file_get_test.go
- shortcuts/apps/apps_db_data_import.go
- shortcuts/apps/apps_db_data_import_test.go
- shortcuts/apps/apps_file_download.go
- shortcuts/apps/apps_db_changelog_list.go
- shortcuts/apps/apps_db_data_export.go
- shortcuts/apps/apps_db_audit_set.go
- shortcuts/apps/apps_db_data_export_test.go
- shortcuts/apps/file_common.go
- shortcuts/apps/apps_db_changelog_list_test.go
- shortcuts/apps/apps_db_audit_list.go
- shortcuts/apps/apps_file_download_test.go
- shortcuts/apps/db_common.go
- shortcuts/apps/apps_db_audit_test.go
- shortcuts/apps/apps_db_env_migrate.go
- shortcuts/apps/apps_file_list_test.go
- shortcuts/apps/apps_file_upload.go
- shortcuts/apps/apps_db_env_recovery_quota_test.go
…IError Migrate sqlStatementError from legacy output.ErrAPI to errs.NewAPIError (CategoryAPI -> exit 1, subtype server_error). errs.* envelopes are flat with no detail container, so the failure diagnostics that previously lived in error.detail (statement_index / completed / rolled_back) are now carried in the message + hint text: - message ends with '(at statement N of M)' for the failure position; - hint distinguishes the rollback semantics: failure inside an explicit BEGIN…COMMIT transaction -> 'rolled back ... NO statements persisted'; otherwise DBA-mode auto-commit -> 'statements 1-N already applied ... not rolled back'. rolled_back is still inferred by inferRolledBack (BEGIN/COMMIT counting). Failure-path unit tests rewritten to assert via errs.ProblemOf + hint substrings. Addresses CodeRabbit review on PR #1530. Change-Id: I26d028af13926a4875e3e8cb22885913d58348d5
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/apps/apps_db_execute_test.go (1)
667-678: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAssert category/subtype for this typed error path.
Line 667 only proves the error is typed, then relies on message/hint substrings. Add the same
CategoryAPI/SubtypeServerErrorcheck used in the other failure tests so this catches wrong typed classification too.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.”Suggested test assertion
p, ok := errs.ProblemOf(err) if !ok { t.Fatalf("want a typed errs.* error, got %T: %v", err, err) } + if p.Category != errs.CategoryAPI || p.Subtype != errs.SubtypeServerError { + t.Errorf("category/subtype = %s/%s, want api/server_error", p.Category, p.Subtype) + } if !strings.Contains(p.Message, "(at statement 1 of 1)") { t.Errorf("message missing locator: %q", p.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/apps/apps_db_execute_test.go` around lines 667 - 678, The typed-error test only checks the message and hint, so it can miss wrong error classification. In the same test that uses errs.ProblemOf(err), add assertions for the expected CategoryAPI and SubtypeServerError values, matching the other failure-path tests, so this path verifies the typed metadata as well as the text. Use errs.ProblemOf and the existing failure-test pattern to locate the right assertion block.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.
Duplicate comments:
In `@shortcuts/apps/apps_db_execute_test.go`:
- Around line 667-678: The typed-error test only checks the message and hint, so
it can miss wrong error classification. In the same test that uses
errs.ProblemOf(err), add assertions for the expected CategoryAPI and
SubtypeServerError values, matching the other failure-path tests, so this path
verifies the typed metadata as well as the text. Use errs.ProblemOf and the
existing failure-test pattern to locate the right assertion block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 408de9ff-7e17-4c1c-bc24-f7de123ffce0
📒 Files selected for processing (3)
shortcuts/apps/apps_db_execute.goshortcuts/apps/apps_db_execute_test.goskills/lark-apps/references/lark-apps-db-execute.md
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/lark-apps/references/lark-apps-db-execute.md
Use 'Previewing migration diff (dev → online)' / 'Applying migration (dev → online)' for env-diff/env-migrate, and 'Restoring database (target: X)' / 'Previewing recovery impact (target: X)' for recovery, matching miaoda-cli's spinner phrasing. Change-Id: I8e87072b3cb6d5220a9d59b36588530f1c36643e
Drop the invented 'DBA mode auto-commits each statement' phrasing. Use
miaoda-cli's exact rolled-back sentence 'Transaction rolled back; no
changes persisted.', and plain SQL semantics for the auto-commit case
('Earlier statements were committed and not rolled back; ...'). Update
tests and the db-execute reference doc to match.
Change-Id: Ia74190a228355f72155bc5663030152bc69e9a4e
…test Match the multi-statement and transaction failure tests by asserting the error is classified as api/server_error via errs.ProblemOf, not only the message/hint substrings (CodeRabbit review on apps_db_execute_test.go). Change-Id: I054ec0233755d24865e5679e36eae71f1af069a1
Document previously-uncommented functions, methods, and test cases across the new apps db/file commands, their tests, the shortcut runner, and the output spinner, so CodeRabbit docstring coverage clears the 80% threshold. Change-Id: I5d9a3a1b91795788a862460e3994a8e81fbe5c10
…output - Mark transient network errors retryable (.WithRetryable) on the async poll timeout and the presigned-URL transfer 5xx/transport failures in export/import/download/upload, so agents retry instead of giving up; 4xx (e.g. expired signature) stays non-retryable. - file-download: surface the real save failure reason in the --output validation error message instead of an empty '--output'. - db/file quota-get: project output through an explicit field whitelist instead of passing the server map through, so future backend fields do not leak into agent context. - Add 2nd examples / --jq guidance to file-get/-download/-upload/-quota-get and db-quota-get help. - Tests for the new quota projections. Change-Id: I04eb137867e997f90b547e8199577070bfcdaeb0
- db-execute: document the third failure-hint case (first statement failed → 'No statements were applied'). - db-env-create: describe the re-init conflict by behavior instead of quoting a server-side literal message. - file-list: clarify pretty shows 5 columns; uploader/download_url are JSON-only (use +file-get for single-file detail). Change-Id: I69d8473ceaf0a0166ed14fd1b8e00125cf332cba
Summary
Add the second batch of apps db commands — data import/export, DDL changelog, row-level audit (status/enable/disable/list), dev→online env diff/migrate, point-in-time recovery diff/apply, and db quota — plus the full file-storage command set (list/get/sign/download/upload/delete/quota-get). Also consolidate the apps skill references.
Changes
+db-data-export,+db-data-import,+db-changelog-list,+db-audit-status,+db-audit-enable,+db-audit-disable,+db-audit-list,+db-env-diff,+db-env-migrate,+db-recovery-diff,+db-recovery-apply,+db-quota-get+file-list,+file-get,+file-sign,+file-download,+file-upload,+file-delete,+file-quota-getlark-apps-db.mdfor the whole db domain (+db-executekept aslark-apps-db-execute.md) and onelark-apps-file.mdfor storage; drop the per-command db reference files; updateSKILL.mdrouting//nolint:forbidigo— it bypasses the Lark gateway by design, soRuntimeContext.DoAPIdoesn't applyinternal/overrides (LARK_CLI_OPEN_API_BASE/x-tt-env)Test Plan
shortcuts/appsgreen, including under-race)lark-cli apps +db-*/+file-*flow works as expected (validated end-to-end on BOE)gofmt -l/go vet ./.../golangci-lint(changed lines) cleanRelated Issues
Summary by CodeRabbit
+db-executenow returns SQL-type/statement-awaredataand clearer multi-statement error hints.