feat(packaging): promote shipper-cli to real library adapter (#95 PR 2)#141
Merged
Merged
Conversation
Second step of the #95 three-crate split. `shipper-cli` was a 9-line compat shim after PR 1; this PR makes it the real CLI adapter. ## Architecture after this PR shipper (install façade, library re-export) -> shipper-cli (CLI: clap, subcommands, output, exposes pub fn run()) -> shipper-core (engine — no CLI deps) The old arrow `shipper-cli -> shipper` is inverted: `shipper-cli` no longer depends on `shipper`. The `shipper` package now depends on `shipper-cli` (for the binary) and `shipper-core` (for the re-export surface). ## What moved - `crates/shipper/src/cli/mod.rs` -> `crates/shipper-cli/src/lib.rs` - `crates/shipper/src/cli/output/` -> `crates/shipper-cli/src/output/` - 22 insta snapshots renamed `shipper__cli__output__*.snap` -> `shipper_cli__output__*.snap` (new module_path: `shipper_cli::output::...`) - Duplicate `shipper__output__*.snap` files (without the `cli::` segment) removed — they were stale copies from a previous rename. ## Import rewrites Inside the moved CLI: - `crate::engine::X` -> `shipper_core::engine::X` - `crate::plan` -> `shipper_core::plan` - `crate::types::X` -> `shipper_core::types::X` - `crate::config::X` -> `shipper_core::config::X` - `crate::cargo` -> `shipper_core::cargo` - `crate::state::X` -> `shipper_core::state::X` - `crate::lock::X` -> `shipper_core::lock::X` - `crate::webhook::X` -> `shipper_core::webhook::X` - `crate::encryption::X` -> `shipper_core::encryption::X` - `crate::auth::X` -> `shipper_core::auth::X` - `crate::registry::X` -> `shipper_core::registry::X` - `crate::retry::X` -> `shipper_core::retry::X` - `crate::git::X` -> `shipper_core::git::X` - `crate::cli::output::X` -> `crate::output::X` (cli flattened) And in `crates/shipper-cli/tests/` (bdd_publish, bdd_parallel, bdd_error_recovery, bdd_error_handling): - `shipper::cargo_failure::X` -> `shipper_core::cargo_failure::X` - `shipper::types::X` -> `shipper_core::types::X` - `shipper::plan::X` -> `shipper_core::plan::X` - `shipper::state::X` -> `shipper_core::state::X` - `shipper::store::X` -> `shipper_core::store::X` ## Cargo.toml changes - **`crates/shipper-cli/Cargo.toml`**: drops `shipper.workspace = true`, adds `shipper-core.workspace = true` + `clap` + `clap_complete` + `indicatif` + `serde`/`serde_json`/`toml`/`chrono` (CLI-local deps). - **`crates/shipper/Cargo.toml`**: shrinks massively. Runtime deps go from 20+ to just `shipper-core` + `shipper-cli` + `anyhow`. All engine and CLI-only dependencies drop out (they're now transitive). The dev-dep set shrinks to what the integration tests in `crates/shipper/tests/` actually reach for directly. ## Binaries - `crates/shipper/src/bin/shipper.rs` — now forwards to `shipper_cli::run()` (was `shipper::cli::run()`). - `crates/shipper-cli/src/main.rs` — now forwards to `shipper_cli::run()` (same-crate call; was `shipper::cli::run()`). Both binaries end up calling the same `pub fn run()` in `shipper-cli/src/lib.rs`. `cargo install shipper` and `cargo install shipper-cli` both produce working CLIs. ## Library surface `crates/shipper/src/lib.rs` drops `pub mod cli;` — the CLI is no longer part of the `shipper` library's public surface. Programmatic callers who wanted the CLI entrypoint should depend on `shipper-cli` directly and call `shipper_cli::run()`. The engine re-exports (`shipper::engine`, `shipper::plan`, etc.) stay in place, so integration tests in `crates/shipper/tests/` that use `shipper::X` paths keep working. PR 3 tightens this to a curated re-export list. ## Verification - `cargo check --workspace --all-targets` — clean - `cargo clippy --workspace --all-targets --all-features -- -D warnings` — clean - `cargo test --workspace --doc` — all doc tests pass - `cargo test -p shipper-cli --lib` — 106 passed - Full `cargo test --workspace` — passed except pre-existing Windows-only `preflight_command_*` temp-dir flakes (unrelated).
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 14 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (45)
📒 Files selected for processing (18)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request implements a major architectural refactor by splitting the project into three crates: a facade (shipper), a CLI adapter (shipper-cli), and a core engine (shipper-core). While the restructuring is comprehensive, several critical issues were identified. A high-severity bug exists in the receipt inspection logic where references to temporary strings are returned, which will cause a compilation error. Additionally, there are significant inconsistencies across the Yank, PlanYank, and FixForward commands regarding the resolution of the state directory relative to the workspace root, as well as issues with registry defaulting and workspace root usage in the Yank command implementation.
Comment on lines
+1689
to
1700
| shipper_core::types::PackageState::Published => "\x1b[32mPublished\x1b[0m", | ||
| shipper_core::types::PackageState::Pending => "Pending", | ||
| shipper_core::types::PackageState::Uploaded => "\x1b[33mUploaded\x1b[0m", | ||
| shipper_core::types::PackageState::Skipped { reason } => { | ||
| &format!("Skipped: {}", reason) | ||
| } | ||
| shipper_core::types::PackageState::Failed { class, message } => { | ||
| &format!("\x1b[31mFailed ({:?}): {}\x1b[0m", class, message) | ||
| } | ||
| crate::types::PackageState::Ambiguous { message } => { | ||
| shipper_core::types::PackageState::Ambiguous { message } => { | ||
| &format!("\x1b[33mAmbiguous: {}\x1b[0m", message) | ||
| } |
There was a problem hiding this comment.
This code contains a critical bug: it attempts to return references to temporary String objects created by format!. These temporaries are dropped at the end of the match expression, leaving state_str as a dangling pointer when it is used in the subsequent println! call. This will cause a compilation error.
To fix this, you should ensure that all arms of the match return an owned String. While slightly less efficient than using &str for static constants, it is safe and correct for a CLI reporting tool.
shipper_core::types::PackageState::Published => "\x1b[32mPublished\x1b[0m".to_string(),
shipper_core::types::PackageState::Pending => "Pending".to_string(),
shipper_core::types::PackageState::Uploaded => "\x1b[33mUploaded\x1b[0m".to_string(),
shipper_core::types::PackageState::Skipped { reason } => format!("Skipped: {}", reason),
shipper_core::types::PackageState::Failed { class, message } => {
format!("\x1b[31mFailed ({:?}): {}\x1b[0m", class, message)
}
shipper_core::types::PackageState::Ambiguous { message } => {
format!("\x1b[33mAmbiguous: {}\x1b[0m", message)
}| let receipt_path = from_receipt.unwrap_or_else(|| { | ||
| opts.state_dir | ||
| .join(crate::state::execution_state::RECEIPT_FILE) | ||
| .join(shipper_core::state::execution_state::RECEIPT_FILE) |
There was a problem hiding this comment.
The opts.state_dir is used here directly without being resolved against planned.workspace_root. This is inconsistent with other commands like publish or inspect-events, which correctly handle relative state directory paths by joining them with the workspace root. If a user runs this command from a subdirectory with a relative --manifest-path, this command will likely fail to find the receipt file. Consider resolving the state directory path before joining the file name.
| let receipt_path = from_receipt.unwrap_or_else(|| { | ||
| opts.state_dir | ||
| .join(crate::state::execution_state::RECEIPT_FILE) | ||
| .join(shipper_core::state::execution_state::RECEIPT_FILE) |
There was a problem hiding this comment.
Similar to the PlanYank command, FixForward uses opts.state_dir directly without resolving it against planned.workspace_root. This inconsistency can lead to file-not-found errors when running the tool from outside the workspace root with the default relative state directory.
Comment on lines
+789
to
+793
| use shipper_core::cargo; | ||
| use shipper_core::engine::plan_yank; | ||
| use shipper_core::state::events::{EventLog, events_path}; | ||
| use shipper_core::state::execution_state::{load_receipt, receipt_path, write_receipt}; | ||
| use shipper_core::types::{EventType, PublishEvent}; |
There was a problem hiding this comment.
In the Yank command implementation below these imports, there are two issues related to consistency and robustness:
- State Directory Resolution: The code uses
opts.state_dirdirectly (e.g., forevents_pathandload_receipt) without resolving it againstplanned.workspace_root. This differs from thepublishcommand logic. - Registry Defaulting: In single-yank mode, the code defaults to
"crates-io"ifopts.registriesis empty, ignoring the resolved registry name stored inplanned.plan.registry.name. This means global flags like--registrymight be ignored for this specific command. - Workspace Root: The code uses
std::env::current_dir()as the workspace root forcargo_yank, but it should useplanned.workspace_rootto ensure consistency with the provided manifest path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Apr 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second step of the #95 three-crate split.
shipper-cliwas a 9-line compat shim after PR 1 (#140); this PR makes it the real CLI adapter.Architecture after this PR
The old dependency arrow
shipper-cli -> shipperis inverted:shipper-clino longer depends onshipper. Theshipperpackage depends onshipper-cli(binary) +shipper-core(re-exports).What moved
crates/shipper/src/cli/mod.rs→crates/shipper-cli/src/lib.rscrates/shipper/src/cli/output/→crates/shipper-cli/src/output/shipper_cli::output::...module pathImport rewrites
Inside the moved CLI: all
crate::<core-module>::Xnow point atshipper_core::<module>::X. Incrates/shipper-cli/tests/, the fourbdd_*files that reached directly intoshipper::Xnow reach intoshipper_core::X.Cargo.toml
shipper-cli: dropsshipper.workspace = true; addsshipper-core.workspace = true+clap+clap_complete+indicatif+serde/serde_json/toml/chrono.shipper: runtime deps shrink from 20+ to three:shipper-core,shipper-cli,anyhow. All engine deps and all CLI-only deps drop out (transitive via the downstream crates). Dev-deps shrink to whatcrates/shipper/tests/actually reaches for directly.Binaries
crates/shipper/src/bin/shipper.rs—shipper_cli::run()crates/shipper-cli/src/main.rs—shipper_cli::run()Both install paths (
cargo install shipper,cargo install shipper-cli) produce working CLIs that run the same code.Library surface
crates/shipper/src/lib.rsdropspub mod cli;. The engine re-exports (shipper::engine,shipper::plan, etc.) stay so integration tests incrates/shipper/tests/keep working. PR 3 will tighten this to a curated list.Verification
cargo check --workspace --all-targets— cleancargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo test --workspace --doc— cleancargo test -p shipper-cli --lib— 106 passedcargo test --workspace— passed except pre-existing Windows-onlypreflight_command_*temp-dir flakesTest plan
Part of #95. PR 3 (shrink
shipperREADME + finalize façade) and PR 4 (docs/examples/imports sweep) to follow.