Uh oh!
There was an error while loading. Please reload this page.
feat: interactive capability picker for bare add/remove - #58
Conversation
Let terminal users multi-select grillers/lenses with no arg, while non-TTY keeps a usage error and named-arg paths stay unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Warning Review limit reached
Next review available in:36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughBare ChangesCapability picker contracts and models
Interactive add flow
Interactive remove flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)Interactive addsequenceDiagram
participant CLI
participant Picker
participant Installer
participant Config
CLI->>Picker: Build and present available grouped capabilities
Picker-->>CLI: Return selected capabilities
CLI->>Installer: Install each selected capability
Installer->>Config: Persist accumulated capabilities
Interactive removesequenceDiagram
participant CLI
participant Picker
participant Confirmation
participant Filesystem
CLI->>Picker: Build and present installed grouped capabilities
Picker-->>CLI: Return selected capabilities
CLI->>Confirmation: Request one confirmation
Confirmation-->>CLI: Confirm deletion
CLI->>Filesystem: Delete selected capability directories
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.dev/features/capability-picker/GRILL.md:
- Line 60: Update the advisory verdict text in GRILL.md to use the canonical
/pharn-grill command name instead of /pharn-dev-grill, unless the repository
explicitly exposes both commands; keep the surrounding floor-gate guidance
unchanged.
- Around line 20-21: Update the review wording to refer specifically to the bare
interactive “pharn add” picker test rather than a named add test. Preserve the
requirement that the test verifies the final persisted capabilities contain all
selected picks, order-independent, not only that installCapabilityDirs or
writePharnConfig were called.
In `@docs/commands/remove.md`:
- Around line 19-24: Update the later --yes option description in the remove
command documentation to state that --yes remains a no-op and does not bypass
the interactive picker confirmation. Keep the description consistent with the
picker and confirmation behavior documented in the no-argument removal flow.
In `@README.md`:
- Line 98: Update the README command table’s `pharn remove` row to document the
optional capability argument and no-argument interactive multi-select behavior.
Align its wording with the `remove [capability]` usage in `src/index.ts` and
describe that multiple capabilities can be removed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bca5fd0-ebd5-449a-905c-7482010b80d5
📒 Files selected for processing (14)
.dev/features/capability-picker/GRILL.md.dev/features/capability-picker/PLAN.md.pharn/writes-scope.jsonCHANGELOG.mdREADME.mddocs/commands/add.mddocs/commands/remove.mdsrc/commands/add.tssrc/commands/remove.tssrc/index.tssrc/lib/capability-picker.tstests/add.test.tstests/capability-picker.test.tstests/remove.test.ts
| problem: "The whole feature's correctness hinges on add's picker threading the growing config through resolveArchetypeAdd, which persists pharn.config.json EACH iteration off the config passed in; a threading bug silently clobbers every pick but the last. The named add test must assert the FINAL persisted capabilities equals ALL picks (order-independent), not merely that installCapabilityDirs/writePharnConfig were called." | ||
| evidence: "PLAN.md:40 'installs each pick via the existing path & threads config so the final capabilities holds all picks'; PLAN.md:19 'loop the existing resolveArchetypeAdd per pick (config threaded in-memory)'." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Refer to the bare interactive add test, not a named add test.
These findings concern the picker path for bare pharn add; “named add test” could send reviewers to the wrong regression case.
Proposed wording
- problem: "The ... named add test must assert ...+ problem: "The ... bare interactive add test must assert ...- problem: "No named test pins add's partial-progress-on-error ...+ problem: "No add-picker test pins the partial-progress-on-error ...Also applies to: 47-49
🤖 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 @.dev/features/capability-picker/GRILL.md around lines 20 - 21, Update the
review wording to refer specifically to the bare interactive “pharn add” picker
test rather than a named add test. Preserve the requirement that the test
verifies the final persisted capabilities contain all selected picks,
order-independent, not only that installCapabilityDirs or writePharnConfig were
called.
| ## Verdict | ||
| **ADVISORY VERDICT: 5 concerns raised (0 blocking, 1 important, 4 minor) — for the human to weigh before/at review; `/pharn-dev-grill` gates nothing.** This is not "grill passed" and not a judgment that the plan is sound — it surfaces what the plan omits or understates; the floor gates (`/pharn-dev-build` spec-hash + `validate.mjs`, `/pharn-dev-regress`, `/pharn-dev-verify`) remain the only proceed/stop authorities. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the canonical advisory command name.
The supplied floor-check context refers to /pharn-grill; update /pharn-dev-grill here unless the repository intentionally exposes both commands.
🤖 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 @.dev/features/capability-picker/GRILL.md at line 60, Update the advisory
verdict text in GRILL.md to use the canonical /pharn-grill command name instead
of /pharn-dev-grill, unless the repository explicitly exposes both commands;
keep the surrounding floor-gate guidance unchanged.
| 2. With no argument **in a terminal**, opens an interactive multi-select picker (grouped by role) over | ||
| the capabilities you have installed, then asks for one confirmation listing your picks. With an | ||
| argument, resolves it to one installed capability. In a non-interactive context (CI, a pipe), | ||
| no-argument `pharn remove` does **not** prompt — it exits with a usage error (unless nothing is | ||
| installed, which is reported plainly). | ||
| 3. Deletes each selected capability's isolated directory and drops its entry from `capabilities`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the stale --yes description.
This section documents the new picker confirmation, but the later --yes paragraph still says removal has no confirmation prompt. State that --yes remains a no-op and does not bypass the interactive picker confirmation. As per coding guidelines, “Keep user-facing documentation synchronized with implemented behavior.”
🤖 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/commands/remove.md` around lines 19 - 24, Update the later --yes option
description in the remove command documentation to state that --yes remains a
no-op and does not bypass the interactive picker confirmation. Keep the
description consistent with the picker and confirmation behavior documented in
the no-argument removal flow.
Source: Coding guidelines
| | --------------------------- | ------------------------------------------------------------------------------- | | ||
| | `pharn init` | Detect archetypes and install the applicable capabilities (default) | | ||
| | `pharn add [capability]` | Add a capability, e.g. `a11y` or `lens:n-plus-one` (no arg: pick interactively) | | ||
| | `pharn remove <capability>` | Remove an installed capability (no arg: pick one interactively) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the remove row with the new no-arg multi-select behavior.
Two inconsistencies vs. the implementation and src/index.ts USAGE (remove [capability]): the argument is now optional (interactive no-arg mode), and the picker is a groupMultiselect that removes multiple capabilities, not one.
📝 Proposed fix
-| `pharn remove <capability>` | Remove an installed capability (no arg: pick one interactively) |+| `pharn remove [capability]` | Remove an installed capability (no arg: pick interactively) |📝 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.
| |`pharn remove <capability>`| Remove an installed capability (no arg: pick one interactively) | | |
| |`pharn remove [capability]`| Remove an installed capability (no arg: pick interactively)| |
🤖 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 `@README.md` at line 98, Update the README command table’s `pharn remove` row
to document the optional capability argument and no-argument interactive
multi-select behavior. Align its wording with the `remove [capability]` usage in
`src/index.ts` and describe that multiple capabilities can be removed.
Summary
pharn add/pharn removein a TTY open a grouped multi-select (grillers / lenses) and run the existing per-name install/remove path for each pickcapability-picker.ts, plus docs/CHANGELOG/testsTest plan
npm test(especiallytests/capability-picker.test.ts,tests/add.test.ts,tests/remove.test.ts)pharn add→ pick capabilities → installs each; empty selection is a no-oppharn remove→ multi-select → confirm → deletes each; decline/cancel leaves config alonepharn add/pharn removeexit with usage error (no prompt)pharn add a11y/pharn remove a11ybehavior unchangedMade with Cursor
Summary by CodeRabbit
New Features
pharn addandpharn removecommands.addlists uninstalled capabilities;removelists installed capabilities and confirms deletions once.Documentation
Bug Fixes