Fix crates.io publishing for new workspace crates, and make dev builds identify themselves - #1223
Conversation
The crates.io publish chain hand-maintained each crate's workspace dependencies in a case statement that nothing validated, while xtask validated only the adjacent publish_crates array. Adding skippy-tokenizer updated the checked list and not the unchecked map, so v0.75.1 published no crates: skippy-protocol's dry-run could not resolve a dependency that was not on crates.io yet. - derive workspace dependencies once from cargo metadata - add an xtask guard so a hand-maintained map cannot return - teach the publish fixture a cargo metadata subcommand - regression test covering a brand-new workspace crate Also stamp non-release Unix host builds with the commit SHA, matching build-windows.ps1. `mesh-llm --version` from main reported a bare release-shaped version, so a dev build was indistinguishable from a release binary, and is_sha_build() could not route native runtime resolution at the latest release.
📝 WalkthroughWalkthroughBuild scripts now derive non-release versions from Git state and validate release versions with Semver build metadata. Crate publishing now derives registry dependencies from ChangesBuild version derivation
Metadata-derived crate publishing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PublishScript
participant CargoMetadata
participant DependencyPairs
participant CratePublisher
PublishScript->>CargoMetadata: request workspace metadata
CargoMetadata-->>PublishScript: return package and path dependency JSON
PublishScript->>DependencyPairs: build registry dependency pairs
PublishScript->>CratePublisher: evaluate crate dependencies
CratePublisher-->>PublishScript: skip crates with unavailable registry dependencies
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tools/xtask/src/publish_consistency.rs (3)
25-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting that the lookup consumes the derived pairs.
The check confirms that
load_registry_dep_pairscallscargo metadataand that neither function has a crate-specific case branch. It does not confirm thatunpublished_registry_depsreadsregistry_dep_pairs. A regression that stubs the lookup to print nothing passes this check and silently disables the skip logic.An
ensure_contains(lookup, "registry_dep_pairs", ...)call closes that gap.Also applies to: 328-354
🤖 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 `@tools/xtask/src/publish_consistency.rs` at line 25, Update check_publish_registry_deps_are_derived to assert that unpublished_registry_deps consumes registry_dep_pairs, adding an ensure_contains check alongside the existing load_registry_dep_pairs and cargo metadata assertions. Keep the current checks unchanged while ensuring a stubbed lookup that ignores the derived pairs cannot pass.
366-375: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe case-branch detector misses common quoting forms.
The candidate must consist only of lowercase letters, digits, and hyphens. These hand-maintained branches therefore pass the check:
"mesh-llm-node")— the quotes fail the byte filter.mesh-llm-node|mesh-llm-api-client)— the|fails the byte filter.model_ref)— the required-is absent.A future author can reintroduce the exact map this check exists to reject. Trim quotes and split alternation patterns before validating.
♻️ Proposed refactor
fn hand_maintained_case_branch(function: &str) -> Option<String> { function.lines().map(str::trim).find_map(|line| { let candidate = line.strip_suffix(')')?; - let is_crate_name = !candidate.is_empty() - && candidate - .bytes() - .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'-'); - (is_crate_name && candidate.contains('-')).then(|| candidate.to_string()) + candidate.split('|').map(|pattern| pattern.trim().trim_matches('"')).find(|pattern| { + !pattern.is_empty() + && pattern.contains(['-', '_']) + && pattern.bytes().all(|byte| { + byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'-' || byte == b'_' + }) + }) + .map(str::to_string) }) }🤖 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 `@tools/xtask/src/publish_consistency.rs` around lines 366 - 375, Update hand_maintained_case_branch to normalize each candidate by trimming surrounding quotes and splitting alternation patterns on '|', then validate each resulting branch using only lowercase letters, digits, and hyphens while still requiring a hyphen. Return a matching branch so quoted and alternation forms are detected, while model_ref remains rejected.
357-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
shell_function_bodydepends on one exact declaration form.The split pattern is
"\n{name}() {". These forms returnNoneand make the check report a missing function:
function load_registry_dep_pairs() {load_registry_dep_pairs () {- the function declared on the first line of the file, because of the leading
\n.The closing split on
"\n}"also ends the body at the first}in column 0. If the embedded Python block ever starts a line with}, the body is truncated and the case-branch scan silently covers less code.🤖 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 `@tools/xtask/src/publish_consistency.rs` around lines 357 - 363, Update shell_function_body to recognize function declarations with optional function keywords, flexible whitespace, and declarations at the beginning of contents instead of relying on one exact newline-prefixed pattern. Locate the matching closing brace without terminating on a standalone brace inside embedded content, so the complete function body is returned for branch scanning.scripts/publish-crates.sh (1)
323-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider path-separator-safe directory derivation.
Line 325 splits
manifest_pathon/. Cargo emits backslash-separated paths on Windows.os.path.dirnamehandles both on the host platform and removes the manualrstrip("/")need on line 335.Also note that line 323 treats
publish = ["some-registry"]as publishable, because only[]is excluded. That is acceptable today, but it will misclassify a crate that is restricted to a private registry.♻️ Proposed refactor
+import os.path import json import sys metadata = json.load(sys.stdin) publishable = [p for p in metadata["packages"] if p.get("publish") != []] by_manifest_dir = { - p["manifest_path"].rsplit("/", 1)[0]: p["name"] for p in publishable + os.path.dirname(p["manifest_path"]): p["name"] for p in publishable } for package in publishable: for dependency in package["dependencies"]: if dependency.get("kind") == "dev": continue path = dependency.get("path") if not path: continue - name = by_manifest_dir.get(path.rstrip("/")) + name = by_manifest_dir.get(os.path.normpath(path)) if name and name != package["name"]: print(package["name"], name)🤖 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/publish-crates.sh` around lines 323 - 337, Update by_manifest_dir construction to derive each manifest’s directory with os.path.dirname rather than splitting on "/" so Windows paths are handled correctly. Adjust dependency path lookup around package["dependencies"] to use the normalized directory without manual rstrip("/") cleanup. Preserve the existing publishable filter behavior, including registry-specific publish lists.
🤖 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/build-host.sh`:
- Around line 110-118: Update the build-version flow around stamp_build_version
so it is invoked for every BUILD_PROFILE before the build proceeds. Move the
stamp call outside the BUILD_PROFILE == release condition, and retain only the
release-specific argument selection and release version handling inside that
condition.
In `@scripts/tests/test_publish_crates.py`:
- Around line 20-26: Update _publish_chain_crates to parse publish_crates
entries like tools/xtask/src/publish_consistency.rs: skip comment lines and
remove surrounding double quotes from each entry before returning crate names.
Preserve the existing array extraction and filtering behavior while ensuring
quoted shell entries match their unquoted package names.
---
Nitpick comments:
In `@scripts/publish-crates.sh`:
- Around line 323-337: Update by_manifest_dir construction to derive each
manifest’s directory with os.path.dirname rather than splitting on "/" so
Windows paths are handled correctly. Adjust dependency path lookup around
package["dependencies"] to use the normalized directory without manual
rstrip("/") cleanup. Preserve the existing publishable filter behavior,
including registry-specific publish lists.
In `@tools/xtask/src/publish_consistency.rs`:
- Line 25: Update check_publish_registry_deps_are_derived to assert that
unpublished_registry_deps consumes registry_dep_pairs, adding an ensure_contains
check alongside the existing load_registry_dep_pairs and cargo metadata
assertions. Keep the current checks unchanged while ensuring a stubbed lookup
that ignores the derived pairs cannot pass.
- Around line 366-375: Update hand_maintained_case_branch to normalize each
candidate by trimming surrounding quotes and splitting alternation patterns on
'|', then validate each resulting branch using only lowercase letters, digits,
and hyphens while still requiring a hyphen. Return a matching branch so quoted
and alternation forms are detected, while model_ref remains rejected.
- Around line 357-363: Update shell_function_body to recognize function
declarations with optional function keywords, flexible whitespace, and
declarations at the beginning of contents instead of relying on one exact
newline-prefixed pattern. Locate the matching closing brace without terminating
on a standalone brace inside embedded content, so the complete function body is
returned for branch scanning.
🪄 Autofix
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: f10c8ec3-cffa-477c-8259-69e186a2211e
📒 Files selected for processing (4)
scripts/build-host.shscripts/publish-crates.shscripts/tests/test_publish_crates.pytools/xtask/src/publish_consistency.rs
The SHA stamping added in the previous commit was unreachable: the only call to stamp_build_version sat inside the `BUILD_PROFILE == release` branch, so debug and dev builds still reported a bare, release-shaped version. Call it for every profile and keep only the --release argument selection inside the release branch. Add a build-host regression test that asserts the version cargo actually sees, so the derivation cannot become dead code again, and parse the publish_crates array in the test fixture the same way xtask does. Both raised in review of #1223.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/tests/test_build_release.py (1)
103-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a dirty-worktree regression test.
Line 112 always returns an empty
git statusresult. The helper cannot validate the required.dirtysuffix. Add a helper option that emits a non-empty status result. Assert that a non-release build reports0.68.0+gABC123.dirty.Proposed test change
-def run_build_host_with_profile(self, profile: str) -> str: +def run_build_host_with_profile(self, profile: str, *, dirty: bool = False) -> str: ... - status) ;; + status) + if [[ "${GIT_DIRTY:-0}" == "1" ]]; then + printf ' M tracked-file\n' + fi + ;; ... "MESH_LLM_SKIP_UI": "1", + "GIT_DIRTY": "1" if dirty else "0", "PATH": f"{bin_dir}{os.pathsep}{env['PATH']}", ... + def test_non_release_dirty_profile_stamps_dirty_suffix(self) -> None: + stamped = self.run_build_host_with_profile("debug", dirty=True) + self.assertEqual(stamped, "0.68.0+gABC123.dirty")🤖 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/tests/test_build_release.py` around lines 103 - 115, Extend the git stub used by write_executable with an option to return non-empty output for status, then add a dirty-worktree regression test using that option. Assert that the non-release build reports the version 0.68.0+gABC123.dirty while preserving the existing clean-status behavior.
🤖 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.
Nitpick comments:
In `@scripts/tests/test_build_release.py`:
- Around line 103-115: Extend the git stub used by write_executable with an
option to return non-empty output for status, then add a dirty-worktree
regression test using that option. Assert that the non-release build reports the
version 0.68.0+gABC123.dirty while preserving the existing clean-status
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 285ff8e8-0bc7-40c0-b761-868220bdd92f
📒 Files selected for processing (3)
scripts/build-host.shscripts/tests/test_build_release.pyscripts/tests/test_publish_crates.py
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/build-host.sh
- scripts/tests/test_publish_crates.py
Two CI failures from the previous commit. The product composer compared the full `--version` output against the runtime's release version, so a debug host stamped `0.72.1+gA2516A` was rejected as drift. Semver build metadata is not part of version identity; compare the release version and ignore any suffix. Genuine drift is still rejected, with or without a suffix. The new build-host stamping test stubbed uname as Linux but did not stub ld.lld, so it passed locally where lld is installed and failed on runners without it. Stub ld.lld like the existing host build fixture does.
Correction to the PR descriptionI claimed in Notes for review:
That was wrong, and CI caught it. I had grepped for version assertions and concluded only Fixed in edc6af4: the composer now compares the release version and ignores semver build metadata, which is not part of version identity. Genuine drift is still rejected, with or without a suffix:
Added Worth noting why this surfaced only on macOS/Linux: Windows debug hosts have carried the Second CI failure, same commitThe new build-host stamping test stubbed That is twice now that testing on my machine gave false confidence — first the extracted-function check that hid the dead call site, now this. Both new guards assert observable behaviour under CI-like conditions instead. Validation after these fixesI could not update the PR description itself — |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/tests/test_ci_artifact_actions.py (1)
1485-1494: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the unmodified host version in the mismatch test.
The test checks only
"composed host version mismatch". It would still pass if the composer dropped+gABC123from the diagnostic. Add an assertion for the complete detected version.This covers the PR objective that mismatch errors retain the unmodified detected version.
Suggested assertion
self.assertNotEqual(result.returncode, 0) self.assertIn("composed host version mismatch", result.stderr) + self.assertIn("9.9.9+gABC123", result.stderr)🤖 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/tests/test_ci_artifact_actions.py` around lines 1485 - 1494, Update test_product_composer_rejects_drift_despite_build_metadata to assert that result.stderr includes the complete unmodified detected host version, including “9.9.9+gABC123”, in addition to the existing mismatch message assertion.
🤖 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.
Nitpick comments:
In `@scripts/tests/test_ci_artifact_actions.py`:
- Around line 1485-1494: Update
test_product_composer_rejects_drift_despite_build_metadata to assert that
result.stderr includes the complete unmodified detected host version, including
“9.9.9+gABC123”, in addition to the existing mismatch message assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bcfdc65-59ba-45e5-afc7-5dd8b6843061
📒 Files selected for processing (3)
scripts/ci-compose-product-input.shscripts/tests/test_build_release.pyscripts/tests/test_ci_artifact_actions.py
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/tests/test_build_release.py
|
Thx for the fix! |
3 participants
What this fixes for you
v0.75.1 published no crates to crates.io. This makes the publish chain resilient to new workspace crates, and makes a dev build's
--versiontell you the truth.Two changes, one shared cause: a fact Cargo already knows was copied by hand, and nothing checked the copy.
The crates.io failure
scripts/publish-crates.shhand-maintained every crate's workspace dependencies in a ~160-linecasestatement.#1214addedskippy-tokenizer,skippy-protocoldepends on it, and the map had noskippy-protocolbranch — so its dry-run was not skipped, andcargo publishcould not resolve a dependency that is not on crates.io yet:This was structurally guaranteed to happen eventually. xtask validated the adjacent
publish_cratesarray (order, duplicates, completeness) but never read the dependency map — so the checked list and the unchecked map drifted apart, and the failure only surfaced at the end of a 2h40m release.The map is now derived from
cargo metadata, which found three edges the hand-written version had missed:skippy-protocolskippy-tokenizer← broke v0.75.1skippy-servermesh-native-serving-plugin-api,model-artifactmesh-llm-host-runtimeskippy-ffiVerified against all 46 crates: zero cases where the derivation lost a dependency the old map had.
An xtask guard now rejects a reintroduced hand-maintained branch, so this class of failure cannot come back.
Dev builds now identify themselves
mesh-llm --versionfrommainreported0.72.1— a bare, release-shaped version. That makes a dev build indistinguishable from a release binary, which directly undermines theAGENTS.mddeploy checklist ("verifymesh-llm --versionon every node" to confirm a binary is new code).build-windows.ps1already did this correctly.build-host.sh— the Unix path — had no SHA logic at all. Non-release Unix builds now match Windows:--version0.75.10.72.1+g1A2FEF(.dirtywhen the tree is dirty)This also feeds existing behaviour:
is_sha_build()already routes native-runtime resolution toreleases/latestfor SHA builds instead of a pinned tag, so a dev build stops asking for a runtime tagged with a version that was never released.Notes for review
registry_dep_pairsrather than shelling out per crate — 1cargo metadatacall, not 46.profile: debug, so their host--versionnow carries a SHA suffix.scripts/ci-compose-product-input.shcompared the full--versionoutput against the runtime's release version and rejected the suffix as drift. It now compares the release version and ignores semver build metadata, which is not part of version identity. Genuine drift is still rejected, with or without a suffix.package-release.ps1asserts--versionmatches the release tag exactly. It is invoked only fromrelease.ymlon release profiles, which are unstamped, so the suffix cannot reach it.Follow-ups intentionally not in this PR, tracked in the linked issue:
main's stale workspace version,known_mesh_llm_versions(), andrelease-version.shrewriting user-facing docs to a dev version.Validation
Both guards were verified to actually fail, not just pass:
cargo metadataderivation removed → rejectedAnd the regression test was run against the old hand-maintained map to confirm it reproduces the v0.75.1 failure, then against the fix to confirm it passes.
Version stamping exercised directly for all three paths: dev →
0.72.1+g1A2FEF.dirty, release →0.72.1, presetMESH_LLM_BUILD_VERSION→ respected.Closes part of #1224.