Sampler: pass template names newline-separated so spaced account template names survive

.github/workflows/run_sampler.yml

@@ -33,12 +33,12 @@ on:
         required: true
         type: string
       handles:
-        description: "Reconciliation text handles to sample, space-separated (directory names under reconciliation_texts/). Optional if account_templates is set."
+        description: "Reconciliation text handles to sample, ONE PER LINE (directory names under reconciliation_texts/). Newline-separated, not space-separated — names may contain spaces. Optional if account_templates is set."
         required: false
         type: string
         default: ""
       account_templates:
-        description: "Account template names to sample, space-separated (directory names under account_templates/). Optional if handles is set."
+        description: "Account template names to sample, ONE PER LINE (directory names under account_templates/). Newline-separated, not space-separated — these names routinely contain spaces. Optional if handles is set."
         required: false
         type: string
         default: ""
@@ -130,10 +130,11 @@ jobs:
 
       - name: Install silverfin-cli
         run: |
-          # Unpinned, matching every other production workflow's convention (none pin to a
-          # branch/SHA/tag) — relies on run-sampler + --compact + result-URL surfacing being on
-          # main (silverfin-cli PR #261, "agustin-bso-sampler-easy-results", merged 2026-07-15).
-          npm install https://github.com/silverfin/silverfin-cli.git
+          # Pinned to a commit on silverfin-cli's sampler-compact-diff-v2 branch, since
+          # --add-diffs-folder (used below) is not yet on main. Switch back to installing
+          # unpinned main, matching every other production workflow's convention, once that
+          # branch's PR merges.
+          npm install https://github.com/silverfin/silverfin-cli.git#5accd6b
           VERSION=$(node ./node_modules/silverfin-cli/bin/cli.js -V)
           echo "CLI version: ${VERSION}"
 
@@ -169,14 +170,22 @@ jobs:
           SAMPLER_ACCOUNT_TEMPLATES: ${{ inputs.account_templates }}
           SAMPLER_FIRM_IDS: ${{ inputs.firm_ids }}
         run: |
-          # Intentional word-splitting: handles/account templates arrive as a single
-          # space-separated string and must become separate CLI args (shellcheck SC2206).
+          # One name per line, and each line becomes exactly ONE CLI arg. Newline-separated (not
+          # space-separated) keeps identifiers that contain spaces intact — account template
+          # names do (e.g. "Investment- and depreciation details"), and word-splitting them
+          # handed the CLI "Investment-" as a template name:
+          #   [error] Config file for account template "Investment-" not found
+          # Same convention as run_tests.yml's TEMPLATE_BUCKETS.
           HANDLE_ARGS=()
-          # shellcheck disable=SC2206
-          [[ -n "${SAMPLER_HANDLES}" ]] && HANDLE_ARGS=(-h ${SAMPLER_HANDLES})
+          if [[ -n "${SAMPLER_HANDLES//[[:space:]]/}" ]]; then
+            mapfile -t HANDLE_NAMES < <(printf '%s\n' "${SAMPLER_HANDLES}" | sed '/^[[:space:]]*$/d')
+            HANDLE_ARGS=(-h "${HANDLE_NAMES[@]}")
+          fi
           ACCOUNT_ARGS=()
-          # shellcheck disable=SC2206
-          [[ -n "${SAMPLER_ACCOUNT_TEMPLATES}" ]] && ACCOUNT_ARGS=(-at ${SAMPLER_ACCOUNT_TEMPLATES})
+          if [[ -n "${SAMPLER_ACCOUNT_TEMPLATES//[[:space:]]/}" ]]; then
+            mapfile -t ACCOUNT_NAMES < <(printf '%s\n' "${SAMPLER_ACCOUNT_TEMPLATES}" | sed '/^[[:space:]]*$/d')
+            ACCOUNT_ARGS=(-at "${ACCOUNT_NAMES[@]}")
+          fi
 
           # GitHub Actions concurrency (group + queue: max) serializes runs within THIS repo, but
           # concurrency groups do not span repositories — two market repos sharing this partner id
@@ -186,7 +195,10 @@ jobs:
           DEADLINE=$(( $(date +%s) + 90*60 ))
           ATTEMPT=1
           while true; do
-            echo "[$(date -u +%H:%M:%S)] run-sampler attempt ${ATTEMPT}: -p ${SAMPLER_PARTNER} ${HANDLE_ARGS[*]} ${ACCOUNT_ARGS[*]} --firm-ids ${SAMPLER_FIRM_IDS} --compact"
+            # %q-quoted so a name containing spaces is visibly one argument in the log.
+            printf '[%s] run-sampler attempt %s: -p %s' "$(date -u +%H:%M:%S)" "${ATTEMPT}" "${SAMPLER_PARTNER}"
+            printf ' %q' "${HANDLE_ARGS[@]}" "${ACCOUNT_ARGS[@]}"
+            printf ' --firm-ids %s --compact\n' "${SAMPLER_FIRM_IDS}"
             set +e
             # shellcheck disable=SC2086 # SAMPLER_FIRM_IDS is intentionally word-split (space-separated ids -> separate args)
             OUTPUT=$(node ./node_modules/silverfin-cli/bin/cli.js run-sampler -p "${SAMPLER_PARTNER}" "${HANDLE_ARGS[@]}" "${ACCOUNT_ARGS[@]}" --firm-ids ${SAMPLER_FIRM_IDS} --compact 2>&1 | tr -d '\r')
@@ -281,21 +293,41 @@ jobs:
 
       - name: Download results.zip for the workflow artifact
         id: download
-        if: steps.sampler.outputs.report_url != ''
+        if: always() && steps.sampler.outputs.report_url != ''
         env:
           REPORT_URL: ${{ steps.sampler.outputs.report_url }}
         run: |
           # The CLI's --compact path downloads to a temp dir and deletes it after printing the
           # diff, so results.zip never lands on disk on its own — fetch it again here for the
           # artifact (Q12: full zip kept for rendering-only regressions the compact diff can't see).
+          # Note: the presigned REPORT_URL itself is short-lived (measured at ~5 minutes past
+          # run completion, not the long-lived link it was originally assumed to be — see
+          # silverfin-cli's CI_AUTH_SAMPLER_PLAN.md §10.4). A failure here means the human-facing
+          # PR comment link below falls back to that presigned URL, which may already be dead.
           if curl -sL --fail --max-time 300 -o results.zip "${REPORT_URL}"; then
             echo "downloaded=true" >> "$GITHUB_OUTPUT"
           else
-            echo "::warning::Could not download results.zip from the report URL for the artifact upload (the report link itself is still valid)."
+            echo "::warning::Could not download results.zip from the report URL for the artifact upload."
             echo "downloaded=false" >> "$GITHUB_OUTPUT"
           fi
 
+      - name: Add diffs/ folder for the entries the compact diff flagged
+        id: add_diffs
+        # Pure local re-analysis of the zip already on disk - no network call, no partner API
+        # (silverfin-cli's --from-zip path). Best-effort: a failure here shouldn't cost the
+        # reviewer the plain results.zip artifact they'd otherwise get.
+        if: always() && steps.download.outputs.downloaded == 'true'
+        continue-on-error: true
+        run: |
+          if node ./node_modules/silverfin-cli/bin/cli.js run-sampler --from-zip results.zip --add-diffs-folder; then
+            echo "diffs_added=true" >> "$GITHUB_OUTPUT"
+          else
+            echo "::warning::Failed to add diffs/ folder to results.zip — the uploaded artifact is the plain sampler output for this run."
+            echo "diffs_added=false" >> "$GITHUB_OUTPUT"
+          fi
+
       - name: Upload results.zip artifact
+        id: upload
         if: always() && steps.download.outputs.downloaded == 'true'
         uses: actions/upload-artifact@v4
         with:
@@ -305,14 +337,16 @@ jobs:
 
       - name: Post result comment on the PR
         if: always() && inputs.pull_request_number != ''
-        uses: actions/github-script@v7
+        uses: actions/github-script@v8
         env:
           PARTNER: ${{ inputs.partner }}
           HANDLES: ${{ inputs.handles }}
           ACCOUNT_TEMPLATES: ${{ inputs.account_templates }}
           FIRM_IDS: ${{ inputs.firm_ids }}
           SAMPLER_OK: ${{ steps.sampler.outputs.sampler_ok }}
           REPORT_URL: ${{ steps.sampler.outputs.report_url }}
+          ARTIFACT_URL: ${{ steps.upload.outputs.artifact-url }}
+          DIFFS_ADDED: ${{ steps.add_diffs.outputs.diffs_added }}
           COMPACT: ${{ steps.sampler.outputs.compact }}
           PR_NUMBER: ${{ inputs.pull_request_number }}
           RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
@@ -321,6 +355,13 @@ jobs:
             const marker = `<!-- silverfin-sampler-result-${process.env.PARTNER} -->`;
             const ok = process.env.SAMPLER_OK === "true";
             const reportUrl = process.env.REPORT_URL || "";
+            // The backend's presigned reportUrl expires ~5 minutes after the run completes
+            // (CI_AUTH_SAMPLER_PLAN.md §10.4) — by the time a human reads this comment it's
+            // always dead. Prefer the GitHub Actions artifact link, which stays valid for the
+            // full 7-day retention window; fall back to the presigned URL only if the artifact
+            // upload itself didn't happen (e.g. the post-run download failed).
+            const artifactUrl = process.env.ARTIFACT_URL || "";
+            const diffsAdded = process.env.DIFFS_ADDED || "";
             const compact = (process.env.COMPACT || "").trim();
             const runUrl = process.env.RUN_URL;
 
@@ -331,10 +372,23 @@ jobs:
               lines.push(`⚠️ The sampler run did not complete cleanly — see the [workflow run](${runUrl}) for details.`);
             }
             lines.push("");
-            if (process.env.HANDLES) lines.push(`- Reconciliation handles: \`${process.env.HANDLES}\``);
-            if (process.env.ACCOUNT_TEMPLATES) lines.push(`- Account templates: \`${process.env.ACCOUNT_TEMPLATES}\``);
+            // Newline-separated on the way in (names can contain spaces) — render one code-quoted
+            // name per entry, comma-separated. A multi-line value inside one backtick pair
+            // renders as garbage in a PR comment.
+            const fmtNames = v => (v || "").split("\n").map(s => s.trim()).filter(Boolean).map(s => `\`${s}\``).join(", ");
+            if (process.env.HANDLES) lines.push(`- Reconciliation handles: ${fmtNames(process.env.HANDLES)}`);
+            if (process.env.ACCOUNT_TEMPLATES) lines.push(`- Account templates: ${fmtNames(process.env.ACCOUNT_TEMPLATES)}`);
             lines.push(`- Firm(s): \`${process.env.FIRM_IDS}\``);
-            if (reportUrl) lines.push(`- **[📊 Open full sampler report](${reportUrl})** (downloads \`results.zip\`)`);
+            if (artifactUrl) {
+              lines.push(`- **[📊 Open full sampler report](${artifactUrl})** (GitHub sign-in required; downloads \`results.zip\`, kept 7 days)`);
+              if (diffsAdded === "false") {
+                lines.push(`  - ⚠️ Could not add the \`diffs/\` folder to this artifact — see the [workflow run](${runUrl}) logs. The plain \`results.zip\` is still there.`);
+              }
+            } else if (reportUrl) {
+              lines.push(`- **[📊 Open full sampler report](${reportUrl})** (downloads \`results.zip\` — this direct link expires ~5 minutes after the run, may already be gone)`);
+            } else {
+              lines.push(`- ⚠️ No sampler report link available for this run — see the [workflow run](${runUrl}) logs.`);
+            }
             lines.push(`- Workflow run: ${runUrl}`);
             lines.push("");
             if (compact) {

README.md

@@ -260,8 +260,9 @@ _Trigger:_
 _Inputs:_
 
 * `partner` (required) — partner environment id (must be authorized — see `PARTNER_CONFIG_JSON` secret).
-* `handles` (optional) — reconciliation text handles to sample, space-separated (directory names under `reconciliation_texts/`). Optional if `account_templates` is set.
-* `account_templates` (optional) — account template names to sample, space-separated (directory names under `account_templates/`). Optional if `handles` is set.
+* `handles` (optional) — reconciliation text handles to sample, **one per line** (directory names under `reconciliation_texts/`). Optional if `account_templates` is set.
+* `account_templates` (optional) — account template names to sample, **one per line** (directory names under `account_templates/`). Optional if `handles` is set.
+  * Both lists are newline-separated, **not** space-separated: account template directory names routinely contain spaces (e.g. `Investment- and depreciation details`), so a space-joined list is ambiguous and gets word-split into template names that don't exist (`Config file for account template "Investment-" not found`). Same convention as [`run_tests.yml`](#run-liquid-tests-run_testsyml). `firm_ids` is the exception — numeric, so it stays space-separated.
 * `firm_ids` (required) — firm id(s) to sample against, space-separated. The backend 422s if empty.
 * `ref` (required) — git ref (commit SHA) to check out — the PR head, so sampled template content matches the PR under review.
 * `pull_request_number` (optional) — PR number to post the result comment on. If empty, no comment is posted (results still upload as an artifact).
@@ -273,8 +274,8 @@ _Steps:_
 * Loads the partner's credentials from the `PARTNER_CONFIG_JSON` secret and captures the token on disk before the run.
 * Runs `run-sampler`, retrying on a cross-repo "already in progress" 422 (the backend allows only one sampler run per partner at a time; retries for up to 90 minutes).
 * Captures the token again after the run and writes it back to `PARTNER_CONFIG_JSON_<partner>` via `gh secret set` only if it rotated (401 refresh mid-run).
-* Downloads `results.zip` and uploads it as a short-lived (7-day) workflow artifact, for the rare rendering-only regression the compact diff can't see.
-* Posts (or updates) a result comment on the PR with the compact diff and, when a report URL was produced, a link to the full report — otherwise the `results.zip` artifact is the fallback.
+* Downloads `results.zip`, best-effort adds a `diffs/` folder of before/after `view.html` for the entries the compact diff flagged, and uploads it as a 7-day workflow artifact.
+* Posts (or updates) a result comment on the PR with the compact diff and a link to the workflow artifact (kept 7 days; GitHub sign-in required) as the primary way to open the full report; falls back to the presigned report URL (short-lived, ~5 min) only if the artifact upload did not happen.
 * Fails the job if the sampler run did not complete successfully.
 
 _Authentication note:_