Repository navigation
Conversation
The "Apply priority from labels" step treated "this item carries no priority: label" as "clear the board's Priority field", and it did so on every opened/labeled/unlabeled event. Because the Priority field is also set by hand on the board, adding or removing any unrelated label (`area: ci`, `needs: discussion`, `type: exploration`) wiped whatever priority a human had set. The clear branch was unreachable dead code until a3cc30f replaced a grep pipeline that was aborting the step under pipefail. That commit was right about the crash, and switched on the destructive branch sitting behind it. Clearing is now gated on the one event that should cause it: an unlabeled event whose removed label is itself a `priority: ` label, with no priority label left on the item. Every other event leaves the field alone. This also defuses a race. Applying several labels at once fires several concurrent runs, each carrying its own snapshot of the label set, so a run for an unrelated label could observe a not-yet-visible priority label and clear the field the sibling run had just set. Such a run now exits without touching anything. Adds .github/scripts/test_sync_project_priority.py, which extracts the step's script out of the workflow YAML and runs it against a stub gh, so the tests exercise the shipped text rather than a copy of it.
The `^` in `capture("^priority: ...")` was the one part of the label
matching that nothing covered: removing it left all 26 tests green, so a
label such as "blocked by priority: high" would silently have started
setting the field. Three cases pin it now, and they cover the guard's own
`priority: ` prefix check at the same time.
sync-project-priority.yaml joins the actionlint job. Its four shellcheck
SC2016 notes are the single-quoted GraphQL query bodies, where $p/$i/$f/$o
are GraphQL variables rather than shell ones, so they get a per-path ignore
in the same shape as the two entries already in actionlint.yaml.
sync-issue-templates.yaml stays out for its floating actions/checkout@v3
pin, and the header note now says only that.
The set/clear asymmetry is stated in the workflow header and in the README
entry that callers actually read. Seven repos consume this workflow, and
"a priority label sets the field on any event, but only an unlabeled event
removing a priority label clears it" is not guessable from the signature.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
sync-project-priority.yamltreats "this item carries nopriority:label" as "clear the board's Priority field", on everyopened/labeled/unlabeledevent. The Priority field is also set by hand on the board, so adding or removing any unrelated label (area: ci,needs: discussion,type: exploration) wipes whatever priority a human had set.Live example, nebari-infrastructure-core run 34585248027: an issue was labelled
needs: discussion 💬/type: exploration 🔮/architecture 🏛, no priority label was involved at any point, and the step loggedNo priority label; clearing field.followed by a successfulclearProjectV2ItemFieldValue.Why it started
The clear branch was unreachable dead code until a3cc30f. Before it,
PRIORITY=$(... | grep -oE ...)exited 1 on no match andpipefailkilled the step before theif [ -z "$PRIORITY" ]branch could run. a3cc30f correctly fixed that crash, and in doing so switched on the destructive branch sitting behind it.The change
Clearing is gated on the one event that should cause it: an
unlabeledevent whose removed label is itself apriority:label, with no priority label left on the item. Every other event leaves the field alone. Addingpriority: high 🔥still sets it; removingpriority: high 🔥still clears it.That also removes an ordering hazard. Applying several labels at once fires several concurrent runs, and nothing serializes them: there is no
concurrencykey in this workflow or in any caller. A run for an unrelated label now exits without issuing a mutation at all, so there is no write of its to land out of order against a sibling's.I went looking for evidence that the event payloads themselves go stale, since that would make the hazard concrete, and did not find it. Across 87 recent
add-to-projectrun logs in nebari-infrastructure-core, 97 pairs of same-issue runs firing within 120 seconds carried identical label snapshots, and the three real same-second priority swaps I found (#398, #463) show theunlabeledrun's payload already containing the newly added priority label. GitHub does not document the ordering either way. So this half is defense against a hazard I can describe but could not demonstrate.Tests
.github/scripts/test_sync_project_priority.pyextracts the step's script out of the workflow YAML and runs it under bash against a stubgh, so the tests exercise the shipped text rather than a copy that can drift from it. 23 cases, 29 tests, covering set, clear, skip, concurrent runs carrying different label snapshots,priority:-lookalike label names, pull requests (which cannot be cleared), a board with no options, and the workflow wiring of the event context into the step env. It runs in the existinglint-test.yamlpytest job with no new dependencies.Every branch of the guard is mutation-tested. Deleting the guard outright fails 9 cases. Dropping the
unlabeledcheck fails 3, dropping thepriority:prefix check fails 3, and removing the safe branch'sexit 0fails 9. Unwiring eitherEVENT_ACTIONorCHANGED_LABELfrom the event fails the env-wiring test and nothing else, which is what that test exists to catch.One gap turned up during that sweep and is fixed here. The
^incapture("^priority: ...")was the single piece of the label matching that nothing covered: removing it left the whole suite green, so a label likeblocked by priority: high 🔥would have started setting the field with no test objecting. Three cases pin it now, and they cover the guard's own prefix check at the same time.Linting and the caller-facing contract
sync-project-priority.yamlnow goes through the actionlint job. It was excluded for shellcheck noise, and the noise is four SC2016 notes on the single-quoted GraphQL query bodies, where$p/$i/$f/$oare GraphQL variables that must reach the API literally rather than shell variables. Those get a per-path ignore in.github/actionlint.yaml, in the same shape as the two entries already there.sync-issue-templates.yamlstays out for its floatingactions/checkout@v3pin, and the header note now says only that.The set/clear asymmetry is stated in the workflow header and in the README entry callers read. Seven repos consume this workflow, and "a priority label sets the field on any event, but only an
unlabeledevent that removes a priority label clears it" is not something a caller can infer from the signature.Notes for reviewers, separate from this PR
The board all seven caller repos point at (
PVT_kwDOBd3CI84BKoKt, Nebari Infrastructure Core) currently has aPrioritysingle-select field with zero options, and no item on it holds a Priority value. With no options,OPTnever resolves and the set path can never fire, so until options are added this workflow can only clear, never set. Adding options named to match the labels (Critical / High / Medium / Low / Icebox) is a board-settings change, not a code change.Two of the seven callers pin this workflow to a commit rather than to
@main:nebari-infrastructure-coreandnebari-landingboth sit on6b25f52, which is the commit that made the clear branch reachable. They keep running the old code after this merges, and the clear is still firing from that pin (run 34838842147, 2026-09-14). Bumping those two pins is follow-up work once this lands. The other five track@mainand pick the fix up on merge.terraform-aws-eks-cluster#24adds an eighth caller on@mainand widens its triggers to[opened, labeled, unlabeled]. This should merge first.