From 8573eefb129edfc986c2581d8ae58a196cec2944 Mon Sep 17 00:00:00 2001 From: David Liu Date: Thu, 8 Oct 2026 15:16:47 +0000 Subject: [PATCH 1/4] Move fork-PR credentialed CI off pull_request_target 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 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 --- .github/scripts/ug-review/review.py | 4 +- .../scripts/user-journey-required/check.py | 7 +- .github/workflows/fork-integration.yml | 127 ++++++++++++++++ .github/workflows/integration.yml | 25 +++- .github/workflows/ug-review.yml | 25 ++-- .../workflows/user-journey-test-required.yml | 55 +++++-- tests/README.md | 9 ++ tests/test_integration_contract.py | 141 ++++++++++++++++++ tests/test_ug_review_bot.py | 2 +- 9 files changed, 365 insertions(+), 30 deletions(-) create mode 100644 .github/workflows/fork-integration.yml diff --git a/.github/scripts/ug-review/review.py b/.github/scripts/ug-review/review.py index bc86ec729..14906ad75 100644 --- a/.github/scripts/ug-review/review.py +++ b/.github/scripts/ug-review/review.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 -"""Post a label-triggered, LLM-generated Unity Gateway pull-request review. +"""Post a comment-triggered, LLM-generated Unity Gateway pull-request review. -The script runs from the trusted default branch under ``pull_request_target``. +The script runs from the trusted default branch, triggered by a ``/ug-review`` comment. It retrieves PR-controlled metadata and patches as untrusted text through the GitHub API; it never checks out or executes code from the PR head. """ diff --git a/.github/scripts/user-journey-required/check.py b/.github/scripts/user-journey-required/check.py index aacb9bcc3..34c7aadae 100644 --- a/.github/scripts/user-journey-required/check.py +++ b/.github/scripts/user-journey-required/check.py @@ -1,9 +1,10 @@ #!/usr/bin/env python3 """Require integration coverage when a PR changes a ug user journey. -This script is run only from the trusted default branch by a -``pull_request_target`` workflow. PR-controlled patches are untrusted text: they -are retrieved through the GitHub API and sent to the judge, never executed. +The workflow checks this script out from the default branch and runs it automatically +for in-repo PRs, or for a fork PR when an org member comments ``/user-journey-check``. +PR-controlled patches are untrusted text: they are retrieved through the +GitHub API and sent to the judge, never executed. """ from __future__ import annotations diff --git a/.github/workflows/fork-integration.yml b/.github/workflows/fork-integration.yml new file mode 100644 index 000000000..e7600f58d --- /dev/null +++ b/.github/workflows/fork-integration.yml @@ -0,0 +1,127 @@ +name: Fork integration + +# Fork PRs get no secrets on `pull_request`. After reviewing a PR, an org member +# comments `/integration-test` to run the Integration workflow with secrets on the PR +# head, or `/integration-test ` to run only if the head is still the reviewed +# commit. Workflow definitions always come from the default branch. +on: + issue_comment: + types: [created] + +permissions: + contents: read + +jobs: + authorize: + name: Authorize fork integration + if: >- + ${{ + github.event.issue.pull_request && + (github.event.comment.body == '/integration-test' || + startsWith(github.event.comment.body, '/integration-test ')) && + contains(fromJSON('["MEMBER", "OWNER"]'), github.event.comment.author_association) + }} + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + pull-requests: write + statuses: write + outputs: + sha: ${{ steps.pr.outputs.sha }} + steps: + - name: Pin the reviewed commit + id: pr + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.issue.number }} + COMMENT_ID: ${{ github.event.comment.id }} + COMMENT_BODY: ${{ github.event.comment.body }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + set -euo pipefail + refuse() { + gh pr comment "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --body "Fork integration not started: $1" + echo "::error::$1" + exit 1 + } + [[ "$COMMENT_BODY" =~ ^/integration-test([[:space:]]+([0-9a-f]{7,40}))?[[:space:]]*$ ]] \ + || refuse "use \`/integration-test\`, or \`/integration-test \` with the head commit you reviewed." + requested="${BASH_REMATCH[2]}" + pr=$(gh api "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER") + sha=$(jq -r '.head.sha' <<<"$pr") + head_repo=$(jq -r '.head.repo.full_name // empty' <<<"$pr") + [[ "$sha" =~ ^[0-9a-f]{40}$ ]] || refuse "could not read the PR head commit." + [ -n "$head_repo" ] || refuse "the fork repository no longer exists." + # Naming the commit binds the run to what was reviewed, not to a later push. + [[ -z "$requested" || "$sha" == "$requested"* ]] \ + || refuse "the PR head is now ${sha:0:7}, not $requested. Review the new commits and comment again." + gh api -X POST "repos/$GITHUB_REPOSITORY/issues/comments/$COMMENT_ID/reactions" \ + -f content=eyes >/dev/null + gh api -X POST "repos/$GITHUB_REPOSITORY/statuses/$sha" -f state=pending \ + -f context="Fork integration" -f description="Running on ${sha:0:7}" \ + -f target_url="$RUN_URL" >/dev/null + echo "sha=$sha" >> "$GITHUB_OUTPUT" + + reject: + name: Reject non-member request + if: >- + ${{ + github.event.issue.pull_request && + (github.event.comment.body == '/integration-test' || + startsWith(github.event.comment.body, '/integration-test ')) && + !contains(fromJSON('["MEMBER", "OWNER"]'), github.event.comment.author_association) + }} + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + pull-requests: write + steps: + - name: Reply with the member-only notice + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.issue.number }} + COMMENTER: ${{ github.event.comment.user.login }} + run: | + gh pr comment "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --body \ + "Sorry @$COMMENTER, only Databricks org members can run integration tests. A maintainer will run \`/integration-test\` after reviewing the changes." + + integration: + name: Integration + needs: authorize + # Job-level so ordinary PR comments, which skip this job, never cancel a run. + concurrency: + group: fork-integration-${{ github.event.issue.number }} + cancel-in-progress: true + permissions: + contents: read + id-token: write + uses: ./.github/workflows/integration.yml + with: + ref: ${{ needs.authorize.outputs.sha }} + secrets: inherit # zizmor: ignore[secrets-inherit] same-repo workflow that needs nearly every secret + + report: + name: Report fork integration + needs: [authorize, integration] + if: ${{ always() && needs.authorize.result == 'success' }} + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + statuses: write + pull-requests: write + steps: + - name: Post the result on the PR + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.issue.number }} + SHA: ${{ needs.authorize.outputs.sha }} + RESULT: ${{ needs.integration.result }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + set -euo pipefail + state=$([ "$RESULT" = success ] && echo success || echo failure) + gh api -X POST "repos/$GITHUB_REPOSITORY/statuses/$SHA" \ + -f state="$state" -f context="Fork integration" \ + -f description="Integration $RESULT on ${SHA:0:7}" -f target_url="$RUN_URL" >/dev/null + gh pr comment "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" \ + --body "Fork integration $RESULT on ${SHA:0:7}: $RUN_URL" diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index f6730bb16..25a5d2004 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -2,6 +2,11 @@ name: Integration on: workflow_call: + inputs: + ref: + description: Commit to test; fork-integration.yml passes the PR head pinned by /integration-test + type: string + default: '' workflow_dispatch: inputs: ug_version: @@ -38,7 +43,7 @@ permissions: contents: read concurrency: - group: integration-${{ github.event.pull_request.number || github.ref }}-${{ github.event_name }} + group: integration-${{ github.event.pull_request.number || github.event.issue.number || github.ref }}-${{ github.event_name }} cancel-in-progress: true env: @@ -61,6 +66,8 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: + ref: ${{ inputs.ref || github.sha }} + persist-credentials: false fetch-depth: 0 persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 @@ -69,6 +76,7 @@ jobs: - uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 with: version: 0.9.8 + enable-cache: ${{ inputs.ref == '' }} - name: Test a fresh installed package without credentials run: | uv run --no-project --python 3.12 python scripts/run_integration.py \ @@ -103,6 +111,8 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: + ref: ${{ inputs.ref || github.sha }} + persist-credentials: false fetch-depth: 0 persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 @@ -111,6 +121,7 @@ jobs: - uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 with: version: 0.9.8 + enable-cache: ${{ inputs.ref == '' }} - name: Authenticate to JFrog with GitHub OIDC id: jfrog uses: jfrog/setup-jfrog-cli@279b1f629f43dd5bc658d8361ac4802a7ef8d2d5 # v4.9.1 @@ -204,6 +215,8 @@ jobs: steps: &live-steps - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: + ref: ${{ inputs.ref || github.sha }} + persist-credentials: false fetch-depth: 0 persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 @@ -212,6 +225,7 @@ jobs: - uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 with: version: 0.9.8 + enable-cache: ${{ inputs.ref == '' }} - uses: databricks/setup-cli@bdb89f81c11a5bd647fd55b585b7c396ec68a25a # v1.0.0 with: version: 1.17.0 @@ -281,6 +295,8 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: + ref: ${{ inputs.ref || github.sha }} + persist-credentials: false fetch-depth: 0 persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 @@ -289,6 +305,7 @@ jobs: - uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 with: version: 0.9.8 + enable-cache: ${{ inputs.ref == '' }} - uses: databricks/setup-cli@bdb89f81c11a5bd647fd55b585b7c396ec68a25a # v1.0.0 with: version: 1.17.0 @@ -481,6 +498,8 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: + ref: ${{ inputs.ref || github.sha }} + persist-credentials: false fetch-depth: 0 persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 @@ -489,6 +508,7 @@ jobs: - uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 with: version: 0.9.8 + enable-cache: ${{ inputs.ref == '' }} - uses: databricks/setup-cli@bdb89f81c11a5bd647fd55b585b7c396ec68a25a # v1.0.0 with: version: 1.17.0 @@ -537,6 +557,7 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: + ref: ${{ inputs.ref || github.sha }} persist-credentials: false - name: Discover every E2E CUJ file id: discover @@ -574,6 +595,7 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: + ref: ${{ inputs.ref || github.sha }} persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 with: @@ -581,6 +603,7 @@ jobs: - uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 with: version: 0.9.8 + enable-cache: ${{ inputs.ref == '' }} - uses: databricks/setup-cli@bdb89f81c11a5bd647fd55b585b7c396ec68a25a # v1.0.0 with: version: 1.17.0 diff --git a/.github/workflows/ug-review.yml b/.github/workflows/ug-review.yml index d23145e10..8a6eca127 100644 --- a/.github/workflows/ug-review.yml +++ b/.github/workflows/ug-review.yml @@ -1,27 +1,28 @@ name: UG Review -# The reviewer always runs trusted code from the default branch. PR-controlled -# metadata and patches are fetched as untrusted text and are never executed. +# An org member comments `/ug-review` on a PR. The reviewer always runs trusted code from +# the default branch. PR-controlled metadata and patches are fetched as untrusted text and +# are never executed. on: - pull_request_target: # zizmor: ignore[dangerous-triggers] - types: [opened, synchronize, reopened, ready_for_review, labeled] + issue_comment: + types: [created] permissions: contents: read -concurrency: - group: ug-review-${{ github.event.pull_request.number }} - cancel-in-progress: true - jobs: review: name: Review with the UG rubric if: >- ${{ - !github.event.pull_request.draft && - contains(github.event.pull_request.labels.*.name, 'ug-review') && - (github.event.action != 'labeled' || github.event.label.name == 'ug-review') + github.event.issue.pull_request && + github.event.comment.body == '/ug-review' && + contains(fromJSON('["MEMBER", "OWNER"]'), github.event.comment.author_association) }} + # Job-level so ordinary PR comments, which skip this job, never cancel a review. + concurrency: + group: ug-review-${{ github.event.issue.number }} + cancel-in-progress: true permissions: contents: read issues: write @@ -46,7 +47,7 @@ jobs: env: GH_TOKEN: ${{ github.token }} REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} + PR_NUMBER: ${{ github.event.issue.number }} DATABRICKS_HOST: https://ai-oss-ecosystem-integration-testing.cloud.databricks.com DATABRICKS_CLIENT_ID: 6476a5e7-7f32-4073-abc5-ee30bb7d0060 DATABRICKS_CLIENT_SECRET: ${{ secrets.UG_CLI_REVIEW_BOT_SP_CLIENT_SECRET }} diff --git a/.github/workflows/user-journey-test-required.yml b/.github/workflows/user-journey-test-required.yml index 7c3243317..e8f5ba72b 100644 --- a/.github/workflows/user-journey-test-required.yml +++ b/.github/workflows/user-journey-test-required.yml @@ -1,27 +1,40 @@ name: User Journey Test Required -# This gate runs from the trusted base branch and reads PR patches through the -# GitHub API. It never checks out or executes code from the PR head, including -# for fork PRs. The Databricks bearer is sent only to the configured workspace. +# This gate runs the default branch's script and reads PR patches through the GitHub +# API; it never executes code from the PR head. It runs automatically for in-repo PRs. +# Fork PRs get no secrets on `pull_request`, so an org member comments +# `/user-journey-check` to run it for them. on: - pull_request_target: # zizmor: ignore[dangerous-triggers] + pull_request: types: [opened, synchronize, reopened, ready_for_review, labeled, unlabeled] + issue_comment: + types: [created] permissions: contents: read -concurrency: - group: user-journey-test-required-${{ github.event.pull_request.number }} - cancel-in-progress: true - jobs: require-user-journey-test: name: User Journey Test Required - if: ${{ !github.event.pull_request.draft }} + if: >- + ${{ + (github.event_name == 'pull_request' && + !github.event.pull_request.draft && + github.event.pull_request.head.repo.full_name == github.repository) || + (github.event_name == 'issue_comment' && + github.event.issue.pull_request && + github.event.comment.body == '/user-journey-check' && + contains(fromJSON('["MEMBER", "OWNER"]'), github.event.comment.author_association)) + }} + # Job-level so ordinary PR comments, which skip this job, never cancel a check. + concurrency: + group: user-journey-test-required-${{ github.event.pull_request.number || github.event.issue.number }} + cancel-in-progress: true permissions: contents: read issues: read - pull-requests: read + pull-requests: write + statuses: write runs-on: group: databricks-protected-runner-group labels: linux-ubuntu-latest @@ -38,13 +51,14 @@ jobs: persist-credentials: false - name: Require meaningful user-journey coverage + id: gate # Temporarily advisory while runner access is blocked by the org IP allowlist. # Keep failures visible in logs without failing the required job check. continue-on-error: true env: GH_TOKEN: ${{ github.token }} REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} + PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }} DATABRICKS_HOST: ${{ secrets.UCODE_TEST_WORKSPACE }} DATABRICKS_BEARER: ${{ secrets.DATABRICKS_BEARER }} USER_JOURNEY_JUDGE_MODEL: ${{ vars.UG_CI_USER_JOURNEY_JUDGE_MODEL }} @@ -53,3 +67,22 @@ jobs: run: | : "${USER_JOURNEY_JUDGE_MODEL:?Set the UG_CI_USER_JOURNEY_JUDGE_MODEL repository variable to a Responses-capable system.ai model}" python3 .github/scripts/user-journey-required/check.py + + # Comment-triggered runs are not attached to the PR, so report back on its head commit. + - name: Report the result on the fork PR + if: ${{ github.event_name == 'issue_comment' }} + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.issue.number }} + OUTCOME: ${{ steps.gate.outcome }} + 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) + [[ "$sha" =~ ^[0-9a-f]{40}$ ]] || { echo "::error::could not read the PR head commit"; exit 1; } + state=$([ "$OUTCOME" = success ] && echo success || echo failure) + gh api -X POST "repos/$GITHUB_REPOSITORY/statuses/$sha" -f state="$state" \ + -f context="User Journey Test Required" -f description="User-journey check $OUTCOME" \ + -f target_url="$RUN_URL" >/dev/null + gh pr comment "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" \ + --body "User-journey check $OUTCOME on ${sha:0:7}: $RUN_URL" diff --git a/tests/README.md b/tests/README.md index 9e9a0667b..2b3bccb2f 100644 --- a/tests/README.md +++ b/tests/README.md @@ -411,6 +411,15 @@ or skipped dependency fails the gate, and a running integration suite keeps it pending. The advisory Windows installation and headless lanes are not part of `All integration tests` yet. The descriptive jobs provide the actual coverage and diagnostics. +Fork PRs get no secrets, so their `pull_request` run skips every credentialed integration +job. After reviewing a PR, an org member comments `/integration-test` to run the Integration +workflow with secrets on the PR's current head commit, or `/integration-test ` to run +only if the head is still the commit they reviewed. It posts a `Fork integration` status, +and works on in-repo PRs too as an on-demand rerun. Each new push needs a new command. +It runs main's workflow definitions and never caches fork builds. `/ug-review` requests +the UG rubric review the same way, and `/user-journey-check` runs the user-journey gate, +which runs automatically only for in-repo PRs. + ## Gaps and deferred scope Custom OAuth lock tests cover release after use, Windows contention retries, and diff --git a/tests/test_integration_contract.py b/tests/test_integration_contract.py index e4bb63ee0..69e410a80 100644 --- a/tests/test_integration_contract.py +++ b/tests/test_integration_contract.py @@ -174,6 +174,147 @@ def test_integration_jobs_that_run_on_fork_pull_requests_need_no_credentials(): ) +MEMBER_ONLY = 'contains(fromJSON(\'["MEMBER", "OWNER"]\'), github.event.comment.author_association)' + + +def _workflow(name): + return (Path(__file__).parent.parent / ".github/workflows" / name).read_text() + + +def test_fork_integration_runs_only_member_requested_pr_heads(): + workflow = _workflow("fork-integration.yml") + authorize = workflow.split("\n authorize:\n", 1)[1].split("\n reject:\n", 1)[0] + reject = workflow.split("\n reject:\n", 1)[1].split("\n integration:\n", 1)[0] + integration = workflow.split("\n integration:\n", 1)[1].split("\n report:\n", 1)[0] + report = workflow.split("\n report:\n", 1)[1] + + assert re.search(r"(?m)^on:\n issue_comment:\n types: \[created\]\n", workflow) + assert "pull_request_target" not in workflow + assert "actions/checkout" not in workflow, "Only integration.yml checks out the pinned commit" + command = "startsWith(github.event.comment.body, '/integration-test ')" + assert command in authorize and command in reject + assert MEMBER_ONLY in authorize + assert f"!{MEMBER_ONLY}" in reject + assert "only Databricks org members can run integration tests" in reject + assert "set -euo pipefail" in authorize + assert '[[ "$sha" =~ ^[0-9a-f]{40}$ ]]' in authorize + assert "state=pending" in authorize + assert "needs: authorize" in integration + assert "group: fork-integration-${{ github.event.issue.number }}" in integration + assert "uses: ./.github/workflows/integration.yml" in integration + assert "ref: ${{ needs.authorize.outputs.sha }}" in integration + assert "needs.authorize.result == 'success'" in report + assert "repos/$GITHUB_REPOSITORY/statuses/$SHA" in report + assert "concurrency:" not in workflow.split("\njobs:\n", 1)[0], ( + "Workflow-level concurrency would let any PR comment cancel a run" + ) + + +HEAD = "0123456789abcdef0123456789abcdef01234567" + + +@pytest.mark.parametrize( + ("comment", "head_repo", "accepted", "reason"), + [ + ("/integration-test", "fork/unity-gateway", True, ""), + (f"/integration-test {HEAD[:7]}", "fork/unity-gateway", True, ""), + (f"/integration-test {HEAD}", "fork/unity-gateway", True, ""), + ("/integration-test", "databricks/unity-gateway", True, ""), + ("/integration-test 1234567", "fork/unity-gateway", False, "the PR head is now 0123456"), + ("/integration-test please", "fork/unity-gateway", False, "use `/integration-test`"), + ("/integration-test", None, False, "fork repository no longer exists"), + ], +) +def test_fork_integration_authorize_pins_the_pr_head_or_the_reviewed_commit( + tmp_path, comment, head_repo, accepted, reason +): + workflow = _workflow("fork-integration.yml") + authorize = workflow.split("\n authorize:\n", 1)[1].split("\n reject:\n", 1)[0] + script = textwrap.dedent(authorize.split(" run: |\n", 1)[1]) + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + pr = {"head": {"sha": HEAD, "repo": {"full_name": head_repo} if head_repo else None}} + (tmp_path / "pr.json").write_text(json.dumps(pr)) + fake_gh = bin_dir / "gh" + fake_gh.write_text( + "#!/bin/sh\n" + f'echo "$*" >> "{tmp_path}/calls"\n' + f'case "$*" in "api repos/databricks/unity-gateway/pulls/7") cat "{tmp_path}/pr.json";; esac\n' + ) + fake_gh.chmod(0o755) + output = tmp_path / "github-output" + result = subprocess.run( + ["bash", "-c", script], + env={ + "PATH": f"{bin_dir}:{os.environ['PATH']}", + "GITHUB_REPOSITORY": "databricks/unity-gateway", + "GITHUB_OUTPUT": str(output), + "PR_NUMBER": "7", + "COMMENT_ID": "99", + "COMMENT_BODY": comment, + "RUN_URL": "https://example/run", + }, + capture_output=True, + text=True, + timeout=10, + ) + calls = (tmp_path / "calls").read_text() + + if accepted: + assert result.returncode == 0, result.stdout + result.stderr + assert output.read_text() == f"sha={HEAD}\n" + assert f"statuses/{HEAD} -f state=pending" in calls + else: + assert result.returncode != 0 + assert not output.exists() + assert reason in calls and "statuses/" not in calls + + +def test_no_workflow_uses_pull_request_target(): + # GitHub disables pull_request_target by default in public repos from 2026-11-02. + workflows = sorted((Path(__file__).parent.parent / ".github/workflows").glob("*.yml")) + + assert workflows + assert [path.name for path in workflows if "pull_request_target" in path.read_text()] == [] + + +def test_user_journey_gate_runs_for_in_repo_prs_or_a_member_command(): + workflow = _workflow("user-journey-test-required.yml") + trigger = workflow.split("\non:\n", 1)[1].split("\npermissions:\n", 1)[0] + + assert " pull_request:\n" in trigger and " issue_comment:\n types: [created]" in trigger + assert "github.event.pull_request.head.repo.full_name == github.repository" in workflow + assert "github.event.comment.body == '/user-journey-check'" in workflow + assert MEMBER_ONLY in workflow + assert "ref: ${{ github.event.repository.default_branch }}" in workflow + assert "concurrency:" not in workflow.split("\njobs:\n", 1)[0] + + +def test_ug_review_runs_only_on_member_command_from_the_default_branch(): + workflow = _workflow("ug-review.yml") + + assert re.search(r"(?m)^on:\n issue_comment:\n types: \[created\]\n", workflow) + assert "pull_request_target" not in workflow + assert "github.event.comment.body == '/ug-review'" in workflow + assert MEMBER_ONLY in workflow + assert "ref: ${{ github.event.repository.default_branch }}" in workflow + assert "PR_NUMBER: ${{ github.event.issue.number }}" in workflow + assert "concurrency:" not in workflow.split("\njobs:\n", 1)[0] + + +def test_integration_checkouts_and_caches_are_safe_for_fork_heads(): + workflow = (Path(__file__).parent.parent / ".github/workflows/integration.yml").read_text() + checkouts = re.findall(r"(?m)^ - uses: actions/checkout@.*\n((?: .*\n)*)", workflow) + caches = re.findall(r"(?m)^ - uses: astral-sh/setup-uv@.*\n((?: .*\n)*)", workflow) + + assert checkouts and caches + for block in checkouts: + assert "ref: ${{ inputs.ref || github.sha }}" in block, block + assert "persist-credentials: false" in block, block + for block in caches: + assert "enable-cache: ${{ inputs.ref == '' }}" in block, block + + def test_windows_integration_ci_uses_shared_claude_version(): workflow = Path(__file__).parent.parent / ".github/workflows/integration.yml" contents = workflow.read_text() diff --git a/tests/test_ug_review_bot.py b/tests/test_ug_review_bot.py index 4fa23c4dc..b7f1f7f33 100644 --- a/tests/test_ug_review_bot.py +++ b/tests/test_ug_review_bot.py @@ -1,4 +1,4 @@ -"""Focused tests for the label-triggered UG review bot.""" +"""Focused tests for the comment-triggered UG review bot.""" from __future__ import annotations From a2c4119c7ec709766eca9bd3ea55d85ee0982ee7 Mon Sep 17 00:00:00 2001 From: David Liu Date: Fri, 9 Oct 2026 18:21:55 +0000 Subject: [PATCH 2/4] Pin the managed OpenCode checkout to the reviewed ref 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 --- .github/workflows/integration.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 25a5d2004..818cb30a0 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -441,6 +441,8 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: + ref: ${{ inputs.ref || github.sha }} + persist-credentials: false fetch-depth: 0 persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 @@ -449,6 +451,7 @@ jobs: - uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 with: version: 0.9.8 + enable-cache: ${{ inputs.ref == '' }} - uses: databricks/setup-cli@bdb89f81c11a5bd647fd55b585b7c396ec68a25a # v1.0.0 with: version: 1.17.0 From 3c1751283611bf7616dc87b4379da3963a8362d6 Mon Sep 17 00:00:00 2001 From: David Liu Date: Fri, 9 Oct 2026 18:51:27 +0000 Subject: [PATCH 3/4] Skip the fork authorize script test on Windows 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 --- tests/test_integration_contract.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_integration_contract.py b/tests/test_integration_contract.py index 69e410a80..9345ce721 100644 --- a/tests/test_integration_contract.py +++ b/tests/test_integration_contract.py @@ -213,6 +213,7 @@ def test_fork_integration_runs_only_member_requested_pr_heads(): HEAD = "0123456789abcdef0123456789abcdef01234567" +@pytest.mark.skipif(sys.platform == "win32", reason="the authorize step runs bash on Linux") @pytest.mark.parametrize( ("comment", "head_repo", "accepted", "reason"), [ From eb7ce7cedc1666a9d73206de4d6eaab8a70f7e6d Mon Sep 17 00:00:00 2001 From: David Liu Date: Sat, 10 Oct 2026 13:58:24 +0000 Subject: [PATCH 4/4] Pin the fork user-journey status to the evaluated commit The fork report step refetched the PR head after the judge ran, so a push during the check got the previous revision's verdict. The gate now verifies the head is unchanged across its diff reads, outputs that SHA, and the report posts only against it. Also document in AGENTS.md that workflows must not use pull_request_target. Co-authored-by: Isaac --- .../scripts/user-journey-required/check.py | 17 ++++++ .github/workflows/integration.yml | 6 --- .../workflows/user-journey-test-required.yml | 11 ++-- AGENTS.md | 1 + tests/test_integration_contract.py | 10 ++++ tests/test_user_journey_gate.py | 53 +++++++++++++++++++ 6 files changed, 89 insertions(+), 9 deletions(-) diff --git a/.github/scripts/user-journey-required/check.py b/.github/scripts/user-journey-required/check.py index 34c7aadae..9cf71d06b 100644 --- a/.github/scripts/user-journey-required/check.py +++ b/.github/scripts/user-journey-required/check.py @@ -297,6 +297,22 @@ def _sync_waiver_label() -> int: return _pass("waiver label already matches the authorized comment state.") +def _record_evaluated_head(pr_path: str, pr: dict[str, Any]) -> None: + """Pin the verdict to the head the diff was read at, for the fork-PR status report.""" + sha = str(pr.get("head", {}).get("sha", "")) + current = _gh_json(pr_path) + current_sha = str(current.get("head", {}).get("sha", "")) if isinstance(current, dict) else "" + if not re.fullmatch(r"[0-9a-f]{40}", sha) or current_sha != sha: + raise GateError( + f"The PR head changed while its diff was read ({sha[:7] or 'unknown'} -> " + f"{current_sha[:7] or 'unknown'}). Re-run this check." + ) + output = os.environ.get("GITHUB_OUTPUT") + if output: + with open(output, "a") as handle: + handle.write(f"sha={sha}\n") + + def _pass(message: str) -> int: print(f"PASS: {message}") return 0 @@ -316,6 +332,7 @@ def main() -> int: files = _gh_json(f"{pr_path}/files", paginate=True) if not isinstance(pr, dict) or not isinstance(files, list): raise GateError("GitHub returned an unexpected pull-request response.") + _record_evaluated_head(pr_path, pr) if not any(_is_product_path(str(file.get("filename", ""))) for file in files): return _pass("no ug product or packaging files changed; no journey test is required.") diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 818cb30a0..df7c308ae 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -69,7 +69,6 @@ jobs: ref: ${{ inputs.ref || github.sha }} persist-credentials: false fetch-depth: 0 - persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 with: node-version: 22.19.0 @@ -114,7 +113,6 @@ jobs: ref: ${{ inputs.ref || github.sha }} persist-credentials: false fetch-depth: 0 - persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 with: node-version: 22.19.0 @@ -218,7 +216,6 @@ jobs: ref: ${{ inputs.ref || github.sha }} persist-credentials: false fetch-depth: 0 - persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 with: node-version: 22.19.0 @@ -298,7 +295,6 @@ jobs: ref: ${{ inputs.ref || github.sha }} persist-credentials: false fetch-depth: 0 - persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 with: node-version: 22.19.0 @@ -444,7 +440,6 @@ jobs: ref: ${{ inputs.ref || github.sha }} persist-credentials: false fetch-depth: 0 - persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 with: node-version: 22.19.0 @@ -504,7 +499,6 @@ jobs: ref: ${{ inputs.ref || github.sha }} persist-credentials: false fetch-depth: 0 - persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 with: node-version: 22.19.0 diff --git a/.github/workflows/user-journey-test-required.yml b/.github/workflows/user-journey-test-required.yml index e8f5ba72b..8979618d0 100644 --- a/.github/workflows/user-journey-test-required.yml +++ b/.github/workflows/user-journey-test-required.yml @@ -68,18 +68,23 @@ jobs: : "${USER_JOURNEY_JUDGE_MODEL:?Set the UG_CI_USER_JOURNEY_JUDGE_MODEL repository variable to a Responses-capable system.ai model}" python3 .github/scripts/user-journey-required/check.py - # Comment-triggered runs are not attached to the PR, so report back on its head commit. + # Comment-triggered runs are not attached to the PR, so report back on the evaluated commit. - name: Report the result on the fork PR if: ${{ github.event_name == 'issue_comment' }} env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.issue.number }} OUTCOME: ${{ steps.gate.outcome }} + SHA: ${{ steps.gate.outputs.sha }} 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) - [[ "$sha" =~ ^[0-9a-f]{40}$ ]] || { echo "::error::could not read the PR head commit"; exit 1; } + sha=$SHA + if ! [[ "$sha" =~ ^[0-9a-f]{40}$ ]]; then + gh pr comment "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" \ + --body "User-journey check could not pin the evaluated commit; comment \`/user-journey-check\` to retry: $RUN_URL" + exit 1 + fi state=$([ "$OUTCOME" = success ] && echo success || echo failure) gh api -X POST "repos/$GITHUB_REPOSITORY/statuses/$sha" -f state="$state" \ -f context="User Journey Test Required" -f description="User-journey check $OUTCOME" \ diff --git a/AGENTS.md b/AGENTS.md index b4223bacd..369d5a7da 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,6 +27,7 @@ Tests live in `tests/`. - Use `acquire_exclusive_file_lock` / `release_file_lock` from `ucode.os_compatibility.file_lock_cross_os` instead of calling `fcntl.flock`, `fcntl.lockf`, or `msvcrt.locking` directly. Ruff enforces this outside the compatibility module and tests. - Use `launcher.exec_or_spawn` when handing the terminal to an agent. Shell strings and explicit executable overrides keep their existing subprocess semantics. - Add or update focused tests for behavior changes. +- Never trigger a workflow with `pull_request_target`; `tests/test_integration_contract.py` rejects it. Use `pull_request` for checks that need no secrets, and a member-only `issue_comment` command that checks out the default branch (as in `fork-integration.yml`) for checks that need secrets on fork PRs. - Do not modify generated or lock files unless the dependency graph intentionally changes. ## Smart-routing configuration diff --git a/tests/test_integration_contract.py b/tests/test_integration_contract.py index 9345ce721..f87da8849 100644 --- a/tests/test_integration_contract.py +++ b/tests/test_integration_contract.py @@ -291,6 +291,16 @@ def test_user_journey_gate_runs_for_in_repo_prs_or_a_member_command(): assert "concurrency:" not in workflow.split("\njobs:\n", 1)[0] +def test_user_journey_fork_report_uses_the_evaluated_commit(): + report = _workflow("user-journey-test-required.yml").split( + "Report the result on the fork PR", 1 + )[1] + + assert "SHA: ${{ steps.gate.outputs.sha }}" in report + assert "repos/$GITHUB_REPOSITORY/statuses/$sha" in report + assert "/pulls/" not in report and ".head.sha" not in report + + def test_ug_review_runs_only_on_member_command_from_the_default_branch(): workflow = _workflow("ug-review.yml") diff --git a/tests/test_user_journey_gate.py b/tests/test_user_journey_gate.py index 6f2a6769d..4609a55c3 100644 --- a/tests/test_user_journey_gate.py +++ b/tests/test_user_journey_gate.py @@ -158,3 +158,56 @@ def test_waiver_command_must_match_exactly(): ] assert gate._waiver_status(comments, {"rohita5l", "lilly-luo"}) == (False, "") + + +EVALUATED = "a" * 40 + + +def _gate_with_heads(monkeypatch, tmp_path, *heads, filename="README.md"): + output = tmp_path / "github-output" + output.touch() + monkeypatch.setenv("REPO", "databricks/unity-gateway") + monkeypatch.setenv("PR_NUMBER", "1") + monkeypatch.setenv("GITHUB_OUTPUT", str(output)) + pulls = iter({"title": "Change", "head": {"sha": sha}} for sha in heads) + files = [{"filename": filename, "status": "modified", "patch": "+change"}] + + def gh_json(path, paginate=False): + if path.endswith("/files"): + return files + if path.endswith("/comments"): + return [] + return next(pulls) + + monkeypatch.setattr(gate, "_gh_json", gh_json) + return gate.main(), output.read_text() + + +def test_gate_records_the_head_its_diff_was_read_at(monkeypatch, tmp_path): + assert _gate_with_heads(monkeypatch, tmp_path, EVALUATED, EVALUATED) == ( + 0, + f"sha={EVALUATED}\n", + ) + + +def test_gate_fails_without_a_verdict_sha_when_the_head_moves(monkeypatch, tmp_path, capsys): + assert _gate_with_heads(monkeypatch, tmp_path, EVALUATED, "b" * 40) == (1, "") + assert ( + "The PR head changed while its diff was read (aaaaaaa -> bbbbbbb)" + in capsys.readouterr().out + ) + + +def test_gate_records_the_head_when_the_verdict_fails(monkeypatch, tmp_path): + policy = tmp_path / "AGENTS.md" + policy.write_text("policy") + monkeypatch.setenv("TEST_POLICY_PATH", str(policy)) + monkeypatch.setenv("USER_JOURNEY_WAIVER_ADMINS", "rohita5l") + monkeypatch.setattr(gate, "_judge", lambda title, diff, test_policy: (True, "new launch flow")) + + assert _gate_with_heads( + monkeypatch, tmp_path, EVALUATED, EVALUATED, filename="src/ucode/cli.py" + ) == ( + 1, + f"sha={EVALUATED}\n", + )