Skip to content

ci: auto-commit pre-commit fixes on same-repo PRs - #484

Open
rm3l wants to merge 3 commits into
redhat-developer:mainfrom
rm3l:ci/auto-commit-precommit-fixes
Open

ci: auto-commit pre-commit fixes on same-repo PRs#484
rm3l wants to merge 3 commits into
redhat-developer:mainfrom
rm3l:ci/auto-commit-precommit-fixes

Conversation

@rm3l

@rm3l rm3l commented Jul 22, 2026

Copy link
Copy Markdown
Member

Description of the change

Similar to redhat-developer/rhdh-operator#3228 for the Operator, this auto-commits and pushes pre-commit hook changes for same-repo PRs using the rhdh-bot token, reducing friction for maintainers, especially when reviewing PRs from Renovate in the same repo.

For fork PRs, the existing behavior is preserved: the workflow fails and a comment is posted.

Which issue(s) does this PR fix or relate to

How to test changes / Special notes to the reviewer

  • Same-repo PR: if pre-commit hooks modify files, rhdh-bot auto-commits and pushes the fixes; workflow passes.
  • Fork PR with changes: auto-push step is skipped, workflow fails, and the existing pre-commit-comment.yaml workflow posts a comment.
  • Missing RHDH_BOT_TOKEN or push failure: workflow fails gracefully with the existing error message.

Checklist

  • For each Chart updated, version bumped in the corresponding Chart.yaml according to Semantic Versioning. N/A — CI-only change.
  • For each Chart updated, variables are documented in the values.yaml and added to the corresponding README.md. N/A — CI-only change.
  • JSON Schema template updated and re-generated the raw schema via the pre-commit hook. N/A — CI-only change.
  • Tests pass using the Chart Testing tool and the ct lint command. N/A — CI-only change.
  • If you updated the orchestrator-infra chart, make sure the versions of the Knative CRDs are aligned with the versions of the CRDs installed by the OpenShift Serverless operators declared in the values.yaml file. N/A — CI-only change.

Similar to redhat-developer/rhdh-operator#3228, auto-commit and push
pre-commit hook changes for same-repo PRs using the rhdh-bot token.
Fork PRs retain the existing fail-and-comment behavior.
@rm3l
rm3l requested a review from a team as a code owner July 22, 2026 08:38
@rhdh-qodo-merge

rhdh-qodo-merge Bot commented Jul 22, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Context used
✅ Cross-repo context
  Not relevant to this PR: redhat-developer/rhdh
  Not relevant to this PR: redhat-developer/rhdh-plugins
  Not relevant to this PR: redhat-developer/rhdh-must-gather

Grey Divider


Action required

1. Token stored in git 🐞 Bug ⛨ Security ⭐ New
Description
The auto-push step writes RHDH_BOT_TOKEN into the origin remote URL, persisting it in
.git/config for the remainder of the job and making it readable by later steps (including the
second pre-commit run). This creates a credible token exfiltration path via repository-controlled
pre-commit hooks and unnecessarily broadens exposure of a write-capable credential.
Code

.github/workflows/pre-commit.yaml[R80-94]

+          git config user.name "rhdh-bot"
+          git config user.email "rhdh-bot@redhat.com"
+          git remote set-url origin "https://x-access-token:${RHDH_BOT_TOKEN}@github.com/${{ github.repository }}.git"
+          git add -A
+          git commit -m "chore: apply pre-commit fixes"
+          if git push origin "HEAD:${HEAD_REF}"; then
+            echo "pushed=true" >> "$GITHUB_OUTPUT"
+          else
+            echo "pushed=false" >> "$GITHUB_OUTPUT"
+            git reset --mixed HEAD~1
+          fi
+
+      - name: Re-run pre-commit after auto-push
+        if: steps.autopush.outputs.pushed == 'true'
+        uses: pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd # v3.0.1
Relevance

⭐⭐⭐ High

PR #306 explicitly avoided token leakage to hooks (persist-credentials: false); embedding token in
remote URL undermines that.

PR-#306
PR-#223
PR-#230

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The workflow explicitly embeds the secret token into the remote URL and then runs another pre-commit
invocation afterward, which executes hooks defined by the checked-out repository; the repo’s
pre-commit configuration includes repo: local hooks with arbitrary command entry fields,
demonstrating that PR-controlled code can execute during pre-commit and could read .git/config if
the token is persisted there.

.github/workflows/pre-commit.yaml[67-97]
.pre-commit-config.yaml[14-28]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The workflow persists `RHDH_BOT_TOKEN` in the git remote URL via `git remote set-url ...x-access-token:${RHDH_BOT_TOKEN}...`, which writes the token into `.git/config`. Because the job later executes repository-controlled code (e.g., pre-commit hooks during the re-run), that token can be read/exfiltrated by subsequent commands.

## Issue Context
This is introduced in the new “Auto-commit and push pre-commit changes” step and is made worse by the follow-up “Re-run pre-commit after auto-push” step, which executes hooks defined in the checked-out PR content.

## Fix
Authenticate only for the single push command and do not store the token in repo config.

Concrete options:
- Push using an explicit URL without changing `origin`:
 - `git push "https://x-access-token:${RHDH_BOT_TOKEN}@github.com/${{ github.repository }}.git" "HEAD:${HEAD_REF}"`
- Or set the remote URL only temporarily and then immediately restore it (or remove the remote) *before* any further steps that run repo-controlled code.
- Prefer `http.extraheader`/`GIT_ASKPASS` approaches if you want to avoid putting the token into config entirely.

## Fix Focus Areas
- .github/workflows/pre-commit.yaml[67-97]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Untracked changes missed ✓ Resolved 🐞 Bug ☼ Reliability
Description
The “Check for changes” step uses git diff --quiet, which does not account for untracked files, so
hooks that generate new files can leave the workspace dirty while changed=false and the workflow
proceeds as if it’s clean. Because both auto-push and the final failure gate depend on changed,
this can incorrectly produce a passing run.
Code

.github/workflows/pre-commit.yaml[R56-65]

+      - name: Check for changes
+        id: check
+        run: |
+          if git diff --quiet; then
+            echo "✅ All files are up to date"
+            echo "changed=false" >> "$GITHUB_OUTPUT"
+          else
+            echo "Pre-commit hooks modified files"
+            echo "changed=true" >> "$GITHUB_OUTPUT"
+          fi
Relevance

⭐⭐ Medium

No historical review evidence about detecting untracked files in “check for changes” logic in
workflows.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The workflow’s changed output is computed solely from git diff --quiet, and both auto-push and
the final failing step are conditioned on changed, so missing untracked-file detection directly
causes missed enforcement.

.github/workflows/pre-commit.yaml[56-65]
.github/workflows/pre-commit.yaml[67-71]
.github/workflows/pre-commit.yaml[92-94]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The workflow determines whether pre-commit modified files using `git diff --quiet`, which ignores untracked files. This can miss newly generated files and incorrectly set `changed=false`, skipping both auto-push and the failure step.

### Issue Context
Auto-push and the final failing step are gated entirely on `steps.check.outputs.changed`, so missing untracked files breaks the intended enforcement.

### Fix Focus Areas
- .github/workflows/pre-commit.yaml[56-71]

Suggested implementation direction:
- Replace the change check with a working-tree cleanliness check that includes untracked files, e.g.:
 - `if [[ -z "$(git status --porcelain)" ]]; then ... else ... fi`
- If you intentionally want to ignore ignored files, keep default `git status --porcelain` behavior (it won’t list ignored files unless `--ignored` is used).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Pre-commit failures ignored ✓ Resolved 🐞 Bug ≡ Correctness
Description
Because the pre-commit action is continue-on-error and the workflow only fails when
steps.check.outputs.changed == 'true', any hook failure that doesn’t leave a tracked diff can
incorrectly result in a successful workflow run. This also prevents the Pre-commit comment
workflow from posting guidance because it only runs on failed conclusions.
Code

.github/workflows/pre-commit.yaml[R92-94]

+      - name: Fail if pre-commit changes are not committed
+        if: steps.check.outputs.changed == 'true' && steps.autopush.outputs.pushed != 'true'
+        run: |
Relevance

⭐⭐ Medium

Team changed pre-commit flow before (PR #306/#230), but no prior accepted feedback on avoiding
continue-on-error masking failures.

PR-#306
PR-#230

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The workflow explicitly ignores the pre-commit step’s failure, and the only enforced failure
condition is tied to changed==true, so a failing pre-commit with no detected diff will still pass;
the comment workflow only posts on failed conclusions, so this case won’t get a comment either.

.github/workflows/pre-commit.yaml[50-55]
.github/workflows/pre-commit.yaml[56-65]
.github/workflows/pre-commit.yaml[92-94]
.github/workflows/pre-commit-comment.yaml[70-72]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Run pre-commit` is configured with `continue-on-error: true`, but the workflow never fails based on the pre-commit step’s outcome; it only fails when the repo is detected as changed and the auto-push didn’t succeed. This can produce false-green CI when pre-commit fails without leaving a tracked diff (e.g., a non-auto-fixable lint failure).

### Issue Context
This also suppresses the separate `pre-commit-comment` workflow, which posts guidance only when the `Pre-commit` workflow’s conclusion is `failure`.

### Fix Focus Areas
- .github/workflows/pre-commit.yaml[50-104]

Suggested implementation direction:
- Give the pre-commit step an `id` (e.g., `id: precommit`).
- Add a final failing step (or extend the existing final step) to also fail when `steps.precommit.outcome == 'failure'`.
 - If you intend to allow “failure due to auto-fixable changes” to pass only after auto-push, then gate failure like:
   - fail when `steps.precommit.outcome == 'failure'` AND (`steps.check.outputs.changed != 'true'` OR `steps.autopush.outputs.pushed != 'true'`)
 - Optionally, re-run pre-commit after applying changes (and after auto-push attempt) without `continue-on-error` to ensure the working tree is actually clean.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Overbroad token permissions ✓ Resolved 🐞 Bug ⛨ Security
Description
The job elevates GITHUB_TOKEN to contents: write for the entire pre-commit job even though
pushes are performed via RHDH_BOT_TOKEN and checkout disables persisted credentials. This unused
write permission increases blast radius if any step/action in the job (now or later) uses
github.token for API calls.
Code

.github/workflows/pre-commit.yaml[R21-22]

+    permissions:
+      contents: write
Relevance

⭐⭐ Medium

Repo values workflow security (PRs #223,#306), but no prior accepted “reduce contents:write”
permission tightening evidence.

PR-#223
PR-#306

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The workflow defaults to contents: read but the job overrides to contents: write; checkout
explicitly avoids persisting credentials and the push is done by injecting RHDH_BOT_TOKEN, so the
job-scoped write permission is not shown as necessary for the implemented logic.

.github/workflows/pre-commit.yaml[13-16]
.github/workflows/pre-commit.yaml[21-22]
.github/workflows/pre-commit.yaml[26-33]
.github/workflows/pre-commit.yaml[72-86]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The pre-commit job grants `contents: write` at job scope, but the workflow’s push path uses a separate `RHDH_BOT_TOKEN` and does not rely on persisted checkout credentials. Keeping `GITHUB_TOKEN` write-scoped unnecessarily broadens what the job can do.

### Issue Context
Workflow-level permissions are `contents: read`, but the job overrides them to `write`.

### Fix Focus Areas
- .github/workflows/pre-commit.yaml[13-33]
- .github/workflows/pre-commit.yaml[67-85]

Suggested implementation direction:
- Drop the job-level `permissions: contents: write` override (or set it back to `contents: read`) if not required.
- If a write-scoped token is needed only for pushing, consider moving the auto-push logic into a separate job that runs only for same-repo PRs (using `if:` at the job level) and grant `contents: write` only to that job.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 603f64b ⚖️ Balanced

Results up to commit d420d6b ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. Pre-commit failures ignored ✓ Resolved 🐞 Bug ≡ Correctness
Description
Because the pre-commit action is continue-on-error and the workflow only fails when
steps.check.outputs.changed == 'true', any hook failure that doesn’t leave a tracked diff can
incorrectly result in a successful workflow run. This also prevents the Pre-commit comment
workflow from posting guidance because it only runs on failed conclusions.
Code

.github/workflows/pre-commit.yaml[R92-94]

+      - name: Fail if pre-commit changes are not committed
+        if: steps.check.outputs.changed == 'true' && steps.autopush.outputs.pushed != 'true'
+        run: |
Relevance

⭐⭐ Medium

Team changed pre-commit flow before (PR #306/#230), but no prior accepted feedback on avoiding
continue-on-error masking failures.

PR-#306
PR-#230

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The workflow explicitly ignores the pre-commit step’s failure, and the only enforced failure
condition is tied to changed==true, so a failing pre-commit with no detected diff will still pass;
the comment workflow only posts on failed conclusions, so this case won’t get a comment either.

.github/workflows/pre-commit.yaml[50-55]
.github/workflows/pre-commit.yaml[56-65]
.github/workflows/pre-commit.yaml[92-94]
.github/workflows/pre-commit-comment.yaml[70-72]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Run pre-commit` is configured with `continue-on-error: true`, but the workflow never fails based on the pre-commit step’s outcome; it only fails when the repo is detected as changed and the auto-push didn’t succeed. This can produce false-green CI when pre-commit fails without leaving a tracked diff (e.g., a non-auto-fixable lint failure).

### Issue Context
This also suppresses the separate `pre-commit-comment` workflow, which posts guidance only when the `Pre-commit` workflow’s conclusion is `failure`.

### Fix Focus Areas
- .github/workflows/pre-commit.yaml[50-104]

Suggested implementation direction:
- Give the pre-commit step an `id` (e.g., `id: precommit`).
- Add a final failing step (or extend the existing final step) to also fail when `steps.precommit.outcome == 'failure'`.
 - If you intend to allow “failure due to auto-fixable changes” to pass only after auto-push, then gate failure like:
   - fail when `steps.precommit.outcome == 'failure'` AND (`steps.check.outputs.changed != 'true'` OR `steps.autopush.outputs.pushed != 'true'`)
 - Optionally, re-run pre-commit after applying changes (and after auto-push attempt) without `continue-on-error` to ensure the working tree is actually clean.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Untracked changes missed ✓ Resolved 🐞 Bug ☼ Reliability
Description
The “Check for changes” step uses git diff --quiet, which does not account for untracked files, so
hooks that generate new files can leave the workspace dirty while changed=false and the workflow
proceeds as if it’s clean. Because both auto-push and the final failure gate depend on changed,
this can incorrectly produce a passing run.
Code

.github/workflows/pre-commit.yaml[R56-65]

+      - name: Check for changes
+        id: check
+        run: |
+          if git diff --quiet; then
+            echo "✅ All files are up to date"
+            echo "changed=false" >> "$GITHUB_OUTPUT"
+          else
+            echo "Pre-commit hooks modified files"
+            echo "changed=true" >> "$GITHUB_OUTPUT"
+          fi
Relevance

⭐⭐ Medium

No historical review evidence about detecting untracked files in “check for changes” logic in
workflows.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The workflow’s changed output is computed solely from git diff --quiet, and both auto-push and
the final failing step are conditioned on changed, so missing untracked-file detection directly
causes missed enforcement.

.github/workflows/pre-commit.yaml[56-65]
.github/workflows/pre-commit.yaml[67-71]
.github/workflows/pre-commit.yaml[92-94]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The workflow determines whether pre-commit modified files using `git diff --quiet`, which ignores untracked files. This can miss newly generated files and incorrectly set `changed=false`, skipping both auto-push and the failure step.

### Issue Context
Auto-push and the final failing step are gated entirely on `steps.check.outputs.changed`, so missing untracked files breaks the intended enforcement.

### Fix Focus Areas
- .github/workflows/pre-commit.yaml[56-71]

Suggested implementation direction:
- Replace the change check with a working-tree cleanliness check that includes untracked files, e.g.:
 - `if [[ -z "$(git status --porcelain)" ]]; then ... else ... fi`
- If you intentionally want to ignore ignored files, keep default `git status --porcelain` behavior (it won’t list ignored files unless `--ignored` is used).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended
3. Overbroad token permissions ✓ Resolved 🐞 Bug ⛨ Security
Description
The job elevates GITHUB_TOKEN to contents: write for the entire pre-commit job even though
pushes are performed via RHDH_BOT_TOKEN and checkout disables persisted credentials. This unused
write permission increases blast radius if any step/action in the job (now or later) uses
github.token for API calls.
Code

.github/workflows/pre-commit.yaml[R21-22]

+    permissions:
+      contents: write
Relevance

⭐⭐ Medium

Repo values workflow security (PRs #223,#306), but no prior accepted “reduce contents:write”
permission tightening evidence.

PR-#223
PR-#306

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The workflow defaults to contents: read but the job overrides to contents: write; checkout
explicitly avoids persisting credentials and the push is done by injecting RHDH_BOT_TOKEN, so the
job-scoped write permission is not shown as necessary for the implemented logic.

.github/workflows/pre-commit.yaml[13-16]
.github/workflows/pre-commit.yaml[21-22]
.github/workflows/pre-commit.yaml[26-33]
.github/workflows/pre-commit.yaml[72-86]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The pre-commit job grants `contents: write` at job scope, but the workflow’s push path uses a separate `RHDH_BOT_TOKEN` and does not rely on persisted checkout credentials. Keeping `GITHUB_TOKEN` write-scoped unnecessarily broadens what the job can do.

### Issue Context
Workflow-level permissions are `contents: read`, but the job overrides them to `write`.

### Fix Focus Areas
- .github/workflows/pre-commit.yaml[13-33]
- .github/workflows/pre-commit.yaml[67-85]

Suggested implementation direction:
- Drop the job-level `permissions: contents: write` override (or set it back to `contents: read`) if not required.
- If a write-scoped token is needed only for pushing, consider moving the auto-push logic into a separate job that runs only for same-repo PRs (using `if:` at the job level) and grant `contents: write` only to that job.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

CI: auto-commit pre-commit fixes on same-repo PRs

✨ Enhancement ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

AI Description

• Allow workflow to push pre-commit fixes on same-repo PR branches.
• Preserve fail-and-comment behavior for forks or missing bot token.
• Improve checkout to target PR head repo/ref without persisting credentials.
Diagram

graph TD
  PR["Pull request event"] --> CO["Checkout PR branch"] --> PC["Run pre-commit"] --> CHK["Detect git diff"] --> D{{"Files changed?"}}
  D -->|"No"| OK["Job succeeds"]
  D -->|"Yes"| SR{{"Same-repo PR?"}}
  SR -->|"Yes"| AP["Commit+push via RHDH_BOT_TOKEN"] --> P{{"Pushed?"}}
  SR -->|"No"| FAIL["Fail job (forks get comment)"]
  P -->|"Yes"| OK
  P -->|"No/Skipped"| FAIL
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use git-auto-commit-action for push step
  • ➕ Less custom bash (standardized commit/push behavior)
  • ➕ Built-in options for commit message, file patterns, and push behavior
  • ➖ Still requires same-repo gating and bot token handling
  • ➖ Adds an additional third-party action dependency to pin and maintain
2. Adopt a dedicated pre-commit bot service (e.g., pre-commit.ci)
  • ➕ Offloads auto-fix/auto-commit behavior to a purpose-built service
  • ➕ Reduces CI workflow complexity
  • ➖ Introduces external service dependency and policy/security review overhead
  • ➖ May not align with org constraints for hosted services
3. Split into read-only check + separate privileged fixer workflow
  • ➕ Clearer separation of untrusted PR execution vs privileged push logic
  • ➕ Can use pull_request_target with tighter, audited steps
  • ➖ More moving parts and workflow coordination (artifacts, status reporting)
  • ➖ Greater maintenance complexity than the current single-workflow approach

Recommendation: The PR’s approach (best-effort auto-push only for same-repo PRs, then fail with clear guidance otherwise) is a pragmatic balance of contributor UX and security. If the bash push logic grows further, consider switching the commit/push portion to a pinned, well-vetted auto-commit action to reduce bespoke scripting while keeping the same-repo gate.

Files changed (1) +57 / -2

Other (1) +57 / -2
pre-commit.yamlAuto-push pre-commit fixes for same-repo PRs +57/-2

Auto-push pre-commit fixes for same-repo PRs

• Updates the pre-commit workflow to check out the PR head repository/ref, run pre-commit in a best-effort mode, and detect whether hooks modified files. For same-repo PRs, it attempts to commit and push fixes using the rhdh-bot token; otherwise it fails with an error message instructing contributors to run pre-commit locally.

.github/workflows/pre-commit.yaml

@rhdh-qodo-merge rhdh-qodo-merge Bot added the enhancement New feature or request label Jul 22, 2026
rm3l added 2 commits July 22, 2026 10:44
Add id to pre-commit step and fail the workflow when pre-commit reports
failure without leaving auto-fixable changes. Re-run pre-commit after
auto-push to verify the tree is actually clean.
- Drop unnecessary job-level contents:write override; the push
  authenticates via RHDH_BOT_TOKEN, not GITHUB_TOKEN.
- Use git status --porcelain instead of git diff --quiet to also
  detect untracked files generated by pre-commit hooks.
@sonarqubecloud

Copy link
Copy Markdown

@rm3l

rm3l commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

/agentic_review

@rm3l

rm3l commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

/cherry-pick release-1.9
/cherry-pick release-1.10

@openshift-cherrypick-robot

Copy link
Copy Markdown

@rm3l: once the present PR merges, I will cherry-pick it on top of release-1.10, release-1.9 in new PRs and assign them to you.

Details

In response to this:

/cherry-pick release-1.9
/cherry-pick release-1.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@rhdh-qodo-merge

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 603f64b

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants