feat(apps): route init and html-publish by arch_type#1762
Conversation
github-actions
Bot
added
the
size/L
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds an environment-variable-derived ChangesApp creation and init flow updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1762 +/- ##
==========================================
- Coverage 74.42% 74.39% -0.04%
==========================================
Files 854 858 +4
Lines 88477 88988 +511
==========================================
+ Hits 65852 66205 +353
- Misses 17555 17674 +119
- Partials 5070 5109 +39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@db77bd6520f0fb045320eb10bf6523dfd5c9e6a9🧩 Skill updatenpx skills add larksuite/cli#feat/doubao-miaoda-integration -y -g |
anngo-nk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
shortcuts/apps/apps_meta.go (1)
25-30: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove the unused
ctxparameterqueryAppMetanever reads itscontext.Context, andrctx.CallAPITypedalready usesRuntimeContext.ctx, so the extra parameter and call-site plumbing can be dropped.🤖 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_meta.go` around lines 25 - 30, Remove the unused context parameter from queryAppMeta and update its callers accordingly. The function currently never reads ctx, and rctx.CallAPITyped already relies on RuntimeContext.ctx, so simplify the signature of queryAppMeta and drop the extra call-site plumbing wherever queryAppMeta is invoked.shortcuts/apps/apps_init.go (1)
88-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDrop the unused meta branches from
resolveTemplate, or pass app meta into DryRun if the preview should match execute-time selection.resolveTemplateonly receivesniltoday, so theAppType/ArchTypecases never run; the dry-run scaffold string stays generic instead of reflecting the meta-awareapp initbehavior.🤖 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_init.go` around lines 88 - 98, The DryRun preview in apps_init.go is not using the meta-aware template selection path, so the scaffold description can diverge from what execute-time initialization does. Update the DryRun closure that builds the common.NewDryRunAPI preview to either pass the app meta into resolveTemplate or remove the unused AppType/ArchType branches from resolveTemplate if meta is intentionally unavailable; make sure the values used by Set("template", ...) and the scaffold string reflect the same selection logic as the real app init flow.shortcuts/apps/apps_html_publish.go (2)
281-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
pollDeployStatusbypasses theappsHTMLPublishTOSClientinterface.
runHTMLPublishTOStakes both atosClientinterface and a raw*common.RuntimeContextsolely to callpollDeployStatusdirectly, breaking the abstraction the interface exists for. This is why tests passnilforrctxand none exercise the polling branch — doing so withnilwould panic. Consider adding aPollStatus(ctx, appID, releaseID) (*deployResponse, error)method toappsHTMLPublishTOSClientso the whole flow is mockable and testable through one seam.🤖 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_html_publish.go` around lines 281 - 343, The polling path in runHTMLPublishTOS is bypassing the appsHTMLPublishTOSClient abstraction by calling pollDeployStatus with a raw *common.RuntimeContext. Add a PollStatus(ctx, appID, releaseID) method to appsHTMLPublishTOSClient and move the polling call behind that interface so runHTMLPublishTOS uses only tosClient. Update the mock/test implementations to cover the building/finished branch through the same seam and remove the need for passing nil rctx just to avoid the direct pollDeployStatus call.
128-140: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOutput schema diverges between legacy and TOS publish paths.
The legacy path exposes
out["url"], while the TOS path exposesout["online_url"]/out["release_id"]for the same+html-publishcommand (which hasHasFormat: true). Scripts parsing structured output for a stableurl/statusfield will silently break depending on the app's internalarch_type, which the caller has no way to predict.Also applies to: 281-343
🤖 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_html_publish.go` around lines 128 - 140, The `+html-publish` command is returning different structured fields in the legacy and TOS paths, so normalize the output schema in `apps_html_publish.go` and the shared publish flow used by `runHTMLPublishTOS`/legacy handling. Make both paths expose the same stable fields for `url` and build state (for example, a consistent URL plus status/release identifier), and update the `rctx.OutFormat` callback to emit that unified schema regardless of `arch_type`. This should be applied in the TOS branch here and in the corresponding legacy code path around the referenced publish logic.shortcuts/apps/html_publish_tos_client_test.go (1)
1-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the "building" + polling branch.
No test drives
deployResponse{Status: "building", ReleaseID: ...}throughrunHTMLPublishTOS, sopollDeployStatus's interaction and the resulting output fields (status/online_url/release_id/hint) are untested. This gap would have caught the status-masking issue flagged inapps_html_publish.go(a poll timeout/failure being reported as "building" instead of "failed"). NotepollDeployStatusis called directly onrctx(not through theappsHTMLPublishTOSClientinterface), so testing this branch requires a real*common.RuntimeContextrather thannil.🤖 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_tos_client_test.go` around lines 1 - 197, Add test coverage for the building/polling path in runHTMLPublishTOS by using a real common.RuntimeContext and a fake TOS client that returns deployResponse with Status set to "building" and a ReleaseID. Verify the branch exercises pollDeployStatus and asserts the final output fields status, online_url, release_id, and hint, so the behavior in appsHTMLPublishTOSClient and pollDeployStatus is covered. Also include a case where polling fails or times out to ensure the status is reported as failed rather than being left as building.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/apps/apps_html_publish.go`:
- Around line 324-341: The publish flow in apps_html_publish.go is treating
every non-success poll as “still building”; update the logic around
pollDeployStatus in the app publish response assembly so failed polls and
polled.Status == "failed" are surfaced as failures instead of falling through to
the release_id/hint branch. In the block that builds out for spec.AppID and
resp.ReleaseID, branch on pollErr and the returned polled.Status explicitly,
preserve the actual error/status in out, and only keep the current “finished”
path when pollDeployStatus succeeds with polled.Status == "finished".
- Around line 122-142: queryAppMeta is swallowing API/parsing failures by
returning nil, which causes the doubao-html branch in appsHTMLPublish to
silently fall back to the legacy multipart path. Update queryAppMeta to return
an error (or a typed result) for lookup failures, and change the caller in
appsHTMLPublish to only use the legacy multipart path on an explicit legacy
response, not on transient errors; keep the special-case routing for
meta.AppType and meta.ArchType intact.
In `@shortcuts/apps/apps_init_test.go`:
- Around line 1649-1728: Add a test covering runScaffold/scaffoldInitArgs when
meta is non-nil but AppType is not 7, since current coverage only checks static
HTML, doubao-html, nil meta, and explicit template override. Create a case in
TestRunScaffold_* that uses a regular app meta with a non-HTML AppType and
verifies the npx args choose the expected template/flags behavior. Use
runScaffold and the scaffoldInitArgs behavior to confirm whether
--app-type/--arch-type should be omitted or included for this non-HTML metadata
path.
In `@shortcuts/apps/html_publish_tos_client.go`:
- Around line 86-108: The polling loop in pollDeployStatus ignores context
cancellation because it uses time.Sleep directly. Update pollDeployStatus to
respect ctx.Done() during each poll interval by waiting on a select between a
timer and ctx.Done(), and return promptly when the context is cancelled. Keep
the existing CallAPITyped/status handling intact while making the loop exit
early on cancellation.
- Around line 57-66: The TOS upload path currently uses
http.DefaultClient.Do(req), which can block indefinitely if the request context
has no deadline. Update the upload logic around the existing request creation
and Do call to use a client with an explicit timeout or ensure the request
context is always given a deadline before calling Do. Keep the same error
handling via appsExternalToolError and preserve the current status-code check
and response closing behavior.
- Around line 34-49: The TOS upload path in GetUploadURL and the surrounding
upload flow currently classifies request construction, Do failures, and non-2xx
responses as file I/O or external tool errors, which is the wrong subtype for
transport/server failures. Update the error wrapping in the
appsHTMLPublishTOSAPI path to use the network error subtype for
http.NewRequestWithContext, client.Do, and response-status failures so callers
can treat these as retryable network issues consistently.
In `@shortcuts/apps/html_publish_zip_test.go`:
- Around line 99-113: The error-path tests for writeHTMLPublishZipEntry
currently validate only error text, so update
TestWriteHTMLPublishZipEntry_OpenFailure,
TestWriteHTMLPublishZipEntry_CopyFailure, and
TestWriteHTMLPublishZipEntry_RejectsPathTraversal to assert typed metadata with
errs.ProblemOf instead of strings.Contains. Check the returned error for the
expected category/subtype/param values, and also verify the underlying cause is
preserved where applicable. Use the writeHTMLPublishZipEntry helper and the
htmlPublishCandidate fields to keep the assertions aligned with the specific
failure mode being exercised.
---
Nitpick comments:
In `@shortcuts/apps/apps_html_publish.go`:
- Around line 281-343: The polling path in runHTMLPublishTOS is bypassing the
appsHTMLPublishTOSClient abstraction by calling pollDeployStatus with a raw
*common.RuntimeContext. Add a PollStatus(ctx, appID, releaseID) method to
appsHTMLPublishTOSClient and move the polling call behind that interface so
runHTMLPublishTOS uses only tosClient. Update the mock/test implementations to
cover the building/finished branch through the same seam and remove the need for
passing nil rctx just to avoid the direct pollDeployStatus call.
- Around line 128-140: The `+html-publish` command is returning different
structured fields in the legacy and TOS paths, so normalize the output schema in
`apps_html_publish.go` and the shared publish flow used by
`runHTMLPublishTOS`/legacy handling. Make both paths expose the same stable
fields for `url` and build state (for example, a consistent URL plus
status/release identifier), and update the `rctx.OutFormat` callback to emit
that unified schema regardless of `arch_type`. This should be applied in the TOS
branch here and in the corresponding legacy code path around the referenced
publish logic.
In `@shortcuts/apps/apps_init.go`:
- Around line 88-98: The DryRun preview in apps_init.go is not using the
meta-aware template selection path, so the scaffold description can diverge from
what execute-time initialization does. Update the DryRun closure that builds the
common.NewDryRunAPI preview to either pass the app meta into resolveTemplate or
remove the unused AppType/ArchType branches from resolveTemplate if meta is
intentionally unavailable; make sure the values used by Set("template", ...) and
the scaffold string reflect the same selection logic as the real app init flow.
In `@shortcuts/apps/apps_meta.go`:
- Around line 25-30: Remove the unused context parameter from queryAppMeta and
update its callers accordingly. The function currently never reads ctx, and
rctx.CallAPITyped already relies on RuntimeContext.ctx, so simplify the
signature of queryAppMeta and drop the extra call-site plumbing wherever
queryAppMeta is invoked.
In `@shortcuts/apps/html_publish_tos_client_test.go`:
- Around line 1-197: Add test coverage for the building/polling path in
runHTMLPublishTOS by using a real common.RuntimeContext and a fake TOS client
that returns deployResponse with Status set to "building" and a ReleaseID.
Verify the branch exercises pollDeployStatus and asserts the final output fields
status, online_url, release_id, and hint, so the behavior in
appsHTMLPublishTOSClient and pollDeployStatus is covered. Also include a case
where polling fails or times out to ensure the status is reported as failed
rather than being left as building.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f79badbd-9f44-4be3-9c6a-eb9e0cc0db2c
📒 Files selected for processing (11)
shortcuts/apps/apps_create.goshortcuts/apps/apps_create_test.goshortcuts/apps/apps_html_publish.goshortcuts/apps/apps_init.goshortcuts/apps/apps_init_test.goshortcuts/apps/apps_meta.goshortcuts/apps/apps_meta_test.goshortcuts/apps/html_publish_tos_client.goshortcuts/apps/html_publish_tos_client_test.goshortcuts/apps/html_publish_zip.goshortcuts/apps/html_publish_zip_test.go
|
|
||
| meta := queryAppMeta(ctx, rctx, spec.AppID) | ||
|
|
||
| // doubao-html (app_type=7, arch_type=4): zip + TOS path | ||
| if meta != nil && meta.AppType == 7 && meta.ArchType == 4 { | ||
| tosClient := appsHTMLPublishTOSAPI{runtime: rctx} | ||
| out, err := runHTMLPublishTOS(ctx, rctx.FileIO(), tosClient, rctx, spec) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| rctx.OutFormat(out, nil, func(w io.Writer) { | ||
| if url, ok := out["online_url"].(string); ok && url != "" { | ||
| fmt.Fprintf(w, "url: %s\n", url) | ||
| } else if rid, ok := out["release_id"].(string); ok { | ||
| fmt.Fprintf(w, "release_id: %s (still building)\n", rid) | ||
| } | ||
| }) | ||
| return nil | ||
| } | ||
|
|
||
| // Legacy html or meta query failed: existing multipart path |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether queryAppMeta swallows/logs errors or just returns nil.
rg -n -A20 'func queryAppMeta' shortcuts/apps/apps_meta.goRepository: larksuite/cli
Length of output: 863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the publish flow around the app-meta routing and the legacy fallback.
sed -n '1,220p' shortcuts/apps/apps_html_publish.go
# Inspect the meta lookup helper in context.
sed -n '1,120p' shortcuts/apps/apps_meta.goRepository: larksuite/cli
Length of output: 10144
Surface app-meta lookup failures queryAppMeta returns nil on any API or parsing error, so a doubao-html app hit by a transient lookup failure is routed to the legacy multipart endpoint with no signal. Propagate the failure, or fall back only on an explicit legacy response.
🤖 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_html_publish.go` around lines 122 - 142, queryAppMeta is
swallowing API/parsing failures by returning nil, which causes the doubao-html
branch in appsHTMLPublish to silently fall back to the legacy multipart path.
Update queryAppMeta to return an error (or a typed result) for lookup failures,
and change the caller in appsHTMLPublish to only use the legacy multipart path
on an explicit legacy response, not on transient errors; keep the special-case
routing for meta.AppType and meta.ArchType intact.
| out := map[string]interface{}{ | ||
| "app_id": spec.AppID, | ||
| "status": resp.Status, | ||
| } | ||
| if resp.URL != "" { | ||
| out["online_url"] = resp.URL | ||
| } | ||
| if resp.Status == "building" && resp.ReleaseID != "" { | ||
| polled, pollErr := pollDeployStatus(ctx, rctx, spec.AppID, resp.ReleaseID) | ||
| if pollErr == nil && polled.Status == "finished" { | ||
| out["status"] = "finished" | ||
| out["online_url"] = polled.URL | ||
| return out, nil | ||
| } | ||
| out["release_id"] = resp.ReleaseID | ||
| out["hint"] = fmt.Sprintf("run `lark-cli apps +release-get --app-id %s --release-id %s` to check status", | ||
| spec.AppID, resp.ReleaseID) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Failed/errored poll results are misreported as "still building".
out["status"] is only overwritten to "finished" on success; if pollErr != nil or polled.Status == "failed", the code falls through to the same branch that reports release_id + a "still building" hint — actual poll errors are discarded and a "failed" deployment is never surfaced as failed.
🐛 Proposed fix
if resp.Status == "building" && resp.ReleaseID != "" {
polled, pollErr := pollDeployStatus(ctx, rctx, spec.AppID, resp.ReleaseID)
- if pollErr == nil && polled.Status == "finished" {
- out["status"] = "finished"
- out["online_url"] = polled.URL
- return out, nil
+ if pollErr != nil {
+ return nil, pollErr
+ }
+ out["status"] = polled.Status
+ if polled.Status == "finished" {
+ out["online_url"] = polled.URL
+ return out, nil
}
out["release_id"] = resp.ReleaseID
- out["hint"] = fmt.Sprintf("run `lark-cli apps +release-get --app-id %s --release-id %s` to check status",
- spec.AppID, resp.ReleaseID)
+ if polled.Status != "failed" {
+ out["hint"] = fmt.Sprintf("run `lark-cli apps +release-get --app-id %s --release-id %s` to check status",
+ spec.AppID, resp.ReleaseID)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| out := map[string]interface{}{ | |
| "app_id": spec.AppID, | |
| "status": resp.Status, | |
| } | |
| if resp.URL != "" { | |
| out["online_url"] = resp.URL | |
| } | |
| if resp.Status == "building" && resp.ReleaseID != "" { | |
| polled, pollErr := pollDeployStatus(ctx, rctx, spec.AppID, resp.ReleaseID) | |
| if pollErr == nil && polled.Status == "finished" { | |
| out["status"] = "finished" | |
| out["online_url"] = polled.URL | |
| return out, nil | |
| } | |
| out["release_id"] = resp.ReleaseID | |
| out["hint"] = fmt.Sprintf("run `lark-cli apps +release-get --app-id %s --release-id %s` to check status", | |
| spec.AppID, resp.ReleaseID) | |
| } | |
| out := map[string]interface{}{ | |
| "app_id": spec.AppID, | |
| "status": resp.Status, | |
| } | |
| if resp.URL != "" { | |
| out["online_url"] = resp.URL | |
| } | |
| if resp.Status == "building" && resp.ReleaseID != "" { | |
| polled, pollErr := pollDeployStatus(ctx, rctx, spec.AppID, resp.ReleaseID) | |
| if pollErr != nil { | |
| return nil, pollErr | |
| } | |
| out["status"] = polled.Status | |
| if polled.Status == "finished" { | |
| out["online_url"] = polled.URL | |
| return out, nil | |
| } | |
| out["release_id"] = resp.ReleaseID | |
| if polled.Status != "failed" { | |
| out["hint"] = fmt.Sprintf("run `lark-cli apps +release-get --app-id %s --release-id %s` to check status", | |
| spec.AppID, resp.ReleaseID) | |
| } | |
| } |
🤖 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_html_publish.go` around lines 324 - 341, The publish flow
in apps_html_publish.go is treating every non-success poll as “still building”;
update the logic around pollDeployStatus in the app publish response assembly so
failed polls and polled.Status == "failed" are surfaced as failures instead of
falling through to the release_id/hint branch. In the block that builds out for
spec.AppID and resp.ReleaseID, branch on pollErr and the returned polled.Status
explicitly, preserve the actual error/status in out, and only keep the current
“finished” path when pollDeployStatus succeeds with polled.Status == "finished".
| func TestRunScaffold_StaticHtmlSkipped(t *testing.T) { | ||
| f := &fakeCommandRunner{} | ||
| withFakeRunner(t, f) | ||
| meta := &appMeta{AppType: 7, ArchType: 3} | ||
| kind, err := runScaffold(context.Background(), t.TempDir(), "app_x", meta, "") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if kind != scaffoldKindSkipped { | ||
| t.Errorf("kind = %q, want %q", kind, scaffoldKindSkipped) | ||
| } | ||
| if len(f.calls) != 0 { | ||
| t.Errorf("expected no calls for static html, got %d: %v", len(f.calls), f.calls) | ||
| } | ||
| } | ||
|
|
||
| func TestRunScaffold_DoubaoHtmlPassesTypes(t *testing.T) { | ||
| f := &fakeCommandRunner{results: map[string]fakeCallResult{"git ls-files": {stdout: ""}}} | ||
| withFakeRunner(t, f) | ||
| meta := &appMeta{AppType: 7, ArchType: 4} | ||
| kind, err := runScaffold(context.Background(), t.TempDir(), "app_x", meta, "") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if kind != scaffoldKindInit { | ||
| t.Errorf("kind = %q, want %q", kind, scaffoldKindInit) | ||
| } | ||
| c := findCall(f.calls, "npx", "-y") | ||
| if c == nil { | ||
| t.Fatal("npx not called") | ||
| } | ||
| if !containsAll(c, "--app-type", "7", "--arch-type", "4") { | ||
| t.Errorf("expected --app-type 7 --arch-type 4 in args: %v", c) | ||
| } | ||
| if containsAll(c, "--template") { | ||
| t.Errorf("should not contain --template when meta is present: %v", c) | ||
| } | ||
| } | ||
|
|
||
| func TestRunScaffold_NilMetaFallback(t *testing.T) { | ||
| f := &fakeCommandRunner{results: map[string]fakeCallResult{"git ls-files": {stdout: ""}}} | ||
| withFakeRunner(t, f) | ||
| kind, err := runScaffold(context.Background(), t.TempDir(), "app_x", nil, "") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if kind != scaffoldKindInit { | ||
| t.Errorf("kind = %q, want %q", kind, scaffoldKindInit) | ||
| } | ||
| c := findCall(f.calls, "npx", "-y") | ||
| if c == nil { | ||
| t.Fatal("npx not called") | ||
| } | ||
| if !containsAll(c, "--template", defaultTemplate) { | ||
| t.Errorf("expected --template %s in args: %v", defaultTemplate, c) | ||
| } | ||
| } | ||
|
|
||
| func TestRunScaffold_ExplicitTemplateOverride(t *testing.T) { | ||
| f := &fakeCommandRunner{results: map[string]fakeCallResult{"git ls-files": {stdout: ""}}} | ||
| withFakeRunner(t, f) | ||
| meta := &appMeta{AppType: 7, ArchType: 4} | ||
| kind, err := runScaffold(context.Background(), t.TempDir(), "app_x", meta, "custom-template") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if kind != scaffoldKindInit { | ||
| t.Errorf("kind = %q, want %q", kind, scaffoldKindInit) | ||
| } | ||
| c := findCall(f.calls, "npx", "-y") | ||
| if c == nil { | ||
| t.Fatal("npx not called") | ||
| } | ||
| if !containsAll(c, "--template", "custom-template") { | ||
| t.Errorf("expected --template custom-template in args: %v", c) | ||
| } | ||
| if containsAll(c, "--app-type") { | ||
| t.Errorf("--template should override --app-type: %v", c) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Good new coverage, but missing a case for non-HTML meta.
These new tests nicely cover static-HTML skip, doubao-html type-flag passing, nil-meta fallback, and explicit-template override. However, none of them cover meta != nil with AppType != 7 (e.g. a regular non-HTML app whose metadata lookup succeeds) — which is the scenario flagged in apps_init.go's scaffoldInitArgs (Lines 364-380) as potentially over-broad. Adding such a test would make the intended scope of the app-type/arch-type behavior explicit either way.
🤖 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_init_test.go` around lines 1649 - 1728, Add a test
covering runScaffold/scaffoldInitArgs when meta is non-nil but AppType is not 7,
since current coverage only checks static HTML, doubao-html, nil meta, and
explicit template override. Create a case in TestRunScaffold_* that uses a
regular app meta with a non-HTML AppType and verifies the npx args choose the
expected template/flags behavior. Use runScaffold and the scaffoldInitArgs
behavior to confirm whether --app-type/--arch-type should be omitted or included
for this non-HTML metadata path.
| func (api appsHTMLPublishTOSAPI) GetUploadURL(ctx context.Context, appID string) (string, string, error) { | ||
| path := fmt.Sprintf("%s/apps/%s/pre_release_html_code", apiBasePath, validate.EncodePathSegment(appID)) | ||
| data, err := api.runtime.CallAPITyped("POST", path, nil, nil) | ||
| if err != nil { | ||
| return "", "", err | ||
| } | ||
| uploadURL, _ := data["upload_url"].(string) | ||
| tosKey, _ := data["tos_key"].(string) | ||
| if uploadURL == "" || tosKey == "" { | ||
| return "", "", appsSubprocessEnvelopeError("pre_release_html_code returned empty upload_url or tos_key") | ||
| } | ||
| if !strings.HasPrefix(uploadURL, "https://") { | ||
| return "", "", appsSubprocessEnvelopeError("upload_url must be https, got %q", uploadURL) | ||
| } | ||
| return uploadURL, tosKey, nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how these helpers classify errors.
rg -n -A6 'func apps(FileIOError|ExternalToolError|SubprocessEnvelopeError|NetworkError)\(' shortcuts/apps
rg -n 'errs.NewNetworkError' shortcuts/appsRepository: larksuite/cli
Length of output: 4362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' shortcuts/apps/html_publish_tos_client.goRepository: larksuite/cli
Length of output: 3752
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A3 'http.NewRequestWithContext|NewNetworkError\(errs\.SubtypeNetwork|appsExternalToolError|appsFileIOError' shortcuts/appsRepository: larksuite/cli
Length of output: 21334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' shortcuts/apps/apps_errors.goRepository: larksuite/cli
Length of output: 4103
Classify the TOS upload failures as network errors (shortcuts/apps/html_publish_tos_client.go:52-63) http.NewRequestWithContext, Do, and non-2xx responses here are transport/server failures, but they're wrapped as SubtypeFileIO/SubtypeExternalTool; use the network error subtype so callers handle retries and reporting consistently.
🤖 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_tos_client.go` around lines 34 - 49, The TOS
upload path in GetUploadURL and the surrounding upload flow currently classifies
request construction, Do failures, and non-2xx responses as file I/O or external
tool errors, which is the wrong subtype for transport/server failures. Update
the error wrapping in the appsHTMLPublishTOSAPI path to use the network error
subtype for http.NewRequestWithContext, client.Do, and response-status failures
so callers can treat these as retryable network issues consistently.
Source: Coding guidelines
| resp, err := http.DefaultClient.Do(req) | ||
| if err != nil { | ||
| return appsExternalToolError(err, "TOS upload failed: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | ||
| return appsExternalToolError(nil, "TOS upload returned HTTP %d", resp.StatusCode) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
No request timeout on TOS upload — blocking call risk.
http.DefaultClient.Do(req) has no explicit timeout; if ctx lacks a deadline, a stalled TOS connection can hang the CLI indefinitely while uploading a potentially large zip archive.
⏱️ Proposed fix
+var tosHTTPClient = &http.Client{Timeout: 5 * time.Minute}
+
func (api appsHTMLPublishTOSAPI) UploadToTOS(ctx context.Context, uploadURL string, archive *htmlPublishArchive) error {
req, err := http.NewRequestWithContext(ctx, http.MethodPut, uploadURL, bytes.NewReader(archive.Body))
if err != nil {
return appsFileIOError(err, "create TOS upload request: %v", err)
}
req.Header.Set("Content-Type", "application/zip")
- resp, err := http.DefaultClient.Do(req)
+ resp, err := tosHTTPClient.Do(req)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resp, err := http.DefaultClient.Do(req) | |
| if err != nil { | |
| return appsExternalToolError(err, "TOS upload failed: %v", err) | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | |
| return appsExternalToolError(nil, "TOS upload returned HTTP %d", resp.StatusCode) | |
| } | |
| return nil | |
| } | |
| var tosHTTPClient = &http.Client{Timeout: 5 * time.Minute} | |
| resp, err := tosHTTPClient.Do(req) | |
| if err != nil { | |
| return appsExternalToolError(err, "TOS upload failed: %v", err) | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | |
| return appsExternalToolError(nil, "TOS upload returned HTTP %d", resp.StatusCode) | |
| } | |
| return nil | |
| } |
🤖 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_tos_client.go` around lines 57 - 66, The TOS
upload path currently uses http.DefaultClient.Do(req), which can block
indefinitely if the request context has no deadline. Update the upload logic
around the existing request creation and Do call to use a client with an
explicit timeout or ensure the request context is always given a deadline before
calling Do. Keep the same error handling via appsExternalToolError and preserve
the current status-code check and response closing behavior.
| func pollDeployStatus(ctx context.Context, rctx *common.RuntimeContext, appID, releaseID string) (*deployResponse, error) { | ||
| path := fmt.Sprintf("%s/apps/%s/releases/%s", | ||
| apiBasePath, | ||
| validate.EncodePathSegment(appID), | ||
| validate.EncodePathSegment(releaseID)) | ||
| deadline := time.Now().Add(pollTimeout) | ||
| for time.Now().Before(deadline) { | ||
| data, err := rctx.CallAPITyped("GET", path, nil, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| status, _ := data["status"].(string) | ||
| switch status { | ||
| case "finished": | ||
| url, _ := data["online_url"].(string) | ||
| return &deployResponse{Status: "finished", URL: url, ReleaseID: releaseID}, nil | ||
| case "failed": | ||
| return &deployResponse{Status: "failed", ReleaseID: releaseID}, nil | ||
| } | ||
| time.Sleep(pollInterval) | ||
| } | ||
| return &deployResponse{Status: "building", ReleaseID: releaseID}, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Polling loop ignores context cancellation.
time.Sleep(pollInterval) doesn't observe ctx.Done(), so cancelling ctx mid-poll has no effect until pollTimeout elapses.
🛑 Proposed fix
switch status {
case "finished":
url, _ := data["online_url"].(string)
return &deployResponse{Status: "finished", URL: url, ReleaseID: releaseID}, nil
case "failed":
return &deployResponse{Status: "failed", ReleaseID: releaseID}, nil
}
- time.Sleep(pollInterval)
+ select {
+ case <-ctx.Done():
+ return nil, ctx.Err()
+ case <-time.After(pollInterval):
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func pollDeployStatus(ctx context.Context, rctx *common.RuntimeContext, appID, releaseID string) (*deployResponse, error) { | |
| path := fmt.Sprintf("%s/apps/%s/releases/%s", | |
| apiBasePath, | |
| validate.EncodePathSegment(appID), | |
| validate.EncodePathSegment(releaseID)) | |
| deadline := time.Now().Add(pollTimeout) | |
| for time.Now().Before(deadline) { | |
| data, err := rctx.CallAPITyped("GET", path, nil, nil) | |
| if err != nil { | |
| return nil, err | |
| } | |
| status, _ := data["status"].(string) | |
| switch status { | |
| case "finished": | |
| url, _ := data["online_url"].(string) | |
| return &deployResponse{Status: "finished", URL: url, ReleaseID: releaseID}, nil | |
| case "failed": | |
| return &deployResponse{Status: "failed", ReleaseID: releaseID}, nil | |
| } | |
| time.Sleep(pollInterval) | |
| } | |
| return &deployResponse{Status: "building", ReleaseID: releaseID}, nil | |
| } | |
| func pollDeployStatus(ctx context.Context, rctx *common.RuntimeContext, appID, releaseID string) (*deployResponse, error) { | |
| path := fmt.Sprintf("%s/apps/%s/releases/%s", | |
| apiBasePath, | |
| validate.EncodePathSegment(appID), | |
| validate.EncodePathSegment(releaseID)) | |
| deadline := time.Now().Add(pollTimeout) | |
| for time.Now().Before(deadline) { | |
| data, err := rctx.CallAPITyped("GET", path, nil, nil) | |
| if err != nil { | |
| return nil, err | |
| } | |
| status, _ := data["status"].(string) | |
| switch status { | |
| case "finished": | |
| url, _ := data["online_url"].(string) | |
| return &deployResponse{Status: "finished", URL: url, ReleaseID: releaseID}, nil | |
| case "failed": | |
| return &deployResponse{Status: "failed", ReleaseID: releaseID}, nil | |
| } | |
| select { | |
| case <-ctx.Done(): | |
| return nil, ctx.Err() | |
| case <-time.After(pollInterval): | |
| } | |
| } | |
| return &deployResponse{Status: "building", ReleaseID: releaseID}, nil | |
| } |
🤖 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_tos_client.go` around lines 86 - 108, The polling
loop in pollDeployStatus ignores context cancellation because it uses time.Sleep
directly. Update pollDeployStatus to respect ctx.Done() during each poll
interval by waiting on a select between a timer and ctx.Done(), and return
promptly when the context is cancelled. Keep the existing CallAPITyped/status
handling intact while making the loop exit early on cancellation.
| func TestWriteHTMLPublishZipEntry_OpenFailure(t *testing.T) { | ||
| zw := zip.NewWriter(io.Discard) | ||
| defer zw.Close() | ||
| err := writeHTMLPublishZipEntry(newTestFIO(), zw, htmlPublishCandidate{ | ||
| RelPath: "x.html", | ||
| AbsPath: "/nonexistent-path-for-test/x.html", | ||
| Size: 0, | ||
| }) | ||
| if err == nil { | ||
| t.Fatalf("expected error for nonexistent abs path") | ||
| } | ||
| if !strings.Contains(err.Error(), "open") { | ||
| t.Fatalf("expected open error, got %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Error-path tests assert only message substrings, not typed metadata.
TestWriteHTMLPublishZipEntry_OpenFailure, _CopyFailure, and _RejectsPathTraversal all check strings.Contains(err.Error(), ...) only. As per coding guidelines, **/*_test.go: "Error-path tests must assert typed metadata via errs.ProblemOf (category / subtype / param) and cause preservation, not message substrings alone."
✅ Proposed fix pattern (apply to all three tests)
if err == nil {
t.Fatalf("expected error for nonexistent abs path")
}
- if !strings.Contains(err.Error(), "open") {
- t.Fatalf("expected open error, got %v", err)
- }
+ problem, ok := errs.ProblemOf(err)
+ if !ok {
+ t.Fatalf("expected typed error, got %v", err)
+ }
+ if problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeFileIO {
+ t.Fatalf("unexpected classification: %+v", problem)
+ }Also applies to: 115-131, 147-175
🤖 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_zip_test.go` around lines 99 - 113, The
error-path tests for writeHTMLPublishZipEntry currently validate only error
text, so update TestWriteHTMLPublishZipEntry_OpenFailure,
TestWriteHTMLPublishZipEntry_CopyFailure, and
TestWriteHTMLPublishZipEntry_RejectsPathTraversal to assert typed metadata with
errs.ProblemOf instead of strings.Contains. Check the returned error for the
expected category/subtype/param values, and also verify the underlying cause is
preserved where applicable. Use the writeHTMLPublishZipEntry helper and the
htmlPublishCandidate fields to keep the assertions aligned with the specific
failure mode being exercised.
Source: Coding guidelines
…pe for arch_type=4 html in +init
… with arch_type-based routing
anngo-nk
force-pushed
the
feat/doubao-miaoda-integration
branch
from
1625a9d to
13a24b7
Compare
… in +init, revert html-publish changes
github-actions
Bot
added
size/M
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/apps_create_test.go (1)
337-364: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTest relies on ambient env, not explicit unset.
TestAppsCreate_AgentEnvVarNotSetnever unsetsLARKSUITE_CLI_AGENT; it only relies on the variable being absent from the process environment. If it happens to be exported in a developer's shell or CI runner (plausible, given it's meant to identify calling agents), this test becomes flaky/order-dependent instead of deterministically exercising the "unset" branch.Explicitly unset the var (with restore) to guarantee test isolation:
🔧 Proposed fix
func TestAppsCreate_AgentEnvVarNotSet(t *testing.T) { + if prev, ok := os.LookupEnv("LARKSUITE_CLI_AGENT"); ok { + os.Unsetenv("LARKSUITE_CLI_AGENT") + t.Cleanup(func() { os.Setenv("LARKSUITE_CLI_AGENT", prev) }) + } factory, stdout, reg := newAppsExecuteFactory(t)🤖 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_create_test.go` around lines 337 - 364, TestAppsCreate_AgentEnvVarNotSet is relying on the ambient process environment instead of explicitly exercising the unset case. Update this test to temporarily clear LARKSUITE_CLI_AGENT before calling runAppsShortcut, and restore the original value afterward so the behavior is deterministic and isolated. Keep the existing assertions against the captured request body in the same test.
🧹 Nitpick comments (2)
shortcuts/apps/apps_create_test.go (1)
277-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider table-driven consolidation.
The three tests share nearly identical setup/execute/decode logic, differing only in the env var value and assertion. A table-driven test would reduce duplication.
🤖 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_create_test.go` around lines 277 - 364, Consolidate TestAppsCreate_WithAgentEnvVar, TestAppsCreate_WithoutAgentEnvVar, and TestAppsCreate_AgentEnvVarNotSet into a table-driven test because they repeat the same setup, runAppsShortcut execution, and JSON decoding logic. Keep the shared flow in one loop, vary only the LARKSUITE_CLI_AGENT value and the expected source_agent assertion, and reuse newAppsExecuteFactory, runAppsShortcut, and the stub registration to reduce duplication.shortcuts/apps/apps_init.go (1)
127-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
resolveTemplateandscaffoldInitArgsduplicate the same precedence logic.Both implement identical "explicit template → appType → defaultTemplate" resolution independently.
resolveTemplateis only exercised byDryRun(Line 89); the realExecutepath bypasses it and reimplements the same three-way branching insidescaffoldInitArgs. If the precedence or trimming rules change in one place, dry-run output and actual scaffold behavior can silently diverge.Consider having
scaffoldInitArgsaccept a single already-resolved template string (computed once viaresolveTemplate-equivalent logic) instead of re-deriving it fromappType/explicitTemplate.♻️ Proposed consolidation
-func scaffoldInitArgs(appType, appID, explicitTemplate string) []string { - base := []string{"-y", "--prefer-online", miaodaCLIPkg, "app", "init"} - if explicitTemplate != "" { - return append(base, "--template", explicitTemplate, "--app-id", appID) - } - if appType != "" { - return append(base, "--template", appType, "--app-id", appID) - } - return append(base, "--template", defaultTemplate, "--app-id", appID) -} +func scaffoldInitArgs(appID, resolvedTemplate string) []string { + base := []string{"-y", "--prefer-online", miaodaCLIPkg, "app", "init"} + return append(base, "--template", resolvedTemplate, "--app-id", appID) +}Then compute
resolvedTemplateonce (e.g. via a shared helper mirroringresolveTemplate's precedence) and pass it into bothrunScaffold/scaffoldInitArgsand the dry-run path.Also applies to: 355-368
🤖 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_init.go` around lines 127 - 137, `resolveTemplate` and `scaffoldInitArgs` currently duplicate the same template precedence logic, which can cause DryRun and Execute to drift. Refactor the `runScaffold`/`scaffoldInitArgs` flow so the template is resolved once using the existing `resolveTemplate` precedence (explicit template, then appType, then defaultTemplate), and pass that single resolved value into both the dry-run and real scaffold paths. Keep `resolveTemplate` as the single source of truth and remove the re-derivation from `scaffoldInitArgs`.
🤖 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/apps_create_test.go`:
- Around line 337-364: TestAppsCreate_AgentEnvVarNotSet is relying on the
ambient process environment instead of explicitly exercising the unset case.
Update this test to temporarily clear LARKSUITE_CLI_AGENT before calling
runAppsShortcut, and restore the original value afterward so the behavior is
deterministic and isolated. Keep the existing assertions against the captured
request body in the same test.
---
Nitpick comments:
In `@shortcuts/apps/apps_create_test.go`:
- Around line 277-364: Consolidate TestAppsCreate_WithAgentEnvVar,
TestAppsCreate_WithoutAgentEnvVar, and TestAppsCreate_AgentEnvVarNotSet into a
table-driven test because they repeat the same setup, runAppsShortcut execution,
and JSON decoding logic. Keep the shared flow in one loop, vary only the
LARKSUITE_CLI_AGENT value and the expected source_agent assertion, and reuse
newAppsExecuteFactory, runAppsShortcut, and the stub registration to reduce
duplication.
In `@shortcuts/apps/apps_init.go`:
- Around line 127-137: `resolveTemplate` and `scaffoldInitArgs` currently
duplicate the same template precedence logic, which can cause DryRun and Execute
to drift. Refactor the `runScaffold`/`scaffoldInitArgs` flow so the template is
resolved once using the existing `resolveTemplate` precedence (explicit
template, then appType, then defaultTemplate), and pass that single resolved
value into both the dry-run and real scaffold paths. Keep `resolveTemplate` as
the single source of truth and remove the re-derivation from `scaffoldInitArgs`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 853bb83e-4731-4ddf-8704-64f2d1ee1c17
📒 Files selected for processing (6)
shortcuts/apps/apps_create.goshortcuts/apps/apps_create_test.goshortcuts/apps/apps_init.goshortcuts/apps/apps_init_test.goshortcuts/apps/apps_meta.goshortcuts/apps/apps_meta_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/apps/apps_create.go
1 participant
Summary
Route +init and +html-publish behavior by arch_type, enabling different initialization and publish paths for different html app variants.
Changes
LARKSUITE_CLI_AGENTenv var and passapp_sourcein request body when set--app-type/--arch-typeto miaoda-cli for template selection; falls back to--template nestjs-react-fullstackwhen meta query failsapps_meta.gowith sharedqueryAppMetaandisStaticHtmlfunctionshtml_publish_zip.gofor zip packaginghtml_publish_tos_client.gowith TOS upload interface and implementationTest Plan
go build ./...go test ./shortcuts/apps/Related Issues
GET /apps/{id}endpoint (returning app_type/arch_type), TOS presigned URL API, new deploy APISummary by CodeRabbit
source_agent) when the relevant environment variable is set.--template→ app type → default) and supports a new--source-pathflag.