Skip to content

ci: unblock fork PRs (SENTRY_CLIENT_ID fallback + fork-safe checkout)#813

Merged
BYK merged 3 commits intomainfrom
byk/fix-fork-pr-ci
Apr 22, 2026
Merged

ci: unblock fork PRs (SENTRY_CLIENT_ID fallback + fork-safe checkout)#813
BYK merged 3 commits intomainfrom
byk/fix-fork-pr-ci

Conversation

@BYK
Copy link
Copy Markdown
Member

@BYK BYK commented Apr 22, 2026

Summary

  • Three failures on PR #806 / run 24747631157 traced to fork-PR restrictions:
    1. check-generated checks out github.head_ref from the base repo, but that branch only exists on the fork → git fetch exits 1.
    2. build-binary and build-npm hard-fail because ${{ vars.SENTRY_CLIENT_ID }} isn't reaching fork jobs (likely blocked by the getsentry org's "Send variables to fork PR workflows" policy).
    3. CI Status fails because of (1) and (2).

Fix

  • check-generated checkout: condition ref: on whether the app token step succeeded. Same-repo PRs keep the branch-head checkout (so auto-commit can git push regenerated docs back). Fork PRs leave ref empty → actions/checkout defaults to GITHUB_REF (the PR merge SHA, always fetchable from the base repo with github.token).
  • build-binary / build-npm env: fall back to a dummy SENTRY_CLIENT_ID when vars.SENTRY_CLIENT_ID is empty. All tests in the repo already tolerate any non-empty value (see test/preload.ts and the "test-client-id" defaults in e2e tests); CI smoke tests are --help only. PR binaries never ship, so runtime OAuth paths aren't exercised.

What doesn't change

  • Same-repo PRs and main/release/** pushes still bake the real vars.SENTRY_CLIENT_ID into the binary.
  • script/build.ts and script/bundle.ts keep their hard-fail guard — useful dev ergonomic for local builds.
  • Security posture: no pull_request_target, no new secret exposure, no org-policy change.

Test plan

- build-binary/build-npm: fall back to a dummy SENTRY_CLIENT_ID when
  vars.SENTRY_CLIENT_ID isn't available (fork PRs). The resulting binary
  is only smoke-tested (--help) and never shipped, so any non-empty value
  works; tests already tolerate the dummy via test/preload.ts.
- check-generated: fork PRs leave `ref` empty so checkout uses
  GITHUB_REF (PR merge SHA) instead of requesting a branch that only
  exists on the fork. Same-repo PRs still check out the branch head so
  auto-commit can push back.

Fixes the three failures on PR #806.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 22, 2026

Codecov Results 📊

138 passed | Total: 138 | Pass Rate: 100% | Execution Time: 0ms

📊 Comparison with Base Branch

Metric Change
Total Tests
Passed Tests
Failed Tests
Skipped Tests

✨ No test changes detected

All tests are passing successfully.

✅ Patch coverage is 100.00%. Project has 1951 uncovered lines.
❌ Project coverage is 95.19%. Comparing base (base) to head (head).

Coverage diff
@@            Coverage Diff             @@
##          main       #PR       +/-##
==========================================
- Coverage    95.21%    95.19%    -0.02%
==========================================
  Files          282       282         —
  Lines        40577     40576        -1
  Branches         0         0         —
==========================================
+ Hits         38633     38625        -8
- Misses        1944      1951        +7
- Partials         0         0         —

Generated by Codecov Action

Both values are referenced in multiple jobs/steps (build-binary, build-npm,
build-docs). Moving them to the workflow-level `env:` block eliminates
repetition and keeps the fork-PR fallback in one place.
Copy link
Copy Markdown
Contributor

@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, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 343bff7. Configure here.

Comment thread .github/workflows/ci.yml Outdated
Cursor Bugbot caught that SENTRY_AUTH_TOKEN is scoped to the production
GitHub environment. Workflow-level env is evaluated before a job's
environment: is applied, so hoisting it there would resolve the secret
against repo-level secrets only — silently breaking sourcemap uploads.

- build-binary / build-npm: re-add SENTRY_AUTH_TOKEN at step level.
- build-docs: declare at job level so the `if: env.SENTRY_AUTH_TOKEN != ''`
  guard on the sourcemap-upload step can see it (job-level env is
  resolved after environment: is applied).

SENTRY_CLIENT_ID stays at workflow level since it's a repo-level var,
not environment-scoped.
Comment thread .github/workflows/ci.yml
@BYK BYK merged commit 7d1d9ff into main Apr 22, 2026
25 checks passed
@BYK BYK deleted the byk/fix-fork-pr-ci branch April 22, 2026 11:43
BYK added a commit that referenced this pull request Apr 22, 2026
## Summary
Follow-up to #813. The `Docs Preview` workflow (`docs-preview.yml`)
tries to push to the base repo's `gh-pages` branch, but fork PRs get a
read-only `GITHUB_TOKEN` and fail with HTTP 403 (`Permission to
getsentry/cli.git denied to github-actions[bot]`).

Observed on [PR #806 / run
24776423153](https://github.com/getsentry/cli/actions/runs/24776423153/job/72495750855?pr=806)
after rebasing onto main.

## Fix
Gate the `.nojekyll` setup step and the `Deploy Preview` step with `if:
github.event.pull_request.head.repo.full_name == github.repository ||
github.event_name != 'pull_request'`. The build step still runs
unconditionally so docs compilation errors surface on fork PRs — just no
live preview published.

## Test plan
- Same-repo PRs / pushes to main: unchanged, preview still deploys.
- Fork PR #806: re-run CI after this merges; `preview` job should
succeed (build + skip deploy).
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.

1 participant