Fix ephemeral sampler report link; add diffs-only zip folder - #35
Merged
Conversation
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 <noreply@anthropic.com>
![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.
|
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 (1)
WalkthroughThe sampler workflow pins its CLI, conditionally enriches downloaded results with ChangesSampler results workflow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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
🤖 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 302-308: Pin the silverfin-cli installation in the workflow to the
specific revision that introduced and supports the run-sampler --from-zip
--add-diffs-folder option, replacing the unpinned main reference. Keep the
existing Add diffs/ folder step and its best-effort behavior unchanged.
- Around line 302-308: Update the “Add diffs/ folder” step to avoid writing
directly over results.zip: run the enrichment into a temporary archive, then
atomically rename it to results.zip only after successful completion. Preserve
the existing best-effort behavior and ensure a failed enrichment leaves the
original results.zip available for upload.
🪄 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: f67d64b9-b4f2-4502-a2e7-fc7dc16f7c4e
📒 Files selected for processing (1)
.github/workflows/run_sampler.yml
michieldegezelle
left a comment
michieldegezelle
left a comment
Collaborator
Author
There was a problem hiding this comment.
🟠 Major — README.md:277 is now stale relative to this PR (not touched in this diff, so posting as a general comment)
README.md's run_sampler.yml documentation still reads (line 277):
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.zipartifact is the fallback.
This PR flips that priority: the GitHub Actions artifact link (steps.upload.outputs.artifact-url) is now the primary link, and the presigned report_url is only the fallback when the artifact upload did not happen — the opposite of what the README describes. The README also has no mention at all of the new "Add diffs/ folder for the entries the compact diff flagged" step.
run_sampler.yml is the one reusable workflow in this repo with a maintained README section (unlike check_auth.yml/run_tests.yml/push_to_review_firm.yml), and past PRs on this workflow (#24, #30) kept that section in sync with behavior changes in the same PR — worth doing the same here, e.g.:
* 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.
* Re-runs `run-sampler --from-zip results.zip --add-diffs-folder` locally (best-effort, no network) to add a `diffs/` folder to the uploaded artifact for the entries the compact diff flagged.- 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Around line 276-277: Add a brief note in the README near the workflow artifact
link description explaining that opening the retained artifact requires GitHub
authentication/sign-in. Keep the existing artifact and presigned-URL behavior
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: 54aa7210-e298-4231-b217-13eee833e972
📒 Files selected for processing (2)
.github/workflows/run_sampler.ymlREADME.md
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
actions/github-script@v7 targets Node 20, which GitHub Actions now forces onto Node 24 with a deprecation warning. Co-authored-by: Cursor <cursoragent@cursor.com>
michieldegezelle
added a commit
that referenced
this pull request
Aug 5, 2026
The pin added in #35 (5accd6b) is not an ancestor of silverfin-cli's sampler-compact-diff-v2 branch: it's a pre-rebase copy of "Make --add-diffs-folder's zip write atomic" that the rebase orphaned. It still runs, so nothing failed loudly — it just silently froze this workflow five fixes behind the branch it claims to track: 0bf8f15 Skip the diffs/ zip rewrite when no view.html is found b148ffa Flag-array dedup, diffs-folder count, radio groups 4598087 Flag-array flip direction + radio-checked attribute boundary 8b05852 Flag-flip suffix only on the after side of a results diff 1ba5a0f Keep byte-identical render pairs out of the diffs/ folder That last one matters most for adopters of this workflow: without it, --add-diffs-folder copies before/after view.html for entries flagged on a data-only change, so diffs/ fills with byte-identical pairs (lu_market run 30920292551 produced 3 such pairs and nothing else). The market wrappers don't have this problem because they install #sampler-compact-diff-v2, the branch, and so track fixes automatically. Documented that re-pinning is required whenever that branch moves, until silverfin-cli#265 merges and this goes back to unpinned main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
michieldegezelle
added a commit
that referenced
this pull request
Aug 5, 2026
…late names survive (#37) * Pass sampler template names newline-separated so spaced names survive Account template identifiers are directory names and routinely contain spaces ("Investment- and depreciation details"). The sampler encoded the handles/account_templates inputs as space-separated strings and then word-split them into CLI args, so -at received "Investment-", "and", "depreciation", "details" and the run died with: [error] Config file for account template "Investment-" not found Switch both inputs to one-name-per-line and build the arg arrays with mapfile, so each line becomes exactly one argv entry. This is already the convention in run_tests.yml (TEMPLATE_BUCKETS) — the sampler was the only place that regressed to space-joining. firm_ids stays space-separated; it's numeric. Also render the names in the PR comment as individually code-quoted, comma-separated entries (a multi-line value inside one backtick pair renders as garbage) and %q-quote the attempt log line so a spaced name is visibly one argument. Callers must now pass these inputs newline-separated — README and the input descriptions updated accordingly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Escape backticks when rendering template names in the PR comment Template names are directory names taken from the PR's own tree, so they are untrusted text. Wrapping them in a single backtick pair let a name containing a backtick close its code span and inject markdown into the sampler's PR comment (CWE-116; spoofed links/formatting, not code execution) — flagged by CodeRabbit on bso_github_actions#37. Fence each name per CommonMark instead: one more backtick than the longest run inside the name, plus a space pad when it starts or ends with one. Names without backticks render exactly as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Reject template names starting with "-" before invoking the CLI Template names are directory names from the PR's own tree. A name starting with "-" is read by the CLI's option parser as a FLAG rather than a value: commander stops consuming a variadic option (-h/-at take <names...>) at the first "-"-prefixed token, so a directory named e.g. "--from-zip" would be reinterpreted as an option instead of a template to sample. A leading "--" separator does not protect variadic values, so the fix is an explicit guard: fail the step with an actionable message (rename the directory) before any CLI call. Flagged by CodeRabbit as CWE-88 on lu_market#785. Names with internal or trailing hyphens ("Cut-off", "Investment- and depreciation details") are unaffected — only a leading "-" is rejected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Re-pin silverfin-cli to the current sampler branch tip (1ba5a0f) The pin added in #35 (5accd6b) is not an ancestor of silverfin-cli's sampler-compact-diff-v2 branch: it's a pre-rebase copy of "Make --add-diffs-folder's zip write atomic" that the rebase orphaned. It still runs, so nothing failed loudly — it just silently froze this workflow five fixes behind the branch it claims to track: 0bf8f15 Skip the diffs/ zip rewrite when no view.html is found b148ffa Flag-array dedup, diffs-folder count, radio groups 4598087 Flag-array flip direction + radio-checked attribute boundary 8b05852 Flag-flip suffix only on the after side of a results diff 1ba5a0f Keep byte-identical render pairs out of the diffs/ folder That last one matters most for adopters of this workflow: without it, --add-diffs-folder copies before/after view.html for entries flagged on a data-only change, so diffs/ fills with byte-identical pairs (lu_market run 30920292551 produced 3 such pairs and nothing else). The market wrappers don't have this problem because they install #sampler-compact-diff-v2, the branch, and so track fixes automatically. Documented that re-pinning is required whenever that branch moves, until silverfin-cli#265 merges and this goes back to unpinned main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Name the value kind in the leading-hyphen rejection, and document it Two review points from CodeRabbit on #37: 1. The guard checks handles AND account templates, but the error called every value a "Template name" and told the user to rename "the template directory" — wrong words for a reconciliation handle. Rather than a neutral label, check each list separately so the message names the kind ("Reconciliation handle" / "Account template name") and the directory to rename (reconciliation_texts/ / account_templates/). 2. The restriction wasn't part of the documented input contract. Added it to both `handles`/`account_templates` input descriptions and the README input list, including why (variadic -h/-at stop at the first "-"-prefixed token; "--" doesn't protect variadic values) and that only a LEADING hyphen is affected — "Cut-off" and "Investment- and depreciation details" are fine. Verified in bash: empty list, both lists populated, hostile handle, and hostile account template each behave correctly (the empty-array call passes only the two label args, so the loop is a no-op). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
report_urlposted as "Open full sampler report" on PR comments expires ~5 minutes after the run completes (confirmed against a realnl_market#899comment) - the link is essentially always dead by the time a reviewer opens it. Now links 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 didn't happen.diffs/folder to the uploadedresults.zip, containing before/afterview.htmlonly for the entries the compact diff flagged (data/scope/vanished-output/visual-only tiers) - via silverfin-cli's newrun-sampler --from-zip --add-diffs-folder(silverfin-cli#265). Lets a reviewer open just the changed entries instead of digging through the full (often ~150 MB) archive. Best-effort (continue-on-error: true) so a failure here never costs the plainresults.zipartifact.Test plan
actionlintclean (aside from the knownqueue: maxfalse-positive already present onmain)--add-diffs-folderactually exists when this workflow runsdiffs/folder🤖 Generated with Claude Code