Repository navigation
Create change-management-check composite action - #1712
Conversation
Fails the check when a Testing evidence, Success / failure criteria, or Rollback plan section in the PR body is empty or missing. Exempt: Renovate branches, docs-only PRs, dev-only PRs.
🦋 Changeset is good to goLatest commit: e2aba6b We got this. Not sure what this means? Click here to learn what changesets are. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Strip HTML comments with an indexOf loop instead of a regex (CodeQL js/incomplete-multi-character-sanitization). An unterminated comment now drops the rest of the body, matching how GitHub renders it. Call core.summary.write(); addRaw alone leaves the buffer unwritten. Treat an explicit empty docs-paths or head-prefixes input as no exemption instead of falling back to a default.
Same matching behavior, one loop, no index arithmetic. Adds a test for the ? token.
Replace the token loop with a replace() over an ordered token regex and a lookup map. Same behavior, fewer lines.
Adds the Standard link, a copy-paste workflow, the inputs table, the exemption rules, and the known limitations.
The old heading regex backtracked super-linearly on a hostile body line: 2.5 s for a 4 KB line, hours extrapolated for 64 KB. The new one is linear and handles CommonMark closing hashes. A section now ends at a heading of the same or higher level, or at a configured alias, so subheadings no longer read as empty.
Avoids the 0.0.0 placeholder. The major changeset still takes the first release to 1.0.0.
| async function listFiles(github, context) { | ||
| const { owner, repo } = context.repo; | ||
| const pull_number = context.payload.pull_request.number; | ||
| const files = []; | ||
| // The API caps at 3000 files; 30 pages of 100. | ||
| for (let page = 1; page <= 30; page++) { | ||
| const { data } = await github.rest.pulls.listFiles({ | ||
| owner, | ||
| repo, | ||
| pull_number, | ||
| per_page: 100, | ||
| page, | ||
| }); | ||
| files.push(...data.map((file) => file.filename)); | ||
| if (data.length < 100) break; | ||
| } | ||
| return files; | ||
| } |
There was a problem hiding this comment.
I think you're better off doing something different than pulling modified files, and hardcoding paths to ignore.
My personal recommendation would be have this action check the PR description, and use some other dedicated action to determine path changes in the workflow: https://github.com/dorny/paths-filter
This action could then be skipped at the workflow level if path changes aren't included.
This is a matter of opinion though, so happy to approve if you'd still prefer it done as part of this action.
There was a problem hiding this comment.
Done, thanks. The action now checks only the PR body; it reads the body from the event payload and nothing else. The workflow decides applicability: dorny/paths-filter for the docs and dev paths, and a step condition for the renovate prefix. The docs-paths, dev-paths, and head-prefixes inputs are gone. The README example shows the wiring.
| pull_request_target: | ||
| types: [opened, edited, synchronize, reopened, ready_for_review] |
There was a problem hiding this comment.
We should almost never use pull_request_target events. Why do you need this over pull_request events?
There was a problem hiding this comment.
Two reasons. First, tamper resistance: the check will be required, so a PR must not be able to alter its own gate. With pull_request_target the workflow and the action (pinned to the change-management-check/v1 tag) come from the base branch; with pull_request the workflow comes from the PR head, so a PR could rename the job or edit the workflow to pass. Second, fork PRs: with pull_request a fork PR's workflow waits for maintainer approval, and the required check shows Expected until then; pull_request_target runs without that step. The usual pull_request_target risk does not apply here: no checkout of PR code, no secrets, the only permission is pull-requests: read (used by the path filter), and the action itself reads only the event payload. The action is trigger-agnostic; this is a recommendation for the required-check wiring.
There was a problem hiding this comment.
First, tamper resistance: the check will be required, so a PR must not be able to alter its own gate. With pull_request_target the workflow and the action (pinned to the change-management-check/v1 tag) come from the base branch; with pull_request the workflow comes from the PR head, so a PR could rename the job or edit the workflow to pass.
This is understandable. We typically use pull_request regardless - but make sure that the CODEOWNERS is properly set and therefore gates unwanted merges that bypass a workflow.
Second, fork PRs: with pull_request a fork PR's workflow waits for maintainer approval, and the required check shows Expected until then; pull_request_target runs without that step. The usual pull_request_target risk does not apply here: no checkout of PR code, no secrets, the only permission is pull-requests: read (used by the path filter), and the action itself reads only the event payload. The action is trigger-agnostic; this is a recommendation for the required-check wiring.
Is this expected to run against PR from forks?
There was a problem hiding this comment.
Not expected. All target repos are private, and the last 300 PRs in each (900 total) include zero fork PRs. If one appeared, the workflow would wait for maintainer approval before running, and the required check would sit Expected until then.
On CODEOWNERS: agreed, and the catch-all rules already cover the workflow path in each target repo. The wiring step will confirm the code-owner review requirement is actually enforced; the enforcement audit found gaps in the current rulesets, and closing them is part of this rollout.
- check.js: drop the files API, glob engine, and exemption inputs. - check.js: parse CRLF bodies; fail when the sections input is empty. - check.js: blank comment spans in place instead of deleting them. - README.md: usage example uses dorny/paths-filter with a conditional step; document the required-check skip rules. - action.yml: remove the docs-paths, dev-paths, and head-prefixes inputs.
New composite action
change-management-check: fails the check when a change-management section in the PR body (Testing evidence, Success / failure criteria, Rollback plan) is empty or missing. The failure message names the empty sections and links the Standard.The action reads only the PR body from the event payload. No checkout, no PR code execution, no API calls, read-only scope.
Exemptions are the workflow's job: the caller skips the check step for
renovate/*head branches (step condition) and for docs-only or dev-only changes (dorny/paths-filter withpredicate-quantifier: some-with-excludes). Heading names and strip-lines come from the caller's inputs, so one action serves repos with different PR templates.Inputs:
sections(required),strip-lines,standard-url.Tests:
node --test, 26 cases, with the three production-path PR templates as fixtures.Ships with a major changeset, so the first release is
change-management-check/1.0.0with the movingchange-management-check/v1tag. Consumers pin@change-management-check/v1, same asdo-not-merge@do-not-merge/v1.Part 1 of IP-4105 (epic IP-3990): the per-repo workflows and the required-check wiring follow.
Ticket: https://smartcontract-it.atlassian.net/browse/IP-4105