From 999b7d73f5ed7a783c7ae27bef7003d94ed96a3e Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 15 Jul 2026 15:39:35 +0200 Subject: [PATCH 1/7] Add reusable run_sampler.yml workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Given a partner id, classified handles/account templates, and firm ids, loads that partner's PARTNER_CONFIG_JSON secret, runs the Liquid Sampler CLI with retry-on-cross-repo-422, writes the partner token back only if it rotated mid-run, uploads results.zip as a short-lived artifact, and posts a per-partner PR comment. Market-repo-specific classification (which templates changed, which partner they belong to) stays in each caller's wrapper workflow. Implements the locked sampler design from silverfin-cli's CI_AUTH_SAMPLER_PLAN.md (§6 D1-D9, §8) — first caller is lu_market. --- .github/workflows/run_sampler.yml | 332 ++++++++++++++++++++++++++++++ 1 file changed, 332 insertions(+) create mode 100644 .github/workflows/run_sampler.yml diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml new file mode 100644 index 0000000..9f431fd --- /dev/null +++ b/.github/workflows/run_sampler.yml @@ -0,0 +1,332 @@ +# Reusable workflow. Runs the Liquid Sampler (silverfin-cli `run-sampler`) for a single +# partner's changed reconciliation/account templates and posts the result on a PR. +# +# This is deliberately partner-agnostic and repo-layout-agnostic: given a partner id, a list +# of already-classified handles/account templates, and firm ids, it loads that partner's +# credentials, runs the sampler, and reports back. All market-specific logic (which templates +# changed, which partner they belong to) lives in the calling wrapper — see +# docs/liquid_sampler.md and the CI_AUTH_SAMPLER_PLAN.md design notes (§6, §8) this implements. +# +# Design decisions this encodes (see plan §6 D1-D9, §8): +# - Serialized per partner: the backend allows only one sampler run per partner at a time. +# `concurrency: { group, queue: max }` queues overlapping runs FIFO instead of cancelling — +# cancelling the CI job does NOT cancel the backend run, it just orphans it and blocks the +# partner until it clears. +# - Cross-repo collisions (two market repos sharing a partner id) can't be solved by GitHub +# Actions concurrency groups (they don't span repos), so a 422 "already in progress" from the +# backend is treated as the real mutex: poll-and-retry instead of failing immediately. +# - The partner api_key is a single rotate-on-401 credential stored in its own +# PARTNER_CONFIG_JSON_ secret (never the firm CONFIG_JSON). This workflow is the +# sole writer of that secret, and only writes back if the on-disk token actually changed +# during the run (diffed before/after, not inferred from log output). +# - `results.zip` is uploaded as a short-lived workflow artifact (7 days) for the rare +# rendering-only regression the compact named_results diff can't see; the presigned report +# URL is the long-lived source of truth. +name: run-liquid-sampler +run-name: "Liquid sampler · partner ${{ inputs.partner }}" + +on: + workflow_call: + inputs: + partner: + description: "Partner environment id (must be authorized — see PARTNER_CONFIG_JSON secret)" + required: true + type: string + handles: + description: "Reconciliation text handles to sample, space-separated (directory names under reconciliation_texts/). 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." + required: false + type: string + default: "" + firm_ids: + description: "Firm id(s) to sample against, space-separated. The backend 422s if empty." + required: true + type: string + ref: + description: "Git ref (commit SHA) to check out — the PR head, so sampled template content matches the PR under review." + required: true + type: string + pull_request_number: + description: "PR number to post the result comment on. If empty, no comment is posted (results still upload as an artifact)." + required: false + type: string + default: "" + secrets: + SF_API_CLIENT_ID: + description: "Silverfin API OAuth client id (silverfin-cli needs it at startup)." + required: true + SF_API_SECRET: + description: "Silverfin API OAuth client secret." + required: true + SF_BASIC_AUTH: + description: "HTTP Basic credentials (base64) for the staging gateway. The CLI only sends this when it auto-detects a *.staging.getsilverfin.com host; a no-op elsewhere." + required: false + PARTNER_CONFIG_JSON: + description: "This partner's stored credentials (host + partnerCredentials..token). Caller resolves the per-partner secret, e.g. secrets[format('PARTNER_CONFIG_JSON_{0}', matrix.partner)]." + required: true + REPO_ACCESS_TOKEN: + description: "PAT with repo scope, used to write the rotated partner token back via `gh secret set`." + required: true + +permissions: + contents: read + pull-requests: write + +# Lives inside the reusable workflow itself (not the wrapper) so every adopter gets the +# serialization for free with no way to forget it. `github.repository` here resolves to the +# CALLER's repo (GitHub Actions behavior for reusable workflows), so two different market repos +# each get their own group — see the cross-repo mitigation below for why that's not sufficient +# on its own. +concurrency: + group: sampler-${{ github.repository }}-${{ inputs.partner }} + # 'queue: max' is a GitHub Actions beta sub-key (FIFO, nothing cancelled). If a target repo's + # plan rejects it, fall back to `queue: single` or plain `cancel-in-progress: false` — never + # cancel-in-progress: true, since cancelling this job does not cancel the backend run. + queue: max + +jobs: + run-sampler: + runs-on: ubuntu-latest + # Generous ceiling: a single run is ~30-60 min, plus up to ~90 min of poll-and-retry on a + # cross-repo 422 collision (see the retry loop below). + timeout-minutes: 180 + steps: + - name: Validate inputs + run: | + if [[ -z "${{ inputs.handles }}" && -z "${{ inputs.account_templates }}" ]]; then + echo "::error::Neither handles nor account_templates was supplied — nothing to sample." + exit 1 + fi + if [[ -z "${{ inputs.firm_ids }}" ]]; then + echo "::error::firm_ids is empty. The sampler backend 422s without firm ids." + exit 1 + fi + + - name: Checkout repository at PR head + uses: actions/checkout@v6 + with: + ref: ${{ inputs.ref }} + fetch-depth: 1 + + - name: Setup Node + uses: actions/setup-node@v6 + with: + node-version: 22 + + - 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", not yet merged as of + # this writing). Merge that first, or this step still installs a CLI without --compact. + npm install https://github.com/silverfin/silverfin-cli.git + VERSION=$(node ./node_modules/silverfin-cli/bin/cli.js -V) + echo "CLI version: ${VERSION}" + + - name: Load PARTNER_CONFIG_JSON (host + partnerCredentials) + env: + PARTNER_CONFIG_JSON: ${{ secrets.PARTNER_CONFIG_JSON }} + run: | + echo "::add-mask::${PARTNER_CONFIG_JSON}" + mkdir -p "$HOME/.silverfin" + printf '%s' "${PARTNER_CONFIG_JSON}" > "$HOME/.silverfin/config.json" + if ! jq -e --arg p "${{ inputs.partner }}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json" > /dev/null 2>&1; then + echo "::error::PARTNER_CONFIG_JSON has no partnerCredentials entry for partner ${{ inputs.partner }}. Authorize it first with 'silverfin authorize-partner -i ${{ inputs.partner }} -k ' and store the resulting config.json as this secret." + exit 1 + fi + echo "Host: $(jq -r '.host // \"(default)\"' "$HOME/.silverfin/config.json")" + + - name: Capture partner token before the run + id: before + run: | + TOKEN=$(jq -r --arg p "${{ inputs.partner }}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json") + echo "::add-mask::${TOKEN}" + echo "token=${TOKEN}" >> "$GITHUB_OUTPUT" + + - name: Run liquid sampler (retries on a cross-repo 422 collision) + id: sampler + env: + SF_API_CLIENT_ID: ${{ secrets.SF_API_CLIENT_ID }} + SF_API_SECRET: ${{ secrets.SF_API_SECRET }} + SF_BASIC_AUTH: ${{ secrets.SF_BASIC_AUTH }} + SAMPLER_PARTNER: ${{ inputs.partner }} + SAMPLER_HANDLES: ${{ inputs.handles }} + 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). + HANDLE_ARGS=() + # shellcheck disable=SC2206 + [[ -n "${SAMPLER_HANDLES}" ]] && HANDLE_ARGS=(-h ${SAMPLER_HANDLES}) + ACCOUNT_ARGS=() + # shellcheck disable=SC2206 + [[ -n "${SAMPLER_ACCOUNT_TEMPLATES}" ]] && ACCOUNT_ARGS=(-at ${SAMPLER_ACCOUNT_TEMPLATES}) + + # 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 + # can still collide. The backend's own one-run-per-partner 422 is the real cross-repo + # mutex, so treat it as a "try again shortly" signal, not a hard failure. Bounded to + # match the worst-case run length (~60 min) plus margin. + 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" + 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') + RC=$? + set -e + echo "----- run-sampler output (exit ${RC}) -----" + echo "$OUTPUT" + echo "--------------------------------------------" + + if echo "$OUTPUT" | grep -qi "already in progress"; then + NOW=$(date +%s) + if (( NOW >= DEADLINE )); then + # Don't exit here: fall through to the normal output-setting logic below (which + # will correctly record sampler_ok=false / no report_url) so downstream steps + # (write-back, PR comment) still run via their own `if:` conditions instead of + # being skipped by a hard step failure. The final "Fail job" step still fails + # the job overall. + echo "::error::Partner ${SAMPLER_PARTNER} still has a sampler run in progress after 90 minutes of retrying. Giving up — check whether a previous run is stuck (§2.3/2.4c: no CLI cancel exists for a backend run)." + break + fi + echo "[$(date -u +%H:%M:%S)] partner ${SAMPLER_PARTNER} has a run in progress elsewhere (cross-repo or stale) — retrying in 60s..." + sleep 60 + ATTEMPT=$((ATTEMPT+1)) + continue + fi + break + done + + { + echo "rc=${RC}" + } >> "$GITHUB_OUTPUT" + + if [[ "${RC}" -ne 0 ]]; then + echo "sampler_ok=false" >> "$GITHUB_OUTPUT" + elif echo "$OUTPUT" | grep -qi "completed successfully"; then + echo "sampler_ok=true" >> "$GITHUB_OUTPUT" + else + echo "sampler_ok=false" >> "$GITHUB_OUTPUT" + fi + + # Report URL: "Sampler report: https://..." (strip ANSI first). + REPORT_URL=$(echo "$OUTPUT" | sed 's/\x1b\[[0-9;]*m//g' | grep -oE 'Sampler report: https?://[^[:space:]]+' | head -n1 | sed 's/^Sampler report: //') + echo "report_url=${REPORT_URL}" >> "$GITHUB_OUTPUT" + + # Compact named_results diff, between the CLI's own markers. + COMPACT=$(echo "$OUTPUT" | sed 's/\x1b\[[0-9;]*m//g' | awk '/SAMPLER_COMPACT_START/{f=1;next} /SAMPLER_COMPACT_END/{f=0} f') + { + echo "compact<> "$GITHUB_OUTPUT" + + - name: Capture partner token after the run and write back if rotated + id: after + if: always() + env: + GH_TOKEN: ${{ secrets.REPO_ACCESS_TOKEN }} + run: | + if [[ ! -f "$HOME/.silverfin/config.json" ]]; then + echo "No config.json on disk (load step must have failed) — nothing to write back." + exit 0 + fi + AFTER_TOKEN=$(jq -r --arg p "${{ inputs.partner }}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json") + echo "::add-mask::${AFTER_TOKEN}" + if [[ "${AFTER_TOKEN}" == "${{ steps.before.outputs.token }}" ]]; then + echo "Partner token unchanged — no write-back needed (sole-writer secret stays untouched)." + exit 0 + fi + echo "Partner token rotated mid-run (401 refresh) — writing PARTNER_CONFIG_JSON_${{ inputs.partner }} back." + CONFIG_JSON=$(cat "$HOME/.silverfin/config.json" | tr -d '\n') + echo "::add-mask::${CONFIG_JSON}" + echo "$CONFIG_JSON" | gh secret set "PARTNER_CONFIG_JSON_${{ inputs.partner }}" + + - name: Download results.zip for the workflow artifact + id: download + if: 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). + if curl -sL --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 "downloaded=false" >> "$GITHUB_OUTPUT" + fi + + - name: Upload results.zip artifact + if: steps.download.outputs.downloaded == 'true' + uses: actions/upload-artifact@v4 + with: + name: sampler-results-partner-${{ inputs.partner }} + path: results.zip + retention-days: 7 + + - name: Post result comment on the PR + if: inputs.pull_request_number != '' + uses: actions/github-script@v7 + 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 }} + COMPACT: ${{ steps.sampler.outputs.compact }} + PR_NUMBER: ${{ inputs.pull_request_number }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + with: + script: | + const marker = ``; + const ok = process.env.SAMPLER_OK === "true"; + const reportUrl = process.env.REPORT_URL || ""; + const compact = (process.env.COMPACT || "").trim(); + const runUrl = process.env.RUN_URL; + + const lines = [`## Liquid sampler · partner \`${process.env.PARTNER}\``, ""]; + if (ok) { + lines.push("This is a **review artifact, not a pass/fail gate** — check the diff below (or the full report) for unintended changes."); + } else { + 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}\``); + lines.push(`- Firm(s): \`${process.env.FIRM_IDS}\``); + if (reportUrl) lines.push(`- **[📊 Open full sampler report](${reportUrl})** (downloads \`results.zip\`)`); + lines.push(`- Workflow run: ${runUrl}`); + lines.push(""); + if (compact) { + lines.push(compact); + } else if (ok) { + lines.push("_No compact diff was produced (see the run log)._"); + } + lines.push("", marker); + const body = lines.join("\n"); + + const { owner, repo } = context.repo; + const prNumber = parseInt(process.env.PR_NUMBER, 10); + const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: prNumber, per_page: 100 }); + const existing = comments.find(c => c.user && c.user.type === "Bot" && c.body && c.body.includes(marker)); + if (existing) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number: prNumber, body }); + } + + - name: Fail job if the sampler run did not complete successfully + if: steps.sampler.outputs.sampler_ok != 'true' + run: | + echo "::error::Sampler run for partner ${{ inputs.partner }} did not complete successfully — see the run-sampler step output above." + exit 1 From 47fd951feb685d3a288935c436a2dc732acd0f69 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 15 Jul 2026 16:18:54 +0200 Subject: [PATCH 2/7] fix: prevent the report-URL extraction from aborting the sampler step grep -oE finding no match (any run that doesn't complete - a timed-out retry, an unrelated hard failure) made the whole pipeline's exit status non-zero under set -eo pipefail, even though head/sed after it succeed on empty input. That aborted the step immediately, before report_url and the compact-diff output were ever set - silently dropping the PR comment on every non-completed run, since the comment step has no `if: always()`. Caught by extracting this step's script and running it locally against a mocked CLI across three scenarios (always-in-progress timeout, retry-then- succeed, immediate hard failure) - all three now produce correct outputs without aborting. --- .github/workflows/run_sampler.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 9f431fd..30b9f4a 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -217,7 +217,13 @@ jobs: fi # Report URL: "Sampler report: https://..." (strip ANSI first). - REPORT_URL=$(echo "$OUTPUT" | sed 's/\x1b\[[0-9;]*m//g' | grep -oE 'Sampler report: https?://[^[:space:]]+' | head -n1 | sed 's/^Sampler report: //') + # `|| true`: under `set -eo pipefail`, grep finding no match (a failed/timed-out run + # never prints this line) makes the whole pipeline's exit status non-zero even though + # head/sed after it succeed on empty input - that would abort this step here, before + # the compact-diff extraction below and before the "always run" downstream steps ever + # see a value, silently dropping the PR comment on failure. Confirmed by reproducing + # locally: without `|| true` this line aborted the step on every non-completed run. + REPORT_URL=$(echo "$OUTPUT" | sed 's/\x1b\[[0-9;]*m//g' | grep -oE 'Sampler report: https?://[^[:space:]]+' | head -n1 | sed 's/^Sampler report: //') || true echo "report_url=${REPORT_URL}" >> "$GITHUB_OUTPUT" # Compact named_results diff, between the CLI's own markers. From 06aaaf717b77051f3733dc39328b451f72dcc4f2 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 15 Jul 2026 18:16:54 +0200 Subject: [PATCH 3/7] fix: hoist SF_API_CLIENT_ID/SF_API_SECRET to job-level env The CLI checks for these at process startup for every command, including a bare -V version probe - the "Install silverfin-cli" step only had them on the later "Run liquid sampler" step, so it failed with "Missing API credentials" before ever reaching the actual sampler call. Caught live via the throwaway lu_market#763 harness (pull_request_review firing this branch's copy of the workflow pre-merge). --- .github/workflows/run_sampler.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 30b9f4a..b5a7f1f 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -94,6 +94,12 @@ jobs: # Generous ceiling: a single run is ~30-60 min, plus up to ~90 min of poll-and-retry on a # cross-repo 422 collision (see the retry loop below). timeout-minutes: 180 + # Job-level (not just on the sampler step): the CLI checks for these at process startup for + # every command, including a bare `-V` version probe - confirmed live (the "Install + # silverfin-cli" step failed with "Missing API credentials" before these were job-scoped). + env: + SF_API_CLIENT_ID: ${{ secrets.SF_API_CLIENT_ID }} + SF_API_SECRET: ${{ secrets.SF_API_SECRET }} steps: - name: Validate inputs run: | @@ -150,8 +156,6 @@ jobs: - name: Run liquid sampler (retries on a cross-repo 422 collision) id: sampler env: - SF_API_CLIENT_ID: ${{ secrets.SF_API_CLIENT_ID }} - SF_API_SECRET: ${{ secrets.SF_API_SECRET }} SF_BASIC_AUTH: ${{ secrets.SF_BASIC_AUTH }} SAMPLER_PARTNER: ${{ inputs.partner }} SAMPLER_HANDLES: ${{ inputs.handles }} From 7dfa1a64d975c32f497315b7a18c1f269e27fe55 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 15 Jul 2026 18:20:23 +0200 Subject: [PATCH 4/7] fix: gate write-back on the before-token capture actually succeeding if: always() meant a run that failed at "Load PARTNER_CONFIG_JSON" (e.g. the secret is missing a partnerCredentials entry) still ran this step, compared an empty before-token (steps.before was skipped) against whatever AFTER_TOKEN happened to parse to, read that as "rotated", and wrote the same invalid content back to the secret. Caught live. No data loss occurred (it wrote back the same content it read), but the logic was wrong regardless. Now requires steps.before.outcome == 'success'. --- .github/workflows/run_sampler.yml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index b5a7f1f..f5547df 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -240,7 +240,14 @@ jobs: - name: Capture partner token after the run and write back if rotated id: after - if: always() + # Gated on steps.before succeeding, not just always(): if "Load"/"Capture...before" never + # got a valid token for this partner (e.g. the secret is missing a partnerCredentials + # entry), steps.before.outputs.token is empty - comparing that against whatever + # AFTER_TOKEN happens to parse to would read as "rotated" and write back a config that + # was never actually valid to begin with. Caught live: a run that failed at "Load" + # (invalid secret content) still had this step fire under bare always() and wrote the + # same bad content back to the secret - a no-op here, but the wrong behavior in general. + if: always() && steps.before.outcome == 'success' env: GH_TOKEN: ${{ secrets.REPO_ACCESS_TOKEN }} run: | From ed16c9184e83b235b484935f1497e8a1c8530913 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 15 Jul 2026 19:56:35 +0200 Subject: [PATCH 5/7] Update stale comment now that silverfin-cli#261 is merged to main --- .github/workflows/run_sampler.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index f5547df..75e93e9 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -127,8 +127,7 @@ jobs: 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", not yet merged as of - # this writing). Merge that first, or this step still installs a CLI without --compact. + # main (silverfin-cli PR #261, "agustin-bso-sampler-easy-results", merged 2026-07-15). npm install https://github.com/silverfin/silverfin-cli.git VERSION=$(node ./node_modules/silverfin-cli/bin/cli.js -V) echo "CLI version: ${VERSION}" From 07b40a13cf8fea3bebda35a5b0645282628c88c5 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Thu, 16 Jul 2026 09:14:41 +0200 Subject: [PATCH 6/7] fix: address PR review findings on run_sampler.yml - Splice handles/account_templates/firm_ids/partner through env: instead of directly into run: scripts (GitHub Actions expression-injection anti-pattern), including the highest-impact instance in the write-back step that runs with a repo-write PAT active. - Reject whitespace-only handles/account_templates/firm_ids, not just empty string. - Add persist-credentials: false to the checkout step (unused here). - Add --fail to the results.zip curl download so a non-2xx response isn't treated as a successful download. - Add always() to the post-comment, upload-artifact, and fail-job steps so an earlier hard failure doesn't skip them silently. - Fix a jq filter broken by invalid backslash-escaping inside a single-quoted string (silently blanked the Host: diagnostic). - Document run_sampler.yml in README.md's Individual Action Documentation section, matching the push_to_review_firm.yml precedent. --- .github/workflows/run_sampler.yml | 39 ++++++++++++++++--------- README.md | 47 ++++++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 75e93e9..d9886e6 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -102,12 +102,16 @@ jobs: SF_API_SECRET: ${{ secrets.SF_API_SECRET }} steps: - name: Validate inputs + env: + HANDLES: ${{ inputs.handles }} + ACCOUNT_TEMPLATES: ${{ inputs.account_templates }} + FIRM_IDS: ${{ inputs.firm_ids }} run: | - if [[ -z "${{ inputs.handles }}" && -z "${{ inputs.account_templates }}" ]]; then + if [[ -z "${HANDLES// /}" && -z "${ACCOUNT_TEMPLATES// /}" ]]; then echo "::error::Neither handles nor account_templates was supplied — nothing to sample." exit 1 fi - if [[ -z "${{ inputs.firm_ids }}" ]]; then + if [[ -z "${FIRM_IDS// /}" ]]; then echo "::error::firm_ids is empty. The sampler backend 422s without firm ids." exit 1 fi @@ -117,6 +121,7 @@ jobs: with: ref: ${{ inputs.ref }} fetch-depth: 1 + persist-credentials: false - name: Setup Node uses: actions/setup-node@v6 @@ -135,20 +140,23 @@ jobs: - name: Load PARTNER_CONFIG_JSON (host + partnerCredentials) env: PARTNER_CONFIG_JSON: ${{ secrets.PARTNER_CONFIG_JSON }} + PARTNER: ${{ inputs.partner }} run: | echo "::add-mask::${PARTNER_CONFIG_JSON}" mkdir -p "$HOME/.silverfin" printf '%s' "${PARTNER_CONFIG_JSON}" > "$HOME/.silverfin/config.json" - if ! jq -e --arg p "${{ inputs.partner }}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json" > /dev/null 2>&1; then - echo "::error::PARTNER_CONFIG_JSON has no partnerCredentials entry for partner ${{ inputs.partner }}. Authorize it first with 'silverfin authorize-partner -i ${{ inputs.partner }} -k ' and store the resulting config.json as this secret." + if ! jq -e --arg p "${PARTNER}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json" > /dev/null 2>&1; then + echo "::error::PARTNER_CONFIG_JSON has no partnerCredentials entry for partner ${PARTNER}. Authorize it first with 'silverfin authorize-partner -i ${PARTNER} -k ' and store the resulting config.json as this secret." exit 1 fi - echo "Host: $(jq -r '.host // \"(default)\"' "$HOME/.silverfin/config.json")" + echo "Host: $(jq -r '.host // "(default)"' "$HOME/.silverfin/config.json")" - name: Capture partner token before the run id: before + env: + PARTNER: ${{ inputs.partner }} run: | - TOKEN=$(jq -r --arg p "${{ inputs.partner }}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json") + TOKEN=$(jq -r --arg p "${PARTNER}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json") echo "::add-mask::${TOKEN}" echo "token=${TOKEN}" >> "$GITHUB_OUTPUT" @@ -249,21 +257,22 @@ jobs: if: always() && steps.before.outcome == 'success' env: GH_TOKEN: ${{ secrets.REPO_ACCESS_TOKEN }} + PARTNER: ${{ inputs.partner }} run: | if [[ ! -f "$HOME/.silverfin/config.json" ]]; then echo "No config.json on disk (load step must have failed) — nothing to write back." exit 0 fi - AFTER_TOKEN=$(jq -r --arg p "${{ inputs.partner }}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json") + AFTER_TOKEN=$(jq -r --arg p "${PARTNER}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json") echo "::add-mask::${AFTER_TOKEN}" if [[ "${AFTER_TOKEN}" == "${{ steps.before.outputs.token }}" ]]; then echo "Partner token unchanged — no write-back needed (sole-writer secret stays untouched)." exit 0 fi - echo "Partner token rotated mid-run (401 refresh) — writing PARTNER_CONFIG_JSON_${{ inputs.partner }} back." + echo "Partner token rotated mid-run (401 refresh) — writing PARTNER_CONFIG_JSON_${PARTNER} back." CONFIG_JSON=$(cat "$HOME/.silverfin/config.json" | tr -d '\n') echo "::add-mask::${CONFIG_JSON}" - echo "$CONFIG_JSON" | gh secret set "PARTNER_CONFIG_JSON_${{ inputs.partner }}" + echo "$CONFIG_JSON" | gh secret set "PARTNER_CONFIG_JSON_${PARTNER}" - name: Download results.zip for the workflow artifact id: download @@ -274,7 +283,7 @@ jobs: # 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). - if curl -sL --max-time 300 -o results.zip "${REPORT_URL}"; then + 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)." @@ -282,7 +291,7 @@ jobs: fi - name: Upload results.zip artifact - if: steps.download.outputs.downloaded == 'true' + if: always() && steps.download.outputs.downloaded == 'true' uses: actions/upload-artifact@v4 with: name: sampler-results-partner-${{ inputs.partner }} @@ -290,7 +299,7 @@ jobs: retention-days: 7 - name: Post result comment on the PR - if: inputs.pull_request_number != '' + if: always() && inputs.pull_request_number != '' uses: actions/github-script@v7 env: PARTNER: ${{ inputs.partner }} @@ -342,7 +351,9 @@ jobs: } - name: Fail job if the sampler run did not complete successfully - if: steps.sampler.outputs.sampler_ok != 'true' + if: always() && steps.sampler.outputs.sampler_ok != 'true' + env: + PARTNER: ${{ inputs.partner }} run: | - echo "::error::Sampler run for partner ${{ inputs.partner }} did not complete successfully — see the run-sampler step output above." + echo "::error::Sampler run for partner ${PARTNER} did not complete successfully — see the run-sampler step output above." exit 1 diff --git a/README.md b/README.md index a4434d6..9335904 100644 --- a/README.md +++ b/README.md @@ -48,6 +48,8 @@ The document will go over all the Github Actions that currently automate a coupl * [Automated slack update (slack_changelog.yml)](https://silverfin.quip.com/avPDA9TrpJ9Y#temp:C:EBf862cf4257e68475d8b47891c6) * [Review firm deployment](#review-firm-deployment) * [Push templates to review firm (push_to_review_firm.yml)](#push-templates-to-review-firm-push_to_review_firmyml) + * [Liquid sampler](#liquid-sampler) + * [Run liquid sampler (run_sampler.yml)](#run-liquid-sampler-run_sampleryml) ## Overview @@ -239,4 +241,47 @@ _Prerequisites:_ * `SF_API_CLIENT_ID`, `SF_API_SECRET` and `CONFIG_JSON` available to the caller (e.g. `secrets: inherit`). * The review firm must already be authorized with the Silverfin CLI (its OAuth tokens present in `CONFIG_JSON`). If it isn't, the run fails with a message asking the developer who implemented the linked development ticket(s) to authorize it. -* `FIRM_ID_REVIEW` repository variable in the market repo (used as the default when no `firm_id` is supplied). \ No newline at end of file +* `FIRM_ID_REVIEW` repository variable in the market repo (used as the default when no `firm_id` is supplied). + + + +### Liquid sampler + +#### Run liquid sampler `(run_sampler.yml)` + +_Description_: +Reusable workflow. Runs the Liquid Sampler (`silverfin-cli run-sampler`) for a single partner's changed reconciliation/account templates and posts the result on a PR. It is deliberately partner-agnostic and repo-layout-agnostic: given a partner id, a list of already-classified handles/account templates, and firm ids, it loads that partner's credentials, runs the sampler, and reports back. All market-specific logic (which templates changed, which partner they belong to) lives in the calling wrapper. + +_Trigger:_ + +* Called via `workflow_call` from a market-repo wrapper. + +_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. +* `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). + +_Steps:_ + +* Validates that at least one of `handles`/`account_templates`, and `firm_ids`, were supplied. +* Checks out the repo at `ref` and installs `silverfin-cli`. +* 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 a link to the full report. +* Fails the job if the sampler run did not complete successfully. + +_Authentication note:_ + +* This workflow is the sole writer of `PARTNER_CONFIG_JSON_` — it only writes back if the on-disk token actually changed during the run (diffed before/after, not inferred from log output). + +_Prerequisites:_ + +* `SF_API_CLIENT_ID`, `SF_API_SECRET`, `PARTNER_CONFIG_JSON` (per-partner secret resolved by the caller) and `REPO_ACCESS_TOKEN` (repo-scoped write PAT, for the token write-back) available to the caller. +* `SF_BASIC_AUTH` if the partner's host is a `*.staging.getsilverfin.com` gateway. +* The partner must already be authorized with the Silverfin CLI (`silverfin authorize-partner`) and its `config.json` stored as the `PARTNER_CONFIG_JSON_` secret. \ No newline at end of file From d7f75799b3dd123fd3e1e06d37de85297c92abbf Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Thu, 16 Jul 2026 09:25:29 +0200 Subject: [PATCH 7/7] fix: address second round of PR review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reject tabs/newlines, not just literal spaces, as whitespace-only handles/account_templates/firm_ids. - Require the partner token to be a non-empty string during the PARTNER_CONFIG_JSON validation (previously accepted empty string, numbers, objects, null as "present"). - Stop splicing steps.before.outputs.token into shell source in the write-back step's comparison — route it through env: like the other values in this file. - README: qualify that the full-report link is only included when a report URL was produced; results.zip artifact is the fallback. --- .github/workflows/run_sampler.yml | 9 +++++---- README.md | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index d9886e6..baf16f9 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -107,11 +107,11 @@ jobs: ACCOUNT_TEMPLATES: ${{ inputs.account_templates }} FIRM_IDS: ${{ inputs.firm_ids }} run: | - if [[ -z "${HANDLES// /}" && -z "${ACCOUNT_TEMPLATES// /}" ]]; then + if [[ -z "${HANDLES//[[:space:]]/}" && -z "${ACCOUNT_TEMPLATES//[[:space:]]/}" ]]; then echo "::error::Neither handles nor account_templates was supplied — nothing to sample." exit 1 fi - if [[ -z "${FIRM_IDS// /}" ]]; then + if [[ -z "${FIRM_IDS//[[:space:]]/}" ]]; then echo "::error::firm_ids is empty. The sampler backend 422s without firm ids." exit 1 fi @@ -145,7 +145,7 @@ jobs: echo "::add-mask::${PARTNER_CONFIG_JSON}" mkdir -p "$HOME/.silverfin" printf '%s' "${PARTNER_CONFIG_JSON}" > "$HOME/.silverfin/config.json" - if ! jq -e --arg p "${PARTNER}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json" > /dev/null 2>&1; then + if ! jq -e --arg p "${PARTNER}" '(.partnerCredentials[$p].token | type == "string" and length > 0)' "$HOME/.silverfin/config.json" > /dev/null 2>&1; then echo "::error::PARTNER_CONFIG_JSON has no partnerCredentials entry for partner ${PARTNER}. Authorize it first with 'silverfin authorize-partner -i ${PARTNER} -k ' and store the resulting config.json as this secret." exit 1 fi @@ -258,6 +258,7 @@ jobs: env: GH_TOKEN: ${{ secrets.REPO_ACCESS_TOKEN }} PARTNER: ${{ inputs.partner }} + BEFORE_TOKEN: ${{ steps.before.outputs.token }} run: | if [[ ! -f "$HOME/.silverfin/config.json" ]]; then echo "No config.json on disk (load step must have failed) — nothing to write back." @@ -265,7 +266,7 @@ jobs: fi AFTER_TOKEN=$(jq -r --arg p "${PARTNER}" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json") echo "::add-mask::${AFTER_TOKEN}" - if [[ "${AFTER_TOKEN}" == "${{ steps.before.outputs.token }}" ]]; then + if [[ "${AFTER_TOKEN}" == "${BEFORE_TOKEN}" ]]; then echo "Partner token unchanged — no write-back needed (sole-writer secret stays untouched)." exit 0 fi diff --git a/README.md b/README.md index 9335904..b272c0d 100644 --- a/README.md +++ b/README.md @@ -273,7 +273,7 @@ _Steps:_ * 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 a link to the full report. +* 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. * Fails the job if the sampler run did not complete successfully. _Authentication note:_