ci: fail closed on API compatibility checks

.github/workflows/api-diff.yml

@@ -12,32 +12,23 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
-  test-conventional-commit-logic:
-    runs-on: ubuntu-latest
-    permissions:
-      contents: read
-    steps:
-    - name: Checkout code
-      uses: actions/checkout@v4
-
-    - name: Run conventional commit logic unit tests
-      run: ./scripts/api-diff/api-diff.test.sh
-
   api-diff:
     runs-on: ubuntu-latest
-    needs: test-conventional-commit-logic
+    timeout-minutes: 20
     permissions:
       contents: read
-      pull-requests: write
 
     steps:
     - name: Checkout code
-      uses: actions/checkout@v4
+      uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0
       with:
         fetch-depth: 0
 
     - name: Make script executable
       run: chmod +x scripts/api-diff/api-diff.sh
 
+    - name: Test fail-closed behaviour
+      run: bash scripts/api-diff/api-diff.test.sh
+
     - name: Run API diff check
       run: ./scripts/api-diff/api-diff.sh

scripts/api-diff/README.md

@@ -10,56 +10,65 @@ Main script that compares OpenAPI specifications against the master branch.
 **Usage:**
 ```bash
 # From the repo root
-./scripts/api-diff/api-diff.sh [--fail-on-breaking] [filename.yaml]
+./scripts/api-diff/api-diff.sh [filename.yaml]
 
 # Check all xero*.yaml files
 ./scripts/api-diff/api-diff.sh
 
 # Check a single file
 ./scripts/api-diff/api-diff.sh xero_accounting.yaml
-
-# Fail on breaking changes (CI mode)
-./scripts/api-diff/api-diff.sh --fail-on-breaking
 ```
 
+The script always fails on breaking changes. There is no flag to waive that, and
+any unrecognised argument is rejected with exit code 2.
+
+**Exit Codes:**
+- `0` - No breaking changes detected
+- `1` - Breaking changes detected, or the check could not be completed
+- `2` - Unsupported argument
+
 **Environment Variables:**
-- `OASDIFF_DOCKER_IMAGE` - Docker image to use (default: `tufin/oasdiff:latest`)
+- `OASDIFF_DOCKER_IMAGE` - Docker image to use (default: oasdiff 1.28.0 pinned by image digest in `api-diff.sh`)
 - `BASE_BRANCH` - Branch to compare against (default: `origin/master`)
 
+When updating oasdiff, verify the release tag and the image manifest digest from the publisher, then update both the tag and digest together. Do not replace the default with a mutable tag such as `latest`.
+
 ### `api-diff.test.sh`
-Unit tests for conventional commit breaking marker detection used in GitHub Actions.
+Fail-closed tests for `api-diff.sh`. They stub `docker` on `PATH` so the script's
+exit codes can be checked without running oasdiff.
 
 **Usage:**
 ```bash
 ./scripts/api-diff/api-diff.test.sh
 ```
 
 Tests validate that:
-- Commits with `!` in the conventional commit header are correctly identified
-- Commits with `BREAKING CHANGE:` footer are correctly identified
-- Other commits are handled with breaking change enforcement
+- A clean comparison exits `0`
+- A breaking change reported by oasdiff exits `1`
+- A changelog generation failure exits `1` instead of being ignored
+- An unsupported argument exits `2`
 
 ## Integration
 
-These scripts are integrated into the GitHub Actions workflow at `.github/workflows/api-diff.yml`:
-- **test-conventional-commit-logic** job - Runs unit tests
-- **api-diff** job - Runs API diff checks with conditional breaking change enforcement
-
-### Conventional Commit Breaking Markers
-The API diff script automatically adjusts behavior based on commit messages:
-
-**Allow Breaking Changes:**
-- Commit header with `!`, for example: `feat!: remove deprecated endpoint`
-- Commit header with scope and `!`, for example: `feat(api)!: remove deprecated endpoint`
-- Commit body/footer containing `BREAKING CHANGE: ...`
-- The `--fail-on-breaking` flag is NOT passed to the script
-
-**Fail on Breaking Changes:**
-- Commits without these conventional commit breaking markers
-- The `--fail-on-breaking` flag IS passed to the script
-- Build will fail if breaking changes are detected
-
-This keeps enforcement aligned with [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/#summary) and semantic-release expectations.
+These scripts run in the GitHub Actions workflow at `.github/workflows/api-diff.yml`:
+- **api-diff** job - Runs the fail-closed tests, then the API diff check
+
+### Fail-Closed Behaviour
+The check is built so that no failure mode is reported as a pass:
+
+- Breaking changes always fail the build. Conventional Commit markers such as
+  `feat!:` in the header or a `BREAKING CHANGE:` footer no longer waive the
+  check, and the `--fail-on-breaking` flag has been removed.
+- A spec deleted or renamed in the pull request counts as a breaking change.
+  Specs are enumerated from the union of the base ref and the working tree, so
+  removing a file cannot skip the comparison.
+- A base ref that cannot be fetched or resolved aborts the run rather than
+  comparing against nothing.
+- A changelog that cannot be generated fails the run rather than logging a
+  warning and continuing.
+
+If a breaking change is intentional, coordinate the major version bump and the
+release notes. There is no in-repo way to waive the check.
 
 ## Known Limitations
 

scripts/api-diff/api-diff.sh

@@ -1,97 +1,37 @@
 #!/bin/bash
 
 # Script to check API diffs using oasdiff
-# Usage: ./scripts/api-diff/api-diff.sh [--fail-on-breaking] [filename.yaml]
+# Usage: ./scripts/api-diff/api-diff.sh [filename.yaml]
 # Assumes you have Docker installed and the repo is checked out with master branch available
 
-set -e  # Exit on error
-set -o pipefail  # Catch errors in pipes
+set -euo pipefail
 
 # Change to repo root
 cd "$(dirname "$0")/../.."
 
 # Configuration
-DOCKER_IMAGE="${OASDIFF_DOCKER_IMAGE:-tufin/oasdiff:latest}"
+DOCKER_IMAGE="${OASDIFF_DOCKER_IMAGE:-tufin/oasdiff:v1.28.0@sha256:86830f988eaafcf589acb2794ee5ab78e3300ded071d6517bf085469300cbf36}"
 
 # Detect base branch from GitHub Actions environment or fallback to local defaults
-if [ -n "$GITHUB_BASE_REF" ]; then
+if [ -n "${GITHUB_BASE_REF:-}" ]; then
     # In GitHub Actions PR, use the base ref (e.g., "master")
     BASE_BRANCH="${BASE_BRANCH:-origin/$GITHUB_BASE_REF}"
 else
     # Local development: default to origin/master
     BASE_BRANCH="${BASE_BRANCH:-origin/master}"
 fi
 
-FAIL_ON_BREAKING=false
 TARGET_FILE=""
-DRY_RUN=false
-
-detect_breaking_commit_marker() {
-    local commits="$1"
-
-    # Conventional Commits breaking indicators:
-    # 1) An exclamation mark in the type/scope header, e.g. feat!: ... or feat(api)!: ...
-    # 2) A BREAKING CHANGE footer in the commit body
-    if echo "$commits" | grep -Eiq '^[[:space:]]*[a-z]+(\([^)]+\))?!:'; then
-        return 0
-    fi
-
-    if echo "$commits" | grep -Eiq 'BREAKING[ -]CHANGE:'; then
-        return 0
-    fi
-
-    return 1
-}
-
-get_commit_messages() {
-    # For tests and local overrides
-    if [ -n "$COMMIT_MESSAGES" ]; then
-        echo "$COMMIT_MESSAGES"
-        return 0
-    fi
-
-    # In GitHub Actions PRs, scan all commit subjects + bodies in the PR range.
-    if [ -n "$GITHUB_BASE_REF" ]; then
-        git log --format='%s%n%b%n----' "origin/$GITHUB_BASE_REF..HEAD" 2>/dev/null || true
-        return 0
-    fi
-
-    # Local default: inspect HEAD commit only.
-    git log -1 --format='%s%n%b' 2>/dev/null || true
-}
 
 # Parse arguments
 for arg in "$@"; do
-    if [ "$arg" = "--fail-on-breaking" ]; then
-        FAIL_ON_BREAKING=true
-    elif [ "$arg" = "--dry-run" ]; then
-        DRY_RUN=true
-    elif [[ "$arg" == *.yaml ]]; then
+    if [[ "$arg" == *.yaml ]] && [ -z "$TARGET_FILE" ]; then
         TARGET_FILE="$arg"
-    fi
-done
-
-# If --fail-on-breaking not explicitly set, determine based on conventional commit markers.
-if [ "$FAIL_ON_BREAKING" = false ]; then
-    COMMIT_TEXT=$(get_commit_messages)
-    if detect_breaking_commit_marker "$COMMIT_TEXT"; then
-        echo "Detected conventional commit breaking marker ('!' in header or 'BREAKING CHANGE:' footer), allowing breaking changes"
-        FAIL_ON_BREAKING=false
-    else
-        echo "No conventional commit breaking marker found, failing on breaking changes"
-        FAIL_ON_BREAKING=true
-    fi
-fi
-
-if [ "$DRY_RUN" = true ]; then
-    if [ "$FAIL_ON_BREAKING" = true ]; then
-        echo "Mode: Failing on breaking changes"
     else
-        echo "Mode: Allowing breaking changes"
+        echo "Error: unsupported argument '$arg'" >&2
+        exit 2
     fi
-    echo "Dry run mode, exiting after commit message check"
-    exit 0
-fi
+done
 
 echo "Starting API diff check..."
 
@@ -101,12 +41,18 @@ if [ ! -f "xero_accounting.yaml" ]; then
     exit 1
 fi
 
-# Fetch master if not already done
-git fetch "${BASE_BRANCH%%/*}" "${BASE_BRANCH##*/}" 2>/dev/null || echo "Warning: Could not fetch ${BASE_BRANCH}"
+# Refresh and verify the exact base. A missing or stale base must not turn this
+# compatibility check into a pass.
+# Strip only the remote prefix: a base such as origin/feature/prism-changes
+# must fetch "feature/prism-changes", not just the last path segment.
+BASE_REMOTE="${BASE_BRANCH%%/*}"
+BASE_REF="${BASE_BRANCH#"$BASE_REMOTE"/}"
+git fetch "$BASE_REMOTE" "$BASE_REF"
+git rev-parse --verify "${BASE_BRANCH}^{commit}" >/dev/null
 
 # Create temp directory for master branch files (outside repo to avoid overlap with /current mount)
 TEMP_DIR=$(mktemp -d)
-trap "rm -rf $TEMP_DIR" EXIT
+trap 'rm -rf "$TEMP_DIR"' EXIT
 
 # Get list of xero*.yaml files (excluding any master_*.yaml files)
 if [ -n "$TARGET_FILE" ]; then
@@ -118,15 +64,22 @@ if [ -n "$TARGET_FILE" ]; then
     files="$TARGET_FILE"
     echo "Running diff for single file: $TARGET_FILE"
 else
-    # All xero*.yaml files
-    files=$(ls xero*.yaml 2>/dev/null | grep -v "^master_")
+    # Union of the specs present in the base ref and in the working tree.
+    # Enumerating the working tree alone would silently skip a spec deleted or
+    # renamed in this PR, so the removal of an entire API would never be
+    # compared and the check would pass.
+    files=$( {
+        ls xero*.yaml 2>/dev/null || true
+        git ls-tree -r --name-only "$BASE_BRANCH" 2>/dev/null || true
+    } | grep -E '^xero[^/]*\.yaml$' | grep -v "^master_" | sort -u || true )
     if [ -z "$files" ]; then
         echo "No xero*.yaml files found"
         exit 1
     fi
 fi
 
 BREAKING_CHANGES_FOUND=false
+EXECUTION_FAILED=false
 FILES_WITH_BREAKING_CHANGES=()
 TOTAL_FILES=0
 PROCESSED_FILES=0
@@ -142,18 +95,26 @@ for file in $files; do
     echo ""
     echo "========== $file =========="
 
-    # Get the file from master branch
-    if ! git show "$BASE_BRANCH:$file" > "$TEMP_DIR/$file" 2>/dev/null; then
-        echo "ℹ️  New file (does not exist in master branch)"
+    # A spec present in the base but absent at head removes the entire API
+    # surface it described. That is the most breaking change possible and must
+    # never be reported as a pass.
+    if [ ! -f "$file" ]; then
+        echo "❌ Spec removed (present in $BASE_BRANCH, absent at HEAD)"
+        BREAKING_CHANGES_FOUND=true
+        FILES_WITH_BREAKING_CHANGES+=("$file")
+        PROCESSED_FILES=$((PROCESSED_FILES + 1))
         continue
     fi
 
-    # Verify the temp file was created
-    if [ ! -f "$TEMP_DIR/$file" ]; then
-        echo "❌ Failed to create temp file"
+    # Get the file from master branch
+    if ! git cat-file -e "$BASE_BRANCH:$file" 2>/dev/null; then
+        echo "ℹ️  New file (does not exist in master branch)"
+        PROCESSED_FILES=$((PROCESSED_FILES + 1))
         continue
     fi
 
+    git show "$BASE_BRANCH:$file" > "$TEMP_DIR/$file"
+
     # Note: oasdiff has some non-deterministic behavior in change counts due to
     # unordered map iteration in Go. Error counts are consistent, but warning
     # counts may vary by ~2-3% between runs. This is a known limitation.
@@ -170,7 +131,8 @@ for file in $files; do
     if [ $CHANGELOG_EXIT -eq 0 ]; then
         echo "✓ Changelog generated successfully"
     else
-        echo "⚠ Could not generate changelog (exit code: $CHANGELOG_EXIT)"
+        echo "❌ Could not generate changelog (exit code: $CHANGELOG_EXIT)"
+        EXECUTION_FAILED=true
     fi
 
     # Run breaking changes check
@@ -201,26 +163,26 @@ echo "Processed: $PROCESSED_FILES/$TOTAL_FILES files"
 echo "========================================"
 
 # Summary
+if [ "$EXECUTION_FAILED" = true ]; then
+    echo "❌ API diff execution failed"
+    exit 1
+fi
+
 if [ "$BREAKING_CHANGES_FOUND" = true ]; then
     echo ""
     echo "❌ Breaking changes detected in the following files:"
     for file in "${FILES_WITH_BREAKING_CHANGES[@]}"; do
         echo "   - $file"
         # Output GitHub Actions annotation
-        if [ -n "$GITHUB_ACTIONS" ]; then
+        if [ -n "${GITHUB_ACTIONS:-}" ]; then
             echo "::warning file=${file}::Breaking changes detected in this API spec file"
         fi
     done
 
-    if [ "$FAIL_ON_BREAKING" = true ]; then
-        echo ""
-        echo "Exiting with error due to breaking changes"
-        exit 1
-    else
-        echo ""
-        echo "Note: Not failing build (use --fail-on-breaking to fail on breaking changes)"
-    fi
+    echo ""
+    echo "Exiting with error due to breaking changes"
+    exit 1
 else
     echo ""
     echo "✓ No breaking changes detected across all files"
-fi
+fi