Fix ephemeral sampler report link; add diffs-only zip folder - #35
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 |
|
@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
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>
1 participant
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