feat: support separate and suite skill layouts - #2211
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe update command supports separate and suite skill layouts. Synchronization uses explicit sources, archive validation, source fallback, suite cropping, stale-skill removal, persisted layout state, and structured failure reporting. ChangesSkills layout synchronization
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant UpdateCommand
participant SkillsSynchronizer
participant Updater
participant SkillsSource
UpdateCommand->>SkillsSynchronizer: Synchronize requested layout
SkillsSynchronizer->>Updater: FetchSkillsIndex(source)
Updater->>SkillsSource: Request archive metadata
SkillsSource-->>Updater: Return index and digests
SkillsSynchronizer->>Updater: Install skills or stage suite
Updater-->>SkillsSynchronizer: Return synchronization result
SkillsSynchronizer-->>UpdateCommand: Return layout, warnings, or typed error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
github-actions
Bot
added
the
size/L
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@e28d7b536ba5aff0da0eeec72d8ba55157e38e27🧩 Skill updatenpx skills add larksuite/cli#feat/skills-layout-suite -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2211 +/- ##
==========================================
+ Coverage 75.85% 76.18% +0.33%
==========================================
Files 958 987 +29
Lines 101701 104510 +2809
==========================================
+ Hits 77150 79626 +2476
- Misses 18684 18812 +128
- Partials 5867 6072 +205 ☔ 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: 8
🧹 Nitpick comments (10)
internal/skillscheck/state_test.go (1)
65-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the nil-state case to
TestEffectiveLayoutDefaultsLegacyStateToSeparate.
EffectiveLayoutguardsstate != nil, andResolveLayoutcalls it with a possibly nil state whenreadableis true. That branch has no direct assertion.💚 Proposed addition
func TestEffectiveLayoutDefaultsLegacyStateToSeparate(t *testing.T) { + if got := EffectiveLayout(nil); got != LayoutSeparate { + t.Fatalf("EffectiveLayout(nil) = %q, want separate", got) + } if got := EffectiveLayout(&SkillsState{Version: "1.0.0"}); got != LayoutSeparate { t.Fatalf("EffectiveLayout(legacy) = %q, want separate", got) }🤖 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 `@internal/skillscheck/state_test.go` around lines 65 - 72, Add a nil-state assertion to TestEffectiveLayoutDefaultsLegacyStateToSeparate, verifying that EffectiveLayout(nil) returns LayoutSeparate. Keep the existing legacy-version and explicit-suite assertions unchanged.internal/selfupdate/updater_test.go (1)
243-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the stale
ListOfficialSkillsIndexreferences.The method is now
FetchSkillsIndex. The test names (TestListOfficialSkillsIndexSuccess,TestListOfficialSkillsIndexHTTPError,TestListOfficialSkillsIndexBodyTooLarge,TestListOfficialSkillsIndexTimeout,TestListOfficialSkillsIndexRejectsNonHTTPSRedirect,TestListOfficialSkillsIndexUsesOverride) and everyt.Fatalfmessage still name the removed method. Failure output now points at a symbol that no longer exists.♻️ Example rename for one case
-func TestListOfficialSkillsIndexSuccess(t *testing.T) { +func TestFetchSkillsIndexSuccess(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, `{"skills":[{"name":"lark-calendar"}]}`) })) defer server.Close() @@ result := New().FetchSkillsIndex("https://open.feishu.cn/lark-cli") if result.Err != nil { - t.Fatalf("ListOfficialSkillsIndex() err = %v, want nil", result.Err) + t.Fatalf("FetchSkillsIndex() err = %v, want nil", result.Err) }🤖 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 `@internal/selfupdate/updater_test.go` around lines 243 - 260, Rename all stale ListOfficialSkillsIndex references in the affected tests to FetchSkillsIndex, including each TestListOfficialSkillsIndex* function name and every corresponding t.Fatalf message. Keep the test behavior and assertions unchanged.internal/selfupdate/updater.go (2)
281-284: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider passing
sourcetoSkillsIndexFetchOverride.
FetchSkillsIndexnow takes an explicitsource, but the override signature staysfunc() *NpmResult. Tests that drive the two-source retry loop throughUpdatertherefore cannot simulate a per-source index outcome (for example, primary fails and secondary succeeds).SkillsCommandInDirOverridealready carries its new parameter.♻️ Proposed signature change
- SkillsIndexFetchOverride func() *NpmResult + SkillsIndexFetchOverride func(source string) *NpmResultfunc (u *Updater) FetchSkillsIndex(source string) *NpmResult { if u.SkillsIndexFetchOverride != nil { - return u.SkillsIndexFetchOverride() + return u.SkillsIndexFetchOverride(source) }This also requires updating the override call sites in
internal/selfupdate/updater_test.goandcmd/update/update_test.go.🤖 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 `@internal/selfupdate/updater.go` around lines 281 - 284, Update FetchSkillsIndex and the SkillsIndexFetchOverride function type so the source argument is passed through to the override, enabling source-specific results during retries. Adjust all override definitions and call sites in updater_test.go and update_test.go to accept and use the new source parameter, while preserving existing behavior for normal fetches.
353-358: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
RemoveGlobalSkillsagainst an emptynamesslice.With
namesempty, the built arguments are-y skills remove -g -s -y. The trailing-yis then consumed as the value of-s, and the command targets a skill namedy. Today every caller guards the empty case (removeSkillsininternal/skillscheck/layout.goreturns early, and the other call sites pass a one-element slice), so this is not currently reachable. Add the guard inside the method because the sink is a destructiveskills remove.♻️ Proposed guard
func (u *Updater) RemoveGlobalSkills(names []string) *NpmResult { + if len(names) == 0 { + return &NpmResult{} + } args := []string{"-y", "skills", "remove", "-g", "-s"}🤖 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 `@internal/selfupdate/updater.go` around lines 353 - 358, Update Updater.RemoveGlobalSkills to return without invoking runSkillsCommand when names is empty, before constructing or executing the destructive removal arguments. Preserve the existing command construction and execution for non-empty names.cmd/update/update_test.go (2)
100-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
--listfixture branch.
successfulSkillsCommandstill handles-y skills add https://open.feishu.cn --list. No production code emits that argument list any more; the official index now comes fromFetchSkillsIndex, andsuccessfulSkillsIndexFetchsupplies it in these tests. The branch can never match, and it suggests a code path that no longer exists.♻️ Proposed cleanup
switch strings.Join(args, " ") { - case "-y skills add https://open.feishu.cn --list": - r.Stdout.WriteString("Available Skills\n │ lark-calendar\n │ lark-mail\n") case "-y skills ls -g --json":Confirm no other test relies on this branch before removing it.
🤖 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_test.go` around lines 100 - 102, Remove the unreachable “-y skills add https://open.feishu.cn --list” case from successfulSkillsCommand, along with its fixture output. Confirm the remaining tests obtain the official index through FetchSkillsIndex and successfulSkillsIndexFetch, and preserve all other command handling unchanged.
1185-1195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo new tests omit the
LARKSUITE_CLI_CONFIG_DIRisolation that the rest of the suite applies. Both tests exercise a code path that returns before any state read, so neither touches the config directory today. The shared root cause is the same: the isolation is treated as optional when the current control flow happens to short-circuit, which makes the tests fragile against any reordering that moves state access earlier.
cmd/update/update_test.go#L1185-L1195: addt.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())as the first statement ofTestUpdateRejectsInvalidSkillsLayout, and also extend the assertion to coverproblem.Categoryalongside the existingSubtypeandParamchecks.internal/skillscheck/sync_test.go#L466-L471: addt.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())as the first statement ofTestSyncSkillsNilRunnerFails.As per coding guidelines: "Use
cmdutil.TestFactory(t, config)for test factories and setLARKSUITE_CLI_CONFIG_DIRtot.TempDir()witht.Setenvto isolate configuration state" and "Error-path tests must assert typed metadata througherrs.ProblemOf(category,subtype, andparam)".🤖 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_test.go` around lines 1185 - 1195, Isolate configuration state in both error-path tests and fully validate typed error metadata. In cmd/update/update_test.go lines 1185-1195, add LARKSUITE_CLI_CONFIG_DIR isolation as the first statement of TestUpdateRejectsInvalidSkillsLayout and extend its errs.ProblemOf assertion to check problem.Category in addition to Subtype and Param; in internal/skillscheck/sync_test.go lines 466-471, add the same environment isolation as the first statement of TestSyncSkillsNilRunnerFails.Source: Coding guidelines
cmd/update/update.go (2)
488-488: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
layoutcan be empty in a failure summary.
applySkillsResultcallsskillsSummaryon ther.Err != nilbranch. Some failure results carry no layout — for example the nil-runner guard and theResolveLayouterror path ininternal/skillscheck/sync.goboth build&SyncResult{Action: "failed", ...}withoutLayout. The JSON output then contains"layout": "", which no consumer can interpret.Omit the key when the layout is empty.
♻️ Proposed change
summary := map[string]interface{}{ "official": len(r.Official), "updated": len(r.Updated), "added": len(r.Added), "skipped_deleted": len(r.SkippedDeleted), - "layout": r.Layout, } + if r.Layout != "" { + summary["layout"] = r.Layout + }🤖 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` at line 488, Update the layout field assembled by skillsSummary so empty r.Layout values are omitted from the failure-summary JSON, while preserving the existing layout output when non-empty. Use the surrounding summary construction and its JSON serialization behavior rather than changing SyncResult creation paths.
130-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreserve the
ParseLayoutcause.The error from
skillscheck.ParseLayoutis discarded. Attach it so the chain survives forerrors.Is/errors.Asand for debug output.♻️ Proposed change
- if _, err := skillscheck.ParseLayout(opts.SkillsLayout); err != nil { + if _, err := skillscheck.ParseLayout(opts.SkillsLayout); err != nil { return reportError(opts, io, "validation", - errs.NewValidationError(errs.SubtypeInvalidArgument, "--skills-layout must be one of separate or suite").WithParam("--skills-layout")) + errs.NewValidationError(errs.SubtypeInvalidArgument, "--skills-layout must be one of separate or suite"). + WithParam("--skills-layout"). + WithCause(err)) }As per coding guidelines: "Preserve typed lower-layer errors unchanged and preserve causes with
.WithCause(err)."🤖 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 130 - 133, The validation error returned by the skillscheck.ParseLayout call currently discards the parser cause. Update the errs.NewValidationError chain in the surrounding validation flow to attach the original err with .WithCause(err), preserving errors.Is/errors.As behavior and debug details while keeping the existing validation subtype and parameter.Source: Coding guidelines
internal/skillscheck/sync.go (1)
121-134: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueStrict index rejection makes one bad entry fail the whole index.
Any single entry that is not a complete
archiverecord abortsParseOfficialSkillsIndexJSON. Both brand sources publish the same index shape, so a malformed or newly introduced entry type fails both sources. For separate layout,fallbackSeparateabsorbs this with a warning. For suite layout,SyncSkillsreturns a hard failure, and users cannot update skills until the index is fixed.If the index is expected to gain new entry types over time, consider skipping unknown
typevalues while still rejecting malformedarchiveentries. If strict rejection is intentional for supply-chain reasons, keep it and add an index-shape contract check to the publish pipeline so a bad index cannot ship.🤖 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 `@internal/skillscheck/sync.go` around lines 121 - 134, The validation loop in ParseOfficialSkillsIndexJSON currently rejects the entire index for any non-archive entry. Update the handling around skill.Type to skip unknown entry types with an appropriate warning while continuing to strictly validate archive entries, including URL, digest, name, and duplicate checks; preserve hard failures for malformed archive records.internal/skillscheck/sync_test.go (1)
429-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the
runner.removals[0]index.If
syncSuitestops callingRemoveGlobalSkills,removalsis empty and line 432 panics with an index-out-of-range error. A panic hides which assertion failed.💚 Proposed fix
+ if len(runner.removals) == 0 { + t.Fatal("RemoveGlobalSkills was not called, want the separate skills removed") + } assertStrings(t, runner.removals[0], []string{"lark-calendar"})🤖 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 `@internal/skillscheck/sync_test.go` around lines 429 - 437, Guard the removals assertion in the syncSuite test before accessing runner.removals[0]: first assert that runner.removals contains an entry, then validate its value with assertStrings. Keep the existing expected removal assertion while ensuring an empty removals slice reports a test failure 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/update/update_test.go`:
- Around line 1163-1183: Add a reverse-layout test alongside
TestRunSkillsAndState_RequestedLayoutBypassesVersionDedup, seeding SkillsState
with LayoutSuite and requesting "separate". Stub syncSkills to assert
opts.Layout is LayoutSeparate, then verify runSkillsAndState succeeds and
invokes the sync, covering the suite-to-separate transition.
In `@cmd/update/update.go`:
- Around line 384-385: Change runSkillsAndState to accept a parsed
skillscheck.Layout instead of requestedLayout string, and remove its internal
ParseLayout call and discarded error. In updateRun, retain the validated layout
from ParseLayout and pass it to all three runSkillsAndState call sites; update
test callers to use skillscheck.Layout values, including skillscheck.LayoutSuite
where appropriate.
- Around line 334-337: Update the failure path around reportSkillsFailure in the
auto-update flow so a skills-sync error still reports the successful binary
transition from the previous version to latest, including the changelog URL
where applicable. Extend reportError to accept and emit these transition fields
in JSON mode, and ensure the resulting exit/report distinguishes “CLI updated,
skills failed” from a completely unsuccessful update without hiding the version
change.
In `@internal/selfupdate/updater_test.go`:
- Around line 185-191: Extend the updater tests around the “stage suite” case to
directly verify runSkillsCommandInDir’s working-directory plumbing. Use a
temporary directory or SkillsCommandInDirOverride, invoke StageSuite with that
directory, and assert the observed directory value so reverting cmd.Dir
assignment or the override propagation causes the test to fail.
In `@internal/skillscheck/layout.go`:
- Around line 142-155: Add coverage for the shipped suite file used by
cropSuiteRoutes, reading isolated-skills/lark-suite/SKILL.md and asserting
descriptionPrefix appears with the "等）。" suffix after it; retain the existing
fixture-based test. Also update suiteKeywords so each retained route lacking a
（...） keyword group returns an error instead of being silently skipped, and
propagate that error through cropSuiteRoutes.
In `@internal/skillscheck/sync_test.go`:
- Around line 315-327: Extend TestSyncSkillsSeparateUsesGitHubLast to inspect
the persisted state produced by finishSync, asserting the expected state.Layout
and state.OfficialSkills values and the expected result.Action. Keep the
existing warning and InstallAllSkills assertions, ensuring the test fails if
fallbackSeparate writes an empty-plan state.
In `@internal/skillscheck/sync.go`:
- Around line 296-303: Update localOfficialSkills and its SyncSkills call path
so an uninspectable lark-suite is treated as having no known local official
skills: handle empty skill.Path or failures from listDirectSubdirs for the suite
references directory without returning a sync failure, allowing source
processing and the normal install path to rebuild it. Preserve existing behavior
for successfully inspected suites; alternatively, ensure opts.Force bypasses
this inspection.
- Around line 447-473: Update internal/skillscheck/sync.go lines 447-473 in
fallbackSeparate: preserve the skills covered by InstallAllSkills in the
SyncPlan, or mark the persisted state unknown so it does not report a clean
sync; also emit the legacy fallback warning only when installResult is non-nil.
Update internal/skillscheck/sync_test.go lines 315-327 in
TestSyncSkillsSeparateUsesGitHubLast to assert result.Action, persisted
state.Layout, and state.OfficialSkills, ensuring the empty-plan regression
fails.
---
Nitpick comments:
In `@cmd/update/update_test.go`:
- Around line 100-102: Remove the unreachable “-y skills add
https://open.feishu.cn --list” case from successfulSkillsCommand, along with its
fixture output. Confirm the remaining tests obtain the official index through
FetchSkillsIndex and successfulSkillsIndexFetch, and preserve all other command
handling unchanged.
- Around line 1185-1195: Isolate configuration state in both error-path tests
and fully validate typed error metadata. In cmd/update/update_test.go lines
1185-1195, add LARKSUITE_CLI_CONFIG_DIR isolation as the first statement of
TestUpdateRejectsInvalidSkillsLayout and extend its errs.ProblemOf assertion to
check problem.Category in addition to Subtype and Param; in
internal/skillscheck/sync_test.go lines 466-471, add the same environment
isolation as the first statement of TestSyncSkillsNilRunnerFails.
In `@cmd/update/update.go`:
- Line 488: Update the layout field assembled by skillsSummary so empty r.Layout
values are omitted from the failure-summary JSON, while preserving the existing
layout output when non-empty. Use the surrounding summary construction and its
JSON serialization behavior rather than changing SyncResult creation paths.
- Around line 130-133: The validation error returned by the
skillscheck.ParseLayout call currently discards the parser cause. Update the
errs.NewValidationError chain in the surrounding validation flow to attach the
original err with .WithCause(err), preserving errors.Is/errors.As behavior and
debug details while keeping the existing validation subtype and parameter.
In `@internal/selfupdate/updater_test.go`:
- Around line 243-260: Rename all stale ListOfficialSkillsIndex references in
the affected tests to FetchSkillsIndex, including each
TestListOfficialSkillsIndex* function name and every corresponding t.Fatalf
message. Keep the test behavior and assertions unchanged.
In `@internal/selfupdate/updater.go`:
- Around line 281-284: Update FetchSkillsIndex and the SkillsIndexFetchOverride
function type so the source argument is passed through to the override, enabling
source-specific results during retries. Adjust all override definitions and call
sites in updater_test.go and update_test.go to accept and use the new source
parameter, while preserving existing behavior for normal fetches.
- Around line 353-358: Update Updater.RemoveGlobalSkills to return without
invoking runSkillsCommand when names is empty, before constructing or executing
the destructive removal arguments. Preserve the existing command construction
and execution for non-empty names.
In `@internal/skillscheck/state_test.go`:
- Around line 65-72: Add a nil-state assertion to
TestEffectiveLayoutDefaultsLegacyStateToSeparate, verifying that
EffectiveLayout(nil) returns LayoutSeparate. Keep the existing legacy-version
and explicit-suite assertions unchanged.
In `@internal/skillscheck/sync_test.go`:
- Around line 429-437: Guard the removals assertion in the syncSuite test before
accessing runner.removals[0]: first assert that runner.removals contains an
entry, then validate its value with assertStrings. Keep the existing expected
removal assertion while ensuring an empty removals slice reports a test failure
instead of panicking.
In `@internal/skillscheck/sync.go`:
- Around line 121-134: The validation loop in ParseOfficialSkillsIndexJSON
currently rejects the entire index for any non-archive entry. Update the
handling around skill.Type to skip unknown entry types with an appropriate
warning while continuing to strictly validate archive entries, including URL,
digest, name, and duplicate checks; preserve hard failures for malformed archive
records.
🪄 Autofix
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 Plus
Run ID: 62a449c4-9371-4cdd-851a-f29057a6f2a3
📒 Files selected for processing (12)
cmd/update/update.gocmd/update/update_test.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.gointernal/skillscheck/layout.gointernal/skillscheck/state.gointernal/skillscheck/state_test.gointernal/skillscheck/sync.gointernal/skillscheck/sync_test.goisolated-skills/lark-suite/SKILL.mdlark_suite_assets_test.goskill-template/lark-suite-business-info.json
| func TestRunSkillsAndState_RequestedLayoutBypassesVersionDedup(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | ||
| if err := skillscheck.WriteState(skillscheck.SkillsState{Version: "1.0.21", Layout: skillscheck.LayoutSeparate}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| originalSync := syncSkills | ||
| defer func() { syncSkills = originalSync }() | ||
| called := false | ||
| syncSkills = func(opts skillscheck.SyncOptions) *skillscheck.SyncResult { | ||
| called = true | ||
| if opts.Layout != skillscheck.LayoutSuite { | ||
| t.Fatalf("layout = %q, want suite", opts.Layout) | ||
| } | ||
| return &skillscheck.SyncResult{Action: "synced", Layout: skillscheck.LayoutSuite} | ||
| } | ||
|
|
||
| got := runSkillsAndState(&selfupdate.Updater{}, newTestIO(), "1.0.21", false, "suite") | ||
| if !called || got == nil || got.Err != nil { | ||
| t.Fatalf("runSkillsAndState() = %+v, called = %v", got, called) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the suite-to-separate switch case.
TestRunSkillsAndState_RequestedLayoutBypassesVersionDedup covers a recorded separate state with a requested suite layout. The reverse is not covered anywhere in this file or in internal/skillscheck/sync_test.go: a recorded suite state with --skills-layout separate. That path drives syncLayout to call RemoveGlobalSkills([]string{"lark-suite"}), which is one of the stated behaviors of this PR ("Removes stale official skills when switching layouts").
Mirror the existing test with Layout: skillscheck.LayoutSuite in the seeded state and "separate" as the requested layout, and assert opts.Layout == skillscheck.LayoutSeparate.
As per coding guidelines: "Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
🤖 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_test.go` around lines 1163 - 1183, Add a reverse-layout
test alongside TestRunSkillsAndState_RequestedLayoutBypassesVersionDedup,
seeding SkillsState with LayoutSuite and requesting "separate". Stub syncSkills
to assert opts.Layout is LayoutSeparate, then verify runSkillsAndState succeeds
and invokes the sync, covering the suite-to-separate transition.
Source: Coding guidelines
| func runSkillsAndState(updater *selfupdate.Updater, io *cmdutil.IOStreams, stateVersion string, force bool, requestedLayout string) *skillscheck.SyncResult { | ||
| layout, _ := skillscheck.ParseLayout(requestedLayout) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
runSkillsAndState silently coerces an invalid layout to "auto".
skillscheck.ParseLayout(requestedLayout) discards its error. An unsupported value such as "hybrid" yields layout == "", which ResolveLayout then treats as "use the layout recorded in state". The requested behavior is not honored and nothing reports it.
updateRun validates the flag before any call site reaches this function, so the CLI cannot hit this today. The function still accepts the raw string, and stateVersion and requestedLayout are adjacent same-typed parameters that can be swapped without a compile error.
Pass the already-parsed skillscheck.Layout from updateRun instead of re-parsing a string. That removes the duplicate parse, removes the discarded error, and makes the swap impossible.
♻️ Proposed change
-func runSkillsAndState(updater *selfupdate.Updater, io *cmdutil.IOStreams, stateVersion string, force bool, requestedLayout string) *skillscheck.SyncResult {
- layout, _ := skillscheck.ParseLayout(requestedLayout)
+func runSkillsAndState(updater *selfupdate.Updater, io *cmdutil.IOStreams, stateVersion string, force bool, layout skillscheck.Layout) *skillscheck.SyncResult {
if !force {updateRun then keeps the parsed value and passes it to all three call sites:
layout, err := skillscheck.ParseLayout(opts.SkillsLayout)
if err != nil { /* existing typed validation error */ }
// ... later: runSkillsAndState(updater, io, cur, opts.Force, layout)The test call sites in cmd/update/update_test.go change from "" to skillscheck.Layout("") and from "suite" to skillscheck.LayoutSuite.
As per coding guidelines: "never silently coerce unsupported inputs, ignore unhonored options, default missing identities, or discard writes" and "prefer distinct types when same-typed values could be silently swapped."
🤖 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 384 - 385, Change runSkillsAndState to
accept a parsed skillscheck.Layout instead of requestedLayout string, and remove
its internal ParseLayout call and discarded error. In updateRun, retain the
validated layout from ParseLayout and pass it to all three runSkillsAndState
call sites; update test callers to use skillscheck.Layout values, including
skillscheck.LayoutSuite where appropriate.
Source: Coding guidelines
| { | ||
| name: "list official primary", | ||
| name: "stage suite", | ||
| run: func(u *Updater) *NpmResult { | ||
| return u.runSkillsListOfficial("https://open.feishu.cn") | ||
| return u.StageSuite("https://open.feishu.cn/lark-cli", ".") | ||
| }, | ||
| want: "-y skills add https://open.feishu.cn --list", | ||
| want: "-y skills add https://open.feishu.cn/lark-cli/isolated-skills -s lark-suite -y", | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add coverage for the working directory of runSkillsCommandInDir.
The "stage suite" case asserts only the argument list. It passes ".", which is indistinguishable from the previous behavior with an empty cmd.Dir. The new dir plumbing (cmd.Dir = dir and SkillsCommandInDirOverride) is therefore not verified. Reverting cmd.Dir = dir to the old code would not fail any test.
Add a case that stages into a t.TempDir() and asserts the script observed that directory, or a case that sets SkillsCommandInDirOverride and asserts the received dir.
As per coding guidelines: "Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
🤖 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 `@internal/selfupdate/updater_test.go` around lines 185 - 191, Extend the
updater tests around the “stage suite” case to directly verify
runSkillsCommandInDir’s working-directory plumbing. Use a temporary directory or
SkillsCommandInDirOverride, invoke StageSuite with that directory, and assert
the observed directory value so reverting cmd.Dir assignment or the override
propagation causes the test to fail.
Source: Coding guidelines
| keywords := suiteKeywords(content) | ||
| const descriptionPrefix = "description: 飞书/Lark 聚合能力入口：管理飞书/Lark 产品能力（" | ||
| start := strings.Index(content, descriptionPrefix) | ||
| if start < 0 { | ||
| return "", fmt.Errorf("suite description prefix is missing") | ||
| } | ||
| valueStart := start + len(descriptionPrefix) | ||
| valueEndOffset := strings.Index(content[valueStart:], "等）。") | ||
| if valueEndOffset < 0 { | ||
| return "", fmt.Errorf("suite description keyword suffix is missing") | ||
| } | ||
| valueEnd := valueStart + valueEndOffset | ||
| content = content[:valueStart] + strings.Join(keywords, "、") + content[valueEnd:] | ||
| return content, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The description rewrite is coupled to exact wording in the published SKILL.md, and nothing tests that coupling.
descriptionPrefix and the "等）。" suffix must both appear verbatim in the suite SKILL.md that the archive delivers. If either string changes in isolated-skills/lark-suite/SKILL.md or in the publish pipeline, cropSuiteRoutes returns "suite description prefix is missing". Every source then fails, and SyncSkills reports a hard failure for suite layout because the GitHub fallback is restricted to separate layout.
The existing coverage does not protect this. TestPrepareSuiteCropsRoutesKeywordsAndReferences uses the local suiteFixture constant, not the shipped isolated-skills/lark-suite/SKILL.md, and lark_suite_assets_test.go only checks path lengths. A wording change in the shipped file passes CI and breaks suite sync at runtime.
Add a test that reads isolated-skills/lark-suite/SKILL.md and asserts it contains descriptionPrefix and the "等）。" suffix after it.
Related: suiteKeywords skips any route line without a （...） group without reporting it, so a reformatted route line silently shortens the description instead of failing. Consider returning an error when a kept route contributes no keyword.
#!/bin/bash
# Description: Check that the Go description constants match the shipped suite SKILL.md and locate the publish-time templating.
set -uo pipefail
echo "== Go constants =="
rg -n 'descriptionPrefix|等）。|LARK_SUITE_KEYS|LARK_SUITE_ROUTES' --type=go
echo "== Shipped SKILL.md description line =="
fd -t f 'SKILL.md' isolated-skills --exec rg -n 'description:' {}
echo "== Publish/templating references to the placeholders =="
rg -n 'LARK_SUITE_KEYS|LARK_SUITE_ROUTES|lark-suite-business-info' -g '!**/*.md'
echo "== Route line format in shipped/template assets =="
fd -t f 'SKILL.md' isolated-skills --exec rg -n 'lark-suite-route' {}🤖 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 `@internal/skillscheck/layout.go` around lines 142 - 155, Add coverage for the
shipped suite file used by cropSuiteRoutes, reading
isolated-skills/lark-suite/SKILL.md and asserting descriptionPrefix appears with
the "等）。" suffix after it; retain the existing fixture-based test. Also update
suiteKeywords so each retained route lacking a （...） keyword group returns an
error instead of being silently skipped, and propagate that error through
cropSuiteRoutes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/skillscheck/sync_test.go`:
- Around line 508-513: Update the SyncSkills test to assert that
runner.localSuite is populated after a successful sync, alongside the existing
Updated and stages assertions. This must directly verify local installation so
the test fails if the staged suite is not installed.
🪄 Autofix
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 Plus
Run ID: 2cfdb5e6-67b1-4dd4-a479-f45e1c395c4d
📒 Files selected for processing (5)
cmd/update/update.gocmd/update/update_test.gointernal/skillscheck/state.gointernal/skillscheck/sync.gointernal/skillscheck/sync_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/update/update_test.go
- internal/skillscheck/sync.go
1 participant
Summary
Add Agent Skills v0.2 distribution support and allow users to choose between separate official skills and a consolidated lark-suite installation.
Changes
Test Plan
Related Issues
Summary by CodeRabbit
New Features
lark-suitecapability router and multi-source installation with automatic fallback.Bug Fixes