Support remote project skills#11459
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| return; | ||
| }; | ||
| let repo_metadata = RepoMetadataModel::as_ref(ctx); | ||
| let skill_paths = find_skill_files_in_tree(repo_id, repo_metadata, ctx); |
There was a problem hiding this comment.
lmk if this approach makes sense to get the remote skills scan_remote_repository_for_skills -> find_skill_files_in_tree -> spawn_read_remote_skills
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR threads LocalOrRemotePath through skill discovery/invocation and adds remote project skill hydration from remote repo metadata.
Concerns
- Remote project skills are only scanned on the initial remote repository update; later remote file-tree updates and repository removals are ignored, so the skill cache can become stale.
- Remote skill hydration reads every discovered remote
SKILL.mdwithout a cumulative batch limit.
Security
- The remote skill-content read has no cumulative byte budget, allowing a repository with many large skill files to drive unbounded network and memory work during metadata updates.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| RepoMetadataEvent::RepositoryRemoved { | ||
| id: RepositoryIdentifier::Local(_) | RepositoryIdentifier::Remote(_), | ||
| } | ||
| | RepoMetadataEvent::FileTreeEntryUpdated { | ||
| id: RepositoryIdentifier::Remote(_), | ||
| } |
There was a problem hiding this comment.
SKILL.md files after the initial snapshot; handle remote removals by deleting cached paths and rescan on remote file-tree updates.
| }) | ||
| .collect(), | ||
| max_file_bytes: None, | ||
| max_batch_bytes: None, |
There was a problem hiding this comment.
SKILL.md with no cumulative byte limit (max_batch_bytes: None). A repo with many large skill files can force unbounded network/memory work during metadata updates; set a bounded batch budget or cap the number of files before reading remote skill content.
| match skill_ref.skill_reference { | ||
| Some(api::skill_ref::SkillReference::Path(path)) => Some(SkillReference::Path(path.into())), | ||
| Some(api::skill_ref::SkillReference::Path(path)) => Some(SkillReference::Path( | ||
| warp_util::local_or_remote_path::LocalOrRemotePath::Local(path.into()), |
There was a problem hiding this comment.
Why are we assuming local path here?
| .and_then(|skill_path| { | ||
| SkillManager::as_ref(app).skill_by_path(&skill_path) | ||
| SkillManager::as_ref(app) | ||
| .skill_by_path(&LocalOrRemotePath::Local(skill_path)) |
There was a problem hiding this comment.
Also why are we assuming Local path here?
| .and_then(|skill_path| { | ||
| SkillManager::as_ref(app).skill_by_path(&skill_path) | ||
| SkillManager::as_ref(app) | ||
| .skill_by_path(&LocalOrRemotePath::Local(skill_path)) |
There was a problem hiding this comment.
Also also why are we assuming Local path here?
| .and_then(|common| skill_path_from_file_path(&common)) | ||
| .and_then(|skill_path| SkillManager::as_ref(app).skill_by_path(&skill_path)); | ||
| .and_then(|skill_path| { | ||
| SkillManager::as_ref(app).skill_by_path(&LocalOrRemotePath::Local(skill_path)) |
There was a problem hiding this comment.
Also also also why are we assuming Local path here?
| @@ -71,7 +77,9 @@ impl SkillWatcher { | |||
There was a problem hiding this comment.
Should read_skills_for_repos take in LocalOrRemotePaths?
It's unclear what method assumes local and what assumes remote
| RepoMetadataEvent::RepositoryUpdated { | ||
| id: remote_id @ RepositoryIdentifier::Remote(_), | ||
| } => { | ||
| me.scan_remote_repository_for_skills(remote_id, ctx); |
There was a problem hiding this comment.
I would like to avoid having separate APIs for remote / local especially now we have LocalOrRemotePath. We should think about taking in LocalOrRemotePath as parameter and only branch off on local / remote when we need to
moirahuang
changed the base branch from
graphite-base/11459
to
moira/skills-acquisition-repometadata
Co-Authored-By: Oz <oz-agent@warp.dev>
2 participants
Description
Support remote project skills by threading through
LocalOrRemotePathand fetching remote project skill contentTesting
Added unit tests + locally tested
./script/runScreenshots / Videos
Agent Mode