Add configutation files for Claude Code plugin definition#172
Conversation
🤖 chore: sync skills directory from dart-lang/skills
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds Claude Code plugins, MCP server configurations, and a comprehensive set of Dart and Flutter skill guides, along with a synchronization script tool/sync_skills.dart. The code review identified several critical issues in the synchronization script, including cross-platform path separator mismatches on Windows, missing exit code checks and rename handling in git diff commands, silent failures when full synchronization fails, and a lack of defensive checks for the existence of the source directory. Additionally, a minor syntax typo (unmatched double quote) was flagged in the dart-migrate-to-checks-package skill documentation.
|
|
||
| await for (final entity in sourceSkillsDir.list(recursive: true)) { | ||
| if (entity is File) { | ||
| final relPath = entity.path.replaceFirst('$srcDir/skills/', ''); |
There was a problem hiding this comment.
On Windows, entity.path uses backslashes (\\) as path separators, whereas $srcDir/skills/ uses forward slashes (/). This mismatch causes replaceFirst to fail to match, leading to incorrect destination paths during a full sync.
Normalize the path separators to forward slashes before performing the replacement to ensure cross-platform compatibility.
| final relPath = entity.path.replaceFirst('$srcDir/skills/', ''); | |
| final relPath = entity.path.replaceAll('\\', '/').replaceFirst('${srcDir.replaceAll('\\', '/')}/skills/', ''); |
| ..has((e) => e.message, 'message').equals('invalid input'); | ||
|
|
||
| // NO (Passing a callback to sync throws will cause a compiler error!) | ||
| check(() => triggerSync").throws<ArgumentError>((it) => ...); // ERROR! |
There was a problem hiding this comment.
There is an unmatched double quote in the code example (triggerSync"). It should be triggerSyncError() to match the context of the example.
| check(() => triggerSync").throws<ArgumentError>((it) => ...); // ERROR! | |
| check(() => triggerSyncError()).throws<ArgumentError>((it) => ...); // ERROR! |
| elif [ $EXIT_CODE -eq 10 ]; then | ||
| echo "changes_detected=false" >> $GITHUB_OUTPUT | ||
| echo "✅ Dart script signaled NO changes were found. Stopping workflow." | ||
| else |
There was a problem hiding this comment.
Silent Failure Bug
GitHub Actions run bash scripts with set -e by default. If dart run exits with 10 (no changes) or 1 (real error), the bash script will immediately terminate on this line. It will never execute EXIT_CODE=$? or the if/else block below it.
Because continue-on-error: true is set on the step, the step will still be marked as green, changes_detected won't be set, and the PR creation step will be skipped. This means real errors (exit code 1) will be silently swallowed and ignored.
The Fix: Remove continue-on-error: true and handle the exit code directly so set -e doesn't abort the script. For example:
- name: Execute Dart Sync Engine
id: sync_engine
run: |
cd dest_repo
# Temporarily disable exit on error to capture the exit code
set +e
dart run tool/sync_skills.dart ../src_repo skills
EXIT_CODE=$?
set -e
if [ $EXIT_CODE -eq 0 ]; then
echo "changes_detected=true" >> $GITHUB_OUTPUT
elif [ $EXIT_CODE -eq 10 ]; then
echo "changes_detected=false" >> $GITHUB_OUTPUT
echo "✅ Dart script signaled NO changes were found. Stopping workflow."
else
echo "❌ Dart script failed with error code $EXIT_CODE"
exit $EXIT_CODE
fi| final versionParts = versionStr.split('.'); | ||
| if (versionParts.length == 3) { | ||
| final patch = int.parse(versionParts[2]) + 1; | ||
| versionParts[2] = patch.toString(); |
There was a problem hiding this comment.
Minor Edge Case in Version Bumping
If the version ever includes a pre-release or build suffix (e.g., 1.0.0-beta.1), int.parse will throw a FormatException. The script currently catches the exception cleanly (so it won't crash the sync), but it will fail to bump the version. If you ever plan to use pre-release versions, you may want to handle stripping suffixes before parsing.
3 participants
This PR adds
Tested by manually running the workflow to sync in dart-lang/skills.