From f7113a2c7387124017fc3cd76c65c18c54b6b1ab Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Sun, 7 Dec 2025 02:30:27 +0100 Subject: [PATCH 1/6] Remove API token requirement from Danger workflow Use secure two-workflow pattern from dblock's blog post: - First workflow runs on pull_request, executes danger dry_run without write permissions, uploads JSON report as artifact - Second workflow runs on workflow_run after first completes, downloads artifact and posts PR comment with write access This is secure because untrusted fork code never has access to write permissions. The comment workflow runs trusted code from the base branch. Inlined Dangerfile checks from ruby-grape-danger to avoid plugins that require GitHub API methods not available in dry_run mode. --- .github/workflows/danger-comment.yml | 89 ++++++++++++++++++++++++++++ .github/workflows/danger.yml | 18 ++++-- Dangerfile | 43 +++++++++++++- 3 files changed, 143 insertions(+), 7 deletions(-) create mode 100644 .github/workflows/danger-comment.yml diff --git a/.github/workflows/danger-comment.yml b/.github/workflows/danger-comment.yml new file mode 100644 index 000000000..d7bd8986f --- /dev/null +++ b/.github/workflows/danger-comment.yml @@ -0,0 +1,89 @@ +--- +name: danger-comment +on: + workflow_run: + workflows: [danger] + types: [completed] + +permissions: + contents: read + pull-requests: write + +jobs: + comment: + runs-on: ubuntu-latest + if: > + github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.pull_requests[0] + steps: + - name: Download Danger report + uses: actions/download-artifact@v6 + with: + name: danger-report + github-token: ${{ github.token }} + run-id: ${{ github.event.workflow_run.id }} + - name: Post or update PR comment + uses: actions/github-script@v8 + with: + script: | + const fs = require('fs'); + const report = JSON.parse(fs.readFileSync('danger_report.json', 'utf8')); + + let body = '## Danger Report\n\n'; + + if (report.errors && report.errors.length > 0) { + body += '### Errors\n'; + report.errors.forEach(e => body += `- :no_entry_sign: ${e}\n`); + body += '\n'; + } + + if (report.warnings && report.warnings.length > 0) { + body += '### Warnings\n'; + report.warnings.forEach(w => body += `- :warning: ${w}\n`); + body += '\n'; + } + + if (report.messages && report.messages.length > 0) { + body += '### Messages\n'; + report.messages.forEach(m => body += `- :book: ${m}\n`); + body += '\n'; + } + + if ((!report.errors || report.errors.length === 0) && + (!report.warnings || report.warnings.length === 0) && + (!report.messages || report.messages.length === 0)) { + body += ':white_check_mark: All checks passed!'; + } + + const prNumber = ${{ github.event.workflow_run.pull_requests[0].number }}; + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber + }); + + const botComment = comments.find(c => + c.user.login === 'github-actions[bot]' && + c.body.includes('## Danger Report') + ); + + if (botComment) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: botComment.id, + body: body + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: body + }); + } + + // Fail if there are errors + if (report.errors && report.errors.length > 0) { + core.setFailed('Danger found errors'); + } diff --git a/.github/workflows/danger.yml b/.github/workflows/danger.yml index 12372470c..57792a75a 100644 --- a/.github/workflows/danger.yml +++ b/.github/workflows/danger.yml @@ -6,16 +6,22 @@ jobs: danger: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 with: - fetch-depth: 100 + fetch-depth: 0 - name: Set up Ruby uses: ruby/setup-ruby@v1 with: ruby-version: 3.4 bundler-cache: true - name: Run Danger - run: | - # the token is public, has public_repo scope and belongs to the grape-bot user owned by @dblock, this is ok - TOKEN=$(echo -n Z2hwX2lYb0dPNXNyejYzOFJyaTV3QUxUdkNiS1dtblFwZTFuRXpmMwo= | base64 --decode) - DANGER_GITHUB_API_TOKEN=$TOKEN bundle exec danger --verbose + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + DANGER_REPORT_PATH: danger_report.json + run: bundle exec danger dry_run --base=$BASE_SHA --head=$HEAD_SHA --verbose + - name: Upload Danger report + uses: actions/upload-artifact@v5 + with: + name: danger-report + path: danger_report.json diff --git a/Dangerfile b/Dangerfile index 82881902a..eb2ff4497 100644 --- a/Dangerfile +++ b/Dangerfile @@ -1,3 +1,44 @@ # frozen_string_literal: true -danger.import_dangerfile(gem: 'ruby-grape-danger') +# Inline checks from ruby-grape-danger (avoids plugins requiring GitHub API token) + +has_app_changes = !git.modified_files.grep(/lib/).empty? +has_spec_changes = !git.modified_files.grep(/spec/).empty? + +if has_app_changes && !has_spec_changes + warn("There're library changes, but not tests. That's OK as long as you're refactoring existing code.", sticky: false) +end + +if !has_app_changes && has_spec_changes + message('We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!', sticky: false) +end + +# Simplified changelog check (replaces danger-changelog plugin which requires github.* methods) +# Note: toc.check! from danger-toc plugin removed (not essential for CI) +has_changelog_changes = git.modified_files.include?('CHANGELOG.md') || git.added_files.include?('CHANGELOG.md') +if has_app_changes && !has_changelog_changes + warn('Please update CHANGELOG.md with a description of your changes.', sticky: false) +end + +(git.modified_files + git.added_files - %w[Dangerfile]).each do |file| + next unless File.file?(file) + + contents = File.read(file) + if file.start_with?('spec') + fail("`xit` or `fit` left in tests (#{file})") if contents =~ /^\w*[xf]it/ + fail("`fdescribe` left in tests (#{file})") if contents =~ /^\w*fdescribe/ + end +end + +# Output JSON report for GitHub Actions workflow_run to post as PR comment +if ENV['DANGER_REPORT_PATH'] + require 'json' + + report = { + errors: violation_report[:errors].map(&:message), + warnings: violation_report[:warnings].map(&:message), + messages: violation_report[:messages].map(&:message) + } + + File.write(ENV['DANGER_REPORT_PATH'], JSON.pretty_generate(report)) +end From 65958a649d538496a388b10ce3141fc26c2a25d8 Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Sun, 7 Dec 2025 02:45:36 +0100 Subject: [PATCH 2/6] Fix rubocop offenses in Dangerfile Apply Style/IfUnlessModifier and Performance/RegexpMatch fixes. Disable Style/SignalException for Danger's `fail` DSL method. --- Dangerfile | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/Dangerfile b/Dangerfile index eb2ff4497..7937d807f 100644 --- a/Dangerfile +++ b/Dangerfile @@ -5,29 +5,25 @@ has_app_changes = !git.modified_files.grep(/lib/).empty? has_spec_changes = !git.modified_files.grep(/spec/).empty? -if has_app_changes && !has_spec_changes - warn("There're library changes, but not tests. That's OK as long as you're refactoring existing code.", sticky: false) -end +warn("There're library changes, but not tests. That's OK as long as you're refactoring existing code.", sticky: false) if has_app_changes && !has_spec_changes -if !has_app_changes && has_spec_changes - message('We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!', sticky: false) -end +message('We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!', sticky: false) if !has_app_changes && has_spec_changes # Simplified changelog check (replaces danger-changelog plugin which requires github.* methods) # Note: toc.check! from danger-toc plugin removed (not essential for CI) has_changelog_changes = git.modified_files.include?('CHANGELOG.md') || git.added_files.include?('CHANGELOG.md') -if has_app_changes && !has_changelog_changes - warn('Please update CHANGELOG.md with a description of your changes.', sticky: false) -end +warn('Please update CHANGELOG.md with a description of your changes.', sticky: false) if has_app_changes && !has_changelog_changes (git.modified_files + git.added_files - %w[Dangerfile]).each do |file| next unless File.file?(file) contents = File.read(file) + # rubocop:disable Style/SignalException -- `fail` is Danger's DSL method, not Kernel#fail if file.start_with?('spec') - fail("`xit` or `fit` left in tests (#{file})") if contents =~ /^\w*[xf]it/ - fail("`fdescribe` left in tests (#{file})") if contents =~ /^\w*fdescribe/ + fail("`xit` or `fit` left in tests (#{file})") if /^\w*[xf]it/.match?(contents) + fail("`fdescribe` left in tests (#{file})") if /^\w*fdescribe/.match?(contents) end + # rubocop:enable Style/SignalException end # Output JSON report for GitHub Actions workflow_run to post as PR comment From b4d4719cf1c4b5dcc993a9a1078f29a8811a6825 Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Sun, 7 Dec 2025 02:46:27 +0100 Subject: [PATCH 3/6] Add comment for tests --- lib/grape.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/grape.rb b/lib/grape.rb index 0015d67eb..a539ac74f 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -77,6 +77,8 @@ def self.deprecator end end +# Comment for testing purposes + # https://api.rubyonrails.org/classes/ActiveSupport/Deprecation.html # adding Grape.deprecator to Rails App if any require 'grape/railtie' if defined?(Rails::Railtie) && ActiveSupport.gem_version >= Gem::Version.new('7.1') From 7d8ff3e3a7a19da8da9a85b88513fe6aa2006632 Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Sun, 7 Dec 2025 03:00:44 +0100 Subject: [PATCH 4/6] Pass PR number via artifact for fork PRs For fork PRs, github.event.workflow_run.pull_requests is always empty due to GitHub security restrictions. Include PR number in the danger_report.json artifact instead. --- .github/workflows/danger-comment.yml | 14 ++++++++------ .github/workflows/danger.yml | 1 + Dangerfile | 1 + 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.github/workflows/danger-comment.yml b/.github/workflows/danger-comment.yml index d7bd8986f..4a2913952 100644 --- a/.github/workflows/danger-comment.yml +++ b/.github/workflows/danger-comment.yml @@ -12,9 +12,7 @@ permissions: jobs: comment: runs-on: ubuntu-latest - if: > - github.event.workflow_run.event == 'pull_request' && - github.event.workflow_run.pull_requests[0] + if: github.event.workflow_run.event == 'pull_request' steps: - name: Download Danger report uses: actions/download-artifact@v6 @@ -29,6 +27,11 @@ jobs: const fs = require('fs'); const report = JSON.parse(fs.readFileSync('danger_report.json', 'utf8')); + if (!report.pr_number) { + console.log('No PR number found in report, skipping comment'); + return; + } + let body = '## Danger Report\n\n'; if (report.errors && report.errors.length > 0) { @@ -55,11 +58,10 @@ jobs: body += ':white_check_mark: All checks passed!'; } - const prNumber = ${{ github.event.workflow_run.pull_requests[0].number }}; const { data: comments } = await github.rest.issues.listComments({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: prNumber + issue_number: report.pr_number }); const botComment = comments.find(c => @@ -78,7 +80,7 @@ jobs: await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: prNumber, + issue_number: report.pr_number, body: body }); } diff --git a/.github/workflows/danger.yml b/.github/workflows/danger.yml index 57792a75a..e852a52de 100644 --- a/.github/workflows/danger.yml +++ b/.github/workflows/danger.yml @@ -18,6 +18,7 @@ jobs: env: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} + PR_NUMBER: ${{ github.event.pull_request.number }} DANGER_REPORT_PATH: danger_report.json run: bundle exec danger dry_run --base=$BASE_SHA --head=$HEAD_SHA --verbose - name: Upload Danger report diff --git a/Dangerfile b/Dangerfile index 7937d807f..d191103da 100644 --- a/Dangerfile +++ b/Dangerfile @@ -31,6 +31,7 @@ if ENV['DANGER_REPORT_PATH'] require 'json' report = { + pr_number: ENV['PR_NUMBER']&.to_i, errors: violation_report[:errors].map(&:message), warnings: violation_report[:warnings].map(&:message), messages: violation_report[:messages].map(&:message) From 4c68d532ba3405a7f7c5906df416ecc75c6169a7 Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Sun, 7 Dec 2025 03:23:36 +0100 Subject: [PATCH 5/6] Improve Danger workflow configuration - Add explicit PR event types (opened, synchronize, reopened) - Set artifact retention to 1 day - Fix regex patterns with word boundaries to avoid false positives --- .github/workflows/danger-comment.yml | 5 +++-- .github/workflows/danger.yml | 5 ++++- Dangerfile | 8 ++++---- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/workflows/danger-comment.yml b/.github/workflows/danger-comment.yml index 4a2913952..2dd5037a1 100644 --- a/.github/workflows/danger-comment.yml +++ b/.github/workflows/danger-comment.yml @@ -6,8 +6,9 @@ on: types: [completed] permissions: - contents: read - pull-requests: write + actions: read # Required to download the artifact + contents: read # Standard safe default + issues: write # Required to post comments (PRs are Issues in the API) jobs: comment: diff --git a/.github/workflows/danger.yml b/.github/workflows/danger.yml index e852a52de..dd0eef550 100644 --- a/.github/workflows/danger.yml +++ b/.github/workflows/danger.yml @@ -1,6 +1,8 @@ --- name: danger -on: pull_request +on: + pull_request: + types: [opened, synchronize, reopened] jobs: danger: @@ -26,3 +28,4 @@ jobs: with: name: danger-report path: danger_report.json + retention-days: 1 diff --git a/Dangerfile b/Dangerfile index d191103da..17c4726a3 100644 --- a/Dangerfile +++ b/Dangerfile @@ -2,8 +2,8 @@ # Inline checks from ruby-grape-danger (avoids plugins requiring GitHub API token) -has_app_changes = !git.modified_files.grep(/lib/).empty? -has_spec_changes = !git.modified_files.grep(/spec/).empty? +has_app_changes = !git.modified_files.grep(%r{^lib/}).empty? +has_spec_changes = !git.modified_files.grep(%r{^spec/}).empty? warn("There're library changes, but not tests. That's OK as long as you're refactoring existing code.", sticky: false) if has_app_changes && !has_spec_changes @@ -20,8 +20,8 @@ warn('Please update CHANGELOG.md with a description of your changes.', sticky: f contents = File.read(file) # rubocop:disable Style/SignalException -- `fail` is Danger's DSL method, not Kernel#fail if file.start_with?('spec') - fail("`xit` or `fit` left in tests (#{file})") if /^\w*[xf]it/.match?(contents) - fail("`fdescribe` left in tests (#{file})") if /^\w*fdescribe/.match?(contents) + fail("`xit` or `fit` left in tests (#{file})") if /^\s*[xf]it\b/.match?(contents) + fail("`fdescribe` left in tests (#{file})") if /^\s*fdescribe\b/.match?(contents) end # rubocop:enable Style/SignalException end From 8c59d3e9ac14c8c4490afc776195497e4dfef68b Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Sun, 7 Dec 2025 03:41:26 +0100 Subject: [PATCH 6/6] Cleanup from temporary changes --- lib/grape.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/grape.rb b/lib/grape.rb index a539ac74f..0015d67eb 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -77,8 +77,6 @@ def self.deprecator end end -# Comment for testing purposes - # https://api.rubyonrails.org/classes/ActiveSupport/Deprecation.html # adding Grape.deprecator to Rails App if any require 'grape/railtie' if defined?(Rails::Railtie) && ActiveSupport.gem_version >= Gem::Version.new('7.1')