refactor(docs): remove v1 execution paths and standardize on v2#1291
Conversation
|
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:
📝 WalkthroughWalkthroughDocs shortcuts ( ChangesDocs shortcuts v2-only migration
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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 #1291 +/- ##
==========================================
+ Coverage 71.20% 71.33% +0.12%
==========================================
Files 688 686 -2
Lines 65997 65369 -628
==========================================
- Hits 46996 46631 -365
+ Misses 15340 15089 -251
+ Partials 3661 3649 -12 ☔ 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@2d344186beb4d15d02b4ecba38c4063e19b9ec99🧩 Skill updatenpx skills add larksuite/cli#sun/chore -y -g |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cli_e2e/docs/docs_update_dryrun_test.go (1)
16-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIsolate config-dir state in this E2E dry-run test setup.
The test sets credential env vars but leaves config-dir shared, which can make dry-run assertions sensitive to local config state.
Suggested patch
func TestDocs_DryRunDefaultsToV2OpenAPI(t *testing.T) { // Fake creds are enough — dry-run short-circuits before any real API call. t.Setenv("LARKSUITE_CLI_APP_ID", "app") t.Setenv("LARKSUITE_CLI_APP_SECRET", "secret") t.Setenv("LARKSUITE_CLI_BRAND", "feishu") + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())As per coding guidelines:
**/*_test.go: Uset.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())to isolate config state in tests.🤖 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/docs/docs_update_dryrun_test.go` around lines 16 - 23, In TestDocs_DryRunDefaultsToV2OpenAPI add isolation for the CLI config dir by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) (e.g., immediately after setting the other env vars and before creating the context) so the test uses a fresh temp config directory; update the TestDocs_DryRunDefaultsToV2OpenAPI function to include this call.
🤖 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/doc/docs_update_test.go`:
- Around line 106-135: newUpdateShortcutTestRuntime currently doesn't isolate
LARKSUITE_CLI_CONFIG_DIR and can leak config between parallel tests; update the
helper (newUpdateShortcutTestRuntime) to call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) early in the function so each
test gets its own temporary config dir before creating the cobra.Command and
flags.
---
Outside diff comments:
In `@tests/cli_e2e/docs/docs_update_dryrun_test.go`:
- Around line 16-23: In TestDocs_DryRunDefaultsToV2OpenAPI add isolation for the
CLI config dir by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
(e.g., immediately after setting the other env vars and before creating the
context) so the test uses a fresh temp config directory; update the
TestDocs_DryRunDefaultsToV2OpenAPI function to include this call.
🪄 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: e352719b-7cf5-480c-b50f-633253c120b1
📒 Files selected for processing (30)
cmd/build.gocmd/skill/skill.gocmd/skill/skill_test.gointernal/skillcontent/reader.gointernal/skillcontent/reader_test.goshortcuts/doc/code_fence.goshortcuts/doc/docs_create.goshortcuts/doc/docs_create_test.goshortcuts/doc/docs_create_v2.goshortcuts/doc/docs_fetch.goshortcuts/doc/docs_fetch_v2.goshortcuts/doc/docs_fetch_v2_test.goshortcuts/doc/docs_update.goshortcuts/doc/docs_update_check.goshortcuts/doc/docs_update_check_test.goshortcuts/doc/docs_update_test.goshortcuts/doc/docs_update_v2.goshortcuts/doc/get_skill.goshortcuts/doc/shortcuts.goshortcuts/doc/v2_only.goshortcuts/doc/v2_only_test.goshortcuts/doc/versioned_help.goshortcuts/doc/versioned_help_test.goshortcuts/register_test.goskills_embed.gotests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_create_fetch_test.gotests/cli_e2e/docs/docs_update_dryrun_test.gotests/cli_e2e/docs/docs_update_test.gotests/cli_e2e/docs/helpers_test.go
💤 Files with no reviewable changes (4)
- shortcuts/doc/versioned_help.go
- shortcuts/doc/docs_update_check_test.go
- shortcuts/doc/docs_update_check.go
- shortcuts/doc/versioned_help_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/doc/docs_update_v2.go (1)
61-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
str_replacecannot reliably carry an explicit empty replacementAt Line 156,
contentis only serialized when non-empty, so--content ""gets dropped from the request body. That conflicts with the stated behavior that empty content deletes matches and makes omission vs explicit-empty indistinguishable.💡 Proposed fix
case "str_replace": if pattern == "" { return common.FlagErrorf("--command str_replace requires --pattern") } + if !runtime.Changed("content") { + return common.FlagErrorf("--command str_replace requires --content (use --content \"\" to delete match)") + } @@ - if v := runtime.Str("content"); v != "" { - body["content"] = v - } + if runtime.Changed("content") { + body["content"] = runtime.Str("content") + }Also applies to: 156-158
🤖 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/doc/docs_update_v2.go` around lines 61 - 64, The request body drops an explicit empty replacement because the serialization omits content when it's "", making omission indistinguishable from explicit-empty for the "str_replace" command; modify the code that builds the request payload so the content field is always included when the user provided the --content flag (even if empty) — e.g., track presence of the flag or switch the payload field to a *string (or explicit boolean like ContentSet) and set it to "" (or pointer to "") when the flag was passed; update the "str_replace" case and the serialization logic that currently skips content (around the content serialization at lines ~156-158) to ensure the content key is present in the sent JSON when intended.
🤖 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/doc/v2_only.go`:
- Around line 52-59: docsLegacyFlagDefinitions currently registers legacy flags
without their type metadata, causing Cobra to parse booleans/ints incorrectly
before validateDocsV2Only runs; update docsLegacyFlagDefinitions to copy the
legacy flag's type information into the generated common.Flag (e.g., set the
Type/Kind field on common.Flag from the docsLegacyFlag.Type or similar
identifier) so the parser treats legacy flags with their original types and only
validation/redirect happens in validateDocsV2Only; locate
docsLegacyFlagDefinitions and docsLegacyFlag and ensure the common.Flag
construction includes the legacy type field when appending.
---
Outside diff comments:
In `@shortcuts/doc/docs_update_v2.go`:
- Around line 61-64: The request body drops an explicit empty replacement
because the serialization omits content when it's "", making omission
indistinguishable from explicit-empty for the "str_replace" command; modify the
code that builds the request payload so the content field is always included
when the user provided the --content flag (even if empty) — e.g., track presence
of the flag or switch the payload field to a *string (or explicit boolean like
ContentSet) and set it to "" (or pointer to "") when the flag was passed; update
the "str_replace" case and the serialization logic that currently skips content
(around the content serialization at lines ~156-158) to ensure the content key
is present in the sent JSON when intended.
🪄 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: 663a70ae-59f3-498e-85dc-50b7774eaaa4
📒 Files selected for processing (30)
shortcuts/doc/docs_create.goshortcuts/doc/docs_create_test.goshortcuts/doc/docs_create_v2.goshortcuts/doc/docs_fetch.goshortcuts/doc/docs_fetch_v2.goshortcuts/doc/docs_fetch_v2_test.goshortcuts/doc/docs_update.goshortcuts/doc/docs_update_check.goshortcuts/doc/docs_update_check_test.goshortcuts/doc/docs_update_test.goshortcuts/doc/docs_update_v2.goshortcuts/doc/markdown_fix.goshortcuts/doc/markdown_fix_hardening_test.goshortcuts/doc/markdown_fix_test.goshortcuts/doc/shortcuts.goshortcuts/doc/v2_only.goshortcuts/doc/v2_only_test.goshortcuts/doc/versioned_help.goshortcuts/doc/versioned_help_test.goshortcuts/register_test.goskills/lark-doc/SKILL.mdskills/lark-doc/references/lark-doc-create.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-doc/references/lark-doc-md.mdskills/lark-doc/references/lark-doc-update.mdtests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_create_fetch_test.gotests/cli_e2e/docs/docs_update_dryrun_test.gotests/cli_e2e/docs/docs_update_test.gotests/cli_e2e/docs/helpers_test.go
💤 Files with no reviewable changes (7)
- shortcuts/doc/markdown_fix_hardening_test.go
- shortcuts/doc/markdown_fix_test.go
- shortcuts/doc/versioned_help_test.go
- shortcuts/doc/docs_update_check_test.go
- shortcuts/doc/markdown_fix.go
- shortcuts/doc/docs_update_check.go
- shortcuts/doc/versioned_help.go
✅ Files skipped from review due to trivial changes (4)
- skills/lark-doc/references/lark-doc-md.md
- skills/lark-doc/SKILL.md
- skills/lark-doc/references/lark-doc-fetch.md
- tests/cli_e2e/docs/coverage.md
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/cli_e2e/docs/docs_create_fetch_test.go
- tests/cli_e2e/docs/docs_update_test.go
- tests/cli_e2e/docs/helpers_test.go
- shortcuts/doc/docs_fetch_v2_test.go
- shortcuts/doc/docs_create_v2.go
- shortcuts/doc/docs_update.go
- shortcuts/doc/docs_fetch_v2.go
- tests/cli_e2e/docs/docs_update_dryrun_test.go
- shortcuts/doc/v2_only_test.go
- shortcuts/doc/docs_update_test.go
- shortcuts/doc/docs_fetch.go
- shortcuts/doc/docs_create_test.go
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)
tests/cli_e2e/docs/docs_update_dryrun_test.go (1)
18-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd config-dir isolation for this E2E test setup.
This test sets credential env vars but does not isolate CLI config state, which can make outcomes depend on host/user config.
Suggested patch
func TestDocs_DryRunDefaultsToV2OpenAPI(t *testing.T) { // Fake creds are enough — dry-run short-circuits before any real API call. + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) t.Setenv("LARKSUITE_CLI_APP_ID", "app") t.Setenv("LARKSUITE_CLI_APP_SECRET", "secret") t.Setenv("LARKSUITE_CLI_BRAND", "feishu")As per coding guidelines:
**/*_test.go:Use t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate config state in tests.🤖 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/docs/docs_update_dryrun_test.go` around lines 18 - 20, The test currently sets credential env vars but doesn't isolate CLI config state; add a call to set LARKSUITE_CLI_CONFIG_DIR to a temporary directory (use t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())) alongside the existing t.Setenv calls so the test uses an isolated config directory and does not depend on host/user config; place this new t.Setenv immediately with the other env setup in the test function.Source: Coding guidelines
🤖 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 `@tests/cli_e2e/docs/docs_update_dryrun_test.go`:
- Around line 18-20: The test currently sets credential env vars but doesn't
isolate CLI config state; add a call to set LARKSUITE_CLI_CONFIG_DIR to a
temporary directory (use t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()))
alongside the existing t.Setenv calls so the test uses an isolated config
directory and does not depend on host/user config; place this new t.Setenv
immediately with the other env setup in the test function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97fa37ed-e19b-4992-a425-9049bcb2f963
📒 Files selected for processing (37)
cmd/build.gocmd/skill/skill.gocmd/skill/skill_test.gointernal/cmdutil/factory.gointernal/skillcontent/reader.gointernal/skillcontent/reader_test.goshortcuts/doc/docs_create.goshortcuts/doc/docs_create_test.goshortcuts/doc/docs_create_v2.goshortcuts/doc/docs_fetch.goshortcuts/doc/docs_fetch_v2.goshortcuts/doc/docs_fetch_v2_test.goshortcuts/doc/docs_update.goshortcuts/doc/docs_update_check.goshortcuts/doc/docs_update_check_test.goshortcuts/doc/docs_update_test.goshortcuts/doc/docs_update_v2.goshortcuts/doc/markdown_fix.goshortcuts/doc/markdown_fix_hardening_test.goshortcuts/doc/markdown_fix_test.goshortcuts/doc/shortcuts.goshortcuts/doc/v2_only.goshortcuts/doc/v2_only_test.goshortcuts/doc/versioned_help.goshortcuts/doc/versioned_help_test.goshortcuts/register_test.goskills/lark-doc/SKILL.mdskills/lark-doc/references/lark-doc-create.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-doc/references/lark-doc-md.mdskills/lark-doc/references/lark-doc-update.mdskills_embed.gotests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_create_fetch_test.gotests/cli_e2e/docs/docs_update_dryrun_test.gotests/cli_e2e/docs/docs_update_test.gotests/cli_e2e/docs/helpers_test.go
💤 Files with no reviewable changes (7)
- shortcuts/doc/docs_update_check_test.go
- shortcuts/doc/versioned_help_test.go
- shortcuts/doc/markdown_fix.go
- shortcuts/doc/markdown_fix_test.go
- shortcuts/doc/docs_update_check.go
- shortcuts/doc/markdown_fix_hardening_test.go
- shortcuts/doc/versioned_help.go
✅ Files skipped from review due to trivial changes (5)
- skills/lark-doc/SKILL.md
- skills/lark-doc/references/lark-doc-md.md
- skills/lark-doc/references/lark-doc-update.md
- skills/lark-doc/references/lark-doc-fetch.md
- tests/cli_e2e/docs/coverage.md
🚧 Files skipped from review as they are similar to previous changes (16)
- shortcuts/doc/docs_update_v2.go
- skills/lark-doc/references/lark-doc-create.md
- tests/cli_e2e/docs/helpers_test.go
- shortcuts/doc/docs_fetch_v2.go
- shortcuts/doc/docs_create_v2.go
- shortcuts/doc/shortcuts.go
- shortcuts/doc/docs_fetch_v2_test.go
- shortcuts/doc/docs_update.go
- tests/cli_e2e/docs/docs_update_test.go
- shortcuts/doc/docs_create_test.go
- shortcuts/doc/v2_only_test.go
- internal/skillcontent/reader_test.go
- shortcuts/doc/docs_fetch.go
- shortcuts/doc/docs_update_test.go
- shortcuts/doc/docs_create.go
- shortcuts/register_test.go
Change-Id: I29d0af3e5325261f94949d3ab3f65051fb6bd52b
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/doc/v2_only.go (1)
52-62:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffHidden compatibility flags should preserve legacy flag types.
The
docsLegacyFlagDefinitionsfunction registers legacy flags without type metadata. This can cause Cobra parse errors for boolean/integer legacy flags beforevalidateDocsV2Onlyruns, preventing users from receiving the intended v2-only guidance error.🤖 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/doc/v2_only.go` around lines 52 - 62, The docsLegacyFlagDefinitions function is registering legacy flags without preserving their type, which causes Cobra to parse booleans/integers incorrectly before validateDocsV2Only runs; update docsLegacyFlagDefinitions to copy the flag.Type (and any other necessary metadata like Default/Value if present on docsLegacyFlag) into the returned common.Flag so the legacy flag Type is preserved when creating the common.Flag (referencing docsLegacyFlag and common.Flag) and ensure validateDocsV2Only still performs the v2-only guidance check.
🤖 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.
Duplicate comments:
In `@shortcuts/doc/v2_only.go`:
- Around line 52-62: The docsLegacyFlagDefinitions function is registering
legacy flags without preserving their type, which causes Cobra to parse
booleans/integers incorrectly before validateDocsV2Only runs; update
docsLegacyFlagDefinitions to copy the flag.Type (and any other necessary
metadata like Default/Value if present on docsLegacyFlag) into the returned
common.Flag so the legacy flag Type is preserved when creating the common.Flag
(referencing docsLegacyFlag and common.Flag) and ensure validateDocsV2Only still
performs the v2-only guidance check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 555d85e2-51cf-44eb-8fc1-6bdbaa28cd2e
📒 Files selected for processing (30)
shortcuts/doc/docs_create.goshortcuts/doc/docs_create_test.goshortcuts/doc/docs_create_v2.goshortcuts/doc/docs_fetch.goshortcuts/doc/docs_fetch_v2.goshortcuts/doc/docs_fetch_v2_test.goshortcuts/doc/docs_update.goshortcuts/doc/docs_update_check.goshortcuts/doc/docs_update_check_test.goshortcuts/doc/docs_update_test.goshortcuts/doc/docs_update_v2.goshortcuts/doc/markdown_fix.goshortcuts/doc/markdown_fix_hardening_test.goshortcuts/doc/markdown_fix_test.goshortcuts/doc/shortcuts.goshortcuts/doc/v2_only.goshortcuts/doc/v2_only_test.goshortcuts/doc/versioned_help.goshortcuts/doc/versioned_help_test.goshortcuts/register_test.goskills/lark-doc/SKILL.mdskills/lark-doc/references/lark-doc-create.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-doc/references/lark-doc-md.mdskills/lark-doc/references/lark-doc-update.mdtests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_create_fetch_test.gotests/cli_e2e/docs/docs_update_dryrun_test.gotests/cli_e2e/docs/docs_update_test.gotests/cli_e2e/docs/helpers_test.go
💤 Files with no reviewable changes (7)
- shortcuts/doc/markdown_fix_test.go
- shortcuts/doc/docs_update_check.go
- shortcuts/doc/versioned_help_test.go
- shortcuts/doc/docs_update_check_test.go
- shortcuts/doc/versioned_help.go
- shortcuts/doc/markdown_fix.go
- shortcuts/doc/markdown_fix_hardening_test.go
✅ Files skipped from review due to trivial changes (5)
- skills/lark-doc/references/lark-doc-md.md
- skills/lark-doc/references/lark-doc-fetch.md
- skills/lark-doc/SKILL.md
- skills/lark-doc/references/lark-doc-update.md
- tests/cli_e2e/docs/coverage.md
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/cli_e2e/docs/docs_create_fetch_test.go
- tests/cli_e2e/docs/docs_update_test.go
- skills/lark-doc/references/lark-doc-create.md
- shortcuts/doc/docs_create_v2.go
- tests/cli_e2e/docs/helpers_test.go
- shortcuts/doc/docs_update_v2.go
- shortcuts/doc/shortcuts.go
- shortcuts/doc/docs_fetch_v2.go
- tests/cli_e2e/docs/docs_update_dryrun_test.go
- shortcuts/doc/docs_fetch.go
- shortcuts/doc/docs_update.go
- shortcuts/doc/docs_create.go
- shortcuts/register_test.go
Change-Id: I29d0af3e5325261f94949d3ab3f65051fb6bd52b
refactor(docs): remove v1 execution paths and standardize on v2
Summary
This PR removes the legacy docs v1 shortcut implementation and makes docs +create, docs +fetch, and docs +update always use the docs v2/OpenAPI flow.
It keeps rollback-safe compatibility parsing for deprecated flags such as --api-version v1, but both v1 and v2 now execute the v2 path. Legacy v1-only flags are hidden and rejected with structured, actionable migration errors
that point users and AI agents to the current lark-cli skills read lark-doc ... guidance.
Key Changes
Summary by CodeRabbit
Release Notes
New Features
--content,--doc-format,--parent-token) for improved usability.Improvements
docs +create,docs +fetch, anddocs +updateto exclusively use v2 (OpenAPI) endpoints; v1 (MCP) support removed.--api-versionflag now hidden but still accepted for backward compatibility.Documentation