Separate the Windows and Linux binaries

[simonmarty]

Description

Why is this change being made?

1. Separate the Windows and linux binaries into separate paths and buckets for signing.

What is changing?

1. Simple if/else statements to populate the S3 bucket name and the object path from GH secrets.

Related Links

• Issue #, if available:

---

Testing

How was this tested?

When testing locally, provide testing artifact(s):

---

Reviewee Checklist

Update the checklist after submitting the PR

• I have reviewed, tested and understand all changes
  If not, why:
• I have filled out the Description and Testing sections above
  If not, why:
• Build and Unit tests are passing
  If not, why:
• Unit test coverage check is passing
  If not, why:
• Integration tests pass locally
  If not, why:
• I have updated integration tests (if needed)
  If not, why:
• I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
  If not, why:
• I have added explanatory comments for complex logic, new classes/methods and new tests
  If not, why:
• I have updated README/documentation (if needed)
  If not, why:
• I have clearly called out breaking changes (if any)
  If not, why:

---

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

• Reviewee checklist has been accurately filled out
• Code changes align with stated purpose in description
• Test coverage adequately validates the changes

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.35%. Comparing base (f76ef6f) to head (8c6b4ce).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #252   +/-   ##
=======================================
  Coverage   86.35%   86.35%           
=======================================
  Files          34       34           
  Lines        9422     9422           
  Branches     9422     9422           
=======================================
  Hits         8136     8136           
  Misses       1153     1153           
  Partials      133      133           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[secrets-manager-ai-code-review]

Claude finished @simonmarty's task in 1m 11s —— View job

---

Claude Code Review

• Read PR diff and both workflow files
• Analyze for bugs, security, best practices
• Post review

---

Summary

This PR routes Windows binaries to a separate S3 bucket/key-prefix in both release.yml and staging.yml. The staging.yml refactor (introducing matrix.ext, shell: bash, and env-var indirection) is a nice cleanup that removes several repeated inline ternaries. A few things worth addressing before merge:

---

🔴 Potential silent misrouting (staging.yml)

.github/workflows/staging.yml:71

BUCKET: ${{ runner.os == 'Windows' && secrets.WINDOWS_STAGING_BUCKET || secrets.STAGING_BUCKET }}

This uses the GitHub Actions && / || ternary idiom, which has a well-known footgun: if the left-hand value is falsy (empty/unset), it falls through to the right-hand side. So if secrets.WINDOWS_STAGING_BUCKET is ever unset or empty, Windows binaries will be silently uploaded to secrets.STAGING_BUCKET (the Linux bucket) — exactly the separation this PR is trying to prevent, with no error.

Given the whole point of the change is isolation, consider failing loudly instead. For example, resolve the bucket unconditionally and validate it in the script:

        env:
          BUCKET: ${{ runner.os == 'Windows' && secrets.WINDOWS_STAGING_BUCKET || secrets.STAGING_BUCKET }}
        run: |
          if [[ -z "$BUCKET" ]]; then
            echo "::error::Target bucket is empty for ${{ matrix.target }}"
            exit 1
          fi
          ...

🟠 Leading slash in S3 key when the bin-path secret is empty

.github/workflows/release.yml:82 and .github/workflows/staging.yml:72

# release.yml
key_prefix="${WINDOWS_BIN_PATH}/"
# staging.yml
KEY_PREFIX: ${{ runner.os == 'Windows' && format('{0}/', secrets.STAGING_WINDOWS_BIN_PATH) || '' }}

If WINDOWS_BIN_PATH / STAGING_WINDOWS_BIN_PATH is unset, the key becomes /<version>/<target>/... with a leading slash, which S3 accepts but produces an odd empty-named prefix ("folder"). If these secrets are guaranteed to always be set this is harmless, but a guard (or trimming a trailing/leading slash) would make it robust. Worth confirming the secrets are required in the environment config.

🟡 Inconsistent approach between the two files

The two files solve the same problem two different ways:

• release.yml uses an in-script if [[ "$target" == *windows* ]] branch.
• staging.yml uses expression-level && / || ternaries in env:.

Both work, but the release.yml in-script style is easier to read and avoids the falsy-fallthrough footgun noted above. Aligning staging.yml to the same pattern (or vice-versa) would make these easier to maintain together.

🟡 Secret naming inconsistency

The new secret names don't follow a consistent scheme, which is easy to trip over when configuring environments:

• release.yml: WINDOWS_RELEASE_BUCKET (Windows-first) but RELEASE_WINDOWS_BIN_PATH (Release-first), mapped to env WINDOWS_BIN_PATH.
• staging.yml: WINDOWS_STAGING_BUCKET and STAGING_WINDOWS_BIN_PATH.

Not a functional bug, but standardizing (e.g. <STAGE>_WINDOWS_BUCKET / <STAGE>_WINDOWS_BIN_PATH) would reduce misconfiguration risk.

---

Nits

• release.yml: the find for the binary is run twice (once for count, once for binary). Minor; fine as-is.
• Nice touch using --if-none-match "*" to make release uploads immutable.

Overall the logic is sound; the main thing I'd resolve before merge is the silent Linux-bucket fallthrough in staging.yml.

Note: I'm not able to modify files under .github/workflows/ (GitHub App workflow-permission restriction), so I've left this as review feedback rather than pushing changes.

[i-am-SR]

Claude has good suggestions worth fixing including Potential silent misrouting, Leading slash in S3 key when the bin-path secret is empty and Inconsistent approach between the two files.

The testing section is empty in the description - are we planning to test after merging?

[simonmarty]

I'm going to fix the naming inconsistency for the secrets. The other edge cases strike me as overly defensive. I think it's obvious that all secrets can be expected to be set but I can add a step that actually validates that at the top. I think that's cleaner than having a check in every place where the secrets are used.

[simonmarty]

I'm testing this on my fork. I will post results shortly.