Skip to content

feat: add LintTool and UnitTestTool for code quality - #1197

Closed
LifeJiggy wants to merge 1 commit into
Twigpine:mainfrom
LifeJiggy:tool/code-quality
Closed

LifeJiggy wants to merge 1 commit into
Twigpine:mainfrom
LifeJiggy:tool/code-quality

Conversation

@LifeJiggy

Copy link
Copy Markdown
Contributor

Summary

  • what changed: Added LintTool and UnitTestTool for code quality. LintTool runs linters (eslint, prettier, ruff, golangci-lint, shellcheck). UnitTestTool runs unit tests (jest, pytest, vitest, go test, cargo test). Both auto-detect tool from config files.
  • why it changed: Developers need to check code quality and run tests while working in Claude. Instead of switching to terminal, they can run these commands directly with proper security validation.

Impact

  • user-facing impact: Users can run linters and unit tests directly in Claude. Supports auto-detection of tools from config files. Permission prompts for potentially destructive operations.
  • developer/maintainer impact: Two new tools add to codebase with input validation per tool type for security.

Testing

  • bun run build
  • bun run smoke
  • focused tests: bun test src/tools/LintTool/ src/tools/UnitTestTool/ (25 pass)

Notes

  • provider/model path tested: N/A
  • screenshots attached (if UI changed): N/A
  • follow-up work or known limitations: Could add more linters/test runners

LintTool:
- Run linters (eslint, prettier, ruff, golangci-lint, shellcheck, etc.)
- Auto-detects linter from config files
- Actions: check, fix, format

UnitTestTool:
- Run unit tests (jest, pytest, vitest, go test, cargo test, etc.)
- Auto-detects test runner from config
- Actions: run, coverage, watch mode

Both tools include:
- Input validation per tool for security
- Permission prompts for destructive actions
- Timeout handling

@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 tools so the model can call them
    src/tools.ts:183
    The PR adds LintTool and UnitTestTool, but src/tools.ts never imports them or includes them in getAllBaseTools(). Because the built-in tool pool is assembled from this list, neither Lint nor UnitTest is exposed to the model in normal CLI/SDK execution, so the advertised user-facing feature is currently unreachable. Please wire both tools into the tool registry (and any relevant presets/filtering surfaces) and add coverage that getTools() includes them.

  • [P2] Use the parent directory as cwd for existing file targets
    src/tools/LintTool/LintTool.ts:134
    The working directory fallback only uses dirname(targetPath) when the target looks like a file and does not exist. For the common case where the user asks to lint or test an existing file, workingDir becomes the file path itself, so spawnSync(..., { cwd: workingDir }) fails before the linter/test runner can run. The same pattern exists in src/tools/UnitTestTool/UnitTestTool.ts:95. Please treat existing file paths as file targets too, and keep cwd on the containing directory while passing the file path as the command argument.

@gnanam1990

Copy link
Copy Markdown
Collaborator

Thanks for this — built-in lint and test tooling is a nice quality-of-life addition, and the prompt structure is well thought out.

I reviewed it independently alongside @jatmn's feedback, and his findings are accurate: neither LintTool nor UnitTestTool is imported into src/tools.ts / getAllBaseTools(), so the feature isn't reachable as it stands, and the workingDir fallback in LintTool.ts:134 / UnitTestTool.ts:95 resolves to the file path itself for an existing-file target, which fails when passed as spawnSync's cwd (it must be a directory).

Once those are sorted, the next review will also want to look at command-execution and sandboxing safety and permission gating for tools that spawnSync external runners, and whether the two tools are better split into separate PRs for easier review. Flagging now so this doesn't sit idle waiting on us — the ball's with you, and happy to re-review promptly once it's updated.

@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.

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