Repository navigation
ci: the layout oracle gates merges - #27600
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Changes limited to the split-orientation module can still merge without the layout oracle, leaving a gap in the new gate. The reported headless token failure path is fixed, but the routing gap should be addressed before relying on this gate. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 8 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly and concisely identifies the primary change: making the layout oracle gate merges. Description check The description is detailed and covers the required sections, including the change, rationale, testing, visual proof, scope considerations, and checklist. It notes that no linked issue exists and leav… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
12642142-4935-4d42-9aa8-adcded5d0c07
📥 Commits
Reviewing files that changed from the base of the PR and between bd54fc7 and 5ae0c042e7432a753f44b560a41921362fa46f0a.
📒 Files selected for processing (6)
.github/workflows/e2e.yml.github/workflows/pr.ymlconfig/scripts/pr-code-change-scope.mjsconfig/scripts/pr-code-change-scope.test.mjsconfig/scripts/pr-e2e-layout-oracle-routing.test.mjsconfig/scripts/pr-e2e-source-routing.mjs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| const WORKSPACE_LAYOUT_ORACLE_MODULE = | ||
| /^(?:src\/(?:main|renderer\/src)\/runtime\/(?:[^/]+\/)*[^/]*(?:pane-admission|workspace-layout|session-tab|headless-[^/]*tab|layout-client|create-terminal|split-pty-backed-terminal|terminal-split-layout|spawn-placement)[^/]*\.ts$|tests\/e2e\/(?:helpers\/)?(?:workspace-layout-oracle|headless-layout-oracle|terminal-layout-journeys))/ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Route split-orientation resolution through the layout oracle.
For a PR that changes only src/renderer/src/runtime/remote-terminal-layout-resolution.ts, this matcher returns false and the required layout_oracle job skips. The existing split-orientation E2E route identifies that module as a source of layout values that are published and written back to the tree. Add its exact path to the classifier and cover it with a job-classification test.
An oracle-only job joins verify's needs. It runs only the two layout oracle specs through the existing e2e workflow, routed by layout writer paths; a PR outside the route skips it and verify treats the skip as a pass. The route is widened to the terminal store, terminal pane, graph sync, frozen mirror and compatibility folders while the layout-holder stack is open. The oracle specs leave the advisory e2e job so a PR does not run them twice.
5ae0c04 to
bfa0946
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
83df0525-6e10-4c8c-9339-1fb8bfa2ba8e
📥 Commits
Reviewing files that changed from the base of the PR and between 5ae0c042e7432a753f44b560a41921362fa46f0a and bfa0946.
📒 Files selected for processing (3)
.github/workflows/pr.ymlconfig/scripts/pr-code-change-scope.mjsconfig/scripts/pr-e2e-source-routing.mjs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| uses: ./.github/workflows/e2e.yml | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| test_files: '["tests/e2e/workspace-layout-oracle-headless.spec.ts","tests/e2e/workspace-layout-oracle.spec.ts"]' | ||
| run_changed_e2e: true | ||
| needs_build: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep this required gate behind the headless-oracle fix.
The PR summary reports that the headless oracle failed every scenario in both oracle-only runs on this branch. The same summary reports green runs with #27601 applied. If this job becomes required before #27601 merges, routed PRs will fail verify even when their layout changes pass. Merge the prerequisite first, or keep this gate from becoming required until the headless oracle passes on the target branch.
🧰 Tools
🪛 zizmor (1.30.1)
[warning] 1395-1395: use GitHub's dedicated self-repository syntax (self-repository): use '$/...' instead of './...'
(self-repository)
…nd stack writers Review fixes: run on the merge commit so a stale-base PR cannot pass on an old main; route the oracle's own e2e harness and runtime client (a change there broke main's oracle without running it), and the window-side layout writers the stack edits; move the route into its own module.
ELI5
The layout oracle is the test that checks every view of a workspace's panes and tabs agrees with what the app actually holds. Until now it ran on some PRs but could not block a merge. This makes it a required check for PRs that touch layout code, and leaves every other PR alone.
What Changed
Before: the oracle ran inside the general end-to-end job, which is deliberately left out of the required
verifycheck because the full suite is red on main. A PR that broke layout could merge with a red oracle.After:
layout oraclejob in.github/workflows/pr.ymlis inverify'sneeds. It calls the existinge2e.ymlwith only the two oracle specs: the window spec runs in the changed-spec lane, and the headless spec runs in the orcad lane on both serve hosts (orcad and Electronorca serve). Nothing else from the end-to-end suite runs.pr-code-change-scope.mjsgets alayout_oracleoutput, andverifyrequiressuccesswhen it should run andskippedwhen it should not. A PR outside the route shows the job as skipped, and that counts as a pass. A ready-for-review rerun of an already-passed source skips it the same way the other required jobs are skipped.github.sha), the same commit the other required jobs check out, so a PR whose base is stale cannot pass against an old main.config/scripts/pr-layout-oracle-route.mjs, a readable list):store/terminals/,components/terminal-pane/, graph sync,web-session-tabs-sync, tab-group repair and drag-drop commit, the runtime split, sync-window-graph and layout-projection modules, and the snapshot and leaf-move helpers.e2e.yml,ci-e2e-job-selection.mjsand the route module.src/and test harnesses do not route.e2e.ymlgets an optionalartifact_suffixinput. GitHub requires artifact names to be unique within one run, andpr.ymlnow callse2e.ymltwice. Without the suffix, the build artifact upload would fail when both calls run.Why
Reusing
e2e.ymlkeeps the required run the same as the oracle lanes people already dispatch by hand, so there is no second copy of the build and orcad setup to keep in step. A separate workflow file would have duplicated about 40 lines of setup. It also could not be dispatched before it reached main. Routing throughpr-code-change-scope.mjs(which imports the route module), like the other required jobs, means the readiness-reuse and skipped-is-pass rules apply to it without any new code.This changes workflow and CI-script files only; repository settings are untouched.
mainhas no branch protection or ruleset, soverifyblocks a merge by team convention, as it does today.CI cost: about 19 job-minutes per routed PR, in parallel: build about 1 min, native cache about 1 min, window oracle about 10 min, headless orcad + Electron about 6 min. Wall time is about 12 min. Over the last 300 main merges, 73 (24%) would have run it, including those that touch force-all files such as the lockfile or
pr.yml.yml`. Under the old non-blocking route, 19 did.Linked Issue
None. This is part of the terminal layout rework.
Visual Proof
N/A. This is a CI-only change with no app behaviour change.
Testing
config/scriptsvitest: 302 files passed.pr-e2e-layout-oracle-routing.test.mjsis rewritten for the gate: route coverage including the widened paths, exclusions, the oracle staying out of the advisory job, the job running exactly the two specs, andverifygating it.Oxlint,
pnpm run check:code-quality:changedandpnpm tcpass.The oracle-only run of
e2e.yml(main + test(e2e): open the oracle's headless layout stream with a paired connection #27601 + this change) passed twice: https://github.com/stablyai/orca/actions/runs/38178161828 and https://github.com/stablyai/orca/actions/runs/38178163450. Each took about 1 min build, 6 min headless and 10.5 min window. Before test(e2e): open the oracle's headless layout stream with a paired connection #27601 merged, the headless half failed on main (TypeError: Cannot read properties of undefined (reading 'deviceToken')). An earlier check run on this PR showed the gate working:verifyfailed withlayout_oracle: expected success, got failure. This branch is rebased onto main, which now includes test(e2e): open the oracle's headless layout stream with a paired connection #27601.This PR changes
pr.yml, which forces every routed job, so its own checks run the newlayout oraclejob.Automated tests added/updated, or explained why not below
Review
Agent skill upstream boundary
Notes
No app code changes, so security, cross-platform, SSH and mobile behaviour are unaffected. The oracle runs on Linux, as before.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)