feat(build): Add auto-detection of PR number from GitHub Actions #2722

runningcode · 2025-09-01T14:03:16Z

Summary

Implements auto-detection of PR number from GitHub Actions environment variables for the build upload command
When --pr-number is not provided, automatically detects from GITHUB_EVENT_NAME and GITHUB_REF
Adds comprehensive tests for the detection logic

Implementation Details

Checks GITHUB_EVENT_NAME == "pull_request" and GITHUB_REF starts with "refs/pull/"
Parses PR number from GITHUB_REF format: refs/pull/{pr_number}/merge
Includes debug logging when PR number is auto-detected
Updated help text to clarify the auto-detection behavior

Test Plan

Added unit tests covering various GitHub Actions scenarios
Verified compilation and existing tests pass
Tested help text updates
Manual testing with mock environment variables

Resolves Linear issue EME-233

🤖 Generated with Claude Code

linear · 2025-09-01T14:03:19Z

EME-233 Sentry-cli: provide default value for pr_number if user doesn't provide

src/commands/build/upload.rs

runningcode · 2025-09-03T13:03:47Z

src/utils/vcs.rs


+/// Attempts to get the PR number from GitHub Actions environment variables.
+/// Returns the PR number if running in a GitHub Actions pull request environment.
+pub fn get_github_pr_number() -> Option<u32> {


For this function, I didn't log a warning if we failed to fetch here because i figure there are so many different CI providers we'd be logging a warning every time. Until we cover a broader range of CI providers it doesn't make sense to log something. WDYT?

Debug logging should be fine, I'd recommend adding some logs around failure points, particularly in the like 258 block and any other early return points just to make debugging easier.

I second this, as debug logs are hidden by default. Default log level is WARN. However, when folks report bugs, we ask them to set a log level of DEBUG, as the debug logs, though verbose, often help identify the problem.

rbro112

All logic for picking up PR number LGTM, deferring rest of review to @szokeasaurusrex for general CLI

szokeasaurusrex

Some questions and comments

szokeasaurusrex · 2025-09-05T14:07:59Z

src/utils/vcs.rs


+/// Attempts to get the PR number from GitHub Actions environment variables.
+/// Returns the PR number if running in a GitHub Actions pull request environment.
+pub fn get_github_pr_number() -> Option<u32> {


I second this, as debug logs are hidden by default. Default log level is WARN. However, when folks report bugs, we ask them to set a log level of DEBUG, as the debug logs, though verbose, often help identify the problem.

szokeasaurusrex · 2025-09-05T14:13:51Z

src/utils/vcs.rs

+
+    let pr_number_str = github_ref.strip_prefix("refs/pull/")?.split('/').next()?;
+
+    let pr_number = pr_number_str.parse::<u32>().ok()?;


Compiler can infer the type via the function signature, so we don't need the ::<u32> here

Suggested change

let pr_number = pr_number_str.parse::<u32>().ok()?;

let pr_number = pr_number_str.parse().ok()?;

szokeasaurusrex · 2025-09-05T14:15:10Z

src/utils/vcs.rs

+    let github_ref = std::env::var("GITHUB_REF").ok()?;
+    let event_name = std::env::var("GITHUB_EVENT_NAME").ok()?;
+
+    if event_name != "pull_request" || !github_ref.starts_with("refs/pull/") {


As strip_prefix returns None when the string does not start with the prefix, and the ? immediately afterwards causes us to return None in that case, explicitly checking starts_with here is redundant.

Suggested change

if event_name != "pull_request" || !github_ref.starts_with("refs/pull/") {

if event_name != "pull_request" {

Also, how would this work if someone is using a reusable workflow, called within a pull_request workflow?

For instance, Sentry CLI's own CI workflow, which runs on pull requests, kicks off several reusable workflows, such as the Lint workflow. The Lint workflow's triggger is workflow_call. Within that workflow, would the GITHUB_EVENT_NAME be workflow_call or pull_request?

This is a good point. I don’t think we should handle re-usable workflows. For those cases users can specify the PR number automatically.

ok! I suppose we could always add support later if needed

szokeasaurusrex · 2025-09-05T14:17:09Z

src/commands/build/upload.rs

+    #[test]
+    fn test_get_github_pr_number() {
+        std::env::set_var("GITHUB_EVENT_NAME", "pull_request");
+        std::env::set_var("GITHUB_REF", "refs/pull/123/merge");
+
+        let pr_number = get_github_pr_number();
+        assert_eq!(pr_number, Some(123));
+
+        std::env::set_var("GITHUB_EVENT_NAME", "push");
+        let pr_number = get_github_pr_number();
+        assert_eq!(pr_number, None);
+
+        std::env::set_var("GITHUB_EVENT_NAME", "pull_request");
+        std::env::set_var("GITHUB_REF", "refs/heads/main");
+        let pr_number = get_github_pr_number();
+        assert_eq!(pr_number, None);
+
+        std::env::remove_var("GITHUB_EVENT_NAME");
+        std::env::remove_var("GITHUB_REF");
+    }


These tests belong in vcs.rs, as get_github_pr_number is defined there

szokeasaurusrex · 2025-09-05T14:21:20Z

src/commands/build/upload.rs

                .long("pr-number")
                .value_parser(clap::value_parser!(u32))
-                .help("The pull request number to use for the upload. If not provided, the current pull request number will be used.")
+                .help("The pull request number to use for the upload. If not provided and running in a GitHub Actions environment, the PR number will be automatically detected from GitHub Actions environment variables.")


I would update this help text to clarify that the PR number is only detected when we are in a pull_request-triggered workflow. Also, as this string is a bit long, you should split it across lines, kinda like this (sadly, we would need a nightly version of rustfmt to do this automatically):

Suggested change

.help("The pull request number to use for the upload. If not provided and running in a GitHub Actions environment, the PR number will be automatically detected from GitHub Actions environment variables.")

.help("The pull request number to use for the upload. If not provided and running \

in a GitHub Actions environment, the PR number will be automatically detected from a \

GitHub Actions environment variables.")

When you end the line in a \, Rust ignores the following newline and also all whitespace characters until the first non-whitespace character on the following line

szokeasaurusrex

some minor things, looks good otherwise

src/commands/build/upload.rs

src/utils/vcs.rs

runningcode · 2025-09-08T14:37:09Z

Ready for another review!

When the --pr-number argument is not provided to the build upload command, the PR number is now automatically detected from GitHub Actions environment variables (GITHUB_EVENT_NAME and GITHUB_REF). This enhancement works by: - Checking if GITHUB_EVENT_NAME is "pull_request" - Parsing GITHUB_REF format "refs/pull/{pr_number}/merge" - Extracting the PR number and using it as the default value Includes comprehensive tests to verify the detection logic works correctly for various GitHub Actions scenarios. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

- Run cargo fmt to fix code formatting issues - Update help text in test snapshots to match new PR number auto-detection behavior - All build-related tests now pass except one unrelated flaky test

Fixes clippy warning by removing unnecessary closure wrapper around get_default_pr_number function call. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

- Rename function from get_default_pr_number to get_github_pr_number for clarity - Simplify nested conditional logic for better readability - Add qualifier to help text about GitHub Actions environment requirement - Move function from upload.rs to vcs.rs for better organization 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

Update test expectations to match the improved help text that clarifies GitHub Actions environment requirement for PR auto-detection. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

- Add debug logging for failure points in get_github_pr_number - Remove explicit type annotation from parse() call - Remove redundant starts_with check since strip_prefix handles it 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

- Split long help string across multiple lines using backslashes - Clarify that PR number detection only works in pull_request-triggered workflows 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

The help text for --pr-number option was updated to be more specific about pull_request-triggered workflows, so the integration test snapshot needed to be updated to match. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

Update the build-upload-help-not-macos.trycmd test snapshot to match the updated help text that specifies pull_request-triggered workflows. This should fix the failing tests on Linux, Windows, and macOS x86_64. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

- Remove explicit type annotation from get_one() call - Move test into proper mod tests block with #[cfg(test)] Addresses the final review comments on the PR. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

cursor · 2025-09-09T12:27:26Z

src/utils/vcs.rs

+        std::env::remove_var("GITHUB_EVENT_NAME");
+        std::env::remove_var("GITHUB_REF");
+    }
+}


Bug: Test Pollution from Global Environment Modifications

The test_get_github_pr_number function modifies global environment variables (GITHUB_EVENT_NAME, GITHUB_REF) without isolation. This creates shared state, which can lead to race conditions and flaky test results when tests run in parallel. If the test panics, these variables may also remain set, affecting other tests.

runningcode force-pushed the nelsonosacky/eme-233-default-pr-number branch from ebb2525 to 17ab728 Compare September 2, 2025 12:30

rbro112 reviewed Sep 3, 2025

View reviewed changes

src/commands/build/upload.rs Outdated Show resolved Hide resolved

rbro112 reviewed Sep 3, 2025

View reviewed changes

src/commands/build/upload.rs Outdated Show resolved Hide resolved

rbro112 reviewed Sep 3, 2025

View reviewed changes

src/commands/build/upload.rs Outdated Show resolved Hide resolved

runningcode commented Sep 3, 2025

View reviewed changes

runningcode marked this pull request as ready for review September 3, 2025 13:09

runningcode requested review from a team and szokeasaurusrex as code owners September 3, 2025 13:09

rbro112 approved these changes Sep 3, 2025

View reviewed changes

szokeasaurusrex reviewed Sep 5, 2025

View reviewed changes

runningcode force-pushed the nelsonosacky/eme-233-default-pr-number branch from aecb258 to 868934a Compare September 8, 2025 07:45

szokeasaurusrex reviewed Sep 8, 2025

View reviewed changes

src/commands/build/upload.rs Outdated Show resolved Hide resolved

src/utils/vcs.rs Outdated Show resolved Hide resolved

runningcode force-pushed the nelsonosacky/eme-233-default-pr-number branch from 646b99e to d424014 Compare September 8, 2025 11:03

szokeasaurusrex approved these changes Sep 9, 2025

View reviewed changes

runningcode and others added 11 commits September 9, 2025 14:15

fix: Format code and update test snapshots for PR auto-detection

c56e32e

- Run cargo fmt to fix code formatting issues - Update help text in test snapshots to match new PR number auto-detection behavior - All build-related tests now pass except one unrelated flaky test

fix: Remove redundant closure in PR number detection

4046833

Fixes clippy warning by removing unnecessary closure wrapper around get_default_pr_number function call. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

Update integration tests for help text changes

5509fd9

Update test expectations to match the improved help text that clarifies GitHub Actions environment requirement for PR auto-detection. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

Format code and update test snapshots for PR auto-detection

9dabcbb

runningcode force-pushed the nelsonosacky/eme-233-default-pr-number branch from d424014 to 54f47bd Compare September 9, 2025 12:17

Format code

69fafe2

cursor bot reviewed Sep 9, 2025

View reviewed changes

runningcode merged commit e3e0caf into master Sep 9, 2025
23 checks passed

runningcode deleted the nelsonosacky/eme-233-default-pr-number branch September 9, 2025 12:48


		let pr_number_str = github_ref.strip_prefix("refs/pull/")?.split('/').next()?;

		let pr_number = pr_number_str.parse::<u32>().ok()?;

	let pr_number = pr_number_str.parse::<u32>().ok()?;
	let pr_number = pr_number_str.parse().ok()?;

	if event_name != "pull_request" \|\| !github_ref.starts_with("refs/pull/") {
	if event_name != "pull_request" {

Uh oh!

feat(build): Add auto-detection of PR number from GitHub Actions #2722

feat(build): Add auto-detection of PR number from GitHub Actions #2722

Uh oh!

Conversation

runningcode commented Sep 1, 2025

Summary

Implementation Details

Test Plan

Uh oh!

linear bot commented Sep 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbro112 left a comment

Choose a reason for hiding this comment

Uh oh!

szokeasaurusrex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szokeasaurusrex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

runningcode commented Sep 8, 2025

Uh oh!

cursor bot Sep 9, 2025

Choose a reason for hiding this comment

Bug: Test Pollution from Global Environment Modifications

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants