feat(apps): support modern_html app type with TOS publish path and app type querying - #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:
📝 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 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.40% 74.58% +0.17%
==========================================
Files 860 863 +3
Lines 89215 90123 +908
==========================================
+ Hits 66384 67220 +836
+ Misses 17701 17696 -5
- Partials 5130 5207 +77 ☔ 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@d33d594bbb9c61f5d2c8c5b825a811602419d69f🧩 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
1625a9d to
13a24b7
Compare
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
…pe for arch_type=4 html in +init
… with arch_type-based routing
… in +init, revert html-publish changes
db77bd6 to
b16f1d3
Compare
…om queryAppType with full_stack fallback
…plate to miaoda-cli
github-actions
Bot
added
the
size/L
…l, auto-detect modern_html from doubao agent env
…rnal to html-publish)
…ver decide via source_agent
…Install, skipEnvPull, skipSkillsSync)
…omments, simplify queryAppType
anngo-nk
changed the title
…e for dry-run coverage
2 participants
Summary
Support modern_html app type for the doubao agent HTML publish workflow. Adds app type querying, conditional init behavior, and a TOS-based publish path alongside the existing multipart flow.
Changes
New commands
+create
LARKSUITE_CLI_AGENT_NAMEenv var viaenvvars.AgentName()and pass assource_agentin request body+init
GET /apps/{id}(queryAppType) to determine init behaviorappTypePolicyfor per-type control:skipInstall,skipEnvPull,skipSkillsSyncmodern_htmlapps skip dependency install, env-pull, and skills sync--app-type <type>to miaoda-cli instead of--template--source-pathflag for incorporating existing HTML source files--source-pathwithfilepath.Absand control character rejectionConditionalScopes: ["spark:app:read"]for the queryAppType call+html-publish
modern_html: tar.gz → pre_release API (GET KV list) → TOS presigned PUT upload → internal release-create → returnrelease_idurlprepareHTMLPublishTarballto deduplicate validation+packagingspark:app:readto scopes+release-create
syncfield in response outputShared
queryAppTypeinapps_meta.go: GET /apps/{id}, parseappInfostruct, normalize to lowercase, stderr warning on failureTest Plan
go build ./...and full unit tests passRelated Issues
GET /apps/{id}endpoint andGET /apps/{id}/pre_releaseendpoint