feat: add skills version drift notice and unify update flow#723
Open
niuchong0523 wants to merge 1 commit intolarksuite:mainfrom
Open
feat: add skills version drift notice and unify update flow#723niuchong0523 wants to merge 1 commit intolarksuite:mainfrom
niuchong0523 wants to merge 1 commit intolarksuite:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #723 +/- ##
==========================================
+ Coverage 64.06% 64.25% +0.18%
==========================================
Files 503 508 +5
Lines 44091 44414 +323
==========================================
+ Hits 28249 28538 +289
- Misses 13382 13399 +17
- Partials 2460 2477 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5c4e808b6dcc5a443cfb7c94cbcff02b832d5b1e🧩 Skill updatenpx skills add niuchong0523/cli#feat/skills-autosync -y -g |
niuchong0523
force-pushed
the
feat/skills-autosync
branch
from
987a1db to
fca0cb1
Compare
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds a skills-staleness notifier and integrates it with existing update-notice wiring: new internal Changes
Sequence DiagramsequenceDiagram
participant Root as Root Command
participant Skills as SkillsCheck
participant Stamp as Stamp Storage
participant Output as Output Notifier
Root->>Skills: Init(binary_version)
activate Skills
Skills->>Skills: shouldSkip(version)?
alt skip
Skills-->>Root: no-op
else proceed
Skills->>Stamp: ReadStamp()
Stamp-->>Skills: stored_version (or "")
alt versions match
Skills-->>Root: no pending notice
else mismatch
Skills->>Skills: SetPending(StaleNotice{Current,Target})
Skills-->>Root: pending skills notice
end
end
deactivate Skills
Root->>Output: PendingNotice()
activate Output
Output->>Skills: GetPending()
Skills-->>Output: *StaleNotice or nil
alt skills present
Output-->>Root: emit `_notice.skills` JSON (current,target,message)
else absent
Output-->>Root: omit skills key
end
deactivate Output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/update/update.go (1)
321-343: Consider usingio.ErrOutinstead ofos.Stderrfor consistency.The warning on line 339 uses
os.Stderrdirectly, which bypasses theIOStreamsabstraction used elsewhere in this file. While this is acceptable for a best-effort warning (and the function doesn't receiveIOStreams), it creates a minor inconsistency.If you want to maintain consistency, consider passing
IOStreamsto this function or accepting this as an intentional deviation for a low-criticality warning path.♻️ Optional: Pass IOStreams for consistency
-func runSkillsAndStamp(updater *selfupdate.Updater, stampVersion string, force bool) *selfupdate.NpmResult { +func runSkillsAndStamp(updater *selfupdate.Updater, stampVersion string, force bool, errOut io.Writer) *selfupdate.NpmResult { if !force { if existing, _ := skillscheck.ReadStamp(); existing == stampVersion { return nil } } r := updater.RunSkillsUpdate() if r.Err == nil { if err := skillscheck.WriteStamp(stampVersion); err != nil { - fmt.Fprintf(os.Stderr, "warning: skills synced but stamp not written: %v\n", err) + fmt.Fprintf(errOut, "warning: skills synced but stamp not written: %v\n", err) } } return r }Then update call sites to pass
io.ErrOut.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/update/update.go` around lines 321 - 343, The function runSkillsAndStamp writes a warning directly to os.Stderr (fmt.Fprintf(os.Stderr, ...)) which bypasses the project's IOStreams abstraction; change the function to accept an io.ErrOut-style writer (e.g., an io.Writer or the existing IOStreams type) and use that writer when emitting the warning (pass the writer into runSkillsAndStamp call sites), so replace the direct os.Stderr usage with fmt.Fprintf(errWriter, ...) and ensure callers supply the appropriate IOStreams.ErrOut or io.ErrOut equivalent; keep behavior otherwise unchanged and preserve the best-effort non-failing semantics around skillscheck.WriteStamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root_integration_test.go`:
- Around line 498-611: Add a new test case exercising the combined update+skills
envelope: seed a non-nil pending update via update.SetPending(...) along with
the existing skills state (use skillscheck.WriteStamp(...) or clear as
appropriate), call setupNotices(), then call output.GetNotice() and assert the
returned map contains both "skills" and "update" keys with their expected
subfields/messages; reference existing tests
TestSetupNotices_ColdStart/TestSetupNotices_Drift for setup pattern and use
setupNotices, update.SetPending, skillscheck.SetPending/WriteStamp, and
output.GetNotice to locate the code to change.
In `@docs/superpowers/harness-state.md`:
- Line 13: The link in the markdown line "Spec 已完成并 approved
[spec](docs/superpowers/specs/2026-04-29-skills-startup-autosync-design.md)"
uses a duplicated path fragment; update the href to a path relative to the
current file (e.g. change
"docs/superpowers/specs/2026-04-29-skills-startup-autosync-design.md" to
"./specs/2026-04-29-skills-startup-autosync-design.md" or
"specs/2026-04-29-skills-startup-autosync-design.md") so the [spec](...) link in
harness-state.md resolves correctly.
In `@internal/skillscheck/check.go`:
- Around line 19-33: Init can return early and leave a previous global pending
notice set; to avoid leaking stale notices call SetPending(nil) at the start of
Init (before the shouldSkip(currentVersion) check) so any prior StaleNotice is
cleared on each run. Ensure the nil clear happens before invoking shouldSkip,
ReadStamp or creating a new StaleNotice so all early-return paths leave no
pending state.
---
Nitpick comments:
In `@cmd/update/update.go`:
- Around line 321-343: The function runSkillsAndStamp writes a warning directly
to os.Stderr (fmt.Fprintf(os.Stderr, ...)) which bypasses the project's
IOStreams abstraction; change the function to accept an io.ErrOut-style writer
(e.g., an io.Writer or the existing IOStreams type) and use that writer when
emitting the warning (pass the writer into runSkillsAndStamp call sites), so
replace the direct os.Stderr usage with fmt.Fprintf(errWriter, ...) and ensure
callers supply the appropriate IOStreams.ErrOut or io.ErrOut equivalent; keep
behavior otherwise unchanged and preserve the best-effort non-failing semantics
around skillscheck.WriteStamp.
🪄 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: 51cc7408-79c0-4283-9214-33e28e7c0bb1
📒 Files selected for processing (20)
AGENTS.mdcmd/root.gocmd/root_integration_test.gocmd/update/update.gocmd/update/update_test.godocs/superpowers/harness-state.jsondocs/superpowers/harness-state.mddocs/superpowers/plans/2026-04-29-skills-startup-autosync.mddocs/superpowers/specs/2026-04-29-skills-startup-autosync-design.mdinternal/binding/types.gointernal/skillscheck/check.gointernal/skillscheck/check_test.gointernal/skillscheck/notice.gointernal/skillscheck/notice_test.gointernal/skillscheck/skip.gointernal/skillscheck/skip_test.gointernal/skillscheck/stamp.gointernal/skillscheck/stamp_test.gointernal/update/update.gointernal/update/update_test.go
| - **意外**: 无 | ||
| - **摩擦**: 无 | ||
| ### ✅ Phase 2: Design | ||
| Spec 已完成并 approved [spec](docs/superpowers/specs/2026-04-29-skills-startup-autosync-design.md) |
There was a problem hiding this comment.
Fix the spec link path.
From docs/superpowers/harness-state.md, this relative link resolves with docs/superpowers/ duplicated, so it will break in the repo view. Use a path relative to the current file instead.
Suggested fix
-Spec 已完成并 approved [spec](docs/superpowers/specs/2026-04-29-skills-startup-autosync-design.md)
+Spec 已完成并 approved [spec](./specs/2026-04-29-skills-startup-autosync-design.md)📝 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
| Spec 已完成并 approved [spec](docs/superpowers/specs/2026-04-29-skills-startup-autosync-design.md) | |
| Spec 已完成并 approved [spec](./specs/2026-04-29-skills-startup-autosync-design.md) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/harness-state.md` at line 13, The link in the markdown line
"Spec 已完成并 approved
[spec](docs/superpowers/specs/2026-04-29-skills-startup-autosync-design.md)"
uses a duplicated path fragment; update the href to a path relative to the
current file (e.g. change
"docs/superpowers/specs/2026-04-29-skills-startup-autosync-design.md" to
"./specs/2026-04-29-skills-startup-autosync-design.md" or
"specs/2026-04-29-skills-startup-autosync-design.md") so the [spec](...) link in
harness-state.md resolves correctly.
fca0cb1 to
cf30b9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/root_integration_test.go (1)
498-611:⚠️ Potential issue | 🟡 MinorAdd one test for the combined
update+skills_noticeenvelope.Current cases validate skills-only states, but
setupNotices()now composes two independent notice providers. Please add a case that seeds both pending states and assertsoutput.GetNotice()contains bothupdateandskillskeys together.As per coding guidelines, "Every behavior change must have a test alongside the change".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root_integration_test.go` around lines 498 - 611, Add a new integration test that verifies the composed notice contains both "update" and "skills" keys when both providers have pending state: create a test (e.g. TestSetupNotices_UpdateAndSkills) that calls clearNoticeEnv(), sets LARKSUITE_CLI_CONFIG_DIR to a temp dir, sets build.Version to "1.0.21" (with cleanup), seed both providers via skillscheck.SetPending(map[string]interface{}{"current":"1.0.20","target":"1.0.21","message":"..."} ) and update.SetPending(map[string]interface{}{"current":"1.0.20","target":"1.0.21","message":"..."} ), ensure output.PendingNotice=nil and cleanup, call setupNotices(), call notice := output.GetNotice(), and assert notice is non-nil and contains both notice["skills"] and notice["update"] maps with the expected fields and messages; use the existing patterns in TestSetupNotices_ColdStart/TestSetupNotices_Drift for assertions and cleanup.
🧹 Nitpick comments (1)
cmd/update/update_test.go (1)
900-900: AssertReadStamp()errors instead of discarding them.These tests ignore
ReadStamperrors; if fixture I/O breaks, assertions can fail noisily or misleadingly. Please fail fast on error before comparing stamp values.Also applies to: 923-923, 973-973, 1015-1015, 1061-1061, 1151-1151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/update/update_test.go` at line 900, Tests currently ignore the error returned by ReadStamp(); modify each occurrence (e.g., the calls that assign "stamp, _ := skillscheck.ReadStamp()") to capture the error and fail the test on error (for example, "stamp, err := skillscheck.ReadStamp(); if err != nil { t.Fatalf(\"ReadStamp failed: %v\", err) }" or use your test helper like require.NoError(t, err)). Apply this change for every similar call (the ones at the noted locations) so the test fails immediately if fixture I/O errors instead of proceeding to comparisons.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-29-skills-startup-autosync-design.md`:
- Around line 33-51: The fenced code block in the docs containing the lark-cli
call graph (showing "lark-cli <subcmd>", "cmd/root.go: Execute()",
"skillscheck.Init(build.Version)", and "cmd/update/update.go: updateRun()") is
missing a language identifier which triggers MD040; add a language label (e.g.,
```text) to the opening fence so the block becomes ```text ... ``` and commit
the change.
---
Duplicate comments:
In `@cmd/root_integration_test.go`:
- Around line 498-611: Add a new integration test that verifies the composed
notice contains both "update" and "skills" keys when both providers have pending
state: create a test (e.g. TestSetupNotices_UpdateAndSkills) that calls
clearNoticeEnv(), sets LARKSUITE_CLI_CONFIG_DIR to a temp dir, sets
build.Version to "1.0.21" (with cleanup), seed both providers via
skillscheck.SetPending(map[string]interface{}{"current":"1.0.20","target":"1.0.21","message":"..."}
) and
update.SetPending(map[string]interface{}{"current":"1.0.20","target":"1.0.21","message":"..."}
), ensure output.PendingNotice=nil and cleanup, call setupNotices(), call notice
:= output.GetNotice(), and assert notice is non-nil and contains both
notice["skills"] and notice["update"] maps with the expected fields and
messages; use the existing patterns in
TestSetupNotices_ColdStart/TestSetupNotices_Drift for assertions and cleanup.
---
Nitpick comments:
In `@cmd/update/update_test.go`:
- Line 900: Tests currently ignore the error returned by ReadStamp(); modify
each occurrence (e.g., the calls that assign "stamp, _ :=
skillscheck.ReadStamp()") to capture the error and fail the test on error (for
example, "stamp, err := skillscheck.ReadStamp(); if err != nil {
t.Fatalf(\"ReadStamp failed: %v\", err) }" or use your test helper like
require.NoError(t, err)). Apply this change for every similar call (the ones at
the noted locations) so the test fails immediately if fixture I/O errors instead
of proceeding to comparisons.
🪄 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: 3bc7a87e-abb8-4944-af46-e23b2053ca3c
📒 Files selected for processing (20)
AGENTS.mdcmd/root.gocmd/root_integration_test.gocmd/update/update.gocmd/update/update_test.godocs/superpowers/harness-state.jsondocs/superpowers/harness-state.mddocs/superpowers/plans/2026-04-29-skills-startup-autosync.mddocs/superpowers/specs/2026-04-29-skills-startup-autosync-design.mdinternal/binding/types.gointernal/skillscheck/check.gointernal/skillscheck/check_test.gointernal/skillscheck/notice.gointernal/skillscheck/notice_test.gointernal/skillscheck/skip.gointernal/skillscheck/skip_test.gointernal/skillscheck/stamp.gointernal/skillscheck/stamp_test.gointernal/update/update.gointernal/update/update_test.go
✅ Files skipped from review due to trivial changes (6)
- internal/binding/types.go
- internal/skillscheck/stamp_test.go
- docs/superpowers/harness-state.json
- docs/superpowers/harness-state.md
- internal/skillscheck/notice.go
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/skillscheck/notice_test.go
- internal/skillscheck/skip_test.go
- internal/skillscheck/check.go
- internal/skillscheck/stamp.go
- internal/skillscheck/check_test.go
- internal/update/update_test.go
- docs/superpowers/plans/2026-04-29-skills-startup-autosync.md
- cmd/update/update.go
Comment on lines
+33
to
+51
| ``` | ||
| lark-cli <subcmd> | ||
| └─ cmd/root.go: Execute() | ||
| ├─ setupNotices() | ||
| │ ├─ update.SetPending if cached (existing) | ||
| │ ├─ go update.RefreshCache (existing) | ||
| │ ├─ skillscheck.Init(build.Version) ← NEW: 同步本地戳比对 | ||
| │ └─ output.PendingNotice = composed{update, skills} | ||
| └─ rootCmd.Execute() | ||
|
|
||
| lark-cli update | ||
| └─ cmd/update/update.go: updateRun() | ||
| ├─ 二进制更新（npm / manual / 已最新） (existing 三分支) | ||
| ├─ runSkillsAndStamp(updater, version, force) ← 提到顶层 | ||
| │ ├─ if !force && stamp == version → no-op (dedup) | ||
| │ ├─ else updater.RunSkillsUpdate() | ||
| │ └─ on success: skillscheck.WriteStamp(version) | ||
| └─ report 含 skills_action 字段 | ||
| ``` |
There was a problem hiding this comment.
Specify a language for the fenced block (MD040).
markdownlint warns because the code fence has no language identifier.
Suggested fix
-```
+```text
lark-cli <subcmd>
└─ cmd/root.go: Execute()
├─ setupNotices()
│ ├─ update.SetPending if cached (existing)
│ ├─ go update.RefreshCache (existing)
│ ├─ skillscheck.Init(build.Version) ← NEW: 同步本地戳比对
│ └─ output.PendingNotice = composed{update, skills}
└─ rootCmd.Execute()
lark-cli update
└─ cmd/update/update.go: updateRun()
├─ 二进制更新（npm / manual / 已最新） (existing 三分支)
├─ runSkillsAndStamp(updater, version, force) ← 提到顶层
│ ├─ if !force && stamp == version → no-op (dedup)
│ ├─ else updater.RunSkillsUpdate()
│ └─ on success: skillscheck.WriteStamp(version)
└─ report 含 skills_action 字段</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/superpowers/specs/2026-04-29-skills-startup-autosync-design.md around
lines 33 - 51, The fenced code block in the docs containing the lark-cli call
graph (showing "lark-cli ", "cmd/root.go: Execute()",
"skillscheck.Init(build.Version)", and "cmd/update/update.go: updateRun()") is
missing a language identifier which triggers MD040; add a language label (e.g.,
text) to the opening fence so the block becomes text ... ``` and commit
the change.
</details>
<!-- fingerprinting:phantom:poseidon:hawk:9e385b5b-3b6e-401c-a4cd-e98fc9a23858 -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
cf30b9e to
4f1e0b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/update/update.go`:
- Around line 301-305: The code always prints the "Updating skills ..." heading
even when skillsResult is nil (meaning dedup skipped execution), causing a
dangling heading; modify the post-update block in update.go to only print the
progress heading and call emitSkillsTextHints(io, skillsResult) when
skillsResult != nil (i.e., wrap the fmt.Fprintf("\nUpdating skills ...\n") and
emitSkillsTextHints call in a nil-check for skillsResult), ensuring no orphaned
output is emitted when skills are already in sync.
- Line 139: Calls to write stamp/write warnings currently go directly to
os.Stderr (e.g., the call site invoking runSkillsAndStamp and any code paths
that print warnings around it); update these to use the command's IOStreams
error writer instead (use opts.IOStreams.ErrOut or the command's io.ErrOut) so
output capture is consistent. Locate usages in runSkillsAndStamp and related
stamp-write paths invoked from update.go (references: runSkillsAndStamp,
updater, skillsResult) and replace any os.Stderr writes with writes to the
provided IOStreams error writer, passing the writer into helper functions if
necessary.
🪄 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: 3c45b35b-a3e6-4a6c-902c-f4951d9736a5
📒 Files selected for processing (16)
AGENTS.mdcmd/root.gocmd/root_integration_test.gocmd/update/update.gocmd/update/update_test.gointernal/binding/types.gointernal/skillscheck/check.gointernal/skillscheck/check_test.gointernal/skillscheck/notice.gointernal/skillscheck/notice_test.gointernal/skillscheck/skip.gointernal/skillscheck/skip_test.gointernal/skillscheck/stamp.gointernal/skillscheck/stamp_test.gointernal/update/update.gointernal/update/update_test.go
✅ Files skipped from review due to trivial changes (4)
- internal/binding/types.go
- AGENTS.md
- internal/skillscheck/skip_test.go
- internal/skillscheck/stamp_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/skillscheck/skip.go
- internal/skillscheck/check.go
- internal/skillscheck/check_test.go
- internal/skillscheck/notice_test.go
- internal/update/update_test.go
- cmd/root.go
- cmd/root_integration_test.go
- cmd/update/update_test.go
niuchong0523
force-pushed
the
feat/skills-autosync
branch
from
4f1e0b1 to
728f54d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/update/update.go`:
- Around line 339-340: Add two unit tests for the update command: one that
simulates skillscheck.WriteStamp returning an error and asserts the warning
"warning: skills synced but stamp not written" is written to io.ErrOut, and one
that simulates a successful stamp write and asserts the successful skills sync
message is written to io.Out. In each test, temporarily replace/mock
skillscheck.WriteStamp (and restore it after), capture io.Out and io.ErrOut into
buffers, invoke the update command entrypoint used in tests (the function that
triggers the sync flow and calls skillscheck.WriteStamp, e.g., the update
command handler/run function), and assert the expected substrings are present in
the respective buffers; use stampVersion or the same invocation flags that
exercise the skills sync branch so the code paths that call
skillscheck.WriteStamp and emit the success/warning messages are exercised.
- Around line 333-334: Normalize version strings before comparing the stored
stamp and the current stampVersion to avoid mismatches caused by a leading "v";
update the comparison in the early-return check (where existing, _ :=
skillscheck.ReadStamp() and stampVersion are compared) to strip a leading "v"
(or otherwise canonicalize) from both values before equality testing, or perform
this normalization when stampVersion is initialized (e.g., in
internal/build/build.go) so that functions like ReadStamp and the variable
stampVersion use the same normalized format.
In `@internal/skillscheck/stamp.go`:
- Around line 45-46: Add a unit test that forces the MkdirAll failure path by
making the parent location unwritable and asserting WriteStamp returns the mkdir
error: create a test (e.g., internal/skillscheck/stamp_test.go) that makes a
temp directory, remove write permission on it, set core.GetBaseConfigDir() to
return a non-existent subpath under that unwritable dir, call WriteStamp and
assert it returns an error (permission/ mkdir failure) coming from vfs.MkdirAll;
ensure the test restores permissions/cleans up after itself. Reference the
WriteStamp function, core.GetBaseConfigDir(), and the vfs.MkdirAll failure
branch in your test.
🪄 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: 66cfc28d-8dbf-4364-be42-6d944be64d9b
📒 Files selected for processing (16)
AGENTS.mdcmd/root.gocmd/root_integration_test.gocmd/update/update.gocmd/update/update_test.gointernal/binding/types.gointernal/skillscheck/check.gointernal/skillscheck/check_test.gointernal/skillscheck/notice.gointernal/skillscheck/notice_test.gointernal/skillscheck/skip.gointernal/skillscheck/skip_test.gointernal/skillscheck/stamp.gointernal/skillscheck/stamp_test.gointernal/update/update.gointernal/update/update_test.go
✅ Files skipped from review due to trivial changes (7)
- internal/binding/types.go
- internal/skillscheck/skip.go
- internal/skillscheck/notice.go
- AGENTS.md
- internal/skillscheck/check_test.go
- cmd/root_integration_test.go
- internal/update/update_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/skillscheck/notice_test.go
- internal/skillscheck/skip_test.go
- internal/skillscheck/stamp_test.go
- internal/skillscheck/check.go
- cmd/update/update_test.go
Comment on lines
+339
to
+340
| if err := skillscheck.WriteStamp(stampVersion); err != nil { | ||
| fmt.Fprintf(io.ErrOut, "warning: skills synced but stamp not written: %v\n", err) |
There was a problem hiding this comment.
Please add tests for new skills warning/success text branches.
The new warning path (Line 340) and success text path (Line 412) are behavior changes but currently uncovered in patch coverage. Add targeted tests for:
- stamp write failure warning emission
- successful skills sync message emission.
Based on learnings: Every behavior change must include a corresponding test.
Also applies to: 411-412
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 340-340: cmd/update/update.go#L340
Added line #L340 was not covered by tests
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/update/update.go` around lines 339 - 340, Add two unit tests for the
update command: one that simulates skillscheck.WriteStamp returning an error and
asserts the warning "warning: skills synced but stamp not written" is written to
io.ErrOut, and one that simulates a successful stamp write and asserts the
successful skills sync message is written to io.Out. In each test, temporarily
replace/mock skillscheck.WriteStamp (and restore it after), capture io.Out and
io.ErrOut into buffers, invoke the update command entrypoint used in tests (the
function that triggers the sync flow and calls skillscheck.WriteStamp, e.g., the
update command handler/run function), and assert the expected substrings are
present in the respective buffers; use stampVersion or the same invocation flags
that exercise the skills sync branch so the code paths that call
skillscheck.WriteStamp and emit the success/warning messages are exercised.
Users who install or upgrade lark-cli via make install, go install, or
direct binary download end up with a binary but no AI agent skills,
degrading agent UX. This PR adds a startup-time skills version drift
notice (injected into JSON envelope _notice.skills, mirroring the
existing _notice.update pattern) and unifies lark-cli update's skills
sync across all three branches (npm / manual / already-latest) with
stamp-based dedup, so any explicit update invocation keeps skills in
sync regardless of how the binary was installed.
Changes:
- new internal/skillscheck package: notice (StaleNotice + atomic
pending), stamp (~/.lark-cli/skills.stamp), skip (CI / DEV /
non-release / LARKSUITE_CLI_NO_SKILLS_NOTIFIER opt-out), check
(synchronous Init)
- cmd/root.go: rename setupUpdateNotice -> setupNotices, compose
output.PendingNotice returning {update?, skills?}; capture
build.Version locally before spawning the async update goroutine
- cmd/update/update.go: add runSkillsAndStamp helper with stamp-based
dedup; rewire the three branches through shared applySkillsResult /
emitSkillsTextHints helpers; add skills_status block to --check JSON
output as a pure report (no side effects)
- internal/update: export IsRelease(version) bool / IsCIEnv() bool
for cross-package reuse; refresh UpdateInfo.Message to append
', run: lark-cli update' so both notices recommend the same fix
- AGENTS.md: add Notification Opt-Outs section documenting
LARKSUITE_CLI_NO_UPDATE_NOTIFIER and LARKSUITE_CLI_NO_SKILLS_NOTIFIER
- internal/binding/types.go: bump default exec-provider timeout from
5s to 10s (out-of-scope flake fix for TestResolveExecRef_JSONResponse
under heavy parallel test load)
niuchong0523
force-pushed
the
feat/skills-autosync
branch
from
728f54d to
5c4e808
Compare
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
Users who install or upgrade
lark-cliviamake install,go install, or download a binary directly currently end up with a binary but no AI agent skills, degrading agent UX. This PR adds a startup-time skills version drift notice (injected into JSON envelope_notice.skills, mirroring the existing_notice.updatepattern) and unifieslark-cli update's skills sync across all three branches (npm / manual / already-latest) with stamp-based dedup, so any explicit update invocation now keeps skills in sync regardless of how the binary was installed.Changes
internal/skillscheckpackage:notice.go(StaleNotice + atomic.Pointer pending),stamp.go(~/.lark-cli/skills.stampread/write, atomic),skip.go(CI / DEV / non-release /LARKSUITE_CLI_NO_SKILLS_NOTIFIERopt-out),check.go(synchronousInit)cmd/root.go: renamesetupUpdateNotice→setupNotices, composeoutput.PendingNoticereturning{update?, skills?}; capturebuild.Versionlocally before spawning the async update goroutine to avoid a data race with testscmd/update/update.go: addrunSkillsAndStamphelper with stamp-based dedup; rewire the three branches (doNpmUpdate,doManualUpdate, already-up-to-date) through sharedapplySkillsResult/emitSkillsTextHintshelpers; addskills_statusblock to--checkJSON output (pure report, no side effects); extractmaxStderrDetailconstantinternal/update/update.go: exportIsRelease(version) boolandIsCIEnv() boolfor cross-package reuse; refreshUpdateInfo.Message()text to append, run: lark-cli updateso both notices recommend the same canonical fix commandcmd/root_integration_test.go: cover the composed PendingNotice envelope injection paths (cold start / in-sync / drift)AGENTS.md: add "Notification Opt-Outs" section documentingLARKSUITE_CLI_NO_UPDATE_NOTIFIERand the newLARKSUITE_CLI_NO_SKILLS_NOTIFIERinternal/binding/types.go: bump default exec-provider timeout from 5s to 10s — fixes a pre-existingTestResolveExecRef_JSONResponseflake under heavy parallel test load (out of scope for the spec but blocksmake unit-test)Test Plan
make unit-testpassedmake validate(go build+go vet+ unit + integration + convention guard + security) passed--checkside-effect violation)lark-cli foo --format jsonwith cold stamp →_notice.skills.messagematcheslark-cli skills not installed, run: lark-cli updatelark-cli foo --format jsonwith drift stamp → matcheslark-cli skills <current> out of sync with binary <target>, run: lark-cli updatelark-cli foo --format jsonwith matching stamp → no_notice.skillskeylark-cli updatealready-up-to-date → runs npx, writes stamp; second invocation hits dedup (16.5s → 1.65s,skills_action: in_sync)lark-cli update --check --jsonalready-up-to-date → no side effects, returnsskills_status: {current, target, in_sync}in <2sLARKSUITE_CLI_NO_SKILLS_NOTIFIER=1andCI=1both suppress the notice independently of update notifierRelated Issues
N/A
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests