fix(ci): move PR e2e to dedicated runner pool - #882
Conversation
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
🔗 Paperclip issue: BLO-19728 |
1 similar comment
|
🔗 Paperclip issue: BLO-19728 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b811c36
The routing change itself is verified working, not just plausible: the e2e check on this exact head completed success in 15m01s on runner arc-e2e-z8p44-runner-stptr with labels:["arc-e2e"]. That settles the only question a runner-label move really raises — the pool is provisioned and still has the root/apt access playwright install-deps chromium needs plus the embedded-Postgres capability the suite depends on. No Critical issues.
Both Important findings are about the guard rails added alongside the move, not the move. Each was reproduced by execution, not inferred.
Important Issues (2)
-
[pr-review-toolkit/tests]
scripts/check-github-runner-labels.test.js:44— The drift guard does not actually pin thee2ejob, because[\s\S]*?in/\n e2e:\n[\s\S]*?\n runs-on: arc-e2e\n/is not constrained to thee2eblock and will happily run past the end of that job into later ones. Reproduced: taking the realpr.yml, revertinge2etoruns-on: default, and appending any later job onarc-e2estill yieldsassert.match→ pass. Sincee2eis currently the last job in the file (pr.yml:513), the natural next edit — appending a job, e.g. an e2e shard — is exactly what silently disarms it, defeating the test's stated purpose of preventing drift back onto shared capacity.- Anchor the span so it cannot cross into the next top-level job key:
/\n e2e:\n(?:(?!\n [A-Za-z0-9_-]+:)[\s\S])*?\n runs-on: arc-e2e\n/. Verified against the current file (passes) and against the drifted file above (correctly fails). Sturdier still: parse the YAML and assertjobs.e2e["runs-on"] === "arc-e2e", which removes this whole bug class and the layout-coupling of reading the livepr.ymlat:41.
- Anchor the span so it cannot cross into the next top-level job key:
-
[gstack/review]
scripts/check-github-runner-labels.mjs:24— The updated message advertises an allowlist (use default, arc-light, arc-dind, arc-deploy, or arc-e2e) that the script never enforces. The only check is the denylist at:17(ubuntu|windows|macos|self-hosted), so every label outside it passes. Reproduced against a fixture ofarc-e2ee,defualt, andtotally-made-up-pool: the guard printsValidated 1 workflows: all runner labels use ARC.and exits 0. That makes the new accept-path fixture atcheck-github-runner-labels.test.js:20vacuous — it assertsarc-e2eis accepted, but no possible label could have failed there, so it reads as coverage of an allowlist decision that was never made. The gap predates this PR; introducing a new label is what makes it load-bearing, since a typo'd pool label is not a loud CI failure — the job queues with no runner able to claim it.- Make it a real allowlist:
const ALLOWED = new Set(["default","arc-light","arc-dind","arc-deploy","arc-e2e"]), flag anyruns-onoutside it, and build the:24message from that same constant so the prose cannot drift from the rule again. The fixture at:20then tests something real, and thee2elabel gains enforcement in every workflow rather than only inpr.yml's one job.
- Make it a real allowlist:
Suggestions (3)
- [native-codex]
.github/workflows/e2e.yml:19— The samepnpm run test:e2ebrowser suite still runs ondefaulthere, and attimeout-minutes: 30(:20) — the ceilingpr.yml:518-519documents as too short ("remained active through test 42" of 43 flows). It isworkflow_dispatch-only, so it does not contribute to the PR bursts this PR targets, but a manual run is likely to be truncated. Worth either the same treatment or a comment recording that the divergence is deliberate. - [pr-review-toolkit/comments]
.github/workflows/pr.yml:515-516— The comment hardcodes "28 shared slots". That number lives in cluster config, not this repo, so it will rot silently on the next resize. Referring to the shared pool's capacity without the figure keeps the rationale true indefinitely. - [pr-review-toolkit/tests]
scripts/check-github-runner-labels.test.js:41— Reading the live.github/workflows/pr.ymlviarepoRootputs a repo-layout integration assertion inside the guard script's unit-test file. It works, but it couples this file to a path it does not own; a rename ofpr.ymlfails a test whose name is about the runner-label guard.
Strengths
- The change is genuinely surgical: only
runs-onmoved.needs: [policy],timeout-minutes: 60,PAPERCLIP_E2E_SKIP_LLM, andpnpm run test:e2eare all byte-identical, so the PR body's "preserves the existing command, environment, and timeout" claim holds up under inspection. - The Playwright cache key (
playwright-${{ runner.os }}-<version>) keys on OS and version rather than runner label, so the pool move does not cold-start the browser cache — an easy regression to miss when repointing a job. - The
runs-oncomment explains why rather than what, which is the form that survives future edits. - Adding a drift regression test at all is the right instinct; the finding above is about its precision, not its existence.
Recommended Action
- No Critical issues — the routing is proven working on this head, so nothing blocks on the change itself.
- Address the two Important items this cycle: anchor the drift regex so it cannot be disarmed by the next appended job, and convert the guard to a real allowlist so the newly introduced
arc-e2elabel is actually enforced and its accept-path test becomes meaningful. - Consider the Suggestions opportunistically.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Addressed both Important findings at
Verified with I left |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 68baf5a
The routing is verified working on this head, not carried over from the last one: the e2e check completed success in 29m25s on runner arc-e2e-z8p44-runner-r66ft with labels:["arc-e2e"]. The guard also passes against the real tree — I fetched all 19 workflows at this exact SHA and ran check-github-runner-labels.mjs, which printed Validated 19 workflows and exited 0, and node --test passes 5/5. That matters more than usual here, because this revision converts the guard from a denylist to an allowlist, and an allowlist can reject workflows the denylist accepted. All 36 runs-on values in the repo are inside the new set (default ×16, arc-dind ×12, arc-deploy ×4, arc-light ×3, arc-e2e ×1), so nothing regresses today. No Critical issues.
Both prior Important findings are properly fixed. The one remaining Important is a new observation about the allowlist's matching layer, reproduced by execution.
Prior Findings Dispositioned (2)
- prior:b811c36 important 1 — fixed —
scripts/check-github-runner-labels.test.js:11— the drift pattern is now the anchored, tempered-greedy form/\n e2e:\n(?:(?!\n [A-Za-z0-9_-]+:)[\s\S])*?\n runs-on: arc-e2e\n/, so the span can no longer run past thee2eblock into a later job. Better than requested::64-68adds a negative self-test that revertse2etodefault, appends alater-jobonarc-e2e, and assertsdoesNotMatch— the exact disarming scenario, now pinned by an assertion rather than by reviewer trust. Confirmed the anchor behaves as claimed againstpr.ymlat this head (e2e:at:513,runs-on: arc-e2eat:517, with two 4-space comment lines in between that the lookahead correctly does not treat as a job key). - prior:b811c36 important 2 — fixed —
scripts/check-github-runner-labels.mjs:18— the check is now a real allowlist:ALLOWED_RUNNERSat:8, enforced asif (!ALLOWED_RUNNERS.has(runner))at:18, and the operator message at:25is interpolated from that same Set, so the prose cannot drift from the rule. Thearc-e2eetypo fixture added at:40-48now fails as intended (verified: exit 1,unknown.yml:3: runs-on: arc-e2ee), which also makes the accept-path fixture non-vacuous.
Important Issues (1)
- [gstack/review]
scripts/check-github-runner-labels.mjs:15— The allowlist compares a regex-captured raw scalar against aSet, so it both fails open and fails closed on legal YAML spellings of the same thing. Reproduced against this exact file:- Fails open:
runs-on:in block form emits nothing on theruns-on:line, so the capture is empty,if (!runner) continueat:16skips it, and the labels on the following lines are never examined. A job declaringruns-on:\n - self-hosted\n - linuxyieldsValidated 1 workflows: all runner labels use ARC.and exit 0 —self-hostedis precisely the label this guard exists to forbid.runs-on:\n group: ubuntu-hostedpasses the same way. (Inline[self-hosted, linux]is correctly rejected, so the hole is specific to the block forms.) - Fails closed:
runs-on: "default"is rejected, because the quotes are part of the captured string. That is a behavior change from this PR — the old denylist accepted it. No workflow uses the quoted style today, so it is latent, but the failure text readsRunner labels must use one of: default, ...against a file that literally saysruns-on: "default", which is a confusing thing to hand a developer. - The denylist tolerated both because it only ever asserted "this specific string is banned." An allowlist asserts "everything outside the set is banned," and the
:25message now states that unconditionally — so the block-form bypass is a gap between the guarantee advertised and the one enforced, and it is cheapest to close now, while this code is already open. - Fix at the parsing layer rather than adding cases: parse the YAML and read
jobs.*["runs-on"], normalizing scalar / sequence /group:forms into a list of labels, then require every label to be inALLOWED_RUNNERS. That resolves quoting for free and removes the whole class. If a dependency-free script is preferred, at minimum strip surrounding quotes from the capture and treat aruns-on:line with an empty value as a violation to be resolved rather than as a line to skip — fail-closed is the correct default for a policy guard.
- Fails open:
Suggestions (3)
- [pr-review-toolkit/tests]
scripts/check-github-runner-labels.test.js:60—workflow.replace("\n runs-on: arc-e2e\n", ...)replaces only the first occurrence. That is correct today (pr.ymlhas exactly onearc-e2e, at:517), but if a secondarc-e2ejob is ever added abovee2e, the drift simulation mutates that job instead,e2ekeeps itsarc-e2e, and the test fails witha later job must not satisfy the PR e2e runner assertion— a message that points away from the actual cause. Anchoring the replace to thee2eblock, or asserting on a parsedjobs.e2e["runs-on"], keeps the failure legible. - [native-codex]
.github/workflows/e2e.yml:19— the samepnpm run test:e2ebrowser suite still runs ondefaulthere, attimeout-minutes: 30(:20) — the ceilingpr.yml:518-519documents as too short. It isworkflow_dispatch-only so it does not contribute to the PR bursts this PR targets, but a manual run is likely to be truncated at a point the PR lane has already proven insufficient. Worth the same treatment, or a comment recording that the divergence is deliberate. - [pr-review-toolkit/tests]
scripts/check-github-runner-labels.test.js:53— reading the live.github/workflows/pr.ymlthroughrepoRootputs a repo-layout integration assertion in the guard script's unit-test file, coupling it to a path it does not own. A rename ofpr.ymlfails a test whose name is about the runner-label guard. Non-blocking; it is the pragmatic place for it today.
Strengths
- The routing change stayed surgical: only
runs-onmoved.needs: [policy],timeout-minutes: 60,PAPERCLIP_E2E_SKIP_LLM, andpnpm run test:e2eare byte-identical, so the PR body's "preserves the existing command, environment, and timeout" claim holds under inspection. - Both prior findings were fixed at the root rather than papered over — the regex was re-anchored and given a negative test that reproduces the disarming case, and the allowlist message is derived from the constant so it cannot drift again. The
arc-e2eefixture shows the accept path was made meaningful, not just green. - The earlier hardcoded "28 shared slots" figure is gone;
pr.yml:515-516now explains the rationale without a number that lives in cluster config, so the comment will not rot on the next resize. - The Playwright cache key (
playwright-${{ runner.os }}-<version>) keys on OS and version rather than runner label, so repointing the pool does not cold-start the browser cache — an easy regression to miss.
Recommended Action
- No Critical issues; the routing is proven working on this head and the guard passes the real tree, so nothing blocks on the change itself.
- Address the one Important item this cycle: make the allowlist match YAML rather than raw line text, so block-form
runs-oncannot smuggleself-hostedpast a guard whose message now promises a total allowlist. - Consider the Suggestions opportunistically.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Addressed the remaining Important finding in
The current-head e2e failure is unrelated to these files: it ran on |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 08e6ad8
Note: the review request named 68baf5a9, but the branch has since advanced to 08e6ad84 (a master merge). Reviewing the stale SHA would have left the real head unreviewed, so this review attests the current head.
The guard work is in good shape and the last outstanding finding is genuinely fixed: I fetched all 19 workflows at this exact SHA, ran check-github-runner-labels.mjs (exit 0, Validated 19 workflows) and node --test (6/6 pass), and reproduced the prior finding's three cases directly against this head.
The blocker is not the guard — it is the routing claim itself. e2e is red at this head, and the proximate cause is a per-test timeout that this PR's own change to the job ceiling has left behind.
Prior Findings Dispositioned (1)
- prior:68baf5a important 1 — fixed —
scripts/check-github-runner-labels.mjs:118— the allowlist no longer compares a raw captured line against theSet.extractRunsOnEntries(:72) now walks the indented block when the scalar is empty, handling sequence items (:88) and map keys (:95), andunquote(:26) strips surrounding quotes. Verified by execution against this head, covering both directions the finding described: block-formruns-on:\n - self-hosted\n - linuxnow exits 1 (t.yml:4: - self-hosted),group: ubuntu-hostednow exits 1, andruns-on: "default"/'default'now exit 0. The oldif (!runner) continueskip is gone — a bareruns-on:with no value is now a violation (verified exit 1), so the parser fails closed. This took the dependency-free branch the prior finding explicitly offered, and did both things it asked for.
Critical Issues (1)
-
[native-codex]
.github/workflows/pr.yml:517— Thee2echeck fails on this head (job91308156175, 48m,arc-e2e-z8p44-runner-dlq7j,labels:["arc-e2e"]):4 failed, 1 did not run, 36 passed (43.7m). Routing and pool capability are fine — every setup step includingplaywright install-depssucceeded, so this is not a provisioning gap. The long pole issmoke-lab.spec.ts:310, killed byTest timeout of 900000ms exceededat exactly15.0m.That cap is the part this PR invalidated.
tests/e2e/smoke-lab.spec.ts:308setstest.setTimeout(900_000), and its own comment at:305-307sizes that number against "the PR job's 30-minute ceiling" — the ceiling this PR raises to 60 atpr.yml:520. The job ceiling went up; the per-test cap did not. So the binding constraint silently moved from the job to the test: the job still had ~12 minutes of headroom (48m of 60m) when its longest test was terminated.The pool is slower, which is what exposes this. Same 41-test suite, adjacent in time: 43.7m on
arc-e2e(4 failed) vs 31.0m ondefault(41 passed).smoke-labalone measured 5.5m / 9.4m / 11.1m across threedefaultruns (699d84e1,de439e12,866f19bf— all passing) against 15.0m killed here. At 11.1m ondefaultit already had only ~26% headroom under a 15m cap, so a ~40% slower pool is more than enough to cross it. Job wall-clock across the threearc-e2eheads has trended 15m → 29m → 48m, so treat pool throughput as variable rather than uniformly slow.In fairness, not all four failures are attributable to this change:
pipelines-tutorial-flow.spec.ts:447also fails on adefault-pool run at this same master state (de439e12, 02:15), so that one is a pre-existing flake and should not be counted against the move.- Raise
test.setTimeoutinsmoke-lab.spec.ts:308in proportion to the new 60m job ceiling (1_800_000restores the original ~50%-of-ceiling margin) and update the stale:305-307comment, which still cites the 30-minute ceiling this PR replaced. Then re-run and confirme2eis green onarc-e2ebefore merging — the PR's whole claim is that this pool is a safe home for the suite, and that is exactly what is currently unproven. If the ~40% slowdown is inherent to the pool rather than a noisy neighbour, right-sizing thearc-e2enode spec is the more durable fix, since raising caps only buys headroom until the next slow test.
- Raise
Suggestions (3)
- [gstack/review]
scripts/check-github-runner-labels.mjs:113— The residual of the parsing class:^\s*runs-on:still misses legal YAML spellings of the same key. Verified bypassing entirely (exit 0, guard reports all-clear) at this head:"runs-on": self-hosted(quoted key),runs-on : self-hosted(space before colon), and flow mappinga: {runs-on: self-hosted}. None is idiomatic in a workflow file and none appears in the repo, so this is low-priority — but it is the reason a YAML parse remains the only way to close the class outright. - [pr-review-toolkit/types]
scripts/check-github-runner-labels.mjs:95—group:is checked againstALLOWED_RUNNERS, but in GitHub Actions a runner group and a runner label are separate namespaces. Two consequences, both verified: a group named like a label passes (group: default→ exit 0, asserted as intended atcheck-github-runner-labels.test.js:21viagroup: arc-deploy), and the legal combined formgroup: my-group+labels: [arc-e2e]is rejected (exit 1) because the group name is not in the label allowlist. Nothing uses either form today, so this is latent — worth either validatinglabels:only, or a comment recording that group names are deliberately held to the label allowlist. - [native-codex]
.github/workflows/e2e.yml:19-20— The samepnpm run test:e2esuite still runs ondefaultattimeout-minutes: 30here. This is no longer just a theoretical mismatch: the suite measured 43.7m onarc-e2eand 31.0m ondefaultin the runs above, so both exceed this ceiling and a manualworkflow_dispatchrun would be truncated. Worth the same treatment, or a comment recording that the divergence is deliberate.
Strengths
- The prior finding was fixed at the root, not patched case-by-case: an actual block-walker with quote handling and a fail-closed empty case, rather than another regex branch. The
block.ymlfixture atcheck-github-runner-labels.test.js:40-51pins all three spellings the finding named, so the fix is held by assertions rather than by reviewer trust. - Converting a denylist to an allowlist is the risky direction — it can reject workflows the old check accepted — and it was landed safely: all 36
runs-onvalues across the 19 workflows are literal scalars inside the allowed set, with no${{ }}expressions anywhere, so the stricter parser is a no-op on the real tree today. - The routing change itself stayed surgical.
needs: [policy],PAPERCLIP_E2E_SKIP_LLM, andpnpm run test:e2eare byte-identical; onlyruns-onand the job ceiling moved, which is what made the 43.7m-vs-31.0m comparison above a clean read on the pool. - The Playwright cache key (
playwright-${{ runner.os }}-<version>) keys on OS and version rather than runner label, so repointing the pool does not cold-start the browser cache — confirmed by the skippedInstall Playwright browserstep on this run.
Recommended Action
- Fix the Critical before merge: raise
smoke-lab.spec.ts:308alongside the new 60m job ceiling, refresh the stale:305-307comment, and re-run untile2eis green onarc-e2e. Ignorepipelines-tutorial-flow.spec.ts:447when judging that run — it fails ondefaulttoo. - No Important issues; the guard work is done and verified.
- Consider the Suggestions opportunistically — the
e2e.ymlceiling is the one with real evidence behind it now.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a179f0c
Note: the review request named 68baf5a9, but that head was already reviewed (2026-07-31T22:30:08Z) and the branch has since advanced twice, through 08e6ad84 to a179f0c7. Reviewing the requested SHA would have left the real head unreviewed, so this review attests the current head.
The guard work remains done and verified — I fetched the scripts and pr.yml at this exact SHA and node --test scripts/check-github-runner-labels.test.js passes 6/6, including the block-form, quoted-scalar, and arc-e2ee typo fixtures. Nothing has regressed there.
The one new commit on this head (a179f0c7, test(e2e): wait for pipeline settings details) touches a single file. It does not address the outstanding Critical, and the comparison run I pulled below makes the routing claim look weaker than it did at the last head, not stronger.
Prior Findings Dispositioned (1)
- prior:08e6ad8 critical 1 — still-present —
tests/e2e/smoke-lab.spec.ts:308— the per-test cap is stilltest.setTimeout(900_000)at this head, and the comment at:307still sizes it against "the PR job's 30-minute ceiling" thatpr.yml:520replaced with 60.smoke-lab.spec.tsis not among this PR's four changed files, so neither the cap nor the stale comment moved. Fetched ata179f0c7and inspected directly; the finding stands unchanged.
Critical Issues (1)
-
[native-codex]
.github/workflows/pr.yml:517— prior:08e6ad8 critical 1. The suite is not proven onarc-e2e, and a like-for-like comparison now indicates the pool — not a set of flaky tests — is the variable. Two runs on the same master state, ~30 minutes apart:pool wall clock result job 91308156175(08e6ad84)arc-e2e43.7m 4 failed, 1 did not run, 36 passed job 91314240338(de439e12)default(arc-default-qb6tz-runner-qcmdx)25.2m 1 failed, 40 passed The
defaultrun's single failure ispipelines-tutorial-flow.spec.ts:447— the one this head fixes. The other three fail only onarc-e2e, and all three are timeouts:signoff-policy.spec.ts:291—Test timeout of 60000ms exceeded(passes ondefault)smoke-lab.spec.ts:310—Test timeout of 900000ms exceededat exactly 15.0m (passes ondefault)mcp-user-stories.spec.ts:427(US-9) —409 {"reasonCode":"action_not_pending"}fromapproveActionRequest, a race that only loses on the slower pool (passes ondefault)
So this head fixes the single failure that is not pool-specific and leaves all three that are. At ~73% slower wall clock (25.2m → 43.7m), the suite's existing per-test caps no longer hold on the target pool, and
smoke-labis the clearest case: the job still had ~12 minutes of headroom (48m of 60m) when its longest test was killed by a cap the raised ceiling was supposed to relieve. Thee2echeck on this head was stillin_progressat review time (job91320173403, started 03:21:26Z, ~15m elapsed against a ~44m expected runtime), so it is unproven rather than green — and nothing in this diff changes the caps that failed last time.- Raise
smoke-lab.spec.ts:308in proportion to the new 60m ceiling (1_800_000restores the original ~50%-of-ceiling margin) and refresh the stale:307comment.signoff-policy.spec.ts:291needs the same treatment against its 60s cap. Then re-run and confirme2eis green onarc-e2ebefore merging — that green run is the PR's entire claim and is the one artifact still missing. Given three independent tests crossed their caps on this pool and none did ondefault, right-sizing thearc-e2enode spec is the more durable fix than raising each cap until it fits; raising caps only buys headroom until the next slow test, and it makes the suite slower to fail.
Important Issues (1)
- [pr-review-toolkit/tests]
tests/e2e/pipelines-tutorial-flow.spec.ts:471— The new wait hardcodestimeout: 15_000in a file that already has a named constant for exactly this purpose:slowUiTimeout = 30_000at:288, in scope here and used at twelve other sites in this same test (:482,:492,:501,:536,:548,:564,:570,:596,:605,:611,:618,:621). The new value is half the file's own slow-UI budget, and it is applied at the coldest moment in the test — the first assertion afterpage.goto(...)at:469, where the SPA route, the pipeline fetch, and first paint all have to land. Every warm in-page interaction afterwards gets 30s; the cold navigation gets 15s. That is backwards, and it is the wrong direction for a PR whose premise is a pool measured ~73% slower. The original 5s cap failed withelement(s) not found— a not-yet-rendered element, not a wrong selector — so the timeout is the load-bearing half of this fix, and it is the half most likely to still be short.- Use
slowUiTimeouthere. It documents intent, keeps the one knob that tunes this file's pool sensitivity in one place, and gives the cold path at least the budget the warm paths already get.
- Use
Suggestions (2)
- [gstack/review]
tests/e2e/pipelines-tutorial-flow.spec.ts:470— SwappinggetByLabel("Pipeline name")for the CSS locatormain input[aria-label="Pipeline name"]narrows the match in two ways that were not needed to fix this failure. I verified it does resolve at this head —PipelineSettings.tsx:2593setsaria-label="Pipeline name", andLayout.tsx:624renders<main id="main-content">unconditionally (theisMobilebranch only variesclassName) around the route registered atApp.tsx:225— so this is not broken. But the input atPipelineSettings.tsx:2589-2596is also wrapped in a<label>carrying ansr-only"Pipeline name" span, which is what madegetByLabelunambiguous in the first place. The failure was a timing miss, not an ambiguity or a wrong selector, so the added coupling buys nothing: the locator now breaks if the page is ever rendered outside the<main>landmark, and it breaks if an a11y cleanup drops thearia-labelas redundant with the wrapping label — a changegetByLabelwould have survived. KeepinggetByLabeland changing only the timeout is the smaller, more durable fix. - [native-codex]
.github/workflows/e2e.yml:19-20— The samepnpm run test:e2esuite still runs ondefaultattimeout-minutes: 30. The evidence against that ceiling keeps accumulating: the suite measured 25.2m ondefaultin the comparison run above and 43.7m onarc-e2e, so thedefaultfigure is now within five minutes of the cap and thearc-e2efigure is well past it. A manualworkflow_dispatchrun is likely to be truncated. Worth the same treatment, or a comment recording that the divergence is deliberate.
Strengths
- The guard work is finished and holds at this head: 6/6 tests pass, and the block-form walker still rejects
- self-hosted,group: ubuntu-hosted, and thearc-e2eetypo while accepting quoted and sequence spellings. Three consecutive heads of findings on this script have all been fixed at the root rather than patched case-by-case, and each fix arrived with a fixture that pins it. - The new locator change is genuinely diagnosed rather than guessed at: the prior run's error was
element(s) not foundwithTimeout: 5000ms, and raising the wait is the correct response to that specific message. Reaching for the actual failure text instead of retrying the test is the right instinct. - Fixing
pipelines-tutorial-flowwas worth doing on its own merits — it is the one failure reproducible ondefault, so it was a real pre-existing flake and clearing it makes the nextarc-e2erun easier to read. - The routing change itself is still surgical.
needs: [policy],PAPERCLIP_E2E_SKIP_LLM, andpnpm run test:e2eremain byte-identical; onlyruns-onand the ceiling moved, which is exactly what made the 25.2m-vs-43.7m comparison a clean read on the pool rather than a confounded one.
Recommended Action
- Fix the Critical before merge. The comparison run reframes it: three tests that pass on
defaultfail onarc-e2e, all on timeouts, so treat this as a pool-throughput question first — right-size thearc-e2espec, or raisesmoke-lab.spec.ts:308andsignoff-policy.spec.ts:291together and refresh the stale:307comment. Either way the merge gate is one greene2erun onarc-e2e. - Address the Important item this cycle: use
slowUiTimeoutat:471rather than a hardcoded 15s that undercuts the file's own convention at its slowest moment. - Consider the Suggestions opportunistically — reverting to
getByLabelis a one-line simplification, and thee2e.ymlceiling now has measurements behind it.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
@ally please re-review at head Addressed the current-head review:
Focused verification: runner guard tests pass 6/6, all 19 workflows validate, and |
|
@ally please re-review PR #882 at exact head Focus on dispositioning the prior Critical and Important findings after the timeout and locator fixes. All 20 current-head checks are now terminal with required jobs green, including |
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 05517e1
Looks good. No Critical or Important blockers remain on this head. The current-head e2e check completed successfully, and the timeout and locator changes directly address the two active prior findings without broadening production behavior.
Prior Findings Dispositioned (2)
- prior:08e6ad8 critical 1 — fixed —
tests/e2e/smoke-lab.spec.ts:308— the Smoke Lab test now has the requested 30-minute budget, its comment is aligned with the 60-minute PR job ceiling, andtests/e2e/signoff-policy.spec.ts:261raises the slower-pool budget to 120 seconds. The exact-heade2echeck completed successfully, demonstrating that the suite now finishes onarc-e2erather than hitting the prior per-test caps. - prior:a179f0c important 1 — fixed —
tests/e2e/pipelines-tutorial-flow.spec.ts:470— the first post-navigation assertion keeps the accessiblegetByLabel("Pipeline name")locator and uses the file's existingslowUiTimeoutconstant, removing the hardcoded 15-second budget that undercut the surrounding slow-runner convention.
Strengths
- The fixes are narrowly scoped to the demonstrated slow-runner failures: no application behavior or workflow command changed.
- The Smoke Lab comment and timeout now describe the same 60-minute ceiling, avoiding the stale rationale identified in the prior review.
- Current-head CI is terminal and green across the required lanes, including
e2eon the dedicated pool and all serialized server shards.
Recommended Action
No further review changes are required for these findings.
3 participants
Thinking Path
Linked Issues or Issue Description
Paperclip: https://paperclip.blockcast.net/BLO/issues/BLO-19728
Problem or motivation
PR Actions bursts were saturating the shared ARC
defaultpool because each long browser e2e job held one of those runner slots for the suite duration.Proposed solution
Route the PR workflow's
e2ejob toarc-e2e, and update the runner-label guard test coverage so the dedicated e2e label is accepted and the PR workflow keeps using it.Alternatives considered
Leaving the e2e job on
defaultkeeps starving shorter PR lanes during bursts. Workflow-level concurrency already cancels superseded runs per PR, but it does not isolate the long browser lane from the shared runner pool.What Changed
.github/workflows/pr.ymle2efromdefaulttoarc-e2e.arc-e2eas an allowed ARC label.arc-e2eand for the PR e2e workflow using the dedicated runner.Verification
node scripts/check-github-runner-labels.mjsnode --test scripts/check-github-runner-labels.test.jsgit diff --checkRisks
Low operational risk if the
arc-e2escale set remains provisioned with the same browser-test capabilities. The main risk is queueing e2e behind a smaller dedicated pool, but that is intentional so the long browser lane cannot starve the shared PR lanes.Model Used
OpenAI Codex, GPT-5-based coding agent with terminal and GitHub CLI tool use.
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template