refactor: simplify report/format helpers (/simplify pass) (#118)

Quality cleanups from a 4-angle review of the composite-action diff:

- report-common.mjs: add buildReportContext(context); both report wrappers
  (format-report, jscpd-report) now call it instead of hand-building the identical
  owner/repo/sha/cwd/runId/commit object.
- jscpd-report.mjs: buildJscpdMessage takes the pre-computed `header` (like
  generateReport) instead of recomputing getReportHeader internally; wrapper passes it.
- format-args.mjs: collapse buildDefaultOptions' 4 near-identical blocks to a base +
  spread (only the two real overrides remain); simplify checkIsDryRun's granular check
  via a `dry()` helper; drop the unused `config` field from planFormat's return; make
  internal-only buildArgs/FormatType module-private.
- comment.mjs: run the independent listComments + getAuthenticated calls with
  Promise.all (getAuthenticated still degrades to undefined on 403).
- action.yml: write the push stderr log to $RUNNER_TEMP instead of the repo tree, and
  drop the duplicate ::error:: annotation on exec failure (per-command one remains).

Skipped (documented as follow-ups): unifying the jscpd config resolution that happens
in both the shell runner and the JS threshold read (structural; risky without an e2e
run), and the github-script import boilerplate (explicit per-step is clearer).

biome check + 33 node:test green; wrappers smoke-tested; bash -n + YAML clean.

Claude-Session: https://claude.ai/code/session_01JxEe85CyGm5rCeoLFdUZed

Author: maxisam

__tests__/jscpd-report.test.mjs

@@ -34,7 +34,8 @@ describe('isOverThreshold', () => {
 
 describe('buildJscpdMessage', () => {
     it('strips the default header, adds links and footer', () => {
-        const md = buildJscpdMessage('# Copy/paste detection report\n\nsome table', duplicates, '/repo/src', '/repo/src', ctx);
+        const header = getReportHeader('/repo/src');
+        const md = buildJscpdMessage('# Copy/paste detection report\n\nsome table', duplicates, header, '/repo/src', ctx);
         assert.match(md, /## ❌ DUPLICATED CODE FOUND - \/repo\/src/);
         assert.doesNotMatch(md, /# Copy\/paste detection report/);
         assert.match(md, /<summary> JSCPD Details <\/summary>/);

action.yml

@@ -213,9 +213,10 @@ runs:
         echo "formatResult=$format_result" >> "$GITHUB_OUTPUT"
 
         # Infrastructure/runner failures fail unconditionally; formatting findings
-        # remain gated by the failFast step.
+        # remain gated by the failFast step. The per-command ::error:: above already
+        # annotated the failure, so just exit here.
         if [ "$exec_failed" = "true" ]; then
-          echo "::error title=DOTNET FORMAT Check::dotnet format failed to execute"
+          echo "dotnet format failed to execute (see error above)"
           exit 1
         fi
 
@@ -311,11 +312,12 @@ runs:
         attempt=1
         while [ "$attempt" -le 3 ]; do
           echo "Pushing changes… (attempt $attempt of 3)"
+          ERR_LOG="$RUNNER_TEMP/df-push-err.log"
           set +e
-          push_out=$(git push --porcelain origin "$BRANCH:$BRANCH" 2>push_err.log)
+          push_out=$(git push --porcelain origin "$BRANCH:$BRANCH" 2>"$ERR_LOG")
           set -e
-          push_err=$(cat push_err.log 2>/dev/null || true)
-          rm -f push_err.log
+          push_err=$(cat "$ERR_LOG" 2>/dev/null || true)
+          rm -f "$ERR_LOG"
           if printf '%s' "$push_out" | grep -qE '\[rejected\]|\[remote rejected\]'; then
             echo "Updates were rejected; fetching and rebasing onto $BRANCH"
             git fetch

scripts/format-args.mjs

@@ -10,7 +10,7 @@ import process from 'node:process';
 /** Mirrors REPORT_PATH from src/common.ts (computed from cwd at load time). */
 export const DEFAULT_REPORT_DIR = `${process.cwd()}/.dotnet-format`;
 
-export const FormatType = {
+const FormatType = {
     all: 'all',
     style: 'style',
     analyzers: 'analyzers',
@@ -45,7 +45,7 @@ function resolveEnabled(block, defaultValue) {
  * @param {string} type
  * @returns {string[]}
  */
-export function buildArgs(options, onlyChangedFiles, changedFiles, type) {
+function buildArgs(options, onlyChangedFiles, changedFiles, type) {
     /** @type {string[]} */
     const args = [];
     if (options.verifyNoChanges) {
@@ -119,37 +119,22 @@ export function generateFormatCommandArgs(config, workspace, changedFiles = [],
  * @returns {Record<string, any>}
  */
 export function buildDefaultOptions(inputs) {
-    const verifyNoChanges = inputs.action === 'check';
+    // Shared per-block defaults; the blocks differ only in the two overrides below.
+    const base = {
+        verifyNoChanges: inputs.action === 'check',
+        severity: inputs.severityLevel,
+        verbosity: inputs.logLevel,
+        noRestore: !!inputs.nugetConfigPath
+    };
     return {
         nugetConfigPath: inputs.nugetConfigPath,
         projectFileName: inputs.projectFileName,
         onlyChangedFiles: inputs.onlyChangedFiles,
-        options: {
-            verifyNoChanges,
-            severity: inputs.severityLevel,
-            verbosity: inputs.logLevel,
-            noRestore: !!inputs.nugetConfigPath
-        },
-        whitespaceOptions: {
-            verifyNoChanges,
-            folder: true,
-            severity: inputs.severityLevel,
-            verbosity: inputs.logLevel,
-            noRestore: !!inputs.nugetConfigPath
-        },
-        analyzersOptions: {
-            verifyNoChanges,
-            severity: inputs.severityLevel,
-            verbosity: inputs.logLevel,
-            noRestore: !!inputs.nugetConfigPath
-        },
-        styleOptions: {
-            verifyNoChanges,
-            severity: inputs.severityLevel,
-            verbosity: inputs.logLevel,
-            // Quirk preserved from the original: style uses dotnetFormatConfigPath here.
-            noRestore: !!inputs.dotnetFormatConfigPath
-        }
+        options: { ...base },
+        whitespaceOptions: { ...base, folder: true },
+        analyzersOptions: { ...base },
+        // Quirk preserved from the original: style keys noRestore off dotnetFormatConfigPath.
+        styleOptions: { ...base, noRestore: !!inputs.dotnetFormatConfigPath }
     };
 }
 
@@ -177,25 +162,22 @@ export function checkIsDryRun(config) {
     if (isEnabledBlock(config.options, false)) {
         return config.options?.verifyNoChanges ?? false;
     }
-    const wEnabled = isEnabledBlock(config.whitespaceOptions, false);
-    const aEnabled = isEnabledBlock(config.analyzersOptions, false);
-    const sEnabled = isEnabledBlock(config.styleOptions, false);
-    const w = (wEnabled && !!config.whitespaceOptions?.verifyNoChanges) || !wEnabled;
-    const a = (aEnabled && !!config.analyzersOptions?.verifyNoChanges) || !aEnabled;
-    const s = (sEnabled && !!config.styleOptions?.verifyNoChanges) || !sEnabled;
-    return w && a && s;
+    // A granular block is "dry" when it is disabled, or enabled and verifying no
+    // changes — i.e. `(enabled && verify) || !enabled` simplifies to `!enabled || verify`.
+    const dry = block => !isEnabledBlock(block, false) || !!block?.verifyNoChanges;
+    return dry(config.whitespaceOptions) && dry(config.analyzersOptions) && dry(config.styleOptions);
 }
 
 /**
  * Convenience: finalize a merged config and emit everything the runner needs.
  * @param {Record<string, any>} mergedConfig
  * @param {{ workspace: string, changedFiles?: string[], isOnlyChangedFiles?: boolean, reportDir?: string }} opts
- * @returns {{ config: Record<string, any>, commands: string[][], isDryRun: boolean }}
+ * @returns {{ commands: string[][], isDryRun: boolean }}
  */
 export function planFormat(mergedConfig, opts) {
     const { workspace, changedFiles = [], isOnlyChangedFiles = false, reportDir = DEFAULT_REPORT_DIR } = opts;
     const config = finalizeEnabled(mergedConfig);
     const commands = generateFormatCommandArgs(config, workspace, changedFiles, { isOnlyChangedFiles, reportDir });
     const isDryRun = checkIsDryRun(config);
-    return { config, commands, isDryRun };
+    return { commands, isDryRun };
 }

scripts/jscpd-report.mjs

@@ -49,15 +49,17 @@ export function isOverThreshold(report, threshold) {
 
 /**
  * Build the PR comment / summary message from the jscpd markdown report content
- * and the parsed duplicates. Mirrors the message built in postReport.
+ * and the parsed duplicates. Mirrors the message built in postReport. Takes the
+ * pre-computed `header` (like `generateReport` in dotnet-report.mjs) so the caller
+ * does not compute it twice.
  * @param {string} markdownReportContent  Raw contents of jscpd's markdown report.
  * @param {any[]} duplicates
- * @param {string} workspace
+ * @param {string} header
  * @param {string} scanPath
  * @param {import('./report-common.mjs').ReportContext} ctx
  * @returns {string}
  */
-export function buildJscpdMessage(markdownReportContent, duplicates, workspace, scanPath, ctx) {
+export function buildJscpdMessage(markdownReportContent, duplicates, header, scanPath, ctx) {
     const report = markdownReportContent.replace('# Copy/paste detection report', '');
     let markdown = '<details>\n';
     markdown += ` <summary> JSCPD Details </summary>\n\n`;
@@ -68,7 +70,6 @@ export function buildJscpdMessage(markdownReportContent, duplicates, workspace,
         markdown += '\n';
     }
     markdown += '</details>\n';
-    const header = getReportHeader(workspace);
     return `${header} \n\n${report}\n\n ${markdown}\n\n ${getReportFooter(ctx)}`;
 }
 

scripts/report-common.mjs

@@ -12,6 +12,24 @@
  * @property {string} [commit]  Commit to credit in the footer (PR head sha or sha).
  */
 
+/**
+ * Build the {@link ReportContext} from github-script's `context` (owner/repo/sha +
+ * the PR-head-or-sha commit to credit). Shared by the dotnet format and jscpd report
+ * steps so the context shape lives in one place.
+ * @param {any} context
+ * @returns {ReportContext}
+ */
+export function buildReportContext(context) {
+    return {
+        owner: context.repo.owner,
+        repo: context.repo.repo,
+        sha: context.sha,
+        cwd: process.cwd(),
+        runId: context.runId,
+        commit: context.payload?.pull_request?.head?.sha || context.sha
+    };
+}
+
 /**
  * @param {ReportContext} ctx
  * @returns {string}

scripts/steps/comment.mjs

@@ -10,18 +10,19 @@
  * @returns {Promise<number | undefined>}
  */
 export async function getExistingCommentId(github, context, header) {
-    const comments = await github.rest.issues.listComments({
-        owner: context.repo.owner,
-        repo: context.repo.repo,
-        issue_number: context.issue.number
-    });
-    let userLogin;
-    try {
-        userLogin = (await github.rest.users.getAuthenticated()).data?.login;
-    } catch {
-        // token without user scope; fall back to matching the Bot user type
-        userLogin = undefined;
-    }
+    // The two calls are independent; run them together. getAuthenticated may 403 on a
+    // token without user scope — fall back to matching the Bot user type in that case.
+    const [comments, userLogin] = await Promise.all([
+        github.rest.issues.listComments({
+            owner: context.repo.owner,
+            repo: context.repo.repo,
+            issue_number: context.issue.number
+        }),
+        github.rest.users
+            .getAuthenticated()
+            .then(r => r.data?.login)
+            .catch(() => undefined)
+    ]);
     const existing = comments.data?.find(c => {
         const isBotUserType = c.user?.type === 'Bot' || c.user?.login === userLogin;
         return isBotUserType && c.body?.startsWith(header);

scripts/steps/format-report.mjs

@@ -3,6 +3,7 @@
 // github-script's { github, context, core }; report dir + inputs arrive via env.
 
 import { generateReport, getReportFiles, getReportHeader } from '../dotnet-report.mjs';
+import { buildReportContext } from '../report-common.mjs';
 import { upsertComment } from './comment.mjs';
 
 /**
@@ -21,15 +22,7 @@ export async function run({ github, context, core }) {
     }
 
     const header = getReportHeader(workspace);
-    const reportCtx = {
-        owner: context.repo.owner,
-        repo: context.repo.repo,
-        sha: context.sha,
-        cwd: process.cwd(),
-        runId: context.runId,
-        commit: context.payload?.pull_request?.head?.sha || context.sha
-    };
-    const message = generateReport(reportFiles, header, reportCtx);
+    const message = generateReport(reportFiles, header, buildReportContext(context));
     if (!message) {
         return;
     }

scripts/steps/jscpd-report.mjs

@@ -8,6 +8,7 @@ import * as fs from 'node:fs';
 import path from 'node:path';
 import { buildAnnotations, buildJscpdMessage, getReportHeader, isOverThreshold } from '../jscpd-report.mjs';
 import { resolveConfig } from '../read-config.mjs';
+import { buildReportContext } from '../report-common.mjs';
 import { upsertComment } from './comment.mjs';
 import { makeLoader } from './load-config.mjs';
 
@@ -46,21 +47,14 @@ export async function run({ github, context, core, exec, io }) {
     const threshold = cfg.threshold ?? 0;
 
     const cwd = process.cwd();
-    const reportCtx = {
-        owner: context.repo.owner,
-        repo: context.repo.repo,
-        sha: context.sha,
-        cwd,
-        runId: context.runId,
-        commit: context.payload?.pull_request?.head?.sha || context.sha
-    };
+    const reportCtx = buildReportContext(context);
     const header = getReportHeader(workspace);
 
     // Build the message from jscpd's markdown report, post it, and overwrite the
     // markdown file with the full message (so the uploaded artifact matches).
     const mdFile = findByExt(outputDir, '.md');
     const mdContent = mdFile ? fs.readFileSync(mdFile, 'utf8') : '';
-    const message = buildJscpdMessage(mdContent, duplicates, workspace, scanPath, reportCtx);
+    const message = buildJscpdMessage(mdContent, duplicates, header, scanPath, reportCtx);
     if (mdFile) {
         fs.writeFileSync(mdFile, message);
     }