Repository navigation
perf(ci): take advisory unit-selection evidence off the gate - #23776
Conversation
selection_evidence is continue-on-error on both the job and its comparison step, so it can never fail a PR -- it downloads the shard reports, compares selection against the full results and uploads a review artifact. But a caller's `needs: test` waits for every job in the called workflow, so living inside unit-tests.yml it held verify for ~36s after the last shard finished. It moves to its own reusable workflow called as a sibling, so it still runs on every PR and still uploads its artifact, but verify no longer waits for it. It is deliberately absent from verify's needs, and a contract test pins both that and its advisory status so it cannot drift back onto the critical path. Measured on a recent run: the shards finished, then selection_evidence ran 36s, then verify 3s. Only the last of those gates anything.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # after the last shard. Deliberately absent from verify's needs for the same reason. | ||
| unit_selection_evidence: | ||
| needs: [test] | ||
| if: ${{ !cancelled() && needs.test.result == 'success' }} |
There was a problem hiding this comment.
If a unit shard fails, or the separate relay integration job fails after the shards finish, needs.test.result is failure and this condition skips the evidence workflow. The shards upload their evidence even on failure, and the comparison can identify failing tests outside the proposed selection. Skipping it loses that advisory report when it could be most useful, though it does not affect the required gate.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one scope change worth confirming before merge.
Reviewed changes
- Advisory job hoisted out of
unit-tests.yml—selection_evidencenow lives in a new reusableunit-selection-evidence.yml, called frompr.ymlas a sibling job (needs: [test]) soverifyno longer waits on it. - Sibling gated on
needs.test.result == 'success'— narrows it to successful runs; previouslyif: !cancelled()compared partial results after a failure. - Contract test pinned — new
keeps advisory unit-selection evidence off the gateasserts the sibling exists, is absent fromverify.needs, is gone fromunit-tests.yml, and stayscontinue-on-error.
The mechanism is sound: the called workflow's only job is continue-on-error, so the caller job's result is success, shard artifacts remain run-scoped (a same-run sibling resolves them as before), and the new workflow file doesn't trip any workflow census test.
ℹ️ The daily x86 lane loses its selection evidence
The PR notes state that node-next-compat.yml "never called it" and that "the daily lane is unaffected." That is not accurate: node-next-compat.yml calls unit-tests.yml, which defined selection_evidence, so the daily lane did run it and uploaded unit-selection-review-attempt-N — with complete shadow-mode references for both Node 24 and 26. Moving the job out of unit-tests.yml silently stops that collection. It doesn't block anything, but it contradicts the stated rationale; either add a sibling call to node-next-compat.yml or correct the notes.
Technical details
# Daily lane selection evidence
## Affected sites
- `.github/workflows/node-next-compat.yml:40-45` — `test:` calls `./.github/workflows/unit-tests.yml`, which previously produced `selection_evidence`; no equivalent sibling exists here after this PR.
- `.github/workflows/unit-tests.yml` — the removed job was the only producer of `unit-selection-review-attempt-N` in that caller's run.
- `config/scripts/ci-unit-selection-review.mjs:66-67` — in `shadow` mode (the daily lane's default via `unit-plan.yml`) a full reference run yields `completeFullRun: true`, so those artifacts are exactly the "representative complete references" the script asks for.
## Required outcome
- Either restore daily evidence (e.g. sibling `needs: [test]` call in `node-next-compat.yml`) or drop the "daily lane is unaffected" claim so the behavior change is recorded truthfully.
## Open questions for the human
- Is daily `unit-selection-review` evidence intentionally retired, or is the daily lane meant to remain an evidence source for promoting selection out of shadow mode?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe PR workflow now runs unit-selection evidence through a separate reusable workflow after the Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The advisory evidence report remains available after successful tests without delaying the required verify check. No issue identified here blocks merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The required test gate remains intact and the evidence workflow gains no apparent privileges. However, evidence is now skipped when tests fail, removing the report that can identify failures the proposed test selection would have missed. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
…3700, #22943, #23720, #23793, #23776, #23796) into #23059 #23573 (keep a turn's bar on the prompt that opened it) is ported onto the turn-record membership instead of kept as a second owner lookup: - The running turn's bar comes from nativeChatTurnMembership, which now returns barTurnKey beside liveTurnKey. Where the host states each row's turn they are the same key: the running root turn's anchor, and a message handed over mid-turn is already scoped to that turn. Where the host states no scope, rows still group by position, so barTurnKey is the user entry the running record names (through the same anchors) while liveTurnKey keeps the positional live group, as #23573 kept row liveness on the newest prompt. - activeTurnOpenedBy, selectStructuredAgentTurnBars' openedBy and selectNativeChatActiveTurnKey are removed; the desktop list, the slot builder and both turn-status hooks read barTurnKey. - The queued-send clock (queuedUntil: a turn counts from the previous turn's end when its send waited behind it) is kept, read in the same one-pass walk over root turn records that keys timing by anchor. - Its tests are adapted to pass a journal: each runs on a host that states no scope (the original expectations), and the desktop and mobile bar tests also run on a scoped host. #23621 (a message Claude folds into its running turn no longer splits the turn) is host-only and merged as is: the translator contract carries both openTurnInLiveProviderCycle and #23059's command-turn members. Both sides grew the Claude translator past its line budget, so its result branch moves unchanged into claude-result-journaling.ts, beside claude-message-journaling.ts (the pre-commit lint refuses the merge without it). #23801's Codex child path (no error-driven end) and #23059's Codex /compact guard (a command turn ends only on turn/completed) both stand.
ELI5
After the tests finish, one extra job compares "which tests we would have picked" against "all the tests we actually ran" and files a report. It can't fail your PR — it's purely a note for us. But the green checkmark was waiting ~36 seconds for it anyway. Now it runs alongside instead of in front.
What Changed
selection_evidencemoves out ofunit-tests.ymlinto its ownunit-selection-evidence.yml, called frompr.ymlas a sibling withneeds: [test].Why it was on the critical path. The job is
continue-on-error: trueon both the job and its comparison step, so it can never fail a PR. But a caller'sneeds: testwaits for every job in the called workflow, andverifyneedstest— so verify sat behind an advisory job.From a recent run, the tail of the chain was: last shard finishes →
selection_evidence36s →verify3s. Only the last of those gates anything.It still runs on every PR and still uploads its
unit-selection-review-attempt-Nartifact. The only change is that the required check no longer waits for it.Why
This is the second instance of the same structural issue — a caller's
needsgating an entire reusable workflow, making independent work serial. #23743 fixed the front of the chain by hoistingplanout; this fixes the back by hoistingselection_evidenceout. Same mechanism, same fix.unit_selection_evidenceis deliberately absent fromverify'sneeds, which is the whole point, and a contract test pins that along with its advisory status so it cannot quietly become blocking again.Things I checked and rejected while looking for this
Recording these so nobody re-treads them:
config/scripts/ci-shard-timings.jsonwas refreshed 5 hours ago, so the spread is inherent to file-level granularity, not stale weights. No lever here.pool: threadsfor vitest. There is a real ~20% prize (a full shard: 70.55s forks → 56.49s threads locally), and the blocker is solvable —execArgv: ['--no-experimental-webstorage', '--expose-gc']is incompatible withworker_threads, which is why a naive--pool=threadsruns zero tests; V8 flags are per-process so moving them toNODE_OPTIONSmakesgc()work inside threads. But threads shareprocess.envandprocess.cwd(), and 9 files / 33 tests fail deterministically as a result (e.g.ssh-known-hosts-source.test.tsstubsHOMEand gets the real one back). One of them,ai-vault-service-ready-retention.test.ts, asserts aWeakRefwas collected and cannot work in a shared heap at all. Worth pursuing as its own deliberate change — it needs ~7 test files reworked plus a standing invariant that tests not mutate process globals — but it is not a config flip and does not belong here.selectedmode is already built, but gated onevent.pull_request.draft === true, so it only applies to drafts. Widening it is the single biggest lever in the whole workflow and also the riskiest; it is clearly being staged deliberately (shadow → drafts → …) and is not mine to accelerate.Linked Issue
N/A — internal CI performance work, no pre-existing issue.
Visual Proof
N/A — CI configuration only, no user-facing surface.
Testing
111 contract tests pass across
pr-workflow-parallelism,pr-code-change-scope,ci-background-step-barriersandpr-e2e-gate-contract.Added a new test,
keeps advisory unit-selection evidence off the gate, asserting the sibling job exists withneeds: ['test'], thatverify.needsdoes not contain it, thatunit-tests.ymlno longer defines it, and that it is stillcontinue-on-error: true. That last assertion matters: if someone later makes it blocking, the reason it was moved evaporates and the test should fail.Confirmed nothing else referenced the job: no contract test did, and
node-next-compat.ymlnever called it (it is PR-specific by nature — comparing selection against full results only makes sense on a PR diff), so the daily lane is unaffected.Artifacts still resolve: the shard reports are run-scoped, so a sibling called workflow in the same run downloads them exactly as before.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
needs.test.result == 'success', so it no longer runs when the shards fail. Previouslyif: !cancelled()meant it ran even then and compared partial results. That seemed like the right trade — a selection comparison against a failed run is not evidence of anything — but it is a deliberate narrowing and worth a reviewer's agreement.ubuntu-slimjob runs, just not in front of the gate.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)