From c4719c95b14a5b03abc8f734d43509da7a068c2e Mon Sep 17 00:00:00 2001 From: Josef Schlehofer Date: Sun, 26 Jul 2026 00:26:16 +0200 Subject: [PATCH 1/2] cloudflare-worker: accept git revert commit subjects Commits made by `git revert` have the subject Revert "", so the package prefix ends up inside the quotes and the bot rejects them. The author cannot fix that without breaking the link to the reverted commit. Accept this format, including reverts of reverts and the prefixed : Revert "..." variant. For them the subject layout rules are skipped, the Revert "..." wrapper is excluded from the length limit, and the PKG_VERSION check is skipped, because a revert restores the previous version. Subjects like 'Revert the broken change' are still rejected. The new allow_revert config key (default true) turns this off. Signed-off-by: Josef Schlehofer --- README.md | 3 + cloudflare-worker/src/config.js | 1 + cloudflare-worker/src/validators.js | 100 ++++++++++++++----- cloudflare-worker/test/validators.test.js | 115 +++++++++++++++++++++- 4 files changed, 192 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 4fa1844..6f8c9d8 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,7 @@ Focuses on Git history hygiene, developer metadata constraints, and layout stand * **Identity Integrity:** Validates author and committer name formats and strictly blocks generic GitHub `noreply.github.com` email addresses. * **Linked GitHub Account:** Verifies that the commit author email address is registered and verified on a GitHub account, linking the commit to a valid GitHub username. Can be downgraded to a non-blocking warning or disabled entirely, see `require_linked_github_account` below. * **Autosquash Compliance:** Automatically bypasses style constraints for development-phase `fixup!` and `squash!` syntax blocks. +* **Revert Compliance (`allow_revert`):** Accepts the subject line `git revert` generates — `Revert ""`, its nested `Revert "Revert """` form, and the prefixed `: Revert "..."` variant. The quoted part is copied verbatim from the reverted commit, so the prefix and lowercase rules are not applied to it, and the `Revert "..."` wrapper is excluded from the subject length limits. `check_pkg_version` is skipped for reverts too, since a revert restores the `PKG_VERSION` that preceded the commit its subject quotes. * **Subject String Hygiene:** Enforces `: ` prefix headers, checks lowercase starting strings post-prefix, and rejects trailing periods. * **Length Constraints:** Implements dual-layered (soft and hard) line width boundaries for both subject lines and description body text blocks. * **Signed-off-by Check:** Ensures a consistent, properly structured `Signed-off-by:` declaration is present and matches the original author metadata. @@ -122,6 +123,7 @@ Some configuration keys offer advanced options: * `check_pkg_name_reuse`: Set to `true` (default) to detect and reject reuse of the `PKG_NAME` variable in `call`, `define`, and `eval` lines, or `false` to disable. * `show_force_push_tip`: Set to `true` (default) to append a helpful tip regarding how to correct validation errors using force-pushing. Set to `false` to disable. * `check_openwrt_spelling`: Set to `true` (default) to validate the correct capitalization of "OpenWrt" in commit subjects and descriptions. Set to `false` to disable. +* `allow_revert`: Set to `true` (default) to accept the subject format produced by `git revert` (`Revert ""`, nested reverts, and the prefixed `: Revert "..."` variant). Set to `false` to hold revert commits to the regular subject rules. * `enable_stale_bot`: Set to `true` to enable the stale PR bot cleanup for this repository. Defaults to `false` (opt-in). * `enable_labeler_yml`: Set to `true` to enable dynamic pull request labeling based on matching files in the `.github/labeler.yml` configuration file. Defaults to `false` (opt-in). * `enable_issue_labeller`: Set to `true` to enable automated issue form validation and labelling (replaces the GitHub Actions `issue-labeller.yml` workflow). Defaults to `false` (opt-in). @@ -136,6 +138,7 @@ Here is a comprehensive example containing all available toggle options: "check_signoff": true, "check_signature": true, "allow_autosquash": true, + "allow_revert": true, "enable_comments": true, "show_force_push_tip": true, "max_subject_len_soft": 60, diff --git a/cloudflare-worker/src/config.js b/cloudflare-worker/src/config.js index a8503c8..76a106b 100644 --- a/cloudflare-worker/src/config.js +++ b/cloudflare-worker/src/config.js @@ -6,6 +6,7 @@ export const DEFAULT_CONFIG = { check_signoff: true, check_signature: true, allow_autosquash: true, + allow_revert: true, enable_comments: true, show_force_push_tip: true, max_subject_len_soft: 60, diff --git a/cloudflare-worker/src/validators.js b/cloudflare-worker/src/validators.js index a404f8e..dc4ed0a 100644 --- a/cloudflare-worker/src/validators.js +++ b/cloudflare-worker/src/validators.js @@ -80,6 +80,32 @@ export function isVirtuallyIdentical(subject, body, pkgName) { return !hasMeaningfulWord; } +// `git revert` builds the subject from the reverted commit verbatim as +// `Revert ""`, reverting a revert nests the wrapper another +// level, and OpenWrt also uses a prefixed `: Revert ""` variant. +// None of these can satisfy the regular subject rules - the package prefix sits +// inside the quotes, `Revert` is capitalized, and the wrapper eats into the +// length budget - and the author cannot rewrite the quoted part without losing +// the reference to the commit being reverted. +// Returns { prefix, original, depth } for a revert subject, null otherwise. +export function parseRevertSubject(subject) { + if (typeof subject !== 'string') return null; + + // `(?:: )*` also covers `tools/cmake: ` and chained `toolchain: binutils: ` prefixes. + const outer = subject.trim().match(/^((?:[a-zA-Z0-9_-]+(?:\/[a-zA-Z0-9_-]+)?: )*)[Rr]evert "(.+)"$/); + if (!outer) return null; + + let original = outer[2]; + let depth = 1; + let nested; + while ((nested = original.match(/^[Rr]evert "(.+)"$/))) { + original = nested[1]; + depth++; + } + + return { prefix: outer[1], original, depth }; +} + async function getSshKeyFingerprint(sigText) { try { let cleanSig = sigText.replace(/-----[a-zA-Z0-9\s]+-----/g, ''); @@ -191,42 +217,57 @@ export async function validateFormalities(fullCommit, CONFIG) { subject = subject.replace(/^(fixup!|squash!)\s+/, ''); } + const revert = CONFIG.allow_revert === false ? null : parseRevertSubject(subject); + if (!isAutosquash) { if (/^\s/.test(lines[0])) subjectErrors.push("Commit subject must not start with whitespace"); - - // Special case for tools/* prefix (e.g., tools/cmake: backport bootstrap fix) - // These use a subdirectory naming convention like tools/cmake, tools/bison, etc. - const toolsPrefixMatch = subject.match(/^(tools\/[a-zA-Z0-9_-]+): /); - if (toolsPrefixMatch) { - const afterPrefix = subject.replace(/^(tools\/[a-zA-Z0-9_-]+): \s*/, ''); - if (afterPrefix.length > 0 && afterPrefix[0] === afterPrefix[0].toUpperCase() && /[a-zA-Z]/.test(afterPrefix[0])) { - subjectErrors.push("Commit subject must start with a lower-case word after the prefix"); - } - if (subject.endsWith('.')) { - subjectErrors.push("Commit subject must not end with a period"); - } - } else if (!/^[a-zA-Z0-9_-]+: /.test(subject)) { - subjectErrors.push("Commit subject must start with `: `"); - } else { - const afterPrefix = subject.replace(/^[a-zA-Z0-9_-]+: \s*/, ''); - if (afterPrefix.length > 0 && afterPrefix[0] === afterPrefix[0].toUpperCase() && /[a-zA-Z]/.test(afterPrefix[0])) { - subjectErrors.push("Commit subject must start with a lower-case word after the prefix"); - } - if (subject.endsWith('.')) { - subjectErrors.push("Commit subject must not end with a period"); + + // The quoted part of a revert subject is copied from the reverted commit, + // so the prefix/lower-case/period rules apply to the original subject and + // not to the wrapper `git revert` generated around it. + if (!revert) { + // Special case for tools/* prefix (e.g., tools/cmake: backport bootstrap fix) + // These use a subdirectory naming convention like tools/cmake, tools/bison, etc. + const toolsPrefixMatch = subject.match(/^(tools\/[a-zA-Z0-9_-]+): /); + if (toolsPrefixMatch) { + const afterPrefix = subject.replace(/^(tools\/[a-zA-Z0-9_-]+): \s*/, ''); + if (afterPrefix.length > 0 && afterPrefix[0] === afterPrefix[0].toUpperCase() && /[a-zA-Z]/.test(afterPrefix[0])) { + subjectErrors.push("Commit subject must start with a lower-case word after the prefix"); + } + if (subject.endsWith('.')) { + subjectErrors.push("Commit subject must not end with a period"); + } + } else if (!/^[a-zA-Z0-9_-]+: /.test(subject)) { + subjectErrors.push("Commit subject must start with `: `"); + } else { + const afterPrefix = subject.replace(/^[a-zA-Z0-9_-]+: \s*/, ''); + if (afterPrefix.length > 0 && afterPrefix[0] === afterPrefix[0].toUpperCase() && /[a-zA-Z]/.test(afterPrefix[0])) { + subjectErrors.push("Commit subject must start with a lower-case word after the prefix"); + } + if (subject.endsWith('.')) { + subjectErrors.push("Commit subject must not end with a period"); + } } } } - const subjectLen = lines[0].length; + // Measure a revert against the subject underneath the `Revert "..."` wrapper: + // the wrapper is generated from the reverted commit, so the author cannot + // shorten it without breaking the reference. + const subjectLen = revert ? (revert.prefix + revert.original).length : lines[0].length; + const lenSuffix = revert ? ' chars, excluding the `Revert "..."` wrapper' : ' chars'; if (subjectLen > CONFIG.max_subject_len_hard) { - subjectErrors.push(`Subject line exceeds hard limit (${subjectLen}/${CONFIG.max_subject_len_hard} chars)`); + subjectErrors.push(`Subject line exceeds hard limit (${subjectLen}/${CONFIG.max_subject_len_hard}${lenSuffix})`); } else if (subjectLen > CONFIG.max_subject_len_soft) { - warnings.push(`Subject line exceeds soft limit (${subjectLen}/${CONFIG.max_subject_len_soft} chars)`); + warnings.push(`Subject line exceeds soft limit (${subjectLen}/${CONFIG.max_subject_len_soft}${lenSuffix})`); } if (subjectErrors.length === 0) { - successes.push(`✅ Commit subject layout and length are valid: "${lines[0]}"`); + if (revert) { + successes.push(`✅ Commit subject layout and length are valid (revert of "${revert.original}")`); + } else { + successes.push(`✅ Commit subject layout and length are valid: "${lines[0]}"`); + } } else { subjectErrors.forEach(err => errors.push("- " + err)); } @@ -496,7 +537,14 @@ export function validateMakefileContext(fullCommit, commitPatch, CONFIG, state) successes.push(`✅ PKG_VERSION is dynamically defined: '${newVersion}', skipping subject validation`); } else { const cleanSubject = subject.replace(/^(fixup!|squash!)\s+/, ''); - if (!matchVersionString(cleanSubject, newVersion)) { + const isRevert = CONFIG.allow_revert !== false && parseRevertSubject(cleanSubject) !== null; + if (isRevert) { + // A revert restores the PKG_VERSION that preceded the reverted commit, + // while its subject quotes that commit (and therefore the version being + // undone). Requiring the restored version here would force the author + // away from the `git revert` subject format. + successes.push(`✅ Commit reverts a previous change, skipping subject validation for restored PKG_VERSION '${newVersion}'`); + } else if (!matchVersionString(cleanSubject, newVersion)) { errors.push(`- Makefile introduces PKG_VERSION '${newVersion}', but this version string is missing in the commit subject line. Please mention the new version in the subject, e.g. ': update to ${newVersion}'.`); } else { successes.push(`✅ PKG_VERSION bump matches context information inside subject line (${newVersion})`); diff --git a/cloudflare-worker/test/validators.test.js b/cloudflare-worker/test/validators.test.js index 2b901af..4cd0e49 100644 --- a/cloudflare-worker/test/validators.test.js +++ b/cloudflare-worker/test/validators.test.js @@ -1,6 +1,6 @@ import { describe, test } from 'node:test'; import assert from 'node:assert'; -import { isValidName, validateFormalities, validateMakefileContext, validateEmbeddedPatches, validatePkgReleaseBumps, findPkgRoot, validateUciConfigs } from '../src/validators.js'; +import { isValidName, parseRevertSubject, validateFormalities, validateMakefileContext, validateEmbeddedPatches, validatePkgReleaseBumps, findPkgRoot, validateUciConfigs } from '../src/validators.js'; // Mock Config Object const CONFIG = { @@ -510,6 +510,94 @@ describe('validateFormalities', () => { }); }); +// ─── Revert Subjects ───────────────────────────────────────────── + +const revertBody = (sha) => `\n\nThis reverts commit ${sha}.\nIt broke the build on several targets.\n\nSigned-off-by: John Doe `; + +const revertCommit = (subject) => ({ + commit: { + message: subject + revertBody('9fceb02d0ae598e95dc970b74767f19372d61af8'), + author: { name: 'John Doe', email: 'john@doe.com' }, + committer: { name: 'John Doe', email: 'john@doe.com' } + } +}); + +describe('parseRevertSubject', () => { + test('parses the plain git revert format', () => { + const res = parseRevertSubject('Revert "generic: permit support of standalone PCS for external kernel module"'); + assert.deepStrictEqual(res, { + prefix: '', + original: 'generic: permit support of standalone PCS for external kernel module', + depth: 1 + }); + }); + + test('parses a prefixed revert (e.g. sing-box: Revert "...")', () => { + const res = parseRevertSubject('sing-box: Revert "sing-box: update to 1.12.3"'); + assert.deepStrictEqual(res, { prefix: 'sing-box: ', original: 'sing-box: update to 1.12.3', depth: 1 }); + }); + + test('parses chained and tools/ style prefixes', () => { + assert.strictEqual(parseRevertSubject('tools/cmake: revert "tools/cmake: update to 4.0"').prefix, 'tools/cmake: '); + assert.strictEqual(parseRevertSubject('toolchain: binutils: Revert "toolchain: binutils: update to 2.45"').prefix, 'toolchain: binutils: '); + }); + + test('unwraps a revert of a revert', () => { + const res = parseRevertSubject('Revert "Revert "ramips: mt7620: fix patching mac address in caldata""'); + assert.strictEqual(res.depth, 2); + assert.strictEqual(res.original, 'ramips: mt7620: fix patching mac address in caldata'); + }); + + test('rejects subjects that only mention a revert', () => { + assert.strictEqual(parseRevertSubject('mypkg: revert the broken change'), null); + assert.strictEqual(parseRevertSubject('Revert the broken change'), null); + assert.strictEqual(parseRevertSubject('Reverted "mypkg: update to 1.2.3"'), null); + assert.strictEqual(parseRevertSubject('toolchain: binutils: partially revert commit 525a1e94b343 "fix update to 2.45.1"'), null); + }); +}); + +describe('validateFormalities revert subjects', () => { + test('accepts the plain git revert format without a package prefix', async () => { + const res = await validateFormalities(revertCommit('Revert "generic: permit support of standalone PCS for external kernel module"'), CONFIG); + assert.strictEqual(res.errors.length, 0, `Unexpected errors: ${res.errors.join(', ')}`); + assert.ok(res.successes.some(s => s.includes('Commit subject layout and length are valid (revert of'))); + }); + + test('accepts a revert of a revert', async () => { + const res = await validateFormalities(revertCommit('Revert "Revert "ramips: mt7620: fix patching mac address in caldata""'), CONFIG); + assert.strictEqual(res.errors.length, 0, `Unexpected errors: ${res.errors.join(', ')}`); + }); + + test('accepts an upper-case Revert after a package prefix', async () => { + const res = await validateFormalities(revertCommit('irqbalance: Revert "irqbalance: update to 1.9.5"'), CONFIG); + assert.strictEqual(res.errors.length, 0, `Unexpected errors: ${res.errors.join(', ')}`); + }); + + test('excludes the Revert wrapper from the subject length limits', async () => { + // 86 chars as written, 77 without the wrapper. + const res = await validateFormalities(revertCommit('Revert "base-files: handle name collision between kernel UBI volume and MTD partition"'), CONFIG); + assert.strictEqual(res.errors.length, 0, `Unexpected errors: ${res.errors.join(', ')}`); + }); + + test('still enforces the hard limit on the reverted subject itself', async () => { + const original = 'base-files: handle a name collision between the kernel UBI volume and the MTD partition'; + assert.ok(original.length > CONFIG.max_subject_len_hard); + const res = await validateFormalities(revertCommit(`Revert "${original}"`), CONFIG); + assert.ok(res.errors.some(e => e.includes('exceeds hard limit') && e.includes('excluding the `Revert "..."` wrapper'))); + }); + + test('still rejects a revert-like subject without the quoted original', async () => { + const res = await validateFormalities(revertCommit('Revert the broken PCS support'), CONFIG); + assert.ok(res.errors.some(e => e.includes('must start with `: `'))); + }); + + test('enforces the regular subject rules when allow_revert is disabled', async () => { + const customConfig = { ...CONFIG, allow_revert: false }; + const res = await validateFormalities(revertCommit('Revert "generic: permit support of standalone PCS for external kernel module"'), customConfig); + assert.ok(res.errors.some(e => e.includes('must start with `: `'))); + }); +}); + // ─── Makefile Context ──────────────────────────────────────────── describe('validateMakefileContext', () => { @@ -561,6 +649,31 @@ describe('validateMakefileContext', () => { assert.ok(res.errors.some(e => e.includes('PKG_VERSION'))); }); + test('skips subject validation for a revert restoring the previous PKG_VERSION', () => { + const commit = { commit: { message: 'sing-box: Revert "sing-box: update to 1.12.3"\n\nThis reverts commit 9fceb02d0ae598e95dc970b74767f19372d61af8.' } }; + const patch = ` +--- a/package/net/sing-box/Makefile ++++ b/package/net/sing-box/Makefile ++PKG_VERSION:=1.12.2 + `; + const state = { isNewPackage: false, isDroppedPackage: false }; + const res = validateMakefileContext(commit, patch, CONFIG, state); + assert.strictEqual(res.errors.length, 0, `Unexpected errors: ${res.errors.join(', ')}`); + assert.ok(res.successes.some(s => s.includes('Commit reverts a previous change'))); + }); + + test('catches version mismatch on a revert when allow_revert is disabled', () => { + const commit = { commit: { message: 'sing-box: Revert "sing-box: update to 1.12.3"' } }; + const patch = ` +--- a/package/net/sing-box/Makefile ++++ b/package/net/sing-box/Makefile ++PKG_VERSION:=1.12.2 + `; + const state = { isNewPackage: false, isDroppedPackage: false }; + const res = validateMakefileContext(commit, patch, { ...CONFIG, allow_revert: false }, state); + assert.ok(res.errors.some(e => e.includes('PKG_VERSION'))); + }); + test('skips subject validation for dynamic/templated PKG_VERSION', () => { const commit = { commit: { message: 'apk: update to 2.14.0' } }; const patch = ` From edd98beeaa24100275db3eddb6732cbb234472e4 Mon Sep 17 00:00:00 2001 From: Josef Schlehofer Date: Sun, 26 Jul 2026 00:37:45 +0200 Subject: [PATCH 2/2] cloudflare-worker: accept versions restored by a revert A revert puts PKG_VERSION and PKG_RELEASE back to the values from before the reverted commit. The release audit reads that as a version update and demands PKG_RELEASE reset to 1, which a revert must not do, so reverting a version bump fails the audit. Reverting the removal of a package hits the same problem through the new package rule. Skip both rules for packages that only revert commits touched, and say so in the audit output instead of reporting a bump to a lower release. A revert that changes package content without touching the version or release still has to bump PKG_RELEASE, otherwise users would keep the content that was reverted. Attribution needs commit messages, so PRs with more than 15 commits, where only one patch for the whole PR is fetched, keep the strict rules. Signed-off-by: Josef Schlehofer --- README.md | 6 +- cloudflare-worker/src/validators.js | 34 ++++++++ cloudflare-worker/test/validators.test.js | 97 +++++++++++++++++++++++ 3 files changed, 134 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 6f8c9d8..142daa4 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,7 @@ Focuses on Git history hygiene, developer metadata constraints, and layout stand * **Identity Integrity:** Validates author and committer name formats and strictly blocks generic GitHub `noreply.github.com` email addresses. * **Linked GitHub Account:** Verifies that the commit author email address is registered and verified on a GitHub account, linking the commit to a valid GitHub username. Can be downgraded to a non-blocking warning or disabled entirely, see `require_linked_github_account` below. * **Autosquash Compliance:** Automatically bypasses style constraints for development-phase `fixup!` and `squash!` syntax blocks. -* **Revert Compliance (`allow_revert`):** Accepts the subject line `git revert` generates — `Revert ""`, its nested `Revert "Revert """` form, and the prefixed `: Revert "..."` variant. The quoted part is copied verbatim from the reverted commit, so the prefix and lowercase rules are not applied to it, and the `Revert "..."` wrapper is excluded from the subject length limits. `check_pkg_version` is skipped for reverts too, since a revert restores the `PKG_VERSION` that preceded the commit its subject quotes. +* **Revert Compliance (`allow_revert`):** Accepts the subject line `git revert` generates — `Revert ""`, its nested `Revert "Revert """` form, and the prefixed `: Revert "..."` variant. The quoted text is copied verbatim from the reverted commit, so the prefix and lowercase rules are not applied to it, and the `Revert "..."` wrapper is excluded from the subject length limits. A revert also puts `PKG_VERSION` and `PKG_RELEASE` back to the values that preceded the reverted commit, so those restored values are accepted instead of a bump (see **PKG_RELEASE Validation** below). * **Subject String Hygiene:** Enforces `: ` prefix headers, checks lowercase starting strings post-prefix, and rejects trailing periods. * **Length Constraints:** Implements dual-layered (soft and hard) line width boundaries for both subject lines and description body text blocks. * **Signed-off-by Check:** Ensures a consistent, properly structured `Signed-off-by:` declaration is present and matches the original author metadata. @@ -43,7 +43,7 @@ Inspects file modification trees targeting OpenWrt build recipes: * **Conffiles Tracker:** Mandates the definition of the `Package/.../conffiles` tracking macro whenever configuration file installations (`INSTALL_CONF`) are triggered. * **Line Ending Sanitization:** Inspects modifications for Windows-style Carriage Returns (CRLF) to guarantee exclusive UNIX (LF) formatting compliance. * **Trailing Newline Check:** Verifies that newly created or modified files end with a trailing newline character, catching the common `\ No newline at end of file` issue in diffs (customizable level: warning/error/disabled). -* **PKG_RELEASE Validation:** Enforces correct release values on package changes: new packages must initialize `PKG_RELEASE` to `1`, version updates must reset `PKG_RELEASE` to `1`, and modifications to package files must be accompanied by a version/release change (customizable level: warning/error/disabled). +* **PKG_RELEASE Validation:** Enforces correct release values on package changes: new packages must initialize `PKG_RELEASE` to `1`, version updates must reset `PKG_RELEASE` to `1`, and modifications to package files must be accompanied by a version/release change (customizable level: warning/error/disabled). Packages that only revert commits touched are exempt from the reset/initialize rules, because a revert restores the version and release of an already released state. They must still carry a release bump if the revert changes package content without touching the version or release, otherwise users would never receive it. * **UCI Config Validation:** Ensures that any configuration files destined to be installed into `/etc/config/` conform to the standard OpenWrt UCI format (consisting of only `package`, `config`, `option`, `list` statements, comments, and empty lines). * **PKG_NAME Reuse Prevention:** Ensures `PKG_NAME` is not reused inside `call`, `define`, and `eval` Makefile lines, requiring the literal package name instead to keep recipes readable and searchable (default true). @@ -123,7 +123,7 @@ Some configuration keys offer advanced options: * `check_pkg_name_reuse`: Set to `true` (default) to detect and reject reuse of the `PKG_NAME` variable in `call`, `define`, and `eval` lines, or `false` to disable. * `show_force_push_tip`: Set to `true` (default) to append a helpful tip regarding how to correct validation errors using force-pushing. Set to `false` to disable. * `check_openwrt_spelling`: Set to `true` (default) to validate the correct capitalization of "OpenWrt" in commit subjects and descriptions. Set to `false` to disable. -* `allow_revert`: Set to `true` (default) to accept the subject format produced by `git revert` (`Revert ""`, nested reverts, and the prefixed `: Revert "..."` variant). Set to `false` to hold revert commits to the regular subject rules. +* `allow_revert`: Set to `true` (default) to accept the subject format produced by `git revert` (`Revert ""`, nested reverts, and the prefixed `: Revert "..."` variant) and the `PKG_VERSION`/`PKG_RELEASE` values a revert restores. Set to `false` to hold revert commits to the regular subject and release bump rules. * `enable_stale_bot`: Set to `true` to enable the stale PR bot cleanup for this repository. Defaults to `false` (opt-in). * `enable_labeler_yml`: Set to `true` to enable dynamic pull request labeling based on matching files in the `.github/labeler.yml` configuration file. Defaults to `false` (opt-in). * `enable_issue_labeller`: Set to `true` to enable automated issue form validation and labelling (replaces the GitHub Actions `issue-labeller.yml` workflow). Defaults to `false` (opt-in). diff --git a/cloudflare-worker/src/validators.js b/cloudflare-worker/src/validators.js index dc4ed0a..626cf95 100644 --- a/cloudflare-worker/src/validators.js +++ b/cloudflare-worker/src/validators.js @@ -1458,7 +1458,18 @@ export async function validatePkgReleaseBumps(commitDetails, CONFIG, fetchFileCo const fileChanges = {}; // filePath -> { added: [], deleted: [] } const candidateFiles = []; + // A revert puts PKG_VERSION and PKG_RELEASE back to the values that preceded + // the reverted commit, so the version moves backwards and PKG_RELEASE is + // whatever it was before instead of 1. Record which commits touched a file so + // packages changed by reverts alone can skip the bump requirements. The + // PR-wide patch fallback carries no per-commit message and therefore never + // qualifies, keeping the audit strict when the origin of a change is unknown. + const nonRevertedFiles = new Set(); + for (const item of commitDetails) { + const isRevertCommit = CONFIG.allow_revert !== false && + parseRevertSubject((item.fullCommit?.commit?.message || '').split('\n')[0]) !== null; + if (item.commitPatch) { const states = parseDiffFileStates(item.commitPatch); states.addedFiles.forEach(f => addedFiles.add(f)); @@ -1496,6 +1507,7 @@ export async function validatePkgReleaseBumps(commitDetails, CONFIG, fetchFileCo const files = getChangedFilesFromPatch(item.commitPatch); for (const file of files) { modifiedFiles.add(file); + if (!isRevertCommit) nonRevertedFiles.add(file); if (isHiddenOrSpecial(file)) continue; // Ignore test files that serve only within CI/CD (e.g. test.sh, test-version.sh) @@ -1541,6 +1553,11 @@ export async function validatePkgReleaseBumps(commitDetails, CONFIG, fetchFileCo return empty; } + // Only revert commits touched this package, so its PKG_VERSION and + // PKG_RELEASE are back at the values that preceded the reverted commit. + const pkgFiles = [...modifiedFiles].filter(file => file === pkgRoot || file.startsWith(pkgRoot + '/')); + const isRevertOnly = pkgFiles.length > 0 && pkgFiles.every(file => !nonRevertedFiles.has(file)); + // OPTIMIZATION: If the Makefile itself was not modified in the PR, // then the version and release cannot have changed (bumped = false). // We can skip fetching the Makefile contents entirely! @@ -1570,6 +1587,11 @@ export async function validatePkgReleaseBumps(commitDetails, CONFIG, fetchFileCo headRelease = resolveMakefileVar(headContent, 'PKG_RELEASE'); if (isNew) { + if (isRevertOnly) { + // Reverting the removal of a package restores it with the PKG_RELEASE + // it was dropped with; it is not a new package starting from scratch. + return { errors: [], successes: [`✅ Package \`${pkgRoot}\` is restored by a revert with its previous PKG_RELEASE ('${headRelease || 'not defined'}')`] }; + } if (headRelease !== '1') { return { errors: [`New package \`${pkgRoot}\` must start with PKG_RELEASE set to 1 (currently: '${headRelease || 'not defined'}')`], successes: [] }; } @@ -1677,6 +1699,18 @@ export async function validatePkgReleaseBumps(commitDetails, CONFIG, fetchFileCo }; } + // A revert takes the package back to a state that was already released, so + // its version legitimately moves backwards and PKG_RELEASE keeps the value + // it had before the reverted commit. Requiring a reset to 1 here would ask + // for a version bump that a revert must not make. + if (isRevertOnly) { + const restoredVersion = headVersion || headSourceVer || headSourceDate; + const restoredState = restoredVersion + ? `PKG_VERSION '${restoredVersion}' with PKG_RELEASE '${headRelease || 'not defined'}'` + : `PKG_RELEASE '${headRelease || 'not defined'}'`; + return { errors: [], successes: [`✅ Package \`${pkgRoot}\` is reverted to ${restoredState}, matching its state before the reverted commit`] }; + } + if (versionChanged) { if (headRelease !== '1') { return { errors: [`Package \`${pkgRoot}\` version updated from '${baseVersion || baseSourceVer || baseSourceDate}' to '${headVersion || headSourceVer || headSourceDate}', but PKG_RELEASE was not reset to 1 (currently: '${headRelease || 'not defined'}')`], successes: [] }; diff --git a/cloudflare-worker/test/validators.test.js b/cloudflare-worker/test/validators.test.js index 4cd0e49..8fd3773 100644 --- a/cloudflare-worker/test/validators.test.js +++ b/cloudflare-worker/test/validators.test.js @@ -2689,6 +2689,103 @@ diff --git a/utils/prometheus-node-exporter-ucode/Makefile b/utils/prometheus-no const res = await validatePkgReleaseBumps(commitDetails, defaultConf, headFetch, baseFetch); assert.ok(res.errors.some(e => e.includes('content changed without a PKG_RELEASE or version bump'))); }); + + // ─── Reverts ─────────────────────────────────────────────────── + + const commitWith = (subject) => ({ commit: { message: `${subject}\n\nThis reverts commit 9fceb02d0ae598e95dc970b74767f19372d61af8.` } }); + + // Reverting `mypkg: update to 1.2.3` restores both the older version and the + // PKG_RELEASE that preceded the bump. + const versionRevertPatch = ` +diff --git a/package/utils/mypkg/Makefile b/package/utils/mypkg/Makefile +--- a/package/utils/mypkg/Makefile ++++ b/package/utils/mypkg/Makefile +-PKG_VERSION:=1.2.3 ++PKG_VERSION:=1.2.2 +-PKG_RELEASE:=1 ++PKG_RELEASE:=3 +`; + const versionRevertHead = async (path) => + path === 'package/utils/mypkg/Makefile' ? 'PKG_NAME:=mypkg\nPKG_VERSION:=1.2.2\nPKG_RELEASE:=3\n' : null; + const versionRevertBase = async (path) => + path === 'package/utils/mypkg/Makefile' ? 'PKG_NAME:=mypkg\nPKG_VERSION:=1.2.3\nPKG_RELEASE:=1\n' : null; + + test('accepts a revert restoring an older version and its previous PKG_RELEASE', async () => { + const commitDetails = [{ fullCommit: commitWith('mypkg: Revert "mypkg: update to 1.2.3"'), commitPatch: versionRevertPatch }]; + const res = await validatePkgReleaseBumps(commitDetails, defaultConf, versionRevertHead, versionRevertBase); + assert.strictEqual(res.errors.length, 0, `Unexpected errors: ${res.errors.join(', ')}`); + assert.ok(res.successes.some(s => s.includes('matching its state before the reverted commit'))); + }); + + test('still demands a PKG_RELEASE reset for the same downgrade in a regular commit', async () => { + const commitDetails = [{ fullCommit: commitWith('mypkg: downgrade to 1.2.2'), commitPatch: versionRevertPatch }]; + const res = await validatePkgReleaseBumps(commitDetails, defaultConf, versionRevertHead, versionRevertBase); + assert.ok(res.errors.some(e => e.includes('PKG_RELEASE was not reset to 1'))); + }); + + test('keeps the audit strict when no commit message is available (PR-wide patch fallback)', async () => { + const commitDetails = [{ commitPatch: versionRevertPatch }]; + const res = await validatePkgReleaseBumps(commitDetails, defaultConf, versionRevertHead, versionRevertBase); + assert.ok(res.errors.some(e => e.includes('PKG_RELEASE was not reset to 1'))); + }); + + test('keeps the audit strict when allow_revert is disabled', async () => { + const commitDetails = [{ fullCommit: commitWith('mypkg: Revert "mypkg: update to 1.2.3"'), commitPatch: versionRevertPatch }]; + const res = await validatePkgReleaseBumps(commitDetails, { ...defaultConf, allow_revert: false }, versionRevertHead, versionRevertBase); + assert.ok(res.errors.some(e => e.includes('PKG_RELEASE was not reset to 1'))); + }); + + test('keeps the audit strict when a regular commit touches the same package', async () => { + const commitDetails = [ + { fullCommit: commitWith('mypkg: Revert "mypkg: update to 1.2.3"'), commitPatch: versionRevertPatch }, + { + fullCommit: { commit: { message: 'mypkg: refresh patches' } }, + commitPatch: ` +diff --git a/package/utils/mypkg/patches/001-fix.patch b/package/utils/mypkg/patches/001-fix.patch +--- a/package/utils/mypkg/patches/001-fix.patch ++++ b/package/utils/mypkg/patches/001-fix.patch ++context +` + } + ]; + const res = await validatePkgReleaseBumps(commitDetails, defaultConf, versionRevertHead, versionRevertBase); + assert.ok(res.errors.some(e => e.includes('PKG_RELEASE was not reset to 1'))); + }); + + test('accepts a revert restoring a dropped package with its previous PKG_RELEASE', async () => { + const commitDetails = [{ + fullCommit: commitWith('Revert "oldpkg: remove abandoned package"'), + commitPatch: ` +diff --git a/package/utils/oldpkg/Makefile b/package/utils/oldpkg/Makefile +new file mode 100644 +--- /dev/null ++++ b/package/utils/oldpkg/Makefile +` + }]; + const headFetch = async (path) => + path === 'package/utils/oldpkg/Makefile' ? 'PKG_NAME:=oldpkg\nPKG_VERSION:=2.0\nPKG_RELEASE:=5\n' : null; + + const res = await validatePkgReleaseBumps(commitDetails, defaultConf, headFetch, async () => null); + assert.strictEqual(res.errors.length, 0, `Unexpected errors: ${res.errors.join(', ')}`); + assert.ok(res.successes.some(s => s.includes('restored by a revert with its previous PKG_RELEASE'))); + }); + + test('still requires a bump when a revert changes content without touching version or release', async () => { + const commitDetails = [{ + fullCommit: commitWith('Revert "mypkg: tweak init script"'), + commitPatch: ` +diff --git a/package/utils/mypkg/files/mypkg.init b/package/utils/mypkg/files/mypkg.init +--- a/package/utils/mypkg/files/mypkg.init ++++ b/package/utils/mypkg/files/mypkg.init ++start_service() { +` + }]; + const headFetch = async (path) => + path === 'package/utils/mypkg/Makefile' ? 'PKG_NAME:=mypkg\nPKG_VERSION:=1.2.2\nPKG_RELEASE:=3\n' : null; + + const res = await validatePkgReleaseBumps(commitDetails, defaultConf, headFetch, headFetch); + assert.ok(res.errors.some(e => e.includes('content changed without a PKG_RELEASE or version bump'))); + }); }); describe('findPkgRoot', () => {