Skip to content

fix(#2852): re-stage and retry when pre-commit hooks auto-fix files - #2855

Merged
waynesun09 merged 2 commits into
mainfrom
agent/2852-precommit-autofix-retry
Jul 1, 2026
Merged

fix(#2852): re-stage and retry when pre-commit hooks auto-fix files#2855
waynesun09 merged 2 commits into
mainfrom
agent/2852-precommit-autofix-retry

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

When a pre-commit hook auto-fixes files (e.g. gofmt, ruff format, prettier), it exits non-zero but leaves the corrected files as unstaged modifications. Previously, post-code.sh and post-fix.sh treated any non-zero exit as a hard failure, wasting the entire agent run.

After a failed pre-commit run, check git diff for unstaged changes. If hooks auto-fixed files, re-stage them, amend the commit, and retry once. If the retry still fails or no unstaged changes exist, exit 1 as before.

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


Closes #2852

Post-script verification

  • Branch is not main/master (agent/2852-precommit-autofix-retry)
  • Secret scan passed (gitleaks — 14b0b41dd016b3af0c10c288978708f4e37a4c2f..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner July 1, 2026 16:10
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Jul 1, 2026
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

Site preview

Preview: https://2fd1afab-site.fullsend-ai.workers.dev

Commit: 186468d4d5f89d06737f6d088e14b1b44333b3a0

@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

@waynesun09
waynesun09 force-pushed the agent/2852-precommit-autofix-retry branch from 8928c02 to 1674813 Compare July 1, 2026 17:41
@waynesun09 waynesun09 added the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jul 1, 2026
@waynesun09
waynesun09 force-pushed the agent/2852-precommit-autofix-retry branch from 1674813 to 2d85447 Compare July 1, 2026 19:41
@github-actions github-actions Bot removed the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jul 1, 2026
@waynesun09
waynesun09 force-pushed the agent/2852-precommit-autofix-retry branch from 2d85447 to a6f5a17 Compare July 1, 2026 19:55
@waynesun09

Copy link
Copy Markdown
Member

/review

@qodo-code-review

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Warning

/review is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2852 - Partially compliant

Compliant requirements:

  • Detect when pre-commit failures correspond to auto-fixes via checking for unstaged diffs
  • Re-stage auto-fixed files and amend the commit
  • Retry pre-commit once (no loop)
  • Avoid staging untracked files (uses git add with tracked paths)
  • Preserve failure behavior and error messaging when retry fails or when there was no auto-fix

Non-compliant requirements:

  • Only re-stage appropriate changes (safe scope; avoid staging unrelated files)

Requires further human verification:

  • Confirm the scoping to changed_array does not accidentally miss required files for hooks that modify related files outside the initially-changed set (e.g., formatters touching generated files or shared modules).
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Scope mismatch

The retry detection/staging is scoped to changed_array, but some hooks may legitimately modify additional tracked files outside that initial list. This could cause the script to miss auto-fixes (no retry) or leave repo in a modified state while still failing. Consider whether detection/staging should be based on overall tracked diffs (e.g., git diff --name-only) while still keeping a safety boundary, or explicitly document/handle hooks that touch additional files.

# Scope detection/staging to changed_array so hooks can't inject files
# outside the pre-commit scope into the commit.
if git diff --name-only -- "${changed_array[@]}" | grep -q .; then
  echo "::warning::Pre-commit hooks auto-fixed files — re-staging and retrying"
  echo "Auto-fixed files:"
  git diff --name-only -- "${changed_array[@]}" | sed 's/^/  /'
  git diff --name-only -z -- "${changed_array[@]}" | xargs -0 -r git add --
  git commit --amend --no-edit

  echo "Re-running secret scan on amended commit..."
  if ! gitleaks detect --source . --log-opts="${SCAN_RANGE}" --redact; then
    echo "::error::BLOCKED — secret detected in amended commit after auto-fix" >&2
    exit 1
  fi
  if git log --format='%b' "${SCAN_RANGE}" | grep -q '^Signed-off-by:'; then
    echo "::error::BLOCKED — amended commit contains a Signed-off-by trailer" >&2
    exit 1
  fi

  if [ -n "${MERGE_BASE}" ]; then
    CHANGED_FILES="$(git diff --name-only "${MERGE_BASE}..HEAD")"
  else
    CHANGED_FILES="$(git diff --name-only "origin/${TARGET_BRANCH}..HEAD" 2>/dev/null \
      || git diff --name-only HEAD~1..HEAD 2>/dev/null || true)"
  fi
  if [ -z "${CHANGED_FILES}" ]; then
    echo "::error::BLOCKED — pre-commit hooks removed all changes; commit is now empty" >&2
    exit 1
  fi
  mapfile -t changed_array <<< "${CHANGED_FILES}"
  if pre-commit run --files "${changed_array[@]}"; then
    if git diff --name-only -- "${changed_array[@]}" | grep -q .; then
      echo "::error::BLOCKED — retry pre-commit left additional unstaged changes" >&2
      echo "::error::Committed content would diverge from what pre-commit validated." >&2
      exit 1
    fi
    echo "Pre-commit passed after auto-fix re-stage"
Portability

xargs -0 -r is not portable across all environments (notably BSD/macOS xargs lacks -r). If these scripts can run on non-GNU systems, this could break. Consider avoiding -r (and guarding empty input another way), or using a shell loop with NUL-delimited reads.

git diff --name-only -- "${changed_array[@]}" | sed 's/^/  /'
git diff --name-only -z -- "${changed_array[@]}" | xargs -0 -r git add --
git commit --amend --no-edit
Amend assumptions

git commit --amend --no-edit can fail if the environment lacks required git identity/config or if hooks run on commit amend unexpectedly. Ensure the runner environment guarantees user.name/user.email, and consider clearer error output if amend fails (currently the script would proceed only if set -e behavior is present elsewhere; not visible in diff).

git diff --name-only -z -- "${changed_array[@]}" | xargs -0 -r git add --
git commit --amend --no-edit

echo "Re-running secret scan on amended commit..."
if ! gitleaks detect --source . --log-opts="${SCAN_RANGE}" --redact; then
  echo "::error::BLOCKED — secret detected in amended commit after auto-fix" >&2
  exit 1
fi
if git log --format='%b' "${SCAN_RANGE}" | grep -q '^Signed-off-by:'; then
  echo "::error::BLOCKED — amended commit contains a Signed-off-by trailer" >&2
  exit 1
fi

fullsend-ai-coder Bot and others added 2 commits July 1, 2026 16:30
When a pre-commit hook auto-fixes files (e.g. gofmt, ruff format,
prettier), it exits non-zero but leaves the corrected files as unstaged
modifications. Previously, post-code.sh and post-fix.sh treated any
non-zero exit as a hard failure, wasting the entire agent run.

After a failed pre-commit run, check git diff for unstaged changes. If
hooks auto-fixed files, re-stage them, amend the commit, and retry once.
If the retry still fails or no unstaged changes exist, exit 1 as before.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Re-run gitleaks secret scan and signed-off-by check after amending
the commit with auto-fixed files, closing the bypass window where
hooks could inject unscanned content. Scope git-add to only
hook-modified files instead of the entire tracked tree. Rebuild the
changed file list from merge-base after amend so the retry pre-commit
runs on the correct set. Add SYNC cross-reference comments between
post-code.sh and post-fix.sh retry blocks.

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — 4 rounds of multi-agent review (Claude, Gemini, Codex) with 0 verified MEDIUM+ findings remaining.

Round 4 (final): 4 agents, 3 produced findings. All MEDIUM+ findings are false positives, by-design choices, or self-downgraded by the agents after analysis. Key design decisions validated:

  • Scoped git diff/add to changed_array — security defense against hook file injection
  • Single-retry cap with residual-unstaged guard — handles non-idempotent hooks correctly
  • SYNC comments over shared helper extraction — different variable names make extraction complex
  • SCAN_RANGE remains valid after amend — HEAD resolves dynamically

@waynesun09
waynesun09 added this pull request to the merge queue Jul 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 1, 2026
@waynesun09
waynesun09 added this pull request to the merge queue Jul 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 1, 2026
@waynesun09
waynesun09 added this pull request to the merge queue Jul 1, 2026
Merged via the queue into main with commit dcd3b43 Jul 1, 2026
16 checks passed
@waynesun09
waynesun09 deleted the agent/2852-precommit-autofix-retry branch July 1, 2026 21:24
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 1, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 9:28 PM UTC · Completed 9:36 PM UTC
Commit: 186468d · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2855 — pre-commit auto-fix retry

Timeline:

  1. Code agent run 28530739183 produced a working implementation in ~10 minutes (16:00–16:10 UTC).
  2. Human reviewer (waynesun09) pushed a hardening commit (186468d) at 17:30 UTC that added 6 security-critical changes the code agent missed: scoped git diff/git add to changed_array, post-amend gitleaks re-scan, post-amend Signed-off-by check, empty-commit guard, retry-left-unstaged detection, and SYNC comments.
  3. Four rounds of multi-agent review (Claude, Gemini, Codex) found zero verified MEDIUM+ findings — all security gaps were caught exclusively by the human reviewer.
  4. PR merged at 21:24 UTC.

Key finding: The code agent solved the functional requirement (retry on auto-fix) but missed that amending a commit in a security-critical post-script reopens the security boundary — requiring re-scanning for secrets, re-validating changed file scope, and guarding against hook file injection. Four rounds of automated review also missed these gaps.

Existing issues considered:

  • #2107 — code agent replicating security patterns. Overlaps but focuses on in-file pattern replication, not on documenting invariants for agent consumption.
  • #836 — shared helper extraction for post-code/post-fix. Already tracked; skipped.
  • #2644 — review agent deeper security analysis. Overlaps on the review side but is about authorization/privilege-escalation, not post-script commit-integrity invariants.

1 proposal filed below.

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review Triggers review agent dispatch Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

post-code: re-stage and retry when pre-commit hooks auto-fix files instead of failing

1 participant