Skip to content

Conversation

@hadley
Copy link
Member

@hadley hadley commented Aug 12, 2025

Don't see any obvious way to test this, but I tried it out a bunch interactively. Snapshot test deliberately failing so I can double check that the message is useful.

Fixes #1779

hadley added 8 commits August 12, 2025 14:31
Don't see any obvious way to test this, but I tried it out a bunch interactively. Snapshot test deliberately failing so I can double check that the message is useful.

Fixes #1779
@claude
Copy link

claude bot commented Aug 12, 2025

Claude finished @hadley's task —— View job


PR Review: Download snapshots from GitHub

Reviewed the snapshot download functionality for potential bugs and issues.

Issues Found

High Priority:

  1. No input validation for run_id parameter (R/snapshot-github.R:18): The function accepts run_id as a number but GitHub run IDs are strings that can overflow numeric types. Should use check_string() instead of check_number_whole().

  2. Unsafe regex parsing (R/snapshot-github.R:82): The regex "Artifact download URL: (?<artifact_url>.*)" uses .* which could match malicious URLs. Should validate URL format or use more restrictive pattern.

Medium Priority:
3. No error handling for GitHub API calls (R/snapshot-github.R:55,74,96): All gh::gh() calls lack try-catch blocks. Network failures or auth issues will crash with unhelpful errors.

  1. Potential directory traversal (R/snapshot-github.R:39,40): No validation that src_snaps path is safe before copying. Malicious artifacts could contain ../../../ paths.

  2. Overwrites files without confirmation (R/snapshot-github.R:40,138): dir_copy() and file.copy() overwrite existing files. Should warn users about overwrites.

Low Priority:
6. Missing dependency check (R/snapshot-github.R:21): Only checks for gh package but also uses withr, cli, and utils functions that could fail.

  1. No tests for new functionality: The main snapshot_download_gh() function has no direct tests, only integration via the hint system.

Minor:
8. Inconsistent variable naming (R/snapshot-github.R:34-38): Mixed use of files vs inner_dir could be clearer.

The deliberately failing test mentioned in the PR description appears to be the snapshot hint integration test, which correctly shows the new download command format.

@hadley hadley merged commit c6072b4 into main Aug 13, 2025
13 checks passed
@hadley hadley deleted the gh-download branch August 13, 2025 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document ways to make expect_snapshot_file() work on CI

2 participants