diff --git a/package-lock.json b/package-lock.json index 7a71b31..ff0ec1b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13,11 +13,11 @@ }, "devDependencies": { "@types/js-yaml": "^4.0.9", - "@types/node": "^26.0.0", - "@workos/oagen-emitters": "^0.19.0", + "@types/node": "^26.0.1", + "@workos/oagen-emitters": "^0.19.1", "diff2html": "^3.4.56", "husky": "^9.1.7", - "js-yaml": "^4.1.1", + "js-yaml": "^5.1.0", "openapi-to-postmanv2": "^6.1.0", "tsdown": "^0.22.3", "tsx": "^4.22.4", @@ -1024,9 +1024,9 @@ "license": "MIT" }, "node_modules/@types/node": { - "version": "26.0.0", - "resolved": "https://registry.npmjs.org/@types/node/-/node-26.0.0.tgz", - "integrity": "sha512-vf2YFi1iY9lHGwNJMs01biZFbKJkrZR1T6/MlzjhJLPdntOHLhTrDSnSVcdtvjihi4VQNlrFRIxLsDBlQpAipA==", + "version": "26.0.1", + "resolved": "https://registry.npmjs.org/@types/node/-/node-26.0.1.tgz", + "integrity": "sha512-fc3KiUoBt6kie0N9bIW3E47vZsuaMf0PM2AaUpLCLT0s/LvX1nxAim6Fc049cNxODPpGm6qRAuUOB86SkRuPQw==", "dev": true, "license": "MIT", "dependencies": { @@ -1063,9 +1063,9 @@ } }, "node_modules/@workos/oagen-emitters": { - "version": "0.19.0", - "resolved": "https://registry.npmjs.org/@workos/oagen-emitters/-/oagen-emitters-0.19.0.tgz", - "integrity": "sha512-8AryTC4t8w4IbPSI8vk/unrwkuFVvcnwFkQjVvftEVnELYwaRgn/p0tq/p85ecKCxzNJWPembOyGlX0Qwn1alw==", + "version": "0.19.1", + "resolved": "https://registry.npmjs.org/@workos/oagen-emitters/-/oagen-emitters-0.19.1.tgz", + "integrity": "sha512-Z5tGIGmOPtXkGflfEsXMytGq9H+PB+iH78UzQ8AYy+Wmjqlv9RV2BKwYVRqZ6H0Bje5rvPBxrut1KhB42ns6YA==", "dev": true, "license": "MIT", "dependencies": { @@ -1844,9 +1844,9 @@ } }, "node_modules/js-yaml": { - "version": "4.2.0", - "resolved": "https://registry.npmjs.org/js-yaml/-/js-yaml-4.2.0.tgz", - "integrity": "sha512-ePWsvanv0DWuDRsW8dnt+R4jQ31SCRCQ7hhNcPXZPsoBZiemuZNYGf7adZdqX2D86j6rvKp3RpCxVTSb8WQlOw==", + "version": "5.1.0", + "resolved": "https://registry.npmjs.org/js-yaml/-/js-yaml-5.1.0.tgz", + "integrity": "sha512-s8VA5jkR8f22S3NAXmhKPFqGUduqZGlsufabVOgN14iTdw/RXcym7bKkbwjxLK9Yw2lEvvmJjFp119+KPeo8Kg==", "funding": [ { "type": "github", @@ -1862,7 +1862,7 @@ "argparse": "^2.0.1" }, "bin": { - "js-yaml": "bin/js-yaml.js" + "js-yaml": "bin/js-yaml.mjs" } }, "node_modules/jsesc": { diff --git a/package.json b/package.json index 49e2069..351ce2d 100644 --- a/package.json +++ b/package.json @@ -55,16 +55,16 @@ "prepare": "husky" }, "overrides": { - "js-yaml": "^4.1.1", + "js-yaml": "^5.1.0", "lodash": "^4.17.23" }, "devDependencies": { "@types/js-yaml": "^4.0.9", - "@types/node": "^26.0.0", - "@workos/oagen-emitters": "^0.19.0", + "@types/node": "^26.0.1", + "@workos/oagen-emitters": "^0.19.1", "diff2html": "^3.4.56", "husky": "^9.1.7", - "js-yaml": "^4.1.1", + "js-yaml": "^5.1.0", "openapi-to-postmanv2": "^6.1.0", "tsdown": "^0.22.3", "tsx": "^4.22.4", diff --git a/scripts/__tests__/open-batch-pr.spec.mjs b/scripts/__tests__/open-batch-pr.spec.mjs index 52abe66..73d379e 100644 --- a/scripts/__tests__/open-batch-pr.spec.mjs +++ b/scripts/__tests__/open-batch-pr.spec.mjs @@ -8,6 +8,7 @@ import { fileURLToPath } from 'node:url'; import { batchBranchName, + catchAllMessage, entryHeadline, orderedEntries, parseServices, @@ -15,6 +16,11 @@ import { rewriteOverrideRefs, } from '../open-batch-pr.mjs'; +// A conventional-commit title must start with a valid type, an optional +// (scope), an optional ! and then ": " — this is what each SDK repo's +// lint_pr_title / "Validate PR title" check enforces. +const CONVENTIONAL_TITLE = /^(feat|fix|chore)(\([^)]+\))?!?: \S/; + const SCRIPT = fileURLToPath(new URL('../open-batch-pr.mjs', import.meta.url)); // ── pure helpers (no git) ──────────────────────────────────────────────────── @@ -22,9 +28,74 @@ test('batchBranchName is deterministic', () => { assert.equal(batchBranchName('abc'), 'oagen/batch-abc'); }); -test('prTitle lists services; empty → all services', () => { +test('prTitle with no entries falls back to services; empty → all services', () => { assert.equal(prTitle('Vault, SSO', 'b1'), 'feat(generated): Vault, SSO (batch b1)'); + assert.equal(prTitle('Vault, SSO', 'b1', []), 'feat(generated): Vault, SSO (batch b1)'); assert.equal(prTitle('', 'b1'), 'feat(generated): all services (batch b1)'); + // Even the fallback titles are valid conventional-commit titles. + assert.match(prTitle('Vault, SSO', 'b1'), CONVENTIONAL_TITLE); + assert.match(prTitle('', 'b1'), CONVENTIONAL_TITLE); +}); + +test('prTitle with a single entry uses that entry summary', () => { + const entries = [{ prefix: 'feat', scope: 'organization_membership', summary: 'Add `roles` to organization membership models' }]; + assert.equal( + prTitle('OrganizationMembership', 'e471ddef', entries), + 'feat(generated): Add `roles` to organization membership models', + ); + assert.match(prTitle('OrganizationMembership', 'e471ddef', entries), CONVENTIONAL_TITLE); +}); + +test('prTitle with multiple entries leads with the top entry and counts the rest', () => { + const entries = [ + { prefix: 'feat', scope: 'sso', summary: 'Add SSO API surface' }, + { prefix: 'fix', scope: 'vault', summary: 'Change vault response' }, + { prefix: 'chore', scope: 'events', summary: 'Update events' }, + ]; + assert.equal(prTitle('SSO,Vault,Events', 'b2', entries), 'feat(generated): Add SSO API surface (+2 more)'); + assert.match(prTitle('SSO,Vault,Events', 'b2', entries), CONVENTIONAL_TITLE); +}); + +test('prTitle rolls the type up feat! → feat → fix and orders by it', () => { + // A breaking entry promotes the whole title to feat(generated)! and leads it, + // matching how the changelog override block's rollup type is computed. + const breaking = [ + { prefix: 'fix', scope: 'vault', summary: 'Change vault response' }, + { prefix: 'feat!', scope: 'sso', summary: 'Remove SSO API surface' }, + ]; + assert.equal(prTitle('Vault,SSO', 'b3', breaking), 'feat(generated)!: Remove SSO API surface (+1 more)'); + assert.match(prTitle('Vault,SSO', 'b3', breaking), CONVENTIONAL_TITLE); + + // No feat at all → fix(generated). + const fixesOnly = [{ prefix: 'fix', scope: 'vault', summary: 'Change vault response' }]; + assert.equal(prTitle('Vault', 'b4', fixesOnly), 'fix(generated): Change vault response'); + assert.match(prTitle('Vault', 'b4', fixesOnly), CONVENTIONAL_TITLE); + + // chore-only rolls up to fix(generated), mirroring rollupForEntries exactly + // (which never returns chore) so the title type matches the changelog override. + const choreOnly = [{ prefix: 'chore', scope: 'events', summary: 'Regenerate boilerplate' }]; + assert.equal(prTitle('Events', 'b5', choreOnly), 'fix(generated): Regenerate boilerplate'); + assert.match(prTitle('Events', 'b5', choreOnly), CONVENTIONAL_TITLE); +}); + +test('prTitle falls back gracefully on malformed entries (never throws or emits undefined)', () => { + // Entries present but no recognized prefix → fall back to the batch title. + const unrecognized = [{ prefix: 'docs', scope: 'readme', summary: 'Tweak docs' }]; + assert.equal(prTitle('Vault', 'b6', unrecognized), 'feat(generated): Vault (batch b6)'); + assert.match(prTitle('Vault', 'b6', unrecognized), CONVENTIONAL_TITLE); + + // Recognized prefix but missing summary → fall back rather than emit `undefined`. + const noSummary = [{ prefix: 'feat', scope: 'sso' }]; + const title = prTitle('SSO', 'b7', noSummary); + assert.equal(title, 'feat(generated): SSO (batch b7)'); + assert.doesNotMatch(title, /undefined/); + assert.match(title, CONVENTIONAL_TITLE); +}); + +test('catchAllMessage names the services and stays a chore(...) prefix', () => { + assert.equal(catchAllMessage('Vault, SSO'), 'chore(generated): regenerate shared files for Vault, SSO'); + assert.equal(catchAllMessage(''), 'chore(generated): regenerate shared files for all services'); + assert.match(catchAllMessage('Vault'), /^chore(\([^)]+\))?: \S/); }); test('parseServices trims and drops empties', () => { @@ -192,7 +263,7 @@ test('confinement: only the (already scoped) changed paths land on the branch', } }); -test('--dry-run prints the gh pr create command with the batch title', () => { +test('--dry-run prints the gh pr create command with the fallback batch title (no entries)', () => { const { root, origin } = setupOrigin(); try { const work = freshClone(root, origin, 'work'); @@ -206,6 +277,32 @@ test('--dry-run prints the gh pr create command with the batch title', () => { } }); +test('--dry-run derives a descriptive PR title from the classify entries', () => { + const { root, origin } = setupOrigin(); + try { + const work = freshClone(root, origin, 'work'); + writeFileSync(join(work, 'src/workos/vault/client.py'), 'x\n'); + const entries = [ + { prefix: 'feat', scope: 'vault', summary: 'Add `roles` to vault models', file_paths: ['src/workos/vault/client.py'] }, + { prefix: 'fix', scope: 'sso', summary: 'Change SSO response' }, + ]; + const entriesFile = join(root, 'title-entries.json'); // OUTSIDE the work tree + writeFileSync(entriesFile, JSON.stringify(entries)); + + const r = runScript([ + '--batch-id', 't10', '--lang', 'python', '--services', 'Vault,SSO', + '--sdk-dir', work, '--entries-file', entriesFile, '--dry-run', + ]); + assert.equal(r.code, 0, r.stderr); + assert.match(r.stdout, /DRY-RUN gh pr create/); + // Title is entry-derived, not the generic services/batch fallback. + assert.match(r.stdout, /feat\(generated\): Add `roles` to vault models \(\+1 more\)/); + assert.doesNotMatch(r.stdout, /Vault, SSO \(batch t10\)/); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + test('per-entry ordered commits + shared catch-all over the scoped diff', () => { const { root, origin } = setupOrigin(); try { @@ -229,7 +326,7 @@ test('per-entry ordered commits + shared catch-all over the scoped diff', () => const log = g(origin, 'log', '--format=%s', 'main..oagen/batch-pe1').split('\n').filter(Boolean); assert.deepEqual(log, [ - 'chore(generated): shared regenerated files', + 'chore(generated): regenerate shared files for Vault, SSO', 'fix(sso): fix conn', 'feat(vault): add object', ]); diff --git a/scripts/open-batch-pr.mjs b/scripts/open-batch-pr.mjs index 18c0853..05f2473 100644 --- a/scripts/open-batch-pr.mjs +++ b/scripts/open-batch-pr.mjs @@ -29,7 +29,15 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { pathToFileURL } from 'node:url'; -const CATCH_ALL_MESSAGE = 'chore(generated): shared regenerated files'; +// Catch-all commit for the behavior-change ripple + any unattributed files left +// after the per-entry commits. Names the services (when known) so reviewers can +// see which regeneration the shared churn belongs to; still a valid `chore(...)` +// conventional prefix. +export function catchAllMessage(services) { + const list = parseServices(services); + const scope = list.length > 0 ? list.join(', ') : 'all services'; + return `chore(generated): regenerate shared files for ${scope}`; +} // Files that, on their own, do not warrant a PR: the generator manifest and the // spec-sync marker the workflow copies in before every run (so it is always // staged). A diff containing only these is a no-op batch (FR-2.7). @@ -87,7 +95,32 @@ export function parseServices(csv) { .filter(Boolean); } -export function prTitle(services, batchId) { +// Roll the entries' prefixes up into a single conventional type+bang for the PR +// title, mirroring rollupForEntries in sdk-release-metadata.mjs EXACTLY (any +// feat! → feat!, else any feat → feat, else fix) so the title type always +// matches the changelog override block — including fix-only and chore-only +// batches, which both roll up to fix. Each SDK repo lints the PR title, so the +// result keeps a valid `(generated): ` prefix. +function rollupPrefix(entries) { + if ((entries ?? []).some((e) => e.prefix === 'feat!')) return 'feat(generated)!'; + if ((entries ?? []).some((e) => e.prefix === 'feat')) return 'feat(generated)'; + return 'fix(generated)'; +} + +// PR title. When classify entries with a usable summary are present, derive a +// descriptive, conventional-commit title from them — the lead entry's summary +// (entries ordered feat! → feat → fix → chore) plus a `(+N more)` suffix — so +// every language's PR says what changed and the title-lint check still passes. +// Otherwise (no entries, or malformed input with no recognized prefix or no +// summary) fall back to the services/batch title rather than throwing or +// emitting a literal `undefined`. +export function prTitle(services, batchId, entries = []) { + const ordered = orderedEntries(entries ?? []); + const lead = ordered[0]?.summary; + if (lead) { + const more = ordered.length - 1; + return `${rollupPrefix(entries)}: ${lead}${more > 0 ? ` (+${more} more)` : ''}`; + } const list = parseServices(services); const label = list.length > 0 ? list.join(', ') : 'all services'; return `feat(generated): ${label} (batch ${batchId})`; @@ -162,7 +195,7 @@ function gh(ghArgs, { dryRun, stub = '' }) { } // ── commit assembly ──────────────────────────────────────────────────────────── -function buildCommits(cwd, entries) { +function buildCommits(cwd, entries, services) { const initialStaged = git(cwd, ['diff', '--cached', '--name-only']) .split('\n') .map((l) => l.trim()) @@ -186,7 +219,7 @@ function buildCommits(cwd, entries) { // Catch-all: behavior-change ripple + any unattributed files. if (hasStagedChanges(cwd)) { - git(cwd, ['commit', '-m', CATCH_ALL_MESSAGE]); + git(cwd, ['commit', '-m', catchAllMessage(services)]); } } @@ -230,13 +263,14 @@ export function run(args) { const entries = args.entriesFile && existsSync(args.entriesFile) ? JSON.parse(readFileSync(args.entriesFile, 'utf8')) : []; - buildCommits(sdkDir, entries); + buildCommits(sdkDir, entries, services); // Force-push: same batch_id → same branch, overwritten (idempotent, NFR-2.2). git(sdkDir, ['push', '--force', remote, branch]); - // Create-or-reuse the PR for this branch. - const title = prTitle(services, batchId); + // Create-or-reuse the PR for this branch. Derive the title from the classify + // entries so it describes what changed; fall back to services/batch otherwise. + const title = prTitle(services, batchId, entries); const bodyText = args.bodyFile && existsSync(args.bodyFile) ? readFileSync(args.bodyFile, 'utf8') : defaultBody(services, batchId, lang);