-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: add pre-push git hook with delta lint mode #833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
c858700
be74f5e
7ee4a84
e919321
4f387d9
e0865ce
fccd038
3e6eb93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| # Pre-push hook: runs quality gate before pushing | ||
| # Skip with: git push --no-verify | ||
|
|
||
| REPO_ROOT="$(git rev-parse --show-toplevel)" | ||
| SCRIPT_DIR="$REPO_ROOT/scripts/ci" | ||
|
|
||
| # Default: baseline quality gate | ||
| "$SCRIPT_DIR/quality_gate.sh" | ||
|
|
||
| # Optional strict delta lint (env-gated) | ||
| if [ "${IRONCLAW_STRICT_DELTA_LINT:-0}" = "1" ]; then | ||
| "$SCRIPT_DIR/delta_lint.sh" | ||
| elif [ "${IRONCLAW_STRICT_LINT:-0}" = "1" ]; then | ||
|
Comment on lines
+12
to
+15
|
||
| echo "==> clippy (strict: all warnings)" | ||
| cargo clippy --locked --all-targets -- -D warnings | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By design — the pre-push hook is a fast local sanity check, not a CI replacement. Running full feature matrices and |
||
| fi | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,186 @@ | ||||||
| #!/usr/bin/env bash | ||||||
| set -euo pipefail | ||||||
| # Delta lint: only fail on clippy warnings/errors that touch changed lines. | ||||||
| # Compares the current branch against the merge base with the upstream default branch. | ||||||
|
|
||||||
| CLIPPY_OUT="" | ||||||
| DIFF_OUT="" | ||||||
|
|
||||||
| cleanup() { | ||||||
| [ -n "$CLIPPY_OUT" ] && rm -f "$CLIPPY_OUT" | ||||||
| [ -n "$DIFF_OUT" ] && rm -f "$DIFF_OUT" | ||||||
| } | ||||||
|
Comment on lines
+10
to
+14
|
||||||
| trap cleanup EXIT | ||||||
|
|
||||||
| # Verify python3 is available (needed for diagnostic filtering) | ||||||
| if ! command -v python3 &>/dev/null; then | ||||||
| echo "ERROR: python3 is required for delta lint but not found" | ||||||
| exit 1 | ||||||
| fi | ||||||
|
Comment on lines
+17
to
+21
|
||||||
|
|
||||||
| # Determine the upstream base ref dynamically | ||||||
| BASE_REF="" | ||||||
| # Try the remote HEAD symbolic ref (works for any default branch name) | ||||||
| if [ -z "$BASE_REF" ]; then | ||||||
| BASE_REF=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's|refs/remotes/||' || true) | ||||||
| fi | ||||||
| # Fall back to common default branch names | ||||||
| if [ -z "$BASE_REF" ] && git rev-parse --verify origin/main &>/dev/null; then | ||||||
| BASE_REF="origin/main" | ||||||
| fi | ||||||
| if [ -z "$BASE_REF" ] && git rev-parse --verify origin/master &>/dev/null; then | ||||||
| BASE_REF="origin/master" | ||||||
| fi | ||||||
| if [ -z "$BASE_REF" ]; then | ||||||
| echo "WARNING: could not determine upstream base branch, skipping delta lint" | ||||||
| exit 0 | ||||||
| fi | ||||||
|
|
||||||
| # Compute merge base | ||||||
| BASE=$(git merge-base "$BASE_REF" HEAD) | ||||||
|
||||||
|
|
||||||
| # Find changed .rs files | ||||||
| CHANGED_RS=$(git diff --name-only "$BASE" -- '*.rs' || true) | ||||||
| if [ -z "$CHANGED_RS" ]; then | ||||||
| echo "==> delta lint: no .rs files changed, skipping" | ||||||
| exit 0 | ||||||
| fi | ||||||
|
|
||||||
| echo "==> delta lint: checking changed lines since $(echo "$BASE" | head -c 10)..." | ||||||
|
|
||||||
| # Extract unified-0 diff for changed line ranges | ||||||
| DIFF_OUT=$(mktemp "${TMPDIR:-/tmp}/ironclaw-diff.XXXXXX") | ||||||
| git diff --unified=0 "$BASE" -- '*.rs' > "$DIFF_OUT" | ||||||
|
|
||||||
| # Run clippy with JSON output (stderr shows compilation progress/errors) | ||||||
| CLIPPY_OUT=$(mktemp "${TMPDIR:-/tmp}/ironclaw-clippy.XXXXXX") | ||||||
| CLIPPY_STDERR=$(mktemp "${TMPDIR:-/tmp}/ironclaw-clippy-err.XXXXXX") | ||||||
| cargo clippy --locked --all-targets --message-format=json -- -D warnings > "$CLIPPY_OUT" 2>"$CLIPPY_STDERR" || true | ||||||
|
||||||
| cargo clippy --locked --all-targets --message-format=json -- -D warnings > "$CLIPPY_OUT" 2>"$CLIPPY_STDERR" || true | |
| cargo clippy --locked --all-targets --message-format=json > "$CLIPPY_OUT" 2>"$CLIPPY_STDERR" || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Since -D warnings promotes warnings to error level, the current implementation (errors always blocking) is consistent with the quality gate intent. Distinguishing compiler errors from promoted warnings could be a future refinement via the code field.
Copilot
AI
Mar 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parse_diff() sets current_file from the +++ b/... line, but doesn’t handle the deletion case (+++ /dev/null). In that situation current_file can remain set to the previous file and subsequent hunks may be mis-attributed. Treat +++ /dev/null (and/or diff --git boundaries) as a reset of current_file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e0865ce — parse_diff() handles +++ /dev/null by resetting current_file = None.
Copilot
AI
Mar 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors that don’t include spans (or where spans is empty) are currently skipped entirely, which can let real build/compiler errors slip through delta lint. Treat diagnostics with level="error" and missing/empty spans as blocking (or fall back to failing on any error-level message regardless of span availability).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e0865ce — error-level diagnostics are now always blocking regardless of spans or changed-line overlap.
Copilot
AI
Mar 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed-line matching only considers the diagnostic’s line_start. Multi-line spans that overlap a changed hunk but start outside it will be missed. Use line_end too and treat any overlap between [line_start,line_end] and a changed range as blocking (and consider checking all primary spans).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e0865ce — in_changed_range() now checks [line_start, line_end] interval overlap.
Copilot
AI
Mar 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the blocking/baseline decision is based only on whether the span touches changed lines, error-level diagnostics can end up in baseline and the script exits 0. Compilation errors should always fail regardless of diff range. Special-case level=="error" to always be blocking (and optionally keep filtering only warnings by changed lines).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e0865ce — same commit. Errors always block; changed-lines filter only applies to warnings.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| echo "==> fmt check" | ||
| cargo fmt --all -- --check | ||
|
|
||
| echo "==> clippy (correctness)" | ||
| cargo clippy --locked --all-targets -- -D clippy::correctness | ||
|
Comment on lines
+7
to
+8
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By design — the pre-push hook is a fast local sanity check, not a CI replacement. Running full feature matrices and |
||
|
|
||
| echo "==> tests" | ||
| cargo test --locked | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,9 @@ if [ -n "$HOOKS_DIR" ]; then | |
| echo " commit-msg hook installed (regression test enforcement)" | ||
| ln -sf "$SCRIPTS_ABS/pre-commit-safety.sh" "$HOOKS_DIR/pre-commit" | ||
| echo " pre-commit hook installed (UTF-8, case-sensitivity, /tmp, redaction checks)" | ||
| REPO_ROOT="$(git rev-parse --show-toplevel)" | ||
| ln -sf "$REPO_ROOT/.githooks/pre-push" "$HOOKS_DIR/pre-push" | ||
| echo " pre-push hook installed (quality gate + optional delta lint)" | ||
|
Comment on lines
+59
to
+61
|
||
| else | ||
| echo " Skipped: not a git repository" | ||
| fi | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR title/description focus on adding a pre-push hook + delta linting, but this PR also includes substantial runtime and architecture changes (e.g., orchestrator setup refactor, tunnel startup refactor, lightweight routine tool execution, token budgeting, secrets/db handle factories, etc.). Please either update the PR description/title to reflect the full scope or split the non-hook changes into separate PRs to keep review/rollback risk manageable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR only adds/modifies
.githooks/pre-push,scripts/ci/delta_lint.sh,scripts/ci/quality_gate.sh,scripts/dev-setup.sh. Other files in the diff are from the staging base.