fix: only treat call-signature and type changes as breaking in release metadata

scripts/sdk-release-metadata.mjs

@@ -1,7 +1,7 @@
 #!/usr/bin/env node
 
 import { execFileSync } from 'node:child_process';
-import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
+import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 
@@ -101,16 +101,21 @@ Inputs:
   --diff-report <file>   JSON output from "oagen diff".
   --old-ir <file>        JSON output from "oagen parse" for the previous spec.
   --new-ir <file>        JSON output from "oagen parse" for the current spec.
-  --compat-report <file> Optional SDK compat report JSON.
+  --compat-report <file> Optional SDK compat report JSON. When omitted and
+                         --sdk-repo is given, one is derived from the checkout
+                         (baseline ref vs current tree) without regenerating.
   --changed-files <file> Optional newline-delimited SDK file list for file attribution.
 
 Commit convenience:
   --spec-commit <sha>    Use the spec state at this openapi-spec commit, then diff
                          against the previous spec-changing commit.
   --openapi-repo <path>  openapi-spec checkout for --spec-commit. Defaults to cwd.
   --spec-path <path>     Spec path in the repo. Defaults to spec/open-api-spec.yaml.
-  --sdk-repo <path>      SDK checkout used only to derive changed files.
+  --sdk-repo <path>      SDK checkout used to derive changed files and, when
+                         --compat-report is omitted, an SDK compat report.
   --sdk-base <rev>       Diff base for --sdk-repo. Defaults to origin/main...HEAD.
+  --lang <language>      SDK language for compat extraction. Inferred from the
+                         --sdk-repo basename (e.g. workos-python → python).
 
 Output:
   --format json          Default. Emits entries consumed by generate-prs.yml.
@@ -242,6 +247,98 @@ function prepareSpecInputs(args) {
   }
 }
 
+function inferLanguage(sdkRepo) {
+  const base =
+    String(sdkRepo)
+      .replace(/[\\/]+$/, '')
+      .split(/[\\/]/)
+      .pop() ?? '';
+  const match = base.match(/^workos-(.+)$/);
+  return match ? match[1] : base;
+}
+
+// Derive an SDK compat report from an existing checkout WITHOUT regenerating
+// the SDK: snapshot the current tree (candidate) and the tree at the base ref
+// (baseline, via a throwaway git worktree), then diff. This lets the changelog
+// surface SDK-surface changes — e.g. a method rename — that the spec diff alone
+// cannot see. Fails open: any problem just means the changelog is built from
+// the spec diff only. Skipped when --compat-report is passed explicitly (so
+// CI, which supplies its own per-language reports, is unaffected).
+function prepareCompatInputs(args) {
+  if (!args['sdk-repo'] || args['compat-report']) return args;
+
+  const sdkRepo = args['sdk-repo'];
+  const lang = args.lang ?? inferLanguage(sdkRepo);
+  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], {
+        cwd: openapiRepo,
+      });
+    extract(baselineSrc, baselineOut);
+    extract(sdkRepo, candidateOut);
+
+    const reportPath = join(tmp, 'compat-report.json');
+    run(
+      'npx',
+      [
+        'oagen',
+        'compat-diff',
+        '--baseline',
+        join(baselineOut, '.oagen-compat-snapshot.json'),
+        '--candidate',
+        join(candidateOut, '.oagen-compat-snapshot.json'),
+        '--output',
+        reportPath,
+        '--fail-on',
+        'none',
+      ],
+      { cwd: openapiRepo, allowExitCodes: [1] },
+    );
+
+    succeeded = true;
+    return { ...args, 'compat-report': reportPath, _compatTmpdir: tmp };
+  } catch (err) {
+    process.stderr.write(
+      `warning: could not derive SDK compat report (${err.message}); changelog will use the spec diff only\n`,
+    );
+    return args;
+  } finally {
+    if (worktreeAdded) {
+      try {
+        git(sdkRepo, ['worktree', 'remove', '--force', baselineSrc]);
+      } catch {
+        try {
+          git(sdkRepo, ['worktree', 'prune']);
+        } catch {
+          // best-effort cleanup
+        }
+      }
+    }
+    if (!succeeded) rmSync(tmp, { recursive: true, force: true });
+  }
+}
+
 function toSnakeCase(value) {
   return String(value)
     .replace(/([a-z0-9])([A-Z])/g, '$1_$2')
@@ -341,12 +438,37 @@ function code(value) {
   return `\`${value}\``;
 }
 
+// Drill through wrappers (list/optional/etc.) to the underlying model or enum
+// name, so a response/request type can be named in the changelog.
+function primaryTypeName(type) {
+  if (!type || typeof type !== 'object') return null;
+  if ((type.kind === 'model' || type.kind === 'enum') && type.name) return type.name;
+  return primaryTypeName(type.inner) ?? primaryTypeName(type.items) ?? null;
+}
+
 function buildIndexes(specs) {
   const modelByName = new Map();
   const enumByName = new Map();
   const enumWireValues = new Map();
   const symbolScopes = new Map();
   const operationByKey = new Map();
+  const serviceNames = new Set();
+  // Per-side (old vs new) maps of operation → response/request type name, used
+  // to describe *what* changed in a modified operation. specs is [oldIr, newIr].
+  const responseTypeByKey = { old: new Map(), new: new Map() };
+  const requestTypeByKey = { old: new Map(), new: new Map() };
+  specs.forEach((spec, i) => {
+    const slot = i === 0 ? 'old' : 'new';
+    for (const service of spec?.services ?? []) {
+      for (const operation of service.operations ?? []) {
+        const key = `${service.name}.${operation.name}`;
+        const resp = primaryTypeName(operation.response);
+        if (resp) responseTypeByKey[slot].set(key, resp);
+        const req = primaryTypeName(operation.requestBody);
+        if (req) requestTypeByKey[slot].set(key, req);
+      }
+    }
+  });
 
   for (const spec of specs.filter(Boolean)) {
     for (const model of spec.models ?? []) modelByName.set(model.name, model);
@@ -393,6 +515,7 @@ function buildIndexes(specs) {
   for (const spec of specs.filter(Boolean)) {
     for (const service of spec.services ?? []) {
       const scope = publicScopeFromService(service.name);
+      serviceNames.add(service.name);
       for (const operation of service.operations ?? []) {
         operationByKey.set(`${service.name}.${operation.name}`, operation);
 
@@ -417,7 +540,8 @@ function buildIndexes(specs) {
     }
   }
 
-  return { enumWireValues, symbolScopes, operationByKey };
+  const typeNames = new Set([...modelByName.keys(), ...enumByName.keys()]);
+  return { enumWireValues, symbolScopes, operationByKey, responseTypeByKey, requestTypeByKey, serviceNames, typeNames };
 }
 
 function resolveServiceScope(serviceName) {
@@ -474,6 +598,26 @@ function severityToPrefix(severity) {
   return 'fix';
 }
 
+// Per policy, only a changed call signature or a removed/renamed *type* is
+// breaking. Field-, enum-value-, and response/request-shape changes are backend
+// API changes — never breaking, even when the spec differ classifies them so.
+// Cap those kinds: a would-be-breaking severity becomes `fix` (it is neither a
+// feature nor a major bump); additive severities pass through unchanged.
+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',
+]);
+function capSeverity(kind, severity) {
+  return BACKEND_ONLY_DIFF_KINDS.has(kind) && severity === 'breaking' ? 'fix' : severity;
+}
+
 function addFact(facts, fact) {
   facts.push({
     symbols: [],
@@ -516,17 +660,19 @@ function factsFromDiff(diffReport, indexes) {
     } else if (change.kind === 'model-modified') {
       const scope = resolveSymbolScope('model', change.name, indexes);
       for (const fieldChange of change.fieldChanges ?? []) {
-        const severity = fieldChange.classification;
+        // The spec differ encodes a field's required/optional *direction* in the
+        // classification (made-required reads as breaking). Capture it before the
+        // severity is capped, since the changelog wording depends on it.
+        const madeRequired = fieldChange.classification === 'breaking';
         let detail;
         if (fieldChange.kind === 'field-added') {
           detail = `Added \`${fieldChange.fieldName}\` to \`${change.name}\`.`;
         } else if (fieldChange.kind === 'field-removed') {
           detail = `Removed \`${fieldChange.fieldName}\` from \`${change.name}\`.`;
         } else if (fieldChange.kind === 'field-required-changed') {
-          detail =
-            severity === 'breaking'
-              ? `Made \`${change.name}.${fieldChange.fieldName}\` required.`
-              : `Made \`${change.name}.${fieldChange.fieldName}\` optional.`;
+          detail = madeRequired
+            ? `Made \`${change.name}.${fieldChange.fieldName}\` required.`
+            : `Made \`${change.name}.${fieldChange.fieldName}\` optional.`;
         } else if (fieldChange.kind === 'field-type-changed') {
           detail = `Changed the type of \`${change.name}.${fieldChange.fieldName}\`.`;
         } else if (fieldChange.kind === 'field-format-changed') {
@@ -535,12 +681,13 @@ function factsFromDiff(diffReport, indexes) {
           detail = `Changed access for \`${change.name}.${fieldChange.fieldName}\`.`;
         }
         addFact(facts, {
-          severity,
+          severity: capSeverity(fieldChange.kind, fieldChange.classification),
           ...scopeFields(scope),
           kind: fieldChange.kind,
           symbols: [change.name, fieldChange.fieldName],
           fieldName: fieldChange.fieldName,
           modelName: change.name,
+          ...(fieldChange.kind === 'field-required-changed' ? { madeRequired } : {}),
           detail,
         });
       }
@@ -565,7 +712,6 @@ function factsFromDiff(diffReport, indexes) {
     } else if (change.kind === 'enum-modified') {
       const scope = resolveSymbolScope('enum', change.name, indexes);
       for (const valueChange of change.valueChanges ?? []) {
-        const severity = valueChange.classification;
         let detail;
         if (valueChange.kind === 'value-added') {
           detail = `Added ${enumDisplay(change.name, valueChange.valueName, indexes)} to \`${change.name}\`.`;
@@ -575,7 +721,7 @@ function factsFromDiff(diffReport, indexes) {
           detail = `Changed ${enumDisplay(change.name, valueChange.valueName, indexes)} in \`${change.name}\`.`;
         }
         addFact(facts, {
-          severity,
+          severity: capSeverity(valueChange.kind, valueChange.classification),
           ...scopeFields(scope),
           kind: valueChange.kind,
           symbols: [change.name, valueChange.valueName],
@@ -639,22 +785,33 @@ function factsFromDiff(diffReport, indexes) {
           detail,
         });
       }
+      const opKey = `${change.serviceName}.${change.operationName}`;
       if (change.responseChanged) {
+        const oldType = indexes.responseTypeByKey?.old.get(opKey);
+        const newType = indexes.responseTypeByKey?.new.get(opKey);
         addFact(facts, {
-          severity: change.classification,
+          severity: capSeverity('response-changed', change.classification),
           ...scopeFields(scope),
           kind: 'response-changed',
           symbols: [change.serviceName, change.operationName],
-          detail: `Changed response for \`${change.serviceName}.${change.operationName}\`.`,
+          detail:
+            oldType && newType && oldType !== newType
+              ? `Changed response of \`${opKey}\` from \`${oldType}\` to \`${newType}\`.`
+              : `Changed response for \`${opKey}\`.`,
         });
       }
       if (change.requestBodyChanged) {
+        const oldType = indexes.requestTypeByKey?.old.get(opKey);
+        const newType = indexes.requestTypeByKey?.new.get(opKey);
         addFact(facts, {
-          severity: change.classification,
+          severity: capSeverity('request-body-changed', change.classification),
           ...scopeFields(scope),
           kind: 'request-body-changed',
           symbols: [change.serviceName, change.operationName],
-          detail: `Changed request body for \`${change.serviceName}.${change.operationName}\`.`,
+          detail:
+            oldType && newType && oldType !== newType
+              ? `Changed request body of \`${opKey}\` from \`${oldType}\` to \`${newType}\`.`
+              : `Changed request body for \`${opKey}\`.`,
         });
       }
     }
@@ -704,7 +861,37 @@ function renamesFromCompat(compatReport) {
   return renames;
 }
 
-function factsFromCompat(compatReport, existingFacts) {
+// Compat categories the tool may flag breaking that are still backend API
+// changes under our policy: a field's type, an enum member value, a method's
+// return type, or a default moving. Only call-signature and whole-type changes
+// stay breaking.
+const NON_BREAKING_COMPAT_CATEGORIES = new Set([
+  'field_type_changed',
+  'enum_member_value_changed',
+  'return_type_changed',
+  'default_value_changed',
+]);
+
+// Decide whether a breaking-severity compat change is breaking under our policy.
+// `symbol_removed`/`symbol_renamed` carry no kind, so split owner from member and
+// consult the IR: a member of a model/enum is a property (field rename/removal —
+// not breaking); a whole type/service or a service/client member is call surface.
+function compatChangeIsBreaking(change, indexes) {
+  const category = String(change.category ?? '');
+  if (NON_BREAKING_COMPAT_CATEGORIES.has(category)) return false;
+  if (category === 'symbol_removed' || category === 'symbol_renamed') {
+    const symbol = String(change.symbol ?? '');
+    const dot = symbol.lastIndexOf('.');
+    if (dot === -1) return true; // whole type/service removed or renamed
+    const owner = symbol.slice(0, dot).replace(/^Async(?=[A-Z])/, '');
+    if (indexes?.serviceNames?.has(owner) || owner === 'Client') return true;
+    if (indexes?.typeNames?.has(owner)) return false;
+    return true; // unknown owner — preserve breaking rather than drop a real removal
+  }
+  return true;
+}
+
+function factsFromCompat(compatReport, existingFacts, indexes) {
   const facts = [];
   const existingBreakingScopes = new Set(existingFacts.filter((fact) => fact.severity === 'breaking').map((fact) => fact.scope));
   const renames = renamesFromCompat(compatReport);
@@ -714,7 +901,7 @@ function factsFromCompat(compatReport, existingFacts) {
   // is the primary public API and reads better in the changelog.
   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))
     .sort((a, b) => isAsync(a) - isAsync(b));
 
   for (const change of breakingChanges) {
@@ -784,7 +971,7 @@ function summaryForGroup(group) {
   const commonField = facts.length > 1 && facts.every((fact) => fact.fieldName === facts[0].fieldName) ? facts[0].fieldName : null;
   if (commonField && kinds.size === 1 && kinds.has('field-added')) return `Add ${code(commonField)} to ${label} models`;
   if (commonField && kinds.size === 1 && kinds.has('field-required-changed')) {
-    return group.severity === 'breaking'
+    return facts.every((fact) => fact.madeRequired)
       ? `Make ${code(commonField)} required in ${label} models`
       : `Make ${code(commonField)} optional in ${label} models`;
   }
@@ -826,7 +1013,7 @@ function descriptionForGroup(group) {
     return `- Added ${code(commonField)} to ${label} models.`;
   }
   if (commonField && kinds.size === 1 && kinds.has('field-required-changed')) {
-    return group.severity === 'breaking'
+    return facts.every((fact) => fact.madeRequired)
       ? `- Made ${code(commonField)} required in ${label} models.`
       : `- Made ${code(commonField)} optional in ${label} models.`;
   }
@@ -983,6 +1170,7 @@ if (args.help) {
   process.exit(0);
 }
 args = prepareSpecInputs(args);
+args = prepareCompatInputs(args);
 
 try {
   const diffReport = readJson(args['diff-report'], { changes: [], behaviorChanges: [], summary: {} });
@@ -993,7 +1181,7 @@ try {
 
   const indexes = buildIndexes([oldIr, newIr]);
   const specFacts = factsFromDiff(diffReport, indexes);
-  const compatFacts = factsFromCompat(compatReport, specFacts);
+  const compatFacts = factsFromCompat(compatReport, specFacts, indexes);
   const entries = entriesFromGroups(groupFacts([...specFacts, ...compatFacts]), changedFiles);
   reportScopeValidation(entries, args);
 
@@ -1008,4 +1196,5 @@ try {
   }
 } finally {
   if (args._tmpdir) rmSync(args._tmpdir, { recursive: true, force: true });
+  if (args._compatTmpdir) rmSync(args._compatTmpdir, { recursive: true, force: true });
 }