Skip to content

fix(ci): check-proto-changes-python can never fail - #8205

Open
Abdellah Ouadoudi (holistis) wants to merge 1 commit into
microsoft:mainfrom
holistis:fix/proto-changes-ci-never-fails
Open

fix(ci): check-proto-changes-python can never fail#8205
Abdellah Ouadoudi (holistis) wants to merge 1 commit into
microsoft:mainfrom
holistis:fix/proto-changes-ci-never-fails

Conversation

@holistis

Copy link
Copy Markdown

Why are these changes needed?

The check-proto-changes-python job in .github/workflows/checks.yml is supposed to fail CI when the generated proto files (poe gen-proto / poe gen-test-proto) are out of sync with what's committed. It currently cannot fail, regardless of the actual diff.

Root cause

- name: Check if there are uncommited changes
  id: changes
  uses: UnicornGlobal/has-changes-action@v1.0.11
- name: Process changes
  if: steps.changes.outputs.changed == 1
  run: echo "There are changes in the proto files. Please commit them."
  • UnicornGlobal/has-changes-action only sets an output (changed); by design (see its README) it never fails the step itself - it exists purely so a caller can branch on the result.
  • The Process changes step only runs echo .... There is no exit 1 (or any other failing command), so the step - and therefore the job - always reports success, even when changed == 1.

I verified the has-changes-action's action.yml (docker action, only produces an output) and its README (the linked example is literally run: echo "Changes exist", i.e. a demo of the output variable, not a working check) to confirm there is no other path that could fail the job.

How it got here

Original check (added in #451, refined in a follow-up commit) worked correctly:

- name: Evaluate if there are changes
  run: |
    if [[ `git status --porcelain` ]]; then
      echo "There are changes that need to be generated and commit for the proto files"
      git --no-pager diff
      exit 1
    fi
  shell: bash

Commit 5be7ac7 ("Move reset from a message to a command", #4073 - an unrelated PR) swapped this for the has-changes-action README's demo snippet verbatim, dropping the exit 1. Net effect: this job has shown green on every run since 2024-11-06, independent of whether the generated protos actually match what's committed.

I confirmed on a recent successful run on main (workflow run 24054593367, job check-proto-changes-python) that the job exists and passes today; the logic above shows why a "success" here carries no information.

Fix

Restore a real failure path while keeping the current step structure (minimal diff, no third-party-action swap):

- name: Fail if proto files are not up to date
  if: steps.changes.outputs.changed == 1
  run: |
    echo "There are changes in the proto files. Please run 'poe gen-proto' and 'poe gen-test-proto' and commit the result."
    git --no-pager diff
    exit 1

Verified the fix locally by extracting the exact step body before/after as a shell script and running both with the guard condition true (i.e. simulating "proto files are stale"):

  • Before: echo ... -> exit code 0 (bug: job stays green)
  • After: echo ...; git --no-pager diff; exit 1 -> exit code 1 (job correctly fails)

The if: guard itself is untouched, so the happy path (no diff -> changed == 0 -> step skipped -> job green) is unaffected.

Related issue number

None filed - this is a small, self-contained CI fix with git-history evidence for the regression; happy to open an issue first if preferred.

Checks

🤖 Generated with Claude Code

…stale

The `Process changes` step only echoed a message when
`has-changes-action` detected uncommitted changes; it never exited
non-zero. Combined with `UnicornGlobal/has-changes-action` itself only
setting an output (by design, per its own README) and not failing the
step, the `check-proto-changes-python` job cannot fail, no matter how
out of sync the generated proto files are with what's committed.

This is a regression from commit 5be7ac7 ("Move reset from a
message to a command", microsoft#4073), which replaced the original inline
`git status --porcelain` + `exit 1` check (added in microsoft#451, refined in
a follow-up commit to also print the diff) with the has-changes-action
README's demo snippet verbatim - the demo intentionally omits a
failure step since the action's job is only to expose the `changed`
output for callers to act on.

Net effect: the job has shown green on every run since 2024-11-06
regardless of whether `poe gen-proto` / `poe gen-test-proto` output
matches what's committed, silently defeating the check's only purpose.

Fix: add `exit 1` (and keep printing the diff, as the pre-regression
step did) so the job actually fails when generated proto files are
not up to date.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant