fix: unify dry-run output contract#1817
Closed
Tantanz20020918 wants to merge 3 commits into
Closed
Conversation
Tantanz20020918
added
domain/core
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR standardizes dry-run output as a JSON success envelope with ChangesDry-run output envelope unification
Estimated code review effort: 3 (Moderate) | ~25 minutes Description-based identity override
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
domain/ccm
![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 (3)
shortcuts/apps/dryrun_test.go (1)
19-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLegacy fallback could mask incomplete envelope migration.
The fallback silently accepts the old top-level
apishape wheneverdata.apiis empty. Since this PR's entire goal is enforcing the new nesteddata.apicontract, a shortcut that isn't fully wired throughcmdutil.WriteDryRun(and still emits the legacy shape) would pass these tests undetected instead of failing loudly.Consider dropping the legacy fallback (or asserting on the raw envelope shape once) now that migration is complete, so any regression to the old format fails the test.
🤖 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/dryrun_test.go` around lines 19 - 34, The UnmarshalJSON logic on dryRunAPIEnvelope still falls back to the legacy top-level API field, which can hide regressions in the new envelope format. Update the dryRunAPIEnvelope.UnmarshalJSON path so tests only accept the nested data.api shape (or explicitly assert the raw envelope shape once) and remove the silent fallback to raw.API; this will make any cmdutil.WriteDryRun regression that emits the old format fail in dryrun_test.go.tests/cli_e2e/drive/drive_export_dryrun_test.go (1)
83-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider migrating
TestDriveExportDryRun_MarkdownFetchAPItoclie2e.DryRunGetfor consistency.This test uses
DryRunData+gjson.Getwhile the other two tests in the same file useclie2e.DryRunGet. Migrating the remaininggjson.Getcalls (lines 87–104) toDryRunGetwould allow removing thegjsonimport entirely from this file, matching the pattern in all other migrated files in this PR.♻️ Proposed refactor for TestDriveExportDryRun_MarkdownFetchAPI
require.NoError(t, err) - result.Stdout = clie2e.DryRunData(result.Stdout) result.AssertExitCode(t, 0) out := result.Stdout - if got := gjson.Get(out, "api.0.method").String(); got != "POST" { + if got := clie2e.DryRunGet(out, "api.0.method").String(); got != "POST" { t.Fatalf("method=%q, want POST\nstdout:\n%s", got, out) } - if got := gjson.Get(out, "api.0.url").String(); got != "/open-apis/docs_ai/v1/documents/docxMdDryRun/fetch" { + if got := clie2e.DryRunGet(out, "api.0.url").String(); got != "/open-apis/docs_ai/v1/documents/docxMdDryRun/fetch" { t.Fatalf("url=%q, want docs_ai fetch\nstdout:\n%s", got, out) } - if got := gjson.Get(out, "api.0.body.format").String(); got != "markdown" { + if got := clie2e.DryRunGet(out, "api.0.body.format").String(); got != "markdown" { t.Fatalf("body.format=%q, want markdown\nstdout:\n%s", got, out) } - if gjson.Get(out, "api.0.body.extra_param").Exists() { + if clie2e.DryRunGet(out, "api.0.body.extra_param").Exists() { t.Fatalf("markdown drive export must not enable docs fetch extra_param\nstdout:\n%s", out) } - if got := gjson.Get(out, "file_name").String(); got != "my-notes.md" { + if got := clie2e.DryRunGet(out, "file_name").String(); got != "my-notes.md" { t.Fatalf("file_name=%q, want my-notes.md\nstdout:\n%s", got, out) } - if got := gjson.Get(out, "output_dir").String(); got != "./md-exports" { + if got := clie2e.DryRunGet(out, "output_dir").String(); got != "./md-exports" { t.Fatalf("output_dir=%q, want ./md-exports\nstdout:\n%s", got, out) }🤖 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_export_dryrun_test.go` around lines 83 - 104, The TestDriveExportDryRun_MarkdownFetchAPI check still parses stdout with DryRunData plus multiple gjson.Get calls, unlike the other dry-run tests in this file. Update this test to use clie2e.DryRunGet for the API and output fields, referencing the existing TestDriveExportDryRun_MarkdownFetchAPI helper pattern used elsewhere, and then remove the now-unused gjson import so the file matches the migrated style consistently.tests/cli_e2e/drive/drive_member_add_dryrun_test.go (1)
307-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent migration: raw
gjson.Getwith hardcoded"data."prefix alongsideclie2e.DryRunGet.
bodyPathis manually prefixed with"data."and consumed via rawgjson.Get(out, bodyPath+...)(lines 314-333), while adjacent checks in the same test (lines 290-306, 310) already useclie2e.DryRunGet. This works today, but the hardcoded"data."literal won't track future changes to the shared envelope helper the way the rest of the migrated suite does.♻️ Proposed fix to fully align with clie2e.DryRunGet
- 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, The dry-run assertions in this test still use raw gjson access with a hardcoded "data." prefix, unlike the surrounding checks that already go through clie2e.DryRunGet. Update the bodyPath-based assertions in drive_member_add_dryrun_test.go to read from the clie2e.DryRunGet helper consistently, using the same helper for the members array and all member fields so the test follows the shared envelope behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/meta/identity_test.go`:
- Around line 58-68: Add test coverage in identity_test.go for the new
description parsing in Method.SupportsToken: add a case where Description
contains Identity: `user` only and verify it accepts user and rejects
tenant/bot, and add a true description-only case where AccessTokens is nil or
empty but Description still drives support checks. Update the existing
Method.SupportsToken tests to cover both the new user marker and the fallback
behavior when no tokens are declared, alongside the current bot-only mixed-token
case.
In `@internal/meta/identity.go`:
- Around line 64-66: `RestrictsIdentity()` in `Identity` is missing the
description-only marker check, so description-only bot/user methods are still
treated as unrestricted by the command identity gate. Update
`RestrictsIdentity()` to fold in the same `descriptionOnlyIdentity()` handling
already used by `SupportsToken()` and `Identities()`, returning the restricted
result for those identities as well.
---
Nitpick comments:
In `@shortcuts/apps/dryrun_test.go`:
- Around line 19-34: The UnmarshalJSON logic on dryRunAPIEnvelope still falls
back to the legacy top-level API field, which can hide regressions in the new
envelope format. Update the dryRunAPIEnvelope.UnmarshalJSON path so tests only
accept the nested data.api shape (or explicitly assert the raw envelope shape
once) and remove the silent fallback to raw.API; this will make any
cmdutil.WriteDryRun regression that emits the old format fail in dryrun_test.go.
In `@tests/cli_e2e/drive/drive_export_dryrun_test.go`:
- Around line 83-104: The TestDriveExportDryRun_MarkdownFetchAPI check still
parses stdout with DryRunData plus multiple gjson.Get calls, unlike the other
dry-run tests in this file. Update this test to use clie2e.DryRunGet for the API
and output fields, referencing the existing
TestDriveExportDryRun_MarkdownFetchAPI helper pattern used elsewhere, and then
remove the now-unused gjson import so the file matches the migrated style
consistently.
In `@tests/cli_e2e/drive/drive_member_add_dryrun_test.go`:
- Around line 307-333: The dry-run assertions in this test still use raw gjson
access with a hardcoded "data." prefix, unlike the surrounding checks that
already go through clie2e.DryRunGet. Update the bodyPath-based assertions in
drive_member_add_dryrun_test.go to read from the clie2e.DryRunGet helper
consistently, using the same helper for the members array and all member fields
so the test follows the shared envelope behavior.
🪄 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: e4114d79-6168-4a89-9c9f-e57bcc7ca02e
📒 Files selected for processing (97)
cmd/api/api.gocmd/api/api_test.gocmd/service/service.gocmd/service/service_test.gointernal/cmdutil/dryrun.gointernal/cmdutil/dryrun_test.gointernal/meta/identity.gointernal/meta/identity_test.gointernal/output/envelope.gointernal/output/envelope_success.gointernal/output/envelope_success_test.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_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/slides/slides_replace_pages_test.goshortcuts/task/task_upload_attachment_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/doc/docs_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_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_message_send_dryrun_test.gotests/cli_e2e/wiki/wiki_member_add_dryrun_test.gotests/cli_e2e/wiki/wiki_node_create_dryrun_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@31b9a9e20569b6a7daf1f09aa5f596818e4acc86🧩 Skill updatenpx skills add Tantanz20020918/cli#fix/dry-run-output-contract -y -g |
3 tasks
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
Unify
--dry-runJSON output under the standard success envelope so agents can reliably parseok,identity,dry_run, anddata.This fixes dry-run paths that previously emitted raw payloads or banners on stdout, which broke
--jqand pipe-based consumers.Changes
dry_runsupport to the success envelope and route raw API, service, and shortcut dry-run JSON output through the shared envelope writer.DryRunGethelper for envelope-aware assertions.Test Plan
make builddev-verifyvalidate scriptGOCACHE=/private/tmp/lark-cli-go-build go test ./internal/output ./internal/cmdutil ./internal/meta ./cmd ./cmd/api ./cmd/service ./shortcuts/common ./shortcuts/apps ./shortcuts/drive ./shortcuts/sheets ./shortcuts/slides ./shortcuts/task ./internal/qualitygate/rules -count=1GOCACHE=/private/tmp/lark-cli-go-build go test ./tests/cli_e2e/... -run DryRun -count=1Related Issues
Summary by CodeRabbit
dry_runflag and nested request details.--jqfiltering and structured previews for file uploads.data.apistructures.