Skip to content

ci(lstt): run formatting on develop pushes, not just PRs - #11000

Merged
marifamd merged 1 commit into
developfrom
users/marifamd/spdx-header-drift
Sep 24, 2026
Merged

marifamd merged 1 commit into
developfrom
users/marifamd/spdx-header-drift

Conversation

@marifamd

@marifamd marifamd commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

amdsmi-license-headers is pass_filenames: false, so it rescans the whole tree.
Once a violation lands on develop, every later PR goes red instead of the merge
that caused it. #10010 merged on a stale-green formatting run and reddened #10973
and #10998 until #8561 fixed the headers. Third time against this hook (#10402,
#10845, #8561), each fixed reactively.

Technical Details

.github/workflows/lstt-formatting.yml:

  • push trigger on develop, same path filter as pull_request, so a whole-tree
    hook runs on the merge commit.
  • --from-ref is event-aware via FROM_REF: github.event.before on push, base
    branch on PR. PR behavior unchanged.
  • cancel-in-progress limited to pull_request, so consecutive merges each keep
    their verdict.

Detection at merge, not prevention. Prevention is "Require branches to be up to
date before merging". develop's ruleset has required_status_checks with
strict_required_status_checks_policy: false, so a PR can merge on a check that
last ran against an older base — which is how the stale-green merge happened.

Known gap: github.event.before is unguarded, so a branch re-create or force-push
gives pre-commit a zero/unreachable SHA and it exits 128. Loud, not silent.
Follow-up can fall back to the base branch on github.event.created/forced.

JIRA ID

N/A (CI hygiene). Drift origin #10010, headers fixed by #8561.

Test Plan / Result

  • pre-commit run --from-ref origin/develop --to-ref HEAD in projects/amdsmi — passes.
  • python3 projects/amdsmi/tests/check_license_headers.py — clean.
  • Parsed the YAML and checked the resolved expressions per event: FROM_REF gives
    github.event.before on push and origin/<base> on PR; cancel-in-progress is
    false on push, true on PR.
  • pre-commit run --from-ref 0000000000000000000000000000000000000000 reproduces
    the exit-128 gap above.

@marifamd
marifamd requested a review from a team August 31, 2026 22:01
@marifamd
marifamd requested a review from a team as a code owner August 31, 2026 22:01
@github-actions github-actions Bot added project: amdsmi github actions Pull requests that update GitHub Actions code labels Aug 31, 2026
@therock-pr-bot

therock-pr-bot Bot commented Aug 31, 2026 •

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

- Run the pre-commit job on pushes to develop, diffing the commit range
  that just landed, so whole-tree hooks report against the merge that
  introduced a violation instead of the next unrelated pull request
- Stop cancelling in-progress runs outside pull requests so back-to-back
  merges each keep their own verdict

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@dmitrii-galantsev
dmitrii-galantsev force-pushed the users/marifamd/spdx-header-drift branch from 6ea7f09 to 20f30c7 Compare September 1, 2026 01:17
@dmitrii-galantsev dmitrii-galantsev changed the title fix(amdsmi): restore SPDX headers and gate format drift on develop ci(lstt): report formatting drift against the merge that introduces it Sep 1, 2026
@dmitrii-galantsev dmitrii-galantsev changed the title ci(lstt): report formatting drift against the merge that introduces it ci(lstt): run formatting on develop pushes, not just PRs Sep 1, 2026
@dmitrii-galantsev

Copy link
Copy Markdown
Contributor

claude: rebase looks right — only the workflow change is left, the headers landed independently via #8561.

Two things from testing it:

  1. FROM_REF on push takes github.event.before unguarded. On a branch create or force-push that is the zero SHA, and pre-commit --from-ref 0000... exits 128 (fatal: Invalid revision range). Narrow for develop and it fails loud, so not a blocker.

  2. Worth knowing before fixing 1: amdsmi-license-headers is pass_filenames: false but has no always_run, so it only fires when the diff range contains a file matching its files: pattern. Falling back to origin/develop on a develop push gives an empty range and silently skips the hook — fail-open, which defeats the purpose. HEAD~1 keeps it firing. Your call on which tradeoff you want; that is why I did not just push a guard.

FWIW amdsmi-manylinux-build.yml already uses the same push-on-develop + per-event cancel-in-progress pattern, so this is consistent with existing practice.

@dmitrii-galantsev

Copy link
Copy Markdown
Contributor

claude: correcting myself — I wrote in the description that develop has no branch protection because the protection API 404s. That was wrong: protection is a ruleset (id 9297053), which the legacy API does not report. Description fixed.

The recommendation stands and is now exact: the ruleset does have required_status_checks, but with strict_required_status_checks_policy: false, so a PR can merge on a check that last ran against an older base. That is the setting to flip.

Two related facts, since they came up: the required contexts are only TheRock CI Summary and HIP NVIDIA CI Summary — the amdsmi LSTT Formatting leg is not required, so header drift reddens PRs without actually gating them. And projects/amdsmi/** needs 2 team approvals.

@marifamd
marifamd merged commit 17e617f into develop Sep 24, 2026
21 of 25 checks passed
@marifamd
marifamd deleted the users/marifamd/spdx-header-drift branch September 24, 2026 14:41
anujshuk-amd pushed a commit that referenced this pull request Sep 25, 2026
## Motivation

`amdsmi-license-headers` is `pass_filenames: false`, so it rescans the
whole tree. Once a violation lands on `develop`, every later PR goes 
red instead of the merge that caused it. #10010 merged on a
stale-green formatting run and reddened #10973 and #10998 until #8561
fixed the headers. Third time against this hook (#10402, #10845,
#8561), each fixed reactively.

## Technical Details

`.github/workflows/lstt-formatting.yml`:

- `push` trigger on `develop`, same path filter as `pull_request`, so a
whole-tree hook runs on the merge commit.
- `--from-ref` is event-aware via `FROM_REF`: `github.event.before` on
push, base branch on PR. PR behavior unchanged.
- `cancel-in-progress` limited to `pull_request`, so consecutive merges
each keep their verdict.

Detection at merge, not prevention. Prevention is "Require branches to
be up to date before merging". `develop`'s ruleset has
`required_status_checks` with 
`strict_required_status_checks_policy: false`, so a PR can merge on a
check that last ran against an older base — which is how the stale-green merge
happened.


Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
dgaliffiAMD pushed a commit that referenced this pull request Sep 25, 2026
## Motivation

`amdsmi-license-headers` is `pass_filenames: false`, so it rescans the
whole tree. Once a violation lands on `develop`, every later PR goes 
red instead of the merge that caused it. #10010 merged on a
stale-green formatting run and reddened #10973 and #10998 until #8561
fixed the headers. Third time against this hook (#10402, #10845,
#8561), each fixed reactively.

## Technical Details

`.github/workflows/lstt-formatting.yml`:

- `push` trigger on `develop`, same path filter as `pull_request`, so a
whole-tree hook runs on the merge commit.
- `--from-ref` is event-aware via `FROM_REF`: `github.event.before` on
push, base branch on PR. PR behavior unchanged.
- `cancel-in-progress` limited to `pull_request`, so consecutive merges
each keep their verdict.

Detection at merge, not prevention. Prevention is "Require branches to
be up to date before merging". `develop`'s ruleset has
`required_status_checks` with 
`strict_required_status_checks_policy: false`, so a PR can merge on a
check that last ran against an older base — which is how the stale-green merge
happened.


Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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

Labels

github actions Pull requests that update GitHub Actions code organization: ROCm project: amdsmi

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants