Skip to content

fix: report comparison errors instead of no changes - #198

Closed
lucas-monteiro-g wants to merge 2 commits into
oasdiff:mainfrom
lucas-monteiro-g:fix/report-comparison-errors
Closed

fix: report comparison errors instead of no changes#198
lucas-monteiro-g wants to merge 2 commits into
oasdiff:mainfrom
lucas-monteiro-g:fix/report-comparison-errors

Conversation

@lucas-monteiro-g

Copy link
Copy Markdown

Summary

  • Distinguish clean comparisons, detected breaking changes, and comparison failures.
  • Report unresolved external references instead of emitting No breaking changes.
  • Add comparison-status and comparison-error outputs.
  • Add a regression fixture for the unresolved HorizontalStoreTileGridData reference.

This intentionally keeps the unresolved reference in the fixture so the failure remains observable.

@reuvenharrison

Copy link
Copy Markdown
Contributor

Thanks for this, and sorry it was closed without a note. The bug you found is real and it is now fixed in #199.

You were right that a failed comparison must never read as a clean result. Worth adding that the step did already fail (the entrypoint propagates oasdiff's exit code at the end), so CI was not silently passing. What was wrong is that everything a person reads, the step output, the job summary and the PR comment, said "No breaking changes" for a run that never completed. That is still worth fixing, and it also matters for anyone using continue-on-error.

The reason #199 looks much smaller is that the changelog action already solved this: on exit code >= 2 it prints the error, adds the external-refs remedy when the code is 123, and exits right there, so it never reaches its clean-result branch. breaking and diff were just missing that early exit. Copying it fixes both actions in a few lines, and it keeps the specific 123 message as the most prominent thing the user sees rather than replacing it with a generic failure notice.

For that reason #199 leaves out the comparison-status and comparison-error outputs. Action outputs are permanent contract once released, and the step's own failure plus the existing annotation already signal the condition.

Your regression test is kept in spirit, one job per action. Two changes: the fixture uses a $ref to a file that does not exist rather than a path that escapes the repository root, which is sensitive to how the workspace is checked out, and it uses a generic schema name since fixtures here avoid names carried over from real specs.

You are credited in the commit and the PR. Thanks again for taking the time to report it with a reproducing fixture, that is what made it quick to confirm.

reuvenharrison added a commit that referenced this pull request Aug 10, 2026
* Stop reporting a failed comparison as a clean result

When oasdiff cannot load or parse a spec (exit code >= 2), the breaking and
diff actions printed the error annotation and then carried on to write
'No breaking changes' / 'No changes'. The step still failed, so CI caught
it, but the output, job summary and PR comment all described the run as
clean, which is misleading for anyone reading them or using
continue-on-error.

Exit as soon as the comparison fails, which is what the changelog action
already does. Nothing downstream has anything to report about a comparison
that never ran.

Adds a regression test per action using a spec with an unresolvable $ref,
asserting the step fails and never emits the clean-result text.

Reported by @lucas-monteiro-g in #198.

* Stop validate publishing zero counts for a spec that never loaded

validate had the same fallthrough as breaking and diff, but it shows up
in the outputs rather than in the text. After printing the annotation it
carried on to Run 2, which fails the same way and prints nothing, and
the count that reads it cannot tell that from a valid spec:

    # A valid spec prints nothing, so the count stays 0.

So an unloadable spec published findings=0, error_count=0,
warning_count=0, info_count=0. The step still failed, but a workflow
gating on those outputs reads a broken spec as a clean one, which is
worse than the text case because it is machine-readable.

Same early exit as the other three. The existing error-annotation test
loops over validate already, but only asserts the annotation is emitted,
which it always was; the new job asserts the counts are absent.
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.

2 participants