fix: only treat call-signature and type changes as breaking in release metadata#32
fix: only treat call-signature and type changes as breaking in release metadata#32gjtorikian wants to merge 2 commits into
Conversation
Two improvements to the changelog produced by sdk-release-metadata.mjs, both aimed at the local "--spec-commit --sdk-repo --format changelog" flow: 1. Auto-derive the SDK compat report from the checkout when --compat-report is omitted. It snapshots the current tree (candidate) and the tree at the base ref (baseline, via a throwaway git worktree) and diffs them — no SDK regeneration. This is what surfaces SDK-surface changes like method renames, which the spec diff alone cannot see. Fails open (spec-diff-only changelog) and is skipped when --compat-report is passed, so CI is unaffected. Language is inferred from the repo basename (workos-python → python) or set via --lang. 2. Name the types in operation response/request-body changes: instead of "Changed response for `X.list`", emit "Changed response of `X.list` from `UserOrganizationMembership` to `UserOrganizationMembershipList`", read from the old/new IR (the diff report only carries a boolean). Falls back to the generic wording when a type name isn't resolvable. Verified end-to-end against workos-python PR #670 (rename + enriched response lines) and across all per-language compat reports under --strict-scopes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e metadata sdk-release-metadata.mjs marked field, enum-value, and response/request-shape changes as breaking — it inherited the spec differ's classification and trusted every breaking-severity compat symbol, including the model properties the compat snapshot tracks. That emitted feat! (major-bump) commits for what are really backend API changes. Cap them so only a changed call signature (parameters, constructor) or a removed/renamed type (model/enum/service) or removed method stays breaking. Field renames/removals, removed response data, dropped enum values, and response/request body type renames now land under Fixes. The compat path tells a model property (field — not breaking) apart from a service method using the IR's service/type names, so SDK-surface method removals/renames stay breaking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile Summary
Confidence Score: 4/5The release metadata changes need attention before merge because several remaining paths can still produce incorrect changelog or rollup output. Focused runtime checks exercised the affected CLI paths and confirmed the reported release classification issues with synthetic fixtures and controlled harnesses. scripts/sdk-release-metadata.mjs
 T-Rex LogsWhat T-Rex did
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
scripts/sdk-release-metadata.mjs:974-976
**Mixed requiredness misreports**
When the same field is made required in one model and optional in another model within the same scope, both facts now end up in the `fix` group after severity capping. This branch treats any mixed group as optional because `every((fact) => fact.madeRequired)` is false, so callers can receive a summary like `Make foo optional in SDK models` even though one of the included changes made `foo` required. Please only use the grouped required/optional wording when all facts have the same direction; mixed directions should fall back to the per-fact details.
### Issue 2 of 4
scripts/sdk-release-metadata.mjs:904
**Filtered breaks still bump**
This filters backend-only compat changes out of the metadata entries, but the release workflow still reads the raw compat report and forces `ROLLUP_TYPE=feat` / `ROLLUP_BANG=!` whenever any raw `severity == "breaking"` compat row exists. A `field_type_changed`, `return_type_changed`, or model-property `symbol_removed` that this function drops can still produce a major-bump PR because the workflow does not apply `compatChangeIsBreaking`. The filtered result needs to be the single source used for the rollup, or the workflow needs the same filtering before it counts compat breaks.
### Issue 3 of 4
scripts/sdk-release-metadata.mjs:606-616
**Required field additions break**
`field-added` is not included in the capped backend-only kinds, so a required field addition reported by `oagen diff` with `classification: "breaking"` still flows through as `feat!`. For example, adding a required `Foo.bar` model field produces a breaking release entry even though this PR’s policy treats model field shape changes as backend API fixes. This can still create major-bump changelog fragments for field additions.
```suggestion
const BACKEND_ONLY_DIFF_KINDS = new Set([
'field-added',
'field-removed',
'field-type-changed',
'field-format-changed',
'field-required-changed',
'field-access-changed',
'value-removed',
'value-modified',
'response-changed',
'request-body-changed',
]);
```
### Issue 4 of 4
scripts/sdk-release-metadata.mjs:272-295
**Historical compat uses current spec**
When the documented manual flow passes `--spec-commit --sdk-repo` without `--compat-report`, `prepareSpecInputs()` creates temporary old/new spec and IR files for that commit, but this compat path recomputes `specPath` from the current checkout. A historical changelog repair can therefore run `compat-extract` against today’s `spec/open-api-spec.yaml` instead of the requested commit’s spec, causing false compat failures or missing SDK-surface changes after the fail-open warning path.
Reviews (1): Last reviewed commit: "fix: only treat call-signature and type ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return facts.every((fact) => fact.madeRequired) | ||
| ? `Make ${code(commonField)} required in ${label} models` | ||
| : `Make ${code(commonField)} optional in ${label} models`; |
There was a problem hiding this comment.
When the same field is made required in one model and optional in another model within the same scope, both facts now end up in the fix group after severity capping. This branch treats any mixed group as optional because every((fact) => fact.madeRequired) is false, so callers can receive a summary like Make foo optional in SDK models even though one of the included changes made foo required. Please only use the grouped required/optional wording when all facts have the same direction; mixed directions should fall back to the per-fact details.
Artifacts
Repro: diff fixture with mixed requiredness directions for the same field
- Contains supporting evidence from the run (application/json; charset=utf-8).
Repro: old IR fixture placing both models in the same widget scope
- Contains supporting evidence from the run (application/json; charset=utf-8).
Repro: new IR fixture placing both models in the same widget scope
- Contains supporting evidence from the run (application/json; charset=utf-8).
Repro: JSON CLI output showing the mixed group summarized as optional
- Keeps the command output available without making the summary code-heavy.
Repro: changelog CLI output showing the mixed group rendered as optional
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/sdk-release-metadata.mjs
Line: 974-976
Comment:
**Mixed requiredness misreports**
When the same field is made required in one model and optional in another model within the same scope, both facts now end up in the `fix` group after severity capping. This branch treats any mixed group as optional because `every((fact) => fact.madeRequired)` is false, so callers can receive a summary like `Make foo optional in SDK models` even though one of the included changes made `foo` required. Please only use the grouped required/optional wording when all facts have the same direction; mixed directions should fall back to the per-fact details.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const isAsync = (change) => (/^Async(?=[A-Z])/.test(String(change.symbol ?? '')) ? 1 : 0); | ||
| const breakingChanges = (compatReport?.changes ?? []) | ||
| .filter((change) => change.severity === 'breaking') | ||
| .filter((change) => change.severity === 'breaking' && compatChangeIsBreaking(change, indexes)) |
There was a problem hiding this comment.
This filters backend-only compat changes out of the metadata entries, but the release workflow still reads the raw compat report and forces ROLLUP_TYPE=feat / ROLLUP_BANG=! whenever any raw severity == "breaking" compat row exists. A field_type_changed, return_type_changed, or model-property symbol_removed that this function drops can still produce a major-bump PR because the workflow does not apply compatChangeIsBreaking. The filtered result needs to be the single source used for the rollup, or the workflow needs the same filtering before it counts compat breaks.
Artifacts
Repro: minimal raw compat report fixture with filtered breaking field_type_changed change
- Contains supporting evidence from the run (application/json; charset=utf-8).
Repro: workflow-compatible raw severity rollup harness
- Contains supporting evidence from the run (text/x-shellscript; charset=utf-8).
Repro: sdk-release-metadata output omitting the filtered compat change
- Contains supporting evidence from the run (application/json; charset=utf-8).
Repro: command output showing empty metadata entries but feat bang rollup
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/sdk-release-metadata.mjs
Line: 904
Comment:
**Filtered breaks still bump**
This filters backend-only compat changes out of the metadata entries, but the release workflow still reads the raw compat report and forces `ROLLUP_TYPE=feat` / `ROLLUP_BANG=!` whenever any raw `severity == "breaking"` compat row exists. A `field_type_changed`, `return_type_changed`, or model-property `symbol_removed` that this function drops can still produce a major-bump PR because the workflow does not apply `compatChangeIsBreaking`. The filtered result needs to be the single source used for the rollup, or the workflow needs the same filtering before it counts compat breaks.
How can I resolve this? If you propose a fix, please make it concise.| const BACKEND_ONLY_DIFF_KINDS = new Set([ | ||
| 'field-removed', | ||
| 'field-type-changed', | ||
| 'field-format-changed', | ||
| 'field-required-changed', | ||
| 'field-access-changed', | ||
| 'value-removed', | ||
| 'value-modified', | ||
| 'response-changed', | ||
| 'request-body-changed', | ||
| ]); |
There was a problem hiding this comment.
field-added is not included in the capped backend-only kinds, so a required field addition reported by oagen diff with classification: "breaking" still flows through as feat!. For example, adding a required Foo.bar model field produces a breaking release entry even though this PR’s policy treats model field shape changes as backend API fixes. This can still create major-bump changelog fragments for field additions.
| const BACKEND_ONLY_DIFF_KINDS = new Set([ | |
| 'field-removed', | |
| 'field-type-changed', | |
| 'field-format-changed', | |
| 'field-required-changed', | |
| 'field-access-changed', | |
| 'value-removed', | |
| 'value-modified', | |
| 'response-changed', | |
| 'request-body-changed', | |
| ]); | |
| const BACKEND_ONLY_DIFF_KINDS = new Set([ | |
| 'field-added', | |
| 'field-removed', | |
| 'field-type-changed', | |
| 'field-format-changed', | |
| 'field-required-changed', | |
| 'field-access-changed', | |
| 'value-removed', | |
| 'value-modified', | |
| 'response-changed', | |
| 'request-body-changed', | |
| ]); |
Artifacts
Repro: synthetic oagen diff fixture with breaking field-added change
- Contains supporting evidence from the run (application/json; charset=utf-8).
Repro: old IR fixture used by the CLI
- Contains supporting evidence from the run (application/json; charset=utf-8).
Repro: new IR fixture with required added field used by the CLI
- Contains supporting evidence from the run (application/json; charset=utf-8).
Repro: CLI harness that runs JSON and changelog output paths
- Contains supporting evidence from the run (text/x-shellscript; charset=utf-8).
Repro: real CLI output showing feat! breaking classification
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/sdk-release-metadata.mjs
Line: 606-616
Comment:
**Required field additions break**
`field-added` is not included in the capped backend-only kinds, so a required field addition reported by `oagen diff` with `classification: "breaking"` still flows through as `feat!`. For example, adding a required `Foo.bar` model field produces a breaking release entry even though this PR’s policy treats model field shape changes as backend API fixes. This can still create major-bump changelog fragments for field additions.
```suggestion
const BACKEND_ONLY_DIFF_KINDS = new Set([
'field-added',
'field-removed',
'field-type-changed',
'field-format-changed',
'field-required-changed',
'field-access-changed',
'value-removed',
'value-modified',
'response-changed',
'request-body-changed',
]);
```
How can I resolve this? If you propose a fix, please make it concise.| const openapiRepo = args['openapi-repo'] ?? process.cwd(); | ||
| const specPath = join(openapiRepo, args['spec-path'] ?? 'spec/open-api-spec.yaml'); | ||
| const rawBase = args['sdk-base'] ?? 'origin/main...HEAD'; | ||
| const leftRef = rawBase.split(/\.\.\.?/)[0] || 'origin/main'; | ||
|
|
||
| const tmp = mkdtempSync(join(tmpdir(), 'sdk-release-compat-')); | ||
| const baselineSrc = join(tmp, 'baseline-src'); | ||
| let worktreeAdded = false; | ||
| let succeeded = false; | ||
| try { | ||
| const baselineCommit = rawBase.includes('...') | ||
| ? git(sdkRepo, ['merge-base', leftRef, 'HEAD']) | ||
| : git(sdkRepo, ['rev-parse', leftRef]); | ||
|
|
||
| git(sdkRepo, ['worktree', 'add', '--detach', baselineSrc, baselineCommit]); | ||
| worktreeAdded = true; | ||
|
|
||
| const baselineOut = join(tmp, 'baseline'); | ||
| const candidateOut = join(tmp, 'candidate'); | ||
| mkdirSync(baselineOut, { recursive: true }); | ||
| mkdirSync(candidateOut, { recursive: true }); | ||
|
|
||
| const extract = (sdkPath, output) => | ||
| run('npx', ['oagen', 'compat-extract', '--lang', lang, '--sdk-path', sdkPath, '--output', output, '--spec', specPath], { |
There was a problem hiding this comment.
When the documented manual flow passes --spec-commit --sdk-repo without --compat-report, prepareSpecInputs() creates temporary old/new spec and IR files for that commit, but this compat path recomputes specPath from the current checkout. A historical changelog repair can therefore run compat-extract against today’s spec/open-api-spec.yaml instead of the requested commit’s spec, causing false compat failures or missing SDK-surface changes after the fail-open warning path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/sdk-release-metadata.mjs
Line: 272-295
Comment:
**Historical compat uses current spec**
When the documented manual flow passes `--spec-commit --sdk-repo` without `--compat-report`, `prepareSpecInputs()` creates temporary old/new spec and IR files for that commit, but this compat path recomputes `specPath` from the current checkout. A historical changelog repair can therefore run `compat-extract` against today’s `spec/open-api-spec.yaml` instead of the requested commit’s spec, causing false compat failures or missing SDK-surface changes after the fail-open warning path.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
scripts/sdk-release-metadata.mjsover-classified changes as breaking, producingfeat!(major-bump) commits for what are really backend API changes. It inherited the spec differ'sclassificationfor field / enum-value / response / request changes, and trusted every breaking-severity compat symbol — including the model properties the compat snapshot tracks (~2000 of them).This tightens the rule so a change is breaking (
feat!) only when:parameter_removed,parameter_renamed,parameter_requiredness_increased,parameter_type_narrowed,signature, operation param changes), orEverything else becomes a non-breaking backend API change (
fix):How
capSeverityhelper downgrades would-be-breakingfield-*,value-*,response-changed, andrequest-body-changedfacts tofix. The made-required/optional changelog wording, which previously rode onseverity === 'breaking', now carries an explicitmadeRequiredflag on the fact.compatChangeIsBreakingskips backend-only categories (field_type_changed,enum_member_value_changed,return_type_changed,default_value_changed) and, forsymbol_removed/symbol_renamed, distinguishes a model property (field → not breaking) from a service/client method or whole type (→ breaking) by checking the symbol's owner against the IR's service/type names (now exposed frombuildIndexes).By this same rule the tooling change itself is not breaking →
fix:, no!.Test plan
Verified with synthetic and real fixtures (no automated test exists for this script):
model-removed+operation-removedrender underfield-removed, made-required,value-removed,response-changedrender under Fixes.feat!; rollup isfix..oagen/ruby/compat-report.json→encryptorparameter removal andSSO/UserManagementmethod removals stay breaking.ActionAuthenticationDenied.context(property) and afield_type_changedare dropped;SSO.get_authorization_url(method) stays breaking.--format jsonoutput keeps correct prefixes;madeRequireddoes not leak into the emittedchanges[].🤖 Generated with Claude Code