chore: consolidate git hooks on prek and fix all pre-commit checks#705
Open
HagegeR wants to merge 20 commits intoNVIDIA:mainfrom
Open
chore: consolidate git hooks on prek and fix all pre-commit checks#705HagegeR wants to merge 20 commits intoNVIDIA:mainfrom
HagegeR wants to merge 20 commits intoNVIDIA:mainfrom
Conversation
uv run --with pyright omitted optional dev deps; pytest was unresolved in orchestrator/test_endpoint_validation.py. Align pre-push and pre-commit with nemoclaw-blueprint Makefile (uv run --extra dev --with pyright).
- husky-env: shellcheck shell=bash for SC2148 - Dockerfile: hadolint pragmas and merged pip install with pyyaml pin - vitest pre-commit: use repo-root node_modules vitest (avoid broken npx)
Husky sets core.hooksPath; prek refuses to install in that case. Still run prek install for clones without hooksPath.
- Add default_install_hook_types (pre-commit, commit-msg, pre-push) to .pre-commit-config.yaml - Simplify npm prepare to run prek install without repeated --hook-type flags - Align backup-workspace, brev-setup, and install-openshell with shfmt/shellcheck style
Remove .husky/ directory, scripts/husky-env.sh, and the lint-staged
dependency. All hook logic now lives in .pre-commit-config.yaml:
- pre-commit: formatters, linters, Vitest (unchanged)
- commit-msg: commitlint (unchanged)
- pre-push: tsc, pyright, plus prek-push-range which re-runs
pre-commit hooks on the outgoing commit range
The prepare script unconditionally runs `prek install` (no more
core.hooksPath guard). Contributors with a stale Husky hooksPath
should run: git config --unset core.hooksPath
Replace the ad-hoc make check (eslint + prettier + tsc + ruff) with npx prek run --all-files, which covers every hook in .pre-commit-config.yaml (shfmt, shellcheck, hadolint, gitleaks, SPDX headers, etc.) in addition to the existing linters. Pre-push stage hooks (tsc --noEmit, pyright) run in a second step with --skip prek-push-range to avoid recursion. make check now delegates to prek as well.
- Fix trailing whitespace and missing final newlines in docs, skills, CODE_OF_CONDUCT.md, and scripts/debug.sh. - Set executable bits on scripts with shebangs: install.sh, smoke-macos-install.sh, test-full-e2e.sh, lib/runtime.sh, migrations/snapshot.py. - Fix shellcheck SC2206 in install.sh (safe IFS+read -ra for version parsing) and remove unused SCRIPT_DIR in setup-spark.sh. - Add hadolint ignore pragmas to test Dockerfiles (DL3008, DL3013, DL3042, DL3059, DL4006 — test images don't need version pinning). - Add SPDX license headers to __init__.py, test-double-onboard.sh, test-inference-local.sh, test-inference.sh, test-full-e2e.sh.
prek already runs pre-commit-stage hooks during pre-push, so the prek-push-range hook caused a duplicate pass. Remove it. Also fix: test-full-e2e.sh exec bit, hadolint SC2086/SC2038 ignores in test Dockerfiles.
prek's system hook couldn't resolve npx. Use the repo-local node_modules/.bin/commitlint directly via git rev-parse.
commitlint, tsc-check, and pyright-check were missing priority. Set all three to priority 10 (validation tier).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR applies widespread non-functional edits: normalizing trailing newlines, standardizing shell/redirection formatting, adding SPDX headers, consolidating a Docker install layer, switching CI/pre-commit tooling to use prek (npx prek run --all-files), and updating GitHub Action versions and workflow steps. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/walkthrough.sh (1)
67-77:⚠️ Potential issue | 🟠 MajorAvoid echoing the live API key in fallback instructions.
Line 75 currently expands and prints the real
NVIDIA_API_KEY. Use a literal placeholder to prevent accidental secret disclosure.🔐 Proposed fix
- echo " export NVIDIA_API_KEY=$NVIDIA_API_KEY" + echo " export NVIDIA_API_KEY=nvapi-..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/walkthrough.sh` around lines 67 - 77, The fallback instructions currently expand and print the real NVIDIA_API_KEY (see the echo line that contains export NVIDIA_API_KEY=$NVIDIA_API_KEY); change that echo so the dollar sign is not expanded (e.g., use single quotes or escape the $) and print a literal placeholder like $NVIDIA_API_KEY or <NVIDIA_API_KEY> instead, ensuring the tmux fallback block (the if ! command -v tmux ... echo lines) never exposes the live secret.
🧹 Nitpick comments (4)
test/e2e/test-double-onboard.sh (1)
32-50: Optional: centralize E2E helper functions to reduce duplication.
pass(),fail(),skip(), andsection()are now near-identical to helpers intest/e2e/test-full-e2e.sh; consider sourcing a shared helper script to keep output logic in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-double-onboard.sh` around lines 32 - 50, The test scripts duplicate the helper functions pass(), fail(), skip(), and section()—extract these into a single shared helper script (e.g., test/e2e/e2e-helpers.sh) and update test/e2e/test-double-onboard.sh to source that file instead of redefining pass, fail, skip, and section; ensure the shared script exports any variables it needs (PASS, FAIL, SKIP, TOTAL) or documents they must be defined by the caller and keep function names unchanged so existing calls in test-double-onboard.sh and test-full-e2e.sh continue to work.uninstall.sh (1)
67-68: Same pattern as install.sh — consider consistent refactor.This
wait/statuspattern matchesinstall.sh. The same optional refactor suggestion applies: declarelocal statusbefore the conditional for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@uninstall.sh` around lines 67 - 68, The wait/exit-status handling in uninstall.sh uses "wait \"$pid\"" followed by "local status=$?" which mirrors install.sh; declare "local status" before using it and initialize it (e.g., local status=0) so the variable is clearly scoped prior to the conditional that checks it — update the block around the "wait \"$pid\"" and "local status=$?" lines to declare "local status" earlier (and assign from $? after wait) for consistency with the refactor applied in install.sh.install.sh (1)
172-176: Verifylocalvariable scope inside conditional branches.The pattern here declares
statusaslocalinside bothifandelsebranches. While Bash allows this, it's slightly unconventional. Thelocaldeclaration in each branch meansstatusis function-scoped (not branch-scoped), so it works correctly, but for clarity consider declaring once before the conditional.♻️ Optional: Declare status before the conditional
- if wait "$pid"; then - local status=0 - else - local status=$? - fi + local status + if wait "$pid"; then + status=0 + else + status=$? + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 172 - 176, The code declares the local variable "status" inside both branches after calling wait "$pid"; instead declare "local status" once before the conditional and then assign to it inside the branches (e.g., status=0 or status=$?) so that the scope is clear; update the wait "$pid" block around the "status" variable to use the predeclared local variable and leave the rest unchanged.Makefile (1)
7-13: Consider documenting or removing orphanedlint-tsandlint-pytargets.With
lintnow depending solely oncheck, thelint-tsandlint-pytargets are no longer part of the main dependency chain. If they're intentionally kept for manual/targeted invocation, a brief comment would help future maintainers understand their purpose. Otherwise, they could be removed to reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 7 - 13, The Makefile contains orphaned targets lint-ts and lint-py that are no longer used by the lint target; either remove these targets to avoid confusion or add a short inline comment above each (lint-ts, lint-py) explaining they are retained for manual/targeted invocation (e.g., runs TypeScript and Python checks in subprojects) and update the lint dependency if you want them re-integrated into the main lint chain; modify the Makefile accordingly to either delete the lint-ts/lint-py blocks or add the explanatory comments and adjust the lint target to depend on them if intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-inference-local.sh`:
- Around line 6-7: The script uses a predictable /tmp/req.json which creates a
TOCTOU/race condition and never cleans up; replace the hardcoded path by
creating a secure temporary file via mktemp (e.g., TMPFILE=$(mktemp)), write the
payload into that TMPFILE, call curl -d @"$TMPFILE", and ensure you register a
trap to remove the TMPFILE on EXIT to avoid leakage; apply the same change to
scripts/test-inference.sh and test/e2e-test.sh and remove any remaining
references to /tmp/req.json.
In `@scripts/test-inference.sh`:
- Around line 6-7: The script writes to a predictable /tmp/req.json which
creates a TOCTOU/race condition; replace the hardcoded filename with a secure
temporary file via mktemp, write the JSON into that temp file (use a variable
like TMPFILE=$(mktemp)), set restrictive permissions (chmod 600 "$TMPFILE"),
update the curl invocation to use -d @"$TMPFILE", and ensure the temp is removed
on exit by installing a trap (trap 'rm -f "$TMPFILE"' EXIT) so the temp file is
cleaned up even on errors.
---
Outside diff comments:
In `@scripts/walkthrough.sh`:
- Around line 67-77: The fallback instructions currently expand and print the
real NVIDIA_API_KEY (see the echo line that contains export
NVIDIA_API_KEY=$NVIDIA_API_KEY); change that echo so the dollar sign is not
expanded (e.g., use single quotes or escape the $) and print a literal
placeholder like $NVIDIA_API_KEY or <NVIDIA_API_KEY> instead, ensuring the tmux
fallback block (the if ! command -v tmux ... echo lines) never exposes the live
secret.
---
Nitpick comments:
In `@install.sh`:
- Around line 172-176: The code declares the local variable "status" inside both
branches after calling wait "$pid"; instead declare "local status" once before
the conditional and then assign to it inside the branches (e.g., status=0 or
status=$?) so that the scope is clear; update the wait "$pid" block around the
"status" variable to use the predeclared local variable and leave the rest
unchanged.
In `@Makefile`:
- Around line 7-13: The Makefile contains orphaned targets lint-ts and lint-py
that are no longer used by the lint target; either remove these targets to avoid
confusion or add a short inline comment above each (lint-ts, lint-py) explaining
they are retained for manual/targeted invocation (e.g., runs TypeScript and
Python checks in subprojects) and update the lint dependency if you want them
re-integrated into the main lint chain; modify the Makefile accordingly to
either delete the lint-ts/lint-py blocks or add the explanatory comments and
adjust the lint target to depend on them if intended.
In `@test/e2e/test-double-onboard.sh`:
- Around line 32-50: The test scripts duplicate the helper functions pass(),
fail(), skip(), and section()—extract these into a single shared helper script
(e.g., test/e2e/e2e-helpers.sh) and update test/e2e/test-double-onboard.sh to
source that file instead of redefining pass, fail, skip, and section; ensure the
shared script exports any variables it needs (PASS, FAIL, SKIP, TOTAL) or
documents they must be defined by the caller and keep function names unchanged
so existing calls in test-double-onboard.sh and test-full-e2e.sh continue to
work.
In `@uninstall.sh`:
- Around line 67-68: The wait/exit-status handling in uninstall.sh uses "wait
\"$pid\"" followed by "local status=$?" which mirrors install.sh; declare "local
status" before using it and initialize it (e.g., local status=0) so the variable
is clearly scoped prior to the conditional that checks it — update the block
around the "wait \"$pid\"" and "local status=$?" lines to declare "local status"
earlier (and assign from $? after wait) for consistency with the refactor
applied in install.sh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51c2aae6-9f36-4e80-bae1-7298cd27549b
📒 Files selected for processing (45)
.agents/skills/docs/nemoclaw-overview/references/how-it-works.md.agents/skills/docs/nemoclaw-overview/references/overview.md.agents/skills/docs/nemoclaw-overview/references/release-notes.md.agents/skills/docs/nemoclaw-reference/SKILL.md.agents/skills/docs/nemoclaw-reference/references/architecture.md.agents/skills/docs/nemoclaw-reference/references/commands.md.agents/skills/docs/nemoclaw-reference/references/inference-profiles.md.agents/skills/docs/nemoclaw-reference/references/network-policies.md.agents/skills/docs/nemoclaw-reference/references/troubleshooting.md.github/PULL_REQUEST_TEMPLATE.md.github/workflows/pr.yaml.pre-commit-config.yamlCODE_OF_CONDUCT.mdCONTRIBUTING.mdDockerfileMakefiledocs/about/overview.mddocs/about/release-notes.mdinstall.shnemoclaw-blueprint/migrations/snapshot.pynemoclaw-blueprint/orchestrator/__init__.pypackage.jsonscripts/backup-workspace.shscripts/brev-setup.shscripts/check-spdx-headers.shscripts/debug.shscripts/docs-to-skills.pyscripts/fix-coredns.shscripts/install-openshell.shscripts/install.shscripts/lib/runtime.shscripts/nemoclaw-start.shscripts/setup-spark.shscripts/setup.shscripts/smoke-macos-install.shscripts/start-services.shscripts/test-inference-local.shscripts/test-inference.shscripts/walkthrough.shtest/Dockerfile.sandboxtest/e2e-test.shtest/e2e/Dockerfile.full-e2etest/e2e/test-double-onboard.shtest/e2e/test-full-e2e.shuninstall.sh
Auto-formatted by shfmt -i 2 -ci -bn to pass the prek shfmt hook. No logic changes — indentation and spacing only.
…kthrough - test-inference-local.sh, test-inference.sh: replace hardcoded /tmp/req.json with mktemp + trap cleanup (TOCTOU fix). - walkthrough.sh: print a placeholder instead of expanding the real NVIDIA_API_KEY in the tmux-fallback instructions. - Makefile: add comment clarifying lint-ts/lint-py are for targeted runs.
HagegeR
force-pushed
the
fix/pre-commit-hooks-pass
branch
from
9901a29 to
1c2e0a7
Compare
hadolint-docker times out pulling ghcr.io images in CI. Use a local system hook with the binary installed via curl in the workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr.yaml:
- Around line 39-43: The workflow step "Install hadolint" downloads a binary
without integrity checks; update that step to fetch and verify the SHA256
checksum before making the binary executable. Specifically: download the
corresponding SHA256 (or .sha256sum) for the same release URL used for
"hadolint-Linux-x86_64", compute the downloaded file's SHA256, compare it
against the expected checksum, and fail the job if they don't match; only run
chmod +x /usr/local/bin/hadolint after a successful verification. Ensure the
verification step clearly references the hadolint release URL used in the step
so the checksum and binary align.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43d405ce-e930-4ead-865b-243deda188df
📒 Files selected for processing (17)
.github/workflows/pr.yaml.pre-commit-config.yamlMakefilenemoclaw-blueprint/migrations/snapshot.pyscripts/check-spdx-headers.shscripts/fix-coredns.shscripts/install.shscripts/lib/runtime.shscripts/nemoclaw-start.shscripts/setup.shscripts/smoke-macos-install.shscripts/start-services.shscripts/test-inference-local.shscripts/test-inference.shscripts/walkthrough.shtest/e2e-test.shuninstall.sh
✅ Files skipped from review due to trivial changes (10)
- scripts/fix-coredns.sh
- scripts/check-spdx-headers.sh
- test/e2e-test.sh
- scripts/test-inference.sh
- scripts/nemoclaw-start.sh
- scripts/start-services.sh
- scripts/setup.sh
- scripts/smoke-macos-install.sh
- scripts/install.sh
- uninstall.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/walkthrough.sh
- Makefile
- scripts/lib/runtime.sh
- scripts/test-inference-local.sh
HagegeR
force-pushed
the
fix/pre-commit-hooks-pass
branch
from
1c2e0a7 to
36f36ba
Compare
Add --incremental to tsc --noEmit so subsequent type-checks reuse the .tsbuildinfo cache (supported since TS 4.0). Gitignore the cache file. See: https://thoughtspile.github.io/2021/06/14/faster-pre-commit/
- actions/checkout v4 → v6 - actions/setup-node v4 → v6 - actions/setup-python v5 → v6 - astral-sh/setup-uv v4 → v7 - actions/upload-artifact stays at v4 (latest) - rossjrw/pr-preview-action stays at v1 (latest)
HagegeR
force-pushed
the
fix/pre-commit-hooks-pass
branch
from
36f36ba to
c689f09
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 136-144: Update the CONTRIBUTING.md prerequisites to document that
hadolint must be installed for local development (since .pre-commit-config.yaml
uses the hadolint hook with language: system and hook id/name hadolint), and
include a short install command or link (matching CI setup in pr.yaml where
hadolint is installed before running pre-commit) so developers running npm/npx
prek won't hit "hadolint: command not found".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d307f57d-c5e4-415a-baf7-c5fb331266b9
📒 Files selected for processing (8)
.github/workflows/commit-lint.yaml.github/workflows/docker-pin-check.yaml.github/workflows/docs-preview-pr.yaml.github/workflows/docs.yaml.github/workflows/nightly-e2e.yaml.github/workflows/pr.yaml.gitignore.pre-commit-config.yaml
✅ Files skipped from review due to trivial changes (6)
- .github/workflows/nightly-e2e.yaml
- .github/workflows/docker-pin-check.yaml
- .github/workflows/docs-preview-pr.yaml
- .gitignore
- .github/workflows/commit-lint.yaml
- .github/workflows/docs.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pr.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace Husky + lint-staged with prek as the sole git hook runner, add a comprehensive .pre-commit-config.yaml, wire it into CI, and fix every pre-existing lint failure so the repo passes npx prek run --all-files cleanly.
Changes
Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docscatch up the docs for the new changes I made in this PR."Migration
Contributors with a previous Husky setup should run once:
Summary by CodeRabbit
Documentation
Developer Experience
Build & Infrastructure
Code Quality