From d8c2ebb4f02877ad44bcb593d20446cbd250e0b1 Mon Sep 17 00:00:00 2001 From: CD Cabrera Date: Mon, 11 May 2026 16:19:55 -0400 Subject: [PATCH 1/2] build: pf-4128 contribution workflow actions * build, workflow package audit, combined PR gatekeeper, commits, integration * testing, scripts testing for workflows --- .github/workflows/audit.yml | 43 +- .github/workflows/integration.yml | 97 ++- cspell.config.json | 4 + eslint.config.js | 3 +- jest.config.ts | 12 +- package.json | 10 +- scripts/workflow.commitLint.js | 219 +++++++ scripts/workflow.preCheck.js | 465 ++++++++++++++ .../workflow.preCheck.test.ts.snap | 150 +++++ tests/scripts/workflow.commitLint.test.ts | 273 +++++++++ tests/scripts/workflow.preCheck.test.ts | 576 ++++++++++++++++++ tsconfig.json | 3 +- 12 files changed, 1830 insertions(+), 25 deletions(-) create mode 100644 scripts/workflow.commitLint.js create mode 100644 scripts/workflow.preCheck.js create mode 100644 tests/scripts/__snapshots__/workflow.preCheck.test.ts.snap create mode 100644 tests/scripts/workflow.commitLint.test.ts create mode 100644 tests/scripts/workflow.preCheck.test.ts diff --git a/.github/workflows/audit.yml b/.github/workflows/audit.yml index 4da0ea41..c05b253c 100644 --- a/.github/workflows/audit.yml +++ b/.github/workflows/audit.yml @@ -1,16 +1,21 @@ -name: Data Audit +name: Security & Data Audit on: pull_request: paths: - 'src/docs.json' + - 'scripts/**' + - 'tests/scripts/**' - 'tests/audit/**' + - 'package.json' + - 'package-lock.json' schedule: - cron: '0 0 * * *' # Daily at midnight workflow_dispatch: jobs: - audit-links: + documentation-audit: + name: Documentation Audit runs-on: ubuntu-latest permissions: contents: read @@ -30,3 +35,37 @@ jobs: DOCS_AUDIT_MAX_TOTAL: ${{ github.event_name == 'schedule' && '0' || '50' }} # Advisories are non-blocking for PRs continue-on-error: ${{ github.event_name == 'pull_request' }} + + dependency-audit: + name: Dependency Audit + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@v6 + - name: Setup Node.js + uses: actions/setup-node@v6.3.0 + with: + node-version: 22.x + cache: npm + - name: Run npm audit + run: npm audit --omit-dev --audit-level=critical + # Advisories are non-blocking for PRs + continue-on-error: ${{ github.event_name == 'pull_request' }} + + script-audit: + name: Script Audit + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@v6 + - name: Setup Node.js + uses: actions/setup-node@v6.3.0 + with: + node-version: 22.x + cache: npm + - name: Install dependencies + run: npm ci + - name: Run script audit + run: npm run test:scripts diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 93d1dfc7..4c43fd2f 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -1,14 +1,92 @@ name: Build on: + pull_request: push: branches: [ main ] tags: - "*" - pull_request: jobs: + Gatekeeper: + if: ${{ github.event_name == 'pull_request' }} + runs-on: ubuntu-latest + permissions: + pull-requests: write + contents: read + steps: + - name: Trusted configuration + uses: actions/checkout@v6 + with: + ref: ${{ github.event.pull_request.base.ref }} # Force our main/base branch + sparse-checkout: | + .github + scripts + + - name: Validate Policy + uses: actions/github-script@v9 + with: + script: | + const path = require('path'); + let startFn; + try { + const scriptPath = path.resolve(process.env.GITHUB_WORKSPACE, 'scripts', 'workflow.preCheck.js'); + const module = await import(scriptPath); + startFn = module.start || module.default; + } catch (err) { + console.error('PreCheck loading error.', err?.message || err); + core.setFailed('PreCheck loading error. File is missing or unreadable. Has the file been checked in?'); + } + + if (startFn) { + await startFn({ + LABEL_NEEDS_CLEANUP: 'bot:needs-cleanup', + LABEL_NEEDS_MAINTAINER: 'bot:needs-maintainer', + LABEL_PRECHECKS_PASS: 'bot:policy-ready' + }, { + github, + context, + core, + }); + } + + - name: Commit Lint + uses: actions/github-script@v9 + with: + script: | + const path = require('path'); + let startFn; + try { + const scriptPath = path.resolve(process.env.GITHUB_WORKSPACE, 'scripts', 'workflow.commitLint.js'); + const module = await import(scriptPath); + startFn = module.start || module.default; + } catch (err) { + console.error('Lint loading error.', err?.message || err); + core.setFailed('Lint loading error. File is missing or unreadable. Has the file been checked in?'); + } + + if (startFn) { + const { data: commits } = await github.rest.pulls.listCommits({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.issue.number, + }); + + const { resultsArray, resultsString } = startFn(commits, { + allowIssuesAnywhere: false, + issueNumberExceptions: ['build', 'chore', 'ci', 'docs', 'fix', 'perf', 'refactor', 'test'], + maxMessageLength: 65, + typeScopeExceptions: '*' + }); + + if (resultsArray.length) { + core.setFailed(resultsString); + } + } + Integration-checks: runs-on: ubuntu-latest + needs: Gatekeeper + if: ${{ always() && (github.event_name == 'push' || needs.Gatekeeper.result != 'cancelled') }} permissions: contents: read strategy: @@ -16,27 +94,18 @@ jobs: node-version: [20.x, 22.x, 24.x] steps: - uses: actions/checkout@v6 + - name: Setup Node.js ${{ matrix.node-version }} uses: actions/setup-node@v6.3.0 with: node-version: ${{ matrix.node-version }} cache: npm - - name: Node.js modules cache - uses: actions/cache@v5 - id: modules-cache - with: - path: ${{ github.workspace }}/node_modules - key: ${{ runner.os }}-${{ matrix.node-version }}-modules-${{ hashFiles('**/package-lock.json') }} - restore-keys: | - ${{ runner.os }}-${{ matrix.node-version }}-modules + - name: Install Node.js packages - if: ${{ steps.modules-cache.outputs.cache-hit != 'true' }} run: npm ci - - name: Audit packages - run: npm audit --audit-level=high - continue-on-error: true + - name: Lint and test run: npm run test:ci + - name: Confirm integration - if: ${{ success() }} run: npm run test:integration diff --git a/cspell.config.json b/cspell.config.json index bc46682e..ca7159ad 100644 --- a/cspell.config.json +++ b/cspell.config.json @@ -1,9 +1,11 @@ { "language": "en", "words": [ + "aiignore", "amet", "codemods", "ized", + "junie", "llms", "localappdata", "midrun", @@ -12,6 +14,8 @@ "onsessioninitialized", "onsessionclosed", "patternfly", + "precheck", + "prechecks", "rereview", "rsort", "sparkline", diff --git a/eslint.config.js b/eslint.config.js index b77d361d..4b97a008 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -127,7 +127,8 @@ export default [ { files: [ 'docs/**/*.ts', - 'docs/**/*.js' + 'docs/**/*.js', + 'scripts/**/*.js' ], rules: { 'jsdoc/require-returns': 0, diff --git a/jest.config.ts b/jest.config.ts index 8c02c24a..e759a021 100644 --- a/jest.config.ts +++ b/jest.config.ts @@ -36,7 +36,7 @@ export default { projects: [ { displayName: 'unit', - roots: ['src'], + roots: ['/src'], testMatch: ['/src/**/*.test.ts'], setupFilesAfterEnv: ['/jest.setupTests.ts'], ...baseConfig, @@ -64,7 +64,7 @@ export default { }, { displayName: 'e2e', - roots: ['tests/e2e'], + roots: ['/tests/e2e'], testMatch: ['/tests/e2e/**/*.test.ts'], setupFilesAfterEnv: ['/tests/e2e/jest.setupTests.ts'], transformIgnorePatterns: [ @@ -74,9 +74,15 @@ export default { }, { displayName: 'audit', - roots: ['tests/audit'], + roots: ['/tests/audit'], testMatch: ['/tests/audit/**/*.test.ts'], ...baseConfig + }, + { + displayName: 'scripts', + roots: ['/scripts', '/tests/scripts'], + testMatch: ['/tests/scripts/**/*.test.ts'], + ...baseConfig } ] }; diff --git a/package.json b/package.json index acbb8563..baf1bb6d 100644 --- a/package.json +++ b/package.json @@ -31,16 +31,18 @@ "release": "changelog --non-cc --link-url https://github.com/patternfly/patternfly-mcp.git", "start": "node dist/cli.js --log-stderr", "start:dev": "tsx watch src/cli.ts --verbose --log-stderr", - "test": "npm run test:spell && npm run test:spell-docs && npm run test:lint && npm run test:types && jest --selectProjects unit --roots=src/", - "test:audit": "jest --selectProjects audit --roots=tests/audit/", + "test": "npm run test:spell && npm run test:spell-docs && npm run test:lint && npm run test:types && jest --selectProjects unit", + "test:audit": "jest --selectProjects audit", + "test:scripts": "NODE_OPTIONS='--experimental-vm-modules' jest --selectProjects scripts", + "test:scripts-dev": "npm run test:scripts -- --watchAll", "test:ci": "npm test -- --coverage", "test:dev": "npm test -- --watchAll", - "test:integration": "npm run build && NODE_OPTIONS='--experimental-vm-modules' jest --selectProjects e2e --roots=tests/e2e/", + "test:integration": "npm run build && NODE_OPTIONS='--experimental-vm-modules' jest --selectProjects e2e", "test:integration-dev": "npm run test:integration -- --watchAll", "test:lint": "eslint .", "test:lint-fix": "eslint . --fix", "test:spell-docs": "cspell './README.md' './CONTRIBUTING.md' './docs/**/*.md' './guidelines/**/*.md' './docs/**/*.ts' './docs/**/*.js' --config ./cspell.config.json --fail-fast", - "test:spell": "cspell './src/**/*.ts' './tests/**/*.ts' --exclude './src/**/*test*' --exclude './tests/**/*test*' --config ./cspell.config.json --fail-fast", + "test:spell": "cspell './src/**/*.ts' './scripts/**/*.js' './tests/**/*.ts' --exclude './src/**/*test*' --exclude './tests/**/*test*' --config ./cspell.config.json --fail-fast", "test:types": "tsc --noEmit", "prepare": "npm run build", "prepack": "npm run build" diff --git a/scripts/workflow.commitLint.js b/scripts/workflow.commitLint.js new file mode 100644 index 00000000..db070d69 --- /dev/null +++ b/scripts/workflow.commitLint.js @@ -0,0 +1,219 @@ +/** This is a temporary script meant to get up and running fast. It will be replaced. **/ +/** + * Available message scope types. + * + * @type {Array} + */ +const MESSAGE_TYPES = [ + 'feat', + 'fix', + 'docs', + 'style', + 'refactor', + 'perf', + 'test', + 'build', + 'ci', + 'chore', + 'revert' +]; + +/** + * Parse a commit message + * + * @param {object} params + * @param {string} params.hash - Commit hash + * @param {string} params.message - Original commit message, typically the first line. + * @param {object} settings - Function settings + * @param {Array} settings.messageTypes - List of available conventional commit types. + * @param {boolean} settings.allowIssuesAnywhere - Allow issue numbers to appear anywhere. Setting to `false` + * limits the issue number placement to the beginning of the description. + * @returns {{scope: string, description: string, type: string, prNumber: string, hash: string, + * typeScope: string, isBreaking: boolean, original: string, message: string, messageLength: number, + * issueNumber: string}} + */ +const parseCommitMessage = ({ hash, message }, { messageTypes = MESSAGE_TYPES, allowIssuesAnywhere = true } = {}) => { + let output; + + const trimmedMessage = message.trim(); + const firstColonIndex = trimmedMessage.indexOf(':'); + const baseTypeScope = firstColonIndex > -1 ? trimmedMessage.substring(0, firstColonIndex) : ''; + const descriptionEtAll = firstColonIndex > -1 ? trimmedMessage.substring(firstColonIndex + 1).trim() : trimmedMessage; + const prMatch = descriptionEtAll.match(/\s\(#(\d+)\)$/); + + let prNumber = undefined; + let description = descriptionEtAll; + + if (prMatch) { + prNumber = prMatch[1]; + description = descriptionEtAll.replace(/\s\(#(\d+)\)$/, '').trim(); + } + + const issueNumberMatch = allowIssuesAnywhere ? description.match(/([a-zA-Z]+[/-]+[0-9]+)/) : description.match(/(^[a-zA-Z]+[/-]+[0-9]+)/); + let issueNumber = undefined; + + if (issueNumberMatch) { + issueNumber = issueNumberMatch[0]; + } + + const typeScope = baseTypeScope.replace(/!$/, '').trim(); + let type = typeScope; + let scope = undefined; + + if (typeScope.includes('(')) { + const [splitType, splitScope] = typeScope.split('('); + + type = splitType?.trim(); + scope = splitScope?.split(')')?.[0]?.trim(); + } + + const isType = messageTypes.includes(type) && type; + + output = { + hash, + typeScope: isType ? typeScope : undefined, + type: isType ? type : undefined, + scope: isType ? scope : undefined, + description: isType ? description : trimmedMessage, + issueNumber, + prNumber, + isBreaking: isType ? /!$/.test(baseTypeScope) : undefined + }; + + const updatedMessage = [ + `${output.typeScope || ''}${(output.isBreaking && '!') || ''}${(output.typeScope && ':') || ''}`, + output.description + ] + .filter(value => Boolean(value)) + .join(' ') + .trim(); + + return { + ...output, + messageLength: updatedMessage?.length || 0, + message: updatedMessage, + original: message + }; +}; + +/** + * Apply valid/invalid checks. + * + * @param {Array} parsedMessages + * @param {object} options - Default options, update accordingly + * @param {boolean} options.allowIssuesAnywhere - Updates related messaging. See `parseCommitMessage` for parsing behavior. + * @param {Array|string|undefined} options.issueNumberExceptions - An "undefined" or "false" or "falsy" value + * will ignore issue numbers. A string of "*" will allow every type. An array of issue types can be used + * to identify which commit message type scopes to ignore, i.e. ['chore', 'fix', 'build', 'perf']. + * See NPM conventional-commit-types for full listing options, https://bit.ly/2L0yr6I + * @param {number} options.maxMessageLength - Max length of the main message string. Messages considered "body" + * do not count against this limit. + * @param {Array|string|undefined} options.typeScopeExceptions - see `options.issueNumberExceptions` + * @returns {Array} + */ +const messagesList = ( + parsedMessages, + { + allowIssuesAnywhere = true, + issueNumberExceptions = [], + maxMessageLength = 65, + typeScopeExceptions = '*' + } = {} +) => + parsedMessages.map( + ({ messageLength = 0, type = null, scope = null, description = null, message = null, hash = null, issueNumber = null, prNumber = null }) => { + const typeValid = + (type && 'valid') || 'INVALID: type (expected known types and format ":" or "():")'; + + let scopeException = !typeScopeExceptions || typeScopeExceptions === '*'; + + if (!scopeException && Array.isArray(typeScopeExceptions) && typeScopeExceptions.length > 0) { + scopeException = typeScopeExceptions.includes(type); + } + + const scopeValid = (scopeException && 'valid') || (scope && 'valid') || 'INVALID: scope'; + + let issueNumberException = !issueNumberExceptions || issueNumberExceptions === '*'; + + if (!issueNumberException && Array.isArray(issueNumberExceptions) && issueNumberExceptions.length > 0) { + issueNumberException = issueNumberExceptions.includes(type); + } + + const issueNumberValid = + (issueNumberException && 'valid') || + (issueNumber && 'valid') || + (allowIssuesAnywhere && 'INVALID: issue number (expected format "/" or "-")') || + 'INVALID: issue number (expected format "/" or "-" at beginning of description)'; + + const descriptionValid = (description && 'valid') || 'INVALID: description (missing description)'; + + const adjustedMaxMessageLength = issueNumber ? issueNumber.length + 1 + maxMessageLength : maxMessageLength; + const lengthValid = + (messageLength <= adjustedMaxMessageLength && 'valid') || + `INVALID: message length (${messageLength} > ${maxMessageLength}).${ + prNumber ? ' PRs do not count towards message length.' : ''}${ + issueNumber ? ' Issue numbers do not count towards message length.' : ''}`; + + return { + hash, + commit: message, + type: typeValid, + scope: scopeValid, + description: descriptionValid, + issueNumber: issueNumberValid, + length: lengthValid + }; + } + ); + +/** + * If commits exist, lint them. + * + * @param {Array} commits + * @param {object} options - Configuration + * @param {boolean} options.allowIssuesAnywhere - See `parseCommitMessage` for behavior. + * @param {Array} options.issueNumberExceptions - See `messagesList` for behavior. + * @param {number} options.maxMessageLength - See `messagesList` for behavior. + * @param {Array|string|undefined} options.typeScopeExceptions - See `messagesList` for behavior. + * @returns {{resultsArray: Array, resultsString: string}} Return linting results + * - `resultsArray`: An array of objects representing the "error validated" parts of the message. + * - `resultsString`: A `JSON.stringify` version of the `resultsArray` for display. + */ +const start = (commits, { allowIssuesAnywhere, issueNumberExceptions, maxMessageLength, typeScopeExceptions } = {}) => { + const lintResults = { resultsArray: [], resultsString: '' }; + + if (commits) { + const updatedCommits = commits + .map(({ sha, commit } = {}) => parseCommitMessage({ + hash: sha.substring(0, 7), + message: (commit.message || 'empty').split('\n')[0] + }, { allowIssuesAnywhere })); + + let filteredResults = messagesList(updatedCommits, { + allowIssuesAnywhere, + issueNumberExceptions, + maxMessageLength, + typeScopeExceptions + }); + + // Mutate and filter valid commits out + filteredResults.forEach(obj => { + const updatedObj = obj; + + Object.entries(updatedObj).forEach(([key, value]) => { + if (value === 'valid') { + delete updatedObj[key]; + } + }); + }); + + // eslint-disable-next-line @typescript-eslint/no-unused-vars + filteredResults = filteredResults.filter(({ hash, commit, ...rest }) => Object.keys(rest).length > 0); + lintResults.resultsArray = filteredResults; + lintResults.resultsString = JSON.stringify(filteredResults, null, 2); + } + + return lintResults; +}; + +export { messagesList, parseCommitMessage, start, MESSAGE_TYPES }; diff --git a/scripts/workflow.preCheck.js b/scripts/workflow.preCheck.js new file mode 100644 index 00000000..a07ca477 --- /dev/null +++ b/scripts/workflow.preCheck.js @@ -0,0 +1,465 @@ +import fs from 'node:fs'; + +/** + * Confirm specific authors/contributors from an available CODEOWNERS file. + * + * @note This check will pull authors/contributors regardless of CODEOWNERS' rights. + * + * @param params - Passed author parameters for review. + * @param params.author + * @param params.authorType + * @param params.authorRole + * @param options - Optional settings + * @param options.allowBots - Allow known bots to skip preCheck + * @param options.allowMaintainers - Allow general members + * @returns {boolean} A `boolean` indicating whether an author/contributor is allowed to skip pre-checks. + */ +const coreContributors = ({ author, authorType, authorRole } = {}, { allowBots = true, allowMaintainers = true } = {}) => { + const bots = ['Bot', 'dependabot[bot]']; + const contributors = ['OWNER']; + const codeOwnersPaths = ['.github/CODEOWNERS', 'CODEOWNERS']; + + const isBot = allowBots && (bots.includes(authorType) || bots.includes(author)); + const isMaintainer = allowMaintainers && contributors.includes(authorRole); + let isCodeOwner = false; + + for (const filePath of codeOwnersPaths) { + if (!fs.existsSync(filePath)) { + continue; + } + + const content = fs.readFileSync(filePath, 'utf8'); + const isAvailable = content + .split(/[\s,()]+/) + .filter(Boolean) + .includes(`@${author}`); + + if (isAvailable) { + isCodeOwner = true; + break; + } + } + + return isBot || isMaintainer || isCodeOwner; +}; + +/** + * Does one list contain another list's values? + * + * @param {{ filename: string }[]} listBase - Base array of strings to match. + * @param {string[]} listCheck - Array of strings to confirm matches in base. + * @returns {string[]} An array of value matches. + */ +const doesListContainAnotherListValues = (listBase, listCheck) => + ((Array.isArray(listBase) && listBase) || []) + .filter(file => { + const updatedFileName = file?.filename?.trim()?.toLowerCase() || undefined; + const updatedListCheck = ((Array.isArray(listCheck) && listCheck) || []).map(item => item?.toLowerCase()); + + if (!updatedFileName) { + return false; + } + + return updatedListCheck.includes(updatedFileName) || + updatedListCheck.some( + item => (item && (updatedFileName.startsWith(item) || updatedFileName.endsWith(item) || updatedFileName.includes(item) || item.includes(updatedFileName))) + ); + }).map(file => file?.filename); + +/** + * Scan PR for signatures using basic logic. + * + * @param params - Passed code parameters for review. + * @param params.description + * @param params.files + * @param params.fileCount + * @returns {{errors: string[], isMaxFilesUpdated: boolean, isPrTemplateModified: boolean, isAgentModified: boolean, + * isCoreModified: boolean, isExtraModified: boolean, isGenModified: boolean, isSecModified: boolean, + * hasFailed: boolean, hasTell: boolean}} An `object` containing code scan results. + */ +const signatureScan = ({ description, files, fileCount } = {}) => { + // Make sure this is within the PR template, or we'll get false positives. + const prTemplateStr = ''; + + // Max file updates outside of core contributors before alerting. + const fileChangeLimit = 15; + + // Signature checks. This can be a list of existing or non-existent files, directories, and/or extensions. + const coreList = [ + 'src/cli', + 'src/declarations', + 'src/index', + 'src/mcpSdk', + 'src/options.default', + 'src/patternFly.getResources', + 'src/resource.', + 'src/server.ts', + 'src/tool.', + 'tests/audit', + 'tests/e2e' + ]; + + // generated check. This can be a list of existing or non-existent files, directories, and/or extensions. + const genList = [ + 'tests/e2e/utils/stdioTransportClient.ts', + 'tests/e2e/__snapshots__/stdioTransport.test.ts.snap' + ]; + + // double check. This can be a list of existing or non-existent files, directories, and/or extensions. + const secList = [ + '.github', + '.gitignore', + '.npmrc', + '.sh', + 'package-lock.json', + 'src/index', + 'scripts/workflow' + ]; + + // more than needed. This can be a list of existing or non-existent files, directories, and/or extensions. + const extrasList = [ + '__fixtures__', + '__mocks__', + 'src/fixtures', + 'src/mocks' + ]; + + // agent exceptions. This can be a list of existing or non-existent files, directories, and/or extensions. + const agentList = [ + '.aiignore', + '.agents', + '.claude', + '.cursor', + '.junie', + 'guidelines/' + ]; + + try { + const isMaxFilesUpdated = typeof fileCount === 'number' ? fileCount > fileChangeLimit : undefined; + const isPrTemplateModified = typeof description === 'string' ? description.includes(prTemplateStr) === false : undefined; + + const coreModified = doesListContainAnotherListValues(files, coreList); + const isCoreModified = coreModified.length > 0; + + const genModified = doesListContainAnotherListValues(files, genList); + const isGenModified = genModified.length > 0; + + const secModified = doesListContainAnotherListValues(files, secList); + const isSecModified = secModified.length > 0; + + const extraModified = doesListContainAnotherListValues(files, extrasList); + const isExtraModified = extraModified.length > 0; + + const agentModified = doesListContainAnotherListValues(files, agentList); + const isAgentModified = agentModified.length > 0; + + // Aggregate errors + const errors = []; + + if (isMaxFilesUpdated === true) { + errors.push(`⚠️ You've updated a lot of files (${fileCount}/${fileChangeLimit}). **Resolution:** Please reduce the scope of these changes by splitting them into smaller, more focused PR contributions.`); + } + + if (isCoreModified) { + errors.push(`⚠️ I detected core file modifications. Updates to core files require an associated issue (${coreModified.join(', ')}). **Resolution:** Please link an issue in your PR description. If no issue exists your PR will be delayed for review, and may be closed.`); + } + + if (isExtraModified) { + errors.push(`⚠️ I've found extra file updates. You may be attempting to tailor the codebase to your workflow (${extraModified.join(', ')}). **Resolution:** Please align to the codebase style and remove these changes. You can also provide an explanation in your PR description but expect a delay in review.`); + } + + if (isAgentModified) { + errors.push(`⚠️ I found local agent modifications in your changes (${agentModified.join(', ')}). **Resolution:** Changes to agent guidelines require maintainer approval. Please remove these changes unless instructed otherwise.`); + } + + if (isSecModified) { + errors.push(`⚠️ I've found updates that may require a core contributor's involvement (${secModified.join(', ')}). **Resolution:** A core contributor must review these changes. You can remove these changes, or provide an explanation in your PR description and expect a delay in review.`); + } + + return { + errors, + isMaxFilesUpdated: isMaxFilesUpdated === true, + isPrTemplateModified: isPrTemplateModified === true, + isAgentModified, + isCoreModified, + isExtraModified, + isGenModified, + isSecModified, + hasFailed: false, + hasTell: isMaxFilesUpdated === true && isPrTemplateModified === true && isCoreModified && isGenModified + }; + } catch (e) { + console.error(`Workflow PreCheck signatureScan failed`, e?.message || e); + } + + return { + errors: [ + `📡 I'm calling for backup! I encountered an unexpected issue while processing your work. A maintainer has been notified.` + ], + isMaxFilesUpdated: false, + isPrTemplateModified: false, + isAgentModified: false, + isCoreModified: false, + isExtraModified: false, + isGenModified: false, + isSecModified: false, + hasFailed: true, + hasTell: false + }; +}; + +/** + * Set labels + * + * @param config + * @param config.github + * @param config.context + * @returns {{add: function(*): Promise, remove: function(*): Promise}} + */ +const setLabels = ({ github, context } = {}) => { + const { owner, repo } = context?.repo || {}; + const issueNumber = context?.issue?.number; + const addLabels = github?.rest?.issues?.addLabels; + const removeLabel = github?.rest?.issues?.removeLabel; + + return { + add: async labels => { + if (Array.isArray(labels)) { + await addLabels({ owner, repo, issue_number: issueNumber, labels }).catch(err => { + console.error(`Workflow add labels failed: ${labels.join(', ')}`, err?.message || err); + }); + } + }, + remove: async labels => { + if (Array.isArray(labels)) { + for (const label of labels) { + await removeLabel({ owner, repo, issue_number: issueNumber, name: label }).catch(err => { + console.error(`Workflow remove label failed: ${label}`, err?.message || err); + }); + } + } + } + }; +}; + +/** + * Get a comment ID from an issue number using a signature. + * + * @param signature + * @param config + * @param config.github + * @param config.context + * @returns {Promise<*>} + */ +const getCommentId = async (signature, { github, context } = {}) => { + const { owner, repo } = context?.repo || {}; + const issueNumber = context?.issue?.number; + + const listComments = github?.rest?.issues?.listComments; + let commentId; + + if (listComments && issueNumber) { + const { data: comments } = await listComments({ owner, repo, issue_number: issueNumber }) || {}; + + const foundComment = comments.find(comment => comment?.body?.includes(signature)); + + commentId = foundComment?.id; + } + + return commentId; +}; + +/** + * Set comments + * + * @param config + * @param config.signature + * @param config.github + * @param config.context + * @returns {Promise<{add: function(*): Promise<*>, remove: function(): Promise<*>, existingCommentId: *, isComment: boolean}>} + */ +const setComment = async ({ signature, github, context } = {}) => { + const { owner, repo } = context?.repo || {}; + const issueNumber = context?.issue?.number; + const createComment = github?.rest?.issues?.createComment; + const deleteComment = github?.rest?.issues?.deleteComment; + const updateComment = github?.rest?.issues?.updateComment; + + const getBody = bod => String(bod ?? '') + signature; + const commentId = await getCommentId(signature, { github, context }); + + return { + add: async body => { + if (commentId) { + return updateComment({ owner, repo, comment_id: commentId, body: getBody(body) }).catch(err => { + console.error(`Workflow update comment failed`, err?.message || err); + }); + } + + return createComment({ owner, repo, issue_number: issueNumber, body: getBody(body) }).catch(err => { + console.error(`Workflow create comment failed`, err?.message || err); + }); + }, + remove: async () => deleteComment({ owner, repo, comment_id: commentId }).catch(err => { + console.error(`Workflow remove comment failed`, err?.message || err); + }), + existingCommentId: commentId, + isComment: commentId !== undefined + }; +}; + +/** + * Convert a PR to draft. + * + * @param config + * @param config.github + * @param config.context + * @returns {Promise} + */ +const convertPrToDraft = async ({ github, context } = {}) => { + const prNodeId = context.payload.pull_request.node_id; + const mutation = `mutation($id: ID!) { + convertPullRequestToDraft(input: { pullRequestId: $id }) { + pullRequest { id isDraft } + } + }`; + + return github.graphql(mutation, { id: prNodeId }); +}; + +/** + * Get a pull request context. + * + * @param config + * @param config.github + * @param config.context + * @returns {Promise<{}|{author: *, authorType: *, authorRole: *, description: string, fileCount: *, files: *, comments: *}>} + */ +const getPullRequest = async ({ github, context } = {}) => { + try { + const { login: author, type: authorType } = context.payload.pull_request.user; + const authorRole = context.payload.pull_request.author_association; + + const description = context.payload.pull_request.body || ''; + const fileCount = context.payload.pull_request.changed_files; + const { data: files } = await github.rest.pulls.listFiles({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.issue.number, + per_page: 50 + }); + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number + }); + + return { + author, authorType, authorRole, description, fileCount, files, comments + }; + } catch (err) { + console.error('Failed to get pull request context', err?.message || err); + } + + return {}; +}; + +/** + * Start the pre-check process. + * + * @param config - Configuration params + * @param config.LABEL_NEEDS_CLEANUP - Label string + * @param config.LABEL_NEEDS_MAINTAINER - Label string + * @param config.LABEL_PRECHECKS_PASS - Label string + * @param env - Environment params + * @param env.github + * @param env.context + * @param env.core + * @returns {Promise} + */ +const start = async ({ + LABEL_NEEDS_CLEANUP, + LABEL_NEEDS_MAINTAINER, + LABEL_PRECHECKS_PASS +} = {}, { github, context, core } = {}) => { + const { author, authorType, authorRole, description: prDescription, fileCount: prFileCount, files: prFiles } = await getPullRequest({ github, context }); + const { add: addLabels, remove: removeLabels } = await setLabels({ github, context }); + + // Core contributors get a pass + if (coreContributors({ author, authorType, authorRole })) { + console.log(`Contributor found, skipping pre-checks: ${author}`); + await addLabels([LABEL_PRECHECKS_PASS]); + + return; + } + + // Signature checks found feature-like work, notify the user they may not be following guidance + const codeSignature = signatureScan({ description: prDescription, files: prFiles, fileCount: prFileCount }); + const botCommentSignature = ''; + const { add: addBotComment } = await setComment({ signature: botCommentSignature, github, context }); + + if (codeSignature.hasTell) { + const botComment = `### 🤖 PR Quality Guidance\n` + + `I've moved this to **Draft** due to the scope of changes, and to avoid confusion.\n` + + `Once you've focused your changes I'll take another look.\n\n` + + `_Read our [contribution guidelines](https://github.com/patternfly/patternfly-mcp/blob/main/CONTRIBUTING.md). This comment updates automatically._`; + + await convertPrToDraft({ github, context }); + await addBotComment(botComment); + await addLabels([LABEL_NEEDS_CLEANUP]); + + core.setFailed('PR moved to Draft. Make sure to review the contributing guidelines regarding potential feature and generated work and why your PatternFly MCP contribution may require planning.'); + + return; + } + + // Sec check + if (codeSignature.isSecModified) { + await addLabels([LABEL_NEEDS_MAINTAINER]); + } + + // Signature checks found something, alert the contributor in good faith + if (codeSignature.errors.length > 0) { + const botComment = `### 🤖 PR Quality Guidance\n` + + `I found some issues with your work. Once the following updates are addressed, you'll be queued for review:\n\n` + + `${codeSignature.errors.map(err => `- ${err}`).join('\n')}\n\n` + + `_Read our [contribution guidelines](https://github.com/patternfly/patternfly-mcp/blob/main/CONTRIBUTING.md). This comment updates automatically._`; + + await addBotComment(botComment); + await addLabels([LABEL_NEEDS_CLEANUP]); + + core.setFailed('PR pre-check requirements not met. Make sure to review the contributing guidelines.'); + + return; + } + + // Fallback if signature checks fail, alert the maintainers, non-blocking + if (codeSignature.hasFailed) { + const errorComment = `### 🤖 PR Quality Guidance\n` + + `${codeSignature.errors.map(err => `- ${err}`).join('\n')}\n\n` + + `_This comment updates automatically._`; + + await addBotComment(errorComment); + await addLabels([LABEL_NEEDS_MAINTAINER]); + } else { + // Or confirm the work has passed pre-check + const successComment = `### 🤖 PR Quality Guidance\n` + + `I finished my scan and all pre-checks pass!\n\n` + + `_Read our [contribution guidelines](https://github.com/patternfly/patternfly-mcp/blob/main/CONTRIBUTING.md). This comment updates automatically._`; + + await addBotComment(successComment); + await addLabels([LABEL_PRECHECKS_PASS]); + await removeLabels([LABEL_NEEDS_CLEANUP, LABEL_NEEDS_MAINTAINER]); + } +}; + +export { + coreContributors, + doesListContainAnotherListValues, + getCommentId, + getPullRequest, + setComment, + setLabels, + signatureScan, + start +}; diff --git a/tests/scripts/__snapshots__/workflow.preCheck.test.ts.snap b/tests/scripts/__snapshots__/workflow.preCheck.test.ts.snap new file mode 100644 index 00000000..bf02205e --- /dev/null +++ b/tests/scripts/__snapshots__/workflow.preCheck.test.ts.snap @@ -0,0 +1,150 @@ +// Jest Snapshot v1, https://jestjs.io/docs/snapshot-testing + +exports[`getPullRequest should resolve and aggregate PR metadata and resources: pr 1`] = ` +{ + "author": "author1", + "authorRole": "MEMBER", + "authorType": "User", + "comments": [ + "comment1", + ], + "description": "description", + "fileCount": 10, + "files": [ + "file1", + ], +} +`; + +exports[`signatureScan should scan for signatures, "The Tell" 1`] = ` +{ + "errors": [ + "⚠️ You've updated a lot of files (20/15). **Resolution:** Please reduce the scope of these changes by splitting them into smaller, more focused PR contributions.", + "⚠️ I detected core file modifications. Updates to core files require an associated issue (src/server.ts, tests/e2e/__snapshots__/stdioTransport.test.ts.snap). **Resolution:** Please link an issue in your PR description. If no issue exists your PR will be delayed for review, and may be closed.", + ], + "hasFailed": false, + "hasTell": true, + "isAgentModified": false, + "isCoreModified": true, + "isExtraModified": false, + "isGenModified": true, + "isMaxFilesUpdated": true, + "isPrTemplateModified": true, + "isSecModified": false, +} +`; + +exports[`signatureScan should scan for signatures, agent modifications 1`] = ` +{ + "errors": [ + "⚠️ I found local agent modifications in your changes (.aiignore). **Resolution:** Changes to agent guidelines require maintainer approval. Please remove these changes unless instructed otherwise.", + ], + "hasFailed": false, + "hasTell": false, + "isAgentModified": true, + "isCoreModified": false, + "isExtraModified": false, + "isGenModified": false, + "isMaxFilesUpdated": false, + "isPrTemplateModified": false, + "isSecModified": false, +} +`; + +exports[`signatureScan should scan for signatures, core modifications 1`] = ` +{ + "errors": [ + "⚠️ I detected core file modifications. Updates to core files require an associated issue (src/server.ts). **Resolution:** Please link an issue in your PR description. If no issue exists your PR will be delayed for review, and may be closed.", + ], + "hasFailed": false, + "hasTell": false, + "isAgentModified": false, + "isCoreModified": true, + "isExtraModified": false, + "isGenModified": false, + "isMaxFilesUpdated": false, + "isPrTemplateModified": false, + "isSecModified": false, +} +`; + +exports[`signatureScan should scan for signatures, extra/generated files 1`] = ` +{ + "errors": [ + "⚠️ I've found extra file updates. You may be attempting to tailor the codebase to your workflow (scripts/__mocks__). **Resolution:** Please align to the codebase style and remove these changes. You can also provide an explanation in your PR description but expect a delay in review.", + ], + "hasFailed": false, + "hasTell": false, + "isAgentModified": false, + "isCoreModified": false, + "isExtraModified": true, + "isGenModified": false, + "isMaxFilesUpdated": false, + "isPrTemplateModified": false, + "isSecModified": false, +} +`; + +exports[`signatureScan should scan for signatures, missing template metadata 1`] = ` +{ + "errors": [], + "hasFailed": false, + "hasTell": false, + "isAgentModified": false, + "isCoreModified": false, + "isExtraModified": false, + "isGenModified": false, + "isMaxFilesUpdated": false, + "isPrTemplateModified": true, + "isSecModified": false, +} +`; + +exports[`signatureScan should scan for signatures, security modifications 1`] = ` +{ + "errors": [ + "⚠️ I've found updates that may require a core contributor's involvement (.github/workflows/pr_precheck.yml). **Resolution:** A core contributor must review these changes. You can remove these changes, or provide an explanation in your PR description and expect a delay in review.", + ], + "hasFailed": false, + "hasTell": false, + "isAgentModified": false, + "isCoreModified": false, + "isExtraModified": false, + "isGenModified": false, + "isMaxFilesUpdated": false, + "isPrTemplateModified": false, + "isSecModified": true, +} +`; + +exports[`signatureScan should scan for signatures, too many files 1`] = ` +{ + "errors": [ + "⚠️ You've updated a lot of files (16/15). **Resolution:** Please reduce the scope of these changes by splitting them into smaller, more focused PR contributions.", + ], + "hasFailed": false, + "hasTell": false, + "isAgentModified": false, + "isCoreModified": false, + "isExtraModified": false, + "isGenModified": false, + "isMaxFilesUpdated": true, + "isPrTemplateModified": false, + "isSecModified": false, +} +`; + +exports[`signatureScan should scan for signatures, valid PR 1`] = ` +{ + "errors": [], + "hasFailed": false, + "hasTell": false, + "isAgentModified": false, + "isCoreModified": false, + "isExtraModified": false, + "isGenModified": false, + "isMaxFilesUpdated": false, + "isPrTemplateModified": false, + "isSecModified": false, +} +`; diff --git a/tests/scripts/workflow.commitLint.test.ts b/tests/scripts/workflow.commitLint.test.ts new file mode 100644 index 00000000..4f6158db --- /dev/null +++ b/tests/scripts/workflow.commitLint.test.ts @@ -0,0 +1,273 @@ +import { messagesList, MESSAGE_TYPES, parseCommitMessage, start } from '../../scripts/workflow.commitLint.js'; + +describe('parseCommitMessage', () => { + it.each([ + { + description: 'standard commit with type, scope, and PR', + message: 'feat(core): add something (#123)', + expected: { + type: 'feat', + scope: 'core', + description: 'add something', + prNumber: '123' + } + }, + { + description: 'multiple colons in description (should not mutate description)', + message: 'feat: check: missing colon (#789)', + expected: { + type: 'feat', + description: 'check: missing colon', + prNumber: '789' + } + }, + { + description: 'greedy PR extraction (should not merge description numbers)', + message: 'fix: resolve 500 error (#123)', + expected: { + type: 'fix', + description: 'resolve 500 error', + prNumber: '123' + } + }, + { + description: 'breaking change with bang', + message: 'feat(ui)!: breaking change', + expected: { + type: 'feat', + isBreaking: true, + description: 'breaking change' + } + }, + { + description: 'fallback when type is invalid', + message: 'unknown: some message', + expected: { + type: undefined, + description: 'unknown: some message' + } + }, + { + description: 'issue number parsing', + message: 'feat(ui)!: issues/123 breaking change', + expected: { + type: 'feat', + description: 'issues/123 breaking change', + issueNumber: 'issues/123' + } + }, + { + description: 'issue number parsing, misplaced', + message: 'feat(ui): a change issues/123', + settings: { allowIssuesAnywhere: false }, + expected: { + type: 'feat', + description: 'a change issues/123', + issueNumber: undefined + } + }, + { + description: 'issue number parsing, jira', + message: 'feat(ui): jira-12345 a change', + expected: { + type: 'feat', + description: 'jira-12345 a change', + issueNumber: 'jira-12345' + } + } + ])('should parse $description', ({ message, settings, expected }) => { + const result = parseCommitMessage({ hash: 'abc1234', message }, { messageTypes: MESSAGE_TYPES, ...settings } as any); + + expect(result).toMatchObject(expected); + }); +}); + +describe('messagesList', () => { + it.each([ + { + description: 'valid standard commit', + parsed: [{ + type: 'feat', + scope: 'any', + description: 'JIRA-123 add feature', + issueNumber: 'JIRA-123', + messageLength: 30, + hash: 'abc1234', + message: 'feat(any): JIRA-123 add feature' + }], + options: { typeScopeExceptions: [], issueNumberExceptions: [] }, + expected: { + type: 'valid', + issueNumber: 'valid' + } + }, + { + description: 'validation masking show issue number error even if type is invalid', + parsed: [{ + type: undefined, + scope: 'any', + description: 'no issue here', + messageLength: 30, + hash: 'abc1234', + message: 'foo(any): no issue here' + }], + options: { typeScopeExceptions: [], issueNumberExceptions: ['feat'] }, + expected: { + type: expect.stringContaining('INVALID: type'), + issueNumber: expect.stringContaining('INVALID: issue number') + } + }, + { + description: 'message length validation', + parsed: [{ + type: 'feat', + description: 'very long description', + messageLength: 100, + hash: 'abc1234', + message: 'feat: very long description' + }], + options: { maxMessageLength: 50 }, + expected: { + length: 'INVALID: message length (100 > 50).' + } + }, + { + description: 'message length validation, pr and issue number', + parsed: [{ + type: 'feat', + description: 'issues/123 very long description', + messageLength: 100, + hash: 'abc1234', + message: 'feat: issues/123 very long description (#123)', + issueNumber: 'issues/123', + prNumber: '123' + }], + options: { maxMessageLength: 50 }, + expected: { + length: 'INVALID: message length (100 > 50). PRs do not count towards message length. Issue numbers do not count towards message length.' + } + }, + { + description: 'typeScopeExceptions using wildcard', + parsed: [{ + type: 'feat', + scope: undefined, + description: 'JIRA-123 desc', + messageLength: 20, + hash: 'abc1234' + }], + options: { typeScopeExceptions: '*' }, + expected: { + scope: 'valid' + } + } + ])('should validate $description', ({ parsed, options, expected }) => { + const results = messagesList(parsed, options as any); + + expect(results[0]).toMatchObject(expected); + }); +}); + +describe('start', () => { + const options = { + allowIssuesAnywhere: false, + issueNumberExceptions: [], + maxMessageLength: 65, + typeScopeExceptions: '*' + }; + + it.each([ + { + description: 'valid commits', + commits: [ + { sha: 'abcdef123456', commit: { message: 'feat(core): JIRA-123 add something (#1)' } }, + { sha: '123456abcdef', commit: { message: 'fix: JIRA-456 resolve bug (#2)' } } + ], + options, + expected: { + resultsArray: [], + resultsString: '[]' + } + }, + { + description: 'mixed valid and invalid commits', + commits: [ + { sha: 'abcdef123456', commit: { message: 'feat(core): JIRA-123 add something' } }, + { sha: '111111222222', commit: { message: 'invalid message format' } } + ], + options, + expected: { + resultsArray: [ + { + hash: '1111112', + commit: 'invalid message format', + type: expect.stringContaining('INVALID: type'), + issueNumber: expect.stringContaining('INVALID: issue number') + } + ] + } + }, + { + description: 'enforcing max message length with metadata bypass', + commits: [ + { + sha: 'abcdef123456', + commit: { message: 'feat: JIRA-123 a very long description that exceeds the normal limit (#1)' } + } + ], + options: { ...options, maxMessageLength: 10 }, + expected: { + resultsArray: [ + { + hash: 'abcdef1', + length: expect.stringContaining('INVALID: message length') + } + ] + } + }, + { + description: 'handling allowIssuesAnywhere toggle', + commits: [ + { sha: 'abcdef123456', commit: { message: 'feat: add feature (JIRA-123)' } } + ], + options, + expected: { + resultsArray: [ + { + hash: 'abcdef1', + issueNumber: expect.stringContaining('INVALID: issue number') + } + ] + } + }, + { + description: 'respecting issueNumberExceptions', + commits: [ + { sha: 'abcdef123456', commit: { message: 'chore: cleanup code' } } + ], + options: { ...options, issueNumberExceptions: ['chore'] }, + expected: { + resultsArray: [], + resultsString: '[]' + } + }, + { + description: 'empty or missing commits', + commits: null, + options, + expected: { + resultsArray: [], + resultsString: '' + } + } + ])('should handle $description', ({ commits, options, expected }) => { + const result = start(commits as any, options as any); + + if (expected.resultsArray) { + expect(result.resultsArray).toMatchObject(expected.resultsArray); + } + if (expected.resultsString !== undefined) { + expect(result.resultsString).toBe(expected.resultsString); + } + }); +}); diff --git a/tests/scripts/workflow.preCheck.test.ts b/tests/scripts/workflow.preCheck.test.ts new file mode 100644 index 00000000..7caddb7a --- /dev/null +++ b/tests/scripts/workflow.preCheck.test.ts @@ -0,0 +1,576 @@ +import fs from 'node:fs'; +import { jest } from '@jest/globals'; +import { + coreContributors, + doesListContainAnotherListValues, + signatureScan, + getCommentId, + getPullRequest, + setLabels, + setComment, + start +} from '../../scripts/workflow.preCheck.js'; + +describe('coreContributors', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it.each([ + { + description: 'dependabot', + params: { + authorType: 'Bot', + author: 'dependabot[bot]' + }, + expected: true + }, + { + description: 'dependabot, user', + params: { + authorType: 'User', + author: 'dependabot[bot]' + }, + expected: true + }, + { + description: 'other bot', + params: { + authorType: 'Bot', + author: 'lorem-ipsum-bot' + }, + expected: true + }, + { + description: 'general user', + params: { + authorType: 'User', + author: 'lorem-ipsum-user' + }, + expected: false + }, + { + description: 'owner', + params: { + authorRole: 'OWNER', + author: 'dolor-sit-user' + }, + expected: true + }, + { + description: 'member', + params: { + authorRole: 'MEMBER', + author: 'dolor-sit-user' + }, + expected: false + }, + { + description: 'collaborator', + params: { + authorRole: 'COLLABORATOR', + author: 'dolor-sit-user' + }, + expected: false + }, + { + description: 'contributor', + params: { + authorRole: 'CONTRIBUTOR', + author: 'dolor-sit-user' + }, + expected: false + } + ])('should handle actual authors, $description', ({ params, expected }) => { + // This is a live test against the actual CODEOWNERS file + const result = coreContributors(params); + + expect(result).toBe(expected); + }); + + it.each([ + { + description: 'codeowner, owner', + account: '@lorem', + params: { + author: 'lorem', authorRole: 'OWNER', authorType: 'User' + }, + expected: true + }, + { + description: 'owner', + account: undefined, + params: { + author: 'lorem', authorRole: 'OWNER', authorType: 'User' + }, + expected: true + }, + { + description: 'codeowner, member', + account: '@lorem', + params: { + author: 'lorem', authorRole: 'MEMBER', authorType: 'User' + }, + expected: true + }, + { + description: 'member', + account: undefined, + params: { + author: 'lorem', authorRole: 'MEMBER', authorType: 'User' + }, + expected: false + }, + { + description: 'codeowner with comma delimiter', + account: '@other, @lorem', + params: { + author: 'lorem', authorRole: 'MEMBER', authorType: 'User' + }, + expected: true + }, + { + description: 'codeowner with parentheses', + account: '(@lorem) @other', + params: { + author: 'lorem', authorRole: 'MEMBER', authorType: 'User' + }, + expected: true + }, + { + description: 'prevent partial name match (suffix)', + account: '@lorem-suffix', + params: { + author: 'lorem', authorRole: 'MEMBER', authorType: 'User' + }, + expected: false + }, + { + description: 'prevent partial name match (prefix)', + account: 'prefix-@lorem', + params: { + author: 'lorem', authorRole: 'MEMBER', authorType: 'User' + }, + expected: false + } + ])('should verify simulated authors against a simulated CODEOWNERS file, $description', ({ account, params, expected }) => { + jest.spyOn(fs, 'existsSync').mockReturnValue(true); + const mockReadFileSyncSpy = jest.spyOn(fs, 'readFileSync').mockReturnValue(account ? `package*.json ${account} @dolor-sit` : 'package*.json @dolor-sit'); + const result = coreContributors(params); + + expect(result).toBe(expected); + expect(mockReadFileSyncSpy).toHaveBeenCalledWith(expect.stringContaining('CODEOWNERS'), 'utf8'); + }); +}); + +describe('doesListContainAnotherListValues', () => { + it.each([ + { + description: 'empty lists', + listBase: [], + listCheck: ['src/'], + expected: [] + }, + { + description: 'exact match', + listBase: [{ filename: 'src/server.ts' }], + listCheck: ['src/server.ts'], + expected: ['src/server.ts'] + }, + { + description: 'case insensitivity', + listBase: [{ filename: 'src/SERVER.ts' }], + listCheck: ['SRC/server.TS'], + expected: ['src/SERVER.ts'] + }, + { + description: 'prefix/directory match', + listBase: [{ filename: 'src/cli.ts' }], + listCheck: ['src/cli'], + expected: ['src/cli.ts'] + }, + { + description: 'contains match', + listBase: [{ filename: 'some/path/scripts/workflow.script.js' }], + listCheck: ['scripts/workflow'], + expected: ['some/path/scripts/workflow.script.js'] + }, + { + description: 'multiple matches', + listBase: [ + { filename: 'src/server.ts' }, + { filename: 'tests/e2e/test.ts' } + ], + listCheck: ['src/', 'tests/e2e'], + expected: ['src/server.ts', 'tests/e2e/test.ts'] + }, + { + description: 'no match', + listBase: [{ filename: 'README.md' }], + listCheck: ['src/'], + expected: [] + } + ])('should verify list containment, $description', ({ listBase, listCheck, expected }) => { + const result = doesListContainAnotherListValues(listBase, listCheck); + + expect(result).toEqual(expected); + }); +}); + +describe('signatureScan', () => { + it.each([ + { + description: 'valid PR', + params: { + description: '', + files: [{ filename: 'src/patternfly.ts' }], + fileCount: 1 + }, + expected: 0 + }, + { + description: 'too many files', + params: { + description: '', + files: [], + fileCount: 16 + }, + expected: 1 + }, + { + description: 'missing template metadata', + params: { + description: 'Just a PR description', + files: [], + fileCount: 1 + }, + expected: 0 + }, + { + description: 'core modifications', + params: { + description: '', + files: [{ filename: 'src/server.ts' }], + fileCount: 1 + }, + expected: 1 + }, + { + description: 'security modifications', + params: { + description: '', + files: [{ filename: '.github/workflows/pr_precheck.yml' }], + fileCount: 1 + }, + expected: 1 + }, + { + description: 'agent modifications', + params: { + description: '', + files: [{ filename: '.aiignore' }], + fileCount: 1 + }, + expected: 1 + }, + { + description: 'extra/generated files', + params: { + description: '', + files: [{ filename: 'scripts/__mocks__' }], + fileCount: 1 + }, + expected: 1 + }, + { + description: '"The Tell"', + params: { + description: 'Missing metadata', + files: [ + { filename: 'src/server.ts' }, + { filename: 'tests/e2e/__snapshots__/stdioTransport.test.ts.snap' } + ], + fileCount: 20 + }, + expected: 2 + } + ])('should scan for signatures, $description', ({ params, expected }) => { + const result = signatureScan(params); + + expect(result.errors.length).toBe(expected); + expect(result).toMatchSnapshot(); + }); +}); + +describe('getCommentId', () => { + it('should return the ID of a comment matching the signature', async () => { + const github = { + rest: { + issues: { + listComments: jest.fn().mockResolvedValue({ + data: [ + { id: 101, body: 'some other comment' }, + { id: 202, body: 'matching signature ' } + ] + }) + } + } + }; + const context = { repo: { owner: 'lorem', repo: 'ipsum' }, issue: { number: 1 } }; + const id = await getCommentId('', { github, context }); + + expect(id).toBe(202); + }); +}); + +describe('getPullRequest', () => { + it('should resolve and aggregate PR metadata and resources', async () => { + const github = { + rest: { + pulls: { listFiles: jest.fn().mockResolvedValue({ data: ['file1'] }) }, + issues: { listComments: jest.fn().mockResolvedValue({ data: ['comment1'] }) } + } + }; + const context = { + payload: { + pull_request: { + user: { login: 'author1', type: 'User' }, + author_association: 'MEMBER', + body: 'description', + changed_files: 10 + } + }, + repo: { owner: 'lorem', repo: 'ipsum' }, + issue: { number: 1 } + }; + + const result = await getPullRequest({ github, context }); + + expect(result).toMatchSnapshot('pr'); + }); +}); + +describe('setLabels', () => { + it('should provide methods to add and remove labels', async () => { + const github = { + rest: { + issues: { + addLabels: jest.fn().mockResolvedValue({}), + removeLabel: jest.fn().mockResolvedValue({}) + } + } + }; + const context = { repo: { owner: 'lorem', repo: 'ipsum' }, issue: { number: 1 } } as any; + + const labels = setLabels({ github, context }); + + await labels.add(['label-a']); + await labels.remove(['label-b']); + + expect(github.rest.issues.addLabels).toHaveBeenCalled(); + expect(github.rest.issues.removeLabel).toHaveBeenCalled(); + }); +}); + +describe('setComment', () => { + it('should update an existing comment if a signature match is found', async () => { + const github = { + rest: { + issues: { + createComment: jest.fn().mockResolvedValue({}), + listComments: jest.fn().mockResolvedValue({ + data: [{ id: 500, body: 'matching signature ' }] + }), + updateComment: jest.fn().mockResolvedValue({}) + } + } + }; + const context = { repo: { owner: 'lorem', repo: 'ipsum' }, issue: { number: 1 } }; + const comment = await setComment({ signature: '', github, context }); + + await comment.add('new body'); + + expect(github.rest.issues.updateComment).toHaveBeenCalledWith(expect.objectContaining({ + comment_id: 500, + body: 'new body' + })); + expect(github.rest.issues.createComment).not.toHaveBeenCalled(); + }); + + it('should create a new comment if no signature match is found', async () => { + const github = { + rest: { + issues: { + createComment: jest.fn().mockResolvedValue({}), + listComments: jest.fn().mockResolvedValue({ data: [] }), + updateComment: jest.fn().mockResolvedValue({}) + } + } + }; + const context = { repo: { owner: 'lorem', repo: 'ipsum' }, issue: { number: 1 } }; + const comment = await setComment({ signature: '', github, context }); + + await comment.add('new body'); + + expect(github.rest.issues.createComment).toHaveBeenCalledWith(expect.objectContaining({ + issue_number: 1, + body: 'new body' + })); + expect(github.rest.issues.updateComment).not.toHaveBeenCalled(); + }); + + it('should remove a comment if a signature match is found', async () => { + const github = { + rest: { + issues: { + listComments: jest.fn().mockResolvedValue({ data: [{ id: 500, body: '' }] }), + deleteComment: jest.fn().mockResolvedValue({}) + } + } + }; + const context = { repo: { owner: 'lorem', repo: 'ipsum' }, issue: { number: 1 } }; + const comment = await setComment({ signature: '', github, context }); + + await comment.remove(); + + expect(github.rest.issues.deleteComment).toHaveBeenCalledWith(expect.objectContaining({ + comment_id: 500 + })); + }); +}); + +describe('start', () => { + let github: any; + let context: any; + let core: any; + let config: any; + + beforeEach(() => { + github = { + rest: { + pulls: { listFiles: jest.fn().mockResolvedValue({ data: [] }) }, + issues: { + listComments: jest.fn().mockResolvedValue({ data: [] }), + addLabels: jest.fn().mockResolvedValue({}), + removeLabel: jest.fn().mockResolvedValue({}), + createComment: jest.fn().mockResolvedValue({}), + updateComment: jest.fn().mockResolvedValue({}), + deleteComment: jest.fn().mockResolvedValue({}) + } + }, + graphql: jest.fn().mockResolvedValue({}) + }; + context = { + payload: { + pull_request: { + user: { login: 'contributor', type: 'User' }, + author_association: 'CONTRIBUTOR', + body: '', + changed_files: 1, + labels: [], + node_id: 'PR_123' + } + }, + repo: { owner: 'o', repo: 'r' }, + issue: { number: 123 } + }; + core = { setFailed: jest.fn(), log: jest.fn() }; + config = { + LABEL_PRECHECKS_PASS: 'bot:policy-ready', + LABEL_NEEDS_CLEANUP: 'bot:needs-cleanup', + LABEL_NEEDS_MAINTAINER: 'bot:needs-maintainer' + }; + }); + + it('should immediately apply the pass label for core contributors', async () => { + // 1. Simulate an OWNER opening a PR + context.payload.pull_request.author_association = 'OWNER'; + + await start(config, { github, context, core }); + + // 2. Verify that it applied the pass label and skipped further checks + expect(github.rest.issues.addLabels).toHaveBeenCalledWith(expect.objectContaining({ + labels: [config.LABEL_PRECHECKS_PASS] + })); + + expect(github.rest.issues.createComment).not.toHaveBeenCalled(); + expect(core.setFailed).not.toHaveBeenCalled(); + }); + + it('should convert PR to draft if signature scan finds complex changes (hasTell)', async () => { + // 1. Mock fileCount and description to trigger hasTell + context.payload.pull_request.changed_files = 100; + context.payload.pull_request.body = 'Missing template signature'; + github.rest.pulls.listFiles.mockResolvedValue({ + data: [{ filename: 'src/server.ts' }, { filename: 'tests/e2e/utils/stdioTransportClient.ts' }] + }); + + await start(config, { github, context, core }); + + // 2. Verify draft conversion and labeling + expect(github.graphql).toHaveBeenCalledWith(expect.stringContaining('convertPullRequestToDraft'), expect.objectContaining({ id: 'PR_123' })); + + expect(github.rest.issues.addLabels).toHaveBeenCalledWith(expect.objectContaining({ + labels: [config.LABEL_NEEDS_CLEANUP] + })); + + expect(github.rest.issues.createComment).toHaveBeenCalledWith(expect.objectContaining({ + body: expect.stringContaining("I've moved this to **Draft**") + })); + + expect(core.setFailed).toHaveBeenCalledWith(expect.stringContaining('PR moved to Draft')); + }); + + it('should apply needs-cleanup label and fail if scan find errors', async () => { + // 1. Mock fileCount to trigger the error list but not hasTell + context.payload.pull_request.changed_files = 20; + + await start(config, { github, context, core }); + + // 2. Verify cleanup label and error comment + expect(github.rest.issues.addLabels).toHaveBeenCalledWith(expect.objectContaining({ + labels: [config.LABEL_NEEDS_CLEANUP] + })); + + expect(github.rest.issues.createComment).toHaveBeenCalledWith(expect.objectContaining({ + body: expect.stringContaining('I found some issues with your work') + })); + + expect(core.setFailed).toHaveBeenCalledWith(expect.stringContaining('PR pre-check requirements not met')); + }); + + it('should apply needs-maintainer label for security-sensitive changes', async () => { + github.rest.pulls.listFiles.mockResolvedValue({ + data: [{ filename: '.github/workflows/integration.yml' }] + }); + + await start(config, { github, context, core }); + + expect(github.rest.issues.addLabels).toHaveBeenCalledWith(expect.objectContaining({ + labels: [config.LABEL_NEEDS_MAINTAINER] + })); + }); + + it('should notify success and apply pass label when all pre-checks pass', async () => { + await start(config, { github, context, core }); + + // 1. Verify Success Notification + expect(github.rest.issues.createComment).toHaveBeenCalledWith(expect.objectContaining({ + body: expect.stringContaining('I finished my scan and all pre-checks pass!') + })); + + // 2. Verify Labeling + expect(github.rest.issues.addLabels).toHaveBeenCalledWith(expect.objectContaining({ + labels: [config.LABEL_PRECHECKS_PASS] + })); + + // 3. Verify Label Cleanup + const removedLabels = github.rest.issues.removeLabel.mock.calls.map((call: any) => call[0].name); + + expect(removedLabels).toContain(config.LABEL_NEEDS_CLEANUP); + expect(removedLabels).toContain(config.LABEL_NEEDS_MAINTAINER); + + // 4. Verify no failure was triggered + expect(core.setFailed).not.toHaveBeenCalled(); + }); +}); diff --git a/tsconfig.json b/tsconfig.json index c874d625..dc81a59c 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -21,7 +21,7 @@ "resolveJsonModule": true, "noEmit": true, "stripInternal": true, - "rootDirs": ["./src", "./tests/e2e", "./tests/audit"], + "rootDirs": ["./src", "./tests/e2e", "./tests/audit", "./scripts", "./tests/scripts"], "paths": { "#docsCatalog": ["./src/docs.json"], "#toolsHost": ["./src/server.toolsHost.ts"] @@ -29,6 +29,7 @@ }, "include": [ "src/**/*", + "scripts/**/*", "tests/**/*", "jest.setupTests.ts", "jest.config.ts", From 5123d5d1d2a176b083a0f9123b06873df033defe Mon Sep 17 00:00:00 2001 From: CD Cabrera Date: Mon, 11 May 2026 16:24:32 -0400 Subject: [PATCH 2/2] docs(contributing): pf-4128 automation, testing standards * agent, behaviors, testing to align with policy * contributions, clarify contributor guidelines * docs, recent architecture adds, index for policy * policy, governance, security policies --- .github/ISSUE_TEMPLATE/bug_report.md | 1 + .github/ISSUE_TEMPLATE/feature_request.md | 2 + .github/PULL_REQUEST_TEMPLATE.md | 32 +++---- CONTRIBUTING.md | 103 +++++++++++++++++++--- GOVERNANCE.md | 35 ++++++++ SECURITY.md | 35 ++++++++ cspell.config.json | 5 +- docs/README.md | 5 ++ docs/architecture.md | 9 ++ guidelines/README.md | 1 + guidelines/agent_behaviors.md | 21 ++++- guidelines/agent_testing.md | 29 +++++- package.json | 2 +- 13 files changed, 245 insertions(+), 35 deletions(-) create mode 100644 GOVERNANCE.md create mode 100644 SECURITY.md diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 62e253c7..593e1755 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -6,6 +6,7 @@ about: Create a report to help us improve ## Description + ### Expected behavior diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md index e5dc2e1c..031d2ad7 100644 --- a/.github/ISSUE_TEMPLATE/feature_request.md +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -6,6 +6,8 @@ about: Suggest an idea for this project ## Description + + As user X, I want Y to happen so that Z. ## Acceptance criteria diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index c5757246..46e217ed 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,27 +1,19 @@ - + + ## What is it? -... ### Notes - ... - +## How to test - +... + + + + + +