-
-
Notifications
You must be signed in to change notification settings - Fork 8
Add legal boilerplate validation for external contributors #145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: vaind <6349682+vaind@users.noreply.github.com>
Co-authored-by: vaind <6349682+vaind@users.noreply.github.com>
|
@copilot as usual, this needs tests... |
Co-authored-by: vaind <6349682+vaind@users.noreply.github.com>
Added comprehensive tests in commit 24ffc9f. The
All 15 new tests pass, and existing tests remain unaffected. |
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Add legal boilerplate validation for external contributors ([#145](https://github.com/getsentry/github-workflows/pull/145))If none of the above apply, you can opt out of this check by adding |
vaind
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review: Legal Boilerplate Validation for External Contributors
Files changed: 4 (+393, -2)
Overview
This PR adds a Danger check that validates external contributors (non-org members) include the "Legal Boilerplate" section from the repository's PR template in their PR description. It auto-detects whether the repo's PR template contains such a section, making it opt-in without configuration.
Positive Aspects
- Good architecture: The pure extraction function (
extractLegalBoilerplateSection) is separated intodangerfile-utils.jsfor testability, following existing patterns. - Solid test coverage: 15 test cases covering header levels, case insensitivity, special characters, edge cases, etc.
- Graceful degradation: Skips silently for org members, repos without templates, or templates without a legal section.
- Security awareness: Template content is wrapped in code blocks to prevent markdown injection.
Issues and Suggestions
1. Bug: fileContents returns empty string, not an exception, on missing files
The code wraps danger.github.utils.fileContents() in try/catch expecting it to throw when a file doesn't exist. However, the Danger fileContents utility returns an empty string for missing files — it doesn't throw. This means the loop will stop at the first template path attempted (hit or miss) since no exception will be thrown.
// Current (broken):
try {
prTemplateContent = await danger.github.utils.fileContents(templatePath);
break;
} catch (error) {
continue;
}Fix: Check for a truthy/non-empty return value instead of catching exceptions:
const content = await danger.github.utils.fileContents(templatePath);
if (content) {
prTemplateContent = content;
break;
}2. Bug: Org membership check may silently pass for non-org repos
The checkMembershipForUser API uses orgs.checkMembershipForUser, which returns 204 for active members and 302 (redirect) for non-members. However, the Octokit client may also return 302 as a non-error status, meaning non-members could be incorrectly treated as members. The author_association field on the PR object (danger.github.pr.author_association) would be a simpler and more reliable signal — values like "MEMBER", "OWNER", or "CONTRIBUTOR" vs "NONE" or "FIRST_TIME_CONTRIBUTOR".
3. Missing: No integration test for checkLegalBoilerplate itself
The tests only cover the pure extractLegalBoilerplateSection function. The main checkLegalBoilerplate() function — which contains the org membership check, template discovery, and validation logic — has no tests.
4. Validation only checks for header presence, not content
The check only verifies the PR body contains a "Legal Boilerplate" header. A contributor could add ### Legal Boilerplate with no actual legal text below it and pass the check. Consider also verifying the actual boilerplate text (or a significant substring like "Functional Software, Inc.") is present.
5. Test file placement
The new test file is dangerfile-legal-boilerplate.test.js, but it only tests functions from dangerfile-utils.js. Consider either adding these tests to the existing dangerfile-utils.test.js, or renaming to dangerfile-utils-legal-boilerplate.test.js to clarify it tests utility functions.
6. Minor: undefined arguments to fail()
fail(
'This PR is missing...',
undefined,
undefined
);The undefined arguments are unnecessary — fail() accepts just the message. Other calls in this file only pass the message and file.
Summary
The overall approach is sound and addresses the issue well. The main concerns are the fileContents bug (#1), which will cause template discovery to fail silently in practice, and the org membership detection (#2) which should be revisited for reliability. The remaining items are improvements that would strengthen the implementation.
@copilot address this
- Fix fileContents bug: check for non-empty string instead of catching exceptions - Use author_association field instead of org membership API for reliability - Validate boilerplate content (min 50 chars) not just header presence - Merge tests into dangerfile-utils.test.js following existing patterns - Remove unnecessary undefined arguments from fail() calls All 35 tests pass (20 existing + 15 legal boilerplate tests) Co-authored-by: vaind <6349682+vaind@users.noreply.github.com>
Addressed all issues in commit 71f5985:
The implementation is now more robust and follows best practices. |
vaind
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix the issue bellow and try to simplify the code, seems excesive
…ry length Replace arbitrary 50-character check with actual content comparison. Now validates that the exact legal boilerplate from the template is present in the PR description by normalizing whitespace and comparing content. Simplified logic removes unnecessary complexity and ensures the actual legal text stays intact, not just that something exists. Co-authored-by: vaind <6349682+vaind@users.noreply.github.com>
Simplified in commit 791297a. The code now:
All 35 tests still pass. |
Extract checkLegalBoilerplate() from dangerfile.js into dangerfile-utils.js
with dependency injection (accepts { danger, fail, markdown } as parameters)
so the orchestration logic can be tested without the DangerJS runtime.
Adds 19 tests covering author association filtering, template discovery,
missing/mismatched boilerplate detection, success cases, and markdown output.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Plan: Add Legal Boilerplate Validation for External Contributors
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.