Single source of truth for rebuild_lockfiles resolution (#106) - #116
Merged
Conversation
cli.bump duplicated the None-coalescing of rebuild_lockfiles against
configuration.bump.rebuild_lockfiles that
bump._initialize_bump_context already performs, giving two sources of
truth for the same decision.
Forward the raw nullable flag from the CLI and let the command own the
resolution. Note that the Cyclopts TOML loader may still hydrate the
CLI flag from lading.toml before dispatch; cli.bump itself performs no
coalescing.
Add a parametrised matrix test (flag in {None, True, False} x
configuration in {True, False}) exercising the resolution through the
command layer, update the CLI test to assert raw forwarding, and
record the option-defaulting convention in the developers' guide.
Closes #106
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
This PR implements a single source of truth for resolving the nullable rebuild_lockfiles option (closes Changes
Validation
Notes
WalkthroughForward the raw nullable rebuild_lockfiles flag from the CLI into BumpOptions; resolve and log the effective value only inside bump._initialize_bump_context. Update docs and add tests (unit, integration, Hypothesis) plus snapshots to assert and observe lockfile-regeneration behaviour. ChangesCentralise rebuild_lockfiles resolution to command layer
Sequence DiagramsequenceDiagram
participant CLI as lading.cli
participant BumpInit as lading.commands.bump._initialize_bump_context
participant Config as configuration.bump
CLI->>BumpInit: forward rebuild_lockfiles (bool | None)
BumpInit->>Config: consult configuration.bump.rebuild_lockfiles when flag is None
BumpInit->>BumpInit: resolve effective rebuild_lockfiles (bool)
BumpInit->>bump.bump_lockfiles: call regenerate_lockfiles when resolved True
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideCentralizes resolution of the nullable rebuild_lockfiles option in the bump command layer by having the CLI forward the raw flag value, updates tests to cover the new resolution behavior, and documents the responsibility split between the CLI adapter and command layer. Sequence diagram for rebuild_lockfiles resolution in bump commandsequenceDiagram
actor User
participant cli_bump as cli.bump
participant bump_cmd as bump._initialize_bump_context
participant Config as configuration.bump
User->>cli_bump: bump(..., rebuild_lockfiles)
cli_bump->>bump_cmd: BumpOptions(rebuild_lockfiles, configuration)
alt [rebuild_lockfiles is None]
bump_cmd->>Config: read rebuild_lockfiles
bump_cmd-->>bump_cmd: use Config.rebuild_lockfiles
else [rebuild_lockfiles is not None]
bump_cmd-->>bump_cmd: use BumpOptions.rebuild_lockfiles
end
Flow diagram for rebuild_lockfiles defaulting logicflowchart TD
A[Start bump._initialize_bump_context] --> B[Read BumpOptions.rebuild_lockfiles]
B --> C{rebuild_lockfiles is None?}
C -- Yes --> D[resolved = configuration.bump.rebuild_lockfiles]
C -- No --> E[resolved = BumpOptions.rebuild_lockfiles]
D --> F[Proceed with resolved value]
E --> F[Proceed with resolved value]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
June 10, 2026 12:19
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Update the cli module docstring to describe the current forwarding semantics: bump forwards rebuild_lockfiles as bool | None and the command layer (_initialize_bump_context) owns coalescing against the configuration default. Add a CLI test case asserting a Cyclopts-hydrated True is passed through unchanged, matching the existing hydrated-false case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai Have the following now been resolved? lading/cli.py (1)
the only coalescing against configuration defaults. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
Owner
Author
|
@coderabbitai Has this now been resolved in the latest commit? Use codegraph analysis to determine your answer. If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
head -30 lading/cli.pyLength of output: 1261
That covers every requirement: the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n "rebuild_lockfiles" tests/unit/test_cli.py -A 3 -B 3Length of output: 1476 🏁 Script executed: #!/bin/bash
# Find the review comment to resolve
gh api graphql -f query='
{
repository(owner: "leynos", name: "lading") {
pullRequest(number: 116) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
path
line
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].author.login == "coderabbitai" and (.comments.nodes[0].path == "tests/unit/test_cli.py")) | {id, isResolved, path: .comments.nodes[0].path, line: .comments.nodes[0].line, body: (.comments.nodes[0].body | .[0:100])}'Length of output: 978 🏁 Script executed: #!/bin/bash
gh api graphql -f query='
{
repository(owner: "leynos", name: "lading") {
pullRequest(number: 116) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
path
line
body
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | {id, isResolved, author: .comments.nodes[0].author.login, path: .comments.nodes[0].path, line: .comments.nodes[0].line, body: (.comments.nodes[0].body | .[0:120])}'Length of output: 936
The test param has been added at lines 477–481 of pytest.param(
"[bump]\nrebuild_lockfiles = true\n",
[],
True,
id="configuration-hydrated-by-cyclopts-true",
),This covers the missing matrix cell: TOML config with |
…variant
Address review warnings that the resolution matrix only exercised the
private _initialize_bump_context helper and lacked property-based coverage.
- Add a parametrised behavioural test exercising bump.run() across
rebuild_lockfiles in {None, True, False} x configuration in {True, False},
observing the resolved value through whether lockfile regeneration runs.
- Add a Hypothesis property test asserting the single-source-of-truth
invariant over the same input domain, building a fresh workspace per
example since each run mutates manifests.
- Keep the white-box helper test as the direct field-level counterpart and
cross-reference the two.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. tests/unit/test_bump_command_integration.py Comment on file import dataclasses as dc
import pathlib
import tempfile❌ New issue: Low Cohesion |
This comment was marked as resolved.
This comment was marked as resolved.
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Resolve the CodeScene low-cohesion finding (16 responsibilities across 23 functions) for tests/unit/test_bump_command_integration.py by splitting it into four focused modules: - test_bump_manifest_updates.py - manifest version/dependency updates - test_bump_lockfile_rebuild.py - lockfile rebuild behaviour - test_bump_rebuild_lockfiles_resolution.py - issue #106 resolution matrix - test_bump_documentation_updates.py - README/doc transposition Promote the shared stub_lockfile_regeneration fixture to tests/unit/conftest.py as an autouse fixture. It is scoped by module name to the four bump-run split modules so it never shadows test_bump_lockfiles, which exercises regenerate_lockfiles directly and needs the real implementation. Test logic, assertions, parametrisation, and the fixture body are unchanged; the full unit suite still reports 569 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/unit/conftest.py (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the module docstring to reflect bump fixtures.
The docstring still says "Shared fixtures and helpers for publish unit tests" but the module now includes bump-related fixtures (
stub_lockfile_regeneration,_LOCKFILE_STUB_MODULES). Update it to acknowledge both publish and bump fixtures.📝 Proposed fix
-"""Shared fixtures and helpers for publish unit tests.""" +"""Shared fixtures and helpers for publish and bump unit tests."""🤖 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 `@tests/unit/conftest.py` at line 1, Update the module docstring at the top of tests/unit/conftest.py to mention both publish and bump fixtures: change the existing "Shared fixtures and helpers for publish unit tests." to a brief description that includes bump-related fixtures (e.g., reference stub_lockfile_regeneration and _LOCKFILE_STUB_MODULES) so the docstring accurately reflects that the module provides fixtures/helpers for both publish and bump tests.tests/unit/test_bump_documentation_updates.py (1)
65-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explanatory messages to assertions.
Lines 86, 87, 88-90, 92-95 use bare assertions. Provide failure messages to clarify test intent and improve debuggability.
✏️ Proposed fix
crate_readme = tmp_path / "crates" / "alpha" / "README.md" - assert "readme file(s)" in message - assert "- crates/alpha/README.md (readme)" in message.splitlines() - assert crate_readme.read_text(encoding="utf-8") == ( + assert "readme file(s)" in message, "bump output should report readme transposition" + assert "- crates/alpha/README.md (readme)" in message.splitlines(), ( + "transposed README path should appear in output" + ) + assert crate_readme.read_text(encoding="utf-8") == ( "# Sample\n\nSee [Guide](../../docs/guide.md).\n" - ) + ), "transposed README should rewrite relative paths from workspace root" if scenario.check_version_unchanged: - assert ( + assert ( _load_version(tmp_path / "crates" / "alpha" / "Cargo.toml", ("package",)) == "0.1.0" - ) + ), "excluded crate version should remain unchanged at 0.1.0"🤖 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 `@tests/unit/test_bump_documentation_updates.py` around lines 65 - 96, The test test_run_transposes_workspace_readme_to_crates uses bare assertions which makes failures unclear; update each assertion to include an explanatory failure message. Specifically, for the assertions checking contents of message ("readme file(s)"), presence of the "- crates/alpha/README.md (readme)" line, the crate_readme content equality, and the version equality check that uses _load_version, add concise messages describing the expected condition (e.g., "expected bump.run message to mention readme file(s)", "expected crate README path entry in bump output", "expected crate README to rewrite relative links", "expected crate version to remain 0.1.0 when version unchanged") so failures point directly at the intent in test_run_transposes_workspace_readme_to_crates and the helpers bump.run and _load_version.Source: Coding guidelines
tests/unit/test_bump_manifest_updates.py (1)
209-361:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explanatory messages to assertions.
Lines 253, 281-288, 332 use bare assertions. Provide failure messages to clarify test expectations and improve debuggability. As per coding guidelines, use
assert …, "message"over bare asserts.✏️ Example fix for lines 281-288
- assert message.splitlines() == [ + assert message.splitlines() == [ "Dry run; would update version to 1.2.3 in 3 manifest(s):", "- Cargo.toml", "- crates/alpha/Cargo.toml", "- crates/beta/Cargo.toml", - ] + ], "dry-run output should list manifests that would be updated" for path in manifest_paths: - assert path.read_text(encoding="utf-8") == original_contents[path] + assert path.read_text(encoding="utf-8") == original_contents[path], ( + f"dry-run should not modify {path.relative_to(tmp_path)}" + )🤖 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 `@tests/unit/test_bump_manifest_updates.py` around lines 209 - 361, Add explicit failure messages to the bare assertions in the listed tests: in test_run_reports_when_versions_already_match update the assertion "assert message == scenario.expected_message" to include a message referencing scenario.test_id and both expected vs actual; in test_run_dry_run_reports_changes_without_modifying_files add messages to the list-equality assertion on message.splitlines() and to the loop assertion that compares path.read_text() with original_contents[path] (include the manifest path and expected vs actual content); in test_run_updates_workspace_dependency_sections change "assert actual_version == expected_version" to include the section name, manifest path and the actual vs expected version; and in test_run_updates_workspace_dependency_prefixes add messages to the three asserts that check alpha value, beta_entry["version"], and beta_entry["path"] to show which field failed and the expected vs actual values. Ensure each assert uses the comma-form "assert <cond>, <message>" and reference the local variables message, original_contents, actual_version, expected_version, document, and beta_entry to construct informative messages.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 `@tests/unit/conftest.py`:
- Around line 18-26: Update the comment above _LOCKFILE_STUB_MODULES to
explicitly state that these specific test modules are allowlisted because they
invoke bump.run to exercise manifest updates or dependency resolution logic
without requiring Cargo to actually build or regenerate lockfiles, so the stub
prevents shelling out to Cargo while still allowing test_bump_lockfiles to use
the real regenerate_lockfiles implementation; mention the intent to scope the
stub by module name to avoid shadowing test_bump_lockfiles and clarify that the
listed tests focus on manifest/resolution behavior rather than lockfile
generation mechanics.
In `@tests/unit/test_bump_documentation_updates.py`:
- Around line 29-49: The three bare assertions in
test_run_updates_documentation_snippets (the asserts checking "documentation
file(s)" in message, "- README.md (documentation)" in message.splitlines(), and
'alpha = "1.2.3"' in updated_readme) should include descriptive failure
messages; update each assert to provide a helpful message explaining what was
expected and include the actual value (e.g., message or updated_readme) to aid
debugging when the test fails. Ensure you modify the assertions in the
test_run_updates_documentation_snippets function to use the form assert
<condition>, "<descriptive failure message with relevant variable
interpolation>" referencing the variables message and updated_readme.
In `@tests/unit/test_bump_lockfile_rebuild.py`:
- Around line 30-102: The test combines both enabled and disabled
rebuild_lockfiles behavior in test_run_rebuilds_lockfiles_by_default; split it
into two focused tests named test_run_rebuilds_lockfiles_when_enabled and
test_run_skips_lockfiles_when_disabled. For the enabled test keep the
fake_regenerate_lockfiles spy, monkeypatch
bump.bump_lockfiles.regenerate_lockfiles, call bump.run with
BumpOptions(rebuild_lockfiles=True, ...), and assert the captured calls, message
contents and lockfile lines; for the disabled test monkeypatch
regenerate_lockfiles to pytest.fail, call bump.run with
BumpOptions(rebuild_lockfiles=False, ...), and assert that the message does not
mention "lockfile". Ensure to move shared setup (tmp_path workspace/config
helpers) into each test or a small fixture and update the original test name
removal.
In `@tests/unit/test_bump_manifest_updates.py`:
- Around line 91-207: Several tests use bare assertions; update the assertion
calls to include explanatory failure messages. In
test_run_skips_excluded_crates, add messages to the three asserts that call
_load_version to indicate which crate/version check failed; in
test_run_updates_internal_dependency_versions, add messages to the assertions
checking dependency_version, dev_entry fields and build_entry fields returned by
_extract_alpha_dependency_entries to describe expected updated versions/paths;
in test_run_updates_renamed_internal_dependency_versions add a message to each
assert that inspects dependency_entry from
parse_toml(beta_manifest.read_text(...)) to state the expected version and
package; and in test_run_normalises_workspace_root and
test_run_uses_loaded_configuration_and_workspace add messages to their single
asserts referencing _load_version to clarify expected workspace package
versions. For each change, keep the same assertion expressions but pass a final
string argument that clearly describes the expectation (e.g., "expected
workspace package version X but got Y" or "expected dependency 'alpha-core'
version '^2.3.4'").
- Around line 51-89: The tests use several bare assertions in
test_run_updates_workspace_and_members (asserting message.splitlines() equals
expected list, the workspace manifest version via _load_version(tmp_path /
"Cargo.toml", ("workspace", "package")), and each crate version inside the for
loop) and in test_run_updates_root_package_section (asserting
_load_version(manifest_path, ("package",)) and _load_version(manifest_path,
("workspace", "package"))). Replace those bare asserts with assertions that
include helpful failure messages (e.g., assert message.splitlines() == expected,
f"Unexpected bump output: {message!r}" and assert _load_version(...) == "x.y.z",
f"Expected version x.y.z in <manifest identifier> but found
{_load_version(...)}"), and likewise in the loop include the crate.manifest_path
in the message to identify which crate failed.
---
Outside diff comments:
In `@tests/unit/conftest.py`:
- Line 1: Update the module docstring at the top of tests/unit/conftest.py to
mention both publish and bump fixtures: change the existing "Shared fixtures and
helpers for publish unit tests." to a brief description that includes
bump-related fixtures (e.g., reference stub_lockfile_regeneration and
_LOCKFILE_STUB_MODULES) so the docstring accurately reflects that the module
provides fixtures/helpers for both publish and bump tests.
In `@tests/unit/test_bump_documentation_updates.py`:
- Around line 65-96: The test test_run_transposes_workspace_readme_to_crates
uses bare assertions which makes failures unclear; update each assertion to
include an explanatory failure message. Specifically, for the assertions
checking contents of message ("readme file(s)"), presence of the "-
crates/alpha/README.md (readme)" line, the crate_readme content equality, and
the version equality check that uses _load_version, add concise messages
describing the expected condition (e.g., "expected bump.run message to mention
readme file(s)", "expected crate README path entry in bump output", "expected
crate README to rewrite relative links", "expected crate version to remain 0.1.0
when version unchanged") so failures point directly at the intent in
test_run_transposes_workspace_readme_to_crates and the helpers bump.run and
_load_version.
In `@tests/unit/test_bump_manifest_updates.py`:
- Around line 209-361: Add explicit failure messages to the bare assertions in
the listed tests: in test_run_reports_when_versions_already_match update the
assertion "assert message == scenario.expected_message" to include a message
referencing scenario.test_id and both expected vs actual; in
test_run_dry_run_reports_changes_without_modifying_files add messages to the
list-equality assertion on message.splitlines() and to the loop assertion that
compares path.read_text() with original_contents[path] (include the manifest
path and expected vs actual content); in
test_run_updates_workspace_dependency_sections change "assert actual_version ==
expected_version" to include the section name, manifest path and the actual vs
expected version; and in test_run_updates_workspace_dependency_prefixes add
messages to the three asserts that check alpha value, beta_entry["version"], and
beta_entry["path"] to show which field failed and the expected vs actual values.
Ensure each assert uses the comma-form "assert <cond>, <message>" and reference
the local variables message, original_contents, actual_version,
expected_version, document, and beta_entry to construct informative messages.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e3e55ad4-21e4-4fb6-ad85-a727774f23d9
📒 Files selected for processing (5)
tests/unit/conftest.pytests/unit/test_bump_documentation_updates.pytests/unit/test_bump_lockfile_rebuild.pytests/unit/test_bump_manifest_updates.pytests/unit/test_bump_rebuild_lockfiles_resolution.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/unit/conftest.py (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the module docstring to reflect bump fixtures.
The docstring still says "Shared fixtures and helpers for publish unit tests" but the module now includes bump-related fixtures (
stub_lockfile_regeneration,_LOCKFILE_STUB_MODULES). Update it to acknowledge both publish and bump fixtures.📝 Proposed fix
-"""Shared fixtures and helpers for publish unit tests.""" +"""Shared fixtures and helpers for publish and bump unit tests."""🤖 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 `@tests/unit/conftest.py` at line 1, Update the module docstring at the top of tests/unit/conftest.py to mention both publish and bump fixtures: change the existing "Shared fixtures and helpers for publish unit tests." to a brief description that includes bump-related fixtures (e.g., reference stub_lockfile_regeneration and _LOCKFILE_STUB_MODULES) so the docstring accurately reflects that the module provides fixtures/helpers for both publish and bump tests.tests/unit/test_bump_documentation_updates.py (1)
65-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explanatory messages to assertions.
Lines 86, 87, 88-90, 92-95 use bare assertions. Provide failure messages to clarify test intent and improve debuggability.
✏️ Proposed fix
crate_readme = tmp_path / "crates" / "alpha" / "README.md" - assert "readme file(s)" in message - assert "- crates/alpha/README.md (readme)" in message.splitlines() - assert crate_readme.read_text(encoding="utf-8") == ( + assert "readme file(s)" in message, "bump output should report readme transposition" + assert "- crates/alpha/README.md (readme)" in message.splitlines(), ( + "transposed README path should appear in output" + ) + assert crate_readme.read_text(encoding="utf-8") == ( "# Sample\n\nSee [Guide](../../docs/guide.md).\n" - ) + ), "transposed README should rewrite relative paths from workspace root" if scenario.check_version_unchanged: - assert ( + assert ( _load_version(tmp_path / "crates" / "alpha" / "Cargo.toml", ("package",)) == "0.1.0" - ) + ), "excluded crate version should remain unchanged at 0.1.0"🤖 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 `@tests/unit/test_bump_documentation_updates.py` around lines 65 - 96, The test test_run_transposes_workspace_readme_to_crates uses bare assertions which makes failures unclear; update each assertion to include an explanatory failure message. Specifically, for the assertions checking contents of message ("readme file(s)"), presence of the "- crates/alpha/README.md (readme)" line, the crate_readme content equality, and the version equality check that uses _load_version, add concise messages describing the expected condition (e.g., "expected bump.run message to mention readme file(s)", "expected crate README path entry in bump output", "expected crate README to rewrite relative links", "expected crate version to remain 0.1.0 when version unchanged") so failures point directly at the intent in test_run_transposes_workspace_readme_to_crates and the helpers bump.run and _load_version.Source: Coding guidelines
tests/unit/test_bump_manifest_updates.py (1)
209-361:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explanatory messages to assertions.
Lines 253, 281-288, 332 use bare assertions. Provide failure messages to clarify test expectations and improve debuggability. As per coding guidelines, use
assert …, "message"over bare asserts.✏️ Example fix for lines 281-288
- assert message.splitlines() == [ + assert message.splitlines() == [ "Dry run; would update version to 1.2.3 in 3 manifest(s):", "- Cargo.toml", "- crates/alpha/Cargo.toml", "- crates/beta/Cargo.toml", - ] + ], "dry-run output should list manifests that would be updated" for path in manifest_paths: - assert path.read_text(encoding="utf-8") == original_contents[path] + assert path.read_text(encoding="utf-8") == original_contents[path], ( + f"dry-run should not modify {path.relative_to(tmp_path)}" + )🤖 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 `@tests/unit/test_bump_manifest_updates.py` around lines 209 - 361, Add explicit failure messages to the bare assertions in the listed tests: in test_run_reports_when_versions_already_match update the assertion "assert message == scenario.expected_message" to include a message referencing scenario.test_id and both expected vs actual; in test_run_dry_run_reports_changes_without_modifying_files add messages to the list-equality assertion on message.splitlines() and to the loop assertion that compares path.read_text() with original_contents[path] (include the manifest path and expected vs actual content); in test_run_updates_workspace_dependency_sections change "assert actual_version == expected_version" to include the section name, manifest path and the actual vs expected version; and in test_run_updates_workspace_dependency_prefixes add messages to the three asserts that check alpha value, beta_entry["version"], and beta_entry["path"] to show which field failed and the expected vs actual values. Ensure each assert uses the comma-form "assert <cond>, <message>" and reference the local variables message, original_contents, actual_version, expected_version, document, and beta_entry to construct informative messages.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 `@tests/unit/conftest.py`:
- Around line 18-26: Update the comment above _LOCKFILE_STUB_MODULES to
explicitly state that these specific test modules are allowlisted because they
invoke bump.run to exercise manifest updates or dependency resolution logic
without requiring Cargo to actually build or regenerate lockfiles, so the stub
prevents shelling out to Cargo while still allowing test_bump_lockfiles to use
the real regenerate_lockfiles implementation; mention the intent to scope the
stub by module name to avoid shadowing test_bump_lockfiles and clarify that the
listed tests focus on manifest/resolution behavior rather than lockfile
generation mechanics.
In `@tests/unit/test_bump_documentation_updates.py`:
- Around line 29-49: The three bare assertions in
test_run_updates_documentation_snippets (the asserts checking "documentation
file(s)" in message, "- README.md (documentation)" in message.splitlines(), and
'alpha = "1.2.3"' in updated_readme) should include descriptive failure
messages; update each assert to provide a helpful message explaining what was
expected and include the actual value (e.g., message or updated_readme) to aid
debugging when the test fails. Ensure you modify the assertions in the
test_run_updates_documentation_snippets function to use the form assert
<condition>, "<descriptive failure message with relevant variable
interpolation>" referencing the variables message and updated_readme.
In `@tests/unit/test_bump_lockfile_rebuild.py`:
- Around line 30-102: The test combines both enabled and disabled
rebuild_lockfiles behavior in test_run_rebuilds_lockfiles_by_default; split it
into two focused tests named test_run_rebuilds_lockfiles_when_enabled and
test_run_skips_lockfiles_when_disabled. For the enabled test keep the
fake_regenerate_lockfiles spy, monkeypatch
bump.bump_lockfiles.regenerate_lockfiles, call bump.run with
BumpOptions(rebuild_lockfiles=True, ...), and assert the captured calls, message
contents and lockfile lines; for the disabled test monkeypatch
regenerate_lockfiles to pytest.fail, call bump.run with
BumpOptions(rebuild_lockfiles=False, ...), and assert that the message does not
mention "lockfile". Ensure to move shared setup (tmp_path workspace/config
helpers) into each test or a small fixture and update the original test name
removal.
In `@tests/unit/test_bump_manifest_updates.py`:
- Around line 91-207: Several tests use bare assertions; update the assertion
calls to include explanatory failure messages. In
test_run_skips_excluded_crates, add messages to the three asserts that call
_load_version to indicate which crate/version check failed; in
test_run_updates_internal_dependency_versions, add messages to the assertions
checking dependency_version, dev_entry fields and build_entry fields returned by
_extract_alpha_dependency_entries to describe expected updated versions/paths;
in test_run_updates_renamed_internal_dependency_versions add a message to each
assert that inspects dependency_entry from
parse_toml(beta_manifest.read_text(...)) to state the expected version and
package; and in test_run_normalises_workspace_root and
test_run_uses_loaded_configuration_and_workspace add messages to their single
asserts referencing _load_version to clarify expected workspace package
versions. For each change, keep the same assertion expressions but pass a final
string argument that clearly describes the expectation (e.g., "expected
workspace package version X but got Y" or "expected dependency 'alpha-core'
version '^2.3.4'").
- Around line 51-89: The tests use several bare assertions in
test_run_updates_workspace_and_members (asserting message.splitlines() equals
expected list, the workspace manifest version via _load_version(tmp_path /
"Cargo.toml", ("workspace", "package")), and each crate version inside the for
loop) and in test_run_updates_root_package_section (asserting
_load_version(manifest_path, ("package",)) and _load_version(manifest_path,
("workspace", "package"))). Replace those bare asserts with assertions that
include helpful failure messages (e.g., assert message.splitlines() == expected,
f"Unexpected bump output: {message!r}" and assert _load_version(...) == "x.y.z",
f"Expected version x.y.z in <manifest identifier> but found
{_load_version(...)}"), and likewise in the loop include the crate.manifest_path
in the message to identify which crate failed.
---
Outside diff comments:
In `@tests/unit/conftest.py`:
- Line 1: Update the module docstring at the top of tests/unit/conftest.py to
mention both publish and bump fixtures: change the existing "Shared fixtures and
helpers for publish unit tests." to a brief description that includes
bump-related fixtures (e.g., reference stub_lockfile_regeneration and
_LOCKFILE_STUB_MODULES) so the docstring accurately reflects that the module
provides fixtures/helpers for both publish and bump tests.
In `@tests/unit/test_bump_documentation_updates.py`:
- Around line 65-96: The test test_run_transposes_workspace_readme_to_crates
uses bare assertions which makes failures unclear; update each assertion to
include an explanatory failure message. Specifically, for the assertions
checking contents of message ("readme file(s)"), presence of the "-
crates/alpha/README.md (readme)" line, the crate_readme content equality, and
the version equality check that uses _load_version, add concise messages
describing the expected condition (e.g., "expected bump.run message to mention
readme file(s)", "expected crate README path entry in bump output", "expected
crate README to rewrite relative links", "expected crate version to remain 0.1.0
when version unchanged") so failures point directly at the intent in
test_run_transposes_workspace_readme_to_crates and the helpers bump.run and
_load_version.
In `@tests/unit/test_bump_manifest_updates.py`:
- Around line 209-361: Add explicit failure messages to the bare assertions in
the listed tests: in test_run_reports_when_versions_already_match update the
assertion "assert message == scenario.expected_message" to include a message
referencing scenario.test_id and both expected vs actual; in
test_run_dry_run_reports_changes_without_modifying_files add messages to the
list-equality assertion on message.splitlines() and to the loop assertion that
compares path.read_text() with original_contents[path] (include the manifest
path and expected vs actual content); in
test_run_updates_workspace_dependency_sections change "assert actual_version ==
expected_version" to include the section name, manifest path and the actual vs
expected version; and in test_run_updates_workspace_dependency_prefixes add
messages to the three asserts that check alpha value, beta_entry["version"], and
beta_entry["path"] to show which field failed and the expected vs actual values.
Ensure each assert uses the comma-form "assert <cond>, <message>" and reference
the local variables message, original_contents, actual_version,
expected_version, document, and beta_entry to construct informative messages.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e3e55ad4-21e4-4fb6-ad85-a727774f23d9
📒 Files selected for processing (5)
tests/unit/conftest.pytests/unit/test_bump_documentation_updates.pytests/unit/test_bump_lockfile_rebuild.pytests/unit/test_bump_manifest_updates.pytests/unit/test_bump_rebuild_lockfiles_resolution.py
🛑 Comments failed to post (5)
tests/unit/conftest.py (1)
18-26: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Clarify the allowlist rationale.
The comment explains that the stub is scoped to avoid shadowing
test_bump_lockfiles, which needs the real implementation. Strengthen the explanation by explicitly stating why those specific modules are allowlisted—they exercisebump.runwithout needing actual Cargo invocation because they focus on manifest or resolution logic rather than lockfile generation mechanics.✨ Example clarification
# Modules whose ``bump.run`` exercises must not shell out to Cargo. The stub is -# scoped to these by name so it never shadows ``test_bump_lockfiles``, which -# exercises ``regenerate_lockfiles`` directly and needs the real implementation. +# scoped to these by name because they test manifest updates, documentation, or +# resolution logic without requiring actual lockfile regeneration. The stub never +# shadows ``test_bump_lockfiles``, which exercises ``regenerate_lockfiles`` +# directly and needs the real implementation. _LOCKFILE_STUB_MODULES = frozenset({🤖 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 `@tests/unit/conftest.py` around lines 18 - 26, Update the comment above _LOCKFILE_STUB_MODULES to explicitly state that these specific test modules are allowlisted because they invoke bump.run to exercise manifest updates or dependency resolution logic without requiring Cargo to actually build or regenerate lockfiles, so the stub prevents shelling out to Cargo while still allowing test_bump_lockfiles to use the real regenerate_lockfiles implementation; mention the intent to scope the stub by module name to avoid shadowing test_bump_lockfiles and clarify that the listed tests focus on manifest/resolution behavior rather than lockfile generation mechanics.tests/unit/test_bump_documentation_updates.py (1)
29-49:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explanatory messages to assertions.
Lines 45, 46, and 48 use bare assertions. Provide failure messages to aid debugging when tests fail.
✏️ Proposed fix
- assert "documentation file(s)" in message - assert "- README.md (documentation)" in message.splitlines() + assert "documentation file(s)" in message, "bump output should report documentation updates" + assert "- README.md (documentation)" in message.splitlines(), "README.md should appear in output" updated_readme = readme_path.read_text(encoding="utf-8") - assert 'alpha = "1.2.3"' in updated_readme + assert 'alpha = "1.2.3"' in updated_readme, "TOML fence version should be rewritten to 1.2.3"🤖 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 `@tests/unit/test_bump_documentation_updates.py` around lines 29 - 49, The three bare assertions in test_run_updates_documentation_snippets (the asserts checking "documentation file(s)" in message, "- README.md (documentation)" in message.splitlines(), and 'alpha = "1.2.3"' in updated_readme) should include descriptive failure messages; update each assert to provide a helpful message explaining what was expected and include the actual value (e.g., message or updated_readme) to aid debugging when the test fails. Ensure you modify the assertions in the test_run_updates_documentation_snippets function to use the form assert <condition>, "<descriptive failure message with relevant variable interpolation>" referencing the variables message and updated_readme.Source: Coding guidelines
tests/unit/test_bump_lockfile_rebuild.py (1)
30-102: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Split the enabled and disabled cases into separate tests.
This test exercises both
rebuild_lockfiles=True(lines 30-79) andrebuild_lockfiles=False(lines 81-102) in a single function. Per the single-responsibility principle, split these intotest_run_rebuilds_lockfiles_when_enabledandtest_run_skips_lockfiles_when_disabled. Each test would have a clearer focus and simpler assertion logic.♻️ Proposed split
def test_run_rebuilds_lockfiles_by_default( tmp_path: pathlib.Path, monkeypatch: MonkeyPatch, ) -> None: - """Verify regenerate_lockfiles calls. + """Lockfile regeneration is called when explicitly enabled.""" - - Lockfile regeneration is called when enabled and suppressed when disabled. - """ workspace = _make_workspace(tmp_path) configuration = _make_config() nested_lockfile = tmp_path / "crates/ui/Cargo.lock" captured: dict[str, object] = {} def fake_regenerate_lockfiles( workspace_root: pathlib.Path, lockfile_manifests: tuple[str, ...], *, runner: object | None = None, ) -> tuple[pathlib.Path, ...]: captured["calls"] = int(captured.get("calls", 0)) + 1 captured["workspace_root"] = workspace_root captured["lockfile_manifests"] = lockfile_manifests captured["runner"] = runner return (tmp_path / "Cargo.lock", nested_lockfile) monkeypatch.setattr( bump.bump_lockfiles, "regenerate_lockfiles", fake_regenerate_lockfiles, ) message = bump.run( tmp_path, "1.2.3", options=bump.BumpOptions( rebuild_lockfiles=True, configuration=configuration, workspace=workspace, ), ) assert captured == { "calls": 1, "workspace_root": tmp_path, "lockfile_manifests": (), "runner": None, } assert "2 lockfile(s)" in message assert "- Cargo.lock (lockfile)" in message.splitlines() assert "- crates/ui/Cargo.lock (lockfile)" in message.splitlines() + + +def test_run_skips_lockfiles_when_disabled( + tmp_path: pathlib.Path, + monkeypatch: MonkeyPatch, +) -> None: + """Lockfile regeneration is suppressed when explicitly disabled.""" + workspace = _make_workspace(tmp_path) + configuration = _make_config() - disabled_root = tmp_path / "disabled" - disabled_workspace = _make_workspace(disabled_root) - disabled_configuration = _make_config() monkeypatch.setattr( bump.bump_lockfiles, "regenerate_lockfiles", lambda *args, **kwargs: pytest.fail( "regenerate_lockfiles must not be called when rebuild_lockfiles=False" ), ) message = bump.run( - disabled_root, + tmp_path, "1.2.3", options=bump.BumpOptions( rebuild_lockfiles=False, - configuration=disabled_configuration, - workspace=disabled_workspace, + configuration=configuration, + workspace=workspace, ), ) assert "lockfile" not in message🤖 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 `@tests/unit/test_bump_lockfile_rebuild.py` around lines 30 - 102, The test combines both enabled and disabled rebuild_lockfiles behavior in test_run_rebuilds_lockfiles_by_default; split it into two focused tests named test_run_rebuilds_lockfiles_when_enabled and test_run_skips_lockfiles_when_disabled. For the enabled test keep the fake_regenerate_lockfiles spy, monkeypatch bump.bump_lockfiles.regenerate_lockfiles, call bump.run with BumpOptions(rebuild_lockfiles=True, ...), and assert the captured calls, message contents and lockfile lines; for the disabled test monkeypatch regenerate_lockfiles to pytest.fail, call bump.run with BumpOptions(rebuild_lockfiles=False, ...), and assert that the message does not mention "lockfile". Ensure to move shared setup (tmp_path workspace/config helpers) into each test or a small fixture and update the original test name removal.tests/unit/test_bump_manifest_updates.py (2)
51-89:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explanatory messages to assertions.
Lines 57-65, 63, 65, 87-88 use bare assertions. Provide failure messages to aid debugging when tests fail. As per coding guidelines, use
assert …, "message"over bare asserts.✏️ Example fix for lines 57-65
- assert message.splitlines() == [ + assert message.splitlines() == [ "Updated version to 1.2.3 in 3 manifest(s):", "- Cargo.toml", "- crates/alpha/Cargo.toml", "- crates/beta/Cargo.toml", - ] - assert _load_version(tmp_path / "Cargo.toml", ("workspace", "package")) == "1.2.3" + ], "bump output should list all updated manifests" + assert _load_version(tmp_path / "Cargo.toml", ("workspace", "package")) == "1.2.3", ( + "workspace package version should be updated to 1.2.3" + ) for crate in workspace.crates: - assert _load_version(crate.manifest_path, ("package",)) == "1.2.3" + assert _load_version(crate.manifest_path, ("package",)) == "1.2.3", ( + f"crate {crate.name} version should be updated to 1.2.3" + )🤖 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 `@tests/unit/test_bump_manifest_updates.py` around lines 51 - 89, The tests use several bare assertions in test_run_updates_workspace_and_members (asserting message.splitlines() equals expected list, the workspace manifest version via _load_version(tmp_path / "Cargo.toml", ("workspace", "package")), and each crate version inside the for loop) and in test_run_updates_root_package_section (asserting _load_version(manifest_path, ("package",)) and _load_version(manifest_path, ("workspace", "package"))). Replace those bare asserts with assertions that include helpful failure messages (e.g., assert message.splitlines() == expected, f"Unexpected bump output: {message!r}" and assert _load_version(...) == "x.y.z", f"Expected version x.y.z in <manifest identifier> but found {_load_version(...)}"), and likewise in the loop include the crate.manifest_path in the message to identify which crate failed.Source: Coding guidelines
91-207:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explanatory messages to assertions.
Lines 101-104, 132-136, 175-176, 194, 206 use bare assertions. Provide failure messages to clarify test expectations and improve debuggability.
🤖 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 `@tests/unit/test_bump_manifest_updates.py` around lines 91 - 207, Several tests use bare assertions; update the assertion calls to include explanatory failure messages. In test_run_skips_excluded_crates, add messages to the three asserts that call _load_version to indicate which crate/version check failed; in test_run_updates_internal_dependency_versions, add messages to the assertions checking dependency_version, dev_entry fields and build_entry fields returned by _extract_alpha_dependency_entries to describe expected updated versions/paths; in test_run_updates_renamed_internal_dependency_versions add a message to each assert that inspects dependency_entry from parse_toml(beta_manifest.read_text(...)) to state the expected version and package; and in test_run_normalises_workspace_root and test_run_uses_loaded_configuration_and_workspace add messages to their single asserts referencing _load_version to clarify expected workspace package versions. For each change, keep the same assertion expressions but pass a final string argument that clearly describes the expectation (e.g., "expected workspace package version X but got Y" or "expected dependency 'alpha-core' version '^2.3.4'").Source: Coding guidelines
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Address review feedback on the bump test split: - Expand the tests/unit/conftest.py module docstring and the _LOCKFILE_STUB_MODULES comment to explain that the listed modules drive bump.run for manifest, documentation, and rebuild_lockfiles-resolution behaviour without needing Cargo, and that the stub is scoped by module name so it never shadows test_bump_lockfiles, which exercises the real regenerate_lockfiles. - Split the misleadingly named test_run_rebuilds_lockfiles_by_default (it tested both explicit enable and disable) into test_run_rebuilds_lockfiles_when_enabled and test_run_skips_lockfiles_when_disabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@tests/unit/test_bump_documentation_updates.py`:
- Around line 86-95: The assertions in
tests/unit/test_bump_documentation_updates.py lack descriptive messages; update
each assert that references message, crate_readme.read_text(...), and the
_load_version(...) check (used with tmp_path, scenario) to include a clear
failure message string explaining expected vs actual (e.g., "expected 'readme
file(s)' in message", "expected README contents to equal ...", "expected version
to remain 0.1.0"). Modify the asserts on message (both contains and splitlines
checks), the crate_readme.read_text(...) equality, and the conditional
_load_version(...) equality to pass a third parameter (the assertion message) so
test failures show meaningful context.
- Around line 45-48: Each bare assert should include a concise failure message:
update the four assertions using the variables shown (message, updated_readme,
readme_path) to include descriptive strings—for example, assert "documentation
file(s)" in message, "expected summary mentioning documentation file(s) in
message"; assert "- README.md (documentation)" in message.splitlines(),
"expected README.md listed as documentation in message"; assert 'alpha =
\"1.2.3\"' in updated_readme, "expected README.md to contain the updated version
string"; and if relevant add a message for reading the file (e.g., when checking
updated_readme) so failures clearly state the expected condition and which value
failed. Ensure each assert uses the form assert <condition>, "<descriptive
message>".
In `@tests/unit/test_bump_lockfile_rebuild.py`:
- Line 104: Update the bare assertions in
tests/unit/test_bump_lockfile_rebuild.py (e.g., assert "lockfile" not in message
and the other asserts at the mentioned locations) to include descriptive failure
messages as the second argument to assert (for example: assert "lockfile" not in
message, "expected no 'lockfile' in message but found: {message}"). Locate each
assertion instance (the exact assertion expressions at the lines referenced:
assert "lockfile" not in message and the other similar asserts around lines 132,
180–186, 242–244) and add clear, specific messages that include relevant
variables (like message, result, or value) to aid debugging when the assertion
fails.
- Around line 68-76: Add descriptive assertion messages to each assert in this
test: for the dict equality assert referencing captured, append a message like
"captured dict mismatch" and for the string containment asserts referencing
message, append messages like "summary should report 2 lockfiles" and "should
list Cargo.lock" / "should list crates/ui/Cargo.lock" respectively; update all
other asserts in this module similarly so every assert uses the form assert
<expr>, "descriptive message" to make failures clear (look for symbols captured
and message in this file to locate each assertion to update).
In `@tests/unit/test_bump_manifest_updates.py`:
- Around line 87-88: Update the assertions in
tests/unit/test_bump_manifest_updates.py to include descriptive assertion
messages following the pattern used for _load_version(manifest_path,
("package",)) == "7.8.9"; for every bare assert noted (lines referenced: the
groups around _load_version, package/workspace checks and any other assertions
at the listed locations) append a second argument string describing what is
being asserted (e.g., "expected package version to be 7.8.9 from manifest_path",
or "expected workspace package version to be 7.8.9"). Locate assertions that
call _load_version, use manifest_path, or validate versions and update them to
assert <expr>, "<descriptive message>" so every assertion at the listed spots
has a clear failure message.
- Around line 57-65: Add concise failure messages to each assertion in
test_bump_manifest_updates.py so failures are descriptive: update the equality
assertion for message.splitlines() to include a message like "unexpected output
lines" referencing the variable message; add messages to the two _load_version
assertions such as "root Cargo.toml version not updated" for the tmp_path check
and "crate manifest version not updated for {crate.manifest_path}" for the loop
over workspace.crates; ensure each assert uses the form assert <condition>,
"<message>" and keep messages short and specific to the failing check.
In `@tests/unit/test_bump_rebuild_lockfiles_resolution.py`:
- Line 49: Add descriptive failure messages to the three bare asserts that check
rebuild_lockfiles: replace the assertion at "assert
context.base_options.rebuild_lockfiles is expected" and the two similar
assertions at the other locations with assertions that include a clear message
(e.g., "expected rebuild_lockfiles to be {expected} but was {actual}") so
failures show the expected vs actual; update the message text to reference
context.base_options.rebuild_lockfiles and expected for each of the three
asserts (lines shown) so test failures are informative.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 6e9e0434-3dc2-442d-bfea-103216dd0558
📒 Files selected for processing (5)
tests/unit/conftest.pytests/unit/test_bump_documentation_updates.pytests/unit/test_bump_lockfile_rebuild.pytests/unit/test_bump_manifest_updates.pytests/unit/test_bump_rebuild_lockfiles_resolution.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🤖 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 `@tests/unit/test_bump_documentation_updates.py`:
- Around line 86-95: The assertions in
tests/unit/test_bump_documentation_updates.py lack descriptive messages; update
each assert that references message, crate_readme.read_text(...), and the
_load_version(...) check (used with tmp_path, scenario) to include a clear
failure message string explaining expected vs actual (e.g., "expected 'readme
file(s)' in message", "expected README contents to equal ...", "expected version
to remain 0.1.0"). Modify the asserts on message (both contains and splitlines
checks), the crate_readme.read_text(...) equality, and the conditional
_load_version(...) equality to pass a third parameter (the assertion message) so
test failures show meaningful context.
- Around line 45-48: Each bare assert should include a concise failure message:
update the four assertions using the variables shown (message, updated_readme,
readme_path) to include descriptive strings—for example, assert "documentation
file(s)" in message, "expected summary mentioning documentation file(s) in
message"; assert "- README.md (documentation)" in message.splitlines(),
"expected README.md listed as documentation in message"; assert 'alpha =
\"1.2.3\"' in updated_readme, "expected README.md to contain the updated version
string"; and if relevant add a message for reading the file (e.g., when checking
updated_readme) so failures clearly state the expected condition and which value
failed. Ensure each assert uses the form assert <condition>, "<descriptive
message>".
In `@tests/unit/test_bump_lockfile_rebuild.py`:
- Line 104: Update the bare assertions in
tests/unit/test_bump_lockfile_rebuild.py (e.g., assert "lockfile" not in message
and the other asserts at the mentioned locations) to include descriptive failure
messages as the second argument to assert (for example: assert "lockfile" not in
message, "expected no 'lockfile' in message but found: {message}"). Locate each
assertion instance (the exact assertion expressions at the lines referenced:
assert "lockfile" not in message and the other similar asserts around lines 132,
180–186, 242–244) and add clear, specific messages that include relevant
variables (like message, result, or value) to aid debugging when the assertion
fails.
- Around line 68-76: Add descriptive assertion messages to each assert in this
test: for the dict equality assert referencing captured, append a message like
"captured dict mismatch" and for the string containment asserts referencing
message, append messages like "summary should report 2 lockfiles" and "should
list Cargo.lock" / "should list crates/ui/Cargo.lock" respectively; update all
other asserts in this module similarly so every assert uses the form assert
<expr>, "descriptive message" to make failures clear (look for symbols captured
and message in this file to locate each assertion to update).
In `@tests/unit/test_bump_manifest_updates.py`:
- Around line 87-88: Update the assertions in
tests/unit/test_bump_manifest_updates.py to include descriptive assertion
messages following the pattern used for _load_version(manifest_path,
("package",)) == "7.8.9"; for every bare assert noted (lines referenced: the
groups around _load_version, package/workspace checks and any other assertions
at the listed locations) append a second argument string describing what is
being asserted (e.g., "expected package version to be 7.8.9 from manifest_path",
or "expected workspace package version to be 7.8.9"). Locate assertions that
call _load_version, use manifest_path, or validate versions and update them to
assert <expr>, "<descriptive message>" so every assertion at the listed spots
has a clear failure message.
- Around line 57-65: Add concise failure messages to each assertion in
test_bump_manifest_updates.py so failures are descriptive: update the equality
assertion for message.splitlines() to include a message like "unexpected output
lines" referencing the variable message; add messages to the two _load_version
assertions such as "root Cargo.toml version not updated" for the tmp_path check
and "crate manifest version not updated for {crate.manifest_path}" for the loop
over workspace.crates; ensure each assert uses the form assert <condition>,
"<message>" and keep messages short and specific to the failing check.
In `@tests/unit/test_bump_rebuild_lockfiles_resolution.py`:
- Line 49: Add descriptive failure messages to the three bare asserts that check
rebuild_lockfiles: replace the assertion at "assert
context.base_options.rebuild_lockfiles is expected" and the two similar
assertions at the other locations with assertions that include a clear message
(e.g., "expected rebuild_lockfiles to be {expected} but was {actual}") so
failures show the expected vs actual; update the message text to reference
context.base_options.rebuild_lockfiles and expected for each of the three
asserts (lines shown) so test failures are informative.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 6e9e0434-3dc2-442d-bfea-103216dd0558
📒 Files selected for processing (5)
tests/unit/conftest.pytests/unit/test_bump_documentation_updates.pytests/unit/test_bump_lockfile_rebuild.pytests/unit/test_bump_manifest_updates.pytests/unit/test_bump_rebuild_lockfiles_resolution.py
🛑 Comments failed to post (7)
tests/unit/test_bump_documentation_updates.py (2)
45-48:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd descriptive assertion messages to all asserts.
The guidelines require
assert …, "message"over bare asserts. Every assertion in this module lacks a descriptive failure message, which hinders debugging when tests fail. Add concise messages explaining the expected condition.Example fixes
- assert "documentation file(s)" in message + assert "documentation file(s)" in message, "Expected bump output to report documentation updates" - assert "- README.md (documentation)" in message.splitlines() + assert "- README.md (documentation)" in message.splitlines(), "Expected README.md in the updated file list" - assert 'alpha = "1.2.3"' in updated_readme + assert 'alpha = "1.2.3"' in updated_readme, "Expected TOML fence version to be rewritten to 1.2.3"🤖 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 `@tests/unit/test_bump_documentation_updates.py` around lines 45 - 48, Each bare assert should include a concise failure message: update the four assertions using the variables shown (message, updated_readme, readme_path) to include descriptive strings—for example, assert "documentation file(s)" in message, "expected summary mentioning documentation file(s) in message"; assert "- README.md (documentation)" in message.splitlines(), "expected README.md listed as documentation in message"; assert 'alpha = \"1.2.3\"' in updated_readme, "expected README.md to contain the updated version string"; and if relevant add a message for reading the file (e.g., when checking updated_readme) so failures clearly state the expected condition and which value failed. Ensure each assert uses the form assert <condition>, "<descriptive message>".Source: Coding guidelines
86-95:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd descriptive assertion messages.
Apply the same fix to all assertions in this test (lines 86, 87, 88, 92-95). Descriptive messages clarify intent and aid debugging.
🤖 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 `@tests/unit/test_bump_documentation_updates.py` around lines 86 - 95, The assertions in tests/unit/test_bump_documentation_updates.py lack descriptive messages; update each assert that references message, crate_readme.read_text(...), and the _load_version(...) check (used with tmp_path, scenario) to include a clear failure message string explaining expected vs actual (e.g., "expected 'readme file(s)' in message", "expected README contents to equal ...", "expected version to remain 0.1.0"). Modify the asserts on message (both contains and splitlines checks), the crate_readme.read_text(...) equality, and the conditional _load_version(...) equality to pass a third parameter (the assertion message) so test failures show meaningful context.Source: Coding guidelines
tests/unit/test_bump_lockfile_rebuild.py (2)
68-76:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd descriptive assertion messages to all asserts.
This test (and all others in this module) lacks assertion messages. Follow the guideline requiring
assert …, "message"for clearer test failures.Example fixes
assert captured == { "calls": 1, "workspace_root": tmp_path, "lockfile_manifests": (), "runner": None, - } + }, "Expected regenerate_lockfiles to be called exactly once with correct arguments" - assert "2 lockfile(s)" in message + assert "2 lockfile(s)" in message, "Expected bump output to report 2 regenerated lockfiles" - assert "- Cargo.lock (lockfile)" in message.splitlines() + assert "- Cargo.lock (lockfile)" in message.splitlines(), "Expected workspace Cargo.lock in output" - assert "- crates/ui/Cargo.lock (lockfile)" in message.splitlines() + assert "- crates/ui/Cargo.lock (lockfile)" in message.splitlines(), "Expected nested Cargo.lock in output"🤖 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 `@tests/unit/test_bump_lockfile_rebuild.py` around lines 68 - 76, Add descriptive assertion messages to each assert in this test: for the dict equality assert referencing captured, append a message like "captured dict mismatch" and for the string containment asserts referencing message, append messages like "summary should report 2 lockfiles" and "should list Cargo.lock" / "should list crates/ui/Cargo.lock" respectively; update all other asserts in this module similarly so every assert uses the form assert <expr>, "descriptive message" to make failures clear (look for symbols captured and message in this file to locate each assertion to update).Source: Coding guidelines
104-104:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply assertion messages to all remaining tests.
Lines 104, 132, 180, 184-186, 242, and 244 all require descriptive failure messages per the coding guidelines.
Also applies to: 132-132, 180-186, 242-244
🤖 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 `@tests/unit/test_bump_lockfile_rebuild.py` at line 104, Update the bare assertions in tests/unit/test_bump_lockfile_rebuild.py (e.g., assert "lockfile" not in message and the other asserts at the mentioned locations) to include descriptive failure messages as the second argument to assert (for example: assert "lockfile" not in message, "expected no 'lockfile' in message but found: {message}"). Locate each assertion instance (the exact assertion expressions at the lines referenced: assert "lockfile" not in message and the other similar asserts around lines 132, 180–186, 242–244) and add clear, specific messages that include relevant variables (like message, result, or value) to aid debugging when the assertion fails.Source: Coding guidelines
tests/unit/test_bump_manifest_updates.py (2)
57-65:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd descriptive assertion messages.
This test (and all others in this module) violates the guideline requiring
assert …, "message". Add concise failure messages to aid debugging.Example fixes
- assert message.splitlines() == [ + assert message.splitlines() == [ "Updated version to 1.2.3 in 3 manifest(s):", "- Cargo.toml", "- crates/alpha/Cargo.toml", "- crates/beta/Cargo.toml", - ] + ], "Expected bump output to list all updated manifests" - assert _load_version(tmp_path / "Cargo.toml", ("workspace", "package")) == "1.2.3" + assert _load_version(tmp_path / "Cargo.toml", ("workspace", "package")) == "1.2.3", "Expected workspace package version to be 1.2.3" for crate in workspace.crates: - assert _load_version(crate.manifest_path, ("package",)) == "1.2.3" + assert _load_version(crate.manifest_path, ("package",)) == "1.2.3", f"Expected crate {crate.name} version to be 1.2.3"🤖 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 `@tests/unit/test_bump_manifest_updates.py` around lines 57 - 65, Add concise failure messages to each assertion in test_bump_manifest_updates.py so failures are descriptive: update the equality assertion for message.splitlines() to include a message like "unexpected output lines" referencing the variable message; add messages to the two _load_version assertions such as "root Cargo.toml version not updated" for the tmp_path check and "crate manifest version not updated for {crate.manifest_path}" for the loop over workspace.crates; ensure each assert uses the form assert <condition>, "<message>" and keep messages short and specific to the failing check.Source: Coding guidelines
87-88:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply assertion messages across all remaining tests.
Lines 87-88, 101-104, 132-136, 175-176, 194, 206, 253, 281-288, and 332 all require descriptive messages. Apply the same pattern shown above.
Also applies to: 101-104, 132-136, 175-176, 194-194, 206-206, 253-253, 281-288, 332-332
🤖 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 `@tests/unit/test_bump_manifest_updates.py` around lines 87 - 88, Update the assertions in tests/unit/test_bump_manifest_updates.py to include descriptive assertion messages following the pattern used for _load_version(manifest_path, ("package",)) == "7.8.9"; for every bare assert noted (lines referenced: the groups around _load_version, package/workspace checks and any other assertions at the listed locations) append a second argument string describing what is being asserted (e.g., "expected package version to be 7.8.9 from manifest_path", or "expected workspace package version to be 7.8.9"). Locate assertions that call _load_version, use manifest_path, or validate versions and update them to assert <expr>, "<descriptive message>" so every assertion at the listed spots has a clear failure message.Source: Coding guidelines
tests/unit/test_bump_rebuild_lockfiles_resolution.py (1)
49-49:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd descriptive assertion messages to all asserts.
Lines 49, 108, and 134 all lack descriptive failure messages. Apply the guideline requiring
assert …, "message".Proposed fixes
expected = configured if flag is None else flag - assert context.base_options.rebuild_lockfiles is expected + assert context.base_options.rebuild_lockfiles is expected, f"Expected rebuild_lockfiles={expected} (flag={flag}, configured={configured})"Apply similar messages at lines 108 and 134, adjusting context as needed.
Also applies to: 108-108, 134-134
🤖 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 `@tests/unit/test_bump_rebuild_lockfiles_resolution.py` at line 49, Add descriptive failure messages to the three bare asserts that check rebuild_lockfiles: replace the assertion at "assert context.base_options.rebuild_lockfiles is expected" and the two similar assertions at the other locations with assertions that include a clear message (e.g., "expected rebuild_lockfiles to be {expected} but was {actual}") so failures show the expected vs actual; update the message text to reference context.base_options.rebuild_lockfiles and expected for each of the three asserts (lines shown) so test failures are informative.Source: Coding guidelines
Address the review request to attach explanatory messages to the bare assertions in the four bump test modules so failures explain intent. pytest introspection still reports the compared values; the messages add the "what/why" context. Messages interpolate already-bound locals (message, updated_readme) where cheap and avoid re-reading manifests via _load_version. Covers test_bump_manifest_updates.py, test_bump_lockfile_rebuild.py, test_bump_documentation_updates.py, and test_bump_rebuild_lockfiles_resolution.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Emit a DEBUG record at the single rebuild_lockfiles resolution point in _initialize_bump_context, exposing the raw nullable flag, the configured default, and the resolved value. This makes the coalescing decision visible to operators running with debug logging. The call uses %r formatting to match the existing _log.debug style in the module. Add a caplog-based unit test asserting the debug record is emitted with both the raw flag and the resolved value when the logger is at DEBUG level. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
@coderabbitai resume |
Replace the inline shape assertions against bump.run()'s formatted result string (substring `in message`, `in message.splitlines()`, `not in message`, and inline-literal `splitlines() == [...]`) with syrupy snapshot assertions across the manifest, lockfile-rebuild, and documentation bump test modules. Output-format regressions are now caught automatically with human-readable diffs, and each parametrised case produces a distinct snapshot key. Side-effect assertions (TOML version strings, on-disk file contents, the captured regenerate_lockfiles call, and the exact no-op expected_message parametrisation) are retained unchanged. SnapshotAssertion is imported under TYPE_CHECKING to satisfy the flake8-type-checking (TC) rule, matching the existing MonkeyPatch import style. Generated __snapshots__/*.ambr files are committed alongside. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Replace the last inline `bump.run` output assertions in the bump manifest and lockfile tests with syrupy snapshots. Keep the existing side-effect assertions in place so the tests still distinguish rendered output from filesystem behaviour.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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)
tests/unit/test_bump_lockfile_rebuild.py (1)
20-28: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove unused
expected_messagefield.The
_LockfileSkipScenario.expected_messagefield is defined but never referenced intest_run_skips_lockfile_rebuild. The parametrised test uses onlysnapshotassertions (line 243), making this field dead code.♻️ Proposed fix
`@dc.dataclass`(frozen=True, slots=True) class _LockfileSkipScenario: """Parameters describing lockfile rebuild skip scenarios.""" test_id: str version: str rebuild_lockfiles: bool fail_message: str - expected_message: str | 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 `@tests/unit/test_bump_lockfile_rebuild.py` around lines 20 - 28, The _LockfileSkipScenario dataclass defines an unused field expected_message; remove the expected_message attribute from the class declaration (in the _LockfileSkipScenario dataclass) and clean up any references if present so the test_run_skips_lockfile_rebuild parametrisation and snapshot assertions only use test_id, version, rebuild_lockfiles, and fail_message; ensure the dataclass import/usage remains consistent after removal.
🤖 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 `@tests/unit/test_bump_lockfile_rebuild.py`:
- Around line 20-28: The _LockfileSkipScenario dataclass defines an unused field
expected_message; remove the expected_message attribute from the class
declaration (in the _LockfileSkipScenario dataclass) and clean up any references
if present so the test_run_skips_lockfile_rebuild parametrisation and snapshot
assertions only use test_id, version, rebuild_lockfiles, and fail_message;
ensure the dataclass import/usage remains consistent after removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee0f4b4c-b34f-4bdf-bd58-107b113c251d
📒 Files selected for processing (8)
lading/commands/bump.pytests/unit/__snapshots__/test_bump_documentation_updates.ambrtests/unit/__snapshots__/test_bump_lockfile_rebuild.ambrtests/unit/__snapshots__/test_bump_manifest_updates.ambrtests/unit/test_bump_documentation_updates.pytests/unit/test_bump_lockfile_rebuild.pytests/unit/test_bump_manifest_updates.pytests/unit/test_bump_rebuild_lockfiles_resolution.py
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== syrupy/snapshot usage in the three test files ==="
rg -n "syrupy|snapshot|SnapshotAssertion" \
tests/unit/test_bump_manifest_updates.py \
tests/unit/test_bump_lockfile_rebuild.py \
tests/unit/test_bump_documentation_updates.py 2>/dev/null
echo ""
echo "=== snapshot files present ==="
ls tests/unit/__snapshots__/ 2>/dev/null
echo ""
echo "=== remaining inline output assertions in the three files ==="
rg -n 'assert.*in message\|assert message ==' \
tests/unit/test_bump_manifest_updates.py \
tests/unit/test_bump_lockfile_rebuild.py \
tests/unit/test_bump_documentation_updates.py 2>/dev/null || echo "none found"
echo ""
echo "=== _initialize_bump_context debug logging ==="
rg -n "_log\.debug" lading/commands/bump.py -A 5 -B 5
echo ""
echo "=== caplog test for resolution logging ==="
rg -n "caplog\|rebuild_lockfiles resolution\|test_initialize_bump_context_logs" \
tests/unit/test_bump_rebuild_lockfiles_resolution.py 2>/dev/null | head -30Length of output: 4683 🏁 Script executed: #!/bin/bash
echo "=== check test_bump_rebuild_lockfiles_resolution.py exists ==="
ls -la tests/unit/test_bump_rebuild_lockfiles_resolution.py 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== search for caplog anywhere in tests/unit ==="
rg -rn "caplog\|test_initialize_bump_context_logs" tests/unit/ 2>/dev/null | head -20
echo ""
echo "=== list new test files ==="
ls -la tests/unit/test_bump_*.py 2>/dev/nullLength of output: 1345 🏁 Script executed: #!/bin/bash
echo "=== full content of test_bump_rebuild_lockfiles_resolution.py ==="
cat tests/unit/test_bump_rebuild_lockfiles_resolution.py
echo ""
echo "=== _log.debug call in bump.py with context ==="
rg -n "rebuild_lockfiles resolution" lading/commands/bump.py -A 5 -B 10Length of output: 7152
No further work is required for these two items. |
Drop the unused `_LockfileSkipScenario.expected_message` value now that lockfile rebuild output is asserted through syrupy snapshots.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #106
cli.bumpnow forwards the raw nullablerebuild_lockfilesflag; the onlyNone-coalescing againstconfiguration.bump.rebuild_lockfileshappens inbump._initialize_bump_context.lading.tomlbefore dispatch (use_commands_as_keys=True);cli.bumpitself performs no coalescing. This is called out in a code comment and the developers' guide.docs/developers-guide.mdrecords that option defaulting is the command layer's responsibility, not the CLI adapter's.Testing
rebuild_lockfiles ∈ {None, True, False}×configuration.bump.rebuild_lockfiles ∈ {True, False}.make check-fmt,make lint,make typecheck, andmake test(561 passed) all green.coderabbit review --agent: 0 findings.🤖 Generated with Claude Code
Summary by Sourcery
Centralize resolution of the nullable rebuild_lockfiles option in the bump command layer and ensure the CLI simply forwards the raw flag.
Bug Fixes:
Enhancements:
Documentation:
Tests: