Skip to content

Fix annoying Formatting fails on PR synchronization #98

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.

Sign up for GitHub

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

Closed
schewskone wants to merge 1 commit into from
Closed

Fix annoying Formatting fails on PR synchronization #98

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 45 additions & 41 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,49 +38,53 @@ jobs:
pytest -v tests/

lint-and-format:
name: Lint & Auto-format
runs-on: ubuntu-latest
if: |
github.event_name == 'push' ||
(
github.event_name == 'pull_request' &&
github.repository != github.event.pull_request.head.repo.full_name
)
name: Lint & Auto-format
runs-on: ubuntu-latest
# IMPROVED LOGIC:
# 1. Run on pushes to any branch EXCEPT main (to avoid protection issues)
# 2. Run on PRs only if they come from a FORK (where we can't push)
if: |
(github.event_name == 'push' && github.ref != 'refs/heads/main') ||
(github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository)
Comment on lines +46 to +48
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The workflow is missing explicit permissions configuration for the GITHUB_TOKEN. When the job attempts to push commits (line 81), it requires write access to the repository contents. Without explicit permissions set, the job relies on the default token permissions, which may be read-only depending on the repository settings. Add a permissions block with contents: write to ensure the token has the necessary permissions to push formatting commits.

Copilot uses AI. Check for mistakes.
Comment on lines +46 to +48
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The condition logic is inconsistent with the test job above (lines 15-20). The test job runs on all push events and forked PRs, while this formatting job excludes pushes to the main branch. This creates an inconsistency where tests run on main branch pushes but formatting doesn't. If the goal is to avoid conflicts with branch protection on main, consider whether the same logic should apply to the test job, or if the formatting job should handle main branch pushes differently (e.g., check-only mode).

Copilot uses AI. Check for mistakes.
Comment on lines +45 to +48
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The workflow will never run for PRs from the same repository because the condition excludes them. This means that when a developer pushes to a PR branch in the same repo to synchronize it, the formatting job won't run at all. This could be the intended behavior to avoid the "annoying failures" mentioned in the PR title, but it leaves same-repo PRs without any formatting checks. Consider whether same-repo PRs should at least run in check-only mode to catch formatting issues before merge.

Suggested change
# 2. Run on PRs only if they come from a FORK (where we can't push)
if: |
(github.event_name == 'push' && github.ref != 'refs/heads/main') ||
(github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository)
# 2. Run on ALL PRs (forks and same-repo branches) in check-only mode
if: |
(github.event_name == 'push' && github.ref != 'refs/heads/main') ||
(github.event_name == 'pull_request')

Copilot uses AI. Check for mistakes.

steps:
- name: Checkout repository
uses: actions/checkout@v4
steps:
- name: Checkout repository
uses: actions/checkout@v4
with:
# Use a PAT if you want the 'auto-format' commit to trigger other CI tests
# Use GITHUB_TOKEN if you want it to be "silent"
token: ${{ secrets.GITHUB_TOKEN }}
ref: ${{ github.event.pull_request.head.ref || github.ref_name }}
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The checkout step for PR events from forks may not work correctly. The ref parameter uses github.event.pull_request.head.ref, which provides only the branch name (e.g., "feature-branch"), not a full Git reference. For forks, this will attempt to check out a branch that doesn't exist in the base repository. A proper approach would be to checkout the head SHA or use github.event.pull_request.head.sha to ensure the correct code is checked out from the fork.

Suggested change
ref: ${{ github.event.pull_request.head.ref || github.ref_name }}

Copilot uses AI. Check for mistakes.

- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: '3.12.8'
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: '3.12.8'

- name: Install tools
run: |
python -m pip install --upgrade pip
pip install "black[jupyter]==25.1.0" isort==6.0.1
- name: Install tools
run: |
python -m pip install --upgrade pip
pip install "black[jupyter]==25.1.0" isort==6.0.1

- name: Run Black and Isort
run: |
if [[ "${{ github.event_name }}" == "push" && "${{ github.repository }}" == "${{ github.event.repository.full_name }}" ]]; then
echo "Push event in same repo: Running black and isort with auto-format and commit"
black .
isort .
git config user.name "github-actions"
git config user.email "github-actions@github.com"
if ! git diff --quiet; then
git commit -am "style: auto-format with black and isort"
git push
else
echo "No formatting changes to commit."
fi
- name: Run Black and Isort
run: |
if [[ "${{ github.event_name }}" == "push" ]]; then
echo "Standard Branch Push: Formatting and pushing back to ${{ github.ref_name }}"
black .
isort .

git config user.name "github-actions"
git config user.email "github-actions@github.com"

if ! git diff --quiet; then
git commit -am "style: auto-format with black and isort"
git push
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The simplified logic in the bash script no longer validates that push events are from the same repository. The old code checked github.repository == github.event.repository.full_name before attempting to push. While the YAML condition on line 47 prevents the job from running on main branch pushes, it doesn't prevent the job from attempting to push to protected branches or branches where the GITHUB_TOKEN might not have write access. Consider adding a check or handling potential push failures gracefully.

Suggested change
git push
# Extra safety: only push if the event repository matches this repository
if [[ "${GITHUB_REPOSITORY}" == "${{ github.event.repository.full_name }}" ]]; then
echo "Repository match confirmed (${GITHUB_REPOSITORY}). Attempting to push formatting changes..."
if ! git push; then
echo "git push failed (likely due to permissions or branch protection); skipping auto-push."
fi
else
echo "Repository mismatch (GITHUB_REPOSITORY=${GITHUB_REPOSITORY}, event.repository.full_name=${{ github.event.repository.full_name }})."
echo "Skipping git push to avoid pushing to an unexpected repository."
fi

Copilot uses AI. Check for mistakes.
else
echo "No changes."
fi

elif [[ "${{ github.event_name }}" == "pull_request" ]]; then
echo "PR from fork: Running black and isort in check mode"
black --check . || { echo "Black formatting issues found. Run 'black .' to fix."; exit 1; }
isort --check-only . || { echo "isort import order issues found. Run 'isort .' to fix."; exit 1; }

else
echo "Not a forked PR and not a push. Skipping format."
fi
else
echo "Forked PR: Check-only mode (No push)"
black --check . || { echo "Black failed"; exit 1; }
isort --check-only . || { echo "Isort failed"; exit 1; }
Comment on lines +88 to +89
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The error messages have been changed from more informative messages that explain how to fix the issue (e.g., "Black formatting issues found. Run 'black .' to fix.") to terse messages ("Black failed"). The more detailed error messages help developers understand what went wrong and how to resolve it. Consider keeping the more descriptive error messages from the original implementation.

Suggested change
black --check . || { echo "Black failed"; exit 1; }
isort --check-only . || { echo "Isort failed"; exit 1; }
black --check . || { echo "Black formatting issues found. Run 'black .' to fix them locally."; exit 1; }
isort --check-only . || { echo "Isort formatting issues found. Run 'isort .' to fix them locally."; exit 1; }

Copilot uses AI. Check for mistakes.
fi
Comment on lines +41 to +90
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The entire lint-and-format job definition has incorrect indentation. Comparing with the test job above (lines 12-38), job properties like name, runs-on, if, and steps should be indented with 4 spaces from the job key. Currently, these properties are indented with 6 spaces. This inconsistency may cause YAML parsing issues or workflow failures.

Copilot uses AI. Check for mistakes.
Loading