feat: add pre-commit framework with all linters#636
feat: add pre-commit framework with all linters#636jeremyeder wants to merge 1 commit intoambient-code:mainfrom
Conversation
Replace symlink-based git hooks with the pre-commit framework so every linter that CI runs also runs locally before commit. Hooks added: - File hygiene: trailing-whitespace, end-of-file-fixer, check-yaml, check-added-large-files, check-merge-conflict, detect-private-key - Python: ruff-format, ruff (check + fix) for runners/scripts - Go: gofmt, go vet, golangci-lint (per-module wrappers for backend, operator, public-api) - Frontend: ESLint via wrapper script - Branch/push protection: existing Python scripts, now invoked by pre-commit Go and ESLint use local wrapper scripts (scripts/pre-commit/) because Go has 3 separate modules and ESLint config lives in components/frontend. All wrappers skip gracefully if the toolchain is not installed. Also adds pre-commit to the runner Dockerfile and auto-installs hooks after repo clones in the state-sync hydrate script. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Claude Code ReviewSummaryThis PR successfully introduces the pre-commit framework to standardize local linting and enforce quality gates before commits. The implementation is well-architected with graceful fallbacks, proper module-aware linting for Go, and comprehensive coverage of all project components. The wrapper script pattern elegantly handles the multi-module Go setup and component-specific tooling. Overall Assessment: Ready to merge with minor improvements recommended. Issues by Severity🚫 Blocker IssuesNone identified. 🔴 Critical IssuesNone identified. 🟡 Major Issues1. Missing executable permissions verification in CI The pre-commit wrapper scripts (gofmt-check.sh, go-vet.sh, golangci-lint.sh, eslint.sh) are correctly set as executable (+x) locally, but there is no CI check to ensure they remain executable after future commits. Recommendation: Add the 2. Missing test coverage documentation The test plan shows unchecked items:
Recommendation: Complete test plan verification before merge or document why these are deferred. 🔵 Minor Issues1. Hardcoded timeout in golangci-lint
Recommendation: Make timeout configurable via environment variable. 2. CLAUDE.md checklist duplication The pre-commit checklist mentions "Pre-commit hooks pass" which is now redundant since hooks run automatically. Recommendation: Update checklist items to clarify hooks run automatically. 3. Missing documentation for local override Developers may want to skip specific hooks locally (e.g., golangci-lint during rapid iteration). Recommendation: Document SKIP usage in CONTRIBUTING.md. Positive Highlights✅ Graceful degradation: All wrapper scripts check for tool availability and skip gracefully if not installed. ✅ Module-aware Go linting: The ✅ Security-conscious: Includes ✅ Comprehensive coverage: Covers all project components (Python, Go, frontend, YAML) with appropriate tooling. ✅ Automatic installation: The ✅ Clean migration: Old symlink-based hooks are removed automatically during installation. ✅ Consistent with CI: Hooks mirror what GitHub Actions runs, reducing "works on my machine" issues. ✅ Documentation quality: CLAUDE.md updates are thorough and include practical examples. RecommendationsHigh Priority
Medium Priority
Low Priority
Architecture Alignment✅ Follows CLAUDE.md development standards Final VerdictLGTM with minor improvements recommended ✅ This is a high-quality implementation that significantly improves code quality enforcement. The identified issues are minor and do not block merge. Consider addressing the test plan completion and documentation enhancements post-merge if time-constrained. 🤖 Review completed by Claude Code using platform repository standards 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Summary
make linttarget to run all hooks manuallyHooks included:
Go and ESLint use local wrapper scripts (
scripts/pre-commit/) because Go has 3 separate modules and ESLint config lives incomponents/frontend/. All wrappers skip gracefully if the toolchain is not installed.Files Changed
.pre-commit-config.yamlscripts/pre-commit/{gofmt-check,go-vet,golangci-lint,eslint}.shscripts/install-git-hooks.shMakefilesetup-hooks,remove-hooks; addedlintcomponents/runners/claude-code-runner/Dockerfilepre-committo pip installcomponents/runners/state-sync/hydrate.shCLAUDE.mdCONTRIBUTING.mdscripts/git-hooks/README.mdTest plan
make setup-hooksinstalls pre-commit and registers hookspre-commit run --all-filesruns all hooks (trailing-whitespace and end-of-file-fixer auto-fix pre-existing issues; all other hooks pass on clean files)pre-commit run branch-protectionpasses on feature branchpre-commit run check-yaml --all-filespasses (with--allow-multiple-documentsfor K8s manifests)make remove-hookscleanly uninstallsgit commit🤖 Generated with Claude Code