Replace registries with new add command which installs from git without filtering#32
Merged
Merged
Conversation
…d needs improvement still.
…by git source URI.
…f explicit manifest state This commit removes the explicit `gitRepos` array tracking from the local `SkillManifest` and dynamically infers installed git repositories by inspecting the `sourceUri`s of installed skills. This eliminates duplicate state and simplifies the manifest model. Additionally: - Transitions registries to a unified GitRepo model. - Removes manual tracking of git repos from the `skills add` command and skill_installer. - Modifies `skills get` to dynamically scan explicit gitUris along with inferred repos. - Improves migration logic to seamlessly convert old `package:<name>` URIs to `https://` URIs for registries converted to local git clones. - Isolates package URI prefix migrations to a dedicated function evaluated prior to registry migrations. TAG=agy CONV=84ac14d5-9b15-42f2-9ed9-27befbf8c286
Collaborator
Author
|
cc @kenzieschmoll for review |
kenzieschmoll
left a comment
kenzieschmoll
left a comment
Contributor
There was a problem hiding this comment.
Can you also add tests for the cases I called out where --all and specific skill names may be passed?
Additional test coverage comments:
- No
AddCommandTests: The new command (lib/src/commands/add_command.dart) has no test file - Untested
parseGitRepoArg: The URL parsing utility inlib/src/core/git_repos.darthas no unit tests. - Migration Edge Cases: There is no test coverage for when a user aborts the migration dialog or when migrating SSH-cloned legacy repositories (the latter currently causes a runtime crash).
Collaborator
Author
|
Addressed all comments so I am going to go ahead and land this and start work on the fork into dart-lang/ai |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes dart-lang/ai#470
addcommand with syntax similar tonpx skills add, standard commands from https://skills.sh will work if you substitutedart run skills@fornpx skills.dart run skills@ add dart-lang/skills. I filed a separate issue for us to suggest dart-lang/skills and flutter/skills on the first run in any project.registrycommand entirely,package:<name>.getcommand will now list any installed repos as available to install skills from.