Skip to content

fix: parse grouped skills list output#1188

Open
nguyenngothuong wants to merge 1 commit into
larksuite:mainfrom
nguyenngothuong:codex/fix-skills-list-detection
Open

fix: parse grouped skills list output#1188
nguyenngothuong wants to merge 1 commit into
larksuite:mainfrom
nguyenngothuong:codex/fix-skills-list-detection

Conversation

@nguyenngothuong
Copy link
Copy Markdown

@nguyenngothuong nguyenngothuong commented May 31, 2026

Summary

Fix the incremental skills sync local-skill detection when npx skills ls -g prints grouped, indented human-readable output. The parser now recognizes indented skill rows without treating category headings as skills.

Changes

Test Plan

  • Unit tests pass: go test ./internal/skillscheck -count=1
  • Unit tests pass: go test ./cmd/update -count=1
  • Unit tests pass: go test ./internal/skillscheck ./cmd/update -count=1
  • Manual local verification confirms the lark-cli update flow works as expected

Related Issues

Summary by CodeRabbit

  • Bug Fixes

    • Improved skill extraction to correctly parse grouped and indented skills from the Global Skills list, ensuring all skills are properly identified and processed.
  • Tests

    • Added test coverage for skill extraction with grouped indented sections.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 31, 2026

CLA assistant check
All committers have signed the CLA.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 31, 2026

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: 0e6de5eb-34a3-4b80-a01c-dacb553c8c27

📥 Commits

Reviewing files that changed from the base of the PR and between 99e314f and e31109f.

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

📝 Walkthrough

Walkthrough

Updated parseGlobalSkillsList to handle indented skill listings by validating field count and recognizing skill paths using a new looksLikeSkillPath predicate, replacing prior indentation-based filtering. Added test coverage for grouped/indented output.

Changes

Grouped Skills Output Parsing

Layer / File(s) Summary
Path validation for grouped indented skills
internal/skillscheck/sync.go, internal/skillscheck/sync_test.go
looksLikeSkillPath predicate recognizes common skill path patterns (home, absolute, relative, Windows drives, forward-slashes). parseGlobalSkillsList now requires at least two fields and validates the second field as a path pattern instead of skipping indented lines. New test TestParseGlobalSkillsListWithGroupedIndentedSkills verifies parsing of grouped/categorized skill output.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • larksuite/cli#1042: Introduces the overall skillscheck sync/parsing system that this PR enhances to handle grouped output formats.

Suggested labels

bug

Poem

🐰 Indented skills once slipped away unnoticed,
Now grouped by category, they're nicely noticed!
With paths recognized and validation tight,
The parser hops through grouped lists just right. 🌱

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% 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 'fix: parse grouped skills list output' directly and concisely describes the main change of updating the skills parser to handle grouped, indented output.
Description check ✅ Passed The description includes all required sections: Summary, Changes, Test Plan (with checkmarks showing unit tests pass), and Related Issues with fix reference #1186.
Linked Issues check ✅ Passed The code changes directly address issue #1186 by allowing the parser to handle indented skill rows with path validation and adding regression test coverage for grouped output.
Out of Scope Changes check ✅ Passed All changes are within scope: parser logic update to handle grouped output, new helper function for path validation, and regression test for the specific issue reported.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 May 31, 2026
@nguyenngothuong nguyenngothuong marked this pull request as ready for review June 1, 2026 09:53
@nguyenngothuong
Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 1, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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.

lark-cli update incremental skills sync does not detect installed skills from grouped skills ls output

2 participants