Move fork-PR credentialed CI off pull_request_target by david-siqi-liu · Pull Request #1044 · databricks/unity-gateway

david-siqi-liu · 2026-10-08T15:17:32Z

After this PR, no workflow uses pull_request_target. Fork PRs get credentialed CI when an org member (MEMBER or OWNER) comments a command on the PR. Every comment-triggered run uses the default branch's workflow definitions. Design doc (option B): https://docs.google.com/document/d/1v-6znVBCb8YfGxr12SBd80w3i9n4U0fMEYohyz9Aoyw/edit

Commands

Org members comment these on a PR:

/integration-test runs the full Integration workflow with secrets on the PR's current head commit.
/integration-test <sha> does the same, but only if the head is still <sha> (7 to 40 hex characters). If the fork pushed since, it refuses and replies with the new head.
/ug-review runs the UG rubric review.
/user-journey-check runs the user-journey gate. For in-repo PRs it also runs automatically, so the command is mostly for forks.

Each command posts a commit status and a comment with the run link on the PR. The statuses are Fork integration and User Journey Test Required. Each new push needs a new command. Anyone else who comments /integration-test gets a reply saying only org members can run integration tests.

Before and after

Live integration on fork PRs:

Before: every credentialed job skips, because fork PRs get no secrets on pull_request. That covers the E2E CUJs, workspace validation, and the smoke, full, and managed journeys.
After: a member reviews, then comments /integration-test or /integration-test <sha>. It runs integration.yml on the pinned commit with secrets.

Live integration on in-repo PRs:

Before: runs automatically.
After: unchanged. /integration-test also works as an on-demand rerun, and it never cancels the automatic run.

UG review:

Before: adding the ug-review label, via pull_request_target.
After: a member comments /ug-review. It still runs only default-branch code and never executes the PR.

User Journey Test Required:

Before: pull_request_target on every PR.
After: pull_request for in-repo PRs, and /user-journey-check for fork PRs. It still reads the PR only through the API.

pull_request_target:

Before: used by ug-review.yml and user-journey-test-required.yml.
After: not used anywhere, and a contract test fails if it comes back. No admin event-policy exception is needed before GitHub blocks it on public repos on 2026-11-02.

Fork safety in `integration.yml`

It takes an optional ref input. When the input is empty, existing callers behave exactly as before.
Every checkout pins ref and sets persist-credentials: false.
setup-uv skips its cache for ref-pinned runs, so fork builds cannot poison it.
Concurrency groups key on the PR number, so runs for different PRs never cancel each other. Concurrency is set per job, so ordinary PR comments never cancel a run.

Testing

Contract tests cover the triggers, the member-only gate, and the authorize script, run against a fake gh. They also check the non-member reply, the pinned credential-free checkouts, the disabled caches, and the status reports.
issue_comment workflows only run from the default branch, so the first live run happens after merge. The plan is to comment /integration-test on a fork PR such as Memoize Databricks CLI tokens per process and skip impossible --no-browser re-auth #1017 or Cut ug startup cost: fast --version entry point and lazy heavy imports #1018, and /ug-review on any PR.

OIDC note: under issue_comment, the OIDC subject is the default branch, not a PR ref. If the JFrog trust policy for the Windows installation lane keys on PR refs, that one lane fails on fork runs. The other lanes use secrets, not OIDC.

This pull request and its description were written by Isaac.

Fork PRs get no secrets on pull_request, so every credentialed integration job skips for them, and GitHub is closing the pull_request_target route (default-branch-only workflows, the checkout fork guard, and the default block in public repos from 2026-11-02). Following dbt-databricks, an org member comments /integration-test after reviewing a PR. An issue_comment workflow from the default branch runs integration.yml on the PR head with secrets and posts a Fork integration status and comment. /integration-test <sha> runs only if the head is still the reviewed commit. Non-members get a reply saying only org members can run it, and in-repo PRs can use it as an on-demand rerun. integration.yml takes the commit as a ref input, pins every checkout to it without persisted credentials, skips the uv cache for these runs, and keys its concurrency group per PR for comment events. ug-review.yml moves to an org member /ug-review comment. user-journey-test-required.yml runs on pull_request for in-repo PRs and on an org member /user-journey-check comment for fork PRs, reporting a commit status back. No workflow uses pull_request_target any more. Co-authored-by: Isaac <no-reply@databricks.com>

The managed-opencode job landed on main (#896) after this branch was cut, so it lacked the pinned, credential-free checkout and the disabled uv cache that fork runs require. Co-authored-by: Isaac <no-reply@databricks.com>

The authorize step is bash that only runs on ubuntu-latest, and its fake `gh` shim cannot execute on the Windows unit-test runner. Co-authored-by: Isaac <no-reply@databricks.com>

rohita5l

Reviewed the CI trigger changes. Please address the inline P1 concerning journey-check results being attached to a different revision than the one evaluated. Focused verification: 91 tests passed; live comment-triggered workflows were not exercised.

rohita5l · 2026-10-09T20:24:05Z

+          RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
+        run: |
+          set -euo pipefail
+          sha=$(gh api "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER" --jq .head.sha)


[P1] Bind the journey-check status to the revision actually evaluated.

The gate reads the PR diff before invoking the judge, but this reporting step fetches the current head SHA again after evaluation. If another commit is pushed during the check, a successful verdict for the previous revision is posted to the new, unchecked commit. For fork PRs, the synchronize-triggered gate is skipped, so that push does not cancel this comment-triggered evaluation.

Capture and expose the evaluated SHA, verify the PR revision remains unchanged across the diff reads, and post the result only against that SHA rather than refetching the current head here. Please add coverage for a head change between evaluation and reporting.

max-rozen-oss-db

Can you put something in our AGENTS.md/CLAUDE.md that certain workflows need to run on pull_request not pull_request_target?

Personally I prefer to can trigger CI with a label (less clutter than comments), would be nice to have both options

david-siqi-liu force-pushed the david/ci-fork-integration-approval branch 2 times, most recently from c21bf9a to aa4658a Compare October 8, 2026 15:23

This was referenced Oct 8, 2026

[test, do not merge] Fork integration check on #1017 #1045

Closed

[test, do not merge] Fork integration check on #1018 #1046

Closed

Base automatically changed from david/ci-skip-windows-oidc-on-forks to main October 8, 2026 15:47

david-siqi-liu force-pushed the david/ci-fork-integration-approval branch from aa4658a to 30e828f Compare October 8, 2026 17:02

david-siqi-liu changed the title ~~Run credentialed integration tests on approved fork PRs~~ Run credentialed integration on fork PRs via /integration-test Oct 8, 2026

david-siqi-liu force-pushed the david/ci-fork-integration-approval branch 2 times, most recently from ea0669a to ef6ad0d Compare October 8, 2026 17:20

david-siqi-liu changed the title ~~Run credentialed integration on fork PRs via /integration-test~~ Move fork-PR credentialed CI off pull_request_target Oct 8, 2026

david-siqi-liu force-pushed the david/ci-fork-integration-approval branch 4 times, most recently from 47bbbf4 to e76dce1 Compare October 8, 2026 20:16

david-siqi-liu force-pushed the david/ci-fork-integration-approval branch from e76dce1 to 978df65 Compare October 9, 2026 17:13

david-siqi-liu and others added 2 commits October 9, 2026 18:21

Pin the managed OpenCode checkout to the reviewed ref

d73b010

The managed-opencode job landed on main (#896) after this branch was cut, so it lacked the pinned, credential-free checkout and the disabled uv cache that fork runs require. Co-authored-by: Isaac <no-reply@databricks.com>

Skip the fork authorize script test on Windows

3bf1e47

The authorize step is bash that only runs on ubuntu-latest, and its fake `gh` shim cannot execute on the Windows unit-test runner. Co-authored-by: Isaac <no-reply@databricks.com>

david-siqi-liu marked this pull request as ready for review October 9, 2026 19:12

david-siqi-liu requested review from AarushiShah-db, lilly-luo and rohita5l as code owners October 9, 2026 19:12

rohita5l approved these changes Oct 9, 2026

View reviewed changes

max-rozen-oss-db approved these changes Oct 9, 2026

View reviewed changes

Navigation Menu

Repository navigation

Move fork-PR credentialed CI off pull_request_target - #1044

Move fork-PR credentialed CI off pull_request_target#1044
david-siqi-liu wants to merge 3 commits into
mainfrom
david/ci-fork-integration-approval

david-siqi-liu commented Oct 8, 2026 •

edited

Loading

Uh oh!

rohita5l left a comment

Uh oh!

rohita5l Oct 9, 2026

Uh oh!

max-rozen-oss-db left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

david-siqi-liu commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Commands

Before and after

Fork safety in integration.yml

Testing

Uh oh!

rohita5l left a comment

Choose a reason for hiding this comment

Uh oh!

rohita5l Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

max-rozen-oss-db left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

david-siqi-liu commented Oct 8, 2026 •

edited

Loading

Fork safety in `integration.yml`

max-rozen-oss-db left a comment •

edited

Loading