diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 94faabf..8356ab9 100644 --- a/.github/workflows/run_sampler.yml +++ b/.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,7 +337,7 @@ 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 }} @@ -313,6 +345,8 @@ jobs: 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 = ``; 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) { diff --git a/README.md b/README.md index 53c8211..10a3475 100644 --- a/README.md +++ b/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_` 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:_