ci: add /lint-fix workflow for auto-fixable lint failures#322
ci: add /lint-fix workflow for auto-fixable lint failures#322jarcherNV wants to merge 4 commits into
Conversation
|
/ok to test 6fba70b |
Greptile SummaryThis PR wires up a
Confidence Score: 5/5Safe to merge; all changed paths behave correctly for the common case of auto-fixable failures. The new workflows are structurally sound: the fork guard, idempotent bot comment, branch-name normalisation, and target-branch tooling isolation all work as intended. The sync_version.py refactor is clean with no regressions. The two findings are limited to suboptimal UX — a misleading CI notice when only non-fixable hooks fail, and partial fixes being silently discarded when a later hook fails without producing changes — neither causes incorrect or harmful behaviour. .github/workflows/lint-fix.yml (run_hook early-exit path) and .github/workflows/ci.yml (unconditional /lint-fix hint). Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant GH as GitHub PR
participant SCD as slash-command-dispatch.yml
participant LF as lint-fix.yml
participant PB as PR Branch
participant TB as Target Branch
Dev->>GH: comment /lint-fix
GH->>SCD: issue_comment event
SCD->>SCD: check permission (write)
SCD->>GH: repository_dispatch lint-fix-command
GH->>LF: repository_dispatch event
LF->>LF: Check PR branch scope (fork guard)
LF->>PB: checkout pr-branch
LF->>TB: checkout target-branch
LF->>TB: uv sync --only-group lint --frozen
loop each fixable hook
LF->>LF: run_hook (trailing-whitespace, end-of-file-fixer, ruff-fix, ruff-format)
LF->>LF: compare before/after git diff
end
LF->>PB: run sync_version.py --repo-root .
alt changes produced
LF->>GH: create-pull-request
LF->>GH: post/update bot comment with PR link
LF->>GH: add hooray reaction to trigger comment
else no changes
LF->>GH: post/update bot comment: no fixes produced
else hook failed without fixes
LF->>GH: post/update bot comment: lint fix failed
end
Reviews (4): Last reviewed commit: "ci: create lint-fix PRs without bot fork" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
/ok to test 342e7f6 |
Add a PR comment command that runs fixable pre-commit hooks against the PR branch and opens a bot PR with the resulting changes. The workflow uses trusted target-branch tooling, covers formatting/import/version-sync fixes, and leaves non-fixable ty or REUSE failures for manual repair. Also surface the /lint-fix hint in CI output and CONTRIBUTING.md.
Keep uv cache and tool installs under the runner temp directory, and treat hook infrastructure failures as real failures instead of assuming every nonzero hook exit produced an automatic fix. Compare each hook's before/after diff so exit code 1 is only accepted when that specific hook rewrote files.
Guard /lint-fix against fork-originated PR branches so the workflow reports unsupported cases clearly instead of failing during PR creation. Use the target branch uv lockfile for pre-commit execution, normalize generated fix branch names, and update the bot status comment in place across repeated invocations. Also make sync_version.py pass repo_root explicitly instead of mutating the module-level REPO_ROOT.
Use the automatic GITHUB_TOKEN to push lint-fix branches directly to the main repository and open PRs against internal PR branches. This removes the FLASHDREAMS_BOT_PAT requirement, bot identity lookup, and push-to-fork setup while preserving the explicit guard against fork-originated PR branches.
jarcherNV
force-pushed
the
dev/jarcher/linterfix
branch
from
818df29 to
6ebc6be
Compare
|
/ok to test 6ebc6be |
| <!-- flashdreams-lint-fix --> | ||
| ${{ | ||
| steps.pr-scope.outputs.supported != 'true' | ||
| && format('`/lint-fix` can only update PR branches in NVIDIA/flashdreams. This PR branch is in `{0}`, so no automatic fix PR was created. Use the copied internal PR from the `/ok to test` flow, or apply fixes manually.', github.event.client_payload.pull_request.head.repo.full_name) |
There was a problem hiding this comment.
so this would not work for PRs that were started from remote forks? given this is the main way for external contributions, I'm unsure if adding this complexity is worth the effort?
There was a problem hiding this comment.
Yes, this does not operate on the original branch of a PR opened from a fork. That is intentional for now: the workflow only creates fix PRs against branches that live in NVIDIA/flashdreams, because pushing or opening helper PRs against arbitrary contributor forks gets fragile and permission-dependent.
So this is mainly useful for internal PR branches, or for copied/internal PRs if the copy-pr-bot flow is used. If we want this to support original external fork PRs directly, we would need a different design, which may or may not be worth the complexity.
| path: pr-branch | ||
| commit-message: "lint fixes" | ||
| signoff: true | ||
| title: "Apply lint fixes for PR #${{ github.event.client_payload.pull_request.number }}" |
There was a problem hiding this comment.
is it really worth having yet another PR to fix linter issues in a given PR if all it would take is for developers to run the command locally (some people even register this as a pre-commit hook)?
I might be to conservative, but feels like this could add a lot of PR noise? Or is this just "temporary" / automatically merged into the source PR to be fixed in a transparent way?
There was a problem hiding this comment.
That is fair. The current design intentionally creates a separate short-lived fix PR instead of pushing directly to the contributor’s branch, so the automated changes stay explicit and reviewable. It is not automatically merged; the author or maintainer would still merge the fix PR into the source PR branch.
The noise should be limited because this only runs when someone with write permission comments /lint-fix, but that may be too much process overhead? I am fine dropping this and keeping the CI hint as “run pre-commit locally” guidance instead. What do you think?
There was a problem hiding this comment.
yeah, to be honest, from my point of view this "let the CI fix my stuff" approach feels overly complex, fragile and also likely also confusing (let alone the wait-time for jobs to spin up just to run a trivial 1sec command).
Moreover, even if we'd refine this + update the original branch we'd still have the issue that we need to commit the changes and orchestrate these with the source changes. This would end up with a bunch of "regular" commits + noisy "fix lint" commits in the history, which is another source of noise in the repo as ideally developers would locally run the pre-commit fixes "before" the commits are actually created - if one doesn't run this / or uses a different process, I'd expect developers to then git commit --amend / jj absorb the fixes into the source commits so that we don't end up with myriads of bad-practice "fix format" / "fix lin" commits in the history and keep the commit history atomic + focused.
Hence, I would rather just make sure to use the existing local setup properly and make this as easy as possible (we can also teach agents to just run this trivial step locally before committing as it's meant to be used) - if people run into CI failures due to formatting issues, we can always point to the local pre-commit guidance that seems to not be followed in that cases. If there are ergonomic issues with the local pre-commit guidance that make folks not use it we should rather look into improving that part to try to make that more seamless.
In general, as a personal rule of thumb, I don't think it should be the CI's job to create any commits of any form - it's just supposed to "validate" what developers provide, not produce new stuff (even as simple as auto-fixing formatting), as this is mixing concerns and will always be the source of conflicts + confusion
2 participants
Add a PR comment command that runs fixable pre-commit hooks against the
PR branch and opens a bot PR with the resulting changes. The workflow
uses trusted target-branch tooling, covers formatting/import/version-sync
fixes, and leaves non-fixable ty or REUSE failures for manual repair.
Also surface the /lint-fix hint in CI output and CONTRIBUTING.md.