feat: add pre-push git hook with delta lint mode by ilblackdragon · Pull Request #833 · nearai/ironclaw

ilblackdragon · 2026-03-10T06:08:08Z

Summary

Add pre-push git hook that runs quality gate (fmt + clippy correctness + tests) before push
Add scripts/ci/quality_gate.sh for baseline checks
Add scripts/ci/delta_lint.sh with embedded Python for filtering clippy warnings to only changed lines
Support env-gated strict modes: IRONCLAW_STRICT_LINT=1 and IRONCLAW_STRICT_DELTA_LINT=1
Update scripts/dev-setup.sh to install the pre-push hook

Test plan

Run .githooks/pre-push manually to verify quality gate passes
Test IRONCLAW_STRICT_DELTA_LINT=1 .githooks/pre-push with intentional clippy warning on changed lines
Verify scripts/dev-setup.sh installs the pre-push hook correctly
Test on both macOS and Linux

🤖 Generated with Claude Code

Add pre-push hook and CI quality gate scripts: - .githooks/pre-push: runs quality gate before push - scripts/ci/quality_gate.sh: baseline fmt + clippy correctness + tests - scripts/ci/delta_lint.sh: clippy warnings filtered to changed lines only - Updated dev-setup.sh to install pre-push hook Supports environment-gated modes: - IRONCLAW_STRICT_LINT=1: deny all clippy warnings - IRONCLAW_STRICT_DELTA_LINT=1: deny warnings only on changed lines Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gemini-code-assist · 2026-03-10T06:08:12Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

- Fix SCRIPT_DIR resolution in pre-push hook to work correctly with symlinks by using git rev-parse --show-toplevel - Add python3 availability check in delta_lint.sh Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Stop suppressing clippy stderr; capture it and show compilation errors if clippy produces no JSON output - Add --locked flag to clippy for lockfile consistency - Use repo root (via git rev-parse) for path normalization instead of os.getcwd() which may differ from repo root Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR introduces a local pre-push quality gate (fmt/clippy/tests) with optional “delta lint” and also includes a broad refactor toward “module-owned initialization” plus several runtime behavior changes (onboarding quick mode, token budgeting, lightweight routine tooling, CI/logging adjustments).

Changes:

Add a pre-push git hook and CI scripts for a local quality gate + delta clippy filtering on changed lines.
Refactor initialization logic into module-level factories (db handles, secrets store, MCP client factory, tunnel/orchestrator/wasm setup helpers).
Add/adjust runtime features and data plumbing (quick onboarding, job token budgets, lightweight routine tool loop, job_id/user_id persistence, logging verbosity changes).

Reviewed changes

Copilot reviewed 61 out of 61 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/support/test_rig.rs	Update RoutineEngine construction + routine config fields in tests
tests/module_init_integration.rs	New integration tests for module-owned factories (db/secrets/extensions)
tests/e2e_routine_heartbeat.rs	Update RoutineEngine creation with ToolRegistry + SafetyLayer
src/worker/mod.rs	Add worker/bridge runner helpers for CLI subcommands
src/tunnel/mod.rs	Add `start_managed_tunnel` helper returning updated Config + tunnel handle
src/tracing_fmt.rs	Add CLI/worker tracing init helpers (EnvFilter-based)
src/tools/wasm/loader.rs	Treat missing tools dir as empty; add test
src/tools/registry.rs	Downgrade tool registration logs from info → debug
src/tools/mcp/mod.rs	Export MCP client factory module + re-exports
src/tools/mcp/factory.rs	New MCP client factory handling transport dispatch/auth
src/tools/builtin/skill_tools.rs	Make skill removal description stronger; require Always approval + tests
src/tools/builtin/extension_tools.rs	Make extension removal description stronger; require Always approval + tests
src/setup/wizard.rs	Add onboarding quick mode + NEAR AI model-fetch config helper + env tests
src/setup/mod.rs	Add `check_onboard_needed()` helper
src/setup/README.md	Document quick mode + updated wizard steps
src/settings.rs	Add `agent.max_tokens_per_job` setting
src/secrets/mod.rs	Add `create_secrets_store(crypto, handles)` factory
src/sandbox/proxy/http.rs	Reduce shutdown log to debug
src/sandbox/manager.rs	Reduce shutdown log to debug
src/orchestrator/mod.rs	Add `setup_orchestrator()` factory returning orchestrator handles
src/main.rs	Use new module-owned factories; add auto quick onboarding; reduce logs
src/llm/nearai_chat.rs	Reduce pricing-load log to debug
src/llm/mod.rs	Reduce provider initialization logs to debug
src/history/store.rs	Persist `user_id` in job saves; add PG regression test
src/extensions/manager.rs	Add McpProcessManager dependency; use MCP factory for activation
src/db/mod.rs	Introduce `DatabaseHandles` + `connect_with_handles`; log on connect
src/db/libsql/mod.rs	Add libsql regression test for job user_id persistence
src/db/libsql/jobs.rs	Persist `user_id` in libsql job upsert
src/config/routines.rs	Add lightweight routine tool config + env parsing/cap
src/config/embeddings.rs	Reduce embeddings logs to debug
src/config/agent.rs	Add env/config wiring for `max_tokens_per_job`
src/cli/tool.rs	Reuse shared CLI secrets-store initializer
src/cli/mod.rs	Add shared `init_secrets_store()` + move memory command runner here; add `--quick`
src/cli/mcp.rs	Reuse shared CLI secrets-store initializer
src/channels/webhook_server.rs	Reduce shutdown log to debug
src/channels/web/types.rs	Add `job_id` to routine run info
src/channels/web/server.rs	Include routine run `job_id`; update tests for ExtensionManager signature
src/channels/web/handlers/routines.rs	Include routine run `job_id` in handler responses
src/channels/web/handlers/jobs.rs	Improve cancellation fallback: stop via scheduler before DB update
src/channels/wasm/setup.rs	New WASM channel setup module (loading, router registration, credential injection)
src/channels/wasm/mod.rs	Export WASM setup helpers
src/channels/wasm/loader.rs	Treat missing channels dir as empty; add test
src/channels/manager.rs	Reduce channel lifecycle logs to debug
src/bootstrap.rs	Add `upsert_bootstrap_vars*` to preserve existing `.env`; add tests
src/app.rs	Use `connect_with_handles` + `create_secrets_store(handles)`; MCP factory; reduce logs
src/agent/worker.rs	Track token usage vs budget; change iteration/rate-limit failure to Failed; add tests
src/agent/scheduler.rs	Apply per-job token budget from metadata/config
src/agent/routine_engine.rs	Add lightweight routine tool loop with safety/approval gating + config tests
src/agent/dispatcher.rs	Add tests around Always approval semantics; update config struct fields
src/agent/agent_loop.rs	Wire ToolRegistry + SafetyLayer into RoutineEngine; reduce logs
scripts/dev-setup.sh	Install pre-push hook symlink during dev setup
scripts/ci/quality_gate.sh	New quality gate script (fmt + clippy correctness + tests)
scripts/ci/delta_lint.sh	New delta clippy lint script filtering to changed lines
FEATURE_PARITY.md	Update parity notes/review date and feature rows
CLAUDE.md	Document module-owned initialization and new module layout notes
.github/workflows/test.yml	Restrict PR test workflow trigger to PRs targeting main
.github/workflows/staging-ci.yml	Remove GitHub App token step guards (risk)
.github/workflows/code_style.yml	Run Windows clippy only for PRs into main; adjust roll-up job
.github/workflows/claude-review.yml	Only run on labeled; restrict allowed bot actor
.githooks/pre-push	New pre-push hook to run quality gate + optional strict/delta lint
.env.example	Document `AGENT_MAX_TOKENS_PER_JOB` env var

Comments suppressed due to low confidence (2)

.github/workflows/staging-ci.yml:122

The actions/create-github-app-token@v2 step is now unconditional. If GH_RELEASES_MANAGER_APP_ID/GH_RELEASES_MANAGER_APP_PRIVATE_KEY are unset (common in forks or if secrets are temporarily missing), this action will fail and block the workflow before the fallback-to-github.token logic runs. Reintroduce the if: guard (or add an explicit condition) so the job can proceed with github.token when the app secrets are not available.

      - name: Generate GitHub App token
        id: app-token
        uses: actions/create-github-app-token@v2
        with:
          app-id: ${{ secrets.GH_RELEASES_MANAGER_APP_ID }}
          private-key: ${{ secrets.GH_RELEASES_MANAGER_APP_PRIVATE_KEY }}

.github/workflows/staging-ci.yml:236

Same issue as earlier in this workflow: the GitHub App token generation step runs unconditionally and will fail when the app secrets are empty/unset, preventing the later fallback to github.token. Add back an if: guard or conditional logic around this step.

      - name: Generate GitHub App token
        id: app-token
        uses: actions/create-github-app-token@v2
        with:
          app-id: ${{ secrets.GH_RELEASES_MANAGER_APP_ID }}
          private-key: ${{ secrets.GH_RELEASES_MANAGER_APP_PRIVATE_KEY }}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-10T06:17:19Z

scripts/ci/delta_lint.sh

+# 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


delta_lint.sh hard-codes the merge base as origin/main (git merge-base origin/main HEAD). This will fail in repos without an origin remote, without a main branch locally fetched, or when the default branch differs. Consider deriving the upstream ref dynamically (e.g., from @{upstream} / the push remote), and/or adding a fallback that fetches the base ref or skips delta lint gracefully when the base can’t be determined.

Fixed in e0865ce — delta_lint.sh now dynamically detects the upstream base branch.

Copilot · 2026-03-10T06:17:20Z

src/agent/dispatcher.rs

+        // However, ApprovalRequirement::Always should always require approval
+        // This is verified by the dispatcher logic: Always => true (ignores session state)
+        let always_req = ApprovalRequirement::Always;
+        let requires_approval = match always_req {
+            ApprovalRequirement::Never => false,
+            ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(tool_name),
+            ApprovalRequirement::Always => true,
+        };


These new tests re-implement the dispatcher’s approval decision logic locally (a match on ApprovalRequirement) instead of exercising the actual dispatcher code path (e.g., preflight/approval handling). As a result, they won’t fail if the real dispatcher logic changes/regresses. Consider refactoring the approval decision into a small helper that both dispatcher and tests call, or build a minimal dispatcher/session scenario that triggers the real approval gate.

This file is not changed by this PR — it is part of the staging base. Will track separately.

Copilot · 2026-03-10T06:17:20Z

src/setup/wizard.rs

 /// Mask an API key for display: show first 6 + last 4 chars.
 ///
 /// Uses char-based indexing to avoid panicking on multi-byte UTF-8.
+/// Build the `LlmConfig` used by `fetch_nearai_models` to list available models.
+///
+/// Reads `NEARAI_API_KEY` from the environment so that users who authenticated
+/// via Cloud API key (option 4) don't get re-prompted during model selection.
+fn build_nearai_model_fetch_config() -> crate::config::LlmConfig {


The doc comment for mask_api_key now accidentally includes the documentation for build_nearai_model_fetch_config() (lines 3390-3393). As written, rustdoc will attach the model-fetch docs to mask_api_key, and build_nearai_model_fetch_config has no doc comment of its own. Split these into two separate doc comments so each function is documented correctly.

This file is not changed by this PR — it is part of the staging base. Will track separately.

Copilot · 2026-03-10T06:17:20Z

src/agent/routine_engine.rs

+/// This is a simplified version of the full dispatcher loop:
+/// - Max 3-5 iterations (configurable)
+/// - Sequential tool execution (not parallel)
+/// - Auto-approval of non-Always tools
+/// - No hooks or approval dialogs


The doc comment for execute_lightweight_with_tools says it does “Auto-approval of non-Always tools”, but execute_routine_tool() actually blocks both UnlessAutoApproved and Always and only permits ApprovalRequirement::Never. Update the doc comment to match the implemented policy (or adjust the policy if auto-approving UnlessAutoApproved tools is intended).

This file is not changed by this PR — it is part of the staging base. Will track separately.

Copilot · 2026-03-10T06:17:20Z

src/agent/routine_engine.rs

+            // Tool-enabled iteration
+            let tool_defs = ctx.tools.tool_definitions().await;
+
+            let request = ToolCompletionRequest::new(messages.clone(), tool_defs)
+                .with_max_tokens(effective_max_tokens)
+                .with_temperature(0.3);


execute_lightweight_with_tools passes all tool definitions (ctx.tools.tool_definitions()) to the LLM, but execute_routine_tool later rejects any tool that isn’t ApprovalRequirement::Never. This mismatch makes it likely the LLM will attempt disallowed tools (wasting iterations/tokens) and exposes schemas/descriptions for tools that can never run in this mode. Consider filtering the tool definitions up-front to only the tools allowed in lightweight routines (e.g., build a list of names where requires_approval(...) == Never and use tool_definitions_for).

This file is not changed by this PR — it is part of the staging base. Will track separately.

Copilot · 2026-03-10T06:17:20Z

.githooks/pre-push

+#!/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


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.

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.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-10T07:39:35Z

.githooks/pre-push

+# 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


This pre-push hook ignores the standard hook arguments (remote name/URL) and always runs delta_lint with its internal default-remote assumptions. That can make delta lint compare against origin even when pushing to a different remote. Pass "$1" (remote name) through to delta_lint.sh and have delta_lint use that remote when resolving the default branch/merge-base.

Fixed in e0865ce — pre-push hook now passes $1 (remote name) to delta_lint.sh.

Copilot · 2026-03-10T07:39:36Z

scripts/ci/delta_lint.sh

+            if not primary:
+                if spans:
+                    primary = spans[0]
+                else:
+                    continue


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).

Fixed in e0865ce — error-level diagnostics are now always blocking regardless of spans or changed-line overlap.

Copilot · 2026-03-10T07:39:36Z

scripts/ci/delta_lint.sh

+
+            if in_changed_range(file_name, line_start, changed_ranges, repo_root):
+                blocking.append(rendered)
+            else:
+                baseline.append(rendered)


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).

Fixed in e0865ce — same commit. Errors always block; changed-lines filter only applies to warnings.

Copilot · 2026-03-10T07:39:36Z

scripts/ci/delta_lint.sh

+            file_name = primary.get("file_name", "")
+            line_start = primary.get("line_start", 0)
+            rendered = cm.get("rendered", "").strip()
+
+            if in_changed_range(file_name, line_start, changed_ranges, repo_root):


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).

Fixed in e0865ce — in_changed_range() now checks [line_start, line_end] interval overlap.

Copilot · 2026-03-10T07:39:36Z

scripts/ci/delta_lint.sh

+            # Match +++ b/path/to/file.rs
+            m = re.match(r'^\+\+\+ b/(.+)$', line)
+            if m:
+                current_file = m.group(1)
+                if current_file not in changed:


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.

Fixed in e0865ce — parse_diff() handles +++ /dev/null by resetting current_file = None.

Copilot · 2026-03-10T07:39:37Z

scripts/ci/delta_lint.sh

+# Compute merge base
+BASE=$(git merge-base "$BASE_REF" HEAD)


git merge-base "$BASE_REF" HEAD can fail (e.g., if the remote default branch hasn’t been fetched or there’s no common ancestor). With set -e, that will hard-fail delta lint and block the push. Consider catching merge-base failures and emitting a warning + exit 0, consistent with the earlier “could not determine upstream base branch, skipping delta lint” behavior.

Fixed in e0865ce — git merge-base failure caught with warning + exit 0.

Copilot · 2026-03-10T07:39:37Z

scripts/ci/delta_lint.sh

+cleanup() {
+    [ -n "$CLIPPY_OUT" ] && rm -f "$CLIPPY_OUT"
+    [ -n "$DIFF_OUT" ] && rm -f "$DIFF_OUT"
+}


The EXIT cleanup trap only removes DIFF_OUT/CLIPPY_OUT. Since CLIPPY_STDERR is also created as a temp file later, failures before the explicit rm can leave it behind. Track CLIPPY_STDERR and remove it in cleanup as well.

Fixed in e0865ce — CLIPPY_STDERR added to EXIT trap cleanup.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

zmanian

Review: pre-push hook with delta lint mode

Overall

Clean, well-structured PR. The delta lint approach is sound -- filtering clippy diagnostics to only changed lines is a good compromise between "lint nothing" and "lint everything" pre-push. A few issues to address.

Issues

1. quality_gate.sh runs the full test suite on every push (blocking)

cargo test --locked in the baseline quality gate (which runs unconditionally) can take minutes on a large codebase. This will slow down every push significantly and create friction. Most teams gate full test runs on CI, not pre-push. Consider either:

Moving cargo test behind an env flag (like the strict lint modes), or
Running only cargo test --lib (unit tests) by default, leaving integration tests to CI.

A pre-push hook that takes 5+ minutes will train developers to habitually use --no-verify, defeating the purpose.

2. Inconsistent symlink pattern in dev-setup.sh

The existing hooks use ln -sf "$SCRIPTS_ABS/..." to symlink scripts from scripts/. The new pre-push hook symlinks from .githooks/pre-push using a separately computed $REPO_ROOT. This works but is inconsistent. Consider either:

Moving the pre-push script to scripts/ci/pre-push.sh and using the same $SCRIPTS_ABS pattern, or
At minimum, using the already-available $SCRIPTS_ABS/../.githooks/pre-push instead of re-computing $REPO_ROOT.

Minor, but consistency in setup scripts prevents future confusion.

3. delta_lint.sh cleanup trap does not cover CLIPPY_STDERR

Line ~65 creates CLIPPY_STDERR=$(mktemp ...) but the cleanup trap only removes $CLIPPY_OUT and $DIFF_OUT. If the script is interrupted between creating that tempfile and the explicit rm -f "$CLIPPY_STDERR" on lines 74/76, it leaks. Add it to the trap.

4. --locked flag assumes Cargo.lock is committed and current

All three cargo commands in quality_gate.sh use --locked. If a developer has made dependency changes and not yet committed the updated Cargo.lock, the push will fail with a confusing lockfile error rather than a lint error. This is arguably correct behavior but worth documenting in the hook's skip instructions or a comment.

Minor / Nits

delta_lint.sh line 68: cargo clippy ... 2>"$CLIPPY_STDERR" || true -- the || true means compilation failures (not just warnings) are silently swallowed. The subsequent check on lines 71-76 catches the case where no JSON is produced, but a partial JSON output with compilation errors mixed in could pass silently. Consider checking the exit code explicitly.
The embedded Python is well-written. The [:10] truncation for baseline warnings with a count summary is a nice touch.

Bypass / Non-intrusiveness

Good: --no-verify bypass is documented in the hook header comment. The strict modes are env-gated and off by default. The baseline quality gate (fmt + correctness-only clippy + tests) is reasonable as a default, assuming the test suite concern above is addressed.

Summary

The delta lint concept is solid. Main concern is the unconditional full test suite in the baseline gate -- that needs to be either scoped down or made opt-in to avoid push friction.

- Pass remote name ($1) from pre-push hook to delta_lint.sh - Accept optional remote name arg, fall back to dynamic detection - Treat error-level diagnostics as always blocking - Check span overlap [line_start, line_end] vs changed ranges - Handle +++ /dev/null (file deletions) in parse_diff - Catch git merge-base failure with graceful skip - Add CLIPPY_STDERR to EXIT trap cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-10T18:21:00Z

scripts/dev-setup.sh

+    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)"


If core.hooksPath is configured to .githooks (as suggested in .githooks/pre-commit), git rev-parse --git-path hooks will resolve to .githooks. In that case this ln -sf "$REPO_ROOT/.githooks/pre-push" "$HOOKS_DIR/pre-push" attempts to link the hook to itself and will fail under set -e. Consider detecting when HOOKS_DIR already points at .githooks and skipping the symlink (or linking to a different source path) to avoid a self-link error.

Good catch — will verify core.hooksPath interaction. If set to .githooks, the symlink approach is redundant but harmless.

Copilot · 2026-03-10T18:21:01Z

scripts/ci/delta_lint.sh

+# 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 ... -- -D warnings promotes all warnings to level="error" in the JSON output. Since the Python filter treats all error diagnostics as always-blocking, this effectively defeats delta linting and will fail even for warnings outside the changed lines. To make delta lint work, run clippy without -D warnings (leave warnings as warnings) and only fail based on the changed-line overlap logic, while still treating true compiler errors as blocking.

Suggested change

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

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.

zmanian

Re-review after `e0865ce`

The latest commit addresses most of the Copilot feedback and several of my earlier points. Good progress. Two issues remain from my previous review.

1. `-D warnings` defeats delta lint (blocking)

delta_lint.sh line 79 still runs cargo clippy ... -- -D warnings. This promotes every warning to error level in clippy's JSON output. Combined with the (correct) change in e0865ce to treat all error-level diagnostics as always-blocking, the delta filtering is now completely bypassed -- every warning in the entire codebase will block the push, not just warnings on changed lines.

Fix: drop -D warnings from the clippy invocation inside delta_lint.sh. The Python filter already handles the blocking decision based on changed-line overlap for warnings. Keep -D warnings only in the IRONCLAW_STRICT_LINT path (pre-push line 16) where whole-codebase enforcement is intentional.

ilblackdragon's reply said this is "consistent with the quality gate intent," but I think there's a misunderstanding: if -D warnings stays, there is no point in having delta lint at all -- it becomes identical to IRONCLAW_STRICT_LINT=1. The entire value proposition of the delta approach requires warnings to remain at warning level so the filter can selectively promote only the ones touching changed lines.

2. `quality_gate.sh` runs full test suite on every push (same as before)

cargo test --locked runs unconditionally in the baseline gate. On this codebase that takes multiple minutes. This will cause developers to default to --no-verify. Consider gating cargo test behind an env var (e.g., IRONCLAW_PREPUSH_TEST=1) or defaulting to cargo test --lib --locked (unit tests only).

Resolved from previous review

CLIPPY_STDERR cleanup trap: fixed
Inconsistent symlink pattern: acknowledged, minor
--locked documentation: fine as-is

dev-setup.sh self-link edge case

ilblackdragon noted the core.hooksPath self-link issue is "harmless." It's actually not -- ln -sf on the same path with set -e will succeed on some platforms but the hook will be a self-referencing symlink and silently do nothing. Worth a guard: if [ "$HOOKS_DIR" != "$REPO_ROOT/.githooks" ]; then .... Low priority though.

Summary

The delta lint concept is sound and the latest fixes are good. The -D warnings issue (#1) is a functional bug that needs fixing before merge -- without it, delta lint has no effect. The test suite concern (#2) is about developer experience and can be addressed post-merge if needed.

1. Remove `-D warnings` from the clippy invocation in delta_lint.sh. With -D warnings, all warnings are promoted to error level in JSON output, which bypasses the delta filter entirely (errors are always blocking). The Python filter already handles the blocking decision for warnings based on changed-line overlap. 2. Scope pre-push tests to `cargo test --lib` (unit tests only) instead of the full test suite. Full integration tests can take minutes and will train developers to use --no-verify. The full suite runs in CI. Skip tests entirely with IRONCLAW_PREPUSH_TEST=0. Addresses zmanian's review feedback on PR #833. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ilblackdragon · 2026-03-11T07:32:38Z

@zmanian — Thanks for the thorough reviews. Both remaining issues from your re-review are addressed in fccd038:

1. -D warnings defeats delta lint (blocking): Fixed — removed -D warnings from the clippy invocation in delta_lint.sh. The Python filter now handles the blocking decision for warnings based on changed-line overlap, and errors (actual compilation errors) remain always-blocking. This restores the intended delta lint behavior.

2. quality_gate.sh runs full test suite: Fixed — scoped to cargo test --locked --lib (unit tests only). Full integration/e2e tests run in CI. Tests can be skipped entirely with IRONCLAW_PREPUSH_TEST=0.

Also incorporated from your first review:

CLIPPY_STDERR cleanup trap: fixed in e0865ce
--locked flag behavior: documented via the skip instructions
|| true swallowing compilation failures: addressed via the stderr check fallback

zmanian

Looks good. Clean implementation, well-structured delta lint approach.

A few notes (non-blocking):

Delta lint correctly scopes to changed lines -- the Python filter parses unified-0 diff hunks and only blocks on warnings overlapping changed ranges. Errors are always blocking regardless of location, which is the right call.
Existing hook pattern followed -- dev-setup.sh symlinks match the existing commit-msg and pre-commit pattern. The new hook lives in .githooks/pre-push (source of truth) with the symlink installed by dev-setup.sh.
Graceful fallbacks -- delta_lint.sh exits 0 with a warning when it can't determine the base branch or merge-base fails, which avoids blocking devs in edge cases (detached HEAD, shallow clones).
Env-gated opt-in -- Both IRONCLAW_STRICT_DELTA_LINT and IRONCLAW_STRICT_LINT are opt-in (off by default), so this doesn't change the default push experience beyond the baseline quality gate (fmt + correctness clippy + tests).
No Rust production code touched -- this is pure shell/Python tooling, so the .unwrap()/.expect() rule doesn't apply.

Minor observations (take or leave):

quality_gate.sh runs cargo clippy --locked --all-targets -- -D clippy::correctness while the existing quality_gate_strict.sh uses --all-features. The baseline gate intentionally uses default features only, which seems fine for a pre-push guard, but worth noting the difference.
The python3 dependency is checked upfront in delta_lint.sh with a clear error, which is good. Might be worth mentioning the python3 requirement in the PR description or dev-setup docs for completeness.

…a-lint # Conflicts: # .githooks/pre-push

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ilblackdragon · 2026-03-15T04:31:52Z

.githooks/pre-push

+    "$SCRIPT_DIR/delta_lint.sh" "$1"
+elif [ "${IRONCLAW_STRICT_LINT:-0}" = "1" ]; then
+    echo "==> clippy (strict: all warnings)"
+    cargo clippy --locked --all-targets -- -D warnings


By design — the pre-push hook is a fast local sanity check, not a CI replacement. Running full feature matrices and --all --benches --tests --examples locally would take too long and cause developers to skip with --no-verify. CI handles the comprehensive checks.

ilblackdragon · 2026-03-15T04:31:54Z

scripts/ci/quality_gate.sh

+echo "==> clippy (correctness)"
+cargo clippy --locked --all-targets -- -D clippy::correctness


By design — the pre-push hook is a fast local sanity check, not a CI replacement. Running full feature matrices and --all --benches --tests --examples locally would take too long and cause developers to skip with --no-verify. CI handles the comprehensive checks.

ilblackdragon · 2026-03-15T04:31:56Z

scripts/ci/quality_gate.sh

+
+if [ "${IRONCLAW_PREPUSH_TEST:-1}" = "1" ]; then
+    echo "==> tests (skip with IRONCLAW_PREPUSH_TEST=0)"
+    cargo test --locked --lib


By design — the pre-push hook is a fast local sanity check, not a CI replacement. Running full feature matrices and --all --benches --tests --examples locally would take too long and cause developers to skip with --no-verify. CI handles the comprehensive checks.

ilblackdragon · 2026-03-15T04:31:57Z

scripts/ci/delta_lint.sh

+# 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 > "$CLIPPY_OUT" 2>"$CLIPPY_STDERR" || true


By design — the pre-push hook is a fast local sanity check, not a CI replacement. Running full feature matrices and --all --benches --tests --examples locally would take too long and cause developers to skip with --no-verify. CI handles the comprehensive checks.

* feat: add pre-push git hook with delta lint mode Add pre-push hook and CI quality gate scripts: - .githooks/pre-push: runs quality gate before push - scripts/ci/quality_gate.sh: baseline fmt + clippy correctness + tests - scripts/ci/delta_lint.sh: clippy warnings filtered to changed lines only - Updated dev-setup.sh to install pre-push hook Supports environment-gated modes: - IRONCLAW_STRICT_LINT=1: deny all clippy warnings - IRONCLAW_STRICT_DELTA_LINT=1: deny warnings only on changed lines Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use git rev-parse for SCRIPT_DIR, add python3 check - Fix SCRIPT_DIR resolution in pre-push hook to work correctly with symlinks by using git rev-parse --show-toplevel - Add python3 availability check in delta_lint.sh Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: delta lint stderr handling, --locked flag, path normalization - Stop suppressing clippy stderr; capture it and show compilation errors if clippy produces no JSON output - Add --locked flag to clippy for lockfile consistency - Use repo root (via git rev-parse) for path normalization instead of os.getcwd() which may differ from repo root Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: dynamically detect upstream base branch in delta_lint.sh Instead of hard-coding `origin/main`, derive the base ref by checking `refs/remotes/origin/HEAD`, then falling back to `origin/main` and `origin/master`. If none can be resolved, skip delta lint gracefully with a warning and exit 0. Addresses PR nearai#833 review feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: re-trigger CI after adding skip-regression-check label Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR nearai#833 review feedback for delta lint - Pass remote name ($1) from pre-push hook to delta_lint.sh - Accept optional remote name arg, fall back to dynamic detection - Treat error-level diagnostics as always blocking - Check span overlap [line_start, line_end] vs changed ranges - Handle +++ /dev/null (file deletions) in parse_diff - Catch git merge-base failure with graceful skip - Add CLIPPY_STDERR to EXIT trap cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: drop -D warnings from delta lint, scope pre-push tests to --lib 1. Remove `-D warnings` from the clippy invocation in delta_lint.sh. With -D warnings, all warnings are promoted to error level in JSON output, which bypasses the delta filter entirely (errors are always blocking). The Python filter already handles the blocking decision for warnings based on changed-line overlap. 2. Scope pre-push tests to `cargo test --lib` (unit tests only) instead of the full test suite. Full integration tests can take minutes and will train developers to use --no-verify. The full suite runs in CI. Skip tests entirely with IRONCLAW_PREPUSH_TEST=0. Addresses zmanian's review feedback on PR nearai#833. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 10, 2026 06:08

Copilot started reviewing on behalf of ilblackdragon March 10, 2026 06:08 View session

ilblackdragon and others added 2 commits March 9, 2026 23:09

fix: use git rev-parse for SCRIPT_DIR, add python3 check

be74f5e

- Fix SCRIPT_DIR resolution in pre-push hook to work correctly with symlinks by using git rev-parse --show-toplevel - Add python3 availability check in delta_lint.sh Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI reviewed Mar 10, 2026

View reviewed changes

github-actions bot added size: L 200-499 changed lines risk: low Changes to docs, tests, or low-risk modules and removed size: XL 500+ changed lines risk: high Safety, secrets, auth, or critical infrastructure labels Mar 10, 2026

Copilot started reviewing on behalf of ilblackdragon March 10, 2026 07:35 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

ilblackdragon added the skip-regression-check Bypass regression test CI gate (tests exist but not in tests/ dir) label Mar 10, 2026

chore: re-trigger CI after adding skip-regression-check label

4f387d9

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

zmanian reviewed Mar 10, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings March 10, 2026 18:16

Copilot started reviewing on behalf of ilblackdragon March 10, 2026 18:16 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

zmanian reviewed Mar 10, 2026

View reviewed changes

ilblackdragon requested a review from zmanian March 11, 2026 07:35

zmanian previously approved these changes Mar 12, 2026

View reviewed changes

Merge remote-tracking branch 'origin/staging' into feat/pre-push-delt…

3e6eb93

…a-lint # Conflicts: # .githooks/pre-push

Copilot AI review requested due to automatic review settings March 15, 2026 03:27

ilblackdragon dismissed zmanian’s stale review via 3e6eb93 March 15, 2026 03:27

Copilot started reviewing on behalf of ilblackdragon March 15, 2026 03:28 View session

Copilot AI reviewed Mar 15, 2026

View reviewed changes

ilblackdragon merged commit 27e21fd into staging Mar 15, 2026
18 checks passed

ilblackdragon deleted the feat/pre-push-delta-lint branch March 15, 2026 05:41

	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

		echo "==> clippy (correctness)"
		cargo clippy --locked --all-targets -- -D clippy::correctness

Conversation

ilblackdragon commented Mar 10, 2026

Summary

Test plan

Uh oh!

gemini-code-assist bot commented Mar 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Re-review after `e0865ce`

1. `-D warnings` defeats delta lint (blocking)

2. `quality_gate.sh` runs full test suite on every push (same as before)