Skip to content

fix(github): handle x86 release assets as x64 fallback#10103

Merged
jdx merged 2 commits into
jdx:mainfrom
risu729:fix/github-opengrep-x86-assets
May 31, 2026
Merged

fix(github): handle x86 release assets as x64 fallback#10103
jdx merged 2 commits into
jdx:mainfrom
risu729:fix/github-opengrep-x86-assets

Conversation

@risu729
Copy link
Copy Markdown
Contributor

@risu729 risu729 commented May 27, 2026

Summary

  • allow GitHub release assets tagged only as x86 to serve as a low-priority fallback on x64 targets
  • preserve explicit x64/amd64 preference when correctly named assets exist
  • keep the fallback scoped so non-x64 targets still reject mismatched x86 assets

Split scope

Discussion

Part of #10033.

Tests

  • git diff --check upstream/main...HEAD
  • Attempted cargo test test_x86_asset_is_x64_fallback; local test build stops before this test in untouched src/backend/aqua.rs test code with the existing Option<bool>/bool mismatch around complete_windows_ext.
  • GitHub CI passed on the rebased and narrowed branch.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a fallback mechanism to treat x86 assets as low-priority options for x86_64 targets when no exact x64 match is found. It also adds support for recognizing ".cert" extensions as non-binary assets, and expands the recognized OS patterns to include "manylinux", "musllinux", and "osx". Relevant unit tests have been added to verify these changes. I have no feedback to provide as there are no review comments to address.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 27, 2026

Greptile Summary

This PR addresses a real-world install failure for tools (e.g. opengrep) that tag their x86-64 release artifacts as simply x86 instead of x86_64/amd64. It adds a low-priority fallback score of 5 inside score_arch_match so those assets are considered on x64 targets while correctly-named x64 assets still win when both are present.

  • When an asset carries the AssetArch::X86 arch tag and the current target resolves to x64, score_arch_match now returns 5 instead of the usual -150 mismatch penalty, and the has_arch_mismatch guard (which checks for negative scores) leaves the asset eligible.
  • A new test_x86_asset_is_x64_fallback test covers: x86-only release sets (manylinux preferred over musllinux), x64 winning when both x86 and x86_64 assets are present, and aarch64 targets correctly getting no match from an x86/x64-only set.

Confidence Score: 5/5

The change is a small, isolated scoring adjustment in asset selection with no data-loss or security implications; all affected code paths are covered by existing and new tests.

The logic is minimal and correctness follows directly from the score ordering (5 < 50, both > 0) and the pre-existing filter in pick_best_asset. Real 32-bit x86 targets are unaffected because they hit the exact-match branch (score 50) before the new fallback branch. The three test scenarios added exercise the key cases with no gaps visible from the implementation.

No files require special attention.

Important Files Changed

Filename Overview
src/backend/asset_matcher.rs Adds a score-5 x86→x64 fallback in score_arch_match and a corresponding test; the pattern ordering and filter threshold ensure correctness.

Reviews (2): Last reviewed commit: "fix(github): narrow x86 fallback scope" | Re-trigger Greptile

@risu729 risu729 force-pushed the fix/github-opengrep-x86-assets branch from e287d7f to 0a0a5fb Compare May 31, 2026 09:42
@risu729 risu729 marked this pull request as ready for review May 31, 2026 14:43
@jdx jdx merged commit 5e16243 into jdx:main May 31, 2026
32 checks passed
@risu729 risu729 deleted the fix/github-opengrep-x86-assets branch May 31, 2026 15:17
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