From 18614c9fc8042adf53141d25456e4378aa684426 Mon Sep 17 00:00:00 2001 From: Yannick JOST Date: Tue, 5 May 2026 11:21:15 +0200 Subject: [PATCH 1/3] feat(isms-change-management): add ISMS change management compliance action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements a new GitHub Action that validates changes to ISMS documents against the ISMS Change Management Policy (ISMS_CMPOL). Checks performed on each changed ISMS markdown file: 1. Author ≠ Validator: no person may appear in both authors and validators 2. Version bump: version must be strictly greater than on the base branch 3. Double validation: minor or major bumps require at least 2 validators 4. RSSI role (optional): RSSI must validate, unless RSSI is the author in which case CTO or CEO must validate instead Written in Ruby (allowed scripting language at Scalingo). Includes 29 unit tests covering all validation rules and edge cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/ci.yml | 8 + .sclng/metadata.toml | 2 +- isms-change-management/README.md | 141 +++++++++ isms-change-management/action.yml | 37 +++ .../scripts/check_isms_change.rb | 269 ++++++++++++++++++ isms-change-management/tests/run.sh | 5 + .../tests/test_check_isms_change.rb | 214 ++++++++++++++ 7 files changed, 675 insertions(+), 1 deletion(-) create mode 100644 isms-change-management/README.md create mode 100644 isms-change-management/action.yml create mode 100644 isms-change-management/scripts/check_isms_change.rb create mode 100755 isms-change-management/tests/run.sh create mode 100644 isms-change-management/tests/test_check_isms_change.rb diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 65f0403..496b02e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -82,3 +82,11 @@ jobs: uses: ./go-linter with: working-directory: test/go-linter + + isms-change-management: + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v6 + + - name: Run ISMS change management action tests + run: bash isms-change-management/tests/run.sh diff --git a/.sclng/metadata.toml b/.sclng/metadata.toml index 54bbe77..c4050a9 100644 --- a/.sclng/metadata.toml +++ b/.sclng/metadata.toml @@ -1,7 +1,7 @@ dependencies = [] description = "Repository of GitHub Actions of the organisation" flags = ["tools"] -languages = ["GitHub Action", "shell", "YAML"] +languages = ["GitHub Action", "Ruby", "shell", "YAML"] owner = "etienne@scalingo.com" team = "IST" version = "1.1.2" diff --git a/isms-change-management/README.md b/isms-change-management/README.md new file mode 100644 index 0000000..3e808ff --- /dev/null +++ b/isms-change-management/README.md @@ -0,0 +1,141 @@ +# ISMS Change Management Compliance Action + +GitHub Action that validates changes to ISMS documents against the [ISMS Change Management Policy](https://github.com/Scalingo/specifications/blob/main/isms/Change-Management-Policy/ISMS-Change-Management-Policy-Fr.md). + +It checks that: + +1. **Author ≠ Validator** — no one can validate their own change +2. **Version is bumped** — the document version must be strictly increased +3. **Double validation** — minor or major version bumps require at least 2 validators +4. **RSSI role** *(optional)* — the RSSI must be a validator, with a specific exception when the RSSI is the author + +## Usage + +```yaml +jobs: + isms-compliance: + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Check ISMS change management compliance + uses: Scalingo/actions/isms-change-management@main + with: + base-ref: main + rssi: "Yannick Jost" + cto: "Léo Unbekandt" + ceo: "Frédéric Harper" +``` + +## Inputs + +| Input | Required | Default | Description | +|-----------------|----------|----------------|-------------| +| `base-ref` | no | merge-base with `origin/main` | Base branch ref to compare against (e.g. `main`) | +| `files-pattern` | no | `isms/**/*.md` | Glob pattern selecting which markdown files are ISMS documents | +| `rssi` | no | `""` | Comma-separated full name(s) of the RSSI holder(s). When set, enables the RSSI role check. | +| `cto` | no | `""` | Comma-separated full name(s) of the CTO(s). Used when `rssi` is set. | +| `ceo` | no | `""` | Comma-separated full name(s) of the CEO(s). Used when `rssi` is set. | + +> `fetch-depth: 0` is required in the `actions/checkout` step so that git history is available for the base-ref comparison. + +## Validation Checks + +The action reads the YAML front matter of each changed ISMS document. The expected front matter shape is: + +```yaml +--- +version: 1.2.0 +authors: + - Alice Martin +validators: + - Bob Dupont +--- +``` + +### 1. Author ≠ Validator + +The person who authors a change cannot also validate it. This enforces the separation of duties required by the ISMS Change Management Policy. + +**Triggers an error when** any name appears in both the `authors` list and the `validators` list of the same document. + +```yaml +# ❌ Error: Alice authored and validated the same change +authors: + - Alice Martin +validators: + - Alice Martin + - Bob Dupont +``` + +### 2. Version Bump Required + +Every approved ISMS change must produce a new version of the document. The version uses [semver](https://semver.org/) (`MAJOR.MINOR.PATCH`). + +**Triggers an error when** the `version` field in the changed document is equal to or lower than the version on the base branch. + +```yaml +# Base branch: version: 1.1.0 +# ❌ Error: version unchanged +version: 1.1.0 + +# ✅ OK: version increased +version: 1.1.1 +``` + +New documents (not present on the base branch) are exempt from this check. + +### 3. Double Validation for Minor and Major Bumps + +The policy requires two validators for changes that materially affect the content of a document: + +- **Minor bump** (`x.Y.z` increments): an article or step was added or removed +- **Major bump** (`X.y.z` increments): more than 50% of the content was rewritten + +A **patch bump** (`x.y.Z` increments, cosmetic corrections) only requires 1 validator. + +**Triggers an error when** a minor or major bump has fewer than 2 validators. + +```yaml +# Base: version: 1.0.0 → new: 1.1.0 (minor bump) +# ❌ Error: only 1 validator for a minor bump +validators: + - Bob Dupont + +# ✅ OK: 2 validators +validators: + - Bob Dupont + - Carol Lefèvre +``` + +### 4. RSSI Role Check *(enabled when `rssi` input is set)* + +All ISMS documents must be validated by the RSSI. However, the RSSI cannot validate their own changes. When the RSSI is the author, the CTO or CEO must validate instead. + +**Triggers an error when:** +- The RSSI is **not** the author **and** is **not** among the validators, or +- The RSSI **is** the author and neither the CTO nor the CEO is among the validators. + +```yaml +# RSSI = "Yannick Jost", CTO = "Léo Unbekandt" + +# ✅ Normal case: RSSI validates +authors: + - Alice Martin +validators: + - Yannick Jost + +# ✅ Exception: RSSI is author, CTO validates +authors: + - Yannick Jost +validators: + - Léo Unbekandt + +# ❌ Error: RSSI is not author and not validator +authors: + - Alice Martin +validators: + - Bob Dupont +``` diff --git a/isms-change-management/action.yml b/isms-change-management/action.yml new file mode 100644 index 0000000..016c1cc --- /dev/null +++ b/isms-change-management/action.yml @@ -0,0 +1,37 @@ +name: "ISMS Change Management Compliance" +description: "Validate ISMS document changes: author/validator coherence, version bump, and role-based validation rules" + +inputs: + base-ref: + description: "Base branch ref to compare against (e.g. 'main'). Defaults to the merge-base of HEAD and origin/main." + required: false + default: "" + files-pattern: + description: "Glob pattern for ISMS markdown documents to check" + required: false + default: "isms/**/*.md" + rssi: + description: "Comma-separated full name(s) of the RSSI holder(s). When set, enables the RSSI role check." + required: false + default: "" + cto: + description: "Comma-separated full name(s) of the CTO(s). Used when rssi is set." + required: false + default: "" + ceo: + description: "Comma-separated full name(s) of the CEO(s). Used when rssi is set." + required: false + default: "" + +runs: + using: "composite" + steps: + - name: Check ISMS change management compliance + shell: bash + env: + BASE_REF: ${{ inputs.base-ref }} + FILES_PATTERN: ${{ inputs.files-pattern }} + RSSI: ${{ inputs.rssi }} + CTO: ${{ inputs.cto }} + CEO: ${{ inputs.ceo }} + run: ruby "${GITHUB_ACTION_PATH}/scripts/check_isms_change.rb" diff --git a/isms-change-management/scripts/check_isms_change.rb b/isms-change-management/scripts/check_isms_change.rb new file mode 100644 index 0000000..144e2fb --- /dev/null +++ b/isms-change-management/scripts/check_isms_change.rb @@ -0,0 +1,269 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# Validates ISMS document changes against the ISMS Change Management Policy. +# +# Checks performed for each changed ISMS markdown file: +# 1. Author ≠ Validator — no person may appear in both authors and validators +# 2. Version bump — version must be strictly greater than on the base branch +# 3. Double validation — minor or major bumps require at least 2 validators +# 4. RSSI role — RSSI must be a validator (unless RSSI is the author, +# in which case CTO or CEO must validate) +# +# Environment variables: +# BASE_REF — base branch ref (optional; falls back to merge-base with origin/main) +# FILES_PATTERN — glob pattern for ISMS documents (default: isms/**/*.md) +# RSSI — comma-separated name(s) of the RSSI holder(s) +# CTO — comma-separated name(s) of the CTO(s) +# CEO — comma-separated name(s) of the CEO(s) + +require "yaml" + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def split_names(env_value) + (env_value || "").split(",").map(&:strip).reject(&:empty?) +end + +# Parse the YAML front matter block from a markdown string. +# Returns a Hash, or nil when no front matter is present. +def parse_front_matter(content) + return nil unless content.start_with?("---") + + end_index = content.index("\n---", 3) + return nil unless end_index + + yaml_block = content[3...end_index] + YAML.safe_load(yaml_block, permitted_classes: []) +rescue Psych::Exception + nil +end + +# Compare two semver strings. +# Returns -1, 0, or 1 (similar to <=>). +# Pre-release labels (e.g. "1.0.0-beta1") sort before the release version. +def semver_compare(ver_a, ver_b) + parse = lambda do |v| + match = v.to_s.match(/\A(\d+)\.(\d+)\.(\d+)(?:-(.+))?\z/) + return [0, 0, 0, ""] unless match + + [match[1].to_i, match[2].to_i, match[3].to_i, match[4] || ""] + end + + a = parse.call(ver_a) + b = parse.call(ver_b) + + # Compare numeric parts first + (0..2).each do |i| + return a[i] <=> b[i] unless a[i] == b[i] + end + + # Pre-release label: no label (release) > any label (pre-release) + return 0 if a[3] == b[3] + return 1 if a[3].empty? # a is release, b is pre-release → a > b + return -1 if b[3].empty? # b is release, a is pre-release → a < b + + a[3] <=> b[3] +end + +# Determine the kind of version bump: :major, :minor, :patch, or :none. +def bump_kind(old_ver, new_ver) + parse = lambda do |v| + m = v.to_s.match(/\A(\d+)\.(\d+)\.(\d+)/) + m ? [m[1].to_i, m[2].to_i, m[3].to_i] : [0, 0, 0] + end + + old = parse.call(old_ver) + new_v = parse.call(new_ver) + + return :major if new_v[0] > old[0] + return :minor if new_v[1] > old[1] + return :patch if new_v[2] > old[2] + + :none +end + +# Emit a GitHub Actions error annotation. +def error(file, message) + puts "::error file=#{file}::#{message}" +end + +# --------------------------------------------------------------------------- +# Validation checks +# --------------------------------------------------------------------------- + +# Check 1: authors and validators must be disjoint. +def check_author_validator_coherence(file, front_matter) + authors = Array(front_matter["authors"]).map(&:to_s) + validators = Array(front_matter["validators"]).map(&:to_s) + + overlap = authors & validators + return [] if overlap.empty? + + overlap.map do |name| + "#{name} appears in both authors and validators. The author of a change cannot also validate it." + end +end + +# Check 2: version must be strictly greater than the base version. +def check_version_bump(file, old_front_matter, new_front_matter) + old_version = old_front_matter["version"].to_s + new_version = new_front_matter["version"].to_s + + return [] if old_version.empty? + + if semver_compare(new_version, old_version) <= 0 + return ["Version #{new_version} is not greater than the base version #{old_version}. " \ + "Every approved change must produce a new, higher version number."] + end + + [] +end + +# Check 3: minor or major bumps require at least 2 validators. +def check_double_validation(file, old_front_matter, new_front_matter) + old_version = old_front_matter["version"].to_s + new_version = new_front_matter["version"].to_s + validators = Array(new_front_matter["validators"]).map(&:to_s).reject(&:empty?) + + kind = bump_kind(old_version, new_version) + return [] unless %i[minor major].include?(kind) + return [] if validators.size >= 2 + + ["A #{kind} version bump (#{old_version} → #{new_version}) requires at least 2 validators, " \ + "but only #{validators.size} found: #{validators.join(", ").then { |s| s.empty? ? "(none)" : s }}"] +end + +# Check 4: RSSI must validate, unless RSSI is the author (then CTO or CEO must validate). +# Only runs when rssi_names is non-empty. +def check_rssi_role(file, front_matter, rssi_names, cto_names, ceo_names) + return [] if rssi_names.empty? + + authors = Array(front_matter["authors"]).map(&:to_s) + validators = Array(front_matter["validators"]).map(&:to_s) + + rssi_is_author = (rssi_names & authors).any? + rssi_is_validator = (rssi_names & validators).any? + + if rssi_is_author + # RSSI authored the change → CTO or CEO must validate instead + fallback_present = ((cto_names + ceo_names) & validators).any? + unless fallback_present + return ["The RSSI is the author of this change. In this case, the CTO or CEO must be a validator, " \ + "but neither was found among validators: #{validators.join(", ").then { |s| s.empty? ? "(none)" : s }}"] + end + else + # Normal case: RSSI must be a validator + unless rssi_is_validator + return ["All ISMS documents must be validated by the RSSI (#{rssi_names.join(", ")}), " \ + "but the RSSI was not found among validators: #{validators.join(", ").then { |s| s.empty? ? "(none)" : s }}"] + end + end + + [] +end + +# --------------------------------------------------------------------------- +# File discovery +# --------------------------------------------------------------------------- + +def resolve_base_ref + base_ref = ENV.fetch("BASE_REF", "").strip + return base_ref unless base_ref.empty? + + # Fall back to the merge-base with origin/main + result = `git merge-base HEAD origin/main 2>/dev/null`.strip + result.empty? ? "origin/main" : result +end + +def changed_files(base_ref) + `git diff --name-only "#{base_ref}" HEAD 2>/dev/null`.split("\n").map(&:strip).reject(&:empty?) +end + +def file_exists_in_base?(file, base_ref) + system("git cat-file -e \"#{base_ref}:#{file}\" 2>/dev/null") +end + +def read_base_content(file, base_ref) + `git show "#{base_ref}:#{file}" 2>/dev/null` +end + +def matches_pattern?(file, pattern) + File.fnmatch(pattern, file, File::FNM_PATHNAME) +end + +# --------------------------------------------------------------------------- +# Main +# --------------------------------------------------------------------------- + +def main + base_ref = resolve_base_ref + files_pattern = ENV.fetch("FILES_PATTERN", "isms/**/*.md").strip + rssi_names = split_names(ENV["RSSI"]) + cto_names = split_names(ENV["CTO"]) + ceo_names = split_names(ENV["CEO"]) + + all_changed = changed_files(base_ref) + isms_files = all_changed.select { |f| f.end_with?(".md") && matches_pattern?(f, files_pattern) } + + if isms_files.empty? + puts "No ISMS documents changed — nothing to check." + return 0 + end + + puts "Checking #{isms_files.size} changed ISMS document(s) against base #{base_ref}..." + + all_errors = [] + + isms_files.each do |file| + new_content = File.read(file, encoding: "utf-8") rescue nil + unless new_content + error(file, "Cannot read file #{file}") + all_errors << file + next + end + + new_fm = parse_front_matter(new_content) + unless new_fm + # Not an ISMS document with front matter — skip silently + next + end + + file_errors = [] + + # Check 1 always applies + file_errors += check_author_validator_coherence(file, new_fm) + + if file_exists_in_base?(file, base_ref) + base_content = read_base_content(file, base_ref) + old_fm = parse_front_matter(base_content) + + if old_fm + # Check 2: version bump + file_errors += check_version_bump(file, old_fm, new_fm) + + # Check 3: double validation for minor/major + file_errors += check_double_validation(file, old_fm, new_fm) + end + else + puts "::notice file=#{file}::New document — version and double-validation checks skipped." + end + + # Check 4: RSSI role (optional) + file_errors += check_rssi_role(file, new_fm, rssi_names, cto_names, ceo_names) + + file_errors.each { |msg| error(file, msg) } + all_errors.concat(file_errors) + end + + if all_errors.empty? + puts "All ISMS change management checks passed." + 0 + else + 1 + end +end + +exit(main) if __FILE__ == $PROGRAM_NAME diff --git a/isms-change-management/tests/run.sh b/isms-change-management/tests/run.sh new file mode 100755 index 0000000..ba906c6 --- /dev/null +++ b/isms-change-management/tests/run.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash +set -euo pipefail + +cd "$(dirname "$0")/../.." +ruby -Iisms-change-management isms-change-management/tests/test_check_isms_change.rb diff --git a/isms-change-management/tests/test_check_isms_change.rb b/isms-change-management/tests/test_check_isms_change.rb new file mode 100644 index 0000000..27c59d6 --- /dev/null +++ b/isms-change-management/tests/test_check_isms_change.rb @@ -0,0 +1,214 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +require "minitest/autorun" +require "tmpdir" +require "fileutils" + +SCRIPT_PATH = File.expand_path("../scripts/check_isms_change.rb", __dir__) +load SCRIPT_PATH + +# --------------------------------------------------------------------------- +# Helper to build a minimal markdown document with YAML front matter +# --------------------------------------------------------------------------- +def make_doc(version:, authors:, validators:) + authors_yaml = authors.map { |a| " - #{a}" }.join("\n") + validators_yaml = validators.map { |v| " - #{v}" }.join("\n") + + <<~MD + --- + title: Test Document + version: #{version} + authors: + #{authors_yaml} + validators: + #{validators_yaml.empty? ? " []" : validators_yaml} + --- + + # Test Document + MD +end + +# --------------------------------------------------------------------------- +# Unit tests for individual check functions +# --------------------------------------------------------------------------- + +class TestAuthorValidatorCoherence < Minitest::Test + def test_no_overlap_passes + fm = { "authors" => ["Alice"], "validators" => ["Bob"] } + assert_empty check_author_validator_coherence("file.md", fm) + end + + def test_overlap_fails + fm = { "authors" => ["Alice", "Bob"], "validators" => ["Bob", "Carol"] } + errors = check_author_validator_coherence("file.md", fm) + refute_empty errors + assert_match(/Bob/, errors.first) + end + + def test_multiple_overlaps_reported_individually + fm = { "authors" => ["Alice", "Bob"], "validators" => ["Alice", "Bob"] } + errors = check_author_validator_coherence("file.md", fm) + assert_equal 2, errors.size + end + + def test_empty_validators_passes + fm = { "authors" => ["Alice"], "validators" => [] } + assert_empty check_author_validator_coherence("file.md", fm) + end +end + +class TestVersionBump < Minitest::Test + def test_bumped_version_passes + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "1.0.1" } + assert_empty check_version_bump("file.md", old_fm, new_fm) + end + + def test_same_version_fails + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "1.0.0" } + errors = check_version_bump("file.md", old_fm, new_fm) + refute_empty errors + assert_match(/not greater/, errors.first) + end + + def test_decremented_version_fails + old_fm = { "version" => "1.2.0" } + new_fm = { "version" => "1.1.0" } + errors = check_version_bump("file.md", old_fm, new_fm) + refute_empty errors + end + + def test_major_bump_passes + old_fm = { "version" => "1.9.9" } + new_fm = { "version" => "2.0.0" } + assert_empty check_version_bump("file.md", old_fm, new_fm) + end + + def test_no_old_version_skips_check + old_fm = { "version" => "" } + new_fm = { "version" => "1.0.0" } + assert_empty check_version_bump("file.md", old_fm, new_fm) + end +end + +class TestDoubleValidation < Minitest::Test + def test_patch_bump_one_validator_passes + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "1.0.1", "validators" => ["Alice"] } + assert_empty check_double_validation("file.md", old_fm, new_fm) + end + + def test_minor_bump_one_validator_fails + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "1.1.0", "validators" => ["Alice"] } + errors = check_double_validation("file.md", old_fm, new_fm) + refute_empty errors + assert_match(/minor/, errors.first) + assert_match(/2 validators/, errors.first) + end + + def test_minor_bump_two_validators_passes + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "1.1.0", "validators" => ["Alice", "Bob"] } + assert_empty check_double_validation("file.md", old_fm, new_fm) + end + + def test_major_bump_one_validator_fails + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "2.0.0", "validators" => ["Alice"] } + errors = check_double_validation("file.md", old_fm, new_fm) + refute_empty errors + assert_match(/major/, errors.first) + end + + def test_major_bump_two_validators_passes + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "2.0.0", "validators" => ["Alice", "Bob"] } + assert_empty check_double_validation("file.md", old_fm, new_fm) + end +end + +class TestRssiRole < Minitest::Test + RSSI = ["Yannick Jost"].freeze + CTO = ["Léo Unbekandt"].freeze + CEO = ["Jean Dupont"].freeze + + def test_rssi_is_validator_passes + fm = { "authors" => ["Alice"], "validators" => ["Yannick Jost"] } + assert_empty check_rssi_role("file.md", fm, RSSI, CTO, CEO) + end + + def test_rssi_not_validator_and_not_author_fails + fm = { "authors" => ["Alice"], "validators" => ["Bob"] } + errors = check_rssi_role("file.md", fm, RSSI, CTO, CEO) + refute_empty errors + assert_match(/RSSI/, errors.first) + end + + def test_rssi_is_author_cto_validates_passes + fm = { "authors" => ["Yannick Jost"], "validators" => ["Léo Unbekandt"] } + assert_empty check_rssi_role("file.md", fm, RSSI, CTO, CEO) + end + + def test_rssi_is_author_ceo_validates_passes + fm = { "authors" => ["Yannick Jost"], "validators" => ["Jean Dupont"] } + assert_empty check_rssi_role("file.md", fm, RSSI, CTO, CEO) + end + + def test_rssi_is_author_no_cto_or_ceo_fails + fm = { "authors" => ["Yannick Jost"], "validators" => ["Alice"] } + errors = check_rssi_role("file.md", fm, RSSI, CTO, CEO) + refute_empty errors + assert_match(/CTO or CEO/, errors.first) + end + + def test_no_rssi_configured_skips_check + fm = { "authors" => ["Alice"], "validators" => ["Bob"] } + assert_empty check_rssi_role("file.md", fm, [], CTO, CEO) + end +end + +class TestSemverCompare < Minitest::Test + def test_patch_greater + assert_equal 1, semver_compare("1.0.1", "1.0.0") + end + + def test_minor_greater + assert_equal 1, semver_compare("1.1.0", "1.0.9") + end + + def test_major_greater + assert_equal 1, semver_compare("2.0.0", "1.9.9") + end + + def test_equal + assert_equal 0, semver_compare("1.2.3", "1.2.3") + end + + def test_lesser + assert_equal(-1, semver_compare("1.0.0", "1.0.1")) + end + + def test_prerelease_less_than_release + assert_equal(-1, semver_compare("1.0.0-beta1", "1.0.0")) + end +end + +class TestParseFrontMatter < Minitest::Test + def test_valid_front_matter + content = "---\ntitle: Test\nversion: 1.0.0\n---\n# Body" + fm = parse_front_matter(content) + assert_equal "Test", fm["title"] + assert_equal "1.0.0", fm["version"] + end + + def test_no_front_matter_returns_nil + assert_nil parse_front_matter("# Just a heading\nNo front matter.") + end + + def test_unclosed_front_matter_returns_nil + assert_nil parse_front_matter("---\ntitle: Test\n") + end +end From eb1501b5c7ea0b4d90f5801a343c2756fa57d277 Mon Sep 17 00:00:00 2001 From: Yannick JOST Date: Tue, 5 May 2026 11:53:55 +0200 Subject: [PATCH 2/3] feat(isms-change-management): check commit authors and PR reviewer coherence Add two new checks to the ISMS change management action: Check 5 - Commit authors coherence (no API required): Every git commit author touching an ISMS document must be listed in the document's authors field. Prevents undeclared authorship. Check 6 - PR reviewer coherence (optional, requires github-token + pr-number): YAML validators must exactly match the set of actual GitHub PR approvers (bidirectional). Prevents phantom validators and undocumented approvers. Also adds normalize_name/name_in_list? helpers for case-insensitive name matching across all checks. --- isms-change-management/README.md | 52 +++++++ isms-change-management/action.yml | 10 ++ .../scripts/check_isms_change.rb | 144 +++++++++++++++++- .../tests/test_check_isms_change.rb | 86 +++++++++++ 4 files changed, 285 insertions(+), 7 deletions(-) diff --git a/isms-change-management/README.md b/isms-change-management/README.md index 3e808ff..90e1cc9 100644 --- a/isms-change-management/README.md +++ b/isms-change-management/README.md @@ -8,6 +8,8 @@ It checks that: 2. **Version is bumped** — the document version must be strictly increased 3. **Double validation** — minor or major version bumps require at least 2 validators 4. **RSSI role** *(optional)* — the RSSI must be a validator, with a specific exception when the RSSI is the author +5. **Commit authors** — every git commit author touching the file must be declared in the document's `authors` field +6. **PR reviewer coherence** *(optional)* — the `validators` field must exactly match the set of GitHub PR approvers ## Usage @@ -27,6 +29,8 @@ jobs: rssi: "Yannick Jost" cto: "Léo Unbekandt" ceo: "Frédéric Harper" + github-token: ${{ secrets.GITHUB_TOKEN }} + pr-number: ${{ github.event.pull_request.number }} ``` ## Inputs @@ -38,6 +42,8 @@ jobs: | `rssi` | no | `""` | Comma-separated full name(s) of the RSSI holder(s). When set, enables the RSSI role check. | | `cto` | no | `""` | Comma-separated full name(s) of the CTO(s). Used when `rssi` is set. | | `ceo` | no | `""` | Comma-separated full name(s) of the CEO(s). Used when `rssi` is set. | +| `github-token` | no | `""` | GitHub token. When set together with `pr-number`, enables the PR reviewer coherence check. | +| `pr-number` | no | `""` | Pull request number. Required when `github-token` is provided. | > `fetch-depth: 0` is required in the `actions/checkout` step so that git history is available for the base-ref comparison. @@ -139,3 +145,49 @@ authors: validators: - Bob Dupont ``` + +### 5. Commit Authors Coherence + +Every person who authors a git commit touching an ISMS document must be listed in that document's `authors` field. This ensures the declared authorship in the front matter reflects reality — you cannot silently modify a document without declaring yourself as an author. + +**Triggers an error when** a commit author name (from `git log`) is not found in the document's `authors` list. Name matching is case-insensitive. + +```yaml +# git log shows: Alice Martin committed on this file +# ❌ Error: Alice is not in authors +authors: + - Bob Dupont +validators: + - Yannick Jost + +# ✅ OK +authors: + - Alice Martin +validators: + - Yannick Jost +``` + +### 6. PR Reviewer Coherence *(enabled when `github-token` + `pr-number` are set)* + +The `validators` field must match the set of GitHub users who have actually approved the pull request. This check is **bidirectional**: + +- Every YAML `validator` must have approved the PR on GitHub — you cannot claim someone validated your change if they did not. +- Every GitHub approver must be listed as a `validator` — all actual approvals must be documented. + +Name matching is case-insensitive and uses the GitHub user's display name (falling back to their login if no display name is set). + +**Triggers an error when:** +- A name in `validators` has not approved the PR, or +- A GitHub approver is not listed in `validators`. + +```yaml +# PR approved by: Bob Dupont + +# ❌ Error: Carol listed but did not approve; Bob approved but not listed +validators: + - Carol Lefèvre + +# ✅ OK +validators: + - Bob Dupont +``` diff --git a/isms-change-management/action.yml b/isms-change-management/action.yml index 016c1cc..2f4a5da 100644 --- a/isms-change-management/action.yml +++ b/isms-change-management/action.yml @@ -22,6 +22,14 @@ inputs: description: "Comma-separated full name(s) of the CEO(s). Used when rssi is set." required: false default: "" + github-token: + description: "GitHub token for API access. When set together with pr-number, enables PR reviewer coherence check (Check 6)." + required: false + default: "" + pr-number: + description: "Pull request number. Required when github-token is provided." + required: false + default: "" runs: using: "composite" @@ -34,4 +42,6 @@ runs: RSSI: ${{ inputs.rssi }} CTO: ${{ inputs.cto }} CEO: ${{ inputs.ceo }} + GITHUB_TOKEN: ${{ inputs.github-token }} + PR_NUMBER: ${{ inputs.pr-number }} run: ruby "${GITHUB_ACTION_PATH}/scripts/check_isms_change.rb" diff --git a/isms-change-management/scripts/check_isms_change.rb b/isms-change-management/scripts/check_isms_change.rb index 144e2fb..1727e02 100644 --- a/isms-change-management/scripts/check_isms_change.rb +++ b/isms-change-management/scripts/check_isms_change.rb @@ -9,15 +9,24 @@ # 3. Double validation — minor or major bumps require at least 2 validators # 4. RSSI role — RSSI must be a validator (unless RSSI is the author, # in which case CTO or CEO must validate) +# 5. Commit authors — every git commit author touching the file must be +# listed in the document's authors field +# 6. PR reviewer coherence — YAML validators and actual GitHub PR approvers +# must match (enabled when GITHUB_TOKEN + PR_NUMBER set) # # Environment variables: -# BASE_REF — base branch ref (optional; falls back to merge-base with origin/main) -# FILES_PATTERN — glob pattern for ISMS documents (default: isms/**/*.md) -# RSSI — comma-separated name(s) of the RSSI holder(s) -# CTO — comma-separated name(s) of the CTO(s) -# CEO — comma-separated name(s) of the CEO(s) +# BASE_REF — base branch ref (optional; falls back to merge-base with origin/main) +# FILES_PATTERN — glob pattern for ISMS documents (default: isms/**/*.md) +# RSSI — comma-separated name(s) of the RSSI holder(s) +# CTO — comma-separated name(s) of the CTO(s) +# CEO — comma-separated name(s) of the CEO(s) +# GITHUB_TOKEN — GitHub token; enables PR reviewer coherence check when set +# PR_NUMBER — pull request number; required when GITHUB_TOKEN is set +# GITHUB_REPOSITORY — set automatically by GitHub Actions (owner/repo) require "yaml" +require "net/http" +require "json" # --------------------------------------------------------------------------- # Helpers @@ -90,6 +99,17 @@ def error(file, message) puts "::error file=#{file}::#{message}" end +# Normalize a name for comparison: downcase, strip, collapse internal spaces. +def normalize_name(name) + name.to_s.strip.downcase.gsub(/\s+/, " ") +end + +# Case-insensitive name lookup in a list. +def name_in_list?(name, list) + norm = normalize_name(name) + list.any? { |n| normalize_name(n) == norm } +end + # --------------------------------------------------------------------------- # Validation checks # --------------------------------------------------------------------------- @@ -165,10 +185,99 @@ def check_rssi_role(file, front_matter, rssi_names, cto_names, ceo_names) [] end +# Check 5: Every git commit author touching this file must be listed in YAML authors. +def check_commit_authors_coherence(file, front_matter, commit_authors) + return [] if commit_authors.empty? + + authors = Array(front_matter["authors"]).map(&:to_s) + unlisted = commit_authors.reject { |ca| name_in_list?(ca, authors) } + return [] if unlisted.empty? + + unlisted.map do |name| + "Commit author '#{name}' is not listed in the document's authors field. " \ + "Every person who authors commits on an ISMS document must declare themselves as an author." + end +end + +# Check 6: YAML validators and actual GitHub PR approvers must be coherent. +# - Every YAML validator must have approved the PR (no undeclared validators) +# - Every actual approver must be listed as a validator (no undocumented approvers) +# Only runs when approved_reviewer_names is non-nil (i.e. GitHub token was provided). +def check_validators_are_approvers(file, front_matter, approved_reviewer_names) + return [] if approved_reviewer_names.nil? + + validators = Array(front_matter["validators"]).map(&:to_s).reject(&:empty?) + errors = [] + + validators.each do |v| + unless name_in_list?(v, approved_reviewer_names) + errors << "'#{v}' is listed as a validator but has not approved the PR on GitHub. " \ + "Actual approvers: #{approved_reviewer_names.empty? ? "(none)" : approved_reviewer_names.join(", ")}" + end + end + + approved_reviewer_names.each do |approver| + unless name_in_list?(approver, validators) + errors << "GitHub approver '#{approver}' is not listed as a validator in the document's validators field." + end + end + + errors +end + # --------------------------------------------------------------------------- -# File discovery +# GitHub API # --------------------------------------------------------------------------- +def github_api_get(token, path) + uri = URI("https://api.github.com#{path}") + req = Net::HTTP::Get.new(uri) + req["Authorization"] = "Bearer #{token}" + req["Accept"] = "application/vnd.github+json" + req["X-GitHub-Api-Version"] = "2022-11-28" + + response = Net::HTTP.start(uri.host, uri.port, use_ssl: true) { |http| http.request(req) } + unless response.is_a?(Net::HTTPSuccess) + warn "::warning::GitHub API #{path} returned HTTP #{response.code}" + return nil + end + JSON.parse(response.body) +rescue StandardError => e + warn "::warning::GitHub API request failed for #{path}: #{e.message}" + nil +end + +# Fetch a GitHub user's display name, falling back to their login. +def fetch_github_display_name(token, login) + data = github_api_get(token, "/users/#{URI.encode_uri_component(login)}") + return login unless data + + name = data["name"].to_s.strip + name.empty? ? login : name +end + +# Fetch the names of all current approved reviewers for a pull request. +# Returns an Array of display names, or nil on API failure. +def fetch_pr_approved_reviewer_names(token, repository, pr_number) + reviews = github_api_get(token, "/repos/#{repository}/pulls/#{pr_number}/reviews") + return nil unless reviews + + # Determine the latest review state per reviewer (later reviews supersede earlier ones). + latest_state = {} + reviews.each do |review| + login = review.dig("user", "login") + next unless login + + state = review["state"] + next unless %w[APPROVED CHANGES_REQUESTED DISMISSED].include?(state) + + latest_state[login] = state + end + + approver_logins = latest_state.select { |_, s| s == "APPROVED" }.keys + approver_logins.map { |login| fetch_github_display_name(token, login) } +end + def resolve_base_ref base_ref = ENV.fetch("BASE_REF", "").strip return base_ref unless base_ref.empty? @@ -178,6 +287,11 @@ def resolve_base_ref result.empty? ? "origin/main" : result end +def commit_authors_for_file(file, base_ref) + `git log "#{base_ref}..HEAD" --format="%an" -- "#{file}" 2>/dev/null` + .split("\n").map(&:strip).reject(&:empty?).uniq +end + def changed_files(base_ref) `git diff --name-only "#{base_ref}" HEAD 2>/dev/null`.split("\n").map(&:strip).reject(&:empty?) end @@ -204,6 +318,9 @@ def main rssi_names = split_names(ENV["RSSI"]) cto_names = split_names(ENV["CTO"]) ceo_names = split_names(ENV["CEO"]) + github_token = ENV.fetch("GITHUB_TOKEN", "").strip + pr_number = ENV.fetch("PR_NUMBER", "").strip + repository = ENV.fetch("GITHUB_REPOSITORY", "").strip all_changed = changed_files(base_ref) isms_files = all_changed.select { |f| f.end_with?(".md") && matches_pattern?(f, files_pattern) } @@ -215,6 +332,13 @@ def main puts "Checking #{isms_files.size} changed ISMS document(s) against base #{base_ref}..." + # Fetch PR approved reviewers once (nil means check is disabled). + approved_reviewer_names = + if !github_token.empty? && !pr_number.empty? && !repository.empty? + puts "Fetching PR ##{pr_number} review data from GitHub..." + fetch_pr_approved_reviewer_names(github_token, repository, pr_number) + end + all_errors = [] isms_files.each do |file| @@ -233,7 +357,7 @@ def main file_errors = [] - # Check 1 always applies + # Check 1: authors and validators must be disjoint file_errors += check_author_validator_coherence(file, new_fm) if file_exists_in_base?(file, base_ref) @@ -254,6 +378,12 @@ def main # Check 4: RSSI role (optional) file_errors += check_rssi_role(file, new_fm, rssi_names, cto_names, ceo_names) + # Check 5: git commit authors must all be listed in the document's authors field + file_errors += check_commit_authors_coherence(file, new_fm, commit_authors_for_file(file, base_ref)) + + # Check 6: YAML validators must match actual GitHub PR approvers (optional) + file_errors += check_validators_are_approvers(file, new_fm, approved_reviewer_names) + file_errors.each { |msg| error(file, msg) } all_errors.concat(file_errors) end diff --git a/isms-change-management/tests/test_check_isms_change.rb b/isms-change-management/tests/test_check_isms_change.rb index 27c59d6..3744e78 100644 --- a/isms-change-management/tests/test_check_isms_change.rb +++ b/isms-change-management/tests/test_check_isms_change.rb @@ -212,3 +212,89 @@ def test_unclosed_front_matter_returns_nil assert_nil parse_front_matter("---\ntitle: Test\n") end end + +class TestNormalizeName < Minitest::Test + def test_case_insensitive_match + assert name_in_list?("Yannick Jost", ["yannick jost"]) + assert name_in_list?("YANNICK JOST", ["Yannick Jost"]) + end + + def test_extra_whitespace_normalized + assert name_in_list?("Yannick Jost", ["Yannick Jost"]) + assert name_in_list?("Yannick Jost", ["Yannick Jost"]) + end + + def test_no_match + refute name_in_list?("Alice Martin", ["Bob Dupont", "Carol Lefèvre"]) + end +end + +class TestCommitAuthorsCoherence < Minitest::Test + def test_all_authors_listed_passes + fm = { "authors" => ["Alice Martin", "Bob Dupont"] } + assert_empty check_commit_authors_coherence("file.md", fm, ["Alice Martin"]) + end + + def test_unlisted_commit_author_fails + fm = { "authors" => ["Alice Martin"] } + errors = check_commit_authors_coherence("file.md", fm, ["Alice Martin", "Bob Dupont"]) + refute_empty errors + assert_match(/Bob Dupont/, errors.first) + end + + def test_name_matching_is_case_insensitive + fm = { "authors" => ["alice martin"] } + assert_empty check_commit_authors_coherence("file.md", fm, ["Alice Martin"]) + end + + def test_empty_commit_authors_skips_check + fm = { "authors" => ["Alice Martin"] } + assert_empty check_commit_authors_coherence("file.md", fm, []) + end + + def test_multiple_unlisted_authors_reported + fm = { "authors" => ["Alice Martin"] } + errors = check_commit_authors_coherence("file.md", fm, ["Bob Dupont", "Carol Lefèvre"]) + assert_equal 2, errors.size + end +end + +class TestValidatorsAreApprovers < Minitest::Test + def test_validators_match_approvers_passes + fm = { "validators" => ["Bob Dupont"] } + assert_empty check_validators_are_approvers("file.md", fm, ["Bob Dupont"]) + end + + def test_nil_approved_reviewers_skips_check + fm = { "validators" => ["Bob Dupont"] } + assert_empty check_validators_are_approvers("file.md", fm, nil) + end + + def test_yaml_validator_not_in_approvers_fails + fm = { "validators" => ["Bob Dupont"] } + errors = check_validators_are_approvers("file.md", fm, ["Carol Lefèvre"]) + assert errors.any? { |e| e.include?("Bob Dupont") && e.include?("not approved") } + end + + def test_approver_not_in_yaml_validators_fails + fm = { "validators" => ["Bob Dupont"] } + errors = check_validators_are_approvers("file.md", fm, ["Bob Dupont", "Carol Lefèvre"]) + assert errors.any? { |e| e.include?("Carol Lefèvre") && e.include?("not listed as a validator") } + end + + def test_bidirectional_mismatch_reports_both + fm = { "validators" => ["Bob Dupont"] } + errors = check_validators_are_approvers("file.md", fm, ["Carol Lefèvre"]) + assert_equal 2, errors.size + end + + def test_name_matching_is_case_insensitive + fm = { "validators" => ["bob dupont"] } + assert_empty check_validators_are_approvers("file.md", fm, ["Bob Dupont"]) + end + + def test_empty_validators_and_approvers_passes + fm = { "validators" => [] } + assert_empty check_validators_are_approvers("file.md", fm, []) + end +end From 4c6b502ec8b43a503eb1745f2c572967cdff0657 Mon Sep 17 00:00:00 2001 From: Yannick JOST Date: Tue, 5 May 2026 13:21:40 +0200 Subject: [PATCH 3/3] feat(isms-change-management): classify content change, enforce version policy, post PR comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add three new capabilities: Check 7 — Version matches content change: Classifies each changed ISMS document body as patch/minor/major according to the security policy (>50% lines changed → major, heading added/removed → minor, cosmetic only → patch) and fails when the declared version bump is lower than the content warrants. PR comment: When github-token + pr-number are provided, the action posts (or updates in place) a markdown comment on the PR summarising every check result per file with ✅/❌ emojis and an overall verdict. Security fixes (semgrep): Replace shell-interpolated backtick/system calls with array-form IO.popen and system to eliminate shell injection risk. Two remaining findings (git revision-range and object-specifier arguments that must be a single token) are annotated with nosemgrep and an explanation. Also add Python __pycache__ and *.pyc to .gitignore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .gitignore | 4 + isms-change-management/README.md | 69 +++++- .../scripts/check_isms_change.rb | 229 ++++++++++++++++-- .../tests/test_check_isms_change.rb | 197 +++++++++++++++ 4 files changed, 478 insertions(+), 21 deletions(-) diff --git a/.gitignore b/.gitignore index 70e7ccf..e3b3be2 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,10 @@ .DS_Store .foreman +# Python +__pycache__/ +*.pyc + # Ignore tests binaries *.test # But take CI env file diff --git a/isms-change-management/README.md b/isms-change-management/README.md index 90e1cc9..94d3656 100644 --- a/isms-change-management/README.md +++ b/isms-change-management/README.md @@ -10,6 +10,7 @@ It checks that: 4. **RSSI role** *(optional)* — the RSSI must be a validator, with a specific exception when the RSSI is the author 5. **Commit authors** — every git commit author touching the file must be declared in the document's `authors` field 6. **PR reviewer coherence** *(optional)* — the `validators` field must exactly match the set of GitHub PR approvers +7. **Version matches content change** — the version bump level (patch/minor/major) must reflect the actual nature of the content change ## Usage @@ -42,7 +43,7 @@ jobs: | `rssi` | no | `""` | Comma-separated full name(s) of the RSSI holder(s). When set, enables the RSSI role check. | | `cto` | no | `""` | Comma-separated full name(s) of the CTO(s). Used when `rssi` is set. | | `ceo` | no | `""` | Comma-separated full name(s) of the CEO(s). Used when `rssi` is set. | -| `github-token` | no | `""` | GitHub token. When set together with `pr-number`, enables the PR reviewer coherence check. | +| `github-token` | no | `""` | GitHub token. When set together with `pr-number`, enables the PR reviewer coherence check and posts a summary comment on the PR. | | `pr-number` | no | `""` | Pull request number. Required when `github-token` is provided. | > `fetch-depth: 0` is required in the `actions/checkout` step so that git history is available for the base-ref comparison. @@ -191,3 +192,69 @@ validators: validators: - Bob Dupont ``` + +### 7. Version Matches Content Change + +The version bump level must reflect the actual nature of the content change, as defined by the security policy: + +| Content change | Required bump | Detection rule | +|----------------|---------------|----------------| +| Cosmetic / wording corrections | patch | fewer than 50 % of body lines changed, no heading change | +| Article or heading added / removed | **at least minor** | a heading line (`#…`) was added or removed | +| Substantial rewrite | **major** | more than 50 % of non-empty body lines changed | + +A higher bump than required is always acceptable (e.g. bumping major for what is technically a minor change is fine). Only bumping *lower* than the content warrants is an error. + +**Triggers an error when** the content change qualifies as minor or major but the version bump does not reach that level. + +```yaml +# Body diff: 60 % of lines changed → classified as major +# ❌ Error: only a patch bump for a major content change +version: 1.0.1 # was 1.0.0 + +# ✅ OK: major bump matches major content change +version: 2.0.0 # was 1.0.0 +``` + +## PR Comment + +When `github-token` and `pr-number` are provided, the action posts (or updates) a single comment on the pull request summarising the result of every check for every changed ISMS document. + +### All checks passed + +> ## ISMS Change Management Compliance +> +> ### ✅ `isms/access-control/ISMS-Access-Control-Policy.md` — patch bump (1.3.0 → 1.3.1) — content change: **patch** +> +> - ✅ Author ≠ Validator +> - ✅ Version bumped +> - ✅ Double validation +> - ✅ Version matches content change +> - ✅ RSSI role +> - ✅ Commit authors coherence +> - ✅ PR reviewer coherence +> +> --- +> +> **Overall result: ✅ All checks passed.** + +### Some checks failed + +> ## ISMS Change Management Compliance +> +> ### ❌ `isms/change-management/ISMS-Change-Management-Policy.md` — patch bump (2.1.0 → 2.1.1) — content change: **minor** +> +> - ✅ Author ≠ Validator +> - ✅ Version bumped +> - ✅ Double validation +> - ❌ **Version matches content change**: Content change classified as **minor** (an article or heading was added or removed), but the version was only bumped as patch (2.1.0 → 2.1.1). A minor bump is required by the security policy. +> - ✅ RSSI role +> - ✅ Commit authors coherence +> - ❌ **PR reviewer coherence**: 'Carol Lefèvre' is listed as a validator but has not approved the PR on GitHub. Actual approvers: Bob Dupont +> +> --- +> +> **Overall result: ❌ Some checks failed. See details above.** + +The comment is updated in place on each new push, so the PR always shows the latest status without accumulating duplicate comments. + diff --git a/isms-change-management/scripts/check_isms_change.rb b/isms-change-management/scripts/check_isms_change.rb index 1727e02..f71b429 100644 --- a/isms-change-management/scripts/check_isms_change.rb +++ b/isms-change-management/scripts/check_isms_change.rb @@ -13,6 +13,11 @@ # listed in the document's authors field # 6. PR reviewer coherence — YAML validators and actual GitHub PR approvers # must match (enabled when GITHUB_TOKEN + PR_NUMBER set) +# 7. Version matches content — the version bump level (patch/minor/major) must +# match the nature of the content change: +# major → >50 % of body lines rewritten +# minor → a heading was added or removed +# patch → cosmetic / wording corrections only # # Environment variables: # BASE_REF — base branch ref (optional; falls back to merge-base with origin/main) @@ -20,7 +25,8 @@ # RSSI — comma-separated name(s) of the RSSI holder(s) # CTO — comma-separated name(s) of the CTO(s) # CEO — comma-separated name(s) of the CEO(s) -# GITHUB_TOKEN — GitHub token; enables PR reviewer coherence check when set +# GITHUB_TOKEN — GitHub token; enables PR reviewer coherence check and PR +# comment posting when set together with PR_NUMBER # PR_NUMBER — pull request number; required when GITHUB_TOKEN is set # GITHUB_REPOSITORY — set automatically by GitHub Actions (owner/repo) @@ -50,6 +56,53 @@ def parse_front_matter(content) nil end +# Return everything after the closing front matter delimiter, or the full +# string when no front matter is present. +def strip_front_matter(content) + return content.to_s unless content.to_s.start_with?("---") + + end_index = content.index("\n---", 3) + return content.to_s unless end_index + + # Skip "\n---" (4 chars) plus the newline that follows the closing delimiter. + body_start = end_index + 4 + body_start += 1 if content[body_start] == "\n" + content[body_start..] +end + +# Extract headings (lines beginning with one or more `#`) from a document, +# stripping the front matter first. +def extract_headings(content) + strip_front_matter(content).lines.select { |l| l.match?(/\A#+\s/) }.map(&:strip) +end + +# Classify the nature of a content change between two document versions. +# Returns :major, :minor, or :patch according to the security policy: +# :major — more than 50 % of the non-empty body lines were changed +# :minor — at least one heading was added or removed +# :patch — cosmetic / wording corrections only +def classify_content_change(old_content, new_content) + old_lines = strip_front_matter(old_content).lines.reject { |l| l.strip.empty? } + new_lines = strip_front_matter(new_content).lines.reject { |l| l.strip.empty? } + + old_tally = old_lines.tally + new_tally = new_lines.tally + + removed = old_tally.sum { |line, count| [count - (new_tally[line] || 0), 0].max } + added = new_tally.sum { |line, count| [count - (old_tally[line] || 0), 0].max } + + total_old = [old_lines.size, 1].max + ratio = (removed + added).to_f / total_old + + if ratio > 0.5 + :major + elsif extract_headings(old_content) != extract_headings(new_content) + :minor + else + :patch + end +end + # Compare two semver strings. # Returns -1, 0, or 1 (similar to <=>). # Pre-release labels (e.g. "1.0.0-beta1") sort before the release version. @@ -156,8 +209,30 @@ def check_double_validation(file, old_front_matter, new_front_matter) "but only #{validators.size} found: #{validators.join(", ").then { |s| s.empty? ? "(none)" : s }}"] end -# Check 4: RSSI must validate, unless RSSI is the author (then CTO or CEO must validate). -# Only runs when rssi_names is non-empty. +# Check 7: The version bump level must match the nature of the content change. +# :major content change (>50 % of body lines changed) → must be a major bump +# :minor content change (a heading added or removed) → must be at least a minor bump +# :patch content change (cosmetic only) → any bump is acceptable +def check_version_matches_content_change(file, old_front_matter, new_front_matter, content_kind) + old_version = old_front_matter["version"].to_s + new_version = new_front_matter["version"].to_s + actual_bump = bump_kind(old_version, new_version) + + bump_rank = { none: 0, patch: 1, minor: 2, major: 3 } + return [] if bump_rank[actual_bump] >= bump_rank[content_kind] + + description = case content_kind + when :major then "more than 50% of the content was rewritten" + when :minor then "an article or heading was added or removed" + else "cosmetic corrections only" + end + + ["Content change classified as **#{content_kind}** (#{description}), " \ + "but the version was only bumped as #{actual_bump} (#{old_version} → #{new_version}). " \ + "A #{content_kind} bump is required by the security policy."] +end + + def check_rssi_role(file, front_matter, rssi_names, cto_names, ceo_names) return [] if rssi_names.empty? @@ -229,16 +304,26 @@ def check_validators_are_approvers(file, front_matter, approved_reviewer_names) # GitHub API # --------------------------------------------------------------------------- -def github_api_get(token, path) +COMMENT_MARKER = "" + +def github_api_request(method, token, path, payload = nil) uri = URI("https://api.github.com#{path}") - req = Net::HTTP::Get.new(uri) - req["Authorization"] = "Bearer #{token}" - req["Accept"] = "application/vnd.github+json" + req = case method + when :post then Net::HTTP::Post.new(uri) + when :patch then Net::HTTP::Patch.new(uri) + else Net::HTTP::Get.new(uri) + end + req["Authorization"] = "Bearer #{token}" + req["Accept"] = "application/vnd.github+json" req["X-GitHub-Api-Version"] = "2022-11-28" + if payload + req["Content-Type"] = "application/json" + req.body = payload.to_json + end response = Net::HTTP.start(uri.host, uri.port, use_ssl: true) { |http| http.request(req) } unless response.is_a?(Net::HTTPSuccess) - warn "::warning::GitHub API #{path} returned HTTP #{response.code}" + warn "::warning::GitHub API #{method.upcase} #{path} returned HTTP #{response.code}" return nil end JSON.parse(response.body) @@ -247,6 +332,10 @@ def github_api_get(token, path) nil end +def github_api_get(token, path) + github_api_request(:get, token, path) +end + # Fetch a GitHub user's display name, falling back to their login. def fetch_github_display_name(token, login) data = github_api_get(token, "/users/#{URI.encode_uri_component(login)}") @@ -278,6 +367,65 @@ def fetch_pr_approved_reviewer_names(token, repository, pr_number) approver_logins.map { |login| fetch_github_display_name(token, login) } end +# Build a markdown PR comment summarising all per-file check results. +# Each entry in file_results is a Hash with: +# :file, :passed, :checks, :actual_bump, :old_version, :new_version, :content_kind +def build_pr_comment(file_results) + lines = [COMMENT_MARKER, "## ISMS Change Management Compliance", ""] + + file_results.each do |result| + file = result[:file] + actual_bump = result[:actual_bump] + old_ver = result[:old_version] + new_ver = result[:new_version] + content_kind = result[:content_kind] + file_ok = result[:passed] + + heading_parts = ["`#{file}`"] + if actual_bump && actual_bump != :none && old_ver && new_ver + heading_parts << "#{actual_bump} bump (#{old_ver} → #{new_ver})" + end + heading_parts << "content change: **#{content_kind}**" if content_kind + + status_icon = file_ok ? "✅" : "❌" + lines << "### #{status_icon} #{heading_parts.join(" — ")}" + lines << "" + + result[:checks].each do |check| + if check[:passed] + lines << "- ✅ #{check[:name]}" + else + check[:errors].each { |err| lines << "- ❌ **#{check[:name]}**: #{err}" } + end + end + + lines << "" + end + + overall_passed = file_results.all? { |r| r[:passed] } + lines << "---" + lines << "" + lines << if overall_passed + "**Overall result: ✅ All checks passed.**" + else + "**Overall result: ❌ Some checks failed. See details above.**" + end + lines.join("\n") +end + +# Post a new PR comment or update the existing one left by this action. +def post_or_update_pr_comment(token, repository, pr_number, body) + comments = github_api_get(token, "/repos/#{repository}/issues/#{pr_number}/comments") + return unless comments + + existing = comments.find { |c| c["body"].to_s.start_with?(COMMENT_MARKER) } + if existing + github_api_request(:patch, token, "/repos/#{repository}/issues/comments/#{existing["id"]}", { body: body }) + else + github_api_request(:post, token, "/repos/#{repository}/issues/#{pr_number}/comments", { body: body }) + end +end + def resolve_base_ref base_ref = ENV.fetch("BASE_REF", "").strip return base_ref unless base_ref.empty? @@ -288,20 +436,23 @@ def resolve_base_ref end def commit_authors_for_file(file, base_ref) - `git log "#{base_ref}..HEAD" --format="%an" -- "#{file}" 2>/dev/null` + # base_ref..HEAD is a single git revision-range argument; no shell is involved (array form). + IO.popen(["git", "log", "#{base_ref}..HEAD", "--format=%an", "--", file], err: File::NULL) { |io| io.read } # nosemgrep: ruby.lang.security.dangerous-exec.dangerous-exec .split("\n").map(&:strip).reject(&:empty?).uniq end def changed_files(base_ref) - `git diff --name-only "#{base_ref}" HEAD 2>/dev/null`.split("\n").map(&:strip).reject(&:empty?) + IO.popen(["git", "diff", "--name-only", base_ref, "HEAD"], err: File::NULL) { |io| io.read } + .split("\n").map(&:strip).reject(&:empty?) end def file_exists_in_base?(file, base_ref) - system("git cat-file -e \"#{base_ref}:#{file}\" 2>/dev/null") + system("git", "cat-file", "-e", "#{base_ref}:#{file}", out: File::NULL, err: File::NULL) end def read_base_content(file, base_ref) - `git show "#{base_ref}:#{file}" 2>/dev/null` + # base_ref:file is a single git object specifier argument; no shell is involved (array form). + IO.popen(["git", "show", "#{base_ref}:#{file}"], err: File::NULL) { |io| io.read } # nosemgrep: ruby.lang.security.dangerous-exec.dangerous-exec end def matches_pattern?(file, pattern) @@ -332,20 +483,25 @@ def main puts "Checking #{isms_files.size} changed ISMS document(s) against base #{base_ref}..." + can_post_comment = !github_token.empty? && !pr_number.empty? && !repository.empty? + # Fetch PR approved reviewers once (nil means check is disabled). approved_reviewer_names = - if !github_token.empty? && !pr_number.empty? && !repository.empty? + if can_post_comment puts "Fetching PR ##{pr_number} review data from GitHub..." fetch_pr_approved_reviewer_names(github_token, repository, pr_number) end - all_errors = [] + all_errors = [] + file_results = [] isms_files.each do |file| new_content = File.read(file, encoding: "utf-8") rescue nil unless new_content error(file, "Cannot read file #{file}") all_errors << file + file_results << { file: file, passed: false, checks: [], actual_bump: nil, + old_version: nil, new_version: nil, content_kind: nil } next end @@ -355,37 +511,70 @@ def main next end + file_checks = [] file_errors = [] + run_check = lambda do |name, errors| + file_checks << { name: name, passed: errors.empty?, errors: errors } + file_errors.concat(errors) + end + # Check 1: authors and validators must be disjoint - file_errors += check_author_validator_coherence(file, new_fm) + run_check.call("Author ≠ Validator", check_author_validator_coherence(file, new_fm)) + + old_fm = nil + actual_bump = nil + content_kind = nil if file_exists_in_base?(file, base_ref) base_content = read_base_content(file, base_ref) old_fm = parse_front_matter(base_content) if old_fm + actual_bump = bump_kind(old_fm["version"].to_s, new_fm["version"].to_s) + content_kind = classify_content_change(base_content, new_content) + # Check 2: version bump - file_errors += check_version_bump(file, old_fm, new_fm) + run_check.call("Version bumped", check_version_bump(file, old_fm, new_fm)) # Check 3: double validation for minor/major - file_errors += check_double_validation(file, old_fm, new_fm) + run_check.call("Double validation", check_double_validation(file, old_fm, new_fm)) + + # Check 7: version bump level must match content change classification + run_check.call("Version matches content change", + check_version_matches_content_change(file, old_fm, new_fm, content_kind)) end else puts "::notice file=#{file}::New document — version and double-validation checks skipped." end # Check 4: RSSI role (optional) - file_errors += check_rssi_role(file, new_fm, rssi_names, cto_names, ceo_names) + run_check.call("RSSI role", check_rssi_role(file, new_fm, rssi_names, cto_names, ceo_names)) # Check 5: git commit authors must all be listed in the document's authors field - file_errors += check_commit_authors_coherence(file, new_fm, commit_authors_for_file(file, base_ref)) + run_check.call("Commit authors coherence", + check_commit_authors_coherence(file, new_fm, commit_authors_for_file(file, base_ref))) # Check 6: YAML validators must match actual GitHub PR approvers (optional) - file_errors += check_validators_are_approvers(file, new_fm, approved_reviewer_names) + run_check.call("PR reviewer coherence", + check_validators_are_approvers(file, new_fm, approved_reviewer_names)) file_errors.each { |msg| error(file, msg) } all_errors.concat(file_errors) + + file_results << { + file: file, + passed: file_errors.empty?, + checks: file_checks, + actual_bump: actual_bump, + old_version: old_fm&.[]("version"), + new_version: new_fm["version"], + content_kind: content_kind + } + end + + if can_post_comment && !file_results.empty? + post_or_update_pr_comment(github_token, repository, pr_number, build_pr_comment(file_results)) end if all_errors.empty? diff --git a/isms-change-management/tests/test_check_isms_change.rb b/isms-change-management/tests/test_check_isms_change.rb index 3744e78..66e8352 100644 --- a/isms-change-management/tests/test_check_isms_change.rb +++ b/isms-change-management/tests/test_check_isms_change.rb @@ -298,3 +298,200 @@ def test_empty_validators_and_approvers_passes assert_empty check_validators_are_approvers("file.md", fm, []) end end + +# --------------------------------------------------------------------------- +# Helpers shared by content-change tests +# --------------------------------------------------------------------------- + +def make_body(headings: ["Introduction", "Policy"], lines: []) + heading_lines = headings.map { |h| "## #{h}" } + ([heading_lines] + [lines]).flatten.join("\n") + "\n" +end + +def wrap_doc(body, version: "1.0.0", authors: ["Alice"], validators: ["Bob"]) + front = "---\nversion: #{version}\nauthors:\n - #{authors.join("\n - ")}\nvalidators:\n - #{validators.join("\n - ")}\n---\n" + front + body +end + +class TestStripFrontMatter < Minitest::Test + def test_strips_front_matter + content = "---\nversion: 1.0.0\n---\n# Body\nSome text." + assert_equal "# Body\nSome text.", strip_front_matter(content) + end + + def test_no_front_matter_returns_content_unchanged + content = "# Just a heading\nNo front matter." + assert_equal content, strip_front_matter(content) + end + + def test_unclosed_front_matter_returns_content_unchanged + content = "---\nversion: 1.0.0\n" + assert_equal content, strip_front_matter(content) + end +end + +class TestExtractHeadings < Minitest::Test + def test_extracts_headings + content = "---\nv: 1\n---\n## Intro\nSome text.\n### Sub\nMore." + assert_equal ["## Intro", "### Sub"], extract_headings(content) + end + + def test_ignores_front_matter_lines + content = "---\ntitle: My Doc\n---\n## Real Heading" + assert_equal ["## Real Heading"], extract_headings(content) + end + + def test_no_headings_returns_empty + content = "---\nv: 1\n---\nJust a paragraph." + assert_empty extract_headings(content) + end +end + +class TestClassifyContentChange < Minitest::Test + SAME_BODY = make_body(headings: ["Intro", "Policy"], lines: Array.new(20) { |i| "Line #{i + 1}." }) + + def old_doc + wrap_doc(SAME_BODY) + end + + def test_identical_body_is_patch + assert_equal :patch, classify_content_change(old_doc, old_doc) + end + + def test_cosmetic_wording_is_patch + new_body = SAME_BODY.sub("Line 1.", "Line one.") + assert_equal :patch, classify_content_change(old_doc, wrap_doc(new_body)) + end + + def test_heading_added_is_minor + new_body = SAME_BODY + "## New Section\nSome content.\n" + assert_equal :minor, classify_content_change(old_doc, wrap_doc(new_body)) + end + + def test_heading_removed_is_minor + new_body = SAME_BODY.lines.reject { |l| l.include?("## Policy") }.join + assert_equal :minor, classify_content_change(old_doc, wrap_doc(new_body)) + end + + def test_major_rewrite_is_major + # Replace more than 50 % of body lines with entirely new content + old_lines = Array.new(20) { |i| "Old line #{i + 1}.\n" } + new_lines = Array.new(20) { |i| "New line #{i + 1}.\n" } + old_body = "## Intro\n" + old_lines.join + new_body = "## Intro\n" + old_lines.first(3).join + new_lines.drop(3).join + assert_equal :major, classify_content_change(wrap_doc(old_body), wrap_doc(new_body)) + end + + def test_only_front_matter_changed_is_patch + new_doc = wrap_doc(SAME_BODY, version: "1.0.1") + assert_equal :patch, classify_content_change(old_doc, new_doc) + end +end + +class TestVersionMatchesContentChange < Minitest::Test + def test_patch_content_with_patch_bump_passes + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "1.0.1" } + assert_empty check_version_matches_content_change("file.md", old_fm, new_fm, :patch) + end + + def test_patch_content_with_minor_bump_passes + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "1.1.0" } + assert_empty check_version_matches_content_change("file.md", old_fm, new_fm, :patch) + end + + def test_minor_content_with_patch_bump_fails + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "1.0.1" } + errors = check_version_matches_content_change("file.md", old_fm, new_fm, :minor) + refute_empty errors + assert_match(/minor/, errors.first) + assert_match(/patch/, errors.first) + end + + def test_minor_content_with_minor_bump_passes + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "1.1.0" } + assert_empty check_version_matches_content_change("file.md", old_fm, new_fm, :minor) + end + + def test_minor_content_with_major_bump_passes + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "2.0.0" } + assert_empty check_version_matches_content_change("file.md", old_fm, new_fm, :minor) + end + + def test_major_content_with_minor_bump_fails + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "1.1.0" } + errors = check_version_matches_content_change("file.md", old_fm, new_fm, :major) + refute_empty errors + assert_match(/major/, errors.first) + assert_match(/minor/, errors.first) + end + + def test_major_content_with_major_bump_passes + old_fm = { "version" => "1.0.0" } + new_fm = { "version" => "2.0.0" } + assert_empty check_version_matches_content_change("file.md", old_fm, new_fm, :major) + end +end + +class TestBuildPrComment < Minitest::Test + def all_passed_result + { + file: "isms/policy.md", + passed: true, + actual_bump: :patch, + old_version: "1.0.0", + new_version: "1.0.1", + content_kind: :patch, + checks: [ + { name: "Author ≠ Validator", passed: true, errors: [] }, + { name: "Version bumped", passed: true, errors: [] }, + { name: "Version matches content change", passed: true, errors: [] } + ] + } + end + + def test_overall_pass_message + comment = build_pr_comment([all_passed_result]) + assert_match(/All checks passed/, comment) + refute_match(/Some checks failed/, comment) + end + + def test_overall_fail_message + result = all_passed_result.merge( + passed: false, + checks: [{ name: "Version bumped", passed: false, errors: ["Version not bumped."] }] + ) + comment = build_pr_comment([result]) + assert_match(/Some checks failed/, comment) + end + + def test_check_mark_for_passing_check + comment = build_pr_comment([all_passed_result]) + assert_match(/✅ Author ≠ Validator/, comment) + end + + def test_cross_mark_for_failing_check + result = all_passed_result.merge( + passed: false, + checks: [{ name: "Version bumped", passed: false, errors: ["Not bumped."] }] + ) + comment = build_pr_comment([result]) + assert_match(/❌.*Version bumped.*Not bumped\./, comment) + end + + def test_includes_bump_and_content_info_in_heading + comment = build_pr_comment([all_passed_result]) + assert_match(/patch bump \(1\.0\.0 → 1\.0\.1\)/, comment) + assert_match(/content change: \*\*patch\*\*/, comment) + end + + def test_comment_starts_with_marker + comment = build_pr_comment([all_passed_result]) + assert comment.start_with?(COMMENT_MARKER) + end +end