Create summary comment for lintcheck runs

Alexendoo · Alexendoo · commit 686cf7e0b10e · 2025-05-15T18:19:32.000Z
diff --git a/.github/workflows/lintcheck.yml b/.github/workflows/lintcheck.yml
@@ -128,21 +128,27 @@ jobs:
     - name: Download JSON
       uses: actions/download-artifact@v4
 
+    - name: Store PR number
+      run: echo ${{ github.event.pull_request.number }} > pr.txt
+
     - name: Diff results
-      # GH's summery has a maximum size of 1024k:
-      # https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary
-      # That's why we first log to file and then to the summary and logs
+      # GH's summery has a maximum size of 1MiB:
+      # https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#step-isolation-and-limits
+      # We upload the full diff as an artifact in case it's truncated
       run: |
-        ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json --truncate >> truncated_diff.md
-        head -c 1024000 truncated_diff.md >> $GITHUB_STEP_SUMMARY
-        cat truncated_diff.md
-        ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json >> full_diff.md
+        ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json --truncate | head -c 1M > $GITHUB_STEP_SUMMARY
+        ./target/debug/lintcheck diff {base,head}/ci_crates_logs.json --write-summary summary.json > full_diff.md
 
     - name: Upload full diff
       uses: actions/upload-artifact@v4
       with:
-        name: diff
-        if-no-files-found: ignore
+        name: full_diff
+        path: full_diff.md
+
+    - name: Upload summary
+      uses: actions/upload-artifact@v4
+      with:
+        name: summary
         path: |
-          full_diff.md
-          truncated_diff.md
+          summary.json
+          pr.txt
diff --git a/.github/workflows/lintcheck_summary.yml b/.github/workflows/lintcheck_summary.yml
@@ -0,0 +1,103 @@
+name: Lintcheck summary
+
+# The workflow_run event runs in the context of the Clippy repo giving it write
+# access, needed here to create a PR comment when the PR originates from a fork
+#
+# The summary artifact is a JSON file that we verify in this action to prevent
+# the creation of arbitrary comments
+#
+# This action must not checkout/run code from the originating workflow_run
+# or directly interpolate ${{}} untrusted fields into code
+#
+# https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#workflow_run
+# https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections
+
+on:
+  workflow_run:
+    workflows: [Lintcheck]
+    types: [completed]
+
+# Restrict the default permission scope https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#defining-access-for-the-github_token-scopes
+permissions:
+  pull-requests: write
+
+jobs:
+  download:
+    runs-on: ubuntu-latest
+    if: ${{ github.event.workflow_run.conclusion == 'success' }}
+    steps:
+      - name: Download artifact
+        uses: actions/download-artifact@v4
+        with:
+          name: summary
+          path: untrusted
+          run-id: ${{ github.event.workflow_run.id }}
+          github-token: ${{ github.token }}
+
+      - name: Format comment
+        uses: actions/github-script@v7
+        with:
+          script: |
+            const fs = require("fs");
+            const assert = require("assert/strict");
+
+            function validateName(s) {
+              assert.match(s, /^[a-z0-9_:]+$/);
+              return s;
+            }
+
+            function validateInt(i) {
+              assert.ok(Number.isInteger(i));
+              return i;
+            }
+
+            function tryReadSummary() {
+              try {
+                return JSON.parse(fs.readFileSync("untrusted/summary.json"));
+              } catch {
+                return null;
+              }
+            }
+
+            const prNumber = parseInt(fs.readFileSync("untrusted/pr.txt"), 10);
+            core.exportVariable("PR", prNumber.toString());
+
+            const untrustedSummary = tryReadSummary();
+            if (!Array.isArray(untrustedSummary)) {
+              return;
+            }
+
+            let summary = `Lintcheck changes for ${context.payload.workflow_run.head_sha}
+            
+            | Lint | Added | Removed | Changed |
+            | ---- | ----: | ------: | ------: |
+            `;
+
+            for (const untrustedRow of untrustedSummary) {
+              const name = validateName(untrustedRow.name);
+
+              const added = validateInt(untrustedRow.added);
+              const removed = validateInt(untrustedRow.removed);
+              const changed = validateInt(untrustedRow.changed);
+
+              const id = name.replace("clippy::", "user-content-").replaceAll("_", "-");
+              const url = `https://github.com/${process.env.GITHUB_REPOSITORY}/actions/runs/${context.payload.workflow_run.id}#${id}`;
+
+              summary += `| [\`${name}\`](${url}) | ${added} | ${removed} | ${changed} |\n`;
+            }
+
+            summary += "\nThis comment will be updated if you push new changes";
+
+            fs.writeFileSync("summary.md", summary);
+
+      - name: Create/update comment
+        run: |
+          if [[ -f summary.md ]]; then
+             gh pr comment "$PR" --body-file summary.md --edit-last --create-if-none
+          else
+            # If we already posted a comment and there are now no changes update it to say that
+            gh pr comment "$PR" --body "No changes for ${{ github.event.workflow_run.head_sha }}" --edit-last || true
+          fi
+        env:
+          GH_TOKEN: ${{ github.token }}
+          GH_REPO: ${{ github.repository }}
diff --git a/lintcheck/src/config.rs b/lintcheck/src/config.rs
@@ -68,6 +68,9 @@ pub(crate) enum Commands {
         /// This will limit the number of warnings that will be printed for each lint
         #[clap(long)]
         truncate: bool,
+        /// Write the diff summary to a JSON file if there are any changes
+        #[clap(long, value_name = "PATH")]
+        write_summary: Option<PathBuf>,
     },
     /// Create a lintcheck crates TOML file containing the top N popular crates
     Popular {
diff --git a/lintcheck/src/json.rs b/lintcheck/src/json.rs
@@ -4,8 +4,8 @@
 //! loading warnings from JSON files, and generating human-readable diffs
 //! between different linting runs.
 
-use std::fs;
-use std::path::Path;
+use std::path::{Path, PathBuf};
+use std::{fmt, fs};
 
 use itertools::{EitherOrBoth, Itertools};
 use serde::{Deserialize, Serialize};
@@ -17,7 +17,6 @@ const DEFAULT_LIMIT_PER_LINT: usize = 300;
 /// Target for total warnings to display across all lints when truncating output.
 const TRUNCATION_TOTAL_TARGET: usize = 1000;
 
-/// Representation of a single Clippy warning for JSON serialization.
 #[derive(Debug, Deserialize, Serialize)]
 struct LintJson {
     /// The lint name e.g. `clippy::bytes_nth`
@@ -29,7 +28,6 @@ struct LintJson {
 }
 
 impl LintJson {
-    /// Returns a tuple of name and `file_line` for sorting and comparison.
     fn key(&self) -> impl Ord + '_ {
         (self.name.as_str(), self.file_line.as_str())
     }
@@ -40,6 +38,57 @@ impl LintJson {
     }
 }
 
+#[derive(Debug, Serialize)]
+struct SummaryRow {
+    name: String,
+    added: usize,
+    removed: usize,
+    changed: usize,
+}
+
+#[derive(Debug, Serialize)]
+struct Summary(Vec<SummaryRow>);
+
+impl fmt::Display for Summary {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(
+            "\
+| Lint | Added | Removed | Changed |
+| ---- | ----: | ------: | ------: |
+",
+        )?;
+
+        for SummaryRow {
+            name,
+            added,
+            changed,
+            removed,
+        } in &self.0
+        {
+            let html_id = to_html_id(name);
+            writeln!(f, "| [`{name}`](#{html_id}) | {added} | {changed} | {removed} |")?;
+        }
+
+        Ok(())
+    }
+}
+
+impl Summary {
+    fn new(lints: &[LintWarnings]) -> Self {
+        Summary(
+            lints
+                .iter()
+                .map(|lint| SummaryRow {
+                    name: lint.name.clone(),
+                    added: lint.added.len(),
+                    removed: lint.removed.len(),
+                    changed: lint.changed.len(),
+                })
+                .collect(),
+        )
+    }
+}
+
 /// Creates the log file output for [`crate::config::OutputFormat::Json`]
 pub(crate) fn output(clippy_warnings: Vec<ClippyWarning>) -> String {
     let mut lints: Vec<LintJson> = clippy_warnings
@@ -74,7 +123,7 @@ fn load_warnings(path: &Path) -> Vec<LintJson> {
 ///
 /// Compares warnings from `old_path` and `new_path`, then displays a summary table
 /// and detailed information about added, removed, and changed warnings.
-pub(crate) fn diff(old_path: &Path, new_path: &Path, truncate: bool) {
+pub(crate) fn diff(old_path: &Path, new_path: &Path, truncate: bool, write_summary: Option<PathBuf>) {
     let old_warnings = load_warnings(old_path);
     let new_warnings = load_warnings(new_path);
 
@@ -108,13 +157,16 @@ pub(crate) fn diff(old_path: &Path, new_path: &Path, truncate: bool) {
         }
     }
 
-    print_summary_table(&lint_warnings);
-    println!();
-
     if lint_warnings.is_empty() {
         return;
     }
 
+    let summary = Summary::new(&lint_warnings);
+    if let Some(path) = write_summary {
+        let json = serde_json::to_string(&summary).unwrap();
+        fs::write(path, json).unwrap();
+    }
+
     let truncate_after = if truncate {
         // Max 15 ensures that we at least have five messages per lint
         DEFAULT_LIMIT_PER_LINT
@@ -126,6 +178,7 @@ pub(crate) fn diff(old_path: &Path, new_path: &Path, truncate: bool) {
         usize::MAX
     };
 
+    println!("{summary}");
     for lint in lint_warnings {
         print_lint_warnings(&lint, truncate_after);
     }
@@ -140,13 +193,11 @@ struct LintWarnings {
     changed: Vec<(LintJson, LintJson)>,
 }
 
-/// Prints a formatted report for a single lint type with its warnings.
 fn print_lint_warnings(lint: &LintWarnings, truncate_after: usize) {
     let name = &lint.name;
     let html_id = to_html_id(name);
 
-    // The additional anchor is added for non GH viewers that don't prefix ID's
-    println!(r#"## `{name}` <a id="user-content-{html_id}"></a>"#);
+    println!(r#"<h2 id="{html_id}"><code>{name}</code></h2>"#);
     println!();
 
     print!(
@@ -162,22 +213,6 @@ fn print_lint_warnings(lint: &LintWarnings, truncate_after: usize) {
     print_changed_diff(&lint.changed, truncate_after / 3);
 }
 
-/// Prints a summary table of all lints with counts of added, removed, and changed warnings.
-fn print_summary_table(lints: &[LintWarnings]) {
-    println!("| Lint                                       | Added   | Removed | Changed |");
-    println!("| ------------------------------------------ | ------: | ------: | ------: |");
-
-    for lint in lints {
-        println!(
-            "| {:<62} | {:>7} | {:>7} | {:>7} |",
-            format!("[`{}`](#user-content-{})", lint.name, to_html_id(&lint.name)),
-            lint.added.len(),
-            lint.removed.len(),
-            lint.changed.len()
-        );
-    }
-}
-
 /// Prints a section of warnings with a header and formatted code blocks.
 fn print_warnings(title: &str, warnings: &[LintJson], truncate_after: usize) {
     if warnings.is_empty() {
@@ -248,17 +283,16 @@ fn truncate<T>(list: &[T], truncate_after: usize) -> &[T] {
     }
 }
 
-/// Prints a level 3 heading with an appropriate HTML ID for linking.
 fn print_h3(lint: &str, title: &str) {
     let html_id = to_html_id(lint);
-    // We have to use HTML here to be able to manually add an id.
-    println!(r#"### {title} <a id="user-content-{html_id}-{title}"></a>"#);
+    // We have to use HTML here to be able to manually add an id, GitHub doesn't add them automatically
+    println!(r#"<h3 id="{html_id}-{title}">{title}</h3>"#);
 }
 
-/// GitHub's markdown parsers doesn't like IDs with `::` and `_`. This simplifies
-/// the lint name for the HTML ID.
+/// Creates a custom ID allowed by GitHub, they must start with `user-content-` and cannot contain
+/// `::`/`_`
 fn to_html_id(lint_name: &str) -> String {
-    lint_name.replace("clippy::", "").replace('_', "-")
+    lint_name.replace("clippy::", "user-content-").replace('_', "-")
 }
 
 /// This generates the `x added` string for the start of the job summery.
@@ -270,9 +304,6 @@ fn count_string(lint: &str, label: &str, count: usize) -> String {
         format!("0 {label}")
     } else {
         let html_id = to_html_id(lint);
-        // GitHub's job summaries don't add HTML ids to headings. That's why we
-        // manually have to add them. GitHub prefixes these manual ids with
-        // `user-content-` and that's how we end up with these awesome links :D
-        format!("[{count} {label}](#user-content-{html_id}-{label})")
+        format!("[{count} {label}](#{html_id}-{label})")
     }
 }
diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs
@@ -297,7 +297,12 @@ fn main() {
     let config = LintcheckConfig::new();
 
     match config.subcommand {
-        Some(Commands::Diff { old, new, truncate }) => json::diff(&old, &new, truncate),
+        Some(Commands::Diff {
+            old,
+            new,
+            truncate,
+            write_summary,
+        }) => json::diff(&old, &new, truncate, write_summary),
         Some(Commands::Popular { output, number }) => popular_crates::fetch(output, number).unwrap(),
         None => lintcheck(config),
     }
