QVAC-22749: infra: add fabric-stack CI for npm @qvac/fabric consumers - #3827
QVAC-22749: infra: add fabric-stack CI for npm @qvac/fabric consumers#3827jpgaribotti wants to merge 2 commits into
Conversation
Wire PR-built fabric prebuilds into npm-runtime consumer workflows so stack PRs validate against unreleased qvac-fabric without a dev npm publish. Adds overlay-local-fabric, fabric-prebuilds artifact publishing on on-pr-fabric, classification-ggml integration, and author docs in INTEGRATION.md. Includes CI fixes from feature/QVAC-22749-test (Windows/bash 3.2 compatibility, PR-head action checkout, tags passthrough) rebased onto upstream/main with publish-prebuild-status preserved. Test vcpkg overlay fixture omitted.
Review StatusCurrent Status: ❌ PENDING Pending reviews: Needs 1 Management or Team Lead, and 1 more from Management, Team Lead, or Member. |
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):
|
Workflow security (shadow mode)zizmor found 1112 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/ |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - name: Detect fabric-stack PR | ||
| id: detect | ||
| uses: ./.github/actions/detect-fabric-stack | ||
|
|
||
| resolve-fabric-prebuilds: |
| - name: Wait for fabric-prebuilds from on-pr-fabric | ||
| uses: ./.github/actions/wait-and-download-fabric-prebuilds | ||
| with: | ||
| github-token: ${{ github.token }} | ||
| head-sha: ${{ github.event.pull_request.head.sha }} | ||
| - name: Re-publish fabric-prebuilds for downstream jobs |
| - name: Detect fabric-stack PR | ||
| id: detect | ||
| uses: ./.github/actions/detect-fabric-stack | ||
|
|
||
| fabric-stack-matrix: |
GustavoA1604
left a comment
GustavoA1604
left a comment
There was a problem hiding this comment.
[critical] .github/workflows/on-pr-classification-ggml.yml — line 387 (detect-fabric-stack,)
This patch deletes the publish-prebuild-status job that publishes qvac/prebuild-classification-ggml. The central Merge Guard explicitly requires a fresh status from this workflow, so future classification PRs will fail verify-prebuilds. Restore the publisher and include it in merge-guard.needs.
[critical] .github/workflows/on-pr-classification-ggml.yml — line 139 (uses: ./.github/actions/wait-and-download-fabric-prebuilds)
This pull_request_target workflow executes a composite action checked out from the PR head. A same-repository PR is authorized automatically, allowing modified action code to run in the privileged default-branch context and potentially poison caches or exfiltrate accessible artifacts. Execute only default-branch action code, or move PR-head execution to an unprivileged pull_request workflow. The same issue exists for detect-fabric-stack in both caller workflows.
[critical] .github/actions/detect-fabric-stack/action.yml — line 60 (elif [ -n "$BEFORE_SHA" ] && [ "$BEFORE_SHA" != "$HEAD_SHA" ]; then)
Detection examines only the latest synchronization delta. If an earlier commit changed fabric and the newest push changes only the consumer, this returns false and re-enables stale prebuild reuse. labeled, reopened, and ready_for_review events also default to a head-to-head empty diff. For every PR event, compare the PR base SHA with the current head SHA.
[critical] .github/workflows/on-pr-classification-ggml.yml — line 388 (resolve-fabric-prebuilds,)
Adding this job to needs does not gate Merge Guard on its result. If artifact resolution fails, the dependent build and tests are skipped, but the status expressions below accept skipped jobs as success. Require successful detection/resolution whenever fabric_stack is true before allowing skipped downstream jobs to pass.
[critical] .github/workflows/on-pr-fabric.yml — line 461 (integration-tests-status: ${{ needs.fabric-npm-consumer-smoke.result == 'success' || needs.fabric-npm-consumer-smoke.result == 'skipped' }})
This treats every skipped smoke test as successful, including skips caused by detection, matrix, or artifact-publication failures. That allows a fabric-stack PR to pass without testing any npm consumer. Accept skipped only when detection succeeded and established that this is not a fabric-stack change.
[medium] .github/workflows/on-pr-fabric.yml — line 18 (- ".github/workflows/fabric.yml")
The patch removes vcpkg-overlays/ports/qvac-fabric/** from the trigger even though overlay changes are defined as fabric-stack changes. An overlay plus consumer-only PR will start the consumer workflow, which waits for an artifact from on-pr-fabric, but this workflow never starts and the consumer run times out. Restore the overlay path filter.
[medium] .github/workflows/on-pr-classification-ggml.yml — line 142 (head-sha: ${{ github.event.pull_request.head.sha }})
workflow_dispatch and workflow_call are deliberately classified as fabric-stack events, but they have no pull_request.head.sha. The downloader then queries with an empty head_sha, potentially selecting an unrelated recent fabric artifact. Pass ${{ github.event.pull_request.head.sha || github.sha }} and reject an empty SHA inside the action.
[medium] .github/workflows/on-pr-classification-ggml.yml — line 357 (fabric-overlay-artifact: ${{ needs.detect-fabric-stack.outputs.fabric_stack == 'true' && 'fabric-prebuilds' || '' }})
The PR fabric runtime is passed to desktop integration tests but not to run-mobile-integration-tests. The mobile app installs @qvac/fabric through npm, so it still packages the published runtime while testing classification prebuilds compiled against the PR runtime. Pass and apply the fabric overlay in the mobile workflow as well.
[medium] .github/actions/overlay-local-fabric/action.yml — line 92 (mkdir -p "$dest_plat")
The destination platform directory is merged rather than replaced. Files removed or renamed by the PR remain from the published npm package and can make compile/runtime tests succeed against stale artifacts. Remove dest_plat before copying the PR-built platform tree; apply the same replacement behavior in the all-platform branch.
| needs: | ||
| - fork-approval | ||
| - authorize | ||
| - sanity-checks | ||
| - ci-router | ||
| - detect-fabric-stack | ||
| - resolve-fabric-prebuilds |
There was a problem hiding this comment.
cpp-tests gained resolve-fabric-prebuilds as a dependency, but its if: has no status-check function, so the implicit success() skips it whenever that job skips.
resolve-fabric-prebuilds is skipped on every PR where fabric_stack != 'true' — the large majority. GitHub applies an implicit success() across all of needs unless the if: contains a status-check function, and this one contains none.
The asymmetry inside this file is the giveaway: prebuild (line 281) and run-integration-tests (line 333) both open with always() for exactly this reason, and both were edited in this PR. The inner (fabric_stack != 'true' || ...) clause shows the intent was for cpp-tests to run when fabric_stack is false; the outer implicit success() overrides it.
Impact: on an ordinary classification-ggml PR carrying run-cpp-addon-tests, C++ unit tests and the Linux fuzz targets stop running, and sanity-checks-status (line 403) scores the skipped result as a pass — so the merge guard stays green. This degrades the non-fabric-stack path, which is what separates it from the skipped-as-success issue raised on line 388.
Suggested fix: prefix with always() && so the status-check function suppresses the implicit success(), then re-add the sanity-checks guard that always() removes — without it the job would also run after an upstream failure.
if: |
always() &&
needs.sanity-checks.result == 'success' &&
needs.ci-router.outputs.run_cpp_tests == 'true' &&
needs.authorize.outputs.allowed == 'true' &&
(needs.detect-fabric-stack.outputs.fabric_stack != 'true' || needs.resolve-fabric-prebuilds.result == 'success')
| - name: Re-publish fabric-prebuilds for downstream jobs | ||
| uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # 7.0.0 | ||
| with: | ||
| name: fabric-prebuilds | ||
| path: fabric-prebuilds-staging |
There was a problem hiding this comment.
reusable-prebuilds.yml's merge job downloads every artifact in the run with merge-multiple: true, so the 168 MB fabric tree is absorbed into the classification-ggml prebuilds bundle.
reusable-prebuilds.yml:518-522 calls actions/download-artifact with no name and no pattern, flattening every artifact in the run into prebuilds/ before re-uploading under that name. Reusable workflows share the caller's run id, and prebuild transitively depends on resolve-fabric-prebuilds, so fabric-prebuilds is already present when the merge runs.
Measured on run 31182794170 — fabric-prebuilds 168,270,355 B; the nine classification-ggml-* per-platform artifacts 2,878,435 B; merged prebuilds 171,148,592 B. The difference is 2,878,237 B, so the shipped bundle is the fabric tree plus the addon artifacts.
Impact: integration-test-classification-ggml.yml copies prebuilds-staging/<platform>-<arch>/ into packages/classification-ggml/prebuilds/, so the integration tests run with fabric's runtime inside the addon's own prebuilds directory — masking whether the addon resolves @qvac/fabric from node_modules, which is the packaging property this bridge exists to prove. prebuild-artifact-save then marks the polluted bundle reusable, so a later non-stack push with an unchanged native hash can have it re-published.
Suggested fix: give the hand-off artifact a name the merge job's unfiltered glob will not absorb, and point fabric-overlay-artifact (lines 188, 308, 357) at the new name. This keeps fabric out of the bundle without touching the shared merge job.
- name: Re-publish fabric-prebuilds for downstream jobs
uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # 7.0.0
with:
name: fabric-prebuilds-resolved
path: fabric-prebuilds-stagingAlternatively, scope reusable-prebuilds.yml's merge download with an explicit pattern:.
| found=false | ||
|
|
||
| while [ "$(date +%s)" -lt "$deadline" ]; do | ||
| RUNS=$(gh api "repos/$REPO/actions/workflows/${WORKFLOW_FILE}/runs?head_sha=${HEAD_SHA}&per_page=20" \ |
There was a problem hiding this comment.
The empty-SHA mechanism is already reported on line 142; what is new here is that the failure has already occurred, in the run this PR cites as its validation.
On workflow_dispatch, github.event.pull_request.head.sha is empty, so this query becomes ...runs?head_sha=&per_page=20. GitHub drops an empty filter rather than matching nothing, so every recent on-pr-fabric run qualifies and the first one carrying the artifact wins.
That is not a latent risk here. The consumer run cited in the PR description, 31182794170, ran at head 6ffdb5770f3e and downloaded from run 31109191988, whose head is 5ab657a89870 — a different commit, from the previous day. The run offered as proof the bridge works compiled and tested classification-ggml against fabric from an unrelated commit.
Impact: on a real pull_request_target run head.sha is populated and the filter works, so no merge-affecting path is exposed. It bites workflow_dispatch, which this repo uses routinely for rollout and benchmark validation — the mode whose results are most likely to be trusted as definitive.
Suggested fix: alongside the head.sha || github.sha fallback proposed on line 142, make the action refuse to issue an unfiltered query, so a future caller passing an empty value fails loudly instead of silently selecting someone else's artifact.
if [ -z "$HEAD_SHA" ]; then
echo "::error::head-sha is required — refusing to query without a run filter"
exit 1
fiWorth re-running the validation on a labeled-event PR before merge: workflow_dispatch also short-circuits detect-fabric-stack to true, so neither cited run exercised the detection path either.
| @@ -0,0 +1,13 @@ | |||
| { | |||
| "vcpkg_direct": [ | |||
There was a problem hiding this comment.
vcpkg_direct names six packages, but seven carry a qvac-fabric vcpkg dependency — model-fit is missing — and fabric itself is listed as a consumer of the thing it produces.
Seven packages depend on qvac-fabric in their vcpkg.json at this head: embed-llamacpp, fabric, llm-llamacpp, model-fit, ocr-ggml, translation-nmtcpp, vla-ggml. on-pr-model-fit.yml:94 runs verify-qvac-fabric-lockstep alongside the other six, so model-fit is a first-class fabric consumer by the repo's own definition. INTEGRATION.md:270-273 repeats the omission and drops fabric too, naming only five.
Impact: nothing reads vcpkg_direct today — only .npm_runtime is consumed, at on-pr-fabric.yml:245 — so no build breaks. It matters because INTEGRATION.md:287 positions this file as the roster of record, instructing authors to "move it from vcpkg_direct to npm_runtime" on migration. A package that was never in the roster will not be moved when its turn comes, and this ships wrong on day one.
Suggested fix: add the missing entry so the roster matches the dependency graph.
"vcpkg_direct": [
"fabric",
"llm-llamacpp",
"embed-llamacpp",
"translation-nmtcpp",
"ocr-ggml",
"vla-ggml",
"model-fit"
],fabric is worth a separate decision — it is the producer, so listing it as a consumer of itself is either deliberate (it does carry the vcpkg dependency) or a copy artifact; a one-line comment either way would settle it. And since nothing validates this list, consider either checking it against packages/*/vcpkg.json in CI or dropping the key — an unread manifest that is already inaccurate will only drift further.
4 participants
🎯 What problem does this PR solve?
@qvac/classification-ggmltoday) install@qvac/fabricfrom the registry during CI, so stack PRs that change unreleasedqvac-fabriccannot validate consumer compile/link against the PR-built runtime.qvac-fabric+ addon development already works for direct vcpkg consumers via overlay ports, but there was no equivalent bridge for npm-runtime consumers.@qvac/fabricto npm.📝 How does it solve it?
.github/actions/detect-fabric-stack) when a PR touchesvcpkg-overlays/ports/qvac-fabric/**orpackages/fabricvcpkg manifests; disables stale prebuild reuse on stack PRs.on-pr-fabricto publish afabric-prebuildsartifact and run an npm-consumer smoke matrix driven by.github/fabric-consumers.json.overlay-local-fabriccomposite action to copy PR fabric prebuilds intonode_modules/@qvac/fabricbeforebare-make generate.wait-and-download-fabric-prebuildssoon-pr-classification-ggmlcan consume the fabric artifact from the parallelon-pr-fabricrun.classification-ggmlprebuilds, cpp-tests, and integration tests to overlay PR fabric whenfabric_stack=trueviafabric-overlay-artifactonreusable-prebuilds.fabric-stack-npm-consumer-smoke.yml(linux-x64 compile smoke for npm-runtime consumers).packages/fabric/INTEGRATION.md.publish-prebuild-statusinon-pr-fabric.yml.New files
.github/actions/detect-fabric-stack/action.yml.github/actions/overlay-local-fabric/action.yml.github/actions/wait-and-download-fabric-prebuilds/action.yml.github/fabric-consumers.json.github/workflows/fabric-stack-npm-consumer-smoke.ymlModified workflows
on-pr-fabric.yml,on-pr-classification-ggml.ymlcpp-tests-classification.yml,integration-test-classification-ggml.ymlprebuilds-classification-ggml.yml,reusable-prebuilds.yml🧪 How was it tested?
workflow_dispatchonfeature/QVAC-22749-testagainsttetherto/qvac:on-pr-fabric(run 31109191988): fabric prebuild matrix,fabric-prebuildspublish, andfabric-stack-smoke-classification-ggmlall green.on-pr-classification-ggml(run 31182794170):resolve-fabric-prebuilds→ overlay → cpp-tests (3 platforms) → prebuilds (9 platforms) → desktop integration → Android/iOS Device Farm all green.