feat: add aps list command to display manifest entries and resources#76
Merged
Conversation
Adds a new `list` subcommand that displays all manifest entries with their source configuration, destinations, include filters, and sync status. Supports `--assets` flag to show on-disk asset trees for synced entries, with special handling for Agent Skill structure (SKILL.md, scripts/, references/, assets/ directories). https://claude.ai/code/session_01FwPjCdiNPjqA7SuxrHhwEz
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Parser
participant Handler as cmd_list Handler
participant Manifest as Manifest Loader
participant Lockfile as Lockfile Loader
participant Renderer as Output Renderer
CLI->>Handler: Commands::List(ListArgs)
Handler->>Manifest: Load manifest (path or default)
Manifest-->>Handler: Manifest data
Handler->>Lockfile: Load lockfile (if present)
Lockfile-->>Handler: Lockfile data
Handler->>Renderer: Render entries (ID, kind, sources, dests)
Renderer->>Renderer: Format labels, short sources, count synced
alt ListArgs.assets == true
Handler->>Renderer: Read on-disk asset paths
Renderer->>Renderer: Print asset trees (skill or flat)
end
Renderer-->>Handler: Formatted output
Handler-->>CLI: Exit status
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands.rs (1)
1269-1271: Consider reusing the already-loaded lockfile incount_synced_entries.The lockfile is loaded at line 1271, but
count_synced_entries(line 1382) loads it again internally. Consider passing the already-loadedOption<Lockfile>to avoid redundant I/O.♻️ Proposed refactor
-/// Count how many manifest entries are synced (present in lockfile) -fn count_synced_entries(manifest: &Manifest, manifest_path: &Path) -> usize { - let lockfile_path = Lockfile::path_for_manifest(manifest_path); - match Lockfile::load(&lockfile_path) { - Ok(lockfile) => manifest +/// Count how many manifest entries are synced (present in lockfile) +fn count_synced_entries(manifest: &Manifest, lockfile: Option<&Lockfile>) -> usize { + match lockfile { + Some(lf) => manifest .entries .iter() - .filter(|e| lockfile.entries.contains_key(&e.id)) + .filter(|e| lf.entries.contains_key(&e.id)) .count(), - Err(_) => 0, + None => 0, } }Then update the call site:
- let synced_count = count_synced_entries(&manifest, &manifest_path); + let synced_count = count_synced_entries(&manifest, lockfile.as_ref());Also applies to: 1382-1382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands.rs` around lines 1269 - 1271, The code loads a Lockfile into the local variable lockfile but count_synced_entries still reloads it; modify count_synced_entries to accept the already-loaded Option<Lockfile> (e.g., change signature to count_synced_entries(..., lockfile: Option<Lockfile>) or &Option<Lockfile>) and update the call site to pass the local lockfile (created from Lockfile::load(&lockfile_path).ok()); inside count_synced_entries remove the internal Lockfile::load fallback and use the provided Option instead (or clone/borrow as needed), ensuring types and ownership are adjusted accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands.rs`:
- Around line 1575-1582: The println! invocation formatting in src/commands.rs
(the println! that uses indent, dim.apply_to(connector),
dir_style.apply_to(&*name_str), dim.apply_to("/"), and dim.apply_to(format!("({}
items)", child_count))) doesn't match rustfmt rules; run cargo fmt (or rustfmt)
and commit the reformatted file so the println! macro and its arguments are
reformatted to satisfy CI.
- Around line 1523-1531: The match block that assigns sub_entries (the
std::fs::read_dir(item.path()) match) is not formatted per rustfmt; run `cargo
fmt` (or apply rustfmt) to reformat the match expression and surrounding code so
the items vector creation, sort_by_key, and Err(_) arm follow project style;
ensure the binding name `sub_entries`, the `read_dir(item.path())` call, and the
`items.sort_by_key(|e| e.file_name())` line remain unchanged semantically while
fixing whitespace/indentation to satisfy CI.
---
Nitpick comments:
In `@src/commands.rs`:
- Around line 1269-1271: The code loads a Lockfile into the local variable
lockfile but count_synced_entries still reloads it; modify count_synced_entries
to accept the already-loaded Option<Lockfile> (e.g., change signature to
count_synced_entries(..., lockfile: Option<Lockfile>) or &Option<Lockfile>) and
update the call site to pass the local lockfile (created from
Lockfile::load(&lockfile_path).ok()); inside count_synced_entries remove the
internal Lockfile::load fallback and use the provided Option instead (or
clone/borrow as needed), ensuring types and ownership are adjusted accordingly.
There was a problem hiding this comment.
Pull request overview
This pull request adds a new aps list command that provides a detailed view of manifest entries, including their source configuration, destinations, include filters, and sync status. The implementation supports an --assets flag that displays on-disk asset trees for synced entries, with special handling for Agent Skill structures (SKILL.md, scripts/, references/, assets/ directories).
Changes:
- Added
Listcommand variant to CLI withListArgsstruct supporting--manifestand--assetsflags - Implemented
cmd_listfunction that displays all manifest entries with formatted output including source info, destinations, filters, and sync status - Added helper functions for formatting (
format_kind_label,format_source_short) and asset tree visualization (print_asset_tree,print_skill_tree,print_single_skill_contents,print_flat_tree)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/cli.rs | Added List command variant and ListArgs struct with manifest path and assets flag options |
| src/main.rs | Imported cmd_list and added command routing for the new List command |
| src/commands.rs | Implemented the complete cmd_list command with manifest parsing, styled output formatting, and asset tree visualization logic |
Installs Rust toolchain with rustfmt and clippy components, then pre-builds dependencies so that cargo fmt, clippy, build, and test run quickly during remote sessions. https://claude.ai/code/session_01FwPjCdiNPjqA7SuxrHhwEz
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands.rs (1)
1382-1383: Reuse the already-loaded lockfile for summary counts.Line 1271 already loads the lockfile, but Line 1452 loads it again via
count_synced_entries. This duplicates I/O and can produce an inconsistent snapshot if the lockfile changes during execution.Refactor sketch
- let synced_count = count_synced_entries(&manifest, &manifest_path); + let synced_count = count_synced_entries(&manifest, lockfile.as_ref());-fn count_synced_entries(manifest: &Manifest, manifest_path: &Path) -> usize { - let lockfile_path = Lockfile::path_for_manifest(manifest_path); - match Lockfile::load(&lockfile_path) { - Ok(lockfile) => manifest - .entries - .iter() - .filter(|e| lockfile.entries.contains_key(&e.id)) - .count(), - Err(_) => 0, - } +fn count_synced_entries(manifest: &Manifest, lockfile: Option<&Lockfile>) -> usize { + match lockfile { + Some(lockfile) => manifest + .entries + .iter() + .filter(|e| lockfile.entries.contains_key(&e.id)) + .count(), + None => 0, + } }Also applies to: 1452-1461
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands.rs` around lines 1382 - 1383, The summary currently reloads the lockfile inside count_synced_entries, causing duplicate I/O and a potential inconsistent snapshot; change count_synced_entries to accept the already-loaded manifest (the existing manifest variable) and compute synced_count from that in-memory structure rather than re-reading manifest_path, and update all call sites (where count_synced_entries(&manifest, &manifest_path) is used) to pass the manifest reference only (or call a new helper like count_synced_entries_from_manifest) so no second lockfile read happens and the summary uses the same snapshot as earlier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands.rs`:
- Around line 1492-1494: The check that detects a single-skill directory only
matches uppercase "SKILL.md"; update the detection for has_skill_md to perform a
case-insensitive filename comparison (e.g., use
file_name().to_string_lossy().to_ascii_lowercase() or eq_ignore_ascii_case) so
that "skill.md" is treated the same as "SKILL.md"; modify the
items.iter().any(...) expression that sets has_skill_md accordingly to handle
both cases.
- Around line 1367-1371: The status printing only handles the synced case (if
lf.entries.contains_key(&entry.id)) and leaves unsynced entries without any
status; update the conditional around lockfile handling in commands.rs (the
block using lockfile / lf / entries.contains_key(&entry.id) and the println that
prints green "● synced") to include an else branch that prints an explicit
pending status for unsynced entries (e.g., a different symbol and a
yellow.apply_to("pending") message) so every entry gets a per-entry status line.
---
Nitpick comments:
In `@src/commands.rs`:
- Around line 1382-1383: The summary currently reloads the lockfile inside
count_synced_entries, causing duplicate I/O and a potential inconsistent
snapshot; change count_synced_entries to accept the already-loaded manifest (the
existing manifest variable) and compute synced_count from that in-memory
structure rather than re-reading manifest_path, and update all call sites (where
count_synced_entries(&manifest, &manifest_path) is used) to pass the manifest
reference only (or call a new helper like count_synced_entries_from_manifest) so
no second lockfile read happens and the summary uses the same snapshot as
earlier.
Comment on lines
+1367
to
+1371
| if let Some(ref lf) = lockfile { | ||
| if lf.entries.contains_key(&entry.id) { | ||
| println!(" {} {}", green.apply_to("●"), green.apply_to("synced")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Show explicit pending status for unsynced entries.
Line 1367 only prints a status when an entry is in the lockfile; unsynced entries end up with no per-entry status line, which makes status ambiguous.
Proposed fix
- if let Some(ref lf) = lockfile {
- if lf.entries.contains_key(&entry.id) {
- println!(" {} {}", green.apply_to("●"), green.apply_to("synced"));
- }
- }
+ if let Some(ref lf) = lockfile {
+ if lf.entries.contains_key(&entry.id) {
+ println!(" {} {}", green.apply_to("●"), green.apply_to("synced"));
+ } else {
+ println!(" {} {}", yellow.apply_to("●"), yellow.apply_to("pending"));
+ }
+ } else {
+ println!(" {} {}", yellow.apply_to("●"), yellow.apply_to("pending"));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands.rs` around lines 1367 - 1371, The status printing only handles
the synced case (if lf.entries.contains_key(&entry.id)) and leaves unsynced
entries without any status; update the conditional around lockfile handling in
commands.rs (the block using lockfile / lf / entries.contains_key(&entry.id) and
the println that prints green "● synced") to include an else branch that prints
an explicit pending status for unsynced entries (e.g., a different symbol and a
yellow.apply_to("pending") message) so every entry gets a per-entry status line.
The summary section was re-reading the lockfile from disk via count_synced_entries, duplicating the I/O done earlier in cmd_list. Now computes synced_count directly from the in-memory lockfile loaded at the top of the function, and removes the unused helper. https://claude.ai/code/session_01FwPjCdiNPjqA7SuxrHhwEz
Uses eq_ignore_ascii_case for both the skill directory detection and the SKILL.md highlighting in the asset tree display, consistent with the case-insensitive detection in discover.rs. https://claude.ai/code/session_01FwPjCdiNPjqA7SuxrHhwEz
Updates AGENTS.md to match the structure aps sync would produce when composing from the two agentically partials (AGENTS.rust.md + AGENTS.pull-requests.md), with the new case-insensitive file name checking guideline included in the Rust section. https://claude.ai/code/session_01FwPjCdiNPjqA7SuxrHhwEz
westonplatter
pushed a commit
that referenced
this pull request
Feb 26, 2026
The last PR (#76) added the `aps list` command but the README wasn't updated. Add it to the commands table and document its `--assets` option. https://claude.ai/code/session_018aeZn9BGiJ8MXcdribNi3P
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
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.
Adds a new
listsubcommand that displays all manifest entries with theirsource configuration, destinations, include filters, and sync status.
Supports
--assetsflag to show on-disk asset trees for synced entries,with special handling for Agent Skill structure (SKILL.md, scripts/,
references/, assets/ directories).
https://claude.ai/code/session_01FwPjCdiNPjqA7SuxrHhwEz
Summary by CodeRabbit