Added pre-commit husky hooks that to enforce project standards

[reachraza]

No description provided.

[claude]

Code Review: Pre-commit Hooks Implementation

Thank you for implementing pre-commit hooks to enforce project standards! I have reviewed this PR and have the following feedback:

🔴 Critical Issues

1. Pre-commit Hook Will Block All Commits

The current .husky/pre-commit hook runs the full test suite on every commit: npm run test

Problems:

• Running the entire test suite on every commit will significantly slow down development workflow
• Developers will be blocked from committing even when working on unrelated changes
• This creates a poor developer experience and may encourage bypassing hooks with --no-verify

Recommendation: Only run tests for changed files or remove this line entirely. Most teams run full tests in CI only.

2. Full TypeScript Type Check on Every Commit

npm run lint runs type checking across 3 TypeScript configs (tsconfig.lint.json, tsconfig.main.json, tsconfig.cli.json)

Problems:

• This performs full type checking across the entire codebase
• Can take 10-30+ seconds on large codebases, blocking every commit
• Developers already get type checking feedback from their IDEs

Recommendation: Remove full type checking from pre-commit. Run in CI instead.

⚠️ Major Issues

3. Massive Formatting Changes (84k+ line changes)

This PR reformats ~100 files with 84,233 additions and 82,753 deletions.

Problems:

• Makes code review nearly impossible
• Will cause massive merge conflicts for any open PRs
• Mixes infrastructure changes (husky setup) with formatting changes
• Git blame history becomes less useful

Recommendation:
Split this into two separate PRs:

1. PR Internal Logging #1: Add husky + lint-staged configuration only (no formatting)
2. PR Add Claude Code GitHub Workflow #2: Run formatting across codebase after PR Internal Logging #1 is merged

Consider adding a .git-blame-ignore-revs file to preserve blame history.

4. Missing Husky Installation Verification

New contributors may not have hooks set up properly if npm install fails to run the prepare script.

Recommendation: Document in README or CONTRIBUTING.md that developers should run npm run prepare after cloning.

ℹ️ Positive Notes

5. lint-staged Configuration Looks Good ✅

The lint-staged configuration in package.json is well-structured - only formatting/linting staged files is the right approach.

📋 Summary

Must Fix Before Merge:

• Remove or optimize npm run test from pre-commit hook
• Remove or optimize npm run lint from pre-commit hook
• Split PR into two: (1) infrastructure changes, (2) formatting changes

Should Fix:

• Add documentation for new developers

Recommended Minimal Pre-commit Hook:

# Run lint-staged for formatting and linting on staged files only
npx lint-staged

# Everything else (tests, type checking) should run in CI

This approach ensures code style consistency and fixes common issues automatically while keeping commits fast (typically under 10 seconds).

Overall Assessment: The intent behind this PR is excellent - enforcing code quality standards is important! However, the implementation needs adjustment to avoid creating a poor developer experience. The main issue is running expensive operations (full tests, full type checking) on every commit rather than just on staged files or in CI.

[claude]

Pull Request Review: Pre-commit Hooks & Code Standards

Thank you for this contribution! This PR adds important developer tooling to enforce project standards. Here's my detailed review:

---

✅ Strengths

1. Excellent Developer Experience Improvements

• EditorConfig: Ensures consistent formatting across different editors
• Shared VS Code Settings: Makes onboarding smoother with recommended extensions and auto-formatting
• Husky Pre-commit Hooks: Catches formatting/linting issues before commit, reducing CI failures
• lint-staged: Smart - only processes staged files, keeping commits fast

2. Good Configuration Consistency

• .editorconfig, .prettierrc, and .vscode/settings.json all align on tabs (indent_style: tab, useTabs: true)
• Proper separation of concerns: Prettier for formatting, ESLint for code quality
• eslint-config-prettier correctly disables conflicting ESLint rules

3. Critical Bug Fix

The refactor of SshCommandRunner.ts (lines 110-112) fixes a race condition where resolveSshPath() and getExpandedEnv() were called inside the Promise constructor but after async setup. Moving these calls before the Promise is a good fix.

---

⚠️ Issues & Concerns

1. Missing Pre-commit Script (Critical)

The PR adds .husky/pre-commit but doesn't install Husky properly. The hooks won't work until Husky is initialized.

Issue: package.json has "prepare": "husky", but this should be:

"prepare": "husky install" 

Or (if using Husky v9+):

npm run prepare

Impact: The pre-commit hook will silently fail for anyone who clones the repo until they manually run husky install.

Recommendation: Add initialization to the PR or document it in CONTRIBUTING.md.

---

2. Incomplete VS Code Exclusion (Minor)

The .gitignore change allows .vscode/settings.json and .vscode/extensions.json to be tracked, but other important files like .vscode/launch.json or .vscode/tasks.json might get added accidentally.

Current:

