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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChangesLiquid sampler workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
- 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.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
README.md (1)
281-285: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winClarify the scope of the “sole writer” guarantee.
Because this is reusable and cross-repository concurrency is not enforced, this is a repository/process invariant rather than something the workflow can guarantee globally. State the scope explicitly and clarify that callers must map their
PARTNER_CONFIG_JSON_<partner>secret into the reusable workflow’s genericPARTNER_CONFIG_JSONsecret.🤖 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 `@README.md` around lines 281 - 285, Revise the workflow documentation near the “sole writer” statement to scope that guarantee to this repository/process, not globally across repositories or concurrent callers. Clarify that each caller must map its partner-specific PARTNER_CONFIG_JSON_<partner> secret to the reusable workflow’s generic PARTNER_CONFIG_JSON secret.
🤖 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:
- Line 268: Update the token comparison in the sampler workflow to avoid
interpolating steps.before.outputs.token directly into Bash source. Pass the
output through the workflow environment or another safely quoted mechanism, then
compare AFTER_TOKEN against that runtime variable while preserving the existing
equality check.
- Around line 148-150: Update the jq validation in the partner configuration
check to require that .partnerCredentials[$p].token is a string with non-zero
length. Keep the existing error message and exit behavior for missing, empty, or
non-string tokens.
- Around line 110-113: Update the input validation in the workflow’s
handles/account_templates check to reject values containing only any whitespace,
including tabs and newlines, rather than removing literal spaces only. Apply the
same whitespace normalization and rejection logic to the FIRM_IDS check,
preserving the existing error-and-exit behavior.
In `@README.md`:
- Line 276: Update the README bullet describing the PR result comment to qualify
that the full-report link is included only when a report URL is available, and
state that the artifact remains the fallback when no URL exists.
---
Nitpick comments:
In `@README.md`:
- Around line 281-285: Revise the workflow documentation near the “sole writer”
statement to scope that guarantee to this repository/process, not globally
across repositories or concurrent callers. Clarify that each caller must map its
partner-specific PARTNER_CONFIG_JSON_<partner> secret to the reusable workflow’s
generic PARTNER_CONFIG_JSON secret.
🪄 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: 8ea6f0c4-026d-4cb5-89f8-6d9f5fcad922
📒 Files selected for processing (2)
.github/workflows/run_sampler.ymlREADME.md
- 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.
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)