Create summary comment for lintcheck runs (#14816)

Previously rust-lang/triagebot#1985

After a lintcheck run a comment will be created in the PR thread with an
overview of the changes, example here
Alexendoo/rust-clippy#18 (comment)
(plus the normal GHA debugging experience)

It will only comment if there are some changes, if there's already an
existing comment it will be updated for each run

Similar to
https://github.com/rust-lang/team/blob/master/.github/workflows/dry-run.yml

The PR number is supplied by the lintcheck run, so technically someone
could forge it to be annoying and edit the summaries in other threads,
but that is pretty low impact and easy to deal with. There is a
`pull_requests` field on the event but as @Kobzol [pointed out to
me](rust-lang/triagebot#1985 (comment))
it's not populated for PRs from forks

r? @flip1995

changelog: none

Author: flip1995

.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

.github/workflows/lintcheck_summary.yml

@@ -0,0 +1,106 @@
+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
+            # There were no changes detected by Lintcheck
+            # - If a comment exists from a previous run that did detect changes, edit it (--edit-last)
+            # - If no comment exists do not create one, the `gh` command exits with an error which
+            #   `|| true` ignores
+            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 }}

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 {

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})")
     }
 }

lintcheck/src/main.rs

@@ -303,7 +303,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),
     }