diff --git a/.github/workflows/pr-check.yml b/.github/workflows/pr-check.yml index 4bda74a..3f02dfc 100644 --- a/.github/workflows/pr-check.yml +++ b/.github/workflows/pr-check.yml @@ -1,8 +1,17 @@ name: PR Lesson Check -# Validates lesson submissions on pull requests (branch-based, no fork required) -# and renders the portal pages, then posts a friendly pass/fail summary as a PR -# comment. This does NOT deploy — deployment stays in build-site.yml on main. +# Validates lesson submissions on pull requests (branch PRs and forks) and +# renders the portal pages. This does NOT deploy — deployment stays in +# build-site.yml on main. +# +# This job checks out and runs the PR author's code (scripts/, templates/), so +# it deliberately holds a read-only GITHUB_TOKEN and touches no secrets. That +# means it cannot post its own summary comment: on `pull_request`, GitHub +# forces the fork token to read-only regardless of the declared permissions +# block, so the comment API returns HTTP 403. Instead the rendered comment is +# uploaded as an artifact and posted by pr-comment.yml, which runs on +# `workflow_run` in the base-repo context with a writable token and never +# checks out PR code. on: pull_request: @@ -19,9 +28,7 @@ on: - 'index.html' permissions: - contents: read - pull-requests: write # needed to post/update the summary comment - issues: write # needed to create/update issue comments via the Issues API + contents: read # deliberately read-only — this job runs untrusted PR code # If an author pushes several times quickly, only check the latest commit. concurrency: @@ -60,7 +67,10 @@ jobs: # is silently skipped — it would never be validated or deployed yet the # build still passes. We reject those here. paths.yml (learning paths) and # template.yaml (the reference) are the only YAML files that legitimately - # live at the repo root. + # live at the repo root, and .github/ is CI config rather than lesson + # content — without that exemption every workflow edit fails this check + # with "move it into lessons/", because the paths filter above matches + # '**/*.yml'. # # We diff the PR head SHA against base (base...head), NOT ...HEAD. On a # pull_request event, actions/checkout resolves HEAD to the *merge* ref @@ -81,9 +91,10 @@ jobs: *.yml|*.yaml) ;; *) continue ;; esac - if [ "$f" = "paths.yml" ] || [ "$f" = "template.yaml" ]; then - continue - fi + # Repo infrastructure, not a lesson submission. + case "$f" in + .github/*|paths.yml|template.yaml) continue ;; + esac case "$f" in lessons/*.yml) printf '%s\n' "$f" >> changed_lessons.txt @@ -123,36 +134,25 @@ jobs: if [ "${{ steps.location.outputs.location_failed }}" = "true" ]; then result=failure; fi echo "result=$result" >> "$GITHUB_OUTPUT" + # pr_number.txt is only a last-resort hint for pr-comment.yml. It is + # written by this untrusted job, so that workflow verifies it against the + # run's head SHA before trusting it. - name: Format PR comment if: always() run: | python scripts/format_pr_comment.py "${{ steps.outcome.outputs.result }}" build.log pr_comment.md + echo "${{ github.event.pull_request.number }}" > pr_number.txt - - name: Post or update PR comment + - name: Upload the comment for pr-comment.yml to post if: always() - uses: actions/github-script@v7 + uses: actions/upload-artifact@v4 with: - script: | - const fs = require('fs'); - const body = fs.readFileSync('pr_comment.md', 'utf8'); - const marker = ''; - const { owner, repo } = context.repo; - const issue_number = context.issue.number; - - const { data: comments } = await github.rest.issues.listComments({ - owner, repo, issue_number, per_page: 100, - }); - const existing = comments.find(c => c.body && c.body.includes(marker)); - - if (existing) { - await github.rest.issues.updateComment({ - owner, repo, comment_id: existing.id, body, - }); - } else { - await github.rest.issues.createComment({ - owner, repo, issue_number, body, - }); - } + name: pr-check-comment + path: | + pr_comment.md + pr_number.txt + retention-days: 1 + if-no-files-found: error - name: Fail the check if anything failed if: steps.outcome.outputs.result == 'failure' diff --git a/.github/workflows/pr-comment.yml b/.github/workflows/pr-comment.yml new file mode 100644 index 0000000..34716be --- /dev/null +++ b/.github/workflows/pr-comment.yml @@ -0,0 +1,125 @@ +name: PR Lesson Check Comment + +# Posts the pass/fail summary produced by pr-check.yml as a PR comment. +# +# Why this is a second workflow: on `pull_request`, GitHub forces the fork's +# GITHUB_TOKEN to read-only no matter what `permissions:` declares, so the +# check itself cannot comment on a fork PR (HTTP 403). A `workflow_run` +# workflow runs in the base-repo context and does get a writable token. +# +# The safety property that makes this work: `workflow_run` always uses the copy +# of this file on the default branch, and this job never checks out PR content +# — it only downloads an artifact and posts its text. So the fork's scripts and +# templates never execute in a job holding a write-scoped token. (Switching +# pr-check.yml itself to `pull_request_target` would hand that token to a job +# that runs the fork's scripts/ and templates/ — a repo-takeover vector. Don't.) +# +# Everything in the artifact is untrusted data: it is written by the job that +# ran the PR author's code. It is only ever posted as a comment body, and the +# PR number it claims is verified against the run's head SHA before use. + +on: + workflow_run: + workflows: ["PR Lesson Check"] + types: [completed] + +permissions: + contents: read + actions: read # to download the check run's artifact + pull-requests: write # to post/update the summary comment + +# Serialize per branch so two runs finishing at once can't both conclude that +# no comment exists yet and each create one. +concurrency: + group: pr-comment-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }} + cancel-in-progress: false + +jobs: + comment: + runs-on: ubuntu-latest + # A cancelled run (superseded by a newer push) uploaded no artifact. + if: >- + github.event.workflow_run.event == 'pull_request' && + (github.event.workflow_run.conclusion == 'success' || + github.event.workflow_run.conclusion == 'failure') + + steps: + - name: Download the comment artifact + id: download + continue-on-error: true # a run that died before uploading is not an error here + uses: actions/download-artifact@v4 + with: + name: pr-check-comment + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Post or update the PR comment + if: steps.download.outcome == 'success' + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + const { owner, repo } = context.repo; + const run = context.payload.workflow_run; + + // Untrusted content — post as-is, never interpret. Cap it well + // under GitHub's 65536-character comment limit so a pathological + // build log can't make every comment fail to post. + const LIMIT = 60000; + let body = fs.readFileSync('pr_comment.md', 'utf8'); + if (body.length > LIMIT) { + body = body.slice(0, LIMIT) + + '\n\n_…truncated. See the full log in the **Actions** tab._\n'; + } + + // Resolve the PR from trusted event data first. `pull_requests` is + // empty for fork PRs, so fall back to the commit→PR lookup, and + // only then to the number the check job recorded — accepted just + // when that PR's head really is the commit this run tested. + let prNumber = run.pull_requests?.[0]?.number; + + if (!prNumber) { + const { data: prs } = + await github.rest.repos.listPullRequestsAssociatedWithCommit({ + owner, repo, commit_sha: run.head_sha, + }); + prNumber = prs.find(p => p.head.sha === run.head_sha)?.number; + } + + if (!prNumber && fs.existsSync('pr_number.txt')) { + const claimed = Number(fs.readFileSync('pr_number.txt', 'utf8').trim()); + if (Number.isInteger(claimed) && claimed > 0) { + const { data: pr } = + await github.rest.pulls.get({ owner, repo, pull_number: claimed }); + if (pr.head.sha === run.head_sha) { + prNumber = claimed; + } else { + core.warning(`Ignoring claimed PR #${claimed}: its head is ` + + `${pr.head.sha}, not ${run.head_sha}.`); + } + } + } + + if (!prNumber) { + core.setFailed(`No pull request found for ${run.head_sha}; comment not posted.`); + return; + } + + const marker = ''; + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: prNumber, per_page: 100, + }); + // Only ever update our own comment — a marker in someone else's + // comment must not redirect the check summary. + const existing = comments.find( + c => c.user?.type === 'Bot' && c.body?.includes(marker)); + + if (existing) { + await github.rest.issues.updateComment({ + owner, repo, comment_id: existing.id, body, + }); + } else { + await github.rest.issues.createComment({ + owner, repo, issue_number: prNumber, body, + }); + } diff --git a/.gitignore b/.gitignore index a1b0d6c..94b5bbe 100644 --- a/.gitignore +++ b/.gitignore @@ -196,6 +196,7 @@ cython_debug/ # CI artifacts (generated by the pr-check workflow) pr_comment.md +pr_number.txt #Claude .playwright-mcp