fix: support project-scoped skills sync#1058
Open
hehanlin1996 wants to merge 1 commit into
Open
Conversation
|
hehanlin.lq seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
3 tasks
📝 WalkthroughWalkthroughThis PR extends ChangesProject-Scoped Skills Synchronization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes introduce a new feature flag ( Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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
![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
🧹 Nitpick comments (1)
scripts/install-wizard.test.js (1)
14-37: ⚡ Quick winAdd direct tests for
parseProjectArgto lock CLI flag parsing.The suite covers scope formatting well, but it doesn’t test the argv parser that actually drives
main()behavior. Add present/absent--projectcases to avoid regressions.✅ Suggested test addition
const { + parseProjectArg, skillsAddArgs, skillsAddCommand, skillsScopeFlag, } = require("./install-wizard.js"); describe("install wizard skills scope", () => { + it("parses --project from argv", () => { + const oldArgv = process.argv; + process.argv = ["node", "install-wizard.js", "--project"]; + assert.equal(parseProjectArg(), true); + process.argv = ["node", "install-wizard.js"]; + assert.equal(parseProjectArg(), false); + process.argv = oldArgv; + }); + it("keeps global skills install by default", () => {🤖 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 `@scripts/install-wizard.test.js` around lines 14 - 37, Add direct unit tests for the argv parser by exercising parseProjectArg with both present and absent --project flag to lock CLI flag parsing; specifically, add tests that call parseProjectArg(["--project"]) (or equivalent invocation used in your CLI parsing functions) and assert it returns true, and call parseProjectArg([]) (no flag) and assert it returns false, so regressions in main()/CLI parsing are caught—look for and update the test suite near existing scope tests that reference skillsScopeFlag, skillsAddArgs and skillsAddCommand to include these parseProjectArg present/absent cases.
🤖 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 964-989: The test currently seeds the same stamp value ("1.0.21")
that runSkillsAndStamp is invoked with, so it cannot detect accidental writes;
change the initial seed written by skillscheck.WriteStamp to a different version
(e.g., "1.0.20") while keeping runSkillsAndStamp invoked with "1.0.21", then
after calling runSkillsAndStamp verify via skillscheck.ReadStamp that the global
stamp still equals the original seeded value (the new "1.0.20"), referencing the
test helpers and overrides: skillscheck.WriteStamp, runSkillsAndStamp,
selfupdate.Updater.SkillsUpdateWithScopeOverride, and skillscheck.ReadStamp.
---
Nitpick comments:
In `@scripts/install-wizard.test.js`:
- Around line 14-37: Add direct unit tests for the argv parser by exercising
parseProjectArg with both present and absent --project flag to lock CLI flag
parsing; specifically, add tests that call parseProjectArg(["--project"]) (or
equivalent invocation used in your CLI parsing functions) and assert it returns
true, and call parseProjectArg([]) (no flag) and assert it returns false, so
regressions in main()/CLI parsing are caught—look for and update the test suite
near existing scope tests that reference skillsScopeFlag, skillsAddArgs and
skillsAddCommand to include these parseProjectArg present/absent cases.
🪄 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: e7abb9bc-464a-44b9-8173-9b236828066f
📒 Files selected for processing (6)
cmd/update/update.gocmd/update/update_test.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.goscripts/install-wizard.jsscripts/install-wizard.test.js
Comment on lines
+964
to
+989
| if err := skillscheck.WriteStamp("1.0.21"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| called := false | ||
| projectScope := false | ||
| updater := &selfupdate.Updater{ | ||
| SkillsUpdateWithScopeOverride: func(project bool) *selfupdate.NpmResult { | ||
| called = true | ||
| projectScope = project | ||
| return &selfupdate.NpmResult{} | ||
| }, | ||
| } | ||
| got := runSkillsAndStamp(updater, newTestIO(), "1.0.21", false, true) | ||
| if got == nil || got.Err != nil { | ||
| t.Fatalf("runSkillsAndStamp(project=true) = %+v, want non-nil success", got) | ||
| } | ||
| if !called { | ||
| t.Fatal("RunSkillsUpdateWithScope not called for project scope") | ||
| } | ||
| if !projectScope { | ||
| t.Fatal("RunSkillsUpdateWithScope called with project=false, want true") | ||
| } | ||
| stamp, _ := skillscheck.ReadStamp() | ||
| if stamp != "1.0.21" { | ||
| t.Errorf("project scope must not mutate global stamp, got %q", stamp) | ||
| } |
There was a problem hiding this comment.
Project-scope stamp assertion is currently non-detecting
This test seeds and verifies the same stamp value (1.0.21), so it still passes if project-scope accidentally writes the global stamp. Seed with a different version and assert it stays unchanged.
Suggested test tightening
- if err := skillscheck.WriteStamp("1.0.21"); err != nil {
+ if err := skillscheck.WriteStamp("1.0.20"); err != nil {
t.Fatal(err)
}
@@
- got := runSkillsAndStamp(updater, newTestIO(), "1.0.21", false, true)
+ got := runSkillsAndStamp(updater, newTestIO(), "1.0.21", false, true)
@@
- if stamp != "1.0.21" {
+ if stamp != "1.0.20" {
t.Errorf("project scope must not mutate global stamp, got %q", stamp)
}📝 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
| if err := skillscheck.WriteStamp("1.0.21"); err != nil { | |
| t.Fatal(err) | |
| } | |
| called := false | |
| projectScope := false | |
| updater := &selfupdate.Updater{ | |
| SkillsUpdateWithScopeOverride: func(project bool) *selfupdate.NpmResult { | |
| called = true | |
| projectScope = project | |
| return &selfupdate.NpmResult{} | |
| }, | |
| } | |
| got := runSkillsAndStamp(updater, newTestIO(), "1.0.21", false, true) | |
| if got == nil || got.Err != nil { | |
| t.Fatalf("runSkillsAndStamp(project=true) = %+v, want non-nil success", got) | |
| } | |
| if !called { | |
| t.Fatal("RunSkillsUpdateWithScope not called for project scope") | |
| } | |
| if !projectScope { | |
| t.Fatal("RunSkillsUpdateWithScope called with project=false, want true") | |
| } | |
| stamp, _ := skillscheck.ReadStamp() | |
| if stamp != "1.0.21" { | |
| t.Errorf("project scope must not mutate global stamp, got %q", stamp) | |
| } | |
| if err := skillscheck.WriteStamp("1.0.20"); err != nil { | |
| t.Fatal(err) | |
| } | |
| called := false | |
| projectScope := false | |
| updater := &selfupdate.Updater{ | |
| SkillsUpdateWithScopeOverride: func(project bool) *selfupdate.NpmResult { | |
| called = true | |
| projectScope = project | |
| return &selfupdate.NpmResult{} | |
| }, | |
| } | |
| got := runSkillsAndStamp(updater, newTestIO(), "1.0.21", false, true) | |
| if got == nil || got.Err != nil { | |
| t.Fatalf("runSkillsAndStamp(project=true) = %+v, want non-nil success", got) | |
| } | |
| if !called { | |
| t.Fatal("RunSkillsUpdateWithScope not called for project scope") | |
| } | |
| if !projectScope { | |
| t.Fatal("RunSkillsUpdateWithScope called with project=false, want true") | |
| } | |
| stamp, _ := skillscheck.ReadStamp() | |
| if stamp != "1.0.20" { | |
| t.Errorf("project scope must not mutate global stamp, got %q", stamp) | |
| } |
🤖 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 964 - 989, The test currently seeds
the same stamp value ("1.0.21") that runSkillsAndStamp is invoked with, so it
cannot detect accidental writes; change the initial seed written by
skillscheck.WriteStamp to a different version (e.g., "1.0.20") while keeping
runSkillsAndStamp invoked with "1.0.21", then after calling runSkillsAndStamp
verify via skillscheck.ReadStamp that the global stamp still equals the original
seeded value (the new "1.0.20"), referencing the test helpers and overrides:
skillscheck.WriteStamp, runSkillsAndStamp,
selfupdate.Updater.SkillsUpdateWithScopeOverride, and skillscheck.ReadStamp.
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
Adds a project-scoped skills sync option for the update/install flows so users can keep lark-* skills out of the global agent skill namespace when desired. Default behavior remains global for compatibility.
Changes
lark-cli update --projectand plumb project scope through the self-update skills sync path.npx skills add ... -pwhen project scope is requested, while preserving-gby default.--projectscope to the npm install wizard and retry command text.Test Plan
go test ./internal/selfupdate ./cmd/updatenode --test scripts/install-wizard.test.jsnode --check scripts/install-wizard.jsRelated Issues
Summary by CodeRabbit
--projectflag to thelark-cli updatecommand, allowing AI skills to be installed at project scope instead of globally.