fix(review): page fetchFallbackArtifactShots artifacts-list read past 100 artifacts - #8035
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
… 100 artifacts (JSONbored#8014) fetchFallbackArtifactShots read only page 1 of GET /actions/runs/{id}/artifacts (per_page=100) and .find()'d the fallback artifact there. A run with >100 attached artifacts pushed the target onto page 2+ and the function returned [] exactly as if no artifact existed -- the same silent-truncation failure mode its sibling preview-url.ts documents and was hardened against in JSONbored#7779/JSONbored#7805. Walk the Link: rel="next" pages instead, mirroring preview-url.ts's bounded findAcrossPages walker: page 1's request stays byte-identical to the pre-pagination read, the &page=N cursor is appended for page 2+ only, and the walk is bounded to 10 pages so a pathological response can never spin. Behavior for runs with <=100 artifacts is unchanged. Tests: target artifact found on page 2 (with the page-1 request asserted byte-identical), the 10-page bound under an always-next Link header, no second fetch when the Link header lacks rel="next", and a valid-JSON-no-artifacts page. Changed lines are 100% line- and branch-covered.
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-22 14:32:31 UTC
Review summary Nits — 3 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Summary
fetchFallbackArtifactShots(src/review/visual/actions-fallback.ts) read only page 1 ofGET /repos/{o}/{r}/actions/runs/{runId}/artifacts?per_page=100and.find()'d the fallback artifact there. If a run attaches >100 artifacts, the target silently falls off page 1 and the function returned[]exactly as if no artifact existed — the same silent-truncation failure modepreview-url.tsdocuments and was hardened against in findPreviewUrlFromChecks doesn't paginate check-runs, unlike its sibling functions in the same file #7779/getLatestDeploymentStatus doesn't paginate deployments/statuses reads, unlike its siblings in the same file #7805.Link: rel="next"pages, mirroringpreview-url.ts's boundedfindAcrossPageswalker (the required pattern from the issue): page 1's request stays byte-identical to the pre-pagination read, the&page=Ncursor is appended for page 2+ only, and the walk is bounded to 10 pages (ARTIFACT_LIST_MAX_PAGES, same bound asPREVIEW_LIST_MAX_PAGES/PR_DETAIL_MAX_PAGES/MAX_WORKFLOW_RUN_LIST_PAGES) so a pathological response can never spin.[]contract on any failure.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
vitest run test/unit/actions-fallback.test.ts test/unit/actions-fallback-webhook.test.ts test/unit/visual-capture.test.ts— 234 tests green — plusvitest run --coverage test/unit/actions-fallback.test.ts: every changed line and branch inactions-fallback.tsis covered (the only uncovered lines in the file, 259–260, are pre-existinginflateRawRawinternals untouched by this diff).actionlint/workers/mcp/ui checks are untouched surfaces (no workflow, worker, MCP, or UI files changed); CI runs them all.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.UI Evidence
Not applicable — backend-only change (no UI, docs, or extension surface touched).
Notes
fetchFallbackArtifactShotsdescribe block, reusing itsstubSequencemock pattern): target artifact found on page 2 with the page-1 request asserted byte-identical and the page-2 request asserted to carry&page=2; the 10-page bound under an always-rel="next"Link header (asserts exactly 10 list calls); no second fetch when the Link header lacksrel="next"; and a valid-JSON-but-no-artifacts-array page.