Skip to content

Announce public API changes from main only - #7500

Merged
facumenzella merged 3 commits into
mainfrom
facu/api-diff-slack-main-only
Aug 25, 2026
Merged

Announce public API changes from main only#7500
facumenzella merged 3 commits into
mainfrom
facu/api-diff-slack-main-only

Conversation

@facumenzella

@facumenzella facumenzella commented Aug 25, 2026

Copy link
Copy Markdown
Member

Motivation

#feed-sdk-new-api is noisy: every PR run gets posted so a single change shows up in the feed once per push. Posting from main instead means each change lands there once, when it's actually about to get shipped.

AI session context

AI Context

Metadata

  • PR: Announce public API changes from main only #7500
  • Branch: facu/api-diff-slack-main-only
  • Author / human owner: facumenzella
  • Agent(s): Claude Code, Opus 5 (1M context)
  • Session source: current conversation
  • Generated: 2026-08-25
  • Context document version: 2

Goal

Stop #feed-sdk-new-api from being spammed by per-PR-run announcements, without weakening the PR-time public API checks.

Initial Prompt

"we're posting api changes on #feed-sdk-new-api but it's noisy. Can you make this only run on main? Whenever something gets merged"

(The session opened with a separate question about whether the API check should be skipped when a PR is behind its base branch. Answer: no, the gate is already merge-base anchored, so staleness cannot produce false positives. No change came out of that thread.)

Important Follow-up Prompts

  • "Can you make it run on main, comparing the current commit with the previous commit files?" — confirmed HEAD^ as the base on main.
  • "don't run it on PRs anymore, just on main", then "i don't want to drop the whole check. i just want to drop the slack message on PRs, only on main" — scoped the change to the Slack post. The whole check-api-changes job stays on PRs.
  • "pr_labels_for_api_gate still should work on PRs" / "pr:breaking-api for example should still be a thing on prs" — confirmed the gate is untouched on PR runs; only the main branch of it logs instead of failing.
  • "we don't need the merged PR number anymore… we just post a message saying new api just landed on main", then "and a link to the commit" — replaced the PR link with a commit link and reworded the headline.
  • Feedback that the comment above resolve_comparison_base was too verbose; the comments added in this branch were trimmed to one line each.

Agent Contribution

  • Traced the existing wiring: check-api-changes-* already runs in both the PR workflow and release-or-main (.circleci/default_config.yml:2309, :2757), so no CircleCI change was needed.
  • Identified that gating Slack on main alone would have silenced the feed entirely, because the merge base on main is HEAD.
  • Identified that last_announcement bails on an empty source, so dropping the link outright would have silently disabled duplicate suppression.
  • Implemented the branch-aware base, the main-only announcement, the main-side gate exemption and the commit link.
  • Wrote 8 tests, captured RED against the pre-change implementation before GREEN.

Human Decisions

  • Decision: keep the full check-api-changes job on PRs. The agent offered dropping it entirely as an option; rejected.
  • Decision: base the main-side comparison on the previous commit.
  • Decision: the Slack message links the commit, not the PR. The commit page already carries the PR link.
  • Decision: the message should say the change landed on main.

Key Implementation Decisions

  • Decision: HEAD^ as the comparison base on main, merge base everywhere else.
    • Rationale: the merge base on main is HEAD, so the diff would always be empty.
    • Rejected: comparing against origin/main's tip on PRs. That is what the merge base already avoids, and it would reintroduce false positives on stale branches.
  • Decision: skip the breaking-change gate on main only.
    • Rationale: pr_labels_for_api_gate returns [] without PR context, so once main had a real diff, every intentional pr:breaking-api change would have turned main red. PR runs are unchanged and still fail.
    • Rejected: reading the label off the merged PR on main. More moving parts for a gate the PR run already enforced.
  • Decision: source is the commit link.
    • Rationale: it satisfies the "link the commit" ask and keeps duplicate suppression alive, since last_announcement returns nil on an empty source. The sha is a better dedup key than the PR link, it is unique per merge.
    • Rejected: parsing the PR number out of the squash merge subject (implemented first, then removed), and dropping the source entirely.

Files / Symbols Touched

  • fastlane/api_diff_helper.rb
    • Why: branch-aware base resolution and the reworded headline.
    • Symbols: resolve_comparison_base, resolve_previous_commit, current_branch, main_branch?, slack_summary, BASE_SWIFTINTERFACE_DIR (renamed from MERGE_BASE_SWIFTINTERFACE_DIR)
    • Review relevance: whether HEAD^ is the right base on main, and whether CIRCLE_BRANCH is reliable there.
  • fastlane/Fastfile
    • Why: gate the announcement and the failure on on_main, link the commit.
    • Symbols: check_api_changes, api_gate_commit_link (replaces api_gate_pr_link)
    • Review relevance: the announcement = if on_main branch returns { fingerprint: nil, notice: nil } so the PR comment keeps its shape.
  • fastlane/api_diff_helper_test.rb
    • Why: cover the new behavior.
    • Symbols: test_comparison_base_on_main_is_the_previous_commit, test_comparison_base_off_main_stays_the_merge_base, test_slack_is_announced_only_on_main, test_the_breaking_change_gate_cannot_redden_main, test_the_announcement_source_is_the_commit, and 3 others.

Dependencies / Config / Migrations

  • None. No CircleCI config change, the job already runs on main.

Validation

  • Commands run:
    • ruby fastlane/api_diff_helper_test.rb against the pre-change implementation: 146 runs, 3 failures, 5 errors (RED)
    • ruby fastlane/api_diff_helper_test.rb after: 146 runs, 497 assertions, 0 failures, 0 errors (GREEN)
    • ruby -c fastlane/Fastfile, ruby -c fastlane/api_diff_helper.rb: Syntax OK
  • Manual verification:
    • Rendered a sample slack_summary locally to check the message shape.
    • main is linear squash merges, confirmed on the last 8 commits of origin/main.
    • No shallow-clone config in .circleci/default_config.yml, so HEAD^ resolves under checkout.
  • CI:
    • Not captured at time of writing.

Validation Gaps

  • The lane cannot run outside fastlane, so the main-only announcement, the commit link and the gate exemption are pinned by structural tests that read the Fastfile, not by executing it. First real proof is the first main run after merge.
  • Never exercised against a real main build. If CIRCLE_BRANCH is not main there, the feed goes silent rather than noisy, which fails safe but silently.

Review Focus

  • Is CIRCLE_BRANCH reliably main on release-or-main runs? The whole change hangs on it.
  • The invariant behind HEAD^: does anything merge to main without the swiftinterface baselines regenerated? If so the main-side diff attributes the drift to the wrong commit.
  • release/* also runs release-or-main and now announces nothing. Intended?
  • Is dropping the gate's failure on main acceptable, or should main still fail loudly?

Risks / Reviewer Notes

  • Risk: a commit lands on main with stale api/*.swiftinterface, so HEAD^ vs HEAD misattributes the drift.
    • Evidence: the first half of check_api_changes fails any PR whose committed baselines don't match its build, so this requires the check to be bypassed.
    • Mitigation: none added, the existing PR-time freshness check is the guardrail.
  • Risk: the feed goes quiet and nobody notices.
    • Evidence: no alerting on absence of posts.
    • Mitigation: Not run. Worth a manual check on the first merge after this lands.

Non-goals / Out of Scope

  • Removing check-api-changes from PR runs. Explicitly rejected by the author.
  • Any change to the merge-base behavior, the inline PR comment, or the pr:breaking-api gate on PRs.
  • api_slack_unreachable_notice / slack_credentials_reachable? / SLACK_UNREACHABLE_NOTICE are now near-dead, they only render in a PR comment and PRs no longer announce. Left in place to keep the diff tight, flagged as a follow-up.

Omitted Context

  • Raw transcript, unrelated exploration, sensitive details, repetitive attempts, and chain-of-thought-style content were omitted.

Note

Medium Risk
Behavior depends on CIRCLE_BRANCH being main on post-merge CI and on HEAD^ being the right baseline for squash merges; mis-detection silences Slack or misattributes API drift without failing main on intentional breaks.

Overview
#feed-sdk-new-api posts only when check_api_changes runs on main, so each merge gets one Slack message instead of one per PR push. PR runs still diff baselines, update GitHub comments, and enforce pr:breaking-api; they skip Slack and return a stub announcement shape for the comment flow.

On main, the public-API gate compares HEAD vs HEAD^ (merge base would be empty). Off main it still uses the merge base with origin/main. Slack copy says changes landed on main, and the link is the commit (not the PR) for dedup on reruns. Breaking-change failures are downgraded to a log on main because there is no PR label context.

Reviewed by Cursor Bugbot for commit 023e05b. Bugbot is set up for automated code reviews on this repo. Configure here.

The #feed-sdk-new-api posts were noisy because every PR run announced.
Only main posts now, so each change lands in the feed once.

On main the merge base is HEAD itself, which would compare the commit
against itself and report nothing, so main compares against HEAD^. PRs
and release branches keep the merge base, the inline comment and the
breaking-change gate. main skips the gate, it has no PR to carry the
label and the PR run already enforced it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017vZGFWc7FkaZjuzSDdqzDp
The message now says the change landed on main and links the commit,
which already carries the PR link. Dropping the source entirely was not
an option: last_announcement bails on an empty source, so a rerun of the
same main job would post a second time. The commit sha is a better key
than the PR link anyway, it is unique per merge.

Also trims the comments added in this branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017vZGFWc7FkaZjuzSDdqzDp
@facumenzella
facumenzella marked this pull request as ready for review August 25, 2026 09:03
@facumenzella
facumenzella requested a review from a team as a code owner August 25, 2026 09:03

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d688eb7. Configure here.

changed = unbroken_modifications(modifications, breaks)
headline = if breaks.any?
gate_blocked?(breaks, labels) ? ":warning: *Breaking public API changes*" : ":warning: *Breaking public API changes* (allowed by label)"
gate_blocked?(breaks, labels) ? ":warning: *Breaking public API landed on main*" : ":warning: *Breaking public API landed on main* (allowed by label)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaks always look unapproved

Low Severity

Slack announcements now run only on main, where pr_labels_for_api_gate always returns no labels. slack_summary still picks its breaking headline via gate_blocked?, so intentional pr:breaking-api landings always post the unapproved wording and never the allowed-by-label variant.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d688eb7. Configure here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we only merge approved prs, and only post merged to main, then all posts are approved 🤷

@AlvaroBrey AlvaroBrey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Once we port this to Android too, we can remove a bunch of the deduplication code I did, since it no longer makes sense in main where we're guaranteed 1 post per PR. But that can be done later

@facumenzella
facumenzella enabled auto-merge (squash) August 25, 2026 13:25
@facumenzella
facumenzella merged commit 695c592 into main Aug 25, 2026
18 of 19 checks passed
@facumenzella
facumenzella deleted the facu/api-diff-slack-main-only branch August 25, 2026 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants