Skip to content

refactor github action upload flow#340

Open
noam-starkware wants to merge 2 commits into
mainfrom
noamp/refactor_upload_flow
Open

refactor github action upload flow#340
noam-starkware wants to merge 2 commits into
mainfrom
noamp/refactor_upload_flow

Conversation

@noam-starkware
Copy link
Copy Markdown
Contributor

@noam-starkware noam-starkware commented May 5, 2026

Type

  • feature
  • bugfix
  • dev (no functional changes, no API changes)
  • fmt (formatting, renaming)
  • build
  • docs
  • testing

Description

Breaking changes?

  • yes
  • no

This change is Reviewable

@cursor
Copy link
Copy Markdown

cursor Bot commented May 5, 2026

PR Summary

Low Risk
Low risk refactor of CI workflow logic; main risk is misnaming/paths in the new composite action causing missing or misrouted uploaded artifacts.

Overview
Refactors the artifact upload workflow by extracting the repeated arch-specific Rust build/rename/upload steps into a new composite GitHub Action (.github/actions/build-arch-binary).

The workflow now calls this action for each target CPU (znver{3,4,5}, emeraldrapids, sapphirerapids, graniterapids), standardizing the target-cpu build flags, binary renaming from hyphens to underscores, and the GCS destination naming convention.

Reviewed by Cursor Bugbot for commit b20163c. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.16%. Comparing base (9824175) to head (b20163c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #340   +/-   ##
=======================================
  Coverage   53.16%   53.16%           
=======================================
  Files          35       35           
  Lines        5295     5295           
=======================================
  Hits         2815     2815           
  Misses       2480     2480           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread .github/workflows/upload_artifacts_workflow.yml Outdated
Comment on lines +23 to +29
run: |
BINARY_NAME="${{ inputs.binary_name }}"
BINARY_NAME="${BINARY_NAME//-/_}"
mv "target/x86_64-unknown-linux-gnu/release/${{ inputs.binary_name }}" \
"target/x86_64-unknown-linux-gnu/release/${BINARY_NAME}"
echo "BINARY_NAME=${BINARY_NAME}" >> "$GITHUB_ENV"

Copy link
Copy Markdown

@semgrep-code-starkware-libs semgrep-code-starkware-libs Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using variable interpolation ${{...}} with github context data in a run: step could allow an attacker to inject their own code into the runner. This would allow them to steal secrets and code. github context data can have arbitrary user input and should be treated as untrusted. Instead, use an intermediate environment variable with env: to store the data and use the environment variable in the run: script. Be sure to use double-quotes the environment variable, like this: "$ENVVAR".

🥳 Fixed in commit 4b9d4a7 🥳

@noam-starkware noam-starkware force-pushed the noamp/refactor_upload_flow branch from 85b3f0a to 023cacd Compare May 6, 2026 06:05
@noam-starkware noam-starkware force-pushed the noamp/refactor_upload_flow branch from 023cacd to 4b9d4a7 Compare May 7, 2026 05:56
@noam-starkware noam-starkware force-pushed the noamp/refactor_upload_flow branch from 3e385a6 to b20163c Compare May 7, 2026 09:54
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b20163c. Configure here.

uses: google-github-actions/upload-cloud-storage@v2
with:
path: "target/x86_64-unknown-linux-gnu/release/${{ env.BINARY_NAME_DEST }}"
destination: "${{ env.BINARY_NAME_DEST }}_${{ env.ARCH_NAME }}_artifacts/${{ env.SHORT_HASH }}/release"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Composite action has undeclared dependency on SHORT_HASH

Medium Severity

The composite action references ${{ env.SHORT_HASH }} in the upload destination but never declares it as an input parameter, unlike arch and binary_name. This creates a hidden contract with the calling workflow. If the action is reused in a context where SHORT_HASH isn't set, the destination path silently includes an empty segment, uploading artifacts to the wrong location with no error. Making SHORT_HASH (or a commit_hash equivalent) an explicit required input would make the action self-contained and safe to reuse.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b20163c. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We invoke via the upload script which defines the hash so I don't think this is an issue and passing the value seems redundant since it's already in scope at invoke time.

Copy link
Copy Markdown
Contributor Author

@noam-starkware noam-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@noam-starkware made 1 comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on YairVaknin-starkware).

uses: google-github-actions/upload-cloud-storage@v2
with:
path: "target/x86_64-unknown-linux-gnu/release/${{ env.BINARY_NAME_DEST }}"
destination: "${{ env.BINARY_NAME_DEST }}_${{ env.ARCH_NAME }}_artifacts/${{ env.SHORT_HASH }}/release"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We invoke via the upload script which defines the hash so I don't think this is an issue and passing the value seems redundant since it's already in scope at invoke time.

Copy link
Copy Markdown
Contributor

@YairVaknin-starkware YairVaknin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YairVaknin-starkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on noam-starkware).


a discussion (no related file):
Have u created a dummy commit over this branch so the push binaries workflow runs for pushes to this branch to test it works?

Copy link
Copy Markdown
Contributor Author

@noam-starkware noam-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@noam-starkware made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on YairVaknin-starkware).


a discussion (no related file):

Previously, YairVaknin-starkware wrote…

Have u created a dummy commit over this branch so the push binaries workflow runs for pushes to this branch to test it works?

Yes, sorry, I thought I wrote this here. I compiled an exmaple and made sure the resulting bins are equivalent to the one from the base on main.

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.

2 participants