[BUG] Danger succeeds despite receiving a 403 error from GitHub API and having `--failOnErrors` flag set

[tylermilner]

Describe the bug I'm working to integrate Danger into the GitHub Actions CI one of my open source repos (see this PR). I know I don't have my GitHub PAT set quite right, but the Danger step is still succeeding despite receiving a 403 response and having the --failOnErrors flag set. See the following output of the Danger step in my workflow:

Run npm run ci-danger
  npm run ci-danger
  shell: /usr/bin/bash -e {0}
  env:
    DANGER_GITHUB_API_TOKEN: ***

> [email protected] ci-danger
> npx danger ci --verbose --failOnErrors


Found only messages, passing those to review.
Request failed [403]: https://api.github.com/repos/tylermilner/last-successful-commit-hash-action/issues/30/comments
Response: {
  "message": "Resource not accessible by personal access token",
  "documentation_url": "https://docs.github.com/rest/issues/comments#create-an-issue-comment"
}
Feedback: undefined
Could not add a commit status, the GitHub token for Danger does not have access rights.
If the build fails, then danger will use a failing exit code.
Danger: ✓ passed, found only messages.
## Messages
Changed Files in this PR: 
 - .github/workflows/ci.yml- package-lock.json- package.json

See screenshot below showing overall success of the workflow run despite Danger receiving the 403 error and failing to post the message to the PR.

To Reproduce Steps to reproduce the behavior:

1. Add Danger as a dependency of a basic JavaScript repo.
2. Add a basic dangerfile.js to the repo (see mine below).
3. Setup npm run script in package.json that will run Danger.
4. Setup a basic GitHub Actions workflow that sets up Node.js, installs the project dependencies, and then runs Danger with the failOnErrors flag set.
5. Setup a GitHub PAT on a "bot" account that doesn't have sufficient permissions to comment on a PR. In my case, I set up a fine-grained PAT with "All Repositories" access configured with "Metadata" read-only access and "Pull requests" read and write access. I realize that setting up a "classic" GitHub PATs might be a fix for this, but in this case we are intentionally trying to setup the PAT with insufficient permissions so that the 403 error is triggered.
6. Setup DANGER_GITHUB_API_TOKEN repository secret for GitHub Actions with the value of the GitHub PAT.
7. Trigger a workflow run via PR. Observe that the run completes successfully despite the output of the Danger step indicating a 403 error occurred and no message being posted to the PR by Danger.

Expected behavior Danger should fail since it received a 403 error and wasn't able to post the message to the PR successful, which should ultimately result in my workflow run failing.

Screenshots

Your Environment

software	version
danger.js	12.1.0
node	20.6.0
npm	9.8.1
Operating System	Ubuntu 22.04.4 LTS

Additional context For reference, here is the super basic dangerfile.js that I created based on the Danger "Getting Started" documentation:

import { message, danger } from 'danger'

const modifiedMD = danger.git.modified_files.join('- ')
message(`Changed Files in this PR: \n - ${modifiedMD}`)

And here is what my CI step looks like that runs Danger:

- name: Danger
  id: npm-danger
  run: npm run ci-danger
  env:
    DANGER_GITHUB_API_TOKEN: ${{ secrets.DANGER_GITHUB_API_TOKEN }}

My main concern here is that I plan to setup my GitHub PAT to expire after 1 year, at which point it would be nice for my CI builds to start failing at the "Danger" step so that I can be reminded to go back and regenerate my PAT. As it stands now, the step will silently fail which will negate any value provided by having Danger setup in the first place.

[eppisapiafsl]

👋 @tylermilner

I get this one multiple time too, after a few debugs, I noticed the 403 isn't from Danger itself but Github and it won't impact the Danger JS result

If you are running with the CI option (--ci), you will notice there is a Danger service running in your CI so you have your Github Workflow (This one gets 403) and Danger will launch a service which is the one that evaluates the result to post the messages

With failOnErrors you tell to the Danger Server to fail if there is a log of Danger Error messages

[tylermilner]

Thanks for commenting, @eppisapiafsl.

It's true that the 403 is coming from GitHub's API. However, isn't Danger the one initiating this API call? Therefore, I would expect Danger to surface that error to the user, since some of its functionality with a third party dependency is failing (i.e. the GitHub API).

Instead, Danger is simply logging the failure and leaving it to the user to parse through "successful" log output to try to figure out why Danger didn't do what was expected. In this case, it's failing to create a comment on the PR, which is one of the main benefits of integrating Danger into a project.

Maybe the --failOnErrors flag should be split into 2 flags:

• --failOnDangerErrors to match existing --failOnErrors functionality where only Danger errors cause failures.
• --failOnAllErrors to fail on all errors Danger encounters (first and third party).

[fbartho]

UPDATED: WHOOPS I misread the whole log file. Looking at that error message, I would expect failOnErrors to abort in this situation

[fbartho]

Rereading the log file, actually this case should be covered under the failOnErrors flag. It really looks like something that is not a problem that happens in correctly configured builds.

Specifically this message:

Could not add a commit status, the GitHub token for Danger does not have access rights. If the build fails, then danger will use a failing exit code.

We have to have some way to know to early exit if the DangerFile couldn’t prepare connect to github — might need to be a runtime value.

The reason is that people might use DangerJS without wanting to post to GitHub — you might only care about the exit code at the end, and the messages could go into the logs.

[fbartho]

So after further there thought, perhaps something like

if (!danger.github.hasCommentPermission) {
  error(“DangerJS cannot comment. Check for Token Expiration?”)
}