Skip to content

feat(apps): miaoda db (data/audit/env/recovery/quota) + file storage commands#1530

Open
chenxingyang1019 wants to merge 10 commits into
mainfrom
feat/miaoda-db-file-openapi
Open

feat(apps): miaoda db (data/audit/env/recovery/quota) + file storage commands#1530
chenxingyang1019 wants to merge 10 commits into
mainfrom
feat/miaoda-db-file-openapi

Conversation

@chenxingyang1019

@chenxingyang1019 chenxingyang1019 commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

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 & governance: +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 storage: +file-list, +file-get, +file-sign, +file-download, +file-upload, +file-delete, +file-quota-get
  • skills: consolidate into one lark-apps-db.md for the whole db domain (+db-execute kept as lark-apps-db-execute.md) and one lark-apps-file.md for storage; drop the per-command db reference files; update SKILL.md routing
  • file-storage presigned-URL transfer (upload PUT / download GET) uses raw HTTP with //nolint:forbidigo — it bypasses the Lark gateway by design, so RuntimeContext.DoAPI doesn't apply
  • excludes the local-only BOE-debug internal/ overrides (LARK_CLI_OPEN_API_BASE / x-tt-env)

Test Plan

  • Unit tests pass (shortcuts/apps green, including under -race)
  • Manual local verification confirms the lark-cli apps +db-* / +file-* flow works as expected (validated end-to-end on BOE)
  • gofmt -l / go vet ./... / golangci-lint (changed lines) clean

Related Issues

  • None

Summary by CodeRabbit

  • New Features
    • Added app database tools: row-level audit status/enable/disable/list, DDL changelog listing, env diff/migrate, PITR diff/apply, and DB quota reporting.
    • Added app data import/export and app file management (upload/download/list/get/sign/delete/quota).
  • Updated
    • +db-execute now returns SQL-type/statement-aware data and clearer multi-statement error hints.
  • Documentation
    • Added/updated database and file command guides, including agent execution rules.
  • Tests
    • Expanded end-to-end and dry-run coverage for new commands, formatting, filtering, and pagination.
  • Chores
    • Added terminal spinner support for slow operations.

…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
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds DB audit, changelog, import/export, environment migration, recovery, quota, and file storage shortcuts plus shared helpers, registry wiring, runtime spinner support, tests, and docs.

Changes

Infrastructure: Terminal Output

Layer / File(s) Summary
Terminal spinner and async feedback
internal/cmdutil/iostreams.go, internal/output/spinner.go, internal/output/spinner_test.go, shortcuts/common/runner.go
IOStreams gains stderr terminal detection. StartSpinner renders and stops a spinner with cursor cleanup. RuntimeContext.StartSpinner wraps it with IO availability checks.

DB Operations

Layer / File(s) Summary
DB shared utilities
shortcuts/apps/db_common.go
Adds polling, DB endpoint path helpers, operator parsing and formatting, data format resolution, row counting, and typed app-id validation.
Audit status, toggle, and event listing
shortcuts/apps/apps_db_audit_status.go, shortcuts/apps/apps_db_audit_set.go, shortcuts/apps/apps_db_audit_list.go, shortcuts/apps/apps_db_audit_test.go
Adds audit status query, enable/disable toggles, and multi-table audit listing with prefiltering, skipped reasons, projection, pretty rendering, and coverage tests.
DDL changelog listing
shortcuts/apps/apps_db_changelog_list.go, shortcuts/apps/apps_db_changelog_list_test.go
Adds changelog listing with flag normalization, filtering, operator projection, pretty rendering, and tests.
Data import and export
shortcuts/apps/apps_db_data_export.go, shortcuts/apps/apps_db_data_import.go, shortcuts/apps/apps_db_data_export_test.go, shortcuts/apps/apps_db_data_import_test.go
Adds export/import request flows, format inference, size and limit guards, row counting, multipart upload, table-name inference, and tests.
Env migration, PITR recovery, and quota
shortcuts/apps/apps_db_env_migrate.go, shortcuts/apps/apps_db_recovery.go, shortcuts/apps/apps_db_quota_get.go, shortcuts/apps/apps_db_env_recovery_quota_test.go
Adds diff/migrate and recovery preview/apply flows, async polling, failure shaping, quota rendering, and end-to-end tests.
db-execute JSON normalization
shortcuts/apps/apps_db_execute.go, shortcuts/apps/apps_db_execute_test.go, skills/lark-apps/references/lark-apps-db-execute.md
Changes db-execute success payloads to normalized SQL-type shapes and updates typed error handling, tests, and output-contract documentation.

File Storage Operations

Layer / File(s) Summary
File shared utilities
shortcuts/apps/file_common.go
Provides timestamp normalization, transfer client setup, storage path builders, path validation, file metadata projection, and pretty rendering helpers.
File list, get, and quota
shortcuts/apps/apps_file_list.go, shortcuts/apps/apps_file_get.go, shortcuts/apps/apps_file_quota_get.go, shortcuts/apps/apps_file_list_test.go, shortcuts/apps/apps_file_get_test.go, shortcuts/apps/apps_file_quota_get_test.go
Adds file listing, metadata lookup, and quota retrieval with normalized filters, projection, pretty output, and tests.
File sign and download
shortcuts/apps/apps_file_sign.go, shortcuts/apps/apps_file_download.go, shortcuts/apps/apps_file_sign_test.go, shortcuts/apps/apps_file_download_test.go
Adds signed URL generation and two-step download execution with validation and end-to-end tests.
File upload and delete
shortcuts/apps/apps_file_upload.go, shortcuts/apps/apps_file_delete.go, shortcuts/apps/apps_file_upload_test.go, shortcuts/apps/apps_file_delete_test.go
Adds upload orchestration with presigned PUT and callback completion plus batch delete reporting and tests.

Shortcut Registration and Documentation

Layer / File(s) Summary
Registry, count test, and skill docs
shortcuts/apps/shortcuts.go, shortcuts/apps/shortcuts_test.go, skills/lark-apps/SKILL.md, skills/lark-apps/references/lark-apps-db.md, skills/lark-apps/references/lark-apps-file.md
Registers the new shortcuts, updates the shortcut count, revises routing, and adds DB and file reference docs with command coverage, conventions, time formats, and agent rules.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

  • larksuite/cli#1002: It also expands the shortcuts/apps command registry and related shortcut-count assertions alongside new shortcut additions.

Poem

🐇 I hopped through DB trails and file stacks bright,
With spinners and shortcuts all set just right.
Audit, quota, upload, and restore in a row,
The carrot cache glows as the new commands grow.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.24% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main addition: apps db and file-storage commands.
Description check ✅ Passed The description follows the required template and covers summary, changes, test plan, and related issues.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/miaoda-db-file-openapi

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the size/XL Architecture-level or global-impact change label Jun 22, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Assert typed error metadata, not message substrings.

Per coding guidelines, error-path tests must assert typed metadata (category, subtype, param) via errs.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.go section).

🤖 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 win

Assert typed error metadata, not message substrings.

Per coding guidelines, error-path tests must assert typed metadata (category, subtype, param) via errs.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.go section).

🤖 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 win

Replace legacy output.ErrAPI with typed error constructor.

Same issue as in apps_db_env_migrate.go. Per coding guidelines, use typed errs.* errors instead of legacy output.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 win

Replace legacy output.ErrAPI with typed error constructor.

Same issue as lines 118 and 131. Use typed errs.* error instead of output.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 win

Replace legacy output.ErrAPI with typed error constructor.

Per coding guidelines, command-facing failures must use typed errs.* errors, never legacy output.Err* helpers. The lint rule forbidigo [errs-typed-only] should flag this usage in shortcuts/.

Replace with a typed error constructor such as errs.NewAPIError(...) or errs.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 win

Tests 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 via errs.ProblemOf. Once line 43 in apps_file_delete.go is fixed to use errs.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 win

Tests 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 via errs.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.ValidationError

Also 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 win

Incorrect error hint for quota retrieval.

dbEnvMigrateHint instructs 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 win

Add 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 win

Add 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 win

Consider 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 via errs.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

📥 Commits

Reviewing files that changed from the base of the PR and between 9d4ae94 and 124c59c.

📒 Files selected for processing (38)
  • shortcuts/apps/apps_db_audit_list.go
  • shortcuts/apps/apps_db_audit_set.go
  • shortcuts/apps/apps_db_audit_status.go
  • shortcuts/apps/apps_db_audit_test.go
  • shortcuts/apps/apps_db_changelog_list.go
  • shortcuts/apps/apps_db_changelog_list_test.go
  • shortcuts/apps/apps_db_data_export.go
  • shortcuts/apps/apps_db_data_export_test.go
  • shortcuts/apps/apps_db_data_import.go
  • shortcuts/apps/apps_db_data_import_test.go
  • shortcuts/apps/apps_db_env_migrate.go
  • shortcuts/apps/apps_db_env_recovery_quota_test.go
  • shortcuts/apps/apps_db_quota_get.go
  • shortcuts/apps/apps_db_recovery.go
  • shortcuts/apps/apps_file_delete.go
  • shortcuts/apps/apps_file_delete_test.go
  • shortcuts/apps/apps_file_download.go
  • shortcuts/apps/apps_file_download_test.go
  • shortcuts/apps/apps_file_get.go
  • shortcuts/apps/apps_file_get_test.go
  • shortcuts/apps/apps_file_list.go
  • shortcuts/apps/apps_file_list_test.go
  • shortcuts/apps/apps_file_quota_get.go
  • shortcuts/apps/apps_file_quota_get_test.go
  • shortcuts/apps/apps_file_sign.go
  • shortcuts/apps/apps_file_sign_test.go
  • shortcuts/apps/apps_file_upload.go
  • shortcuts/apps/apps_file_upload_test.go
  • shortcuts/apps/db_common.go
  • shortcuts/apps/file_common.go
  • shortcuts/apps/shortcuts.go
  • shortcuts/apps/shortcuts_test.go
  • skills/lark-apps/SKILL.md
  • skills/lark-apps/references/lark-apps-db-env-create.md
  • skills/lark-apps/references/lark-apps-db-table-get.md
  • skills/lark-apps/references/lark-apps-db-table-list.md
  • skills/lark-apps/references/lark-apps-db.md
  • skills/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

Comment thread shortcuts/apps/apps_db_audit_list.go Outdated
Comment thread shortcuts/apps/apps_db_data_export_test.go
Comment thread shortcuts/apps/apps_db_data_export.go
Comment thread shortcuts/apps/apps_db_data_export.go
Comment thread shortcuts/apps/apps_db_data_export.go
Comment thread shortcuts/apps/db_common.go Outdated
Comment thread shortcuts/apps/db_common.go Outdated
Comment thread shortcuts/apps/db_common.go Outdated
Comment thread shortcuts/apps/file_common.go
Comment thread shortcuts/apps/file_common.go
…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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 124c59c and eb9454f.

📒 Files selected for processing (37)
  • internal/cmdutil/iostreams.go
  • internal/output/spinner.go
  • internal/output/spinner_test.go
  • shortcuts/apps/apps_db_audit_list.go
  • shortcuts/apps/apps_db_audit_set.go
  • shortcuts/apps/apps_db_audit_status.go
  • shortcuts/apps/apps_db_audit_test.go
  • shortcuts/apps/apps_db_changelog_list.go
  • shortcuts/apps/apps_db_changelog_list_test.go
  • shortcuts/apps/apps_db_data_export.go
  • shortcuts/apps/apps_db_data_export_test.go
  • shortcuts/apps/apps_db_data_import.go
  • shortcuts/apps/apps_db_data_import_test.go
  • shortcuts/apps/apps_db_env_migrate.go
  • shortcuts/apps/apps_db_env_recovery_quota_test.go
  • shortcuts/apps/apps_db_execute.go
  • shortcuts/apps/apps_db_execute_test.go
  • shortcuts/apps/apps_db_quota_get.go
  • shortcuts/apps/apps_db_recovery.go
  • shortcuts/apps/apps_file_delete.go
  • shortcuts/apps/apps_file_delete_test.go
  • shortcuts/apps/apps_file_download.go
  • shortcuts/apps/apps_file_download_test.go
  • shortcuts/apps/apps_file_get.go
  • shortcuts/apps/apps_file_get_test.go
  • shortcuts/apps/apps_file_list.go
  • shortcuts/apps/apps_file_list_test.go
  • shortcuts/apps/apps_file_quota_get.go
  • shortcuts/apps/apps_file_sign.go
  • shortcuts/apps/apps_file_sign_test.go
  • shortcuts/apps/apps_file_upload.go
  • shortcuts/apps/apps_file_upload_test.go
  • shortcuts/apps/db_common.go
  • shortcuts/apps/file_common.go
  • shortcuts/common/runner.go
  • skills/lark-apps/references/lark-apps-db-execute.md
  • skills/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

Comment thread shortcuts/apps/apps_db_execute_test.go Outdated
Comment thread shortcuts/apps/apps_db_execute.go Outdated
…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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
shortcuts/apps/apps_db_execute_test.go (1)

667-678: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Assert 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 / SubtypeServerError check 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 via errs.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

📥 Commits

Reviewing files that changed from the base of the PR and between eb9454f and fcc1d8b.

📒 Files selected for processing (3)
  • shortcuts/apps/apps_db_execute.go
  • shortcuts/apps/apps_db_execute_test.go
  • skills/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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Architecture-level or global-impact change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant