Skip to content

QVAC-19792 fix: absent-package selection, cpp-lint overlay scope, carved-out TS checks, embed artifact name - #4555

Merged
sidj-thr merged 7 commits into
mainfrom
fix/nx-skip-packages-absent-from-trusted-ref
Sep 18, 2026
Merged

sidj-thr merged 7 commits into
mainfrom
fix/nx-skip-packages-absent-from-trusted-ref

Conversation

@sidj-thr

@sidj-thr sidj-thr commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

🎯 What problem does this PR solve?

Four independent on-pr failures. The first was this PR's original scope; the other three surfaced on #4390 and are latent main bugs that PR is the first to trip.

# Failure Cause
1 A PR that adds a package makes on-pr-nx select every package and fail at the 25-minute await timeout nx cannot resolve a project absent from the trusted ref, so it marks everything affected
2 cpp-lint (llm-llamacpp) fails at Overlay PR @qvac/fabric prebuilds the overlay is passed to every cpplint row, not only the npm fabric consumers
3 Await PR-head TypeScript checks burns 25 minutes on the VLA and Classification workflows the TS producer reads the post-partition matrix, which has already dropped both
4 Embed integration-tests fails at Download prebuilds bundle downloads prebuilds; the run publishes prebuilds-embed-llamacpp

1. Package absent from the trusted ref

First hit on #4491, which adds packages/sdk-kotlin. Same commit, two workflows, opposite answers:

Workflow Tree it reads Result
on-pr-nx (pull_request_target) the trusted ref, 2924f1e 14 packages
on-pr-ts-nx (pull_request) the merge ref []

on-pr-nx checks out the trusted ref by design, so a package the PR adds is not on disk. Handed a changed packages/sdk-kotlin/package.json for a project it cannot resolve, nx fails open and marks every project affected. on-pr-ts-nx reads the merged tree where the package exists, resolves it, sees no on-pr target, and correctly selects nothing.

2. cpp-lint applies the npm overlay to a vcpkg consumer

llm-llamacpp takes qvac-fabric through the vcpkg port, not npm, so it has no node_modules/@qvac/fabric. overlay-local-fabric hard-exits when it finds none, which is correct; it is being called where it should never run. Same lane, same run:

cpp-lint (embed-llamacpp)     success
cpp-lint (ocr-ggml)           success
cpp-lint (translation-nmtcpp) success
cpp-lint (model-fit)          success
cpp-lint (llm-llamacpp)       failure

The four that passed are in .github/fabric-consumers.json. The one that failed is not.

3. Carved-out packages get no TS check produced

#4497 replaced the eight per-package producers with on-pr-ts-nx.yml, whose static jobs gate on tspackages. That is derived from steps.compute.outputs.matrix, which nx-project-matrix publishes after stripping every carveOut: true row. vla-ggml and classification-ggml are carve-outs that declare TS checks, so they never reach tspackages and their producer jobs skip.

A skipped caller never expands the reusable, so GitHub publishes the check under the caller job name alone, with no / ts-checks suffix. on-pr-vla.yml and on-pr-classification-ggml.yml await the suffixed name unconditionally, and that name cannot exist.

The conceptual error is treating carveOut as if it scoped TS checks. It says which workflow drives a package's native lanes; TS checks are package-scoped and orthogonal.

4. Renamed artifact

integration-test-embed-llamacpp.yml still downloads the bare prebuilds name from before the per-package rename in #3903 and #4362. A sweep of every integration-test-*.yml shows it was the last one.

📝 How does it solve it?

  1. Drop changed paths under a package this checkout has no directory for, before handing the list to nx. Nothing is lost: a package absent from the trusted ref has no options.ci there either, so the matrix could never have built a row for it. This replaces "unknown project, therefore everything" with "unknown project, therefore nothing".

  2. Gate the cpp-lint overlay on fabric-consumers, the pattern cpp-tests-nx.yml already uses at its own call sites. When llm-llamacpp finishes migrating to the npm fabric package, adding it to fabric-consumers.json turns the overlay on with no workflow edit.

  3. Add an allrows output to nx-project-matrix, emitted before the carve-out partition, and read it in the TS producer's filter. matrix, any and carveouts are unchanged, so the native lanes still exclude carve-outs.

  4. Download prebuilds-embed-llamacpp.

