From 4d9a9beb569843d2274faff6105584505239ceca Mon Sep 17 00:00:00 2001 From: Reuven Harrison Date: Mon, 10 Aug 2026 16:07:45 +0300 Subject: [PATCH 1/2] 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. --- .github/workflows/test-breaking.yaml | 27 +++++++++++++++++++++++++++ .github/workflows/test-diff.yaml | 26 ++++++++++++++++++++++++++ breaking/entrypoint.sh | 19 ++++++++++++------- diff/entrypoint.sh | 19 ++++++++++++------- specs/unresolved-ref.yaml | 11 +++++++++++ 5 files changed, 88 insertions(+), 14 deletions(-) create mode 100644 specs/unresolved-ref.yaml diff --git a/.github/workflows/test-breaking.yaml b/.github/workflows/test-breaking.yaml index 80fc579..a0b8244 100644 --- a/.github/workflows/test-breaking.yaml +++ b/.github/workflows/test-breaking.yaml @@ -341,3 +341,30 @@ jobs: echo "Expected output '$OASDIFF_ACTION_TEST_EXPECTED_OUTPUT' (include-checks must be ignored, not forwarded) but got '$output'" >&2 exit 1 fi + + oasdiff_breaking_unresolved_ref: + runs-on: ubuntu-latest + name: Test a failed comparison is not reported as clean + steps: + - name: checkout + uses: actions/checkout@v7 + - name: Run breaking action against a spec that cannot be loaded + id: test_unresolved_ref + continue-on-error: true + uses: ./breaking + with: + base: 'specs/unresolved-ref.yaml' + revision: 'specs/unresolved-ref.yaml' + review: false + - name: Assert the step failed and reported no clean result + run: | + if [ "${{ steps.test_unresolved_ref.outcome }}" != "failure" ]; then + echo "Expected the action to fail when the spec cannot be loaded" >&2 + exit 1 + fi + case "${{ steps.test_unresolved_ref.outputs.breaking }}" in + *"No breaking changes"*) + echo "A failed comparison must never be reported as 'No breaking changes'" >&2 + exit 1 + ;; + esac diff --git a/.github/workflows/test-diff.yaml b/.github/workflows/test-diff.yaml index c398e74..c9f4a30 100644 --- a/.github/workflows/test-diff.yaml +++ b/.github/workflows/test-diff.yaml @@ -122,3 +122,29 @@ jobs: # oasdiff-token, or a reachable oasdiff-service. # --------------------------------------------------------------------- + + oasdiff_diff_unresolved_ref: + runs-on: ubuntu-latest + name: Test a failed comparison is not reported as clean + steps: + - name: checkout + uses: actions/checkout@v7 + - name: Run diff action against a spec that cannot be loaded + id: test_unresolved_ref + continue-on-error: true + uses: ./diff + with: + base: 'specs/unresolved-ref.yaml' + revision: 'specs/unresolved-ref.yaml' + - name: Assert the step failed and reported no clean result + run: | + if [ "${{ steps.test_unresolved_ref.outcome }}" != "failure" ]; then + echo "Expected the action to fail when the spec cannot be loaded" >&2 + exit 1 + fi + case "${{ steps.test_unresolved_ref.outputs.diff }}" in + *"No changes"*) + echo "A failed comparison must never be reported as 'No changes'" >&2 + exit 1 + ;; + esac diff --git a/breaking/entrypoint.sh b/breaking/entrypoint.sh index a636528..a06fa86 100755 --- a/breaking/entrypoint.sh +++ b/breaking/entrypoint.sh @@ -193,13 +193,18 @@ breaking_changes=$(oasdiff breaking "$base" "$revision" $flags $fail_on_flag 2>" # Promote a genuine oasdiff failure to a Checks-tab annotation. Exit 0 is # success and exit 1 is the intended "breaking changes found" / fail-on result; # only codes >=2 (load/parse/etc.) are real errors worth surfacing here. -if [ "$exit_code" -ge 2 ] && [ -s "$_err" ]; then - echo "::error::$(tr '\n' ' ' < "$_err")" -fi -# Exit code 123 = oasdiff refused a disallowed external $ref (stable contract, -# not message text). Surface the action-specific remedy. -if [ "$exit_code" -eq 123 ]; then - echo "::error::oasdiff: this spec resolves external \$refs, which are disabled by default to prevent SSRF on untrusted pull requests. If the spec is trusted, set 'allow-external-refs: true' on the oasdiff action step." +# Stop here on a real failure: the comparison never ran, so there is nothing to +# report about it. Falling through would report a clean result for a run that +# failed. Matches the changelog action. +if [ "$exit_code" -ge 2 ]; then + [ -s "$_err" ] && echo "::error::$(tr '\n' ' ' < "$_err")" + # Exit code 123 = oasdiff refused a disallowed external $ref (stable + # contract, not message text). Surface the action-specific remedy. + if [ "$exit_code" -eq 123 ]; then + echo "::error::oasdiff: this spec resolves external \$refs, which are disabled by default to prevent SSRF on untrusted pull requests. If the spec is trusted, set 'allow-external-refs: true' on the oasdiff action step." + fi + rm -f "$_err" + exit "$exit_code" fi rm -f "$_err" diff --git a/diff/entrypoint.sh b/diff/entrypoint.sh index 7b4e183..9f9ab9a 100755 --- a/diff/entrypoint.sh +++ b/diff/entrypoint.sh @@ -91,13 +91,18 @@ fi # Promote a genuine oasdiff failure to a Checks-tab annotation. Exit 0 is # success and exit 1 is the intended fail-on-diff result; only codes >=2 # (load/parse/etc.) are real errors worth surfacing here. -if [ "$exit_code" -ge 2 ] && [ -s "$_err" ]; then - echo "::error::$(tr '\n' ' ' < "$_err")" -fi -# Exit code 123 = oasdiff refused a disallowed external $ref (stable contract, -# not message text). Surface the action-specific remedy. -if [ "$exit_code" -eq 123 ]; then - echo "::error::oasdiff: this spec resolves external \$refs, which are disabled by default to prevent SSRF on untrusted pull requests. If the spec is trusted, set 'allow-external-refs: true' on the oasdiff action step." +# Stop here on a real failure: the comparison never ran, so there is nothing to +# report about it. Falling through would report a clean result for a run that +# failed. Matches the changelog action. +if [ "$exit_code" -ge 2 ]; then + [ -s "$_err" ] && echo "::error::$(tr '\n' ' ' < "$_err")" + # Exit code 123 = oasdiff refused a disallowed external $ref (stable + # contract, not message text). Surface the action-specific remedy. + if [ "$exit_code" -eq 123 ]; then + echo "::error::oasdiff: this spec resolves external \$refs, which are disabled by default to prevent SSRF on untrusted pull requests. If the spec is trusted, set 'allow-external-refs: true' on the oasdiff action step." + fi + rm -f "$_err" + exit "$exit_code" fi rm -f "$_err" diff --git a/specs/unresolved-ref.yaml b/specs/unresolved-ref.yaml new file mode 100644 index 0000000..7adaf2e --- /dev/null +++ b/specs/unresolved-ref.yaml @@ -0,0 +1,11 @@ +# A spec whose $ref cannot be resolved, so oasdiff fails to load it. +# Used to check that a failed comparison is never reported as a clean result. +openapi: 3.0.0 +info: + title: unresolved-ref + version: 1.0.0 +paths: {} +components: + schemas: + Missing: + $ref: './no-such-file.yaml#/Missing' From a956bc26d047a20b76aa550cdb4e7ab07aeb6cbc Mon Sep 17 00:00:00 2001 From: Reuven Harrison Date: Mon, 10 Aug 2026 23:40:35 +0300 Subject: [PATCH 2/2] 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. --- .github/workflows/test-validate.yaml | 29 ++++++++++++++++++++++++++++ validate/entrypoint.sh | 19 +++++++++++------- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/.github/workflows/test-validate.yaml b/.github/workflows/test-validate.yaml index 1ac46ce..9ec00ad 100644 --- a/.github/workflows/test-validate.yaml +++ b/.github/workflows/test-validate.yaml @@ -84,3 +84,32 @@ jobs: echo "Expected the step to fail with fail-on WARN" >&2 exit 1 fi + + oasdiff_validate_unresolved_ref: + runs-on: ubuntu-latest + name: Test a failed validation is not reported as clean + steps: + - name: checkout + uses: actions/checkout@v7 + - name: Run validate action against a spec that cannot be loaded + id: test_unresolved_ref + continue-on-error: true + uses: ./validate + with: + spec: 'specs/unresolved-ref.yaml' + - name: Assert the step failed and published no finding counts + run: | + if [ "${{ steps.test_unresolved_ref.outcome }}" != "failure" ]; then + echo "Expected the action to fail when the spec cannot be loaded" >&2 + exit 1 + fi + # A spec that never loaded produces no findings to count. Publishing + # 0 would read as a clean spec to anything gating on these. + if [ "${{ steps.test_unresolved_ref.outputs.findings }}" = "0" ]; then + echo "A failed validation must not report findings=0" >&2 + exit 1 + fi + if [ "${{ steps.test_unresolved_ref.outputs.error_count }}" = "0" ]; then + echo "A failed validation must not report error_count=0" >&2 + exit 1 + fi diff --git a/validate/entrypoint.sh b/validate/entrypoint.sh index 331c144..68850f5 100755 --- a/validate/entrypoint.sh +++ b/validate/entrypoint.sh @@ -38,13 +38,18 @@ oasdiff validate $flags --format githubactions "$spec" 2>"$_err" || exit_code=$? # Promote a genuine oasdiff failure to a Checks-tab annotation. Exit 0 is # success and exit 1 is the intended fail-on result; only codes >=2 # (load/parse/etc.) are real errors worth surfacing here. -if [ "$exit_code" -ge 2 ] && [ -s "$_err" ]; then - echo "::error::$(tr '\n' ' ' < "$_err")" -fi -# Exit code 123 = oasdiff refused a disallowed external $ref (stable contract, -# not message text). Surface the action-specific remedy. -if [ "$exit_code" -eq 123 ]; then - echo "::error::oasdiff: this spec resolves external \$refs, which are disabled by default to prevent SSRF on untrusted pull requests. If the spec is trusted, set 'allow-external-refs: true' on the oasdiff validate step." +if [ "$exit_code" -ge 2 ]; then + [ -s "$_err" ] && echo "::error::$(tr '\n' ' ' < "$_err")" + # Exit code 123 = oasdiff refused a disallowed external $ref (stable + # contract, not message text). Surface the action-specific remedy. + if [ "$exit_code" -eq 123 ]; then + echo "::error::oasdiff: this spec resolves external \$refs, which are disabled by default to prevent SSRF on untrusted pull requests. If the spec is trusted, set 'allow-external-refs: true' on the oasdiff validate step." + fi + rm -f "$_err" + # Stop here. Run 2 below would fail the same way and print nothing, and the + # count that reads it cannot tell that from a valid spec, so the outputs + # would say findings=0 and error_count=0 for a spec that never loaded. + exit "$exit_code" fi rm -f "$_err"