Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR removes the CLI Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
40-45: Optional DRY cleanup: reuseGO_RUN_ENVin test targets.
testandtest-raceduplicate env assignments already captured byGO_RUN_ENV.Suggested diff
test: `@mkdir` -p "$(GO_TEST_CACHE_DIR)" "$(GO_TEST_TMP_DIR)" - GOCACHE="$(GO_TEST_CACHE_DIR)" GOTMPDIR="$(GO_TEST_TMP_DIR)" go test ./... + $(GO_RUN_ENV) go test ./... test-race: `@mkdir` -p "$(GO_TEST_CACHE_DIR)" "$(GO_TEST_TMP_DIR)" - GOCACHE="$(GO_TEST_CACHE_DIR)" GOTMPDIR="$(GO_TEST_TMP_DIR)" go test -race ./... + $(GO_RUN_ENV) go test -race ./...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 40 - 45, The test targets duplicate the environment setup; refactor the test and test-race targets to use the existing GO_RUN_ENV variable instead of repeating GOCACHE and GOTMPDIR assignments. Modify the test and test-race rules to call GO_RUN_ENV (which should include GOCACHE="$(GO_TEST_CACHE_DIR)" GOTMPDIR="$(GO_TEST_TMP_DIR)") before the go test ./... command; update references to GO_TEST_CACHE_DIR and GO_TEST_TMP_DIR only within GO_RUN_ENV so the targets simply invoke GO_RUN_ENV go test ./... using the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Around line 40-45: The test targets duplicate the environment setup; refactor
the test and test-race targets to use the existing GO_RUN_ENV variable instead
of repeating GOCACHE and GOTMPDIR assignments. Modify the test and test-race
rules to call GO_RUN_ENV (which should include GOCACHE="$(GO_TEST_CACHE_DIR)"
GOTMPDIR="$(GO_TEST_TMP_DIR)") before the go test ./... command; update
references to GO_TEST_CACHE_DIR and GO_TEST_TMP_DIR only within GO_RUN_ENV so
the targets simply invoke GO_RUN_ENV go test ./... using the same behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39e26924-adca-44d3-8a6d-1ae29d82ed78
📒 Files selected for processing (11)
.gitignoreMakefileREADME.mdinternal/cli/cli.gointernal/cli/cli_integration_test.gointernal/cli/cli_unit_test.gointernal/engine/engine.gointernal/engine/engine_integration_test.gointernal/gitexec/gitexec_test.gointernal/hooks/hooks.gointernal/hooks/hooks_test.go
💤 Files with no reviewable changes (5)
- internal/hooks/hooks_test.go
- internal/hooks/hooks.go
- internal/cli/cli_unit_test.go
- internal/cli/cli.go
- internal/engine/engine.go
There was a problem hiding this comment.
Pull request overview
This PR focuses on stabilizing CI/local developer workflows by adjusting integration test fixtures, simplifying hook-related surface area, and making lint/test tooling more reproducible via repo-local caches and a pinned linter binary.
Changes:
- Update integration tests to use a dedicated include fixture file (
.test.worktreeinclude) instead of.worktreeinclude. - Pin and auto-install
golangci-lintinto.cache/binand run Go tooling with repo-localGOCACHE/GOTMPDIR. - Remove the obsolete
hookCLI command and related engine/hooks code and tests; adjust related docs and assertions.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Removes hook command docs and documents manual post-checkout hook setup; notes pinned linter install behavior. |
| Makefile | Adds repo-local Go cache/tmp dirs and installs a pinned golangci-lint into .cache/bin for make lint. |
| internal/hooks/hooks.go | Removes hook snippet generation helper (feature removed). |
| internal/hooks/hooks_test.go | Removes tests for hook snippet generation (feature removed). |
| internal/gitexec/gitexec_test.go | Adjusts stderr assertion by using an invalid git subcommand in a real repo. |
| internal/engine/engine.go | Removes HookPath functionality (feature removed). |
| internal/engine/engine_integration_test.go | Switches integration tests to use .test.worktreeinclude fixture file. |
| internal/cli/cli.go | Removes hook subcommand wiring and related helpers. |
| internal/cli/cli_unit_test.go | Removes unit tests for hook print (feature removed). |
| internal/cli/cli_integration_test.go | Updates CLI integration tests to pass --include .test.worktreeinclude; removes hook integration tests. |
| .gitignore | Ignores .cache/ for the new repo-local caches/binaries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Makefile
Outdated
| $(GOLANGCI_LINT): | ||
| @mkdir -p "$(GO_TEST_CACHE_DIR)" "$(GO_TEST_TMP_DIR)" "$(GO_BIN_DIR)" | ||
| GOBIN="$(GO_BIN_DIR)" $(GO_RUN_ENV) go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) | ||
|
|
||
| lint: $(GOLANGCI_LINT) | ||
| $(GO_RUN_ENV) $(GOLANGCI_LINT) run |
There was a problem hiding this comment.
The $(GOLANGCI_LINT) target is just the binary path, so if GOLANGCI_LINT_VERSION changes later, make lint may keep using the previously installed binary because the file already exists and the rule won’t re-run. Consider versioning the installed path (e.g., include the version in the filename) or writing a small stamp file keyed by $(GOLANGCI_LINT_VERSION) so bumps reliably trigger a reinstall.
internal/gitexec/gitexec_test.go
Outdated
| if !strings.Contains(msg, "is not a git command") { | ||
| t.Fatalf("error should include git stderr, got %q", msg) |
There was a problem hiding this comment.
This test matches a specific substring from Git’s stderr ("is not a git command"), which can vary across Git versions/builds and with localization settings, making the test potentially flaky. A more robust assertion would be to check that the error includes the invoked command context (already done) and that the stderr portion is non-empty / includes the unknown subcommand name, without depending on the exact phrasing.
| if !strings.Contains(msg, "is not a git command") { | |
| t.Fatalf("error should include git stderr, got %q", msg) | |
| if !strings.Contains(msg, "definitely-not-a-command") { | |
| t.Fatalf("error should mention unknown subcommand, got %q", msg) |
Summary
Testing
Summary by CodeRabbit
Breaking Changes
hooksubcommand and all hook path/print functionality.Documentation
make lintinstalls a pinned linter into the local cache on first run (network required).Chores
Tests
Repository
.cache/to .gitignore.