Implement cargo publish with dry-run by default and --live option - #44
Conversation
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…lished crates - Extend the publish command to support a --live flag to run `cargo publish` without `--dry-run`. - Default to dry-run mode unless explicitly enabled live. - Log a warning and continue publishing other crates if a crate version is already published instead of aborting. - Update CLI interface, internal publish logic, documentation, and tests to support live publishing and error handling. - Add BDD scenarios and unit tests covering live publishing and publishing already uploaded crates. This enables safely executing real crate publications while improving robustness against already published versions. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Reviewer's GuideImplements an integrated cargo publish phase that runs after packaging, defaults to dry-run, supports an opt-in --live mode, and handles already-published crate versions gracefully, with supporting CLI, test, and documentation updates. Sequence diagram for updated publish workflow with dry-run and live cargo publishsequenceDiagram
actor Operator
participant CLI as LadingCLI
participant Publish as PublishCommand
participant Cargo as CargoToolchain
Operator->>CLI: publish [--live?]
CLI->>Publish: run(workspace_root, PublishOptions(live))
rect rgb(230,230,230)
Note over Publish: Pre-flight and packaging
Publish->>Publish: _ensure_configuration(...)
Publish->>Publish: _build_preflight_environment(...)
Publish->>Cargo: cargo package (per crate)
Cargo-->>Publish: exit_code, stdout, stderr
Publish->>Publish: _format_cargo_failure_message("package", ...)
Publish-->>Operator: PublishPreflightError on failure
end
rect rgb(220,245,220)
Note over Publish: New publish phase
loop for each crate in plan.publishable
Publish->>Publish: _resolve_staged_crate_root(...)
alt live == False
Publish->>Cargo: cargo publish --dry-run
else live == True
Publish->>Cargo: cargo publish
end
Cargo-->>Publish: exit_code, stdout, stderr
alt exit_code == 0
Publish->>Publish: continue to next crate
else exit_code != 0
Publish->>Publish: _is_already_published_error(exit_code, stdout, stderr)
alt already published
Publish->>Publish: LOGGER.warning("already published - skipping")
Publish->>Publish: continue to next crate
else other failure
Publish->>Publish: _format_cargo_failure_message("publish", ...)
Publish-->>Operator: PublishError
end
end
end
end
Publish-->>Operator: plan_message (publish summary)
Class diagram for updated publish options and errorsclassDiagram
class PublishOptions {
bool allow_dirty = True
bool live = False
Path build_directory
bool preserve_symlinks = True
bool cleanup = False
bool dry_run
LadingConfig configuration
}
class PublishPreflightError {
<<exception>>
}
class PublishError {
<<exception>>
}
PublishPreflightError <|-- PublishError
class PublishModuleHelpers {
+_format_cargo_failure_message(command, crate_name, exit_code, output) str
+_is_already_published_error(exit_code, stdout, stderr) bool
+_publish_crates(plan, preparation, runner, live) void
}
class PublishPlan {
+publishable
}
class PublishPreparation {
+staging_root
}
PublishModuleHelpers --> PublishOptions : uses
PublishModuleHelpers --> PublishPlan : iterates publishable
PublishModuleHelpers --> PublishPreparation : reads staging_root
PublishError --> PublishModuleHelpers : raised by
PublishPreflightError --> PublishModuleHelpers : raised by
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdd a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "lading publish (CLI)"
participant Publish as "publish module"
participant Preflight as "preflight checks"
participant Cargo as "cargo"
participant Registry as "registry"
CLI->>Publish: run(options={live:false|true})
activate Publish
Publish->>Preflight: perform preflight (check/test/package)
activate Preflight
Preflight->>Cargo: cargo check/test/package
Preflight-->>Publish: preflight results
deactivate Preflight
loop per crate
Publish->>Cargo: cargo publish --dry-run (default)
activate Cargo
Cargo->>Registry: validate publish
Registry-->>Cargo: dry-run ok / errors
Cargo-->>Publish: result
deactivate Cargo
alt live=true
Publish->>Cargo: cargo publish (live)
activate Cargo
Cargo->>Registry: upload crate
alt crate already exists
Registry-->>Cargo: already-published error
Cargo-->>Publish: error (detectable via code/markers)
Note over Publish: log warning and continue next crate
else success
Registry-->>Cargo: success
Cargo-->>Publish: success
end
deactivate Cargo
end
end
Publish-->>CLI: finish (success/failure)
deactivate Publish
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/test_publish_steps.py Comment on file cmd_mox: CmdMox
overrides: dict[tuple[str, ...], _CommandResponse] = dc.field(default_factory=dict)
overrides: dict[tuple[str, ...], ResponseProvider] = dc.field(default_factory=dict)❌ New issue: Low Cohesion |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/test_publish_steps.py Comment on file repo_root: Path,
cmd_mox: CmdMox,
preflight_overrides: dict[tuple[str, ...], _CommandResponse],
preflight_overrides: dict[tuple[str, ...], ResponseProvider],❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/unit/publish/test_packaging.py Comment on lines +291 to +318 def test_publish_crates_raise_on_failure(tmp_path: Path) -> None:
"""Unexpected cargo publish failures abort the workflow."""
workspace_root = tmp_path / "workspace"
crates = make_dependency_chain(workspace_root)
plan = publish.plan_publication(
make_workspace(workspace_root, *crates), make_config()
)
staging_root = _prepare_staging_root(plan, tmp_path)
preparation = publish.PublishPreparation(
staging_root=staging_root,
copied_readmes=(),
)
def failing_runner(
command: cabc.Sequence[str],
*,
cwd: Path | None = None,
env: cabc.Mapping[str, str] | None = None,
) -> tuple[int, str, str]:
del command, cwd, env
return 1, "network offline", ""
with pytest.raises(publish.PublishPreflightError) as excinfo:
publish._publish_crates(plan, preparation, runner=failing_runner, live=False)
message = str(excinfo.value)
assert "cargo publish failed for crate" in message
assert "network offline" in message❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/test_publish_steps.py Comment on file publish_command: tuple[str, ...]
publish_response: ResponseProvider
for command, response in config.overrides.items():
if len(command) >= 2 and command[0] == "cargo" and command[1] == "publish":❌ New issue: Complex Conditional |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Refactor publish-related tests to use new helper functions and classes for tracking command invocations and handling failures. Added fixtures to simplify test setup and reduce code duplication. Replaced manual checks on command invocations with reusable assertions, improving readability and maintainability of test code. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.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.
…dules Refactor the publish BDD tests by deleting the large monolithic `test_publish_steps.py` and replacing it with multiple focused modules: - `test_publish_fixtures.py` for shared pytest fixtures - `test_publish_given_steps.py` for Given steps - `test_publish_when_steps.py` for When steps - `test_publish_then_steps.py` for Then steps - `test_publish_helpers.py` for shared helper utilities - `test_publish_infrastructure.py` for infra helpers and cmd-mox stubs Also updated imports and test conftest registrations to reflect the new modular layout. This improves test code organization, maintainability, and readability. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/test_publish_helpers.py Comment on lines +134 to +143 def _assert_invocations_have_flag(
invocations: list[tuple[tuple[str, ...], dict[str, str]]],
flag: str,
command_name: str,
) -> None:
"""Assert that every invocation contains ``flag``."""
for args, _env in invocations:
if flag not in args:
message = f"Expected {flag!r} in {command_name} invocation"
raise AssertionError(message)❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/test_publish_when_steps.py Comment on lines +40 to +49 def when_invoke_lading_publish(
workspace_directory: Path,
repo_root: Path,
cmd_mox: _ImportedCmdMox,
preflight_overrides: dict[tuple[str, ...], ResponseProvider],
preflight_recorder: _PreflightInvocationRecorder,
) -> dict[str, typ.Any]:
"""Execute the publish CLI via ``python -m`` and capture the result."""
stub_config = _create_stub_config(cmd_mox, preflight_overrides, preflight_recorder)
return _invoke_publish_with_options(repo_root, workspace_directory, stub_config)❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
…single helper function Refactored _assert_invocations_have_flag and _assert_invocations_lack_flag to utilize a unified helper _assert_invocations_flag_presence that handles both presence and absence checks for flags in invocations. This removes code duplication and clarifies assertion logic in test_publish_helpers.py. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…t test setup - Added PreflightTestContext dataclass to encapsulate cmd_mox, overrides, and recorder - Replaced separate fixtures and parameters with PreflightTestContext in test steps - Simplified stub config creation via method on PreflightTestContext - Improved code clarity and reduced duplication in BDD tests for publish preflight checks Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationtests/unit/publish/test_packaging.py: What lead to degradation?The module contains 2 functions with similar structure: test_package_publishable_crates_prefers_stderr_over_stdout,test_package_publishable_crates_reports_stdout_on_failure Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
Refactored failure message assertions in packaging tests by introducing a helper function `_assert_packaging_failure_message_contains`. This reduces code duplication and improves clarity in testing error message contents when packaging crates fails. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/unit/publish/test_packaging.py Comment on lines +91 to +110 def _assert_packaging_failure_message_contains(
plan: publish.PublishPlan,
preparation: publish.PublishPreparation,
runner: cabc.Callable[..., tuple[int, str, str]],
expected_in_message: str,
not_expected_in_message: str | None = None,
) -> None:
"""Assert that packaging failure produces expected error message content."""
with pytest.raises(publish.PublishPreflightError) as excinfo:
publish._package_publishable_crates(
plan,
preparation,
runner=runner,
)
message = str(excinfo.value)
assert "cargo package failed for crate alpha" in message
assert expected_in_message in message
if not_expected_in_message is not None:
assert not_expected_in_message not in message❌ New issue: Excess Number of Function Arguments |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
_is_already_published_errorhelper treats any non-zero exit code whose output contains broad markers like"already exists"as non-fatal, which risks masking unrelated failures; consider tightening the condition (e.g. checking for known cargo exit codes like 101 and/or more specific message patterns) to reduce false positives. - The error handling and message construction in
_publish_cratesclosely mirrors_package_publishable_crates; factoring the common logic into a shared helper (e.g. for picking stderr/stdout and formattingPublishPreflightErrormessages) would reduce duplication and keep behavior consistent between packaging and publishing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_is_already_published_error` helper treats any non-zero exit code whose output contains broad markers like `"already exists"` as non-fatal, which risks masking unrelated failures; consider tightening the condition (e.g. checking for known cargo exit codes like 101 and/or more specific message patterns) to reduce false positives.
- The error handling and message construction in `_publish_crates` closely mirrors `_package_publishable_crates`; factoring the common logic into a shared helper (e.g. for picking stderr/stdout and formatting `PublishPreflightError` messages) would reduce duplication and keep behavior consistent between packaging and publishing.
## Individual Comments
### Comment 1
<location> `lading/commands/publish.py:364-367` </location>
<code_context>
raise PublishPreflightError(message)
+_ALREADY_PUBLISHED_MARKERS: tuple[str, ...] = (
+ "already uploaded",
+ "already published",
+ "already exists",
+)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The "already exists" marker looks broad and might classify unrelated failures as safe-to-ignore.
Matching the generic substring "already exists" on combined stdout/stderr risks treating unrelated errors as harmless "already published" cases (e.g., registry or filesystem failures that include that phrase). Consider tightening this to cargo’s specific publish error wording or including more surrounding context so only true "version already on the registry" errors are downgraded.
</issue_to_address>
### Comment 2
<location> `docs/usage-guide.md:206` </location>
<code_context>
```
+Append `--live` to replace the default dry-run with real `cargo publish`
+invocations when you are ready to ship.
+
Example output:
</code_context>
<issue_to_address>
**issue (review_instructions):** The phrase "when you are ready" uses a second-person pronoun and should be rewritten in neutral form.
To comply with the style guidance, please avoid "you" here. For example, this could be rephrased as "when the release is ready to ship" or similar neutral wording.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>
### Comment 3
<location> `docs/usage-guide.md:234` </location>
<code_context>
-behaviour is a dry-run that validates packaging only.
+workspace, the command runs `cargo package` for every publishable crate in
+plan order inside the staged copy. After packaging, Lading invokes
+`cargo publish --dry-run` for each crate so you can validate the full pipeline
+without uploading. Use `--live` to omit the `--dry-run` flag and perform the
+actual publication. If the registry reports that a crate version already
</code_context>
<issue_to_address>
**issue (review_instructions):** The clause "so you can validate" uses second-person voice and should be rephrased.
Consider rewriting this to avoid addressing the reader directly, for example "so the full pipeline can be validated" or similar neutral phrasing.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>
### Comment 4
<location> `tests/bdd/conftest.py:9` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 5
<location> `tests/bdd/conftest.py:10` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 6
<location> `tests/bdd/conftest.py:11` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 7
<location> `tests/bdd/conftest.py:12` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 8
<location> `tests/bdd/conftest.py:13` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 9
<location> `tests/bdd/conftest.py:14` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 10
<location> `tests/bdd/steps/test_publish_helpers.py:44` </location>
<code_context>
return {} if not isinstance(crates_io, typ.Mapping) else dict(crates_io)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Swap if/else branches of if expression to remove negation ([`swap-if-expression`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/swap-if-expression))
```suggestion
return dict(crates_io) if isinstance(crates_io, typ.Mapping) else {}
```
<br/><details><summary>Explanation</summary>Negated conditions are more difficult to read than positive ones, so it is best
to avoid them where we can. By swapping the `if` and `else` conditions around we
can invert the condition and make it positive.
</details>
</issue_to_address>
### Comment 11
<location> `tests/bdd/steps/test_publish_helpers.py:105-108` </location>
<code_context>
def _has_contiguous_args(args: tuple[str, ...], first: str, second: str) -> bool:
"""Return True when ``first`` is immediately followed by ``second`` in ``args``."""
for index in range(len(args) - 1):
if args[index] == first and args[index + 1] == second:
return True
return False
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use any() instead of for loop ([`use-any`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-any/))
```suggestion
return any(
args[index] == first and args[index + 1] == second
for index in range(len(args) - 1)
)
```
</issue_to_address>
### Comment 12
<location> `tests/bdd/steps/test_publish_infrastructure.py:202` </location>
<code_context>
def _register_preflight_commands(config: _PreflightStubConfig) -> None:
"""Install cmd-mox doubles for publish pre-flight commands."""
defaults: dict[tuple[str, ...], ResponseProvider] = {
("git", "status", "--porcelain"): _CommandResponse(exit_code=0),
(
"cargo",
"check",
"--workspace",
"--all-targets",
): _CommandResponse(exit_code=0),
(
"cargo",
"test",
"--workspace",
): _CommandResponse(exit_code=0),
("cargo", "package"): _CommandResponse(exit_code=0),
}
publish_command: tuple[str, ...]
publish_response: ResponseProvider
for command, response in config.overrides.items():
if _is_cargo_publish_command(command):
publish_command = command
publish_response = response
break
else:
publish_command = ("cargo", "publish", "--dry-run")
publish_response = _CommandResponse(exit_code=0)
filtered_overrides = {
command: response
for command, response in config.overrides.items()
if not _is_cargo_publish_command(command)
}
defaults.update(filtered_overrides)
defaults[publish_command] = publish_response
for command, response in defaults.items():
expectation_program, expectation_args = _resolve_preflight_expectation(command)
config.cmd_mox.stub(expectation_program).runs(
_make_preflight_handler(
response, expectation_args, config.recorder, expectation_program
)
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge dictionary updates via the union operator ([`dict-assign-update-to-union`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/dict-assign-update-to-union/))
```suggestion
defaults |= filtered_overrides
```
</issue_to_address>
### Comment 13
<location> `tests/bdd/steps/test_publish_then_steps.py:147` </location>
<code_context>
@then(parsers.parse('the cargo test pre-flight env contains "{name}"="{value}"'))
def then_cargo_test_env_contains(
preflight_recorder: _PreflightInvocationRecorder,
name: str,
value: str,
) -> None:
"""Assert that cargo test env propagates ``name`` with ``value``."""
envs = _get_test_invocation_envs(preflight_recorder)
if not any(environment.get(name) == value for environment in envs):
message = f"Expected cargo test env {name}={value!r}"
raise AssertionError(message)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Invert any/all to simplify comparisons ([`invert-any-all`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/invert-any-all/))
```suggestion
if all(environment.get(name) != value for environment in envs):
```
</issue_to_address>
### Comment 14
<location> `tests/bdd/steps/test_publish_then_steps.py:159` </location>
<code_context>
@then(parsers.parse('the cargo test pre-flight env includes "{snippet}" in RUSTFLAGS'))
def then_cargo_test_env_rustflags_contains(
preflight_recorder: _PreflightInvocationRecorder,
snippet: str,
) -> None:
"""Assert that cargo test RUSTFLAGS contains ``snippet``."""
envs = _get_test_invocation_envs(preflight_recorder)
if not any(snippet in environment.get("RUSTFLAGS", "") for environment in envs):
message = f"Expected {snippet!r} in cargo test RUSTFLAGS"
raise AssertionError(message)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Invert any/all to simplify comparisons ([`invert-any-all`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/invert-any-all/))
```suggestion
if all(
snippet not in environment.get("RUSTFLAGS", "") for environment in envs
):
```
</issue_to_address>
### Comment 15
<location> `tests/bdd/steps/test_publish_when_steps.py:77-78` </location>
<code_context>
@when(
"I invoke lading publish with that workspace using --live",
target_fixture="cli_run",
)
def when_invoke_lading_publish_live(
workspace_directory: Path,
repo_root: Path,
preflight_test_context: PreflightTestContext,
) -> dict[str, typ.Any]:
"""Execute the publish CLI with live publishing enabled."""
if not any(
command[:2] == ("cargo", "publish")
for command in preflight_test_context.overrides
):
preflight_test_context.overrides[("cargo", "publish")] = _CommandResponse(
exit_code=0
)
stub_config = preflight_test_context.create_stub_config()
return _invoke_publish_with_options(
repo_root,
workspace_directory,
stub_config,
"--live",
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Invert any/all to simplify comparisons ([`invert-any-all`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/invert-any-all/))
```suggestion
if all(
command[:2] != ("cargo", "publish")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
docs/lading-design.md(1 hunks)docs/roadmap.md(1 hunks)docs/usage-guide.md(2 hunks)lading/cli.py(3 hunks)lading/commands/publish.py(6 hunks)tests/bdd/conftest.py(1 hunks)tests/bdd/features/cli.feature(1 hunks)tests/bdd/steps/test_common_steps.py(1 hunks)tests/bdd/steps/test_publish_fixtures.py(1 hunks)tests/bdd/steps/test_publish_given_steps.py(1 hunks)tests/bdd/steps/test_publish_helpers.py(1 hunks)tests/bdd/steps/test_publish_infrastructure.py(1 hunks)tests/bdd/steps/test_publish_steps.py(0 hunks)tests/bdd/steps/test_publish_then_steps.py(1 hunks)tests/bdd/steps/test_publish_when_steps.py(1 hunks)tests/conftest.py(1 hunks)tests/unit/publish/test_packaging.py(3 hunks)
💤 Files with no reviewable changes (1)
- tests/bdd/steps/test_publish_steps.py
🧰 Additional context used
📓 Path-based instructions (5)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update relevant files in thedocs/directory when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve to keep documentation accurate and current.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict) with suffixes: -ize in words like 'realize' and 'organization', -lyse in words like 'analyse' and 'paralyse', -our in words like 'colour' and 'behaviour', -re in words like 'centre' and 'calibre', double 'l' in words like 'cancelled' and 'counsellor', maintain 'e' in words like 'likeable', -ogue in words like 'catalogue'
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns in documentation, for example 'Lille Industries are expanding'
Write headings in sentence case
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Always provide a language identifier for fenced code blocks; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change in Markdown
Prefer inline links using text or angle brackets around the URL in Markdown
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown
Ensure tables have a delimiter line below the header row in Markdown
Expand any uncommon acronym on first use, for example, Continuous Integration (CI)
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with [^label] in Markdown documentation
Include Mermaid diagrams in documentation where they add clarity
When embedding figures in documentation, use 
**/*.md: For Markdown files (.md only), ensure changes pass lint checks viamake markdownlint.
For Markdown files (.md only), ensure Mermaid diagrams pass validation using nixie viamake nixie.
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/lading-design.mddocs/usage-guide.mddocs/roadmap.md
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure changes pass all relevant unit and behavioral tests according to testing guidelines.
For Python files, ensure changes pass lint checks viamake lint.
For Python files, ensure changes adhere to formatting standards viamake check-fmt.
For Python files, ensure changes pass type checking viamake typecheck.
For Python development, refer to detailed guidelines in the.rules/directory including Python Code Style Guidelines, Context Managers, Exception Design/Raising/Handling/Logging, Generators, Project Configuration, Return Patterns, and Typing best practices.
**/*.py: Use snake_case.py for Python file names, naming them for their contents
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright; use typing everywhere
Use TypedDict or @DataClass(slots=True) for structured data
Avoid Any; use precise types (TypeVar, Protocol, Literal, Union) and typing.cast only when necessary with justification
Be explicit with return type annotations using -> None, -> str, etc., for all public functions and class methods
Prefer immutability: use tuples over lists and types.MappingProxyType for read-only mappings
Enforce strict mode in Pyright; treat all Pyright warnings as CI errors and use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Document public functions, classes, and modules using NumPy-format docstrings
Use inline comments to explain tricky code and non-obvious logic
**/*.py: Use context managers (viacontextlib.contextmanageror__enter__/__exit__methods) to encapsulate setup and teardown logic for resource management (files, locks, connections, etc.)
Use@contextlib.contextmanagerdecorator for straightforward procedural setup...
Files:
tests/bdd/steps/test_publish_fixtures.pylading/cli.pytests/bdd/steps/test_publish_given_steps.pytests/conftest.pytests/unit/publish/test_packaging.pytests/bdd/steps/test_publish_when_steps.pytests/bdd/steps/test_publish_then_steps.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/conftest.pytests/bdd/steps/test_common_steps.pylading/commands/publish.pytests/bdd/steps/test_publish_helpers.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/bdd/steps/test_publish_fixtures.pylading/cli.pytests/bdd/steps/test_publish_given_steps.pytests/conftest.pytests/unit/publish/test_packaging.pytests/bdd/steps/test_publish_when_steps.pytests/bdd/steps/test_publish_then_steps.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/conftest.pytests/bdd/steps/test_common_steps.pylading/commands/publish.pytests/bdd/steps/test_publish_helpers.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior, not internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/bdd/steps/test_publish_fixtures.pytests/bdd/steps/test_publish_given_steps.pytests/unit/publish/test_packaging.pytests/bdd/steps/test_publish_when_steps.pytests/bdd/steps/test_publish_then_steps.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/steps/test_common_steps.pytests/bdd/steps/test_publish_helpers.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, use narrow exception assertions with
pytest.raises()specifying the expected type and optionally constraining the message via regex (B017)
Files:
tests/bdd/steps/test_publish_fixtures.pytests/bdd/steps/test_publish_given_steps.pytests/conftest.pytests/unit/publish/test_packaging.pytests/bdd/steps/test_publish_when_steps.pytests/bdd/steps/test_publish_then_steps.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/conftest.pytests/bdd/steps/test_common_steps.pytests/bdd/steps/test_publish_helpers.py
🧬 Code graph analysis (4)
tests/bdd/steps/test_publish_fixtures.py (1)
tests/bdd/steps/test_publish_infrastructure.py (2)
PreflightTestContext(68-77)_PreflightInvocationRecorder(40-55)
lading/cli.py (1)
lading/commands/publish.py (1)
PublishOptions(67-105)
tests/bdd/steps/test_publish_infrastructure.py (2)
tests/bdd/steps/test_common_steps.py (1)
_run_cli(21-46)lading/commands/publish_execution.py (1)
_normalise_cmd_mox_command(185-195)
tests/bdd/steps/test_publish_helpers.py (4)
tests/bdd/steps/test_publish_infrastructure.py (2)
_PreflightInvocationRecorder(40-55)by_label(50-55)tests/unit/conftest.py (1)
staging_root(184-186)lading/testing/toml_utils.py (1)
load_manifest(166-188)crate_tools/unittests/publish_check/conftest.py (1)
env(289-292)
🔍 Remote MCP
The document doesn't mention --dry-run. Let me search for specific cargo publish documentation:Perfect! Now let me search for information about Lading specifically:Let me search for Lading repository directly to understand what it is:Based on the PR context, I can see that Lading is a Python project based on the file structure (.py files). Let me compile my findings into a comprehensive summary.
Summary of Relevant Context for PR Review
Based on my research, here are the key facts relevant to reviewing this PR:
Understanding cargo publish --dry-run Behavior
The --dry-run flag performs all checks without uploading. More specifically, it is recommended that you first run cargo publish --dry-run (or cargo package which is equivalent) to ensure there aren't any warnings or errors before publishing. The flag ensures all steps leading up to uploading are performed, but the actual upload is skipped.
Standard Cargo Publish Workflow
The cargo publish command performs: verification checks on the package, compression of source code into a .crate file, extraction and verification that it compiles, and uploading the .crate file to crates.io.
Workspace Publishing Challenges
Each crate in a workspace needs to be published separately. Multi-package publishing was recently promoted to a stable feature, but is currently only available in nightly builds. If you're using a recent nightly build of Cargo 1.90.0 or later, running cargo publish in a workspace will work.
Design Pattern Alignment
The PR's approach of defaulting to dry-run and requiring an explicit flag for live publishing aligns with industry best practices. Similar tools like cargo-release use --no-verify to speed up dry-run by skipping cargo-publish's verify step, showing that staged validation is a common pattern for workspace publishing tools.
Error Handling Consideration
The PR implements detection of already-published crates and continues processing. This is significant because publishing a crate is permanent and the version can never be overwritten, making this error case a normal operational scenario when publishing a workspace with previously-published crates.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (28)
docs/roadmap.md (1)
205-212: Mark cargo publish execution as complete with accurate criteria.Keep this task entry as written; it correctly describes dry‑run default behaviour, live mode, and continuation on already‑published versions, and matches the implemented workflow and tests.
tests/bdd/conftest.py (1)
9-14: Register publish BDD fixtures and steps within the BDD test package.Retain these imports; they align the BDD step registration with the new split publish fixtures/helpers/steps and follow the established pattern for config, manifest, and metadata fixtures.
tests/bdd/steps/test_common_steps.py (1)
175-179: Align CLI feature step imports with split publish modules.Keep these three imports; they correctly replace the monolithic publish steps module with given/when/then modules and preserve step registration for the CLI feature.
tests/bdd/features/cli.feature (1)
121-139: Exercise publish dry‑run, live mode, and already‑published handling via CLI scenarios.Retain these three scenarios; they give clear, end‑to‑end coverage of the default dry‑run behaviour, the --live flag, and continuation when cargo publish reports an existing version for a crate.
docs/lading-design.md (1)
513-517: Document cargo publish dry‑run default, --live, and already‑published semantics.Keep this updated description; it accurately states that cargo publish runs with --dry-run by default, switches to a real upload with --live, and logs then skips crates whose versions already exist.
tests/conftest.py (1)
18-21: Update pytest plugins to register new publish BDD modules.Keep this plugin list; it correctly replaces the monolithic publish steps plugin with dedicated fixtures and given/when/then modules, matching the refactored BDD layout while preserving scenario registration.
tests/bdd/steps/test_publish_fixtures.py (1)
1-37: Provide dedicated fixtures for publish preflight overrides and recording.Leave these fixtures as implemented; they supply fresh override mappings and recorders per scenario and assemble them into a typed PreflightTestContext, giving isolated, reusable scaffolding for publish BDD steps.
docs/usage-guide.md (2)
205-206: LGTM!The
--liveflag documentation is clear and concise, accurately reflecting the implementation behaviour.
232-238: LGTM!The publish sequence documentation correctly describes the dry-run default, live mode opt-in, and the graceful handling of already-published crate versions. The British English spelling and Oxford comma usage are correct.
tests/bdd/steps/test_publish_when_steps.py (1)
23-45: LGTM!The preflight check step and basic publish invocation step are well-structured with appropriate error capture and fixture wiring.
tests/bdd/steps/test_publish_then_steps.py (5)
1-31: LGTM!The import structure and type checking guards are well-organised. The helper imports from
test_publish_helpersprovide good separation of concerns.
33-42: LGTM!The plan verification step correctly validates the expected output structure.
76-98: LGTM!The preflight exclusion check and helper function correctly verify
--excludeflag presence. The message construction follows the coding guidelines by assigning to a variable before raising.
195-224: LGTM!The dry-run and live publish verification steps correctly distinguish between the two modes by checking for presence or absence of the
--dry-runflag. The crate order assertion provides good coverage.
302-322: LGTM!The README staging verification correctly compares file contents between the staged and source locations using explicit UTF-8 encoding.
lading/commands/publish.py (4)
48-48: LGTM!The module-level logger follows the coding guidelines using
logging.getLogger(__name__).
364-376: LGTM!The already-published detection is robust. The markers cover common cargo registry error messages, and the case-insensitive search combining stdout and stderr ensures reliable detection across cargo versions.
391-408: LGTM!The logging follows coding guidelines with parameterised lazy interpolation. The warning for already-published crates includes both crate name and version, providing sufficient context for operators.
485-490: LGTM!The integration correctly passes the
liveflag fromeffective_optionsand maintains the expected execution order: preflight → plan → prepare → strip patches → package → publish.tests/unit/publish/test_packaging.py (6)
34-49: LGTM!The fixture provides a clean 3-tuple pattern allowing tests to unpack only what they need. The staging root preparation correctly mirrors the plan structure.
52-69: LGTM!
CallTrackingRunnerprovides a clean test double for verifying command invocations. The call recording captures both the command tuple and working directory, enabling precise assertions.
91-112: LGTM with minor observation.The helper correctly validates failure messages. The 2-tuple parameter requires callers to slice the 3-tuple fixture, which is slightly awkward but acceptable. The PR comments mention consolidating this parameter - that refactor would improve ergonomics.
190-209: LGTM!The dry-run test correctly verifies that
--dry-runis passed to each cargo publish invocation and that logging occurs. Thecaplogfixture usage follows pytest idioms.
228-266: LGTM!Excellent coverage for the already-published continuation behaviour. The test correctly simulates the registry error for
alphaand verifies thatbetastill publishes. The warning log assertion confirms the user-facing feedback.
269-281: LGTM!The failure test verifies that unexpected cargo publish errors abort the workflow with appropriate error details.
tests/bdd/steps/test_publish_given_steps.py (1)
1-130: Keep the decomposed given-step module structureKeep this file as the focused “given” step module: the separation of cmd-mox socket setup, preflight overrides, and failure simulations is clear, type-safe, and lines up cleanly with the shared infrastructure and fixtures.
tests/bdd/steps/test_publish_helpers.py (1)
134-169: Reuse_assert_invocations_flag_presenceto centralise flag checksKeep
_assert_invocations_have_flagand_assert_invocations_lack_flagdelegating to_assert_invocations_flag_presence; this de-duplicates the previous flag-checking logic and keeps dry-run vs live assertions simple and uniform across BDD steps.tests/bdd/steps/test_publish_infrastructure.py (1)
234-259: Retain parametrised coverage of_resolve_preflight_expectationKeep this parametrised test: it tightly couples
_resolve_preflight_expectationtopublish._normalise_cmd_mox_commandsemantics and guards a wide range of cargo subcommands with and without extra arguments, which is valuable protection for the preflight stub wiring.
|
@coderabbitai Have the following now been resolved?
|
This comment was marked as resolved.
This comment was marked as resolved.
- Add precise markers for already published crates in publish command - Replace tuple addition with set union for defaults dictionary update - Improve test helpers with clearer logic and better type usage - Amend usage guide wording for clarity on publish command - Remove unnecessary imports and clean BDD conftest These changes improve robustness of publishing checks and maintain code clarity. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Introduced a helper function `_format_cargo_failure_message` to unify and simplify error messages from cargo command failures during package and publish steps. Refactored existing code to use this helper, reducing duplication and improving consistency of messages. Additionally, added clarifying comments around the already-published error detection logic for cargo exit code 101. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.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: 6
♻️ Duplicate comments (4)
tests/bdd/steps/test_publish_helpers.py (1)
16-27: Handle malformed staging lines defensively.The
split(": ", 1)call assumes the separator exists after the assertion. If the staging line is malformed (e.g.,"Staged workspace at"without the colon),splitreturns a single-element list and[1]raisesIndexError. Add a guard for clearer failure diagnostics.def _extract_staging_root_from_plan(lines: list[str]) -> Path: """Return the staging root path parsed from publish plan ``lines``.""" staging_line = next( (line for line in lines if line.startswith("Staged workspace at:")), None ) assert staging_line is not None, "Staging location not found in publish plan output" - return Path(staging_line.split(": ", 1)[1]) + parts = staging_line.split(": ", 1) + assert len(parts) == 2, f"Malformed staging line: {staging_line!r}" + return Path(parts[1])tests/bdd/steps/test_publish_infrastructure.py (3)
67-77: Addslots=TruetoPreflightTestContext.Per prior feedback, declare with
slots=Trueto match other dataclasses in this module and reduce per-instance overhead.-@dc.dataclass +@dc.dataclass(slots=True) class PreflightTestContext:
167-210: Document the single cargo publish override behaviour.Per prior feedback, clarify in the docstring that only the first
cargo publishoverride is honoured.def _register_preflight_commands(config: _PreflightStubConfig) -> None: - """Install cmd-mox doubles for publish pre-flight commands.""" + """Install cmd-mox doubles for publish pre-flight commands. + + Notes + ----- + Only a single cargo publish override is honoured. If multiple cargo + publish entries appear in ``config.overrides``, the first wins. + """
213-231: Use a context manager for environment variable toggling.Per prior feedback, replace the manual try/finally with a context manager for cleaner resource management. The current implementation is functionally correct but could be cleaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
docs/usage-guide.md(2 hunks)lading/commands/publish.py(7 hunks)tests/bdd/conftest.py(1 hunks)tests/bdd/steps/test_publish_helpers.py(1 hunks)tests/bdd/steps/test_publish_infrastructure.py(1 hunks)tests/bdd/steps/test_publish_then_steps.py(1 hunks)tests/bdd/steps/test_publish_when_steps.py(1 hunks)tests/unit/publish/test_packaging.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update relevant files in thedocs/directory when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve to keep documentation accurate and current.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict) with suffixes: -ize in words like 'realize' and 'organization', -lyse in words like 'analyse' and 'paralyse', -our in words like 'colour' and 'behaviour', -re in words like 'centre' and 'calibre', double 'l' in words like 'cancelled' and 'counsellor', maintain 'e' in words like 'likeable', -ogue in words like 'catalogue'
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns in documentation, for example 'Lille Industries are expanding'
Write headings in sentence case
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Always provide a language identifier for fenced code blocks; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change in Markdown
Prefer inline links using text or angle brackets around the URL in Markdown
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown
Ensure tables have a delimiter line below the header row in Markdown
Expand any uncommon acronym on first use, for example, Continuous Integration (CI)
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with [^label] in Markdown documentation
Include Mermaid diagrams in documentation where they add clarity
When embedding figures in documentation, use 
**/*.md: For Markdown files (.md only), ensure changes pass lint checks viamake markdownlint.
For Markdown files (.md only), ensure Mermaid diagrams pass validation using nixie viamake nixie.
Files:
docs/usage-guide.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/usage-guide.md
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure changes pass all relevant unit and behavioral tests according to testing guidelines.
For Python files, ensure changes pass lint checks viamake lint.
For Python files, ensure changes adhere to formatting standards viamake check-fmt.
For Python files, ensure changes pass type checking viamake typecheck.
For Python development, refer to detailed guidelines in the.rules/directory including Python Code Style Guidelines, Context Managers, Exception Design/Raising/Handling/Logging, Generators, Project Configuration, Return Patterns, and Typing best practices.
**/*.py: Use snake_case.py for Python file names, naming them for their contents
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright; use typing everywhere
Use TypedDict or @DataClass(slots=True) for structured data
Avoid Any; use precise types (TypeVar, Protocol, Literal, Union) and typing.cast only when necessary with justification
Be explicit with return type annotations using -> None, -> str, etc., for all public functions and class methods
Prefer immutability: use tuples over lists and types.MappingProxyType for read-only mappings
Enforce strict mode in Pyright; treat all Pyright warnings as CI errors and use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Document public functions, classes, and modules using NumPy-format docstrings
Use inline comments to explain tricky code and non-obvious logic
**/*.py: Use context managers (viacontextlib.contextmanageror__enter__/__exit__methods) to encapsulate setup and teardown logic for resource management (files, locks, connections, etc.)
Use@contextlib.contextmanagerdecorator for straightforward procedural setup...
Files:
tests/bdd/steps/test_publish_when_steps.pytests/unit/publish/test_packaging.pytests/bdd/conftest.pytests/bdd/steps/test_publish_helpers.pylading/commands/publish.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/steps/test_publish_then_steps.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/bdd/steps/test_publish_when_steps.pytests/unit/publish/test_packaging.pytests/bdd/conftest.pytests/bdd/steps/test_publish_helpers.pylading/commands/publish.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/steps/test_publish_then_steps.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior, not internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/bdd/steps/test_publish_when_steps.pytests/unit/publish/test_packaging.pytests/bdd/steps/test_publish_helpers.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/steps/test_publish_then_steps.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, use narrow exception assertions with
pytest.raises()specifying the expected type and optionally constraining the message via regex (B017)
Files:
tests/bdd/steps/test_publish_when_steps.pytests/unit/publish/test_packaging.pytests/bdd/conftest.pytests/bdd/steps/test_publish_helpers.pytests/bdd/steps/test_publish_infrastructure.pytests/bdd/steps/test_publish_then_steps.py
🧬 Code graph analysis (2)
lading/commands/publish.py (4)
lading/commands/publish_plan.py (1)
PublishPlan(22-34)tests/unit/publish/test_packaging.py (1)
runner(244-259)lading/commands/publish_execution.py (1)
_CommandRunner(52-62)tests/unit/conftest.py (1)
staging_root(184-186)
tests/bdd/steps/test_publish_then_steps.py (3)
tests/bdd/steps/test_publish_helpers.py (14)
_assert_crate_order_matches(172-183)_assert_invocations_have_flag(150-158)_assert_invocations_lack_flag(161-169)_extract_crate_names_from_invocations(82-90)_extract_staging_root_from_plan(21-27)_get_package_invocations(62-69)_get_patch_entries(38-44)_get_publish_invocations(72-79)_get_test_invocation_envs(93-100)_get_test_invocations(52-59)_has_contiguous_args(103-108)_load_staged_manifest(30-35)_publish_plan_lines(16-18)_split_names(47-49)tests/bdd/steps/test_publish_infrastructure.py (2)
_PreflightInvocationRecorder(40-55)by_label(50-55)lading/commands/publish.py (1)
PublishPreflightError(116-117)
🪛 LanguageTool
docs/usage-guide.md
[uncategorized] ~234-~234: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...cargo publish --dry-run for each crate so the full pipeline can be validated with...
(COMMA_COMPOUND_SENTENCE_2)
🔍 Remote MCP Ref
Summary of additional facts relevant to this PR
-
cargo publish --dry-run performs full verification steps (packaging, verify/compile) but does not upload; using dry-run before a real publish is recommended because publish is permanent and versions cannot be overwritten.
-
A real cargo publish uploads the .crate and is permanent; therefore treating "already published" as a non-fatal, expected condition (log-and-continue) is a reasonable workflow choice for workspace-wide publication tools that iterate crates.
Tools used: Ref (Ref_ref_search_documentation, Ref_ref_read_url)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (44)
tests/bdd/conftest.py (1)
1-6: Keep this conftest as a documented, minimal placeholder.The docstring clearly explains that BDD step and fixture registration happens via
tests/conftest.py, and the file has no side-effect imports, which resolves earlier static-analysis noise without affecting behaviour. Leave this structure as-is.docs/usage-guide.md (1)
205-206: LGTM!The documentation correctly describes the
--liveflag behaviour and uses neutral phrasing ("once a release is ready to ship") consistent with the style guidelines.tests/unit/publish/test_packaging.py (8)
34-49: LGTM!The
publish_plan_and_prepfixture cleanly encapsulates the common setup pattern, reducing duplication across tests. The return type annotation is explicit and matches the fixture's actual return value.
52-69: LGTM!
CallTrackingRunnercorrectly implements the_CommandRunnerprotocol signature, including the keyword-onlycwdandenvparameters. Recording(tuple(command), cwd)provides sufficient traceability for assertions.
72-86: LGTM!The return type
Callable[..., tuple[int, str, str]]addresses the previous review comment about the annotation mismatch. The ellipsis correctly indicates the inner function accepts additional keyword arguments.
89-109: LGTM!The helper consolidates repeated assertion logic effectively. The optional
not_expected_in_messageparameter handles both positive and negative content checks in a single function.
188-208: LGTM!The test verifies dry-run behaviour including the
--dry-runflag presence and info-level logging. The log capture setup withcaplog.set_levelis appropriate for asserting operational output.
210-223: LGTM!The live-mode test correctly asserts that
--dry-runis absent from the command tuple whenlive=True.
226-264: LGTM!The already-published continuation test correctly simulates exit code 101 with the "already uploaded" message, verifying that:
- Processing continues to the next crate
- A warning is logged
- No exception is raised
This aligns with the documented behaviour for handling existing crate versions.
267-279: LGTM!The failure test validates that unexpected errors (without already-published markers) raise
PublishPreflightErrorwith appropriate message content.lading/commands/publish.py (6)
48-48: LGTM!Module-level logger created with
__name__follows the coding guidelines for logging setup.
74-77: LGTM!The
liveparameter documentation clearly explains the dry-run default behaviour and the opt-in nature of live publishing.Also applies to: 99-99
338-352: LGTM!The shared
_format_cargo_failure_messagehelper eliminates the duplication between packaging and publishing error message construction. The function correctly prefers stderr over stdout for error details.
377-399: LGTM!The
_is_already_published_errorimplementation addresses the previous review concern about overly broad matching:
- Exit code 101 is required (cargo registry error)
- Markers are now specific to crates.io rather than generic "already exists"
This combination significantly reduces false-positive risk from unrelated failures.
402-437: LGTM!The
_publish_cratesfunction correctly implements the dry-run vs live flow:
- Conditionally includes
--dry-runbased onliveflag- Logs each invocation at INFO level
- Continues on already-published crates with a WARNING
- Raises
PublishPreflightErrorfor unexpected failuresThe lazy logging with
%splaceholders follows the coding guidelines.
505-510: LGTM!The integration correctly passes
effective_options.liveto_publish_crates, ensuring the CLI flag propagates through to the publish execution.tests/bdd/steps/test_publish_when_steps.py (4)
1-16: LGTM!The module structure follows the refactored pattern with imports from
test_publish_infrastructure. TheTYPE_CHECKINGguard correctly limits thePathimport to type-checking time.
22-33: LGTM!The preflight check step correctly captures
PublishPreflightErrorand returns it in the result dictionary for assertion in@thensteps.
36-63: LGTM!Both standard and
--forbid-dirtyvariants delegate to_invoke_publish_with_options, maintaining consistency and reducing duplication.
66-89: LGTM!The live-publish step:
- Uses the extracted
_is_cargo_publish_commandhelper as recommended in past review- Ensures a default successful cargo publish override exists if none configured
- Passes
--liveto the invocation helperThis addresses the past review comment about extracting the cargo publish detection logic.
tests/bdd/steps/test_publish_helpers.py (6)
1-14: LGTM!The module structure correctly uses
TYPE_CHECKINGto guard imports only needed for type annotations, avoiding runtime import costs.
30-49: LGTM!
_load_staged_manifestcomposes the parsing helpers cleanly._get_patch_entriesuses the positive conditional form as previously suggested, and_split_namesis a clear utility for comma-separated input.
52-100: LGTM!The invocation retrieval helpers provide consistent error messages when expected cargo commands are missing. Using walrus operator with
if invocations := ...is idiomatic for this pattern.
103-131: LGTM!The
_has_contiguous_argsfunction now usesany()as previously suggested. The separate contiguous and non-contiguous variants with a unified_has_ordered_argsdispatcher is a clean design.
134-169: LGTM!The consolidated
_assert_invocations_flag_presencewithshould_containboolean addresses the PR comment about reducing duplication between assertion functions that differed only by polarity. The wrapper functions_assert_invocations_have_flagand_assert_invocations_lack_flagprovide semantic clarity at call sites.
172-183: LGTM!
_assert_crate_order_matchesprovides clear diagnostic output when order assertions fail, including both observed and expected sequences.tests/bdd/steps/test_publish_then_steps.py (8)
1-31: LGTM!Module structure is clean: future annotations enabled, TYPE_CHECKING guard used correctly for
_PreflightInvocationRecorder, and helper imports are well-organised. The docstring follows NumPy guidelines succinctly.
44-73: LGTM!These three step functions are well-structured and correctly delegate to the helpers for manifest loading and patch entry retrieval.
96-98: LGTM!Clear single-purpose helper that improves readability in the calling code.
101-113: LGTM!Correctly asserts the presence of target-limiting flags in preflight test invocations.
116-125: LGTM!Clear iteration with explicit assertion for the absence of
--excludeflags.
139-161: LGTM!The
all()inversions from prior review feedback are correctly applied, making the assertions readable and correct.
227-292: LGTM!Skip-reporting assertions are well-structured. Each parses the output, locates the relevant section header, and validates entries. The pattern is consistent across all variants.
295-357: LGTM!The README staging assertions correctly verify file existence and content equality. The error-type check in
then_publish_preflight_reports_missing_socketproperly validates the exception class before inspecting the message.tests/bdd/steps/test_publish_infrastructure.py (10)
1-28: LGTM!Module structure is sound. The try/except for
CmdMoximport provides a clean runtime fallback, and TYPE_CHECKING guards are properly applied for type-only imports.
30-36: LGTM!Immutable, slotted dataclass adhering to project guidelines for structured data.
39-55: LGTM!Mutable recorder correctly uses
slots=Truewithoutfrozensince the records list is appended to during test execution.
58-64: LGTM!Frozen, slotted configuration dataclass correctly models the stub setup.
80-86: LGTM!Protocol and type alias are well-defined, enabling type-safe handler signatures.
89-108: LGTM!Argument prefix validation is clear and provides detailed error messages on mismatch.
111-123: LGTM!Clean delegation to the publish module's normalisation logic for cargo commands.
126-128: LGTM!Extracted predicate addresses the complex-conditional feedback from prior reviews.
131-151: LGTM!Handler factory correctly validates arguments, resolves callable or static responses, and records invocations when a recorder is provided.
154-164: LGTM!Straightforward factory function for stub configuration construction.
- Clarify publish.py run() docstring to reflect full publish flow. - Require PWD in environment for cargo publish pre-flight stub, ensuring stricter validation. - Add error handling for malformed staging lines in test helpers. - Make PreflightTestContext dataclass frozen and slot-enabled for better performance and immutability. - Clean up and simplify BDD test step functions handling publish plans and invocations. - Minor docs fix for usage-guide.md punctuation. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…onment handling - Expand docstring for publish CLI command to better describe pre-flight checks and publishing steps. - Refactor test_publish_infrastructure to add context manager for managing CMD_MOX_STUB_ENV_VAR environment variable. - Add notes to _register_preflight_commands explaining behavior of multiple cargo publish overrides. - Replace manual environment setup/teardown with context manager for clarity and safety in tests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Define a new exception PublishError inheriting from PublishPreflightError to represent failures occurring during the publish step after preflight checks. Update _publish_crates to raise PublishError instead of PublishPreflightError when cargo publish fails. This improves error semantics and classification. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/unit/publish/test_packaging.py:226-235` </location>
<code_context>
+ assert runner.calls == [(("cargo", "publish"), root) for root in expected_roots]
+
+
+def test_publish_crates_continue_when_version_already_uploaded(
+ tmp_path: Path, caplog: pytest.LogCaptureFixture
+) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Add a companion test for live mode when a crate version is already uploaded
`test_publish_crates_continue_when_version_already_uploaded` only covers the dry-run path (`live=False`). Since `_publish_crates` is supposed to treat already-published errors the same in live and dry-run modes, add coverage for `live=True` (either via a separate test or parametrization) using the same runner, and assert that both crates are still attempted in order and that the already-published error remains non-fatal. This will guard against the live branch diverging from dry-run behavior.
Suggested implementation:
```python
publish._publish_crates(plan, preparation, runner=runner, live=True)
expected_roots = [
staging_root / crate.root_path.relative_to(plan.workspace_root)
for crate in plan.publishable
]
assert runner.calls == [(("cargo", "publish"), root) for root in expected_roots]
def test_publish_crates_continue_when_version_already_uploaded_live(
plan_and_prep, caplog: pytest.LogCaptureFixture
) -> None:
plan, preparation = plan_and_prep
staging_root = preparation.staging_root
class RecordingRunner:
def __init__(self) -> None:
self.calls = []
self._call_count = 0
def __call__(self, args, cwd) -> None:
self._call_count += 1
self.calls.append((tuple(args), cwd))
if self._call_count == 1:
# Simulate an "already uploaded" error for the first crate; this
# should be treated as non-fatal by _publish_crates even in live mode.
raise RuntimeError("current package version is already uploaded")
runner = RecordingRunner()
# Exercise the live branch; already-uploaded errors should be non-fatal and
# all crates should still be attempted in order.
publish._publish_crates(plan, preparation, runner=runner, live=True)
expected_roots = [
staging_root / crate.root_path.relative_to(plan.workspace_root)
for crate in plan.publishable
]
assert runner.calls == [(("cargo", "publish"), root) for root in expected_roots]
```
1. In your codebase, you likely already have a reusable “recording runner” and/or a helper to simulate the “already uploaded” cargo error in the existing `test_publish_crates_continue_when_version_already_uploaded` (dry-run) test. You may want to:
- Replace the inner `RecordingRunner` class with that shared helper to keep behavior consistent across tests.
- Adjust the simulated exception type/message (`RuntimeError("current package version is already uploaded")`) to match whatever `_publish_crates` actually inspects (e.g. a specific exception class or error message substring).
2. If you prefer parametrization instead of a separate test, you can refactor both the existing dry-run test and this new live-mode test into a single `@pytest.mark.parametrize("live", [False, True])` test using the same runner and assertions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Add pytest parameterize decorator to test_publish_crates_continue_when_version_already_uploaded to test both live and dry-run scenarios by passing 'live' argument to _publish_crates. This improves test coverage for different publish modes. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- lading/lading/cli.py is usually changed with: lading/tests/unit/test_cli.py
- lading/lading/commands/publish.py is usually changed with: lading/tests/bdd/steps/test_publish_steps.py
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@coderabbitai Have the following now been resolved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9768 @leynos Yes, this issue has been resolved! ✅ The test @pytest.mark.parametrize(
"live",
[pytest.param(False, id="dry-run"), pytest.param(True, id="live")],
)
def test_publish_crates_continue_when_version_already_uploaded(
tmp_path: Path, caplog: pytest.LogCaptureFixture, *, live: bool
) -> None:
"""Already-published versions log a warning and continue."""
# ... test implementation ...
publish._publish_crates(plan, preparation, runner=runner, live=live)This implementation:
The parametrised approach (option 2 from the review comment) is cleaner than creating a separate test, as it ensures both modes remain consistent whilst avoiding duplication. The test guards against the live branch diverging from dry-run behaviour, exactly as requested. |
1 participant
Summary
Changes
Core functionality
CLI
Tests
Documentation
Design & Behavior
Testing plan
How to test manually
Compatibility
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/05f608c8-11dc-4ca3-b5b0-ccfb3a41a386
Summary by Sourcery
Implement default dry-run cargo publishing with an opt-in live mode and integrate crate publishing into the existing publish workflow after preflight checks and packaging.
New Features:
Enhancements:
Documentation:
Tests:
Chores: