feat: pre-commit hook 설정 추가 (SwiftLint + action-validator)#39
Conversation
- .githooks/pre-commit: 커밋 전 자동 린트 검사 - SwiftLint: staged Swift 파일 검사 - action-validator: staged workflow YAML 검사 - Makefile: setup, lint, lint-swift, lint-actions 명령어 추가 - README.md, AGENTS.md: 개발 환경 설정 가이드 업데이트 Node.js 의존성 없이 순수 Git hooks 방식 사용 clone 후 'make setup' 한 줄로 설정 완료 Co-authored-by: Claude (Sisyphus, oMo) <no-reply@anthropic.com> Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Pull request overview
This PR adds Git pre-commit hook configuration using Git's native core.hooksPath feature to automatically run code quality checks before commits. The implementation replaces the previous scripts/setup-git-hooks.sh approach with a simpler Makefile-based setup.
Changes:
- Adds
.githooks/pre-commitscript that runs SwiftLint and action-validator on staged files - Adds Makefile with convenience targets for setup and manual linting
- Updates README.md and AGENTS.md documentation to reflect the new setup process
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
.githooks/pre-commit |
Pre-commit hook script that validates staged Swift and workflow files |
Makefile |
Provides make setup for Git hooks configuration and make lint commands |
README.md |
Updates contribution guide to use new make setup command |
AGENTS.md |
Adds pre-development setup instructions for the Git hooks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @echo "Running action-validator..." | ||
| @for f in .github/workflows/*.yml; do \ | ||
| echo "Validating $$f..."; \ | ||
| npx --yes @action-validator/cli "$$f"; \ |
There was a problem hiding this comment.
The Makefile's lint-actions target uses npx --yes @action-validator/cli, which requires Node.js and npm to be installed. This contradicts the PR description's claim of "No Node.js dependency" and "npm install 불필요". Either the description should be updated to clarify that Node.js is optional (only needed for workflow validation), or the implementation should be changed to not require Node.js/npx.
|
|
||
| lint-swift: | ||
| @echo "Running SwiftLint..." | ||
| @swiftlint lint CopilotMonitor/CopilotMonitor |
There was a problem hiding this comment.
The SwiftLint command here runs on the entire CopilotMonitor/CopilotMonitor directory, which is consistent with the full manual lint but potentially misleading when comparing to the pre-commit hook behavior. The pre-commit hook checks for staged files but also runs on the entire directory. For consistency and clarity, consider documenting that both run on the full directory, or update both to support file-specific linting.
| @@ -0,0 +1,81 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
The pre-commit hook script needs to be executable. Ensure that the file has executable permissions (chmod +x .githooks/pre-commit). Git should preserve these permissions when the file is committed with executable bits set. Verify that the file mode is 755 or similar when committing.
| for file in $STAGED_WORKFLOWS; do | ||
| echo "Validating $file..." | ||
| npx --yes @action-validator/cli "$file" | ||
| if [ $? -ne 0 ]; then | ||
| echo "❌ action-validator found issues in $file" | ||
| EXIT_CODE=1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
The STAGED_WORKFLOWS variable should be quoted in the for loop to properly handle filenames with spaces or special characters. Change for file in $STAGED_WORKFLOWS; do to while IFS= read -r file; do ... done <<< "$STAGED_WORKFLOWS" or ensure the variable is properly quoted. While workflow filenames are unlikely to contain spaces, this is a shell scripting best practice.
| for file in $STAGED_WORKFLOWS; do | |
| echo "Validating $file..." | |
| npx --yes @action-validator/cli "$file" | |
| if [ $? -ne 0 ]; then | |
| echo "❌ action-validator found issues in $file" | |
| EXIT_CODE=1 | |
| fi | |
| done | |
| while IFS= read -r file; do | |
| [ -z "$file" ] && continue | |
| echo "Validating $file..." | |
| npx --yes @action-validator/cli "$file" | |
| if [ $? -ne 0 ]; then | |
| echo "❌ action-validator found issues in $file" | |
| EXIT_CODE=1 | |
| fi | |
| done <<< "$STAGED_WORKFLOWS" |
| - **Pre-commit Hook**: Runs on `git commit` (install via `./scripts/setup-git-hooks.sh`) | ||
| - **Pre-commit Hook**: Runs on `git commit` (setup via `make setup`) | ||
| - SwiftLint for `.swift` files | ||
| - action-validator for `.github/workflows/*.yml` files |
There was a problem hiding this comment.
The documentation mentions "action-validator" but the CI workflows (ci.yml, lint.yml) use "actionlint". This inconsistency should be clarified to avoid confusion about which tool is used where.
| ``` | ||
| This configures pre-commit hooks for: | ||
| - **SwiftLint**: Checks Swift code style on staged `.swift` files | ||
| - **action-validator**: Validates GitHub Actions workflow files |
There was a problem hiding this comment.
The documentation mentions "action-validator" for validating GitHub Actions workflow files, but the CI workflows use "actionlint" for this purpose. This creates confusion about which tool is actually being used. The description should clarify that action-validator is used locally while actionlint is used in CI, or better yet, align both to use the same tool for consistency.
|
|
||
| if [ -n "$STAGED_SWIFT" ]; then | ||
| echo "Checking staged Swift files..." | ||
| swiftlint lint --quiet CopilotMonitor/CopilotMonitor |
There was a problem hiding this comment.
The SwiftLint command runs on the entire CopilotMonitor/CopilotMonitor directory, not just the staged Swift files. This is inconsistent with the check on line 25 which identifies staged Swift files. The hook should either:
- Pass the staged files to SwiftLint:
echo "$STAGED_SWIFT" | xargs swiftlint lint --quiet - Or remove the staged files check and always lint the entire directory
The old setup-git-hooks.sh script correctly linted only staged files, which is faster and more appropriate for a pre-commit hook.
| swiftlint lint --quiet CopilotMonitor/CopilotMonitor | |
| echo "$STAGED_SWIFT" | xargs swiftlint lint --quiet |
| if command -v npx >/dev/null 2>&1; then | ||
| for file in $STAGED_WORKFLOWS; do | ||
| echo "Validating $file..." | ||
| npx --yes @action-validator/cli "$file" |
There was a problem hiding this comment.
The pre-commit hook uses @action-validator/cli via npx, but the CI workflows (ci.yml and lint.yml) use actionlint for validation. This creates an inconsistency where different tools validate the same files locally vs in CI, potentially leading to discrepancies where one tool accepts what the other rejects. Consider using actionlint locally as well for consistency with the CI pipeline.
| if command -v npx >/dev/null 2>&1; then | ||
| for file in $STAGED_WORKFLOWS; do | ||
| echo "Validating $file..." | ||
| npx --yes @action-validator/cli "$file" | ||
| if [ $? -ne 0 ]; then |
There was a problem hiding this comment.
This pre-commit hook invokes npx --yes @action-validator/cli "$file", which downloads and executes a third-party CLI from the public npm registry on every commit without pinning a specific version or verifying integrity. If the @action-validator/cli package or the npm resolution for this name is compromised, an attacker can achieve code execution on developer machines, modify repository contents, or exfiltrate local secrets during commits. To reduce this supply-chain risk, pin the tool to a specific trusted version or vendored install and invoke that fixed binary instead of relying on on-demand npx resolution.
| @for f in .github/workflows/*.yml; do \ | ||
| echo "Validating $$f..."; \ | ||
| npx --yes @action-validator/cli "$$f"; \ |
There was a problem hiding this comment.
The lint-actions target uses npx --yes @action-validator/cli "$$f", which causes a third-party npm CLI to be downloaded and executed on demand without version pinning or integrity verification. A compromised @action-validator/cli release or npm registry attack would give an attacker code execution in developer environments when make lint-actions is run, allowing tampering with source code or stealing credentials. Prefer installing a pinned version of this tool (for example via a lockfile or vendored binary) and invoking that fixed artifact instead of relying on npx with an unpinned package name.
Summary
core.hooksPath방식 사용Changes
새 파일
.githooks/pre-commit: Pre-commit hook 스크립트.swift파일 검사.github/workflows/*.yml파일 검사Makefile: 개발 편의 명령어make setup: Git hooks 설정make lint: 전체 린트 실행make lint-swift: SwiftLint만 실행make lint-actions: action-validator만 실행문서 업데이트
README.md: Development Setup 가이드 업데이트AGENTS.md: Pre-Development Setup 섹션 추가Usage
Why this approach?
npm install불필요make setup한 줄로 완료git config core.hooksPath사용