QVAC-19792 feat: consolidate the ts-checks producers into on-pr-ts-nx - #4497
Merged
Merged
Conversation
on-pr-nx awaits a ts-check for every nx-affected package with hasTsChecks, but each on-pr-<pkg>-ts.yml only triggers on its own package's paths. nx treats pnpm-lock.yaml, the root package.json and nx.json as inputs to every project, so a change to any of them marks all 14 packages affected while triggering none of the producers. The awaits then burn their 25 minute timeout and fail, taking merge-guard with them via sanity-checks-status. First hit on #4454, run 35089161423: 23 changed paths, 14 affected, 5 of 6 awaits timed out. That PR cannot avoid it, since it adds a dependency to packages/llm-llamacpp/package.json and CONTRIBUTING requires the lockfile be regenerated in the same commit. Narrowing the nx side is not an option: nx.json fans out through a hard-coded implicits map, and the root package.json through the js plugin's npm-packages locator. Neither reads namedInputs, so only the producers can be aligned. Also adds the missing reusable-ts-checks.yml path to diffusion-cpp, which the other seven already had, so a change to the shared reusable stops silently skipping diffusion's checks. Costs one short ts job per package on dependency-bump PRs.
Contributor
Review StatusCurrent Status: ✅ APPROVED |
Contributor
Workflow security (shadow mode)zizmor found 1124 finding(s) in Findings are annotated inline on the changed files and listed in the job summary. Reproduce locally: pipx run zizmor==1.27.0 --offline .github/ |
Contributor
License compliance — findings detected (warn-only)Critical: 0 · High: 2 · Medium: 0
How to resolve a blocking finding:
Warn-only (shadow) mode — this check does not block merges yet. Updated automatically by the canonical license compliance workflow. NOTICE presence (advisory)Missing NOTICE (advisory, does not block):
|
Supersedes the three-path change earlier on this branch. Adding pnpm-lock.yaml, package.json and nx.json to each producer closed the global-input fan-out, but not the dependency-edge one: #4445 touches only registry-server and nx correctly selects the six packages depending on @qvac/registry-client, whose producers still never fire. Chasing each nx selection mechanism with static paths is whack-a-mole, and every miss costs a 25 minute timeout. on-pr-ts-nx triggers broadly on packages/** and lets nx narrow, so producer and consumer run the same nx computation against the same target and cannot select different sets. Only affected packages run a ts job, so this is cheaper than broadening the eight producers to packages/**. Stays on pull_request: these run PR-head code and must remain unprivileged. That is also where nx-set-shas works natively. Job ids are load-bearing, since a reusable call publishes "<job id> / ts-checks" and on-pr-nx polls those exact strings. All eight are preserved, including diffusion's inline job, which predates the reusable and has no "/ ts-checks" suffix. Deletes the eight per-package producers in the same commit: both publish identical check names, so keeping them would run every ts check twice.
…s-nx A renamed job id stops satisfying its pollers silently; the only symptom is an await burning its 25 minute timeout on someone else's PR days later. Reads the awaited names out of on-pr-nx's jq map, derives the published names from the producer's job ids and name: overrides, and scans the repo for any other literal poller. Verified to fail on a renamed job id, a dropped name: override and a missing if: gate.
…nx's Review feedback. Tests 1-2 close the check-name loop and test 3 the per-job gating, but nothing asserted trigger parity, which is the invariant the design rests on: narrow the producer below the consumer and on-pr-nx runs while on-pr-ts-nx does not, so nx never narrows anything and every await times out. That is the regression the superseded approach shipped. Compares both paths: blocks, treating a trailing /** as covering everything beneath it so a broad producer glob still satisfies a narrower consumer path. Verified to fail both when the producer is narrowed to one package and when the consumer gains an unmirrored path. Also softens the header comment: the two share the graph and the on-pr target, but not the changed-file input, since this workflow uses nx-set-shas' git range while on-pr-nx feeds the API file list to nx --stdin.
Proletter
approved these changes
Sep 17, 2026
GSServita
approved these changes
Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the eight
on-pr-<pkg>-ts.ymlproducers with one nx-driven workflow, so the checkson-pr-nxawaits are always produced.🎯 What problem does this PR solve?
on-pr-nxawaits a ts-check for every nx-affected package withhasTsChecks, but each producer triggered on its own package's paths. nx selects a package for reasons those paths never see, so the check is never produced and the await burns the hard 25-minuteTIMEOUT_MSinawait-ts-checks/await.mjs.merge-guard / validate-prfails with it, becausesanity-checks-statusfolds inneeds.ts-checks.result.Two mechanisms have hit production, hours apart:
Global inputs. #4454, run 35089161423 —
pnpm-lock.yamlmarks all 14 packages affected while triggering no producer. Five of six awaits timed out. Also hit #4439 twice and #4389; both are SDK-side PRs that touch no native addon at all.Dependency edges. #4445 touches only
packages/registry-server/**, includingclient/index.d.ts, which publishes@qvac/registry-client. Six packages depend on it, so nx is right to select them — their types really could break. The bug is that the checks proving it never run.An earlier commit on this branch added
pnpm-lock.yaml,package.jsonandnx.jsonto each producer. That closes the first mechanism and not the second, and nx has others still (project-glob-changes,workspace-json-changes). Chasing each with static paths is whack-a-mole where every miss costs 25 minutes.📝 How does it solve it?
on-pr-ts-nx.ymltriggers broadly and lets nx narrow:A
matrixjob runsnx-project-matrixwithtarget: on-pr, filters tohasTsChecks, and eight static jobs gate on the result:Producer and consumer now resolve against the same nx graph and the same
on-prtarget, so a package selected for any reason is selected on both sides. Only affected packages run a ts job, which is why this is cheaper than broadening the eight producers topackages/**.Same graph and target is not the same as identical inputs. This workflow computes its changed files from
nx-set-shas' git range;on-pr-nxfeeds the API file list tonx --stdin. Base-sha resolution and rename handling can diverge at the edges, and that asymmetry is inherent to the privileged/unprivileged split rather than something this PR removes.It stays on
pull_requestbecause these run PR-head code and must remain unprivileged. That is also the event wherenx-set-shasworks natively.Job ids are load-bearing. A reusable call publishes
<job id> / ts-checks, andon-pr-nxpolls those exact strings. All eight are preserved, including diffusion's, which predates the reusable, runs inline withname: diffusion-pr-head-ts-checks, and has no/ ts-checkssuffix. Its steps are copied verbatim; migrating it onto the reusable would rename a status check and is a separate decision.The eight producers are deleted in the same commit. Both publish identical check names, so keeping them would run every ts check twice on the same SHA.
Check names
Not a detail: these strings are the entire contract between producer and consumer, matched literally. A rename does not error, it makes the await poll for something that will never appear and fail 25 minutes later, on an unrelated PR.
Every name is unchanged by this PR. The published name is
<job id> / ts-checksfor the seven that call the reusable, and the job'sname:for diffusion, which runs inline.classification-ggml-pr-head-ts-checks / ts-checksdiffusion-pr-head-ts-checksname:override, inline job, no suffixembed-pr-head-ts-checks / ts-checksllm-pr-head-ts-checks / ts-checksmodel-fit-pr-head-ts-checks / ts-checksocr-ggml-pr-head-ts-checks / ts-checkstranslation-nmtcpp-pr-head-ts-checks / ts-checksvla-pr-head-ts-checks / ts-checksDiffusion's exception is why the job id in
on-pr-ts-nx.ymlists-checkswith aname:override rather thandiffusion-pr-head-ts-checks: it reproduces the old workflow's structure exactly, and changing it would rename the check.Each name has two or three pollers, all of which must keep resolving:
on-pr-nx.yml— the jq map that buildsmatrix.tsCheckNameon-pr-<pkg>.ymlconsumers — a literalcheck_name:each, eight of them, deleted by #3976.github/scripts/test/await-ts-checks.test.mjs— hard-codesllm-pr-head-ts-checks / ts-checksThat last one is easy to miss, which is the point of the next section.
How to verify
.github/scripts/test/ts-check-names.test.mjsis added here and checks four things:on-pr-nxawaits is published byon-pr-ts-nxmatrixjobon-pr-ts-nx's trigger paths are a superset ofon-pr-nx'sThe fourth is the invariant the whole design rests on, and it closes the trigger loop the way 1–2 close the name loop. Narrow the producer below the consumer and
on-pr-nxruns whileon-pr-ts-nxdoes not, so nx never narrows anything and every await times out. That is precisely the regression the superseded approach shipped, and nothing asserted it until now. A trailing/**is treated as covering everything beneath it, so a broad producer glob still satisfies a narrower consumer path.It is mutation-tested rather than merely passing:
name:overrideif:gate🧪 How was it tested?
Probed with the eight producers deleted, so nothing else could have produced the checks:
on-pr-ts-nxmatrix["ocr-ggml"]ocr-ggml-pr-head-ts-checks / ts-checksawait ts-checks (ocr-ggml) / await-ts-checksThe two sides agreed exactly:
on-pr-nxunderpull_request_targetloggedAffected (on-pr, transitive): ["ocr-ggml"], andon-pr-ts-nxunderpull_requestcomputed the same. That agreement is what the design turns on, though as noted above it rests on a shared graph and target rather than on identical changed-file inputs.The run pages show the published names directly. The producer run lists
ocr-ggml-pr-head-ts-checks / ts-checksas succeeded and the other seven job names as skipped, so all eight can be read off one page and compared against the table above. The consumer'sawait ts-checks (ocr-ggml) / await-ts-checksresolved in 7 seconds, against the 25-minute timeout it hits today.Check names asserted mechanically, see the section above. Eight for eight against
on-pr-nx's map, plus a whole-repo scan for orphaned pollers, plus mutation testing to prove the assertion can actually fail.Static:
actionlint -shellcheck=clean.ts-check-names4/0,await-ts-checks13/0,ci-trust-policy74/0,prebuild-status19/0,publish-gate-policy5/0,publish-gpr-skip-policy33/0,runner-names13/0. No reference to any deleted file remains anywhere in the repo.Expect one noisy CI run on this PR
Deleting the eight files matches
.github/workflows/*<pkg>*.ymlin the legacyon-pr-<pkg>.ymlconsumers, so six of them wake up. Those packages are not nx-affected, the new producer correctly skips them, and their awaits have nothing to wait for. Observed on the probe: sixOn PR Trigger (<pkg>)runs whoseAwait PR-head TypeScript checksjobs would time out.It is one-time, confined to this PR, and disappears once #3976 removes those legacy workflows. Landing this after #3976 avoids it entirely.