test#1295
Open
SunPeiYang996 wants to merge 5 commits into
Open
Conversation
Add a top-level `lark-cli skills` meta command group that reads skill content compiled into the binary at build time via go:embed, so the content an AI agent reads always matches the running CLI version. It is an additional, version-consistent source and a fallback for when skills are not installed or are stale; it does not replace or thin the on-disk skills. Commands: - `skills list` lists every embedded skill with name, description, and the frontmatter metadata block. - `skills list <name>` / `skills list <name>/<sub>` list one directory layer (ls-style, no recursion); entries are skill-prefixed paths that can be fed straight back to `read`. - `skills read <name>` prints the skill's SKILL.md and appends a one-line tip steering the model to fetch reference files via this command. - `skills read <name> <path>` / `skills read <name>/<path>` print any file under the skill directory (no tip). - `--json` wraps output in an envelope; on the main SKILL.md it carries a separate `guidance` field instead of merging the tip into content. Implementation: - skills/ is embedded from the repo-root package main (go:embed cannot cross parent dirs) and injected into cmd/skill via a package variable, avoiding any change to cmd.Execute's signature. - internal/skillcontent is pure logic over an injected fs.FS; reference reads and directory listings share one path-traversal guard that rejects absolute paths, "..", and "..\\" escapes. - Errors are typed (validation -> exit 2, file_io -> exit 5); rejected paths never write anything to stdout. Risk is set per leaf subcommand (read); the group carries none, matching the config/service command groups. The commands need no auth (local embed reads only).
Set LARKSUITE_CLI_CONFIG_DIR to a per-test temp dir in the shared run() helper so the command tests never touch the real config dir, matching the repo-wide test convention. Addresses a CodeRabbit review comment.
github-actions
Bot
added
domain/ccm
📝 WalkthroughWalkthroughThe PR introduces an embedded skills CLI subsystem for accessing skill documentation and migrates all docs shortcuts from dual V1/V2 support to v2-only with improved help guidance. Legacy V1 (MCP) runtime paths, semantic warnings, and versioned help are removed. The skills system is wired into the CLI command tree and provides ChangesSkills System & Docs V2-Only Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@65b80cee82313e426080a43b7caab69ae4952b6e🧩 Skill updatenpx skills add larksuite/cli#sun/pre -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1295 +/- ##
==========================================
+ Coverage 70.33% 70.66% +0.33%
==========================================
Files 672 680 +8
Lines 65322 65477 +155
==========================================
+ Hits 45941 46272 +331
+ Misses 15728 15532 -196
- Partials 3653 3673 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
skills/lark-doc/SKILL.md (1)
7-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale
--api-versionguidance to match current docs command surface.Line 7 still recommends
lark-cli docs --api-version v2 --help, but the docs parent command no longer exposes a service-level--api-version. Line 12 also states the shortcut commands must carry--api-version v2, which is now misleading for v2-default flows. Please align this section with the current help contract to avoid directing users/agents to invalid or unnecessary invocation patterns.Suggested patch
- cliHelp: "lark-cli docs --api-version v2 --help; lark-cli docs +create --api-version v2 --help; lark-cli docs +fetch --api-version v2 --help; lark-cli docs +update --api-version v2 --help" + cliHelp: "lark-cli docs --help; lark-cli docs +create --help; lark-cli docs +fetch --help; lark-cli docs +update --help" @@ -> **⚠️ API 版本：本 skill 使用 v2 API。所有 `docs +create --api-version v2`、`docs +fetch --api-version v2`、`docs +update --api-version v2` 命令必须携带 `--api-version v2`。** +> **⚠️ API 版本：本 skill 使用 v2 API。`docs +create` / `docs +fetch` / `docs +update` 默认走 v2；如需显式声明可附加 `--api-version v2`。**🤖 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 `@skills/lark-doc/SKILL.md` around lines 7 - 12, Update the stale guidance that tells users to run the parent command with --api-version v2: modify the cliHelp string (the "cliHelp" entry) and the docs vignette under the "docs (v2)" heading so it no longer instructs using `lark-cli docs --api-version v2` or claims subcommands "must" carry the flag; instead state that v2 is the default for this skill and that only when invoking a non-default API version should callers add `--api-version`, or explicitly indicate that `docs +create|+fetch|+update --api-version v2` is optional when v2 is default. Ensure the text and examples consistently reflect the current help contract and remove any reference to a service-level --api-version on the parent `docs` command.tests/cli_e2e/docs/docs_update_dryrun_test.go (1)
16-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd config-dir isolation to this E2E test setup.
Set
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())to avoid cross-test config coupling.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 16 - 24, The test TestDocs_DryRunDefaultsToV2OpenAPI needs to isolate CLI config state; add a call to set the config dir to a temp directory by invoking t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) in the test setup (near where other t.Setenv calls are made) so the test uses its own config directory and avoids cross-test coupling.tests/cli_e2e/docs/docs_create_fetch_test.go (1)
19-22: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd per-test config-dir isolation at test startup.
Both test entry points should set
LARKSUITE_CLI_CONFIG_DIRto a temp directory to avoid shared config bleed across test runs.As per coding guidelines,
**/*_test.go: uset.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())to isolate config state in tests.Also applies to: 56-60
🤖 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_create_fetch_test.go` around lines 19 - 22, Add per-test config-dir isolation by setting the LARKSUITE_CLI_CONFIG_DIR env to a fresh temp dir at test startup: in TestDocs_CreateAndFetchWorkflowAsBot (and other test entry points mentioned around lines 56-60) call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) immediately after creating the test context so each test gets an isolated config directory and avoids shared config bleed.
🧹 Nitpick comments (1)
shortcuts/doc/docs_update_v2.go (1)
13-39: ⚡ Quick winUnify command definitions to one source of truth.
validCommandsV2andvalidCommandsV2Keys()duplicate the same command list. If one side changes first, flag enum/help and runtime validation can diverge.Proposed refactor
+var validCommandsV2List = []string{ + "str_replace", + "block_delete", + "block_insert_after", + "block_copy_insert_after", + "block_replace", + "block_move_after", + "overwrite", + "append", +} + var validCommandsV2 = map[string]bool{ - "str_replace": true, - "block_delete": true, - "block_insert_after": true, - "block_copy_insert_after": true, - "block_replace": true, - "block_move_after": true, - "overwrite": true, - "append": true, +} + +func init() { + for _, c := range validCommandsV2List { + validCommandsV2[c] = true + } } @@ func validCommandsV2Keys() []string { - return []string{"str_replace", "block_delete", "block_insert_after", "block_copy_insert_after", "block_replace", "block_move_after", "overwrite", "append"} + return append([]string(nil), validCommandsV2List...) }🤖 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 13 - 39, validCommandsV2 and validCommandsV2Keys duplicate the same list causing drift; consolidate to a single source of truth (e.g., a single slice variable like validCommandsV2Keys) and derive the map and the v2UpdateFlags Enum from that slice: keep one canonical slice (referenced by validCommandsV2Keys), update v2UpdateFlags to use that slice for the Flag Enum, and build validCommandsV2 map programmatically from the slice for runtime validation (refer to symbols validCommandsV2, validCommandsV2Keys, and v2UpdateFlags).
🤖 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 `@tests/cli_e2e/docs/docs_update_dryrun_test.go`:
- Around line 70-84: The test currently concatenates result.Stdout and
result.Stderr into combined and asserts request-shape strings against that;
update the assertions to check only result.Stdout (not combined) for the
positive expectations (tt.wantURL and "docs_ai/v1") and for the negative checks
("/mcp", "MCP tool", "--api-version") so that JSON envelope output is validated
strictly from result.Stdout while stderr remains separate.
---
Outside diff comments:
In `@skills/lark-doc/SKILL.md`:
- Around line 7-12: Update the stale guidance that tells users to run the parent
command with --api-version v2: modify the cliHelp string (the "cliHelp" entry)
and the docs vignette under the "docs (v2)" heading so it no longer instructs
using `lark-cli docs --api-version v2` or claims subcommands "must" carry the
flag; instead state that v2 is the default for this skill and that only when
invoking a non-default API version should callers add `--api-version`, or
explicitly indicate that `docs +create|+fetch|+update --api-version v2` is
optional when v2 is default. Ensure the text and examples consistently reflect
the current help contract and remove any reference to a service-level
--api-version on the parent `docs` command.
In `@tests/cli_e2e/docs/docs_create_fetch_test.go`:
- Around line 19-22: Add per-test config-dir isolation by setting the
LARKSUITE_CLI_CONFIG_DIR env to a fresh temp dir at test startup: in
TestDocs_CreateAndFetchWorkflowAsBot (and other test entry points mentioned
around lines 56-60) call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
immediately after creating the test context so each test gets an isolated config
directory and avoids shared config bleed.
In `@tests/cli_e2e/docs/docs_update_dryrun_test.go`:
- Around line 16-24: The test TestDocs_DryRunDefaultsToV2OpenAPI needs to
isolate CLI config state; add a call to set the config dir to a temp directory
by invoking t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) in the test setup
(near where other t.Setenv calls are made) so the test uses its own config
directory and avoids cross-test coupling.
---
Nitpick comments:
In `@shortcuts/doc/docs_update_v2.go`:
- Around line 13-39: validCommandsV2 and validCommandsV2Keys duplicate the same
list causing drift; consolidate to a single source of truth (e.g., a single
slice variable like validCommandsV2Keys) and derive the map and the
v2UpdateFlags Enum from that slice: keep one canonical slice (referenced by
validCommandsV2Keys), update v2UpdateFlags to use that slice for the Flag Enum,
and build validCommandsV2 map programmatically from the slice for runtime
validation (refer to symbols validCommandsV2, validCommandsV2Keys, and
v2UpdateFlags).
🪄 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: 026ba463-f61a-4b10-a00d-8b37abcc615f
📒 Files selected for processing (38)
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/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/lark-doc/references/lark-doc-xml.mdskills/lark-doc/references/style/lark-doc-create-workflow.mdskills/lark-doc/references/style/lark-doc-style.mdskills/lark-doc/references/style/lark-doc-update-workflow.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 (12)
- skills/lark-doc/references/lark-doc-create.md
- skills/lark-doc/references/style/lark-doc-style.md
- skills/lark-doc/references/style/lark-doc-create-workflow.md
- skills/lark-doc/references/style/lark-doc-update-workflow.md
- shortcuts/doc/docs_update_check_test.go
- shortcuts/doc/versioned_help.go
- skills/lark-doc/references/lark-doc-md.md
- skills/lark-doc/references/lark-doc-xml.md
- skills/lark-doc/references/lark-doc-fetch.md
- shortcuts/doc/versioned_help_test.go
- shortcuts/doc/docs_update_check.go
- skills/lark-doc/references/lark-doc-update.md
Comment on lines
+70
to
+84
| combined := result.Stdout + "\n" + result.Stderr | ||
| for _, want := range []string{ | ||
| tt.wantURL, | ||
| "docs_ai/v1", | ||
| } { | ||
| if !strings.Contains(combined, want) { | ||
| t.Fatalf("dry-run output missing %q\nstdout:\n%s\nstderr:\n%s", want, result.Stdout, result.Stderr) | ||
| } | ||
| } | ||
| if strings.Contains(combined, "/mcp") || strings.Contains(combined, "MCP tool") { | ||
| t.Fatalf("dry-run output should not use MCP\nstdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr) | ||
| } | ||
| if strings.Contains(combined, "--api-version") { | ||
| t.Fatalf("dry-run output should not ask for --api-version\nstdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr) | ||
| } |
There was a problem hiding this comment.
Don’t merge stdout and stderr in dry-run success assertions.
Line 70 currently combines streams; this can pass even if the dry-run JSON envelope stops being written to stdout. Assert request-shape expectations from result.Stdout only.
Suggested change
- combined := result.Stdout + "\n" + result.Stderr
+ payload := result.Stdout
for _, want := range []string{
tt.wantURL,
"docs_ai/v1",
} {
- if !strings.Contains(combined, want) {
+ if !strings.Contains(payload, want) {
t.Fatalf("dry-run output missing %q\nstdout:\n%s\nstderr:\n%s", want, result.Stdout, result.Stderr)
}
}
- if strings.Contains(combined, "/mcp") || strings.Contains(combined, "MCP tool") {
+ if strings.Contains(payload, "/mcp") || strings.Contains(payload, "MCP tool") {
t.Fatalf("dry-run output should not use MCP\nstdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr)
}
- if strings.Contains(combined, "--api-version") {
+ if strings.Contains(payload, "--api-version") {
t.Fatalf("dry-run output should not ask for --api-version\nstdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr)
}📝 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.
Suggested change
| combined := result.Stdout + "\n" + result.Stderr | |
| for _, want := range []string{ | |
| tt.wantURL, | |
| "docs_ai/v1", | |
| } { | |
| if !strings.Contains(combined, want) { | |
| t.Fatalf("dry-run output missing %q\nstdout:\n%s\nstderr:\n%s", want, result.Stdout, result.Stderr) | |
| } | |
| } | |
| if strings.Contains(combined, "/mcp") || strings.Contains(combined, "MCP tool") { | |
| t.Fatalf("dry-run output should not use MCP\nstdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr) | |
| } | |
| if strings.Contains(combined, "--api-version") { | |
| t.Fatalf("dry-run output should not ask for --api-version\nstdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr) | |
| } | |
| payload := result.Stdout | |
| for _, want := range []string{ | |
| tt.wantURL, | |
| "docs_ai/v1", | |
| } { | |
| if !strings.Contains(payload, want) { | |
| t.Fatalf("dry-run output missing %q\nstdout:\n%s\nstderr:\n%s", want, result.Stdout, result.Stderr) | |
| } | |
| } | |
| if strings.Contains(payload, "/mcp") || strings.Contains(payload, "MCP tool") { | |
| t.Fatalf("dry-run output should not use MCP\nstdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr) | |
| } | |
| if strings.Contains(payload, "--api-version") { | |
| t.Fatalf("dry-run output should not ask for --api-version\nstdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr) | |
| } |
🤖 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 70 - 84, The test
currently concatenates result.Stdout and result.Stderr into combined and asserts
request-shape strings against that; update the assertions to check only
result.Stdout (not combined) for the positive expectations (tt.wantURL and
"docs_ai/v1") and for the negative checks ("/mcp", "MCP tool", "--api-version")
so that JSON envelope output is validated strictly from result.Stdout while
stderr remains separate.
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
Changes
Test Plan
lark-cli <domain> <command>flow works as expectedRelated Issues
Summary by CodeRabbit
New Features
skillscommand for listing and reading embedded skill documentation.Chores
+create,+fetch,+update) exclusively to v2 API; v1 (MCP) support removed.