Skip to content

fix(cargo_provider): support workspace virtual manifests - #1733

Draft
jcfangc wants to merge 5 commits into
commitizen-tools:masterfrom
jcfangc:fix/cargo-workspace-virtual-manifest
Draft

fix(cargo_provider): support workspace virtual manifests#1733
jcfangc wants to merge 5 commits into
commitizen-tools:masterfrom
jcfangc:fix/cargo-workspace-virtual-manifest

Conversation

@jcfangc

@jcfangcjcfangc commented Dec 23, 2025

Copy link
Copy Markdown

Description

Fix cargo_provider so cz bump works with Cargo workspace virtual manifests (root Cargo.toml containing [workspace] / [workspace.package] but no [package]) when workspace members inherit the version via package.version.workspace = true.

Previously, cz bump could crash in commitizen/providers/cargo_provider.py with:

  • NonExistentKey: Key "package" does not exist (virtual manifest has no [package])
  • TypeError: expected root version to be a string / unsafe assumptions around package.version shape

This change makes the provider:

  • Read/write the version from [workspace.package].version when present (fallback to [package].version for non-workspace manifests).

  • When updating Cargo.lock:

    • For non-workspace manifests, update the root package entry.
    • For workspace virtual manifests, scan workspace.members (respecting exclude) and update lock entries for crates that declare package.version.workspace = true.

Fixes#1732


Checklist

  • I have read the contributing guidelines
  • Added test coverage for the introduced behavior and edge cases
  • Ran the test suite locally
  • Manually tested the behavior (workspace virtual manifest + non-workspace crate)
  • Updated documentation (not needed for this change)

Note: the project uses uv / poe tasks (not Poetry). I ran the equivalent poe / uv-based test commands.


Expected Behavior

cz bump with version_provider = "cargo" should succeed for Cargo workspaces using a virtual manifest and centralized versioning in [workspace.package].version, without crashing. The workspace version should be updated, and Cargo.lock entries for crates inheriting the workspace version (package.version.workspace = true) should be updated safely.

Non-workspace projects (root [package]) keep existing behavior.


Steps to Test This Pull Request

Manual test (workspace virtual manifest)

  1. Create a workspace root Cargo.toml:

    [workspace]
    members = ["crates/*"]
    [workspace.package]
    version = "1.0.0"edition = "2024"rust-version = "1.92"repository = "https://example.com/repo.git"
  2. Add a member crate crates/common-core/Cargo.toml:

    [package]
    name = "common-core"version.workspace = trueedition.workspace = truerust-version.workspace = truerepository.workspace = true
  3. Generate Cargo.lock once:

    cargo build
  4. Configure Commitizen:

    [tool.commitizen]
    name = "cz_conventional_commits"tag_format = "v$version"version_scheme = "semver"version_provider = "cargo"update_changelog_on_bump = trueversion_files = ["Cargo.toml:workspace.package.version"]
  5. Run:

    cz bump --yes
  6. Verify:

    • [workspace.package].version is updated (e.g., 1.0.0 -> 1.1.0)
    • Cargo.lock entries for workspace crates inheriting the version are updated
    • no exceptions are raised

Manual test (non-workspace manifest)

  1. Use a standard crate with root [package] and an existing Cargo.lock.
  2. Run cz bump --yes and verify the root package version in Cargo.lock is updated as before.

Additional Context

@bearomorphism

Copy link
Copy Markdown
Collaborator

Hi @jcfangc ,
Thank you for your contribution! Please follow the contribution guideline. You forget to run poetry format on the latest iteration.

@jcfangc

Copy link
Copy Markdown
Author

Hi @jcfangc , Thank you for your contribution! Please follow the contribution guideline. You forget to run poetry format on the latest iteration.

Sorry about that—I’ll run poetry format and push an update asap.

@codecov

codecovBot commented Jan 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.01%. Comparing base (26e5d80) to head (6b955fc).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@ Coverage Diff @@## master #1733 +/- ##
==========================================
+ Coverage 97.98% 98.01% +0.02% 
==========================================
Files 60 60 Lines 2686 2714 +28 ==========================================
+ Hits 2632 2660 +28 
Misses 54 54 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jcfangc

Copy link
Copy Markdown
Author

When I first pulled the branch locally, I didn’t have uv.lock in the working tree, and poetry format isn’t a Poetry subcommand in my setup (Poetry 1.1.12 reports: “The command 'format' is not defined”).

Following the repo’s uv/poe workflow, I synced dependencies (needed uv sync --all-groups to make ruff available), then ran uv run poe format, and pushed the formatting update.

Just to clarify / FYI.

@noirbizarre

Copy link
Copy Markdown
Member

I think it would be easier and cleaner to just rebase/squash on the current master.
You are not changing dependencies, so your PR should not touch the uv.lock file.
To avoid this side effect, run uv sync --frozen --all-groups instead of just uv sync --all-groups (uv default behavior is to check and update the lock).
Also, you might want to update your uv version, it is trying to downgrade the lock version so it might be outdated.

@jcfangc
jcfangcforce-pushed the fix/cargo-workspace-virtual-manifest branch from a933a55 to c932fc0CompareJanuary 5, 2026 01:42
@jcfangc

Copy link
Copy Markdown
Author

I think it would be easier and cleaner to just rebase/squash on the current master. You are not changing dependencies, so your PR should not touch the uv.lock file. To avoid this side effect, run uv sync --frozen --all-groups instead of just uv sync --all-groups (uv default behavior is to check and update the lock). Also, you might want to update your uv version, it is trying to downgrade the lock version so it might be outdated.

Thanks for the feedback.

I’ve rebased/squashed the branch on top of the current master and ensured the PR no longer touches lockfiles:

  • reverted/restored uv.lock and poetry.lock to match master (no lockfile changes in the PR diff now)

Going forward I’ll use uv sync --frozen --all-groups to avoid accidental lock updates, and I’ll also update my local uv to prevent lock format/version downgrades.

All checks are now green.

@woile

woile commented Jan 5, 2026

Copy link
Copy Markdown
Member

The rebase did not succeed, only your commits should appear on "Commits".

On top of that, I wonder if #1715 (in-progress) addresses this issue, can you take a look? It also changes the read order, if there are no complaints about that I was planning on merging it

@jcfangc

Copy link
Copy Markdown
Author

The rebase did not succeed, only your commits should appear on "Commits".

On top of that, I wonder if #1715 (in-progress) addresses this issue, can you take a look? It also changes the read order, if there are no complaints about that I was planning on merging it

Hi @woile, thanks for flagging #1715 — they are related but not the same.

  • fix: workspace member with fixed version #1715 focuses on two smaller cargo-provider behaviors:

    1. ensuring we use the same precedence/order when reading vs writing the version between [package] and workspace metadata, and
    2. clarifying/fixing the “fixed version for a workspace member” case: there is no workspace.version = "x.y.z" in Cargo spec; a member crate must use version = "x.y.z" even if it’s part of a workspace.
  • fix(cargo_provider): support workspace virtual manifests #1733 targets a broader and different failure mode: workspace virtual manifests (root Cargo.toml with [workspace] / [workspace.package] but no [package]). It:

    1. prevents crashes when [package] is missing / when version.workspace shape appears,
    2. reads/writes version from [workspace.package].version when present (fallback to [package].version for non-workspace manifests), and
    3. updates Cargo.lock appropriately for virtual manifests by scanning workspace.members and updating lock entries for crates with package.version.workspace = true.

So #1715 does not address the virtual-manifest crash nor the lockfile update logic; #1733 does. The overlap is mostly around “precedence when both [package] and [workspace.package] exist”, which is a policy choice.

If you’d like to merge #1715 first, I can rebase #1733 on top of it and drop any overlapping hunks/tests. Alternatively, I can incorporate the #1715 precedence decision into #1733 so we end up with a single coherent behavior.

@jcfangc
jcfangcforce-pushed the fix/cargo-workspace-virtual-manifest branch from d99d5af to 6bea1b9CompareJanuary 5, 2026 09:40
@jcfangc

jcfangc commented Jan 5, 2026

Copy link
Copy Markdown
Author

The rebase did not succeed, only your commits should appear on "Commits".

On top of that, I wonder if #1715 (in-progress) addresses this issue, can you take a look? It also changes the read order, if there are no complaints about that I was planning on merging it

I rebuilt the branch on top of current upstream/master by cherry-picking only my 4 commits (no merge commits).
The PR no longer touches any lockfiles (uv.lock / poetry.lock).

Could you please take another look? @woile

@bearomorphismbearomorphism left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! Here are some feedbacks:

Comment threadcommitizen/providers/cargo_provider.py Outdated
Comment threadcommitizen/providers/cargo_provider.py Outdated
Comment threadcommitizen/providers/cargo_provider.py Outdated
Comment threadcommitizen/providers/cargo_provider.py Outdated
Comment on lines +33 to +37
t = _root_version_table(document)
v = t.get("version")
if not isinstance(v, str):
raise TypeError("expected root version to be a string")
return v

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use more meaningful variable names. Do not use t or v.

Comment threadcommitizen/providers/cargo_provider.py Outdated
@jcfangc
jcfangcforce-pushed the fix/cargo-workspace-virtual-manifest branch from 07c23f5 to 3d11e37CompareJanuary 6, 2026 01:39
@jcfangc
jcfangcforce-pushed the fix/cargo-workspace-virtual-manifest branch 2 times, most recently from 8bad439 to df118dcCompareJanuary 7, 2026 01:19
@jcfangc

Copy link
Copy Markdown
Author

Hi @bearomorphism@woile@noirbizarre@Lee-W — I’ve addressed the requested changes and force-pushed updates.

  • Fixed the review comments in cargo_provider.py, hardened parsing/lock updates, removed an unused type: ignore, and ran the formatter.
  • No uv.lock / poetry.lock changes; all checks are green.

Could you please re-review when you have a moment? Thanks.

@jcfangc
jcfangcforce-pushed the fix/cargo-workspace-virtual-manifest branch 3 times, most recently from 5327c04 to 0eead00CompareJanuary 14, 2026 01:27
- prefer [workspace.package] version with safe fallback to [package]
- handle virtual workspace + version.workspace=true members when updating Cargo.lock
- add regression tests for error/continue branches and workspace edge cases
@jcfangc
jcfangcforce-pushed the fix/cargo-workspace-virtual-manifest branch from 0eead00 to 6b955fcCompareFebruary 2, 2026 09:08

@noirbizarrenoirbizarre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fix the naming concerns (t, v...) which makes maintenance harder when you hop from review to review and then it's good to merge (to me)

@Lee-WLee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few nits, but high-level idea looks good

class CargoProvider(TomlProvider):
"""
Cargo version management
DictLike = dict[str, Any]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this 🤔 As it's actually a dict

Comment on lines +33 to +37
t = _root_version_table(document)
v = t.get("version")
if not isinstance(v, str):
raise TypeError("expected root version to be a string")
return v

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
t=_root_version_table(document)
v=t.get("version")
ifnotisinstance(v, str):
raiseTypeError("expected root version to be a string")
returnv
table=_root_version_table(document)
version=table.get("version")
ifnotisinstance(version, str):
raiseTypeError("expected root version to be a string")
returnversion

return cast("DictLike", value) if hasattr(value, "get") else None


def _root_version_table(doc: TOMLDocument) -> DictLike:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def_root_version_table(doc: TOMLDocument) ->DictLike:
def_extract_root_version_table(doc: TOMLDocument) ->DictLike:

Comment on lines +55 to +56
root_pkg = _table_get(cargo_toml_content, "package")
if root_pkg is not None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
root_pkg=_table_get(cargo_toml_content, "package")
ifroot_pkgisnotNone:
if (root_pkg:=_table_get(cargo_toml_content, "package")) isnotNone:

return inheriting_member_names


def _lock_set_versions(packages: Any, package_names: set[str], version: str) -> None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def_lock_set_versions(packages: Any, package_names: set[str], version: str) ->None:
def_set_lock_versions(packages: Any, package_names: set[str], version: str) ->None:

Comment on lines +104 to +105
workspace_table = _table_get(doc, "workspace")
if workspace_table is not None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
workspace_table=_table_get(doc, "workspace")
ifworkspace_tableisnotNone:
if (workspace_table:=_table_get(doc, "workspace")) isnotNone:

Comment on lines +110 to +111
package_table = _table_get(doc, "package")
if package_table is None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
package_table=_table_get(doc, "package")
ifpackage_tableisNone:
if (package_table:=_table_get(doc, "package")) isNone:

Comment on lines +116 to +117
def _is_workspace_inherited_version(v: Any) -> bool:
return hasattr(v, "get") and cast("DictLike", v).get("workspace") is True

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def_is_workspace_inherited_version(v: Any) ->bool:
returnhasattr(v, "get") andcast("DictLike", v).get("workspace") isTrue
def_is_workspace_inherited_version(version_table: Any) ->bool:
returnhasattr(version_table, "get") andcast("DictLike", version_table).get("workspace") isTrue

yield Path(match)


def _workspace_inheriting_member_names(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def_workspace_inheriting_member_names(
def_collect_workspace_version_inheriting_members(

@bearomorphism

Copy link
Copy Markdown
Collaborator

conflict. converting this PR to draft

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cz bump crashes with version_provider=cargo on workspace virtual manifest (missing [package] / TypeError on version.workspace)

5 participants

@jcfangc@bearomorphism@noirbizarre@woile@Lee-W