feat: Implement per-run Git cache and restructure add/sync flows.#67
feat: Implement per-run Git cache and restructure add/sync flows.#67EriiYenn wants to merge 54 commits intowestonplatter:mainfrom
Conversation
Add PR_MESSAGE markdown file (WIP) to store Pull Request message back to Weston's origin.
Fixed based on PR westonplatter#51 issues from CodeRabbit.
Fix of Minor issue with `exists` based on CodeRabbit.
Removed Claude Hooks integration due to problematic approach with `settings.json` and already existing implementation of Claude Settings.
Fixed Symlink remove check based on CodeRabbit.
Removed bumped version configuration, as the `release-please` will take care of it.
Finalization of the pull request. Upgraded Trunk packages. Fixed README formatting.
Verified correct implementation of Git caching. Upgraded some Trunk linters.
Fixed based on CodeRabbit.
Added better integration of `aps add` command with processing SSH urls and more customizability.
Changed the Lockfile name to derive from manifest name to allow multiple manifests in the same repository, e.g. having manifest for Cursor and Codex.
Added documentation about new Lockfile changes.
Added simple Sync Progress outside the Verbose output.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPer-run Git clone caching added; lockfile naming moved to manifest-derived ".lock.yaml" with legacy-name migration; add command refactored to accept flexible positional arguments (asset type + URL/path) plus --path/--ref; git shallow flag exposed; README, docs, and tests updated; new error variants and per-entry sync progress added. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Parser
participant AddCmd as Add Command
participant RepoParser as Repo Parser
participant Cache as GitCloneCache
participant GitOps as Git Operations
User->>CLI: aps add [ASSET_TYPE] <URL/PATH>
CLI->>AddCmd: parsed AddArgs (positionals, --path, --ref)
AddCmd->>RepoParser: parse_add_positionals() / parse_repo_identifier()
RepoParser-->>AddCmd: ParsedRepoIdentifier{repo_url, git_ref, path}
AddCmd->>Cache: resolve_git_source_with_cache(repo_url, git_ref, shallow)
alt cache hit
Cache-->>AddCmd: ResolvedGitSource (cached)
else cache miss
Cache->>GitOps: clone_and_resolve(repo_url, git_ref, shallow)
GitOps-->>Cache: ResolvedGitSource
Cache->>Cache: store resolved source
Cache-->>AddCmd: ResolvedGitSource
end
AddCmd->>AddCmd: create manifest entry
AddCmd-->>User: asset added
sequenceDiagram
actor User
participant Sync as Sync Command
participant Manifest as Manifest Loader
participant Install as Install Flow
participant Cache as GitCloneCache
participant GitOps as Git Operations
participant Lockfile as Lockfile Manager
User->>Sync: aps sync [--manifest PATH]
Sync->>Manifest: load(manifest_path)
Manifest-->>Sync: Entries
Sync->>Install: create GitCloneCache
loop each entry
Install->>Cache: resolve_git_source_at_commit_with_cache(repo, commit, shallow)
alt cache hit
Cache-->>Install: cached clone path
else
Cache->>GitOps: clone_at_commit(repo, commit, shallow)
GitOps-->>Cache: cloned path
Cache->>Cache: store by commit
Cache-->>Install: cloned path
end
Install->>Install: install_entry(entry)
Install->>Lockfile: update checksums/[current]
end
Sync->>Lockfile: path_for_manifest(manifest)
Lockfile-->>Sync: <manifest>.lock.yaml
Sync->>Lockfile: save(canonical_path)
Lockfile->>Lockfile: remove legacy lockfiles
Lockfile-->>User: sync complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Fixed Clippy build in CI Lint.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.trunk/trunk.yaml:
- Around line 25-33: The Trunk plugin version bumps for checkov@3.2.500 and
trufflehog@3.93.0 must be validated against Trunk v1.7.4: review the release
notes/changelogs for checkov and trufflehog between the validated versions
(checkov 3.2.490 and trufflehog 3.90.13) and run the Trunk integration tests
locally using Trunk v1.7.4 (exercise the checkov and trufflehog checks), then
either confirm no breaking changes and add a short comment in the PR that
compatibility is validated, or revert these entries to the tested pins
(checkov@3.2.490, trufflehog@3.90.13) or update the Trunk plugin manifest to a
new compatible manifest version if breaking changes require it.
🧹 Nitpick comments (3)
src/lockfile.rs (1)
382-407: Verify path comparison handles relative vs. absolute paths correctly.The comparison
legacy_path == pathat line 388 could fail ifpathwas provided as a relative path whilelegacy_pathis constructed as an absolute path (or vice versa), potentially leading to the canonical lockfile being incorrectly deleted if it matches a legacy name.Consider canonicalizing both paths before comparison for robustness:
♻️ Proposed fix
for legacy_name in LEGACY_LOCKFILE_NAMES { let legacy_path = dir.join(legacy_name); - if !legacy_path.exists() || legacy_path == path { + // Canonicalize to handle relative vs. absolute path comparison + let same_file = legacy_path + .canonicalize() + .ok() + .zip(path.canonicalize().ok()) + .map(|(a, b)| a == b) + .unwrap_or(false); + if !legacy_path.exists() || same_file { continue; }src/github_url.rs (1)
160-263: Local path fallback may produce unexpected results.The function checks
Path::new(input).exists()(line 250) as a fallback, which could match unintended inputs if a directory happens to exist with a name that looks like a repository identifier. For example, if a user typos a URL and a local directory with that name exists, it will silently succeed.Consider whether this behavior is intentional. If local paths are a supported use case, it may be worth documenting this behavior prominently or requiring a
file://prefix for local paths to make the intent explicit.tests/cli.rs (1)
967-1104: Avoid asserting cache behavior via log strings.
Matching on"Cloning git repository:"/"Reusing cached clone"is a brittle implementation detail; consider asserting a stable user-visible signal (e.g., a reported cache hit metric) or injecting a lightweight counter for tests. As per coding guidelines: Add unit tests for behavior, not implementation in Rust.
EriiYenn
changed the title
|
@EriiYenn can you explain more about the features/functionality changes in the PR related to the changes in the README? I'm thinking of specifically this part, |
@westonplatter Yes, absolutely. I made the lockfiles dependent on the specific manifest because, in my case, multiple people can work in the same repository and not everyone uses the same provider (e.g., Cursor vs. Copilot). I tried maintaining two separate manifests (one for Cursor and one for Copilot), but the second run ended up overwriting the existing lockfile. My goal with this approach was to avoid that collision and make the lockfile deterministic per manifest. That said, if lockfiles are kept separate and/or not committed to Git, this may not be an issue at all. Let me know what you prefer — I’m happy to revert the change if you’d rather keep the current behavior. |
Resolved conflicts with recent changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/error.rs`:
- Around line 168-178: The enum variant MissingAddPath is declared but unused;
either remove the MissingAddPath { asset_type: String, hint: String } variant
from the error enum to eliminate dead code, or add a short doc comment above the
variant explaining it's intentionally reserved for future use and why (e.g.,
"Reserved for add subcommand validations when an asset type requires a repo
path"). Update references near InvalidRepoSpecifier and InvalidAddArguments to
ensure no callers expect MissingAddPath, and run cargo check to confirm there
are no remaining references to MissingAddPath.
🧹 Nitpick comments (5)
src/github_url.rs (1)
225-234: Minor inconsistency:Some("")vsNonefor path/ref on repo-level URLs.When a GitHub
tree/<ref>URL with no subpath (e.g.,https://github.com/owner/repo/tree/main) matches Lines 226–228,parse_github_urlreturns an emptypathand a concretegit_ref. This producesParsedRepoIdentifier { path: Some(""), git_ref: Some("main") }, while a bare repo URL (e.g.,https://github.com/owner/repo) at Line 237 returns{ path: None, git_ref: None }. Callers incommands.rstreat empty-string path andNoneequivalently (unwrap_or_default), so this works today, but the semantic gap could surprise future consumers.src/lockfile.rs (1)
370-406: Serialization error usesLockfileReadError— slightly misleading variant name.Line 372 maps a serialization failure to
ApsError::LockfileReadError, but this is a write path. This predates your changes and is a pre-existing issue, so not blocking.tests/cli.rs (1)
967-1036: Good integration test for clone caching, but note log-message coupling.The test validates a key optimization (reusing clones for same-repo entries) by matching on verbose log messages (
"Cloning git repository:","Reusing cached clone"). This is inherently coupled to the log format — if the message changes, the test breaks silently (passes with 0 counts). Consider adding a comment noting this dependency, or asserting on the absence of unexpected counts as a safety net.src/commands.rs (2)
1056-1113: Early return on sync error discards partial lockfile updates.If entry N fails, Lines 1111–1112 return immediately. The lockfile updates at Lines 1124–1144 are never reached, so successfully-synced entries (1..N−1) won't be recorded. On the next run, those entries will be re-synced unnecessarily. This is the transactional "all-or-nothing" approach, which is safe but suboptimal for long sync runs. Consider whether persisting partial results (or at least logging a warning) would improve the experience.
315-340:--pathoverride logic incmd_addworks but has a dead branch.Lines 337–339 contain an empty
else ifwith a comment:} else if search_path.is_empty() && asset_kind == AssetKind::AgentSkill { // Repo-level agent_skill with no path: keep discovery (will clone to find skills) }This is a no-op conditional that exists solely as documentation. While harmless, it could be clearer as a plain comment before the
if letblock rather than an empty branch — or collapsed into theif let Some(ref path) = args.pathguard.
Commented unused error.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/error.rs`:
- Around line 176-179: Remove the commented-out enum variant and its attributes
related to MissingAddPath in src/error.rs: delete the three commented lines (the
#[error(...)] and #[diagnostic(...)] attributes and the MissingAddPath {
asset_type: String, hint: String }, declaration) so the unused "Not used at the
moment" placeholder is removed from the codebase; rely on VCS to restore it if
needed later.
|
@EriiYenn thanks for the description. Could you send over a real world example of the manifests/resource list for aps-rules.yaml and custom? I'm curious to see what this looks like from a dev experience. |
@westonplatter The core challenge across providers is that each one has different target locations for where rules/skills/etc. need to be written. For example, the same “skill” ends up in Beyond paths, there are also real format and compatibility differences. Skills are fairly standardized, but rules vary between Cursor/Copilot/Codex, Cursor Commands don’t translate cleanly to Copilot or Codex, and there are other provider-specific behaviors on top of that. So in practice, each provider needs its own sync handling, and separate lockfiles are the cleanest way to track and archive exactly what was synchronized for each provider. Skills# Cursor Manifest
- id: general-skills
kind: cursor_skills_root
source:
type: git
repo: <SAME_REPO>
ref: master
path: skills/general
dest: ~/.cursor/skills/general
# Copilot Manifest
- id: general-skills
kind: cursor_skills_root
source:
type: git
repo: <SAME_REPO>
ref: master
path: skills/general
dest: ~/.copilot/skills/generalRules# Cursor Manifest
- id: general-rules
kind: cursor_rules
source:
type: git
repo: <SAME_REPO>
ref: master
path: rules/general
dest: .cursor/rules/general
# Copilot Manifest
- id: ai-task-flow-instruction
kind: agents_md
source:
type: git
repo: <SAME_REPO>
ref: master
path: rules/general/ai-task-flow.mdc # Cursor Rule with additional Copilot headers
dest: .github/instructions/ai-task-flow.instruction.md
# ... |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/commands.rs (3)
104-105:⚠️ Potential issue | 🟡 MinorFile name comparison should be case-insensitive.
The check for
SKILL.mdonly handles two specific cases (SKILL.mdandskill.md). Per coding guidelines, all file name comparisons must be case-insensitive to handle variations likeSkill.mdorSKILL.MD.🛠️ Proposed fix using directory listing with case-insensitive match
- let has_skill_md = - expanded_path.join("SKILL.md").exists() || expanded_path.join("skill.md").exists(); + let has_skill_md = expanded_path + .read_dir() + .ok() + .and_then(|entries| { + entries + .filter_map(|e| e.ok()) + .find(|e| e.file_name().to_string_lossy().eq_ignore_ascii_case("skill.md")) + }) + .is_some();As per coding guidelines: "All file name comparisons must be case-insensitive using
eq_ignore_ascii_casein Rust."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands.rs` around lines 104 - 105, The check for SKILL.md is currently case-sensitive; update the logic that sets has_skill_md to perform a case-insensitive filename match instead of only checking "SKILL.md" and "skill.md". Modify the code around has_skill_md / expanded_path to iterate the directory entries (e.g., std::fs::read_dir(expanded_path) or expanded_path.read_dir()) and set has_skill_md true if any entry.file_name().to_string_lossy().eq_ignore_ascii_case("skill.md"); ensure you handle and ignore any read_dir errors appropriately so the rest of the function (the code referencing has_skill_md) behaves unchanged.
1425-1428:⚠️ Potential issue | 🟡 MinorFile name comparison should be case-insensitive.
The check for
SKILL.mduses exact case matching. To align with coding guidelines, this should use case-insensitive comparison.🛠️ Proposed fix
- let skill_md_path = skill_path.join("SKILL.md"); - - if !skill_md_path.exists() { + let has_skill_md = skill_path + .read_dir() + .ok() + .and_then(|entries| { + entries + .filter_map(|e| e.ok()) + .find(|e| e.file_name().to_string_lossy().eq_ignore_ascii_case("skill.md")) + }) + .is_some(); + + if !has_skill_md {As per coding guidelines: "All file name comparisons must be case-insensitive using
eq_ignore_ascii_casein Rust."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands.rs` around lines 1425 - 1428, The code currently builds skill_md_path = skill_path.join("SKILL.md") and tests its existence with skill_md_path.exists(), which does a case-sensitive check; replace that logic with a case-insensitive filename check by scanning the entries in the skill_path (e.g., using fs::read_dir or iterating dir_entry entries) and matching each entry.file_name().to_string_lossy().eq_ignore_ascii_case("SKILL.md") to determine presence of the SKILL.md file; update the logic around skill_name and the block that used skill_md_path.exists() to use this case-insensitive test instead.
1-5:⚠️ Potential issue | 🔴 CriticalRemove unused
AddAssetKindimport that no longer exists in the CLI module.The
AddAssetKindenum has been removed fromcrate::clibut is still imported on line 3. This will cause a compilation error. The import is also unused—no code incommands.rsreferences it.Fix
use crate::cli::{ - AddArgs, AddAssetKind, CatalogGenerateArgs, InitArgs, ListArgs, ManifestFormat, StatusArgs, + AddArgs, CatalogGenerateArgs, InitArgs, ListArgs, ManifestFormat, StatusArgs, SyncArgs, ValidateArgs, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands.rs` around lines 1 - 5, Remove the obsolete AddAssetKind import from the use crate::cli import list in commands.rs and update the import line to only include existing symbols (AddArgs, CatalogGenerateArgs, InitArgs, ListArgs, ManifestFormat, StatusArgs, SyncArgs, ValidateArgs); search the file for any remaining references to AddAssetKind to confirm none exist and run cargo build to verify the compilation error is resolved.
🧹 Nitpick comments (1)
src/commands.rs (1)
338-341: Empty conditional block reduces readability.The
else ifbranch on lines 338-340 contains only a comment and no code. This can be refactored for clarity.♻️ Suggested refactor to eliminate empty block
if let ParsedAddTarget::GitHubDiscovery { repo_url, git_ref, search_path, } = &target { - if let Some(ref path) = args.path { + // --path overrides discovery: treat as single entry with explicit path + if let Some(ref path) = args.path { target = ParsedAddTarget::GitHubSkill { repo_url: repo_url.clone(), git_ref: git_ref.clone(), skill_path: path.clone(), skill_name: path .rsplit('/') .next() .filter(|s| !s.is_empty()) .map(String::from), }; - } else if search_path.is_empty() && asset_kind == AssetKind::AgentSkill { - // Repo-level agent_skill with no path: keep discovery (will clone to find skills) } + // Note: Repo-level agent_skill with no --path keeps discovery mode }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands.rs` around lines 338 - 341, The else-if branch that checks search_path.is_empty() && asset_kind == AssetKind::AgentSkill contains only a comment, harming readability; remove this empty branch and either fold its intent into the surrounding logic (e.g., include the condition in the previous if/else chain) or replace it with a brief explanatory comment above the relevant if to document "repo-level agent_skill with no path: keep discovery (will clone to find skills)"; update any control flow so the case is still handled consistently without an empty else-if for search_path and AssetKind::AgentSkill.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/commands.rs`:
- Around line 104-105: The check for SKILL.md is currently case-sensitive;
update the logic that sets has_skill_md to perform a case-insensitive filename
match instead of only checking "SKILL.md" and "skill.md". Modify the code around
has_skill_md / expanded_path to iterate the directory entries (e.g.,
std::fs::read_dir(expanded_path) or expanded_path.read_dir()) and set
has_skill_md true if any
entry.file_name().to_string_lossy().eq_ignore_ascii_case("skill.md"); ensure you
handle and ignore any read_dir errors appropriately so the rest of the function
(the code referencing has_skill_md) behaves unchanged.
- Around line 1425-1428: The code currently builds skill_md_path =
skill_path.join("SKILL.md") and tests its existence with skill_md_path.exists(),
which does a case-sensitive check; replace that logic with a case-insensitive
filename check by scanning the entries in the skill_path (e.g., using
fs::read_dir or iterating dir_entry entries) and matching each
entry.file_name().to_string_lossy().eq_ignore_ascii_case("SKILL.md") to
determine presence of the SKILL.md file; update the logic around skill_name and
the block that used skill_md_path.exists() to use this case-insensitive test
instead.
- Around line 1-5: Remove the obsolete AddAssetKind import from the use
crate::cli import list in commands.rs and update the import line to only include
existing symbols (AddArgs, CatalogGenerateArgs, InitArgs, ListArgs,
ManifestFormat, StatusArgs, SyncArgs, ValidateArgs); search the file for any
remaining references to AddAssetKind to confirm none exist and run cargo build
to verify the compilation error is resolved.
---
Nitpick comments:
In `@src/commands.rs`:
- Around line 338-341: The else-if branch that checks search_path.is_empty() &&
asset_kind == AssetKind::AgentSkill contains only a comment, harming
readability; remove this empty branch and either fold its intent into the
surrounding logic (e.g., include the condition in the previous if/else chain) or
replace it with a brief explanatory comment above the relevant if to document
"repo-level agent_skill with no path: keep discovery (will clone to find
skills)"; update any control flow so the case is still handled consistently
without an empty else-if for search_path and AssetKind::AgentSkill.
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation