fix(version): show published Cargo sources without the dev marker - #5899
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_1835f992-8df8-4543-96d3-22a32028d546) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @Hmbown's task in 1m 37s —— View job Claude's review of #5899
SummaryThis is a small, well-scoped fix: Correctness / safety: No bugs found. The one thing worth a closer look was Provider/model routing: Not applicable — this PR only touches build-script version stamping. Scope: Change is minimal, matches the stated intent (no VCS reads, no new provenance source, Not verified in this session: I wasn't able to execute Branch: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| packaged_source: bool, | ||
| ) -> String { | ||
| match commit { | ||
| Some(sha) => format!("{package_version} ({})", &sha[..12]), |
There was a problem hiding this comment.
Minor reuse note: this reimplements the truncation that short_sha (below, line 161) already does — and does it less defensively. short_sha truncates via .chars().take(12).collect(), which cannot panic regardless of input length; &sha[..12] panics if sha.len() < 12.
It's not reachable today — commit only ever holds a full_sha-validated 40-char hex string (see build_commit_with/full_sha), so the slice is always in bounds. But format_build_version doesn't itself enforce that invariant, so the safety is only true by virtue of its one caller. Reusing short_sha(sha.to_string()) here would keep the guarantee local to the function instead of relying on the caller, per the "already in this codebase → reuse it" rule.
| Some(sha) => format!("{package_version} ({})", &sha[..12]), | |
| Some(sha) => match short_sha(sha.to_string()) { | |
| Some(short) => format!("{package_version} ({short})"), | |
| None => format!("{package_version} (dev)"), | |
| }, |
(would need short_sha in scope, e.g. use super::short_sha isn't needed since it's the same module — just call short_sha(...) directly.)
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to build-time version string formatting and includes a targeted unit test covering the new packaging vs. checkout distinction.
Pull request overview
This PR adjusts build-time version string formatting so that unstamped builds from Cargo’s normalized source packages (e.g., crates.io / cargo install) display the plain package version (no misleading (dev)), while unstamped local checkouts still show (dev) and stamped builds continue to show the short SHA.
Changes:
- Detect Cargo’s normalized source layout via
Cargo.toml.origand use that to decide when to omit(dev)for unstamped builds. - Refactor version string construction into a dedicated
format_build_versionhelper and add a regression test for the three cases (stamped / packaged-unstamped / checkout-unstamped).
File summaries
| File | Description |
|---|---|
| crates/build-support/src/lib.rs | Adds Cargo source-package detection and refactors build-version formatting + tests to avoid showing (dev) for published sources. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| match commit { | ||
| Some(sha) => format!("{package_version} ({})", &sha[..12]), | ||
| None if packaged_source => package_version.to_string(), | ||
| None => format!("{package_version} (dev)"), | ||
| } |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_cad45d39-a703-45bd-880d-7b9492e2059d) |
|
Claude finished @Hmbown's task in 49s —— View job Claude's review of #5899 (updated after new commit)
SummaryThe new commit fn format_build_version(
package_version: &str,
commit: Option<&str>,
packaged_source: bool,
) -> String {
match commit.and_then(|sha| short_sha(sha.to_string())) {
Some(sha) => format!("{package_version} ({sha})"),
None if packaged_source => package_version.to_string(),
None => format!("{package_version} (dev)"),
}
}This is a strict improvement: Correctness: No bugs found in the current state. The three-way branch (stamped / packaged-unstamped / checkout-unstamped) is unambiguous and the new unit test Provider/model routing: Not applicable — build-script version stamping only, no model/provider logic touched. Reuse/simplification: The refactor is exactly the right kind of simplification — one location ( Scope: Still minimal and matches stated intent — no VCS reads added, no new Not independently re-verified in this session: cargo test/clippy invocations are not permitted in this sandbox (same limitation as the prior review pass), so I relied on static reading of the diff plus the PR's reported "5 passed, 0 failed" test run. The logic change is small and low-risk enough that static review gives good confidence here. No further findings — this looks ready as-is. Branch: |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Updates build-time version formatting so unstamped Cargo source packages show the plain package version instead of a misleading (dev) marker, while checkouts and stamped builds retain prior behavior. The change is low-risk and focused on build-support formatting.
Findings
- [INFO] Packaged-source detection is not covered by an emit_build_version test (
crates/build-support/src/lib.rs:173)
The new behavior depends onmanifest_dir.join("Cargo.toml.orig").is_file(), but the added test only callsformat_build_versiondirectly. The path construction and emittedcargo:rustc-envlines for packaged versus unpackaged layouts are exactly the PR's behavioral change and remain unprotected by an automated unit test.
Assessment
Low-risk build-time formatting change with no correctness blockers found. The main gap is automated coverage for the filesystem-based detection path added in emit_build_version.
Advisory review by Codewhale (codewhale review --pr 5899 --post, head 26d0f5d9d7ee2e00ebb24bc073fa8c799dba5e08). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| mod tests { | ||
| use super::{full_sha, release_build_sha, short_sha}; | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
[INFO] Packaged-source detection is not covered by an emit_build_version test
The new behavior depends on manifest_dir.join("Cargo.toml.orig").is_file(), but the added test only calls format_build_version directly. The path construction and emitted cargo:rustc-env lines for packaged versus unpackaged layouts are exactly the PR's behavioral change and remain unprotected by an automated unit test.
26d0f5d to
e3db79c
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_43a076cf-25ba-4f5f-98b8-ed6d3dd95f53) |
|
Claude finished @Hmbown's task in 1m 0s —— View job Claude's review of #5899 (final pass)
SummaryReviewed the full diff at Correctness: No bugs. Verified the
Provider/model routing: N/A — build-script version stamping only, no model/provider code touched. Reuse/simplification: The Tests: The unit test ( Scope: Matches stated intent — no VCS reads added, no new No blocking issues. This looks ready to merge as-is. |
There was a problem hiding this comment.
Codewhale review
The PR correctly stops unstamped Cargo source packages from reporting a (dev) marker and keeps stamped/local checkout behavior intact. The formatting helper is unit tested, but the build-script rerun conditions do not account for the new Cargo.toml.orig input, and the filesystem classification is not covered by automated tests.
Findings
- [WARNING] Build script does not rerun when Cargo.toml.orig changes (
crates/build-support/src/lib.rs:62)
emit_build_version now changes its output based on manifest_dir.join("Cargo.toml.orig").is_file(), but declare_rerun_conditions(_manifest_dir) still ignores manifest_dir and only emits environment-based rerun conditions. In incremental builds where the packaged/unpackaged layout changes without an environment variable change, CODEWHALE_BUILD_VERSION can remain stale. Add a cargo:rerun-if-changed directive for the marker file or update declare_rerun_conditions to use manifest_dir. - [INFO] Packaged-source filesystem detection is not covered by automated tests (
crates/build-support/src/lib.rs:62)
The new tests exercise format_build_version directly, but the crucial Cargo.toml.orig presence check in emit_build_version is only validated by a manual fixture. Consider extracting the path check into a small helper and unit-testing both true and false cases, or adding a tempdir integration test, so the crates.io/checkout distinction cannot regress.
Suggestions
-
crates/build-support/src/lib.rs:58— Declare the marker file as an input so Cargo reruns the build script when the packaged/unpackaged source layout changes.println!("cargo:rerun-if-changed=Cargo.toml.orig"); let commit = build_commit();
Assessment
The change is low risk and fixes the reported version marker problem. The formatting logic is sound and tests cover the helper, but the build script should be made rerun-sensitive to Cargo.toml.orig and the detection path deserves automated coverage.
Advisory review by Codewhale (codewhale review --pr 5899 --post, head e3db79c031a3339b93bcbc83e88c87cb3ed1a888). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| let build_version = format_build_version( | ||
| package_version, | ||
| commit.as_deref(), | ||
| manifest_dir.join("Cargo.toml.orig").is_file(), |
There was a problem hiding this comment.
[WARNING] Build script does not rerun when Cargo.toml.orig changes
emit_build_version now changes its output based on manifest_dir.join("Cargo.toml.orig").is_file(), but declare_rerun_conditions(_manifest_dir) still ignores manifest_dir and only emits environment-based rerun conditions. In incremental builds where the packaged/unpackaged layout changes without an environment variable change, CODEWHALE_BUILD_VERSION can remain stale. Add a cargo:rerun-if-changed directive for the marker file or update declare_rerun_conditions to use manifest_dir.
| let build_version = format_build_version( | ||
| package_version, | ||
| commit.as_deref(), | ||
| manifest_dir.join("Cargo.toml.orig").is_file(), |
There was a problem hiding this comment.
[INFO] Packaged-source filesystem detection is not covered by automated tests
The new tests exercise format_build_version directly, but the crucial Cargo.toml.orig presence check in emit_build_version is only validated by a manual fixture. Consider extracting the path check into a small helper and unit-testing both true and false cases, or adding a tempdir integration test, so the crates.io/checkout distinction cannot regress.
| /// Its presence classifies the source layout, not release provenance: no VCS | ||
| /// metadata is read and no additional commit value is emitted. | ||
| pub fn emit_build_version(manifest_dir: &Path, package_version: &str) { | ||
| let commit = build_commit(); |
There was a problem hiding this comment.
Declare the marker file as an input so Cargo reruns the build script when the packaged/unpackaged source layout changes.
| let commit = build_commit(); | |
| println!("cargo:rerun-if-changed=Cargo.toml.orig"); | |
| let commit = build_commit(); |
Unstamped Cargo source packages show the package version, while unpackaged checkouts retain (dev) and explicit build SHA stamps retain precedence. No Git metadata is read or added to telemetry. Validation: 5/5 build-support tests passed; all-target clippy -D warnings, fmt, and diff checks passed. Real offline Cargo package/install fixture verified packaged, development, and stamped outputs and absent provenance for unstamped sources. Runtime root has no npm test/check:web scripts. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Use short_sha instead of byte slicing, retaining total behavior if future callers provide a shorter string. Validation: 5/5 build-support tests and all-target clippy -D warnings pass; formatting and diff checks pass. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
e3db79c to
de1a531
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ea8fd652-2abf-4362-b759-19ad1367ec5e) |
|
Claude finished @Hmbown's task in 1m 27s —— View job Review of #5899
SummaryCurrent head ( 1. Correctness: No bugs in the current diff. 2. Provider/model routing: N/A — build-script version stamping only. 3. Reuse/simplification: Good — one truncation implementation ( 4. One real, low-severity gap — confirming the automated Codewhale bot's WARNING is correct, not just plausible: 5. Tests: The pure 6. Security: No secret handling or exec paths touched; nothing to flag. AssessmentReady to merge as-is on correctness grounds — no panics, no scope creep, |
There was a problem hiding this comment.
Codewhale review
PR updates build-support version formatting so crates.io-style packaged Cargo sources without CI SHA print the plain package version instead of (dev), while local dev checkouts and stamped builds keep previous behavior. The helper is split into a testable format_build_version and uses Cargo.toml.orig presence as the packaged-source signal.
Findings
- [WARNING] Build script rerun conditions do not account for Cargo.toml.orig (
crates/build-support/src/lib.rs:62)
format_build_version now depends on whether manifest_dir/Cargo.toml.orig exists, but the build script's declared rerun conditions only cover the stamping environment variables. In a reused target directory where a source tree gains or loses Cargo.toml.orig, Cargo may not rerun the build script and can keep a stale CODEWHALE_BUILD_VERSION. Fresh Cargo package builds are unaffected, which limits impact, but the new file dependency should be declared. - [WARNING] Invalid stamping SHA now yields plain version for packaged sources (
crates/build-support/src/lib.rs:81)
format_build_version treats any short_sha failure as an unstamped build. If CODEWHALE_BUILD_SHA/GITHUB_SHA is set but is not a valid 40-hex value, packaged sources now emit the plain package version, whereas the previous fallback was (dev). That can hide a misconfigured stamped build. Consider preserving the dev marker for invalid values or adding a test to lock in the desired behavior. - [INFO] Cargo.toml.orig detection lacks automated coverage (
crates/build-support/src/lib.rs:62)
The new tests exercise format_build_version directly, but no unit test creates a temporary Cargo.toml.orig next to a manifest to verify emit_build_version packaged/unpackaged classification. The PR mentions an offline fixture, but that is not part of the automated test suite and could let the filesystem branch regress.
Assessment
The change is small, focused, and likely correct for the reported crates.io install case. Before merging, I would address the build-script rerun dependency on Cargo.toml.orig and add coverage for invalid SHA input and the filesystem classification path.
Advisory review by Codewhale (codewhale review --pr 5899 --post, head de1a53127feae47a1858ea5714a023dc68208e61). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| let build_version = format_build_version( | ||
| package_version, | ||
| commit.as_deref(), | ||
| manifest_dir.join("Cargo.toml.orig").is_file(), |
There was a problem hiding this comment.
[WARNING] Build script rerun conditions do not account for Cargo.toml.orig
format_build_version now depends on whether manifest_dir/Cargo.toml.orig exists, but the build script's declared rerun conditions only cover the stamping environment variables. In a reused target directory where a source tree gains or loses Cargo.toml.orig, Cargo may not rerun the build script and can keep a stale CODEWHALE_BUILD_VERSION. Fresh Cargo package builds are unaffected, which limits impact, but the new file dependency should be declared.
| ) -> String { | ||
| match commit.and_then(|sha| short_sha(sha.to_string())) { | ||
| Some(sha) => format!("{package_version} ({sha})"), | ||
| None if packaged_source => package_version.to_string(), |
There was a problem hiding this comment.
[WARNING] Invalid stamping SHA now yields plain version for packaged sources
format_build_version treats any short_sha failure as an unstamped build. If CODEWHALE_BUILD_SHA/GITHUB_SHA is set but is not a valid 40-hex value, packaged sources now emit the plain package version, whereas the previous fallback was (dev). That can hide a misconfigured stamped build. Consider preserving the dev marker for invalid values or adding a test to lock in the desired behavior.
| let build_version = format_build_version( | ||
| package_version, | ||
| commit.as_deref(), | ||
| manifest_dir.join("Cargo.toml.orig").is_file(), |
There was a problem hiding this comment.
[INFO] Cargo.toml.orig detection lacks automated coverage
The new tests exercise format_build_version directly, but no unit test creates a temporary Cargo.toml.orig next to a manifest to verify emit_build_version packaged/unpackaged classification. The PR mentions an offline fixture, but that is not part of the automated test suite and could let the filesystem branch regress.
A successful crates.io install currently reports
codewhale 0.9.12 (dev), making the published source package look like an unreleased checkout. Unstamped Cargo source packages now show their package version without the development marker. Unpackaged development checkouts retain(dev), and explicitly stamped builds retain their SHA.The shared build helper recognizes Cargo's normalized source layout through
Cargo.toml.orig. This is not a release-authenticity check: it reads no VCS hash, adds no Git watcher, and leaves release/telemetry provenance absent unless the build environment supplied it.Validation:
0.9.13from the unpacked tarball, with both commit and release provenance absent.0.9.13 (dev); an explicitly stamped packaged fixture retained its SHA and provenance.npm testorcheck:webscripts.Fixes #5891.
Note
Low Risk
Build-time version string formatting only; stamped CI/release and local
(dev)behavior for git checkouts are unchanged, with a narrow new branch for Cargo-packaged sources.Overview
Fixes crates.io-style installs reporting
(dev)when the build was never stamped with a CI SHA. Unstamped builds from Cargo’s normalized source tree (detected viaCargo.toml.orignext to the manifest) now embedCODEWHALE_BUILD_VERSIONas the plain package version; local unpackaged checkouts still get(dev), and env-stamped builds still get the short SHA suffix.emit_build_versiondelegates to a newformat_build_versionhelper instead of defaulting unstamped builds to(dev)unconditionally. Docs clarify that this layout check is not release provenance—no git watchers and no extra commit metadata unless the environment supplies a SHA. Tests cover packaged vs dev vs stamped version strings and that packaging alone does not invent release telemetry SHA.Reviewed by Cursor Bugbot for commit de1a531. Bugbot is set up for automated code reviews on this repo. Configure here.