refactor: multi-crate workspace + mise-style CLI architecture - #60
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a Cargo workspace with new ChangesWorkspace extraction and CLI modularization
Estimated code review effort: 3 (Moderate) | ~30 minutes Refactor plan and skill documentation
Sequence Diagram(s)sequenceDiagram
participant MainRs as main.rs
participant CliMod as cli::Cli
participant Commands as cli::Commands
participant SubcommandArgs as Subcommand Args
participant Handler as Existing Handler
MainRs->>CliMod: Cli::parse()
CliMod->>Commands: cli.command
Commands->>SubcommandArgs: run()
SubcommandArgs->>Handler: forward parsed args
Handler-->>MainRs: result / exit code
Related issues: Suggested labels: refactor, cli, workspace Suggested reviewers: maintainers familiar with the agentflare CLI dispatch and agent detection code 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (5)
skills/coderabbit-fix/SKILL.md (1)
73-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
unwrap_or_default()in the parsing example.This example silently turns malformed input into a default value, which teaches the workflow to mask failures instead of surfacing them. Propagate the error so the skill stays aligned with the root-cause-first guidance below.
Suggested fix
-let config = serde_json::from_str(&data).unwrap_or_default(); +let config = serde_json::from_str(&data)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/coderabbit-fix/SKILL.md` around lines 73 - 79, The parsing example in the skill currently uses unwrap_or_default() on serde_json::from_str, which hides malformed input by falling back to a default value. Update the example to propagate the parsing failure instead of defaulting, and keep the guidance aligned with the root-cause-first approach; use the serde_json::from_str example in SKILL.md as the reference point.crates/agent-registry/src/detect.rs (1)
18-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
PATH_LOCKis now permanent public API, only useful for tests.Making
PATH_LOCKunconditionallypub(vs. the previous#[cfg(test)] pub(crate)) is understandable —cfg(test)doesn't propagate across crate boundaries, so downstream test suites need real access to synchronizePATHenv mutations. But this permanently exposes a test-onlyMutex<()>in the crate's production public API/docs.Consider gating it behind a dedicated feature (e.g.,
test-util) enabled only as a dev-dependency feature by consumer crates, instead of leaving it always-public with#[allow(dead_code)].♻️ Sketch of a feature-gated alternative
+[features] +test-util = [] + // detect.rs -#[allow(dead_code)] +#[cfg(any(test, feature = "test-util"))] pub static PATH_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());Then consumer crates'
[dev-dependencies]would enableagent-registry = { path = "...", features = ["test-util"] }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/agent-registry/src/detect.rs` around lines 18 - 24, The public PATH_LOCK exposed from detect.rs should not remain part of the دائم production API since it is only needed for test synchronization. Update the detect module to gate PATH_LOCK behind a dedicated test-only feature such as test-util, and ensure any downstream access in tests is enabled only when that feature is requested. Keep the symbol location in detect.rs and preserve its current use for with_temp_path_dir, find_binary_tests, detect_all_tests, and resolve_version_tests without exposing it unconditionally.crates/ponytail/Cargo.toml (1)
10-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider budgeting an upgrade for
ureqandsysinfo.ureq3.x is a breaking rewrite, andsysinfo0.39.x has API changes from 0.34, so this is a deferred maintenance bump rather than a blocker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ponytail/Cargo.toml` around lines 10 - 14, Plan a deferred dependency maintenance update for the ponytail crate by reviewing the current ureq and sysinfo usages before bumping versions. Check the code paths that depend on ureq and sysinfo APIs, then adjust any affected call sites, feature flags, or type imports so the crate remains compatible with the newer major/minor releases. Keep the changes centered around the existing dependency declarations in Cargo.toml and the corresponding call sites that use ureq and sysinfo.src/cli/ponytail.rs (2)
91-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate logic between
SessionStartandSubagentStart.Both branches fetch the active/default mode, build instructions, detect platform, and format/print output almost identically (differing only in the hook name string and the off-mode handling). Consider extracting a shared helper to reduce duplication.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/ponytail.rs` around lines 91 - 125, Duplicate the repeated hook-processing logic in PonytailAction::Hook by extracting a shared helper used by both PonytailHookEvent::SessionStart and PonytailHookEvent::SubagentStart. The helper should handle fetching active/default mode, the off-mode check, building instructions, detecting the platform, and formatting/printing the hook output, while allowing the hook name and any off-mode behavior differences to be passed in as parameters.
63-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent path used for clearing active state (
ponytail::clear_active()vsponytail::state::clear_active()).Line 64 calls
ponytail::clear_active()while line 96 callsponytail::state::clear_active()for what appears to be the same operation. If these resolve to the same re-exported function this is just a style inconsistency; if not, please verify they have identical semantics.Also applies to: 92-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/ponytail.rs` around lines 63 - 66, The active-state clearing call is inconsistent between the PonytailAction::Off branch and the other path, so update the CLI logic to use a single canonical entry point for clearing state. In the ponytail command handling, make PonytailAction::Off and the code near the second clear call both invoke the same function path, either ponytail::clear_active or ponytail::state::clear_active, so the behavior is identical and the implementation is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-07-07-cli-refactor-mise.md`:
- Line 16: The tree diagram fence in the plan document is missing a language
tag, which triggers markdownlint. Update the fenced block around the tree
diagram to use a readable tag like text so the diagram stays intact while
satisfying the linter; locate the affected fence in the markdown content near
the tree diagram and adjust only that fenced block.
In `@skills/coderabbit-fix/SKILL.md`:
- Around line 20-24: The current PR comment fetch uses gh pr view with --json
reviews, which only returns top-level review bodies and can miss inline
CodeRabbit comments. Update the comment-collection step in the workflow/docs to
use the pull request comments endpoint instead, and adjust the command or script
so it retrieves line-level review comments as well as general review text.
Reference the existing GitHub CLI usage in the SKILL.md instructions so the
replacement stays aligned with the current PR comment gathering flow.
- Around line 57-58: The test command in the ponytail guidance is using a name
filter instead of selecting the workspace package, so update the instruction to
use the correct package-targeted cargo test invocation in the relevant SKILL.md
entry and the duplicate example. Refer to the affected test guidance text around
the ponytail note and replace the existing cargo test form with the
package-based one so it actually runs the ponytail crate tests.
In `@src/cli/ponytail.rs`:
- Around line 46-54: The PonytailAction::Set branch in
ponytail::normalize_config_mode currently hides invalid modes by defaulting to
"full", which should instead fail like the other action handlers. Update the Set
match arm to treat a None from normalize_config_mode(&mode) as an error, print a
clear message with eprintln!, and exit non-zero rather than calling
ponytail::set_active with a fallback. Keep the existing error handling style
used in the Default and Update branches so invalid input is surfaced
consistently.
- Around line 55-62: The active mode is being saved from the raw CLI input in
PonytailAction::Default, so exact-string checks can see unnormalized values.
Update the Default branch in the ponytail::set_default_mode /
ponytail::set_active flow to normalize or canonicalize the mode first, then pass
the normalized value to set_active and print that same normalized value.
---
Nitpick comments:
In `@crates/agent-registry/src/detect.rs`:
- Around line 18-24: The public PATH_LOCK exposed from detect.rs should not
remain part of the دائم production API since it is only needed for test
synchronization. Update the detect module to gate PATH_LOCK behind a dedicated
test-only feature such as test-util, and ensure any downstream access in tests
is enabled only when that feature is requested. Keep the symbol location in
detect.rs and preserve its current use for with_temp_path_dir,
find_binary_tests, detect_all_tests, and resolve_version_tests without exposing
it unconditionally.
In `@crates/ponytail/Cargo.toml`:
- Around line 10-14: Plan a deferred dependency maintenance update for the
ponytail crate by reviewing the current ureq and sysinfo usages before bumping
versions. Check the code paths that depend on ureq and sysinfo APIs, then adjust
any affected call sites, feature flags, or type imports so the crate remains
compatible with the newer major/minor releases. Keep the changes centered around
the existing dependency declarations in Cargo.toml and the corresponding call
sites that use ureq and sysinfo.
In `@skills/coderabbit-fix/SKILL.md`:
- Around line 73-79: The parsing example in the skill currently uses
unwrap_or_default() on serde_json::from_str, which hides malformed input by
falling back to a default value. Update the example to propagate the parsing
failure instead of defaulting, and keep the guidance aligned with the
root-cause-first approach; use the serde_json::from_str example in SKILL.md as
the reference point.
In `@src/cli/ponytail.rs`:
- Around line 91-125: Duplicate the repeated hook-processing logic in
PonytailAction::Hook by extracting a shared helper used by both
PonytailHookEvent::SessionStart and PonytailHookEvent::SubagentStart. The helper
should handle fetching active/default mode, the off-mode check, building
instructions, detecting the platform, and formatting/printing the hook output,
while allowing the hook name and any off-mode behavior differences to be passed
in as parameters.
- Around line 63-66: The active-state clearing call is inconsistent between the
PonytailAction::Off branch and the other path, so update the CLI logic to use a
single canonical entry point for clearing state. In the ponytail command
handling, make PonytailAction::Off and the code near the second clear call both
invoke the same function path, either ponytail::clear_active or
ponytail::state::clear_active, so the behavior is identical and the
implementation is consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e3cdd2e2-fbe7-44c6-8af9-2a02e643d422
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
Cargo.tomlcrates/agent-registry/Cargo.tomlcrates/agent-registry/src/detect.rscrates/agent-registry/src/lib.rscrates/agent-registry/src/registry.rscrates/ponytail/Cargo.tomlcrates/ponytail/src/config.rscrates/ponytail/src/detect.rscrates/ponytail/src/instructions.rscrates/ponytail/src/lib.rscrates/ponytail/src/platform.rscrates/ponytail/src/skill-audit.mdcrates/ponytail/src/skill-debt.mdcrates/ponytail/src/skill-gain.mdcrates/ponytail/src/skill-help.mdcrates/ponytail/src/skill-review.mdcrates/ponytail/src/skill.mdcrates/ponytail/src/state.rscrates/ponytail/src/sub_skills.rscrates/ponytail/src/switcher.rsdocs/superpowers/plans/2026-07-07-cli-refactor-mise.mdskills/coderabbit-fix/SKILL.mdsrc/agent_install.rssrc/agent_launch.rssrc/agents.rssrc/alias.rssrc/auth_runner.rssrc/cli/agents.rssrc/cli/alias.rssrc/cli/auth.rssrc/cli/coaching.rssrc/cli/cost.rssrc/cli/hook.rssrc/cli/init.rssrc/cli/mcp.rssrc/cli/mod.rssrc/cli/ponytail.rssrc/cli/uninstall.rssrc/cli/update.rssrc/main.rssrc/state.rs
|
|
||
| ## Target workspace structure | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the tree fence.
Markdownlint flags this fence; text keeps the diagram readable and avoids the lint failure.
Suggested fix
-```
+```text📝 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.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-07-cli-refactor-mise.md` at line 16, The tree
diagram fence in the plan document is missing a language tag, which triggers
markdownlint. Update the fenced block around the tree diagram to use a readable
tag like text so the diagram stays intact while satisfying the linter; locate
the affected fence in the markdown content near the tree diagram and adjust only
that fenced block.
Source: Linters/SAST tools
| Use GitHub CLI to get all review comments on the PR: | ||
|
|
||
| ```bash | ||
| gh pr view <PR_NUMBER> --repo getappz/agentflare --json reviews --jq '.reviews[].body' | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'skills/coderabbit-fix/SKILL.md'
wc -l skills/coderabbit-fix/SKILL.md
cat -n skills/coderabbit-fix/SKILL.md | sed -n '1,120p'Repository: getappz/agentflare
Length of output: 4559
🌐 Web query:
GitHub CLI gh pr view --json reviews inline review comments body reviews array comments endpoint
💡 Result:
The GitHub CLI command gh pr view does not provide first-class support for retrieving inline (line-specific) pull request review comments [1]. While the --json flag allows you to fetch various pull request details such as reviews, the reviews object returned contains only high-level information—such as the review body, state, and author—and does not include the associated inline discussion threads [2][3]. To access inline review comments for a pull request, you must use the gh api command to query the GitHub REST API directly [1]. The endpoint for retrieving all review comments on a pull request is [1]: GET /repos/{owner}/{repo}/pulls/{pull_number}/comments You can execute this via the CLI as follows [1]: gh api repos/{owner}/{repo}/pulls/{pull_number}/comments If you need to retrieve comments for a specific review, you can use the following endpoint [1]: GET /repos/{owner}/{repo}/pulls/{pull_number}/reviews/{review_id}/comments When using gh api, you can filter the output using the -q flag and a jq expression to extract the specific fields you need, such as the comment body, file path, and line number [1].
Citations:
- 1: https://eliteai.tools/agent-skills/using-github-api-with-gh-cli
- 2: Retrieving pull request reviews... cli/cli#3993
- 3: https://github.com/majiayu000/claude-skill-registry/blob/main/skills/development/gh-pr-view/SKILL.md
Fetch inline review comments too. gh pr view --json reviews only returns review bodies, so this workflow can miss line-level CodeRabbit comments. Switch to the pull-request comments endpoint instead.
Suggested fix
- gh pr view <PR_NUMBER> --repo getappz/agentflare --json reviews --jq '.reviews[].body'
+ gh api repos/getappz/agentflare/pulls/<PR_NUMBER>/comments --jq '.[].body'📝 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.
| Use GitHub CLI to get all review comments on the PR: | |
| ```bash | |
| gh pr view <PR_NUMBER> --repo getappz/agentflare --json reviews --jq '.reviews[].body' | |
| ``` | |
| Use GitHub CLI to get all review comments on the PR: | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/coderabbit-fix/SKILL.md` around lines 20 - 24, The current PR comment
fetch uses gh pr view with --json reviews, which only returns top-level review
bodies and can miss inline CodeRabbit comments. Update the comment-collection
step in the workflow/docs to use the pull request comments endpoint instead, and
adjust the command or script so it retrieves line-level review comments as well
as general review text. Reference the existing GitHub CLI usage in the SKILL.md
instructions so the replacement stays aligned with the current PR comment
gathering flow.
| 4. Run `cargo test ponytail` if touching ponytail code | ||
| 5. Mark todo complete |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'skills/coderabbit-fix/SKILL.md'
echo
wc -l skills/coderabbit-fix/SKILL.md
echo
cat -n skills/coderabbit-fix/SKILL.md | sed -n '45,115p'
echo
rg -n "cargo test ponytail|cargo test -p ponytail|ponytail" -S skills/coderabbit-fix/SKILL.mdRepository: getappz/agentflare
Length of output: 3110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'Cargo.toml' .
echo
rg -n '^\s*name\s*=\s*"(ponytail|agentflare|.*)"|^\s*\[workspace\]|\bponytail\b' -S Cargo.toml **/Cargo.toml 2>/dev/null || true
echo
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('Cargo.toml'):
txt = p.read_text(errors='ignore')
if 'ponytail' in txt or '[workspace]' in txt:
print(f'--- {p}')
for i, line in enumerate(txt.splitlines(), 1):
if 'ponytail' in line or '[workspace]' in line or 'name =' in line:
print(f'{i}: {line}')
PYRepository: getappz/agentflare
Length of output: 731
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cargo test --help | sed -n '1,120p' | rg -n 'TESTNAME|--package|-p|name' -n -SRepository: getappz/agentflare
Length of output: 403
Use cargo test -p ponytail here. cargo test ponytail filters test names instead of selecting the ponytail workspace package, so it can miss the crate under change. Same fix applies to the duplicate example below.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/coderabbit-fix/SKILL.md` around lines 57 - 58, The test command in the
ponytail guidance is using a name filter instead of selecting the workspace
package, so update the instruction to use the correct package-targeted cargo
test invocation in the relevant SKILL.md entry and the duplicate example. Refer
to the affected test guidance text around the ponytail note and replace the
existing cargo test form with the package-based one so it actually runs the
ponytail crate tests.
1 participant
Summary
Refactors agentflare into mise-style multi-crate workspace with modular CLI. Each subcommand gets its own file under src/cli/. Independent modules extracted into workspace crates.
Closes #44
Changes
Phase 1: CLI modularization
Phase 2: Workspace extraction
Test results
Summary by CodeRabbit