Conversation
PR Review: Add prek git hooksSummary: This PR adds IssuesCritical: Missing configuration file
The PR should include the config file defining which hooks to run, otherwise this change has no effect and potentially breaks the dev environment. Minor: flake.lock not updated
Minor: No error handling in shellHookIf shellHook = ''
prek install || echo "Warning: prek install failed (missing config file?)"
...
'';Or better, only run it when a config file is present: shellHook = ''
if [ -f .pre-commit-config.yaml ]; then
prek install
fi
...
'';Suggestions
VerdictThe intent is good — automating git hook setup via the Nix dev shell is a clean approach. However, the PR is incomplete without the hook configuration file. Merging as-is will likely break |
Code ReviewThis is a clean, focused PR that adds What's good
Issues / SuggestionsMinor:
|
| Area | Assessment |
|---|---|
| Code quality | Good - minimal, focused changes |
| Correctness | No issues identified |
| Security | Positive addition (detect-private-key) |
| Performance | Minor: unconditional prek install on shell entry |
| Test coverage | N/A (infrastructure change) |
The PR is ready to merge as-is. The suggestions above are minor improvements rather than blockers.
PR Review: Add prek git hooksThis PR adds two git hook tools — prek (pre-commit file checks) and lefthook (commit-msg and pre-push hooks). The overall goal is good: standardizing commit quality. Here are the findings: Bugs / Logic Issues1.
2. The 3.
Conflict with CLAUDE.md Conventions4.
The Design Considerations5. Parallel commit-msg hooks produce interleaved output
6. Subject length limit of 50 characters is quite strict The 50-char limit follows the conventional recommendation, but it's restrictive in practice. Looking at recent commits in this repo ("Add lefthook commit-msg and pre-push hooks" = 42 chars, "Add prek to nix dev shell packages" = 34 chars), descriptive commit messages can easily approach the limit. Consider raising to 72 characters or clearly documenting the expectation for contributors. 7. Consider TypeScript/Astro-aware pre-commit hooks in The current Minor Notes8. Two hook managers is fine, but worth a note in the README The comment at the top of 9. Confirm
Summary
The |
PR Review: Add prek git hooksOverall this is a solid addition — automating git hook setup via flake.nixPositive:
Issues:
prek.tomlPositive:
Issues:
Minor Nits
Summary
Issue #3 is the most important to resolve before merging — it will block Claude Code's automated commits that use the beads |
PR Review: Add prek git hooksGood addition of code quality tooling — prek is a solid choice for fast, Rust-based pre-commit hooks. The Nix derivation approach for pinning a specific binary version is well-executed. Here are some observations: SecurityGitHub Actions pinned to Using - uses: DeterminateSystems/nix-installer-action@v14 # or a specific SHA
- uses: DeterminateSystems/magic-nix-cache-action@v8Correctness
The commits in this PR ("Add prek git hooks", "Add prek.toml config", etc.) don't contain
This is likely idempotent if prek handles re-installs gracefully, but it adds noise on every shell entry. A small guard would help: prek install 2>/dev/null || trueMinor IssuesOff-by-one in
Complex inline awk/sh in The What's Good
SummaryMain actionable items before merging:
Overall a clean, well-scoped PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f76160152
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { id = "no-trailing-period", name = "No trailing period", language = "system", entry = "sh -c 'head -1 \"$1\" | grep -q \"\\\\.$\" && { echo \"Commit subject must not end with a period\"; exit 1; } || true' --", stages = ["commit-msg"] }, | ||
| { id = "github-issue-ref", name = "GitHub issue reference", language = "system", entry = "sh -c 'head -1 \"$1\" | grep -q \"#[0-9]\" || { echo \"Commit subject must reference a GitHub issue (e.g. #123)\"; exit 1; }' --", stages = ["commit-msg"] }, | ||
| { id = "max-message-length", name = "Max 500 chars total", language = "system", entry = "sh -c 'chars=$(wc -c < \"$1\"); [ \"$chars\" -le 500 ] || { echo \"Commit message too long ($chars chars, max 500)\"; exit 1; }' --", stages = ["commit-msg"] }, | ||
| { id = "no-branch-push", name = "No push to main/master", language = "system", entry = "sh -c 'branch=$(git symbolic-ref --short HEAD 2>/dev/null); case \"$branch\" in main|master|development) echo \"Direct push to $branch is not allowed\"; exit 1;; esac' --", stages = ["pre-push"] }, |
There was a problem hiding this comment.
The no-branch-push hook only inspects the local branch name via git symbolic-ref --short HEAD, so it blocks git push only when your current branch is main/master/development but still allows git push origin HEAD:main from any other branch. That means the rule labeled “No push to main/master” is bypassable and will not reliably prevent accidental pushes to those remote branches.
Useful? React with 👍 / 👎.
| { id = "subject-max-length", name = "Subject max 50 chars", language = "system", entry = "sh -c 'len=$(head -1 \"$1\" | wc -m); [ \"$len\" -le 51 ] || { echo \"Commit subject too long ($len chars, max 50)\"; exit 1; }' --", stages = ["commit-msg"] }, | ||
| { id = "body-max-length", name = "Body lines max 72 chars", language = "system", entry = "sh -c 'grep -v \"^#\" \"$1\" | tail -n +3 | awk \"length > 72 { print NR\\\": \\\"\\$0; found=1 } END { if (found) { print \\\"Body lines must be 72 chars or less\\\"; exit 1 } }\"' --", stages = ["commit-msg"] }, | ||
| { id = "no-trailing-period", name = "No trailing period", language = "system", entry = "sh -c 'head -1 \"$1\" | grep -q \"\\\\.$\" && { echo \"Commit subject must not end with a period\"; exit 1; } || true' --", stages = ["commit-msg"] }, | ||
| { id = "github-issue-ref", name = "GitHub issue reference", language = "system", entry = "sh -c 'head -1 \"$1\" | grep -q \"#[0-9]\" || { echo \"Commit subject must reference a GitHub issue (e.g. #123)\"; exit 1; }' --", stages = ["commit-msg"] }, |
There was a problem hiding this comment.
This check hard-requires #[0-9] in every commit subject, but the repository guidance requires Beads IDs in commit messages (for example Fix bug (bd-abc)). With hooks installed, commits that follow the documented Beads format are rejected during commit-msg, which creates a workflow-breaking mismatch between the enforced hook and the project’s stated commit convention.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80431e2900
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| shellHook = '' | ||
| if ! command -v bd &> /dev/null; then | ||
| echo "Installing beads (bd) for AI agent task tracking..." | ||
| go install github.com/steveyegge/beads/cmd/bd@latest | ||
| fi | ||
| echo " Beads: $(bd --version 2>/dev/null || echo 'run: go install github.com/steveyegge/beads/cmd/bd@latest')" | ||
| prek install | ||
| echo " Beads: $(br --version 2>/dev/null || echo 'not available on this platform')" |
There was a problem hiding this comment.
bd command in Nix shell
This shell setup no longer installs or exposes bd, and now only checks br, but repo automation still requires bd (for example loop.sh’s beads_check exits when command -v bd fails at lines 534-536). In a clean nix develop environment, core task-tracking workflows now fail immediately unless contributors manually install bd outside the declared dev shell.
Useful? React with 👍 / 👎.
1 participant
Summary
Test plan
nix develop --command prek run --all-filespasses locally🤖 Generated with Claude Code