Skip to content

Commit c9dbb73

Browse files
reuvenharrisonclaude
reuvenharrison
and
claude
authored
fix(review): format-independent no-change detection; skip --open in composed mode (#164)
* fix(review): format-independent no-change detection; skip --open in composed mode Two fixes to the free side-by-side review the breaking/changelog actions post. 1. changelog: detect "no changes" independently of --format. The old test grepped the first output line for "^No ", which only holds for the default text format. With format: json or yaml an empty changelog renders as "[]", and with markup it's a header line followed by "No changes detected" -- both slipped past the test, so every clean PR got a spurious review link (and an uploaded comparison). Detection now uses a json probe ("[]" == no changes), which is unambiguous across formats. To make that probe (and the --open upload) safe, format/template are pulled out of the semantic $flags into a separate $fmt_flags used only for the user-facing render -- otherwise the probe would pass --format twice and, worse, error out when a template is set (--template is rejected for json). 2. breaking + changelog: in composed mode (-c) the action no longer runs --open. The encrypted review carries exactly two specs and can't represent a glob of many files, so --open now returns a clear non-fatal message in the CLI; the action would otherwise surface the generic "couldn't upload, re-run" warning. Emit a single ::notice:: explaining the review isn't available in composed mode instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(review): drop stale "reuse the same semantic flags" comment The sentence was a remnant of an abandoned approach and added no information (passing $flags to the --open run is plain from the code). Keep the part of the comment that still explains the real subtleties: --open prints the URL on stdout, the browser-open soft-fails in CI, we grep the URL by its path shape, and the run is tolerated as non-fatal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(changelog): detect changes via --fail-on=INFO exit code, not json parsing Replace the json-render "[]" probe with oasdiff's own changes-present signal: run changelog with --fail-on=INFO and read the exit code (1 = at least one change at or above --level, 0 = empty). This is format-proof by construction (no output parsing) and is the canonical "are there changes" check. --level stays in the probe so detection matches what the user sees; the explicit --fail-on overrides any config fail-on for the probe only, and the user-facing render is untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(breaking): detect changes via --fail-on=WARN exit code, not "^No " text The "^No " first-line heuristic assumed the breaking output is always text, but .oasdiff.yaml can set 'format' (the CLI flag exists even though the action has no format input), so a config format=json/html/markup makes an empty result render as "[]" / "<!DOCTYPE...>" / a header line and the heuristic flags a clean PR as having changes (spurious review + upload every run). Detect via oasdiff's exit code with --fail-on=WARN instead (WARN is breaking's display level and the lowest fail-on it accepts): exit 1 = at least one breaking change, 0 = none. Matches the changelog approach. The explicit --fail-on overrides any config fail-on for the probe only; Run 1's authoritative gate exit code is untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(review): harden actions against disruptive .oasdiff config .oasdiff.yaml can set most oasdiff flags, and the action's control flow assumed some of them. Override the ones the logic depends on, on the command line (cmd-line beats config): - composed: pin --composed=<input> in $flags (all runs). A config composed:true would otherwise run composed mode while the action's --open guard / flag building still think it's off, desyncing them (the review run would fail the new CLI composed check or build a bogus review). - template: pin --template= on the runs that force/assume a non-template format and whose output the action depends on -- the changelog/breaking change-probes (default text), both --open runs (default text), and breaking's githubactions annotations run. A config 'template' is rejected for those formats and would make a probe a false negative, drop the review URL, or (via || true) silently suppress the breaking annotations. - changelog: treat exit 1 as non-fatal (>=2 are real errors), matching breaking. A fail-on set in .oasdiff.yaml would otherwise make the changelog main run exit 1 and abort before the review is posted. (--fail-on= can't be pinned: empty is rejected.) Note: 'open' cannot be set via config -- it's not in oasdiff's Config struct, so UnmarshalExact rejects it; no override needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent dc68976 commit c9dbb73

2 files changed

Lines changed: 111 additions & 63 deletions

File tree

‎breaking/entrypoint.sh‎

Lines changed: 47 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,10 @@ fi
143143
if [ -n "$filter_extension" ]; then
144144
flags="$flags --filter-extension $filter_extension"
145145
fi
146-
if [ "$composed" = "true" ]; then
147-
flags="$flags -c"
148-
fi
146+
# Pin composed to the action input so a 'composed' setting in .oasdiff.yaml can't
147+
# desync oasdiff's mode from the action's logic (the --open guard and flag
148+
# building both key off this input). A cmd-line flag overrides config.
149+
flags="$flags --composed=$composed"
149150
if [ "$flatten_allof" = "true" ]; then
150151
flags="$flags --flatten-allof"
151152
fi
@@ -186,8 +187,10 @@ rm -f "$_err"
186187
# Run 2: render annotations to stdout via --format githubactions so
187188
# GitHub parses them onto the PR's "Files changed" tab. Tolerate
188189
# non-zero exit (could be triggered by oasdiff.yaml fail-on); the
189-
# authoritative exit code is from Run 1.
190-
oasdiff breaking "$base" "$revision" $flags --format githubactions || true
190+
# authoritative exit code is from Run 1. --template= overrides a 'template'
191+
# set in .oasdiff.yaml, which is rejected for the githubactions format and
192+
# would (via || true) silently suppress the annotations.
193+
oasdiff breaking "$base" "$revision" $flags --format githubactions --template= || true
191194

192195
# *** GitHub Action step output ***
193196

@@ -199,7 +202,16 @@ oasdiff breaking "$base" "$revision" $flags --format githubactions || true
199202
delimiter=$(cat /proc/sys/kernel/random/uuid | tr -d '-')
200203
echo "breaking<<$delimiter" >>"$GITHUB_OUTPUT"
201204

202-
if [ -n "$breaking_changes" ] && ! echo "$breaking_changes" | head -n 1 | grep -q "^No "; then
205+
# Whether there are breaking changes is read off oasdiff's exit code with
206+
# --fail-on=WARN (breaking renders WARN-and-above, and WARN is the lowest level
207+
# it accepts for fail-on): exit 1 means at least one breaking change, 0 means
208+
# none. This is format-proof; parsing $breaking_changes is not, because its shape
209+
# follows any 'format' set in .oasdiff.yaml (e.g. json renders "[]", not "No ...").
210+
# The explicit --fail-on overrides a config fail-on for this probe only; Run 1's
211+
# authoritative gate exit code is untouched.
212+
changes_exit=0
213+
oasdiff breaking "$base" "$revision" $flags --fail-on=WARN --template= >/dev/null 2>&1 || changes_exit=$?
214+
if [ "$changes_exit" -eq 1 ]; then
203215
write_output "$(echo "$breaking_changes" | head -n 1)" "$breaking_changes"
204216

205217
free_review_url=""
@@ -211,25 +223,36 @@ if [ -n "$breaking_changes" ] && ! echo "$breaking_changes" | head -n 1 | grep -
211223
# entirely, so no spec ever leaves CI; the breaking-change detection and
212224
# the inline annotations are unaffected either way.
213225
if [ "$review" != "false" ]; then
214-
# Reuse the same semantic flags as the diff above so the uploaded
215-
# comparison matches. --open prints the review URL on stdout; in CI the
216-
# browser-open step soft-fails. We grep the /review/e/ URL out by its
217-
# stable path shape (not by surrounding prose). Tolerate a non-zero
218-
# exit / no match so `set -e` doesn't abort the run.
219-
free_review_url=$(oasdiff breaking "$base" "$revision" $flags --open 2>/dev/null \
220-
| grep -oE 'https://[^[:space:]]+/review/e/[^[:space:]]+' | head -n 1) || true
221-
if [ -n "$free_review_url" ]; then
222-
echo "### 📋 [View these breaking changes in a side-by-side review](${free_review_url})" >> "$GITHUB_STEP_SUMMARY"
223-
# Also surface the link on the PR itself (best-effort) so reviewers
224-
# don't have to find the job summary.
225-
post_review_comment "$free_review_url"
226+
if [ "$composed" = "true" ]; then
227+
# Composed mode (-c) diffs globs of many files; the side-by-side
228+
# review represents exactly two specs, so --open can't build it.
229+
# Say so once instead of running --open only to hit the generic
230+
# "couldn't upload" warning below.
231+
echo "::notice::oasdiff: the side-by-side review isn't available in composed mode (-c). The breaking-change report above is unaffected."
226232
else
227-
# review was requested but no link came back: an offline runner,
228-
# oasdiff.com unreachable, or an older oasdiff in the base image.
229-
# Warn rather than emit a link -- there's no useful local fallback
230-
# (if the upload failed because the host is unreachable, a manual
231-
# run would fail the same way), and the report above still stands.
232-
echo "::warning::oasdiff: couldn't upload the side-by-side review (the breaking-change report still ran). Re-run the job, or set 'review: false' to skip the upload."
233+
# --open prints the review URL on stdout; in CI the browser-open
234+
# step soft-fails. We grep the /review/e/ URL out by its stable path
235+
# shape (not by surrounding prose). Tolerate a non-zero exit / no
236+
# match so `set -e` doesn't abort the run. --template= overrides a
237+
# 'template' set in .oasdiff.yaml, which would otherwise error this
238+
# render (templates are rejected for the default text format) and
239+
# yield no URL.
240+
free_review_url=$(oasdiff breaking "$base" "$revision" $flags --open --template= 2>/dev/null \
241+
| grep -oE 'https://[^[:space:]]+/review/e/[^[:space:]]+' | head -n 1) || true
242+
if [ -n "$free_review_url" ]; then
243+
echo "### 📋 [View these breaking changes in a side-by-side review](${free_review_url})" >> "$GITHUB_STEP_SUMMARY"
244+
# Also surface the link on the PR itself (best-effort) so
245+
# reviewers don't have to find the job summary.
246+
post_review_comment "$free_review_url"
247+
else
248+
# review was requested but no link came back: an offline runner,
249+
# oasdiff.com unreachable, or an older oasdiff in the base image.
250+
# Warn rather than emit a link -- there's no useful local
251+
# fallback (if the upload failed because the host is unreachable,
252+
# a manual run would fail the same way), and the report above
253+
# still stands.
254+
echo "::warning::oasdiff: couldn't upload the side-by-side review (the breaking-change report still ran). Re-run the job, or set 'review: false' to skip the upload."
255+
fi
233256
fi
234257
fi
235258
else

‎changelog/entrypoint.sh‎

Lines changed: 64 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -134,9 +134,10 @@ fi
134134
if [ -n "$filter_extension" ]; then
135135
flags="$flags --filter-extension $filter_extension"
136136
fi
137-
if [ "$composed" = "true" ]; then
138-
flags="$flags -c"
139-
fi
137+
# Pin composed to the action input so a 'composed' setting in .oasdiff.yaml can't
138+
# desync oasdiff's mode from the action's logic (the --open guard and flag
139+
# building both key off this input). A cmd-line flag overrides config.
140+
flags="$flags --composed=$composed"
140141
if [ "$flatten_allof" = "true" ]; then
141142
flags="$flags --flatten-allof"
142143
fi
@@ -149,16 +150,22 @@ fi
149150
if [ "$case_insensitive_headers" = "true" ]; then
150151
flags="$flags --case-insensitive-headers"
151152
fi
153+
if [ -n "$level" ]; then
154+
flags="$flags --level $level"
155+
fi
156+
# format and template are presentation-only. Keep them out of $flags so the
157+
# changes probe and the --open upload below run on the semantic flags only: the
158+
# probe doesn't need the user's format, and --open renders the changelog as json
159+
# internally where --template is rejected (it would error the upload). $fmt_flags
160+
# is applied only to the user-facing changelog render.
161+
fmt_flags=""
152162
if [ -n "$format" ]; then
153-
flags="$flags --format $format"
163+
fmt_flags="$fmt_flags --format $format"
154164
fi
155165
if [ -n "$template" ]; then
156-
flags="$flags --template $template"
157-
fi
158-
if [ -n "$level" ]; then
159-
flags="$flags --level $level"
166+
fmt_flags="$fmt_flags --template $template"
160167
fi
161-
echo "flags: $flags"
168+
echo "flags: $flags, presentation flags: $fmt_flags"
162169

163170
# *** github action step output ***
164171

@@ -172,18 +179,14 @@ echo "changelog<<$delimiter" >>"$GITHUB_OUTPUT"
172179

173180
exit_code=0
174181
_err=$(mktemp)
175-
if [ -n "$flags" ]; then
176-
output=$(oasdiff changelog "$base" "$revision" $flags 2>"$_err") || exit_code=$?
177-
else
178-
output=$(oasdiff changelog "$base" "$revision" 2>"$_err") || exit_code=$?
179-
fi
180-
if [ "$exit_code" -ne 0 ]; then
182+
output=$(oasdiff changelog "$base" "$revision" $flags $fmt_flags 2>"$_err") || exit_code=$?
183+
# Only codes >=2 are real errors. Exit 1 is a fail-on result (the changelog
184+
# action has no fail-on input, but .oasdiff.yaml can set one); the changelog
185+
# action doesn't gate, so don't let it abort the run or suppress the review below.
186+
if [ "$exit_code" -ge 2 ]; then
181187
[ -s "$_err" ] && cat "$_err" >&2
182-
# Promote a genuine failure to a Checks-tab annotation. Exit 1 is the
183-
# intended fail-on result (not an error); only codes >=2 are real errors.
184-
if [ "$exit_code" -ge 2 ] && [ -s "$_err" ]; then
185-
echo "::error::$(tr '\n' ' ' < "$_err")"
186-
fi
188+
# Promote a genuine failure to a Checks-tab annotation.
189+
[ -s "$_err" ] && echo "::error::$(tr '\n' ' ' < "$_err")"
187190
# Exit code 123 = oasdiff refused a disallowed external $ref (stable
188191
# contract, not message text). Surface the action-specific remedy.
189192
if [ "$exit_code" -eq 123 ]; then
@@ -194,7 +197,18 @@ if [ "$exit_code" -ne 0 ]; then
194197
fi
195198
rm -f "$_err"
196199

197-
if [ -n "$output" ] && ! echo "$output" | head -n 1 | grep -q "^No "; then
200+
# Decide whether there are changes independently of --format. The user-facing
201+
# $output varies by format (json/yaml render "[]", markup adds a header line), so
202+
# parsing it is fragile. Instead read oasdiff's exit code with --fail-on=INFO:
203+
# exit 1 means the changelog has at least one change at or above --level, exit 0
204+
# means it's empty. --level stays in $flags so detection matches what the user
205+
# sees; the explicit --fail-on overrides any config fail-on for this probe only,
206+
# and the rendered run above is untouched. --template= overrides a 'template' set
207+
# in .oasdiff.yaml, which would otherwise error this probe (templates are rejected
208+
# for the default text format) and make it a false negative.
209+
changes_exit=0
210+
oasdiff changelog "$base" "$revision" $flags --fail-on=INFO --template= >/dev/null 2>&1 || changes_exit=$?
211+
if [ "$changes_exit" -eq 1 ]; then
198212
write_output "$output"
199213

200214
free_review_url=""
@@ -206,25 +220,36 @@ if [ -n "$output" ] && ! echo "$output" | head -n 1 | grep -q "^No "; then
206220
# entirely, so no spec ever leaves CI; the changelog output and the inline
207221
# annotations are unaffected either way.
208222
if [ "$review" != "false" ]; then
209-
# Reuse the same semantic flags as the diff above so the uploaded
210-
# comparison matches. --open prints the review URL on stdout; in CI the
211-
# browser-open step soft-fails. We grep the /review/e/ URL out by its
212-
# stable path shape (not by surrounding prose). Tolerate a non-zero
213-
# exit / no match so `set -e` doesn't abort the run.
214-
free_review_url=$(oasdiff changelog "$base" "$revision" $flags --open 2>/dev/null \
215-
| grep -oE 'https://[^[:space:]]+/review/e/[^[:space:]]+' | head -n 1) || true
216-
if [ -n "$free_review_url" ]; then
217-
echo "### 📋 [View these API changes in a side-by-side review](${free_review_url})" >> "$GITHUB_STEP_SUMMARY"
218-
# Also surface the link on the PR itself (best-effort) so reviewers
219-
# don't have to find the job summary.
220-
post_review_comment "$free_review_url"
223+
if [ "$composed" = "true" ]; then
224+
# Composed mode (-c) diffs globs of many files; the side-by-side
225+
# review represents exactly two specs, so --open can't build it.
226+
# Say so once instead of running --open only to hit the generic
227+
# "couldn't upload" warning below.
228+
echo "::notice::oasdiff: the side-by-side review isn't available in composed mode (-c). The changelog above is unaffected."
221229
else
222-
# review was requested but no link came back: an offline runner,
223-
# oasdiff.com unreachable, or an older oasdiff in the base image.
224-
# Warn rather than emit a link -- there's no useful local fallback
225-
# (if the upload failed because the host is unreachable, a manual
226-
# run would fail the same way), and the changelog above still stands.
227-
echo "::warning::oasdiff: couldn't upload the side-by-side review (the changelog still ran). Re-run the job, or set 'review: false' to skip the upload."
230+
# --open prints the review URL on stdout; in CI the browser-open
231+
# step soft-fails. We grep the /review/e/ URL out by its stable path
232+
# shape (not by surrounding prose). Tolerate a non-zero exit / no
233+
# match so `set -e` doesn't abort the run. --template= overrides a
234+
# 'template' set in .oasdiff.yaml, which would otherwise error this
235+
# render (templates are rejected for the default text format) and
236+
# yield no URL.
237+
free_review_url=$(oasdiff changelog "$base" "$revision" $flags --open --template= 2>/dev/null \
238+
| grep -oE 'https://[^[:space:]]+/review/e/[^[:space:]]+' | head -n 1) || true
239+
if [ -n "$free_review_url" ]; then
240+
echo "### 📋 [View these API changes in a side-by-side review](${free_review_url})" >> "$GITHUB_STEP_SUMMARY"
241+
# Also surface the link on the PR itself (best-effort) so
242+
# reviewers don't have to find the job summary.
243+
post_review_comment "$free_review_url"
244+
else
245+
# review was requested but no link came back: an offline runner,
246+
# oasdiff.com unreachable, or an older oasdiff in the base image.
247+
# Warn rather than emit a link -- there's no useful local
248+
# fallback (if the upload failed because the host is unreachable,
249+
# a manual run would fail the same way), and the changelog above
250+
# still stands.
251+
echo "::warning::oasdiff: couldn't upload the side-by-side review (the changelog still ran). Re-run the job, or set 'review: false' to skip the upload."
252+
fi
228253
fi
229254
fi
230255
else

0 commit comments

Comments
 (0)