Consume and verify product-v2 release bundles - #14
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request changes packaging from verified binaries to product-v2 bundles containing a backend-neutral host, one native runtime, manifests, and provenance. Workflows, packages, images, Homebrew, QA scripts, certification rules, tests, and documentation now validate and preserve those inputs. ChangesProduct-v2 bundle packaging
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UpstreamArchive
participant NativePackageBuilder
participant ReleaseWorkflow
participant RuntimeImage
UpstreamArchive->>NativePackageBuilder: provide verified product-v2 bundle
NativePackageBuilder->>NativePackageBuilder: install host, runtime, manifests, provenance
ReleaseWorkflow->>ReleaseWorkflow: verify provenance and host invariants
ReleaseWorkflow->>RuntimeImage: pass immutable host/runtime build arguments
RuntimeImage->>RuntimeImage: emit OCI provenance labels and run readiness QA
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
1404606 to
54f435d
Compare
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
scripts/upstream-archive.ts (2)
142-153: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
sha256Treebuffers whole files into memory; native runtime libraries can be large.Unlike
sha256File, which streams viacreateReadStream,sha256TreecallsreadFileSync(path)per file, loading the entire content into memory. Backend runtime libraries (CUDA/ROCm.sofiles) can be very large, so this risks memory pressure/blocking on every package build across the matrix.♻️ Proposed fix: hash files incrementally
- digest.update(createHash("sha256").update(readFileSync(path)).digest()); + const fileHash = createHash("sha256"); + const fd = openSync(path, "r"); + try { + const buffer = Buffer.alloc(1 << 20); + let bytesRead: number; + while ((bytesRead = readSync(fd, buffer, 0, buffer.length, null)) > 0) { + fileHash.update(buffer.subarray(0, bytesRead)); + } + } finally { + closeSync(fd); + } + digest.update(fileHash.digest());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/upstream-archive.ts` around lines 142 - 153, Update sha256Tree to hash each file incrementally using the existing streaming approach from sha256File instead of readFileSync(path). Preserve the current path ordering, relative-path length encoding, and per-file digest contribution while avoiding whole-file buffering.
172-176: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
outputDirisn't cleared before extraction; stale files from a prior run can persist.
cpSyncoverwrites same-named entries but leaves anything else already inoutputDiruntouched. If a previous invocation left a differently-namednative-runtimes/<old-id>directory (e.g., a different runtime selected on a retry), it would silently remain alongside the freshly extracted bundle.♻️ Proposed fix: reset outputDir before copying
mkdirSync(input.outputDir, { recursive: true }); + rmSync(input.outputDir, { recursive: true, force: true }); + mkdirSync(input.outputDir, { recursive: true }); const temporary = mkdtempSync(resolve(tmpdir(), "mesh-llm-upstream-"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/upstream-archive.ts` around lines 172 - 176, Clear and recreate input.outputDir before copying the extracted mesh-bundle so files from prior runs cannot persist. Update the extraction flow around mkdirSync and cpSync, preserving recursive copying into the now-empty output directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/images-release.yml:
- Around line 294-304: Update the “Read immutable product inputs” step in
.github/workflows/images-release.yml at lines 294-304 and 585-595 to fail
closed: require exactly one .upstream-provenance.json file, validate host_sha256
and runtime_sha256 as valid SHA-256 values and runtime_id as non-empty using jq
-e, then write only the validated values to GITHUB_OUTPUT without masking jq
failures.
In `@docs/gpu-runbooks.md`:
- Around line 17-22: Update the backend-import failure guidance in the GPU
runbook to reference the concrete dependency patterns checked by
docker/qa-runtime-image.sh instead of the ecosystem names “ROCm” and “Vulkan.”
Describe the mechanical check procedure and its expected failure boundary, while
preserving the requirement that host commands work without device passthrough.
In `@docs/matrix.md`:
- Around line 3-6: Update the schema 2 description in docs/matrix.md to describe
each upstream row as containing a backend-neutral host for its specific
OS/architecture, rather than an OS/architecture-neutral host. Preserve the
statement that each row contains exactly one runtime.
In `@packaging/native/build-package.sh`:
- Around line 18-21: Add an explicit existence check for the
bundle/native-runtimes directory before the runtime_count find in
build-package.sh, using the script’s established diagnostic-and-exit pattern.
Keep the existing exactly-one-runtime validation and manifest check unchanged.
In `@schemas/product-v2.schema.json`:
- Around line 59-84: Add the same character-allowlist pattern required by the
consolidated id/path validation guidance to runtime_artifact.id and
runtime_artifact.path, while preserving their existing non-empty string
constraints and the object’s required/additionalProperties rules.
In `@scripts/upstream-archive.ts`:
- Around line 111-133: Prevent runtime path traversal by adding the same
safe-character allowlist (such as ^[A-Za-z0-9][A-Za-z0-9._-]*$) to
manifest.runtime.id in validateProductManifest before validating or accepting
runtime.path; update schemas/product-v2.schema.json’s runtime_artifact
definition at lines 59-84 with the matching id pattern, and optionally apply it
to path as well, keeping both validators consistent.
---
Nitpick comments:
In `@scripts/upstream-archive.ts`:
- Around line 142-153: Update sha256Tree to hash each file incrementally using
the existing streaming approach from sha256File instead of readFileSync(path).
Preserve the current path ordering, relative-path length encoding, and per-file
digest contribution while avoiding whole-file buffering.
- Around line 172-176: Clear and recreate input.outputDir before copying the
extracted mesh-bundle so files from prior runs cannot persist. Update the
extraction flow around mkdirSync and cpSync, preserving recursive copying into
the now-empty output directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d209d4b-bffb-4274-aca5-99fe1ffe079d
📒 Files selected for processing (28)
.github/workflows/images-precheck.yml.github/workflows/images-release.yml.skills/distribution-certification/SKILL.md.skills/distribution-certification/references/format-checks.md.skills/distribution-certification/references/report-contract.mdAGENTS.mdREADME.mdTODO.mddocker/Dockerfile.mesh-llmdocker/qa-runtime-image.shdocs/gpu-runbooks.mddocs/matrix.mddocs/native-packages.mddocs/package-signing.mddocs/packaging-readiness-gaps.mddocs/packaging-readiness-scorecard.mddocs/publishing.mddocs/release-checklist.mddocs/runner-capacity.mddocs/tagging.mdpackaging/homebrew/Formula/mesh-llm.rb.templatepackaging/homebrew/README.mdpackaging/native/README.mdpackaging/native/build-package.shschemas/product-v2.schema.jsonscripts/native-package-qa.shscripts/upstream-archive.tstests/upstream-archive.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/upstream-archive.ts`:
- Around line 148-167: Update the tree-digest contract consistently between the
producer in the runtime hashing flow and sha256Tree/legacySha256Tree
expectations in tests/upstream-archive.test.ts. Either preserve the existing
digest wire format in the producer or atomically update the compatibility
implementation and all affected tests so existing product-v2 bundles continue to
be accepted; do not leave sha256Tree(runtime) and legacySha256Tree(runtime)
producing mismatched results.
In `@tests/workflow-provenance.test.ts`:
- Around line 56-62: Add newline and other invalid runtime_id cases to
invalidCases, then update the workflow’s runtime_id validation predicate to
require the archive allowlist pattern ^[A-Za-z0-9][A-Za-z0-9._-]*$ rather than
only checking for a non-empty string. Reuse the validation rule from
scripts/upstream-archive.ts and preserve acceptance of valid identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 05b85100-974c-452b-a906-e8981bbad551
📒 Files selected for processing (9)
.github/workflows/images-release.ymldocs/gpu-runbooks.mddocs/matrix.mdpackaging/native/build-package.shschemas/product-v2.schema.jsonscripts/upstream-archive.tstests/native-package.test.tstests/upstream-archive.test.tstests/workflow-provenance.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/gpu-runbooks.md
- schemas/product-v2.schema.json
- packaging/native/build-package.sh
- .github/workflows/images-release.yml
- tests/upstream-archive.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/qa-runtime-image.sh (1)
27-32: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
lddfailures are masked by the pipeline, letting the leak/dependency checks silently pass.Under
sh -eu(nopipefail),ldd ... | awk .../ldd ... | grep ...only propagate the last stage's exit status. Ifldditself errors out on/usr/local/bin/mesh-llm(e.g. non-dynamic binary, permission issue), both checks below receive empty input and report "no missing deps" / "no leaked backend libs" — masking the very regression this QA step exists to catch.🛡️ Proposed fix: check `ldd`'s exit status explicitly
-missing="$(ldd /usr/local/bin/mesh-llm | awk '/not found/ { print $1 }')" +ldd_output="$(ldd /usr/local/bin/mesh-llm)" || { echo "ldd failed to inspect mesh-llm" >&2; exit 1; } +missing="$(printf '%s\n' "$ldd_output" | awk '/not found/ { print $1 }')" [ -z "$missing" ] || { echo "host has unresolved dependencies: $missing" >&2; exit 1; } -if ldd /usr/local/bin/mesh-llm | grep -Eiq 'cuda|cublas|nccl|hip|hsa|vulkan|ggml|llama'; then +if printf '%s\n' "$ldd_output" | grep -Eiq 'cuda|cublas|nccl|hip|hsa|vulkan|ggml|llama'; then echo "backend dependency leaked into the mesh-llm host" >&2 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker/qa-runtime-image.sh` around lines 27 - 32, Update the dependency checks in the QA script around the ldd invocations for /usr/local/bin/mesh-llm so ldd’s own exit status is captured and treated as a failure before processing its output with awk or grep. Preserve the existing missing-dependency and backend-leak checks, but ensure neither pipeline can silently pass when ldd fails.
🧹 Nitpick comments (1)
scripts/node-sdk-runtime-smoke.cjs (1)
81-108: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTimeout escalates straight to
SIGKILL, skipping the child's own cleanup.On timeout,
child.kill('SIGKILL')preventsrunChild'sfinallyblock from ever runningnode.stop()or removingsmokeRoot, leaking temp state (and possibly an orphaned native runtime process) in CI. Consider aSIGTERM-first escalation with a bounded grace period, paired with aSIGTERMhandler in the child that triggers the same stop+cleanup path, consistent with the bounded-shutdown pattern used elsewhere in this PR (scripts/client-readiness-smoke.sh).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/node-sdk-runtime-smoke.cjs` around lines 81 - 108, Update supervise so timeout sends SIGTERM first, then escalates to SIGKILL only after a bounded grace period if the child has not exited. Add SIGTERM handling in runChild that invokes the existing node.stop() and smokeRoot cleanup path before exiting, preserving normal completion and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docker/qa-runtime-image.sh`:
- Around line 27-32: Update the dependency checks in the QA script around the
ldd invocations for /usr/local/bin/mesh-llm so ldd’s own exit status is captured
and treated as a failure before processing its output with awk or grep. Preserve
the existing missing-dependency and backend-leak checks, but ensure neither
pipeline can silently pass when ldd fails.
---
Nitpick comments:
In `@scripts/node-sdk-runtime-smoke.cjs`:
- Around line 81-108: Update supervise so timeout sends SIGTERM first, then
escalates to SIGKILL only after a bounded grace period if the child has not
exited. Add SIGTERM handling in runChild that invokes the existing node.stop()
and smokeRoot cleanup path before exiting, preserving normal completion and
error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7da71a7f-98fe-4771-a613-6146293f1b1a
📒 Files selected for processing (26)
.github/workflows/images-precheck.yml.github/workflows/images-release.yml.skills/distribution-certification/SKILL.md.skills/distribution-certification/references/format-checks.mdAGENTS.mdREADME.mdTODO.mddocker/Dockerfile.mesh-llmdocker/qa-runtime-image.shdocs/matrix.mddocs/native-packages.mddocs/packaging-readiness-gaps.mddocs/packaging-readiness-scorecard.mdpackaging/homebrew/Formula/mesh-llm.rb.templatepackaging/homebrew/README.mdschemas/product-v2.schema.jsonscripts/client-readiness-smoke.shscripts/native-package-qa.shscripts/node-sdk-runtime-smoke.cjsscripts/verify-host-invariant.tsscripts/verify-product-schema.tstests/client-readiness-smoke.test.tstests/homebrew-release.test.tstests/node-sdk-runtime-smoke.test.tstests/product-contract.test.tstests/workflow-provenance.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- schemas/product-v2.schema.json
- packaging/homebrew/README.md
- AGENTS.md
- docs/packaging-readiness-gaps.md
- docs/matrix.md
- docker/Dockerfile.mesh-llm
- docs/packaging-readiness-scorecard.md
- .skills/distribution-certification/references/format-checks.md
- scripts/native-package-qa.sh
- tests/workflow-provenance.test.ts
- docs/native-packages.md
- .skills/distribution-certification/SKILL.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/client-readiness-smoke.sh`:
- Around line 118-121: Update the readiness check in
scripts/client-readiness-smoke.sh to parse each log record as JSON using an
available JSON parser, then test the event, status, and role properties
independently from the same parsed record. Remove the order-dependent chained
grep matching while preserving the existing “Client ready” readiness condition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 77857c5f-bcd5-4e23-bdfa-d8ebdfc8a3c1
📒 Files selected for processing (7)
TODO.mddocker/qa-runtime-image.shdocs/release-checklist.mdscripts/client-readiness-smoke.shscripts/node-sdk-runtime-smoke.cjstests/client-readiness-smoke.test.tstests/node-sdk-runtime-smoke.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docker/qa-runtime-image.sh
- tests/client-readiness-smoke.test.ts
- scripts/node-sdk-runtime-smoke.cjs
- TODO.md
1 participant
Summary
product-v2bundle as the packaging source of truthWhy
Downstream packaging previously treated upstream archives as looser binary inputs. The new contract makes composition immutable and verifiable, prevents unsafe archive extraction, and keeps native packages and runtime images aligned with the same product artifact.
Validation
actionlintpassedRemaining environment validation
Linux package/OCI dry-runs, Homebrew installation on a clean runner, PowerShell parsing, and hardware-qualified CUDA/ROCm/Vulkan lanes remain CI or target-host checks; the corresponding TODO contract items remain open.
Coordination
This is a stacked PR:
product-v2foundation: task: unify release hosts, native runtimes, and product bundles mesh-llm#1106Merge #13 first, then retarget this PR to
mainif GitHub does not do so automatically.Summary by CodeRabbit