From 7688e7a5782699e9e043f91961efc164fe5eae3a Mon Sep 17 00:00:00 2001 From: Chuck McAndrew <6248903+dcmcand@users.noreply.github.com> Date: Fri, 11 Sep 2026 15:52:54 +0200 Subject: [PATCH 1/2] fix: only clear project Priority when a priority label is removed 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. --- .github/scripts/test_sync_project_priority.py | 204 ++++++++++++++++++ .github/workflows/sync-project-priority.yaml | 22 +- 2 files changed, 224 insertions(+), 2 deletions(-) create mode 100644 .github/scripts/test_sync_project_priority.py diff --git a/.github/scripts/test_sync_project_priority.py b/.github/scripts/test_sync_project_priority.py new file mode 100644 index 0000000..1d636fb --- /dev/null +++ b/.github/scripts/test_sync_project_priority.py @@ -0,0 +1,204 @@ +"""Tests for the "Apply priority from labels" step of sync-project-priority.yaml. + +The step is an inline bash script, so these tests extract that exact script out +of the workflow YAML and run it under bash with a stub `gh` on PATH. Testing the +shipped text rather than a copy means the tests cannot drift away from what +actually runs in CI. + +Run with: python -m pytest .github/scripts/test_sync_project_priority.py -v +""" +import json +import os +import subprocess +from pathlib import Path + +import pytest +from ruamel.yaml import YAML + +WORKFLOW = ( + Path(__file__).resolve().parents[1] / "workflows" / "sync-project-priority.yaml" +) +STEP_NAME = "Apply priority from labels" + +# Mirrors the Priority single-select options a board is expected to carry. +OPTIONS = [ + {"id": "opt-critical", "name": "Critical"}, + {"id": "opt-high", "name": "High"}, + {"id": "opt-medium", "name": "Medium"}, + {"id": "opt-low", "name": "Low"}, + {"id": "opt-icebox", "name": "Icebox"}, +] + +# Label names carry a trailing emoji in the Nebari repos; the step is expected to +# read the priority out of the "priority: " prefix and ignore the rest. +HIGH = "priority: high \U0001f525" +LOW = "priority: low \U0001f331" +DISCUSSION = "needs: discussion \U0001f4ac" +AREA_CI = "area: ci \U0001f527" + +# A stub `gh` that records every invocation as one JSON line and answers with a +# response shaped like the real GraphQL API, so the step's `--jq` still works. +GH_STUB = """#!/usr/bin/env python3 +import json, os, sys +with open(os.environ["GH_CALLS"], "a") as fh: + fh.write(json.dumps(sys.argv[1:]) + "\\n") +print(json.dumps({"data": {"clearProjectV2ItemFieldValue": {"projectV2Item": {"id": "item"}}, + "updateProjectV2ItemFieldValue": {"projectV2Item": {"id": "item"}}}})) +""" + + +def _step_script(): + yaml = YAML() + with open(WORKFLOW) as fh: + doc = yaml.load(fh) + for step in doc["jobs"]["sync"]["steps"]: + if step.get("name") == STEP_NAME: + return step["run"] + raise AssertionError(f"step {STEP_NAME!r} not found in {WORKFLOW}") + + +def _step_env(): + yaml = YAML() + with open(WORKFLOW) as fh: + doc = yaml.load(fh) + for step in doc["jobs"]["sync"]["steps"]: + if step.get("name") == STEP_NAME: + return dict(step.get("env") or {}) + raise AssertionError(f"step {STEP_NAME!r} not found in {WORKFLOW}") + + +@pytest.fixture +def run_step(tmp_path): + """Run the real step script and report which GraphQL mutations it issued.""" + bindir = tmp_path / "bin" + bindir.mkdir() + gh = bindir / "gh" + gh.write_text(GH_STUB) + gh.chmod(0o755) + + runner_temp = tmp_path / "runner" + runner_temp.mkdir() + calls = tmp_path / "calls.jsonl" + + def _run(labels, *, action, changed_label, is_issue=True, options=OPTIONS): + (runner_temp / "options.json").write_text(json.dumps(options)) + script = tmp_path / "step.sh" + script.write_text(_step_script()) + + env = { + **os.environ, + "PATH": f"{bindir}:{os.environ['PATH']}", + "GH_CALLS": str(calls), + "GH_TOKEN": "stub", + "RUNNER_TEMP": str(runner_temp), + "PROJECT_ID": "PVT_project", + "FIELD_ID": "PVTSSF_field", + "ITEM_ID": "PVTI_item", + "LABELS": json.dumps([{"name": name} for name in labels]), + "IS_ISSUE": "true" if is_issue else "false", + "EVENT_ACTION": action, + "CHANGED_LABEL": changed_label, + } + proc = subprocess.run( + ["bash", str(script)], env=env, capture_output=True, text=True + ) + assert proc.returncode == 0, f"step failed: {proc.stdout}\n{proc.stderr}" + + mutations, option_ids = [], [] + if calls.exists(): + for line in calls.read_text().splitlines(): + argv = json.loads(line) + blob = " ".join(argv) + if "clearProjectV2ItemFieldValue" in blob: + mutations.append("clear") + elif "updateProjectV2ItemFieldValue" in blob: + mutations.append("update") + option_ids += [ + a.split("=", 1)[1] for a in argv if a.startswith("o=") + ] + calls.unlink() + return mutations, option_ids, proc.stdout + + return _run + + +# (case id, labels on the item, event action, label added/removed, expected mutations) +CASES = [ + # An issue that never had a priority label must not be touched, whatever + # unrelated label activity happens on it. This is the reported bug. + ("opened_no_labels", [], "opened", "", []), + ("opened_only_other_labels", [DISCUSSION], "opened", "", []), + ("unrelated_label_added", [AREA_CI], "labeled", AREA_CI, []), + ("unrelated_label_added_beside_others", [DISCUSSION, AREA_CI], "labeled", AREA_CI, []), + ("unrelated_label_removed", [AREA_CI], "unlabeled", DISCUSSION, []), + # Removing the priority label is the one case that may clear the field. + ("priority_label_removed", [DISCUSSION], "unlabeled", HIGH, ["clear"]), + ("last_priority_label_removed_no_labels_left", [], "unlabeled", LOW, ["clear"]), + # Setting still works, and wins over clearing when a priority label remains. + ("priority_label_added", [HIGH], "labeled", HIGH, ["update"]), + ("priority_label_present_on_open", [HIGH, DISCUSSION], "opened", "", ["update"]), + ("one_of_two_priority_labels_removed", [LOW], "unlabeled", HIGH, ["update"]), + ("unrelated_label_added_with_priority_present", [HIGH, AREA_CI], "labeled", AREA_CI, ["update"]), + # Applying several labels at once fires several concurrent runs, each + # carrying its own snapshot of the label set. A run whose snapshot has not + # caught up with a priority label that was just added must not read that as + # "no priority" and clear the field. + ("stale_snapshot_on_priority_label_added", [], "labeled", HIGH, []), + # Only the exact "priority: " prefix counts. + ("lookalike_label_removed", [], "unlabeled", "prioritisation notes", []), + ("prefix_without_separator_removed", [], "unlabeled", "priority:high", []), +] + + +@pytest.mark.parametrize( + "case_id,labels,action,changed_label,expected", + CASES, + ids=[c[0] for c in CASES], +) +def test_mutations_issued(run_step, case_id, labels, action, changed_label, expected): + mutations, _, stdout = run_step(labels, action=action, changed_label=changed_label) + assert mutations == expected, stdout + + +def test_priority_label_maps_to_matching_option(run_step): + _, option_ids, stdout = run_step([HIGH], action="labeled", changed_label=HIGH) + assert option_ids == ["opt-high"], stdout + + +def test_pull_requests_are_never_cleared(run_step): + # clearProjectV2ItemFieldValue only supports issues. + mutations, _, stdout = run_step( + [], action="unlabeled", changed_label=HIGH, is_issue=False + ) + assert mutations == [], stdout + + +def test_board_without_options_issues_no_update(run_step): + # A Priority field with no single-select options cannot be set; the step + # must skip rather than send a mutation with an empty option id. + mutations, _, stdout = run_step( + [HIGH], action="labeled", changed_label=HIGH, options=[] + ) + assert mutations == [], stdout + + +def test_unknown_priority_value_issues_no_mutation(run_step): + mutations, _, stdout = run_step( + ["priority: bogus"], action="labeled", changed_label="priority: bogus" + ) + assert mutations == [], stdout + + +def test_missing_event_context_does_not_clear(run_step, tmp_path): + # If the event carries no label context at all the step must take the safe + # branch, not abort under `set -u` and not clear. + mutations, _, stdout = run_step([], action="", changed_label="") + assert mutations == [], stdout + + +def test_step_env_wires_the_event_context(): + # The shell logic above is only correct if the workflow actually passes the + # event action and the changed label into the step. + env = _step_env() + assert "github.event.action" in str(env.get("EVENT_ACTION", "")) + assert "github.event.label.name" in str(env.get("CHANGED_LABEL", "")) diff --git a/.github/workflows/sync-project-priority.yaml b/.github/workflows/sync-project-priority.yaml index 8f0fb53..201a895 100644 --- a/.github/workflows/sync-project-priority.yaml +++ b/.github/workflows/sync-project-priority.yaml @@ -93,17 +93,35 @@ jobs: ITEM_ID: ${{ steps.add.outputs.item_id }} LABELS: ${{ toJSON(github.event.issue.labels || github.event.pull_request.labels) }} IS_ISSUE: ${{ github.event.issue.node_id != '' && github.event.issue.node_id != null }} + EVENT_ACTION: ${{ github.event.action }} + CHANGED_LABEL: ${{ github.event.label.name }} run: | set -uo pipefail PRIORITY=$(echo "$LABELS" | jq -r '[.[].name | capture("^priority: (?

[a-zA-Z]+)") | .p][0] // ""') if [ -z "$PRIORITY" ]; then + # The Priority field is also set by hand on the board, so "this item + # has no priority label" is not on its own a reason to clear it. + # Only the removal of a priority label is. Without this guard every + # opened/labeled/unlabeled event on an unprioritised item wipes the + # field, including events about labels that have nothing to do with + # priority. + # Defaulted rather than bare: github.event.label is absent on + # "opened", and under `set -u` a bare expansion would abort the step + # instead of taking the safe branch. + case "${EVENT_ACTION:-}/${CHANGED_LABEL:-}" in + "unlabeled/priority: "*) ;; + *) + echo "No priority label, and no priority label was removed; leaving Priority untouched." + exit 0 + ;; + esac # clearProjectV2ItemFieldValue only supports issues, not pull requests. if [ "$IS_ISSUE" != "true" ]; then - echo "No priority label; skipping clear (only issues support clearing fields)." + echo "Priority label removed; skipping clear (only issues support clearing fields)." exit 0 fi - echo "No priority label; clearing field." + echo "Priority label removed and none remain; clearing field." gh api graphql -f query=' mutation($p:ID!,$i:ID!,$f:ID!){ clearProjectV2ItemFieldValue(input:{projectId:$p,itemId:$i,fieldId:$f}){ projectV2Item { id } } From 588f70f0964ae6f4d94a056c548548c3f7745162 Mon Sep 17 00:00:00 2001 From: Chuck McAndrew <6248903+dcmcand@users.noreply.github.com> Date: Mon, 14 Sep 2026 17:33:11 +0200 Subject: [PATCH 2/2] test: pin the priority-label anchor; lint and document the contract 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. --- .github/actionlint.yaml | 9 +++++++++ .github/scripts/test_sync_project_priority.py | 10 ++++++++++ .github/workflows/lint-test.yaml | 15 ++++++--------- .github/workflows/sync-project-priority.yaml | 7 +++++++ README.md | 3 +++ 5 files changed, 35 insertions(+), 9 deletions(-) diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml index faf9f5f..b998776 100644 --- a/.github/actionlint.yaml +++ b/.github/actionlint.yaml @@ -23,3 +23,12 @@ paths: # same actionlint gap, see the comment on the pack-release.yaml entry. - 'property "workflow_repository" is not defined in object type' - 'property "workflow_sha" is not defined in object type' + .github/workflows/sync-project-priority.yaml: + ignore: + # Every GraphQL query in this workflow is single-quoted on purpose: the + # $p/$i/$f/$o inside are GraphQL variable references that must reach the + # API literally, not shell variables. Their values are passed separately + # via `gh api graphql -F/-f`. shellcheck cannot tell the two kinds of `$` + # apart, so each query body trips SC2016. Drop this entry if the queries + # ever stop using GraphQL variables. + - "shellcheck reported issue in this script: SC2016:.+" diff --git a/.github/scripts/test_sync_project_priority.py b/.github/scripts/test_sync_project_priority.py index 1d636fb..923eda4 100644 --- a/.github/scripts/test_sync_project_priority.py +++ b/.github/scripts/test_sync_project_priority.py @@ -35,6 +35,10 @@ LOW = "priority: low \U0001f331" DISCUSSION = "needs: discussion \U0001f4ac" AREA_CI = "area: ci \U0001f527" +# Labels that contain "priority: " but do not start with it. The step anchors +# its match to the start of the label name, so neither is a priority label. +PHRASE = "blocked by priority: high \U0001f525" +HYPHENATED = "de-priority: high \U0001f525" # A stub `gh` that records every invocation as one JSON line and answers with a # response shaped like the real GraphQL API, so the step's `--jq` still works. @@ -147,6 +151,12 @@ def _run(labels, *, action, changed_label, is_issue=True, options=OPTIONS): # Only the exact "priority: " prefix counts. ("lookalike_label_removed", [], "unlabeled", "prioritisation notes", []), ("prefix_without_separator_removed", [], "unlabeled", "priority:high", []), + # ...and it has to be a prefix. A label that merely contains "priority: " + # somewhere inside it is an ordinary label, so it neither sets the field + # nor authorises a clear. + ("priority_text_inside_phrase_added", [PHRASE], "labeled", PHRASE, []), + ("hyphenated_priority_text_added", [HYPHENATED], "labeled", HYPHENATED, []), + ("priority_text_inside_phrase_removed", [], "unlabeled", PHRASE, []), ] diff --git a/.github/workflows/lint-test.yaml b/.github/workflows/lint-test.yaml index 7040ff4..3fef30b 100644 --- a/.github/workflows/lint-test.yaml +++ b/.github/workflows/lint-test.yaml @@ -2,14 +2,10 @@ name: lint-test # CI for this repo's own reusable workflows and helper scripts. # -# Scoped to the workflows introduced/maintained under this effort -# (pack-build-image.yaml, pack-release.yaml, sign-smoke.yaml, and this -# file) rather than the whole .github/workflows/ directory: the other -# pre-existing workflows here (sync-issue-templates.yaml, sync-project-priority.yaml) -# already have unrelated actionlint findings (a floating, EOL -# actions/checkout@v3 pin and a few shellcheck info-level notes) that -# predate this change and are out of scope for it. Widen the file list -# below once those are cleaned up separately. +# An explicit file list rather than the whole .github/workflows/ directory: +# sync-issue-templates.yaml still carries a pre-existing floating, EOL +# actions/checkout@v3 pin that is out of scope here. Add it below once that +# is cleaned up. on: pull_request: @@ -34,7 +30,8 @@ jobs: .github/workflows/pack-build-image.yaml \ .github/workflows/pack-release.yaml \ .github/workflows/lint-test.yaml \ - .github/workflows/sign-smoke.yaml + .github/workflows/sign-smoke.yaml \ + .github/workflows/sync-project-priority.yaml pytest: name: pytest diff --git a/.github/workflows/sync-project-priority.yaml b/.github/workflows/sync-project-priority.yaml index 201a895..d20229b 100644 --- a/.github/workflows/sync-project-priority.yaml +++ b/.github/workflows/sync-project-priority.yaml @@ -3,6 +3,13 @@ name: Sync project priority from labels # Reusable workflow: adds an issue or PR to a Projects v2 board and syncs the # Priority single-select field based on labels of the form "priority: ". # +# The sync is deliberately asymmetric. A "priority: " label sets the +# field on any event, but the field is cleared only when an "unlabeled" event +# removes a "priority: " label and none remains on the item. The field +# is also set by hand on the board, so treating "this item carries no priority +# label" as "clear it" would wipe a manual value every time an unrelated label +# was added or removed. +# # Triggered by caller repos on issues and pull_request_target events # (opened, labeled, unlabeled). The caller passes the project's GraphQL node ID # and a PAT with project: write scope. diff --git a/README.md b/README.md index f11b1cf..9d2ad1a 100644 --- a/README.md +++ b/README.md @@ -38,6 +38,9 @@ Below is a quick list of what you'll find in this repository: > **Note** > For the PR trigger, callers must use `pull_request_target`, **not** `pull_request`. PRs opened from forks do not receive secrets under `pull_request`, so the PAT is empty and the job fails. `pull_request_target` runs in the base repo's context, making the secret available. This is safe because the workflow checks out no PR code — it only reads trusted event metadata and calls the GitHub API. + > **Note** + > Setting and clearing are not symmetric. A `priority: ` label sets the field on any event the caller subscribes to, but the field is cleared only when an `unlabeled` event removes a `priority: ` label and no priority label remains on the item. Every other event leaves the field alone. This matters because the `Priority` field is also set by hand on the board: treating "this item carries no priority label" as "clear it" would wipe a manual value whenever an unrelated label was added or removed. + > **Warning** > The syncing action requires a Personal Authentication Token (PAT) which is currently set up through [Nebari-sensei](https://github.com/nebari-sensei)