From dc1292a4514c4c8a51630404cfb9cbfe1889b486 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Sun, 2 Aug 2026 14:23:46 -0500 Subject: [PATCH 1/4] fix(release): address Copilot review feedback --- docs/rxjs-next/PROJECT_PLAN.md | 16 +++++++ package.json | 2 +- scripts/release/release-doctor-policy.mjs | 29 ++++++++++++ .../release/release-doctor-policy.test.mjs | 45 +++++++++++++++++++ scripts/release/release-doctor.mjs | 26 ++++++++--- scripts/release/release-policy.mjs | 2 +- scripts/release/release-policy.test.mjs | 11 +++++ 7 files changed, 124 insertions(+), 7 deletions(-) create mode 100644 scripts/release/release-doctor-policy.mjs create mode 100644 scripts/release/release-doctor-policy.test.mjs diff --git a/docs/rxjs-next/PROJECT_PLAN.md b/docs/rxjs-next/PROJECT_PLAN.md index ae1e29c135..51b8654bed 100644 --- a/docs/rxjs-next/PROJECT_PLAN.md +++ b/docs/rxjs-next/PROJECT_PLAN.md @@ -1645,6 +1645,10 @@ names. - Added the sole-maintainer public runbook and security-assurance document. npm routes are never guessed: setup records a manually verified URL, rendering validates its origin, and comments retain stage-ID CLI fallbacks. +- Copilot review follow-up recognizes indented multi-line `BREAKING CHANGE` + footers and makes the release doctor verify the exact GitHub-hosted runner on + every release-PR, qualification, authorization, and staging job individually, + with regression tests for both security-sensitive cases. - Local verification is recorded in the P6.10 session entry. Live App, ruleset, trusted-publisher, disposable-package, TFA, rejection, staged digest, tag, and immutable-release evidence remain required before `DONE`. @@ -3603,3 +3607,15 @@ conformance implementation depends on a runnable harness. so regeneration cannot restore duplicate, unused exceptions. The configured root scan has no unreviewed findings or unused exceptions, while P6.10 remains the sole `NEXT` item. + +### 2026-08-02 — P6.10 Copilot review follow-up + +- Classified indented multi-line `BREAKING CHANGE` and `BREAKING CHANGES` + footers as breaking while retaining the non-rendering pull-request-template + placeholder behavior. +- Replaced the release doctor's workflow-wide runner substrings with exact + release-PR, qualification, authorization, and staging job runner validation, + including a false-positive test where an unrelated job still uses the + expected runner. +- Kept P6.10 as the sole `NEXT` item; these local review fixes do not satisfy + the remaining administrator setup or disposable-package rehearsal gates. diff --git a/package.json b/package.json index d2025bfb83..026077d2fc 100644 --- a/package.json +++ b/package.json @@ -9,7 +9,7 @@ "analyze:bundles": "node scripts/analyze-bundles.mjs", "prepare-packages": "pnpm nx run-many -t build,lint,test:circular,dtslint,copy_common_package_files --exclude rxjs.dev", "release": "node scripts/release/release-doctor.mjs", - "release:check": "node --test scripts/analyze-bundles.test.mjs scripts/check-package-docs.test.mjs scripts/check-release-coherence.test.mjs scripts/finalize-esm-package.test.mjs scripts/prerelease-adoption-lib.test.mjs scripts/release/authorize-release-commit.test.mjs scripts/release/authorize-stage.test.mjs scripts/release/install-pinned-npm.test.mjs scripts/release/release-config.test.mjs scripts/release/release-policy.test.mjs scripts/release/release-candidate.test.mjs scripts/release/stage-release.test.mjs scripts/security/check-osv-exceptions.test.mjs && node scripts/security/check-osv-exceptions.mjs && node scripts/check-package-docs.mjs && node scripts/check-release-coherence.mjs && node scripts/release/release-doctor.mjs", + "release:check": "node --test scripts/analyze-bundles.test.mjs scripts/check-package-docs.test.mjs scripts/check-release-coherence.test.mjs scripts/finalize-esm-package.test.mjs scripts/prerelease-adoption-lib.test.mjs scripts/release/authorize-release-commit.test.mjs scripts/release/authorize-stage.test.mjs scripts/release/install-pinned-npm.test.mjs scripts/release/release-config.test.mjs scripts/release/release-policy.test.mjs scripts/release/release-doctor-policy.test.mjs scripts/release/release-candidate.test.mjs scripts/release/stage-release.test.mjs scripts/security/check-osv-exceptions.test.mjs && node scripts/security/check-osv-exceptions.mjs && node scripts/check-package-docs.mjs && node scripts/check-release-coherence.mjs && node scripts/release/release-doctor.mjs", "test:bundle-analysis": "node --test scripts/analyze-bundles.test.mjs", "test:workflows": "prettier --check .github/workflows/*.yml .github/actions/install-dependencies/action.yml .github/dependabot.yml", "test:kernel": "pnpm --filter rxjs run test:kernel", diff --git a/scripts/release/release-doctor-policy.mjs b/scripts/release/release-doctor-policy.mjs new file mode 100644 index 0000000000..3f03f86994 --- /dev/null +++ b/scripts/release/release-doctor-policy.mjs @@ -0,0 +1,29 @@ +export function requireWorkflowJobRunners(source, workflowName, expectedRunners) { + const lines = source.split(/\r?\n/); + const errors = []; + + for (const [jobName, expectedRunner] of Object.entries(expectedRunners)) { + const jobStart = lines.findIndex((line) => line === ` ${jobName}:`); + if (jobStart === -1) { + errors.push(`${workflowName} is missing the ${jobName} job.`); + continue; + } + + let actualRunner; + for (let index = jobStart + 1; index < lines.length; index += 1) { + const line = lines[index]; + if (/^(?:\S| \S)/.test(line)) break; + const runner = /^ {4}runs-on:\s*([^#]+?)(?:\s+#.*)?$/.exec(line)?.[1]?.trim(); + if (runner) { + actualRunner = runner; + break; + } + } + + if (actualRunner !== expectedRunner) { + errors.push(`${workflowName} ${jobName} job must run on ${expectedRunner}; found ${actualRunner ?? 'no runner'}.`); + } + } + + return errors; +} diff --git a/scripts/release/release-doctor-policy.test.mjs b/scripts/release/release-doctor-policy.test.mjs new file mode 100644 index 0000000000..de956be763 --- /dev/null +++ b/scripts/release/release-doctor-policy.test.mjs @@ -0,0 +1,45 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { requireWorkflowJobRunners } from './release-doctor-policy.mjs'; + +const privilegedRunners = { build: 'ubuntu-latest', stage: 'ubuntu-latest' }; + +test('accepts the required runner on each privileged release job', () => { + const workflow = `jobs: + build: + runs-on: ubuntu-latest + browser: + runs-on: self-hosted + stage: + runs-on: ubuntu-latest +`; + + assert.deepEqual(requireWorkflowJobRunners(workflow, 'release-stage.yml', privilegedRunners), []); +}); + +test('rejects a privileged job on another runner even when a different job uses ubuntu-latest', () => { + const workflow = `jobs: + build: + runs-on: self-hosted + browser: + runs-on: ubuntu-latest + stage: + runs-on: ubuntu-latest +`; + + assert.deepEqual(requireWorkflowJobRunners(workflow, 'release-stage.yml', privilegedRunners), [ + 'release-stage.yml build job must run on ubuntu-latest; found self-hosted.', + ]); +}); + +test('rejects a missing privileged job or runner', () => { + const workflow = `jobs: + build: + name: Build +`; + + assert.deepEqual(requireWorkflowJobRunners(workflow, 'release-stage.yml', privilegedRunners), [ + 'release-stage.yml build job must run on ubuntu-latest; found no runner.', + 'release-stage.yml is missing the stage job.', + ]); +}); diff --git a/scripts/release/release-doctor.mjs b/scripts/release/release-doctor.mjs index 2d604df2d4..d0cc62fa13 100644 --- a/scripts/release/release-doctor.mjs +++ b/scripts/release/release-doctor.mjs @@ -4,6 +4,7 @@ import { readFile, readdir } from 'node:fs/promises'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { assertNpmWebUrl, releaseOperatorLogin, releasePackages, releaseToolchain, stagedPackagesVariable } from './release-config.mjs'; +import { requireWorkflowJobRunners } from './release-doctor-policy.mjs'; const root = fileURLToPath(new URL('../..', import.meta.url)); const strict = process.argv.includes('--strict'); @@ -52,15 +53,19 @@ for (const requirement of [ 'authorize-release-commit.mjs', 'stage-release.mjs publish', 'release-candidate.mjs verify', - 'runs-on: ubuntu-24.04', ]) { if (!stageWorkflow.includes(requirement)) errors.push(`release-stage.yml is missing ${requirement}.`); } +errors.push( + ...requireWorkflowJobRunners(stageWorkflow, 'release-stage.yml', { + authorize: 'ubuntu-24.04', + stage: 'ubuntu-24.04', + }) +); for (const requirement of [ 'matrix: { build: [a, b] }', 'compare-release-candidates.mjs', 'Exact tarballs / package, type, import, and migration gates', - 'ubuntu-24.04', "node-version: '24.12.0'", 'generate-release-evidence.mjs', 'osv-scanner-action@', @@ -68,6 +73,19 @@ for (const requirement of [ ]) { if (!qualificationWorkflow.includes(requirement)) errors.push(`release-qualify.yml is missing ${requirement}.`); } +errors.push( + ...requireWorkflowJobRunners(qualificationWorkflow, 'release-qualify.yml', { + build: 'ubuntu-24.04', + compare: 'ubuntu-24.04', + package: 'ubuntu-24.04', + node: 'ubuntu-24.04', + browser: 'ubuntu-24.04', + 'alternate-runtime': 'ubuntu-24.04', + safari: 'macos-15', + wpt: 'ubuntu-24.04', + evidence: 'ubuntu-24.04', + }) +); if (!stageWorkflow.includes('install-pinned-npm.mjs')) { errors.push('release-stage.yml must install the checked npm CLI through install-pinned-npm.mjs.'); } @@ -89,9 +107,7 @@ if (!stageScript.includes("['stage', 'download', stageId]")) { } const releasePullRequestWorkflow = await readFile(path.join(root, '.github/workflows/release-pr.yml'), 'utf8').catch(() => ''); -if (!releasePullRequestWorkflow.includes('runs-on: ubuntu-24.04')) { - errors.push('release-pr.yml must use the checked Ubuntu 24.04 runner.'); -} +errors.push(...requireWorkflowJobRunners(releasePullRequestWorkflow, 'release-pr.yml', { 'release-pr': 'ubuntu-24.04' })); if (!releasePullRequestWorkflow.includes("node-version: '24.12.0'")) { errors.push('release-pr.yml must use exact Node 24.12.0.'); } diff --git a/scripts/release/release-policy.mjs b/scripts/release/release-policy.mjs index 69a0d90a4e..c32c653dc6 100644 --- a/scripts/release/release-policy.mjs +++ b/scripts/release/release-policy.mjs @@ -17,7 +17,7 @@ export function parseVersion(version) { export function classifyConventionalCommit(subject, body = '') { const match = conventionalTitle.exec(subject); if (!match?.groups) return { level: 'invalid', subject, reason: 'title is not a supported Conventional Commit' }; - const breakingDescription = body.match(/(?:^|\n)(?:\*\*)?BREAKING CHANGES?:(?:\*\*)?[^\S\r\n]*([^\r\n]*)/i)?.[1]; + const breakingDescription = body.match(/(?:^|\n)(?:\*\*)?BREAKING CHANGES?:(?:\*\*)?[^\S\r\n]*([^\r\n]*(?:\r?\n[ \t]+[^\r\n]*)*)/i)?.[1]; const breakingFooter = breakingDescription !== undefined && hasTextOutsideHtmlComments(breakingDescription); const breaking = match.groups.breaking === '!' || Boolean(breakingFooter); const level = breaking diff --git a/scripts/release/release-policy.test.mjs b/scripts/release/release-policy.test.mjs index 6f648c5a2b..2bc1aa3adc 100644 --- a/scripts/release/release-policy.test.mjs +++ b/scripts/release/release-policy.test.mjs @@ -86,3 +86,14 @@ test('selects beta versions monotonically for arbitrary counters and releasable { numRuns: 200 } ); }); + +test('classifies an indented multi-line breaking-change footer as breaking', () => { + assert.equal( + classifyConventionalCommit('fix(core): correct teardown', 'BREAKING CHANGE:\n changes cancellation ownership').level, + 'breaking' + ); + assert.equal( + classifyConventionalCommit('fix(core): correct teardown', '**BREAKING CHANGES:**\n\tchanges cancellation ownership').level, + 'breaking' + ); +}); From 991c94d934b517af4f1ccf5bf54c5fcb745ef3f0 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Mon, 3 Aug 2026 18:12:36 -0500 Subject: [PATCH 2/4] fix(release): inspect every breaking footer --- docs/rxjs-next/PROJECT_PLAN.md | 2 ++ scripts/release/release-policy.mjs | 12 +++++++++--- scripts/release/release-policy.test.mjs | 7 +++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/docs/rxjs-next/PROJECT_PLAN.md b/docs/rxjs-next/PROJECT_PLAN.md index 51b8654bed..7bae4aab3d 100644 --- a/docs/rxjs-next/PROJECT_PLAN.md +++ b/docs/rxjs-next/PROJECT_PLAN.md @@ -3613,6 +3613,8 @@ conformance implementation depends on a runnable harness. - Classified indented multi-line `BREAKING CHANGE` and `BREAKING CHANGES` footers as breaking while retaining the non-rendering pull-request-template placeholder behavior. +- Ensured that an unchanged template placeholder cannot mask a later populated + breaking footer in the same squash-commit body. - Replaced the release doctor's workflow-wide runner substrings with exact release-PR, qualification, authorization, and staging job runner validation, including a false-positive test where an unrelated job still uses the diff --git a/scripts/release/release-policy.mjs b/scripts/release/release-policy.mjs index c32c653dc6..1e66bd5beb 100644 --- a/scripts/release/release-policy.mjs +++ b/scripts/release/release-policy.mjs @@ -17,9 +17,7 @@ export function parseVersion(version) { export function classifyConventionalCommit(subject, body = '') { const match = conventionalTitle.exec(subject); if (!match?.groups) return { level: 'invalid', subject, reason: 'title is not a supported Conventional Commit' }; - const breakingDescription = body.match(/(?:^|\n)(?:\*\*)?BREAKING CHANGES?:(?:\*\*)?[^\S\r\n]*([^\r\n]*(?:\r?\n[ \t]+[^\r\n]*)*)/i)?.[1]; - const breakingFooter = breakingDescription !== undefined && hasTextOutsideHtmlComments(breakingDescription); - const breaking = match.groups.breaking === '!' || Boolean(breakingFooter); + const breaking = match.groups.breaking === '!' || hasPopulatedBreakingFooter(body); const level = breaking ? 'breaking' : match.groups.type === 'feat' @@ -30,6 +28,14 @@ export function classifyConventionalCommit(subject, body = '') { return { description: match.groups.description, level, subject, type: match.groups.type }; } +function hasPopulatedBreakingFooter(body) { + const footerPattern = /(?:^|\n)(?:\*\*)?BREAKING CHANGES?:(?:\*\*)?[^\S\r\n]*([^\r\n]*(?:\r?\n[ \t]+[^\r\n]*)*)/gi; + for (const match of body.matchAll(footerPattern)) { + if (hasTextOutsideHtmlComments(match[1])) return true; + } + return false; +} + function hasTextOutsideHtmlComments(value) { let index = 0; while (index < value.length) { diff --git a/scripts/release/release-policy.test.mjs b/scripts/release/release-policy.test.mjs index 2bc1aa3adc..068b5d7e47 100644 --- a/scripts/release/release-policy.test.mjs +++ b/scripts/release/release-policy.test.mjs @@ -96,4 +96,11 @@ test('classifies an indented multi-line breaking-change footer as breaking', () classifyConventionalCommit('fix(core): correct teardown', '**BREAKING CHANGES:**\n\tchanges cancellation ownership').level, 'breaking' ); + assert.equal( + classifyConventionalCommit( + 'fix(core): correct teardown', + '**BREAKING CHANGE:** \n\nBREAKING CHANGE:\n changes cancellation ownership' + ).level, + 'breaking' + ); }); From b3ae71329ca6bde21f2a2c1dc547b905311f97af Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Mon, 3 Aug 2026 18:14:02 -0500 Subject: [PATCH 3/4] fix(release): scope privileged runner checks --- docs/rxjs-next/PROJECT_PLAN.md | 3 ++ scripts/release/release-doctor-policy.mjs | 25 +++++++-- .../release/release-doctor-policy.test.mjs | 53 +++++++++++++++---- 3 files changed, 66 insertions(+), 15 deletions(-) diff --git a/docs/rxjs-next/PROJECT_PLAN.md b/docs/rxjs-next/PROJECT_PLAN.md index 7bae4aab3d..66a0add8cb 100644 --- a/docs/rxjs-next/PROJECT_PLAN.md +++ b/docs/rxjs-next/PROJECT_PLAN.md @@ -3619,5 +3619,8 @@ conformance implementation depends on a runnable harness. release-PR, qualification, authorization, and staging job runner validation, including a false-positive test where an unrelated job still uses the expected runner. +- Scoped privileged runner discovery to the workflow's `jobs` mapping and + rejected duplicate privileged job definitions so top-level lookalikes cannot + satisfy the audit. - Kept P6.10 as the sole `NEXT` item; these local review fixes do not satisfy the remaining administrator setup or disposable-package rehearsal gates. diff --git a/scripts/release/release-doctor-policy.mjs b/scripts/release/release-doctor-policy.mjs index 3f03f86994..0965aaade3 100644 --- a/scripts/release/release-doctor-policy.mjs +++ b/scripts/release/release-doctor-policy.mjs @@ -1,18 +1,31 @@ export function requireWorkflowJobRunners(source, workflowName, expectedRunners) { const lines = source.split(/\r?\n/); const errors = []; + const jobsStart = lines.findIndex((line) => /^jobs:\s*(?:#.*)?$/.test(line)); + const jobsEnd = + jobsStart === -1 ? -1 : lines.findIndex((line, index) => index > jobsStart && /^(?!#)[A-Za-z_][A-Za-z0-9_-]*:\s*/.test(line)); + const scopedEnd = jobsEnd === -1 ? lines.length : jobsEnd; for (const [jobName, expectedRunner] of Object.entries(expectedRunners)) { - const jobStart = lines.findIndex((line) => line === ` ${jobName}:`); - if (jobStart === -1) { + const jobPattern = new RegExp(`^ {2}${escapeRegExp(jobName)}:\\s*(?:#.*)?$`); + const jobStarts = []; + for (let index = jobsStart + 1; jobsStart !== -1 && index < scopedEnd; index += 1) { + if (jobPattern.test(lines[index])) jobStarts.push(index); + } + if (jobStarts.length === 0) { errors.push(`${workflowName} is missing the ${jobName} job.`); continue; } + if (jobStarts.length > 1) { + errors.push(`${workflowName} defines the ${jobName} job more than once.`); + continue; + } + const [jobStart] = jobStarts; let actualRunner; - for (let index = jobStart + 1; index < lines.length; index += 1) { + for (let index = jobStart + 1; index < scopedEnd; index += 1) { const line = lines[index]; - if (/^(?:\S| \S)/.test(line)) break; + if (/^ {2}(?!#)\S/.test(line)) break; const runner = /^ {4}runs-on:\s*([^#]+?)(?:\s+#.*)?$/.exec(line)?.[1]?.trim(); if (runner) { actualRunner = runner; @@ -27,3 +40,7 @@ export function requireWorkflowJobRunners(source, workflowName, expectedRunners) return errors; } + +function escapeRegExp(value) { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} diff --git a/scripts/release/release-doctor-policy.test.mjs b/scripts/release/release-doctor-policy.test.mjs index de956be763..d7198a7ac3 100644 --- a/scripts/release/release-doctor-policy.test.mjs +++ b/scripts/release/release-doctor-policy.test.mjs @@ -2,44 +2,75 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { requireWorkflowJobRunners } from './release-doctor-policy.mjs'; -const privilegedRunners = { build: 'ubuntu-latest', stage: 'ubuntu-latest' }; +const privilegedRunners = { authorize: 'ubuntu-24.04', stage: 'ubuntu-24.04' }; test('accepts the required runner on each privileged release job', () => { const workflow = `jobs: - build: - runs-on: ubuntu-latest + authorize: + runs-on: ubuntu-24.04 browser: runs-on: self-hosted stage: - runs-on: ubuntu-latest + runs-on: ubuntu-24.04 `; assert.deepEqual(requireWorkflowJobRunners(workflow, 'release-stage.yml', privilegedRunners), []); }); -test('rejects a privileged job on another runner even when a different job uses ubuntu-latest', () => { +test('rejects a privileged job on another runner even when a different job uses the expected runner', () => { const workflow = `jobs: - build: + authorize: runs-on: self-hosted browser: - runs-on: ubuntu-latest + runs-on: ubuntu-24.04 stage: - runs-on: ubuntu-latest + runs-on: ubuntu-24.04 `; assert.deepEqual(requireWorkflowJobRunners(workflow, 'release-stage.yml', privilegedRunners), [ - 'release-stage.yml build job must run on ubuntu-latest; found self-hosted.', + 'release-stage.yml authorize job must run on ubuntu-24.04; found self-hosted.', + ]); +}); + +test('does not accept a runner from a similarly named value outside the jobs mapping', () => { + const workflow = `env: + authorize: + runs-on: ubuntu-24.04 +jobs: + authorize: + runs-on: self-hosted + stage: + runs-on: ubuntu-24.04 +`; + + assert.deepEqual(requireWorkflowJobRunners(workflow, 'release-stage.yml', privilegedRunners), [ + 'release-stage.yml authorize job must run on ubuntu-24.04; found self-hosted.', + ]); +}); + +test('rejects duplicate privileged job definitions', () => { + const workflow = `jobs: + authorize: + runs-on: ubuntu-24.04 + authorize: + runs-on: ubuntu-24.04 + stage: + runs-on: ubuntu-24.04 +`; + + assert.deepEqual(requireWorkflowJobRunners(workflow, 'release-stage.yml', privilegedRunners), [ + 'release-stage.yml defines the authorize job more than once.', ]); }); test('rejects a missing privileged job or runner', () => { const workflow = `jobs: - build: + authorize: name: Build `; assert.deepEqual(requireWorkflowJobRunners(workflow, 'release-stage.yml', privilegedRunners), [ - 'release-stage.yml build job must run on ubuntu-latest; found no runner.', + 'release-stage.yml authorize job must run on ubuntu-24.04; found no runner.', 'release-stage.yml is missing the stage job.', ]); }); From b4d133bd8fd2f2f7b19bc50e88696479b3aa3a65 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Tue, 4 Aug 2026 09:19:26 -0500 Subject: [PATCH 4/4] docs(release): record conflict resolution --- docs/rxjs-next/PROJECT_PLAN.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/rxjs-next/PROJECT_PLAN.md b/docs/rxjs-next/PROJECT_PLAN.md index 66a0add8cb..2929692b64 100644 --- a/docs/rxjs-next/PROJECT_PLAN.md +++ b/docs/rxjs-next/PROJECT_PLAN.md @@ -3624,3 +3624,15 @@ conformance implementation depends on a runnable harness. satisfy the audit. - Kept P6.10 as the sole `NEXT` item; these local review fixes do not satisfy the remaining administrator setup or disposable-package rehearsal gates. + +### 2026-08-04 — P6.10 review-branch conflict resolution + +- Rebased the Copilot follow-up onto the merged single-maintainer hardening and + dependency-advisory work, dropping the two release commits already present on + `master` instead of replaying or reverting them. +- Adapted runner validation to the split release-PR, qualification, + authorization, and staging workflows while preserving exact Ubuntu 24.04, + macOS 15, Node 24.12.0, OSV, property-test, and checked-toolchain controls. +- Passed all 53 release/security tests, 177 OSV exception validations, release + coherence and doctor checks, workflow formatting, and diff hygiene. P6.10 + remains the sole `NEXT` item pending its external setup and rehearsal gates.