feat(apps): emit typed error envelopes across the apps domain#1288
Merged
Conversation
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates the ChangesApps Domain Typed Error Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/L
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@361b8bb71217fb0ea93d3bb472a81044772179c6🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-apps -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1288 +/- ##
==========================================
+ Coverage 72.75% 72.83% +0.07%
==========================================
Files 730 732 +2
Lines 69034 69140 +106
==========================================
+ Hits 50228 50356 +128
+ Misses 15034 15003 -31
- Partials 3772 3781 +9 ☔ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/apps/html_publish_client_test.go (1)
21-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd config-dir isolation in the test runtime helper.
newAppsClientRuntimeusescmdutil.TestFactorybut does not isolate config state. This can leak state across tests when config-backed paths are touched.💡 Suggested fix
func newAppsClientRuntime(t *testing.T) (*common.RuntimeContext, *httpmock.Registry) { t.Helper() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) cfg := &core.CliConfig{ AppID: "test-app-" + strings.ToLower(t.Name()), AppSecret: "test-secret", Brand: core.BrandFeishu, UserOpenId: "ou_test", }As per coding guidelines:
**/*_test.gomustUse t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate config state in tests.🤖 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/html_publish_client_test.go` around lines 21 - 31, The test helper newAppsClientRuntime should isolate config state: before calling cmdutil.TestFactory set the environment var LARKSUITE_CLI_CONFIG_DIR to a temp dir via t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()), so tests don't share config-backed paths; update newAppsClientRuntime to call t.Setenv(...) as the first action in the function (reference newAppsClientRuntime and the call site cmdutil.TestFactory).
🤖 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.
Outside diff comments:
In `@shortcuts/apps/html_publish_client_test.go`:
- Around line 21-31: The test helper newAppsClientRuntime should isolate config
state: before calling cmdutil.TestFactory set the environment var
LARKSUITE_CLI_CONFIG_DIR to a temp dir via t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()), so tests don't share config-backed paths; update
newAppsClientRuntime to call t.Setenv(...) as the first action in the function
(reference newAppsClientRuntime and the call site cmdutil.TestFactory).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cad262b6-f9b7-409c-93ee-75177089ac7c
📒 Files selected for processing (17)
.golangci.ymlinternal/errclass/codemeta_apps.golint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/apps/apps_access_scope_get.goshortcuts/apps/apps_access_scope_set.goshortcuts/apps/apps_create.goshortcuts/apps/apps_create_test.goshortcuts/apps/apps_errors.goshortcuts/apps/apps_html_publish.goshortcuts/apps/apps_html_publish_test.goshortcuts/apps/apps_list.goshortcuts/apps/apps_update.goshortcuts/apps/html_publish_client.goshortcuts/apps/html_publish_client_test.goshortcuts/apps/html_publish_tarball.goshortcuts/apps/walk_html_publish_candidates.go
evandance
force-pushed
the
feat/errs-migrate-apps
branch
from
deb12eb to
8920914
Compare
evandance
changed the title
github-actions
Bot
added
size/M
evandance
force-pushed
the
feat/errs-migrate-apps
branch
7 times, most recently
from
e76456c to
32d9a82
Compare
github-actions
Bot
added
size/L
evandance
force-pushed
the
feat/errs-migrate-apps
branch
4 times, most recently
from
c629720 to
e16b6d9
Compare
Apps command errors previously surfaced as legacy output.Err* envelopes or untyped helper errors, so agent and script callers had to recover validation, API, subprocess, credential, and file failures from prose. Every error producer in the apps domain now emits typed errs.* errors with stable type/subtype/param/hint/code/log_id/retryable metadata: - Flag and input validation carries validation/invalid_argument with param; state preconditions (missing git/npx binary, login mismatch, foreign git helper) carry validation/failed_precondition with recovery hints. - A new internal/external_tool subtype classifies runtime failures of tools the CLI shells out to (git clone/push, npx scaffold, git config): the tool output is carried in the message so agents act on it instead of retrying with different flags. - Standard apps API calls use runtime.CallAPITyped; the multipart HTML publish and the git-credential issue endpoint classify responses through the shared classifier, so generic codes (missing scope) and HTTP 5xx keep their canonical category/subtype/retryable classification. - Local credential/state storage failures classify as internal/storage; malformed subprocess and API payloads as internal/invalid_response; defense-in-depth invariant guards report internal errors instead of blaming user input. - +db-execute statement failures report via the partial-failure contract: per-statement results stay machine-readable on stdout (ok:false) with a non-zero exit, in both json and pretty formats. - shortcuts/apps/ (including gitcred/) is locked into the golangci and errscontract migrated-path guards; the full tree is lint-clean. Validation: gofmt, go vet, go build, go test ./shortcuts/apps/... ./errs and the lint module, errscontract repo-wide clean, golangci-lint clean both diff-scoped and full-tree, make unit-test.
evandance
force-pushed
the
feat/errs-migrate-apps
branch
from
e16b6d9 to
361b8bb
Compare
liangshuo-1
approved these changes
Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Apps errors previously left several command-facing paths as legacy
output.Err*/output.Errorfenvelopes or untyped helper errors, so callers had to recover validation, API, file, subprocess, and credential failures from prose. This PR makes every error producer in the apps domain — flag validation, local HTML publish file handling, SDK/API boundaries, Miaoda HTML publish business errors, app init/scaffold subprocess orchestration, the local SQL execution flow, and the git credential helper stack — emit typederrs.*errors with stabletype,subtype,param/params,hint,code,log_id, andretryablemetadata where available.What to focus on (reviewer guide)
validation/invalid_argumentplusparam; missingindex.htmlisvalidation/failed_precondition; local pack/read failures carry typed validation or internal/file_io classification.runtime.CallAPITyped; multipart HTML publish keeps rawDoAPIbecause it posts multipart form data, but classifies responses viaruntime.ClassifyAPIResponseand wraps SDK-boundary failures withclient.WrapDoAPIError.90001/90002are scoped to this endpoint, so both their classification (90002→api/not_found) and their recovery hints live in the apps domain instead of the global API code table — the global table stays reserved for service-namespaced codes. Enrichment never overrides a stronger upstream classification and preservescode/log_id.+initgit/npx orchestration, git config helper): a missing git/npx binary classifies asvalidation/failed_preconditionwith an install hint; a runtime failure of an external tool (clone, push, npx scaffold) classifies under the newinternal/external_toolsubtype with the tool output in the message and the cause attached, so agents read the tool output instead of retrying with different flags; malformed envelopes from lark-cli subprocess calls classify asinternal/invalid_response.storage.go, gitcred lock/store): failures classify asinternal/storage; the not-logged-in path is a typedauth/token_missingwith a login hint.+db-execute): a failed statement in DBA mode now reports viaOutPartialFailure— per-statement results plusstatement_index/error_code/rolled_back/noteland on stdout as machine-readableok:falsedata, with a non-zero partial-failure exit signal, instead of a legacyExitErrordetail blob.urlnow fails asinternal/invalid_responseinstead of returning an empty url..golangci.ymlandlint/errscontractoptshortcuts/apps/(including thegitcredsubpackage) into typed-only, no-bare-wrap, no-legacy-helper, legacy-envelope, and legacy-runtime-helper guards; the full apps tree is lint-clean under those guards.Changes
errs.*builders and apps-local helper functions.runtime.CallAPItoruntime.CallAPITyped.DoAPI, while routing response classification throughruntime.ClassifyAPIResponseand SDK boundary errors throughclient.WrapDoAPIError.90002→api/not_found) and keep their recovery hints local, since these codes are endpoint-scoped rather than service-global; hints are normalized to English.internal/invalid_response.+db-executestatement failures to the partial-failure contract (OutPartialFailure): full per-statement results stay machine-readable on stdout, the exit code stays non-zero.internal/external_toolerror subtype for runtime failures of tools the CLI shells out to; classify+init/git-config subprocess runtime failures under it, missing git/npx binaries asvalidation/failed_preconditionwith install hints, and subprocess envelope parse failures plus a non-http(s)repository_urlasinternal/invalid_response.storage.go, gitcred lock) asinternal/storage, and the not-logged-in path as typedauth/token_missing.*output.ExitErrorpassthrough arm fromgitCredentialLocalError(everything the apps domain and shared runtime produce is typed).shortcuts/apps/gitcred/) to golangci and errscontract migrated-path guards to prevent legacy error regressions.errs.ProblemOfmetadata and the partial-failure stdout contract instead of legacyoutput.ExitErrordetails, and add helper-level tests for path, file I/O, API boundary, and HTML publish hint enrichment.LARKSUITE_CLI_CONFIG_DIRso they do not read or write user config state.Test Plan
gofmt -l .go vet ./...go test ./shortcuts/apps/... ./internal/errclass ./internal/clientgo test ./errscontractfrom thelintmodulego run -C lint . ..— 0 violations repo-widegolangci-lint v2.1.6 run --new-from-rev=origin/main— 0 issuesgolangci-lint v2.1.6 run ./shortcuts/apps/...(full-tree, not diff-scoped) — 0 issuesmake unit-testOutput changes for consumers
type/subtypeplus stable fields such asparam,hint,code,log_id, andretryablewhere the underlying failure provides them.code/log_idand gain command-specific recovery hints; code90002is classifiedapi/not_found.internal/invalid_responseinstead of printing an empty url.+db-executestatement failures emit anok:falseenvelope on stdout carrying per-statement results,statement_index,error_code,error_message,rolled_back, and a re-run guidancenote, with a non-zero exit code.+initfailures report typed classifications instead of opaqueapi_errorstrings: missing binaries asvalidation/failed_preconditionwith install hints, tool runtime failures asinternal/external_toolcarrying the tool output.Out of scope / deferred
Related Issues
N/A