ci(pr): run E2E when a PR touches tests/e2e paths (advisory) (#11131)

* ci(pr): run E2E when a PR touches tests/e2e paths

Regression specs under tests/e2e never ran on PR CI — only schedule and
release called e2e.yml — so a red regression test could merge green.
Path-filter and workflow_call the E2E suite when E2E-relevant files change.

Use merge-base diffs so base-branch drift does not false-trigger E2E, fail
the detector when git diff cannot compute the PR range, and pin
least-privilege contents:read on both the detector and reusable E2E workflow.

Closes #10518

Co-authored-by: Wooseong Kim <innocarpe@gmail.com>

Co-authored-by: Orca <help@stably.ai>

* ci(pr): make the E2E path gate actually block, and match the real config path

Two fixes to the new path-filtered E2E job.

The gate did not gate. pr.yml's `verify` job is the required check, and it
enumerates its dependencies explicitly — `e2e` was in neither `needs` nor the
result list, so a failing shard left `verify` green. That reproduces the exact
hole this job exists to close: a red spec merges green, just with a red box
further down the page. Add `e2e` to both.

Because the job is path-filtered, `skipped` is the normal result on a PR that
touches no E2E files and has to keep passing. That allowance is checked after
the strict loop rather than inside it, so it can never leak to the six jobs
that are always required.

The `playwright.` pattern matched nothing. The config is
tests/playwright.config.ts — beside tests/e2e/, not inside it — so no tracked
file starts with `playwright.` and editing the runner config would silently
skip E2E. Anchor it at `tests/playwright.`.

Adds a contract test alongside the existing release-e2e one. Verified it fails
when either fix is reverted, and simulated the gate across
success/skipped/failure/cancelled plus the skip-must-not-mask-a-real-failure
case.

* test(ci): close two gaps in the E2E gate contract

CodeRabbit was right on both counts — verified by reverting each and watching
the contract stay green.

The path filter was unasserted, so `e2e` could lose its `if:` and run on every
PR — the cost the filter exists to avoid — without failing anything.

The strict-loop check hardcoded four of the six required jobs, so dropping
GIT_COMPATIBILITY or SHELL_CONTRACTS left them unenforced while the contract
passed. Derive the list from verify.needs instead, so a newly added required
job that misses the loop fails here rather than silently going unchecked.

* ci(pr): land the E2E path gate advisory instead of blocking

The E2E suite is currently failing every scheduled run on main — 22 of the last
22 — so making verify depend on it would block any PR touching tests/e2e/**,
including the PRs that fix the suite. This PR's own run reproduced that: 3 of 12
shards failed on specs unrelated to it (agent-session resume, Jira linking,
plugin containment, terminal artifacts).

So the job runs and reports on E2E-path PRs but is left out of verify.needs for
now. The detector, the tests/playwright. path fix, and the contract tests are
unaffected — those stand on their own and were the substance of the review.

Flipping to blocking is a three-line change once the suite is green; the exact
wiring, including why the skipped allowance must sit outside the strict loop, is
recorded on verify's Require-successful-checks step. The contract test pins the
advisory choice so it reads as deliberate rather than as the unwired-gate bug it
originally caught, and still fails if the path filter, the strict-loop coverage,
or the config path regress.

---------

Co-authored-by: Wooseong Kim <innocarpe@gmail.com>
Co-authored-by: Orca <help@stably.ai>

.github/workflows/e2e.yml

@@ -2,6 +2,10 @@ name: E2E
 
 run-name: E2E ${{ inputs.ref || github.ref }}
 
+# Why: checkout + artifact upload only; callers can only further restrict.
+permissions:
+  contents: read
+
 on:
   workflow_call:
     inputs:

.github/workflows/pr.yml

@@ -265,6 +265,55 @@ jobs:
       - name: Smoke packaged CLI
         run: node config/scripts/smoke-packaged-cli.mjs --app-dir=dist/linux-unpacked
 
+  # Why: regression specs under tests/e2e/** used to merge green without ever
+  # running — e2e.yml only fired on schedule/release (#10518). Path-filter so
+  # ordinary PRs stay light; any E2E suite change still gets a full shard run.
+  e2e-paths:
+    name: detect e2e path changes
+    runs-on: ubuntu-latest
+    if: github.event.pull_request.draft != true
+    # Why: detector only needs to read the checkout; do not inherit repo defaults.
+    permissions:
+      contents: read
+    outputs:
+      should_run: ${{ steps.filter.outputs.should_run }}
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v6
+        with:
+          fetch-depth: 0
+          persist-credentials: false
+
+      - name: Filter E2E-relevant paths
+        id: filter
+        run: |
+          set -euo pipefail
+          BASE="${{ github.event.pull_request.base.sha }}"
+          HEAD="${{ github.event.pull_request.head.sha }}"
+          # Why: capture first so a failed git diff does not look like "no matches"
+          # (pipeline status in `if` is not aborted by set -e). Merge-base limits the
+          # list to files this PR introduced, not base-branch drift.
+          CHANGED="$(git diff --name-only --merge-base "$BASE" "$HEAD")"
+          # Why: tests/playwright.config.ts sits beside tests/e2e/, not inside it, so
+          # it needs its own pattern — a bare `playwright.` prefix matches no tracked
+          # file and would silently skip E2E when the runner config changes.
+          if printf '%s\n' "$CHANGED" | grep -E '^(tests/e2e/|tests/playwright\.|\.github/workflows/e2e\.yml$)' >/dev/null; then
+            echo "should_run=true" >> "$GITHUB_OUTPUT"
+            echo "E2E path changes detected"
+          else
+            echo "should_run=false" >> "$GITHUB_OUTPUT"
+            echo "No E2E path changes"
+          fi
+
+  e2e:
+    name: e2e
+    needs: e2e-paths
+    if: needs.e2e-paths.outputs.should_run == 'true'
+    # Why: reusable e2e.yml only checkouts, builds, and uploads artifacts.
+    permissions:
+      contents: read
+    uses: ./.github/workflows/e2e.yml
+
   verify:
     if: always()
     needs:
@@ -277,6 +326,14 @@ jobs:
     runs-on: ubuntu-latest
 
     steps:
+      # Why: e2e is deliberately absent from needs. The suite is currently red on
+      # main (every scheduled run), so gating merges on it would block any PR that
+      # touches tests/e2e/** — including the ones fixing the suite. Until it is
+      # green the job runs and reports for E2E-path PRs without blocking. To flip
+      # it on: add `e2e` to needs, add E2E to the env below, and require
+      # `"$E2E" = success || skipped` after the loop — skipped is the normal
+      # result for a path-filtered job and must keep passing, so it has to be
+      # checked outside the loop or it would excuse the jobs above.
       - name: Require successful checks
         env:
           STATIC_ANALYSIS: ${{ needs.static_analysis.result }}

config/scripts/pr-e2e-gate-contract.test.mjs

@@ -0,0 +1,64 @@
+import { readFileSync } from 'node:fs'
+import { join, resolve } from 'node:path'
+import { describe, expect, it } from 'vitest'
+import { parse } from 'yaml'
+
+const projectDir = resolve(import.meta.dirname, '../..')
+const prWorkflow = parse(readFileSync(join(projectDir, '.github/workflows/pr.yml'), 'utf8'))
+
+const filterStep = prWorkflow.jobs['e2e-paths'].steps.find(
+  (step) => step.name === 'Filter E2E-relevant paths'
+)
+const verifyStep = prWorkflow.jobs.verify.steps.find(
+  (step) => step.name === 'Require successful checks'
+)
+
+describe('PR E2E gate contract', () => {
+  it('keeps E2E advisory while the suite is red on main', () => {
+    // Why: pin the deliberate choice so it reads as intentional rather than as
+    // the "forgot to wire the gate" bug this file originally caught. Gating on a
+    // suite that fails every scheduled run would block the PRs that fix it.
+    // Flipping to blocking means updating this expectation too — see the comment
+    // on verify's Require-successful-checks step for the exact wiring.
+    expect(prWorkflow.jobs.verify.needs).not.toContain('e2e')
+    expect(verifyStep.env.E2E).toBeUndefined()
+    expect(verifyStep.run).not.toContain('$E2E')
+  })
+
+  it('runs E2E only when the detector says the PR touches E2E paths', () => {
+    // Why: without this the job could lose its filter and run on every PR — the
+    // cost the path filter exists to avoid — while the gate assertions above
+    // stay green.
+    expect(prWorkflow.jobs.e2e.needs).toBe('e2e-paths')
+    expect(prWorkflow.jobs.e2e.if).toBe("needs.e2e-paths.outputs.should_run == 'true'")
+    expect(prWorkflow.jobs['e2e-paths'].outputs.should_run).toBe(
+      '${{ steps.filter.outputs.should_run }}'
+    )
+  })
+
+  it('enforces every job verify depends on', () => {
+    // Why: derive from verify.needs rather than hardcoding, so adding a required
+    // job without adding it to the strict loop fails here instead of silently
+    // leaving that job unenforced. This is what caught GIT_COMPATIBILITY and
+    // SHELL_CONTRACTS being absent from an earlier hardcoded list.
+    const strictLoop = verifyStep.run.slice(0, verifyStep.run.indexOf('done'))
+    for (const job of prWorkflow.jobs.verify.needs) {
+      const envVar = job.toUpperCase()
+      expect(verifyStep.env[envVar]).toBe(`\${{ needs.${job}.result }}`)
+      expect(strictLoop).toContain(`"$${envVar}"`)
+    }
+  })
+
+  it('matches the Playwright config where it actually lives', () => {
+    // Why: the config is tests/playwright.config.ts, beside tests/e2e/ rather
+    // than inside it. A bare `playwright.` prefix matches no tracked file, so
+    // editing the runner config would silently skip E2E.
+    expect(filterStep.run).toContain('tests/playwright\\.')
+    expect(filterStep.run).not.toMatch(/\(\^?\|\|]tests\/e2e\/\|playwright\\\./)
+  })
+
+  it('scopes detection to the PR range so base drift cannot false-trigger', () => {
+    expect(filterStep.run).toContain('git diff --name-only --merge-base "$BASE" "$HEAD"')
+    expect(filterStep.run).toContain('set -euo pipefail')
+  })
+})