Skip to content

feat: add LintTool and UnitTestTool for code quality#1197

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

feat: add LintTool and UnitTestTool for code quality#1197
LifeJiggy wants to merge 1 commit into
Gitlawb: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
Copy link
Copy Markdown
Collaborator

@jatmn jatmn left a comment

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.

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.

3 participants