Skip to content

fix: only clear project Priority when a priority label is removed - #51

Open
dcmcand wants to merge 2 commits into
mainfrom
fix/priority-sync-no-spurious-clear
Open

dcmcand wants to merge 2 commits into
mainfrom
fix/priority-sync-no-spurious-clear

Conversation

@dcmcand

@dcmcand dcmcand commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

What

sync-project-priority.yaml treats "this item carries no priority: label" as "clear the board's Priority field", on every opened / labeled / unlabeled event. 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 logged No priority label; clearing field. followed by a successful clearProjectV2ItemFieldValue.

Why it started

The clear branch was unreachable dead code until a3cc30f. Before it, PRIORITY=$(... | grep -oE ...) exited 1 on no match and pipefail killed the step before the if [ -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 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. Adding priority: high 🔥 still sets it; removing priority: 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 concurrency key 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-project run 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 the unlabeled run'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.py extracts the step's script out of the workflow YAML and runs it under bash against a stub gh, 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 existing lint-test.yaml pytest job with no new dependencies.

Every branch of the guard is mutation-tested. Deleting the guard outright fails 9 cases. Dropping the unlabeled check fails 3, dropping the priority: prefix check fails 3, and removing the safe branch's exit 0 fails 9. Unwiring either EVENT_ACTION or CHANGED_LABEL from 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 ^ in capture("^priority: ...") was the single piece of the label matching that nothing covered: removing it left the whole suite green, so a label like blocked 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.yaml now 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 / $o are 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.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 callers read. Seven repos consume this workflow, and "a priority label sets the field on any event, but only an unlabeled event 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 a Priority single-select field with zero options, and no item on it holds a Priority value. With no options, OPT never 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-core and nebari-landing both sit on 6b25f52, 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 @main and pick the fix up on merge.

terraform-aws-eks-cluster#24 adds an eighth caller on @main and widens its triggers to [opened, labeled, unlabeled]. This should merge first.

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.
@dcmcand
dcmcand requested a review from viniciusdc September 15, 2026 14:52
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.

1 participant