Repository navigation
Stop the release version constants failing the API compatibility gate - #941
Conversation
Every release bump changes `Version`, and `DefaultUserAgent` is `"basecamp-sdk-go/" + Version + " (api:" + APIVersion + ")"` so it follows. apidiff classifies an exported constant's value change as incompatible, which for these two is true and beside the point — a consumer pinning the literal is asserting "compiled against exactly 0.19.0", and no release can preserve that. Changing is what the constant is for. The reason to fix it is the gate's integrity, not convenience. Cutting v0.20.0 (#940) failed this required check with 12/12 contexts green and both reported lines version constants; the printed remedy is to apply `breaking` and re-run. Every release hits that, so the label becomes a reflex on the one PR where a real break is least likely to be read — the diff is machine-generated and skimmed. The label should mean a real break was looked at and accepted, which it cannot mean while it is also the routine cost of shipping. The two lines are partitioned out of the verdict only. They are still printed, under their own heading, and the full "All API Changes" report is untouched. The exemption is narrow: only `value changed`, only those two symbols. Removing `Version`, changing its type or renaming it still fails, as does any other symbol's value change. grep's exit 1 ("no lines matched") is an ordinary outcome for both partitions and must not abort under `set -e`; 2 or above is a tool failure and must not read as an empty partition, so it aborts loudly — the same distinction run_apidiff already draws. Verified against the real #940 output plus five neighbours: the two version lines alone pass; those lines beside a removed method still fail; `Version` removed, `Version` retyped and another constant's value change each still fail; a clean 0-byte report stays clean. actionlint and zizmor clean.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
github-actions
Bot
added
the
github-actions
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The filter also suppresses arbitrary DefaultUserAgent value changes unrelated to releases.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Narrows Go API compatibility verdicts so routine release-version changes remain visible without failing the release gate.
Changes:
- Separates release constants from other incompatible changes.
- Preserves both dedicated and complete API reports.
- Handles
grepno-match and failure statuses distinctly.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
.github/workflows/test.yml |
Partitions version-constant findings from the breaking verdict. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5674ccd7b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Both reviewers found the same hole, independently: keying the exemption on the symbol name waived every `DefaultUserAgent` value change, not only the one a release bump causes. Rewriting the header to "unexpected-client", or dropping the `(api:...)` component, emits the same generic `value changed` line and would have passed the gate silently. The user-agent line is now exempt only when it is exactly what this bump's `Version` change implies: same `basecamp-sdk-go/` prefix, same API-version component on both sides, version component moving from the old value to the new one. A user-agent change with no `Version` change beside it is never exempt. Naming the format here as well as in client.go is deliberate. If the format changes this stops matching and the gate fails, which is the direction a duplicated fact should fail in. awk rather than grep because the decision is no longer a line-level regex — it reads the `Version` line to decide about the user-agent line — and awk avoids the heredoc indentation trap inside a YAML block scalar. Verified against seven cases including both reviewers' counterexamples: the real v0.20.0 bump passes; user agent to "unexpected-client" with a version bump fails; the API component dropped fails; a user-agent change with no version bump fails; a bump beside a removed method fails; `Version: removed` fails; a clean 0-byte report stays clean. My own verification missed this. I tested the symbol axis and the change-kind axis and never the value axis — the exemption was only ever sound for values derived from the bump, and I never asserted the derivation.
|
Review threads: 2 resolved (2 fixed). Head is Both reviewers found the same defect independently, and they were right about it.
Fixed in Switched from grep to awk because the decision is no longer a line-level regex: it reads the VerificationSeven cases, including both reviewers' counterexamples verbatim:
Worth recordingMy own verification missed this. I tested the symbol axis and the change-kind axis and never the value axis — and wrote "narrow on purpose" in the commit message while it wasn't narrow. The exemption was only ever sound for values derived from the bump, and the first version never asserted the derivation it depended on. Reviewer stateCodex completed a clean pass on |
Cutting v0.20.0 (#940) failed the required
API Compatibilitycheck with all 12 required contexts green. The entire finding:scripts/bump-version.shwritesconst Version;DefaultUserAgentis"basecamp-sdk-go/" + Version + " (api:" + APIVersion + ")", so it follows by construction. Every release changes both, so every release PR fails this gate, and the printed remedy is to applybreakingand re-run.Why this is worth changing rather than living with
apidiff is not wrong. It flags an exported constant's value change because a consumer can use one in a constant expression. For these two that's true and beside the point — a consumer pinning the literal is asserting "compiled against exactly 0.19.0", which no release can preserve. Changing is what the constant is for.
The argument for fixing it is the gate's own integrity. A required check that fails on every release trains "bump PR, slap the label on" as reflex, and it does so on the one PR where a real break is least likely to be noticed: the diff is machine-generated and reviewers skim it.
breakingshould mean a real break was looked at and accepted. It cannot mean that while it is also the routine cost of shipping.What changed
The two lines are partitioned out of the verdict only. They are still printed, under their own heading, and the
All API Changesreport is untouched — nothing becomes invisible.The exemption is deliberately narrow: only
value changed, only those two symbols.grep's exit 1 ("no lines matched") is an ordinary outcome for both partitions and must not abort under
set -e; exit 2+ is a tool failure and must not be read as an empty partition, so it aborts loudly — the same distinctionrun_apidiffalready draws a few lines above.Verification
Ran the partition against the real #940 output and five neighbours:
Client.GetProject: removedVersion: removedVersion: changed from untyped string to intAPIVersion: value changed(a different constant)The third through fifth rows are the ones that matter: the exemption keys on the symbol and the kind of change, so it cannot swallow a removal, a retype, or any other constant.
actionlintclean;zizmor(same online audit CI runs) reports 0 findings.Not addressed here
Removing
breakingafter a green run still leaves that run green — the existing comment says so, and closing it needs a label-event trigger. Unchanged by this.Summary by cubic
Stops release version bumps from failing the API compatibility gate. The expected
Versionand derivedDefaultUserAgentchanges no longer count as breaking, while unrelated API changes still block the check.Behavior
DefaultUserAgentonly when its old and new values exactly follow theVersionchange and retain the same API version.Written for commit 0d3046e. Summary will update on new commits.