|| true on each grep in fix 1 is load-bearing: grep exits 1 when it prints nothing, and under set -e that aborts the step. Both cases are normal, covered by B and C below.

🧪 How was it tested?

Fix 1. Ran nx directly against #4491's real 135-path list with main checked out, reproducing the CI job:

Input Affected
all 135 paths 14 packages, matches CI exactly
packages/sdk-kotlin/package.json alone 14
135 paths, pnpm-workspace.yaml removed 14, not the trigger
135 paths, sdk-kotlin paths dropped []

Then the filter itself, verbatim from the action, across four diff shapes:

Diff Paths Affected
A, the real #4491 diff 135 → 10 []
B, only the new package 125 → 0 []
C, no package touched at all 10 → 10 []
D, an existing package 1 → 1 ["@qvac/ocr-ggml"]

D is the one that matters for everyone else: normal PRs are unaffected. Also confirmed the two front-ends are not the cause. On the PR head tree --base/--head and --stdin both return []; on the base tree both return 14. The variable is the tree, not the file list.

Fix 3. Verified in CI on probe PR #4573, whose base carries this branch. The matrix job selected both packages the probe touches, and the two consumers of that computation correctly diverge:

Affected (on-pr, transitive): ["embed-llamacpp","vla-ggml"]
Matrix (on-pr):               [embed-llamacpp only]
Carve-outs (on-pr):           ["vla-ggml"]
TS-check packages:            ["embed-llamacpp","vla-ggml"]

Matrix (on-pr) still excluding vla-ggml is the regression guard: allrows must not widen the generic matrix, or the carve-out's native jobs would run twice. vla-pr-head-ts-checks / ts-checks then ran and passed, and the job it unblocks, Await PR-head TypeScript checks / await-ts-checks in the VLA workflow, completed in 91 seconds where it previously failed at its 25-minute timeout.

The same run shows the naming rule behind the original failure. Executed producers publish as <job id> / ts-checks; skipped ones publish the bare <job id>, which is the name no awaiter can match.

The carveOut and hasTsChecks intersection across every packages/*/project.json is exactly two packages:

package carveOut hasTsChecks
classification-ggml true true
vla-ggml true true
fabric true false
diffusion-cpp, embed-llamacpp, llm-llamacpp, model-fit, ocr-ggml, translation-nmtcpp false true

fabric is a carve-out with no TS checks, so it produces no spurious entry, and on-pr-ts-nx.yml has no fabric job in any case. Ran the filter against both inputs: allrows yields ["classification-ggml","ocr-ggml","vla-ggml"], matrix yields ["ocr-ggml"].

Fix 4. Static plus a reproduction of the failure. name: prebuilds appears nowhere else under .github/workflows/, and the producer emits prebuilds-${{ steps.pkg.outputs.name }}.

The fix itself cannot be exercised before it lands. A relative uses: ./.github/workflows/... resolves from the repository's default branch, not from the caller's ref, so no choice of PR base reaches the patched copy. Probe PR #4573 confirms this directly: its Embed run reports

integration-test-embed-llamacpp.yml@058d07ef6 | ref=refs/heads/main

for every reusable it called, 058d07ef6 being main's tip at the time. That run reproduces the bug verbatim, Unable to download artifact(s): Artifact not found for name: prebuilds, while the branch under review carries prebuilds-embed-llamacpp on both its base and head.

The same mechanism is why this has to land here rather than ride in #4390: the PR-head copy of a relative reusable never executes.

The workflow_dispatch route is also closed, since the download step is if: ${{ !inputs.prebuild_package }} and prebuild_package is required on dispatch, so a manual run always takes the npm-package path and skips the step. It reaches the artifact path only through workflow_call.

actionlint -shellcheck= clean. ts-check-names 4/0, ci-trust-policy 74/0, prebuild-status 19/0.

What this does not do

A brand-new package still gets no nx CI in the PR that adds it, because its project.json is not on the trusted ref for on-pr-nx to read options.ci from. That is inherent to reading config from a trusted ref and is unchanged here. This PR only stops the absence from selecting everything else. To have a new package tested in its introducing PR, land its project.json on the base branch first.

… trusted ref

on-pr-nx checks out the trusted ref, never the PR, so a package the PR adds is
not on disk. Handed a changed packages/<new>/package.json for a project it
cannot resolve, nx fails open and marks every project affected. PR #4491 adds
packages/sdk-kotlin and selected all 14, which made on-pr-nx await six ts-checks
that on-pr-ts-nx correctly never published, so all six hit the 25 minute timeout.

Drop those paths before handing the list to nx. Nothing is lost: a package
absent from the trusted ref has no options.ci there either, so it could never
have produced a matrix row. One notice per package names the remedy.

Verified against #4491's 135 paths with main checked out: 135 paths in, 10 kept,
sdk-kotlin dropped, affected goes from 14 to []. Matches what on-pr-ts-nx
computes on the merged tree, where the package does exist.
@sidj-thr
sidj-thr requested review from a team as code owners September 17, 2026 15:35
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Review Status

Current Status: ✅ APPROVED
Approvals so far: Team Lead: 1, Member: 2

@github-actions

Copy link
Copy Markdown
Contributor

License compliance — clean

No new dependency license findings in this PR.

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):

  • ./docs/website
  • ./packages/fabric/test/integration
  • ./packages/llm-llamacpp/benchmarks/server
  • ./packages/llm-llamacpp/benchmarks/performance
  • ./packages/inference-addon-cpp/mobile
  • ./packages/asr-ggml/benchmarks/server
  • ./packages/embed-llamacpp/benchmarks/server
  • ./packages/embed-llamacpp/benchmarks/performance
  • ./packages/sdk/e2e
  • ./packages/vla-ggml/sim/server
  • ./.github/actions/release-merge-guard

@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Workflow security (shadow mode)

zizmor found 1102 finding(s) in .github/ (highest severity: high). This check is warn-only and does not block the merge.

Findings are annotated inline on the changed files and listed in the job summary.

Reproduce locally:

pipx run zizmor==1.27.0 --offline .github/

@sidj-thr sidj-thr changed the title QVAC-19792 fix: ignore changed paths whose package is absent from the trusted ref QVAC-19792 fix: absent-package selection, cpp-lint overlay scope, carved-out TS checks, embed artifact name Sep 18, 2026
@sidj-thr
sidj-thr merged commit 8b62fa9 into main Sep 18, 2026
54 of 55 checks passed
@sidj-thr
sidj-thr deleted the fix/nx-skip-packages-absent-from-trusted-ref branch September 18, 2026 10:45
@lauripiisang

lauripiisang commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

The changed-files filter in nx-project-matrix/action.yml keys on whether packages/<first-segment> is a directory, but nx keys on the config path itself:

// nx@23.1.0 project-glob-changes.js — glob is **/package.json, any depth
if (isProjectFile && !existsSync(join(workspaceRoot, touchedFile.file))) {
  return Object.keys(projectGraphNodes)
}

Those agree only when the missing file is exactly packages/<pkg>/package.json.

Repro. Two real cases, each a path the filter leaves in the list:

Feed either to nx show projects --affected -t on-pr --stdin on a checkout lacking that file and you get all 14. Same for #3030 (packages/sdk/e2e/fixtures/echo-plugin/package.json) and #3624 (packages/inference-addon-cpp/mobile/package.json).

Fix. Keep the directory drop, then also drop surviving config paths that aren't on disk:

while IFS= read -r f; do
  case "$f" in
    package.json|project.json|*/package.json|*/project.json)
      [ -f "$f" ] || continue ;;
  esac
  printf '%s\n' "$f"
done < "$filtered" > "$filtered.tmp"
mv "$filtered.tmp" "$filtered"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants