Skip to content

fix: recover toUpdate skills empty fallback#1233

Merged
zhangheng023 merged 1 commit into
mainfrom
fix/update_skills_fallback
Jun 2, 2026
Merged

fix: recover toUpdate skills empty fallback#1233
zhangheng023 merged 1 commit into
mainfrom
fix/update_skills_fallback

Conversation

@zhangheng023

@zhangheng023 zhangheng023 commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Changes

  • Change 1
  • Change 2

Test Plan

  • Unit tests pass
  • Manual local verification confirms the lark-cli <domain> <command> flow works as expected

Related Issues

  • None

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Enhanced sync process reliability by ensuring fallback to full installation when incremental updates are unavailable.
  • Tests

    • Added test coverage for sync fallback behavior.

@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e29627de-0959-4cf6-aa59-59d232c23c11

📥 Commits

Reviewing files that changed from the base of the PR and between 04932c2 and 55d0618.

📒 Files selected for processing (2)
  • internal/skillscheck/sync.go
  • internal/skillscheck/sync_test.go

📝 Walkthrough

Walkthrough

When the incremental update plan is empty, SyncSkills now calls fallbackFullInstall instead of proceeding to conditional incremental install logic. A new test verifies this fallback path triggers a full install with expected result action and skill slices.

Changes

Empty ToUpdate Fallback

Layer / File(s) Summary
Fallback guard and test coverage
internal/skillscheck/sync.go, internal/skillscheck/sync_test.go
SyncSkills checks plan.ToUpdate immediately after building the sync result; if empty, it calls fallbackFullInstall instead of proceeding. New test TestSyncSkills_EmptyToUpdateFallsBackToFullInstall verifies that when there are no incremental updates, the sync falls back to a full install, makes no incremental runner calls, and returns expected result action and skill slices.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • larksuite/cli#1042: Both PRs touch the skills synchronization logic in internal/skillscheck/sync.go—specifically SyncSkills' incremental vs full-install behavior driven by the computed sync plan (including plan.ToUpdate).

Suggested labels

size/M

Suggested reviewers

  • MaxHuang22
  • ZKHelloworld

Poem

🐰 When updates are empty, the guard stands tall,
No incremental tricks, just fallback to all.
Full install rides in, a noble retreat,
The sync dance completes, the skills all replete!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description follows the required template structure but contains only placeholder text with no actual content about the motivation, specific changes, or testing approach. Fill in the Summary, Changes, and Test Plan sections with actual details about what was fixed, why it was needed, how it was verified, and what testing was performed.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: recover toUpdate skills empty fallback' is specific and relates to the code changes, clearly indicating a fallback mechanism for empty toUpdate.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/update_skills_fallback

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the size/M Single-domain feat or fix with limited business impact label Jun 2, 2026
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@55d0618125b0f23d69fb97a550e27df3aef82f97

🧩 Skill update

npx skills add larksuite/cli#fix/update_skills_fallback -y -g

@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.22%. Comparing base (04932c2) to head (55d0618).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1233   +/-   ##
=======================================
  Coverage   69.22%   69.22%           
=======================================
  Files         635      635           
  Lines       59687    59689    +2     
=======================================
+ Hits        41320    41322    +2     
  Misses      15033    15033           
  Partials     3334     3334           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zhangheng023
zhangheng023 merged commit c8e205e into main Jun 2, 2026
20 checks passed
@zhangheng023
zhangheng023 deleted the fix/update_skills_fallback branch June 2, 2026 15:26
tuxedomm pushed a commit to zhumiaoxin/cli that referenced this pull request Jun 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants