Skip to content

Fix CI pre-commit file-mode failure #581

Open
xyao-nv wants to merge 3 commits into
mainfrom
xyao/ci/precommit_dirty
Open

Fix CI pre-commit file-mode failure #581
xyao-nv wants to merge 3 commits into
mainfrom
xyao/ci/precommit_dirty

Conversation

@xyao-nv
Copy link
Copy Markdown
Collaborator

@xyao-nv xyao-nv commented Apr 10, 2026

Summary

Fix pre-commit CI job failing with "pre-commit hook(s) made changes". E.g. https://github.com/isaac-sim/IsaacLab-Arena/actions/runs/24260814760/job/70843875557

Detailed description

  • Every file showing a 644→755 mode-only diff in pre-commit failed CI jobs
  • It is caused by the runner's persistent workspace retaining executable permissions across runs, which hooks then propagate when rewriting files.
  • Set to ignore it form git as a fix.

@xyao-nv xyao-nv marked this pull request as ready for review April 10, 2026 21:25
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 10, 2026

Greptile Summary

This PR adds git config --global core.fileMode false to the pre_commit job to prevent self-hosted runner workspace file-mode drift from causing spurious pre-commit failures.

  • The new step is placed before the "Install git" step, but python:3.11-slim does not ship with git — so git config will fail with command not found, leaving the original CI failure unfixed. The step must be moved to after the "Install git" step.

Confidence Score: 4/5

The intent is correct but the step ordering makes the fix self-defeating — needs one reorder before merging.

There is a single P1 issue: the git config step runs before git is installed in the slim container, so the job will fail at that step. The root cause diagnosis and the chosen fix (core.fileMode false) are sound; the only change needed is moving the new step after "Install git".

.github/workflows/ci.yml — step ordering around git config and git installation.

Important Files Changed

Filename Overview
.github/workflows/ci.yml Adds git config --global core.fileMode false to fix 644→755 mode drift on self-hosted runners, but the step is placed before git is installed — the command will fail in the python:3.11-slim container where git is not pre-installed.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Ignore file-mode changes\ngit config --global core.fileMode false\n⚠️ git not yet installed!"] -->|FAILS: git not found| B["Install git\napt-get install git ..."]
    B --> C["Clean up symlinks"]
    C --> D["Mark repo as safe\ngit config safe.directory"]
    D --> E["Checkout Code\nactions/checkout@v4"]
    E --> F["Git LFS pull"]
    F --> G["git status"]
    G --> H["Run pre-commit\npre-commit/action@v3.0.1"]

    style A fill:#ff6b6b,color:#fff
    style B fill:#ffd93d
Loading

Reviews (1): Last reviewed commit: "try if it fix" | Re-trigger Greptile

Comment thread .github/workflows/ci.yml Outdated
Comment on lines +49 to +50
- name: Ignore file-mode changes
run: git config --global core.fileMode false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 git config runs before git is installed

The python:3.11-slim image does not ship with git. The "Install git" step (line 52–58) is where git is first made available, but git config --global core.fileMode false runs before it — so this step will fail with git: command not found, breaking the job it was meant to fix.

Move the "Ignore file-mode changes" step to after the "Install git" step:

Suggested change
- name: Ignore file-mode changes
run: git config --global core.fileMode false
- &install_git_step
name: Install git (and tools needed by hooks)
run: |
apt-get update
apt-get install -y --no-install-recommends git git-lfs clang-format ca-certificates make
git --version
git lfs version
# On self-hosted runners the workspace persists between runs and files
# can end up with 755 permissions. Hooks that rewrite files preserve
# the 755 mode, causing a 644→755 diff that fails pre-commit.
# Telling git to ignore file-mode changes avoids this.
- name: Ignore file-mode changes
run: git config --global core.fileMode false

(Note: a multi-hunk edit is needed; the original block at lines 49–50 should also be removed.)

Copy link
Copy Markdown

@kellyguo11 kellyguo11 left a comment

Choose a reason for hiding this comment

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

LGTM — clean, minimal fix.

What it does: On self-hosted runners, the persistent workspace can leave files with executable (755) permissions from previous runs. Pre-commit hooks that rewrite files preserve those modes, producing spurious 644→755 diffs that fail the check-executables-have-shebangs and similar checks. Setting core.fileMode false tells git to ignore these permission-only changes.

Step ordering is correct: The git config runs after checkout (so .git/ exists) and right before pre-commit/action, which is the ideal placement. Uses local config (not --global), scoped to this repo only — good.

CI confirms: Pre-commit job passes on the latest commit (a18564ca). The Run policy-related tests failure is unrelated to this change.

Note: Greptile's comment about step ordering was based on the first commit; the second commit (reorder) fixed that, so it's a stale concern.

Nit (non-blocking): Commit messages could be more descriptive ("try if it fix" → something like "ci: ignore file-mode changes in pre-commit job"), but that's cosmetic.

alexmillane
alexmillane previously approved these changes Apr 13, 2026
Copy link
Copy Markdown
Collaborator

@alexmillane alexmillane left a comment

Choose a reason for hiding this comment

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

Looks reasonable if it works!

Copy link
Copy Markdown
Contributor

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

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

🤖 Isaac Lab Review Bot

Summary

Adds git config core.fileMode false before the pre-commit step in CI to prevent spurious 644→755 file-mode diffs on self-hosted runners. Clean, targeted fix for a known CI flake.

Design Assessment

Design is sound. Setting core.fileMode false at the workflow level is the standard fix for permission-drift on persistent workspaces. The step is correctly placed after checkout/LFS but before pre-commit/action, and the comment explains the why clearly.

Findings

No issues found. This is a well-scoped, minimal CI fix:

  • ✅ Correct placement in the workflow (after git status, before pre-commit)
  • ✅ Uses git config (local scope by default) — won't leak to other jobs or repos
  • ✅ Comment explains the root cause concisely
  • ✅ Pre-commit check is now passing on this branch

Test Coverage

No new tests needed — this is a CI workflow configuration change with no runtime impact on the codebase. The pre-commit CI check itself serves as the validation (and it's passing ✅).

CI Status

  • Pre-commit: ✅ Passing
  • Docs build: ✅ Passing
  • Tests: 🔄 In progress

Verdict

APPROVE — Correct fix, minimal blast radius, well-documented. Ship it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants