diff --git a/.github/actions/accept-screenshots/accept_screenshots.rb b/.github/actions/accept-screenshots/accept_screenshots.rb new file mode 100644 index 00000000..2a4a1178 --- /dev/null +++ b/.github/actions/accept-screenshots/accept_screenshots.rb @@ -0,0 +1,471 @@ +# frozen_string_literal: true + +# Accept screenshot baselines from a PR comment or a new issue (the report's +# "Commit decisions" button opens one prefilled): +# +# /snap-diff accept run= [pr=] @ @ ... +# /snap-diff reject ... (optional, informational) +# +# or, rejecting only (nothing is committed; the list goes on the PR): +# +# /snap-diff reject run= [pr=] ... +# +# is a `file` from the report's snap_diff_manifest.json; is the +# first 16 hex digits of the SHA-256 of its bytes, as the report showed them. +# The bytes come from that run's `*-report-full` artifact and are committed to +# the PR branch at the manifest `path`. Nothing from the repository is +# executed: git only fetches, reads the tree into an index, stages blobs, +# commits and pushes; there is no working tree. +# Stdlib only; shells out to `gh` and `git`. +# +# Environment (all required unless noted): +# GH_TOKEN token for gh and git (contents/pull-requests/actions) +# GITHUB_REPOSITORY owner/name +# GITHUB_SERVER_URL optional, default https://github.com +# EVENT_NAME issue_comment (a PR comment) or issues (a new issue) +# ISSUE_NUMBER the commented PR, or the new issue +# BODY the comment or issue body: the command +# AUTHOR its author's login +# AUTHOR_ASSOCIATION pre-filter: OWNER, MEMBER or COLLABORATOR +# SCREENSHOTS_PATH optional, default doc/screenshots: every target must be under it +# CI_WORKFLOW optional: workflow file to dispatch on the branch after the push +# RUNNER_TEMP optional: scratch dir, default a fresh Dir.mktmpdir +# +# The PR is the commented one, or in an issue `pr=`, or else the single open +# same-repository PR on the run's branch. Every refusal is a reply on the +# comment's PR or the issue (then closed as not planned) plus exit status 1; +# a done issue gets a link to the PR and is closed as completed. Replies and +# logs carry only validated strings or fixed text, never raw input. + +require "json" +require "open3" +require "base64" +require "digest" +require "fileutils" +require "tmpdir" + +module AcceptScreenshots + class Refusal < StandardError; end + + MAX_FILES = 200 + MAX_BODY = 20_000 + MAX_BYTES = 20 * 1024 * 1024 + MAX_TOTAL_BYTES = 200 * 1024 * 1024 + MAX_ARTIFACT_BYTES = 100 * 1024 * 1024 + MAX_MANIFEST_BYTES = 1024 * 1024 + MAX_CANDIDATES = 10 + ALLOWED_ASSOCIATIONS = %w[OWNER MEMBER COLLABORATOR].freeze + ALLOWED_CHARS = "only A-Z a-z 0-9 . _ - /, no spaces, no `..`" + # No spaces: the command is whitespace-split, so a name with one cannot be written. + SAFE_PATH = %r{\A[A-Za-z0-9._/-]+\z} + LOGIN = /\A[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})\z/ + FIRST_LINE = %r{\A/snap-diff (?:accept|reject)(?: |\z)} + HEAD_LINE = %r{\A/snap-diff (accept|reject) run=(\d+)(?: pr=(\d+))?((?: +\S+)*)\z} + REJECT = %r{\A/snap-diff reject((?: +\S+)*)\z} + HASHED = /\A([^@]+)@([0-9a-f]{16})\z/ + MAGIC = {".png" => ["PNG", /\A\x89PNG\r\n\x1A\n/n], ".webp" => ["WebP", /\ARIFF....WEBP/mn]}.freeze + BOT = ["github-actions[bot]", "41898282+github-actions[bot]@users.noreply.github.com"].freeze + + module_function + + # {run:, pr: nil | digits, accept: {file => sha16}, reject: [file]} + def parse(body) + raise Refusal, "Comment too long (over #{MAX_BODY} characters)." if body.length > MAX_BODY + + lines = body.lines.map(&:strip).reject(&:empty?) + unless lines.first&.match?(FIRST_LINE) + raise Refusal, "The first line must be `/snap-diff accept run= @ ...` or `/snap-diff reject run= ...`." + end + + head = lines.first.match(HEAD_LINE) or raise Refusal, "Missing or invalid `run=` (digits only)." + kind, run, pr, rest = head.captures + tokens = rest.split.uniq + raise Refusal, "Invalid `pr=` (digits only, right after `run=`)." if tokens.any? { |t| t.start_with?("pr=", "run=") } + raise Refusal, "The command needs at least one file." if tokens.empty? + raise Refusal, "Too many files (#{tokens.size}, max #{MAX_FILES})." if tokens.size > MAX_FILES + + reject = (kind == "reject") ? tokens : [] + lines.drop(1).each do |line| + match = (kind == "accept") && line.match(REJECT) or raise Refusal, "Unexpected line after the first line; only `/snap-diff reject ...` may follow an accept line." + reject.concat(match[1].split) + end + reject.each_with_index { |file, i| raise Refusal, "Rejected file ##{i + 1} is not allowed (#{ALLOWED_CHARS})." unless safe_path?(file) } + return {run: run, pr: pr, accept: {}, reject: reject.uniq} if kind == "reject" + + files = tokens.each_with_index.with_object({}) do |(token, i), acc| + match = token.match(HASHED) or raise Refusal, "Accepted file ##{i + 1} lacks its @ (16 lowercase hex); copy the command from the report again." + file, sha16 = match.captures + raise Refusal, "Accepted file ##{i + 1} is not allowed (#{ALLOWED_CHARS})." unless safe_path?(file) + raise Refusal, "`#{file}` is accepted twice with different hashes." if acc.key?(file) + + acc[file] = sha16 + end + {run: run, pr: pr, accept: files, reject: reject.uniq} + end + + # A comment acts on the PR it is posted on; a `pr=` in it must agree. + def comment_pr(command, issue_number) + raise Refusal, "The command names PR ##{command[:pr]} but was posted on PR ##{issue_number}." if command[:pr] && command[:pr] != issue_number + + issue_number + end + + # An issue without `pr=`: the single open same-repository PR on the run's branch. + def resolve_pr(pulls, repo, head_branch) + matches = pulls.select { |p| p.dig("head", "repo", "full_name") == repo && p.dig("head", "ref") == head_branch } + raise Refusal, "No open pull request of this repository is on the run's branch." if matches.empty? + raise Refusal, "More than one open pull request is on the run's branch; add `pr=`." if matches.size > 1 + + matches.first["number"].to_s + end + + # Relative, no empty / `.` / `..` segments, conservative characters. + def safe_path?(path) + return false unless path.is_a?(String) && path.match?(SAFE_PATH) + + path.split("/", -1).none? { |segment| segment.empty? || segment == "." || segment == ".." } + end + + # `permission` is the base role: maintain reads as write, custom roles as their base. + def writer?(permission_json) = %w[admin write].include?(permission_json["permission"]) + + def manifest_entries(manifest) + entries = manifest.is_a?(Hash) && manifest["entries"] + (entries.is_a?(Array) && entries.all?(Hash)) ? entries : nil + end + + # { requested file => target path }, validated. + def entries_for(manifest, files, screenshots_path) + entries = manifest_entries(manifest) or raise Refusal, "The report manifest is malformed." + + targets = files.to_h do |file| + matches = entries.select { |entry| entry["file"] == file } + raise Refusal, "`#{file}` is not in the report manifest." if matches.empty? + raise Refusal, "`#{file}` is listed more than once in the report manifest." if matches.size > 1 + raise Refusal, "`#{file}`: only changed or new screenshots can be accepted." unless %w[changed new].include?(matches.first["status"]) + + [file, check_target!(file, matches.first["path"], screenshots_path)] + end + duplicate = targets.values.tally.find { |_, count| count > 1 } + raise Refusal, "Two accepted files share the same target `#{duplicate.first}`." if duplicate + + targets + end + + def check_target!(file, path, screenshots_path) + allowed = safe_path?(path) && path.start_with?("#{screenshots_path}/") && + path.split("/").none? { |segment| segment.downcase.start_with?(".git") } + raise Refusal, "Target path for `#{file}` is not allowed (under `#{screenshots_path}/`, #{ALLOWED_CHARS}, no `.git*` segment)." unless allowed + + ext = File.extname(path).downcase + raise Refusal, "`#{file}`: extension must be .png or .webp and match its target." unless MAGIC.key?(ext) && File.extname(file).downcase == ext + + path + end + + def check_image!(file, bytes) + kind, magic = MAGIC.fetch(File.extname(file).downcase) + raise Refusal, "`#{file}` is not a #{kind} image." unless bytes.b.match?(magic) + + nil + end + + def check_hash!(file, bytes, sha16) + raise Refusal, "`#{file}` does not match the reviewed hash #{sha16}; review the report again." unless Digest::SHA256.hexdigest(bytes).start_with?(sha16) + + nil + end + + # Sizes are checked before any byte is read. + def read_images(dir, files, max_file: MAX_BYTES, max_total: MAX_TOTAL_BYTES) + root = File.realpath(dir) + total = 0 + paths = files.to_h do |file| + path = File.join(dir, file) + raise Refusal, "`#{file}` is missing from the artifact." unless File.exist?(path) || File.symlink?(path) + raise Refusal, "`#{file}` in the artifact is not a regular file." unless File.lstat(path).file? + raise Refusal, "`#{file}` in the artifact is not a regular file." unless File.realpath(path).start_with?("#{root}/") + + size = File.size(path) + raise Refusal, "`#{file}` is larger than #{max_file} bytes." if size > max_file + total += size + raise Refusal, "The accepted files together are larger than #{max_total} bytes." if total > max_total + + [file, path] + end + paths.transform_values { |path| File.binread(path) } + end + + # Report artifact names, sorted; expired and oversized ones left out. + def report_artifacts(artifacts_json) + artifacts_json.fetch("artifacts") + .select { |a| !a["expired"] && a["name"].to_s.end_with?("-report-full") && a["size_in_bytes"].to_i <= MAX_ARTIFACT_BYTES } + .map { |a| a["name"] }.sort + end + + # The first of at most MAX_CANDIDATES artifacts whose manifest lists every + # file with the command's hash: [index, manifest]. The block loads one + # manifest (index, name), so later artifacts are never downloaded. The + # manifest is only a pointer; the gate is check_hash! on the picked bytes. + def pick_artifact(names, accept) + names.first(MAX_CANDIDATES).each_with_index do |name, i| + manifest = yield(name, i) + entries = manifest_entries(manifest) or next + covered = accept.all? { |file, sha16| entries.any? { |e| e["file"] == file && e["sha256"].to_s.start_with?(sha16) } } + return [i, manifest] if covered + end + raise Refusal, "No report artifact of this run lists all of #{accept.keys.join(", ")} with those hashes." + end + + def load_manifest(dir) + path = File.join(dir, "snap_diff_manifest.json") + return unless File.file?(path) && !File.symlink?(path) && File.size(path) <= MAX_MANIFEST_BYTES + + JSON.parse(File.read(path)) + rescue JSON::ParserError + nil + end + + # The run must come from this repository (no forks) and from this PR: a run + # of another PR or event on the same commit carries another report. A + # workflow_dispatch run (this bot's CI re-dispatch) lists no PRs, so it + # counts when it ran on the PR's branch. head_sha equality is checked apart. + def check_run!(run, repo, pr, run_id, head_ref) + raise Refusal, "Run #{run_id} was not built from this repository." unless run.dig("head_repository", "full_name") == repo + + of_pr = Array(run["pull_requests"]).any? { |p| p["number"].to_s == pr } + dispatched_on_branch = run["event"] == "workflow_dispatch" && run["head_branch"] == head_ref + raise Refusal, "Run #{run_id} is not a run of this pull request." unless of_pr || dispatched_on_branch + + nil + end + + # Paths whose `git check-attr filter` output says lfs. + def lfs_paths(check_attr_output) + check_attr_output.lines.filter_map do |line| + path, value = line.chomp.split(": filter: ", 2) + path if value == "lfs" + end + end + + def summary(command, targets, sha, ci_line) + lines = [sha ? "Accepted #{targets.size} screenshot(s) from run #{command[:run]} in #{sha}:" : + "Nothing to commit: these screenshots from run #{command[:run]} already match the PR branch:"] + targets.each { |file, path| lines << "- `#{file}` -> `#{path}`" } + unless command[:reject].empty? + lines << "" << "Rejected (not committed):" + command[:reject].each { |file| lines << "- `#{file}`" } + end + lines << "" << ci_line if sha + lines.join("\n") + end + + def reject_summary(command) + (["Rejected in run #{command[:run]} (not committed):"] + command[:reject].map { |file| "- `#{file}`" }).join("\n") + end + + def commit_message(targets, run_id, login, pr) + bullets = targets.map { |file, path| "- #{file} -> #{path}" }.join("\n") + "Accept screenshot baselines\n\nAccepted by @#{login} in PR ##{pr} from run #{run_id}:\n#{bullets}\n" + end + + # Workflow commands start a line with `::`; nothing logged may. + # A lone \r ends a line for the runner too. + def log_text(text) = text.to_s[0, 4000].gsub(/\r\n?/, "\n").gsub(/^(\s*)::/, '\1: :') + + def log(text) = warn(log_text(text)) + + # ---- side effects --------------------------------------------------------- + + def env(name) = ENV.fetch(name) { abort "#{name} is not set" } + + def sh(*cmd, stdin: nil, env: {}, chdir: nil) + opts = {stdin_data: stdin, binmode: true} + opts[:chdir] = chdir if chdir + out, err, status = Open3.capture3(env, *cmd, **opts) + log(err) unless err.empty? + [out, status.success?] + end + + def sh!(*cmd, **opts) + out, ok = sh(*cmd, **opts) + raise "#{cmd.first(2).join(" ")} failed" unless ok + + out + end + + def gh_json(path) = JSON.parse(sh!("gh", "api", path)) + + def reply(repo, number, text) + sh!("gh", "api", "-X", "POST", "repos/#{repo}/issues/#{number}/comments", "--input", "-", stdin: JSON.generate(body: text)) + end + + def close_issue(repo, number, reason) + sh!("gh", "api", "-X", "PATCH", "repos/#{repo}/issues/#{number}", "--input", "-", stdin: JSON.generate(state: "closed", state_reason: reason)) + end + + # A fresh, isolated git: no global or system config, no hooks, no LFS + # downloads, the bot as author. `auth` adds the token as an HTTP header via + # GIT_CONFIG_* env -- never argv, a remote URL or .git/config. + def git_env(auth = nil) + config = [["core.hooksPath", "/dev/null"]] + config << ["http.#{ENV.fetch("GITHUB_SERVER_URL", "https://github.com")}/.extraheader", "AUTHORIZATION: basic #{auth}"] if auth + vars = { + "GIT_CONFIG_GLOBAL" => "/dev/null", "GIT_CONFIG_NOSYSTEM" => "1", + "GIT_LFS_SKIP_SMUDGE" => "1", "GIT_TERMINAL_PROMPT" => "0", + "GIT_AUTHOR_NAME" => BOT[0], "GIT_AUTHOR_EMAIL" => BOT[1], + "GIT_COMMITTER_NAME" => BOT[0], "GIT_COMMITTER_EMAIL" => BOT[1], + "GIT_CONFIG_COUNT" => config.size.to_s + } + config.each_with_index { |(key, value), i| vars.merge!("GIT_CONFIG_KEY_#{i}" => key, "GIT_CONFIG_VALUE_#{i}" => value) } + vars + end + + def run! + repo = env("GITHUB_REPOSITORY") + issue = env("ISSUE_NUMBER") + raise "ISSUE_NUMBER is not a number" unless issue.match?(/\A\d+\z/) + issue_mode = {"issues" => true, "issue_comment" => false}.fetch(env("EVENT_NAME")) + + exit 1 unless accept!(repo, issue, issue_mode) + rescue Refusal => e + log(e.message) + reply(repo, issue, "Screenshot accept refused: #{e.message}") + close_issue(repo, issue, "not_planned") if issue_mode + exit 1 + rescue => e + log("#{e.class}: #{e.message}") + reply(repo, issue, "Screenshot accept failed; see the workflow log.") if repo && issue + exit 1 + end + + # True unless the commit landed but the CI dispatch failed. + def accept!(repo, issue, issue_mode) + raise Refusal, "Only owners, members and collaborators can accept screenshots." unless ALLOWED_ASSOCIATIONS.include?(env("AUTHOR_ASSOCIATION")) + + login = env("AUTHOR") + raise Refusal, "Only people with write access can accept screenshots." unless login.match?(LOGIN) + # MEMBER/COLLABORATOR includes read-only people; the permission API does not. + raise Refusal, "Only people with write access can accept screenshots." unless writer?(gh_json("repos/#{repo}/collaborators/#{login}/permission")) + + screenshots_path = ENV.fetch("SCREENSHOTS_PATH", "doc/screenshots").delete_suffix("/") + raise "SCREENSHOTS_PATH must be a safe relative path" unless safe_path?(screenshots_path) + workflow = ENV["CI_WORKFLOW"].to_s + raise "CI_WORKFLOW must be a workflow file name" unless workflow.empty? || workflow.match?(/\A[\w.-]+\.ya?ml\z/) + + command = parse(env("BODY")) + run_id = command[:run] + files = command[:accept].keys + + run = gh_json("repos/#{repo}/actions/runs/#{run_id}") + pr = if !issue_mode then comment_pr(command, issue) + elsif command[:pr] then command[:pr] + else + raise Refusal, "Run #{run_id} has no branch; add `pr=`." unless run["head_branch"].is_a?(String) + + pulls = sh!("gh", "api", "-X", "GET", "repos/#{repo}/pulls", "-f", "state=open", "-f", "per_page=100", + "-f", "head=#{repo.split("/").first}:#{run["head_branch"]}") + resolve_pr(JSON.parse(pulls), repo, run["head_branch"]) + end + pull = gh_json("repos/#{repo}/pulls/#{pr}") + branch = pull.dig("head", "ref") + check_run!(run, repo, pr, run_id, branch) + raise Refusal, "The pull request is not open." unless pull["state"] == "open" + return finish(repo, issue, issue_mode, pr, reject_summary(command), "Recorded on PR ##{pr}.") if files.empty? + + raise Refusal, "The pull request comes from a fork." unless pull.dig("head", "repo", "full_name") == repo + raise Refusal, "PR has new commits since run #{run_id} — review the new report." unless pull.dig("head", "sha") == run["head_sha"] + raise "unexpected head_sha" unless run["head_sha"].to_s.match?(/\A\h{40}\z/) + raise Refusal, "Refusing to push to the base or default branch." if [pull.dig("base", "ref"), pull.dig("base", "repo", "default_branch")].include?(branch) + + work = Dir.mktmpdir("snap-diff-accept-", ENV["RUNNER_TEMP"] || Dir.mktmpdir) + names = report_artifacts(gh_json("repos/#{repo}/actions/runs/#{run_id}/artifacts?per_page=100")) + picked, manifest = pick_artifact(names, command[:accept]) do |name, i| + dir = File.join(work, "artifact-#{i}") + sh!("gh", "run", "download", run_id, "-R", repo, "-n", name, "-D", dir) + load_manifest(dir) + end + targets = entries_for(manifest, files, screenshots_path) + images = read_images(File.join(work, "artifact-#{picked}"), files) + images.each do |file, bytes| + check_image!(file, bytes) + check_hash!(file, bytes, command[:accept].fetch(file)) + end + + auth = Base64.strict_encode64("x-access-token:#{env("GH_TOKEN")}") + puts "::add-mask::#{auth}" if ENV["GITHUB_ACTIONS"] == "true" + sha = commit_and_push(remote: "#{ENV.fetch("GITHUB_SERVER_URL", "https://github.com")}/#{repo}.git", head_sha: run["head_sha"], + branch: branch, targets: targets, images: images, dir: File.join(work, "repo"), + message: commit_message(targets, run_id, login, pr), git_env: git_env(auth)) + + ci_line = workflow.empty? ? "Re-run CI to verify." : "Dispatching `#{workflow}` on the PR branch to verify." + finish(repo, issue, issue_mode, pr, summary(command, targets, sha, ci_line), "Applied to PR ##{pr}: #{sha || "nothing to commit"}.") + puts "accepted #{targets.size} file(s): #{sha || "no change"}" + return true unless sha && !workflow.empty? + + dispatch_ci(repo, pr, workflow, branch) + ensure + FileUtils.rm_rf(work) if work # nothing of this run outlives it on a persistent runner + end + + # The outcome goes on the PR; an issue also gets a link to it and is closed. + def finish(repo, issue, issue_mode, pr, pr_text, issue_text) + reply(repo, pr, pr_text) + return true unless issue_mode + + reply(repo, issue, issue_text) + close_issue(repo, issue, "completed") + true + end + + # Best effort, after the reply: a failure must not hide the pushed commit. + # A push made with GITHUB_TOKEN does not start workflows; a + # workflow_dispatch made with it does. + def dispatch_ci(repo, pr, workflow, branch) + _, ok = sh("gh", "workflow", "run", workflow, "-R", repo, "--ref", branch) + return true if ok + + log("CI dispatch failed; the commit is pushed.") + reply(repo, pr, "The commit is pushed, but dispatching `#{workflow}` failed. Re-run CI to verify.") + false + end + + # A scratch repository holding only the reviewed commit, read into the + # index; no working tree. Blobs are written from the verified bytes with + # --no-filters and staged by id, so the PR's .gitattributes cannot rewrite + # them. + # Returns the pushed commit, or nil when the bytes are already committed. + def commit_and_push(remote:, head_sha:, branch:, targets:, images:, dir:, message:, git_env:) + git = ->(*args, stdin: nil) { sh!("git", *args, stdin: stdin, env: git_env, chdir: dir) } + + FileUtils.mkdir_p(dir) + git.call("init", "-q") + git.call("remote", "add", "origin", remote) + git.call("fetch", "-q", "--depth=1", "origin", head_sha) + git.call("read-tree", head_sha) + + lfs = lfs_paths(git.call("check-attr", "--cached", "filter", "--", *targets.values)) + raise Refusal, lfs.map { |path| "`#{path}` is tracked by Git LFS; accept it locally." }.join(" ") unless lfs.empty? + + targets.each do |file, path| + bytes = images.fetch(file) + blob = git.call("hash-object", "-w", "--no-filters", "--stdin", stdin: bytes).strip + raise "staged blob differs from the verified bytes" unless blob == Digest::SHA1.hexdigest("blob #{bytes.bytesize}\0".b + bytes.b) + + git.call("update-index", "--add", "--cacheinfo", "100644,#{blob},#{path}") + end + _, unchanged = sh("git", "diff-index", "--cached", "--quiet", head_sha, env: git_env, chdir: dir) + return nil if unchanged + + commit = git.call("commit-tree", git.call("write-tree").strip, "-p", head_sha, "-F", "-", stdin: message).strip + # A lease, not a force: the push lands only while the branch is exactly the + # reviewed head_sha. A plain push would still fast-forward a branch reset + # to an ancestor of head_sha and bring the removed commits back. + _, pushed = sh("git", "push", "-q", "--force-with-lease=refs/heads/#{branch}:#{head_sha}", "origin", "#{commit}:refs/heads/#{branch}", env: git_env, chdir: dir) + raise Refusal, "Push rejected: the branch moved or is protected. Review the new report." unless pushed + + commit + end +end + +AcceptScreenshots.run! if $PROGRAM_NAME == __FILE__ diff --git a/.github/actions/accept-screenshots/action.yml b/.github/actions/accept-screenshots/action.yml new file mode 100644 index 00000000..56e4f429 --- /dev/null +++ b/.github/actions/accept-screenshots/action.yml @@ -0,0 +1,34 @@ +--- +name: 'Accept SnapDiff screenshots' +description: >- + Commit screenshot baselines accepted in a PR comment or a new issue + (`/snap-diff accept run= [pr=] @ ...`) from that run's report artifact. + Run it from an issue_comment + issues workflow gated to OWNER/MEMBER/COLLABORATOR. +inputs: + token: + description: 'Token with contents: write, pull-requests: write, issues: write, actions: read (actions: write to dispatch ci-workflow); reads the author''s permission' + default: ${{ github.token }} + screenshots-path: + description: 'Every accepted file must be committed under this directory (relative to the repository root)' + default: 'doc/screenshots' + ci-workflow: + description: >- + Optional workflow file name (with workflow_dispatch) to run on the PR branch after the commit. + A push made with GITHUB_TOKEN does not start workflows; a workflow_dispatch does. + default: '' +runs: + using: 'composite' + steps: + - name: Accept screenshots + shell: bash + env: + GH_TOKEN: ${{ inputs.token }} + CI_WORKFLOW: ${{ inputs.ci-workflow }} + SCREENSHOTS_PATH: ${{ inputs.screenshots-path }} + EVENT_NAME: ${{ github.event_name }} + ISSUE_NUMBER: ${{ github.event.issue.number }} + # issue_comment: the comment on the PR; issues: the new issue itself. + BODY: ${{ github.event_name == 'issues' && github.event.issue.body || github.event.comment.body }} + AUTHOR: ${{ github.event_name == 'issues' && github.event.issue.user.login || github.event.comment.user.login }} + AUTHOR_ASSOCIATION: ${{ github.event_name == 'issues' && github.event.issue.author_association || github.event.comment.author_association }} + run: ruby "$GITHUB_ACTION_PATH/accept_screenshots.rb" diff --git a/.github/actions/upload-screenshots/action.yml b/.github/actions/upload-screenshots/action.yml index 7a937a8e..6e32323f 100644 --- a/.github/actions/upload-screenshots/action.yml +++ b/.github/actions/upload-screenshots/action.yml @@ -77,6 +77,9 @@ runs: - name: Job summary if: steps.check-report.outputs.exists == 'true' shell: bash + env: + HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} + REPO: ${{ github.repository }} run: | cat >> "$GITHUB_STEP_SUMMARY" <> "$GITHUB_STEP_SUMMARY" + fi - name: Find existing PR comment if: inputs.pr-comment == 'true' && github.event_name == 'pull_request' @@ -111,3 +117,5 @@ runs: | HTML report (inline) | ${{ steps.upload-report.outputs.artifact-url || 'N/A' }} | | Full report with images | ${{ steps.upload-report-full.outputs.artifact-url || 'N/A' }} | | [All artifacts](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}#artifacts) | Browse all | + + ${{ steps.check-report.outputs.exists == 'true' && github.event.pull_request.head.repo.full_name == github.repository && 'To accept: open the HTML report, mark Accept/Reject, then Commit decisions.' || '' }} diff --git a/.github/workflows/snap-diff-accept.yml b/.github/workflows/snap-diff-accept.yml new file mode 100644 index 00000000..d368d5ba --- /dev/null +++ b/.github/workflows/snap-diff-accept.yml @@ -0,0 +1,46 @@ +--- +name: Accept screenshots + +# The report's "Commit decisions" button opens a new issue whose body is the +# review command (or a maintainer pastes it as a PR comment): +# /snap-diff accept run= [pr=] @ ... +# The action commits those files from that run's report artifact to the PR branch. + +on: + issue_comment: + types: [ created ] + issues: + types: [ opened ] + +jobs: + accept: + if: >- + (github.event_name == 'issue_comment' && github.event.issue.pull_request && + startsWith(github.event.comment.body, '/snap-diff ') && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) || + (github.event_name == 'issues' && + startsWith(github.event.issue.body, '/snap-diff ') && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.issue.author_association)) + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: write + pull-requests: write + issues: write + actions: write + # Per PR for comments, per issue for issues (the PR is not known yet); + # two issues for one PR may run at once, and the push lease lets only one land. + concurrency: + group: snap-diff-accept-${{ github.event.issue.number }} + cancel-in-progress: false + steps: + # The default branch (the ref of both events): this repository's trusted + # code, needed for the local action. The PR's code is never checked out here. + - uses: actions/checkout@v7 + with: + persist-credentials: false + + - uses: ./.github/actions/accept-screenshots + with: + screenshots-path: test/fixtures/app/doc/screenshots + ci-workflow: test.yml diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6dc565a0..22460eab 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -95,6 +95,8 @@ jobs: if: failure() with: name: base-screenshots + # The suite writes its report under its own SnapDiff root. + report-path: test/fixtures/app/doc/screenshots pr-comment: 'true' - name: Uploading Coverage Report diff --git a/.gitignore b/.gitignore index ca07113a..82d398f6 100644 --- a/.gitignore +++ b/.gitignore @@ -20,6 +20,7 @@ *.diff.png *.base.png snap_diff_report.html +snap_diff_manifest.json *.attempt_*.webp *.diff.webp diff --git a/CHANGELOG.md b/CHANGELOG.md index 64b0677e..fb9437cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,13 @@ kept as history. This entry is the one to read if you are coming from **1.15.1** ### Added +- **Accept screenshots from the CI report.** On GitHub Actions, the HTML report gets Accept/Reject + buttons and a "Commit decisions" button that opens a prefilled GitHub issue. Submitting it (or pasting + the copied command as a PR comment) runs the new `accept-screenshots` action, which commits the exact + CI pixels to the PR branch with the repository's `GITHUB_TOKEN`. The report also writes `snap_diff_manifest.json`, and + `upload-screenshots` adds a how-to-accept line to its PR comment and job summary. See + [CI integration](docs/ci-integration.md#accept-screenshots-from-the-pull-request). + - **`SNAP_DIFF_ACCEPT_SCREENSHOTS=1` re-records changed baselines for one run.** The variable selects `record = :all` for the run that sees it, outranking `record =` and `fail_if_new`; a `record:` on the screenshot still outranks it. `1` and `true` turn it on; any other value is the diff --git a/README.md b/README.md index 699b2032..1a022be7 100644 --- a/README.md +++ b/README.md @@ -115,6 +115,7 @@ Add diff artifacts to `.gitignore` — these are generated at runtime and should *.diff.webp *.base.webp snap_diff_report.html +snap_diff_manifest.json ``` If you skip Step 2 and push to CI, the build will fail — `fail_if_new` is `true` by default in CI. @@ -232,7 +233,7 @@ After tests run, open `doc/screenshots/snap_diff_report.html`: ![SnapDiff Web UI — annotated diff showing changed regions highlighted in red](docs/images/snap_diff_annotated.png) -See [Web UI & Custom Reporters](docs/reporters.md) for full feature details and [CI Integration](docs/ci-integration.md) for GitHub Actions setup. +See [Web UI & Custom Reporters](docs/reporters.md) for full feature details and [CI Integration](docs/ci-integration.md) for GitHub Actions setup, including [accepting screenshots from a PR comment](docs/ci-integration.md#accept-screenshots-from-the-pull-request). ## AI-Assisted Triage (optional) diff --git a/Rakefile b/Rakefile index 3562df55..ad1be8c0 100644 --- a/Rakefile +++ b/Rakefile @@ -69,7 +69,7 @@ desc "Remove screenshot diff artifacts (keeps baselines)" task "snap_diff:clean" do patterns = ["**/*.diff.png", "**/*.base.png", "**/*.base.diff.png", "**/*.heatmap.diff.png", "**/*.diff.webp", "**/*.base.webp", "**/*.base.diff.webp", "**/*.heatmap.diff.webp", - "**/snap_diff_report.html"] + "**/snap_diff_report.html", "**/snap_diff_manifest.json"] removed = patterns.flat_map { |p| Dir.glob("tmp/#{p}") + Dir.glob("doc/screenshots/#{p}") }.uniq removed.each { |f| FileUtils.rm_f(f) } puts "Removed #{removed.size} diff artifacts" diff --git a/docs/ci-integration.md b/docs/ci-integration.md index c4e11e1e..25f815df 100644 --- a/docs/ci-integration.md +++ b/docs/ci-integration.md @@ -283,4 +283,81 @@ Two things this minimal recipe does not do: 2. Enter the branch name (e.g. your PR branch) 3. The workflow records new and changed baselines, commits, and pushes +## Accept screenshots from the pull request + +When a CI run fails on screenshot diffs, a reviewer can accept the new pixels without checking out the +branch. In the HTML report (the inline link in the PR comment), each changed or new screenshot has +Accept and Reject buttons (keys `Y` and `N`). "Commit decisions" opens a new GitHub issue already +filled in with a command like `/snap-diff accept run=123 pr=45 linux/cuprite/index.png@0123456789abcdef`. +Click Submit. A workflow commits those exact files from that run's report artifact to the PR branch +with the repository's own `GITHUB_TOKEN`, posts the result on the PR, closes the issue and re-runs CI. +The buttons appear only in reports built on GitHub Actions. + +If the link would be too long for GitHub, the button is disabled. Use "Copy review command" and paste +the command as a comment on the pull request instead; it does the same thing. + +To install it, copy this workflow. Set `screenshots-path` to your `save_path`. `ci-workflow` is +optional: it names your test workflow, which needs an `on: workflow_dispatch` trigger. Without it the +reply asks you to re-run CI by hand, because a push made with `GITHUB_TOKEN` does not start workflows. + +```yaml +# .github/workflows/snap-diff-accept.yml +name: Accept screenshots + +on: + issue_comment: + types: [created] + issues: + types: [opened] + +jobs: + accept: + if: >- + (github.event_name == 'issue_comment' && github.event.issue.pull_request && + startsWith(github.event.comment.body, '/snap-diff ') && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) || + (github.event_name == 'issues' && + startsWith(github.event.issue.body, '/snap-diff ') && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.issue.author_association)) + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: write + pull-requests: write + issues: write + actions: write # read is enough if you leave out ci-workflow + concurrency: + group: snap-diff-accept-${{ github.event.issue.number }} + cancel-in-progress: false + steps: + - uses: snap-diff/snap_diff-capybara/.github/actions/accept-screenshots@master + with: + screenshots-path: doc/screenshots + ci-workflow: test.yml +``` + +`issue_comment` and `issues` workflows run only from the default branch, so merge this file first. +Until then the issue or comment does nothing. + +Reviewers need write access to the repository. The workflow checks the permission API as well as the +`author_association`, so read-only collaborators are refused, and their issue is closed with the reason. + +Limits: + +- Pull requests from the same repository only. A fork PR is refused. +- Git LFS paths are refused. Accept those locally. +- Screenshot paths may use only `A-Z a-z 0-9 . _ - /` (no spaces) and must sit under `screenshots-path`. +- The report artifact must still exist. `upload-screenshots` keeps it `retention-days` days (default 2). +- Only the PR's current head can be accepted. After a new push, review the new run's report. +- Only the last capture of a file can be accepted. Earlier captures of it show a note instead of the buttons. + +Security: the workflow never checks out or runs the PR's code. It downloads the report artifact, takes +the paths from the report's manifest, and commits a file only if its SHA-256 matches the hash in the +reviewer's command and its bytes are a PNG or WebP. It pushes only to the PR branch, and only while +that branch is still at the commit the run tested. The design does rely on trust you already extend: +whoever opens a same-repository PR can already push to the repository, and their CI run produces the +artifact. Beyond that, the permission check and your branch protection limit the damage. Do not enable +this for forks, and do not give the bot a ruleset bypass: with branch protection in place, a push the +rules reject fails instead of landing. + [← Back to README](../README.md) diff --git a/docs/organization.md b/docs/organization.md index 541f6f27..004e5859 100644 --- a/docs/organization.md +++ b/docs/organization.md @@ -33,7 +33,7 @@ version control system (git). Screenshots are compared to the previously COMMITTED version of the same screenshot. -**Note:** When a screenshot differs, diff artifacts (`.diff.png`, `.heatmap.diff.png`, etc.) are generated alongside the baseline. Add `*.diff.png`, `*.base.png`, `*.diff.webp`, `*.base.webp`, and `snap_diff_report.html` to your `.gitignore`. Clean up artifacts with `rake snap_diff:clean`. +**Note:** When a screenshot differs, diff artifacts (`.diff.png`, `.heatmap.diff.png`, etc.) are generated alongside the baseline. Add `*.diff.png`, `*.base.png`, `*.diff.webp`, `*.base.webp`, `snap_diff_report.html`, and `snap_diff_manifest.json` to your `.gitignore`. Clean up artifacts with `rake snap_diff:clean`. ## Screenshot groups diff --git a/docs/reporters.md b/docs/reporters.md index b70d50b1..153239c8 100644 --- a/docs/reporters.md +++ b/docs/reporters.md @@ -24,6 +24,10 @@ committing it is what accepts it). Their count in the report is the summary line **Note:** The report is not written when every screenshot matched and none is new. +It also writes `snap_diff_manifest.json`, one entry per changed or new screenshot. The +[accept workflow](ci-integration.md#accept-screenshots-from-the-pull-request) reads it to find where +each accepted file goes. Add it to your `.gitignore` next to `snap_diff_report.html`. + ## The end-of-run summary Every run ends with what it actually did, whether or not you require a reporter: diff --git a/lib/snap_diff/reporters/html.rb b/lib/snap_diff/reporters/html.rb index 3f4e4105..0e0d9bc4 100644 --- a/lib/snap_diff/reporters/html.rb +++ b/lib/snap_diff/reporters/html.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "base64" +require "digest" require "erb" require "fileutils" require "pathname" @@ -10,6 +11,7 @@ # SnapDiff::Reporting. require "snap_diff/reporting" require "snap_diff/config" +require "snap_diff/vcs" module SnapDiff module Reporters @@ -129,6 +131,51 @@ def render ERB.new(File.read(self.class.template_path)).result(binding) end + # Review controls (Accept/Reject, the review command) exist only in a + # CI run, where the run id names the artifact the bot reads. A + # pull_request run's merge ref names the PR; the repository gives the + # new-issue page that "Commit decisions" opens. + def review + run = ENV["GITHUB_RUN_ID"].to_s + return if run.empty? + + files = manifest_entries.map { |entry| entry[:accept]["file"] } + {run: run, pr: ENV["GITHUB_REF"].to_s[%r{\Arefs/pull/(\d+)/merge\z}, 1], issue_url: new_issue_url, + files: files, sha16: files.to_h { |file| [file, file_sha256(file)[0, 16]] }} + end + + def new_issue_url + repo = ENV["GITHUB_REPOSITORY"].to_s + server = ENV.fetch("GITHUB_SERVER_URL", "https://github.com").to_s + return unless repo.match?(%r{\A[\w.-]+/[\w.-]+\z}) && server.match?(%r{\Ahttps://[\w.-]+(?::\d+)?\z}) + + "#{server}/#{repo}/issues/new" + end + + # The entries the page shows. In a CI run the manifest's entry for a + # file (its last capture) carries that `file`; an earlier capture of the + # same file points at it (`superseded_by`, an index); any other entry + # says why it has no controls (`review_reason`). The rest of :accept + # stays internal. + def report_data + in_review = review + chosen = in_review ? manifest_entries : [] + entries.map do |entry| + data = entry.except(:accept) + next data unless in_review + + if chosen.any? { |m| m.equal?(entry) } + data[:file] = entry[:accept]["file"] + elsif reviewable_status?(entry) && captured?(entry) + later = last_captures.find { |m| m[:accept]["path"] == entry[:accept]["path"] } + data[:superseded_by] = entries.index { |e| e.equal?(later) } + else + data[:review_reason] = (entry[:status] == "rerecorded") ? "rerecorded" : "no_capture" + end + data + end + end + def self.template_path File.expand_path("templates/report.html.erb", __dir__) end @@ -165,7 +212,8 @@ def compared_entry_for(name, compare) { name: name, original: resolve_image(compare.base_image_path), - new: resolve_image(compare.image_path) + new: resolve_image(compare.image_path), + accept: accept_paths(compare.image_path) }.merge(failure_details(compare)).compact end @@ -228,7 +276,8 @@ def new_screenshot_entry(name, path) image = resolve_image(path) return entry unless image - entry.merge(new: image, git_add: "git add -- #{Shellwords.escape(display_path(path))}") + entry.merge(new: image, accept: accept_paths(path), + git_add: "git add -- #{Shellwords.escape(display_path(path))}") end # Relative to the project root, where the user runs `git add` -- the @@ -239,6 +288,22 @@ def display_path(path) relative.start_with?("..") ? path.to_s : relative end + # For snap_diff_manifest.json: the capture overwrote the baseline, so it + # is both what to commit (`path`, from the git root) and what the + # artifact holds (`file`, from the report dir). String keys: entries + # pass through JSON in dump_state. + def accept_paths(capture) + return unless capture && File.exist?(capture) + + git_root = SnapDiff::Vcs.git_root_for(SnapDiff.config.root.to_s) + return unless git_root + + file = Pathname(capture).expand_path.relative_path_from(output_path.dirname.expand_path).to_s + return if file.start_with?("..") # outside the report dir, so not in the artifact + + {"path" => Pathname(capture).realpath.relative_path_from(Pathname(git_root).realpath).to_s, "file" => file} + end + def resolve_image(path) return unless path @@ -254,9 +319,42 @@ def data_uri(pathname) "data:#{mime};base64,#{Base64.strict_encode64(pathname.binread)}" end + # One digest per file for the page and the manifest alike. def write_report FileUtils.mkdir_p(output_path.dirname) + @digests = {} File.write(output_path, render) + write_manifest + ensure + @digests = nil + end + + def reviewable_status?(entry) = %w[changed new].include?(entry[:status]) + + def captured?(entry) = entry[:accept] && (output_path.dirname / entry[:accept]["file"]).file? + + # Of the bytes on disk when the report is written -- what the artifact + # uploads. The accept command carries its first 16 hex digits, and the + # bot refuses a file whose bytes no longer match. + def file_sha256(file) + digest = -> { Digest::SHA256.file(output_path.dirname / file).hexdigest } + @digests ? (@digests[file] ||= digest.call) : digest.call + end + + # One file, one entry: the last capture, whatever its status -- a changed + # one re-recorded afterwards is already accepted. + def last_captures + entries.select { |entry| captured?(entry) }.reverse.uniq { |entry| entry[:accept]["path"] }.reverse + end + + def manifest_entries = last_captures.select { |entry| reviewable_status?(entry) } + + # What a reviewer page may accept: the captures that differ or are new. + def write_manifest + manifest = manifest_entries.map do |entry| + {"name" => entry[:name], "status" => entry[:status]}.merge(entry[:accept], "sha256" => file_sha256(entry[:accept]["file"])) + end + File.write(output_path.dirname / "snap_diff_manifest.json", JSON.pretty_generate("version" => 1, "entries" => manifest)) end end end diff --git a/lib/snap_diff/reporters/templates/report.html.erb b/lib/snap_diff/reporters/templates/report.html.erb index 9237553c..0770f9a0 100644 --- a/lib/snap_diff/reporters/templates/report.html.erb +++ b/lib/snap_diff/reporters/templates/report.html.erb @@ -153,6 +153,20 @@ .zoom-fit { font-size: .625rem; font-family: var(--font); font-weight: 500; background: none; border: 1px solid var(--border); border-radius: var(--radius-sm); color: var(--text2); padding: .2rem .5rem; cursor: pointer; transition: all .1s; } .zoom-fit:hover { background: var(--bg4); color: var(--text); } +<% if review %> + /* Review controls (CI runs only) */ + .review-btn { padding: .3rem .625rem; font-size: .625rem; font-family: var(--font); font-weight: 500; background: var(--bg); color: var(--text3); border: 1px solid var(--border); border-radius: var(--radius); cursor: pointer; white-space: nowrap; } + .review-btn:hover { color: var(--text2); border-color: var(--text3); } + .review-btn.accept.active { background: var(--green-dim); color: var(--green); border-color: var(--green); } + .review-btn.reject.active { background: var(--red-dim); color: var(--red); border-color: var(--red); } + #review-panel { display: flex; align-items: flex-start; gap: .5rem; padding: .375rem .75rem; background: var(--bg2); border-top: 1px solid var(--border); flex-shrink: 0; } + #review-side { display: flex; flex-direction: column; gap: .25rem; align-items: flex-start; } + #review-count, #review-hint, #review-note, #review-commit-note { font-size: .625rem; color: var(--text2); } + .review-btn.primary { background: var(--green-dim); color: var(--green); border-color: var(--green); } + .review-btn:disabled { opacity: .5; cursor: not-allowed; } + #review-command { flex: 1; min-width: 0; height: 2.75rem; resize: none; font-family: var(--font-mono); font-size: .625rem; background: var(--surface); color: var(--text); border: 1px solid var(--border); border-radius: var(--radius-sm); padding: .25rem .4rem; } +<% end %> + /* Scrollbar */ ::-webkit-scrollbar { width: 5px; height: 5px; } ::-webkit-scrollbar-track { background: transparent; } @@ -224,6 +238,14 @@ +<% if review %> +
+ Review + + + + +<% end %>
@@ -232,6 +254,19 @@
+<% if review %> +
+
+ + + Opens a GitHub issue with your decisions; submit it to commit. + + +
+ +
+<% end %> +
100% @@ -244,7 +279,10 @@