ci: Fix pr-code-format permissions for private forks

[whisperity]

The action requires the ability to change the contents of a pull request (by adding the formatting-related comment) which is only possible through the write permission, but if this permission is missing, potential private forks with Actions enabled will break, and because the action triggers on pull_request_target even a PR initially fixing this issue downstream will not exhibit the fix until merged.

[llvmbot]

@llvm/pr-subscribers-github-workflow

Author: None (whisperity)

Changes

The action requires the ability to change the contents of a pull request (by adding the formatting-related comment) which is only possible through the write permission, but if this permission is missing, potential private forks with Actions enabled will break, and because the action triggers on pull_request_target even a PR initially fixing this issue downstream will not exhibit the fix until merged.

---

Full diff: https://github.com/llvm/llvm-project/pull/120838.diff

1 Files Affected:

• (modified) .github/workflows/pr-code-format.yml (+2)

diff --git a/.github/workflows/pr-code-format.yml b/.github/workflows/pr-code-format.yml
index f2bb37316d3a8b..fda5b6d96e78c2 100644
--- a/.github/workflows/pr-code-format.yml
+++ b/.github/workflows/pr-code-format.yml
@@ -2,6 +2,8 @@ name: "Check code formatting"
 
 permissions:
   contents: read
+  issues: write
+  pull-requests: write
 
 on:
   pull_request:

[tstellar]

This workflow should not be adding comments to the pull requests directly. The comments are added by this workflow instead: https://github.com/llvm/llvm-project/blob/main/.github/workflows/issue-write.yml

So, I don't think it needs these permissions. Can you show me what failures you are seeing?

[tstellar]

I think you may have been looking at an old version of this action, because it uses pull_request now and not pull_request_target.

[whisperity]

So, I don't think it needs these permissions. Can you show me what failures you are seeing?

We were getting two issues. First, the inability to download the contents of the (otherwise private) repository when pull_request_target was being used instead of pull_request (indeed, the fork might be older, the branch-off point is somewhere on the 18.y version line), and the second was this:

Run python llvm/utils/git/code-format-helper.py
Traceback (most recent call last):
  File "/home/runner/_work/llvm-project/llvm-project/llvm/utils/git/code-format-helper.py", line 230, in <module>
    if not fmt.run(changed_files, args):
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/_work/llvm-project/llvm-project/llvm/utils/git/code-format-helper.py", line 100, in run
    self.update_pr_success(args)
  File "/home/runner/_work/llvm-project/llvm-project/llvm/utils/git/code-format-helper.py", line 83, in update_pr_success
    pr = repo.get_issue(args.issue_number).as_pull_request()
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/_work/_tool/Python/3.11.11/x64/lib/python3.11/site-packages/github/Repository.py", line 2845, in get_issue
    headers, data = self._requester.requestJsonAndCheck(
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/_work/_tool/Python/3.11.11/x64/lib/python3.11/site-packages/github/Requester.py", line 442, in requestJsonAndCheck
    return self.__check(
           ^^^^^^^^^^^^^
  File "/home/runner/_work/_tool/Python/3.11.11/x64/lib/python3.11/site-packages/github/Requester.py", line 487, in __check
    raise self.__createException(status, responseHeaders, data)
github.GithubException.GithubException: 403 {"message": "Resource not accessible by integration", "documentation_url": "https://docs.github.com/rest/issues/issues#get-an-issue", "status": "403"}
Error: Process completed with exit code 1.

This is not a problem for this repository because it is public, so the query can run.

This workflow should not be adding comments to the pull requests directly. The comments are added by this workflow instead: https://github.com/llvm/llvm-project/blob/main/.github/workflows/issue-write.yml

Both the latest tagged release and the default branch contains the following code where the PR is commented directly by the formatting workflow:

llvm-project/llvm/utils/git/code-format-helper.py

Lines 133 to 136 in e21dc4b

	if existing_comment:
	existing_comment.edit(comment_text)
	elif create_new:
	pr.as_issue().create_comment(comment_text)

[tstellar]

llvm-project/llvm/utils/git/code-format-helper.py

Lines 133 to 136 in e21dc4b

	if existing_comment:
	existing_comment.edit(comment_text)
	elif create_new:
	pr.as_issue().create_comment(comment_text)

In the llvm/llvm-project main branch, this code never gets called, because we pass the --write-comment-to-file option.

[boomanaiden154]

Also, running the LLVM workflows in forks is not really a well-supported use case currently.

People hacking on the actions typically make sure they work in the monorepo at head and leave it at that. If people want to maintain this and the changes are simple enough and don't impact how they operate in the monorepo, I'm probably fine with them going upstream.

This patch significantly increases the permissions scope for this workflow and thus does impact how things operate in the monorepo, which I'm not a big fan of.

[whisperity]

Also, running the LLVM workflows in forks is not really a well-supported use case currently.

People hacking on the actions typically make sure they work in the monorepo at head and leave it at that.

In that case, should I create a patch that restricts the running of the action if: github.repository_owner == 'llvm' like it is done for many of the other workflow files?

If people want to maintain this and the changes are simple enough and don't impact how they operate in the monorepo

It is very unlikely that anything would be changed by such a change.

@tstellar Can you (or someone who has the rights) check how LLVM organisation is set up? Likely the write permission isn't needed in

The comments are added by this workflow instead: https://github.com/llvm/llvm-project/blob/main/.github/workflows/issue-write.yml

either, because the org is set to allow write rights implicitly?

[boomanaiden154]

In that case, should I create a patch that restricts the running of the action if: github.repository_owner == 'llvm' like it is done for many of the other workflow files?

Probably. It is worth checking the git history though as I remember some discussion about someone else removing it in a fork. I believe the consensus was that they should remove the check downstream, but I forget what ended up happening.

[tstellar]

These are the llvm org settings:

[whisperity]

I'll come back to this once we update our fork to the latest LLVM snapshot and see if there are still issues with it. So far I've realised I am a bit lost in the full diff between what we have (what existed in LLVM 18.1) and what exists in the upstream right now.

I would apologise for the noise, and will make sure to check the larger picture in the diffs next time it happens — I made the wrong assumption that the CI scripts would not be so constantly evolving. 😇