fix: silence misleading "skills not installed" startup notice#801
fix: silence misleading "skills not installed" startup notice#801niuchong0523 wants to merge 1 commit intomainfrom
Conversation
|
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 (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR eliminates the intermediate "cold-start with empty Current" notice state. When no skills stamp exists, ChangesCold-Start No-Notice Behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🤖 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/root_integration_test.go`:
- Around line 507-514: Update the test comment to reflect the new behavior:
remove or rewrite the stale lines that claim the composed PendingNotice includes
a "skills" key with an empty Current and cold-start message (the lines
referenced by TestSetupNotices_ColdStart), since
TestSetupNotices_ColdStart_NoNotice and the updated assertions expect no
"skills" key; locate the comment block around
TestSetupNotices_ColdStart/TestSetupNotices_ColdStart_NoNotice and either delete
the contradictory sentences or replace them with a concise note that a missing
stamp produces no "skills" key so the cold-start notice is not emitted.
🪄 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: 39588b6d-80d1-4a43-b83e-50e359231fa6
📒 Files selected for processing (5)
cmd/root_integration_test.gointernal/skillscheck/check.gointernal/skillscheck/check_test.gointernal/skillscheck/notice.gointernal/skillscheck/notice_test.go
💤 Files with no reviewable changes (1)
- internal/skillscheck/notice_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #801 +/- ##
=======================================
Coverage 65.46% 65.46%
=======================================
Files 510 510
Lines 47129 47129
=======================================
Hits 30851 30851
Misses 13607 13607
Partials 2671 2671 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Remove the cold-start _notice.skills that fires whenever
~/.lark-cli/skills.stamp is missing. The stamp is written
exclusively by `lark-cli update`, so users who installed skills via
`npx skills add larksuite/cli -g` (the documented path) saw the
notice on every run despite a fully populated ~/.agents/skills/.
The version-drift notice (stamp != binary) is preserved unchanged
for users who opted into tracking by running `lark-cli update`.
- internal/skillscheck/check.go: Init returns silently on empty stamp
- internal/skillscheck/notice.go: drop dead cold-start branch in Message;
Current field is now guaranteed non-empty
- tests updated in skillscheck package + cmd/root_integration_test.go
to assert the new contract
No new files, no env vars, no JSON schema change. The _notice.skills
shape stays {current, target, message} — only the cold-start message
string is no longer possible.
niuchong0523
force-pushed
the
fix/skillscheck-disk-fallback
branch
from
7359ba5 to
79fa471
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@79fa471e1a5db1ab65ef6dd7a773f678f23dc762🧩 Skill updatenpx skills add larksuite/cli#fix/skillscheck-disk-fallback -y -g |
Note:
|
| commit | timestamp (UTC) | e2e-live |
|---|---|---|
4aceae9 (this PR's base) |
2026-05-09 12:35 | ❌ failure (same TestSheets_* tests + some TestCalendar_*) |
44ffa98 |
2026-05-09 08:54 | ❌ failure |
f9792f0 |
2026-05-09 06:16 | ✅ success |
| earlier commits | ✅ success |
e2e-live started consistently failing on main around 2026-05-09 ~09:00 UTC. Likely a Sheets API test-account / quota / live-service issue on the harness side; not a code regression.
This PR's diff does not touch the sheets package
cmd/root_integration_test.go
internal/skillscheck/check.go
internal/skillscheck/check_test.go
internal/skillscheck/notice.go
internal/skillscheck/notice_test.go
No file under tests/cli_e2e/sheets/, shortcuts/sheets/, or any sheets code path is modified.
All other checks pass
unit-test, lint, license-header, security, coverage, deadcode, e2e-dry-run, Analyze (go), Analyze (javascript-typescript), Analyze (python), fast-gate, publish, codecov/patch, codecov/project, Continuous Releases, CLA, sync-pr-labels — all green.
Requesting merge despite the red e2e-live. Happy to rebase once main's e2e-live is restored if reviewers prefer to wait.
1 participant
Summary
Remove the cold-start
_notice.skills("skills not installed, run: lark-cli update") that fires whenever~/.lark-cli/skills.stampis missing. The stamp is written exclusively bylark-cli update, so users who installed skills vianpx skills add larksuite/cli -g(the documented path) saw the misleading notice on every CLI run despite a fully populated~/.agents/skills/.The version-drift notice (stamp version != binary version) is preserved unchanged for users who opted into tracking by running
lark-cli update.Changes
internal/skillscheck/check.go-Initreturns silently when the stamp file is missing; documents cold-start as an explicit failure mode.internal/skillscheck/notice.go-StaleNotice.Messagedrops the dead cold-start branch;Currentis now guaranteed non-empty.internal/skillscheck/check_test.go- replaceTestInit_ColdStart_NoticeWithEmptyCurrentwithTestInit_ColdStart_NoNoticeasserting silence.internal/skillscheck/notice_test.go- drop thecold_startrow fromTestStaleNotice_Message.cmd/root_integration_test.go- renameTestSetupNotices_ColdStarttoTestSetupNotices_ColdStart_NoNoticeand assert theskillskey is absent._notice.skillskeeps the same{current, target, message}shape - only the cold-start message string is no longer possible.Test Plan
make unit-testpassedauth-loginpre-flight failed on the test account's scope grant for 7 unrelated scopes (approval/attendance/im:message.send_as_user/mail:user_mailbox.message:send), so the sandbox exited before invoking E2E. Cross-validated via the Phase 3 dev-e2e-testcase-writer agent: 5/5 E2E cases (tests_e2e/skillscheck/...) pass against the patched code.echo 1.0.27 > skills.stamp(in-sync) -> no notice;echo 1.0.20 > skills.stamp+ binary 1.0.27 -> expected "out of sync" notice;LARKSUITE_CLI_NO_SKILLS_NOTIFIER=1-> silent in every state.Related Issues
N/A
Summary by CodeRabbit
Bug Fixes
Behavior
Tests