fix(apps): detect the no-database failure by code or message - #2217
Conversation
github-actions
Bot
added
the
size/M
|
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:
📝 WalkthroughWalkthroughThe app error flow detects database initialization failures through current and legacy codes or matching server messages. ChangesDatabase error detection and recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/apps/common_test.go`:
- Around line 98-135: Extend the affected tests around the legacy, unknown-code,
and unrelated-failure cases to assert the resulting error’s expected Category
and Subtype, retaining each input error for comparison. Add a wrapped-cause
scenario and verify the error returned through errs.ProblemOf preserves that
cause chain, without asserting a nonexistent Param field.
In `@tests/cli_e2e/apps/apps_env_pull_live_test.go`:
- Around line 31-33: Remove the LARKSUITE_CLI_CONFIG_DIR requirement from the
setup of this live workflow test. Use the established live-token gate for
skipping unavailable credentials, while retaining
LARK_CLI_E2E_APPS_ENV_PULL_APP_ID as the fixture gate.
🪄 Autofix
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 Plus
Run ID: 3863f1f6-3e80-4aa0-8922-c48d80da1c81
📒 Files selected for processing (4)
shortcuts/apps/common.goshortcuts/apps/common_test.gotests/cli_e2e/apps/apps_env_pull_live_test.gotests/cli_e2e/apps/coverage.md
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@329b571ec0ee9ae08b1c7cb8270370c16f951a1e🧩 Skill updatenpx skills add larksuite/cli#fix/apps-no-database-error-match -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2217 +/- ##
==========================================
+ Coverage 76.22% 76.34% +0.12%
==========================================
Files 987 991 +4
Lines 104280 106029 +1749
==========================================
+ Hits 79486 80952 +1466
- Misses 18751 18942 +191
- Partials 6043 6135 +92 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d4162da to
4d1a04a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
chenxingyang1019
changed the title
|
Addressed the two review threads:
Also closed the coverage gap the report flagged: |
The recovery flow for "db command against an app that has no database" keyed on one business code (500002759). The server has since renumbered that case to 400002465, which silently disabled the flow: users now see the raw internal message about workspace / app-id mapping and lose the cloud-development recovery steps entirely. Nothing catches the regression. There is no compile error, the unit tests compare against the same constant they set, and the dry-run E2E does not exercise a real response — the failure only shows up against a server that has already renumbered. Detect on code OR message instead. Both known codes are kept, plus narrow lowercase markers of the server's internal wording. The two channels have opposite failure modes: a code is precise but gets renumbered, a message survives renumbering but breaks on rewording or localization. Requiring either to match means one channel changing degrades nothing, and only a simultaneous change of both regresses. Markers stay deliberately narrow. "no db branch" in particular must not also swallow env-pull's "invalid db branch" case, which needs its own hint; a comment records that widening them requires a test proving the neighbours still pass through. Classification and the cause chain are untouched: the helper still mutates the problem in place and returns the same error value.
Review feedback: the new subtests checked only Message and Hint, so a change that reclassified the failure — or replaced the error value and dropped the cause chain — would still have passed. Each case now asserts Category, Subtype and Code are untouched by the rewrite, and that the helper returns the same error value. Inputs use a concrete subtype rather than Unknown, so a clobbered classification is actually observable. One new case wraps a cause and asserts errors.Is still finds it through the rewrite. Also covers the predicate's defensive nil guard, which withAppsHint cannot reach on its own (ProblemOf returns ok=false for untyped errors), closing the two uncovered lines the coverage report flagged. Both withAppsHint and isAppNoDatabaseError are now at 100%.
2f82630 to
b877e53
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/errclass/codemeta_spark_test.go`:
- Around line 34-37: Add parameter assertions to the shared validation-case test
covering codes 400002465 and 500002759: use errors.As to extract
*errs.ValidationError from the returned error and assert its Param is empty,
while preserving the existing category, subtype, code, log ID, and retryability
checks.
🪄 Autofix
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 Plus
Run ID: bfae2f15-b270-48dc-b2c7-d452413a452c
📒 Files selected for processing (2)
internal/errclass/codemeta_spark.gointernal/errclass/codemeta_spark_test.go
Three codes reaching the Apps db commands were absent from the Spark table, so BuildAPIError fell through to the CategoryAPI + SubtypeUnknown catch-all and the envelope carried no usable classification. "App has no database yet" registers as Validation / FailedPrecondition: the app resolves fine and the request is well-formed, but a prerequisite the caller must create first is missing, so retrying unchanged can never succeed. This moves its exit code from 1 to 2 — "fix the state" rather than "the call failed" — and a test pins that so a future reclassification has to be deliberate. Two codes cover it because the server renumbered the case into the 4xx band; the legacy one stays for older servers. "Table does not exist" registers as API / NotFound, an ordinary missing-resource lookup with no exit-code change. SubtypeNotFound has no APIHint default, which matters here: the Apps layer fills its command-scoped hint only when the classifier left Hint empty, so a context-free default would displace the more actionable one. A test guards that too.
0de0c88 to
329b571
Compare
2 participants
Problem
withAppsHintspecial-cases one failure — a db command run against an app that has no database yet — by rewriting the server's message into a user-facing explanation and attaching a cloud-development recovery hint. That special case was keyed on a single business code,500002759.The server has since renumbered this case to
400002465, moving it into the 4xx band with the rest of its client-class errors. The branch no longer fires, so:+db-env-create, which is not a valid next step for an app that has no database at all.Nothing catches a renumber. There is no compile error, and the unit tests compare against the same constant they set, so they pass either way.
Separately, none of the db-domain codes were registered in the Spark code table, so
BuildAPIErrorfell through to theCategoryAPI+SubtypeUnknowncatch-all: the envelope carried no usable classification and an agent could only branch on the raw number.Change
Detect the no-database failure on code OR message. Codes
400002465(current) and500002759(legacy, kept so older servers still match), plus narrow lowercase markers of the server's internal wording. The two channels have opposite failure modes — a code is precise but gets renumbered, a message survives renumbering but breaks on rewording or localization. Requiring either to match means one channel changing degrades nothing, and only a simultaneous change of both regresses.Markers stay deliberately narrow:
"no db branch"must not also swallow env-pull's"invalid db branch"case, which needs its own hint. A comment records that widening them requires a test proving the neighbours still pass through.Classify the db-domain codes. No-database registers as
Validation/FailedPrecondition— the app resolves fine and the request is well-formed, but a prerequisite the caller must create first is missing, so retrying unchanged can never succeed. This moves its exit code from 1 to 2 ("fix the state" rather than "the call failed"). Table-not-found registers asAPI/NotFound, an ordinary missing-resource lookup with no exit-code change.The two layers stay separate on purpose: the code table sets classification, the shortcut layer owns the message rewrite and the recovery hint, and its message channel is what still fires if the codes are renumbered again.
Nothing outside these paths changes. For the no-database failure, the cause chain is untouched and the helper still mutates in place rather than returning a replacement error.
Tests
TestWithAppsHintgains cases for the legacy code, an unknown code matched by message (case-insensitive, substring), and a wrapped cause asserted to survive the rewrite. Negative controls prove"invalid db branch: dev","数据表格不存在"and"permission denied"keep the caller's hint. Every case assertsCategory,SubtypeandCodeare unchanged and that the same error value comes back.withAppsHintandisAppNoDatabaseErrorare both at 100% statement coverage.On the classifier side, one test pins the exit code the no-database codes route to, so a future reclassification has to be deliberate; another pins that
SubtypeNotFoundleavesHintempty, since the shortcut layer fills its command-scoped hint only when the classifier left one empty.Summary by CodeRabbit