From 865e83679c1d3c850ce702b6b5e78e03c3fd1cc6 Mon Sep 17 00:00:00 2001 From: ssvoss Date: Sun, 8 Nov 2026 11:40:07 -0500 Subject: [PATCH 1/3] CORE-6263: tighten version label validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validation let through labels that the merge workflow later tagged wrongly or failed on partway: - The changelog check used a substring match, so `## 1.0.10` or `### 1.0.0` satisfied a 1.0.1 or 1.0.0 label. It now needs a line that is exactly `## X.Y.Z`, optionally followed by ` - title` or ` β€” title`, which every existing changelog uses. - The workflow file check read `isFile` without calling it, so a directory named like a workflow passed. - Two version labels for one component collapsed to the last one here, while the merge workflow tagged both. VERSIONING.md already says duplicates fail; now they do. - A version that was already released passed, and `git tag` then failed mid-loop on merge. The workflow now lists the release tags with `git ls-remote` and validation fails on a label whose tag exists. The workflow also drops its `set -x` debug output, turns off setup-node's package manager cache as the other workflows do, and cancels superseded runs for the same PR, since only the latest label set matters. Co-Authored-By: Claude Opus 5.5 --- ...ernal_on_pr_validate_component_version.yml | 20 +++- VERSIONING.md | 9 +- .../index.integration.test.js | 43 ++++++++ .../validate-version-labels/index.js | 99 +++++++++++++---- .../index.unit.test.js | 104 +++++++++++++++++- 5 files changed, 246 insertions(+), 29 deletions(-) diff --git a/.github/workflows/internal_on_pr_validate_component_version.yml b/.github/workflows/internal_on_pr_validate_component_version.yml index 34cd5f7..a2529fb 100644 --- a/.github/workflows/internal_on_pr_validate_component_version.yml +++ b/.github/workflows/internal_on_pr_validate_component_version.yml @@ -11,6 +11,10 @@ on: - unlabeled - reopened +concurrency: + group: validate-version-labels-${{ github.event.pull_request.number }} + cancel-in-progress: true + permissions: {} jobs: @@ -83,13 +87,19 @@ jobs: echo "$all_labels" > all_labels.txt + - name: Get released version tags + run: git ls-remote --tags --refs origin | cut -f2 | sed 's#^refs/tags/##' > released_tags.txt + - name: Set up Node.js uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 with: node-version-file: .nvmrc + package-manager-cache: false - name: Label Validation id: validate + env: + RELEASED_TAGS_FILE: released_tags.txt run: node scripts/internal-ci/validate-version-labels/index.js all_labels.txt - name: Validation Job Summary @@ -102,8 +112,10 @@ jobs: INVALID_VERSION_LABELS: ${{ steps.validate.outputs.invalidVersionLabels }} HAS_UNTRACKED_VERSION: ${{ steps.validate.outputs.hasUntrackedVersion }} COMPONENT_VERSION_LABELS: ${{ steps.validate.outputs.componentVersionLabels }} + DUPLICATE_COMPONENTS: ${{ steps.validate.outputs.duplicateComponents }} INVALID_COMPONENTS: ${{ steps.validate.outputs.invalidComponents }} MISSING_CHANGELOGS: ${{ steps.validate.outputs.missingChangelogs }} + RELEASED_VERSIONS: ${{ steps.validate.outputs.releasedVersions }} run: | { echo "## Validation Outcome" @@ -125,12 +137,18 @@ jobs: if [ -n "$INVALID_VERSION_LABELS" ]; then echo "**Invalid version labels**: $INVALID_VERSION_LABELS" fi + if [ -n "$DUPLICATE_COMPONENTS" ]; then + echo "**Components with more than one version label**: $DUPLICATE_COMPONENTS" + fi if [ -n "$INVALID_COMPONENTS" ]; then echo "**Nonexistent/Invalid components**: $INVALID_COMPONENTS" fi if [ -n "$MISSING_CHANGELOGS" ]; then echo "**Components missing changelogs/entries**: $MISSING_CHANGELOGS" fi + if [ -n "$RELEASED_VERSIONS" ]; then + echo "**Versions already released**: $RELEASED_VERSIONS" + fi elif [ "$IS_VALID" == "true" ]; then echo "βœ… Version label validation passed" fi @@ -146,13 +164,11 @@ jobs: HAS_UNTRACKED_VERSION: ${{ steps.validate.outputs.hasUntrackedVersion }} VALID_COMPONENTS: ${{ steps.validate.outputs.validComponents }} run: | - set -x summary="## Tags\n" if [ "${HAS_UNTRACKED_VERSION}" = "true" ]; then summary+="- No Tags will be created on main" else summary+="The following tags will be created on main after merge\n\n" - echo "$VALID_COMPONENTS" IFS=',' read -ra components <<< "$VALID_COMPONENTS" for component_version in "${components[@]}"; do summary+="🏷️ \`$component_version\`\n" diff --git a/VERSIONING.md b/VERSIONING.md index 900bbc2..e6a3e9b 100644 --- a/VERSIONING.md +++ b/VERSIONING.md @@ -81,7 +81,7 @@ v:a/pr-open-check/1.1.0 v:wf/run_semgrep_scan/1.0.0 ``` -Multiple version labels are allowed on a PR. +Multiple version labels are allowed on a PR, one per component. ### Special Case: Untracked Versions @@ -109,7 +109,9 @@ A PR will fail automated validation if any of the following are true: - There are multiple version labels and one of them is `v:untracked` - A version label uses an invalid component name - X.Y.Z version does not appear in the component’s CHANGELOG.md -- There are duplicate or malformed version labels +- A component has more than one version label +- A version label's tag already exists, so the version is already released +- There are malformed version labels A PR cannot merge until version validation passes. @@ -163,7 +165,8 @@ Each released version must have a heading that includes the version label Minimum requirements: -- The header must contain `## X.Y.Z` exactly (this is what validation looks for). +- The header must be a line that is exactly `## X.Y.Z`, optionally followed by ` - title` + (this is what validation looks for). - The PR must add or update an entry for the version used in the label (`v:a/{component-name}/X.Y.Z` or `v:wf/{component-name}/X.Y.Z`). diff --git a/scripts/internal-ci/validate-version-labels/index.integration.test.js b/scripts/internal-ci/validate-version-labels/index.integration.test.js index a2b7d66..d540c7e 100644 --- a/scripts/internal-ci/validate-version-labels/index.integration.test.js +++ b/scripts/internal-ci/validate-version-labels/index.integration.test.js @@ -46,6 +46,49 @@ describe('validate-version-labels main module integration', () => { fs.unlinkSync(outputFile); }); + it('fails when a component has more than one version label', () => { + const labelInput = `${versionLabelPrefix}a/pr-open-check/1.0.0\n${versionLabelPrefix}a/pr-open-check/1.1.0`; + const outputFile = path.join(tmp, 'gha_duplicate_output.txt'); + const result = spawnSync('node', [scriptPath], { + cwd: projectRoot, + input: labelInput, + encoding: 'utf8', + env: { ...process.env, GITHUB_OUTPUT: outputFile }, + }); + const outputs = parseGithubOutput(outputFile); + expect(outputs.duplicateComponents).toBe('actions/pr-open-check'); + expect(outputs.validComponents).toBe(''); + expect(outputs.validationMessage).toContain('More than one version label'); + expect(outputs.isValid).toBe('false'); + expect(result.status).not.toBe(0); + fs.unlinkSync(outputFile); + }); + + it('fails when the tag listed in RELEASED_TAGS_FILE already exists', () => { + const labelInput = `${versionLabelPrefix}a/pr-open-check/1.0.0`; + const releasedTagsFile = path.join(tmp, 'released_tags.txt'); + fs.writeFileSync( + releasedTagsFile, + 'actions/pr-open-check/1.0.0\nworkflows/run_semgrep_scan/1.0.0\n', + 'utf8' + ); + const outputFile = path.join(tmp, 'gha_released_output.txt'); + const result = spawnSync('node', [scriptPath], { + cwd: projectRoot, + input: labelInput, + encoding: 'utf8', + env: { ...process.env, GITHUB_OUTPUT: outputFile, RELEASED_TAGS_FILE: releasedTagsFile }, + }); + const outputs = parseGithubOutput(outputFile); + expect(outputs.releasedVersions).toBe('actions/pr-open-check/1.0.0'); + expect(outputs.validComponents).toBe(''); + expect(outputs.validationMessage).toContain('already released'); + expect(outputs.isValid).toBe('false'); + expect(result.status).not.toBe(0); + fs.unlinkSync(releasedTagsFile); + fs.unlinkSync(outputFile); + }); + it('rejects the previous version label format', () => { const labelInput = 'version:actions/pr-open-check/1.0.0'; const outputFile = path.join(tmp, 'gha_old_format_output.txt'); diff --git a/scripts/internal-ci/validate-version-labels/index.js b/scripts/internal-ci/validate-version-labels/index.js index 63cc021..644bad8 100644 --- a/scripts/internal-ci/validate-version-labels/index.js +++ b/scripts/internal-ci/validate-version-labels/index.js @@ -1,12 +1,13 @@ -// validate-version-labels.js -// Script to validate version labels for CI workflows +// Validates the version labels on a pull request. // // Usage: -// node validate-version-labels.js -// echo -e "v:a/foo/1.2.3\nv:wf/bar/2.0.0\nother" | node scripts/internal-ci/validate-version-labels.js +// node scripts/internal-ci/validate-version-labels/index.js +// echo -e "v:a/foo/1.2.3\nv:wf/bar/2.0.0\nother" | node scripts/internal-ci/validate-version-labels/index.js // // Inputs: // - Newline-separated list of label strings (from file or stdin) +// - RELEASED_TAGS_FILE (optional): newline-separated list of existing release tags. When set, +// a label whose tag is listed fails validation. // // Outputs: // - Writes to $GITHUB_OUTPUT variable: @@ -15,8 +16,10 @@ // invalidVersionLabels: comma-separated list of invalid version labels // hasUntrackedVersion: true/false if untracked version label is present // componentVersionLabels: comma-separated list of valid component version labels +// duplicateComponents: comma-separated list of components with more than one version label // invalidComponents: comma-separated list of invalid component paths // missingChangelogs: comma-separated list of components missing changelog entries +// releasedVersions: comma-separated list of component versions that already have a tag // validComponents: comma-separated list of valid components with versions in format component-path/X.Y.Z // - Exits with code 1 on validation failure, 0 on success @@ -36,7 +39,7 @@ const componentTypeAliases = { a: 'actions', wf: 'workflows', }; -const semverPattern = '\\d+\\.\\d+\\.\\d+'; // Semantic versioning pattern e.g., v1.2.3 +const semverPattern = '\\d+\\.\\d+\\.\\d+'; // Semantic versioning pattern e.g., 1.2.3 // Pattern for valid version label: v:/component-name/X.Y.Z const componentVersionRegEx = new RegExp( `^${versionLabelPrefix}(${Object.keys(componentTypeAliases).join('|')})/([a-z0-9_-]+)/(${semverPattern})$` @@ -84,6 +87,26 @@ function parseComponentVersionLabels(componentVersionLabels) { return componentVersionMap; } +/// Returns the components that have more than one version label +/// It is assumed that component version labels are valid when this function is called +function getDuplicateComponents(componentVersionLabels) { + const labelCounts = {}; + componentVersionLabels.forEach(label => { + const componentTag = expandComponentVersionLabel(label); + const component = componentTag.substring(0, componentTag.lastIndexOf('/')); + labelCounts[component] = (labelCounts[component] || 0) + 1; + }); + return Object.keys(labelCounts).filter(component => labelCounts[component] > 1); +} + +/// Returns the component versions, as component-path/X.Y.Z, that already have a release tag +function getReleasedVersions(componentVersionMap, releasedTags) { + const tagSet = new Set(releasedTags); + return Object.entries(componentVersionMap) + .map(([component, version]) => `${component}/${version}`) + .filter(tag => tagSet.has(tag)); +} + /// Checks for existence of paths in the repo matching the component function getInvalidComponents(componentVersionMap) { if (Object.keys(componentVersionMap).length == 0) { @@ -99,7 +122,7 @@ function getInvalidComponents(componentVersionMap) { } } else if (path.startsWith('workflows/')) { const expectedFile = `.github/${path}.yml`; - if (!fs.existsSync(expectedFile) || !fs.lstatSync(expectedFile).isFile) { + if (!fs.existsSync(expectedFile) || !fs.lstatSync(expectedFile).isFile()) { invalidComponents.push(path); } } else { @@ -110,6 +133,14 @@ function getInvalidComponents(componentVersionMap) { return invalidComponents; } +/// Checks for a `## X.Y.Z` heading, optionally followed by ` - title` or ` β€” title` +function hasChangelogEntry(changelogContent, version) { + const escapedVersion = version.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + return new RegExp(`^## ${escapedVersion}(?:[ \\t]+[-β€”][ \\t].*)?[ \\t\\r]*$`, 'm').test( + changelogContent + ); +} + /// Checks for CHANGELOG.md entry matching the version for each component /// It is assumed that the components exist when this function is called function getMissingChangelogs(componentVersionMap) { @@ -133,8 +164,7 @@ function getMissingChangelogs(componentVersionMap) { continue; } const changelogContent = fs.readFileSync(changelogPath, 'utf8'); - const expectedVersionHeader = `## ${componentVersionMap[path]}`; - if (!changelogContent.includes(expectedVersionHeader)) { + if (!hasChangelogEntry(changelogContent, componentVersionMap[path])) { missingChangelogs.push(path); } } @@ -147,7 +177,9 @@ function validate( hasComponentVersionLabels, hasUntrackedVersion, hasInvalidComponents, - hasMissingChangelogs + hasMissingChangelogs, + hasDuplicateComponents = false, + hasReleasedVersions = false ) { const hasVersionLabels = hasInvalidVersionLabels || hasComponentVersionLabels || hasUntrackedVersion; @@ -165,12 +197,18 @@ function validate( validationMessage += '- Conflicting version labels found: both untracked and component version labels present\n'; } + if (hasDuplicateComponents) { + validationMessage += '- More than one version label found for a component\n'; + } if (hasInvalidComponents) { validationMessage += '- Non-existent components found (invalid paths)\n'; } if (hasMissingChangelogs) { validationMessage += '- Missing or incomplete CHANGELOG.md entries found\n'; } + if (hasReleasedVersions) { + validationMessage += '- Versions that are already released found (the tag exists)\n'; + } if (validationMessage === '') { validationMessage = 'Version validation passed'; @@ -186,12 +224,7 @@ function validate( // Main entry if (require.main === module) { // Accept labels from stdin or as a file argument - let rawLabels = ''; - if (process.argv.length > 2) { - rawLabels = fs.readFileSync(process.argv[2], 'utf8'); - } else { - rawLabels = fs.readFileSync(0, 'utf8'); // Read from stdin - } + const rawLabels = fs.readFileSync(process.argv.length > 2 ? process.argv[2] : 0, 'utf8'); const labels = getLabelArray(rawLabels); console.log('Normalized Labels:', labels); @@ -203,30 +236,45 @@ if (require.main === module) { const hasUntrackedVersion = versionLabels.includes(untrackedLabel); const componentVersionMap = parseComponentVersionLabels(componentVersionLabels); + const duplicateComponents = getDuplicateComponents(componentVersionLabels); const invalidComponents = getInvalidComponents(componentVersionMap); - let validComponentMap = Object.fromEntries( - Object.entries(componentVersionMap).filter( - ([component]) => !invalidComponents.includes(component) - ) + const withoutComponents = (componentMap, excluded) => + Object.fromEntries( + Object.entries(componentMap).filter(([component]) => !excluded.includes(component)) + ); + + let validComponentMap = withoutComponents( + componentVersionMap, + duplicateComponents.concat(invalidComponents) ); const missingChangelogs = getMissingChangelogs(validComponentMap); - + validComponentMap = withoutComponents(validComponentMap, missingChangelogs); + + const releasedTagsFile = process.env.RELEASED_TAGS_FILE; + const releasedVersions = releasedTagsFile + ? getReleasedVersions( + validComponentMap, + getLabelArray(fs.readFileSync(releasedTagsFile, 'utf8')) + ) + : []; validComponentMap = Object.fromEntries( Object.entries(validComponentMap).filter( - ([component]) => !missingChangelogs.includes(component) + ([component, version]) => !releasedVersions.includes(`${component}/${version}`) ) ); // Validate - let { isValid, validationMessage } = validate( + const { isValid, validationMessage } = validate( invalidVersionLabels.length > 0, componentVersionLabels.length, hasUntrackedVersion, invalidComponents.length > 0, - missingChangelogs.length > 0 + missingChangelogs.length > 0, + duplicateComponents.length > 0, + releasedVersions.length > 0 ); // write outputs for use in later steps @@ -239,8 +287,10 @@ if (require.main === module) { githubOutput, `componentVersionLabels=${componentVersionLabels.join(', ')}\n` ); + fs.appendFileSync(githubOutput, `duplicateComponents=${duplicateComponents.join(', ')}\n`); fs.appendFileSync(githubOutput, `invalidComponents=${invalidComponents.join(', ')}\n`); fs.appendFileSync(githubOutput, `missingChangelogs=${missingChangelogs.join(', ')}\n`); + fs.appendFileSync(githubOutput, `releasedVersions=${releasedVersions.join(', ')}\n`); fs.appendFileSync( githubOutput, `validationMessage=${validationMessage.replace(/\n/g, '\\n')}\n` @@ -270,7 +320,10 @@ module.exports = { getComponentVersionLabels, expandComponentVersionLabel, parseComponentVersionLabels, + getDuplicateComponents, + getReleasedVersions, getInvalidComponents, + hasChangelogEntry, getMissingChangelogs, validate, versionLabelPrefix, diff --git a/scripts/internal-ci/validate-version-labels/index.unit.test.js b/scripts/internal-ci/validate-version-labels/index.unit.test.js index ead4567..b19c5dd 100644 --- a/scripts/internal-ci/validate-version-labels/index.unit.test.js +++ b/scripts/internal-ci/validate-version-labels/index.unit.test.js @@ -5,7 +5,10 @@ const { getComponentVersionLabels, expandComponentVersionLabel, parseComponentVersionLabels, + getDuplicateComponents, + getReleasedVersions, getInvalidComponents, + hasChangelogEntry, getMissingChangelogs, validate, versionLabelPrefix, @@ -104,6 +107,45 @@ describe('parseComponentVersionLabels', () => { }); }); +describe('getDuplicateComponents', () => { + it('should return components with more than one version label', () => { + const componentLabels = [ + `${versionLabelPrefix}a/component-a/1.2.3`, + `${versionLabelPrefix}wf/component-b/2.3.4`, + `${versionLabelPrefix}a/component-a/1.2.4`, + `${versionLabelPrefix}wf/component-a/1.0.0`, + ]; + expect(getDuplicateComponents(componentLabels)).toEqual(['actions/component-a']); + }); + + it('should return an empty array when each component has one label', () => { + const componentLabels = [ + `${versionLabelPrefix}a/component-a/1.2.3`, + `${versionLabelPrefix}wf/component-a/1.2.3`, + ]; + expect(getDuplicateComponents(componentLabels)).toEqual([]); + }); +}); + +describe('getReleasedVersions', () => { + const releasedTags = ['actions/component-a/1.0.0', 'workflows/component-b/2.0.0']; + + it('should return component versions that already have a tag', () => { + const componentVersionMap = { + 'actions/component-a': '1.0.0', + 'workflows/component-b': '2.0.1', + }; + expect(getReleasedVersions(componentVersionMap, releasedTags)).toEqual([ + 'actions/component-a/1.0.0', + ]); + }); + + it('should not match a tag of the same version for another component type', () => { + const componentVersionMap = { 'workflows/component-a': '1.0.0' }; + expect(getReleasedVersions(componentVersionMap, releasedTags)).toEqual([]); + }); +}); + // It is assumed that path is correctly formatted when this function is called (dir1/dir2) describe('getInvalidComponents', () => { const fs = require('fs'); @@ -117,6 +159,7 @@ describe('getInvalidComponents', () => { '.github/actions/not-a-directory': 'file', // invalid action, extensionless file instead of directory '.github/workflows/not-a-file': 'directory', // invalid workflow - subdirectory instead of file '.github/workflows/not-yaml.txt': 'file', // invalid workflow - wrong file type + '.github/workflows/directory-workflow.yml': 'directory', // invalid workflow - directory named like a workflow '.github/foo/bar.yml': 'file', // invalid type }; @@ -190,6 +233,38 @@ describe('getInvalidComponents', () => { const invalidComponents = getInvalidComponents(componentVersionMap); expect(invalidComponents).toEqual(['workflows/not-a-file', 'workflows/not-yaml']); }); + + it('should return workflows whose .yml path is a directory', () => { + const componentVersionMap = { + 'workflows/directory-workflow': '1.0.0', + }; + const invalidComponents = getInvalidComponents(componentVersionMap); + expect(invalidComponents).toEqual(['workflows/directory-workflow']); + }); +}); + +describe('hasChangelogEntry', () => { + it.each([ + ['## 1.0.0\n- Initial release', '1.0.0'], + ['# Changelog\n\n## 1.0.1\n\n## 1.0.0', '1.0.0'], + ['## 1.0.0 - Initial Release', '1.0.0'], + ['## 1.0.0 β€” Initial Release', '1.0.0'], + ['## 1.0.0\r\n- Windows line endings', '1.0.0'], + ])('should find the entry in %j for %s', (content, version) => { + expect(hasChangelogEntry(content, version)).toBe(true); + }); + + it.each([ + ['## 1.0.10', '1.0.1'], + ['### 1.0.0', '1.0.0'], + ['## 11.0.0', '1.0.0'], + ['## 1x0x0', '1.0.0'], + ['## v1.0.0', '1.0.0'], + ['## 1.0.0-beta', '1.0.0'], + ['Released in ## 1.0.0', '1.0.0'], + ])('should not find the entry in %j for %s', (content, version) => { + expect(hasChangelogEntry(content, version)).toBe(false); + }); }); // It is assumed that path is correctly formatted when this function is called (dir1/dir2) @@ -340,6 +415,31 @@ describe('validate', () => { }, ]; + scenarios.push( + { + description: 'a component has more than one version label', + hasUntrackedLabel: false, + hasInvalidLabels: false, + hasComponentLabels: true, + hasInvalidComponents: false, + hasMissingChangelogs: false, + hasDuplicateComponents: true, + expectedIsValid: false, + expectedMessageFragment: '- More than one version label found for a component', + }, + { + description: 'a version is already released', + hasUntrackedLabel: false, + hasInvalidLabels: false, + hasComponentLabels: true, + hasInvalidComponents: false, + hasMissingChangelogs: false, + hasReleasedVersions: true, + expectedIsValid: false, + expectedMessageFragment: '- Versions that are already released', + } + ); + scenarios.forEach(scenario => { it(`should ${scenario.expectedIsValid ? 'pass' : 'fail'} when ${scenario.description}`, () => { const result = validate( @@ -347,7 +447,9 @@ describe('validate', () => { scenario.hasComponentLabels, scenario.hasUntrackedLabel, scenario.hasInvalidComponents, - scenario.hasMissingChangelogs + scenario.hasMissingChangelogs, + scenario.hasDuplicateComponents, + scenario.hasReleasedVersions ); expect(result.isValid).toBe(scenario.expectedIsValid); expect(result.validationMessage).toContain(scenario.expectedMessageFragment); From 7618d4b6fe09a067f950fc6e2bfeecf9521aecdf Mon Sep 17 00:00:00 2001 From: ssvoss Date: Sun, 8 Nov 2026 11:40:07 -0500 Subject: [PATCH 2/3] CORE-6263: tag only main merges, and all tags or none Set Version Tags ran for every closed pull request, so a labelled PR merged into any other branch pushed release tags. It now runs only for pull requests into main, and checks out the PR's merge commit so the tags point at it explicitly. Tags were created and pushed one at a time, so one that already existed stopped the loop after the earlier ones were pushed. The step now checks every tag against the remote first and fails without pushing any if one exists, then pushes them with a single atomic push. get-version-tags reuses the validation script's label filters and fails on more than one version label for a component, rather than tagging both. Co-Authored-By: Claude Opus 5.5 --- ...nternal_on_merge_tag_versions.unit.test.js | 111 ++++++++++++++++++ .../internal_on_merge_tag_versions.yml | 29 ++++- .../index.integration.test.js | 17 +++ scripts/internal-ci/get-version-tags/index.js | 27 +++-- 4 files changed, 174 insertions(+), 10 deletions(-) create mode 100644 .github/workflows/TESTS/internal_on_merge_tag_versions.unit.test.js diff --git a/.github/workflows/TESTS/internal_on_merge_tag_versions.unit.test.js b/.github/workflows/TESTS/internal_on_merge_tag_versions.unit.test.js new file mode 100644 index 0000000..77012a9 --- /dev/null +++ b/.github/workflows/TESTS/internal_on_merge_tag_versions.unit.test.js @@ -0,0 +1,111 @@ +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); +const { extractRunScript, readWorkflow, runScript } = require('./workflow-test-helpers'); + +const workflowFile = 'internal_on_merge_tag_versions.yml'; + +const gitEnv = { + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_NOSYSTEM: '1', + GIT_AUTHOR_NAME: 'Test', + GIT_AUTHOR_EMAIL: 'test@example.com', + GIT_COMMITTER_NAME: 'Test', + GIT_COMMITTER_EMAIL: 'test@example.com', +}; + +function git(cwd, args) { + const result = spawnSync('git', args, { + cwd, + encoding: 'utf8', + env: { ...process.env, ...gitEnv }, + }); + if (result.status !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${result.stderr}`); + } + return result.stdout.trim(); +} + +describe('internal_on_merge_tag_versions trigger', () => { + const workflow = readWorkflow(workflowFile); + + test('runs only for pull requests into main', () => { + expect(workflow).toMatch(/pull_request:\n {4}branches: \[main\]\n {4}types: \[closed\]/); + }); + + test('tags the merge commit of the pull request', () => { + expect(workflow).toContain('ref: ${{ github.event.pull_request.merge_commit_sha }}'); + }); +}); + +describe('internal_on_merge_tag_versions Create Version Tags', () => { + let directory; + let remote; + let checkout; + + beforeEach(() => { + directory = fs.mkdtempSync(path.join(os.tmpdir(), 'tag-versions-')); + remote = path.join(directory, 'remote.git'); + checkout = path.join(directory, 'checkout'); + git(directory, ['init', '--quiet', '--bare', remote]); + git(directory, ['init', '--quiet', checkout]); + fs.writeFileSync(path.join(checkout, 'file.txt'), 'first\n'); + git(checkout, ['add', 'file.txt']); + git(checkout, ['commit', '--quiet', '-m', 'first']); + git(checkout, ['remote', 'add', 'origin', remote]); + git(checkout, ['tag', 'actions/existing/1.0.0']); + git(checkout, ['push', '--quiet', 'origin', 'refs/tags/actions/existing/1.0.0']); + git(checkout, ['tag', '--delete', 'actions/existing/1.0.0']); + fs.writeFileSync(path.join(checkout, 'file.txt'), 'second\n'); + git(checkout, ['commit', '--quiet', '-am', 'second']); + }); + + afterEach(() => { + fs.rmSync(directory, { recursive: true, force: true }); + }); + + const createTags = componentTags => + runScript(extractRunScript(workflowFile, 'set-version-tags', 'Create Version Tags'), { + cwd: checkout, + env: { ...gitEnv, COMPONENT_TAGS: componentTags }, + }); + + const remoteTags = () => + git(checkout, ['ls-remote', '--tags', '--refs', 'origin']) + .split('\n') + .filter(Boolean) + .map(line => line.split('\t')[1].replace('refs/tags/', '')) + .sort(); + + test('pushes every tag on the checked-out commit', () => { + const result = createTags('actions/new-action/1.0.0,workflows/new_workflow/2.0.0'); + + expect(result.status).toBe(0); + expect(remoteTags()).toEqual([ + 'actions/existing/1.0.0', + 'actions/new-action/1.0.0', + 'workflows/new_workflow/2.0.0', + ]); + const head = git(checkout, ['rev-parse', 'HEAD']); + expect(git(remote, ['rev-parse', 'workflows/new_workflow/2.0.0'])).toBe(head); + expect(result.summary).toContain('βœ… Created and pushed tag: actions/new-action/1.0.0'); + }); + + test('pushes no tags when any of them already exists', () => { + const result = createTags('actions/new-action/1.0.0,actions/existing/1.0.0'); + + expect(result.status).toBe(1); + expect(result.stdout).toContain( + '::error title=Tags Already Exist::These tags already exist, so no tags were created: actions/existing/1.0.0' + ); + expect(remoteTags()).toEqual(['actions/existing/1.0.0']); + }); + + test('does nothing when there are no component tags', () => { + const result = createTags(''); + + expect(result.status).toBe(0); + expect(remoteTags()).toEqual(['actions/existing/1.0.0']); + }); +}); diff --git a/.github/workflows/internal_on_merge_tag_versions.yml b/.github/workflows/internal_on_merge_tag_versions.yml index ed90223..c9c71d3 100644 --- a/.github/workflows/internal_on_merge_tag_versions.yml +++ b/.github/workflows/internal_on_merge_tag_versions.yml @@ -2,6 +2,7 @@ name: Set Version Tags on: pull_request: + branches: [main] types: [closed] permissions: {} @@ -27,12 +28,14 @@ jobs: - name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + ref: ${{ github.event.pull_request.merge_commit_sha }} persist-credentials: true # Required for git push of the version tags. - name: Set up Node.js uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 with: node-version-file: .nvmrc + package-manager-cache: false - name: Get all labels from PR id: get_labels @@ -73,8 +76,32 @@ jobs: COMPONENT_TAGS: ${{ steps.get_tags.outputs.componentTags }} run: | IFS=',' read -r -a tagsArray <<< "$COMPONENT_TAGS" + if [ "${#tagsArray[@]}" -eq 0 ]; then + exit 0 + fi + + released_tags=$(git ls-remote --tags --refs origin | cut -f2) + existing=() + refs=() + for tag in "${tagsArray[@]}"; do + if grep -Fxq "refs/tags/$tag" <<< "$released_tags"; then + existing+=("$tag") + fi + refs+=("refs/tags/$tag") + done + + if [ "${#existing[@]}" -gt 0 ]; then + message="These tags already exist, so no tags were created: ${existing[*]}" + echo "❌ $message" >> "$GITHUB_STEP_SUMMARY" + echo "::error title=Tags Already Exist::$message" + exit 1 + fi + for tag in "${tagsArray[@]}"; do git tag "$tag" - git push origin "$tag" + done + git push --atomic origin "${refs[@]}" + + for tag in "${tagsArray[@]}"; do echo "βœ… Created and pushed tag: $tag" >> "$GITHUB_STEP_SUMMARY" done diff --git a/scripts/internal-ci/get-version-tags/index.integration.test.js b/scripts/internal-ci/get-version-tags/index.integration.test.js index 4006ca9..ee3770b 100644 --- a/scripts/internal-ci/get-version-tags/index.integration.test.js +++ b/scripts/internal-ci/get-version-tags/index.integration.test.js @@ -50,4 +50,21 @@ describe('get-version-tags main module integration', () => { fs.unlinkSync(labelFile); fs.unlinkSync(outputFile); }); + + it('fails without writing tags when a component has more than one version label', () => { + const labelInput = `${versionLabelPrefix}a/pr-open-check/1.0.0\n${versionLabelPrefix}a/pr-open-check/1.1.0`; + const unique = Date.now() + Math.random(); + const labelFile = path.join(tmp, `labels-${unique}.txt`); + const outputFile = path.join(tmp, `gha_output-${unique}.txt`); + fs.writeFileSync(labelFile, labelInput, 'utf8'); + const result = spawnSync('node', [scriptPath, labelFile], { + cwd: projectRoot, + encoding: 'utf8', + env: { ...process.env, GITHUB_OUTPUT: outputFile }, + }); + expect(result.status).toBe(1); + expect(result.stderr).toContain('More than one version label for: actions/pr-open-check'); + expect(fs.existsSync(outputFile)).toBe(false); + fs.unlinkSync(labelFile); + }); }); diff --git a/scripts/internal-ci/get-version-tags/index.js b/scripts/internal-ci/get-version-tags/index.js index 64ce4f1..51e9a63 100644 --- a/scripts/internal-ci/get-version-tags/index.js +++ b/scripts/internal-ci/get-version-tags/index.js @@ -1,32 +1,41 @@ -/// Reads labels from all_labels.txt and outputs whether untracked is present +/// Reads labels from a file and outputs whether untracked is present /// and a comma-separated list of component version tags for GitHub Actions. /// /// Outputs: -/// HAS_UNTRACKED=true|false +/// hasUntracked=true|false /// componentTags=type/name/X.Y.Z,... +/// Exits with code 1, writing no outputs, when a component has more than one version label. /// Usage: /// node scripts/internal-ci/get-version-tags/index.js const fs = require('fs'); const { getLabelArray, + getVersionLabelArray, + getComponentVersionLabels, + getDuplicateComponents, untrackedLabel, - componentVersionRegEx, expandComponentVersionLabel, } = require('../validate-version-labels/index.js'); // Main entry if (require.main === module) { - // Accept labels from stdin or as a file argument const rawLabels = fs.readFileSync(process.argv[2], 'utf8'); - const labels = getLabelArray(rawLabels); + const versionLabels = getVersionLabelArray(getLabelArray(rawLabels)); - const hasUntracked = labels.includes(untrackedLabel); + const hasUntracked = versionLabels.includes(untrackedLabel); - const componentTags = labels - .filter(label => componentVersionRegEx.test(label)) - .map(expandComponentVersionLabel); + const componentVersionLabels = getComponentVersionLabels(versionLabels); + const duplicateComponents = getDuplicateComponents(componentVersionLabels); + if (duplicateComponents.length > 0) { + console.error( + `::error title=Duplicate Version Labels::More than one version label for: ${duplicateComponents.join(', ')}` + ); + process.exit(1); + } + + const componentTags = componentVersionLabels.map(expandComponentVersionLabel); // write outputs for use in later steps const githubOutput = process.env.GITHUB_OUTPUT; From a435b4edbdeddbf3a83d180b6154f4f24176c7c8 Mon Sep 17 00:00:00 2001 From: ssvoss Date: Sun, 8 Nov 2026 11:55:40 -0500 Subject: [PATCH 3/3] CORE-6263: fail a tag lookup that can't read the remote Steps without an explicit shell run as `bash -e`, without pipefail, so a failed `git ls-remote` piped into `cut` and `sed` passed with an empty tag list. Validation could then pass an already-released version, and the merge-time check could miss a tag that exists. Both workflows now default to `shell: bash`, which adds pipefail, as the reusable workflows do. A rejected atomic push now says that no tags were pushed and that another run may have created one of them since the check, instead of leaving only git's error. VERSIONING.md also names the em dash title separator that validation accepts and existing changelogs use. Co-Authored-By: Claude Opus 5.5 --- ...nternal_on_merge_tag_versions.unit.test.js | 18 +++++ ...pr_validate_component_version.unit.test.js | 81 +++++++++++++++++++ .../internal_on_merge_tag_versions.yml | 11 ++- ...ernal_on_pr_validate_component_version.yml | 7 +- VERSIONING.md | 4 +- 5 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/TESTS/internal_on_pr_validate_component_version.unit.test.js diff --git a/.github/workflows/TESTS/internal_on_merge_tag_versions.unit.test.js b/.github/workflows/TESTS/internal_on_merge_tag_versions.unit.test.js index 77012a9..4aa363a 100644 --- a/.github/workflows/TESTS/internal_on_merge_tag_versions.unit.test.js +++ b/.github/workflows/TESTS/internal_on_merge_tag_versions.unit.test.js @@ -37,6 +37,10 @@ describe('internal_on_merge_tag_versions trigger', () => { test('tags the merge commit of the pull request', () => { expect(workflow).toContain('ref: ${{ github.event.pull_request.merge_commit_sha }}'); }); + + test('runs steps in bash with pipefail, so a failed tag lookup fails the step', () => { + expect(workflow).toContain('defaults:\n run:\n shell: bash\n'); + }); }); describe('internal_on_merge_tag_versions Create Version Tags', () => { @@ -102,6 +106,20 @@ describe('internal_on_merge_tag_versions Create Version Tags', () => { expect(remoteTags()).toEqual(['actions/existing/1.0.0']); }); + test('reports a rejected push and pushes no tags', () => { + const hook = path.join(remote, 'hooks', 'pre-receive'); + fs.writeFileSync(hook, '#!/bin/sh\nexit 1\n', { mode: 0o755 }); + + const result = createTags('actions/new-action/1.0.0,workflows/new_workflow/2.0.0'); + + expect(result.status).toBe(1); + expect(result.stdout).toContain( + '::error title=Tag Push Rejected::The atomic push was rejected, so no tags were pushed.' + ); + expect(result.summary).not.toContain('βœ…'); + expect(remoteTags()).toEqual(['actions/existing/1.0.0']); + }); + test('does nothing when there are no component tags', () => { const result = createTags(''); diff --git a/.github/workflows/TESTS/internal_on_pr_validate_component_version.unit.test.js b/.github/workflows/TESTS/internal_on_pr_validate_component_version.unit.test.js new file mode 100644 index 0000000..4e6b312 --- /dev/null +++ b/.github/workflows/TESTS/internal_on_pr_validate_component_version.unit.test.js @@ -0,0 +1,81 @@ +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); +const { extractRunScript, readWorkflow, runScript } = require('./workflow-test-helpers'); + +const workflowFile = 'internal_on_pr_validate_component_version.yml'; + +const gitEnv = { + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_NOSYSTEM: '1', + GIT_AUTHOR_NAME: 'Test', + GIT_AUTHOR_EMAIL: 'test@example.com', + GIT_COMMITTER_NAME: 'Test', + GIT_COMMITTER_EMAIL: 'test@example.com', +}; + +function git(cwd, args) { + const result = spawnSync('git', args, { + cwd, + encoding: 'utf8', + env: { ...process.env, ...gitEnv }, + }); + if (result.status !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${result.stderr}`); + } + return result.stdout.trim(); +} + +describe('internal_on_pr_validate_component_version shell', () => { + test('runs steps in bash with pipefail, so a failed tag lookup fails the step', () => { + expect(readWorkflow(workflowFile)).toContain('defaults:\n run:\n shell: bash\n'); + }); +}); + +describe('internal_on_pr_validate_component_version Get released version tags', () => { + let directory; + let checkout; + + beforeEach(() => { + directory = fs.mkdtempSync(path.join(os.tmpdir(), 'released-tags-')); + const remote = path.join(directory, 'remote.git'); + checkout = path.join(directory, 'checkout'); + git(directory, ['init', '--quiet', '--bare', remote]); + git(directory, ['init', '--quiet', checkout]); + fs.writeFileSync(path.join(checkout, 'file.txt'), 'first\n'); + git(checkout, ['add', 'file.txt']); + git(checkout, ['commit', '--quiet', '-m', 'first']); + git(checkout, ['remote', 'add', 'origin', remote]); + git(checkout, ['tag', 'actions/pr-open-check/1.0.0']); + git(checkout, ['tag', 'workflows/run_semgrep_scan/2.0.0']); + git(checkout, ['push', '--quiet', 'origin', '--tags']); + }); + + afterEach(() => { + fs.rmSync(directory, { recursive: true, force: true }); + }); + + const getReleasedTags = () => + runScript( + extractRunScript(workflowFile, 'validate-version-labels', 'Get released version tags'), + { cwd: checkout, env: gitEnv } + ); + + test('writes the remote tag names to released_tags.txt', () => { + const result = getReleasedTags(); + + expect(result.status).toBe(0); + expect(fs.readFileSync(path.join(checkout, 'released_tags.txt'), 'utf8')).toBe( + 'actions/pr-open-check/1.0.0\nworkflows/run_semgrep_scan/2.0.0\n' + ); + }); + + test('fails when the remote cannot be read', () => { + git(checkout, ['remote', 'set-url', 'origin', path.join(directory, 'missing.git')]); + + const result = getReleasedTags(); + + expect(result.status).not.toBe(0); + }); +}); diff --git a/.github/workflows/internal_on_merge_tag_versions.yml b/.github/workflows/internal_on_merge_tag_versions.yml index c9c71d3..1a1a19d 100644 --- a/.github/workflows/internal_on_merge_tag_versions.yml +++ b/.github/workflows/internal_on_merge_tag_versions.yml @@ -7,6 +7,10 @@ on: permissions: {} +defaults: + run: + shell: bash + jobs: no-merge: if: github.event.pull_request.merged == false @@ -100,7 +104,12 @@ jobs: for tag in "${tagsArray[@]}"; do git tag "$tag" done - git push --atomic origin "${refs[@]}" + if ! git push --atomic origin "${refs[@]}"; then + message="The atomic push was rejected, so no tags were pushed. Another run may have created one of these tags since the check: ${tagsArray[*]}" + echo "❌ $message" >> "$GITHUB_STEP_SUMMARY" + echo "::error title=Tag Push Rejected::$message" + exit 1 + fi for tag in "${tagsArray[@]}"; do echo "βœ… Created and pushed tag: $tag" >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/internal_on_pr_validate_component_version.yml b/.github/workflows/internal_on_pr_validate_component_version.yml index a2529fb..68ff3ca 100644 --- a/.github/workflows/internal_on_pr_validate_component_version.yml +++ b/.github/workflows/internal_on_pr_validate_component_version.yml @@ -17,6 +17,10 @@ concurrency: permissions: {} +defaults: + run: + shell: bash + jobs: validate-version-labels: name: Validate PR Version Labels @@ -88,7 +92,8 @@ jobs: echo "$all_labels" > all_labels.txt - name: Get released version tags - run: git ls-remote --tags --refs origin | cut -f2 | sed 's#^refs/tags/##' > released_tags.txt + run: | + git ls-remote --tags --refs origin | cut -f2 | sed 's#^refs/tags/##' > released_tags.txt - name: Set up Node.js uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 diff --git a/VERSIONING.md b/VERSIONING.md index e6a3e9b..8c1732c 100644 --- a/VERSIONING.md +++ b/VERSIONING.md @@ -165,8 +165,8 @@ Each released version must have a heading that includes the version label Minimum requirements: -- The header must be a line that is exactly `## X.Y.Z`, optionally followed by ` - title` - (this is what validation looks for). +- The header must be a line that is exactly `## X.Y.Z`, optionally followed by ` - title` or + ` β€” title` (this is what validation looks for). - The PR must add or update an entry for the version used in the label (`v:a/{component-name}/X.Y.Z` or `v:wf/{component-name}/X.Y.Z`).