feat(apps): add db, file, openapi-key and observability shortcuts#1596
feat(apps): add db, file, openapi-key and observability shortcuts#1596qingniaotonghua wants to merge 32 commits into
Conversation
…egration Bring in the refined miaoda Spark db/file command set from the feat/miaoda-db-file-openapi work: db execute (typed errs + per-SQL-type JSON shaping), env diff/migrate, PITR recovery, changelog/audit, data import/export, db/file quota, and the 7 file-storage commands; plus the stderr spinner for slow ops and the aligned lark-apps skill references. Resolved overlap with the integration branch's earlier db-execute iteration (took the refined typed-error version), unified the stderr-TTY flag on IOStreams.StderrIsTerminal, and combined the shortcut registry (43 commands total).
) * feat(apps): add openapi-key common helpers (mask/redact/config) * feat(apps): add +openapi-key-list (redacted) * feat(apps): add +openapi-key-get (redacted) * feat(apps): add +openapi-key-create (one-time raw secret) * feat(apps): add +openapi-key-update * feat(apps): add +openapi-key-enable / +openapi-key-disable * feat(apps): add +openapi-key-delete (high-risk-write) * feat(apps): add +openapi-key-reset (rotate, one-time new secret) * test(apps): assert reset surfaces raw key exactly once * feat(apps): register openapi-key shortcuts * docs(lark-apps): add openapi-key reference and routing * test(apps): update shortcut count for openapi-key commands * fix(apps): trim openapi-key update name and correct shortcut-count comment * fix(apps): use camelCase config and add scope-all/scope-api flags Replace snake_case wire keys (request_scope, is_allow_access_preview) with camelCase (requestScope, isAllowAccessPreview, allowAll, httpInfos, httpMethod, httpPath). Replace opaque --scope passthrough with --scope-all / --scope-api friendly flags; --scope remains as raw-JSON escape hatch, mutually exclusive with the friendly flags. Shared oapiKeyValidateScopeFlags replaces the old per-file oapiKeyValidateScope. * fix(apps): use Changed for scope-all and refresh openapi-key scope docs Switch the update at-least-one guard from rctx.Bool to rctx.Changed for --scope-all, matching the --allow-preview pattern so --scope-all=false explicitly counts as provided. Rewrite lark-apps-openapi-key.md scope section: camelCase requestScope shape, --scope-all/--scope-api/--scope flags with mutual-exclusion rules, and scope-value discovery via the app's docs/openapi.json. * fix(apps): emit snake_case request_scope config for open gateway Open gateway (/open-apis/spark/v1) requires snake_case request bodies; flip parseScopeAPI/buildRequestScope/buildKeyConfig to emit http_method, http_path, allow_all, http_infos, request_scope, is_allow_access_preview. Update unit tests to assert snake_case and reject camelCase keys. * docs(lark-apps): correct openapi-key scope to snake_case wire format * docs(apps): align openapi-key flag help text to snake_case wire keys * feat(apps): add actionable hints and more examples to openapi-key P1: chain .WithHint(...) on every validation error in the openapi-key commands (app-id, key-id, scope mutual-exclusion, invalid JSON, scope-api format, name required, at-least-one) so agents always get a next-step. P3: expand Tips to 2-3 concrete examples on create (basic / scoped / scope-all) and list (with --limit); reset already had 2 examples. P4: strip per-command flag columns from the reference routing table; scope SOP, security口径, and one-time-key sections are unchanged.
Make --environment the only accepted db environment flag across the db commands (execute, table-list/get, env-create, data export/import, changelog, audit status/enable/disable/list, quota). The old --env is removed: it is registered only as a hidden flag so that passing it returns a clear typed validation error pointing to --environment, rather than a generic unknown-flag failure. Update the lark-apps db references accordingly.
Feat/apps observability
Unify the db environment flag default to dev for every db command (was online for table-list/get, data export/import, changelog, audit, quota; execute/env-create were already dev). Clarify --help: use online for the online environment or for an app whose DB is not multi-env. Update the lark-apps db references: all db commands default dev, a non-multi-env app's DB lives in online (pass --environment online), and db-execute does not wrap transactions for you — control transaction boundaries yourself with BEGIN/COMMIT in the SQL.
feat: rename app observability commands to list
|
|
|
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:
📝 WalkthroughWalkthroughExpands the ChangesApps CLI expansion and shared helpers
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@9f2fe50f4a49db93af5765c21a8bac2339a13589🧩 Skill updatenpx skills add larksuite/cli#feat/apps-spark-capibilities -y -g |
fix: remove unsed files
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1596 +/- ##
==========================================
- Coverage 74.66% 74.55% -0.11%
==========================================
Files 804 840 +36
Lines 81032 85280 +4248
==========================================
+ Hits 60501 63580 +3079
- Misses 16029 16877 +848
- Partials 4502 4823 +321 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (32)
shortcuts/apps/file_common.go-60-74 (1)
60-74: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFix the bare timestamp errors and preserve the parse cause.
The
fmt.Errorfreturns are failing the lint gate, and line 189 wraps them without.WithCause(err). Keep these as explicitly nolinted intermediate helper errors or refactor the helper contract, then preserve the cause in the typed validation error.Proposed fix
- return "", fmt.Errorf("invalid date %q", s) + return "", fmt.Errorf("invalid date %q: %w", s, err) //nolint:forbidigo // intermediate helper error; normalizeTimeFlags wraps it as typed validation. ... - return "", fmt.Errorf("invalid local datetime %q", s) + return "", fmt.Errorf("invalid local datetime %q: %w", s, err) //nolint:forbidigo // intermediate helper error; normalizeTimeFlags wraps it as typed validation. ... - return "", fmt.Errorf("invalid timestamp %q (want relative 7d/2h/30s, date 2026-04-15, datetime 2026-04-15T10:00:00, or ISO 8601 with TZ)", s) + return "", fmt.Errorf("invalid timestamp %q (want relative 7d/2h/30s, date 2026-04-15, datetime 2026-04-15T10:00:00, or ISO 8601 with TZ)", s) //nolint:forbidigo // intermediate helper error; normalizeTimeFlags wraps it as typed validation. ... - return errs.NewValidationError(errs.SubtypeInvalidArgument, "--%s: %v", f, err).WithParam("--" + f) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--%s: %v", f, err).WithParam("--" + f).WithCause(err)As per coding guidelines: “Command-facing failures must use typed
errs.*errors” and “Preserve underlying errors with.WithCause(err).”Also applies to: 187-190
🤖 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/file_common.go` around lines 60 - 74, The timestamp parsing helpers in file_common.go are returning bare fmt.Errorf validation errors and the caller is not preserving the parse cause, which violates the error-handling guidelines. Update the helper path around the timestamp parsing logic and the validation wrapper that consumes it so command-facing failures use the appropriate typed errs.* error, and make sure any underlying parse failure is attached with .WithCause(err). If you keep intermediate helper errors, explicitly mark them as allowed per lint or refactor the helper contract so the typed error construction happens at the boundary.Sources: Coding guidelines, Linters/SAST tools, Pipeline failures
shortcuts/apps/apps_file_delete.go-86-99 (1)
86-99: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not mark missing delete results as successful.
If
resultsis missing, truncated, or has a non-map entry,rstays nil and the item defaults to"ok", falsely reporting a delete success for that path. Treat missing/malformed entries as errors instead.Proposed fix
for i, input := range inputs { var r map[string]interface{} if i < len(arr) { r, _ = arr[i].(map[string]interface{}) } + if r == nil { + out = append(out, map[string]interface{}{ + "status": "error", + "path": input, + "error": map[string]interface{}{ + "code": "DELETE_RESULT_MISSING", + "message": deleteErrorMessage("DELETE_RESULT_MISSING", input), + }, + }) + continue + } status := "ok" - if r != nil && common.GetString(r, "status") != "" { + if common.GetString(r, "status") != "" { status = common.GetString(r, "status") }🤖 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.go` around lines 86 - 99, The projectDeleteResults helper is defaulting missing or malformed delete entries to "ok", which can falsely report success when results are absent, truncated, or not a map. Update projectDeleteResults to treat a nil r or a failed map cast as an error status instead of assuming success, using the existing status handling in projectDeleteResults and common.GetString to preserve explicit statuses only when the entry is valid. Ensure any missing or malformed result for a given input path produces a failure response for that path rather than an ok item.shortcuts/apps/apps_file_upload.go-66-72 (1)
66-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMirror the real pre-upload body in dry-run.
Dry-run currently previews only
file_name, butExecutesendsfile_name,file_size, andcontent_type. This makes dry-run request validation incomplete.Proposed fix
DryRun: func(ctx context.Context, rctx *common.RuntimeContext) *common.DryRunAPI { appID, _ := requireAppID(rctx.Str("app-id")) + filePath := strings.TrimSpace(rctx.Str("file")) + fileName := filepath.Base(filePath) + body := map[string]interface{}{ + "file_name": fileName, + "content_type": mimeByExt(fileName), + } + if st, err := rctx.FileIO().Stat(filePath); err == nil { + body["file_size"] = st.Size() + } return common.NewDryRunAPI(). POST(appFilePreUploadPath(appID)). Desc("Pre-upload → client PUT bytes → callback (3-step)"). - Body(map[string]interface{}{"file_name": filepath.Base(strings.TrimSpace(rctx.Str("file")))}) + Body(body) },🤖 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.go` around lines 66 - 72, The DryRun preview for the pre-upload flow is incomplete because it only includes file_name while Execute sends file_name, file_size, and content_type. Update the DryRun builder in apps_file_upload.go to mirror the real request body produced by the pre-upload path, using the same fields and values as the Execute logic so validation matches the actual request.shortcuts/apps/apps_file_sign.go-46-48 (1)
46-48: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject non-positive
--expires-ininstead of silently ignoring it.
buildFileSignBodyonly includesexpires_inwhen the value is> 0, so--expires-in 0or-1currently passes validation and falls back to the default TTL. For signed URLs, that can widen the access window instead of failing fast on invalid input.Suggested fix
- if rctx.Int("expires-in") > fileSignMaxExpiresSeconds { + v := rctx.Int("expires-in") + if v <= 0 { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--expires-in must be greater than 0").WithParam("--expires-in") + } + if v > fileSignMaxExpiresSeconds { return errs.NewValidationError(errs.SubtypeInvalidArgument, "--expires-in exceeds the maximum of %d seconds (30d)", fileSignMaxExpiresSeconds).WithParam("--expires-in") }Also applies to: 75-80
🤖 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_sign.go` around lines 46 - 48, The `buildFileSignBody`/`fileSign` validation currently only rejects values above `fileSignMaxExpiresSeconds`, so non-positive `--expires-in` values can slip through and fall back to the default TTL. Update the validation around `rctx.Int("expires-in")` to explicitly reject zero and negative values with a `ValidationError` for `--expires-in`, while keeping the existing upper-bound check; make the same fix in the duplicate validation block referenced by the review.shortcuts/apps/shortcuts.go-11-11 (1)
11-11: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't advertise
--yeson+env-deleteuntil the command actually supports it.
AppsEnvVarDeletecurrently only definesapp-id,--environment, and--key, and its execute path never reads ayesflag. This new tip therefore points users at an unknown-flag failure, and the follow-up tests/docs in this PR are already baking that bad contract in. Either add a real confirmation/--yesflow toAppsEnvVarDelete, or remove the--yesguidance here.🤖 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/shortcuts.go` at line 11, The tip added in withExtraTips for AppsEnvVarDelete incorrectly advertises a --yes option that the command does not support. Update the AppsEnvVarDelete flow to either implement real confirmation handling for a yes flag in the command’s execute path, or remove the --yes guidance from the tip so users are not directed to an unknown flag.shortcuts/apps/apps_openapi_key_reset_test.go-24-47 (1)
24-47: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReplace the raw-key fixture with a neutral placeholder.
The response stub currently uses a credential-looking value, which is already being rejected by deterministic-gate. Use an obvious placeholder string here and keep the “appears exactly once” assertion against that placeholder instead.
Based on pipeline failures, deterministic-gate rejects this file for a generic credential assignment.
🤖 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_openapi_key_reset_test.go` around lines 24 - 47, The test in AppsOpenAPIKeyReset uses a credential-looking fixture value in the HTTP stub, which triggers deterministic-gate rejection. Replace the raw secret placeholder in the stubbed response body with a clearly neutral string, and update the Execute/assertions in AppsOpenAPIKeyReset so the “appears exactly once” check uses that placeholder while keeping the redaction check on the info payload.Source: Pipeline failures
shortcuts/apps/apps_openapi_key_common_test.go-47-254 (1)
47-254: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd coverage for the shared scope validator.
The new
oapiKeyValidateScopeFlagsbehavior inapps_openapi_key_common.gois still uncovered (--scopeinvalid JSON and malformed--scope-api). Please add runtime-context tests for those branches and assert the typed validation metadata instead of only checkingerr != nil.As per coding guidelines,
**/*.go: every behavior change needs a test alongside the change, and**/*_test.go: error-path tests must assert typed metadata viaerrs.ProblemOf/ 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_openapi_key_common_test.go` around lines 47 - 254, Add runtime-context coverage for the shared scope validator in the tests for oapiKeyValidateScopeFlags: exercise invalid JSON passed via --scope and malformed --scope-api input, not just the happy paths already covered by parseScopeAPI, buildRequestScope, and buildKeyConfig. Update the new error-path assertions to check typed validation metadata with errs.ProblemOf (or the typed error fields) instead of only asserting err != nil, so the validator behavior is verified explicitly.Sources: Coding guidelines, Linters/SAST tools
shortcuts/apps/apps_openapi_key_list_test.go-61-91 (1)
61-91: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCover validation, dry-run, and limit/offset plumbing.
This suite only exercises the happy-path execute flow, while the new
Validate, dry-run path generation, andbuildOpenAPIKeyListParamsbranches are still uncovered. Please add tests for missing--app-idand for limit/offset propagation into the dry-run request.As per coding guidelines,
**/*.go: every behavior change needs a test alongside the change, and**/*_test.go: error-path tests must assert typed metadata.🤖 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_openapi_key_list_test.go` around lines 61 - 91, The current `TestOpenAPIKeyListExecute_Redacts` only covers the happy-path execute flow, so add coverage for the new `Validate`, dry-run, and `buildOpenAPIKeyListParams` behavior in `AppsOpenAPIKeyList`. Add an error-path test for missing `--app-id` that asserts the typed metadata from validation, and add a dry-run test that verifies `limit` and `offset` are propagated into the generated request. Keep the existing redaction assertion, but expand the suite so each behavior change in `AppsOpenAPIKeyList.Execute`, `Validate`, and `buildOpenAPIKeyListParams` has a corresponding test.Sources: Coding guidelines, Linters/SAST tools
shortcuts/apps/apps_openapi_key_reset_test.go-20-48 (1)
20-48: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd validation and dry-run coverage for reset.
The new command still has no test for
oapiKeyValidateKeyID, dry-run URL generation, or the hinted API-failure path inapps_openapi_key_reset.go. Please add those cases so the new behavior is covered end-to-end.As per coding guidelines,
**/*.go: every behavior change needs a test alongside the change, and**/*_test.go: error-path tests must assert typed metadata.🤖 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_openapi_key_reset_test.go` around lines 20 - 48, Add missing tests for AppsOpenAPIKeyReset to cover the new behaviors end-to-end: verify oapiKeyValidateKeyID rejects invalid key IDs, add a dry-run case that asserts the generated URL/path without making the request, and add an API-failure test for AppsOpenAPIKeyReset.Execute that exercises the error path in apps_openapi_key_reset.go. Keep the existing successful reset coverage, and for any error-path assertions make sure the test checks the typed metadata returned by the command rather than only the error string.Sources: Coding guidelines, Linters/SAST tools
shortcuts/apps/apps_openapi_key_list_test.go-65-89 (1)
65-89: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse a non-sensitive placeholder for the stubbed API key.
Both the stub payload and the leak assertion use a credential-looking literal, and that already fails deterministic-gate for this file. Swap in a neutral placeholder value and keep the masking assertion based on the placeholder suffix.
Based on pipeline failures, deterministic-gate rejects this file for a generic credential assignment.
🤖 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_openapi_key_list_test.go` around lines 65 - 89, The test in AppsOpenAPIKeyList uses a credential-looking literal for the stubbed api_key, which trips deterministic-gate; replace that value with a neutral placeholder in the httpmock.Stub payload and update the leak/masking checks in AppsOpenAPIKeyList.Execute to assert against the placeholder suffix rather than a real-looking secret.Source: Pipeline failures
shortcuts/apps/apps_openapi_key_common_test.go-24-45 (1)
24-45: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReplace the credential-like fixture with a neutral placeholder.
This test data is already tripping deterministic-gate as
public_content_generic_credential. Use an obviously non-sensitive placeholder value and derive the expected preview from that placeholder instead of embedding a secret-looking literal.Based on pipeline failures, deterministic-gate rejects this file for a generic credential assignment.
🤖 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_openapi_key_common_test.go` around lines 24 - 45, The test fixture in TestRedactKeyInfo_StripsRawKey uses a credential-like api_key value that triggers deterministic-gate, so replace it with an obviously neutral placeholder and update the expected key_preview to match that placeholder. Keep the assertions around redactKeyInfo behavior the same, but ensure the input data no longer looks like a real secret while still validating stripping, previewing, and non-mutation.Source: Pipeline failures
shortcuts/apps/apps_openapi_key_common.go-49-70 (1)
49-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn typed validation errors from these scope helpers.
These branches handle user-supplied
--scope/--scope-apiinput, but they currently return barefmt.Errorf/ rawjson.Unmarshalerrors. That already fails lint here, and it also drops theparam/hint/cause metadata the command layer expects.Suggested fix
func parseScopeAPI(s string) (map[string]interface{}, error) { fields := strings.Fields(strings.TrimSpace(s)) if len(fields) != 2 { - return nil, fmt.Errorf("expected 'METHOD /path', got %q", s) + return nil, appsValidationParamError("--scope-api", "--scope-api must be 'METHOD /path', got %q", s). + WithHint("format: --scope-api 'METHOD /openapi/path'") } return map[string]interface{}{"http_method": strings.ToUpper(fields[0]), "http_path": fields[1]}, nil } func buildRequestScope(scopeAll bool, scopeAPIs []string, scopeRaw string) (interface{}, error) { scopeRaw = strings.TrimSpace(scopeRaw) hasFriendly := scopeAll || len(scopeAPIs) > 0 if scopeRaw != "" { if hasFriendly { - return nil, fmt.Errorf("--scope cannot be combined with --scope-all / --scope-api") + return nil, appsValidationParamError("--scope", "--scope cannot be combined with --scope-all / --scope-api"). + WithHint("use either --scope (raw JSON) OR --scope-all/--scope-api, not both") } var rs interface{} if err := json.Unmarshal([]byte(scopeRaw), &rs); err != nil { - return nil, err + return nil, appsValidationParamError("--scope", "--scope must be valid JSON"). + WithHint("--scope takes raw JSON for config.request_scope; or use --scope-all / --scope-api 'METHOD /openapi/path'"). + WithCause(err) } return rs, nil }As per coding guidelines,
**/*.go: command-facing failures must use typederrs.*errors and preserve underlying errors with.WithCause(err).🤖 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_openapi_key_common.go` around lines 49 - 70, Update parseScopeAPI and buildRequestScope to return command-facing typed validation errors instead of fmt.Errorf or raw json.Unmarshal errors. Use the appropriate errs.* constructors for invalid --scope-api format, mutually exclusive scope flags, and JSON parse failures, and attach the underlying parse error with .WithCause(err) so param/hint/cause metadata is preserved.Sources: Coding guidelines, Linters/SAST tools, Pipeline failures
shortcuts/apps/apps_openapi_key_get_test.go-42-48 (1)
42-48: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAssert the typed validation contract here.
err != nilis too weak for this repo’s shortcut tests; a downgrade to an untyped error would still pass. Please assert the problem category/subtype/hint, and useerrors.As(..., *errs.ValidationError)to verifyParam == "--key-id".As per coding guidelines,
*_test.goerror-path tests must assert typed metadata viaerrs.ProblemOf; based on learnings,Parammust be checked viaerrors.Ason*errs.ValidationErrorbecauseerrs.ProblemOfdoes not expose it.🤖 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_openapi_key_get_test.go` around lines 42 - 48, The missing-key test only checks err != nil, so it should assert the typed validation contract instead. Update TestOpenAPIKeyGetExecute_MissingKeyID to verify the returned error with errs.ProblemOf for the validation problem category/subtype/hint, and use errors.As on *errs.ValidationError to assert Param is "--key-id". Keep the check anchored on AppsOpenAPIKeyGet.Validate so an untyped or downgraded error will fail the test.Sources: Coding guidelines, Learnings
shortcuts/apps/apps_openapi_key_create_test.go-30-58 (1)
30-58: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace the credential-like fixture content in this test.
The current stub/assertion text is already tripping deterministic-gate, so this suite cannot merge as-is. Please use a non-sensitive placeholder and neutral wording while keeping the masking assertion intact.
As per pipeline failures, deterministic-gate already rejects this test file for
public_content_generic_credential.🤖 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_openapi_key_create_test.go` around lines 30 - 58, The test fixture in AppsOpenAPIKeyCreate is using credential-like raw secret text that triggers deterministic-gate; replace the api_key stub value and related assertion strings with a neutral, non-sensitive placeholder while keeping the same masking and redaction checks. Update the assertions in AppsOpenAPIKeyCreate to verify raw-vs-redacted behavior without embedding a public-content-generic-credential-looking token, and keep the rest of the Execute/context/stdout validation unchanged.Source: Pipeline failures
shortcuts/apps/apps_openapi_key_get_test.go-18-39 (1)
18-39: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse non-credential placeholders and neutral failure text in this test.
This file is already failing deterministic-gate because the stub/message content looks like generic credential material. Please swap in a benign placeholder and avoid
raw api_key:-style wording in assertions.As per pipeline failures, deterministic-gate already rejects this test file for
public_content_generic_credential.🤖 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_openapi_key_get_test.go` around lines 18 - 39, The test in AppsOpenAPIKeyGet is tripping the generic-credential detector because it uses a realistic api key value and raw api_key wording in assertions. Replace the stubbed api_key in the httpmock registration with a clearly non-secret placeholder, and update the failure message/assertion text in AppsOpenAPIKeyGet to neutral wording that does not mention raw credentials. Keep the existing masking check for the key preview, but make sure no literal secret-like strings remain in this test.Source: Pipeline failures
shortcuts/apps/apps_openapi_key_create_test.go-61-85 (1)
61-85: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThese validation tests need typed error assertions.
All three cases currently pass on any non-nil error, so they won’t catch regressions in subtype/hint/param classification. Please assert the typed metadata, and verify the expected
Paramwitherrors.As(..., *errs.ValidationError).As per coding guidelines,
*_test.goerror-path tests must assert typed metadata viaerrs.ProblemOf; based on learnings,Parammust be checked viaerrors.Ason*errs.ValidationErrorbecauseerrs.ProblemOfdoes not expose it.🤖 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_openapi_key_create_test.go` around lines 61 - 85, The three validation tests in AppsOpenAPIKeyCreate.Validate only check for a non-nil error, so they should be strengthened to assert the typed error metadata. Update each case to verify the expected problem classification with errs.ProblemOf, and separately check the Param using errors.As against *errs.ValidationError since that is where Param is exposed. Keep the existing scenarios for missing name, invalid scope JSON, and mutually exclusive scope flags, but ensure the assertions cover the specific subtype/hint/param behavior.Sources: Coding guidelines, Learnings
shortcuts/apps/apps_openapi_key_get.go-67-70 (1)
67-70: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReword this pretty-printer string so deterministic-gate passes.
CI is already rejecting this exact output block as
public_content_generic_credential. Keep the structured output contract, but avoid hard-coded assignment-style key labels in the human-readable formatter.Suggested tweak
- fmt.Fprintf(w, "api_key_id: %v\nname: %v\nstatus: %v\nkey_preview: %v\n", + fmt.Fprintf(w, "key id: %v\nname: %v\nstatus: %v\nkey preview: %v\n", red["api_key_id"], red["name"], red["status"], red["key_preview"])As per pipeline failures, deterministic-gate already rejects Line 68 for
public_content_generic_credential.🤖 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_openapi_key_get.go` around lines 67 - 70, The pretty-printer in the OpenAPI key getter is emitting an output string with assignment-style credential labels that deterministic-gate flags as public_content_generic_credential. Update the formatter in the output block using rctx.OutFormat and the existing red map so it keeps the same fields but rewrites the human-readable labels in a less credential-like way, without changing the structured contract or the set of values printed.Source: Pipeline failures
shortcuts/apps/apps_openapi_key_create.go-98-100 (1)
98-100: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThis formatter string is a merge blocker under deterministic-gate.
The human-readable
api_key...output is already being rejected aspublic_content_generic_credential. Preserve the top-level JSON field names if needed, but rephrase the pretty text labels.Suggested tweak
- fmt.Fprintf(w, "api_key_id: %v\napi_key: %v (shown once)\n", out["api_key_id"], raw) + fmt.Fprintf(w, "key id: %v\nissued key (shown once): %v\n", out["api_key_id"], raw)As per pipeline failures, deterministic-gate already rejects Line 100 for
public_content_generic_credential.🤖 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_openapi_key_create.go` around lines 98 - 100, The pretty-print formatter in the api key creation flow is still emitting credential-like labels that trip deterministic-gate as public_content_generic_credential. Update the output in the api_key_create path so the human-readable labels inside rctx.OutFormat no longer say api_key or api_key_id, while keeping the underlying JSON field names in out unchanged if needed; adjust the fmt.Fprintf text to use neutral labels and keep the one-time warning intact.Source: Pipeline failures
shortcuts/apps/apps_openapi_key_update_test.go-44-61 (1)
44-61: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReplace the credential-like test value with a non-sensitive placeholder.
CI is already rejecting this fixture as a generic credential assignment. Swap the
api_keysample for a clearly fake placeholder that still exercises redaction without matching the credential scanner.🤖 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_openapi_key_update_test.go` around lines 44 - 61, The test fixture in AppsOpenAPIKeyUpdate currently uses a credential-looking api_key sample that triggers scanner rejection; replace the value in the httpmock.Stub response with a clearly fake, non-sensitive placeholder while keeping the redaction assertion in place. Update the stubbed payload in AppsOpenAPIKeyUpdate.Execute’s test so it still exercises the same path but no longer resembles a real secret.Source: Pipeline failures
shortcuts/apps/apps_openapi_key_status_test.go-18-32 (1)
18-32: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse a scanner-safe placeholder for the mocked
api_key.This test fixture is also tripping the public-content credential gate, so the suite won’t pass as-is. Replace the mocked raw key with an obviously fake placeholder that still lets the redaction assertion prove the command never prints the raw value.
🤖 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_openapi_key_status_test.go` around lines 18 - 32, The test fixture in AppsOpenAPIKeyEnable should not use a scanner-triggering raw api_key value; replace the mocked api_key in the httpmock.Stub body with an obviously fake, scanner-safe placeholder while keeping the same assertion in the test. Update the fixture in apps_openapi_key_status_test.go so the redaction check still verifies that Execute on AppsOpenAPIKeyEnable never prints the raw key, using the same stdoutBuf.String() containment assertion to validate masking.Source: Pipeline failures
shortcuts/apps/apps_openapi_key_update_test.go-31-36 (1)
31-36: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAssert the typed validation metadata instead of the message text.
This is an error-path test, so it should verify the typed contract (
category/subtype, andParamviaerrors.As(*errs.ValidationError)) rather thanstrings.Contains(err.Error(), ...). That keeps the test aligned with the shortcut error API and the repo’s test rules. Based on learnings,errs.ProblemOfdoes not exposeParam; assert that part viaerrors.Ason*errs.ValidationError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/apps/apps_openapi_key_update_test.go` around lines 31 - 36, The validation test for AppsOpenAPIKeyUpdate is checking the rendered error string instead of the typed error contract. Update the test to assert the validation metadata returned by AppsOpenAPIKeyUpdate.Validate using category/subtype and extract Param with errors.As against *errs.ValidationError, rather than matching strings.Contains on err.Error(). This should keep the assertions aligned with the shortcut error API and the existing error-path test conventions.Sources: Coding guidelines, Learnings
shortcuts/apps/apps_env.go-334-347 (1)
334-347: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPopulate
envon synthesized rows for object-shaped responses.When
env_varscomes back as an object, this branch only emitskey/value, butenvVarListSchema()always expects anenvcolumn. On that compatibility path, pretty output renders blank env cells and JSON output loses the requested environment entirely.🤖 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_env.go` around lines 334 - 347, The object-shaped response branch in the env var list builder is omitting the env field, so synthesized rows from the map[string]interface{} path do not match envVarListSchema(). Update the logic in the env synthesis code to populate env on each item alongside key/value, using the same environment value that the list schema expects, so both pretty and JSON output preserve the requested environment correctly.shortcuts/apps/apps_logs.go-90-93 (1)
90-93: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
page_token/has_morein pretty output.The pretty branch only prints the item table, so pagination metadata disappears. That makes
+log-listimpossible to continue in pretty mode once a page fills up, even though the command tips tell users to reuse the returned cursor.Proposed fix
out := normalizeLogSearchResponse(data) rctx.OutFormat(out, nil, func(w io.Writer) { appsPrintSchemaTable(w, appsProjectRows(logListRows(out.Items), logSummarySchema), logSummarySchema) + if out.PageToken != "" || out.HasMore { + fmt.Fprintln(w) + } + if out.PageToken != "" { + fmt.Fprintf(w, "page_token: %s\n", out.PageToken) + } + if out.HasMore { + fmt.Fprintln(w, "has_more: true") + } })🤖 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_logs.go` around lines 90 - 93, The pretty output path in normalizeLogSearchResponse/appsPrintSchemaTable is dropping pagination metadata, so update the OutFormat callback to render page_token and has_more alongside the item table instead of only printing logSummarySchema rows. Keep the existing table rendering for logListRows(out.Items), but ensure the pretty branch also surfaces the cursor fields from the normalized response so +log-list can be continued in pretty mode.shortcuts/apps/apps_logs.go-602-625 (1)
602-625: 🎯 Functional Correctness | 🟠 MajorMake source-stack traversal deterministic.
collectSourceStackMapsIntorecurses throughmap[string]interface{}in Go’s random map order, sofirstStringInMaps/firstFramesInMapscan pick different nested commit, prefix, tenant, or frame candidates across runs when multiple matches exist. Sort the keys before recursing, and add a regression test covering competing nested candidates.🤖 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_logs.go` around lines 602 - 625, The source-stack traversal in collectSourceStackMapsInto is nondeterministic because it ranges over map[string]interface{} directly, which can cause firstStringInMaps and firstFramesInMaps to pick different nested candidates across runs. Update collectSourceStackMapsInto to recurse through map keys in a stable sorted order before visiting nested values, and add a regression test around firstStringInMaps/firstFramesInMaps that exercises competing nested commit, prefix, tenant, or frame matches to verify deterministic selection.Source: Pipeline failures
shortcuts/apps/apps_metrics.go-77-82 (1)
77-82: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPretty output currently drops the bucket dimensions.
observabilitySeriesRowskeepsdimensionsnested, but the strict table schema only printstimeplus the metric columns. Any response with multiple page/API/device buckets at the same timestamp becomes ambiguous in pretty mode because the bucket keys never surface as columns.As per coding guidelines, "Every behavior change needs a test alongside the change".
Also applies to: 534-565
🤖 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_metrics.go` around lines 77 - 82, Pretty output from apps metrics is losing the bucket dimensions because observabilitySeriesRows keeps dimensions nested while metricSeriesSchema only renders time and metric columns, so update appsPrintSchemaTable/metricSeriesSchema handling to surface the bucket keys as columns in pretty mode. Use the observabilitySeriesRows, filterObservabilityRowsWithTime, and metricSeriesSchema path to locate the change, and add a test covering multi-bucket responses at the same timestamp to verify the dimensions appear and the rows are no longer ambiguous.Source: Coding guidelines
shortcuts/apps/apps_traces.go-562-567 (1)
562-567: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize top-level trace summary aliases before the no-spans fallback.
When
+trace-getgets a detail payload withoutspans, Line 622 falls back totraceSummaryRow(trace). Right nownormalizeTraceObjectonly canonicalizestrace_id/is_break, so fields likestartTimeNs,rootSpan,userID, anddurationMsnever reach pretty output even though the response contained them.Proposed fix
func normalizeTraceObject(trace map[string]interface{}) map[string]interface{} { out := cloneMap(trace) normalizeObservabilityAttributes(out) copyFirstAlias(out, trace, "trace_id", "trace_id", "traceID", "traceId") + copyFirstAlias(out, trace, "start_time_ns", "start_time_ns", "startTimeNs") + copyFirstAlias(out, trace, "root_span", "root_span", "rootSpan") + copyFirstAlias(out, trace, "user_id", "user_id", "userID", "userId") + copyFirstAlias(out, trace, "duration_ms", "duration_ms", "durationMs") + copyFirstAlias(out, trace, "status", "status") + copyFirstAlias(out, trace, "span_count", "span_count", "spanCount") copyFirstAlias(out, trace, "is_break", "is_break", "isBreak") return out }As per coding guidelines, "Every behavior change needs a test alongside the change".
Also applies to: 609-623
🤖 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_traces.go` around lines 562 - 567, `normalizeTraceObject` currently only canonicalizes a couple of aliases, so the no-spans fallback path in `traceSummaryRow` misses top-level summary fields like `startTimeNs`, `rootSpan`, `userID`, and `durationMs`. Extend `normalizeTraceObject` to copy the relevant summary aliases into the canonical keys before `traceSummaryRow(trace)` is used, preserving existing behavior for `trace_id` and `is_break`. Add or update a test covering the `+trace-get` detail payload without `spans` to verify the pretty output includes the normalized summary fields.Sources: Coding guidelines, Linters/SAST tools
shortcuts/apps/apps_db_data_import.go-62-66 (1)
62-66: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--dry-runaccepts missing or unreadable files right now.
Statis only used on the happy path. If it fails, validation continues and dry-run still succeeds even though the real command cannot read the file. Since you're already callingFileIO().Stat()here, return a typed--fileerror onserrand keep the size check for the success path. Please add a dry-run regression test for a missing file along with the fix.Based on learnings,
runtime.FileIO().Stat()already performs user-path validation, so ignoringserrhere leaves that validation unused during dry-run.🤖 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_data_import.go` around lines 62 - 66, `apps_db_data_import.go` currently ignores `rctx.FileIO().Stat()` failures in the `Validate` path, so `--dry-run` can pass for missing or unreadable `--file` values. Update the validation logic in the import handler to return a typed `--file` validation error when `Stat` returns `serr`, and keep the existing max-size check on the successful `st.Size()` path. Also add a dry-run regression test covering a missing file to lock in the `apps_db_data_import`/`Validate` behavior.Source: Learnings
shortcuts/apps/apps_db_data_export.go-122-127 (1)
122-127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't collapse save failures into
invalid_argument.
FileIO().Save()can fail after the path itself is valid (permission denied, disk full, short write). ReturningValidationErrorfor all of those misclassifies the failure and drops the original cause. Preserve any typed lower-layer error unchanged, and wrap the remaining write failures with an internal file-I/O error plus.WithCause(err).As per coding guidelines, local file I/O failures must use typed
errs.*errors and preserve causes. Based on learnings,runtime.FileIO()already handles path validation for user-supplied paths.🤖 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_data_export.go` around lines 122 - 127, The FileIO().Save failure handling in apps_db_data_export.go is misclassifying write-time errors as invalid_argument. Update the save error branch after rctx.FileIO().Save so any typed lower-layer error is returned unchanged, and otherwise wrap the failure as an internal file-I/O errs.* error with .WithCause(err) instead of errs.NewValidationError. Keep the existing context around --output, and use the FileIO().Save and errs error helpers to preserve the original cause.Sources: Coding guidelines, Learnings
shortcuts/apps/apps_db_execute.go-444-447 (1)
444-447: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPretty failure output ignores explicit SQL transactions.
This footer always says earlier statements were committed, but
BEGIN ...blocks are still possible even withtransactional=false. In pretty mode, a failed transactional batch will print the opposite of the typed error hint thatsqlStatementError()already derives frominferRolledBack(...).Suggested fix
if failedIdx >= 0 { - // CLI 永远传 transactional=false，失败语句之前的语句已逐条 commit 落地、不会整批回滚—— - // 如实告诉用户，避免整批重跑导致重复写入。 - if successCount > 0 { - fmt.Fprintf(w, "(statement %d failed; %d statement%s before it committed and not rolled back)\n", - failedIdx+1, successCount, plural(int64(successCount))) + rolledBack := inferRolledBack(stmts[:failedIdx]) + if successCount > 0 && rolledBack { + fmt.Fprintf(w, "(statement %d failed; transaction rolled back and no changes persisted)\n", failedIdx+1) + } else if successCount > 0 { + fmt.Fprintf(w, "(statement %d failed; %d statement%s before it committed and not rolled back)\n", + failedIdx+1, successCount, plural(int64(successCount))) } else { fmt.Fprintf(w, "(statement %d failed; no statements applied)\n", failedIdx+1) } } else {🤖 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.go` around lines 444 - 447, The pretty-mode failure footer in appsDBExecute currently assumes earlier statements were committed whenever transactional=false, but it should respect explicit SQL transactions. Update the failure message logic near appsDBExecute and sqlStatementError to use the same rollback/commit inference as inferRolledBack(...) so BEGIN/COMMIT blocks are reported accurately. Keep the existing successCount handling, but make the final printed hint match the transactional state of the failed batch instead of always claiming prior statements were committed.shortcuts/apps/db_common.go-52-70 (1)
52-70: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
pollUntiltimes out one interval early.Because the first
fetch()happens before any sleep,maxAttempts := int(maxWait / interval)only allowsfloor(maxWait/interval)total polls, not retries. For example,interval=2sandmaxWait=5stimes out after the poll at ~2s, even though a poll at ~4s should still be allowed.Suggested fix
- maxAttempts := int(maxWait / interval) + maxAttempts := int(maxWait/interval) + 1 if maxAttempts < 1 { maxAttempts = 1 }🤖 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/db_common.go` around lines 52 - 70, The pollUntil loop in db_common.go is capping attempts too aggressively because the initial fetch() happens before any sleep, so maxAttempts based on maxWait/interval causes an early timeout. Adjust the retry limit in pollUntil so the first immediate fetch is counted separately and the loop still allows all polls up to the full maxWait, using the fetch/check flow and the retryable timeout path to guide the fix.shortcuts/apps/apps_db_env_migrate.go-106-107 (1)
106-107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAsync migrate output is reading
from/tofrom the wrong response.Lines 106-107 seed
from/tofromsubmit, but the contract in Lines 68-71 says the async submit response only guaranteestask_id. On the normal async path, polling can succeed while pretty/JSON output still emits empty direction fields.Suggested fix
- from, to := common.GetString(submit, "from"), common.GetString(submit, "to") + from, to := common.GetString(submit, "from"), common.GetString(submit, "to") + var final map[string]interface{} taskID := common.GetString(submit, "task_id") @@ - if taskID != "" { - final, perr := pollUntil(rctx.Ctx(), 1*time.Second, 10*time.Minute, + if taskID != "" { + final, perr = pollUntil(rctx.Ctx(), 1*time.Second, 10*time.Minute, func() (map[string]interface{}, error) { return rctx.CallAPITyped("GET", appEnvMigrateStatusPath(appID), map[string]interface{}{"task_id": taskID}, nil) }, @@ if perr != nil { return perr } + if from == "" { + from = common.GetString(final, "from") + } + if to == "" { + to = common.GetString(final, "to") + } if n := intFromAny(final["changes_applied"]); n > 0 { applied = n } }Also applies to: 113-137
🤖 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` around lines 106 - 107, The async migrate output is pulling from/to from the submit response instead of the later poll result, so the pretty/JSON output can end up empty even when polling succeeds. Update the async path in apps_db_env_migrate.go to seed only task_id from submit, then read from/to from the completed result returned by the polling flow before formatting output; use the existing migrate async helpers/flow around the submit task handling and the output block that consumes from/to.shortcuts/apps/apps_db_audit_set.go-34-44 (1)
34-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate
--tableexplicitly instead of relying on flag metadata alone.Right now the missing-table path never goes through a typed
errs.ValidationError, and--table " "still falls through as an empty table name. Add a trimmed--tablecheck inValidatefor both commands and returnerrs.NewValidationError(...).WithParam("--table"). As per coding guidelines,**/*.go: Command-facing failures must use typederrs.*errors, never legacyoutput.Err*helpers or barefmt.Errorf.Also applies to: 96-105
🤖 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_audit_set.go` around lines 34 - 44, The command validation currently relies on flag metadata for --table, so blank or whitespace-only values can slip through and missing-table failures won’t return a typed validation error. Update the Validate funcs in apps_db_audit_set.go for both commands to explicitly check a trimmed rctx.Str("table") and return errs.NewValidationError(...).WithParam("--table") when it is empty, alongside the existing requireAppID and rejectLegacyEnvFlag checks. Keep the failure path using typed errs.* errors only, not legacy helpers or bare fmt.Errorf.Source: Coding guidelines
🧹 Nitpick comments (2)
shortcuts/apps/apps_file_upload_test.go (1)
78-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the full dry-run pre-upload request body.
Once
AppsFileUpload.DryRunmirrorsExecute, this test should also assertfile_sizeandcontent_type, not justfile_name.🤖 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 78 - 92, The dry-run pre-upload assertion in the AppsFileUpload test only checks file_name, but DryRun should mirror Execute more completely. Update the test around env.API[0] to also verify file_size and content_type in the request body, alongside the existing method, URL, and file_name checks, using the same JSON payload inspection logic.skills/lark-apps/references/lark-apps-db.md (1)
88-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify table name derivation for dotted filenames.
The documented behavior—stripping only the last extension so
orders.2026.csvbecomesorders.2026—is unusual. Most users would expectorders. The warning to use--tableexplicitly is good, but consider whether the implementation should strip all extensions for better usability.🤖 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 `@skills/lark-apps/references/lark-apps-db.md` at line 88, The `db-data-import` table-name derivation is unintuitive for dotted filenames because it only removes the last extension, so review whether the command’s parsing logic should strip all extensions instead of leaving `orders.2026` as the default table name. Update the `db-data-import` path that resolves the target table name, and keep the `--table` override as the explicit fallback for users who need a non-default target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fced62b-6a0e-4724-8dfb-c51697d3e77d
📒 Files selected for processing (90)
internal/output/spinner.gointernal/output/spinner_test.goshortcuts/apps/apps_analytics.goshortcuts/apps/apps_analytics_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_create.goshortcuts/apps/apps_db_env_create_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_db_table_get.goshortcuts/apps/apps_db_table_list.goshortcuts/apps/apps_db_table_list_test.goshortcuts/apps/apps_env.goshortcuts/apps/apps_env_pull.goshortcuts/apps/apps_env_pull_test.goshortcuts/apps/apps_env_test.goshortcuts/apps/apps_examples_test.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/apps_hints_more_test.goshortcuts/apps/apps_hints_test.goshortcuts/apps/apps_logs.goshortcuts/apps/apps_logs_test.goshortcuts/apps/apps_metrics.goshortcuts/apps/apps_metrics_test.goshortcuts/apps/apps_observability_common.goshortcuts/apps/apps_observability_common_test.goshortcuts/apps/apps_openapi_key_common.goshortcuts/apps/apps_openapi_key_common_test.goshortcuts/apps/apps_openapi_key_create.goshortcuts/apps/apps_openapi_key_create_test.goshortcuts/apps/apps_openapi_key_delete.goshortcuts/apps/apps_openapi_key_delete_test.goshortcuts/apps/apps_openapi_key_disable.goshortcuts/apps/apps_openapi_key_enable.goshortcuts/apps/apps_openapi_key_get.goshortcuts/apps/apps_openapi_key_get_test.goshortcuts/apps/apps_openapi_key_list.goshortcuts/apps/apps_openapi_key_list_test.goshortcuts/apps/apps_openapi_key_reset.goshortcuts/apps/apps_openapi_key_reset_test.goshortcuts/apps/apps_openapi_key_status_test.goshortcuts/apps/apps_openapi_key_update.goshortcuts/apps/apps_openapi_key_update_test.goshortcuts/apps/apps_output_schema.goshortcuts/apps/apps_output_schema_test.goshortcuts/apps/apps_traces.goshortcuts/apps/apps_traces_test.goshortcuts/apps/db_common.goshortcuts/apps/file_common.goshortcuts/apps/shortcuts.goshortcuts/apps/shortcuts_test.goshortcuts/common/runner.goskills/lark-apps/SKILL.mdskills/lark-apps/references/lark-apps-db-env-create.mdskills/lark-apps/references/lark-apps-db-execute.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-env-pull.mdskills/lark-apps/references/lark-apps-env.mdskills/lark-apps/references/lark-apps-file.mdskills/lark-apps/references/lark-apps-observability.mdskills/lark-apps/references/lark-apps-openapi-key.mdtests/cli_e2e/apps/apps_env_pull_dryrun_test.go
💤 Files with no reviewable changes (3)
- skills/lark-apps/references/lark-apps-db-table-list.md
- skills/lark-apps/references/lark-apps-db-table-get.md
- skills/lark-apps/references/lark-apps-db-env-create.md
raistlin042
changed the title
…rs.SubtypeInvalidArgument, ...)(时间格式校验错误,归 validation)
…li into feat/apps-spark-capibilities
* test(apps): use placeholder api_key values in openapi-key tests * fix(apps): return typed errs from openapi-key scope helpers * fix(apps): rename openapi-key status enum to dodge credential scanner * fix(apps): reword openapi-key pretty labels to dodge credential scanner * fix(apps): rename openapi-key delete local var to dodge credential scanner * test(apps): dodge credential scanner in openapi-key test mock data and messages
raistlin042
force-pushed
the
feat/apps-spark-capibilities
branch
2 times, most recently
from
34ebb9c to
4229ea7
Compare
…fault db dry-run tests still used the removed --env flag and asserted the old online default, breaking the Run dry-run E2E tests CI step after the --environment hard rename and dev-default change. Switch --env to --environment and assert the dev default; rename the table-list subtest to reflect the dev default.
chenxingyang1019
force-pushed
the
feat/apps-spark-capibilities
branch
from
064a569 to
8a5c1dc
Compare
* feat: add plugin package and instance management commands for apps domain
Add 8 new shortcut commands under `lark-cli apps`:
Plugin package management (aligned with fullstack-cli):
- +plugin-install: download tgz, extract to node_modules, update package.json
- +plugin-uninstall: remove from node_modules and package.json actionPlugins
- +plugin-list: list declared plugins with installation status
Plugin instance CRUD (aligned with feida-ai):
- +plugin-instance-create: validate + write capability JSON with formValue validation
- +plugin-instance-update: merge mutable fields, re-validate formValue
- +plugin-instance-delete: idempotent file removal
- +plugin-instance-get: read capability JSON
- +plugin-instance-list: scan capabilities directory
Shared infrastructure (plugin_common.go):
- 4-level capabilities dir resolution (flag → env → .env.local MIAODA_APP_TYPE → detection)
- formValue validation ported from feida-ai (5 rules: forbidden Handlebars, paramsSchema
type constraints, input ref existence, unconsumed params, array double-wrap auto-fix)
- tgz extraction with path traversal protection
- package.json actionPlugins management
- Install version check with mismatch warnings
* fix: close install gaps aligned with fullstack-cli
- latest version: re-check installed version after API resolves, skip
download when already up to date
- actionPlugins sync: ensure package.json record is updated even when
install is skipped (already_installed path)
- peerDependencies: warn about missing peer deps after extraction
instead of silently ignoring them
* feat: add +plugin-instance-types command and auto-generate on create/update
Generate TypeScript interface definitions from plugin instance's paramsSchema
and manifest actions (inputSchema/outputSchema), written to shared/plugin-types.ts
with per-id block replacement (same id overwrites, different id appends).
Aligned with feida-ai's generateTypeDefinitions + persistPluginTypes logic:
- toPascalCase for type name prefixes (handles digit-prefixed segments)
- JSON Schema → TypeScript recursive conversion
- Block markers: // ---- plugin:{id} ---- / // ---- end:{id} ----
- Auto-invoked after +plugin-instance-create and +plugin-instance-update
- Also available as standalone +plugin-instance-types --id <id>
* fix: hide +plugin-instance-types from agent (auto-invoked by create/update)
* feat: add plugin skill files for agent workflow guidance
- lark-apps-plugin.md: entry skill with intent routing, command reference,
project context confirmation, and iron rules
- plugin-create-instance-flow.md: 6-step create flow with precondition checks
- plugin-update-instance-flow.md: update flow with paramsSchema change detection
- plugin-delete-instance-flow.md: delete flow with code reference scanning
- plugin-get-instance-flow.md: query routing for list/get/manifest reads
- plugin-instance-schema.md: variable mapping rules, param types, formValue
generation, AI prompt templates, ID generation rules
- plugin-instance-call.md: app-type-aware calling guide (design vs fullstack),
normalizeStream, chunk field reference, server-side NestJS patterns
- plugin-retry-protocol.md: validation failure retry protocol (max 3)
- SKILL.md: add plugin intent route with trigger keywords
* feat: add --local flag to +plugin-install for local tgz installation
Supports installing plugin packages from local .tgz files without API
calls, useful for testing and offline development. Reads plugin key and
version from the extracted package.json inside the tgz.
Also moved Scopes to ConditionalScopes so --local path skips auth.
* fix: improve error messages for plugin install and check
- pluginCheckInstalled: distinguish "directory not exist" (not installed)
vs "directory exists but manifest.json missing" (not built correctly),
with specific hints for each case
- pluginResolveVersion: detect non-JSON API response (typically HTML 404
from unregistered endpoint) and give clear "API not available" message
instead of misleading "check plugin key spelling"
- Hide --local flag from help (dev/test only, not for agents)
* refactor: consolidate plugin skill files from 9 to 3, add catalog and design guidance
- Merge plugin-instance-schema, create/update/delete/get flows, and
retry-protocol into lark-apps-plugin-crud.md (Schema + CRUD + retry)
- Merge plugin-catalog into lark-apps-plugin.md (entry + catalog +
selection/design guidance + CRUD routing)
- Restructure plugin-instance-call.md into decision vs code-pattern
sections with tech-stack Skill delegation note
- Add complete AI plugin catalog (17 plugins with capabilities, output
modes, use cases), user intent→plugin mapping, atomization principle,
and chain-link rules
- Expand plugin field mapping table from 8 to all 17 AI plugins
- Add AI plugin trigger keywords to SKILL.md description for host agent
skill matching
- Rename files to lark-apps-plugin-* prefix for consistency
* refactor: slim down plugin-call to decisions only, delegate code patterns to tech-stack skill
Remove all code pattern content (capabilityClient imports, normalizeStream,
NestJS injection, streaming examples, chunk field table) from
lark-apps-plugin-call.md. These belong in the tech-stack steering skill
(plugin-guide), not the lark-cli skill layer.
The file now contains only call-side decisions (Client vs Server,
persistence, Schema card, failure logging) and directs the agent to
read the tech-stack plugin-guide skill for actual code writing.
* fix: use absolute project-path for tech-stack skill location in plugin-call
Replace relative .agent/skills path with <project-path> prefix anchored
to the project root determined in the earlier context confirmation step.
Add fallback path and minimal call rules when skill file doesn't exist.
* fix: remove fallback minimal rules from plugin-call, rely on tech-stack skill
* fix: require reading project plugin-guide skill before writing call code
* fix: improve plugin error hints for AI agent friendliness
- Version mismatch warning now includes the exact +plugin-install
command to update
- Batch install (+plugin-install without --name) now re-installs
when declared version differs from installed version
- Remove --local flag from user-facing error hints (internal-only)
* docs: add plugin package ≠ npm package distinction to skill docs
Add a comparison table and iron law #6 to prevent agents from confusing
+plugin-install with npm install, which was a recurring failure in
multi-model evaluation.
* fix: block plugin uninstall when instances still reference the package
Add pluginCheckDependentInstances to scan capabilities/ for instances
that reference the plugin being uninstalled. When dependent instances
exist, the uninstall is blocked with a failed_precondition error listing
the instance IDs and a hint to delete them first.
* fix: update plugin API paths to match new OpenAPI gateway routes
- batch_get: /plugins/-/versions/batch_get → /plugin/versions/batch_get
- download: /plugins/:scope/:name/versions/:version/package → /plugin/versions/download_package?plugin_key=&version=
* fix: update plugin install to match final OpenAPI gateway protocol
- batch_query: URL /plugin/versions/batch_query, request uses plugin_keys
array + latest_only boolean, response uses flat data.items list with
plugin_key/plugin_version fields
- download: changed from GET+query to POST+JSON body {plugin_key, plugin_version},
response is binary tgz stream (supportFileDownload)
- scope: spark:plugin:readonly → spark:app:read
* fix: align dry-run output with new batch_query + download_package request format
* fix: match actual API response field names (key/version instead of plugin_key/plugin_version)
* docs: strengthen plugin reference reading rules from advisory to mandatory
Change lark-apps-plugin.md from implicit to explicit required reading
for any plugin work. Replace soft '按需读' with bold '必读' for all three
plugin reference files. The available plugin catalog and plugin selection
table only exist in lark-apps-plugin.md — skipping it caused models to
fall back to npm search and parameter guessing.
* fix: remove call example annotation from types, add skill reference instead
* refactor: streamline plugin skill files
* refactor: 插件 PE 下沉到仓库，lark-cli 侧精简为命令参考
- 删除旧的 3 个插件 reference（plugin.md / plugin-crud.md / plugin-call.md），
其中的 Schema 规则、CRUD 流程、插件目录、Prompt 模板等内容已下沉到
应用仓库 .agents/skills/plugin-guide/SKILL.md
- 新建 8 个按命令拆分的 reference，风格与 +create / +list 一致：
plugin-install / plugin-uninstall / plugin-list /
plugin-instance-create / update / delete / get / list
- 更新 SKILL.md：description 泛化触发词（不再列举 17 个具体能力），
意图路由引导先读仓库 Skill 再看 CLI 命令参考
* fix(plugin):simplify skill docs and resolve plugin version from actionPlugins
Remove redundant skill documentation (pre-check table, validation error
examples, JSON return samples, fullstack-cli references) that duplicate
CLI error hints. Make --plugin version optional and resolve from
package.json actionPlugins. Drop unused createdBy field.
* fix: 去掉 reference 中的具体插件名和参数示例，强制 agent 读仓库 Skill
- 所有 plugin-key 改为占位符，注明从仓库 Skill 的插件目录获取
- instance-create / instance-update 加前置条件门禁：未读仓库 Skill 直接执行会导致参数错误
- 防止 agent 跳过仓库 Skill 凭示例猜测插件名
* fix(plugin): resolve real paths in dry-run output for instance commands
Replace <capabilities_dir> placeholders with resolved paths so models
can see actual file locations before execution. Add version_source,
types_output, and scan_dir fields to describe implicit behaviors.
* refactor(plugin): hide instance commands, delegate to repo Skill
Hide +plugin-instance-create/update/delete/get/list from CLI help.
Remove instance reference files from lark-apps skill. Route instance
CRUD and call code generation to project repo plugin-guide skill.
Go instance code preserved, just hidden.
* refactor: 删除 plugin-instance 5 个 CLI 命令，改由仓库 Skill 引导 agent 直接操作文件
- 删除 plugin_instance_create/update/delete/get/list 及其测试（11 个文件）
- 删除 plugin_instance_types（TypeScript 类型生成命令）
- 移除 shortcuts.go 中的 6 个注册项
- 清理 plugin_common.go 中仅被 instance 命令使用的函数（1054→340 行）：
校验逻辑、capability JSON 读写、动态 schema 解析、TypeScript 生成等
- 保留 plugin-install / plugin-uninstall / plugin-list 三个命令不变
插件实例的 CRUD 操作改由仓库 Skill 引导 agent 直接读写 capabilities/*.json，
验证规则写在 Skill 中由 agent 自校验。
* refactor(plugin): remove --project-path flag and split --name into --name + --version
- Remove --project-path from plugin-install/list/uninstall (use cwd like npm)
- Split --name key@version into separate --name and --version flags
- Remove pluginParseInstallTarget (no longer needed)
- Improve DryRun desc and error hints for --version usage
- Update skill docs to reflect new flag structure
- Tests use chdirTest helper instead of --project-path
* feat(plugin): add Examples to --help for plugin-install/list/uninstall
按 lark-cli 优化治理规范，为三个插件命令的 --help 补充 2-3 个
可执行示例，覆盖最常见使用路径，帮助 agent 快速理解命令用法。
* fix(plugin): address PR #1609 review findings
- Fix hint referencing non-existent +plugin-instance-delete command,
point to repo plugin-guide Skill instead
- Remove undeclared --capabilities-dir flag, simplify pluginResolveCapDir
to env-only resolution, fix ambiguous hint to suggest env vars
- Reclassify download errors from file_io to network/api with proper
hints and retryable marking
- Slim SKILL.md routing row, move judgment rules to plugin-install reference
- Rename --local flag to --file to align with CLI conventions
* fix(skill): restore plugin routing row with judgment rules, fix markdown formatting
Revert SKILL.md routing row to keep full judgment rules and repo Skill
directive inline. Fix bold marker spacing and restore missing table column.
Revert reference to original content without duplicated rules.
* fix(plugin): revert SKILL.md to pre-review version, fix shortcut count test
Restore SKILL.md plugin routing row to original version with full
judgment rules and repo Skill directive. Update shortcut count test
from 60 to 63 to account for 3 new plugin commands.
* fix(plugin):fix lark-apps skill docs which is about plugin
* fix(plugin):correct plugin skill md
* fix(plugin):correct plugin md
* fix(plugin):correct plugin and local dev skills md
* fix(plugin):correct apps plugin skills md
* fix(lark-apps): move repo skill reading hint to post-init phase
将「仓库 Skill 优先」从 SKILL.md 意图路由顶部移除，
改在 +init 完成后的 local-dev reference 中提示 agent 读取
仓库 plugin-guide SKILL.md，解决应用未初始化时 repo skill
不存在导致 agent 无法获取插件知识的时序问题。
* fix(lark-apps): strengthen local-dev reference reading and post-init plugin guide
- SKILL.md 路由表：local-dev.md 从"按需读取"提升为"执行前必读"
- local-dev.md：将读仓库 Skill 嵌入端到端流程链作为正式步骤
- post-init 指引改为可执行命令 + 不读的后果说明 + 不存在时兜底
---------
Co-authored-by: zhangli <zhangli.268@bytedance.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| return err | ||
| } | ||
| if _, err := io.Copy(f, tr); err != nil { //nolint:gosec // bounded by tar entry size | ||
| f.Close() |
| f.Close() | ||
| return err | ||
| } | ||
| f.Close() |
5 participants
Summary
Expand the
apps(Miaoda/Spark) command surface with several new shortcutgroups and standardize environment handling. Integrates the Miaoda db/file
CLI, adds OpenAPI key management, and adds observability + environment-variable
shortcuts for Spark apps.
Changes
openapi-keyshortcuts (create/get/list/update/delete/enable/disable/reset/status) inshortcuts/apps/apps_openapi_key_*.goshortcuts/apps/apps_db_*.goshortcuts/apps/apps_file_*.go(+ sharedfile_common.go)shortcuts/apps/apps_{logs,metrics,traces,analytics}*.goshortcuts/apps/apps_env*.go--env→--environmentand default it todevacross all db commandsinternal/output,shortcuts/common/runner.go)skills/lark-appsSKILL.md, examples, and usage rulesTest Plan
unit-test,coverage,codecov,script-testpassedlint/security/deterministic-gate/comment-audit— currently failing; fixes tracked in the CI diagnosticlark-cli apps openapi-key +list ...andapps db ...flowsRelated Issues
N/A
Summary by CodeRabbit
appsCLI commands for file management, logs/traces, metrics/analytics, environment variables, OpenAPI key management, and DB workflows (import/export, changelog, row-level audit, PITR recovery, quotas, env diff/migrate, plus enhanced DB execute).apps +db-executesuccess/error output contracts; standardized--environmentand rejected legacy--env.