fix: unify dry-run output contract#1870
Merged
Merged
Conversation
- transcribe HTTP method verbatim in previews (HEAD/OPTIONS were
reported as GET); reject an empty method in api with a typed error
- unify the dry-run data payload across api/service/shortcut paths:
{api, context?: {app_id, user_open_id}}; drop data.as — the envelope
top-level identity is the single identity source
- mark pretty dry-run stdout with '# dry-run: request not sent' so logs
that drop stderr still show it was a preview
- extract the shared preview builder, collapse PrintDryRunWithFile's
loose params into FileUploadMeta, and fail loudly on nil previews
- revert description-marker identity parsing: stale prose must not
override corrected accessTokens (blocks legal user calls on
images.create); identity gating keys off accessTokens only
- pin the new contracts with tests: verbatim method, three-way context
parity, nil-preview error, empty-context omission, marker line
…st conventions
- typed struct at the boundary over map[string]interface{} threading;
distinct types where values could swap silently (internal/meta.Token)
- transcribe input verbatim in previews/transformations; reject
unhonorable flag combinations with typed errors instead of silently
substituting behavior
- contract tests must fail when the implementation is reverted
main gained raw-format dry-run readers while the PR was in flight (wiki drive export #1802, drive list comments #1845, slash commands, sheets history, docs fetch, mail draft-send/triage, vc meeting events). Migrate them to the envelope accessors (clie2e.DryRunGet / data-wrapped decoders) and drop the now-redundant DryRunData extractions in files unified on DryRunGet.
github-actions
Bot
added
domain/ccm
📝 WalkthroughWalkthroughDry-run output is standardized into a success envelope with nested ChangesDry-run rendering and command integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 #1870 +/- ##
==========================================
+ Coverage 74.60% 74.66% +0.06%
==========================================
Files 877 877
Lines 91720 91731 +11
==========================================
+ Hits 68426 68494 +68
+ Misses 17981 17926 -55
+ Partials 5313 5311 -2 ☔ 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@636ab9391d60d3e7bac2c7cdf3baac303fdac6ed🧩 Skill updatenpx skills add larksuite/cli#fix/dry-run-output-contract-v2 -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
tests/cli_e2e/drive/drive_member_add_dryrun_test.go (1)
307-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent migration: mixes
clie2e.DryRunGetwith rawgjson.Get+ manual "data." prefix.Lines 290-310 correctly use the new
clie2e.DryRunGethelper, butbodyPathis built with a hardcoded"data."prefix and the subsequentmember_id/member_type/perm/type/perm_typeassertions (Lines 314-331) fall back to rawgjson.Get. It's functionally equivalent today, but this is the only file in the migration that didn't fully adopt the shared helper, leaving a stalegjsonimport and a path-construction pattern that will silently drift ifDryRunGet's prefixing logic ever changes.♻️ Proposed fix to use `clie2e.DryRunGet` consistently
- bodyPath := "data.api.0.body" + bodyPath := "api.0.body" if tt.wantBatch { - bodyPath = "data.api.0.body.members.0" + bodyPath = "api.0.body.members.0" if count := len(clie2e.DryRunGet(out, "api.0.body.members").Array()); count != 2 { t.Fatalf("body.members count = %d, want 2\nstdout:\n%s", count, out) } } - if got := gjson.Get(out, bodyPath+".member_id").String(); got != tt.wantMemberID { + if got := clie2e.DryRunGet(out, bodyPath+".member_id").String(); got != tt.wantMemberID { t.Fatalf("body.member_id = %q, want %q\nstdout:\n%s", got, tt.wantMemberID, out) } - if got := gjson.Get(out, bodyPath+".member_type").String(); got != tt.wantMemberType { + if got := clie2e.DryRunGet(out, bodyPath+".member_type").String(); got != tt.wantMemberType { t.Fatalf("body.member_type = %q, want %q\nstdout:\n%s", got, tt.wantMemberType, out) } - if got := gjson.Get(out, bodyPath+".perm").String(); got != tt.wantPerm { + if got := clie2e.DryRunGet(out, bodyPath+".perm").String(); got != tt.wantPerm { t.Fatalf("body.perm = %q, want %q\nstdout:\n%s", got, tt.wantPerm, out) } - if got := gjson.Get(out, bodyPath+".type").String(); got != tt.wantMemberKind { + if got := clie2e.DryRunGet(out, bodyPath+".type").String(); got != tt.wantMemberKind { t.Fatalf("body.type = %q, want %q\nstdout:\n%s", got, tt.wantMemberKind, out) } - permType := gjson.Get(out, bodyPath+".perm_type") + permType := clie2e.DryRunGet(out, bodyPath+".perm_type")🤖 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 `@tests/cli_e2e/drive/drive_member_add_dryrun_test.go` around lines 307 - 333, Update the assertions in the dry-run test around bodyPath to use clie2e.DryRunGet for all member_id, member_type, perm, type, and perm_type lookups. Remove the hardcoded "data." prefix from bodyPath, preserve the batch/non-batch paths and existing expectations, and remove the now-unused gjson import.tests/cli_e2e/sheets/sheets_gridline_dryrun_test.go (1)
47-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign DryRunData usage with other dry-run tests for consistency.
The nil check and in-place mutation of
result.Stdoutis unnecessary—require.NoErroron line 50 will fail the test beforeresultis accessed on line 51 iferr != nil. Other tests in this PR (e.g.,sheets_table_put_dryrun_test.goline 40) use the cleaner pattern of assigning after the error/exit-code checks.♻️ Suggested refactor
DefaultAs: "user", }) - if result != nil { - result.Stdout = clie2e.DryRunData(result.Stdout) - } require.NoError(t, err) result.AssertExitCode(t, 0) - out := result.Stdout + out := clie2e.DryRunData(result.Stdout)🤖 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 `@tests/cli_e2e/sheets/sheets_gridline_dryrun_test.go` around lines 47 - 53, In the test flow around result.AssertExitCode, remove the nil check and in-place mutation of result.Stdout. First require no error and assert the exit code, then assign the output variable from DryRunData(result.Stdout), matching the pattern used by the other dry-run tests.shortcuts/apps/apps_file_upload_test.go (1)
80-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilently ignored
json.Unmarshalerrors in dry-run tests cause confusing panics. All three sites discard the unmarshal error with_ = json.Unmarshal(...), so a decode failure produces an index-out-of-range panic on the subsequentenv.API[0]access instead of a clear test failure. Other tests in this PR (apps_traces_test.go,apps_env_test.go) properly check the error witht.Fatalf.
shortcuts/apps/apps_file_upload_test.go#L80-L80: replace_ = json.Unmarshal(...)with anif err := ...; err != nil { t.Fatalf(...) }guard.shortcuts/apps/apps_db_changelog_list_test.go#L41-L41: same fix.shortcuts/apps/apps_file_sign_test.go#L26-L26: same fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/apps/apps_file_upload_test.go` at line 80, Replace each discarded json.Unmarshal error with an if err guard that calls t.Fatalf with the decode error in shortcuts/apps/apps_file_upload_test.go:80-80, shortcuts/apps/apps_db_changelog_list_test.go:41-41, and shortcuts/apps/apps_file_sign_test.go:26-26. Apply the checks at the existing dry-run response parsing sites before accessing env.API.shortcuts/sheets/helpers_test.go (1)
255-262: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFallback to legacy top-level
apikey is likely dead code under the new envelope contract.
dryRunAPIEntriesprefersdryRun["data"]["api"]but falls back to a top-leveldryRun["api"]. Per the unified contract, dry-run stdout always nestsapiunderdatanow, so the fallback branch should never trigger in practice and mainly obscures that the migration is complete. Consider removing the fallback once confirmed no callers still emit the old shape.🤖 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/sheets/helpers_test.go` around lines 255 - 262, Update dryRunAPIEntries to rely exclusively on the unified envelope contract by returning entries only from dryRun["data"]["api"]. Remove the legacy top-level dryRun["api"] fallback while preserving the existing success and failure behavior for missing or incorrectly typed nested data.shortcuts/common/runner_jq_test.go (1)
255-269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecode the envelope into a typed projection.
The untyped map and unchecked assertions can panic on a contract regression. Use a small envelope struct with pointer booleans/context fields so required fields are asserted explicitly.
As per coding guidelines, “Parse
map[string]interface{}into typed structs at system boundaries.”🤖 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/common/runner_jq_test.go` around lines 255 - 269, The dry-run response validation in the test uses untyped maps and unchecked assertions that can panic on malformed output. Replace the map-based decoding around env, data, api, and call with a small typed envelope projection, using pointer booleans and context fields to distinguish missing values; explicitly assert all required fields before accessing nested data, while preserving the existing validation expectations.Source: Coding guidelines
shortcuts/apps/apps_db_env_recovery_quota_test.go (1)
318-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
len(env.API)guard to the other subtests for consistency.
TestAppsDBQuotaGet_DryRunOmitsEnvWhenUnsetcorrectly checkslen(env.API) != 1before accessingenv.API[0], butTestAppsDBEnvDiff_DryRunBody(line 35),TestAppsDBEnvMigrate_DryRunBody(line 90), andTestAppsDBRecoveryDiff_DryRunNormalizesTarget(line 175) accessenv.API[0]without a length check. If the unmarshal fails silently (the error is discarded with_ =), those tests will panic with an index-out-of-range instead of producing a useful failure message.♻️ Add length guards to the remaining subtests
Apply the same pattern used at lines 320-322 to the other three subtests:
var env dryRunAPIEnvelope _ = json.Unmarshal([]byte(stdout.String()), &env) +if len(env.API) != 1 { + t.Fatalf("dry-run API calls = %d, want 1; stdout=%s", len(env.API), stdout.String()) +} a := env.API[0]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/apps/apps_db_env_recovery_quota_test.go` around lines 318 - 322, Add len(env.API) validation before every env.API[0] access in TestAppsDBEnvDiff_DryRunBody, TestAppsDBEnvMigrate_DryRunBody, and TestAppsDBRecoveryDiff_DryRunNormalizesTarget, matching the existing guard in TestAppsDBQuotaGet_DryRunOmitsEnvWhenUnset. Preserve the same failure message style, including the API count and captured stdout, so malformed or empty unmarshaled responses fail clearly instead of panicking.
🤖 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 `@cmd/api/api_test.go`:
- Around line 1049-1065: Update TestApiCmd_DryRunWithFile to replace each
unchecked assertion for data, api, call, and body/file with checked assertions
using ok and t.Fatalf, matching the diagnostic pattern in TestApiCmd_DryRun;
retain the existing validation and failure messages for valid structure and file
values.
- Around line 185-199: Update TestApiCmd_EmptyMethodRejected to validate the
typed error returned by cmd.Execute: extract problem metadata with
errs.ProblemOf and assert the expected category and subtype, then use errors.As
to verify the parameter metadata identifies the HTTP method. Remove the
strings.Contains message assertion while preserving the existing nil-error
check.
---
Nitpick comments:
In `@shortcuts/apps/apps_db_env_recovery_quota_test.go`:
- Around line 318-322: Add len(env.API) validation before every env.API[0]
access in TestAppsDBEnvDiff_DryRunBody, TestAppsDBEnvMigrate_DryRunBody, and
TestAppsDBRecoveryDiff_DryRunNormalizesTarget, matching the existing guard in
TestAppsDBQuotaGet_DryRunOmitsEnvWhenUnset. Preserve the same failure message
style, including the API count and captured stdout, so malformed or empty
unmarshaled responses fail clearly instead of panicking.
In `@shortcuts/apps/apps_file_upload_test.go`:
- Line 80: Replace each discarded json.Unmarshal error with an if err guard that
calls t.Fatalf with the decode error in
shortcuts/apps/apps_file_upload_test.go:80-80,
shortcuts/apps/apps_db_changelog_list_test.go:41-41, and
shortcuts/apps/apps_file_sign_test.go:26-26. Apply the checks at the existing
dry-run response parsing sites before accessing env.API.
In `@shortcuts/common/runner_jq_test.go`:
- Around line 255-269: The dry-run response validation in the test uses untyped
maps and unchecked assertions that can panic on malformed output. Replace the
map-based decoding around env, data, api, and call with a small typed envelope
projection, using pointer booleans and context fields to distinguish missing
values; explicitly assert all required fields before accessing nested data,
while preserving the existing validation expectations.
In `@shortcuts/sheets/helpers_test.go`:
- Around line 255-262: Update dryRunAPIEntries to rely exclusively on the
unified envelope contract by returning entries only from dryRun["data"]["api"].
Remove the legacy top-level dryRun["api"] fallback while preserving the existing
success and failure behavior for missing or incorrectly typed nested data.
In `@tests/cli_e2e/drive/drive_member_add_dryrun_test.go`:
- Around line 307-333: Update the assertions in the dry-run test around bodyPath
to use clie2e.DryRunGet for all member_id, member_type, perm, type, and
perm_type lookups. Remove the hardcoded "data." prefix from bodyPath, preserve
the batch/non-batch paths and existing expectations, and remove the now-unused
gjson import.
In `@tests/cli_e2e/sheets/sheets_gridline_dryrun_test.go`:
- Around line 47-53: In the test flow around result.AssertExitCode, remove the
nil check and in-place mutation of result.Stdout. First require no error and
assert the exit code, then assign the output variable from
DryRunData(result.Stdout), matching the pattern used by the other dry-run tests.
🪄 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: 18ee93d7-fa8a-4764-a4fe-494a9fa9af36
📒 Files selected for processing (102)
AGENTS.mdcmd/api/api.gocmd/api/api_test.gocmd/service/service.gocmd/service/service_test.gointernal/cmdutil/dryrun.gointernal/cmdutil/dryrun_test.gointernal/output/envelope.gointernal/output/envelope_success.gointernal/output/envelope_success_test.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.goshortcuts/application/slash_command_delete_test.goshortcuts/application/slash_command_update_test.goshortcuts/apps/apps_analytics_test.goshortcuts/apps/apps_db_audit_test.goshortcuts/apps/apps_db_changelog_list_test.goshortcuts/apps/apps_db_data_export_test.goshortcuts/apps/apps_db_data_import_test.goshortcuts/apps/apps_db_env_recovery_quota_test.goshortcuts/apps/apps_db_execute_test.goshortcuts/apps/apps_db_table_get_test.goshortcuts/apps/apps_db_table_list_test.goshortcuts/apps/apps_env_test.goshortcuts/apps/apps_file_delete_test.goshortcuts/apps/apps_file_download_test.goshortcuts/apps/apps_file_get_test.goshortcuts/apps/apps_file_list_test.goshortcuts/apps/apps_file_sign_test.goshortcuts/apps/apps_file_upload_test.goshortcuts/apps/apps_init_test.goshortcuts/apps/apps_logs_test.goshortcuts/apps/apps_metrics_test.goshortcuts/apps/apps_traces_test.goshortcuts/apps/dryrun_test.goshortcuts/apps/git_credential_test.goshortcuts/common/runner.goshortcuts/common/runner_jq_test.goshortcuts/drive/drive_add_comment_test.goshortcuts/drive/drive_member_add_test.goshortcuts/sheets/helpers_test.goshortcuts/sheets/lark_sheet_history_test.goshortcuts/slides/slides_replace_pages_test.goshortcuts/task/task_upload_attachment_test.gotests/cli_e2e/application/slash_command_dryrun_test.gotests/cli_e2e/apps/apps_access_scope_get_dryrun_test.gotests/cli_e2e/apps/apps_access_scope_set_dryrun_test.gotests/cli_e2e/apps/apps_create_dryrun_test.gotests/cli_e2e/apps/apps_db_env_create_dryrun_test.gotests/cli_e2e/apps/apps_db_execute_dryrun_test.gotests/cli_e2e/apps/apps_db_table_get_dryrun_test.gotests/cli_e2e/apps/apps_db_table_list_dryrun_test.gotests/cli_e2e/apps/apps_env_pull_dryrun_test.gotests/cli_e2e/apps/apps_git_credential_dryrun_test.gotests/cli_e2e/apps/apps_html_publish_dryrun_test.gotests/cli_e2e/apps/apps_list_dryrun_test.gotests/cli_e2e/apps/apps_update_dryrun_test.gotests/cli_e2e/base/base_attachment_dryrun_test.gotests/cli_e2e/base/base_block_dryrun_test.gotests/cli_e2e/base/base_create_dryrun_test.gotests/cli_e2e/base/base_field_dryrun_test.gotests/cli_e2e/base/base_limit_dryrun_test.gotests/cli_e2e/calendar/calendar_update_dryrun_test.gotests/cli_e2e/core.gotests/cli_e2e/docs/docs_fetch_dryrun_test.gotests/cli_e2e/docs/docs_update_dryrun_test.gotests/cli_e2e/drive/drive_add_comment_dryrun_test.gotests/cli_e2e/drive/drive_apply_permission_dryrun_test.gotests/cli_e2e/drive/drive_export_dryrun_test.gotests/cli_e2e/drive/drive_import_dryrun_test.gotests/cli_e2e/drive/drive_inspect_dryrun_test.gotests/cli_e2e/drive/drive_list_comments_dryrun_test.gotests/cli_e2e/drive/drive_member_add_dryrun_test.gotests/cli_e2e/drive/drive_preview_dryrun_test.gotests/cli_e2e/drive/drive_pull_dryrun_test.gotests/cli_e2e/drive/drive_push_dryrun_test.gotests/cli_e2e/drive/drive_search_dryrun_test.gotests/cli_e2e/drive/drive_secure_label_dryrun_test.gotests/cli_e2e/drive/drive_status_dryrun_test.gotests/cli_e2e/drive/drive_sync_dryrun_test.gotests/cli_e2e/drive/drive_upload_dryrun_test.gotests/cli_e2e/event/event_subscribe_dryrun_test.gotests/cli_e2e/im/im_download_resources_dryrun_test.gotests/cli_e2e/mail/mail_draft_send_dryrun_test.gotests/cli_e2e/mail/mail_share_to_chat_dryrun_test.gotests/cli_e2e/mail/mail_triage_dryrun_test.gotests/cli_e2e/markdown/markdown_dryrun_test.gotests/cli_e2e/note/note_dryrun_test.gotests/cli_e2e/sheets/sheets_gridline_dryrun_test.gotests/cli_e2e/sheets/sheets_image_upload_dryrun_test.gotests/cli_e2e/sheets/sheets_sheet_shortcuts_dryrun_test.gotests/cli_e2e/sheets/sheets_table_get_dryrun_test.gotests/cli_e2e/sheets/sheets_table_put_dryrun_test.gotests/cli_e2e/sheets/sheets_workbook_export_dryrun_test.gotests/cli_e2e/sheets/sheets_workbook_import_dryrun_test.gotests/cli_e2e/stdin_regression_test.gotests/cli_e2e/task/task_get_my_tasks_dryrun_test.gotests/cli_e2e/task/task_upload_attachment_dryrun_test.gotests/cli_e2e/vc/vc_meeting_events_dryrun_test.gotests/cli_e2e/vc/vc_meeting_message_send_dryrun_test.gotests/cli_e2e/wiki/wiki_member_add_dryrun_test.gotests/cli_e2e/wiki/wiki_node_create_dryrun_test.go
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
Continues #1817 (rebased onto latest main; the original branch lives on a fork and could not be updated directly). Unifies all dry-run output under the standard success envelope, with review fixes on top.
BREAKING: dry-run JSON stdout is now the standard envelope — migrate
.api→.data.api;data.appId/data.as/data.userOpenId→ top-levelidentityanddata.context.{app_id,user_open_id}.Changes
ok/identity/dry_run/data); JSON stdout is clean, pretty keeps the banner on stderr plus a# dry-run: request not sentmarker on stdoutdatashape across all three paths:{api, context?, description?}; identity is reported only at the envelope top level:asno longer corrupts:assignee_id)accessTokens(it would block legal user calls onimages.create)Test Plan
go test ./tests/cli_e2e/... -run DryRun— all domains greenRelated Issues
Summary by CodeRabbit
New Features
--dry-runoutput in a structured success envelope withok,identity,dry_run, and request details underdata.Bug Fixes
Tests