Add reusable run_sampler.yml workflow#30
Conversation
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.
WalkthroughChangesLiquid sampler workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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.
|
Ran a local test pass on the two riskiest hand-written bash sections (couldn't do a live GitHub Actions dispatch pre-merge — Classify logic (lu_market wrapper): extracted the script, ran it against synthetic fixtures covering every branch — single in-scope partner, zero in-scope partner, multiple in-scope partners, missing This reusable workflow's retry/output-parsing logic: extracted the "Run liquid sampler" step, mocked the CLI, and ran 3 scenarios (persistent "already in progress" → graceful timeout, retry-once-then-succeed, immediate unrelated hard failure → no retry). Found and fixed a real bug: the Not yet tested: an actual live sampler run against beta (blocked on |
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).
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'.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/run_sampler.yml (1)
126-134: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider pinning the
silverfin-cliinstall to a commit SHA/tag.
npm install https://github.com/silverfin/silverfin-cli.gitpulls whatever is onmainat run time, and the comment notes this workflow already has a real, time-sensitive dependency on a specific just-merged PR. Any later unrelated change onmain(or a compromised push) would silently change what this workflow executes, with no lockfile/integrity check backing it (zizmor'sadhoc-packagesfinding). This matches an existing convention elsewhere in the repo (seeadd_shared_parts_production.yml), so treat as a deferrable, repo-wide follow-up rather than blocking this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/run_sampler.yml around lines 126 - 134, Pin the silverfin-cli dependency in the “Install silverfin-cli” workflow step to a specific immutable commit SHA or release tag, following the established convention in add_shared_parts_production.yml, instead of installing the moving GitHub main branch. Keep the existing version-reporting command unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/run_sampler.yml:
- Around line 115-120: Update the Checkout repository at PR head step to set
persist-credentials to false, leaving the existing ref and fetch-depth settings
unchanged.
- Around line 268-283: Update the curl invocation in the “Download results.zip
for the workflow artifact” step to include the --fail option, ensuring HTTP
4xx/5xx responses enter the existing warning path instead of being marked as
downloaded.
---
Nitpick comments:
In @.github/workflows/run_sampler.yml:
- Around line 126-134: Pin the silverfin-cli dependency in the “Install
silverfin-cli” workflow step to a specific immutable commit SHA or release tag,
following the established convention in add_shared_parts_production.yml, instead
of installing the moving GitHub main branch. Keep the existing version-reporting
command unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc980b28-c491-4f78-9aa9-a198b2bef132
📒 Files selected for processing (1)
.github/workflows/run_sampler.yml
| - name: Checkout repository at PR head | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| ref: ${{ inputs.ref }} | ||
| fetch-depth: 1 | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on checkout.
This job never git pushes — the write-back path uses gh secret set with REPO_ACCESS_TOKEN (Lines 251-266), not the checkout token. Persisting the ephemeral GITHUB_TOKEN-derived credentials in the local git config on disk (zizmor's artipacked finding) is unnecessary here and needlessly widens the credential's exposure window on the runner.
Suggested fix
- name: Checkout repository at PR head
uses: actions/checkout@v6
with:
ref: ${{ inputs.ref }}
fetch-depth: 1
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Checkout repository at PR head | |
| uses: actions/checkout@v6 | |
| with: | |
| ref: ${{ inputs.ref }} | |
| fetch-depth: 1 | |
| - name: Checkout repository at PR head | |
| uses: actions/checkout@v6 | |
| with: | |
| ref: ${{ inputs.ref }} | |
| fetch-depth: 1 | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 115-119: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/run_sampler.yml around lines 115 - 120, Update the
Checkout repository at PR head step to set persist-credentials to false, leaving
the existing ref and fetch-depth settings unchanged.
Source: Linters/SAST tools
| - 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 | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add --fail to the curl download so HTTP error responses aren't treated as a successful artifact.
Without --fail, curl -sL returns exit 0 and writes the response body to results.zip even on a 4xx/5xx response (e.g. an expired or malformed report URL), so the artifact upload step would then publish an HTML error page mislabeled as results.zip.
Suggested fix
- 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📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - 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: 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 --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 "downloaded=false" >> "$GITHUB_OUTPUT" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/run_sampler.yml around lines 268 - 283, Update the curl
invocation in the “Download results.zip for the workflow artifact” step to
include the --fail option, ensuring HTTP 4xx/5xx responses enter the existing
warning path instead of being marked as downloaded.
| steps: | ||
| - name: Validate inputs | ||
| run: | | ||
| if [[ -z "${{ inputs.handles }}" && -z "${{ inputs.account_templates }}" ]]; then |
There was a problem hiding this comment.
🟠 Major — two issues in this validation step
-
Expression injection:
${{ inputs.handles }}/${{ inputs.account_templates }}(this line) and${{ inputs.firm_ids }}(line 110) are spliced directly into therun:script instead of being passed throughenv:. This is the classic GitHub Actions script-injection anti-pattern — the${{ }}substitution happens as raw text before bash parses the script, so a value containing shell metacharacters becomes executable code withSF_API_CLIENT_ID/SF_API_SECRETalready in scope via the job-levelenv:. It's inconsistent with this same file's own correct pattern two steps later (SAMPLER_PARTNER/SAMPLER_HANDLESpassed viaenv:) and with this repo's existing convention (check_dependencies.ymlpassespull_request_numberviaenv:, never inline). The same pattern recurs at lines 142-143 (Load PARTNER_CONFIG_JSON), 151 (Capture ... before), 257/259/263 (Capture ... after / write-back), and 347 (Fail job) — see the review comment on line 266 for the highest-severity instance (it runs with a repo-write PAT active). -
Whitespace-only values slip through: this only checks
-z(empty string). A caller passing a single space forhandles/account_templates/firm_idspasses this validation, then word-splits to zero tokens later (HANDLE_ARGS=(-h ${SAMPLER_HANDLES})etc.), leaving a dangling-h/-at/--firm-idsflag that silently consumes the next CLI token as its value instead of failing loudly here.
Fix for both:
env:
HANDLES: ${{ inputs.handles }}
ACCOUNT_TEMPLATES: ${{ inputs.account_templates }}
FIRM_IDS: ${{ inputs.firm_ids }}
run: |
if [[ -z "${HANDLES// /}" && -z "${ACCOUNT_TEMPLATES// /}" ]]; then
echo "::error::Neither handles nor account_templates was supplied — nothing to sample."
exit 1
fi
if [[ -z "${FIRM_IDS// /}" ]]; then
echo "::error::firm_ids is empty. The sampler backend 422s without firm ids."
exit 1
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 }}" |
There was a problem hiding this comment.
🟠 Major — same expression-injection pattern, highest-impact instance
This step runs with REPO_ACCESS_TOKEN (a repo-scoped write PAT) active via GH_TOKEN. ${{ inputs.partner }} is spliced directly into jq --arg p "${{ inputs.partner }}" (line 257), an echo message (line 263), and here into gh secret set "PARTNER_CONFIG_JSON_${{ inputs.partner }}" — which also means the unsanitized partner value directly determines which secret gets overwritten, with no allowlist check against known/authorized partner ids. Combined with the active write PAT, an injectable partner value here is the worst-case instance of the pattern flagged on line 106 (same root cause also present at lines 142-143 and 151).
Same fix throughout: thread partner through env: (e.g. PARTNER: ${{ inputs.partner }}) and reference "$PARTNER" in each of these steps, matching the SAMPLER_PARTNER pattern this file already uses correctly in the "Run liquid sampler" step.
env:
GH_TOKEN: ${{ secrets.REPO_ACCESS_TOKEN }}
PARTNER: ${{ inputs.partner }}
run: |
...
AFTER_TOKEN=$(jq -r --arg p "$PARTNER" '.partnerCredentials[$p].token' "$HOME/.silverfin/config.json")
...
echo "$CONFIG_JSON" | gh secret set "PARTNER_CONFIG_JSON_${PARTNER}"| # 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 |
There was a problem hiding this comment.
🟠 Major: curl -sL --max-time 300 -o results.zip "${REPORT_URL}" has no -f/--fail. A non-2xx response after following redirects (an expired presigned URL, a permission error) still returns curl exit code 0 with the error body written to results.zip — so this branch reports downloaded=true and the next step uploads a bogus non-zip file as the sampler-results-partner-* artifact, instead of correctly falling back to downloaded=false.
if curl -sfL --max-time 300 -o results.zip "${REPORT_URL}"; then| retention-days: 7 | ||
|
|
||
| - name: Post result comment on the PR | ||
| if: inputs.pull_request_number != '' |
There was a problem hiding this comment.
🟡 Minor — downstream steps skip silently on an early hard failure
This step's if: (and "Upload results.zip artifact" at line 285, and "Fail job if the sampler run did not complete successfully" at line 345) don't include always(). If an earlier step hard-fails — checkout, npm install, the "Load PARTNER_CONFIG_JSON" validation, or the write-back step (lines 249-266) — GitHub Actions' implicit success() skips all three of these. The job still ends up red overall (the earlier step's own failure fails the job), but reviewers get no PR comment at all, and the job fails with whatever raw error the earlier step produced instead of this workflow's own clean ::error::Sampler run for partner ... did not complete successfully message.
if: always() && inputs.pull_request_number != '' # Post result comment
if: always() && steps.download.outputs.downloaded == 'true' # Upload artifact
if: always() && steps.sampler.outputs.sampler_ok != 'true' # Fail job| echo "::error::PARTNER_CONFIG_JSON has no partnerCredentials entry for partner ${{ inputs.partner }}. Authorize it first with 'silverfin authorize-partner -i ${{ inputs.partner }} -k <api-key>' and store the resulting config.json as this secret." | ||
| exit 1 | ||
| fi | ||
| echo "Host: $(jq -r '.host // \"(default)\"' "$HOME/.silverfin/config.json")" |
There was a problem hiding this comment.
💡 Suggestion (confirmed bug, cosmetic): jq -r '.host // \"(default)\"' ... — inside single quotes, \" is two literal characters (backslash + quote), not an escaped quote; bash single-quoted strings don't support backslash escapes at all. Reproduced locally: this is a jq syntax error on every single run (prints a "jq: 1 compile error" to stderr) and the "Host: ..." diagnostic always prints blank instead of the configured host or the "(default)" fallback. It doesn't fail the step — the failing command substitution is embedded inside echo's argument, which doesn't trip errexit — but it defeats the diagnostic and adds log noise on every run.
echo "Host: $(jq -r '.host // "(default)"' "$HOME/.silverfin/config.json")"| # 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. |
There was a problem hiding this comment.
💡 Suggestion — new reusable workflow has no corresponding README/docs entry.
This file's own header points to docs/liquid_sampler.md, but that file doesn't exist anywhere in this repo, and the README's "Individual Action Documentation" section (which documents every other reusable workflow — check_auth.yml, run_tests.yml, slack_changelog.yml, push_to_review_firm.yml — each with Description/Trigger/Inputs/Steps/Prerequisites) has no section for run_sampler.yml. The most recent precedent (#24, adding push_to_review_firm.yml) updated README.md in the same PR. Not a functional blocker, but worth adding before/shortly after merge so this workflow doesn't become the one undocumented exception, and so the header comment's doc reference isn't dangling.
1 participant
Summary
run_sampler.yml, aworkflow_call-only reusable workflow that runs the Liquid Sampler (silverfin-cli run-sampler) for a single partner's already-classified templates and reports back.silverfin-cli'sCI_AUTH_SAMPLER_PLAN.md§6 (D1-D9) and §8: per-partnerPARTNER_CONFIG_JSON_<partner>secret (sole writer, write-back gated on a before/after token diff, not log-scraping), per-partnerconcurrency: { group, queue: max }living inside this reusable workflow, poll-and-retry on the backend's cross-repo "already in progress" 422,results.zipuploaded as a 7-day artifact, and a per-partner PR comment (<!-- silverfin-sampler-result-<partner> -->) with the--compactdiff.lu_market(companion PR incoming).Known open items (not blockers for this PR, tracked in the plan doc)
silverfin-cliPR #261 (agustin-bso-sampler-easy-results:--compact, result-URL surfacing, spinner fix) merging tomainbefore a real run will produce a compact diff — the CLI install step is intentionally unpinned (matches every other workflow here), so it will pick that up automatically once merged.queue: maxis a GitHub Actions beta concurrency sub-key;actionlint's schema doesn't recognize it yet (false positive) but it's the same syntax already validated end-to-end on staging inbe_market's TEST harness. Fallback documented inline if a target repo's plan ever rejects it.Test plan
actionlintclean (done locally)