Skip to content

feat: add PackageManagerTool for dependency management - #1196

Closed
LifeJiggy wants to merge 1 commit into
Twigpine:mainfrom
LifeJiggy:tool/package-manager
Closed

LifeJiggy wants to merge 1 commit into
Twigpine:mainfrom
LifeJiggy:tool/package-manager

Conversation

@LifeJiggy

Copy link
Copy Markdown
Contributor

Summary

  • what changed: Added PackageManagerTool - a new tool to manage project dependencies across npm, pip, go, cargo, bun, and brew with actions: install, update, remove, list, audit, outdated
  • why it changed: Developers need to manage dependencies while working in Claude. Instead of switching to terminal, they can run package commands directly. Auto-detects package manager from lockfiles, validates input per manager for security.

Impact

  • user-facing impact: Users can run npm/pip/go/cargo/bun/brew commands directly in Claude. Supports install, update, remove, list, audit, outdated with proper permission prompts for destructive actions.
  • developer/maintainer impact: New tool adds to codebase. Input validation per manager prevents command injection.

Testing

  • bun run build
  • bun run smoke
  • focused tests: bun test src/tools/PackageManagerTool/ (15 pass)

Notes

  • provider/model path tested: N/A
  • screenshots attached (if UI changed): N/A
  • follow-up work or known limitations: Could add support for more package managers (composer, etc.)

New tool to manage project dependencies across multiple package managers:
- npm, pip, go, cargo, bun, brew
- Actions: install, update, remove, list, audit, outdated
- Auto-detects package manager from lockfiles
- Input validation per manager for security
- Permission prompts for destructive actions
- Timeout handling (120s)

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [P1] Register the new tool with the runtime tool list
    src/tools.ts:182
    The PR adds PackageManagerTool, but it is never imported into src/tools.ts or appended to getAllBaseTools(). getTools() is built from that list, and the only references I can find are inside the new tool/test/prompt files, so users will never see or be able to call this tool despite the PR's stated user-facing impact. Please wire the tool into the same registry path as the other built-ins and add a regression test that getToolsForDefaultPreset() or getTools() includes PackageManager.

  • [P2] Show the exact manager/path/command in permission prompts
    src/tools/PackageManagerTool/PackageManagerTool.ts:140
    The permission prompt only says Run "install" on lodash?, but this tool can run different binaries, mutate different working directories via path, and change behavior with flags such as global, dev, or dryRun. For dependency management, the user needs to approve the actual command context, not just the high-level action/package list. Please build and display the resolved manager, target path, and arguments in the permission message before executing.

  • [P2] Recommit the new source files as UTF-8 text
    src/tools/PackageManagerTool/PackageManagerTool.ts:1
    The three new TypeScript files are committed as UTF-16 LE, so GitHub and git diff --numstat classify them as binary (Binary files /dev/null and ... differ). That prevents normal inline review, hides future diffs/blame from common text tooling, and even git grep skips these files. Please convert PackageManagerTool.ts, PackageManagerTool.test.ts, and prompt.ts to the repo's normal UTF-8 encoding before merge.

@gnanam1990

Copy link
Copy Markdown
Collaborator

Thanks for this — a unified package-manager tool across npm/pip/cargo/etc. is a useful idea, and using spawnSync with array args (no shell) is exactly the right call for avoiding injection.

Reviewed independently alongside @jatmn's review, and his three points are correct: the tool isn't registered in getAllBaseTools() so it can't be invoked yet; the permission prompt (PackageManagerTool.ts:143) doesn't surface the resolved manager, target path, or flags, which matters for a tool that runs install/remove commands; and the three new files are committed as UTF-16 LE/CRLF, so git treats them as binary and they can't be reviewed inline.

If you can address those and re-push, I'd be happy to take another pass — the underlying approach is sound and close.

@Vasanthdev2004

Copy link
Copy Markdown
Collaborator

Closing as Bulk Tool Addition

This PR is being closed because it follows the same pattern as your previous bulk tool PRs (#1139, #1131, #1130, #1129, #1128) which were also closed.

Why we're closing:

  1. No prior discussion — Adding new tools requires maintainer alignment first
  2. Bulk additions create review burden — Each tool should be a separate PR with focused review
  3. Not aligned with product vision — We're not adding arbitrary tools without clear use cases
  4. 25k+ star project — We need to maintain quality and cohesion

How to contribute tools properly:

  1. Open an issue first to discuss the tool's value and design
  2. Get maintainer approval before writing code
  3. Submit one tool per PR with focused review
  4. Ensure the tool aligns with OpenClaude's core workflow

If you believe these tools are valuable, please open a discussion issue first.

@Vasanthdev2004

Copy link
Copy Markdown
Collaborator

Closed — bulk tool addition without prior maintainer discussion.

LifeJiggy added a commit to LifeJiggy/openclaude that referenced this pull request May 19, 2026
…wigpine#1196)

P1 (use-input.ts:64-68): skip setRawMode(true) on isActive false->true
when a deferred reset is pending, preventing counter over-increment
that leaked raw mode on final unmount. Test updated to assert
balanced 1-then-1 call pattern (no redundant setRawMode(true)).

P2 (package.json, externals.ts): move @testing-library/react-hooks
from dependencies to devDependencies; remove from INTENTIONALLY_BUNDLED.
kevincodex1 pushed a commit that referenced this pull request Jun 3, 2026
* fix: preserve raw mode across component re-renders (issue #843)

* fix(input): only reset raw mode on explicit isActive=false, not on MCP re-render churn (issue #843)

* fix: balance raw mode for isActive false transitions + add regression test

Fixes the issue where cleanup closes over stale isActive=true and returns
early without calling setRawMode(false), leaving rawModeEnabledCount
incremented after UI no longer has active useInput.

Changes:
- Use a ref to track whether raw mode was actually enabled
- Check the ref in cleanup instead of stale isActive closure value
- Add 6 regression tests covering the true->false/unmount paths

Addresses jatmn's review feedback: 'fix raw mode balance for isActive: false transitions'

* fix(input): debounce raw-mode reset to survive MCP re-render churn (issue #843)

* fix: add react-test-renderer dep and fix use-input test for CI

- Add react-test-renderer devDependency (required by @testing-library/react-hooks)
- Add @testing-library/react-hooks to INTENTIONALLY_BUNDLED in externals.ts
- Fix use-input.test.ts 'MCP re-render churn' test to use isActive rerender
  instead of separate renderHook calls (refs don't persist across instances)

* fix: address P1 raw-mode counter imbalance and P2 test-dep scope (PR #1196)

P1 (use-input.ts:64-68): skip setRawMode(true) on isActive false->true
when a deferred reset is pending, preventing counter over-increment
that leaked raw mode on final unmount. Test updated to assert
balanced 1-then-1 call pattern (no redundant setRawMode(true)).

P2 (package.json, externals.ts): move @testing-library/react-hooks
from dependencies to devDependencies; remove from INTENTIONALLY_BUNDLED.
hotmanxp pushed a commit to hotmanxp/openclaude that referenced this pull request Jun 5, 2026
… (Twigpine#1198)

* fix: preserve raw mode across component re-renders (issue Twigpine#843)

* fix(input): only reset raw mode on explicit isActive=false, not on MCP re-render churn (issue Twigpine#843)

* fix: balance raw mode for isActive false transitions + add regression test

Fixes the issue where cleanup closes over stale isActive=true and returns
early without calling setRawMode(false), leaving rawModeEnabledCount
incremented after UI no longer has active useInput.

Changes:
- Use a ref to track whether raw mode was actually enabled
- Check the ref in cleanup instead of stale isActive closure value
- Add 6 regression tests covering the true->false/unmount paths

Addresses jatmn's review feedback: 'fix raw mode balance for isActive: false transitions'

* fix(input): debounce raw-mode reset to survive MCP re-render churn (issue Twigpine#843)

* fix: add react-test-renderer dep and fix use-input test for CI

- Add react-test-renderer devDependency (required by @testing-library/react-hooks)
- Add @testing-library/react-hooks to INTENTIONALLY_BUNDLED in externals.ts
- Fix use-input.test.ts 'MCP re-render churn' test to use isActive rerender
  instead of separate renderHook calls (refs don't persist across instances)

* fix: address P1 raw-mode counter imbalance and P2 test-dep scope (PR Twigpine#1196)

P1 (use-input.ts:64-68): skip setRawMode(true) on isActive false->true
when a deferred reset is pending, preventing counter over-increment
that leaked raw mode on final unmount. Test updated to assert
balanced 1-then-1 call pattern (no redundant setRawMode(true)).

P2 (package.json, externals.ts): move @testing-library/react-hooks
from dependencies to devDependencies; remove from INTENTIONALLY_BUNDLED.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants