From 615664da28fe0da1e37d249d6f5e15bee5eb9b8c Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Tue, 28 Jul 2026 14:21:14 +0200 Subject: [PATCH 1/6] Fix ephemeral report link in PR comment; add diffs-only zip folder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The presigned report_url expires ~5 minutes after the run completes (see silverfin-cli's CI_AUTH_SAMPLER_PLAN.md §10.4), so the "Open full sampler report" link posted on the PR is essentially always dead by the time a reviewer clicks it. Link to the GitHub Actions artifact instead (valid for the full 7-day retention window), falling back to the presigned URL only if the artifact upload itself didn't happen. Also adds a diffs/ folder to the uploaded results.zip containing before/after view.html only for the entries the compact diff flagged, via silverfin-cli's new run-sampler --from-zip --add-diffs-folder (silverfin-cli#265) - lets a reviewer open just the changed entries instead of the full archive. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/run_sampler.yml | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 94faabf..73bf090 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -288,14 +288,27 @@ 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). + # 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 + # 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: node ./node_modules/silverfin-cli/bin/cli.js run-sampler --from-zip results.zip --add-diffs-folder + - name: Upload results.zip artifact + id: upload if: always() && steps.download.outputs.downloaded == 'true' uses: actions/upload-artifact@v4 with: @@ -313,6 +326,7 @@ 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 }} 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 +335,12 @@ 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 compact = (process.env.COMPACT || "").trim(); const runUrl = process.env.RUN_URL; @@ -334,7 +354,11 @@ jobs: 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\`)`); + if (artifactUrl) { + lines.push(`- **[📊 Open full sampler report](${artifactUrl})** (downloads \`results.zip\`, kept 7 days)`); + } 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)`); + } lines.push(`- Workflow run: ${runUrl}`); lines.push(""); if (compact) { From e5916ae30670f4ef4cfd9efc5a11ab77dc3c0713 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 29 Jul 2026 10:59:39 +0200 Subject: [PATCH 2/6] Surface add-diffs-folder failures, pin unreleased CLI dep, sync README - Add diffs/ folder step now sets an output flag and echoes ::warning:: on failure instead of silently swallowing it via continue-on-error, matching the sibling download step's pattern. - Pin the silverfin-cli install to a commit that actually has --add-diffs-folder (it isn't on main yet); previously this workflow called a flag that main doesn't have. - Add a final fallback line when neither artifactUrl nor reportUrl is set, and note the artifact link requires GitHub sign-in. - Sync README's run_sampler.yml section, which described the old report-URL-primary priority and didn't mention the diffs/ step. Addresses review feedback from PR #35. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/run_sampler.yml | 22 ++++++++++++++++------ README.md | 4 ++-- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 73bf090..0992d59 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -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}" @@ -300,12 +301,19 @@ jobs: 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: node ./node_modules/silverfin-cli/bin/cli.js run-sampler --from-zip results.zip --add-diffs-folder + 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 @@ -355,9 +363,11 @@ jobs: if (process.env.ACCOUNT_TEMPLATES) lines.push(`- Account templates: \`${process.env.ACCOUNT_TEMPLATES}\``); lines.push(`- Firm(s): \`${process.env.FIRM_IDS}\``); if (artifactUrl) { - lines.push(`- **[📊 Open full sampler report](${artifactUrl})** (downloads \`results.zip\`, kept 7 days)`); + lines.push(`- **[📊 Open full sampler report](${artifactUrl})** (GitHub sign-in required; downloads \`results.zip\`, kept 7 days)`); } 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(""); diff --git a/README.md b/README.md index 53c8211..5632bc8 100644 --- a/README.md +++ b/README.md @@ -273,8 +273,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) 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:_ From d0e3b1d4ceb33b48f3e030e5e3be1a2d17ef2528 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 29 Jul 2026 12:02:20 +0200 Subject: [PATCH 3/6] Gate results.zip download on always(), not implicit success() If the token write-back step above it fails, the default success() guard would skip this download even when report_url is set. Addresses CodeRabbit feedback on PR #35. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/run_sampler.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 0992d59..ecfc394 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -282,7 +282,7 @@ 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: | From cbba189c1ea46530fa27f8d2c9a3328bd63c55f0 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 29 Jul 2026 12:07:36 +0200 Subject: [PATCH 4/6] Surface add_diffs failure in the PR comment, not just workflow logs diffs_added was set but never consumed downstream, so a reviewer had no way to know the diffs/ folder was missing from the artifact short of digging through Actions logs. Addresses re-review feedback on PR #35. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/run_sampler.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index ecfc394..776e3c7 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -335,6 +335,7 @@ jobs: 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 }} @@ -349,6 +350,7 @@ jobs: // 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; @@ -364,6 +366,9 @@ jobs: lines.push(`- Firm(s): \`${process.env.FIRM_IDS}\``); 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 { From 5a7dc51f1c71c0ed6cd21473a23bcd1c5032c43c Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 29 Jul 2026 12:08:28 +0200 Subject: [PATCH 5/6] Note GitHub sign-in requirement for the artifact link in README Co-Authored-By: Claude Sonnet 5 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5632bc8..8f292fd 100644 --- a/README.md +++ b/README.md @@ -274,7 +274,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`, 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) 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. +* 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:_ From 22db1c6932731a2d91d2ae7e4bc91e1df30848ec Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 29 Jul 2026 13:27:02 +0200 Subject: [PATCH 6/6] Upgrade github-script to v8 for Node 24 runner compatibility actions/github-script@v7 targets Node 20, which GitHub Actions now forces onto Node 24 with a deprecation warning. Co-authored-by: Cursor --- .github/workflows/run_sampler.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 776e3c7..a7e29dc 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -326,7 +326,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 }}