feat: support lark suite hybrid skills layout#1772
Open
Tantanz20020918 wants to merge 8 commits into
Open
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 layout-aware skills syncing and persistence, suite installation and assembly, update-command layout flags and reporting, and lark-suite routing documentation. ChangesSkills Layout Support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 #1772 +/- ##
==========================================
- Coverage 74.42% 74.42% -0.01%
==========================================
Files 854 856 +2
Lines 88457 88926 +469
==========================================
+ Hits 65832 66180 +348
- Misses 17556 17629 +73
- Partials 5069 5117 +48 ☔ 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@38d54119cd10319f7da491486a597cef4753eb83🧩 Skill updatenpx skills add larksuite/cli#feat/lark-suite-skill -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: 4
🤖 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/update/update.go`:
- Around line 357-371: `validateSkillsLayoutOptions` currently returns early for
non-hybrid layouts, so `--flat-skills` is ignored when
`resolveSkillsSyncOptions` resolves to `separate`. Update this validation path
to detect when `opts.FlatSet` is true and `opts.FlatSkills` were provided
without `skillscheck.LayoutHybrid`, and return a validation error (or at least a
warning) instead of silently dropping the flag. Use
`validateSkillsLayoutOptions` and `resolveSkillsSyncOptions` as the main entry
points for the fix.
In `@internal/skillscheck/layout.go`:
- Around line 99-115: Route the directory traversal in listSuiteSubskills
through internal/vfs instead of calling os.ReadDir directly, so this logic can
be mocked in tests like the rest of the layout code. Add or use a vfs-backed
recursive directory listing helper to cover the missing WalkDir-style behavior,
then update listSuiteSubskills to use that abstraction while keeping the
existing filtering and sorting logic unchanged.
In `@internal/skillscheck/sync.go`:
- Around line 199-210: Consolidate the duplicate “newly-added official skills”
logic in SyncPlan generation so `newlyOfficialSkills` is the single source of
truth. Update the `SyncPlan` construction in `sync.go` (the branch around
`official`, `layout`, and `skippedDeleted`) to compute `newAddedOfficial` once
via `newlyOfficialSkills`, then reuse it when building `ToUpdate` and `Added`
instead of maintaining a separate inline previous-state comparison loop. Make
the same consolidation anywhere else in this flow that repeats the
hybrid/separate-layout official-skills diff logic so `SyncPlan` stays
consistent.
In `@isolated-skills/lark-suite/SKILL.md`:
- Around line 13-25: The `lark-suite` prompt is still telling the agent to
discover and read other skills’ SKILL.md files, which breaks skill isolation;
remove the cross-skill enumeration/discovery guidance from the suite routing
instructions and keep `lark-suite` limited to directing users to the appropriate
known `lark-*` sub-skill. Update the routing text in `lark-suite/SKILL.md` so it
no longer instructs reading unrelated peer skill manifests, and preserve only
the minimal handoff logic for selecting a sub-skill and relying on `lark-shared`
where applicable.
🪄 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: da095477-0fd1-49ad-a37e-e70fccaab18d
📒 Files selected for processing (9)
cmd/update/update.gocmd/update/update_test.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.gointernal/skillscheck/layout.gointernal/skillscheck/state.gointernal/skillscheck/sync.gointernal/skillscheck/sync_test.goisolated-skills/lark-suite/SKILL.md
Comment on lines
+357
to
+371
| func validateSkillsLayoutOptions(opts *UpdateOptions) errs.TypedError { | ||
| if _, ok := skillscheck.NormalizeLayout(opts.SkillsLayout); !ok { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--skills-layout must be one of separate or hybrid").WithParam("--skills-layout") | ||
| } | ||
| layout, flat := resolveSkillsSyncOptions(opts.SkillsLayout, opts.FlatSkills, opts.FlatSet) | ||
| if layout != skillscheck.LayoutHybrid { | ||
| return nil | ||
| } | ||
| for _, skill := range flat { | ||
| if skill == "lark-shared" { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "lark-shared cannot be selected by --flat-skills; it is managed automatically for lark-suite compatibility").WithParam("--flat-skills") | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--flat-skills is silently ignored when layout isn't hybrid.
If a user passes --flat-skills foo without --skills-layout hybrid (and no prior hybrid state), resolveSkillsSyncOptions resolves layout to separate, and validateSkillsLayoutOptions returns nil at Line 363 without ever checking opts.FlatSet. The flag is silently dropped with no error or warning, which can surprise users on this high-risk-write command.
🐛 Proposed fix
layout, flat := resolveSkillsSyncOptions(opts.SkillsLayout, opts.FlatSkills, opts.FlatSet)
+ if opts.FlatSet && layout != skillscheck.LayoutHybrid {
+ return errs.NewValidationError(errs.SubtypeInvalidArgument, "--flat-skills requires --skills-layout hybrid (or a previously saved hybrid layout)").WithParam("--flat-skills")
+ }
if layout != skillscheck.LayoutHybrid {
return nil
}📝 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
| func validateSkillsLayoutOptions(opts *UpdateOptions) errs.TypedError { | |
| if _, ok := skillscheck.NormalizeLayout(opts.SkillsLayout); !ok { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--skills-layout must be one of separate or hybrid").WithParam("--skills-layout") | |
| } | |
| layout, flat := resolveSkillsSyncOptions(opts.SkillsLayout, opts.FlatSkills, opts.FlatSet) | |
| if layout != skillscheck.LayoutHybrid { | |
| return nil | |
| } | |
| for _, skill := range flat { | |
| if skill == "lark-shared" { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "lark-shared cannot be selected by --flat-skills; it is managed automatically for lark-suite compatibility").WithParam("--flat-skills") | |
| } | |
| } | |
| return nil | |
| } | |
| func validateSkillsLayoutOptions(opts *UpdateOptions) errs.TypedError { | |
| if _, ok := skillscheck.NormalizeLayout(opts.SkillsLayout); !ok { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--skills-layout must be one of separate or hybrid").WithParam("--skills-layout") | |
| } | |
| layout, flat := resolveSkillsSyncOptions(opts.SkillsLayout, opts.FlatSkills, opts.FlatSet) | |
| if opts.FlatSet && layout != skillscheck.LayoutHybrid { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--flat-skills requires --skills-layout hybrid (or a previously saved hybrid layout)").WithParam("--flat-skills") | |
| } | |
| if layout != skillscheck.LayoutHybrid { | |
| return nil | |
| } | |
| for _, skill := range flat { | |
| if skill == "lark-shared" { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "lark-shared cannot be selected by --flat-skills; it is managed automatically for lark-suite compatibility").WithParam("--flat-skills") | |
| } | |
| } | |
| return nil | |
| } |
🤖 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 `@cmd/update/update.go` around lines 357 - 371, `validateSkillsLayoutOptions`
currently returns early for non-hybrid layouts, so `--flat-skills` is ignored
when `resolveSkillsSyncOptions` resolves to `separate`. Update this validation
path to detect when `opts.FlatSet` is true and `opts.FlatSkills` were provided
without `skillscheck.LayoutHybrid`, and return a validation error (or at least a
warning) instead of silently dropping the flag. Use
`validateSkillsLayoutOptions` and `resolveSkillsSyncOptions` as the main entry
points for the fix.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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
1 participant
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
Add layout-aware official skill sync so users can keep the default flat separate layout or use hybrid to route non-flat Lark skills through lark-suite. The suite template is fetched from an isolated source and assembled from the actually installed official skills during update.
Changes
Test Plan
Related Issues
Summary by CodeRabbit
--flat-skills.