feat(cli): add list command for managed prompts#9
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Determine paths (default to home directory) | ||
| actual_target_path = target_path if target_path is not None else Path.home() | ||
| actual_detection_path = detection_path if detection_path is not None else Path.home() |
There was a problem hiding this comment.
The new list command claims to mirror generate flag behaviour, but detection defaults to the home directory regardless of --target-path. When users store prompts under a custom base (e.g. --target-path /tmp/prompts) and omit --detection-path—which is the workflow supported by generate—agent discovery runs against Path.home() and exits with “No agents detected”, even though the prompts live under the supplied target path. This makes listing managed prompts fail for any installation that uses a non‑home target path unless users also remember to pass --detection-path explicitly. The detection base should fall back to the target path just as the generation flow does.
Useful? React with 👍 / 👎.
| else: | ||
| selected_agents = agents | ||
|
|
||
| # Discover managed prompts | ||
| discovered = discover_managed_prompts(actual_target_path, selected_agents) |
There was a problem hiding this comment.
When agents are supplied via --agent, the code passes the raw list to discover_managed_prompts without validation. Any typo (e.g. --agent cusr) causes get_agent_config inside the discovery routine to raise KeyError, which bubbles up as an uncaught traceback. Other commands (such as generate) catch this error and show a friendly message. The list command should perform the same validation or wrap the call in a try/except so the CLI exits cleanly and informs the user which keys are supported.
Useful? React with 👍 / 👎.
ryderstorm
force-pushed
the
feat/add-list-command
branch
2 times, most recently
from
71c3d24 to
67a32e6
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as list_cmd
participant Utils as cli_utils
participant Discover as list_discovery
participant Builder as builder
participant Renderer as renderer
User->>CLI: run `slash-man list` (flags: --agent / --target-path / --detection-path / --all-files)
CLI->>Utils: find_project_root(), display_local_path(), relative_to_candidates()
CLI->>CLI: detect/validate agents
alt --all-files
CLI->>Discover: discover_all_files(target_path, agents)
Discover-->>CLI: files_by_agent + directory_status
CLI->>Renderer: render_all_files_tables(files_by_agent, target_path, directory_status)
else managed prompts flow
CLI->>Discover: discover_managed_prompts(detection_path/target_path, agents)
Discover-->>CLI: discovered_prompts
CLI->>Discover: count_unmanaged_prompts(...)
Discover-->>CLI: unmanaged_counts
CLI->>Builder: build_list_data_structure(discovered_prompts, unmanaged_counts)
Builder-->>CLI: structured_data
CLI->>Renderer: render_list_tree(structured_data)
end
Renderer-->>CLI: rendered output
CLI-->>User: print output / exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)slash_commands/cli.py (4)
slash_commands/list_discovery.py (3)
🪛 LanguageTooldocs/specs/08-spec-list-all-files-flag/08-proofs/08-task-01-proofs.md[style] ~36-~36: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE) [style] ~38-~38: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE) [style] ~40-~40: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE) [style] ~42-~42: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE) docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-02-proofs.md[style] ~43-~43: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE) [style] ~45-~45: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE) [style] ~47-~47: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
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 (8)
docs/specs/07-spec-list-command/07-questions-1-list-command.md (1)
3-3: Minor style suggestion: replace "Feel free" with more formal phrasing.The opening instruction uses "Feel free," which can be made more concise and professional in a specification document. Consider: "Please answer each question below (select one or more options or add notes)."
tests/integration/test_generate_command.py (1)
5-10: Managed-by integration test is solid; consider reusing shared frontmatter parsingThe new test does a good job asserting
managed_by == "slash-man"for both Markdown and TOML outputs and nicely exercises the real CLI flow.The only minor concern is the manual Markdown frontmatter parsing via
md_content.split("---"), which is a bit brittle if the format ever changes. If you already have a shared helper (e.g., the same frontmatter/TOML parser used in production discovery), reusing it here would keep tests and runtime behavior aligned and reduce duplication, but this is purely optional.Also applies to: 332-392
docs/specs/07-spec-list-command/07-spec-list-command.md (1)
51-55: Minor docs polish: capitalize “GitHub” in examplesIn the source-format examples (
github: owner/repo@branch:path), consider capitalizing toGitHubfor consistency with the rest of the docs and the official product name. Purely stylistic, but it keeps the spec tidy.Also applies to: 175-179
slash_commands/cli.py (1)
785-863: Consider showing unmanaged counts even when no managed prompts are found
list_cmdcurrently exits early whendiscover_managed_promptsreturns an empty list, printing the “No managed prompts found…” message and returning with code 0. That meanscount_unmanaged_promptsis never called in an all‑unmanaged installation, so users can’t see how many unmanaged prompt files they have even though the spec emphasizes unmanaged counts per agent directory.If you want to fully realize FR 8, consider computing
unmanaged_countsand rendering the tree even whendiscoveredis empty (the renderer already shows “No managed prompts found” for an empty prompts dict), and optionally still print the explanatory message aboutmanaged_by. That would let users on older installations see unmanaged counts while keeping the current empty‑state guidance.slash_commands/cli_utils.py (1)
10-47: Makefind_project_roottruly “robust” and avoid repeated root discoveryCurrent behavior caps the upward search at 10 levels, which can miss the actual project root in deeply nested layouts despite already having a safe
parent == currentbreak. You can simplify and strengthen this logic by removing the depth limit and looping until the filesystem root. Also,display_local_pathrecomputes the project root on each call; if this is used in hot paths (e.g., many prompts), consider cachingfind_project_root()or passing in precomputed candidates to avoid repeated filesystem scans.Also applies to: 50-86
tests/test_list_discovery.py (1)
147-187: Tighten test clarity and consider direct coverage for_is_backup_fileThe tests are comprehensive overall. Two minor polish points:
- The comment in
test_build_list_data_structure_includes_all_fieldsmentions mockingcount_backupsandformat_source_info, but the test now uses the real implementations; updating the comment would avoid confusion for future maintainers.- Backup handling is currently only verified indirectly (via glob patterns and
count_backups). If you want stronger guarantees against regressions in the backup filename heuristic, consider a small unit test that exercises_is_backup_filedirectly with a few representative filenames.Also applies to: 349-389, 814-860
tests/integration/test_list_command.py (1)
64-110: Consider avoidingsleep(1)by using fixed backup timestamps
test_list_shows_backup_countsis correct, but it relies on real timestamps andtime.sleep(1)solely to produce two distinct backup filenames. You could instead write two backups with hard-coded, different timestamps (as in the unit tests) to keep the suite a bit faster and less dependent on wall‑clock time, while still validating thatcount_backupsrecognizes the{filename}.{extension}.YYYYMMDD-HHMMSS.bakpattern.slash_commands/list_discovery.py (1)
69-87: Backup detection is slightly redundant but consistent
_is_backup_filecorrectly recognizes the{filename}.{extension}.YYYYMMDD-HHMMSS.bakpattern, andcount_backupsseparately re-implements a compatible regex. Because discovery/counting useglob(f"*{agent.command_file_extension}"), the_is_backup_filecheck indiscover_managed_prompts/count_unmanaged_promptsis effectively redundant today (those backups won’t match the glob). This isn’t wrong, just a bit duplicative. If you ever broaden the glob to*or similar, the helper will become essential; otherwise, you could simplify by relying solely on the glob or, conversely, by using_is_backup_fileinsidecount_backupsto keep the pattern logic in one place.Also applies to: 208-241
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.markdownlint.yaml(1 hunks)README.md(6 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-01-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-02-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-03-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-04-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-05-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-06-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-questions-1-list-command.md(1 hunks)docs/specs/07-spec-list-command/07-spec-list-command.md(1 hunks)docs/specs/07-spec-list-command/07-tasks-list-command.md(1 hunks)docs/specs/07-spec-list-command/07-validation-list-command.md(1 hunks)slash_commands/cli.py(4 hunks)slash_commands/cli_utils.py(1 hunks)slash_commands/generators.py(2 hunks)slash_commands/list_discovery.py(1 hunks)tests/integration/test_generate_command.py(2 hunks)tests/integration/test_list_command.py(1 hunks)tests/test_generators.py(1 hunks)tests/test_list_discovery.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/integration/test_list_command.py (3)
slash_commands/cli_utils.py (1)
format_source_info(88-129)slash_commands/list_discovery.py (5)
build_list_data_structure(243-300)count_backups(209-240)count_unmanaged_prompts(160-206)discover_managed_prompts(24-66)render_list_tree(303-378)tests/integration/conftest.py (3)
get_slash_man_command(81-89)temp_test_dir(28-35)test_prompts_dir(39-45)
tests/test_list_discovery.py (2)
slash_commands/list_discovery.py (6)
count_unmanaged_prompts(160-206)discover_managed_prompts(24-66)_parse_command_file(89-109)count_backups(209-240)build_list_data_structure(243-300)render_list_tree(303-378)slash_commands/cli_utils.py (1)
format_source_info(88-129)
tests/integration/test_generate_command.py (1)
tests/integration/conftest.py (3)
temp_test_dir(28-35)test_prompts_dir(39-45)get_slash_man_command(81-89)
slash_commands/list_discovery.py (3)
mcp_server/prompt_utils.py (1)
parse_frontmatter(88-102)slash_commands/cli_utils.py (1)
format_source_info(88-129)slash_commands/config.py (2)
AgentConfig(18-31)get_agent_config(102-108)
tests/test_generators.py (3)
tests/conftest.py (1)
sample_prompt(103-146)slash_commands/config.py (1)
get_agent_config(102-108)slash_commands/generators.py (6)
MarkdownCommandGenerator(155-234)_build_meta(206-234)TomlCommandGenerator(237-293)generate(27-33)generate(158-199)generate(240-289)
slash_commands/cli.py (4)
slash_commands/cli_utils.py (3)
display_local_path(50-66)find_project_root(10-47)relative_to_candidates(69-85)slash_commands/list_discovery.py (4)
build_list_data_structure(243-300)count_unmanaged_prompts(160-206)discover_managed_prompts(24-66)render_list_tree(303-378)slash_commands/detection.py (1)
detect_agents(11-25)slash_commands/config.py (1)
list_agent_keys(96-99)
🪛 LanguageTool
docs/specs/07-spec-list-command/07-spec-list-command.md
[style] ~19-~19: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...oper troubleshooting prompt issues**, I want to see backup counts and last updated time...
(REP_WANT_TO_VB)
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...and how many recovery points exist. - As a user upgrading from older versions,...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...anism and can regenerate if needed. - **As a user managing a mix of managed and un...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~53-~53: The official name of this software platform is spelled with a capital “H”.
Context: ...ne (e.g., "local: /path/to/prompts" or "github: owner/repo@branch:path"). Unmanaged pr...
(GITHUB)
docs/specs/07-spec-list-command/07-proofs/07-task-03-proofs.md
[uncategorized] ~56-~56: The official name of this software platform is spelled with a capital “H”.
Context: ...e_path`) - ✅ GitHub source formatting: "github: owner/repo@branch:path" - ✅ Missing fi...
(GITHUB)
[uncategorized] ~176-~176: The official name of this software platform is spelled with a capital “H”.
Context: ...to source_path) - GitHub sources: "github: owner/repo@branch:path" (handles missi...
(GITHUB)
[uncategorized] ~234-~234: The official name of this software platform is spelled with a capital “H”.
Context: ...mpts" 6. ✅ GitHub sources show format: "github: owner/repo@branch:path" 7. ✅ Missing s...
(GITHUB)
docs/specs/07-spec-list-command/07-tasks-list-command.md
[style] ~91-~91: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... works correctly - TOML parsing works correctly - Empty directories are handled grace...
(ADVERB_REPETITION_PREMIUM)
[uncategorized] ~141-~141: The official name of this software platform is spelled with a capital “H”.
Context: ... - GitHub sources show format (e.g., "github: owner/repo@branch:path") - Test GitHub...
(GITHUB)
[uncategorized] ~173-~173: The official name of this software platform is spelled with a capital “H”.
Context: ...b_source()- formats GitHub source as "github: owner/repo@branch:path" usingmeta.so...
(GITHUB)
docs/specs/07-spec-list-command/07-questions-1-list-command.md
[style] ~3-~3: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...r more options, or add your own notes). Feel free to add additional context under any questi...
(FEEL_FREE_TO_STYLE_ME)
🔇 Additional comments (14)
.markdownlint.yaml (1)
22-23: Clear and well-motivated lint configuration.The addition of MD051 disable with explanatory comment follows the established pattern in the file and provides useful context for future maintainers regarding GitHub's automatic emoji handling in heading IDs.
docs/specs/07-spec-list-command/07-questions-1-list-command.md (1)
1-119: Well-structured specification document with clear decision guidance.This specification document is well-organized and comprehensive, covering all major implementation decisions for the list command feature. The preferred answers (bolded options) are clearly marked and appear consistent with the PR implementation:
- Automatic
managed_bymetadata addition in the generate command (Q1)- Reusable code consolidation to minimize duplication (Q11)
- Comprehensive per-prompt information display (Q4)
- Proper backward compatibility handling—only listing prompts with the metadata field (Q10)
- Rich tree rendering aligned with existing generate output (Q3)
The specification appropriately addresses edge cases, such as the empty-state guidance in Q8, which helpfully notes the detection limitations for users.
docs/specs/07-spec-list-command/07-validation-list-command.md (1)
1-291: Well-documented validation report.This comprehensive validation report provides clear traceability from requirements to implementation through test coverage, proof artifacts, and git commits. The 100% pass rate across all validation gates demonstrates thorough implementation and testing.
docs/specs/07-spec-list-command/07-proofs/07-task-03-proofs.md (1)
1-256: Comprehensive proof artifacts for backup counting and source metadata.The documentation clearly demonstrates the implementation with test results, code examples, and CLI transcripts. The backup pattern matching aligns with the existing writer.py implementation, ensuring consistency.
docs/specs/07-spec-list-command/07-proofs/07-task-04-proofs.md (1)
1-109: Well-documented proof artifacts for Rich tree rendering.The proof artifacts demonstrate comprehensive test coverage for the tree structure rendering, including all key display elements (agent info, file paths, backup counts, source info, timestamps, and unmanaged counts).
README.md (1)
3-339: Excellent README updates.The documentation improvements add clarity with better structure, comprehensive command examples, and the new list command documentation. The version management section and improved quick start guide enhance usability.
slash_commands/generators.py (2)
206-234: Clean metadata addition.The
managed_byfield is properly integrated into the metadata structure, enabling the list command to discover and display managed prompts. The implementation is consistent and well-tested.
240-289: Consistent TOML generator implementation.The TOML generator correctly includes the
managed_byfield in the meta section, maintaining consistency with the Markdown generator approach.tests/test_generators.py (1)
309-330: Appropriate test coverage for new metadata field.The two new tests properly validate that the
managed_byfield is included in both Markdown and TOML generator outputs. The test implementation is clean and follows existing patterns.docs/specs/07-spec-list-command/07-proofs/07-task-01-proofs.md (1)
1-173: Clear proof artifacts for managed_by metadata field.The proof documentation effectively demonstrates the metadata field implementation with CLI output examples, generated file content, and test results for both Markdown and TOML formats.
docs/specs/07-spec-list-command/07-proofs/07-task-05-proofs.md (1)
1-174: Thorough CLI flag and empty state proof artifacts.The proof documentation comprehensively demonstrates all CLI flags (--agent, --target-path, --detection-path) with clear examples and verification notes. The empty state handling provides helpful user guidance.
docs/specs/07-spec-list-command/07-proofs/07-task-02-proofs.md (1)
71-150: Proof transcript aligns with implemented list behaviorThe generate/list CLI transcript matches the current tree rendering (source line, updated timestamp, per‑agent backup text, and an empty Unmanaged Prompts branch when counts are zero), so this proof looks accurate and sufficient.
slash_commands/cli.py (1)
27-31: Shared CLI utilities wiring looks correctImporting
find_project_root,display_local_path, andrelative_to_candidatesfromcli_utilsand aliasing_find_project_root/_display_local_pathpreserves existing internal usage while centralizing the logic. The_relative_to_candidatesbinding inside_build_summary_datakeeps the summary logic consistent with the new shared helper without changing behavior.Also applies to: 74-78, 106-107
docs/specs/07-spec-list-command/07-tasks-list-command.md (1)
1-358: Spec is thorough and aligned with the implemented behaviorThe task breakdown, demo criteria, and proof artifacts line up well with the current
cli_utilsandlist_discoveryimplementations and tests; any remaining issues are minor style/wording nits and not blockers.
| **Diff Statistics:** | ||
|
|
||
| ```bash | ||
| git show 9dc19bd --stat | ||
| ``` | ||
|
|
||
| Output: | ||
|
|
||
| ```text | ||
| slash_commands/cli.py | 70 ++++----------------- | ||
| slash_commands/cli_utils.py | 129 +++++++++++++++++++++++++++++++++++++++ | ||
| slash_commands/list_discovery.py | 45 +------------- | ||
| 3 files changed, 141 insertions(+), 103 deletions(-) | ||
| ``` | ||
|
|
||
| **Net Code Reduction:** 38 lines removed (103 deletions - 141 insertions = -38, but new file adds 129 lines for reusability) | ||
|
|
||
| **Code Consolidation:** |
There was a problem hiding this comment.
Fix inverted “Net Code Reduction” arithmetic in proof doc
The “Net Code Reduction: 38 lines removed (103 deletions - 141 insertions = -38…)” statement has the sign flipped: 141 insertions and 103 deletions actually mean a net increase of 38 lines. Suggest rephrasing to something like “Net change: +38 lines (141 insertions, 103 deletions)” or dropping the reduction wording to avoid confusion.
🤖 Prompt for AI Agents
docs/specs/07-spec-list-command/07-proofs/07-task-06-proofs.md lines 28-45: the
"Net Code Reduction" arithmetic is inverted — it reports "38 lines removed (103
deletions - 141 insertions = -38...)" when 141 insertions and 103 deletions are
actually a net increase of 38 lines; update the wording to correctly reflect a
net change (e.g., "Net change: +38 lines (141 insertions, 103 deletions)" or
remove "reduction" and state the net increase) and adjust the parenthetical
arithmetic to match the corrected sign.
Add comprehensive specification for the new `list` command feature, including questions file for requirements gathering and detailed specification document covering managed prompt discovery, unmanaged prompt counting, source consolidation, and code consolidation opportunities.
Add comprehensive task breakdown document with 6 parent tasks and 111 sub-tasks covering: - managed_by field integration - prompt discovery and filtering - backup counting and source metadata extraction - Rich output display - CLI command with flags and empty state handling - DRY refactoring and shared utilities extraction Includes testing notes, relevant files list, function signatures, data structures, and error handling scenarios. All tasks follow TDD workflow principles with clear commit boundaries.
- Add managed_by: slash-man to Markdown generator metadata - Add managed_by: slash-man to TOML generator metadata - Add unit tests for managed_by field - Add integration test verifying managed_by in generated files - Create proof artifacts demonstrating managed_by field Related to T1.0 in Spec 07
- Add discover_managed_prompts() function to scan agent command directories - Support both Markdown and TOML format parsing - Filter for files with managed_by: slash-man metadata - Exclude backup files and files without managed_by field - Handle empty directories and multiple agents gracefully - Add comprehensive unit tests for discovery logic Related to T2.0 in Spec 07
- Add count_unmanaged_prompts() function - Count valid prompt files without managed_by field - Exclude backup files, managed files, and invalid files - Add comprehensive unit tests Related to T2.0 in Spec 07
- Add tests for malformed frontmatter handling - Add tests for Unicode decode error handling - Add tests for permission error handling - Verify errors are handled gracefully per spec assumption Related to T2.0 in Spec 07
- Add note to task 2.24 about CLI command dependency - Create proof artifact showing unit test results for task 2.0 - Document all 14 passing unit tests - Note pending proof artifacts that require CLI command Related to T2.0 in Spec 07
- Add integration test for prompt discovery across multiple agents - Test creates managed prompts using generate command - Verifies discovery function finds prompts from both agents - Confirms all discovered prompts have managed_by field Related to T2.19-T2.20 in Spec 07
- Add count_backups() function to count backup files
- Counts files matching pattern filename.{extension}.{timestamp}.bak
- Validates timestamp format YYYYMMDD-HHMMSS
- Add unit tests for backup counting
Related to T3.1-T3.4 in Spec 07
- Add integration test for backup counting - Add integration test for source info formatting - Tests verify functions work correctly with generated prompts Related to T3.9-T3.12 in Spec 07
- Add build_list_data_structure() function to group prompts by name - Aggregate agent information per prompt - Include backup counts, source info, and timestamps - Add unit tests for data structure building Related to T4.1-T4.4 in Spec 07
…data - Add comprehensive proof artifacts for Task 3.0 - Document unit tests for backup counting and source formatting - Document integration tests - Include function usage examples - Mark Task 3.0 as complete (3.13 depends on Task 5.0) Related to T3.0 in Spec 07
- Add render_list_tree() function using Rich Tree format - Groups prompts by name (not by agent) - Shows agents, file paths, backup counts, source info, timestamps - Shows unmanaged prompt counts per agent - Matches generate command summary style - Add 8 unit tests for Rich rendering Related to T4.5-T4.8 in Spec 07
- Add integration test for list output structure - Verifies Rich tree format matches expected structure - Tests with multiple agents and local sources - Verifies prompts grouped by name, source info, agents shown Related to T4.9-T4.10 in Spec 07
- Add list command with --agent, --target-path, --detection-path flags - Implement agent detection and empty state handling - Call discovery functions and render Rich tree output - Add integration test for basic list command execution Related to T5.1-T5.4 in Spec 07
- Add test for --agent flag filtering - Add test for --target-path flag - Add test for --detection-path flag - Add test for multiple --agent flags - Add test for empty state handling - All flags already implemented in basic command Related to T5.5-T5.22 in Spec 07
- Integration tests verify all flags work correctly - Empty state handling verified - Mark Task 5.0 as complete (5.25 depends on proof artifacts) Related to T5.23-T5.24 in Spec 07
- Extract path resolution utilities (find_project_root, display_local_path, relative_to_candidates) - Extract source metadata formatting utility (format_source_info) - Update generate and list commands to use shared utilities - Update tests to import from new location - Maintain backward compatibility with aliases Related to T6.1-T6.22 in Spec 07
- Extracted 4 shared utilities: path resolution (3 functions), source formatting - All tests pass, functionality maintained - Mark Task 6.0 as complete Related to T6.23-T6.27 in Spec 07
- Add Task 2.24: CLI transcript for prompt discovery - Add Task 3.13: CLI transcripts for backup counts and source display - Add Task 4.11-4.12: CLI transcripts for Rich tree output - Add Task 5.25: CLI transcripts for flag usage and empty state - All proof artifacts demonstrate working functionality - Fix markdown linting errors Related to T2.24, T3.13, T4.11-T4.12, T5.25 in Spec 07
- CLI transcript proof artifact created in 07-task-02-proofs.md - Task 2.0 now fully complete Related to T2.24 in Spec 07
…ction - Code review showing 4 extracted shared utilities - Unit and integration test results - Code reduction and consolidation metrics - Verification checklist confirming DRY principles applied Related to T6.0 in Spec 07
- Fix TOML file parsing to extract prompt name from meta.source_prompt instead of attempting to parse the prompt field - Reorganize imports in test files to follow code style - Add validation document for spec 07 list command feature
…nt keys - Update actual_detection_path to cascade from detection_path → target_path → home directory - Add try-except handler for KeyError exceptions from invalid agent keys - Provide user-friendly error messages with guidance on valid agent keys - Exit with code 2 for validation errors (invalid agent key)
- Add professional header with badges (license, Python version, status) - Reorganize with clear table of contents and visual hierarchy - Enhance quick start section for reduced friction - Improve feature descriptions with use cases - Add emoji indicators for better visual scanning - Create clearer CLI commands documentation with examples - Improve supported tools documentation with table format - Better organization of version management section - Enhance development and testing documentation - Add footer with call-to-action links - Update .markdownlint.yaml to disable MD051 for GitHub link fragments
Replace loose string length and character validation with strict regex pattern matching for YYYYMMDD-HHMMSS timestamp format. This improves code clarity and ensures more robust validation.
ryderstorm
force-pushed
the
feat/add-list-command
branch
from
67a32e6 to
c803594
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/specs/07-spec-list-command/07-proofs/07-task-06-proofs.md (1)
43-43: Fix inverted net code change arithmetic.The statement "38 lines removed (103 deletions - 141 insertions = -38...)" has inverted arithmetic. With 141 insertions and 103 deletions, the net change is +38 lines (an increase, not a reduction). Consider rephrasing to "Net change: +38 lines (141 insertions, 103 deletions)" to accurately reflect the addition of the new utilities module.
slash_commands/list_discovery.py (1)
112-157: Validate thatmetais a dict before using.get()methods.As flagged in a previous review, both
_parse_markdown_fileand_parse_toml_fileassumemetais a dict without validation. If a user has a syntactically valid file wheremetais not a mapping (e.g.,meta: "some-string"in YAML ormeta = "oops"in TOML), callingmeta.get(...)will raiseAttributeError. This can crash the list command when scanning user directories, violating the spec's goal to "skip malformed files silently."Add type validation in both parsers:
def _parse_markdown_file( file_path: Path, content: str, agent: AgentConfig ) -> dict[str, Any] | None: """Parse Markdown command file with frontmatter.""" try: frontmatter, _body = parse_frontmatter(content) if not frontmatter: return None name = frontmatter.get("name") or file_path.stem - meta = frontmatter.get("meta") or {} + raw_meta = frontmatter.get("meta") or {} + if not isinstance(raw_meta, dict): + return None + meta = raw_meta return { "name": name, "agent": agent.key, "agent_display_name": agent.display_name, "file_path": file_path, "meta": meta, "format": "markdown", } except yaml.YAMLError: return NoneApply the same fix to
_parse_toml_file:def _parse_toml_file(file_path: Path, content: str, agent: AgentConfig) -> dict[str, Any] | None: """Parse TOML command file.""" try: data = tomllib.loads(content) if not isinstance(data, dict): return None - meta = data.get("meta") or {} + raw_meta = data.get("meta") or {} + if not isinstance(raw_meta, dict): + return None + meta = raw_meta # Extract name from meta.source_prompt (where generator stores it) or use filename name = meta.get("source_prompt") or file_path.stem return { "name": name, "agent": agent.key, "agent_display_name": agent.display_name, "file_path": file_path, "meta": meta, "format": "toml", } except tomllib.TOMLDecodeError: return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.markdownlint.yaml(1 hunks)README.md(6 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-01-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-02-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-03-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-04-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-05-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-06-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-questions-1-list-command.md(1 hunks)docs/specs/07-spec-list-command/07-spec-list-command.md(1 hunks)docs/specs/07-spec-list-command/07-tasks-list-command.md(1 hunks)docs/specs/07-spec-list-command/07-validation-list-command.md(1 hunks)slash_commands/cli.py(4 hunks)slash_commands/cli_utils.py(1 hunks)slash_commands/generators.py(2 hunks)slash_commands/list_discovery.py(1 hunks)tests/integration/test_generate_command.py(2 hunks)tests/integration/test_list_command.py(1 hunks)tests/test_generators.py(1 hunks)tests/test_list_discovery.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/specs/07-spec-list-command/07-validation-list-command.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .markdownlint.yaml
- docs/specs/07-spec-list-command/07-proofs/07-task-05-proofs.md
- docs/specs/07-spec-list-command/07-proofs/07-task-04-proofs.md
- slash_commands/generators.py
🧰 Additional context used
🧬 Code graph analysis (6)
tests/test_generators.py (3)
tests/conftest.py (1)
sample_prompt(103-146)slash_commands/config.py (1)
get_agent_config(102-108)slash_commands/generators.py (6)
MarkdownCommandGenerator(155-234)_build_meta(206-234)TomlCommandGenerator(237-293)generate(27-33)generate(158-199)generate(240-289)
slash_commands/cli.py (4)
slash_commands/cli_utils.py (3)
display_local_path(50-66)find_project_root(10-47)relative_to_candidates(69-85)slash_commands/list_discovery.py (4)
build_list_data_structure(243-300)count_unmanaged_prompts(160-206)discover_managed_prompts(24-66)render_list_tree(303-378)slash_commands/detection.py (1)
detect_agents(11-25)slash_commands/config.py (1)
list_agent_keys(96-99)
tests/test_list_discovery.py (2)
slash_commands/list_discovery.py (6)
count_unmanaged_prompts(160-206)discover_managed_prompts(24-66)_parse_command_file(89-109)count_backups(209-240)build_list_data_structure(243-300)render_list_tree(303-378)slash_commands/cli_utils.py (1)
format_source_info(88-129)
slash_commands/list_discovery.py (3)
mcp_server/prompt_utils.py (1)
parse_frontmatter(88-102)slash_commands/cli_utils.py (1)
format_source_info(88-129)slash_commands/config.py (2)
AgentConfig(18-31)get_agent_config(102-108)
tests/integration/test_generate_command.py (1)
tests/integration/conftest.py (3)
temp_test_dir(28-35)test_prompts_dir(39-45)get_slash_man_command(81-89)
tests/integration/test_list_command.py (3)
slash_commands/cli_utils.py (1)
format_source_info(88-129)slash_commands/list_discovery.py (5)
build_list_data_structure(243-300)count_backups(209-240)count_unmanaged_prompts(160-206)discover_managed_prompts(24-66)render_list_tree(303-378)tests/integration/conftest.py (3)
get_slash_man_command(81-89)temp_test_dir(28-35)test_prompts_dir(39-45)
🪛 LanguageTool
docs/specs/07-spec-list-command/07-proofs/07-task-03-proofs.md
[uncategorized] ~56-~56: The official name of this software platform is spelled with a capital “H”.
Context: ...e_path`) - ✅ GitHub source formatting: "github: owner/repo@branch:path" - ✅ Missing fi...
(GITHUB)
[uncategorized] ~176-~176: The official name of this software platform is spelled with a capital “H”.
Context: ...to source_path) - GitHub sources: "github: owner/repo@branch:path" (handles missi...
(GITHUB)
[uncategorized] ~234-~234: The official name of this software platform is spelled with a capital “H”.
Context: ...mpts" 6. ✅ GitHub sources show format: "github: owner/repo@branch:path" 7. ✅ Missing s...
(GITHUB)
docs/specs/07-spec-list-command/07-spec-list-command.md
[style] ~19-~19: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...oper troubleshooting prompt issues**, I want to see backup counts and last updated time...
(REP_WANT_TO_VB)
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...and how many recovery points exist. - As a user upgrading from older versions,...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...anism and can regenerate if needed. - **As a user managing a mix of managed and un...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~53-~53: The official name of this software platform is spelled with a capital “H”.
Context: ...ne (e.g., "local: /path/to/prompts" or "github: owner/repo@branch:path"). Unmanaged pr...
(GITHUB)
docs/specs/07-spec-list-command/07-tasks-list-command.md
[style] ~91-~91: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... works correctly - TOML parsing works correctly - Empty directories are handled grace...
(ADVERB_REPETITION_PREMIUM)
[uncategorized] ~141-~141: The official name of this software platform is spelled with a capital “H”.
Context: ... - GitHub sources show format (e.g., "github: owner/repo@branch:path") - Test GitHub...
(GITHUB)
[uncategorized] ~173-~173: The official name of this software platform is spelled with a capital “H”.
Context: ...b_source()- formats GitHub source as "github: owner/repo@branch:path" usingmeta.so...
(GITHUB)
docs/specs/07-spec-list-command/07-questions-1-list-command.md
[style] ~3-~3: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...r more options, or add your own notes). Feel free to add additional context under any questi...
(FEEL_FREE_TO_STYLE_ME)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Integration Tests (Docker)
🔇 Additional comments (6)
tests/test_generators.py (1)
309-330: LGTM!The new tests appropriately verify that the
managed_byfield is included in both Markdown and TOML generator outputs. Good coverage for both generator types.tests/integration/test_generate_command.py (1)
332-392: LGTM! Comprehensive integration test for managed_by metadata.The test thoroughly validates that both Markdown and TOML outputs contain the
managed_by: slash-manfield, which is essential for the list command's discovery mechanism.slash_commands/cli.py (2)
74-77: Good practice: backward-compatibility aliases.Creating aliases for the moved functions ensures existing code continues to work while the implementation is consolidated in
cli_utils.py.
785-863: LGTM! List command implementation addresses previous concerns.The implementation properly handles:
- Detection path fallback to target_path (lines 816-820) ✓
- Invalid agent key errors with helpful messages (lines 849-856) ✓
- Empty state with informative user guidance (lines 839-845) ✓
Both issues raised in past review comments have been resolved.
tests/test_list_discovery.py (1)
1-1129: LGTM! Comprehensive test coverage.This test suite thoroughly validates:
- Discovery of managed prompts across formats and agents ✓
- Backup counting with correct pattern matching ✓
- Source info formatting for local and GitHub sources ✓
- Data structure building and grouping by prompt name ✓
- Rich tree rendering with all expected elements ✓
- Edge cases: malformed files, Unicode errors, permissions ✓
Excellent test coverage for the list discovery functionality.
tests/integration/test_list_command.py (1)
1-474: LGTM! Comprehensive integration tests.The integration test suite validates:
- End-to-end discovery across multiple agents ✓
- Backup counting with generated files ✓
- Source info formatting ✓
- CLI flag handling (--agent, --target-path, --detection-path) ✓
- Multiple agent flags ✓
- Empty state messaging ✓
Excellent coverage of CLI workflows and user scenarios.
| It includes various frontmatter fields and body content to test the slash command generation process. | ||
| ``` | ||
|
|
||
| **Verification**: The `managed_by: slash-man` field is present in the `meta` section of the frontmatter (line 25). |
There was a problem hiding this comment.
Fix incorrect line number reference.
The verification note states "line 25" but the managed_by: slash-man field appears on line 71 in the example above.
Apply this diff:
-**Verification**: The `managed_by: slash-man` field is present in the `meta` section of the frontmatter (line 25).
+**Verification**: The `managed_by: slash-man` field is present in the `meta` section of the frontmatter (line 71).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Verification**: The `managed_by: slash-man` field is present in the `meta` section of the frontmatter (line 25). | |
| **Verification**: The `managed_by: slash-man` field is present in the `meta` section of the frontmatter (line 71). |
🤖 Prompt for AI Agents
In docs/specs/07-spec-list-command/07-proofs/07-task-01-proofs.md around line
83, the verification note incorrectly references "line 25" for the managed_by:
slash-man frontmatter field; update that reference to "line 71" to match the
example above so the verification message points to the correct line number.
Add FileNotFoundError to exception handlers in discover_managed_prompts(), _parse_command_file(), and count_unmanaged_prompts() to gracefully handle cases where files are deleted between discovery and reading. Add comprehensive tests to verify FileNotFoundError handling works correctly in both discover_managed_prompts() and count_unmanaged_prompts() functions.
- Add classify_file_type() helper function to classify files as managed/unmanaged/backup/other - Add discover_all_files() function to scan and classify all files in agent directories - Add comprehensive unit tests for file discovery and classification - Handle backup files correctly by scanning for backup pattern Related to T1.0 in Spec 08
- Add _build_agent_summary_panel() helper function - Add _build_agent_file_table() helper function with sorting and color coding - Add render_all_files_tables() main rendering function - Add comprehensive unit tests for table rendering Related to T2.0 in Spec 08
- Add all_files parameter to list_cmd() function - Implement conditional logic to use discover_all_files() and render_all_files_tables() when flag is set - Add integration tests for flag combinations - Ensure agent detection logic works consistently for both modes Related to T03 in Spec 08
- Modified discover_all_files() to return directory existence status - Updated render_all_files_tables() to handle empty and missing directories - Added empty state messages: 'No files found' and 'Directory does not exist' - Added tests for empty directory, missing directory, and multi-agent scenarios - All tests pass, no regressions introduced Related to T4.0 in Spec 08
…tput - Added informational note explaining only files matching expected extension pattern are shown - Note shows single extension or multiple extensions when multiple agents are queried - Helps users understand why some files may not appear in the listing
Update help text to say 'List all prompt files in agent command directories' instead of 'List all files in agent command directories, not just managed prompts' for clarity and consistency. - Updated help text in slash_commands/cli.py - Updated validation report to reflect new help text - All tests pass (98 tests)
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
docs/specs/08-spec-list-all-files-flag/08-questions-1-list-all-files-flag.md (1)
19-23: Clarify "all files" wording to match extension-based discoveryThe selected answers for “File Scope” currently say “All files in agent command directories (managed prompts, unmanaged prompts, backup files, and any other files)”. The implemented behavior and later spec text narrow this to files matching each agent’s
command_file_extensionplus backup files following the timestamped.bakpattern. Consider explicitly mentioning the extension-based filtering here (e.g., “all files matching the command_file_extension plus backups”) so this questionnaire artifact doesn’t imply that arbitrary non-matching files are also included.slash_commands/cli.py (3)
76-80: Good consolidation onto shared cli_utils path helpersThe aliases
_find_project_root = find_project_rootand_display_local_path = display_local_pathkeep existing helper usage intact while centralizing the implementations inslash_commands.cli_utils. That’s a clean way to de-duplicate path logic without touching all call sites.If/when you’re ready to drop the private
_find_*/_display_*names, you could later inline direct calls tocli_utilsand remove these aliases entirely, but it’s not required for this PR.
787-829:list_cmdpath resolution and detection now matchgeneratesemanticsThe new
list_cmdcorrectly mirrorsgenerate’s behavior by:
- Resolving
actual_target_pathastarget_path or Path.home().- Resolving
actual_detection_pathasdetection_path or target_path or Path.home().This ensures that when users specify
--target-pathbut omit--detection-path, agent detection runs against the same base used for command files, which matches the documented behavior and avoids the “home-only detection” pitfall from earlier iterations. The options wiring and defaults look sound.Minor nit: the docstring still says “List all managed slash commands.” but with
--all-filesit can list unmanaged/backup/other files as well. Consider broadening this description (e.g., “List managed prompts (and optionally all prompt files) for configured agents”) to match the actual capabilities.
873-901: Managed-only flow matches spec; consider modest deduplication of error handlingThe standard (non-
--all-files) branch:
- Uses
discover_managed_promptswithactual_target_pathand selected agents.- Prints a clear multi-line message explaining why no managed prompts were found and exits with code 0 when
discoveredis empty.- Calls
count_unmanaged_promptsonly when there is at least one managed prompt, then builds the tree data structure and renders it viarender_list_tree.- Wraps the discovery/count calls in
try/except KeyErrorwith the same user-facing guidance as the all-files branch for invalid agent keys.Functionally this matches the intended behavior and provides good UX. The two
KeyErrorhandlers (all-files vs standard) are nearly identical; if you find yourself touching them again, you might factor the shared printing into a tiny helper to keep them in sync, but it’s not necessary right now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-01-proofs.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-02-proofs.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-03-proofs.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-04-proofs.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-questions-1-list-all-files-flag.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-spec-list-all-files-flag.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-tasks-list-all-files-flag.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-validation-list-all-files-flag.md(1 hunks)slash_commands/cli.py(4 hunks)slash_commands/list_discovery.py(1 hunks)tests/integration/test_list_command.py(1 hunks)tests/test_list_discovery.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-03-proofs.md
- docs/specs/08-spec-list-all-files-flag/08-validation-list-all-files-flag.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_list_command.py
🧰 Additional context used
🧬 Code graph analysis (3)
slash_commands/cli.py (4)
slash_commands/cli_utils.py (3)
display_local_path(50-66)find_project_root(10-47)relative_to_candidates(69-85)slash_commands/list_discovery.py (6)
build_list_data_structure(256-313)count_unmanaged_prompts(167-219)discover_all_files(359-428)discover_managed_prompts(25-73)render_all_files_tables(564-631)render_list_tree(634-709)slash_commands/detection.py (1)
detect_agents(11-25)slash_commands/config.py (1)
list_agent_keys(96-99)
tests/test_list_discovery.py (2)
slash_commands/list_discovery.py (8)
count_unmanaged_prompts(167-219)discover_managed_prompts(25-73)_parse_command_file(96-116)count_backups(222-253)build_list_data_structure(256-313)render_list_tree(634-709)discover_all_files(359-428)render_all_files_tables(564-631)slash_commands/cli_utils.py (1)
format_source_info(88-129)
slash_commands/list_discovery.py (3)
mcp_server/prompt_utils.py (1)
parse_frontmatter(88-102)slash_commands/cli_utils.py (2)
format_source_info(88-129)relative_to_candidates(69-85)slash_commands/config.py (2)
AgentConfig(18-31)get_agent_config(102-108)
🪛 LanguageTool
docs/specs/08-spec-list-all-files-flag/08-questions-1-list-all-files-flag.md
[style] ~3-~3: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...r more options, or add your own notes). Feel free to add additional context under any questi...
(FEEL_FREE_TO_STYLE_ME)
docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-01-proofs.md
[style] ~36-~36: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...orrectly classified as "managed". 3. **test_discover_all_files_classifies_unmanaged...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~38-~38: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rectly classified as "unmanaged". 4. **test_discover_all_files_classifies_backup_fi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~40-~40: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...correctly classified as "backup". 5. **test_discover_all_files_classifies_other_fil...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~42-~42: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... correctly classified as "other". 6. **test_discover_all_files_handles_parsing_erro...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-02-proofs.md
[style] ~43-~43: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... then alphabetically by filename. 3. **test_render_all_files_tables_applies_color_c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~45-~45: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., backup files use default color. 4. **test_render_all_files_tables_shows_summary_p...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~47-~47: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ile count, and breakdown by type. 5. **test_render_all_files_tables_shows_relative_...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/specs/08-spec-list-all-files-flag/08-spec-list-all-files-flag.md
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...identify files that need attention. - *As a user cleaning up old or unused files...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... other files that may need removal. - **As a user troubleshooting detection issues...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...r troubleshooting detection issues**, I want to see the directory structure and file co...
(REP_WANT_TO_VB)
[grammar] ~54-~54: Use a hyphen to join words.
Context: ...ly by filename. - The system shall color code files: green for managed files, red...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Integration Tests (Docker)
🔇 Additional comments (8)
slash_commands/cli.py (3)
108-110: Consistent use ofrelative_to_candidatesvia a local aliasBinding
_relative_to_candidates = relative_to_candidatesinside_build_summary_datais a low-friction way to switch over to the shared utility without changing downstream uses. The behavior stays consistent with previous logic but is now implemented in a single place incli_utils.
831-842: Agent selection behavior is reasonable and consistent across modesWhen
--agentis omitted,list_cmddefers todetect_agents(actual_detection_path)and, if none are found, exits with a yellow informational message and code 0. When agents are supplied, it uses them directly asselected_agents. This aligns with the existing CLI’s behavior: detection-as-convenience, explicit agents as override. No functional issues here.
843-871: All-files flow correctly wires discovery, per-agent grouping, and error handlingThe
--all-filesbranch:
- Calls
discover_all_files(actual_target_path, selected_agents)and passes the returneddirectory_statusintorender_all_files_tables, enabling accurate empty/missing directory messaging.- Pre-populates
files_by_agentfor every selected agent so that agents with empty or non-existent directories still get a summary panel, matching the spec for the all-files flag.- Catches
KeyErrorand converts it into a user-friendly “Invalid agent key” error with--list-agentsguidance and a list of valid keys, rather than surfacing a traceback.This flow matches the all-files spec and addresses prior concerns about invalid agent keys crashing the command.
tests/test_list_discovery.py (1)
1-1799: Excellent test coverage and structure!The test suite is comprehensive, well-organized, and follows best practices. It covers:
- Core discovery and filtering logic
- Multiple file formats (Markdown/TOML)
- Error handling (malformed files, Unicode, permissions, FileNotFound)
- Edge cases (empty directories, missing directories, backup files)
- Rendering outputs with proper assertions
- All-files classification and table rendering
The tests are clear, focused, and properly isolated using
tmp_pathfixtures and mocking where appropriate.docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-04-proofs.md (1)
1-146: Clear documentation with solid proof artifacts.The proofs document clearly demonstrates that Task 4.0 (empty state and directory handling) has been implemented and tested successfully. All tests pass, code quality checks pass, and the implementation summary is thorough.
docs/specs/08-spec-list-all-files-flag/08-tasks-list-all-files-flag.md (1)
1-110: Well-organized task breakdown with clear completion markers.The task list is comprehensive and well-structured, with all tasks marked complete. The proof artifacts section clearly documents what evidence demonstrates successful implementation. This provides good traceability between requirements, implementation, and testing.
slash_commands/list_discovery.py (2)
359-429: Well-implemented comprehensive file discovery with proper deduplication.The
discover_all_filesfunction correctly:
- Tracks directory existence for each agent
- Scans both regular files and backup files separately
- Uses deduplication via
processed_filesset to avoid counting files twice- Returns structured data with both files and directory status
The implementation handles edge cases well and integrates cleanly with the classification logic.
500-562: Excellent table rendering with proper sorting and color coding.The
_build_agent_file_tablefunction implements:
- Clear type-based sorting (managed → unmanaged → backup → other)
- Secondary alphabetical sorting within each type
- Appropriate color coding (green for managed, red for unmanaged/other, default for backup)
- Relative path display using
relative_to_candidatesThe sorting logic is well-defined and the color choices provide clear visual feedback to users.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
docs/specs/07-spec-list-command/07-proofs/07-task-01-proofs.md (1)
83-83: Fix incorrect line number reference.The verification note references "line 25" but the
managed_by: slash-manfield appears on line 71 in the example above.Apply this diff:
-**Verification**: The `managed_by: slash-man` field is present in the `meta` section of the frontmatter (line 25). +**Verification**: The `managed_by: slash-man` field is present in the `meta` section of the frontmatter (line 71).docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-01-proofs.md (1)
73-76: Updatediscover_all_filessignature and return-shape to match current implementationThis section still documents
discover_all_files(base_path: Path, agents: list[str]) -> list[dict[str, Any]]returning only file dicts, but the implementation now returns a mapping with at least"files"and"directory_status"(used bylist_cmdand related tests). Please update the documented signature and the description of the return value to reflect the current structure (e.g.,dict[str, Any]with"files": [...]and"directory_status": {...}) so proofs stay aligned with the real API.docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-02-proofs.md (1)
73-85: Align_build_agent_summary_panelandrender_all_files_tablessignatures with directory-status APIThe “Functions Implemented” section still shows:
_build_agent_summary_panel(agent: AgentConfig, files: list[dict[str, Any]], target_path: Path) -> Panelrender_all_files_tables(files_by_agent: dict[str, list[dict[str, Any]]], target_path: Path, *, record: bool = False) -> str | Nonebut the implementation now also threads directory-existence information (e.g., a
directory_existsflag into the panel helper and adirectory_statusmapping intorender_all_files_tables, as used bylist_cmd). Please update these signatures and briefly document how the extra parameter(s) drive empty/missing-directory messaging so the proofs accurately describe the current renderer behavior.docs/specs/07-spec-list-command/07-proofs/07-task-06-proofs.md (1)
37-41: Fix inverted “Net Code Reduction” wording and arithmeticThe diff stats show
141 insertions(+), 103 deletions(-), which is a net increase of 38 lines, but both the “Net Code Reduction” line and the later “Code Reduction” metric describe this as 38 lines removed. Please reword to something like “Net change: +38 lines (141 insertions, 103 deletions)” and drop “reduction” terminology so the metrics accurately reflect the diff.Also applies to: 43-44, 198-202
slash_commands/list_discovery.py (1)
119-155: Still need defensive handling whenmetais not a dict (can crash on malformed files)
_parse_markdown_fileand_parse_toml_fileboth assignmeta = ... or {}without validating the type, while callers assumemetais a mapping:
discover_managed_prompts,count_unmanaged_prompts, andclassify_file_typeall doprompt_data.get("meta", {}).get("managed_by").If a user has a syntactically valid file with a non‑mapping
meta(e.g.,meta: "oops"in YAML ormeta = "oops"in TOML),metabecomes a string,.get("managed_by")raisesAttributeError, and the list flow can crash instead of “skipping malformed files silently” as intended. The outerexceptblocks don’t catchAttributeError, so this isn’t contained.You can fix this by validating
metain the parsers and treating non‑mappingmetaas an invalid prompt:def _parse_markdown_file( file_path: Path, content: str, agent: AgentConfig ) -> dict[str, Any] | None: @@ - name = frontmatter.get("name") or file_path.stem - meta = frontmatter.get("meta") or {} + name = frontmatter.get("name") or file_path.stem + raw_meta = frontmatter.get("meta") or {} + if not isinstance(raw_meta, dict): + # Treat files with non-mapping meta as invalid prompts + return None + meta = raw_meta @@ def _parse_toml_file(file_path: Path, content: str, agent: AgentConfig) -> dict[str, Any] | None: @@ - meta = data.get("meta") or {} - - # Extract name from meta.source_prompt (where generator stores it) or use filename - name = meta.get("source_prompt") or file_path.stem + raw_meta = data.get("meta") or {} + if not isinstance(raw_meta, dict): + # Treat files with non-table meta as invalid prompts + return None + meta = raw_meta + + # Extract name from meta.source_prompt (where generator stores it) or use filename + name = meta.get("source_prompt") or file_path.stemWith that change, all downstream uses of
metacan safely assume a dict, and malformed-but-parseable files are simply skipped, matching the spec and your tests’ intent.Also applies to: 191-205, 316-345
🧹 Nitpick comments (7)
docs/specs/08-spec-list-all-files-flag/08-spec-list-all-files-flag.md (2)
18-22: Optional: Consider varying sentence structure in user stories.The static analysis tool suggests that three successive user stories begin with "As a user," which could impact readability. Consider varying the phrasing for one or two of them (e.g., "When debugging prompt issues, developers need..." or "During cleanup operations, users require...").
54-54: Optional: Consider hyphenating "color code".The phrase "color code" could be hyphenated as "color-code" when used as a verb phrase for consistency with technical writing conventions.
slash_commands/cli_utils.py (1)
30-47: Consider removing or relaxing the 10-level cap infind_project_rootThe upward walk already stops safely at the filesystem root (
parent == current), so the hardrange(10)limit can incorrectly prevent finding the real project root when the CLI is invoked from a deeply nested directory (more than 10 levels below.git/pyproject.toml). Consider either dropping the depth bound or increasing it substantially sofind_project_root()remains reliable in larger monorepos.docs/specs/08-spec-list-all-files-flag/08-tasks-list-all-files-flag.md (1)
34-42: Clarifydiscover_all_filesreturn type evolution in the task listTask 1.8 still describes
discover_all_files()as returning only a list of file dicts, while Task 4.4 (and the implementation/CLI) expand this to include per-agent directory status. To avoid confusion for readers skimming just the early tasks, consider adding a short note in 1.8 (or a forward reference) that the return shape is later refined to include directory metadata, matching the final"files"+"directory_status"structure.Also applies to: 101-106
slash_commands/cli.py (1)
787-821: Broadenlist_cmddocstring to reflect--all-filesbehaviorThe docstring currently says “List all managed slash commands.”, but with
--all-filesenabled the command lists all prompt files (managed, unmanaged, backup, other). Consider rephrasing to something like “List managed slash commands (or, with--all-files, all prompt files)” so the docstring matches the actual CLI behavior.tests/integration/test_list_command.py (1)
64-110: Backup-count integration test is realistic; consider mocking time if it ever flakesUsing real files and
time.sleep(1)to ensure distinctYYYYMMDD‑HHMMSStimestamps mirrors production behavior and should be stable in practice. If this ever becomes flaky under very slow CI or time-skew, you could switch to injecting/mocking timestamps instead of sleeping, but it’s not strictly necessary right now.docs/specs/07-spec-list-command/07-tasks-list-command.md (1)
85-176: Minor editorial nits (capitalization and repetition)There are a couple of tiny polish opportunities:
- Use the official capitalization “GitHub” instead of
githubin examples/descriptions (e.g., lines describing"github: owner/repo@branch:path").- In the 2.0 proof bullets, “works correctly” appears twice in a single list line; trimming one “correctly” would read a bit cleaner.
Purely cosmetic; no need to block on this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.markdownlint.yaml(1 hunks)README.md(6 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-01-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-02-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-03-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-04-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-05-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-proofs/07-task-06-proofs.md(1 hunks)docs/specs/07-spec-list-command/07-questions-1-list-command.md(1 hunks)docs/specs/07-spec-list-command/07-spec-list-command.md(1 hunks)docs/specs/07-spec-list-command/07-tasks-list-command.md(1 hunks)docs/specs/07-spec-list-command/07-validation-list-command.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-01-proofs.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-02-proofs.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-03-proofs.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-04-proofs.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-questions-1-list-all-files-flag.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-spec-list-all-files-flag.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-tasks-list-all-files-flag.md(1 hunks)docs/specs/08-spec-list-all-files-flag/08-validation-list-all-files-flag.md(1 hunks)slash_commands/cli.py(4 hunks)slash_commands/cli_utils.py(1 hunks)slash_commands/generators.py(2 hunks)slash_commands/list_discovery.py(1 hunks)tests/integration/test_generate_command.py(2 hunks)tests/integration/test_list_command.py(1 hunks)tests/test_generators.py(1 hunks)tests/test_list_discovery.py(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/specs/08-spec-list-all-files-flag/08-spec-list-all-files-flag.md
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...identify files that need attention. - *As a user cleaning up old or unused files...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... other files that may need removal. - **As a user troubleshooting detection issues...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...r troubleshooting detection issues**, I want to see the directory structure and file co...
(REP_WANT_TO_VB)
[grammar] ~54-~54: Use a hyphen to join words.
Context: ...ly by filename. - The system shall color code files: green for managed files, red...
(QB_NEW_EN_HYPHEN)
docs/specs/07-spec-list-command/07-spec-list-command.md
[style] ~19-~19: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...oper troubleshooting prompt issues**, I want to see backup counts and last updated time...
(REP_WANT_TO_VB)
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...and how many recovery points exist. - As a user upgrading from older versions,...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...anism and can regenerate if needed. - **As a user managing a mix of managed and un...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~53-~53: The official name of this software platform is spelled with a capital “H”.
Context: ...ne (e.g., "local: /path/to/prompts" or "github: owner/repo@branch:path"). Unmanaged pr...
(GITHUB)
docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-01-proofs.md
[style] ~36-~36: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...orrectly classified as "managed". 3. **test_discover_all_files_classifies_unmanaged...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~38-~38: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rectly classified as "unmanaged". 4. **test_discover_all_files_classifies_backup_fi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~40-~40: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...correctly classified as "backup". 5. **test_discover_all_files_classifies_other_fil...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~42-~42: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... correctly classified as "other". 6. **test_discover_all_files_handles_parsing_erro...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/specs/07-spec-list-command/07-proofs/07-task-03-proofs.md
[uncategorized] ~56-~56: The official name of this software platform is spelled with a capital “H”.
Context: ...e_path`) - ✅ GitHub source formatting: "github: owner/repo@branch:path" - ✅ Missing fi...
(GITHUB)
[uncategorized] ~176-~176: The official name of this software platform is spelled with a capital “H”.
Context: ...to source_path) - GitHub sources: "github: owner/repo@branch:path" (handles missi...
(GITHUB)
[uncategorized] ~234-~234: The official name of this software platform is spelled with a capital “H”.
Context: ...mpts" 6. ✅ GitHub sources show format: "github: owner/repo@branch:path" 7. ✅ Missing s...
(GITHUB)
docs/specs/08-spec-list-all-files-flag/08-questions-1-list-all-files-flag.md
[style] ~3-~3: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...r more options, or add your own notes). Feel free to add additional context under any questi...
(FEEL_FREE_TO_STYLE_ME)
docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-02-proofs.md
[style] ~43-~43: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... then alphabetically by filename. 3. **test_render_all_files_tables_applies_color_c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~45-~45: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., backup files use default color. 4. **test_render_all_files_tables_shows_summary_p...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~47-~47: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ile count, and breakdown by type. 5. **test_render_all_files_tables_shows_relative_...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/specs/07-spec-list-command/07-questions-1-list-command.md
[style] ~3-~3: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...r more options, or add your own notes). Feel free to add additional context under any questi...
(FEEL_FREE_TO_STYLE_ME)
docs/specs/07-spec-list-command/07-tasks-list-command.md
[style] ~91-~91: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... works correctly - TOML parsing works correctly - Empty directories are handled grace...
(ADVERB_REPETITION_PREMIUM)
[uncategorized] ~141-~141: The official name of this software platform is spelled with a capital “H”.
Context: ... - GitHub sources show format (e.g., "github: owner/repo@branch:path") - Test GitHub...
(GITHUB)
[uncategorized] ~173-~173: The official name of this software platform is spelled with a capital “H”.
Context: ...b_source()- formats GitHub source as "github: owner/repo@branch:path" usingmeta.so...
(GITHUB)
🔇 Additional comments (26)
.markdownlint.yaml (1)
21-23: LGTM!The addition of the MD051 disable is well-justified with a clear comment explaining that GitHub automatically handles emoji removal in heading IDs.
docs/specs/07-spec-list-command/07-proofs/07-task-05-proofs.md (1)
1-174: LGTM!This proof artifact document is comprehensive and well-structured. It effectively demonstrates the CLI command functionality with test results, CLI transcripts, and verification steps for all flags and empty state handling.
docs/specs/07-spec-list-command/07-proofs/07-task-03-proofs.md (1)
1-329: LGTM!This proof artifact thoroughly documents the backup counting and source metadata extraction functionality with comprehensive test coverage, usage examples, and CLI transcripts.
docs/specs/08-spec-list-all-files-flag/08-questions-1-list-all-files-flag.md (1)
1-124: LGTM!This design questions document clearly captures the feature requirements and design decisions for the
--all-filesflag. The selected options provide clear direction for implementation.slash_commands/generators.py (2)
226-226: LGTM!The addition of
managed_by: "slash-man"metadata to the Markdown generator is consistent with the PR objectives and enables discovery of managed prompts.
280-280: LGTM!The addition of
managed_by: "slash-man"metadata to the TOML generator maintains consistency with the Markdown generator implementation, ensuring both output formats can be discovered by the list command.docs/specs/07-spec-list-command/07-validation-list-command.md (1)
1-291: LGTM!This validation report is comprehensive and well-organized. It provides clear evidence that all functional requirements are implemented and verified, with 100% test pass rate and all quality gates passing.
docs/specs/07-spec-list-command/07-questions-1-list-command.md (1)
1-119: LGTM!This design questions document clearly captures the requirements and design decisions for the list command feature. The selected options provide clear guidance for implementation priorities.
docs/specs/08-spec-list-all-files-flag/08-validation-list-all-files-flag.md (1)
1-216: LGTM!This validation report comprehensively documents the implementation and verification of the
--all-filesflag feature. All functional requirements are verified with 100% test pass rate and all quality gates passing.docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-03-proofs.md (1)
1-144: LGTM! Comprehensive proof documentation.The task proof document is well-structured and includes all necessary verification artifacts: test results, CLI help output, code quality checks, and implementation details.
tests/integration/test_generate_command.py (1)
332-392: LGTM! Comprehensive integration test for managed_by field.The test validates that both Markdown and TOML generators include the
managed_by: slash-manmetadata field. Test logic is correct and covers both format paths end-to-end.tests/test_generators.py (2)
309-316: LGTM! Direct unit test for _build_meta method.The test validates that the internal
_build_metamethod includes themanaged_byfield with the correct value.
319-330: LGTM! Comprehensive TOML generator test.The test validates that the TOML generator includes the
managed_byfield in the meta section, covering the end-to-end generation path for TOML format.README.md (1)
1-339: LGTM! Comprehensive README overhaul.The README has been significantly improved with better structure, clear quick start guide, expanded usage documentation, and proper documentation of the new
listcommand. The updates align well with the new features introduced in this PR.docs/specs/07-spec-list-command/07-proofs/07-task-02-proofs.md (1)
1-161: LGTM! Well-documented Task 2 proof artifacts.The documentation comprehensively covers prompt discovery and filtering logic with unit tests, integration tests, and CLI transcripts demonstrating the feature working end-to-end.
docs/specs/07-spec-list-command/07-proofs/07-task-04-proofs.md (1)
1-109: LGTM! Complete Task 4 proof documentation.The documentation thoroughly covers rich output display with tree structure, including unit tests, integration tests, CLI transcripts, and comprehensive verification checklist.
docs/specs/08-spec-list-all-files-flag/08-spec-list-all-files-flag.md (1)
1-156: LGTM! Comprehensive specification document.The specification is well-structured with clear goals, user stories, functional requirements, proof artifacts, and technical considerations. The document provides excellent guidance for implementing the
--all-filesflag feature.docs/specs/07-spec-list-command/07-spec-list-command.md (1)
73-127: List-command spec matches current CLI behaviorThe functional requirements here (managed-only discovery,
managed_bycontract, backup counting, unmanaged-count reporting, and reuse of target/detection-path and agent-filtering semantics fromgenerate) line up with the currentlist_cmdand discovery helpers incli.py/list_discovery.py. No changes needed from a behavioral standpoint.slash_commands/cli.py (1)
76-80: Shared utilities and list-command wiring look consistent and fix earlier issuesThe refactor to import
find_project_root,display_local_path, andrelative_to_candidatesfromcli_utilsand to routelist_cmdthroughdiscover_managed_prompts/discover_all_fileswith explicitKeyErrorhandling brings list behavior in line withgenerate(notably: detection now falls back to--target-pathwhen--detection-pathis omitted, and invalid agent keys no longer crash the process). This wiring matches the specs and resolves the prior detection/validation concerns.Also applies to: 108-110, 787-871
tests/test_list_discovery.py (2)
12-221: Thorough coverage of discovery and unmanaged-counting paths looks solidThe tests around
discover_managed_promptsandcount_unmanaged_promptsexercise markdown vs TOML, backups, malformed content, permission/Unicode/file-not-found errors, multi-agent setups, and empty dirs. This gives very strong confidence in the discovery/core counting behavior and the error-handling contract (“skip malformed files silently”).
623-1800: All-files and rendering tests align well with the spec and CLI behaviorThe
count_backups,build_list_data_structure,render_list_tree,discover_all_files, andrender_all_files_tablestests collectively validate grouping, backup detection, classification, sorting, color-coding, relative paths, and empty/missing-directory messages. The assertions are loose enough to tolerate minor formatting tweaks while still catching regressions in structure and semantics.tests/integration/test_list_command.py (1)
18-663: End-to-end list command coverage is comprehensiveThese tests exercise discovery, backup counting, source formatting, tree vs table output, all flag combinations, multiple agents, and the empty state contract (including messaging and exit code). This is excellent integration coverage for the new
listcommand behavior.docs/specs/08-spec-list-all-files-flag/08-proofs/08-task-04-proofs.md (1)
1-146: Proofs document accurately reflects implementation and testsThe described test runs, API changes (
discover_all_filesreturn shape,render_all_files_tables+_build_agent_summary_panelbehavior), and quality gates line up with the code and tests. Nothing blocking here.slash_commands/list_discovery.py (3)
222-253: Backup counting logic matches writer pattern and tests
count_backupscorrectly mirrors thewriter.pypattern ({filename}{suffix}.{timestamp}.bak), validates the timestamp with a strictYYYYMMDD-HHMMSSregex, and only scans the command file’s directory. This aligns with the unit and integration tests and should behave well even with extra.baknoise in the directory.
256-313: List data-structure and tree rendering are clean and match the spec
build_list_data_structuregroups by prompt name, aggregates per-agent info (including backup counts), and consolidates source/updated_at once per prompt.render_list_treethen renders a stable, readable Rich tree (with prompts, source, updated, agents, file paths, and unmanaged counts). This design is simple, matches the documented structure, and lines up with the tests.Also applies to: 634-709
359-428: All-files discovery and per-agent tables are well-structured
discover_all_filescleanly separates discovery (including backup classification and directory existence tracking) from presentation.render_all_files_tables’s per-agent summary panel plus color-coded, type-ordered table gives a clear diagnostic view, and the extension-note logic is a nice usability touch. Behavior matches the new tests and the 08-spec proofs.Also applies to: 431-631
Address PR review feedback by adding defensive handling for non-dict meta values in file parsers and updating proof documentation to match current implementation. - Add isinstance() validation in _parse_markdown_file and _parse_toml_file to prevent AttributeError crashes on malformed files - Remove artificial 10-level cap in find_project_root, rely on filesystem root check for natural termination - Update proof document function signatures to match implementation (discover_all_files return type, directory_status parameters) - Update list_cmd docstring to reflect --all-files behavior
1 participant
Why?
This PR implements a new
listcommand that provides visibility into all prompts managed by slash-command-manager across configured agent locations. Users currently have no way to see what prompts are installed, where they're located, or how many backup versions exist. This feature addresses this gap by enabling users to:Evidence:
docs/specs/07-spec-list-command/07-spec-list-command.md(commit33447f6)docs/specs/07-spec-list-command/07-tasks-list-command.md(commit968a5c7)docs/specs/07-spec-list-command/07-validation-list-command.mdWhat Changed?
Implemented a comprehensive
listcommand that discovers, filters, and displays managed prompts in a Rich-formatted tree structure. The implementation follows a TDD workflow with extensive test coverage and includes code consolidation to reduce duplication betweengenerateandlistcommands.Key Changes:
listcommand (slash-man list) that discovers managed prompts across agent command directoriesmanaged_by: slash-manfield to all generated command files (Markdown and TOML formats) to enable prompt discovery--agent(multiple),--target-path, and--detection-pathflags matchinggeneratecommand behaviorcli_utils.py) following DRY principles:Files Modified:
New Files:
slash_commands/list_discovery.py(382 lines) - Core discovery, filtering, and rendering logicslash_commands/cli_utils.py(129 lines) - Shared utilities for path resolution and source formattingtests/test_list_discovery.py(1,129 lines) - Comprehensive unit tests (32 tests)tests/integration/test_list_command.py(478 lines) - Integration tests (12 tests)Modified Files:
slash_commands/cli.py(+142 lines) - Addedlistcommand implementationslash_commands/generators.py(+2 lines) - Addedmanaged_bymetadata fieldtests/integration/test_generate_command.py(+67 lines) - Added tests formanaged_byfieldtests/test_generators.py(+24 lines) - Added tests for metadata fieldDocumentation:
docs/specs/07-spec-list-command/Additional Notes
Breaking Changes: None. The
managed_byfield is added to new/updated files but doesn't affect existing functionality. Older generated files without the field won't appear inlistoutput until regenerated (by design).Performance Implications:
Path.glob()patternsSecurity Considerations:
Testing Strategy:
pytest tests/test_list_discovery.py tests/integration/test_list_command.py tests/test_generators.pyDependencies:
rich(for tree rendering),tomllib(Python 3.12+),pyyaml(for frontmatter parsing)Configuration Changes:
Known Limitations:
managed_by: slash-manmetadata are detected (files generated by older versions won't appear until regenerated)Related Issues:
docs/specs/07-spec-list-command/07-spec-list-command.mddocs/specs/07-spec-list-command/07-validation-list-command.mddocs/specs/07-spec-list-command/07-proofs/Review Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.