.vscode/*
!.vscode/settings.json
!.vscode/extensions.json

Suggestion: Be explicit about what's ignored:

# .vscode/ - track shared settings, ignore personal files
.vscode/*
!.vscode/settings.json
!.vscode/extensions.json
!.vscode/.gitkeep

---

3. Missing Documentation Updates

CONTRIBUTING.md mentions running npm run lint and npm run lint:eslint manually before commits, but doesn't mention:

• The new pre-commit hooks that auto-run these checks
• How to bypass hooks (e.g., git commit --no-verify for emergencies)
• How to manually run lint-staged: npx lint-staged

Recommendation: Update CONTRIBUTING.md with a "Pre-commit Hooks" section explaining the new workflow.

---

4. lint-staged Config Could Be More Robust

The current config in package.json:

"lint-staged": {
  "src/**/*.{ts,tsx}": [
    "prettier --write",
    "eslint --fix"
  ],
  "src/**/*.{json,css,md}": [
    "prettier --write"
  ]
}

Issues:

• Doesn't handle failures gracefully
• ESLint fixes might introduce changes that need re-staging

Suggestion: Add a final git add step to ensure fixed files are staged:

"lint-staged": {
  "src/**/*.{ts,tsx}": [
    "prettier --write",
    "eslint --fix",
    "git add"
  ],
  "src/**/*.{json,css,md}": [
    "prettier --write",
    "git add"
  ]
}

Note: Newer versions of lint-staged auto-stage changes, so verify with your version (v16.2.7).

---

5. SSH Runner Refactor Clarity

The SshCommandRunner.ts changes are good but could be clearer:

Before: Async calls inside Promise constructor (potential race)
After: Async calls before Promise constructor (correct)

Recommendation: Since this is a behavior fix (not just formatting), consider:

• Adding a comment explaining why these must be resolved before the Promise
• Mentioning this fix in the PR description (it's currently hidden in "formatting changes")

---

🔍 Testing Recommendations

1. Verify Husky Installation

# After merging, new contributors should run:
npm install
# Verify hook is installed:
ls -la .git/hooks/pre-commit

2. Test Pre-commit Hook

# Make a formatting error
echo "const foo = 'bar'  " >> src/renderer/App.tsx
git add src/renderer/App.tsx
git commit -m "test"
# Should auto-fix trailing space and fail if there are linting errors

3. Test Across Editors

• Open in VS Code (should auto-format on save)
• Open in Vim/Neovim with EditorConfig plugin (should use tabs)
• Verify WebStorm/IntelliJ respects EditorConfig

---

📋 Checklist for Merge

• Fix package.json prepare script: "prepare": "husky install"
• Update CONTRIBUTING.md with pre-commit hooks section
• Verify lint-staged auto-stages fixed files (or add git add manually)
• Test Husky installation on a fresh clone
• Document the SSH runner bugfix in PR description
• Consider adding a .husky/_/.gitignore if not already present

---

🎯 Final Verdict

Approve with changes requested. This is a valuable addition that will improve code quality and consistency, but needs:

1. Husky installation fix (critical)
2. Documentation updates (important)
3. Minor config tweaks (optional but recommended)

The SSH runner refactor is a good catch, but should be highlighted as a bugfix rather than buried in formatting changes.

---

📚 Additional Resources

• Husky Documentation
• lint-staged Best Practices
• EditorConfig Specification

Let me know if you'd like help with any of these changes!

[claude]

PR Review: Pre-commit Hooks Setup

Overall Assessment: ✅ Approved with Minor Suggestions

This PR adds essential development tooling to enforce code quality standards automatically. The implementation is solid and follows best practices.

---

🎯 Strengths

1. Well-structured pre-commit setup

   • Proper separation of concerns: formatting/linting in pre-commit, tests in CI
   • Uses lint-staged to only process changed files (performance-friendly)
   • Clear documentation in CONTRIBUTING.md

2. Good VSCode integration

   • Shared editor settings for consistent team experience
   • Recommended extensions list
   • Proper .gitignore updates to track shared settings while excluding personal ones

3. EditorConfig coverage

   • Comprehensive file type handling
   • Consistent with existing .prettierrc (tabs, 2-space tab width)
   • Smart exclusion of trailing whitespace in markdown

4. Clean refactoring in SshCommandRunner.ts

   • Moving await resolveSshPath() outside the Promise constructor is correct
   • Eliminates anti-pattern of async operations in Promise executor
   • No functional changes, just better code hygiene

---

🔍 Code Quality Observations

✅ Excellent

• eslint-config-prettier integration: Properly prevents ESLint/Prettier conflicts
• Husky setup: Automatic installation via prepare script
• lint-staged config: Correct file patterns and tool ordering (Prettier → ESLint)

⚠️ Minor Suggestions

1. Missing .prettierignore file (optional)
   While not critical, consider adding one to exclude:

   dist/
   release/
   node_modules/
   coverage/
   *.min.js
   

2. EditorConfig: indent_size with tabs (line 14)
   When using indent_style = tab, indent_size is typically redundant. The value 2 here just sets how tabs display, but doesn't affect actual indentation. This is fine but potentially confusing.

3. VSCode settings: Potential redundancy
   .vscode/settings.json line 7-9 duplicates what's in .editorconfig. Modern VSCode reads EditorConfig automatically, so these might be unnecessary:

   "editor.tabSize": 2,
   "editor.insertSpaces": false,
   "editor.detectIndentation": false,

   However, keeping them is totally fine for explicitness.

4. Consider adding .husky/commit-msg (future enhancement)
   For commit message linting (conventional commits, etc.) if the project adopts that standard later.

---

🔒 Security

✅ No security concerns. All changes are development tooling only and don't affect runtime code.

---

🐛 Potential Issues

❌ Critical: Husky installation may fail in some scenarios

The "prepare": "husky" script (line 38 in package.json) will run:

• ✅ After npm install locally
• ✅ In CI after installing dependencies
• ❌ BUT: It might fail in CI environments that don't have a .git directory or in Docker builds

Recommendation: Make the prepare script more resilient:

"prepare": "husky || true"

Or use a conditional script:

"prepare": "[ -d .git ] && husky || true"

This prevents build failures in environments where Git hooks aren't needed (production Docker images, etc.).

---

⚡ Performance

✅ Excellent performance considerations:

• lint-staged only processes staged files (not entire codebase)
• Pre-commit hook is fast and won't slow down commits significantly
• Proper exclusion patterns in VSCode settings (lines 37-41)

---

🧪 Test Coverage

N/A - This PR is purely development tooling configuration. No runtime code changes require tests.

The SshCommandRunner.ts refactoring maintains identical behavior, so existing tests should suffice.

---

📝 Documentation

✅ Excellent documentation:

• Clear explanation in CONTRIBUTING.md of how hooks work
• Emergency bypass instructions included
• Manual lint-staged command documented

Minor suggestion: Consider adding a troubleshooting section for common issues:

• "Hooks not running" → Check if .husky has executable permissions
• "Wrong tool version" → Ensure npx is using local node_modules

---

🎨 Style & Conventions

✅ All changes follow existing repository patterns:

• Matches .prettierrc settings (tabs, single quotes)
• Consistent with existing npm script naming
• Follows CLAUDE.md guidance

---

🔄 SshCommandRunner.ts Changes

The refactoring is technically correct and improves code quality:

Before:

return new Promise(async (resolve) => {
  const sshPath = await resolveSshPath();
  // ...
});

After:

const sshPath = await resolveSshPath();
return new Promise((resolve) => {
  // ...
});

Why this matters:

• Async Promise constructors are an anti-pattern (ESLint warns about this)
• The function was already async, so no need for async inside Promise executor
• Improves stack traces if resolveSshPath() throws

---

✅ Final Recommendation

LGTM with one fix needed:

1. ✅ Approve and merge after addressing the Husky prepare script resilience issue
2. ✅ All other suggestions are optional quality-of-life improvements
3. ✅ No breaking changes, no security issues, no test failures expected

Suggested action:
Update line 38 in package.json:

"prepare": "husky || true"

Great work on improving the project's development experience! 🚀

---

Review generated by Claude Code (Sonnet 4.5)

[claude]

PR Review: Pre-commit Hooks with Husky and lint-staged

Thanks for this contribution! This is a solid addition that will help maintain code quality. Here's my detailed review:

✅ Strengths

1. Well-implemented tooling setup

   • Husky + lint-staged integration is clean and follows best practices
   • prepare script with || true fallback is smart for CI/Docker environments
   • Good documentation in CONTRIBUTING.md with troubleshooting section

2. EditorConfig alignment

   • .editorconfig matches existing .prettierrc settings (tabs, 2-space width)
   • Proper handling of different file types (JSON/YAML use spaces, code uses tabs)

3. VSCode improvements

   • Shared .vscode/settings.json enforces consistent formatting across team
   • Extension recommendations are helpful for new contributors
   • .gitignore properly updated to track these while excluding personal files

4. ESLint-Prettier integration

   • eslint-config-prettier added to prevent formatting conflicts
   • Correctly placed after other configs in eslint.config.mjs

5. Code refactoring

   • SshCommandRunner: Moving await resolveSshPath() outside Promise constructor is correct
   • Fixes potential issue where Promise executor shouldn't be async

🔍 Issues Found

1. *CRITICAL: .prettierignore removes .md

The PR removes *.md from .prettierignore. This is problematic because system prompts in src/prompts/*.md contain carefully crafted AI instructions that could break if reformatted.

Recommendation: Either revert this change and keep *.md ignored, OR use a more selective pattern like src/prompts/*.md

2. Missing lint-staged coverage

Current config only lints src/**/* but the repo has code in scripts/ and root-level config files.

Recommendation: Change pattern to *.{ts,tsx} and *.{js,mjs,cjs,json,css,md} to catch files anywhere in the repo.

3. Pre-commit hook permissions

Git doesn't always preserve execute permissions across clones. Consider automating the chmod in the prepare script.

🧪 Test Coverage

No tests for the SshCommandRunner refactoring. The refactoring changes async behavior (moving await outside Promise), which could affect error handling.

🎯 Summary

Overall, this is a valuable contribution that will improve the development workflow. The main concern is the markdown formatting change - please address the .prettierignore issue for system prompts before merging.

Let me know if you'd like me to clarify any points!