fix: use json skills list during update

[zhangheng023]

Summary

lark-cli update previously parsed the human-readable skills ls -g output to determine locally installed skills. This PR switches that lookup to the structured skills ls -g --json output first, while preserving text fallback and full-install fallback behavior.

Changes

• Add ListGlobalSkillsJSON() in internal/selfupdate/updater.go to run npx -y skills ls -g --json.
• Add ParseGlobalSkillsJSON and JSON-first local skills lookup in internal/skillscheck/sync.go.
• Keep text listing fallback and update text parsing to accept grouped, indented skill rows.
• Preserve valid versioned skill names returned by local JSON/text listing.
• Update tests in internal/skillscheck/sync_test.go, internal/selfupdate/updater_test.go, and cmd/update/update_test.go.

Test Plan

• make unit-test passed
• go vet ./... passed
• gofmt -l . produced no output
• go mod tidy did not change go.mod or go.sum
• go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main passed
• validate passed
• local-eval skipped: lite mode internal update-sync change; no shortcut or skill contract changed
• acceptance-reviewer passed (2/2 cases)
• manual verification: go test ./internal/skillscheck ./internal/selfupdate ./cmd/update

Related Issues

N/A

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR implements JSON-based global skills listing as the preferred path in the skills sync flow. It adds ListGlobalSkillsJSON() to the updater, introduces ParseGlobalSkillsJSON() for JSON unmarshaling and filtering, refines text-based parsing, and wires JSON-first logic into SyncSkills with fallback to text parsing. All existing tests are updated to support and verify the JSON path.

Changes

Global Skills JSON Listing and Integration

Layer / File(s)	Summary
Updater JSON command support cmd/update/update_test.go, internal/selfupdate/updater.go, internal/selfupdate/updater_test.go	New ListGlobalSkillsJSON() method on Updater executes the skills command with -y skills ls -g --json; the test mock is extended to recognize and respond to this argument set.
Sync contract and JSON/text parsing internal/skillscheck/sync.go	SkillsRunner interface gains ListGlobalSkillsJSON() method. ParseGlobalSkillsJSON() unmarshals JSON-formatted skill objects, filters by skillNamePattern, deduplicates, and returns sorted names. Text parsing is updated to skip Agents: lines, detect section headers more reliably, and validate candidates by skillNamePattern.
Sync flow JSON-first integration internal/skillscheck/sync.go	listLocalSkills() helper attempts JSON listing and parsing first; if that path fails or returns empty, it retries with text listing and parsing. SyncSkills() now uses listLocalSkills() and triggers full-install fallback if both paths fail or produce empty results.
Sync test coverage for JSON path internal/skillscheck/sync_test.go	New tests cover ParseGlobalSkillsJSON() with valid/invalid inputs and text-parsing edge cases. fakeSkillsRunner is enhanced with JSON output fields and counters to track listing method calls. globalSkillsJSONOutput() helper generates deterministic JSON. All existing SyncSkills tests are updated to include JSON output configuration and are instrumented to verify correct listing path selection and fallback behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• larksuite/cli#1186: PR implements JSON-based global-skills listing (ParseGlobalSkillsJSON, SkillsRunner.ListGlobalSkillsJSON) and integrates it into SyncSkills to correctly detect grouped/indented installed skills, directly addressing the issue's suggestion to prefer npx -y skills ls -g --json.

Possibly related PRs

• larksuite/cli#1233: Both PRs modify SyncSkills() control flow to trigger fallbackFullInstall in additional scenarios (this PR via JSON/text parsing emptiness; referenced PR via plan.ToUpdate empty handling).
• larksuite/cli#1042: Both PRs extend the SkillsRunner interface and internal/skillscheck/sync.go flow; this PR adds JSON-based discovery while referenced PR introduces state-based incremental sync orchestration.
• larksuite/cli#1008: Both PRs modify skills-sync implementation; this PR adds JSON-based listing/parsing and extends SkillsRunner, while referenced PR removes incremental sync code in favor of stamp-based approach, creating potential overlap/conflict at the sync module level.

Suggested reviewers

• liangshuo-1
• MaxHuang22

Poem

🐰 A JSON path hops first, then text falls back with grace,
Skills listed in the clearest format win the race,
Parsing patterns dance through grouped and sorted rows,
Where fallback keeps the sync flow safe wherever it goes! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: switching the skills list lookup to use JSON format during update operations.
Description check	✅ Passed	The description fully covers required sections including Summary, Changes, Test Plan with checkmarks, and Related Issues, matching the repository template.
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-json-list

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8c6fffff28bf99fa2fd1ef7b65e9104487fcfaac

🧩 Skill update

npx skills add larksuite/cli#fix/update-skills-json-list -y -g

[coderabbitai]

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.go`:
- Around line 330-345: The listLocalSkills function currently treats a
parsed-but-empty skills list as failure because it only returns success when
len(local) > 0; update it so that a successfully parsed empty list is considered
success. Specifically, in listLocalSkills replace the len(local) > 0 guards with
checks that parsing succeeded (i.e.,
ParseGlobalSkillsJSON(jsonResult.Stdout.String()) and
ParseSkillsList(textResult.Stdout.String()) returned a non-nil slice or parsing
indicator) and return local, true immediately when parsing completed without
error (jsonResult != nil && jsonResult.Err == nil or textResult != nil &&
textResult.Err == nil), even if len(local) == 0; keep falling through only when
the runner call failed or parsing returned an error.

🪄 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: 5dd1aeb6-85ea-4cf6-b4f6-66b75a126ffa

📥 Commits

Reviewing files that changed from the base of the PR and between 7e7f716 and 8c6ffff.

📒 Files selected for processing (5)

• cmd/update/update_test.go
• internal/selfupdate/updater.go
• internal/selfupdate/updater_test.go
• internal/skillscheck/sync.go
• internal/skillscheck/sync_test.go

[codecov]

Codecov Report

❌ Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 69.33%. Comparing base (85c7280) to head (8c6ffff).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/skillscheck/sync.go	97.22%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1251      +/-   ##
==========================================
+ Coverage   69.22%   69.33%   +0.11%     
==========================================
  Files         639      645       +6     
  Lines       59798    59994     +196     
==========================================
+ Hits        41395    41598     +203     
+ Misses      15058    15041      -17     
- Partials     3345     3355      +10     

☔ 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.