Complete command pipeline extraction (#170) - #177
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
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:
Summary
WalkthroughChangesCommand pipeline extraction
Sequence Diagram(s)sequenceDiagram
participant CLI
participant publish
participant publish_staging
participant publish_pipeline
participant Cargo
CLI->>publish: run(options)
publish->>publish_staging: prepare_workspace(plan)
publish->>publish_pipeline: _dispatch_publication(state)
publish_pipeline->>Cargo: cargo package
publish_pipeline->>Cargo: cargo publish
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
📋 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 |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. lading/commands/publish_pipeline.py Comment on lines +336 to +388 def _dispatch_publication(
plan: PublishPlan,
preparation: PublishPreparation,
*,
options: _PublishExecutionOptions,
runner: CommandRunner,
) -> None:
"""Route to the live or dry-run publication pipeline.
Design note (issue #72): this helper is more than a relocated branch.
It owns the operator-facing pipeline-mode log line, sequences the
dry-run two-phase pipeline (package everything, then publish
everything), and gives tests a single seam to exercise mode dispatch
without driving ``run()`` end to end. Inlining it would push ``run()``
back toward the complexity ceiling that prompted the extraction.
"""
if options.live:
LOGGER.info("Publication mode: live (interleaved per-crate pipeline)")
_execute_live_publication_pipeline(
plan,
preparation,
options=options,
runner=runner,
)
else:
LOGGER.info("Publication mode: dry-run (batched two-phase pipeline)")
try:
_package_publishable_crates(
plan,
preparation,
options=options,
runner=runner,
)
except PublishPreparationError as exc:
LOGGER.exception("Dry-run pipeline: packaging phase failed")
raise PublishPreflightError(str(exc)) from exc
except PublishPreflightError:
LOGGER.exception("Dry-run pipeline: packaging phase failed")
raise
LOGGER.info("Dry-run pipeline: packaging complete; starting publish phase")
try:
_publish_crates(
plan,
preparation,
runner=runner,
options=options,
)
except PublishPreparationError as exc:
LOGGER.exception("Dry-run pipeline: publish phase failed")
raise PublishPreflightError(str(exc)) from exc
except PublishPreflightError:
LOGGER.exception("Dry-run pipeline: publish phase failed")
raise❌ New issue: Complex Method |
|
@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. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. tests/unit/publish/test_packaging.py Comment on lines +277 to +279 publish_plan_and_prep: tuple[
publish_plan.PublishPlan, publish_staging.PublishPreparation, Path
],❌ New issue: Code Duplication |
![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.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
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. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. tests/unit/publish/test_packaging.py Comment on lines +295 to +302 def test_package_publishable_crates_reports_failure_detail(
publish_plan_and_prep: tuple[
publish_plan.PublishPlan, publish_staging.PublishPreparation, Path
],
stdout: str,
stderr: str,
expected_in_message: str,
not_expected_in_message: str | None,❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (2 errors, 1 warning)
|
|
@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. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationtests/unit/test_publish_staging.py: What lead to degradation?The module contains 2 functions with similar structure: test_prepare_workspace_copies_workspace_readme_verbatim,test_prepare_workspace_does_not_adopt_workspace_readme_for_crates 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 |
bba7bfa to
1ea8053
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
|
@coderabbitai Have the following now been resolved? tests/unit/test_publish_staging.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- test module outline ---'
ast-grep outline tests/unit/test_publish_staging.py --items all
printf '%s\n' '--- reported test and README-related tests ---'
rg -n -A70 -B15 \
'def test_prepare_workspace_skips_readme_staging\b|def test_prepare_workspace_copies_workspace_readme\b|def test_prepare_workspace_.*readme\b|README\.md|copied_readmes' \
tests/unit/test_publish_staging.py
printf '%s\n' '--- current staging entrypoint and copy helper ---'
ast-grep outline lading/commands/publish_staging.py --items all
rg -n -A115 -B20 \
'def prepare_workspace\b|def _copy_workspace_tree\b|README\.md|copied_readmes' \
lading/commands/publish_staging.pyLength of output: 17536
Treat this finding as resolved. The stale test no longer exists in the current codegraph. Its replacement at Lines 284–312 now performs meaningful assertions for both crate-spec variants:
This matches the current Keep the separate duplication follow-up in scope: |
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (2 errors, 1 warning)
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lading/commands/publish.py (1)
254-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
effective_optionstoprepare_workspacefor a single source of truth.
prepare_workspace(plan, options=options)feeds the raw parameter, so whenoptions is Nonestaging rebuilds its own defaultPublishOptionsindependently of theeffective_optionsalready computed on Line 237. The results coincide today, but wiringeffective_optionskeeps one authoritative options object flowing through the run.♻️ Proposed tweak
- preparation = publish_staging.prepare_workspace(plan, options=options) + preparation = publish_staging.prepare_workspace(plan, options=effective_options)🤖 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 `@lading/commands/publish.py` around lines 254 - 272, Update the publish flow to pass effective_options, rather than the raw options parameter, to publish_staging.prepare_workspace. Keep the existing _PublishExecutionOptions construction and _dispatch_publication call unchanged so effective_options remains the single authoritative configuration throughout the run.
🤖 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 `@docs/lading-design.md`:
- Around line 520-541: Align the changed sequence diagram with the call order in
publish.py: show plan_publication and prepare_workspace before
_run_preflight_checks, then dispatch publication only after pre-flight succeeds.
Update the relevant participant messages and success/failure branches while
preserving the documented _invoke behavior and PublishPreflightError handling.
In `@lading/commands/bump_pipeline.py`:
- Around line 138-147: Remove the try/except and _log.exception call around
bump_readme.transpose_readme_to_crate in the pipeline stage, allowing
ReadmeTranspositionError to propagate unchanged to the CLI or bump.run boundary
for single-point logging.
In `@lading/commands/publish_pipeline.py`:
- Line 41: Update the module-level LOGGER declaration to obtain the logger via
logging.getLogger(__name__) instead of the hard-coded "lading.commands.publish"
name, preserving the existing LOGGER symbol for callers.
---
Outside diff comments:
In `@lading/commands/publish.py`:
- Around line 254-272: Update the publish flow to pass effective_options, rather
than the raw options parameter, to publish_staging.prepare_workspace. Keep the
existing _PublishExecutionOptions construction and _dispatch_publication call
unchanged so effective_options remains the single authoritative configuration
throughout the run.
🪄 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: 85279781-11df-432c-a833-a6512aeaa9c2
📒 Files selected for processing (33)
docs/developers-guide.mddocs/lading-design.mdlading/commands/bump.pylading/commands/bump_manifests.pylading/commands/bump_pipeline.pylading/commands/publish.pylading/commands/publish_index_check.pylading/commands/publish_manifest.pylading/commands/publish_pipeline.pylading/commands/publish_staging.pyscripts/typos_rollout.pytests/unit/publish/conftest.pytests/unit/publish/preflight_test_utils.pytests/unit/publish/test_command_logging.pytests/unit/publish/test_downgrade_metrics.pytests/unit/publish/test_index_order_properties.pytests/unit/publish/test_packaging.pytests/unit/publish/test_phase_dispatch.pytests/unit/publish/test_plan_validation.pytests/unit/publish/test_run_preflight.pytests/unit/publish/test_run_publish_ordering.pytests/unit/publish/test_run_workspace_config.pytests/unit/publish/test_snapshot_messages.pytests/unit/test_bump_context_properties.pytests/unit/test_bump_manifest_writing.pytests/unit/test_bump_result_formatting.pytests/unit/test_bump_selectors.pytests/unit/test_dependency_sections.pytests/unit/test_publish_formatting.pytests/unit/test_publish_patch_strategy.pytests/unit/test_publish_planning.pytests/unit/test_publish_staging.pytypos.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/cuprum(auto-detected)leynos/shared-actions(auto-detected)
28746b8 to
e643151
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_publish_staging_properties.py`:
- Around line 51-52: Add diagnostic messages to both assertions in the
path-safety test, including the relevant resolved paths and relationship being
validated so Hypothesis counterexamples identify the failure immediately.
Preserve the existing assertion conditions and behavior.
🪄 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: 93076247-a339-4113-a9da-2ce7c5b4e446
📒 Files selected for processing (33)
docs/developers-guide.mddocs/lading-design.mdlading/commands/bump.pylading/commands/bump_manifests.pylading/commands/bump_pipeline.pylading/commands/publish.pylading/commands/publish_index_check.pylading/commands/publish_manifest.pylading/commands/publish_pipeline.pylading/commands/publish_staging.pyscripts/typos_rollout.pytests/unit/publish/conftest.pytests/unit/publish/preflight_test_utils.pytests/unit/publish/test_command_logging.pytests/unit/publish/test_downgrade_metrics.pytests/unit/publish/test_index_order_properties.pytests/unit/publish/test_packaging.pytests/unit/publish/test_phase_dispatch.pytests/unit/publish/test_run_preflight.pytests/unit/publish/test_run_preflight_properties.pytests/unit/publish/test_run_publish_ordering.pytests/unit/publish/test_run_workspace_config.pytests/unit/publish/test_snapshot_messages.pytests/unit/test_bump_context_properties.pytests/unit/test_bump_manifest_writing.pytests/unit/test_bump_result_formatting.pytests/unit/test_bump_selectors.pytests/unit/test_dependency_sections.pytests/unit/test_publish_formatting.pytests/unit/test_publish_patch_strategy.pytests/unit/test_publish_planning.pytests/unit/test_publish_staging.pytests/unit/test_publish_staging_properties.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/cuprum(auto-detected)leynos/shared-actions(auto-detected)
Move bump orchestration, publication execution, and workspace staging into canonical delegate modules. Repoint tests to those module boundaries and document the extraction map.
Extract the dry-run phase error boundary so publication dispatch retains the existing log and exception contract without duplicating its control flow. Add direct coverage for packaging, publish, and successful phases.
Cover stdout fallback and stderr precedence through one shared test body, removing duplicate setup while retaining both behavioural cases.
Remove unused staging input, test invocation helpers at their canonical owner, and replace the no-op README staging test with behavioural coverage. Document the coordinator, staging, pipeline, and execution boundaries.
Keep the test below the parameter-count threshold while preserving both failure-detail scenarios and their existing assertions.
Retain the parametrized behavioural coverage for README copying and crate non-adoption while removing its redundant one-case counterpart.
Preserve the extracted pipeline boundaries while correcting logger ownership, centralising README error propagation, and retaining the public publish-plan exception contract. Add generated coverage for staging-path safety and preflight failure cut-off behaviour.
e643151 to
6242ccc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Report resolved paths and the expected containment relationship when a Hypothesis staging-path counterexample fails.
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
Absence of Expected Change Pattern
- lading/lading/commands/bump.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
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@lading/commands/publish_staging.py`:
- Around line 147-151: Update _format_preparation_summary to return a tuple
literal containing the two existing summary lines directly, removing the
intermediate lines list and tuple conversion while preserving the current text
and order.
- Around line 36-45: Update the PublishPreparation docstring’s Attributes entry
so staging_root includes its type using numpydoc syntax, changing the field
header to use “staging_root : Path” while preserving the existing description.
- Around line 169-172: Remove the stale “no cover” pragma from the
staged_root.exists() guard in the staged crate publication preparation logic,
leaving the defensive check and PublishPreparationError behavior unchanged.
- Around line 115-144: Track whether build_directory was auto-created before
calling _normalise_build_directory. In the _cleanup closure within the
preparation flow, remove the entire auto-created directory when no explicit
build_directory was supplied; otherwise retain the current staging_root-only
cleanup for caller-supplied directories.
In `@tests/unit/publish/test_phase_dispatch.py`:
- Around line 55-101: Update all three _run_dry_run_phase tests to pass
publish_pipeline.LOGGER.name to caplog.set_level instead of the hard-coded
lading.commands.publish logger name, so the tests configure the logger used by
publish_pipeline.LOGGER.exception.
🪄 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: b99ad2b2-561b-44ca-a120-dc0929cce2e3
📒 Files selected for processing (32)
docs/developers-guide.mddocs/lading-design.mdlading/commands/bump.pylading/commands/bump_manifests.pylading/commands/bump_pipeline.pylading/commands/publish.pylading/commands/publish_index_check.pylading/commands/publish_manifest.pylading/commands/publish_pipeline.pylading/commands/publish_staging.pytests/unit/publish/conftest.pytests/unit/publish/preflight_test_utils.pytests/unit/publish/test_command_logging.pytests/unit/publish/test_downgrade_metrics.pytests/unit/publish/test_index_order_properties.pytests/unit/publish/test_packaging.pytests/unit/publish/test_phase_dispatch.pytests/unit/publish/test_run_preflight.pytests/unit/publish/test_run_preflight_properties.pytests/unit/publish/test_run_publish_ordering.pytests/unit/publish/test_run_workspace_config.pytests/unit/publish/test_snapshot_messages.pytests/unit/test_bump_context_properties.pytests/unit/test_bump_manifest_writing.pytests/unit/test_bump_result_formatting.pytests/unit/test_bump_selectors.pytests/unit/test_dependency_sections.pytests/unit/test_publish_formatting.pytests/unit/test_publish_patch_strategy.pytests/unit/test_publish_planning.pytests/unit/test_publish_staging.pytests/unit/test_publish_staging_properties.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/cuprum(auto-detected)leynos/shared-actions(auto-detected)
| class PublishPreparation: | ||
| """Details about the staged workspace copy. | ||
|
|
||
| Attributes | ||
| ---------- | ||
| staging_root: | ||
| Root of the copied workspace used by publication commands. | ||
| """ | ||
|
|
||
| staging_root: Path |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Fix the numpydoc Attributes entry to include the type.
staging_root: should read staging_root : Path per the project's numpydoc convention.
✏️ Proposed fix
Attributes
----------
- staging_root:
+ staging_root : Path
Root of the copied workspace used by publication commands.As per path instructions, "Docstrings must follow the numpy style guide."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class PublishPreparation: | |
| """Details about the staged workspace copy. | |
| Attributes | |
| ---------- | |
| staging_root: | |
| Root of the copied workspace used by publication commands. | |
| """ | |
| staging_root: Path | |
| class PublishPreparation: | |
| """Details about the staged workspace copy. | |
| Attributes | |
| ---------- | |
| staging_root : Path | |
| Root of the copied workspace used by publication commands. | |
| """ | |
| staging_root: 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 `@lading/commands/publish_staging.py` around lines 36 - 45, Update the
PublishPreparation docstring’s Attributes entry so staging_root includes its
type using numpydoc syntax, changing the field header to use “staging_root :
Path” while preserving the existing description.
Source: Path instructions
| if options is None: | ||
| from lading.commands.publish import PublishOptions | ||
|
|
||
| active_options = PublishOptions() | ||
| else: | ||
| active_options = options | ||
| build_directory = _normalise_build_directory( | ||
| plan.workspace_root, active_options.build_directory | ||
| ) | ||
| LOGGER.info( | ||
| "Preparing staged workspace for publication under %s", | ||
| build_directory, | ||
| ) | ||
| staging_root = _copy_workspace_tree( | ||
| plan.workspace_root, | ||
| build_directory, | ||
| preserve_symlinks=active_options.preserve_symlinks, | ||
| ) | ||
| LOGGER.info("Staged workspace created at %s", staging_root) | ||
| LOGGER.info("Workspace README staging skipped; handled by lading bump") | ||
| preparation = PublishPreparation(staging_root=staging_root) | ||
| if active_options.cleanup: | ||
| cleanup_target = staging_root | ||
|
|
||
| def _cleanup() -> None: | ||
| """Remove the staged workspace tree on process exit.""" | ||
| shutil.rmtree(cleanup_target, ignore_errors=True) | ||
|
|
||
| atexit.register(_cleanup) | ||
| return preparation |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clean up the whole auto-created temp directory, not just the staged copy.
When build_directory is None, _normalise_build_directory mints a fresh tempfile.mkdtemp directory that lading owns exclusively — nothing else can be in it. The _cleanup closure only removes staging_root (the <build_directory>/<workspace_name> subtree), so the empty lading-publish-* parent directory is left behind on every dry-run invocation with cleanup=True and no explicit build_directory. Track whether the directory was auto-created and sweep the whole thing in that case, while still preserving caller-supplied directories as intended.
🧹 Proposed fix
- if options is None:
- from lading.commands.publish import PublishOptions
-
- active_options = PublishOptions()
- else:
- active_options = options
+ if options is None:
+ from lading.commands.publish import PublishOptions
+
+ active_options = PublishOptions()
+ else:
+ active_options = options
+ owns_build_directory = active_options.build_directory is None
build_directory = _normalise_build_directory(
plan.workspace_root, active_options.build_directory
)
LOGGER.info(
"Preparing staged workspace for publication under %s",
build_directory,
)
staging_root = _copy_workspace_tree(
plan.workspace_root,
build_directory,
preserve_symlinks=active_options.preserve_symlinks,
)
LOGGER.info("Staged workspace created at %s", staging_root)
LOGGER.info("Workspace README staging skipped; handled by lading bump")
preparation = PublishPreparation(staging_root=staging_root)
if active_options.cleanup:
- cleanup_target = staging_root
+ cleanup_target = build_directory if owns_build_directory else staging_root
def _cleanup() -> None:
"""Remove the staged workspace tree on process exit."""
shutil.rmtree(cleanup_target, ignore_errors=True)
atexit.register(_cleanup)
return preparation📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if options is None: | |
| from lading.commands.publish import PublishOptions | |
| active_options = PublishOptions() | |
| else: | |
| active_options = options | |
| build_directory = _normalise_build_directory( | |
| plan.workspace_root, active_options.build_directory | |
| ) | |
| LOGGER.info( | |
| "Preparing staged workspace for publication under %s", | |
| build_directory, | |
| ) | |
| staging_root = _copy_workspace_tree( | |
| plan.workspace_root, | |
| build_directory, | |
| preserve_symlinks=active_options.preserve_symlinks, | |
| ) | |
| LOGGER.info("Staged workspace created at %s", staging_root) | |
| LOGGER.info("Workspace README staging skipped; handled by lading bump") | |
| preparation = PublishPreparation(staging_root=staging_root) | |
| if active_options.cleanup: | |
| cleanup_target = staging_root | |
| def _cleanup() -> None: | |
| """Remove the staged workspace tree on process exit.""" | |
| shutil.rmtree(cleanup_target, ignore_errors=True) | |
| atexit.register(_cleanup) | |
| return preparation | |
| if options is None: | |
| from lading.commands.publish import PublishOptions | |
| active_options = PublishOptions() | |
| else: | |
| active_options = options | |
| owns_build_directory = active_options.build_directory is None | |
| build_directory = _normalise_build_directory( | |
| plan.workspace_root, active_options.build_directory | |
| ) | |
| LOGGER.info( | |
| "Preparing staged workspace for publication under %s", | |
| build_directory, | |
| ) | |
| staging_root = _copy_workspace_tree( | |
| plan.workspace_root, | |
| build_directory, | |
| preserve_symlinks=active_options.preserve_symlinks, | |
| ) | |
| LOGGER.info("Staged workspace created at %s", staging_root) | |
| LOGGER.info("Workspace README staging skipped; handled by lading bump") | |
| preparation = PublishPreparation(staging_root=staging_root) | |
| if active_options.cleanup: | |
| cleanup_target = build_directory if owns_build_directory else staging_root | |
| def _cleanup() -> None: | |
| """Remove the staged workspace tree on process exit.""" | |
| shutil.rmtree(cleanup_target, ignore_errors=True) | |
| atexit.register(_cleanup) | |
| return preparation |
🤖 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 `@lading/commands/publish_staging.py` around lines 115 - 144, Track whether
build_directory was auto-created before calling _normalise_build_directory. In
the _cleanup closure within the preparation flow, remove the entire auto-created
directory when no explicit build_directory was supplied; otherwise retain the
current staging_root-only cleanup for caller-supplied directories.
| def _format_preparation_summary(preparation: PublishPreparation) -> tuple[str, ...]: | ||
| """Return formatted summary lines for staging results.""" | ||
| lines = [f"Staged workspace at: {preparation.staging_root}"] | ||
| lines.append("Workspace READMEs are handled by lading bump.") | ||
| return tuple(lines) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Simplify to a tuple literal.
Building a list then wrapping it in tuple() is unnecessary for two static lines.
✏️ Proposed fix
def _format_preparation_summary(preparation: PublishPreparation) -> tuple[str, ...]:
"""Return formatted summary lines for staging results."""
- lines = [f"Staged workspace at: {preparation.staging_root}"]
- lines.append("Workspace READMEs are handled by lading bump.")
- return tuple(lines)
+ return (
+ f"Staged workspace at: {preparation.staging_root}",
+ "Workspace READMEs are handled by lading bump.",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _format_preparation_summary(preparation: PublishPreparation) -> tuple[str, ...]: | |
| """Return formatted summary lines for staging results.""" | |
| lines = [f"Staged workspace at: {preparation.staging_root}"] | |
| lines.append("Workspace READMEs are handled by lading bump.") | |
| return tuple(lines) | |
| def _format_preparation_summary(preparation: PublishPreparation) -> tuple[str, ...]: | |
| """Return formatted summary lines for staging results.""" | |
| return ( | |
| f"Staged workspace at: {preparation.staging_root}", | |
| "Workspace READMEs are handled by lading bump.", | |
| ) |
🤖 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 `@lading/commands/publish_staging.py` around lines 147 - 151, Update
_format_preparation_summary to return a tuple literal containing the two
existing summary lines directly, removing the intermediate lines list and tuple
conversion while preserving the current text and order.
| staged_root = staging_root / relative_root | ||
| if not staged_root.exists(): # pragma: no cover - defensive guard | ||
| message = f"Staged crate root not found for {crate.name!r}: {staged_root}" | ||
| raise PublishPreparationError(message) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drop the stale no cover pragma — this branch is now exercised.
test_execute_live_publication_pipeline_wraps_preparation_errors removes a staged crate directory and drives this exact "staged crate root not found" branch, so the exclusion comment no longer reflects reality and would silently hide a coverage regression if that test is ever removed.
🧹 Proposed fix
staged_root = staging_root / relative_root
- if not staged_root.exists(): # pragma: no cover - defensive guard
+ if not staged_root.exists():
message = f"Staged crate root not found for {crate.name!r}: {staged_root}"
raise PublishPreparationError(message)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| staged_root = staging_root / relative_root | |
| if not staged_root.exists(): # pragma: no cover - defensive guard | |
| message = f"Staged crate root not found for {crate.name!r}: {staged_root}" | |
| raise PublishPreparationError(message) | |
| staged_root = staging_root / relative_root | |
| if not staged_root.exists(): | |
| message = f"Staged crate root not found for {crate.name!r}: {staged_root}" | |
| raise PublishPreparationError(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 `@lading/commands/publish_staging.py` around lines 169 - 172, Remove the stale
“no cover” pragma from the staged_root.exists() guard in the staged crate
publication preparation logic, leaving the defensive check and
PublishPreparationError behavior unchanged.
| def test_run_dry_run_phase_normalises_packaging_preparation_failure( | ||
| caplog: pytest.LogCaptureFixture, | ||
| ) -> None: | ||
| """A packaging preparation error becomes a preflight error.""" | ||
| caplog.set_level(logging.ERROR, logger="lading.commands.publish") | ||
| failure = publish_pipeline.PublishPreparationError("staging failed") | ||
|
|
||
| with pytest.raises( | ||
| publish.PublishPreflightError, match="staging failed" | ||
| ) as excinfo: | ||
| publish_pipeline._run_dry_run_phase( | ||
| "packaging", | ||
| lambda: (_ for _ in ()).throw(failure), | ||
| ) | ||
|
|
||
| assert excinfo.value.__cause__ is failure | ||
| assert caplog.messages == ["Dry-run pipeline: packaging phase failed"] | ||
|
|
||
|
|
||
| def test_run_dry_run_phase_reraises_publish_preflight_failure( | ||
| caplog: pytest.LogCaptureFixture, | ||
| ) -> None: | ||
| """A publish preflight error retains its type and traceback.""" | ||
| caplog.set_level(logging.ERROR, logger="lading.commands.publish") | ||
| failure = publish.PublishPreflightError("publish failed") | ||
|
|
||
| with pytest.raises(publish.PublishPreflightError) as excinfo: | ||
| publish_pipeline._run_dry_run_phase( | ||
| "publish", | ||
| lambda: (_ for _ in ()).throw(failure), | ||
| ) | ||
|
|
||
| assert excinfo.value is failure | ||
| assert caplog.messages == ["Dry-run pipeline: publish phase failed"] | ||
|
|
||
|
|
||
| def test_run_dry_run_phase_succeeds_without_logging( | ||
| caplog: pytest.LogCaptureFixture, | ||
| ) -> None: | ||
| """A successful phase invokes its action without emitting an error.""" | ||
| caplog.set_level(logging.ERROR, logger="lading.commands.publish") | ||
| calls: list[str] = [] | ||
|
|
||
| publish_pipeline._run_dry_run_phase("packaging", lambda: calls.append("ran")) | ||
|
|
||
| assert calls == ["ran"] | ||
| assert caplog.messages == [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Point caplog.set_level at the actual pipeline logger.
All three new _run_dry_run_phase tests call caplog.set_level(logging.ERROR, logger="lading.commands.publish"), but publish_pipeline.LOGGER is logging.getLogger(__name__), i.e. "lading.commands.publish_pipeline" — a sibling logger, not the one _run_dry_run_phase actually writes to. The tests pass today only because Python's root logger defaults to WARNING and LOGGER.exception fires at ERROR, so the level check is a no-op. Point the calls at publish_pipeline.LOGGER.name, matching the pattern already used correctly elsewhere in this cohort (e.g. test_snapshot_messages.py's test_pipeline_info_log_snapshot).
🛠️ Proposed fix
def test_run_dry_run_phase_normalises_packaging_preparation_failure(
caplog: pytest.LogCaptureFixture,
) -> None:
"""A packaging preparation error becomes a preflight error."""
- caplog.set_level(logging.ERROR, logger="lading.commands.publish")
+ caplog.set_level(logging.ERROR, logger=publish_pipeline.LOGGER.name)
failure = publish_pipeline.PublishPreparationError("staging failed")
@@
def test_run_dry_run_phase_reraises_publish_preflight_failure(
caplog: pytest.LogCaptureFixture,
) -> None:
"""A publish preflight error retains its type and traceback."""
- caplog.set_level(logging.ERROR, logger="lading.commands.publish")
+ caplog.set_level(logging.ERROR, logger=publish_pipeline.LOGGER.name)
failure = publish.PublishPreflightError("publish failed")
@@
def test_run_dry_run_phase_succeeds_without_logging(
caplog: pytest.LogCaptureFixture,
) -> None:
"""A successful phase invokes its action without emitting an error."""
- caplog.set_level(logging.ERROR, logger="lading.commands.publish")
+ caplog.set_level(logging.ERROR, logger=publish_pipeline.LOGGER.name)
calls: list[str] = []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_run_dry_run_phase_normalises_packaging_preparation_failure( | |
| caplog: pytest.LogCaptureFixture, | |
| ) -> None: | |
| """A packaging preparation error becomes a preflight error.""" | |
| caplog.set_level(logging.ERROR, logger="lading.commands.publish") | |
| failure = publish_pipeline.PublishPreparationError("staging failed") | |
| with pytest.raises( | |
| publish.PublishPreflightError, match="staging failed" | |
| ) as excinfo: | |
| publish_pipeline._run_dry_run_phase( | |
| "packaging", | |
| lambda: (_ for _ in ()).throw(failure), | |
| ) | |
| assert excinfo.value.__cause__ is failure | |
| assert caplog.messages == ["Dry-run pipeline: packaging phase failed"] | |
| def test_run_dry_run_phase_reraises_publish_preflight_failure( | |
| caplog: pytest.LogCaptureFixture, | |
| ) -> None: | |
| """A publish preflight error retains its type and traceback.""" | |
| caplog.set_level(logging.ERROR, logger="lading.commands.publish") | |
| failure = publish.PublishPreflightError("publish failed") | |
| with pytest.raises(publish.PublishPreflightError) as excinfo: | |
| publish_pipeline._run_dry_run_phase( | |
| "publish", | |
| lambda: (_ for _ in ()).throw(failure), | |
| ) | |
| assert excinfo.value is failure | |
| assert caplog.messages == ["Dry-run pipeline: publish phase failed"] | |
| def test_run_dry_run_phase_succeeds_without_logging( | |
| caplog: pytest.LogCaptureFixture, | |
| ) -> None: | |
| """A successful phase invokes its action without emitting an error.""" | |
| caplog.set_level(logging.ERROR, logger="lading.commands.publish") | |
| calls: list[str] = [] | |
| publish_pipeline._run_dry_run_phase("packaging", lambda: calls.append("ran")) | |
| assert calls == ["ran"] | |
| assert caplog.messages == [] | |
| def test_run_dry_run_phase_normalises_packaging_preparation_failure( | |
| caplog: pytest.LogCaptureFixture, | |
| ) -> None: | |
| """A packaging preparation error becomes a preflight error.""" | |
| caplog.set_level(logging.ERROR, logger=publish_pipeline.LOGGER.name) | |
| failure = publish_pipeline.PublishPreparationError("staging failed") | |
| with pytest.raises( | |
| publish.PublishPreflightError, match="staging failed" | |
| ) as excinfo: | |
| publish_pipeline._run_dry_run_phase( | |
| "packaging", | |
| lambda: (_ for _ in ()).throw(failure), | |
| ) | |
| assert excinfo.value.__cause__ is failure | |
| assert caplog.messages == ["Dry-run pipeline: packaging phase failed"] | |
| def test_run_dry_run_phase_reraises_publish_preflight_failure( | |
| caplog: pytest.LogCaptureFixture, | |
| ) -> None: | |
| """A publish preflight error retains its type and traceback.""" | |
| caplog.set_level(logging.ERROR, logger=publish_pipeline.LOGGER.name) | |
| failure = publish.PublishPreflightError("publish failed") | |
| with pytest.raises(publish.PublishPreflightError) as excinfo: | |
| publish_pipeline._run_dry_run_phase( | |
| "publish", | |
| lambda: (_ for _ in ()).throw(failure), | |
| ) | |
| assert excinfo.value is failure | |
| assert caplog.messages == ["Dry-run pipeline: publish phase failed"] | |
| def test_run_dry_run_phase_succeeds_without_logging( | |
| caplog: pytest.LogCaptureFixture, | |
| ) -> None: | |
| """A successful phase invokes its action without emitting an error.""" | |
| caplog.set_level(logging.ERROR, logger=publish_pipeline.LOGGER.name) | |
| calls: list[str] = [] | |
| publish_pipeline._run_dry_run_phase("packaging", lambda: calls.append("ran")) | |
| assert calls == ["ran"] | |
| assert caplog.messages == [] |
🤖 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/publish/test_phase_dispatch.py` around lines 55 - 101, Update all
three _run_dry_run_phase tests to pass publish_pipeline.LOGGER.name to
caplog.set_level instead of the hard-coded lading.commands.publish logger name,
so the tests configure the logger used by publish_pipeline.LOGGER.exception.
2 participants
Summary
This branch completes the command-module extraction deferred from PR #136. It moves bump run-sequence orchestration, per-crate publication execution, and workspace staging into canonical delegate modules so every affected source file remains below the 400-line guideline without compatibility shims.
All tests now patch and call the modules that own the moved symbols. The developer extraction map records the resulting boundaries, and staging cleanup no longer removes caller-owned build-directory contents.
Closes #170.
Review walkthrough
Validation
make check-fmt: passedmake lint: passed (Ruff, interrogate 100%, Pylint 10/10)make typecheck: passedmake test: passed (741 tests; 70 snapshots)make markdownlint: passedmake nixie: passedcoderabbit review --agent: completed after the required rate-limit back-off; actionable extraction concerns resolvedNotes
The five affected source modules are 208, 256, 280, 388, and 177 lines respectively, all below the 400-line guideline.