Conversation
Entire-Checkpoint: 1016d272df11
Eliminates 2-3 redundant settings.Load() calls (each reading 2 JSON files + JSON parse) on the enable hot path by threading the localDev bool from callers that already have it. Cold paths (setupGitHook hidden command, EnsureSetup self-healing fallback) still call isLocalDev(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 13ed79ed3617
PR SummaryMedium Risk Overview Adds a new Written by Cursor Bugbot for commit 79846ca. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
mise.toml
Outdated
| results[name] = (ns / 1e6, bop, allocs) | ||
| except (ValueError, IndexError): | ||
| continue | ||
| return results |
There was a problem hiding this comment.
Benchmark parser discards all but last run
Medium Severity
The parse_benchmarks function stores results in a dict keyed by benchmark name, overwriting the value each iteration. With BENCH_COUNT defaulting to 6, each benchmark produces 6 output lines, but results[name] = (ns / 1e6, bop, allocs) keeps only the last one. This discards 5 of 6 data points, making the comparison unreliable — the whole point of running multiple iterations is statistical robustness. The previous benchstat tool properly aggregated all runs.
mise.toml
Outdated
|
|
||
| has_changes=false | ||
| trap 'git checkout "$current_branch" --quiet 2>/dev/null; [ "$has_changes" = true ] && git stash pop --quiet 2>/dev/null; rm -rf "$tmpdir"' EXIT | ||
| trap 'rm -rf "$tmpdir"' EXIT |
There was a problem hiding this comment.
Trap doesn't restore git branch or stash
Low Severity
The EXIT trap was changed to only rm -rf "$tmpdir", removing the git branch and stash restoration that the old trap performed. If the script is interrupted (e.g., Ctrl+C) while running benchmarks on the base ref, the user is left on the wrong branch with their uncommitted changes stashed and no indication of how to recover.
There was a problem hiding this comment.
Pull request overview
This PR updates developer tooling and hook installation logic to support local-dev vs installed-binary execution paths more explicitly, and adds benchmarking support for the entire enable hot path.
Changes:
- Refactors git hook installation/warnings to accept an explicit
localDevparameter and updates call sites/tests accordingly. - Updates the
bench:comparemise task to support configurable base refs/packages and to print comparisons without requiringbenchstat. - Adds a new benchmark for the non-interactive
entire enable --agent claude-codepath.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
mise.toml |
Revises bench:compare to compare against a configurable base ref and formats output via a small embedded script. |
cmd/entire/cli/strategy/hooks.go |
Changes InstallGitHook and command prefix selection to be driven by an explicit localDev flag. |
cmd/entire/cli/strategy/hooks_test.go |
Updates hook tests for the new InstallGitHook signature. |
cmd/entire/cli/strategy/manual_commit.go |
Passes local-dev mode into hook installation for manual-commit strategy setup. |
cmd/entire/cli/strategy/auto_commit.go |
Passes local-dev mode into hook installation for auto-commit strategy setup. |
cmd/entire/cli/strategy/hook_managers.go |
Threads localDev into warning text generation so examples match hook command style. |
cmd/entire/cli/strategy/hook_managers_test.go |
Updates warning tests for the new CheckAndWarnHookManagers signature. |
cmd/entire/cli/setup.go |
Passes localDev explicitly for hook installation/warnings; loads settings in setupGitHook to determine local-dev mode. |
cmd/entire/cli/setup_test.go |
Updates uninstall test to use the new InstallGitHook signature. |
cmd/entire/cli/bench_enable_test.go |
Adds a benchmark covering the non-interactive enable flow. |
cmd/entire/cli/agent/claudecode/hooks.go |
Updates a security-lint suppression comment to match repo-root-based path construction. |
| run_bench() { | ||
| local label="$1" out="$2" | ||
| echo "=== Benchmarking: $label ===" | ||
| go test -bench="$BENCH_PATTERN" -benchmem -run='^$' -count="$BENCH_COUNT" -timeout="$BENCH_TIMEOUT" "$BENCH_PKG" 2>/dev/null \ | ||
| | grep -E '^(Benchmark|goos:|goarch:|pkg:|cpu:)' > "$out" | ||
| local count | ||
| count=$(grep -c '^Benchmark' "$out" || true) | ||
| if [ "$count" -eq 0 ]; then | ||
| echo " ERROR: no benchmark results captured. Does the benchmark exist on this branch?" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
In run_bench(), the go test | grep ... > "$out" pipeline will cause the script to exit early under set -euo pipefail when grep finds no matches (exit code 1). That makes the subsequent count=$(grep -c ...) / friendly error path unreachable and breaks the intended behavior when a benchmark doesn't exist. Consider allowing an empty grep result (e.g., guard the pipeline) and handling the “no Benchmark lines” case explicitly.
mise.toml
Outdated
| tmpdir=$(mktemp -d) | ||
| new_out="$tmpdir/new.txt" | ||
| old_out="$tmpdir/old.txt" | ||
|
|
||
| has_changes=false | ||
| trap 'git checkout "$current_branch" --quiet 2>/dev/null; [ "$has_changes" = true ] && git stash pop --quiet 2>/dev/null; rm -rf "$tmpdir"' EXIT | ||
| trap 'rm -rf "$tmpdir"' EXIT | ||
|
|
||
| run_bench() { | ||
| local label="$1" out="$2" | ||
| echo "=== Benchmarking: $label ===" | ||
| go test -bench="$BENCH_PATTERN" -benchmem -run='^$' -count="$BENCH_COUNT" -timeout="$BENCH_TIMEOUT" "$BENCH_PKG" 2>/dev/null \ | ||
| | grep -E '^(Benchmark|goos:|goarch:|pkg:|cpu:)' > "$out" | ||
| local count | ||
| count=$(grep -c '^Benchmark' "$out" || true) | ||
| if [ "$count" -eq 0 ]; then | ||
| echo " ERROR: no benchmark results captured. Does the benchmark exist on this branch?" | ||
| return 1 | ||
| fi | ||
| echo " captured $count benchmark lines" | ||
| } | ||
|
|
||
| echo "=== Benchmarking current branch ($current_branch) ===" | ||
| go test -bench="$BENCH_PATTERN" -benchmem -run='^$' -count="$BENCH_COUNT" -timeout="$BENCH_TIMEOUT" ./... > "$new_out" 2>&1 || true | ||
| # Run on current branch first | ||
| new_out="$tmpdir/new.txt" | ||
| if ! run_bench "$current_branch" "$new_out"; then | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Check for uncommitted changes | ||
| # Stash if needed, switch to base, run, switch back | ||
| has_changes=false | ||
| if ! git diff --quiet || ! git diff --cached --quiet; then | ||
| has_changes=true | ||
| echo "Stashing uncommitted changes..." | ||
| git stash push -m "bench:compare auto-stash" | ||
| git stash push -q -m "bench:compare auto-stash" | ||
| fi | ||
|
|
||
| old_out="$tmpdir/old.txt" | ||
| echo "" | ||
| echo "=== Benchmarking base ($BASE_REF) ===" | ||
| git checkout "$BASE_REF" --quiet | ||
| go test -bench="$BENCH_PATTERN" -benchmem -run='^$' -count="$BENCH_COUNT" -timeout="$BENCH_TIMEOUT" ./... > "$old_out" 2>&1 || true | ||
|
|
||
| echo "" | ||
| echo "=== Switching back to $current_branch ===" | ||
| git checkout "$current_branch" --quiet | ||
| if [ "$has_changes" = true ]; then | ||
| git stash pop --quiet | ||
| git checkout "$BASE_REF" --quiet 2>/dev/null | ||
| if ! run_bench "$BASE_REF" "$old_out"; then | ||
| echo "" | ||
| echo "Benchmark does not exist on $BASE_REF. Showing current branch results only:" | ||
| echo "" | ||
| git checkout "$current_branch" --quiet 2>/dev/null | ||
| [ "$has_changes" = true ] && git stash pop --quiet |
There was a problem hiding this comment.
The EXIT trap now only removes the tempdir. If the script errors after stashing and/or checking out $BASE_REF (e.g., checkout failure, go test failure, Ctrl-C), it can leave the user on the wrong branch and/or with a stash entry that wasn’t popped. Consider restoring the previous branch and popping the stash in the EXIT trap (similar to the prior implementation), so cleanup happens on all exit paths.
mise.toml
Outdated
| go test -bench="$BENCH_PATTERN" -benchmem -run='^$' -count="$BENCH_COUNT" -timeout="$BENCH_TIMEOUT" "$BENCH_PKG" 2>/dev/null \ | ||
| | grep -E '^(Benchmark|goos:|goarch:|pkg:|cpu:)' > "$out" | ||
| local count | ||
| count=$(grep -c '^Benchmark' "$out" || true) | ||
| if [ "$count" -eq 0 ]; then | ||
| echo " ERROR: no benchmark results captured. Does the benchmark exist on this branch?" |
There was a problem hiding this comment.
go test ... 2>/dev/null | grep ... discards stderr and filters the output down to a few prefixes, which can hide the actual failure reason (compile error, missing package, etc.) and make failures look like “no benchmark results”. Consider preserving stderr (or printing it when the benchmark run fails) so users can diagnose why a benchmark didn’t run.
| go test -bench="$BENCH_PATTERN" -benchmem -run='^$' -count="$BENCH_COUNT" -timeout="$BENCH_TIMEOUT" "$BENCH_PKG" 2>/dev/null \ | |
| | grep -E '^(Benchmark|goos:|goarch:|pkg:|cpu:)' > "$out" | |
| local count | |
| count=$(grep -c '^Benchmark' "$out" || true) | |
| if [ "$count" -eq 0 ]; then | |
| echo " ERROR: no benchmark results captured. Does the benchmark exist on this branch?" | |
| go test -bench="$BENCH_PATTERN" -benchmem -run='^$' -count="$BENCH_COUNT" -timeout="$BENCH_TIMEOUT" "$BENCH_PKG" 2>"$out.stderr" \ | |
| | grep -E '^(Benchmark|goos:|goarch:|pkg:|cpu:)' > "$out" | |
| local count | |
| count=$(grep -c '^Benchmark' "$out" || true) | |
| if [ "$count" -eq 0 ]; then | |
| echo " ERROR: no benchmark results captured. Does the benchmark exist on this branch?" | |
| if [ -s "$out.stderr" ]; then | |
| echo " go test stderr:" | |
| sed 's/^/ /' "$out.stderr" | |
| fi |
mise.toml
Outdated
| # Pretty-print single-branch results | ||
| printf "%-40s %12s %12s %12s\n" "Benchmark" "ms/op" "B/op" "allocs/op" | ||
| printf "%-40s %12s %12s %12s\n" "----------------------------------------" "--------" "--------" "---------" | ||
| grep '^Benchmark' "$new_out" | while read -r name iters nsop _ bop _ aop _; do | ||
| ms=$(echo "scale=2; $nsop / 1000000" | bc) | ||
| printf "%-40s %10s ms %12s %12s\n" "$name" "$ms" "$bop" "$aop" | ||
| done | ||
| exit 0 | ||
| fi | ||
|
|
||
| git checkout "$current_branch" --quiet 2>/dev/null | ||
| [ "$has_changes" = true ] && git stash pop --quiet | ||
|
|
||
| # Both branches have results — show comparison table | ||
| echo "" | ||
| echo "=== Results (base=$BASE_REF vs current=$current_branch) ===" | ||
| echo "" | ||
| benchstat "$old_out" "$new_out" | ||
| python3 - "$old_out" "$new_out" "$BASE_REF" "$current_branch" <<'PYEOF' | ||
| import sys, re |
There was a problem hiding this comment.
This task now relies on external tools (python3 for comparison output and bc for the single-branch pretty-print path) but doesn’t check for their presence. If either is missing, the task will fail after doing work (and potentially after stashing/checkouts). Consider adding explicit dependency checks up front or implementing the formatting/conversion in a single tool to reduce prerequisites.
| @@ -176,7 +177,7 @@ func InstallGitHook(silent bool) (int, error) { | |||
| return 0, fmt.Errorf("failed to create hooks directory: %w", err) | |||
| } | |||
|
|
|||
| specs := buildHookSpecs(hookCmdPrefix()) | |||
| specs := buildHookSpecs(hookCmdPrefix(localDev)) | |||
| installedCount := 0 | |||
There was a problem hiding this comment.
InstallGitHook behavior now differs based on the new localDev parameter (writing go run ... vs entire into hook scripts), but the updated tests only exercise localDev=false. Add a test case that installs hooks with localDev=true and asserts the generated hook contents use the expected command prefix, to prevent regressions in local-dev setups.
Adds CWD-keyed caching to GetHooksDir() matching the existing paths.RepoRoot() pattern. Eliminates a redundant `git rev-parse --git-path hooks` subprocess on the enable hot path (~10ms saved). Benchmark: NewRepo 42ms → 32ms (-24%), ReEnable 38ms → 29ms (-24%) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 173efe2e6a72
OpenRepository() called GetWorktreePath() which spawns `git rev-parse --show-toplevel` without caching. paths.RepoRoot() runs the same command but with CWD-keyed caching, and the cache is already warm by the time OpenRepository() runs on the enable path. Benchmark: NewRepo 32ms → 22ms (-31%), ReEnable 29ms → 19ms (-34%) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: c60f1e4ff18d
Entire-Checkpoint: 02dce46af028
…rrupt Two fixes: - Python parser now collects all N runs per benchmark and reports the median instead of silently discarding N-1 data points - EXIT trap tracks git state (branch, stash) and restores on Ctrl+C or any error, preventing the user from being stranded on the wrong branch with stashed changes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e3d38ff20114
Verifies that localDev=true generates hooks with "go run" prefix and localDev=false generates hooks with "entire" prefix. Also tests that switching from localDev=true to false correctly updates hook contents. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4c4d31f36ce3
Replaces the 130-line bash+python bench:compare script with a clean ~50-line script that delegates to benchstat (Go's standard benchmark comparison tool). Handles proper statistical aggregation, confidence intervals, and p-values out of the box. Requires: go install golang.org/x/perf/cmd/benchstat@latest Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8cf6084d566c
Name output files after branches (without .txt extension) and cd into the tmpdir before calling benchstat, so columns show "main" and the branch name instead of full /var/folders/... paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: a0a1fae188cb


Some optimizations to the
entire enablecommand: