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 - +... + + + + + + '; + + // 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",