fix: RC5 release readiness corrections - #918
Conversation
* Add missing runtimes * Add signing attestation key * Correct smoke script * Fix `doctor --json` output
|
Warning Review limit reached
Next review available in: 16 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 (1)
📝 WalkthroughWalkthroughAdds structured GPU probe capture, expands native runtime release coverage for Linux and Windows, hardens release tooling scripts, and documents release attestation signing keys. ChangesStructured GPU probe detection
Release jobs and validation
Release tooling and documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
| ``` | ||
|
|
||
| Store the keypair in 1Password before adding or rotating GitHub secrets. The | ||
| production release-attestation keypair lives in the `mesh-llm` vault as |
There was a problem hiding this comment.
is that a shared vault?
There was a problem hiding this comment.
local on my machine for now, can share with you guys
There was a problem hiding this comment.
Or - if you want to create a shared vault, I can move the secret to it
There was a problem hiding this comment.
depends if we need access to it - or just recreate it when needed?
There was a problem hiding this comment.
Yeah, good callout. It's not really needed for referencing outside of the build process, it can just be recreated/replaced and the next RC / release builds get signed with that instead.
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
nice - just one question about 1password and shared vault
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
RELEASE.md (2)
74-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting the
invalidstatus outcome.The documentation mentions
validandmissingstatuses. If the attestation footer is present but signature verification fails (wrong key, tampered binary), the tool likely reportsinvalid. Documenting this third outcome helps users distinguish between "not signed" and "signed but untrustworthy."🤖 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 `@RELEASE.md` around lines 74 - 76, The release-attestation status documentation needs to cover the third outcome reported by the verifier. Update the section describing valid and missing statuses to also mention the invalid status from the attestation verification flow, making clear that it applies when a footer is present but signature verification fails. Use the existing status wording in RELEASE.md so readers can distinguish missing from signed-but-untrusted outcomes.
52-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded repository name in generic documentation.
The
gh secret setexamples hardcode--repo Mesh-LLM/mesh-llm. Readers copying commands from a fork or different remote will fail silently or set secrets on the wrong repository. Remove--repoto default to the current authenticated repository, or add a comment that readers should substitute their own.gh secret set MESH_RELEASE_ATTESTATION_SIGNING_KEY_FILE \ - --repo Mesh-LLM/mesh-llm \ --app actions \ < /tmp/mesh-release-attestation/mesh-release-attestation-private-key.json gh secret set MESH_RELEASE_ATTESTATION_PUBLIC_KEY_FILE \ - --repo Mesh-LLM/mesh-llm \ --app actions \ < /tmp/mesh-release-attestation/mesh-release-attestation-public-key.json🤖 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 `@RELEASE.md` around lines 52 - 62, The gh secret set examples in RELEASE.md hardcode a specific repository name, which makes the commands unsafe for forks or other remotes. Update the documented commands to avoid embedding Mesh-LLM/mesh-llm in the gh secret set invocations, either by removing the --repo flag so gh uses the current authenticated repository or by clearly telling readers to replace it with their own repository; keep the examples tied to the secret names MESH_RELEASE_ATTESTATION_SIGNING_KEY_FILE and MESH_RELEASE_ATTESTATION_PUBLIC_KEY_FILE.crates/mesh-llm-native-runtime/src/lib.rs (1)
19-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAvoid widening the crate root for
HostGpuProbe.This adds a new root-level re-export for a type owned by
host, which grows the public API surface just to support internal plumbing. Prefer importing from the owning module directly, or treat this as a clearly temporary compatibility shim with a follow-up removal plan. As per coding guidelines, "crates/*/src/lib.rs: Minimize crate-root re-exports. Root re-exports are acceptable as temporary compatibility shims during refactors, but new code should prefer importing from the owning module directly."🤖 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 `@crates/mesh-llm-native-runtime/src/lib.rs` around lines 19 - 20, The crate root is being widened by re-exporting HostGpuProbe, which unnecessarily expands the public API. Remove the new root-level re-export from the lib.rs export list and update any internal callers to import HostGpuProbe directly from its owning host module; if a compatibility shim is truly needed, keep it explicitly temporary and plan its removal.Source: Coding guidelines
🤖 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/release.yml:
- Line 1156: The Windows CUDA packaging and validation are too broad because the
release workflow still uses an unversioned windows/x86_64/cuda target, allowing
non-12 CUDA manifests to pass. Update the release workflow entries that set up
and validate this target so they explicitly use windows/x86_64/cuda12, keeping
the packaging major and validator contract aligned.
- Line 414: The checkout step is leaving persisted Git credentials enabled,
which is unnecessary for these artifact-only release jobs. Update the relevant
actions/checkout invocation in the release workflow to explicitly disable
persisted credentials, and apply the same hardening to any other checkout steps
in this workflow. Use the checkout step(s) themselves as the target for the
change so the GITHUB_TOKEN is not written into .git/config.
- Line 1054: The GPU release job is restoring the Windows SDK installer cache
through the sccache action, which can allow reuse of a cached HIP SDK installer
after only a minimal header check. Update the release workflow to avoid
restoring that cache on the GPU release path, or ensure the install flow in
scripts/install-windows-sdk.ps1 verifies the installer with a checksum or
signature before reusing it. Use the mozilla-actions/sccache-action step and the
install-windows-sdk.ps1 path as the key places to adjust.
In `@crates/mesh-llm-hardware-profile/src/lib.rs`:
- Around line 60-63: The GPU discovery flow in the hardware profile logic
short-circuits on NVIDIA results, which causes hybrid systems to lose non-NVIDIA
GPU labels. Update the discovery path around detect_nvidia_gpu_profiles so it
merges the NVIDIA rows into the existing fallback results instead of returning
early, preserving AMD/Apple/Vulkan-discovered entries while still including
NVIDIA GPUs. Ensure the final aggregate returned by the profile collection code
contains all detected sources rather than replacing them when NVIDIA is present.
In `@scripts/rc-release-smoke.sh`:
- Around line 113-118: The mesh_bundle_pids helper currently passes BINARY
directly to pgrep -f, which treats it as a regex and can match unrelated
processes. Update mesh_bundle_pids to use a fixed-string-safe lookup or escape
the BINARY value before calling pgrep, so the PID sweep and any later pgrep
diagnostics only target the intended bundle path.
In `@scripts/tests/test_validate_release_native_runtime_matrix.py`:
- Line 188: The Windows CUDA target assertion in
test_validate_release_native_runtime_matrix should match the fixture’s
configured CUDA major instead of using the generic target. Update the target
passed to validator.target_from_label in this test to the explicit
windows/x86_64/cuda12 label so it aligns with the release validation target and
the declared toolkit_major in the fixture.
In `@scripts/verify-native-runtime-package.sh`:
- Around line 32-41: Update python_bin() to verify that the selected interpreter
is Python 3.9 or newer before returning it, since the verifier uses newer typing
syntax and may fail under older versions. Keep the same gating logic in the
matching python_bin() helper in scripts/package-native-runtime.sh so both
scripts choose a compatible interpreter consistently, and preserve the existing
error path if no suitable Python is available.
---
Nitpick comments:
In `@crates/mesh-llm-native-runtime/src/lib.rs`:
- Around line 19-20: The crate root is being widened by re-exporting
HostGpuProbe, which unnecessarily expands the public API. Remove the new
root-level re-export from the lib.rs export list and update any internal callers
to import HostGpuProbe directly from its owning host module; if a compatibility
shim is truly needed, keep it explicitly temporary and plan its removal.
In `@RELEASE.md`:
- Around line 74-76: The release-attestation status documentation needs to cover
the third outcome reported by the verifier. Update the section describing valid
and missing statuses to also mention the invalid status from the attestation
verification flow, making clear that it applies when a footer is present but
signature verification fails. Use the existing status wording in RELEASE.md so
readers can distinguish missing from signed-but-untrusted outcomes.
- Around line 52-62: The gh secret set examples in RELEASE.md hardcode a
specific repository name, which makes the commands unsafe for forks or other
remotes. Update the documented commands to avoid embedding Mesh-LLM/mesh-llm in
the gh secret set invocations, either by removing the --repo flag so gh uses the
current authenticated repository or by clearly telling readers to replace it
with their own repository; keep the examples tied to the secret names
MESH_RELEASE_ATTESTATION_SIGNING_KEY_FILE and
MESH_RELEASE_ATTESTATION_PUBLIC_KEY_FILE.
🪄 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: be13bd4b-f81b-4ac0-9a1a-f082de78cd9e
📒 Files selected for processing (9)
.github/workflows/release.ymlRELEASE.mdcrates/mesh-llm-hardware-profile/src/lib.rscrates/mesh-llm-native-runtime/src/host.rscrates/mesh-llm-native-runtime/src/lib.rsscripts/package-native-runtime.shscripts/rc-release-smoke.shscripts/tests/test_validate_release_native_runtime_matrix.pyscripts/verify-native-runtime-package.sh
897904b to
a0c8c1b
Compare
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)
crates/mesh-llm-hardware-profile/src/lib.rs (1)
77-113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid assigning backend ordinals from sorted fallback labels.
Line 82 passes the post-
gpu_labels()enumerate index intobackend_device_from_label, butgpu_labels()sorts/dedups labels before this path, so the index is alphabetical/global rather than a CUDA/ROCm/Metal device ordinal. On mixed hosts this can synthesize the wrongROCm{index}/MTL{index}device for downstream runtime selection. Leave fallbackbackend_deviceunset unless the source carries a real ordinal.Suggested fix
fn fallback_gpu_profiles() -> Vec<HostGpuProfile> { gpu_labels() .into_iter() .enumerate() .map(|(index, label)| { - let backend_device = backend_device_from_label(&label, index); HostGpuProfile { display_name: label, - backend_device, + backend_device: None, stable_id: Some(format!("detected-{index}")), vram_bytes: None, unified_memory: cfg!(target_os = "macos"), probe: None, cuda_sm: None, @@ -fn backend_device_from_label(label: &str, index: usize) -> Option<String> { - let label = label.to_ascii_lowercase(); - if looks_like_nvidia_gpu_label(&label) { - Some(format!("CUDA{index}")) - } else if label.contains("amd") || label.contains("radeon") || label.contains("rocm") { - Some(format!("ROCm{index}")) - } else if cfg!(target_os = "macos") && label.contains("apple") { - Some(format!("MTL{index}")) - } else { - None - } -}🤖 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 `@crates/mesh-llm-hardware-profile/src/lib.rs` around lines 77 - 113, The fallback GPU profile path is incorrectly synthesizing backend ordinals from the sorted gpu_labels() index, which can produce wrong CUDA/ROCm/MTL device names on mixed systems. Update fallback_gpu_profiles() so it no longer passes the enumerate index into backend_device_from_label for ordinal assignment, and instead leaves HostGpuProfile.backend_device unset unless you have a real source ordinal to preserve. Keep the fix localized to fallback_gpu_profiles(), backend_device_from_label(), and the HostGpuProfile construction logic.
🤖 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 `@crates/mesh-llm-hardware-profile/src/lib.rs`:
- Around line 77-113: The fallback GPU profile path is incorrectly synthesizing
backend ordinals from the sorted gpu_labels() index, which can produce wrong
CUDA/ROCm/MTL device names on mixed systems. Update fallback_gpu_profiles() so
it no longer passes the enumerate index into backend_device_from_label for
ordinal assignment, and instead leaves HostGpuProfile.backend_device unset
unless you have a real source ordinal to preserve. Keep the fix localized to
fallback_gpu_profiles(), backend_device_from_label(), and the HostGpuProfile
construction logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cc018c58-476b-4d7d-82f9-2b0f3db5613c
📒 Files selected for processing (8)
.github/workflows/release.ymlRELEASE.mdcrates/mesh-llm-hardware-profile/src/lib.rscrates/mesh-llm-native-runtime/src/lib.rsscripts/package-native-runtime.shscripts/rc-release-smoke.shscripts/tests/test_validate_release_native_runtime_matrix.pyscripts/verify-native-runtime-package.sh
✅ Files skipped from review due to trivial changes (1)
- RELEASE.md
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/verify-native-runtime-package.sh
- scripts/package-native-runtime.sh
- scripts/tests/test_validate_release_native_runtime_matrix.py
- scripts/rc-release-smoke.sh
- .github/workflows/release.yml
2 participants
Summary
This PR bundles the RC5 release-readiness fixes found across the open testing threads:
runtime installtesting;doctor --jsonGPU diagnostics so detailed/procprobe data remains available without being reported as fake GPU rows.Findings From Testing Threads
mesh1 RC4 readiness thread
The earlier mesh1 audit showed that a published bundle could serve a model, but a clean
mesh-llm runtime installpath could still fail because release native-runtime artifacts were missing for the host. It also showed thatdoctor --jsonwas not enough as a readiness gate when the selected runtime could be absent or wrong.This PR carries that lesson into the release workflow by validating explicit native-runtime targets and making the smoke path check the runtime-install surface separately from bundle execution.
carrack RC5 release audit
Carrack detected
cpu, cuda, vulkan, but RC5’s publishednative-runtimes.jsononly offered an x86_64 CPU runtime for that host. The CUDA runtimes present in the manifest were Linux aarch64 targets, so they were correctly rejected asarch_mismatch; the resolver then selected CPU.This PR adds native-runtime jobs for the missing published-platform targets, including the carrack-critical Linux x86_64 CUDA 12/13 runtimes, plus Linux x86_64 ROCm/Vulkan and Windows x86_64 CPU/CUDA/ROCm/Vulkan runtimes.
release matrix / Actions artifact audit
The RC5 matrix published binary bundles for more platform/backend combinations than it published native-runtime artifacts for. The release validator did not require those runtime artifacts, so the gap reached the prerelease manifest.
This PR updates the workflow and matrix validation so
native-runtimes.jsongeneration is driven by the intended explicit target list rather than accidentally passing with only a subset of runtime artifacts.carrack doctor GPU diagnostics thread
On carrack,
doctor --jsonpreviously reported 25host.gpusrows. Many of those rows were useful raw NVIDIA probe details such as DMA masks, IRQs, BIOS versions, bus locations, and device minors, but they were incorrectly shaped as separate GPUs withdetected-*stable IDs.This PR preserves the useful probe information by nesting it under the matching real GPU entry.
host.gpusnow reports the two actual carrack devices, each with aprobeobject containing the/proc/driver/nvidia/gpus/*/informationfields and raw lines.What Changed
HostGpuProbeand nested NVIDIA/procprobe details underHostGpuProfile.Validation
Local validation run before pushing:
Carrack validation after pulling this branch:
Carrack doctor verification from
target/debug/mesh-llm doctor --json:Saved carrack artifacts:
Note: the carrack doctor run still reports
healthy: falsein this branch checkout because that local cache does not currently have a compatible native runtime selected. The GPU diagnostics shape is fixed independently and verified from the built binary.Summary by CodeRabbit