Repository navigation
Conversation
`e2e.yml` is reachable only from `schedule` and `release-cut`'s `workflow_call`, so a PR that adds an E2E regression test never runs it. The test runs for the first time on `main`, where nothing gates on it — twice now a spec has landed and never passed. Adds a `pull_request` job that runs exactly the specs the PR touched. A PR that changes no spec pays nothing: the file list is resolved first, and every setup/build/run step is gated on the result being non-empty. - No `paths:` filter. The check reports on every PR, so it can be made required without wedging the merge box on PRs that touch no spec. - Changed files come from the pulls API (paginated), not a git diff — the PR checkout is shallow and a deleted spec must not reach Playwright. - Selected paths are argv for a shell-invoked command and a fork PR names its own files, so `select-changed-e2e-specs.mjs` allowlists `tests/e2e/**/*.spec.ts` with no shell metacharacters and no traversal, and the run step expands them from an array rather than the command string. - `--pass-with-no-tests` keeps a spec whose tests are all `@headful` from failing the gate, since the `electron-headless` project filters them out. Gating the full sharded suite on every PR stays a separate decision: it is 22 minutes of wall clock, and `main` is red today, so making it required would block every merge until stablyai#10519 closes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow that queries changed pull request files, selects matching E2E specs, conditionally builds and runs them, and uploads failure traces. Adds a selector CLI that validates, filters, deduplicates, and sorts spec paths from stdin. Adds Vitest coverage for selector behavior and workflow wiring, including triggers, API pagination, conditional steps, test command options, and artifact configuration. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f21bff3-18e8-435c-b517-4314021ba982
📒 Files selected for processing (3)
.github/workflows/e2e-changed-specs.ymlconfig/scripts/e2e-changed-specs-workflow.test.mjsconfig/scripts/select-changed-e2e-specs.mjs
… selector - Add node-gyp to devDependencies so the changed-specs workflow uses the pnpm-lock version instead of npm install -g. - Remove .trim() on PR-controlled filenames before allowlist matching and add tests for leading/trailing whitespace rejection.
|
Closing this as superseded by #11131, which merged the upstream path-filtered PR E2E workflow and closed #10518. The changed-spec selector approach may still be useful as a future optimization, but it no longer needs to remain as a parallel implementation PR; it can be revived as a focused follow-up if maintainers want that direction. |
2 participants
Summary
e2e.ymlis reachable only fromscheduleandrelease-cut'sworkflow_call, so a PR that adds an E2E regression test never runs that test — it runs for the first time onmain, where nothing gates on it. Twice now a spec has landed and never passed (#10518).This adds a
pull_requestjob that runs exactly the specs the PR touched. A PR that changes no spec pays nothing: the file list is resolved first, and every setup/build/run step is gated on the result being non-empty.Gating the full sharded suite on every PR stays a separate decision, deliberately not made here: it is 22 minutes of wall clock (build + 10 shards + the docker job), and
mainis red today, so making it required would block every merge until #10519 closes.Screenshots
No visual change.
Proof of fix
The upstream PR changes no spec, so its own
e2e changed specscheck can only demonstrate the skip path. The run path is demonstrated on a throwaway fork PR whose head is this commit plus one comment-only line intests/e2e/markdown-explorer-find-focus.spec.ts:Run path — MumuTW/orca run 30242976474, 2m53s total:
It selected exactly the touched file — not the whole suite, not the shard containing it.
Skip path — MumuTW/orca run 30243157972, same commit with no spec change, 16 seconds end to end:
That is what the vast majority of PRs will pay. This PR's own check on
stablyai/orcaisaction_requiredpending maintainer approval, which is why the evidence above is from the fork.Design notes
paths:filter. The check reports on every PR, so it can be made a required check without wedging the merge box on PRs that touch no spec. Apaths:-filtered workflow simply never reports, and a required check that never reports blocks the merge forever. Selection happens inside the job instead.gh api --paginate .../files), not a git diff: the PR checkout is shallow,--paginateavoids the 100-file cap ofgh pr view --json files, andselect(.status != "removed")keeps a spec the PR deleted from reaching Playwright.--pass-with-no-tests— a spec whose tests are all@headfulis filtered out by theelectron-headlessproject, and "no tests ran" must not fail the gate.xvfb-run --auto-servernum env SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 pnpm run test:e2e, reusing the prebuilt output, so a spec cannot pass here and fail on the nightly for harness reasons. A unit test asserts these tokens againste2e.ymlitself, so the two cannot drift silently.ORCA_E2E_SSH_DOCKERetc.), so selecting one is a cheap no-op rather than a false failure.Testing
pnpm lint(oxlint, changed files)pnpm typecheck— n/a, no TS product code changed; the added script is.mjsand its test runs under vitestpnpm test— ran theconfig/scriptssuite: 54 files, 433 passed, 4 skipped, including the 11 new casespnpm build— not run; no application code changedconfig/scripts/e2e-changed-specs-workflow.test.mjscovers both halves:tests/e2e/**/*.spec.ts, walks nested dirs, dedupes and sorts, emits nothing when no spec changed, and drops paths a fork PR could weaponize (; rm -rf /,$(id), backticks, spaces,--shard=1/2,..traversal, absolute paths).paths:filter, the exacttypes:list, the API-based selection command,permissions: {contents: read, pull-requests: read}, every work step gated on a non-empty selection, the trace upload gated onfailure(), and the run invocation matchinge2e.yml's own shard command token for token.Also verified the command shape locally against the real Playwright config, since "the args are forwarded correctly" is the load-bearing claim:
AI Review Report
Reviewed for: required-check semantics, shell injection through PR-controlled filenames, cost on the common path, and drift between this gate and the scheduled suite.
Flagged and changed as a result:
paths:filter would break a future required check — a filtered workflow never reports on PRs that touch no spec, so the merge box waits forever. Moved selection inside the job and pinned that invariant with a test.mapfileis bash 4+ — replaced with awhile IFS= read -rloop so the snippet is not silently tied to the runner's bash version, and the whole run snippet was then executed locally under bash 3.2.failure()instead; the test now exempts it explicitly rather than loosening the rule.Cross-platform: the job is
ubuntu-latest-only by design (it mirrorse2e.yml, which is Linux-only), and therunner.os == 'Linux'guard on the node-gyp pin matchese2e.yml/pr.ymlso the "this workaround is Linux-only" invariant stays consistent if a matrix is ever added. The selector script is pure Node with POSIX path handling and no shell. No shortcuts, labels, or Electron platform behavior touched.Security Audit
The one real surface is command execution with attacker-controlled input: the selected paths become argv for a shell-invoked
pnpm run test:e2e, and a fork PR chooses its own filenames.^tests/e2e/[A-Za-z0-9._-]+(/[A-Za-z0-9._-]+)*\.spec\.ts$) plus an explicit..rejection — everything else is dropped, not escaped. Six weaponized-path cases are unit-tested."${SPECS[@]}"read from a file, never interpolated into the command string.permissions:is narrowed tocontents: read+pull-requests: read; the checkout usespersist-credentials: false.pull_request, notpull_request_target, so fork code never runs with a privileged token or repo secrets.Follow-up: none required for this change.
Notes
maingoing green ([Other]: main E2E has been red for 24 consecutive scheduled runs #10519, currently 2 red specs with fix(terminal): wait for PTY teardown before close ack #10621 open for one of them). Worth deciding separately.