From bd6a129c09953767ad15f6d0aeba8790267623b1 Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Fri, 14 Aug 2026 09:44:50 -0500 Subject: [PATCH 01/13] Add auto-merge workflow for OpenAPI description update PRs --- .../workflows/auto-merge-openapi-updates.yml | 281 ++++++++++++++++++ 1 file changed, 281 insertions(+) create mode 100644 .github/workflows/auto-merge-openapi-updates.yml diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml new file mode 100644 index 0000000000..d8597d536f --- /dev/null +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -0,0 +1,281 @@ +name: Auto-merge OpenAPI description updates + +# Merges the newest open `github-openapi-bot` "Update OpenAPI 3.x Descriptions" +# PRs and closes the older superseded ones. +# +# Why a workflow instead of native auto-merge / the merge API: these PRs carry +# 100K+ line diffs across 64+ files. `PUT /pulls/{n}/merge` returns 502/504 and +# `GET /pulls/{n}/files` returns 422 on them, so the merge is done with plain +# git against a blobless clone. + +on: + schedule: + - cron: '17 */2 * * *' + workflow_dispatch: + inputs: + dry_run: + description: 'Analyze and report, but do not merge or close anything' + type: boolean + default: false + +permissions: + contents: write + pull-requests: write + +concurrency: + group: auto-merge-openapi-updates + cancel-in-progress: false + +env: + BOT_LOGIN: github-openapi-bot + # Status checks that must be green before merging. CodeQL is deliberately + # excluded: default setup only scans the `actions` language, it is not a + # required check on main, and it routinely reports `timed_out` on these PRs. + REQUIRED_CHECKS: 'Lint OpenAPI 3.0 releases,Lint OpenAPI 3.1 releases' + +jobs: + auto-merge: + name: Auto-merge OpenAPI updates + runs-on: ubuntu-latest + outputs: + status: ${{ steps.merge.outputs.status }} + detail: ${{ steps.merge.outputs.detail }} + steps: + - name: Select candidate PRs + id: select + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + run: | + set -euo pipefail + + open_prs=$(gh pr list \ + --state open \ + --author "$BOT_LOGIN" \ + --limit 100 \ + --json number,title,headRefName,headRefOid,createdAt) + + select_newest() { + jq -r --arg t "$1" \ + '[.[] | select(.title == $t)] | sort_by(.createdAt) | last // empty' <<<"$open_prs" + } + + newest_30=$(select_newest 'Update OpenAPI 3.0 Descriptions') + newest_31=$(select_newest 'Update OpenAPI 3.1 Descriptions') + newest_30=${newest_30:-null} + newest_31=${newest_31:-null} + + pr_30=$(jq -r '.number // empty' <<<"$newest_30") + pr_31=$(jq -r '.number // empty' <<<"$newest_31") + + if [ -z "$pr_30" ] && [ -z "$pr_31" ]; then + echo "No open $BOT_LOGIN description PRs. Nothing to do." + echo "found=false" >>"$GITHUB_OUTPUT" + exit 0 + fi + + { + echo "found=true" + echo "pr_30=$pr_30" + echo "pr_31=$pr_31" + echo "ref_30=$(jq -r '.headRefName // empty' <<<"$newest_30")" + echo "ref_31=$(jq -r '.headRefName // empty' <<<"$newest_31")" + } >>"$GITHUB_OUTPUT" + + # Every open bot PR that is not one of the two selected is superseded. + superseded=$(jq -r \ + --argjson keep30 "${pr_30:-0}" \ + --argjson keep31 "${pr_31:-0}" \ + '[.[].number | select(. != $keep30 and . != $keep31)] | join(" ")' \ + <<<"$open_prs") + echo "superseded=$superseded" >>"$GITHUB_OUTPUT" + + echo "3.0 PR: ${pr_30:-none} / 3.1 PR: ${pr_31:-none}" + echo "Superseded: ${superseded:-none}" + + - name: Verify required checks are green + id: checks + if: steps.select.outputs.found == 'true' + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + PR_30: ${{ steps.select.outputs.pr_30 }} + PR_31: ${{ steps.select.outputs.pr_31 }} + run: | + set -euo pipefail + + blocked='' + for pr in $PR_30 $PR_31; do + [ -n "$pr" ] || continue + sha=$(gh pr view "$pr" --json headRefOid -q .headRefOid) + runs=$(gh api "repos/$GH_REPO/commits/$sha/check-runs" --paginate \ + -q '.check_runs[] | "\(.name)\t\(.status)\t\(.conclusion)"') + + IFS=',' read -ra required <<<"$REQUIRED_CHECKS" + for name in "${required[@]}"; do + matches=$(awk -F'\t' -v n="$name" '$1 == n' <<<"$runs") + if [ -z "$matches" ]; then + blocked+="PR #$pr: required check '$name' has not reported. " + continue + fi + if grep -qv $'\tcompleted\tsuccess$' <<<"$matches"; then + blocked+="PR #$pr: check '$name' is not passing. " + fi + done + done + + if [ -n "$blocked" ]; then + echo "status=blocked" >>"$GITHUB_OUTPUT" + echo "detail=$blocked" >>"$GITHUB_OUTPUT" + echo "::notice::$blocked" + else + echo "status=green" >>"$GITHUB_OUTPUT" + fi + + - name: Checkout (blobless) + if: steps.checks.outputs.status == 'green' + uses: actions/checkout@v4 + with: + # Blobless fetch keeps this off the ~4.6 GB full history while still + # allowing real merges. Blobs for the touched files are fetched lazily. + filter: blob:none + fetch-depth: 0 + token: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} + + - name: Scan for breaking changes + id: breaking + if: steps.checks.outputs.status == 'green' + env: + REF_30: ${{ steps.select.outputs.ref_30 }} + REF_31: ${{ steps.select.outputs.ref_31 }} + run: | + set -euo pipefail + + base="origin/${{ github.event.repository.default_branch }}" + + # api.github.com is the non-dereferenced source of truth: it is compact, + # uses $ref, and every other platform file derives from the same change. + findings='' + for pair in \ + "$REF_30:descriptions/api.github.com/api.github.com.yaml" \ + "$REF_31:descriptions-next/api.github.com/api.github.com.yaml"; do + ref="${pair%%:*}"; file="${pair#*:}" + [ -n "$ref" ] || continue + + git fetch --no-tags --filter=blob:none origin "$ref":"refs/remotes/origin/$ref" + diff=$(git diff "$base...origin/$ref" -- "$file" || true) + [ -n "$diff" ] || continue + + removed=$(grep '^-' <<<"$diff" | grep -v '^---' || true) + [ -n "$removed" ] || continue + + count() { grep -cE "$1" <<<"$removed" || true; } + # Removed top-level path key, e.g. ` "/repos/{owner}/{repo}":` + paths=$(count '^- "/') + # Removed enum member, e.g. ` - archived` + enums=$(count '^-[[:space:]]+- [A-Za-z0-9_.-]+$') + # Removed schema definition under components/schemas + schemas=$(count '^- [a-z0-9][a-z0-9-]*:$') + + if [ "${paths:-0}" -gt 0 ]; then findings+="$ref: $paths removed path key(s). "; fi + if [ "${enums:-0}" -gt 0 ]; then findings+="$ref: $enums removed enum value(s). "; fi + if [ "${schemas:-0}" -gt 0 ]; then findings+="$ref: $schemas removed schema key(s). "; fi + done + + if [ -n "$findings" ]; then + echo "status=breaking" >>"$GITHUB_OUTPUT" + echo "detail=$findings" >>"$GITHUB_OUTPUT" + echo "::warning::Potential breaking changes, skipping auto-merge. $findings" + else + echo "status=clean" >>"$GITHUB_OUTPUT" + fi + + - name: Merge and close superseded PRs + id: merge + if: steps.checks.outputs.status == 'green' && steps.breaking.outputs.status == 'clean' + env: + GH_TOKEN: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} + GH_REPO: ${{ github.repository }} + PR_30: ${{ steps.select.outputs.pr_30 }} + PR_31: ${{ steps.select.outputs.pr_31 }} + REF_30: ${{ steps.select.outputs.ref_30 }} + REF_31: ${{ steps.select.outputs.ref_31 }} + SUPERSEDED: ${{ steps.select.outputs.superseded }} + DRY_RUN: ${{ inputs.dry_run }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + run: | + set -euo pipefail + + if [ "$DRY_RUN" = "true" ]; then + echo "Dry run: would merge PRs ${PR_30:-none} and ${PR_31:-none}, close: ${SUPERSEDED:-none}" + echo "status=dry-run" >>"$GITHUB_OUTPUT" + exit 0 + fi + + git config user.name 'github-actions[bot]' + git config user.email '41898282+github-actions[bot]@users.noreply.github.com' + git checkout "$DEFAULT_BRANCH" + + merged='' + # 3.0 first, then 3.1: they touch disjoint trees but this ordering + # matches the manual runbook and keeps history readable. + for pair in "$PR_30:$REF_30" "$PR_31:$REF_31"; do + pr="${pair%%:*}"; ref="${pair#*:}" + [ -n "$pr" ] && [ -n "$ref" ] || continue + git fetch --no-tags origin "$ref":"refs/remotes/origin/$ref" --filter=blob:none + git merge --no-ff "origin/$ref" -m "Merge pull request #$pr from $ref" + merged+="#$pr " + done + + if [ -n "$merged" ]; then + git push origin "$DEFAULT_BRANCH" + echo "Merged and pushed: $merged" + fi + + for pr in $SUPERSEDED; do + gh pr close "$pr" \ + --comment "Superseded by the newer OpenAPI description update(s) ${merged:-just merged}. Closing automatically." \ + || echo "::warning::Failed to close #$pr" + done + + echo "status=merged" >>"$GITHUB_OUTPUT" + echo "detail=Merged $merged" >>"$GITHUB_OUTPUT" + + - name: Notify #api-platform + if: >- + failure() || + steps.breaking.outputs.status == 'breaking' + env: + CHATTERBOX_URL: ${{ secrets.CHATTERBOX_URL }} + CHATTERBOX_TOKEN: ${{ secrets.CHATTERBOX_TOKEN }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + PR_30: ${{ steps.select.outputs.pr_30 }} + PR_31: ${{ steps.select.outputs.pr_31 }} + BREAKING: ${{ steps.breaking.outputs.detail }} + run: | + set -euo pipefail + + if [ -z "${CHATTERBOX_URL:-}" ] || [ -z "${CHATTERBOX_TOKEN:-}" ]; then + echo "Chatterbox not configured; skipping notification." + exit 0 + fi + + if [ -n "${BREAKING:-}" ]; then + headline=":rotating_light: OpenAPI auto-merge skipped: potential breaking changes need a human." + body="• Findings: ${BREAKING}" + else + headline=":warning: OpenAPI auto-merge failed in ${GITHUB_REPOSITORY}." + body="• Needs manual merge" + fi + + message=$(printf '%s\n' \ + "$headline" \ + "• PRs: #${PR_30:-n/a} (3.0), #${PR_31:-n/a} (3.1)" \ + "$body" \ + "• Run: ${RUN_URL}") + + curl --fail --silent --show-error \ + -X POST \ + -u "${CHATTERBOX_TOKEN}:" \ + "${CHATTERBOX_URL%/}/topics/%23api-platform" \ + --data "$message" From 1b17c1806c35cece318c0d870fe58558f94199d4 Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Fri, 14 Aug 2026 10:28:04 -0500 Subject: [PATCH 02/13] Hard-fail preflight for merge token and chatterbox route --- .../workflows/auto-merge-openapi-updates.yml | 87 ++++++++++++++++++- 1 file changed, 85 insertions(+), 2 deletions(-) diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml index d8597d536f..6e86b662ed 100644 --- a/.github/workflows/auto-merge-openapi-updates.yml +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -17,6 +17,10 @@ on: description: 'Analyze and report, but do not merge or close anything' type: boolean default: false + test_notify: + description: 'Send a test post to #api-platform to prove the chatterbox route works' + type: boolean + default: false permissions: contents: write @@ -41,8 +45,87 @@ jobs: status: ${{ steps.merge.outputs.status }} detail: ${{ steps.merge.outputs.detail }} steps: + - name: Preflight - verify credentials + id: preflight + env: + MERGE_TOKEN: ${{ secrets.OPENAPI_MERGE_TOKEN }} + CHATTERBOX_URL: ${{ secrets.CHATTERBOX_URL }} + CHATTERBOX_TOKEN: ${{ secrets.CHATTERBOX_TOKEN }} + GH_REPO: ${{ github.repository }} + TEST_NOTIFY: ${{ inputs.test_notify }} + run: | + set -euo pipefail + + fail='' + + # --- Merge token --------------------------------------------------- + # Falling back to github.token is fine, but a token that is *set* and + # broken (expired PAT, revoked, wrong scopes) must fail loudly here + # rather than as an opaque `git push` rejection after the merge. + if [ -z "${MERGE_TOKEN:-}" ]; then + echo "::warning::OPENAPI_MERGE_TOKEN is not set; falling back to GITHUB_TOKEN. Pushes will not trigger downstream workflows." + else + if ! login=$(GH_TOKEN="$MERGE_TOKEN" gh api user -q '.login' 2>/dev/null); then + # Fine-grained tokens and app installation tokens cannot call + # /user, so only treat this as fatal if the repo probe also fails. + login='(unknown; /user not available for this token type)' + fi + + # `gh api` writes its error body to stdout, so a `|| echo ERROR` + # sentinel would be appended to that body rather than replacing it. + # Branch on the exit status instead. + if perms=$(GH_TOKEN="$MERGE_TOKEN" gh api "repos/$GH_REPO" \ + -q '"\(.permissions.push)\t\(.permissions.admin)"' 2>/dev/null); then + push=${perms%%$'\t'*} + if [ "$push" != 'true' ]; then + fail+="OPENAPI_MERGE_TOKEN cannot push to $GH_REPO (contents:write missing; permissions.push=$push). " + else + echo "Merge token OK. Identity: $login, push: $push" + fi + else + fail+="OPENAPI_MERGE_TOKEN is set but cannot read $GH_REPO (expired, revoked, or lacking repo access). " + fi + fi + + # --- Chatterbox ---------------------------------------------------- + # The notifier is the only signal that a breaking change was skipped, + # so a silently-dead route would make the guard useless. + if [ -z "${CHATTERBOX_URL:-}" ] || [ -z "${CHATTERBOX_TOKEN:-}" ]; then + fail+="CHATTERBOX_URL/CHATTERBOX_TOKEN are not both set; breaking-change alerts would go nowhere. " + else + # curl already writes `000` on connection failure *and* exits + # non-zero, so a `|| echo 000` fallback would concatenate into + # `000000`. Swallow the exit status with `|| true` instead. + code=$(curl --silent --output /dev/null --write-out '%{http_code}' \ + --max-time 20 \ + -u "${CHATTERBOX_TOKEN}:" \ + "${CHATTERBOX_URL%/}/topics/%23api-platform" \ + --data ':white_check_mark: OpenAPI auto-merge preflight: chatterbox route is alive.' \ + || true) + + case "${code:-000}" in + 2*) echo "Chatterbox OK (HTTP $code)." ;; + 000|'') fail+="Chatterbox unreachable (connection failed or timed out). " ;; + 401|403) fail+="Chatterbox rejected CHATTERBOX_TOKEN (HTTP $code). " ;; + *) fail+="Chatterbox returned HTTP $code. " ;; + esac + fi + + if [ -n "$fail" ]; then + echo "::error::Preflight failed: $fail" + exit 1 + fi + + if [ "${TEST_NOTIFY:-false}" = 'true' ]; then + echo "test_notify requested; preflight post sent. Stopping before any merge." + echo "stop=true" >>"$GITHUB_OUTPUT" + fi + + echo "Preflight passed." + - name: Select candidate PRs id: select + if: steps.preflight.outputs.stop != 'true' env: GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} @@ -243,8 +326,8 @@ jobs: - name: Notify #api-platform if: >- - failure() || - steps.breaking.outputs.status == 'breaking' + steps.preflight.outcome == 'success' && + (failure() || steps.breaking.outputs.status == 'breaking') env: CHATTERBOX_URL: ${{ secrets.CHATTERBOX_URL }} CHATTERBOX_TOKEN: ${{ secrets.CHATTERBOX_TOKEN }} From 0453e20e49c4c8d9f5e67146226aca7093a6b341 Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Fri, 14 Aug 2026 10:58:07 -0500 Subject: [PATCH 03/13] Add semantic OpenAPI breaking-change detector --- .github/scripts/openapi-breaking-changes.py | 236 ++++++++++++++++++++ 1 file changed, 236 insertions(+) create mode 100644 .github/scripts/openapi-breaking-changes.py diff --git a/.github/scripts/openapi-breaking-changes.py b/.github/scripts/openapi-breaking-changes.py new file mode 100644 index 0000000000..1eb65f4673 --- /dev/null +++ b/.github/scripts/openapi-breaking-changes.py @@ -0,0 +1,236 @@ +#!/usr/bin/env python3 +"""Detect breaking changes between two OpenAPI description files. + +Implements the breaking-change list from +`github/api-platform/docs/creating-an-openapi-release.md`: + + 1. An operationId name has been changed. + 2. A URL parameter name has been changed. + 3. An operation has been removed from the description. + 4. A `required: true` has been added to the requestBody. + 5. A parameter has been added to the required list for a requestBody. + 6. A field has been removed from a response body. + 7. A field type has changed in a response body. + 8. A field has been removed from the required list in a response body. + +Plus two structural cases that make the above unrepresentable: + 9. A schema definition has been removed from components/schemas. + 10. An enum value has been removed. + +Note the asymmetry that line-based diffing gets wrong: for a *request* body, +*adding* to `required` is breaking; for a *response* body, *removing* from +`required` is breaking. Both are additive/subtractive in opposite directions, +so they cannot be detected by scanning removed diff lines alone. + +Exit status is always 0; findings go to stdout as JSON. The caller decides +policy. +""" + +import json +import sys + +import yaml + +try: + from yaml import CSafeLoader as Loader +except ImportError: # pragma: no cover - libyaml is present on ubuntu-latest + from yaml import SafeLoader as Loader + +METHODS = ('get', 'put', 'post', 'delete', 'patch', 'options', 'head', 'trace') + + +def load(path): + with open(path, encoding='utf-8') as handle: + return yaml.load(handle, Loader=Loader) + + +def operations(doc): + """Map (path, method) -> operation object.""" + out = {} + for path, item in (doc.get('paths') or {}).items(): + if not isinstance(item, dict): + continue + for method, op in item.items(): + if method in METHODS and isinstance(op, dict): + out[(path, method)] = op + return out + + +def path_params(op): + return { + p.get('name') + for p in (op.get('parameters') or []) + if isinstance(p, dict) and p.get('in') == 'path' + } + + +def request_body(op): + body = op.get('requestBody') + return body if isinstance(body, dict) else {} + + +def json_schema(container): + """Pull the application/json schema out of a requestBody/response.""" + content = container.get('content') + if not isinstance(content, dict): + return {} + for media, spec in content.items(): + if 'json' in media and isinstance(spec, dict): + schema = spec.get('schema') + return schema if isinstance(schema, dict) else {} + return {} + + +def enum_values(schema): + values = schema.get('enum') + return set(map(str, values)) if isinstance(values, list) else set() + + +def compare_operations(base, head, findings): + base_ops, head_ops = operations(base), operations(head) + + for key, op in base_ops.items(): + path, method = key + label = f'{method.upper()} {path}' + + if key not in head_ops: + findings.append({ + 'rule': 'operation-removed', + 'detail': f'{label} was removed', + }) + continue + + new = head_ops[key] + + old_id, new_id = op.get('operationId'), new.get('operationId') + if old_id and new_id and old_id != new_id: + findings.append({ + 'rule': 'operationid-changed', + 'detail': f'{label}: operationId {old_id!r} -> {new_id!r}', + }) + + removed_params = path_params(op) - path_params(new) + if removed_params: + findings.append({ + 'rule': 'url-parameter-renamed', + 'detail': f'{label}: path parameter(s) gone: {sorted(removed_params)}', + }) + + old_body, new_body = request_body(op), request_body(new) + if new_body.get('required') and not old_body.get('required'): + findings.append({ + 'rule': 'requestbody-now-required', + 'detail': f'{label}: requestBody became required', + }) + + added_required = set(json_schema(new_body).get('required') or []) - \ + set(json_schema(old_body).get('required') or []) + if added_required: + findings.append({ + 'rule': 'requestbody-required-added', + 'detail': f'{label}: new required request field(s): {sorted(added_required)}', + }) + + +def compare_schemas(base, head, findings): + """Compare components/schemas. + + Responses overwhelmingly `$ref` into components/schemas in the + non-dereferenced description, so comparing schemas covers "field removed + from a response body" and "field type changed" without resolving refs. + """ + base_schemas = (base.get('components') or {}).get('schemas') or {} + head_schemas = (head.get('components') or {}).get('schemas') or {} + + for name, old in base_schemas.items(): + if not isinstance(old, dict): + continue + + new = head_schemas.get(name) + if new is None: + findings.append({ + 'rule': 'schema-removed', + 'detail': f'schema {name!r} was removed', + }) + continue + if not isinstance(new, dict): + continue + + old_props = old.get('properties') or {} + new_props = new.get('properties') or {} + + for prop in set(old_props) - set(new_props): + findings.append({ + 'rule': 'response-field-removed', + 'detail': f'{name}.{prop} was removed', + }) + + for prop in set(old_props) & set(new_props): + old_p, new_p = old_props[prop], new_props[prop] + if not isinstance(old_p, dict) or not isinstance(new_p, dict): + continue + + old_t, new_t = old_p.get('type'), new_p.get('type') + if old_t and new_t and old_t != new_t: + findings.append({ + 'rule': 'response-field-type-changed', + 'detail': f'{name}.{prop}: type {old_t!r} -> {new_t!r}', + }) + + dropped = enum_values(old_p) - enum_values(new_p) + if dropped: + findings.append({ + 'rule': 'enum-value-removed', + 'detail': f'{name}.{prop}: enum value(s) removed: {sorted(dropped)}', + }) + + # For a response body, losing a guarantee is the breaking direction. + dropped_required = set(old.get('required') or []) - set(new.get('required') or []) + if dropped_required: + findings.append({ + 'rule': 'response-required-removed', + 'detail': f'{name}: no longer guaranteed: {sorted(dropped_required)}', + }) + + dropped = enum_values(old) - enum_values(new) + if dropped: + findings.append({ + 'rule': 'enum-value-removed', + 'detail': f'schema {name}: enum value(s) removed: {sorted(dropped)}', + }) + + +def summarize(base, head): + """Non-breaking additions, used for the informational PR summary.""" + base_ops, head_ops = operations(base), operations(head) + added = [f'{m.upper()} {p}' for (p, m) in set(head_ops) - set(base_ops)] + + base_schemas = set((base.get('components') or {}).get('schemas') or {}) + head_schemas = set((head.get('components') or {}).get('schemas') or {}) + + return { + 'added_operations': sorted(added), + 'added_schemas': sorted(head_schemas - base_schemas), + 'total_operations': len(head_ops), + 'total_schemas': len(head_schemas), + } + + +def main(): + if len(sys.argv) != 3: + print('usage: openapi-breaking-changes.py BASE.yaml HEAD.yaml', file=sys.stderr) + return 2 + + base, head = load(sys.argv[1]), load(sys.argv[2]) + + findings = [] + compare_operations(base, head, findings) + compare_schemas(base, head, findings) + + json.dump({'findings': findings, 'summary': summarize(base, head)}, sys.stdout, indent=2) + print() + return 0 + + +if __name__ == '__main__': + sys.exit(main()) From 2c3eca70514e43f3fa76f7ba7156246077ce55a5 Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Fri, 14 Aug 2026 10:58:13 -0500 Subject: [PATCH 04/13] Use semantic breaking-change scan and post change summary to PRs --- .../workflows/auto-merge-openapi-updates.yml | 118 ++++++++++++++---- 1 file changed, 94 insertions(+), 24 deletions(-) diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml index 6e86b662ed..f62035ff99 100644 --- a/.github/workflows/auto-merge-openapi-updates.yml +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -7,6 +7,13 @@ name: Auto-merge OpenAPI description updates # 100K+ line diffs across 64+ files. `PUT /pulls/{n}/merge` returns 502/504 and # `GET /pulls/{n}/files` returns 422 on them, so the merge is done with plain # git against a blobless clone. +# +# This automates step 6 of the API Platform first-responder runbook +# (`openapi-pr-reviewer`). The runbook runs it on Tuesdays and Thursdays; this +# runs continuously, so the PR backlog never accumulates. The safety properties +# the FR provides by hand are preserved: lint must be green, a semantic +# breaking-change scan must come back clean, and a change summary is posted to +# the PR before merge for release-note authoring. on: schedule: @@ -225,46 +232,78 @@ jobs: fetch-depth: 0 token: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} + - name: Set up Python + if: steps.checks.outputs.status == 'green' + uses: actions/setup-python@v5 + with: + python-version: '3.12' + + - name: Install PyYAML + if: steps.checks.outputs.status == 'green' + # Reuses the repo's existing pinned/hash-verified requirements, which + # already provide pyyaml for the linter workflow. + run: pip install --require-hashes -r requirements.txt + - name: Scan for breaking changes id: breaking if: steps.checks.outputs.status == 'green' env: REF_30: ${{ steps.select.outputs.ref_30 }} REF_31: ${{ steps.select.outputs.ref_31 }} + PR_30: ${{ steps.select.outputs.pr_30 }} + PR_31: ${{ steps.select.outputs.pr_31 }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} run: | set -euo pipefail - base="origin/${{ github.event.repository.default_branch }}" - - # api.github.com is the non-dereferenced source of truth: it is compact, - # uses $ref, and every other platform file derives from the same change. + # A semantic (parsed) comparison rather than a diff scan. The two are + # not equivalent: for a *request* body, ADDING to `required` is + # breaking, while for a *response* body, REMOVING from `required` is + # breaking. A scan of removed diff lines is structurally blind to the + # first case. Rules mirror the breaking-change list in + # github/api-platform docs/creating-an-openapi-release.md. findings='' + : >summary.md + for pair in \ - "$REF_30:descriptions/api.github.com/api.github.com.yaml" \ - "$REF_31:descriptions-next/api.github.com/api.github.com.yaml"; do - ref="${pair%%:*}"; file="${pair#*:}" + "$REF_30|descriptions/api.github.com/api.github.com.yaml|3.0|$PR_30" \ + "$REF_31|descriptions-next/api.github.com/api.github.com.yaml|3.1|$PR_31"; do + IFS='|' read -r ref file label pr <<<"$pair" [ -n "$ref" ] || continue git fetch --no-tags --filter=blob:none origin "$ref":"refs/remotes/origin/$ref" - diff=$(git diff "$base...origin/$ref" -- "$file" || true) - [ -n "$diff" ] || continue - - removed=$(grep '^-' <<<"$diff" | grep -v '^---' || true) - [ -n "$removed" ] || continue - - count() { grep -cE "$1" <<<"$removed" || true; } - # Removed top-level path key, e.g. ` "/repos/{owner}/{repo}":` - paths=$(count '^- "/') - # Removed enum member, e.g. ` - archived` - enums=$(count '^-[[:space:]]+- [A-Za-z0-9_.-]+$') - # Removed schema definition under components/schemas - schemas=$(count '^- [a-z0-9][a-z0-9-]*:$') - - if [ "${paths:-0}" -gt 0 ]; then findings+="$ref: $paths removed path key(s). "; fi - if [ "${enums:-0}" -gt 0 ]; then findings+="$ref: $enums removed enum value(s). "; fi - if [ "${schemas:-0}" -gt 0 ]; then findings+="$ref: $schemas removed schema key(s). "; fi + + # api.github.com is the non-dereferenced source of truth: compact, + # $ref-based, and every platform variant derives from it. + git show "origin/$DEFAULT_BRANCH:$file" >base.yaml + git show "origin/$ref:$file" >head.yaml + + python3 .github/scripts/openapi-breaking-changes.py base.yaml head.yaml >result.json + + count=$(jq '.findings | length' result.json) + { + echo "### OpenAPI $label (#$pr)" + echo + jq -r '.summary | + "- Operations: \(.total_operations) total, \(.added_operations | length) added", + "- Schemas: \(.total_schemas) total, \(.added_schemas | length) added"' result.json + jq -r '.summary.added_operations[]? | " - `\(.)`"' result.json | head -40 + echo + } >>summary.md + + if [ "$count" -gt 0 ]; then + findings+="$label (#$pr): $count breaking finding(s). " + { + echo "#### :rotating_light: Breaking changes in $label" + echo + jq -r '.findings[] | "- **\(.rule)**: \(.detail)"' result.json | head -50 + echo + } >>summary.md + fi done + cat summary.md + if [ -n "$findings" ]; then echo "status=breaking" >>"$GITHUB_OUTPUT" echo "detail=$findings" >>"$GITHUB_OUTPUT" @@ -273,6 +312,37 @@ jobs: echo "status=clean" >>"$GITHUB_OUTPUT" fi + - name: Post change summary to PRs + if: steps.checks.outputs.status == 'green' + env: + GH_TOKEN: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} + GH_REPO: ${{ github.repository }} + PR_30: ${{ steps.select.outputs.pr_30 }} + PR_31: ${{ steps.select.outputs.pr_31 }} + BREAKING: ${{ steps.breaking.outputs.status }} + run: | + set -euo pipefail + + # Release notes are generated from PR descriptions, and the runbook + # asks for a human-written change summary before merge. Auto-merging + # untouched bodies would delete that input, so post the analysis the + # scanner already computed. + { + if [ "$BREAKING" = 'breaking' ]; then + echo "## :rotating_light: Auto-merge skipped: potential breaking changes" + else + echo "## Automated change summary" + fi + echo + cat summary.md + echo "_Posted by the auto-merge workflow ([run](${GITHUB_SERVER_URL}/${GH_REPO}/actions/runs/${GITHUB_RUN_ID}))._" + } >comment.md + + for pr in $PR_30 $PR_31; do + [ -n "$pr" ] || continue + gh pr comment "$pr" --body-file comment.md || echo "::warning::Could not comment on #$pr" + done + - name: Merge and close superseded PRs id: merge if: steps.checks.outputs.status == 'green' && steps.breaking.outputs.status == 'clean' From 46ef8afc29ffcccb9d29033b99ed0e1b9691de5e Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Fri, 14 Aug 2026 11:43:48 -0500 Subject: [PATCH 05/13] Hold merges during GHES release-candidate freeze windows --- .../workflows/auto-merge-openapi-updates.yml | 131 ++++++++++++++++-- 1 file changed, 123 insertions(+), 8 deletions(-) diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml index f62035ff99..8144a7d6d8 100644 --- a/.github/workflows/auto-merge-openapi-updates.yml +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -14,6 +14,11 @@ name: Auto-merge OpenAPI description updates # the FR provides by hand are preserved: lint must be green, a semantic # breaking-change scan must come back clean, and a change summary is posted to # the PR before merge for release-note authoring. +# +# Merges are additionally held during a GHES release-candidate window, when +# Docs asks the team to stop merging description PRs until the corresponding +# RC PR lands in github/docs-internal. See the "Check for an active merge +# freeze" step for how that is detected and how it clears. on: schedule: @@ -222,8 +227,76 @@ jobs: echo "status=green" >>"$GITHUB_OUTPUT" fi + - name: Check for an active merge freeze + id: freeze + if: steps.select.outputs.found == 'true' + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + REF_30: ${{ steps.select.outputs.ref_30 }} + REF_31: ${{ steps.select.outputs.ref_31 }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + run: | + set -euo pipefail + + # Around a GHES release candidate, Docs asks the team to hold off + # merging description PRs. The sequence is: + # 1. github/github sets `published: true` for the new GHES version + # 2. Docs asks #api-platform to hold merges + # 3. Docs merges the RC PR in github/docs-internal + # 4. Docs gives the all-clear and merges resume + # + # Step 1 is what makes the new GHES version's descriptions appear in + # the bot's PR here, so "this PR introduces a GHES version that does + # not exist on the default branch" is a reliable proxy for being + # inside that window. It is also self-clearing: once the version is + # merged the directory exists on main and later PRs stop matching. + # + # The window is genuinely short (for 3.21, descriptions landed 18 + # minutes before the docs RC merged), so this must be checked per-run + # rather than assumed stale. + + reason='' + + # --- Signal 1: an explicit, human-controlled hold ---------------- + # A label is the escape hatch that does not depend on inference, and + # lets Docs or the FR stop merges for any reason at all. + if gh label list --search 'merge-freeze' --json name -q '.[].name' | grep -qx 'merge-freeze'; then + frozen=$(gh issue list --label 'merge-freeze' --state open --json number,title \ + -q '.[] | "#\(.number) \(.title)"' | head -5) + if [ -n "$frozen" ]; then + reason+="Open 'merge-freeze' issue(s): $(tr '\n' ';' <<<"$frozen") " + fi + fi + + # --- Signal 2: a GHES version not yet on the default branch ------ + ghes_on_main=$(gh api "repos/$GH_REPO/contents/descriptions?ref=$DEFAULT_BRANCH" \ + -q '.[].name' | grep '^ghes-' | sort -u) + + for ref in "$REF_30" "$REF_31"; do + [ -n "$ref" ] || continue + for dir in descriptions descriptions-next; do + on_pr=$(gh api "repos/$GH_REPO/contents/$dir?ref=$ref" \ + -q '.[].name' 2>/dev/null | grep '^ghes-' | sort -u || true) + [ -n "$on_pr" ] || continue + new=$(comm -13 <(echo "$ghes_on_main") <(echo "$on_pr") | tr '\n' ' ') + if [ -n "${new// /}" ]; then + reason+="$ref/$dir introduces new GHES version(s): $new " + fi + done + done + + if [ -n "$reason" ]; then + echo "status=frozen" >>"$GITHUB_OUTPUT" + echo "detail=$reason" >>"$GITHUB_OUTPUT" + echo "::warning::Merge freeze in effect, holding. $reason" + else + echo "status=clear" >>"$GITHUB_OUTPUT" + echo "No merge freeze detected." + fi + - name: Checkout (blobless) - if: steps.checks.outputs.status == 'green' + if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' uses: actions/checkout@v4 with: # Blobless fetch keeps this off the ~4.6 GB full history while still @@ -233,20 +306,20 @@ jobs: token: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} - name: Set up Python - if: steps.checks.outputs.status == 'green' + if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' uses: actions/setup-python@v5 with: python-version: '3.12' - name: Install PyYAML - if: steps.checks.outputs.status == 'green' + if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' # Reuses the repo's existing pinned/hash-verified requirements, which # already provide pyyaml for the linter workflow. run: pip install --require-hashes -r requirements.txt - name: Scan for breaking changes id: breaking - if: steps.checks.outputs.status == 'green' + if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' env: REF_30: ${{ steps.select.outputs.ref_30 }} REF_31: ${{ steps.select.outputs.ref_31 }} @@ -313,7 +386,7 @@ jobs: fi - name: Post change summary to PRs - if: steps.checks.outputs.status == 'green' + if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' env: GH_TOKEN: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} GH_REPO: ${{ github.repository }} @@ -345,7 +418,10 @@ jobs: - name: Merge and close superseded PRs id: merge - if: steps.checks.outputs.status == 'green' && steps.breaking.outputs.status == 'clean' + if: >- + steps.checks.outputs.status == 'green' && + steps.freeze.outputs.status == 'clear' && + steps.breaking.outputs.status == 'clean' env: GH_TOKEN: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} GH_REPO: ${{ github.repository }} @@ -397,7 +473,9 @@ jobs: - name: Notify #api-platform if: >- steps.preflight.outcome == 'success' && - (failure() || steps.breaking.outputs.status == 'breaking') + (failure() || + steps.breaking.outputs.status == 'breaking' || + steps.freeze.outputs.status == 'frozen') env: CHATTERBOX_URL: ${{ secrets.CHATTERBOX_URL }} CHATTERBOX_TOKEN: ${{ secrets.CHATTERBOX_TOKEN }} @@ -405,6 +483,9 @@ jobs: PR_30: ${{ steps.select.outputs.pr_30 }} PR_31: ${{ steps.select.outputs.pr_31 }} BREAKING: ${{ steps.breaking.outputs.detail }} + FREEZE: ${{ steps.freeze.outputs.detail }} + GH_TOKEN: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} + GH_REPO: ${{ github.repository }} run: | set -euo pipefail @@ -413,7 +494,41 @@ jobs: exit 0 fi - if [ -n "${BREAKING:-}" ]; then + if [ -n "${FREEZE:-}" ]; then + # A freeze is an expected state, not a fault. It is re-detected + # every 2 hours for the whole GHES RC window, so announce it once + # per PR rather than pinging the channel dozens of times. The PR + # comment is the durable marker; a workspace file would not survive + # between runs. + already='' + for pr in $PR_30 $PR_31; do + [ -n "$pr" ] || continue + if gh pr view "$pr" --json comments \ + -q '.comments[].body' 2>/dev/null | grep -q 'AUTO-MERGE-FREEZE-NOTICE'; then + already='yes' + fi + done + + if [ -n "$already" ]; then + echo "Freeze already announced for these PRs; not re-posting." + exit 0 + fi + + for pr in $PR_30 $PR_31; do + [ -n "$pr" ] || continue + gh pr comment "$pr" --body "$(printf '%s\n' \ + '## :snowflake: Auto-merge is holding (merge freeze)' \ + '' \ + "Reason: ${FREEZE}" \ + '' \ + 'This is expected around a GHES release candidate, while Docs merges the corresponding RC PR in `github/docs-internal`. Merges resume automatically once the new GHES version is present on the default branch and any open `merge-freeze` issue is closed. No action needed unless this persists past the all-clear.' \ + '' \ + '')" || true + done + + headline=":snowflake: OpenAPI auto-merge is holding: merge freeze detected." + body="• Reason: ${FREEZE}"$'\n'"• Merges resume automatically once the new GHES version is on the default branch and any 'merge-freeze' issue is closed." + elif [ -n "${BREAKING:-}" ]; then headline=":rotating_light: OpenAPI auto-merge skipped: potential breaking changes need a human." body="• Findings: ${BREAKING}" else From 0d9bf49fac80eb56b7522123445177c0d67820c5 Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Fri, 14 Aug 2026 15:33:49 -0500 Subject: [PATCH 06/13] Keep public workflow free of private integrations --- .../workflows/auto-merge-openapi-updates.yml | 223 +++--------------- 1 file changed, 31 insertions(+), 192 deletions(-) diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml index 8144a7d6d8..7dafc61577 100644 --- a/.github/workflows/auto-merge-openapi-updates.yml +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -8,17 +8,12 @@ name: Auto-merge OpenAPI description updates # `GET /pulls/{n}/files` returns 422 on them, so the merge is done with plain # git against a blobless clone. # -# This automates step 6 of the API Platform first-responder runbook -# (`openapi-pr-reviewer`). The runbook runs it on Tuesdays and Thursdays; this -# runs continuously, so the PR backlog never accumulates. The safety properties -# the FR provides by hand are preserved: lint must be green, a semantic -# breaking-change scan must come back clean, and a change summary is posted to -# the PR before merge for release-note authoring. +# This runs continuously. The safety properties of the manual process are +# preserved: lint must be green, the semantic breaking-change scan must come +# back clean, and a change summary is posted to the PR before merge. # -# Merges are additionally held during a GHES release-candidate window, when -# Docs asks the team to stop merging description PRs until the corresponding -# RC PR lands in github/docs-internal. See the "Check for an active merge -# freeze" step for how that is detected and how it clears. +# Merges are additionally held during a GHES release-candidate window. See the +# "Check for an active merge freeze" step for how that is detected. on: schedule: @@ -29,10 +24,6 @@ on: description: 'Analyze and report, but do not merge or close anything' type: boolean default: false - test_notify: - description: 'Send a test post to #api-platform to prove the chatterbox route works' - type: boolean - default: false permissions: contents: write @@ -57,87 +48,8 @@ jobs: status: ${{ steps.merge.outputs.status }} detail: ${{ steps.merge.outputs.detail }} steps: - - name: Preflight - verify credentials - id: preflight - env: - MERGE_TOKEN: ${{ secrets.OPENAPI_MERGE_TOKEN }} - CHATTERBOX_URL: ${{ secrets.CHATTERBOX_URL }} - CHATTERBOX_TOKEN: ${{ secrets.CHATTERBOX_TOKEN }} - GH_REPO: ${{ github.repository }} - TEST_NOTIFY: ${{ inputs.test_notify }} - run: | - set -euo pipefail - - fail='' - - # --- Merge token --------------------------------------------------- - # Falling back to github.token is fine, but a token that is *set* and - # broken (expired PAT, revoked, wrong scopes) must fail loudly here - # rather than as an opaque `git push` rejection after the merge. - if [ -z "${MERGE_TOKEN:-}" ]; then - echo "::warning::OPENAPI_MERGE_TOKEN is not set; falling back to GITHUB_TOKEN. Pushes will not trigger downstream workflows." - else - if ! login=$(GH_TOKEN="$MERGE_TOKEN" gh api user -q '.login' 2>/dev/null); then - # Fine-grained tokens and app installation tokens cannot call - # /user, so only treat this as fatal if the repo probe also fails. - login='(unknown; /user not available for this token type)' - fi - - # `gh api` writes its error body to stdout, so a `|| echo ERROR` - # sentinel would be appended to that body rather than replacing it. - # Branch on the exit status instead. - if perms=$(GH_TOKEN="$MERGE_TOKEN" gh api "repos/$GH_REPO" \ - -q '"\(.permissions.push)\t\(.permissions.admin)"' 2>/dev/null); then - push=${perms%%$'\t'*} - if [ "$push" != 'true' ]; then - fail+="OPENAPI_MERGE_TOKEN cannot push to $GH_REPO (contents:write missing; permissions.push=$push). " - else - echo "Merge token OK. Identity: $login, push: $push" - fi - else - fail+="OPENAPI_MERGE_TOKEN is set but cannot read $GH_REPO (expired, revoked, or lacking repo access). " - fi - fi - - # --- Chatterbox ---------------------------------------------------- - # The notifier is the only signal that a breaking change was skipped, - # so a silently-dead route would make the guard useless. - if [ -z "${CHATTERBOX_URL:-}" ] || [ -z "${CHATTERBOX_TOKEN:-}" ]; then - fail+="CHATTERBOX_URL/CHATTERBOX_TOKEN are not both set; breaking-change alerts would go nowhere. " - else - # curl already writes `000` on connection failure *and* exits - # non-zero, so a `|| echo 000` fallback would concatenate into - # `000000`. Swallow the exit status with `|| true` instead. - code=$(curl --silent --output /dev/null --write-out '%{http_code}' \ - --max-time 20 \ - -u "${CHATTERBOX_TOKEN}:" \ - "${CHATTERBOX_URL%/}/topics/%23api-platform" \ - --data ':white_check_mark: OpenAPI auto-merge preflight: chatterbox route is alive.' \ - || true) - - case "${code:-000}" in - 2*) echo "Chatterbox OK (HTTP $code)." ;; - 000|'') fail+="Chatterbox unreachable (connection failed or timed out). " ;; - 401|403) fail+="Chatterbox rejected CHATTERBOX_TOKEN (HTTP $code). " ;; - *) fail+="Chatterbox returned HTTP $code. " ;; - esac - fi - - if [ -n "$fail" ]; then - echo "::error::Preflight failed: $fail" - exit 1 - fi - - if [ "${TEST_NOTIFY:-false}" = 'true' ]; then - echo "test_notify requested; preflight post sent. Stopping before any merge." - echo "stop=true" >>"$GITHUB_OUTPUT" - fi - - echo "Preflight passed." - - name: Select candidate PRs id: select - if: steps.preflight.outputs.stop != 'true' env: GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} @@ -239,22 +151,10 @@ jobs: run: | set -euo pipefail - # Around a GHES release candidate, Docs asks the team to hold off - # merging description PRs. The sequence is: - # 1. github/github sets `published: true` for the new GHES version - # 2. Docs asks #api-platform to hold merges - # 3. Docs merges the RC PR in github/docs-internal - # 4. Docs gives the all-clear and merges resume - # - # Step 1 is what makes the new GHES version's descriptions appear in - # the bot's PR here, so "this PR introduces a GHES version that does - # not exist on the default branch" is a reliable proxy for being - # inside that window. It is also self-clearing: once the version is - # merged the directory exists on main and later PRs stop matching. - # - # The window is genuinely short (for 3.21, descriptions landed 18 - # minutes before the docs RC merged), so this must be checked per-run - # rather than assumed stale. + # Around a GHES release candidate, hold description merges until the + # coordinated release content is ready. A new GHES version appearing + # in the candidate PR before it exists on the default branch is the + # public signal that this window may be active. reason='' @@ -303,7 +203,7 @@ jobs: # allowing real merges. Blobs for the touched files are fetched lazily. filter: blob:none fetch-depth: 0 - token: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} + token: ${{ github.token }} - name: Set up Python if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' @@ -334,7 +234,7 @@ jobs: # breaking, while for a *response* body, REMOVING from `required` is # breaking. A scan of removed diff lines is structurally blind to the # first case. Rules mirror the breaking-change list in - # github/api-platform docs/creating-an-openapi-release.md. + # the project's documented release-compatibility rules. findings='' : >summary.md @@ -388,7 +288,7 @@ jobs: - name: Post change summary to PRs if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' env: - GH_TOKEN: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} + GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} PR_30: ${{ steps.select.outputs.pr_30 }} PR_31: ${{ steps.select.outputs.pr_31 }} @@ -396,10 +296,9 @@ jobs: run: | set -euo pipefail - # Release notes are generated from PR descriptions, and the runbook - # asks for a human-written change summary before merge. Auto-merging - # untouched bodies would delete that input, so post the analysis the - # scanner already computed. + # Release notes are generated from PR descriptions. Auto-merging + # untouched bodies would remove useful context, so post the analysis + # the scanner already computed. { if [ "$BREAKING" = 'breaking' ]; then echo "## :rotating_light: Auto-merge skipped: potential breaking changes" @@ -423,7 +322,7 @@ jobs: steps.freeze.outputs.status == 'clear' && steps.breaking.outputs.status == 'clean' env: - GH_TOKEN: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} + GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} PR_30: ${{ steps.select.outputs.pr_30 }} PR_31: ${{ steps.select.outputs.pr_31 }} @@ -447,7 +346,7 @@ jobs: merged='' # 3.0 first, then 3.1: they touch disjoint trees but this ordering - # matches the manual runbook and keeps history readable. + # keeps the generated merge history readable. for pair in "$PR_30:$REF_30" "$PR_31:$REF_31"; do pr="${pair%%:*}"; ref="${pair#*:}" [ -n "$pr" ] && [ -n "$ref" ] || continue @@ -470,80 +369,20 @@ jobs: echo "status=merged" >>"$GITHUB_OUTPUT" echo "detail=Merged $merged" >>"$GITHUB_OUTPUT" - - name: Notify #api-platform + - name: Summarize exceptions if: >- - steps.preflight.outcome == 'success' && - (failure() || - steps.breaking.outputs.status == 'breaking' || - steps.freeze.outputs.status == 'frozen') - env: - CHATTERBOX_URL: ${{ secrets.CHATTERBOX_URL }} - CHATTERBOX_TOKEN: ${{ secrets.CHATTERBOX_TOKEN }} - RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} - PR_30: ${{ steps.select.outputs.pr_30 }} - PR_31: ${{ steps.select.outputs.pr_31 }} - BREAKING: ${{ steps.breaking.outputs.detail }} - FREEZE: ${{ steps.freeze.outputs.detail }} - GH_TOKEN: ${{ secrets.OPENAPI_MERGE_TOKEN || github.token }} - GH_REPO: ${{ github.repository }} + failure() || + steps.breaking.outputs.status == 'breaking' || + steps.freeze.outputs.status == 'frozen' run: | - set -euo pipefail - - if [ -z "${CHATTERBOX_URL:-}" ] || [ -z "${CHATTERBOX_TOKEN:-}" ]; then - echo "Chatterbox not configured; skipping notification." - exit 0 - fi - - if [ -n "${FREEZE:-}" ]; then - # A freeze is an expected state, not a fault. It is re-detected - # every 2 hours for the whole GHES RC window, so announce it once - # per PR rather than pinging the channel dozens of times. The PR - # comment is the durable marker; a workspace file would not survive - # between runs. - already='' - for pr in $PR_30 $PR_31; do - [ -n "$pr" ] || continue - if gh pr view "$pr" --json comments \ - -q '.comments[].body' 2>/dev/null | grep -q 'AUTO-MERGE-FREEZE-NOTICE'; then - already='yes' - fi - done - - if [ -n "$already" ]; then - echo "Freeze already announced for these PRs; not re-posting." - exit 0 + { + echo "## Auto-merge did not proceed" + echo + if [ "${{ steps.freeze.outputs.status }}" = "frozen" ]; then + echo "A merge freeze was detected: ${{ steps.freeze.outputs.detail }}" + elif [ "${{ steps.breaking.outputs.status }}" = "breaking" ]; then + echo "Potential breaking changes were detected: ${{ steps.breaking.outputs.detail }}" + else + echo "The workflow failed before merging. Review the failed step above." fi - - for pr in $PR_30 $PR_31; do - [ -n "$pr" ] || continue - gh pr comment "$pr" --body "$(printf '%s\n' \ - '## :snowflake: Auto-merge is holding (merge freeze)' \ - '' \ - "Reason: ${FREEZE}" \ - '' \ - 'This is expected around a GHES release candidate, while Docs merges the corresponding RC PR in `github/docs-internal`. Merges resume automatically once the new GHES version is present on the default branch and any open `merge-freeze` issue is closed. No action needed unless this persists past the all-clear.' \ - '' \ - '')" || true - done - - headline=":snowflake: OpenAPI auto-merge is holding: merge freeze detected." - body="• Reason: ${FREEZE}"$'\n'"• Merges resume automatically once the new GHES version is on the default branch and any 'merge-freeze' issue is closed." - elif [ -n "${BREAKING:-}" ]; then - headline=":rotating_light: OpenAPI auto-merge skipped: potential breaking changes need a human." - body="• Findings: ${BREAKING}" - else - headline=":warning: OpenAPI auto-merge failed in ${GITHUB_REPOSITORY}." - body="• Needs manual merge" - fi - - message=$(printf '%s\n' \ - "$headline" \ - "• PRs: #${PR_30:-n/a} (3.0), #${PR_31:-n/a} (3.1)" \ - "$body" \ - "• Run: ${RUN_URL}") - - curl --fail --silent --show-error \ - -X POST \ - -u "${CHATTERBOX_TOKEN}:" \ - "${CHATTERBOX_URL%/}/topics/%23api-platform" \ - --data "$message" + } >>"$GITHUB_STEP_SUMMARY" From 26daecdf1604ec27d095d9802d3832b5e52d9bcf Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Fri, 14 Aug 2026 15:33:50 -0500 Subject: [PATCH 07/13] Remove internal references from public scanner --- .github/scripts/openapi-breaking-changes.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/scripts/openapi-breaking-changes.py b/.github/scripts/openapi-breaking-changes.py index 1eb65f4673..097e8d4577 100644 --- a/.github/scripts/openapi-breaking-changes.py +++ b/.github/scripts/openapi-breaking-changes.py @@ -1,8 +1,7 @@ #!/usr/bin/env python3 """Detect breaking changes between two OpenAPI description files. -Implements the breaking-change list from -`github/api-platform/docs/creating-an-openapi-release.md`: +Implements the project's documented breaking-change list: 1. An operationId name has been changed. 2. A URL parameter name has been changed. From 281260e4cf1cb94eb898b348b33b3898dcb9d2bf Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Wed, 19 Aug 2026 10:37:40 -0500 Subject: [PATCH 08/13] Harden auto-merge selection, SHA pinning and compatibility scan Address review feedback on the auto-merge workflow. Selection now matches only the two exact titles "Update OpenAPI 3.0 Descriptions" and "Update OpenAPI 3.1 Descriptions" for both candidates and superseded pull requests, so unrelated bot pull requests can never be closed. The logic moves out of inline jq into .github/scripts/select_openapi_prs.py so it can be unit tested, and a title is only allowed to supersede its older siblings when its own newest pull request was successfully pinned. Each superseded pull request is re-checked for state, author and title immediately before it is closed. Every step now works from the head commit SHA captured at selection time: check runs, merge-freeze content reads, fetch, file extraction and merge. The merge step re-reads headRefOid for all candidates and aborts safely if any moved. A mutable branch name is never used to obtain validated content. The compatibility scanner is rewritten as a recursive, $ref-aware comparison that walks properties, array and tuple items, additionalProperties, allOf/oneOf/anyOf and operation parameters, and applies asymmetric request and response rules. Traversal is memoised on resolved node identity so shared schemas are compared once. The module docstring states the known limits rather than implying full coverage. Also adds checks: read and issues: read to the workflow permissions, unit tests for selection, SHA pinning and compatibility cases, a workflow to run those tests, and __pycache__/ to .gitignore. GHES freeze behaviour is unchanged apart from reading content at the pinned SHA. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/scripts/openapi-breaking-changes.py | 235 ------ .github/scripts/openapi_breaking_changes.py | 644 +++++++++++++++ .github/scripts/select_openapi_prs.py | 111 +++ .../scripts/test_openapi_breaking_changes.py | 742 ++++++++++++++++++ .github/scripts/test_select_openapi_prs.py | 158 ++++ .../workflows/auto-merge-openapi-updates.yml | 378 +++++---- .github/workflows/scripts-tests.yml | 33 + .gitignore | 3 +- 8 files changed, 1929 insertions(+), 375 deletions(-) delete mode 100644 .github/scripts/openapi-breaking-changes.py create mode 100644 .github/scripts/openapi_breaking_changes.py create mode 100644 .github/scripts/select_openapi_prs.py create mode 100644 .github/scripts/test_openapi_breaking_changes.py create mode 100644 .github/scripts/test_select_openapi_prs.py create mode 100644 .github/workflows/scripts-tests.yml diff --git a/.github/scripts/openapi-breaking-changes.py b/.github/scripts/openapi-breaking-changes.py deleted file mode 100644 index 097e8d4577..0000000000 --- a/.github/scripts/openapi-breaking-changes.py +++ /dev/null @@ -1,235 +0,0 @@ -#!/usr/bin/env python3 -"""Detect breaking changes between two OpenAPI description files. - -Implements the project's documented breaking-change list: - - 1. An operationId name has been changed. - 2. A URL parameter name has been changed. - 3. An operation has been removed from the description. - 4. A `required: true` has been added to the requestBody. - 5. A parameter has been added to the required list for a requestBody. - 6. A field has been removed from a response body. - 7. A field type has changed in a response body. - 8. A field has been removed from the required list in a response body. - -Plus two structural cases that make the above unrepresentable: - 9. A schema definition has been removed from components/schemas. - 10. An enum value has been removed. - -Note the asymmetry that line-based diffing gets wrong: for a *request* body, -*adding* to `required` is breaking; for a *response* body, *removing* from -`required` is breaking. Both are additive/subtractive in opposite directions, -so they cannot be detected by scanning removed diff lines alone. - -Exit status is always 0; findings go to stdout as JSON. The caller decides -policy. -""" - -import json -import sys - -import yaml - -try: - from yaml import CSafeLoader as Loader -except ImportError: # pragma: no cover - libyaml is present on ubuntu-latest - from yaml import SafeLoader as Loader - -METHODS = ('get', 'put', 'post', 'delete', 'patch', 'options', 'head', 'trace') - - -def load(path): - with open(path, encoding='utf-8') as handle: - return yaml.load(handle, Loader=Loader) - - -def operations(doc): - """Map (path, method) -> operation object.""" - out = {} - for path, item in (doc.get('paths') or {}).items(): - if not isinstance(item, dict): - continue - for method, op in item.items(): - if method in METHODS and isinstance(op, dict): - out[(path, method)] = op - return out - - -def path_params(op): - return { - p.get('name') - for p in (op.get('parameters') or []) - if isinstance(p, dict) and p.get('in') == 'path' - } - - -def request_body(op): - body = op.get('requestBody') - return body if isinstance(body, dict) else {} - - -def json_schema(container): - """Pull the application/json schema out of a requestBody/response.""" - content = container.get('content') - if not isinstance(content, dict): - return {} - for media, spec in content.items(): - if 'json' in media and isinstance(spec, dict): - schema = spec.get('schema') - return schema if isinstance(schema, dict) else {} - return {} - - -def enum_values(schema): - values = schema.get('enum') - return set(map(str, values)) if isinstance(values, list) else set() - - -def compare_operations(base, head, findings): - base_ops, head_ops = operations(base), operations(head) - - for key, op in base_ops.items(): - path, method = key - label = f'{method.upper()} {path}' - - if key not in head_ops: - findings.append({ - 'rule': 'operation-removed', - 'detail': f'{label} was removed', - }) - continue - - new = head_ops[key] - - old_id, new_id = op.get('operationId'), new.get('operationId') - if old_id and new_id and old_id != new_id: - findings.append({ - 'rule': 'operationid-changed', - 'detail': f'{label}: operationId {old_id!r} -> {new_id!r}', - }) - - removed_params = path_params(op) - path_params(new) - if removed_params: - findings.append({ - 'rule': 'url-parameter-renamed', - 'detail': f'{label}: path parameter(s) gone: {sorted(removed_params)}', - }) - - old_body, new_body = request_body(op), request_body(new) - if new_body.get('required') and not old_body.get('required'): - findings.append({ - 'rule': 'requestbody-now-required', - 'detail': f'{label}: requestBody became required', - }) - - added_required = set(json_schema(new_body).get('required') or []) - \ - set(json_schema(old_body).get('required') or []) - if added_required: - findings.append({ - 'rule': 'requestbody-required-added', - 'detail': f'{label}: new required request field(s): {sorted(added_required)}', - }) - - -def compare_schemas(base, head, findings): - """Compare components/schemas. - - Responses overwhelmingly `$ref` into components/schemas in the - non-dereferenced description, so comparing schemas covers "field removed - from a response body" and "field type changed" without resolving refs. - """ - base_schemas = (base.get('components') or {}).get('schemas') or {} - head_schemas = (head.get('components') or {}).get('schemas') or {} - - for name, old in base_schemas.items(): - if not isinstance(old, dict): - continue - - new = head_schemas.get(name) - if new is None: - findings.append({ - 'rule': 'schema-removed', - 'detail': f'schema {name!r} was removed', - }) - continue - if not isinstance(new, dict): - continue - - old_props = old.get('properties') or {} - new_props = new.get('properties') or {} - - for prop in set(old_props) - set(new_props): - findings.append({ - 'rule': 'response-field-removed', - 'detail': f'{name}.{prop} was removed', - }) - - for prop in set(old_props) & set(new_props): - old_p, new_p = old_props[prop], new_props[prop] - if not isinstance(old_p, dict) or not isinstance(new_p, dict): - continue - - old_t, new_t = old_p.get('type'), new_p.get('type') - if old_t and new_t and old_t != new_t: - findings.append({ - 'rule': 'response-field-type-changed', - 'detail': f'{name}.{prop}: type {old_t!r} -> {new_t!r}', - }) - - dropped = enum_values(old_p) - enum_values(new_p) - if dropped: - findings.append({ - 'rule': 'enum-value-removed', - 'detail': f'{name}.{prop}: enum value(s) removed: {sorted(dropped)}', - }) - - # For a response body, losing a guarantee is the breaking direction. - dropped_required = set(old.get('required') or []) - set(new.get('required') or []) - if dropped_required: - findings.append({ - 'rule': 'response-required-removed', - 'detail': f'{name}: no longer guaranteed: {sorted(dropped_required)}', - }) - - dropped = enum_values(old) - enum_values(new) - if dropped: - findings.append({ - 'rule': 'enum-value-removed', - 'detail': f'schema {name}: enum value(s) removed: {sorted(dropped)}', - }) - - -def summarize(base, head): - """Non-breaking additions, used for the informational PR summary.""" - base_ops, head_ops = operations(base), operations(head) - added = [f'{m.upper()} {p}' for (p, m) in set(head_ops) - set(base_ops)] - - base_schemas = set((base.get('components') or {}).get('schemas') or {}) - head_schemas = set((head.get('components') or {}).get('schemas') or {}) - - return { - 'added_operations': sorted(added), - 'added_schemas': sorted(head_schemas - base_schemas), - 'total_operations': len(head_ops), - 'total_schemas': len(head_schemas), - } - - -def main(): - if len(sys.argv) != 3: - print('usage: openapi-breaking-changes.py BASE.yaml HEAD.yaml', file=sys.stderr) - return 2 - - base, head = load(sys.argv[1]), load(sys.argv[2]) - - findings = [] - compare_operations(base, head, findings) - compare_schemas(base, head, findings) - - json.dump({'findings': findings, 'summary': summarize(base, head)}, sys.stdout, indent=2) - print() - return 0 - - -if __name__ == '__main__': - sys.exit(main()) diff --git a/.github/scripts/openapi_breaking_changes.py b/.github/scripts/openapi_breaking_changes.py new file mode 100644 index 0000000000..3f0e6ece1b --- /dev/null +++ b/.github/scripts/openapi_breaking_changes.py @@ -0,0 +1,644 @@ +#!/usr/bin/env python3 +"""Detect breaking changes between two OpenAPI description files. + +Scope +----- +Implements the breaking-change list documented for these descriptions: + + 1. An operationId name has been changed. + 2. A URL parameter name has been changed. + 3. An operation has been removed from the description. + 4. A `required: true` has been added to the requestBody. + 5. A parameter has been added to the required list for a requestBody. + 6. A field has been removed from a response body. + 7. A field type has changed in a response body. + 8. A field has been removed from the required list in a response body. + +Plus two structural cases that make the above unrepresentable: + + 9. A schema definition has been removed from `components/schemas`. + 10. An enum value has been removed. + +Comparison model +---------------- +The descriptions are `$ref`-heavy, so a comparison that only looks at the +top level of `components/schemas` misses most of the surface. This module +instead walks the document: + +* Local `$ref` pointers (`#/components/...`) are resolved before comparing, + including refs inside `parameters`, `responses` and `requestBody`. +* Comparison recurses through `properties`, `items` (arrays, including the + 3.1 tuple form), `additionalProperties`, `allOf`/`oneOf`/`anyOf`, and + operation parameter schemas. +* `allOf` members are merged into an effective schema so that inherited + properties and `required` entries participate in the comparison. +* Inline schemas are covered because the walk starts from every operation's + parameters, request body and 2xx responses rather than from + `components/schemas`. +* JSON media types are tracked on both sides of an operation, so dropping + JSON support from a request body or a response is reported, as is removing + a request body outright. +* Traversal is memoised on the identity of the *resolved* base and head nodes + plus the direction, so a widely shared schema is compared once instead of + once per reference site, and recursive schemas terminate. + +Direction matters, and line-based diffing gets it wrong. For a *request* +body, *adding* to `required` is breaking; for a *response* body, *removing* +from `required` is breaking. The walk therefore carries a direction and +applies the asymmetric rules: a removed property, a removed `required` entry +and any change to a declared `type` are breaking on a response, while a newly +required property, a removed union member and a narrowed `type` are breaking +on a request. Parameters are compared with the request rules; webhook +payloads with the response rules, because consumers receive them. + +Known limits (deliberately not claimed as covered): external/file `$ref` +targets are compared by pointer string only; `not`, `discriminator`, +`patternProperties`, `nullable`, `format`, `default` and numeric/length +constraint tightening are not evaluated; only 2xx responses and JSON media +types are compared, so a non-JSON media type or a 4xx/5xx shape change is +invisible; parameter serialisation (`style`, `explode`) is not compared; and +`oneOf`/`anyOf` members are matched by `$ref` target or `title` when +available and by position otherwise, so a reordered anonymous union can +produce imprecise locations. A finding is reported at the first location it +is reached from, not at every location that shares the schema. + +A general-purpose differ was preferred but does not fit: the widely used +Go/Java differs target OpenAPI 3.0 only, while this repository must also +compare 3.1 descriptions, and the Python dependency set here is +hash-pinned. + +Exit status is 0 unless the inputs cannot be read; findings go to stdout as +JSON and the caller decides policy. +""" + +import argparse +import json +import sys + +import yaml + +try: + from yaml import CSafeLoader as Loader +except ImportError: # pragma: no cover - libyaml is present on ubuntu-latest + from yaml import SafeLoader as Loader + +METHODS = ('get', 'put', 'post', 'delete', 'patch', 'options', 'head', 'trace') + +# Findings are advisory output posted to a pull request. Past some volume the +# list stops being reviewable, and an unbounded list on a pathological diff +# would dominate the job log. +MAX_FINDINGS = 250 + + +def load(path): + with open(path, encoding='utf-8') as handle: + return yaml.load(handle, Loader=Loader) + + +def _pointer(doc, ref): + """Resolve a local JSON pointer such as `#/components/schemas/repo`.""" + node = doc + for token in ref[2:].split('/'): + token = token.replace('~1', '/').replace('~0', '~') + if isinstance(node, dict): + if token not in node: + return None + node = node[token] + elif isinstance(node, list): + try: + node = node[int(token)] + except (ValueError, IndexError): + return None + else: + return None + return node + + +class Comparator: + """Walks a base and a head description in parallel collecting findings.""" + + def __init__(self, base, head): + self.base = base + self.head = head + self.findings = [] + self._seen_findings = set() + self._visited = set() + self._effective = {} + + # -- helpers --------------------------------------------------------- + + def report(self, rule, detail): + key = (rule, detail) + if key in self._seen_findings: + return + self._seen_findings.add(key) + if len(self.findings) < MAX_FINDINGS: + self.findings.append({'rule': rule, 'detail': detail}) + + def resolve(self, doc, node): + """Follow local `$ref` chains. + + Returns `(node, ref)` where `ref` is the last pointer followed, or + `(None, ref)` when a local pointer does not resolve. + """ + ref = None + seen = set() + while isinstance(node, dict) and isinstance(node.get('$ref'), str): + ref = node['$ref'] + if not ref.startswith('#/'): + # External target: not loaded, so treat it as opaque and let + # the caller compare pointer strings. + return node, ref + if ref in seen: + return None, ref + seen.add(ref) + node = _pointer(doc, ref) + if node is None: + return None, ref + return node, ref + + def effective(self, doc, schema): + """Merge `allOf` members into a single comparable schema view.""" + if not isinstance(schema, dict) or 'allOf' not in schema: + return schema if isinstance(schema, dict) else {} + + cached = self._effective.get(id(schema)) + if cached is not None: + return cached + + merged = {k: v for k, v in schema.items() if k != 'allOf'} + # Seed the cache before recursing so a self-referential `allOf` + # terminates instead of recursing forever. + self._effective[id(schema)] = merged + + props = dict(merged.get('properties') or {}) + required = list(merged.get('required') or []) + + members = schema['allOf'] + for member in members if isinstance(members, list) else []: + resolved, _ = self.resolve(doc, member) + if not isinstance(resolved, dict): + continue + resolved = self.effective(doc, resolved) + for key, value in resolved.items(): + if key == 'properties' and isinstance(value, dict): + props.update(value) + elif key == 'required' and isinstance(value, list): + required.extend(value) + elif key not in merged: + merged[key] = value + + if props: + merged['properties'] = props + if required: + merged['required'] = required + return merged + + @staticmethod + def types(schema): + declared = schema.get('type') + if isinstance(declared, str): + return {declared} + if isinstance(declared, list): + return {t for t in declared if isinstance(t, str)} + return set() + + @staticmethod + def enum_values(schema): + values = schema.get('enum') + if not isinstance(values, list): + return set() + return {json.dumps(v, sort_keys=True) for v in values} + + def variant_key(self, doc, member, index): + """Stable identity for a `oneOf`/`anyOf` member.""" + resolved, ref = self.resolve(doc, member) + if ref: + return ref + if isinstance(resolved, dict) and isinstance(resolved.get('title'), str): + return f'title:{resolved["title"]}' + return f'index:{index}' + + def variants(self, doc, schema, keyword): + members = schema.get(keyword) + if not isinstance(members, list): + return {} + out = {} + for index, member in enumerate(members): + out.setdefault(self.variant_key(doc, member, index), member) + return out + + # -- schema comparison ----------------------------------------------- + + def compare_schema(self, base, head, where, direction): + base_ref = head_ref = None + if isinstance(base, dict): + base, base_ref = self.resolve(self.base, base) + if isinstance(head, dict): + head, head_ref = self.resolve(self.head, head) + + # Memoise on the *resolved* nodes. Every `$ref` to the same target + # resolves to the same object, so a widely shared schema is compared + # once instead of once per reference site, and recursive schemas + # terminate. + key = (id(base), id(head), direction) + if key in self._visited: + return + self._visited.add(key) + + if base_ref and not base_ref.startswith('#/'): + # Both sides are external pointers; only the target can be compared. + if base_ref != head_ref: + self.report( + 'external-ref-changed', + f'{where}: {base_ref} -> {head_ref or "removed"}', + ) + return + + if not isinstance(base, dict): + return + if not isinstance(head, dict): + self.report( + 'schema-removed', + f'{where}: {base_ref or "schema"} no longer resolves', + ) + return + + base = self.effective(self.base, base) + head = self.effective(self.head, head) + + self._compare_types(base, head, where, direction) + self._compare_enum(base, head, where) + self._compare_required(base, head, where, direction) + self._compare_properties(base, head, where, direction) + self._compare_items(base, head, where, direction) + self._compare_additional(base, head, where, direction) + self._compare_unions(base, head, where, direction) + + def _compare_types(self, base, head, where, direction): + base_types, head_types = self.types(base), self.types(head) + if not base_types or not head_types: + return + # A response consumer breaks on any type change, including a widened + # set it was never written to handle. A request producer only breaks + # when a type it was sending is no longer accepted. + changed = ( + base_types != head_types + if direction == 'response' + else bool(base_types - head_types) + ) + if changed: + self.report( + 'field-type-changed', + f'{where}: type {sorted(base_types)} -> {sorted(head_types)}', + ) + + def _compare_enum(self, base, head, where): + dropped = self.enum_values(base) - self.enum_values(head) + if dropped: + values = [json.loads(value) for value in sorted(dropped)] + self.report( + 'enum-value-removed', + f'{where}: enum value(s) removed: {values}', + ) + + def _compare_required(self, base, head, where, direction): + base_required = {r for r in (base.get('required') or []) if isinstance(r, str)} + head_required = {r for r in (head.get('required') or []) if isinstance(r, str)} + if direction == 'request': + added = head_required - base_required + if added: + self.report( + 'requestbody-required-added', + f'{where}: newly required field(s): {sorted(added)}', + ) + else: + dropped = base_required - head_required + if dropped: + self.report( + 'response-required-removed', + f'{where}: no longer guaranteed: {sorted(dropped)}', + ) + + def _compare_properties(self, base, head, where, direction): + base_props = base.get('properties') + head_props = head.get('properties') + if not isinstance(base_props, dict): + return + if not isinstance(head_props, dict): + head_props = {} + + if direction == 'response': + for name in sorted(set(base_props) - set(head_props)): + self.report( + 'response-field-removed', + f'{where}.{name} was removed', + ) + + for name in sorted(set(base_props) & set(head_props)): + self.compare_schema( + base_props[name], head_props[name], f'{where}.{name}', direction + ) + + def _compare_items(self, base, head, where, direction): + base_items, head_items = base.get('items'), head.get('items') + if base_items is None: + return + if head_items is None: + self.report('array-items-removed', f'{where}: item schema was removed') + return + if isinstance(base_items, dict) and isinstance(head_items, dict): + self.compare_schema(base_items, head_items, f'{where}[]', direction) + elif isinstance(base_items, list) and isinstance(head_items, list): + if len(head_items) < len(base_items): + self.report( + 'tuple-items-removed', + f'{where}: positional item(s) {len(base_items)} -> {len(head_items)}', + ) + for index, (base_item, head_item) in enumerate(zip(base_items, head_items)): + self.compare_schema(base_item, head_item, f'{where}[{index}]', direction) + elif isinstance(base_items, (dict, list)): + self.report( + 'array-items-changed', + f'{where}: item schema changed shape', + ) + + def _compare_additional(self, base, head, where, direction): + base_extra = base.get('additionalProperties') + head_extra = head.get('additionalProperties') + + # Absent means "allowed", so absent -> false is a narrowing too. + if head_extra is False and base_extra is not False: + self.report( + 'additional-properties-restricted', + f'{where}: additionalProperties narrowed to false', + ) + + if isinstance(base_extra, dict) and isinstance(head_extra, dict): + self.compare_schema(base_extra, head_extra, f'{where}.*', direction) + + def _compare_unions(self, base, head, where, direction): + for keyword in ('oneOf', 'anyOf'): + base_variants = self.variants(self.base, base, keyword) + head_variants = self.variants(self.head, head, keyword) + if not base_variants: + continue + + if direction == 'request': + # A client that sent one of the removed shapes now fails. + for key in sorted(set(base_variants) - set(head_variants)): + self.report( + 'request-variant-removed', + f'{where}: {keyword} no longer accepts {key}', + ) + + for key in sorted(set(base_variants) & set(head_variants)): + self.compare_schema( + base_variants[key], + head_variants[key], + f'{where}({keyword}:{key})', + direction, + ) + + # -- document comparison --------------------------------------------- + + def parameters(self, doc, op): + """Resolved parameters for an operation, keyed by `(in, name)`.""" + out = {} + for param in op.get('parameters') or []: + resolved, _ = self.resolve(doc, param) + if isinstance(resolved, dict) and resolved.get('name'): + out[(resolved.get('in'), resolved['name'])] = resolved + return out + + def json_schemas(self, doc, container): + """JSON media-type schemas of a requestBody/response, by media type.""" + resolved, _ = self.resolve(doc, container) + if not isinstance(resolved, dict): + return {}, {} + content = resolved.get('content') + if not isinstance(content, dict): + return resolved, {} + schemas = { + media: spec['schema'] + for media, spec in content.items() + if 'json' in media and isinstance(spec, dict) and isinstance(spec.get('schema'), dict) + } + return resolved, schemas + + def compare_operation(self, base_op, head_op, label, request_direction='request'): + base_id, head_id = base_op.get('operationId'), head_op.get('operationId') + if base_id and head_id and base_id != head_id: + self.report( + 'operationid-changed', + f'{label}: operationId {base_id!r} -> {head_id!r}', + ) + + base_params = self.parameters(self.base, base_op) + head_params = self.parameters(self.head, head_op) + removed_path_params = sorted( + name for (loc, name) in set(base_params) - set(head_params) if loc == 'path' + ) + if removed_path_params: + self.report( + 'url-parameter-renamed', + f'{label}: path parameter(s) gone: {removed_path_params}', + ) + + for key in sorted( + set(base_params) & set(head_params), key=lambda k: (str(k[0]), k[1]) + ): + location, name = key + base_param, head_param = base_params[key], head_params[key] + if head_param.get('required') and not base_param.get('required'): + self.report( + 'parameter-now-required', + f'{label}: {location} parameter {name!r} became required', + ) + if isinstance(base_param.get('schema'), dict) and isinstance( + head_param.get('schema'), dict + ): + # A parameter is something the caller sends, so request rules + # apply: a narrowed type or a dropped enum value breaks it. + self.compare_schema( + base_param['schema'], + head_param['schema'], + f'{label} {location} parameter {name}', + 'request', + ) + + base_body, base_body_schemas = self.json_schemas(self.base, base_op.get('requestBody')) + head_body, head_body_schemas = self.json_schemas(self.head, head_op.get('requestBody')) + if isinstance(base_body, dict) and base_body: + if not isinstance(head_body, dict) or not head_body: + # The operation no longer accepts a body at all, so callers + # that send one may now be rejected. + self.report( + 'request-body-removed', + f'{label}: requestBody was removed', + ) + else: + if head_body.get('required') and not base_body.get('required'): + self.report( + 'requestbody-now-required', + f'{label}: requestBody became required', + ) + # Mirrors the response side: a caller that submits JSON breaks + # when that media type stops being accepted. + for media in sorted(set(base_body_schemas) - set(head_body_schemas)): + self.report( + 'request-media-type-removed', + f'{label} request body: {media} was removed', + ) + for media in sorted(set(base_body_schemas) & set(head_body_schemas)): + self.compare_schema( + base_body_schemas[media], + head_body_schemas[media], + f'{label} request body', + request_direction, + ) + + base_responses = base_op.get('responses') + head_responses = head_op.get('responses') + if not isinstance(base_responses, dict) or not isinstance(head_responses, dict): + return + # Status codes are strings in OpenAPI but YAML may yield integers, so + # compare them in one normalised form. + head_responses = {str(code): value for code, value in head_responses.items()} + for code, base_response in sorted( + (str(code), value) for code, value in base_responses.items() + ): + if not code.startswith('2'): + continue + if code not in head_responses: + self.report( + 'response-status-removed', + f'{label}: {code} response was removed', + ) + continue + _, base_schemas = self.json_schemas(self.base, base_response) + _, head_schemas = self.json_schemas(self.head, head_responses[code]) + for media in sorted(set(base_schemas) - set(head_schemas)): + self.report( + 'response-media-type-removed', + f'{label} {code} response: {media} was removed', + ) + for media in sorted(set(base_schemas) & set(head_schemas)): + self.compare_schema( + base_schemas[media], + head_schemas[media], + f'{label} {code} response', + 'response', + ) + + def compare_paths(self): + base_ops, head_ops = operations(self.base), operations(self.head) + for key in sorted(base_ops): + path, method = key + label = f'{method.upper()} {path}' + if key not in head_ops: + self.report('operation-removed', f'{label} was removed') + continue + self.compare_operation(base_ops[key], head_ops[key], label) + + def compare_webhooks(self): + """Walk `webhooks` (3.1) and `x-webhooks` (3.0) payload schemas. + + A webhook payload is delivered to the consumer, so its request body + is compared with the response rules. + """ + for container in ('webhooks', 'x-webhooks'): + base_hooks = self.base.get(container) + head_hooks = self.head.get(container) + if not isinstance(base_hooks, dict) or not isinstance(head_hooks, dict): + continue + for name in sorted(base_hooks): + base_item, _ = self.resolve(self.base, base_hooks[name]) + head_item, _ = self.resolve(self.head, head_hooks.get(name)) + if not isinstance(base_item, dict): + continue + if not isinstance(head_item, dict): + self.report('webhook-removed', f'webhook {name!r} was removed') + continue + for method in METHODS: + base_op, head_op = base_item.get(method), head_item.get(method) + if isinstance(base_op, dict) and isinstance(head_op, dict): + self.compare_operation( + base_op, + head_op, + f'webhook {name}', + request_direction='response', + ) + + def compare_components(self): + """Report `components/schemas` removals (rule 9). + + Content comparison happens through the operation and webhook walk, + which reaches these schemas via their `$ref`s. + """ + base_schemas = (self.base.get('components') or {}).get('schemas') or {} + head_schemas = (self.head.get('components') or {}).get('schemas') or {} + if not isinstance(base_schemas, dict) or not isinstance(head_schemas, dict): + return + for name in sorted(set(base_schemas) - set(head_schemas)): + self.report('schema-removed', f'schema {name!r} was removed') + + def run(self): + self.compare_paths() + self.compare_webhooks() + self.compare_components() + return self.findings + + +def operations(doc): + """Map `(path, method)` -> operation object.""" + out = {} + paths = doc.get('paths') + for path, item in (paths if isinstance(paths, dict) else {}).items(): + if not isinstance(item, dict): + continue + for method, op in item.items(): + if method in METHODS and isinstance(op, dict): + out[(path, method)] = op + return out + + +def summarize(base, head): + """Non-breaking additions, used for the informational PR summary.""" + base_ops, head_ops = operations(base), operations(head) + added = [f'{m.upper()} {p}' for (p, m) in set(head_ops) - set(base_ops)] + + base_schemas = set((base.get('components') or {}).get('schemas') or {}) + head_schemas = set((head.get('components') or {}).get('schemas') or {}) + + return { + 'added_operations': sorted(added), + 'added_schemas': sorted(head_schemas - base_schemas), + 'total_operations': len(head_ops), + 'total_schemas': len(head_schemas), + } + + +def compare(base, head): + findings = Comparator(base, head).run() + return { + 'findings': findings, + 'truncated': len(findings) >= MAX_FINDINGS, + 'summary': summarize(base, head), + } + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument('base', help='baseline description (YAML or JSON)') + parser.add_argument('head', help='candidate description (YAML or JSON)') + args = parser.parse_args(argv) + + result = compare(load(args.base), load(args.head)) + json.dump(result, sys.stdout, indent=2) + print() + return 0 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/.github/scripts/select_openapi_prs.py b/.github/scripts/select_openapi_prs.py new file mode 100644 index 0000000000..0d770e8c78 --- /dev/null +++ b/.github/scripts/select_openapi_prs.py @@ -0,0 +1,111 @@ +#!/usr/bin/env python3 +"""Select the OpenAPI description update PRs to merge, and the superseded ones. + +Reads the JSON array produced by + + gh pr list --state open --author \ + --json number,title,headRefOid,createdAt + +on stdin and writes `key=value` lines suitable for `$GITHUB_OUTPUT`. + +Two properties matter for safety and are the reason this is a script rather +than an inline `jq` filter: + +* Only pull requests whose title is *exactly* one of the two recognised + description-update titles are considered. Any other pull request by the + same author is neither merged nor closed. +* A title only contributes superseded pull requests when its own newest + pull request was selected, so nothing is closed unless a validated + replacement for that exact title is being merged. +* The head commit SHA of each selected pull request is emitted alongside its + number, so every later step can pin to the exact commit that was inspected + instead of resolving a branch name again. +""" + +import argparse +import json +import sys + +TITLE_30 = 'Update OpenAPI 3.0 Descriptions' +TITLE_31 = 'Update OpenAPI 3.1 Descriptions' +TITLES = (TITLE_30, TITLE_31) + + +def _sort_key(pr): + # `createdAt` is RFC 3339 in UTC, so lexical order is chronological. + # The number breaks ties deterministically. + return (pr.get('createdAt') or '', pr.get('number') or 0) + + +def select(pull_requests): + """Return the pinnable newest PR per recognised title, and the superseded. + + A title only contributes superseded pull requests when its own newest + pull request was actually selected. Nothing is ever closed on the basis + of a replacement that was not validated. + """ + recognised = [ + pr + for pr in pull_requests + if isinstance(pr, dict) and pr.get('title') in TITLES and pr.get('number') + ] + + selected = {} + superseded = [] + for title in TITLES: + candidates = [pr for pr in recognised if pr['title'] == title] + if not candidates: + continue + newest = max(candidates, key=_sort_key) + # Without a head SHA the newest pull request cannot be pinned to an + # exact commit, so it is not merged from a mutable branch instead -- + # and none of its older siblings are closed. + if not newest.get('headRefOid'): + continue + selected[title] = newest + superseded.extend( + pr['number'] for pr in candidates if pr['number'] != newest['number'] + ) + + return selected, sorted(superseded) + + +def outputs(pull_requests): + selected, superseded = select(pull_requests) + + lines = [f'found={"true" if selected else "false"}'] + for suffix, title in (('30', TITLE_30), ('31', TITLE_31)): + pr = selected.get(title) + lines.append(f'pr_{suffix}={pr["number"] if pr else ""}') + lines.append(f'sha_{suffix}={pr["headRefOid"] if pr else ""}') + + lines.append('superseded=' + ' '.join(str(n) for n in superseded)) + return lines + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument( + '--titles', + action='store_true', + help='print the recognised titles, one per line, and exit', + ) + args = parser.parse_args(argv) + + if args.titles: + for title in TITLES: + print(title) + return 0 + + payload = json.load(sys.stdin) + if not isinstance(payload, list): + print('expected a JSON array of pull requests', file=sys.stderr) + return 2 + + for line in outputs(payload): + print(line) + return 0 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/.github/scripts/test_openapi_breaking_changes.py b/.github/scripts/test_openapi_breaking_changes.py new file mode 100644 index 0000000000..269a3362b5 --- /dev/null +++ b/.github/scripts/test_openapi_breaking_changes.py @@ -0,0 +1,742 @@ +#!/usr/bin/env python3 +"""Tests for openapi_breaking_changes. + +Run with: python3 -m unittest discover -s .github/scripts +""" + +import copy +import os +import sys +import unittest + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +import openapi_breaking_changes as obc # noqa: E402 + + +def doc(paths=None, schemas=None, **extra): + out = {'openapi': '3.0.3', 'paths': paths or {}} + if schemas is not None: + out['components'] = {'schemas': schemas} + out.update(extra) + return out + + +def get_op(schema, method='get', path='/thing'): + """A document with one operation returning `schema` as a 200 body.""" + return doc(paths={ + path: { + method: { + 'operationId': 'thing/get', + 'responses': { + '200': { + 'description': 'ok', + 'content': {'application/json': {'schema': schema}}, + } + }, + } + } + }) + + +def post_op(schema, required=False, path='/thing'): + """A document with one operation accepting `schema` as a request body.""" + return doc(paths={ + path: { + 'post': { + 'operationId': 'thing/create', + 'requestBody': { + 'required': required, + 'content': {'application/json': {'schema': schema}}, + }, + 'responses': {'201': {'description': 'created'}}, + } + } + }) + + +def rules(base, head): + return sorted(f['rule'] for f in obc.compare(base, head)['findings']) + + +def details(base, head, rule): + return [f['detail'] for f in obc.compare(base, head)['findings'] if f['rule'] == rule] + + +class NoChangeTest(unittest.TestCase): + def test_identical_documents_are_clean(self): + base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) + self.assertEqual(rules(base, copy.deepcopy(base)), []) + + def test_additions_are_not_breaking(self): + base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) + head = get_op({ + 'type': 'object', + 'properties': {'id': {'type': 'integer'}, 'name': {'type': 'string'}}, + }) + head['paths']['/added'] = {'get': {'operationId': 'a/b', 'responses': {}}} + self.assertEqual(rules(base, head), []) + + +class OperationTest(unittest.TestCase): + def test_operation_removed(self): + base = get_op({'type': 'object'}) + self.assertEqual(rules(base, doc()), ['operation-removed']) + + def test_operationid_changed(self): + base = get_op({'type': 'object'}) + head = copy.deepcopy(base) + head['paths']['/thing']['get']['operationId'] = 'thing/fetch' + self.assertEqual(rules(base, head), ['operationid-changed']) + + def test_path_parameter_renamed_behind_a_ref(self): + base = doc(paths={ + '/repos/{owner}': { + 'get': { + 'operationId': 'repos/get', + 'parameters': [{'$ref': '#/components/parameters/owner'}], + 'responses': {}, + } + } + }) + base['components'] = { + 'parameters': {'owner': {'name': 'owner', 'in': 'path', 'required': True}} + } + head = copy.deepcopy(base) + head['components']['parameters']['owner']['name'] = 'org' + self.assertEqual(rules(base, head), ['url-parameter-renamed']) + self.assertIn('owner', details(base, head, 'url-parameter-renamed')[0]) + + def test_query_parameter_removal_is_not_reported(self): + base = doc(paths={ + '/thing': { + 'get': { + 'operationId': 'thing/get', + 'parameters': [{'name': 'per_page', 'in': 'query'}], + 'responses': {}, + } + } + }) + head = copy.deepcopy(base) + head['paths']['/thing']['get']['parameters'] = [] + self.assertEqual(rules(base, head), []) + + def _param_docs(self, base_param, head_param): + def build(param): + return doc(paths={'/thing': {'get': { + 'operationId': 'thing/get', + 'parameters': [param], + 'responses': {}, + }}}) + + return build(base_param), build(head_param) + + def test_parameter_became_required(self): + base, head = self._param_docs( + {'name': 'state', 'in': 'query', 'schema': {'type': 'string'}}, + {'name': 'state', 'in': 'query', 'required': True, + 'schema': {'type': 'string'}}, + ) + self.assertEqual(rules(base, head), ['parameter-now-required']) + + def test_parameter_enum_value_removed(self): + base, head = self._param_docs( + {'name': 'state', 'in': 'query', + 'schema': {'type': 'string', 'enum': ['open', 'closed', 'all']}}, + {'name': 'state', 'in': 'query', + 'schema': {'type': 'string', 'enum': ['open', 'closed']}}, + ) + self.assertEqual(rules(base, head), ['enum-value-removed']) + + def test_parameter_schema_widening_is_not_breaking(self): + base, head = self._param_docs( + {'name': 'id', 'in': 'query', 'schema': {'type': 'integer'}}, + {'name': 'id', 'in': 'query', 'schema': {'type': ['integer', 'string']}}, + ) + self.assertEqual(rules(base, head), []) + + def test_success_status_removed(self): + base = doc(paths={'/thing': {'get': { + 'operationId': 'thing/get', + 'responses': {'200': {'description': 'ok'}, '204': {'description': 'empty'}}, + }}}) + head = doc(paths={'/thing': {'get': { + 'operationId': 'thing/get', + 'responses': {'200': {'description': 'ok'}}, + }}}) + self.assertEqual(rules(base, head), ['response-status-removed']) + + def test_error_status_removal_is_not_reported(self): + base = doc(paths={'/thing': {'get': { + 'operationId': 'thing/get', + 'responses': {'200': {'description': 'ok'}, '404': {'description': 'gone'}}, + }}}) + head = doc(paths={'/thing': {'get': { + 'operationId': 'thing/get', + 'responses': {'200': {'description': 'ok'}}, + }}}) + self.assertEqual(rules(base, head), []) + + def test_integer_status_keys_are_normalised(self): + base = doc(paths={'/thing': {'get': { + 'operationId': 'thing/get', + 'responses': {200: {'description': 'ok'}}, + }}}) + head = doc(paths={'/thing': {'get': { + 'operationId': 'thing/get', + 'responses': {'200': {'description': 'ok'}}, + }}}) + self.assertEqual(rules(base, head), []) + + def test_json_media_type_removed(self): + base = get_op({'type': 'object'}) + head = copy.deepcopy(base) + content = head['paths']['/thing']['get']['responses']['200']['content'] + content['text/plain'] = content.pop('application/json') + self.assertEqual(rules(base, head), ['response-media-type-removed']) + + +class RequestDirectionTest(unittest.TestCase): + def test_request_body_became_required(self): + base = post_op({'type': 'object'}, required=False) + head = post_op({'type': 'object'}, required=True) + self.assertEqual(rules(base, head), ['requestbody-now-required']) + + def test_request_body_removed_entirely(self): + base = post_op({'type': 'object'}, required=True) + head = copy.deepcopy(base) + del head['paths']['/thing']['post']['requestBody'] + self.assertEqual(rules(base, head), ['request-body-removed']) + + def test_optional_request_body_removed_is_still_reported(self): + base = post_op({'type': 'object'}, required=False) + head = copy.deepcopy(base) + del head['paths']['/thing']['post']['requestBody'] + self.assertEqual(rules(base, head), ['request-body-removed']) + + def test_request_json_media_type_removed(self): + base = post_op({'type': 'object'}) + head = copy.deepcopy(base) + content = head['paths']['/thing']['post']['requestBody']['content'] + content['multipart/form-data'] = content.pop('application/json') + self.assertEqual(rules(base, head), ['request-media-type-removed']) + self.assertEqual( + details(base, head, 'request-media-type-removed'), + ['POST /thing request body: application/json was removed'], + ) + + def test_added_request_media_type_is_not_breaking(self): + base = post_op({'type': 'object'}) + head = copy.deepcopy(base) + head['paths']['/thing']['post']['requestBody']['content']['application/vnd.v3+json'] = { + 'schema': {'type': 'object'} + } + self.assertEqual(rules(base, head), []) + + def test_adding_a_request_body_is_not_breaking(self): + head = post_op({'type': 'object'}, required=False) + base = copy.deepcopy(head) + del base['paths']['/thing']['post']['requestBody'] + self.assertEqual(rules(base, head), []) + + def test_newly_required_request_field(self): + schema = {'type': 'object', 'properties': {'name': {'type': 'string'}}} + head_schema = dict(schema, required=['name']) + self.assertEqual( + rules(post_op(schema), post_op(head_schema)), + ['requestbody-required-added'], + ) + + def test_dropping_a_required_request_field_is_not_breaking(self): + schema = {'type': 'object', 'properties': {'name': {'type': 'string'}}} + self.assertEqual( + rules(post_op(dict(schema, required=['name'])), post_op(schema)), + [], + ) + + def test_removing_a_request_property_is_not_breaking(self): + base = post_op({ + 'type': 'object', + 'properties': {'name': {'type': 'string'}, 'note': {'type': 'string'}}, + }) + head = post_op({'type': 'object', 'properties': {'name': {'type': 'string'}}}) + self.assertEqual(rules(base, head), []) + + def test_widening_a_request_type_is_not_breaking(self): + base = post_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) + head = post_op({ + 'type': 'object', + 'properties': {'id': {'type': ['integer', 'string']}}, + }) + self.assertEqual(rules(base, head), []) + + def test_narrowing_a_request_type_is_breaking(self): + base = post_op({ + 'type': 'object', + 'properties': {'id': {'type': ['integer', 'string']}}, + }) + head = post_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) + self.assertEqual(rules(base, head), ['field-type-changed']) + + +class ResponseDirectionTest(unittest.TestCase): + def test_response_field_removed(self): + base = get_op({ + 'type': 'object', + 'properties': {'id': {'type': 'integer'}, 'name': {'type': 'string'}}, + }) + head = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) + self.assertEqual(rules(base, head), ['response-field-removed']) + + def test_response_required_removed(self): + schema = {'type': 'object', 'properties': {'id': {'type': 'integer'}}} + base = get_op(dict(schema, required=['id'])) + self.assertEqual(rules(base, get_op(schema)), ['response-required-removed']) + + def test_adding_a_response_required_field_is_not_breaking(self): + schema = {'type': 'object', 'properties': {'id': {'type': 'integer'}}} + self.assertEqual(rules(get_op(schema), get_op(dict(schema, required=['id']))), []) + + def test_response_type_change_in_either_direction(self): + base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) + head = get_op({ + 'type': 'object', + 'properties': {'id': {'type': ['integer', 'null']}}, + }) + self.assertEqual(rules(base, head), ['field-type-changed']) + + +class NestedTest(unittest.TestCase): + """The previous shallow comparison missed everything in this class.""" + + def test_nested_inline_object(self): + base = get_op({ + 'type': 'object', + 'properties': { + 'owner': { + 'type': 'object', + 'properties': {'login': {'type': 'string'}, 'id': {'type': 'integer'}}, + } + }, + }) + head = copy.deepcopy(base) + del head['paths']['/thing']['get']['responses']['200']['content'][ + 'application/json']['schema']['properties']['owner']['properties']['login'] + self.assertEqual(rules(base, head), ['response-field-removed']) + self.assertIn('.owner.login', details(base, head, 'response-field-removed')[0]) + + def test_three_levels_deep(self): + def build(leaf): + return get_op({ + 'type': 'object', + 'properties': { + 'a': { + 'type': 'object', + 'properties': { + 'b': {'type': 'object', 'properties': {'c': leaf}}, + }, + } + }, + }) + + self.assertEqual( + rules(build({'type': 'string'}), build({'type': 'integer'})), + ['field-type-changed'], + ) + + def test_array_items(self): + base = get_op({ + 'type': 'array', + 'items': { + 'type': 'object', + 'properties': {'id': {'type': 'integer'}, 'name': {'type': 'string'}}, + }, + }) + head = get_op({ + 'type': 'array', + 'items': {'type': 'object', 'properties': {'id': {'type': 'integer'}}}, + }) + self.assertEqual(rules(base, head), ['response-field-removed']) + self.assertIn('[].name', details(base, head, 'response-field-removed')[0]) + + def test_array_of_arrays(self): + base = get_op({ + 'type': 'array', + 'items': {'type': 'array', 'items': {'type': 'string'}}, + }) + head = get_op({ + 'type': 'array', + 'items': {'type': 'array', 'items': {'type': 'integer'}}, + }) + self.assertEqual(rules(base, head), ['field-type-changed']) + + def test_additional_properties_schema(self): + base = get_op({ + 'type': 'object', + 'additionalProperties': { + 'type': 'object', + 'properties': {'id': {'type': 'integer'}}, + }, + }) + head = get_op({'type': 'object', 'additionalProperties': {'type': 'object'}}) + self.assertEqual(rules(base, head), ['response-field-removed']) + + def test_additional_properties_narrowed_to_false(self): + base = get_op({'type': 'object', 'additionalProperties': True}) + head = get_op({'type': 'object', 'additionalProperties': False}) + self.assertEqual(rules(base, head), ['additional-properties-restricted']) + + def test_additional_properties_absent_then_false(self): + base = get_op({'type': 'object', 'properties': {}}) + head = get_op({'type': 'object', 'properties': {}, 'additionalProperties': False}) + self.assertEqual(rules(base, head), ['additional-properties-restricted']) + + def test_additional_properties_widened_is_not_breaking(self): + base = get_op({'type': 'object', 'additionalProperties': False}) + head = get_op({'type': 'object', 'additionalProperties': True}) + self.assertEqual(rules(base, head), []) + + def test_tuple_item_removed(self): + base = get_op({ + 'type': 'array', + 'items': [{'type': 'string'}, {'type': 'integer'}], + }) + head = get_op({'type': 'array', 'items': [{'type': 'string'}]}) + self.assertEqual(rules(base, head), ['tuple-items-removed']) + + def test_tuple_item_type_changed(self): + base = get_op({ + 'type': 'array', + 'items': [{'type': 'string'}, {'type': 'integer'}], + }) + head = get_op({ + 'type': 'array', + 'items': [{'type': 'string'}, {'type': 'boolean'}], + }) + self.assertEqual(rules(base, head), ['field-type-changed']) + + def test_item_schema_removed(self): + base = get_op({'type': 'array', 'items': {'type': 'string'}}) + head = get_op({'type': 'array'}) + self.assertEqual(rules(base, head), ['array-items-removed']) + + def test_item_schema_shape_changed(self): + base = get_op({'type': 'array', 'items': [{'type': 'string'}]}) + head = get_op({'type': 'array', 'items': {'type': 'string'}}) + self.assertEqual(rules(base, head), ['array-items-changed']) + + +class ReferenceTest(unittest.TestCase): + def _ref_docs(self, base_schema, head_schema): + base = get_op({'$ref': '#/components/schemas/thing'}) + base['components'] = {'schemas': {'thing': base_schema}} + head = get_op({'$ref': '#/components/schemas/thing'}) + head['components'] = {'schemas': {'thing': head_schema}} + return base, head + + def test_field_removed_behind_a_ref(self): + base, head = self._ref_docs( + {'type': 'object', 'properties': {'id': {'type': 'integer'}, 'x': {'type': 'string'}}}, + {'type': 'object', 'properties': {'id': {'type': 'integer'}}}, + ) + self.assertEqual(rules(base, head), ['response-field-removed']) + + def test_ref_chain_is_followed(self): + base = get_op({'$ref': '#/components/schemas/alias'}) + base['components'] = {'schemas': { + 'alias': {'$ref': '#/components/schemas/thing'}, + 'thing': {'type': 'object', 'properties': {'id': {'type': 'integer'}}}, + }} + head = copy.deepcopy(base) + head['components']['schemas']['thing']['properties']['id']['type'] = 'string' + self.assertEqual(rules(base, head), ['field-type-changed']) + + def test_response_object_ref_is_followed(self): + base = doc(paths={'/thing': {'get': { + 'operationId': 'thing/get', + 'responses': {'200': {'$ref': '#/components/responses/thing'}}, + }}}) + base['components'] = {'responses': {'thing': { + 'description': 'ok', + 'content': {'application/json': {'schema': { + 'type': 'object', + 'properties': {'id': {'type': 'integer'}, 'x': {'type': 'string'}}, + }}}, + }}} + head = copy.deepcopy(base) + del head['components']['responses']['thing']['content'][ + 'application/json']['schema']['properties']['x'] + self.assertEqual(rules(base, head), ['response-field-removed']) + + def test_removed_component_schema(self): + base = get_op({'$ref': '#/components/schemas/thing'}) + base['components'] = {'schemas': {'thing': {'type': 'object'}}} + head = get_op({'$ref': '#/components/schemas/thing'}) + head['components'] = {'schemas': {}} + self.assertEqual(rules(base, head), ['schema-removed', 'schema-removed']) + + def test_unreferenced_component_removal_is_reported(self): + base = doc(schemas={'orphan': {'type': 'object'}}) + head = doc(schemas={}) + self.assertEqual(rules(base, head), ['schema-removed']) + + def test_recursive_schema_terminates(self): + def build(leaf_type): + document = get_op({'$ref': '#/components/schemas/node'}) + document['components'] = {'schemas': {'node': { + 'type': 'object', + 'properties': { + 'value': {'type': leaf_type}, + 'parent': {'$ref': '#/components/schemas/node'}, + 'children': { + 'type': 'array', + 'items': {'$ref': '#/components/schemas/node'}, + }, + }, + }}} + return document + + self.assertEqual(rules(build('string'), build('integer')), ['field-type-changed']) + + def test_self_referential_ref_does_not_hang(self): + base = get_op({'$ref': '#/components/schemas/loop'}) + base['components'] = {'schemas': {'loop': {'$ref': '#/components/schemas/loop'}}} + self.assertEqual(rules(base, copy.deepcopy(base)), []) + + +class CompositionTest(unittest.TestCase): + def test_allof_member_field_removed(self): + base = get_op({'allOf': [ + {'type': 'object', 'properties': {'id': {'type': 'integer'}}}, + {'type': 'object', 'properties': {'name': {'type': 'string'}}}, + ]}) + head = get_op({'allOf': [ + {'type': 'object', 'properties': {'id': {'type': 'integer'}}}, + ]}) + self.assertEqual(rules(base, head), ['response-field-removed']) + + def test_allof_via_ref_field_type_changed(self): + base = get_op({'allOf': [ + {'$ref': '#/components/schemas/base'}, + {'type': 'object', 'properties': {'extra': {'type': 'string'}}}, + ]}) + base['components'] = {'schemas': {'base': { + 'type': 'object', 'properties': {'id': {'type': 'integer'}}, + }}} + head = copy.deepcopy(base) + head['components']['schemas']['base']['properties']['id']['type'] = 'string' + self.assertEqual(rules(base, head), ['field-type-changed']) + + def test_allof_inherited_required_removed(self): + def build(required): + document = get_op({'allOf': [ + {'$ref': '#/components/schemas/base'}, + {'type': 'object', 'properties': {'extra': {'type': 'string'}}}, + ]}) + document['components'] = {'schemas': {'base': { + 'type': 'object', + 'properties': {'id': {'type': 'integer'}}, + 'required': required, + }}} + return document + + self.assertEqual(rules(build(['id']), build([])), ['response-required-removed']) + + def test_nested_allof_is_flattened(self): + def build(props): + return get_op({'allOf': [ + {'allOf': [{'type': 'object', 'properties': props}]}, + ]}) + + self.assertEqual( + rules(build({'a': {'type': 'string'}, 'b': {'type': 'string'}}), + build({'a': {'type': 'string'}})), + ['response-field-removed'], + ) + + def test_oneof_member_is_compared(self): + def build(leaf): + document = get_op({'oneOf': [ + {'$ref': '#/components/schemas/simple'}, + {'$ref': '#/components/schemas/full'}, + ]}) + document['components'] = {'schemas': { + 'simple': {'type': 'string'}, + 'full': {'type': 'object', 'properties': {'id': leaf}}, + }} + return document + + self.assertEqual( + rules(build({'type': 'integer'}), build({'type': 'string'})), + ['field-type-changed'], + ) + + def test_anyof_member_matched_by_title(self): + def build(leaf): + return get_op({'anyOf': [ + {'title': 'a', 'type': 'object', 'properties': {'x': leaf}}, + {'title': 'b', 'type': 'string'}, + ]}) + + # Reordering alone is not a finding; the type change is. + head = build({'type': 'integer'}) + head['paths']['/thing']['get']['responses']['200']['content'][ + 'application/json']['schema']['anyOf'].reverse() + self.assertEqual(rules(build({'type': 'string'}), head), ['field-type-changed']) + + def test_request_union_member_removed_is_breaking(self): + base = post_op({'oneOf': [{'type': 'string'}, {'type': 'integer'}]}) + head = post_op({'oneOf': [{'type': 'string'}]}) + self.assertEqual(rules(base, head), ['request-variant-removed']) + + def test_response_union_member_removed_is_not_reported(self): + base = get_op({'oneOf': [ + {'title': 'a', 'type': 'string'}, + {'title': 'b', 'type': 'integer'}, + ]}) + head = get_op({'oneOf': [{'title': 'a', 'type': 'string'}]}) + self.assertEqual(rules(base, head), []) + + +class EnumTest(unittest.TestCase): + def test_enum_value_removed_at_depth(self): + def build(values): + return get_op({ + 'type': 'object', + 'properties': { + 'items': { + 'type': 'array', + 'items': { + 'type': 'object', + 'properties': {'state': {'type': 'string', 'enum': values}}, + }, + } + }, + }) + + base, head = build(['open', 'closed', 'draft']), build(['open', 'closed']) + self.assertEqual(rules(base, head), ['enum-value-removed']) + self.assertIn('draft', details(base, head, 'enum-value-removed')[0]) + + def test_awkward_scalar_enum_values(self): + awkward = ['+1', '-1', "won't fix", 'false positive', True, False, None, 1] + + def build(values): + return get_op({'type': 'object', 'properties': { + 'reaction': {'enum': values}, + }}) + + base = build(awkward) + head = build([v for v in awkward if v != '+1']) + self.assertEqual(rules(base, head), ['enum-value-removed']) + self.assertIn('+1', details(base, head, 'enum-value-removed')[0]) + + def test_boolean_and_string_enum_values_are_distinct(self): + def build(values): + return get_op({'type': 'object', 'properties': {'flag': {'enum': values}}}) + + self.assertEqual(rules(build([True]), build(['true'])), ['enum-value-removed']) + + def test_enum_value_added_is_not_reported(self): + def build(values): + return get_op({'type': 'object', 'properties': { + 'state': {'type': 'string', 'enum': values}, + }}) + + self.assertEqual(rules(build(['open']), build(['open', 'closed'])), []) + + +class WebhookTest(unittest.TestCase): + def _hooks(self, container, leaf): + return doc(**{container: {'push': {'post': { + 'operationId': 'webhook/push', + 'requestBody': {'content': {'application/json': {'schema': { + 'type': 'object', + 'properties': {'ref': leaf}, + }}}}, + 'responses': {}, + }}}}) + + def test_x_webhooks_payload_uses_response_rules(self): + base = self._hooks('x-webhooks', {'type': 'string'}) + head = doc(**{'x-webhooks': {'push': {'post': { + 'operationId': 'webhook/push', + 'requestBody': {'content': {'application/json': {'schema': { + 'type': 'object', 'properties': {}, + }}}}, + 'responses': {}, + }}}}) + self.assertEqual(rules(base, head), ['response-field-removed']) + + def test_webhook_removed(self): + base = self._hooks('webhooks', {'type': 'string'}) + self.assertEqual(rules(base, doc(webhooks={})), ['webhook-removed']) + + +class OutputTest(unittest.TestCase): + def test_summary_counts_additions(self): + base = get_op({'type': 'object'}, path='/a') + head = copy.deepcopy(base) + head['paths']['/b'] = {'get': {'operationId': 'b', 'responses': {}}} + head['components'] = {'schemas': {'new': {'type': 'object'}}} + summary = obc.compare(base, head)['summary'] + self.assertEqual(summary['added_operations'], ['GET /b']) + self.assertEqual(summary['added_schemas'], ['new']) + self.assertEqual(summary['total_operations'], 2) + + def test_findings_are_capped_and_flagged(self): + props = {f'p{i}': {'type': 'string'} for i in range(obc.MAX_FINDINGS + 25)} + base = get_op({'type': 'object', 'properties': props}) + head = get_op({'type': 'object', 'properties': {}}) + result = obc.compare(base, head) + self.assertEqual(len(result['findings']), obc.MAX_FINDINGS) + self.assertTrue(result['truncated']) + + def test_shared_schema_is_compared_once(self): + shared = {'$ref': '#/components/schemas/thing'} + base = doc(paths={ + '/a': {'get': {'operationId': 'a', 'responses': {'200': { + 'description': 'ok', 'content': {'application/json': {'schema': dict(shared)}}}}}}, + '/b': {'get': {'operationId': 'b', 'responses': {'200': { + 'description': 'ok', 'content': {'application/json': {'schema': dict(shared)}}}}}}, + }, schemas={'thing': { + 'type': 'object', 'properties': {'id': {'type': 'integer'}}, + }}) + head = copy.deepcopy(base) + head['components']['schemas']['thing']['properties'] = {} + # Both operations reference the same schema object, so the removal is + # reported once rather than once per reference site. + findings = obc.compare(base, head)['findings'] + self.assertEqual([f['rule'] for f in findings], ['response-field-removed']) + + def test_cli_round_trip(self): + import io + import json + import tempfile + + import yaml + + base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) + head = get_op({'type': 'object', 'properties': {}}) + with tempfile.TemporaryDirectory() as tmp: + paths = [] + for name, document in (('base.yaml', base), ('head.yaml', head)): + path = os.path.join(tmp, name) + with open(path, 'w', encoding='utf-8') as handle: + yaml.safe_dump(document, handle) + paths.append(path) + + captured, sys.stdout = sys.stdout, io.StringIO() + try: + self.assertEqual(obc.main(paths), 0) + payload = json.loads(sys.stdout.getvalue()) + finally: + sys.stdout = captured + + self.assertEqual( + [f['rule'] for f in payload['findings']], ['response-field-removed'] + ) + + +if __name__ == '__main__': + unittest.main() diff --git a/.github/scripts/test_select_openapi_prs.py b/.github/scripts/test_select_openapi_prs.py new file mode 100644 index 0000000000..cd03a87c9f --- /dev/null +++ b/.github/scripts/test_select_openapi_prs.py @@ -0,0 +1,158 @@ +#!/usr/bin/env python3 +"""Tests for select_openapi_prs. + +Run with: python3 -m unittest discover -s .github/scripts +""" + +import os +import sys +import unittest + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +import select_openapi_prs as sel # noqa: E402 + + +def pr(number, title, sha=None, created='2026-01-01T00:00:00Z'): + return { + 'number': number, + 'title': title, + 'headRefOid': sha if sha is not None else f'{number:040x}', + 'createdAt': created, + } + + +def as_dict(lines): + return dict(line.split('=', 1) for line in lines) + + +class SelectionTest(unittest.TestCase): + def test_selects_newest_per_title(self): + prs = [ + pr(1, sel.TITLE_30, created='2026-01-01T00:00:00Z'), + pr(2, sel.TITLE_30, created='2026-01-03T00:00:00Z'), + pr(3, sel.TITLE_31, created='2026-01-02T00:00:00Z'), + ] + out = as_dict(sel.outputs(prs)) + self.assertEqual(out['found'], 'true') + self.assertEqual(out['pr_30'], '2') + self.assertEqual(out['pr_31'], '3') + self.assertEqual(out['superseded'], '1') + + def test_ties_broken_by_number(self): + same = '2026-01-01T00:00:00Z' + out = as_dict(sel.outputs([ + pr(10, sel.TITLE_30, created=same), + pr(11, sel.TITLE_30, created=same), + ])) + self.assertEqual(out['pr_30'], '11') + self.assertEqual(out['superseded'], '10') + + def test_unrelated_bot_pull_requests_are_never_superseded(self): + prs = [ + pr(1, sel.TITLE_30, created='2026-01-01T00:00:00Z'), + pr(2, sel.TITLE_30, created='2026-01-02T00:00:00Z'), + pr(3, 'Bump some dependency'), + pr(4, 'Update OpenAPI 3.0 Descriptions '), + pr(5, 'update openapi 3.0 descriptions'), + pr(6, 'Update OpenAPI 3.2 Descriptions'), + pr(7, 'Revert "Update OpenAPI 3.0 Descriptions"'), + ] + out = as_dict(sel.outputs(prs)) + self.assertEqual(out['pr_30'], '2') + self.assertEqual(out['superseded'], '1') + + def test_only_unrelated_pull_requests_means_nothing_to_do(self): + out = as_dict(sel.outputs([pr(1, 'Bump some dependency')])) + self.assertEqual(out['found'], 'false') + self.assertEqual(out['pr_30'], '') + self.assertEqual(out['pr_31'], '') + self.assertEqual(out['superseded'], '') + + def test_empty_input(self): + out = as_dict(sel.outputs([])) + self.assertEqual(out['found'], 'false') + self.assertEqual(out['superseded'], '') + + def test_one_title_only(self): + out = as_dict(sel.outputs([pr(9, sel.TITLE_31)])) + self.assertEqual(out['found'], 'true') + self.assertEqual(out['pr_30'], '') + self.assertEqual(out['sha_30'], '') + self.assertEqual(out['pr_31'], '9') + self.assertEqual(out['superseded'], '') + + +class ShaPinningTest(unittest.TestCase): + def test_head_sha_is_emitted_for_each_selection(self): + out = as_dict(sel.outputs([ + pr(1, sel.TITLE_30, sha='a' * 40), + pr(2, sel.TITLE_31, sha='b' * 40), + ])) + self.assertEqual(out['sha_30'], 'a' * 40) + self.assertEqual(out['sha_31'], 'b' * 40) + + def test_sha_tracks_the_newest_pull_request_not_the_first(self): + out = as_dict(sel.outputs([ + pr(1, sel.TITLE_30, sha='a' * 40, created='2026-01-01T00:00:00Z'), + pr(2, sel.TITLE_30, sha='c' * 40, created='2026-02-01T00:00:00Z'), + ])) + self.assertEqual(out['pr_30'], '2') + self.assertEqual(out['sha_30'], 'c' * 40) + + def test_pull_request_without_head_sha_blocks_closing_its_siblings(self): + prs = [ + pr(1, sel.TITLE_30, created='2026-01-01T00:00:00Z'), + pr(2, sel.TITLE_30, sha='', created='2026-01-02T00:00:00Z'), + ] + out = as_dict(sel.outputs(prs)) + self.assertEqual(out['found'], 'false') + self.assertEqual(out['pr_30'], '') + self.assertEqual(out['sha_30'], '') + # #1 is older, but its replacement was never validated, so it must + # not be closed. + self.assertEqual(out['superseded'], '') + + def test_unpinnable_title_does_not_block_the_other_title(self): + prs = [ + pr(1, sel.TITLE_30, created='2026-01-01T00:00:00Z'), + pr(2, sel.TITLE_30, sha='', created='2026-01-02T00:00:00Z'), + pr(3, sel.TITLE_31, created='2026-01-01T00:00:00Z'), + pr(4, sel.TITLE_31, created='2026-01-02T00:00:00Z'), + ] + out = as_dict(sel.outputs(prs)) + self.assertEqual(out['found'], 'true') + self.assertEqual(out['pr_30'], '') + self.assertEqual(out['pr_31'], '4') + # Only the 3.1 sibling is superseded; the 3.0 pair is left alone. + self.assertEqual(out['superseded'], '3') + + def test_titles_flag_lists_recognised_titles(self): + self.assertEqual(list(sel.TITLES), [sel.TITLE_30, sel.TITLE_31]) + + def test_malformed_entries_are_ignored(self): + out = as_dict(sel.outputs([ + 'not a dict', + {'title': sel.TITLE_30}, + pr(5, sel.TITLE_30), + ])) + self.assertEqual(out['pr_30'], '5') + self.assertEqual(out['superseded'], '') + + +class CliTest(unittest.TestCase): + def test_titles_flag_prints_exact_titles(self): + import io + + captured, sys.stdout = sys.stdout, io.StringIO() + try: + self.assertEqual(sel.main(['--titles']), 0) + printed = sys.stdout.getvalue().splitlines() + finally: + sys.stdout = captured + + self.assertEqual(printed, [sel.TITLE_30, sel.TITLE_31]) + + +if __name__ == '__main__': + unittest.main() diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml index 7dafc61577..bb173583d3 100644 --- a/.github/workflows/auto-merge-openapi-updates.yml +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -1,19 +1,27 @@ name: Auto-merge OpenAPI description updates # Merges the newest open `github-openapi-bot` "Update OpenAPI 3.x Descriptions" -# PRs and closes the older superseded ones. +# pull requests and closes the older superseded ones. # -# Why a workflow instead of native auto-merge / the merge API: these PRs carry -# 100K+ line diffs across 64+ files. `PUT /pulls/{n}/merge` returns 502/504 and -# `GET /pulls/{n}/files` returns 422 on them, so the merge is done with plain -# git against a blobless clone. +# Why a workflow instead of native auto-merge / the merge API: these pull +# requests carry 100K+ line diffs across 64+ files. `PUT /pulls/{n}/merge` +# returns 502/504 and `GET /pulls/{n}/files` returns 422 on them, so the merge +# is done with plain git against a blobless clone. # -# This runs continuously. The safety properties of the manual process are -# preserved: lint must be green, the semantic breaking-change scan must come -# back clean, and a change summary is posted to the PR before merge. +# Safety properties: +# * Only pull requests titled exactly "Update OpenAPI 3.0 Descriptions" or +# "Update OpenAPI 3.1 Descriptions" are ever merged or closed, and a +# superseded pull request is re-checked immediately before it is closed. +# * Every step pins to the head commit SHA captured during selection. Branch +# names are never re-resolved, so a push that lands mid-run cannot slip an +# unvalidated commit into a merge. +# * Lint must be green, the compatibility scan must come back clean, and a +# change summary is posted to the pull request before merge. +# * Merges are held during a GHES release-candidate window. See the "Check +# for an active merge freeze" step. # -# Merges are additionally held during a GHES release-candidate window. See the -# "Check for an active merge freeze" step for how that is detected. +# Scripts are run from a checkout of this repository's default branch, never +# from the candidate pull request. on: schedule: @@ -28,6 +36,10 @@ on: permissions: contents: write pull-requests: write + # Reading check runs for the candidate commits. + checks: read + # Reading labels and issues for the merge-freeze signal. + issues: read concurrency: group: auto-merge-openapi-updates @@ -37,8 +49,13 @@ env: BOT_LOGIN: github-openapi-bot # Status checks that must be green before merging. CodeQL is deliberately # excluded: default setup only scans the `actions` language, it is not a - # required check on main, and it routinely reports `timed_out` on these PRs. + # required check on the default branch, and it routinely reports `timed_out` + # on these pull requests. REQUIRED_CHECKS: 'Lint OpenAPI 3.0 releases,Lint OpenAPI 3.1 releases' + # The non-dereferenced source of truth: compact, `$ref`-based, and every + # platform variant derives from it. + FILE_30: descriptions/api.github.com/api.github.com.yaml + FILE_31: descriptions-next/api.github.com/api.github.com.yaml jobs: auto-merge: @@ -48,7 +65,27 @@ jobs: status: ${{ steps.merge.outputs.status }} detail: ${{ steps.merge.outputs.detail }} steps: - - name: Select candidate PRs + - name: Checkout workflow scripts + uses: actions/checkout@v4 + with: + path: tools + fetch-depth: 1 + sparse-checkout: | + .github/scripts + requirements.txt + sparse-checkout-cone-mode: false + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.12' + + - name: Install dependencies + # Reuses the repository's existing pinned/hash-verified requirements, + # which already provide pyyaml for the linter workflow. + run: pip install --require-hashes -r tools/requirements.txt + + - name: Select candidate pull requests id: select env: GH_TOKEN: ${{ github.token }} @@ -56,49 +93,15 @@ jobs: run: | set -euo pipefail - open_prs=$(gh pr list \ + gh pr list \ --state open \ --author "$BOT_LOGIN" \ --limit 100 \ - --json number,title,headRefName,headRefOid,createdAt) - - select_newest() { - jq -r --arg t "$1" \ - '[.[] | select(.title == $t)] | sort_by(.createdAt) | last // empty' <<<"$open_prs" - } - - newest_30=$(select_newest 'Update OpenAPI 3.0 Descriptions') - newest_31=$(select_newest 'Update OpenAPI 3.1 Descriptions') - newest_30=${newest_30:-null} - newest_31=${newest_31:-null} + --json number,title,headRefOid,createdAt \ + | python3 tools/.github/scripts/select_openapi_prs.py >selection.txt - pr_30=$(jq -r '.number // empty' <<<"$newest_30") - pr_31=$(jq -r '.number // empty' <<<"$newest_31") - - if [ -z "$pr_30" ] && [ -z "$pr_31" ]; then - echo "No open $BOT_LOGIN description PRs. Nothing to do." - echo "found=false" >>"$GITHUB_OUTPUT" - exit 0 - fi - - { - echo "found=true" - echo "pr_30=$pr_30" - echo "pr_31=$pr_31" - echo "ref_30=$(jq -r '.headRefName // empty' <<<"$newest_30")" - echo "ref_31=$(jq -r '.headRefName // empty' <<<"$newest_31")" - } >>"$GITHUB_OUTPUT" - - # Every open bot PR that is not one of the two selected is superseded. - superseded=$(jq -r \ - --argjson keep30 "${pr_30:-0}" \ - --argjson keep31 "${pr_31:-0}" \ - '[.[].number | select(. != $keep30 and . != $keep31)] | join(" ")' \ - <<<"$open_prs") - echo "superseded=$superseded" >>"$GITHUB_OUTPUT" - - echo "3.0 PR: ${pr_30:-none} / 3.1 PR: ${pr_31:-none}" - echo "Superseded: ${superseded:-none}" + cat selection.txt + cat selection.txt >>"$GITHUB_OUTPUT" - name: Verify required checks are green id: checks @@ -106,15 +109,20 @@ jobs: env: GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} + SHA_30: ${{ steps.select.outputs.sha_30 }} + SHA_31: ${{ steps.select.outputs.sha_31 }} PR_30: ${{ steps.select.outputs.pr_30 }} PR_31: ${{ steps.select.outputs.pr_31 }} run: | set -euo pipefail blocked='' - for pr in $PR_30 $PR_31; do - [ -n "$pr" ] || continue - sha=$(gh pr view "$pr" --json headRefOid -q .headRefOid) + for pair in "$PR_30:$SHA_30" "$PR_31:$SHA_31"; do + pr="${pair%%:*}"; sha="${pair#*:}" + [ -n "$pr" ] && [ -n "$sha" ] || continue + + # Checks are read for the exact commit selected above, which is + # also the commit that is scanned and merged. runs=$(gh api "repos/$GH_REPO/commits/$sha/check-runs" --paginate \ -q '.check_runs[] | "\(.name)\t\(.status)\t\(.conclusion)"') @@ -141,47 +149,54 @@ jobs: - name: Check for an active merge freeze id: freeze - if: steps.select.outputs.found == 'true' + if: steps.checks.outputs.status == 'green' env: GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} - REF_30: ${{ steps.select.outputs.ref_30 }} - REF_31: ${{ steps.select.outputs.ref_31 }} + SHA_30: ${{ steps.select.outputs.sha_30 }} + SHA_31: ${{ steps.select.outputs.sha_31 }} DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} run: | set -euo pipefail # Around a GHES release candidate, hold description merges until the # coordinated release content is ready. A new GHES version appearing - # in the candidate PR before it exists on the default branch is the - # public signal that this window may be active. + # in the candidate before it exists on the default branch is the + # signal that this window may be active. reason='' # --- Signal 1: an explicit, human-controlled hold ---------------- # A label is the escape hatch that does not depend on inference, and - # lets Docs or the FR stop merges for any reason at all. - if gh label list --search 'merge-freeze' --json name -q '.[].name' | grep -qx 'merge-freeze'; then + # lets a human stop merges for any reason at all. + # Output is captured before matching: under `pipefail`, an early + # exiting `grep -q` could otherwise make the producer look failed + # and silently drop this signal. + labels=$(gh label list --search 'merge-freeze' --json name -q '.[].name') + if grep -qxF 'merge-freeze' <<<"$labels"; then frozen=$(gh issue list --label 'merge-freeze' --state open --json number,title \ - -q '.[] | "#\(.number) \(.title)"' | head -5) + -q '.[] | "#\(.number) \(.title)"' | sed -n '1,5p') if [ -n "$frozen" ]; then reason+="Open 'merge-freeze' issue(s): $(tr '\n' ';' <<<"$frozen") " fi fi # --- Signal 2: a GHES version not yet on the default branch ------ - ghes_on_main=$(gh api "repos/$GH_REPO/contents/descriptions?ref=$DEFAULT_BRANCH" \ - -q '.[].name' | grep '^ghes-' | sort -u) - - for ref in "$REF_30" "$REF_31"; do - [ -n "$ref" ] || continue + # Contents are read at the pinned commit, not at a branch name. + # `|| true` keeps "no GHES directories at all" as an empty set + # rather than a failed step. + ghes_on_default=$(gh api "repos/$GH_REPO/contents/descriptions?ref=$DEFAULT_BRANCH" \ + -q '.[].name' | grep '^ghes-' | sort -u || true) + + for sha in "$SHA_30" "$SHA_31"; do + [ -n "$sha" ] || continue for dir in descriptions descriptions-next; do - on_pr=$(gh api "repos/$GH_REPO/contents/$dir?ref=$ref" \ + on_pr=$(gh api "repos/$GH_REPO/contents/$dir?ref=$sha" \ -q '.[].name' 2>/dev/null | grep '^ghes-' | sort -u || true) [ -n "$on_pr" ] || continue - new=$(comm -13 <(echo "$ghes_on_main") <(echo "$on_pr") | tr '\n' ' ') + new=$(comm -13 <(echo "$ghes_on_default") <(echo "$on_pr") | tr '\n' ' ') if [ -n "${new// /}" ]; then - reason+="$ref/$dir introduces new GHES version(s): $new " + reason+="${sha:0:7}/$dir introduces new GHES version(s): $new " fi done done @@ -195,63 +210,78 @@ jobs: echo "No merge freeze detected." fi - - name: Checkout (blobless) + - name: Checkout repository (blobless) if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' uses: actions/checkout@v4 with: + path: repo # Blobless fetch keeps this off the ~4.6 GB full history while still # allowing real merges. Blobs for the touched files are fetched lazily. filter: blob:none fetch-depth: 0 token: ${{ github.token }} - - name: Set up Python + - name: Fetch and pin candidate commits + id: pin if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' - uses: actions/setup-python@v5 - with: - python-version: '3.12' + working-directory: repo + env: + SHA_30: ${{ steps.select.outputs.sha_30 }} + SHA_31: ${{ steps.select.outputs.sha_31 }} + PR_30: ${{ steps.select.outputs.pr_30 }} + PR_31: ${{ steps.select.outputs.pr_31 }} + run: | + set -euo pipefail - - name: Install PyYAML - if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' - # Reuses the repo's existing pinned/hash-verified requirements, which - # already provide pyyaml for the linter workflow. - run: pip install --require-hashes -r requirements.txt + # `refs/pull//head` is fetched rather than the head branch so the + # commit selected earlier is obtained by SHA. If the branch has since + # moved, that object is simply not present and the run stops instead + # of validating one commit and merging another. + for pair in "$PR_30:$SHA_30" "$PR_31:$SHA_31"; do + pr="${pair%%:*}"; sha="${pair#*:}" + [ -n "$pr" ] && [ -n "$sha" ] || continue + + git fetch --no-tags --filter=blob:none origin "refs/pull/$pr/head" + + if ! git cat-file -e "$sha^{commit}" 2>/dev/null; then + echo "::notice::#$pr no longer resolves to $sha; stopping this run." + echo "status=stale" >>"$GITHUB_OUTPUT" + exit 0 + fi + done + + echo "status=pinned" >>"$GITHUB_OUTPUT" - - name: Scan for breaking changes + - name: Scan for compatibility-breaking changes id: breaking - if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' + if: steps.pin.outputs.status == 'pinned' + working-directory: repo env: - REF_30: ${{ steps.select.outputs.ref_30 }} - REF_31: ${{ steps.select.outputs.ref_31 }} + SHA_30: ${{ steps.select.outputs.sha_30 }} + SHA_31: ${{ steps.select.outputs.sha_31 }} PR_30: ${{ steps.select.outputs.pr_30 }} PR_31: ${{ steps.select.outputs.pr_31 }} DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} run: | set -euo pipefail - # A semantic (parsed) comparison rather than a diff scan. The two are - # not equivalent: for a *request* body, ADDING to `required` is - # breaking, while for a *response* body, REMOVING from `required` is - # breaking. A scan of removed diff lines is structurally blind to the - # first case. Rules mirror the breaking-change list in - # the project's documented release-compatibility rules. + # A parsed comparison rather than a diff scan. The two are not + # equivalent: for a *request* body, ADDING to `required` is breaking, + # while for a *response* body, REMOVING from `required` is breaking. + # A scan of removed diff lines is structurally blind to the first + # case. Rules mirror the documented release-compatibility list. findings='' : >summary.md - for pair in \ - "$REF_30|descriptions/api.github.com/api.github.com.yaml|3.0|$PR_30" \ - "$REF_31|descriptions-next/api.github.com/api.github.com.yaml|3.1|$PR_31"; do - IFS='|' read -r ref file label pr <<<"$pair" - [ -n "$ref" ] || continue - - git fetch --no-tags --filter=blob:none origin "$ref":"refs/remotes/origin/$ref" + for triple in "$SHA_30|$FILE_30|3.0|$PR_30" "$SHA_31|$FILE_31|3.1|$PR_31"; do + IFS='|' read -r sha file label pr <<<"$triple" + [ -n "$sha" ] || continue - # api.github.com is the non-dereferenced source of truth: compact, - # $ref-based, and every platform variant derives from it. git show "origin/$DEFAULT_BRANCH:$file" >base.yaml - git show "origin/$ref:$file" >head.yaml + git show "$sha:$file" >head.yaml - python3 .github/scripts/openapi-breaking-changes.py base.yaml head.yaml >result.json + python3 "$GITHUB_WORKSPACE/tools/.github/scripts/openapi_breaking_changes.py" \ + base.yaml head.yaml >result.json count=$(jq '.findings | length' result.json) { @@ -260,16 +290,22 @@ jobs: jq -r '.summary | "- Operations: \(.total_operations) total, \(.added_operations | length) added", "- Schemas: \(.total_schemas) total, \(.added_schemas | length) added"' result.json - jq -r '.summary.added_operations[]? | " - `\(.)`"' result.json | head -40 + # `sed -n` rather than `head`: it consumes all input, so the + # producer is never killed by SIGPIPE under `pipefail`. + jq -r '.summary.added_operations[]? | " - `\(.)`"' result.json | sed -n '1,40p' echo } >>summary.md if [ "$count" -gt 0 ]; then - findings+="$label (#$pr): $count breaking finding(s). " + findings+="$label (#$pr): $count finding(s). " { - echo "#### :rotating_light: Breaking changes in $label" + echo "#### :rotating_light: Compatibility findings in $label" echo - jq -r '.findings[] | "- **\(.rule)**: \(.detail)"' result.json | head -50 + jq -r '.findings[] | "- **\(.rule)**: \(.detail)"' result.json | sed -n '1,50p' + if [ "$count" -gt 50 ] || [ "$(jq -r '.truncated' result.json)" = 'true' ]; then + echo + echo "_Finding list truncated; see the workflow run for the full output._" + fi echo } >>summary.md fi @@ -285,57 +321,98 @@ jobs: echo "status=clean" >>"$GITHUB_OUTPUT" fi - - name: Post change summary to PRs - if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' + - name: Post change summary to pull requests + if: steps.pin.outputs.status == 'pinned' + working-directory: repo env: GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} + SHA_30: ${{ steps.select.outputs.sha_30 }} + SHA_31: ${{ steps.select.outputs.sha_31 }} PR_30: ${{ steps.select.outputs.pr_30 }} PR_31: ${{ steps.select.outputs.pr_31 }} BREAKING: ${{ steps.breaking.outputs.status }} run: | set -euo pipefail - # Release notes are generated from PR descriptions. Auto-merging - # untouched bodies would remove useful context, so post the analysis - # the scanner already computed. - { - if [ "$BREAKING" = 'breaking' ]; then - echo "## :rotating_light: Auto-merge skipped: potential breaking changes" - else - echo "## Automated change summary" + # Release notes are generated from pull request descriptions. + # Auto-merging untouched bodies would remove useful context, so post + # the analysis the scanner already computed. + for pair in "$PR_30:$SHA_30" "$PR_31:$SHA_31"; do + pr="${pair%%:*}"; sha="${pair#*:}" + [ -n "$pr" ] && [ -n "$sha" ] || continue + + # The marker is per commit, so re-running for the same head does + # not add a duplicate comment. Comments are captured before + # matching so an early exiting `grep -q` cannot make `gh` look + # failed under `pipefail`. + marker="" + comments=$(gh pr view "$pr" --json comments -q '.comments[].body') + if grep -qF "$marker" <<<"$comments"; then + echo "Summary for $sha already posted on #$pr." + continue fi - echo - cat summary.md - echo "_Posted by the auto-merge workflow ([run](${GITHUB_SERVER_URL}/${GH_REPO}/actions/runs/${GITHUB_RUN_ID}))._" - } >comment.md - for pr in $PR_30 $PR_31; do - [ -n "$pr" ] || continue + { + echo "$marker" + if [ "$BREAKING" = 'breaking' ]; then + echo "## :rotating_light: Auto-merge skipped: potential breaking changes" + else + echo "## Automated change summary" + fi + echo + cat summary.md + echo "_Posted by the auto-merge workflow ([run](${GITHUB_SERVER_URL}/${GH_REPO}/actions/runs/${GITHUB_RUN_ID}))._" + } >comment.md + gh pr comment "$pr" --body-file comment.md || echo "::warning::Could not comment on #$pr" done - - name: Merge and close superseded PRs + - name: Merge and close superseded pull requests id: merge - if: >- - steps.checks.outputs.status == 'green' && - steps.freeze.outputs.status == 'clear' && - steps.breaking.outputs.status == 'clean' + if: steps.pin.outputs.status == 'pinned' && steps.breaking.outputs.status == 'clean' + working-directory: repo env: GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} + SHA_30: ${{ steps.select.outputs.sha_30 }} + SHA_31: ${{ steps.select.outputs.sha_31 }} PR_30: ${{ steps.select.outputs.pr_30 }} PR_31: ${{ steps.select.outputs.pr_31 }} - REF_30: ${{ steps.select.outputs.ref_30 }} - REF_31: ${{ steps.select.outputs.ref_31 }} SUPERSEDED: ${{ steps.select.outputs.superseded }} DRY_RUN: ${{ inputs.dry_run }} DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} run: | set -euo pipefail + # The recognised titles come from the selection script so there is a + # single source of truth for what this workflow is allowed to touch. + recognised_titles=$(python3 \ + "$GITHUB_WORKSPACE/tools/.github/scripts/select_openapi_prs.py" --titles) + + candidates='' + for pair in "$PR_30:$SHA_30" "$PR_31:$SHA_31"; do + pr="${pair%%:*}"; sha="${pair#*:}" + [ -n "$pr" ] && [ -n "$sha" ] || continue + candidates+="$pair " + done + + # Confirm every candidate still points at the commit that was checked + # and scanned, before anything is merged. A mismatch means the pull + # request moved mid-run, so nothing is merged or closed. + for pair in $candidates; do + pr="${pair%%:*}"; sha="${pair#*:}" + current=$(gh pr view "$pr" --json headRefOid -q .headRefOid) + if [ "$current" != "$sha" ]; then + echo "::notice::#$pr moved from $sha to $current since validation; stopping." + echo "status=stale" >>"$GITHUB_OUTPUT" + echo "detail=#$pr moved during the run" >>"$GITHUB_OUTPUT" + exit 0 + fi + done + if [ "$DRY_RUN" = "true" ]; then - echo "Dry run: would merge PRs ${PR_30:-none} and ${PR_31:-none}, close: ${SUPERSEDED:-none}" + echo "Dry run: would merge ${candidates:-none}, close: ${SUPERSEDED:-none}" echo "status=dry-run" >>"$GITHUB_OUTPUT" exit 0 fi @@ -347,11 +424,9 @@ jobs: merged='' # 3.0 first, then 3.1: they touch disjoint trees but this ordering # keeps the generated merge history readable. - for pair in "$PR_30:$REF_30" "$PR_31:$REF_31"; do - pr="${pair%%:*}"; ref="${pair#*:}" - [ -n "$pr" ] && [ -n "$ref" ] || continue - git fetch --no-tags origin "$ref":"refs/remotes/origin/$ref" --filter=blob:none - git merge --no-ff "origin/$ref" -m "Merge pull request #$pr from $ref" + for pair in $candidates; do + pr="${pair%%:*}"; sha="${pair#*:}" + git merge --no-ff "$sha" -m "Merge pull request #$pr ($sha)" merged+="#$pr " done @@ -361,6 +436,20 @@ jobs: fi for pr in $SUPERSEDED; do + # Re-read current state immediately before closing. Selection + # happened minutes earlier, and a pull request can be retitled, + # transferred or closed in between; only a still-open description + # update by the same author is closed. + meta=$(gh pr view "$pr" --json title,state,author \ + -q '[.title, .state, .author.login] | @tsv') + IFS=$'\t' read -r title state author <<<"$meta" + + if [ "$state" != 'OPEN' ] || [ "$author" != "$BOT_LOGIN" ] \ + || ! grep -qxF "$title" <<<"$recognised_titles"; then + echo "::notice::Leaving #$pr open: no longer a $BOT_LOGIN description update." + continue + fi + gh pr close "$pr" \ --comment "Superseded by the newer OpenAPI description update(s) ${merged:-just merged}. Closing automatically." \ || echo "::warning::Failed to close #$pr" @@ -373,15 +462,26 @@ jobs: if: >- failure() || steps.breaking.outputs.status == 'breaking' || - steps.freeze.outputs.status == 'frozen' + steps.freeze.outputs.status == 'frozen' || + steps.pin.outputs.status == 'stale' || + steps.merge.outputs.status == 'stale' + env: + FREEZE_STATUS: ${{ steps.freeze.outputs.status }} + FREEZE_DETAIL: ${{ steps.freeze.outputs.detail }} + BREAKING_STATUS: ${{ steps.breaking.outputs.status }} + BREAKING_DETAIL: ${{ steps.breaking.outputs.detail }} + PIN_STATUS: ${{ steps.pin.outputs.status }} + MERGE_STATUS: ${{ steps.merge.outputs.status }} run: | { echo "## Auto-merge did not proceed" echo - if [ "${{ steps.freeze.outputs.status }}" = "frozen" ]; then - echo "A merge freeze was detected: ${{ steps.freeze.outputs.detail }}" - elif [ "${{ steps.breaking.outputs.status }}" = "breaking" ]; then - echo "Potential breaking changes were detected: ${{ steps.breaking.outputs.detail }}" + if [ "$FREEZE_STATUS" = 'frozen' ]; then + echo "A merge freeze was detected: $FREEZE_DETAIL" + elif [ "$BREAKING_STATUS" = 'breaking' ]; then + echo "Potential breaking changes were detected: $BREAKING_DETAIL" + elif [ "$PIN_STATUS" = 'stale' ] || [ "$MERGE_STATUS" = 'stale' ]; then + echo "A candidate pull request was updated mid-run, so nothing was merged." else echo "The workflow failed before merging. Review the failed step above." fi diff --git a/.github/workflows/scripts-tests.yml b/.github/workflows/scripts-tests.yml new file mode 100644 index 0000000000..cf5ff53701 --- /dev/null +++ b/.github/workflows/scripts-tests.yml @@ -0,0 +1,33 @@ +--- +name: Test workflow scripts +permissions: + contents: read + +on: + push: + paths: + - '.github/scripts/**' + - '.github/workflows/scripts-tests.yml' + - 'requirements.txt' + pull_request: + paths: + - '.github/scripts/**' + - '.github/workflows/scripts-tests.yml' + - 'requirements.txt' + workflow_dispatch: + +jobs: + unittest: + name: Unit tests + runs-on: ubuntu-latest + steps: + - name: Checkout Code + uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + name: Install Python + with: + python-version: '3.12' + - run: pip install --require-hashes -r requirements.txt + name: Install dependencies + - run: python3 -m unittest discover --start-directory .github/scripts --verbose + name: Run script tests diff --git a/.gitignore b/.gitignore index 40b878db5b..4b7a81db08 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ -node_modules/ \ No newline at end of file +node_modules/ +__pycache__/ From 2d8105d008954c895a16e6c09ad52075c673d209 Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Wed, 19 Aug 2026 11:00:43 -0500 Subject: [PATCH 09/13] Close remaining auto-merge gaps found in review Restrict the merge to description content. A candidate is only merged when every file it changes is under descriptions/ or descriptions-next/, so a pull request carrying the right title and author but touching workflows, scripts or anything else is left for a human. Re-validate a candidate completely before merging, not just its head SHA: it must still be open, still authored by the description bot, still target the default branch, and still carry the exact title of the group it was selected for. Keep superseded pull requests associated with the title they were superseded under. The selector now emits superseded_30 and superseded_31 separately and exposes a single title through --title, and the close step skips any pull request whose live title, base, author or state no longer matches, so a pull request retitled or retargeted mid-run is left open. Extend the compatibility scan: * parameters declared on a path item are compared, with an operation-level parameter of the same name and location taking precedence; * a newly added required parameter is reported; * a required requestBody added where there was none is reported; * removing the whole webhooks or x-webhooks container now reports each removed webhook instead of skipping the comparison; * removing an HTTP method from a surviving webhook is reported. Also report a failed required check and an out-of-scope candidate in the job step summary, so every hold has a visible reason. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/scripts/openapi_breaking_changes.py | 151 +++++++++++++----- .github/scripts/select_openapi_prs.py | 32 ++-- .../scripts/test_openapi_breaking_changes.py | 113 +++++++++++++ .github/scripts/test_select_openapi_prs.py | 56 +++++-- .../workflows/auto-merge-openapi-updates.yml | 139 +++++++++++----- 5 files changed, 391 insertions(+), 100 deletions(-) diff --git a/.github/scripts/openapi_breaking_changes.py b/.github/scripts/openapi_breaking_changes.py index 3f0e6ece1b..ac7b9ffe6f 100644 --- a/.github/scripts/openapi_breaking_changes.py +++ b/.github/scripts/openapi_breaking_changes.py @@ -29,7 +29,9 @@ including refs inside `parameters`, `responses` and `requestBody`. * Comparison recurses through `properties`, `items` (arrays, including the 3.1 tuple form), `additionalProperties`, `allOf`/`oneOf`/`anyOf`, and - operation parameter schemas. + operation parameter schemas. Parameters declared on the path item are + included, with an operation-level parameter of the same name and location + taking precedence, as the specification requires. * `allOf` members are merged into an effective schema so that inherited properties and `required` entries participate in the comparison. * Inline schemas are covered because the walk starts from every operation's @@ -48,8 +50,9 @@ applies the asymmetric rules: a removed property, a removed `required` entry and any change to a declared `type` are breaking on a response, while a newly required property, a removed union member and a narrowed `type` are breaking -on a request. Parameters are compared with the request rules; webhook -payloads with the response rules, because consumers receive them. +on a request. Parameters are compared with the request rules, including a +newly added required parameter; webhook payloads with the response rules, +because consumers receive them. Known limits (deliberately not claimed as covered): external/file `$ref` targets are compared by pointer string only; `not`, `discriminator`, @@ -402,10 +405,15 @@ def _compare_unions(self, base, head, where, direction): # -- document comparison --------------------------------------------- - def parameters(self, doc, op): - """Resolved parameters for an operation, keyed by `(in, name)`.""" + def parameters(self, doc, op, shared=()): + """Resolved parameters for an operation, keyed by `(in, name)`. + + Path-item parameters apply to every operation under that path, and an + operation-level parameter with the same name and location overrides + the shared one, so the shared list is applied first. + """ out = {} - for param in op.get('parameters') or []: + for param in list(shared or []) + list(op.get('parameters') or []): resolved, _ = self.resolve(doc, param) if isinstance(resolved, dict) and resolved.get('name'): out[(resolved.get('in'), resolved['name'])] = resolved @@ -426,7 +434,15 @@ def json_schemas(self, doc, container): } return resolved, schemas - def compare_operation(self, base_op, head_op, label, request_direction='request'): + def compare_operation( + self, + base_op, + head_op, + label, + request_direction='request', + base_shared=(), + head_shared=(), + ): base_id, head_id = base_op.get('operationId'), head_op.get('operationId') if base_id and head_id and base_id != head_id: self.report( @@ -434,8 +450,8 @@ def compare_operation(self, base_op, head_op, label, request_direction='request' f'{label}: operationId {base_id!r} -> {head_id!r}', ) - base_params = self.parameters(self.base, base_op) - head_params = self.parameters(self.head, head_op) + base_params = self.parameters(self.base, base_op, base_shared) + head_params = self.parameters(self.head, head_op, head_shared) removed_path_params = sorted( name for (loc, name) in set(base_params) - set(head_params) if loc == 'path' ) @@ -445,6 +461,23 @@ def compare_operation(self, base_op, head_op, label, request_direction='request' f'{label}: path parameter(s) gone: {removed_path_params}', ) + added_required_params = sorted( + (str(loc), name) + for (loc, name) in set(head_params) - set(base_params) + # Path parameters are excluded: they are required by definition, + # a rename is already reported as `url-parameter-renamed`, and a + # genuinely new one changes the path template, which surfaces as a + # removed operation instead. + if loc != 'path' and head_params[(loc, name)].get('required') + ) + for location, name in added_required_params: + # An existing caller cannot satisfy a requirement it has never + # heard of, so this is as breaking as making a parameter required. + self.report( + 'parameter-added-required', + f'{label}: new required {location} parameter {name!r}', + ) + for key in sorted( set(base_params) & set(head_params), key=lambda k: (str(k[0]), k[1]) ): @@ -469,27 +502,36 @@ def compare_operation(self, base_op, head_op, label, request_direction='request' base_body, base_body_schemas = self.json_schemas(self.base, base_op.get('requestBody')) head_body, head_body_schemas = self.json_schemas(self.head, head_op.get('requestBody')) - if isinstance(base_body, dict) and base_body: - if not isinstance(head_body, dict) or not head_body: - # The operation no longer accepts a body at all, so callers - # that send one may now be rejected. + base_has_body = isinstance(base_body, dict) and bool(base_body) + head_has_body = isinstance(head_body, dict) and bool(head_body) + if base_has_body and not head_has_body: + # The operation no longer accepts a body at all, so callers that + # send one may now be rejected. + self.report( + 'request-body-removed', + f'{label}: requestBody was removed', + ) + elif head_has_body and head_body.get('required'): + if not base_has_body: + # Introducing a required body imposes the same new obligation + # on every existing caller as making an optional one required. self.report( - 'request-body-removed', - f'{label}: requestBody was removed', + 'requestbody-now-required', + f'{label}: a required requestBody was added', + ) + elif not base_body.get('required'): + self.report( + 'requestbody-now-required', + f'{label}: requestBody became required', + ) + if base_has_body and head_has_body: + # Mirrors the response side: a caller that submits JSON breaks + # when that media type stops being accepted. + for media in sorted(set(base_body_schemas) - set(head_body_schemas)): + self.report( + 'request-media-type-removed', + f'{label} request body: {media} was removed', ) - else: - if head_body.get('required') and not base_body.get('required'): - self.report( - 'requestbody-now-required', - f'{label}: requestBody became required', - ) - # Mirrors the response side: a caller that submits JSON breaks - # when that media type stops being accepted. - for media in sorted(set(base_body_schemas) - set(head_body_schemas)): - self.report( - 'request-media-type-removed', - f'{label} request body: {media} was removed', - ) for media in sorted(set(base_body_schemas) & set(head_body_schemas)): self.compare_schema( base_body_schemas[media], @@ -533,13 +575,21 @@ def compare_operation(self, base_op, head_op, label, request_direction='request' def compare_paths(self): base_ops, head_ops = operations(self.base), operations(self.head) + base_shared = shared_parameters(self.base) + head_shared = shared_parameters(self.head) for key in sorted(base_ops): path, method = key label = f'{method.upper()} {path}' if key not in head_ops: self.report('operation-removed', f'{label} was removed') continue - self.compare_operation(base_ops[key], head_ops[key], label) + self.compare_operation( + base_ops[key], + head_ops[key], + label, + base_shared=base_shared.get(path, ()), + head_shared=head_shared.get(path, ()), + ) def compare_webhooks(self): """Walk `webhooks` (3.1) and `x-webhooks` (3.0) payload schemas. @@ -549,9 +599,13 @@ def compare_webhooks(self): """ for container in ('webhooks', 'x-webhooks'): base_hooks = self.base.get(container) - head_hooks = self.head.get(container) - if not isinstance(base_hooks, dict) or not isinstance(head_hooks, dict): + if not isinstance(base_hooks, dict): continue + head_hooks = self.head.get(container) + if not isinstance(head_hooks, dict): + # Dropping the container removes every webhook in it, so it is + # treated as empty rather than skipped. + head_hooks = {} for name in sorted(base_hooks): base_item, _ = self.resolve(self.base, base_hooks[name]) head_item, _ = self.resolve(self.head, head_hooks.get(name)) @@ -562,13 +616,22 @@ def compare_webhooks(self): continue for method in METHODS: base_op, head_op = base_item.get(method), head_item.get(method) - if isinstance(base_op, dict) and isinstance(head_op, dict): - self.compare_operation( - base_op, - head_op, - f'webhook {name}', - request_direction='response', + if not isinstance(base_op, dict): + continue + if not isinstance(head_op, dict): + self.report( + 'webhook-operation-removed', + f'webhook {name}: {method.upper()} was removed', ) + continue + self.compare_operation( + base_op, + head_op, + f'webhook {name}', + request_direction='response', + base_shared=base_item.get('parameters') or (), + head_shared=head_item.get('parameters') or (), + ) def compare_components(self): """Report `components/schemas` removals (rule 9). @@ -603,6 +666,20 @@ def operations(doc): return out +def shared_parameters(doc): + """Map `path` -> the path item's own `parameters` list. + + These apply to every operation under the path unless the operation + declares a parameter with the same name and location. + """ + out = {} + paths = doc.get('paths') + for path, item in (paths if isinstance(paths, dict) else {}).items(): + if isinstance(item, dict) and isinstance(item.get('parameters'), list): + out[path] = item['parameters'] + return out + + def summarize(base, head): """Non-breaking additions, used for the informational PR summary.""" base_ops, head_ops = operations(base), operations(head) diff --git a/.github/scripts/select_openapi_prs.py b/.github/scripts/select_openapi_prs.py index 0d770e8c78..06d9bed09d 100644 --- a/.github/scripts/select_openapi_prs.py +++ b/.github/scripts/select_openapi_prs.py @@ -17,6 +17,9 @@ * A title only contributes superseded pull requests when its own newest pull request was selected, so nothing is closed unless a validated replacement for that exact title is being merged. +* Superseded numbers are emitted per title rather than in one list, so the + workflow can confirm a pull request still carries the title it was + selected under before closing it. * The head commit SHA of each selected pull request is emitted alongside its number, so every later step can pin to the exact commit that was inspected instead of resolving a branch name again. @@ -29,6 +32,9 @@ TITLE_30 = 'Update OpenAPI 3.0 Descriptions' TITLE_31 = 'Update OpenAPI 3.1 Descriptions' TITLES = (TITLE_30, TITLE_31) +# Output suffix -> exact title. The workflow reads a title through `--title` +# so the two files never drift apart. +GROUPS = (('30', TITLE_30), ('31', TITLE_31)) def _sort_key(pr): @@ -42,7 +48,8 @@ def select(pull_requests): A title only contributes superseded pull requests when its own newest pull request was actually selected. Nothing is ever closed on the basis - of a replacement that was not validated. + of a replacement that was not validated. The superseded numbers are keyed + by title so the caller can re-check that association before closing. """ recognised = [ pr @@ -51,7 +58,7 @@ def select(pull_requests): ] selected = {} - superseded = [] + superseded = {title: [] for title in TITLES} for title in TITLES: candidates = [pr for pr in recognised if pr['title'] == title] if not candidates: @@ -63,38 +70,39 @@ def select(pull_requests): if not newest.get('headRefOid'): continue selected[title] = newest - superseded.extend( + superseded[title] = sorted( pr['number'] for pr in candidates if pr['number'] != newest['number'] ) - return selected, sorted(superseded) + return selected, superseded def outputs(pull_requests): selected, superseded = select(pull_requests) lines = [f'found={"true" if selected else "false"}'] - for suffix, title in (('30', TITLE_30), ('31', TITLE_31)): + for suffix, title in GROUPS: pr = selected.get(title) lines.append(f'pr_{suffix}={pr["number"] if pr else ""}') lines.append(f'sha_{suffix}={pr["headRefOid"] if pr else ""}') + lines.append( + f'superseded_{suffix}=' + ' '.join(str(n) for n in superseded[title]) + ) - lines.append('superseded=' + ' '.join(str(n) for n in superseded)) return lines def main(argv=None): parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) parser.add_argument( - '--titles', - action='store_true', - help='print the recognised titles, one per line, and exit', + '--title', + choices=[suffix for suffix, _ in GROUPS], + help='print the exact recognised title for one output group and exit', ) args = parser.parse_args(argv) - if args.titles: - for title in TITLES: - print(title) + if args.title: + print(dict(GROUPS)[args.title]) return 0 payload = json.load(sys.stdin) diff --git a/.github/scripts/test_openapi_breaking_changes.py b/.github/scripts/test_openapi_breaking_changes.py index 269a3362b5..84155496f8 100644 --- a/.github/scripts/test_openapi_breaking_changes.py +++ b/.github/scripts/test_openapi_breaking_changes.py @@ -202,6 +202,98 @@ def test_request_body_became_required(self): head = post_op({'type': 'object'}, required=True) self.assertEqual(rules(base, head), ['requestbody-now-required']) + def test_new_required_request_body_added(self): + head = post_op({'type': 'object'}, required=True) + base = copy.deepcopy(head) + del base['paths']['/thing']['post']['requestBody'] + self.assertEqual(rules(base, head), ['requestbody-now-required']) + self.assertEqual( + details(base, head, 'requestbody-now-required'), + ['POST /thing: a required requestBody was added'], + ) + + def test_added_required_path_parameter_is_not_double_reported(self): + base = get_op({'type': 'object'}, path='/thing/{id}') + base['paths']['/thing/{id}']['get']['parameters'] = [ + {'name': 'id', 'in': 'path', 'required': True, 'schema': {'type': 'string'}} + ] + head = copy.deepcopy(base) + head['paths']['/thing/{id}']['get']['parameters'] = [ + {'name': 'thing_id', 'in': 'path', 'required': True, 'schema': {'type': 'string'}} + ] + self.assertEqual(rules(base, head), ['url-parameter-renamed']) + + def test_path_item_required_parameter_added(self): + base = get_op({'type': 'object'}) + head = copy.deepcopy(base) + head['paths']['/thing']['parameters'] = [ + {'name': 'since', 'in': 'query', 'required': True, 'schema': {'type': 'string'}} + ] + self.assertEqual(rules(base, head), ['parameter-added-required']) + + def test_path_item_parameter_became_required(self): + base = get_op({'type': 'object'}) + base['paths']['/thing']['parameters'] = [ + {'name': 'q', 'in': 'query', 'schema': {'type': 'string'}} + ] + head = copy.deepcopy(base) + head['paths']['/thing']['parameters'][0]['required'] = True + self.assertEqual(rules(base, head), ['parameter-now-required']) + + def test_moving_a_parameter_to_the_operation_is_not_breaking(self): + param = {'name': 'q', 'in': 'query', 'schema': {'type': 'string'}} + base = get_op({'type': 'object'}) + base['paths']['/thing']['parameters'] = [param] + head = get_op({'type': 'object'}) + head['paths']['/thing']['get']['parameters'] = [dict(param)] + self.assertEqual(rules(base, head), []) + + def test_operation_parameter_overrides_the_path_item_one(self): + base = get_op({'type': 'object'}) + base['paths']['/thing']['parameters'] = [ + {'name': 'q', 'in': 'query', 'required': True, 'schema': {'type': 'string'}} + ] + base['paths']['/thing']['get']['parameters'] = [ + {'name': 'q', 'in': 'query', 'schema': {'type': 'string'}} + ] + head = copy.deepcopy(base) + # The operation keeps it optional, so the required path-item entry + # must not be what gets compared. + head['paths']['/thing']['get']['parameters'][0]['schema'] = {'type': 'string'} + self.assertEqual(rules(base, head), []) + + def test_new_required_query_parameter(self): + base = get_op({'type': 'object'}) + head = copy.deepcopy(base) + head['paths']['/thing']['get']['parameters'] = [ + {'name': 'since', 'in': 'query', 'required': True, 'schema': {'type': 'string'}} + ] + self.assertEqual(rules(base, head), ['parameter-added-required']) + self.assertEqual( + details(base, head, 'parameter-added-required'), + ["GET /thing: new required query parameter 'since'"], + ) + + def test_new_optional_parameter_is_not_breaking(self): + base = get_op({'type': 'object'}) + head = copy.deepcopy(base) + head['paths']['/thing']['get']['parameters'] = [ + {'name': 'since', 'in': 'query', 'schema': {'type': 'string'}} + ] + self.assertEqual(rules(base, head), []) + + def test_new_required_parameter_behind_a_ref(self): + base = get_op({'type': 'object'}) + base['components'] = {'parameters': { + 'since': {'name': 'since', 'in': 'header', 'required': True, + 'schema': {'type': 'string'}}, + }} + head = copy.deepcopy(base) + head['paths']['/thing']['get']['parameters'] = [ + {'$ref': '#/components/parameters/since'} + ] + self.assertEqual(rules(base, head), ['parameter-added-required']) + def test_request_body_removed_entirely(self): base = post_op({'type': 'object'}, required=True) head = copy.deepcopy(base) @@ -672,6 +764,27 @@ def test_webhook_removed(self): base = self._hooks('webhooks', {'type': 'string'}) self.assertEqual(rules(base, doc(webhooks={})), ['webhook-removed']) + def test_whole_webhook_container_removed(self): + base = self._hooks('webhooks', {'type': 'string'}) + self.assertEqual(rules(base, doc()), ['webhook-removed']) + + def test_x_webhooks_container_removed(self): + base = self._hooks('x-webhooks', {'type': 'string'}) + self.assertEqual(rules(base, doc()), ['webhook-removed']) + + def test_webhook_method_removed(self): + base = self._hooks('webhooks', {'type': 'string'}) + head = doc(webhooks={'push': {'description': 'still documented'}}) + self.assertEqual(rules(base, head), ['webhook-operation-removed']) + self.assertEqual( + details(base, head, 'webhook-operation-removed'), + ['webhook push: POST was removed'], + ) + + def test_adding_a_webhook_is_not_breaking(self): + head = self._hooks('webhooks', {'type': 'string'}) + self.assertEqual(rules(doc(), head), []) + class OutputTest(unittest.TestCase): def test_summary_counts_additions(self): diff --git a/.github/scripts/test_select_openapi_prs.py b/.github/scripts/test_select_openapi_prs.py index cd03a87c9f..1ff3f9280c 100644 --- a/.github/scripts/test_select_openapi_prs.py +++ b/.github/scripts/test_select_openapi_prs.py @@ -37,7 +37,8 @@ def test_selects_newest_per_title(self): self.assertEqual(out['found'], 'true') self.assertEqual(out['pr_30'], '2') self.assertEqual(out['pr_31'], '3') - self.assertEqual(out['superseded'], '1') + self.assertEqual(out['superseded_30'], '1') + self.assertEqual(out['superseded_31'], '') def test_ties_broken_by_number(self): same = '2026-01-01T00:00:00Z' @@ -46,7 +47,7 @@ def test_ties_broken_by_number(self): pr(11, sel.TITLE_30, created=same), ])) self.assertEqual(out['pr_30'], '11') - self.assertEqual(out['superseded'], '10') + self.assertEqual(out['superseded_30'], '10') def test_unrelated_bot_pull_requests_are_never_superseded(self): prs = [ @@ -60,19 +61,22 @@ def test_unrelated_bot_pull_requests_are_never_superseded(self): ] out = as_dict(sel.outputs(prs)) self.assertEqual(out['pr_30'], '2') - self.assertEqual(out['superseded'], '1') + self.assertEqual(out['superseded_30'], '1') + self.assertEqual(out['superseded_31'], '') def test_only_unrelated_pull_requests_means_nothing_to_do(self): out = as_dict(sel.outputs([pr(1, 'Bump some dependency')])) self.assertEqual(out['found'], 'false') self.assertEqual(out['pr_30'], '') self.assertEqual(out['pr_31'], '') - self.assertEqual(out['superseded'], '') + self.assertEqual(out['superseded_30'], '') + self.assertEqual(out['superseded_31'], '') def test_empty_input(self): out = as_dict(sel.outputs([])) self.assertEqual(out['found'], 'false') - self.assertEqual(out['superseded'], '') + self.assertEqual(out['superseded_30'], '') + self.assertEqual(out['superseded_31'], '') def test_one_title_only(self): out = as_dict(sel.outputs([pr(9, sel.TITLE_31)])) @@ -80,7 +84,7 @@ def test_one_title_only(self): self.assertEqual(out['pr_30'], '') self.assertEqual(out['sha_30'], '') self.assertEqual(out['pr_31'], '9') - self.assertEqual(out['superseded'], '') + self.assertEqual(out['superseded_31'], '') class ShaPinningTest(unittest.TestCase): @@ -111,7 +115,7 @@ def test_pull_request_without_head_sha_blocks_closing_its_siblings(self): self.assertEqual(out['sha_30'], '') # #1 is older, but its replacement was never validated, so it must # not be closed. - self.assertEqual(out['superseded'], '') + self.assertEqual(out['superseded_30'], '') def test_unpinnable_title_does_not_block_the_other_title(self): prs = [ @@ -125,10 +129,21 @@ def test_unpinnable_title_does_not_block_the_other_title(self): self.assertEqual(out['pr_30'], '') self.assertEqual(out['pr_31'], '4') # Only the 3.1 sibling is superseded; the 3.0 pair is left alone. - self.assertEqual(out['superseded'], '3') + self.assertEqual(out['superseded_30'], '') + self.assertEqual(out['superseded_31'], '3') - def test_titles_flag_lists_recognised_titles(self): - self.assertEqual(list(sel.TITLES), [sel.TITLE_30, sel.TITLE_31]) + def test_superseded_numbers_stay_with_their_title(self): + prs = [ + pr(1, sel.TITLE_30, created='2026-01-01T00:00:00Z'), + pr(2, sel.TITLE_30, created='2026-01-04T00:00:00Z'), + pr(3, sel.TITLE_31, created='2026-01-02T00:00:00Z'), + pr(4, sel.TITLE_31, created='2026-01-03T00:00:00Z'), + ] + _, superseded = sel.select(prs) + # The association is what lets the workflow confirm a pull request + # still carries the title it was superseded under before closing it. + self.assertEqual(superseded[sel.TITLE_30], [1]) + self.assertEqual(superseded[sel.TITLE_31], [3]) def test_malformed_entries_are_ignored(self): out = as_dict(sel.outputs([ @@ -137,21 +152,34 @@ def test_malformed_entries_are_ignored(self): pr(5, sel.TITLE_30), ])) self.assertEqual(out['pr_30'], '5') - self.assertEqual(out['superseded'], '') + self.assertEqual(out['superseded_30'], '') class CliTest(unittest.TestCase): - def test_titles_flag_prints_exact_titles(self): + def _run(self, argv): import io captured, sys.stdout = sys.stdout, io.StringIO() try: - self.assertEqual(sel.main(['--titles']), 0) + code = sel.main(argv) printed = sys.stdout.getvalue().splitlines() finally: sys.stdout = captured + return code, printed + + def test_title_flag_prints_the_exact_title_for_a_group(self): + self.assertEqual(self._run(['--title', '30']), (0, [sel.TITLE_30])) + self.assertEqual(self._run(['--title', '31']), (0, [sel.TITLE_31])) - self.assertEqual(printed, [sel.TITLE_30, sel.TITLE_31]) + def test_title_flag_rejects_an_unknown_group(self): + import io + + captured, sys.stderr = sys.stderr, io.StringIO() + try: + with self.assertRaises(SystemExit): + sel.main(['--title', '32']) + finally: + sys.stderr = captured if __name__ == '__main__': diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml index bb173583d3..ceba2f7026 100644 --- a/.github/workflows/auto-merge-openapi-updates.yml +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -11,7 +11,8 @@ name: Auto-merge OpenAPI description updates # Safety properties: # * Only pull requests titled exactly "Update OpenAPI 3.0 Descriptions" or # "Update OpenAPI 3.1 Descriptions" are ever merged or closed, and a -# superseded pull request is re-checked immediately before it is closed. +# superseded pull request is re-checked against the exact title it was +# superseded under, immediately before it is closed. # * Every step pins to the head commit SHA captured during selection. Branch # names are never re-resolved, so a push that lands mid-run cannot slip an # unvalidated commit into a merge. @@ -379,40 +380,86 @@ jobs: SHA_31: ${{ steps.select.outputs.sha_31 }} PR_30: ${{ steps.select.outputs.pr_30 }} PR_31: ${{ steps.select.outputs.pr_31 }} - SUPERSEDED: ${{ steps.select.outputs.superseded }} + SUPERSEDED_30: ${{ steps.select.outputs.superseded_30 }} + SUPERSEDED_31: ${{ steps.select.outputs.superseded_31 }} DRY_RUN: ${{ inputs.dry_run }} DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} run: | set -euo pipefail - # The recognised titles come from the selection script so there is a - # single source of truth for what this workflow is allowed to touch. - recognised_titles=$(python3 \ - "$GITHUB_WORKSPACE/tools/.github/scripts/select_openapi_prs.py" --titles) + # Titles come from the selection script so there is a single source + # of truth for what this workflow is allowed to touch, and each + # group is matched against its own exact title rather than against + # "either of the two". + selector="$GITHUB_WORKSPACE/tools/.github/scripts/select_openapi_prs.py" + title_30=$(python3 "$selector" --title 30) + title_31=$(python3 "$selector" --title 31) + + title_for() { + case "$1" in + 30) printf '%s\n' "$title_30" ;; + 31) printf '%s\n' "$title_31" ;; + esac + } candidates='' - for pair in "$PR_30:$SHA_30" "$PR_31:$SHA_31"; do - pr="${pair%%:*}"; sha="${pair#*:}" + for entry in "30:$PR_30:$SHA_30" "31:$PR_31:$SHA_31"; do + rest="${entry#*:}" + pr="${rest%%:*}"; sha="${rest#*:}" [ -n "$pr" ] && [ -n "$sha" ] || continue - candidates+="$pair " + candidates+="$entry " done - # Confirm every candidate still points at the commit that was checked - # and scanned, before anything is merged. A mismatch means the pull - # request moved mid-run, so nothing is merged or closed. - for pair in $candidates; do - pr="${pair%%:*}"; sha="${pair#*:}" - current=$(gh pr view "$pr" --json headRefOid -q .headRefOid) + # Confirm every candidate is still exactly what was validated: the + # same commit, still open, still authored by the description bot, + # still titled as a description update, and still targeting the + # default branch. Any mismatch means the pull request changed + # mid-run, so nothing is merged or closed. + for entry in $candidates; do + suffix="${entry%%:*}"; rest="${entry#*:}" + pr="${rest%%:*}"; sha="${rest#*:}" + meta=$(gh pr view "$pr" \ + --json headRefOid,state,author,title,baseRefName \ + -q '[.headRefOid, .state, .author.login, .baseRefName, .title] | @tsv') + IFS=$'\t' read -r current state author base title <<<"$meta" + if [ "$current" != "$sha" ]; then echo "::notice::#$pr moved from $sha to $current since validation; stopping." echo "status=stale" >>"$GITHUB_OUTPUT" echo "detail=#$pr moved during the run" >>"$GITHUB_OUTPUT" exit 0 fi + + if [ "$state" != 'OPEN' ] || [ "$author" != "$BOT_LOGIN" ] \ + || [ "$base" != "$DEFAULT_BRANCH" ] \ + || [ "$title" != "$(title_for "$suffix")" ]; then + echo "::notice::#$pr is no longer an open $BOT_LOGIN description update targeting $DEFAULT_BRANCH; stopping." + echo "status=stale" >>"$GITHUB_OUTPUT" + echo "detail=#$pr changed during the run" >>"$GITHUB_OUTPUT" + exit 0 + fi + done + + # The compatibility scan covers the description files, so a merge is + # only allowed to bring in description content. A pull request that + # touches workflows, scripts or anything else is left for a human, + # even when it carries the right title and author. + for entry in $candidates; do + rest="${entry#*:}" + pr="${rest%%:*}"; sha="${rest#*:}" + base_commit=$(git merge-base "origin/$DEFAULT_BRANCH" "$sha") + outside=$(git diff --name-only "$base_commit" "$sha" \ + | grep -v -E '^descriptions(-next)?/' | sed -n '1,10p' || true) + if [ -n "$outside" ]; then + echo "::notice::#$pr changes files outside the description directories; stopping." + echo "status=out-of-scope" >>"$GITHUB_OUTPUT" + echo "detail=#$pr changes $(tr '\n' ' ' <<<"$outside")" >>"$GITHUB_OUTPUT" + exit 0 + fi done if [ "$DRY_RUN" = "true" ]; then - echo "Dry run: would merge ${candidates:-none}, close: ${SUPERSEDED:-none}" + echo "Dry run: would merge ${candidates:-none}, close: ${SUPERSEDED_30:-} ${SUPERSEDED_31:-}" echo "status=dry-run" >>"$GITHUB_OUTPUT" exit 0 fi @@ -424,8 +471,9 @@ jobs: merged='' # 3.0 first, then 3.1: they touch disjoint trees but this ordering # keeps the generated merge history readable. - for pair in $candidates; do - pr="${pair%%:*}"; sha="${pair#*:}" + for entry in $candidates; do + rest="${entry#*:}" + pr="${rest%%:*}"; sha="${rest#*:}" git merge --no-ff "$sha" -m "Merge pull request #$pr ($sha)" merged+="#$pr " done @@ -435,25 +483,33 @@ jobs: echo "Merged and pushed: $merged" fi - for pr in $SUPERSEDED; do - # Re-read current state immediately before closing. Selection - # happened minutes earlier, and a pull request can be retitled, - # transferred or closed in between; only a still-open description - # update by the same author is closed. - meta=$(gh pr view "$pr" --json title,state,author \ - -q '[.title, .state, .author.login] | @tsv') - IFS=$'\t' read -r title state author <<<"$meta" + # Re-read current state immediately before closing. Selection + # happened minutes earlier, and a pull request can be retitled, + # retargeted, transferred or closed in between. A pull request is + # only closed when it is still open, still authored by the + # description bot, still targeting the default branch, and still + # carries the exact title it was superseded under. + close_superseded() { + expected="$1"; shift + for pr in "$@"; do + meta=$(gh pr view "$pr" --json title,state,author,baseRefName \ + -q '[.state, .author.login, .baseRefName, .title] | @tsv') + IFS=$'\t' read -r state author base title <<<"$meta" + + if [ "$state" != 'OPEN' ] || [ "$author" != "$BOT_LOGIN" ] \ + || [ "$base" != "$DEFAULT_BRANCH" ] || [ "$title" != "$expected" ]; then + echo "::notice::Leaving #$pr open: it no longer matches the pull request that was superseded." + continue + fi - if [ "$state" != 'OPEN' ] || [ "$author" != "$BOT_LOGIN" ] \ - || ! grep -qxF "$title" <<<"$recognised_titles"; then - echo "::notice::Leaving #$pr open: no longer a $BOT_LOGIN description update." - continue - fi + gh pr close "$pr" \ + --comment "Superseded by the newer OpenAPI description update(s) ${merged:-just merged}. Closing automatically." \ + || echo "::warning::Failed to close #$pr" + done + } - gh pr close "$pr" \ - --comment "Superseded by the newer OpenAPI description update(s) ${merged:-just merged}. Closing automatically." \ - || echo "::warning::Failed to close #$pr" - done + close_superseded "$title_30" ${SUPERSEDED_30:-} + close_superseded "$title_31" ${SUPERSEDED_31:-} echo "status=merged" >>"$GITHUB_OUTPUT" echo "detail=Merged $merged" >>"$GITHUB_OUTPUT" @@ -461,25 +517,34 @@ jobs: - name: Summarize exceptions if: >- failure() || + steps.checks.outputs.status == 'blocked' || steps.breaking.outputs.status == 'breaking' || steps.freeze.outputs.status == 'frozen' || steps.pin.outputs.status == 'stale' || - steps.merge.outputs.status == 'stale' + steps.merge.outputs.status == 'stale' || + steps.merge.outputs.status == 'out-of-scope' env: + CHECKS_STATUS: ${{ steps.checks.outputs.status }} + CHECKS_DETAIL: ${{ steps.checks.outputs.detail }} FREEZE_STATUS: ${{ steps.freeze.outputs.status }} FREEZE_DETAIL: ${{ steps.freeze.outputs.detail }} BREAKING_STATUS: ${{ steps.breaking.outputs.status }} BREAKING_DETAIL: ${{ steps.breaking.outputs.detail }} PIN_STATUS: ${{ steps.pin.outputs.status }} MERGE_STATUS: ${{ steps.merge.outputs.status }} + MERGE_DETAIL: ${{ steps.merge.outputs.detail }} run: | { echo "## Auto-merge did not proceed" echo - if [ "$FREEZE_STATUS" = 'frozen' ]; then + if [ "$CHECKS_STATUS" = 'blocked' ]; then + echo "Required checks were not green: $CHECKS_DETAIL" + elif [ "$FREEZE_STATUS" = 'frozen' ]; then echo "A merge freeze was detected: $FREEZE_DETAIL" elif [ "$BREAKING_STATUS" = 'breaking' ]; then echo "Potential breaking changes were detected: $BREAKING_DETAIL" + elif [ "$MERGE_STATUS" = 'out-of-scope' ]; then + echo "A candidate changed files outside the description directories: $MERGE_DETAIL" elif [ "$PIN_STATUS" = 'stale' ] || [ "$MERGE_STATUS" = 'stale' ]; then echo "A candidate pull request was updated mid-run, so nothing was merged." else From 518dbf889a40c6593e618f806033dae7766e8471 Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Wed, 19 Aug 2026 11:50:32 -0500 Subject: [PATCH 10/13] Fail closed on unverifiable state and widen scan rules Addresses the remaining review findings on the auto-merge workflow. Selection and merge: * Both checkouts are pinned to the default branch, so a manual run started from another ref cannot supply the selector, the scanner or the merge baseline. * A failed change-summary comment now stops the run before the merge step instead of warning and continuing. * Failures while closing superseded pull requests are collected and reported, and the merge outcome is recorded before that phase so a close failure cannot be summarised as a failure before merging. * The GHES release-window check and the description-only file check no longer treat an unreadable answer as a clear one. Only a genuine 404 counts as an empty listing; anything else holds the merge. Compatibility scan: * A removed operationId is reported, not only a renamed one. * A response schema that drops its declared type is reported; the request direction is unaffected, because dropping it widens what is accepted. Comments now state the scanned surface precisely: the scan reads the two dotcom source descriptions, and the other trees rest on the required lint and the posted summary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/scripts/openapi_breaking_changes.py | 34 +++-- .../scripts/test_openapi_breaking_changes.py | 27 ++++ .../workflows/auto-merge-openapi-updates.yml | 119 ++++++++++++++---- requirements.txt | 3 +- 4 files changed, 152 insertions(+), 31 deletions(-) diff --git a/.github/scripts/openapi_breaking_changes.py b/.github/scripts/openapi_breaking_changes.py index ac7b9ffe6f..00c6d4656c 100644 --- a/.github/scripts/openapi_breaking_changes.py +++ b/.github/scripts/openapi_breaking_changes.py @@ -5,7 +5,7 @@ ----- Implements the breaking-change list documented for these descriptions: - 1. An operationId name has been changed. + 1. An operationId name has been changed or removed. 2. A URL parameter name has been changed. 3. An operation has been removed from the description. 4. A `required: true` has been added to the requestBody. @@ -48,11 +48,12 @@ body, *adding* to `required` is breaking; for a *response* body, *removing* from `required` is breaking. The walk therefore carries a direction and applies the asymmetric rules: a removed property, a removed `required` entry -and any change to a declared `type` are breaking on a response, while a newly -required property, a removed union member and a narrowed `type` are breaking -on a request. Parameters are compared with the request rules, including a -newly added required parameter; webhook payloads with the response rules, -because consumers receive them. +and any change to a declared `type`, including dropping the declaration +altogether, are breaking on a response, while a newly required property, a +removed union member and a narrowed `type` are breaking on a request. +Parameters are compared with the request rules, including a newly added +required parameter; webhook payloads with the response rules, because +consumers receive them. Known limits (deliberately not claimed as covered): external/file `$ref` targets are compared by pointer string only; `not`, `discriminator`, @@ -280,7 +281,17 @@ def compare_schema(self, base, head, where, direction): def _compare_types(self, base, head, where, direction): base_types, head_types = self.types(base), self.types(head) - if not base_types or not head_types: + if not base_types: + return + if not head_types: + # Dropping the declaration widens what may be sent, which a + # request producer survives, but it removes the guarantee a + # response consumer was written against. + if direction == 'response': + self.report( + 'type-declaration-removed', + f'{where}: declared type {sorted(base_types)} was removed', + ) return # A response consumer breaks on any type change, including a widened # set it was never written to handle. A request producer only breaks @@ -444,7 +455,14 @@ def compare_operation( head_shared=(), ): base_id, head_id = base_op.get('operationId'), head_op.get('operationId') - if base_id and head_id and base_id != head_id: + if base_id and not head_id: + # Generators derive client method names from `operationId`, so + # dropping one renames the generated method just as a change does. + self.report( + 'operationid-removed', + f'{label}: operationId {base_id!r} was removed', + ) + elif base_id and head_id and base_id != head_id: self.report( 'operationid-changed', f'{label}: operationId {base_id!r} -> {head_id!r}', diff --git a/.github/scripts/test_openapi_breaking_changes.py b/.github/scripts/test_openapi_breaking_changes.py index 84155496f8..6423c3be80 100644 --- a/.github/scripts/test_openapi_breaking_changes.py +++ b/.github/scripts/test_openapi_breaking_changes.py @@ -89,6 +89,18 @@ def test_operationid_changed(self): head['paths']['/thing']['get']['operationId'] = 'thing/fetch' self.assertEqual(rules(base, head), ['operationid-changed']) + def test_operationid_removed(self): + base = get_op({'type': 'object'}) + head = copy.deepcopy(base) + del head['paths']['/thing']['get']['operationId'] + self.assertEqual(rules(base, head), ['operationid-removed']) + + def test_operationid_added_is_not_breaking(self): + head = get_op({'type': 'object'}) + base = copy.deepcopy(head) + del base['paths']['/thing']['get']['operationId'] + self.assertEqual(rules(base, head), []) + def test_path_parameter_renamed_behind_a_ref(self): base = doc(paths={ '/repos/{owner}': { @@ -197,6 +209,11 @@ def test_json_media_type_removed(self): class RequestDirectionTest(unittest.TestCase): + def test_request_type_declaration_removed_is_not_breaking(self): + base = post_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) + head = post_op({'type': 'object', 'properties': {'id': {'description': 'id'}}}) + self.assertEqual(rules(base, head), []) + def test_request_body_became_required(self): base = post_op({'type': 'object'}, required=False) head = post_op({'type': 'object'}, required=True) @@ -389,6 +406,16 @@ def test_adding_a_response_required_field_is_not_breaking(self): schema = {'type': 'object', 'properties': {'id': {'type': 'integer'}}} self.assertEqual(rules(get_op(schema), get_op(dict(schema, required=['id']))), []) + def test_response_type_declaration_removed(self): + base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) + head = get_op({'type': 'object', 'properties': {'id': {'description': 'id'}}}) + self.assertEqual(rules(base, head), ['type-declaration-removed']) + + def test_response_type_declaration_added_is_not_breaking(self): + head = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) + base = get_op({'type': 'object', 'properties': {'id': {'description': 'id'}}}) + self.assertEqual(rules(base, head), []) + def test_response_type_change_in_either_direction(self): base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) head = get_op({ diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml index ceba2f7026..69342f16e5 100644 --- a/.github/workflows/auto-merge-openapi-updates.yml +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -17,12 +17,19 @@ name: Auto-merge OpenAPI description updates # names are never re-resolved, so a push that lands mid-run cannot slip an # unvalidated commit into a merge. # * Lint must be green, the compatibility scan must come back clean, and a -# change summary is posted to the pull request before merge. +# change summary is posted to the pull request before merge. The scan +# covers the two dotcom source descriptions only; see the FILE_30/FILE_31 +# comment for the exact surface and what that leaves to lint. # * Merges are held during a GHES release-candidate window. See the "Check # for an active merge freeze" step. # # Scripts are run from a checkout of this repository's default branch, never # from the candidate pull request. +# +# Note: the merge is pushed with `GITHUB_TOKEN`, which by design does not +# trigger `on: push` workflows, so the default branch is not re-linted by the +# push itself. The same content was linted on the pull request head before +# merge. on: schedule: @@ -53,8 +60,15 @@ env: # required check on the default branch, and it routinely reports `timed_out` # on these pull requests. REQUIRED_CHECKS: 'Lint OpenAPI 3.0 releases,Lint OpenAPI 3.1 releases' - # The non-dereferenced source of truth: compact, `$ref`-based, and every - # platform variant derives from it. + # The two non-dereferenced dotcom descriptions: compact, `$ref`-based, and + # upstream of the calendar-versioned dotcom variants, which are produced by + # applying the changeset extensions declared in these files. These are the + # only files the compatibility scan reads, and it compares the described + # surface, not those changeset extensions. A bot pull request also updates + # the `ghec`, `github.ae` and `ghes-*` trees and their dereferenced forms, + # which are version-filtered rather than derived, so a change confined to + # one of those is covered by the required lint and the posted change + # summary, not by the scan. FILE_30: descriptions/api.github.com/api.github.com.yaml FILE_31: descriptions-next/api.github.com/api.github.com.yaml @@ -69,6 +83,10 @@ jobs: - name: Checkout workflow scripts uses: actions/checkout@v4 with: + # Pinned to the default branch on purpose. A `workflow_dispatch` run + # can be started from any ref, and without this the write-enabled job + # would run the selector and scanner from that ref instead. + ref: ${{ github.event.repository.default_branch }} path: tools fetch-depth: 1 sparse-checkout: | @@ -184,16 +202,40 @@ jobs: # --- Signal 2: a GHES version not yet on the default branch ------ # Contents are read at the pinned commit, not at a branch name. - # `|| true` keeps "no GHES directories at all" as an empty set - # rather than a failed step. - ghes_on_default=$(gh api "repos/$GH_REPO/contents/descriptions?ref=$DEFAULT_BRANCH" \ - -q '.[].name' | grep '^ghes-' | sort -u || true) + # These reads fail closed: a transient API error that looked like + # "no GHES directories" would silently drop the signal, so only a + # genuine 404 (the directory does not exist at that ref) becomes an + # empty listing. Anything else holds the merge. + errors=$(mktemp) + + ghes_dirs() { + local ref="$1" dir="$2" listing + if listing=$(gh api "repos/$GH_REPO/contents/$dir?ref=$ref" -q '.[].name' 2>"$errors"); then + grep '^ghes-' <<<"$listing" | sort -u || true + return 0 + fi + grep -q 'HTTP 404' "$errors" && return 0 + cat "$errors" >&2 + return 1 + } + + hold_unverifiable() { + echo "::error::Could not read $1 at $2 to check for a GHES release window; holding." + echo "status=frozen" >>"$GITHUB_OUTPUT" + echo "detail=Could not verify the GHES release state for $1 at $2" >>"$GITHUB_OUTPUT" + exit 0 + } + + if ! ghes_on_default=$(ghes_dirs "$DEFAULT_BRANCH" descriptions); then + hold_unverifiable descriptions "$DEFAULT_BRANCH" + fi for sha in "$SHA_30" "$SHA_31"; do [ -n "$sha" ] || continue for dir in descriptions descriptions-next; do - on_pr=$(gh api "repos/$GH_REPO/contents/$dir?ref=$sha" \ - -q '.[].name' 2>/dev/null | grep '^ghes-' | sort -u || true) + if ! on_pr=$(ghes_dirs "$sha" "$dir"); then + hold_unverifiable "$dir" "${sha:0:7}" + fi [ -n "$on_pr" ] || continue new=$(comm -13 <(echo "$ghes_on_default") <(echo "$on_pr") | tr '\n' ' ') if [ -n "${new// /}" ]; then @@ -215,6 +257,10 @@ jobs: if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' uses: actions/checkout@v4 with: + # Pinned like the tools checkout so the scan baseline and the merge + # target are provably the same branch, whatever ref a manual run + # was started from. + ref: ${{ github.event.repository.default_branch }} path: repo # Blobless fetch keeps this off the ~4.6 GB full history while still # allowing real merges. Blobs for the touched files are fetched lazily. @@ -366,7 +412,10 @@ jobs: echo "_Posted by the auto-merge workflow ([run](${GITHUB_SERVER_URL}/${GH_REPO}/actions/runs/${GITHUB_RUN_ID}))._" } >comment.md - gh pr comment "$pr" --body-file comment.md || echo "::warning::Could not comment on #$pr" + # No fallback: the summary is part of the merge contract, so a + # failure here must stop the run before the merge step. The + # per-commit marker makes a later retry safe. + gh pr comment "$pr" --body-file comment.md done - name: Merge and close superseded pull requests @@ -440,16 +489,19 @@ jobs: fi done - # The compatibility scan covers the description files, so a merge is - # only allowed to bring in description content. A pull request that - # touches workflows, scripts or anything else is left for a human, - # even when it carries the right title and author. + # A merge is only allowed to bring in description content. A pull + # request that touches workflows, scripts or anything else is left + # for a human, even when it carries the right title and author. + # This is a wider surface than the compatibility scan reads, so the + # remainder rests on the required lint and the posted summary. for entry in $candidates; do rest="${entry#*:}" pr="${rest%%:*}"; sha="${rest#*:}" base_commit=$(git merge-base "origin/$DEFAULT_BRANCH" "$sha") - outside=$(git diff --name-only "$base_commit" "$sha" \ - | grep -v -E '^descriptions(-next)?/' | sed -n '1,10p' || true) + # Captured before filtering so a failed `git diff` aborts the step + # instead of looking like "nothing outside the descriptions". + changed=$(git diff --name-only "$base_commit" "$sha") + outside=$(grep -v -E '^descriptions(-next)?/' <<<"$changed" | sed -n '1,10p' || true) if [ -n "$outside" ]; then echo "::notice::#$pr changes files outside the description directories; stopping." echo "status=out-of-scope" >>"$GITHUB_OUTPUT" @@ -483,17 +535,28 @@ jobs: echo "Merged and pushed: $merged" fi + # Recorded immediately after the push: everything below can fail, + # and none of it may hide the fact that the merge already landed. + echo "status=merged" >>"$GITHUB_OUTPUT" + echo "detail=Merged $merged" >>"$GITHUB_OUTPUT" + # Re-read current state immediately before closing. Selection # happened minutes earlier, and a pull request can be retitled, # retargeted, transferred or closed in between. A pull request is # only closed when it is still open, still authored by the # description bot, still targeting the default branch, and still # carries the exact title it was superseded under. + close_failed='' close_superseded() { expected="$1"; shift for pr in "$@"; do - meta=$(gh pr view "$pr" --json title,state,author,baseRefName \ - -q '[.state, .author.login, .baseRefName, .title] | @tsv') + # A failed read is treated exactly like a failed close: the pull + # request is still open and still needs a human. + if ! meta=$(gh pr view "$pr" --json title,state,author,baseRefName \ + -q '[.state, .author.login, .baseRefName, .title] | @tsv'); then + close_failed+="#$pr " + continue + fi IFS=$'\t' read -r state author base title <<<"$meta" if [ "$state" != 'OPEN' ] || [ "$author" != "$BOT_LOGIN" ] \ @@ -504,15 +567,20 @@ jobs: gh pr close "$pr" \ --comment "Superseded by the newer OpenAPI description update(s) ${merged:-just merged}. Closing automatically." \ - || echo "::warning::Failed to close #$pr" + || close_failed+="#$pr " done } close_superseded "$title_30" ${SUPERSEDED_30:-} close_superseded "$title_31" ${SUPERSEDED_31:-} - echo "status=merged" >>"$GITHUB_OUTPUT" - echo "detail=Merged $merged" >>"$GITHUB_OUTPUT" + # A superseded pull request left open would be picked up as a + # candidate by the next run, so this must not pass silently. + if [ -n "$close_failed" ]; then + echo "close_failed=$close_failed" >>"$GITHUB_OUTPUT" + echo "::error::Failed to close superseded pull request(s): $close_failed" + exit 1 + fi - name: Summarize exceptions if: >- @@ -533,9 +601,14 @@ jobs: PIN_STATUS: ${{ steps.pin.outputs.status }} MERGE_STATUS: ${{ steps.merge.outputs.status }} MERGE_DETAIL: ${{ steps.merge.outputs.detail }} + MERGE_CLOSE_FAILED: ${{ steps.merge.outputs.close_failed }} run: | { - echo "## Auto-merge did not proceed" + if [ -n "$MERGE_CLOSE_FAILED" ]; then + echo "## Auto-merge needs follow-up" + else + echo "## Auto-merge did not proceed" + fi echo if [ "$CHECKS_STATUS" = 'blocked' ]; then echo "Required checks were not green: $CHECKS_DETAIL" @@ -547,6 +620,8 @@ jobs: echo "A candidate changed files outside the description directories: $MERGE_DETAIL" elif [ "$PIN_STATUS" = 'stale' ] || [ "$MERGE_STATUS" = 'stale' ]; then echo "A candidate pull request was updated mid-run, so nothing was merged." + elif [ -n "$MERGE_CLOSE_FAILED" ]; then + echo "The merge succeeded ($MERGE_DETAIL), but superseded pull request(s) $MERGE_CLOSE_FAILED could not be closed. Close them manually, otherwise the next run treats one of them as a candidate." else echo "The workflow failed before merging. Review the failed step above." fi diff --git a/requirements.txt b/requirements.txt index 4c57eea273..b3635a7f51 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,6 @@ # Generated by pip-compile with --generate-hashes -# Used by .github/workflows/linter.yml for yamllint installation with hash verification +# Used by .github/workflows/linter.yml, auto-merge-openapi-updates.yml and +# scripts-tests.yml for installation with hash verification pathspec==1.0.4 \ --hash=sha256:0210e2ae8a21a9137c0d470578cb0e595af87edaa6ebf12ff176f14a02e0e645 \ --hash=sha256:fb6ae2fd4e7c921a165808a552060e722767cfa526f99ca5156ed2ce45a5c723 From adbbefabe3ad26ef0cc1f0129c18c44dadf7aeae Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Wed, 19 Aug 2026 16:11:07 -0500 Subject: [PATCH 11/13] Drop duplicate compatibility scan, gate scope before commenting The OpenAPI descriptions published here are generated artifacts. A required status check already runs a purpose-built breaking-change differ against the authoritative source, across every release and on dereferenced files, and blocks publication when it fires. Re-checking the published output here duplicated that gate with a narrower, weaker implementation that would drift from it over time, so remove it. Removing the scan also removes the only third-party dependency, so drop both pip-install steps and restore requirements.txt. Move the out-of-scope file gate into the pin step so it runs before any comment is posted. A candidate that touches files outside the description directories no longer collects a comment announcing a merge that will not happen, and the merge step no longer recomputes the same diff. The commit is pinned and revalidated before merge, so evaluating the gate earlier inspects exactly the content that gets merged. Replace the scanner-derived comment with a plain inventory of the changed files, which is what the comment was actually useful for. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b022cc6-cf92-4a1a-a283-cc4bcd70288c --- .github/scripts/openapi_breaking_changes.py | 739 --------------- .../scripts/test_openapi_breaking_changes.py | 882 ------------------ .../workflows/auto-merge-openapi-updates.yml | 186 ++-- .github/workflows/scripts-tests.yml | 4 - requirements.txt | 3 +- 5 files changed, 56 insertions(+), 1758 deletions(-) delete mode 100644 .github/scripts/openapi_breaking_changes.py delete mode 100644 .github/scripts/test_openapi_breaking_changes.py diff --git a/.github/scripts/openapi_breaking_changes.py b/.github/scripts/openapi_breaking_changes.py deleted file mode 100644 index 00c6d4656c..0000000000 --- a/.github/scripts/openapi_breaking_changes.py +++ /dev/null @@ -1,739 +0,0 @@ -#!/usr/bin/env python3 -"""Detect breaking changes between two OpenAPI description files. - -Scope ------ -Implements the breaking-change list documented for these descriptions: - - 1. An operationId name has been changed or removed. - 2. A URL parameter name has been changed. - 3. An operation has been removed from the description. - 4. A `required: true` has been added to the requestBody. - 5. A parameter has been added to the required list for a requestBody. - 6. A field has been removed from a response body. - 7. A field type has changed in a response body. - 8. A field has been removed from the required list in a response body. - -Plus two structural cases that make the above unrepresentable: - - 9. A schema definition has been removed from `components/schemas`. - 10. An enum value has been removed. - -Comparison model ----------------- -The descriptions are `$ref`-heavy, so a comparison that only looks at the -top level of `components/schemas` misses most of the surface. This module -instead walks the document: - -* Local `$ref` pointers (`#/components/...`) are resolved before comparing, - including refs inside `parameters`, `responses` and `requestBody`. -* Comparison recurses through `properties`, `items` (arrays, including the - 3.1 tuple form), `additionalProperties`, `allOf`/`oneOf`/`anyOf`, and - operation parameter schemas. Parameters declared on the path item are - included, with an operation-level parameter of the same name and location - taking precedence, as the specification requires. -* `allOf` members are merged into an effective schema so that inherited - properties and `required` entries participate in the comparison. -* Inline schemas are covered because the walk starts from every operation's - parameters, request body and 2xx responses rather than from - `components/schemas`. -* JSON media types are tracked on both sides of an operation, so dropping - JSON support from a request body or a response is reported, as is removing - a request body outright. -* Traversal is memoised on the identity of the *resolved* base and head nodes - plus the direction, so a widely shared schema is compared once instead of - once per reference site, and recursive schemas terminate. - -Direction matters, and line-based diffing gets it wrong. For a *request* -body, *adding* to `required` is breaking; for a *response* body, *removing* -from `required` is breaking. The walk therefore carries a direction and -applies the asymmetric rules: a removed property, a removed `required` entry -and any change to a declared `type`, including dropping the declaration -altogether, are breaking on a response, while a newly required property, a -removed union member and a narrowed `type` are breaking on a request. -Parameters are compared with the request rules, including a newly added -required parameter; webhook payloads with the response rules, because -consumers receive them. - -Known limits (deliberately not claimed as covered): external/file `$ref` -targets are compared by pointer string only; `not`, `discriminator`, -`patternProperties`, `nullable`, `format`, `default` and numeric/length -constraint tightening are not evaluated; only 2xx responses and JSON media -types are compared, so a non-JSON media type or a 4xx/5xx shape change is -invisible; parameter serialisation (`style`, `explode`) is not compared; and -`oneOf`/`anyOf` members are matched by `$ref` target or `title` when -available and by position otherwise, so a reordered anonymous union can -produce imprecise locations. A finding is reported at the first location it -is reached from, not at every location that shares the schema. - -A general-purpose differ was preferred but does not fit: the widely used -Go/Java differs target OpenAPI 3.0 only, while this repository must also -compare 3.1 descriptions, and the Python dependency set here is -hash-pinned. - -Exit status is 0 unless the inputs cannot be read; findings go to stdout as -JSON and the caller decides policy. -""" - -import argparse -import json -import sys - -import yaml - -try: - from yaml import CSafeLoader as Loader -except ImportError: # pragma: no cover - libyaml is present on ubuntu-latest - from yaml import SafeLoader as Loader - -METHODS = ('get', 'put', 'post', 'delete', 'patch', 'options', 'head', 'trace') - -# Findings are advisory output posted to a pull request. Past some volume the -# list stops being reviewable, and an unbounded list on a pathological diff -# would dominate the job log. -MAX_FINDINGS = 250 - - -def load(path): - with open(path, encoding='utf-8') as handle: - return yaml.load(handle, Loader=Loader) - - -def _pointer(doc, ref): - """Resolve a local JSON pointer such as `#/components/schemas/repo`.""" - node = doc - for token in ref[2:].split('/'): - token = token.replace('~1', '/').replace('~0', '~') - if isinstance(node, dict): - if token not in node: - return None - node = node[token] - elif isinstance(node, list): - try: - node = node[int(token)] - except (ValueError, IndexError): - return None - else: - return None - return node - - -class Comparator: - """Walks a base and a head description in parallel collecting findings.""" - - def __init__(self, base, head): - self.base = base - self.head = head - self.findings = [] - self._seen_findings = set() - self._visited = set() - self._effective = {} - - # -- helpers --------------------------------------------------------- - - def report(self, rule, detail): - key = (rule, detail) - if key in self._seen_findings: - return - self._seen_findings.add(key) - if len(self.findings) < MAX_FINDINGS: - self.findings.append({'rule': rule, 'detail': detail}) - - def resolve(self, doc, node): - """Follow local `$ref` chains. - - Returns `(node, ref)` where `ref` is the last pointer followed, or - `(None, ref)` when a local pointer does not resolve. - """ - ref = None - seen = set() - while isinstance(node, dict) and isinstance(node.get('$ref'), str): - ref = node['$ref'] - if not ref.startswith('#/'): - # External target: not loaded, so treat it as opaque and let - # the caller compare pointer strings. - return node, ref - if ref in seen: - return None, ref - seen.add(ref) - node = _pointer(doc, ref) - if node is None: - return None, ref - return node, ref - - def effective(self, doc, schema): - """Merge `allOf` members into a single comparable schema view.""" - if not isinstance(schema, dict) or 'allOf' not in schema: - return schema if isinstance(schema, dict) else {} - - cached = self._effective.get(id(schema)) - if cached is not None: - return cached - - merged = {k: v for k, v in schema.items() if k != 'allOf'} - # Seed the cache before recursing so a self-referential `allOf` - # terminates instead of recursing forever. - self._effective[id(schema)] = merged - - props = dict(merged.get('properties') or {}) - required = list(merged.get('required') or []) - - members = schema['allOf'] - for member in members if isinstance(members, list) else []: - resolved, _ = self.resolve(doc, member) - if not isinstance(resolved, dict): - continue - resolved = self.effective(doc, resolved) - for key, value in resolved.items(): - if key == 'properties' and isinstance(value, dict): - props.update(value) - elif key == 'required' and isinstance(value, list): - required.extend(value) - elif key not in merged: - merged[key] = value - - if props: - merged['properties'] = props - if required: - merged['required'] = required - return merged - - @staticmethod - def types(schema): - declared = schema.get('type') - if isinstance(declared, str): - return {declared} - if isinstance(declared, list): - return {t for t in declared if isinstance(t, str)} - return set() - - @staticmethod - def enum_values(schema): - values = schema.get('enum') - if not isinstance(values, list): - return set() - return {json.dumps(v, sort_keys=True) for v in values} - - def variant_key(self, doc, member, index): - """Stable identity for a `oneOf`/`anyOf` member.""" - resolved, ref = self.resolve(doc, member) - if ref: - return ref - if isinstance(resolved, dict) and isinstance(resolved.get('title'), str): - return f'title:{resolved["title"]}' - return f'index:{index}' - - def variants(self, doc, schema, keyword): - members = schema.get(keyword) - if not isinstance(members, list): - return {} - out = {} - for index, member in enumerate(members): - out.setdefault(self.variant_key(doc, member, index), member) - return out - - # -- schema comparison ----------------------------------------------- - - def compare_schema(self, base, head, where, direction): - base_ref = head_ref = None - if isinstance(base, dict): - base, base_ref = self.resolve(self.base, base) - if isinstance(head, dict): - head, head_ref = self.resolve(self.head, head) - - # Memoise on the *resolved* nodes. Every `$ref` to the same target - # resolves to the same object, so a widely shared schema is compared - # once instead of once per reference site, and recursive schemas - # terminate. - key = (id(base), id(head), direction) - if key in self._visited: - return - self._visited.add(key) - - if base_ref and not base_ref.startswith('#/'): - # Both sides are external pointers; only the target can be compared. - if base_ref != head_ref: - self.report( - 'external-ref-changed', - f'{where}: {base_ref} -> {head_ref or "removed"}', - ) - return - - if not isinstance(base, dict): - return - if not isinstance(head, dict): - self.report( - 'schema-removed', - f'{where}: {base_ref or "schema"} no longer resolves', - ) - return - - base = self.effective(self.base, base) - head = self.effective(self.head, head) - - self._compare_types(base, head, where, direction) - self._compare_enum(base, head, where) - self._compare_required(base, head, where, direction) - self._compare_properties(base, head, where, direction) - self._compare_items(base, head, where, direction) - self._compare_additional(base, head, where, direction) - self._compare_unions(base, head, where, direction) - - def _compare_types(self, base, head, where, direction): - base_types, head_types = self.types(base), self.types(head) - if not base_types: - return - if not head_types: - # Dropping the declaration widens what may be sent, which a - # request producer survives, but it removes the guarantee a - # response consumer was written against. - if direction == 'response': - self.report( - 'type-declaration-removed', - f'{where}: declared type {sorted(base_types)} was removed', - ) - return - # A response consumer breaks on any type change, including a widened - # set it was never written to handle. A request producer only breaks - # when a type it was sending is no longer accepted. - changed = ( - base_types != head_types - if direction == 'response' - else bool(base_types - head_types) - ) - if changed: - self.report( - 'field-type-changed', - f'{where}: type {sorted(base_types)} -> {sorted(head_types)}', - ) - - def _compare_enum(self, base, head, where): - dropped = self.enum_values(base) - self.enum_values(head) - if dropped: - values = [json.loads(value) for value in sorted(dropped)] - self.report( - 'enum-value-removed', - f'{where}: enum value(s) removed: {values}', - ) - - def _compare_required(self, base, head, where, direction): - base_required = {r for r in (base.get('required') or []) if isinstance(r, str)} - head_required = {r for r in (head.get('required') or []) if isinstance(r, str)} - if direction == 'request': - added = head_required - base_required - if added: - self.report( - 'requestbody-required-added', - f'{where}: newly required field(s): {sorted(added)}', - ) - else: - dropped = base_required - head_required - if dropped: - self.report( - 'response-required-removed', - f'{where}: no longer guaranteed: {sorted(dropped)}', - ) - - def _compare_properties(self, base, head, where, direction): - base_props = base.get('properties') - head_props = head.get('properties') - if not isinstance(base_props, dict): - return - if not isinstance(head_props, dict): - head_props = {} - - if direction == 'response': - for name in sorted(set(base_props) - set(head_props)): - self.report( - 'response-field-removed', - f'{where}.{name} was removed', - ) - - for name in sorted(set(base_props) & set(head_props)): - self.compare_schema( - base_props[name], head_props[name], f'{where}.{name}', direction - ) - - def _compare_items(self, base, head, where, direction): - base_items, head_items = base.get('items'), head.get('items') - if base_items is None: - return - if head_items is None: - self.report('array-items-removed', f'{where}: item schema was removed') - return - if isinstance(base_items, dict) and isinstance(head_items, dict): - self.compare_schema(base_items, head_items, f'{where}[]', direction) - elif isinstance(base_items, list) and isinstance(head_items, list): - if len(head_items) < len(base_items): - self.report( - 'tuple-items-removed', - f'{where}: positional item(s) {len(base_items)} -> {len(head_items)}', - ) - for index, (base_item, head_item) in enumerate(zip(base_items, head_items)): - self.compare_schema(base_item, head_item, f'{where}[{index}]', direction) - elif isinstance(base_items, (dict, list)): - self.report( - 'array-items-changed', - f'{where}: item schema changed shape', - ) - - def _compare_additional(self, base, head, where, direction): - base_extra = base.get('additionalProperties') - head_extra = head.get('additionalProperties') - - # Absent means "allowed", so absent -> false is a narrowing too. - if head_extra is False and base_extra is not False: - self.report( - 'additional-properties-restricted', - f'{where}: additionalProperties narrowed to false', - ) - - if isinstance(base_extra, dict) and isinstance(head_extra, dict): - self.compare_schema(base_extra, head_extra, f'{where}.*', direction) - - def _compare_unions(self, base, head, where, direction): - for keyword in ('oneOf', 'anyOf'): - base_variants = self.variants(self.base, base, keyword) - head_variants = self.variants(self.head, head, keyword) - if not base_variants: - continue - - if direction == 'request': - # A client that sent one of the removed shapes now fails. - for key in sorted(set(base_variants) - set(head_variants)): - self.report( - 'request-variant-removed', - f'{where}: {keyword} no longer accepts {key}', - ) - - for key in sorted(set(base_variants) & set(head_variants)): - self.compare_schema( - base_variants[key], - head_variants[key], - f'{where}({keyword}:{key})', - direction, - ) - - # -- document comparison --------------------------------------------- - - def parameters(self, doc, op, shared=()): - """Resolved parameters for an operation, keyed by `(in, name)`. - - Path-item parameters apply to every operation under that path, and an - operation-level parameter with the same name and location overrides - the shared one, so the shared list is applied first. - """ - out = {} - for param in list(shared or []) + list(op.get('parameters') or []): - resolved, _ = self.resolve(doc, param) - if isinstance(resolved, dict) and resolved.get('name'): - out[(resolved.get('in'), resolved['name'])] = resolved - return out - - def json_schemas(self, doc, container): - """JSON media-type schemas of a requestBody/response, by media type.""" - resolved, _ = self.resolve(doc, container) - if not isinstance(resolved, dict): - return {}, {} - content = resolved.get('content') - if not isinstance(content, dict): - return resolved, {} - schemas = { - media: spec['schema'] - for media, spec in content.items() - if 'json' in media and isinstance(spec, dict) and isinstance(spec.get('schema'), dict) - } - return resolved, schemas - - def compare_operation( - self, - base_op, - head_op, - label, - request_direction='request', - base_shared=(), - head_shared=(), - ): - base_id, head_id = base_op.get('operationId'), head_op.get('operationId') - if base_id and not head_id: - # Generators derive client method names from `operationId`, so - # dropping one renames the generated method just as a change does. - self.report( - 'operationid-removed', - f'{label}: operationId {base_id!r} was removed', - ) - elif base_id and head_id and base_id != head_id: - self.report( - 'operationid-changed', - f'{label}: operationId {base_id!r} -> {head_id!r}', - ) - - base_params = self.parameters(self.base, base_op, base_shared) - head_params = self.parameters(self.head, head_op, head_shared) - removed_path_params = sorted( - name for (loc, name) in set(base_params) - set(head_params) if loc == 'path' - ) - if removed_path_params: - self.report( - 'url-parameter-renamed', - f'{label}: path parameter(s) gone: {removed_path_params}', - ) - - added_required_params = sorted( - (str(loc), name) - for (loc, name) in set(head_params) - set(base_params) - # Path parameters are excluded: they are required by definition, - # a rename is already reported as `url-parameter-renamed`, and a - # genuinely new one changes the path template, which surfaces as a - # removed operation instead. - if loc != 'path' and head_params[(loc, name)].get('required') - ) - for location, name in added_required_params: - # An existing caller cannot satisfy a requirement it has never - # heard of, so this is as breaking as making a parameter required. - self.report( - 'parameter-added-required', - f'{label}: new required {location} parameter {name!r}', - ) - - for key in sorted( - set(base_params) & set(head_params), key=lambda k: (str(k[0]), k[1]) - ): - location, name = key - base_param, head_param = base_params[key], head_params[key] - if head_param.get('required') and not base_param.get('required'): - self.report( - 'parameter-now-required', - f'{label}: {location} parameter {name!r} became required', - ) - if isinstance(base_param.get('schema'), dict) and isinstance( - head_param.get('schema'), dict - ): - # A parameter is something the caller sends, so request rules - # apply: a narrowed type or a dropped enum value breaks it. - self.compare_schema( - base_param['schema'], - head_param['schema'], - f'{label} {location} parameter {name}', - 'request', - ) - - base_body, base_body_schemas = self.json_schemas(self.base, base_op.get('requestBody')) - head_body, head_body_schemas = self.json_schemas(self.head, head_op.get('requestBody')) - base_has_body = isinstance(base_body, dict) and bool(base_body) - head_has_body = isinstance(head_body, dict) and bool(head_body) - if base_has_body and not head_has_body: - # The operation no longer accepts a body at all, so callers that - # send one may now be rejected. - self.report( - 'request-body-removed', - f'{label}: requestBody was removed', - ) - elif head_has_body and head_body.get('required'): - if not base_has_body: - # Introducing a required body imposes the same new obligation - # on every existing caller as making an optional one required. - self.report( - 'requestbody-now-required', - f'{label}: a required requestBody was added', - ) - elif not base_body.get('required'): - self.report( - 'requestbody-now-required', - f'{label}: requestBody became required', - ) - if base_has_body and head_has_body: - # Mirrors the response side: a caller that submits JSON breaks - # when that media type stops being accepted. - for media in sorted(set(base_body_schemas) - set(head_body_schemas)): - self.report( - 'request-media-type-removed', - f'{label} request body: {media} was removed', - ) - for media in sorted(set(base_body_schemas) & set(head_body_schemas)): - self.compare_schema( - base_body_schemas[media], - head_body_schemas[media], - f'{label} request body', - request_direction, - ) - - base_responses = base_op.get('responses') - head_responses = head_op.get('responses') - if not isinstance(base_responses, dict) or not isinstance(head_responses, dict): - return - # Status codes are strings in OpenAPI but YAML may yield integers, so - # compare them in one normalised form. - head_responses = {str(code): value for code, value in head_responses.items()} - for code, base_response in sorted( - (str(code), value) for code, value in base_responses.items() - ): - if not code.startswith('2'): - continue - if code not in head_responses: - self.report( - 'response-status-removed', - f'{label}: {code} response was removed', - ) - continue - _, base_schemas = self.json_schemas(self.base, base_response) - _, head_schemas = self.json_schemas(self.head, head_responses[code]) - for media in sorted(set(base_schemas) - set(head_schemas)): - self.report( - 'response-media-type-removed', - f'{label} {code} response: {media} was removed', - ) - for media in sorted(set(base_schemas) & set(head_schemas)): - self.compare_schema( - base_schemas[media], - head_schemas[media], - f'{label} {code} response', - 'response', - ) - - def compare_paths(self): - base_ops, head_ops = operations(self.base), operations(self.head) - base_shared = shared_parameters(self.base) - head_shared = shared_parameters(self.head) - for key in sorted(base_ops): - path, method = key - label = f'{method.upper()} {path}' - if key not in head_ops: - self.report('operation-removed', f'{label} was removed') - continue - self.compare_operation( - base_ops[key], - head_ops[key], - label, - base_shared=base_shared.get(path, ()), - head_shared=head_shared.get(path, ()), - ) - - def compare_webhooks(self): - """Walk `webhooks` (3.1) and `x-webhooks` (3.0) payload schemas. - - A webhook payload is delivered to the consumer, so its request body - is compared with the response rules. - """ - for container in ('webhooks', 'x-webhooks'): - base_hooks = self.base.get(container) - if not isinstance(base_hooks, dict): - continue - head_hooks = self.head.get(container) - if not isinstance(head_hooks, dict): - # Dropping the container removes every webhook in it, so it is - # treated as empty rather than skipped. - head_hooks = {} - for name in sorted(base_hooks): - base_item, _ = self.resolve(self.base, base_hooks[name]) - head_item, _ = self.resolve(self.head, head_hooks.get(name)) - if not isinstance(base_item, dict): - continue - if not isinstance(head_item, dict): - self.report('webhook-removed', f'webhook {name!r} was removed') - continue - for method in METHODS: - base_op, head_op = base_item.get(method), head_item.get(method) - if not isinstance(base_op, dict): - continue - if not isinstance(head_op, dict): - self.report( - 'webhook-operation-removed', - f'webhook {name}: {method.upper()} was removed', - ) - continue - self.compare_operation( - base_op, - head_op, - f'webhook {name}', - request_direction='response', - base_shared=base_item.get('parameters') or (), - head_shared=head_item.get('parameters') or (), - ) - - def compare_components(self): - """Report `components/schemas` removals (rule 9). - - Content comparison happens through the operation and webhook walk, - which reaches these schemas via their `$ref`s. - """ - base_schemas = (self.base.get('components') or {}).get('schemas') or {} - head_schemas = (self.head.get('components') or {}).get('schemas') or {} - if not isinstance(base_schemas, dict) or not isinstance(head_schemas, dict): - return - for name in sorted(set(base_schemas) - set(head_schemas)): - self.report('schema-removed', f'schema {name!r} was removed') - - def run(self): - self.compare_paths() - self.compare_webhooks() - self.compare_components() - return self.findings - - -def operations(doc): - """Map `(path, method)` -> operation object.""" - out = {} - paths = doc.get('paths') - for path, item in (paths if isinstance(paths, dict) else {}).items(): - if not isinstance(item, dict): - continue - for method, op in item.items(): - if method in METHODS and isinstance(op, dict): - out[(path, method)] = op - return out - - -def shared_parameters(doc): - """Map `path` -> the path item's own `parameters` list. - - These apply to every operation under the path unless the operation - declares a parameter with the same name and location. - """ - out = {} - paths = doc.get('paths') - for path, item in (paths if isinstance(paths, dict) else {}).items(): - if isinstance(item, dict) and isinstance(item.get('parameters'), list): - out[path] = item['parameters'] - return out - - -def summarize(base, head): - """Non-breaking additions, used for the informational PR summary.""" - base_ops, head_ops = operations(base), operations(head) - added = [f'{m.upper()} {p}' for (p, m) in set(head_ops) - set(base_ops)] - - base_schemas = set((base.get('components') or {}).get('schemas') or {}) - head_schemas = set((head.get('components') or {}).get('schemas') or {}) - - return { - 'added_operations': sorted(added), - 'added_schemas': sorted(head_schemas - base_schemas), - 'total_operations': len(head_ops), - 'total_schemas': len(head_schemas), - } - - -def compare(base, head): - findings = Comparator(base, head).run() - return { - 'findings': findings, - 'truncated': len(findings) >= MAX_FINDINGS, - 'summary': summarize(base, head), - } - - -def main(argv=None): - parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) - parser.add_argument('base', help='baseline description (YAML or JSON)') - parser.add_argument('head', help='candidate description (YAML or JSON)') - args = parser.parse_args(argv) - - result = compare(load(args.base), load(args.head)) - json.dump(result, sys.stdout, indent=2) - print() - return 0 - - -if __name__ == '__main__': - sys.exit(main()) diff --git a/.github/scripts/test_openapi_breaking_changes.py b/.github/scripts/test_openapi_breaking_changes.py deleted file mode 100644 index 6423c3be80..0000000000 --- a/.github/scripts/test_openapi_breaking_changes.py +++ /dev/null @@ -1,882 +0,0 @@ -#!/usr/bin/env python3 -"""Tests for openapi_breaking_changes. - -Run with: python3 -m unittest discover -s .github/scripts -""" - -import copy -import os -import sys -import unittest - -sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) - -import openapi_breaking_changes as obc # noqa: E402 - - -def doc(paths=None, schemas=None, **extra): - out = {'openapi': '3.0.3', 'paths': paths or {}} - if schemas is not None: - out['components'] = {'schemas': schemas} - out.update(extra) - return out - - -def get_op(schema, method='get', path='/thing'): - """A document with one operation returning `schema` as a 200 body.""" - return doc(paths={ - path: { - method: { - 'operationId': 'thing/get', - 'responses': { - '200': { - 'description': 'ok', - 'content': {'application/json': {'schema': schema}}, - } - }, - } - } - }) - - -def post_op(schema, required=False, path='/thing'): - """A document with one operation accepting `schema` as a request body.""" - return doc(paths={ - path: { - 'post': { - 'operationId': 'thing/create', - 'requestBody': { - 'required': required, - 'content': {'application/json': {'schema': schema}}, - }, - 'responses': {'201': {'description': 'created'}}, - } - } - }) - - -def rules(base, head): - return sorted(f['rule'] for f in obc.compare(base, head)['findings']) - - -def details(base, head, rule): - return [f['detail'] for f in obc.compare(base, head)['findings'] if f['rule'] == rule] - - -class NoChangeTest(unittest.TestCase): - def test_identical_documents_are_clean(self): - base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) - self.assertEqual(rules(base, copy.deepcopy(base)), []) - - def test_additions_are_not_breaking(self): - base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) - head = get_op({ - 'type': 'object', - 'properties': {'id': {'type': 'integer'}, 'name': {'type': 'string'}}, - }) - head['paths']['/added'] = {'get': {'operationId': 'a/b', 'responses': {}}} - self.assertEqual(rules(base, head), []) - - -class OperationTest(unittest.TestCase): - def test_operation_removed(self): - base = get_op({'type': 'object'}) - self.assertEqual(rules(base, doc()), ['operation-removed']) - - def test_operationid_changed(self): - base = get_op({'type': 'object'}) - head = copy.deepcopy(base) - head['paths']['/thing']['get']['operationId'] = 'thing/fetch' - self.assertEqual(rules(base, head), ['operationid-changed']) - - def test_operationid_removed(self): - base = get_op({'type': 'object'}) - head = copy.deepcopy(base) - del head['paths']['/thing']['get']['operationId'] - self.assertEqual(rules(base, head), ['operationid-removed']) - - def test_operationid_added_is_not_breaking(self): - head = get_op({'type': 'object'}) - base = copy.deepcopy(head) - del base['paths']['/thing']['get']['operationId'] - self.assertEqual(rules(base, head), []) - - def test_path_parameter_renamed_behind_a_ref(self): - base = doc(paths={ - '/repos/{owner}': { - 'get': { - 'operationId': 'repos/get', - 'parameters': [{'$ref': '#/components/parameters/owner'}], - 'responses': {}, - } - } - }) - base['components'] = { - 'parameters': {'owner': {'name': 'owner', 'in': 'path', 'required': True}} - } - head = copy.deepcopy(base) - head['components']['parameters']['owner']['name'] = 'org' - self.assertEqual(rules(base, head), ['url-parameter-renamed']) - self.assertIn('owner', details(base, head, 'url-parameter-renamed')[0]) - - def test_query_parameter_removal_is_not_reported(self): - base = doc(paths={ - '/thing': { - 'get': { - 'operationId': 'thing/get', - 'parameters': [{'name': 'per_page', 'in': 'query'}], - 'responses': {}, - } - } - }) - head = copy.deepcopy(base) - head['paths']['/thing']['get']['parameters'] = [] - self.assertEqual(rules(base, head), []) - - def _param_docs(self, base_param, head_param): - def build(param): - return doc(paths={'/thing': {'get': { - 'operationId': 'thing/get', - 'parameters': [param], - 'responses': {}, - }}}) - - return build(base_param), build(head_param) - - def test_parameter_became_required(self): - base, head = self._param_docs( - {'name': 'state', 'in': 'query', 'schema': {'type': 'string'}}, - {'name': 'state', 'in': 'query', 'required': True, - 'schema': {'type': 'string'}}, - ) - self.assertEqual(rules(base, head), ['parameter-now-required']) - - def test_parameter_enum_value_removed(self): - base, head = self._param_docs( - {'name': 'state', 'in': 'query', - 'schema': {'type': 'string', 'enum': ['open', 'closed', 'all']}}, - {'name': 'state', 'in': 'query', - 'schema': {'type': 'string', 'enum': ['open', 'closed']}}, - ) - self.assertEqual(rules(base, head), ['enum-value-removed']) - - def test_parameter_schema_widening_is_not_breaking(self): - base, head = self._param_docs( - {'name': 'id', 'in': 'query', 'schema': {'type': 'integer'}}, - {'name': 'id', 'in': 'query', 'schema': {'type': ['integer', 'string']}}, - ) - self.assertEqual(rules(base, head), []) - - def test_success_status_removed(self): - base = doc(paths={'/thing': {'get': { - 'operationId': 'thing/get', - 'responses': {'200': {'description': 'ok'}, '204': {'description': 'empty'}}, - }}}) - head = doc(paths={'/thing': {'get': { - 'operationId': 'thing/get', - 'responses': {'200': {'description': 'ok'}}, - }}}) - self.assertEqual(rules(base, head), ['response-status-removed']) - - def test_error_status_removal_is_not_reported(self): - base = doc(paths={'/thing': {'get': { - 'operationId': 'thing/get', - 'responses': {'200': {'description': 'ok'}, '404': {'description': 'gone'}}, - }}}) - head = doc(paths={'/thing': {'get': { - 'operationId': 'thing/get', - 'responses': {'200': {'description': 'ok'}}, - }}}) - self.assertEqual(rules(base, head), []) - - def test_integer_status_keys_are_normalised(self): - base = doc(paths={'/thing': {'get': { - 'operationId': 'thing/get', - 'responses': {200: {'description': 'ok'}}, - }}}) - head = doc(paths={'/thing': {'get': { - 'operationId': 'thing/get', - 'responses': {'200': {'description': 'ok'}}, - }}}) - self.assertEqual(rules(base, head), []) - - def test_json_media_type_removed(self): - base = get_op({'type': 'object'}) - head = copy.deepcopy(base) - content = head['paths']['/thing']['get']['responses']['200']['content'] - content['text/plain'] = content.pop('application/json') - self.assertEqual(rules(base, head), ['response-media-type-removed']) - - -class RequestDirectionTest(unittest.TestCase): - def test_request_type_declaration_removed_is_not_breaking(self): - base = post_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) - head = post_op({'type': 'object', 'properties': {'id': {'description': 'id'}}}) - self.assertEqual(rules(base, head), []) - - def test_request_body_became_required(self): - base = post_op({'type': 'object'}, required=False) - head = post_op({'type': 'object'}, required=True) - self.assertEqual(rules(base, head), ['requestbody-now-required']) - - def test_new_required_request_body_added(self): - head = post_op({'type': 'object'}, required=True) - base = copy.deepcopy(head) - del base['paths']['/thing']['post']['requestBody'] - self.assertEqual(rules(base, head), ['requestbody-now-required']) - self.assertEqual( - details(base, head, 'requestbody-now-required'), - ['POST /thing: a required requestBody was added'], - ) - - def test_added_required_path_parameter_is_not_double_reported(self): - base = get_op({'type': 'object'}, path='/thing/{id}') - base['paths']['/thing/{id}']['get']['parameters'] = [ - {'name': 'id', 'in': 'path', 'required': True, 'schema': {'type': 'string'}} - ] - head = copy.deepcopy(base) - head['paths']['/thing/{id}']['get']['parameters'] = [ - {'name': 'thing_id', 'in': 'path', 'required': True, 'schema': {'type': 'string'}} - ] - self.assertEqual(rules(base, head), ['url-parameter-renamed']) - - def test_path_item_required_parameter_added(self): - base = get_op({'type': 'object'}) - head = copy.deepcopy(base) - head['paths']['/thing']['parameters'] = [ - {'name': 'since', 'in': 'query', 'required': True, 'schema': {'type': 'string'}} - ] - self.assertEqual(rules(base, head), ['parameter-added-required']) - - def test_path_item_parameter_became_required(self): - base = get_op({'type': 'object'}) - base['paths']['/thing']['parameters'] = [ - {'name': 'q', 'in': 'query', 'schema': {'type': 'string'}} - ] - head = copy.deepcopy(base) - head['paths']['/thing']['parameters'][0]['required'] = True - self.assertEqual(rules(base, head), ['parameter-now-required']) - - def test_moving_a_parameter_to_the_operation_is_not_breaking(self): - param = {'name': 'q', 'in': 'query', 'schema': {'type': 'string'}} - base = get_op({'type': 'object'}) - base['paths']['/thing']['parameters'] = [param] - head = get_op({'type': 'object'}) - head['paths']['/thing']['get']['parameters'] = [dict(param)] - self.assertEqual(rules(base, head), []) - - def test_operation_parameter_overrides_the_path_item_one(self): - base = get_op({'type': 'object'}) - base['paths']['/thing']['parameters'] = [ - {'name': 'q', 'in': 'query', 'required': True, 'schema': {'type': 'string'}} - ] - base['paths']['/thing']['get']['parameters'] = [ - {'name': 'q', 'in': 'query', 'schema': {'type': 'string'}} - ] - head = copy.deepcopy(base) - # The operation keeps it optional, so the required path-item entry - # must not be what gets compared. - head['paths']['/thing']['get']['parameters'][0]['schema'] = {'type': 'string'} - self.assertEqual(rules(base, head), []) - - def test_new_required_query_parameter(self): - base = get_op({'type': 'object'}) - head = copy.deepcopy(base) - head['paths']['/thing']['get']['parameters'] = [ - {'name': 'since', 'in': 'query', 'required': True, 'schema': {'type': 'string'}} - ] - self.assertEqual(rules(base, head), ['parameter-added-required']) - self.assertEqual( - details(base, head, 'parameter-added-required'), - ["GET /thing: new required query parameter 'since'"], - ) - - def test_new_optional_parameter_is_not_breaking(self): - base = get_op({'type': 'object'}) - head = copy.deepcopy(base) - head['paths']['/thing']['get']['parameters'] = [ - {'name': 'since', 'in': 'query', 'schema': {'type': 'string'}} - ] - self.assertEqual(rules(base, head), []) - - def test_new_required_parameter_behind_a_ref(self): - base = get_op({'type': 'object'}) - base['components'] = {'parameters': { - 'since': {'name': 'since', 'in': 'header', 'required': True, - 'schema': {'type': 'string'}}, - }} - head = copy.deepcopy(base) - head['paths']['/thing']['get']['parameters'] = [ - {'$ref': '#/components/parameters/since'} - ] - self.assertEqual(rules(base, head), ['parameter-added-required']) - - def test_request_body_removed_entirely(self): - base = post_op({'type': 'object'}, required=True) - head = copy.deepcopy(base) - del head['paths']['/thing']['post']['requestBody'] - self.assertEqual(rules(base, head), ['request-body-removed']) - - def test_optional_request_body_removed_is_still_reported(self): - base = post_op({'type': 'object'}, required=False) - head = copy.deepcopy(base) - del head['paths']['/thing']['post']['requestBody'] - self.assertEqual(rules(base, head), ['request-body-removed']) - - def test_request_json_media_type_removed(self): - base = post_op({'type': 'object'}) - head = copy.deepcopy(base) - content = head['paths']['/thing']['post']['requestBody']['content'] - content['multipart/form-data'] = content.pop('application/json') - self.assertEqual(rules(base, head), ['request-media-type-removed']) - self.assertEqual( - details(base, head, 'request-media-type-removed'), - ['POST /thing request body: application/json was removed'], - ) - - def test_added_request_media_type_is_not_breaking(self): - base = post_op({'type': 'object'}) - head = copy.deepcopy(base) - head['paths']['/thing']['post']['requestBody']['content']['application/vnd.v3+json'] = { - 'schema': {'type': 'object'} - } - self.assertEqual(rules(base, head), []) - - def test_adding_a_request_body_is_not_breaking(self): - head = post_op({'type': 'object'}, required=False) - base = copy.deepcopy(head) - del base['paths']['/thing']['post']['requestBody'] - self.assertEqual(rules(base, head), []) - - def test_newly_required_request_field(self): - schema = {'type': 'object', 'properties': {'name': {'type': 'string'}}} - head_schema = dict(schema, required=['name']) - self.assertEqual( - rules(post_op(schema), post_op(head_schema)), - ['requestbody-required-added'], - ) - - def test_dropping_a_required_request_field_is_not_breaking(self): - schema = {'type': 'object', 'properties': {'name': {'type': 'string'}}} - self.assertEqual( - rules(post_op(dict(schema, required=['name'])), post_op(schema)), - [], - ) - - def test_removing_a_request_property_is_not_breaking(self): - base = post_op({ - 'type': 'object', - 'properties': {'name': {'type': 'string'}, 'note': {'type': 'string'}}, - }) - head = post_op({'type': 'object', 'properties': {'name': {'type': 'string'}}}) - self.assertEqual(rules(base, head), []) - - def test_widening_a_request_type_is_not_breaking(self): - base = post_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) - head = post_op({ - 'type': 'object', - 'properties': {'id': {'type': ['integer', 'string']}}, - }) - self.assertEqual(rules(base, head), []) - - def test_narrowing_a_request_type_is_breaking(self): - base = post_op({ - 'type': 'object', - 'properties': {'id': {'type': ['integer', 'string']}}, - }) - head = post_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) - self.assertEqual(rules(base, head), ['field-type-changed']) - - -class ResponseDirectionTest(unittest.TestCase): - def test_response_field_removed(self): - base = get_op({ - 'type': 'object', - 'properties': {'id': {'type': 'integer'}, 'name': {'type': 'string'}}, - }) - head = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) - self.assertEqual(rules(base, head), ['response-field-removed']) - - def test_response_required_removed(self): - schema = {'type': 'object', 'properties': {'id': {'type': 'integer'}}} - base = get_op(dict(schema, required=['id'])) - self.assertEqual(rules(base, get_op(schema)), ['response-required-removed']) - - def test_adding_a_response_required_field_is_not_breaking(self): - schema = {'type': 'object', 'properties': {'id': {'type': 'integer'}}} - self.assertEqual(rules(get_op(schema), get_op(dict(schema, required=['id']))), []) - - def test_response_type_declaration_removed(self): - base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) - head = get_op({'type': 'object', 'properties': {'id': {'description': 'id'}}}) - self.assertEqual(rules(base, head), ['type-declaration-removed']) - - def test_response_type_declaration_added_is_not_breaking(self): - head = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) - base = get_op({'type': 'object', 'properties': {'id': {'description': 'id'}}}) - self.assertEqual(rules(base, head), []) - - def test_response_type_change_in_either_direction(self): - base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) - head = get_op({ - 'type': 'object', - 'properties': {'id': {'type': ['integer', 'null']}}, - }) - self.assertEqual(rules(base, head), ['field-type-changed']) - - -class NestedTest(unittest.TestCase): - """The previous shallow comparison missed everything in this class.""" - - def test_nested_inline_object(self): - base = get_op({ - 'type': 'object', - 'properties': { - 'owner': { - 'type': 'object', - 'properties': {'login': {'type': 'string'}, 'id': {'type': 'integer'}}, - } - }, - }) - head = copy.deepcopy(base) - del head['paths']['/thing']['get']['responses']['200']['content'][ - 'application/json']['schema']['properties']['owner']['properties']['login'] - self.assertEqual(rules(base, head), ['response-field-removed']) - self.assertIn('.owner.login', details(base, head, 'response-field-removed')[0]) - - def test_three_levels_deep(self): - def build(leaf): - return get_op({ - 'type': 'object', - 'properties': { - 'a': { - 'type': 'object', - 'properties': { - 'b': {'type': 'object', 'properties': {'c': leaf}}, - }, - } - }, - }) - - self.assertEqual( - rules(build({'type': 'string'}), build({'type': 'integer'})), - ['field-type-changed'], - ) - - def test_array_items(self): - base = get_op({ - 'type': 'array', - 'items': { - 'type': 'object', - 'properties': {'id': {'type': 'integer'}, 'name': {'type': 'string'}}, - }, - }) - head = get_op({ - 'type': 'array', - 'items': {'type': 'object', 'properties': {'id': {'type': 'integer'}}}, - }) - self.assertEqual(rules(base, head), ['response-field-removed']) - self.assertIn('[].name', details(base, head, 'response-field-removed')[0]) - - def test_array_of_arrays(self): - base = get_op({ - 'type': 'array', - 'items': {'type': 'array', 'items': {'type': 'string'}}, - }) - head = get_op({ - 'type': 'array', - 'items': {'type': 'array', 'items': {'type': 'integer'}}, - }) - self.assertEqual(rules(base, head), ['field-type-changed']) - - def test_additional_properties_schema(self): - base = get_op({ - 'type': 'object', - 'additionalProperties': { - 'type': 'object', - 'properties': {'id': {'type': 'integer'}}, - }, - }) - head = get_op({'type': 'object', 'additionalProperties': {'type': 'object'}}) - self.assertEqual(rules(base, head), ['response-field-removed']) - - def test_additional_properties_narrowed_to_false(self): - base = get_op({'type': 'object', 'additionalProperties': True}) - head = get_op({'type': 'object', 'additionalProperties': False}) - self.assertEqual(rules(base, head), ['additional-properties-restricted']) - - def test_additional_properties_absent_then_false(self): - base = get_op({'type': 'object', 'properties': {}}) - head = get_op({'type': 'object', 'properties': {}, 'additionalProperties': False}) - self.assertEqual(rules(base, head), ['additional-properties-restricted']) - - def test_additional_properties_widened_is_not_breaking(self): - base = get_op({'type': 'object', 'additionalProperties': False}) - head = get_op({'type': 'object', 'additionalProperties': True}) - self.assertEqual(rules(base, head), []) - - def test_tuple_item_removed(self): - base = get_op({ - 'type': 'array', - 'items': [{'type': 'string'}, {'type': 'integer'}], - }) - head = get_op({'type': 'array', 'items': [{'type': 'string'}]}) - self.assertEqual(rules(base, head), ['tuple-items-removed']) - - def test_tuple_item_type_changed(self): - base = get_op({ - 'type': 'array', - 'items': [{'type': 'string'}, {'type': 'integer'}], - }) - head = get_op({ - 'type': 'array', - 'items': [{'type': 'string'}, {'type': 'boolean'}], - }) - self.assertEqual(rules(base, head), ['field-type-changed']) - - def test_item_schema_removed(self): - base = get_op({'type': 'array', 'items': {'type': 'string'}}) - head = get_op({'type': 'array'}) - self.assertEqual(rules(base, head), ['array-items-removed']) - - def test_item_schema_shape_changed(self): - base = get_op({'type': 'array', 'items': [{'type': 'string'}]}) - head = get_op({'type': 'array', 'items': {'type': 'string'}}) - self.assertEqual(rules(base, head), ['array-items-changed']) - - -class ReferenceTest(unittest.TestCase): - def _ref_docs(self, base_schema, head_schema): - base = get_op({'$ref': '#/components/schemas/thing'}) - base['components'] = {'schemas': {'thing': base_schema}} - head = get_op({'$ref': '#/components/schemas/thing'}) - head['components'] = {'schemas': {'thing': head_schema}} - return base, head - - def test_field_removed_behind_a_ref(self): - base, head = self._ref_docs( - {'type': 'object', 'properties': {'id': {'type': 'integer'}, 'x': {'type': 'string'}}}, - {'type': 'object', 'properties': {'id': {'type': 'integer'}}}, - ) - self.assertEqual(rules(base, head), ['response-field-removed']) - - def test_ref_chain_is_followed(self): - base = get_op({'$ref': '#/components/schemas/alias'}) - base['components'] = {'schemas': { - 'alias': {'$ref': '#/components/schemas/thing'}, - 'thing': {'type': 'object', 'properties': {'id': {'type': 'integer'}}}, - }} - head = copy.deepcopy(base) - head['components']['schemas']['thing']['properties']['id']['type'] = 'string' - self.assertEqual(rules(base, head), ['field-type-changed']) - - def test_response_object_ref_is_followed(self): - base = doc(paths={'/thing': {'get': { - 'operationId': 'thing/get', - 'responses': {'200': {'$ref': '#/components/responses/thing'}}, - }}}) - base['components'] = {'responses': {'thing': { - 'description': 'ok', - 'content': {'application/json': {'schema': { - 'type': 'object', - 'properties': {'id': {'type': 'integer'}, 'x': {'type': 'string'}}, - }}}, - }}} - head = copy.deepcopy(base) - del head['components']['responses']['thing']['content'][ - 'application/json']['schema']['properties']['x'] - self.assertEqual(rules(base, head), ['response-field-removed']) - - def test_removed_component_schema(self): - base = get_op({'$ref': '#/components/schemas/thing'}) - base['components'] = {'schemas': {'thing': {'type': 'object'}}} - head = get_op({'$ref': '#/components/schemas/thing'}) - head['components'] = {'schemas': {}} - self.assertEqual(rules(base, head), ['schema-removed', 'schema-removed']) - - def test_unreferenced_component_removal_is_reported(self): - base = doc(schemas={'orphan': {'type': 'object'}}) - head = doc(schemas={}) - self.assertEqual(rules(base, head), ['schema-removed']) - - def test_recursive_schema_terminates(self): - def build(leaf_type): - document = get_op({'$ref': '#/components/schemas/node'}) - document['components'] = {'schemas': {'node': { - 'type': 'object', - 'properties': { - 'value': {'type': leaf_type}, - 'parent': {'$ref': '#/components/schemas/node'}, - 'children': { - 'type': 'array', - 'items': {'$ref': '#/components/schemas/node'}, - }, - }, - }}} - return document - - self.assertEqual(rules(build('string'), build('integer')), ['field-type-changed']) - - def test_self_referential_ref_does_not_hang(self): - base = get_op({'$ref': '#/components/schemas/loop'}) - base['components'] = {'schemas': {'loop': {'$ref': '#/components/schemas/loop'}}} - self.assertEqual(rules(base, copy.deepcopy(base)), []) - - -class CompositionTest(unittest.TestCase): - def test_allof_member_field_removed(self): - base = get_op({'allOf': [ - {'type': 'object', 'properties': {'id': {'type': 'integer'}}}, - {'type': 'object', 'properties': {'name': {'type': 'string'}}}, - ]}) - head = get_op({'allOf': [ - {'type': 'object', 'properties': {'id': {'type': 'integer'}}}, - ]}) - self.assertEqual(rules(base, head), ['response-field-removed']) - - def test_allof_via_ref_field_type_changed(self): - base = get_op({'allOf': [ - {'$ref': '#/components/schemas/base'}, - {'type': 'object', 'properties': {'extra': {'type': 'string'}}}, - ]}) - base['components'] = {'schemas': {'base': { - 'type': 'object', 'properties': {'id': {'type': 'integer'}}, - }}} - head = copy.deepcopy(base) - head['components']['schemas']['base']['properties']['id']['type'] = 'string' - self.assertEqual(rules(base, head), ['field-type-changed']) - - def test_allof_inherited_required_removed(self): - def build(required): - document = get_op({'allOf': [ - {'$ref': '#/components/schemas/base'}, - {'type': 'object', 'properties': {'extra': {'type': 'string'}}}, - ]}) - document['components'] = {'schemas': {'base': { - 'type': 'object', - 'properties': {'id': {'type': 'integer'}}, - 'required': required, - }}} - return document - - self.assertEqual(rules(build(['id']), build([])), ['response-required-removed']) - - def test_nested_allof_is_flattened(self): - def build(props): - return get_op({'allOf': [ - {'allOf': [{'type': 'object', 'properties': props}]}, - ]}) - - self.assertEqual( - rules(build({'a': {'type': 'string'}, 'b': {'type': 'string'}}), - build({'a': {'type': 'string'}})), - ['response-field-removed'], - ) - - def test_oneof_member_is_compared(self): - def build(leaf): - document = get_op({'oneOf': [ - {'$ref': '#/components/schemas/simple'}, - {'$ref': '#/components/schemas/full'}, - ]}) - document['components'] = {'schemas': { - 'simple': {'type': 'string'}, - 'full': {'type': 'object', 'properties': {'id': leaf}}, - }} - return document - - self.assertEqual( - rules(build({'type': 'integer'}), build({'type': 'string'})), - ['field-type-changed'], - ) - - def test_anyof_member_matched_by_title(self): - def build(leaf): - return get_op({'anyOf': [ - {'title': 'a', 'type': 'object', 'properties': {'x': leaf}}, - {'title': 'b', 'type': 'string'}, - ]}) - - # Reordering alone is not a finding; the type change is. - head = build({'type': 'integer'}) - head['paths']['/thing']['get']['responses']['200']['content'][ - 'application/json']['schema']['anyOf'].reverse() - self.assertEqual(rules(build({'type': 'string'}), head), ['field-type-changed']) - - def test_request_union_member_removed_is_breaking(self): - base = post_op({'oneOf': [{'type': 'string'}, {'type': 'integer'}]}) - head = post_op({'oneOf': [{'type': 'string'}]}) - self.assertEqual(rules(base, head), ['request-variant-removed']) - - def test_response_union_member_removed_is_not_reported(self): - base = get_op({'oneOf': [ - {'title': 'a', 'type': 'string'}, - {'title': 'b', 'type': 'integer'}, - ]}) - head = get_op({'oneOf': [{'title': 'a', 'type': 'string'}]}) - self.assertEqual(rules(base, head), []) - - -class EnumTest(unittest.TestCase): - def test_enum_value_removed_at_depth(self): - def build(values): - return get_op({ - 'type': 'object', - 'properties': { - 'items': { - 'type': 'array', - 'items': { - 'type': 'object', - 'properties': {'state': {'type': 'string', 'enum': values}}, - }, - } - }, - }) - - base, head = build(['open', 'closed', 'draft']), build(['open', 'closed']) - self.assertEqual(rules(base, head), ['enum-value-removed']) - self.assertIn('draft', details(base, head, 'enum-value-removed')[0]) - - def test_awkward_scalar_enum_values(self): - awkward = ['+1', '-1', "won't fix", 'false positive', True, False, None, 1] - - def build(values): - return get_op({'type': 'object', 'properties': { - 'reaction': {'enum': values}, - }}) - - base = build(awkward) - head = build([v for v in awkward if v != '+1']) - self.assertEqual(rules(base, head), ['enum-value-removed']) - self.assertIn('+1', details(base, head, 'enum-value-removed')[0]) - - def test_boolean_and_string_enum_values_are_distinct(self): - def build(values): - return get_op({'type': 'object', 'properties': {'flag': {'enum': values}}}) - - self.assertEqual(rules(build([True]), build(['true'])), ['enum-value-removed']) - - def test_enum_value_added_is_not_reported(self): - def build(values): - return get_op({'type': 'object', 'properties': { - 'state': {'type': 'string', 'enum': values}, - }}) - - self.assertEqual(rules(build(['open']), build(['open', 'closed'])), []) - - -class WebhookTest(unittest.TestCase): - def _hooks(self, container, leaf): - return doc(**{container: {'push': {'post': { - 'operationId': 'webhook/push', - 'requestBody': {'content': {'application/json': {'schema': { - 'type': 'object', - 'properties': {'ref': leaf}, - }}}}, - 'responses': {}, - }}}}) - - def test_x_webhooks_payload_uses_response_rules(self): - base = self._hooks('x-webhooks', {'type': 'string'}) - head = doc(**{'x-webhooks': {'push': {'post': { - 'operationId': 'webhook/push', - 'requestBody': {'content': {'application/json': {'schema': { - 'type': 'object', 'properties': {}, - }}}}, - 'responses': {}, - }}}}) - self.assertEqual(rules(base, head), ['response-field-removed']) - - def test_webhook_removed(self): - base = self._hooks('webhooks', {'type': 'string'}) - self.assertEqual(rules(base, doc(webhooks={})), ['webhook-removed']) - - def test_whole_webhook_container_removed(self): - base = self._hooks('webhooks', {'type': 'string'}) - self.assertEqual(rules(base, doc()), ['webhook-removed']) - - def test_x_webhooks_container_removed(self): - base = self._hooks('x-webhooks', {'type': 'string'}) - self.assertEqual(rules(base, doc()), ['webhook-removed']) - - def test_webhook_method_removed(self): - base = self._hooks('webhooks', {'type': 'string'}) - head = doc(webhooks={'push': {'description': 'still documented'}}) - self.assertEqual(rules(base, head), ['webhook-operation-removed']) - self.assertEqual( - details(base, head, 'webhook-operation-removed'), - ['webhook push: POST was removed'], - ) - - def test_adding_a_webhook_is_not_breaking(self): - head = self._hooks('webhooks', {'type': 'string'}) - self.assertEqual(rules(doc(), head), []) - - -class OutputTest(unittest.TestCase): - def test_summary_counts_additions(self): - base = get_op({'type': 'object'}, path='/a') - head = copy.deepcopy(base) - head['paths']['/b'] = {'get': {'operationId': 'b', 'responses': {}}} - head['components'] = {'schemas': {'new': {'type': 'object'}}} - summary = obc.compare(base, head)['summary'] - self.assertEqual(summary['added_operations'], ['GET /b']) - self.assertEqual(summary['added_schemas'], ['new']) - self.assertEqual(summary['total_operations'], 2) - - def test_findings_are_capped_and_flagged(self): - props = {f'p{i}': {'type': 'string'} for i in range(obc.MAX_FINDINGS + 25)} - base = get_op({'type': 'object', 'properties': props}) - head = get_op({'type': 'object', 'properties': {}}) - result = obc.compare(base, head) - self.assertEqual(len(result['findings']), obc.MAX_FINDINGS) - self.assertTrue(result['truncated']) - - def test_shared_schema_is_compared_once(self): - shared = {'$ref': '#/components/schemas/thing'} - base = doc(paths={ - '/a': {'get': {'operationId': 'a', 'responses': {'200': { - 'description': 'ok', 'content': {'application/json': {'schema': dict(shared)}}}}}}, - '/b': {'get': {'operationId': 'b', 'responses': {'200': { - 'description': 'ok', 'content': {'application/json': {'schema': dict(shared)}}}}}}, - }, schemas={'thing': { - 'type': 'object', 'properties': {'id': {'type': 'integer'}}, - }}) - head = copy.deepcopy(base) - head['components']['schemas']['thing']['properties'] = {} - # Both operations reference the same schema object, so the removal is - # reported once rather than once per reference site. - findings = obc.compare(base, head)['findings'] - self.assertEqual([f['rule'] for f in findings], ['response-field-removed']) - - def test_cli_round_trip(self): - import io - import json - import tempfile - - import yaml - - base = get_op({'type': 'object', 'properties': {'id': {'type': 'integer'}}}) - head = get_op({'type': 'object', 'properties': {}}) - with tempfile.TemporaryDirectory() as tmp: - paths = [] - for name, document in (('base.yaml', base), ('head.yaml', head)): - path = os.path.join(tmp, name) - with open(path, 'w', encoding='utf-8') as handle: - yaml.safe_dump(document, handle) - paths.append(path) - - captured, sys.stdout = sys.stdout, io.StringIO() - try: - self.assertEqual(obc.main(paths), 0) - payload = json.loads(sys.stdout.getvalue()) - finally: - sys.stdout = captured - - self.assertEqual( - [f['rule'] for f in payload['findings']], ['response-field-removed'] - ) - - -if __name__ == '__main__': - unittest.main() diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml index 69342f16e5..d059490e74 100644 --- a/.github/workflows/auto-merge-openapi-updates.yml +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -16,10 +16,15 @@ name: Auto-merge OpenAPI description updates # * Every step pins to the head commit SHA captured during selection. Branch # names are never re-resolved, so a push that lands mid-run cannot slip an # unvalidated commit into a merge. -# * Lint must be green, the compatibility scan must come back clean, and a -# change summary is posted to the pull request before merge. The scan -# covers the two dotcom source descriptions only; see the FILE_30/FILE_31 -# comment for the exact surface and what that leaves to lint. +# * Lint must be green, the candidate must touch nothing outside the +# description directories, and a change summary is posted to the pull +# request before merge. +# * Compatibility review is not repeated here. These descriptions are +# generated and reviewed for compatibility before publication, so a +# re-check against the published files would be a weaker duplicate of a +# gate that has already run against the authoritative source. +# * Every decision is made by deterministic code, so the same inputs always +# produce the same merge/no-merge answer and it can be tested. # * Merges are held during a GHES release-candidate window. See the "Check # for an active merge freeze" step. # @@ -60,17 +65,6 @@ env: # required check on the default branch, and it routinely reports `timed_out` # on these pull requests. REQUIRED_CHECKS: 'Lint OpenAPI 3.0 releases,Lint OpenAPI 3.1 releases' - # The two non-dereferenced dotcom descriptions: compact, `$ref`-based, and - # upstream of the calendar-versioned dotcom variants, which are produced by - # applying the changeset extensions declared in these files. These are the - # only files the compatibility scan reads, and it compares the described - # surface, not those changeset extensions. A bot pull request also updates - # the `ghec`, `github.ae` and `ghes-*` trees and their dereferenced forms, - # which are version-filtered rather than derived, so a change confined to - # one of those is covered by the required lint and the posted change - # summary, not by the scan. - FILE_30: descriptions/api.github.com/api.github.com.yaml - FILE_31: descriptions-next/api.github.com/api.github.com.yaml jobs: auto-merge: @@ -85,13 +79,12 @@ jobs: with: # Pinned to the default branch on purpose. A `workflow_dispatch` run # can be started from any ref, and without this the write-enabled job - # would run the selector and scanner from that ref instead. + # would run the selector from that ref instead. ref: ${{ github.event.repository.default_branch }} path: tools fetch-depth: 1 sparse-checkout: | .github/scripts - requirements.txt sparse-checkout-cone-mode: false - name: Set up Python @@ -99,11 +92,6 @@ jobs: with: python-version: '3.12' - - name: Install dependencies - # Reuses the repository's existing pinned/hash-verified requirements, - # which already provide pyyaml for the linter workflow. - run: pip install --require-hashes -r tools/requirements.txt - - name: Select candidate pull requests id: select env: @@ -257,7 +245,7 @@ jobs: if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' uses: actions/checkout@v4 with: - # Pinned like the tools checkout so the scan baseline and the merge + # Pinned like the tools checkout so the summary baseline and merge # target are provably the same branch, whatever ref a manual run # was started from. ref: ${{ github.event.repository.default_branch }} @@ -277,6 +265,7 @@ jobs: SHA_31: ${{ steps.select.outputs.sha_31 }} PR_30: ${{ steps.select.outputs.pr_30 }} PR_31: ${{ steps.select.outputs.pr_31 }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} run: | set -euo pipefail @@ -295,78 +284,36 @@ jobs: echo "status=stale" >>"$GITHUB_OUTPUT" exit 0 fi - done - - echo "status=pinned" >>"$GITHUB_OUTPUT" - - - name: Scan for compatibility-breaking changes - id: breaking - if: steps.pin.outputs.status == 'pinned' - working-directory: repo - env: - SHA_30: ${{ steps.select.outputs.sha_30 }} - SHA_31: ${{ steps.select.outputs.sha_31 }} - PR_30: ${{ steps.select.outputs.pr_30 }} - PR_31: ${{ steps.select.outputs.pr_31 }} - DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} - run: | - set -euo pipefail - - # A parsed comparison rather than a diff scan. The two are not - # equivalent: for a *request* body, ADDING to `required` is breaking, - # while for a *response* body, REMOVING from `required` is breaking. - # A scan of removed diff lines is structurally blind to the first - # case. Rules mirror the documented release-compatibility list. - findings='' - : >summary.md - - for triple in "$SHA_30|$FILE_30|3.0|$PR_30" "$SHA_31|$FILE_31|3.1|$PR_31"; do - IFS='|' read -r sha file label pr <<<"$triple" - [ -n "$sha" ] || continue - git show "origin/$DEFAULT_BRANCH:$file" >base.yaml - git show "$sha:$file" >head.yaml - - python3 "$GITHUB_WORKSPACE/tools/.github/scripts/openapi_breaking_changes.py" \ - base.yaml head.yaml >result.json - - count=$(jq '.findings | length' result.json) - { - echo "### OpenAPI $label (#$pr)" - echo - jq -r '.summary | - "- Operations: \(.total_operations) total, \(.added_operations | length) added", - "- Schemas: \(.total_schemas) total, \(.added_schemas | length) added"' result.json - # `sed -n` rather than `head`: it consumes all input, so the - # producer is never killed by SIGPIPE under `pipefail`. - jq -r '.summary.added_operations[]? | " - `\(.)`"' result.json | sed -n '1,40p' - echo - } >>summary.md - - if [ "$count" -gt 0 ]; then - findings+="$label (#$pr): $count finding(s). " - { - echo "#### :rotating_light: Compatibility findings in $label" - echo - jq -r '.findings[] | "- **\(.rule)**: \(.detail)"' result.json | sed -n '1,50p' - if [ "$count" -gt 50 ] || [ "$(jq -r '.truncated' result.json)" = 'true' ]; then - echo - echo "_Finding list truncated; see the workflow run for the full output._" - fi - echo - } >>summary.md + # A merge is only allowed to bring in description content. A pull + # request that touches workflows, scripts or anything else is left + # for a human, even when it carries the right title and author. + # This gate is about *what kind of file* changed, not about whether + # the description content itself is compatible. + # + # Evaluated here, before anything is written to the pull request, + # so an out-of-scope candidate never collects a comment promising a + # merge that will not happen. Both the commit and the local + # `origin/$DEFAULT_BRANCH` are fixed for the rest of the run, so + # this answer cannot change before the merge step. + base_commit=$(git merge-base "origin/$DEFAULT_BRANCH" "$sha") + # Captured before filtering so a failed `git diff` aborts the step + # instead of looking like "nothing outside the descriptions". + changed=$(git diff --name-only "$base_commit" "$sha") + # `sed -n` rather than `head`: it consumes all input, so the + # producer is never killed by SIGPIPE under `pipefail`. + outside=$(grep -v -E '^descriptions(-next)?/' <<<"$changed" | sed -n '1,10p' || true) + if [ -n "$outside" ]; then + echo "::notice::#$pr changes files outside the description directories; stopping." + echo "status=out-of-scope" >>"$GITHUB_OUTPUT" + echo "detail=#$pr changes $(tr '\n' ' ' <<<"$outside")" >>"$GITHUB_OUTPUT" + exit 0 fi - done - cat summary.md + git diff --stat "$base_commit" "$sha" | sed -n '1,60p' >"summary-$pr.txt" + done - if [ -n "$findings" ]; then - echo "status=breaking" >>"$GITHUB_OUTPUT" - echo "detail=$findings" >>"$GITHUB_OUTPUT" - echo "::warning::Potential breaking changes, skipping auto-merge. $findings" - else - echo "status=clean" >>"$GITHUB_OUTPUT" - fi + echo "status=pinned" >>"$GITHUB_OUTPUT" - name: Post change summary to pull requests if: steps.pin.outputs.status == 'pinned' @@ -378,13 +325,14 @@ jobs: SHA_31: ${{ steps.select.outputs.sha_31 }} PR_30: ${{ steps.select.outputs.pr_30 }} PR_31: ${{ steps.select.outputs.pr_31 }} - BREAKING: ${{ steps.breaking.outputs.status }} run: | set -euo pipefail - # Release notes are generated from pull request descriptions. - # Auto-merging untouched bodies would remove useful context, so post - # the analysis the scanner already computed. + # Release notes are generated from pull request descriptions, so an + # auto-merged pull request would otherwise land with no record of + # what the automation did. This is an inventory of the changed files, + # not a compatibility judgement: see the header for where + # compatibility review happens. for pair in "$PR_30:$SHA_30" "$PR_31:$SHA_31"; do pr="${pair%%:*}"; sha="${pair#*:}" [ -n "$pr" ] && [ -n "$sha" ] || continue @@ -402,13 +350,14 @@ jobs: { echo "$marker" - if [ "$BREAKING" = 'breaking' ]; then - echo "## :rotating_light: Auto-merge skipped: potential breaking changes" - else - echo "## Automated change summary" - fi + echo "## Automated change summary" + echo + echo "Auto-merging \`${sha:0:7}\`, which changes these files:" + echo + echo '```' + cat "summary-$pr.txt" + echo '```' echo - cat summary.md echo "_Posted by the auto-merge workflow ([run](${GITHUB_SERVER_URL}/${GH_REPO}/actions/runs/${GITHUB_RUN_ID}))._" } >comment.md @@ -420,7 +369,7 @@ jobs: - name: Merge and close superseded pull requests id: merge - if: steps.pin.outputs.status == 'pinned' && steps.breaking.outputs.status == 'clean' + if: steps.pin.outputs.status == 'pinned' working-directory: repo env: GH_TOKEN: ${{ github.token }} @@ -489,27 +438,6 @@ jobs: fi done - # A merge is only allowed to bring in description content. A pull - # request that touches workflows, scripts or anything else is left - # for a human, even when it carries the right title and author. - # This is a wider surface than the compatibility scan reads, so the - # remainder rests on the required lint and the posted summary. - for entry in $candidates; do - rest="${entry#*:}" - pr="${rest%%:*}"; sha="${rest#*:}" - base_commit=$(git merge-base "origin/$DEFAULT_BRANCH" "$sha") - # Captured before filtering so a failed `git diff` aborts the step - # instead of looking like "nothing outside the descriptions". - changed=$(git diff --name-only "$base_commit" "$sha") - outside=$(grep -v -E '^descriptions(-next)?/' <<<"$changed" | sed -n '1,10p' || true) - if [ -n "$outside" ]; then - echo "::notice::#$pr changes files outside the description directories; stopping." - echo "status=out-of-scope" >>"$GITHUB_OUTPUT" - echo "detail=#$pr changes $(tr '\n' ' ' <<<"$outside")" >>"$GITHUB_OUTPUT" - exit 0 - fi - done - if [ "$DRY_RUN" = "true" ]; then echo "Dry run: would merge ${candidates:-none}, close: ${SUPERSEDED_30:-} ${SUPERSEDED_31:-}" echo "status=dry-run" >>"$GITHUB_OUTPUT" @@ -586,19 +514,17 @@ jobs: if: >- failure() || steps.checks.outputs.status == 'blocked' || - steps.breaking.outputs.status == 'breaking' || steps.freeze.outputs.status == 'frozen' || steps.pin.outputs.status == 'stale' || steps.merge.outputs.status == 'stale' || - steps.merge.outputs.status == 'out-of-scope' + steps.pin.outputs.status == 'out-of-scope' env: CHECKS_STATUS: ${{ steps.checks.outputs.status }} CHECKS_DETAIL: ${{ steps.checks.outputs.detail }} FREEZE_STATUS: ${{ steps.freeze.outputs.status }} FREEZE_DETAIL: ${{ steps.freeze.outputs.detail }} - BREAKING_STATUS: ${{ steps.breaking.outputs.status }} - BREAKING_DETAIL: ${{ steps.breaking.outputs.detail }} PIN_STATUS: ${{ steps.pin.outputs.status }} + PIN_DETAIL: ${{ steps.pin.outputs.detail }} MERGE_STATUS: ${{ steps.merge.outputs.status }} MERGE_DETAIL: ${{ steps.merge.outputs.detail }} MERGE_CLOSE_FAILED: ${{ steps.merge.outputs.close_failed }} @@ -614,10 +540,8 @@ jobs: echo "Required checks were not green: $CHECKS_DETAIL" elif [ "$FREEZE_STATUS" = 'frozen' ]; then echo "A merge freeze was detected: $FREEZE_DETAIL" - elif [ "$BREAKING_STATUS" = 'breaking' ]; then - echo "Potential breaking changes were detected: $BREAKING_DETAIL" - elif [ "$MERGE_STATUS" = 'out-of-scope' ]; then - echo "A candidate changed files outside the description directories: $MERGE_DETAIL" + elif [ "$PIN_STATUS" = 'out-of-scope' ]; then + echo "A candidate changed files outside the description directories: $PIN_DETAIL" elif [ "$PIN_STATUS" = 'stale' ] || [ "$MERGE_STATUS" = 'stale' ]; then echo "A candidate pull request was updated mid-run, so nothing was merged." elif [ -n "$MERGE_CLOSE_FAILED" ]; then diff --git a/.github/workflows/scripts-tests.yml b/.github/workflows/scripts-tests.yml index cf5ff53701..c52b1b2e4f 100644 --- a/.github/workflows/scripts-tests.yml +++ b/.github/workflows/scripts-tests.yml @@ -8,12 +8,10 @@ on: paths: - '.github/scripts/**' - '.github/workflows/scripts-tests.yml' - - 'requirements.txt' pull_request: paths: - '.github/scripts/**' - '.github/workflows/scripts-tests.yml' - - 'requirements.txt' workflow_dispatch: jobs: @@ -27,7 +25,5 @@ jobs: name: Install Python with: python-version: '3.12' - - run: pip install --require-hashes -r requirements.txt - name: Install dependencies - run: python3 -m unittest discover --start-directory .github/scripts --verbose name: Run script tests diff --git a/requirements.txt b/requirements.txt index b3635a7f51..4c57eea273 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,5 @@ # Generated by pip-compile with --generate-hashes -# Used by .github/workflows/linter.yml, auto-merge-openapi-updates.yml and -# scripts-tests.yml for installation with hash verification +# Used by .github/workflows/linter.yml for yamllint installation with hash verification pathspec==1.0.4 \ --hash=sha256:0210e2ae8a21a9137c0d470578cb0e595af87edaa6ebf12ff176f14a02e0e645 \ --hash=sha256:fb6ae2fd4e7c921a165808a552060e722767cfa526f99ca5156ed2ce45a5c723 From 107496b96e9c1eed68387a5dc261ee9af4e33507 Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Wed, 19 Aug 2026 17:39:36 -0500 Subject: [PATCH 12/13] Merge through the pull request API and report to chat The default branch requires a pull request before merging, so pushing a local merge commit to it is rejected. Merge each candidate with `gh pr merge --merge --match-head-commit ` instead: the head-commit check and the merge are a single server-side operation, so the commit that was validated is the only one that can land, and a retry after a gateway timeout cannot merge something newer. The two description versions are merged independently. A version that merges gets its superseded pull requests closed even if the other version fails, because leaving them open would let a later run select an older snapshot and put an earlier description back on the default branch. Outcomes are decided in one place and posted to chat for the cases that need someone to act, following the existing linter failure notifier. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b022cc6-cf92-4a1a-a283-cc4bcd70288c --- .../workflows/auto-merge-openapi-updates.yml | 241 +++++++++++++----- 1 file changed, 177 insertions(+), 64 deletions(-) diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml index d059490e74..1438cefeec 100644 --- a/.github/workflows/auto-merge-openapi-updates.yml +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -3,10 +3,12 @@ name: Auto-merge OpenAPI description updates # Merges the newest open `github-openapi-bot` "Update OpenAPI 3.x Descriptions" # pull requests and closes the older superseded ones. # -# Why a workflow instead of native auto-merge / the merge API: these pull -# requests carry 100K+ line diffs across 64+ files. `PUT /pulls/{n}/merge` -# returns 502/504 and `GET /pulls/{n}/files` returns 422 on them, so the merge -# is done with plain git against a blobless clone. +# Why a workflow rather than native auto-merge: these pull requests carry 100K+ +# line diffs across 64+ files, and `GET /pulls/{n}/files` returns 422 on them, +# so the list of changed files is read from a blobless clone instead. The merge +# itself goes through `PUT /pulls/{n}/merge`, because the default branch +# requires a pull request and so refuses a direct push; that call can time out +# at the gateway on diffs this size, which the merge step handles. # # Safety properties: # * Only pull requests titled exactly "Update OpenAPI 3.0 Descriptions" or @@ -14,8 +16,9 @@ name: Auto-merge OpenAPI description updates # superseded pull request is re-checked against the exact title it was # superseded under, immediately before it is closed. # * Every step pins to the head commit SHA captured during selection. Branch -# names are never re-resolved, so a push that lands mid-run cannot slip an -# unvalidated commit into a merge. +# names are never re-resolved, and the merge call itself requires that SHA, +# so a push that lands mid-run cannot slip an unvalidated commit into a +# merge. # * Lint must be green, the candidate must touch nothing outside the # description directories, and a change summary is posted to the pull # request before merge. @@ -30,11 +33,6 @@ name: Auto-merge OpenAPI description updates # # Scripts are run from a checkout of this repository's default branch, never # from the candidate pull request. -# -# Note: the merge is pushed with `GITHUB_TOKEN`, which by design does not -# trigger `on: push` workflows, so the default branch is not re-linted by the -# push itself. The same content was linted on the pull request head before -# merge. on: schedule: @@ -245,16 +243,16 @@ jobs: if: steps.checks.outputs.status == 'green' && steps.freeze.outputs.status == 'clear' uses: actions/checkout@v4 with: - # Pinned like the tools checkout so the summary baseline and merge - # target are provably the same branch, whatever ref a manual run - # was started from. + # Pinned like the tools checkout so the comparison baseline is + # provably the default branch, whatever ref a manual run was started + # from. ref: ${{ github.event.repository.default_branch }} path: repo - # Blobless fetch keeps this off the ~4.6 GB full history while still - # allowing real merges. Blobs for the touched files are fetched lazily. + # Only used to read which files a candidate changes, which the API + # cannot answer for pull requests this large. A blobless fetch keeps + # this off the ~4.6 GB of history; blobs are fetched lazily. filter: blob:none fetch-depth: 0 - token: ${{ github.token }} - name: Fetch and pin candidate commits id: pin @@ -370,7 +368,6 @@ jobs: - name: Merge and close superseded pull requests id: merge if: steps.pin.outputs.status == 'pinned' - working-directory: repo env: GH_TOKEN: ${{ github.token }} GH_REPO: ${{ github.repository }} @@ -444,30 +441,69 @@ jobs: exit 0 fi - git config user.name 'github-actions[bot]' - git config user.email '41898282+github-actions[bot]@users.noreply.github.com' - git checkout "$DEFAULT_BRANCH" + # Merged through the pull request API rather than by pushing the + # default branch: the branch requires a pull request, so a direct + # push is refused, and merging the pull request is the path that + # satisfies that rule without needing an exemption. + # + # `--match-head-commit` makes the server reject the merge unless the + # head is still the validated commit. That check and the merge are + # one atomic operation, so unlike a read followed by a write there is + # no window in which the head can move in between. It also makes the + # retry below safe: a retry can only ever merge the same commit. + merge_pr() { + pr="$1"; sha="$2" + for attempt in 1 2 3 4 5; do + if gh pr merge "$pr" --merge --match-head-commit "$sha"; then + return 0 + fi + + # These pull requests are large enough that the call can time out + # at the gateway while the merge still completes, so the recorded + # state is the authority here, not the exit code. + if [ "$(gh pr view "$pr" --json merged -q .merged 2>/dev/null)" = 'true' ]; then + echo "#$pr is merged; the call that performed it did not report back." + return 0 + fi + + if [ "$attempt" -lt 5 ]; then + echo "::notice::Merge attempt $attempt for #$pr did not succeed; retrying." + sleep $((attempt * 15)) + fi + done + return 1 + } merged='' - # 3.0 first, then 3.1: they touch disjoint trees but this ordering - # keeps the generated merge history readable. + merge_failed='' + # The two versions are independent: they touch disjoint trees and are + # selected and superseded separately, so one failing must not change + # what happens to the other. Each version that merges is recorded so + # its superseded pull requests are still closed below. + merged_groups='' for entry in $candidates; do - rest="${entry#*:}" + suffix="${entry%%:*}"; rest="${entry#*:}" pr="${rest%%:*}"; sha="${rest#*:}" - git merge --no-ff "$sha" -m "Merge pull request #$pr ($sha)" - merged+="#$pr " + + if merge_pr "$pr" "$sha"; then + merged+="#$pr " + merged_groups+="$suffix " + else + merge_failed+="#$pr " + echo "::error::#$pr could not be merged." + fi done - if [ -n "$merged" ]; then - git push origin "$DEFAULT_BRANCH" - echo "Merged and pushed: $merged" + # Recorded immediately after the merges: everything below can fail, + # and none of it may hide what already landed. + if [ -n "$merge_failed" ]; then + echo "status=merge-failed" >>"$GITHUB_OUTPUT" + echo "detail=${merged:+merged ${merged% }, but }${merge_failed% } could not be merged" >>"$GITHUB_OUTPUT" + else + echo "status=merged" >>"$GITHUB_OUTPUT" + echo "detail=${merged% }" >>"$GITHUB_OUTPUT" fi - # Recorded immediately after the push: everything below can fail, - # and none of it may hide the fact that the merge already landed. - echo "status=merged" >>"$GITHUB_OUTPUT" - echo "detail=Merged $merged" >>"$GITHUB_OUTPUT" - # Re-read current state immediately before closing. Selection # happened minutes earlier, and a pull request can be retitled, # retargeted, transferred or closed in between. A pull request is @@ -494,31 +530,46 @@ jobs: fi gh pr close "$pr" \ - --comment "Superseded by the newer OpenAPI description update(s) ${merged:-just merged}. Closing automatically." \ + --comment "Superseded by the newer OpenAPI description update(s) ${merged% }. Closing automatically." \ || close_failed+="#$pr " done } - close_superseded "$title_30" ${SUPERSEDED_30:-} - close_superseded "$title_31" ${SUPERSEDED_31:-} + # Only for versions that actually merged. Closing them is what stops + # the next run from selecting a stale snapshot: once the newest pull + # request is merged and therefore closed, an older one left open + # becomes the newest open one, and merging that would put an earlier + # description back on the default branch. A version whose merge + # failed keeps its superseded pull requests, because its newest one + # is still open and still the right candidate. + for suffix in $merged_groups; do + case "$suffix" in + 30) close_superseded "$title_30" ${SUPERSEDED_30:-} ;; + 31) close_superseded "$title_31" ${SUPERSEDED_31:-} ;; + esac + done # A superseded pull request left open would be picked up as a # candidate by the next run, so this must not pass silently. if [ -n "$close_failed" ]; then - echo "close_failed=$close_failed" >>"$GITHUB_OUTPUT" + echo "close_failed=${close_failed% }" >>"$GITHUB_OUTPUT" echo "::error::Failed to close superseded pull request(s): $close_failed" + fi + + if [ -n "$close_failed" ] || [ -n "$merge_failed" ]; then exit 1 fi - - name: Summarize exceptions - if: >- - failure() || - steps.checks.outputs.status == 'blocked' || - steps.freeze.outputs.status == 'frozen' || - steps.pin.outputs.status == 'stale' || - steps.merge.outputs.status == 'stale' || - steps.pin.outputs.status == 'out-of-scope' + - name: Report outcome + # Runs on every completed path so the outcome is decided in exactly one + # place and the job summary and the notification cannot disagree. + if: ${{ !cancelled() }} env: + CHATTERBOX_URL: ${{ secrets.CHATTERBOX_URL }} + CHATTERBOX_TOKEN: ${{ secrets.CHATTERBOX_TOKEN }} + GH_REPO: ${{ github.repository }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + SELECT_FOUND: ${{ steps.select.outputs.found }} CHECKS_STATUS: ${{ steps.checks.outputs.status }} CHECKS_DETAIL: ${{ steps.checks.outputs.detail }} FREEZE_STATUS: ${{ steps.freeze.outputs.status }} @@ -529,24 +580,86 @@ jobs: MERGE_DETAIL: ${{ steps.merge.outputs.detail }} MERGE_CLOSE_FAILED: ${{ steps.merge.outputs.close_failed }} run: | + set -euo pipefail + + # `notify` marks the outcomes a person needs to see. The schedule runs + # twelve times a day, and a freeze or a red check persists across many + # of those runs, so notifying on them would produce a stream of + # identical messages that trains people to ignore the channel. Those + # outcomes are recorded in the job summary instead. Lint failures + # already have their own notification. + notify='false' + + if [ "$MERGE_STATUS" = 'merge-failed' ]; then + icon=':x:' + headline='Auto-merge failed' + detail="The workflow $MERGE_DETAIL.${MERGE_CLOSE_FAILED:+ Superseded pull request(s) $MERGE_CLOSE_FAILED could not be closed either.}" + notify='true' + elif [ -n "$MERGE_CLOSE_FAILED" ]; then + icon=':warning:' + headline='Auto-merge needs follow-up' + detail="Merged $MERGE_DETAIL, but superseded pull request(s) $MERGE_CLOSE_FAILED could not be closed. Close them manually, otherwise the next run treats one of them as a candidate." + notify='true' + elif [ "$MERGE_STATUS" = 'merged' ]; then + icon=':white_check_mark:' + headline='Auto-merged OpenAPI description updates' + detail="Merged $MERGE_DETAIL" + notify='true' + elif [ "$MERGE_STATUS" = 'dry-run' ]; then + icon=':information_source:' + headline='Auto-merge dry run' + detail='Nothing was merged or closed.' + elif [ "$CHECKS_STATUS" = 'blocked' ]; then + icon=':warning:' + headline='Auto-merge did not proceed' + detail="Required checks were not green: $CHECKS_DETAIL" + elif [ "$FREEZE_STATUS" = 'frozen' ]; then + icon=':snowflake:' + headline='Auto-merge held' + detail="A merge freeze was detected: $FREEZE_DETAIL" + elif [ "$PIN_STATUS" = 'out-of-scope' ]; then + icon=':warning:' + headline='Auto-merge did not proceed' + detail="A candidate changed files outside the description directories: $PIN_DETAIL" + notify='true' + elif [ "$PIN_STATUS" = 'stale' ] || [ "$MERGE_STATUS" = 'stale' ]; then + icon=':information_source:' + headline='Auto-merge did not proceed' + detail='A candidate pull request was updated mid-run, so nothing was merged.' + elif [ "$SELECT_FOUND" = 'false' ]; then + icon=':information_source:' + headline='Nothing to do' + detail='No open OpenAPI description update pull requests.' + else + icon=':x:' + headline='Auto-merge failed' + detail='The workflow failed before merging. Review the failed step above.' + notify='true' + fi + + # Written before the notification is attempted, so the run always + # carries the outcome even if posting it fails. { - if [ -n "$MERGE_CLOSE_FAILED" ]; then - echo "## Auto-merge needs follow-up" - else - echo "## Auto-merge did not proceed" - fi + echo "## $headline" echo - if [ "$CHECKS_STATUS" = 'blocked' ]; then - echo "Required checks were not green: $CHECKS_DETAIL" - elif [ "$FREEZE_STATUS" = 'frozen' ]; then - echo "A merge freeze was detected: $FREEZE_DETAIL" - elif [ "$PIN_STATUS" = 'out-of-scope' ]; then - echo "A candidate changed files outside the description directories: $PIN_DETAIL" - elif [ "$PIN_STATUS" = 'stale' ] || [ "$MERGE_STATUS" = 'stale' ]; then - echo "A candidate pull request was updated mid-run, so nothing was merged." - elif [ -n "$MERGE_CLOSE_FAILED" ]; then - echo "The merge succeeded ($MERGE_DETAIL), but superseded pull request(s) $MERGE_CLOSE_FAILED could not be closed. Close them manually, otherwise the next run treats one of them as a candidate." - else - echo "The workflow failed before merging. Review the failed step above." - fi + echo "$detail" } >>"$GITHUB_STEP_SUMMARY" + + echo "$headline: $detail" + [ "$notify" = 'true' ] || exit 0 + + if [ -z "${CHATTERBOX_URL:-}" ] || [ -z "${CHATTERBOX_TOKEN:-}" ]; then + echo "CHATTERBOX_URL or CHATTERBOX_TOKEN is not configured; skipping notification." + exit 0 + fi + + message=$(printf '%s\n' \ + "$icon $headline in ${GH_REPO}." \ + "• ${detail}" \ + "• Run: ${RUN_URL}") + + curl --fail --silent --show-error \ + -X POST \ + -u "${CHATTERBOX_TOKEN}:" \ + "${CHATTERBOX_URL%/}/topics/%23api-platform" \ + --data "$message" From a2a980081f9385c9efa569e120120698c6c1fc46 Mon Sep 17 00:00:00 2001 From: Shawn Hartsell Date: Wed, 19 Aug 2026 18:09:51 -0500 Subject: [PATCH 13/13] Read every open pull request when selecting Selection listed at most 100 pull requests. The bot's updates accumulate during a freeze, and the ones past the cap are the oldest, which are exactly the ones that need closing: one left open becomes the newest open pull request for its title once the merged one closes, and merging it would put an earlier description back on the default branch. There are already more than twenty open per version. Page through them all instead. The endpoint that does this cannot filter by author, so the author rule moves next to the title rule in the selection script, where it is tested, and an entry whose author cannot be read is skipped rather than assumed to match. Also keep the whole change summary rather than its first sixty lines, since these updates routinely touch more files than that and the total line was being cut, and skip the pull request comment on a dry run so rollout validation stays read-only and does not leave behind a marker that would suppress the real comment later. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b022cc6-cf92-4a1a-a283-cc4bcd70288c --- .github/scripts/select_openapi_prs.py | 63 ++++++++++++++- .github/scripts/test_select_openapi_prs.py | 81 +++++++++++++++++++ .../workflows/auto-merge-openapi-updates.yml | 38 +++++++-- 3 files changed, 170 insertions(+), 12 deletions(-) diff --git a/.github/scripts/select_openapi_prs.py b/.github/scripts/select_openapi_prs.py index 06d9bed09d..b6d3d761f9 100644 --- a/.github/scripts/select_openapi_prs.py +++ b/.github/scripts/select_openapi_prs.py @@ -1,13 +1,18 @@ #!/usr/bin/env python3 """Select the OpenAPI description update PRs to merge, and the superseded ones. -Reads the JSON array produced by +Reads the pages produced by - gh pr list --state open --author \ - --json number,title,headRefOid,createdAt + gh api --paginate --slurp "repos/{repo}/pulls?state=open&per_page=100" on stdin and writes `key=value` lines suitable for `$GITHUB_OUTPUT`. +Every open pull request is read, following pagination to the end, because a +capped listing would silently drop the oldest matches. Those are exactly the +superseded pull requests that must be closed: one left open becomes the +newest open pull request for its title once the merged one closes, and +merging it would put an earlier description back on the default branch. + Two properties matter for safety and are the reason this is a script rather than an inline `jq` filter: @@ -23,6 +28,10 @@ * The head commit SHA of each selected pull request is emitted alongside its number, so every later step can pin to the exact commit that was inspected instead of resolving a branch name again. +* The author is matched here rather than by a listing flag, because the + endpoint that pages over every open pull request cannot filter by author. + Keeping the author and title rules together means one tested place decides + what is eligible. """ import argparse @@ -37,6 +46,44 @@ GROUPS = (('30', TITLE_30), ('31', TITLE_31)) +def normalise(payload, author): + """Flatten paginated pages and map API fields to the names used below. + + `--slurp` yields one list per page, so the payload is a list of lists. + A single flat list is accepted too, which keeps the parsing rules the + same whichever way the input was produced. + """ + flat = [] + for entry in payload: + if isinstance(entry, list): + flat.extend(entry) + else: + flat.append(entry) + + normalised = [] + for pr in flat: + if not isinstance(pr, dict): + continue + # An entry whose author cannot be read is skipped rather than + # assumed to match: it must never become a merge candidate, and it + # must never be closed as superseded. + user = pr.get('user') + login = user.get('login') if isinstance(user, dict) else None + if login != author: + continue + head = pr.get('head') + normalised.append( + { + 'number': pr.get('number'), + 'title': pr.get('title'), + 'headRefOid': head.get('sha') if isinstance(head, dict) else None, + 'createdAt': pr.get('created_at'), + } + ) + + return normalised + + def _sort_key(pr): # `createdAt` is RFC 3339 in UTC, so lexical order is chronological. # The number breaks ties deterministically. @@ -94,6 +141,10 @@ def outputs(pull_requests): def main(argv=None): parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument( + '--author', + help='only consider pull requests opened by this login', + ) parser.add_argument( '--title', choices=[suffix for suffix, _ in GROUPS], @@ -105,12 +156,16 @@ def main(argv=None): print(dict(GROUPS)[args.title]) return 0 + if not args.author: + print('--author is required when selecting', file=sys.stderr) + return 2 + payload = json.load(sys.stdin) if not isinstance(payload, list): print('expected a JSON array of pull requests', file=sys.stderr) return 2 - for line in outputs(payload): + for line in outputs(normalise(payload, args.author)): print(line) return 0 diff --git a/.github/scripts/test_select_openapi_prs.py b/.github/scripts/test_select_openapi_prs.py index 1ff3f9280c..45617fc89c 100644 --- a/.github/scripts/test_select_openapi_prs.py +++ b/.github/scripts/test_select_openapi_prs.py @@ -155,6 +155,78 @@ def test_malformed_entries_are_ignored(self): self.assertEqual(out['superseded_30'], '') +def api_pr(number, title, sha='sha', created='2026-01-01T00:00:00Z', login='bot'): + """One entry shaped like the REST pulls endpoint returns it.""" + return { + 'number': number, + 'title': title, + 'head': {'sha': sha}, + 'created_at': created, + 'user': {'login': login}, + } + + +class NormaliseTest(unittest.TestCase): + """Reading every page matters: a dropped page leaves superseded PRs open.""" + + def test_pages_are_flattened_and_fields_mapped(self): + pages = [ + [api_pr(1, sel.TITLE_30, sha='a', created='2026-01-01T00:00:00Z')], + [api_pr(2, sel.TITLE_30, sha='b', created='2026-02-01T00:00:00Z')], + ] + self.assertEqual( + sel.normalise(pages, 'bot'), + [ + { + 'number': 1, + 'title': sel.TITLE_30, + 'headRefOid': 'a', + 'createdAt': '2026-01-01T00:00:00Z', + }, + { + 'number': 2, + 'title': sel.TITLE_30, + 'headRefOid': 'b', + 'createdAt': '2026-02-01T00:00:00Z', + }, + ], + ) + + def test_a_flat_list_is_accepted_too(self): + self.assertEqual(len(sel.normalise([api_pr(1, sel.TITLE_30)], 'bot')), 1) + + def test_another_author_is_never_eligible(self): + pages = [[api_pr(1, sel.TITLE_30, login='someone-else')]] + self.assertEqual(sel.normalise(pages, 'bot'), []) + + def test_an_unreadable_author_is_skipped_rather_than_assumed(self): + pages = [[{'number': 1, 'title': sel.TITLE_30, 'head': {'sha': 'a'}}]] + self.assertEqual(sel.normalise(pages, 'bot'), []) + + def test_a_malformed_entry_does_not_abort_the_page(self): + pages = [['not a dict', api_pr(2, sel.TITLE_30), None]] + self.assertEqual([pr['number'] for pr in sel.normalise(pages, 'bot')], [2]) + + def test_a_missing_head_survives_normalisation_and_blocks_pinning(self): + pages = [[{'number': 1, 'title': sel.TITLE_30, 'user': {'login': 'bot'}}]] + normalised = sel.normalise(pages, 'bot') + self.assertIsNone(normalised[0]['headRefOid']) + self.assertEqual(sel.outputs(normalised)[0], 'found=false') + + def test_an_older_pull_request_on_a_later_page_is_still_superseded(self): + # The regression this guards: the oldest matches arrive last, and + # dropping them would leave a stale pull request open to be selected + # by a later run once the merged one closes. + pages = [ + [api_pr(9, sel.TITLE_30, sha='new', created='2026-05-01T00:00:00Z')], + [api_pr(1, sel.TITLE_30, sha='old', created='2026-01-01T00:00:00Z')], + ] + out = dict(line.split('=', 1) for line in sel.outputs(sel.normalise(pages, 'bot'))) + self.assertEqual(out['pr_30'], '9') + self.assertEqual(out['sha_30'], 'new') + self.assertEqual(out['superseded_30'], '1') + + class CliTest(unittest.TestCase): def _run(self, argv): import io @@ -171,6 +243,15 @@ def test_title_flag_prints_the_exact_title_for_a_group(self): self.assertEqual(self._run(['--title', '30']), (0, [sel.TITLE_30])) self.assertEqual(self._run(['--title', '31']), (0, [sel.TITLE_31])) + def test_selecting_without_an_author_is_refused(self): + import io + + captured, sys.stderr = sys.stderr, io.StringIO() + try: + self.assertEqual(sel.main([]), 2) + finally: + sys.stderr = captured + def test_title_flag_rejects_an_unknown_group(self): import io diff --git a/.github/workflows/auto-merge-openapi-updates.yml b/.github/workflows/auto-merge-openapi-updates.yml index 1438cefeec..b3f3649b2f 100644 --- a/.github/workflows/auto-merge-openapi-updates.yml +++ b/.github/workflows/auto-merge-openapi-updates.yml @@ -98,12 +98,16 @@ jobs: run: | set -euo pipefail - gh pr list \ - --state open \ - --author "$BOT_LOGIN" \ - --limit 100 \ - --json number,title,headRefOid,createdAt \ - | python3 tools/.github/scripts/select_openapi_prs.py >selection.txt + # Every open pull request by the bot, following pagination rather + # than stopping at a fixed count. A cap that silently drops the + # oldest matches would leave superseded pull requests open, and a + # later run would then treat one of them as the newest open + # candidate and put an earlier description back on the default + # branch. During a long freeze these accumulate well past one page. + gh api --paginate --slurp \ + "repos/$GH_REPO/pulls?state=open&per_page=100" \ + | python3 tools/.github/scripts/select_openapi_prs.py \ + --author "$BOT_LOGIN" >selection.txt cat selection.txt cat selection.txt >>"$GITHUB_OUTPUT" @@ -308,13 +312,31 @@ jobs: exit 0 fi - git diff --stat "$base_commit" "$sha" | sed -n '1,60p' >"summary-$pr.txt" + # Kept whole: a truncated inventory would omit files and the + # total line, which is the part worth reading. These updates + # routinely touch dozens of files but only a few kilobytes of + # stat, so this stays far inside the comment size limit. The + # guard below is for the pathological case only. + git diff --stat "$base_commit" "$sha" >"summary-$pr.txt" + if [ "$(wc -c <"summary-$pr.txt")" -gt 60000 ]; then + echo "::notice::#$pr has an unusually large change summary; truncating." + # `sed -n` rather than `head`: it consumes all input, so the + # producer is never killed by SIGPIPE under `pipefail`. + sed -n '1,500p' "summary-$pr.txt" >"summary-$pr.trimmed" + printf '\n... truncated; see the pull request files tab.\n' \ + >>"summary-$pr.trimmed" + mv "summary-$pr.trimmed" "summary-$pr.txt" + fi done echo "status=pinned" >>"$GITHUB_OUTPUT" - name: Post change summary to pull requests - if: steps.pin.outputs.status == 'pinned' + # A dry run is for confirming selection, so it stays read-only apart + # from the run summary. Posting here would also leave a comment + # carrying this commit's marker, which the later real run would treat + # as "already summarised" and skip. + if: steps.pin.outputs.status == 'pinned' && inputs.dry_run != true working-directory: repo env: GH_TOKEN: ${{ github.token }}