feat(ergonomics): MVP — rich --version, error hints, in-help examples (#108)#156
Conversation
|
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 18 minutes and 29 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 (5)
WalkthroughAdd build-time version metadata generation and a global Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 enhances the CLI by embedding build-time metadata (Git SHA, build profile, and rustc version) into a new long-form version output and significantly expanding the help documentation for the plan, preflight, and publish subcommands. It also introduces context-aware failure hints to guide users during errors. Review feedback highlights a potential issue with build script triggers in non-git environments, a configuration error in clap that causes --version to display the long version instead of remaining terse, and the duplication of test redaction logic across multiple files.
| println!("cargo:rerun-if-changed=../../.git/HEAD"); | ||
| println!("cargo:rerun-if-changed=../../.git/refs/heads"); |
There was a problem hiding this comment.
Emitting cargo:rerun-if-changed for non-existent files or directories causes Cargo to rerun the build script on every build. In environments where the .git directory is missing (e.g., when building from a source tarball or a crates.io package), these lines will trigger unnecessary rebuilds. It is recommended to check for the existence of the .git directory before printing these instructions.
| println!("cargo:rerun-if-changed=../../.git/HEAD"); | |
| println!("cargo:rerun-if-changed=../../.git/refs/heads"); | |
| let git_dir = std::path::Path::new("../../.git"); | |
| if git_dir.exists() { | |
| println!("cargo:rerun-if-changed=../../.git/HEAD"); | |
| println!("cargo:rerun-if-changed=../../.git/refs/heads"); | |
| } |
| /// `-V --verbose`). The plain `--version` stays terse ("shipper X.Y.Z") | ||
| /// so scripts parsing it don't need to adapt. | ||
| /// | ||
| /// Format: | ||
| /// ```text | ||
| /// shipper 0.3.0 | ||
| /// commit: abc1234 | ||
| /// build: release | ||
| /// rustc: rustc 1.92.0 (... ) | ||
| /// ``` | ||
| const LONG_VERSION: &str = concat!( | ||
| env!("CARGO_PKG_VERSION"), | ||
| "\ncommit: ", | ||
| env!("SHIPPER_GIT_SHA"), | ||
| "\nbuild: ", | ||
| env!("SHIPPER_BUILD_PROFILE"), | ||
| "\nrustc: ", | ||
| env!("SHIPPER_RUSTC_VERSION"), | ||
| ); | ||
|
|
||
| #[derive(Parser, Debug)] | ||
| #[command(name = "shipper", version)] | ||
| #[command(name = "shipper", version, long_version = LONG_VERSION)] |
There was a problem hiding this comment.
There is a contradiction between the documentation and the implementation. The comments and PR description state that --version stays terse for script compatibility, but by assigning LONG_VERSION to long_version in the clap command definition, --version will now output the rich multi-line version. In clap, -V shows the short version while --version shows the long version. If the goal is to keep --version terse, LONG_VERSION should not be assigned to long_version.
| fn redact_version_metadata(s: &str) -> String { | ||
| let trailing_nl = s.ends_with('\n'); | ||
| let joined = s | ||
| .lines() | ||
| .map(|line| { | ||
| if line.starts_with("commit: ") { | ||
| "commit: [GIT_SHA]".to_string() | ||
| } else if line.starts_with("build: ") { | ||
| "build: [PROFILE]".to_string() | ||
| } else if line.starts_with("rustc: ") { | ||
| "rustc: [RUSTC_VERSION]".to_string() | ||
| } else { | ||
| line.to_string() | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join("\n"); | ||
| if trailing_nl { joined + "\n" } else { joined } | ||
| } |
There was a problem hiding this comment.
The redact_version_metadata function is duplicated here and in crates/shipper-cli/tests/e2e_expanded.rs. To improve maintainability and follow DRY principles, consider moving this shared logic to a common test utility module (e.g., tests/common/mod.rs).
References
- Code duplication should be avoided to improve maintainability and reduce the risk of inconsistent logic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cb5cbd569
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| #[derive(Parser, Debug)] | ||
| #[command(name = "shipper", version)] | ||
| #[command(name = "shipper", version, long_version = LONG_VERSION)] |
There was a problem hiding this comment.
Keep plain --version output single-line
Setting long_version = LONG_VERSION makes shipper --version print the multi-line metadata block unconditionally, so callers that consume the full output as a single version string now break. This also contradicts the nearby contract comment that plain --version should remain terse and only the verbose form should be rich, so the current wiring does not match the intended CLI compatibility behavior.
Useful? React with 👍 / 👎.
| println!("cargo:rerun-if-changed=../../.git/HEAD"); | ||
| println!("cargo:rerun-if-changed=../../.git/refs/heads"); |
There was a problem hiding this comment.
Resolve git metadata paths dynamically in build script
The rerun triggers hardcode ../../.git/..., which only works when .git is a directory exactly two levels up; in git worktrees/submodules (where .git is a file) or different checkout layouts, these paths do not track ref updates. In those environments Cargo won’t rerun the build script when HEAD moves, so SHIPPER_GIT_SHA can stay stale across commits and the reported binary provenance becomes inaccurate.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
56f328a to
19f585e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/shipper-cli/src/lib.rs`:
- Around line 708-709: The failure hints currently use opts.state_dir which may
be a relative path, causing hints to point at the wrong locations; update the
code that builds the hint (invoked around engine::run_preflight_in_place and
preflight_failure_hint) to resolve opts.state_dir against planned.workspace_root
(i.e., if state_dir is relative, join it with planned.workspace_root and
canonicalize/normalize) before passing it into preflight_failure_hint so
generated paths point at the real on-disk state (apply same change for other
call sites that pass opts.state_dir into failure-hint helpers).
🪄 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
Run ID: c38cafdd-6840-4256-a433-2e2f3ff56eb9
⛔ Files ignored due to path filters (19)
crates/shipper-cli/tests/snapshots/cli_snapshots__help_text.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/cli_snapshots__no_subcommand_error.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/cli_snapshots__plan_help.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/cli_snapshots__preflight_help.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/cli_snapshots__publish_help.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/cli_snapshots__version_flag.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/cli_snapshots__version_flag_verbose.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__help_plan.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__help_preflight.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__help_publish.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__help_root.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__preflight_json_format_non_git.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__preflight_non_git_directory.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__resume_corrupted_state_file.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__resume_empty_state_file.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__resume_minimal_json_state.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__resume_no_state_file.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__resume_wrong_plan_id.snapis excluded by!**/*.snapcrates/shipper-cli/tests/snapshots/e2e_expanded__version_output.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
crates/shipper-cli/Cargo.tomlcrates/shipper-cli/build.rscrates/shipper-cli/src/lib.rscrates/shipper-cli/tests/cli_snapshots.rscrates/shipper-cli/tests/e2e_expanded.rs
| let rep = engine::run_preflight_in_place(&mut planned, &opts, &mut reporter) | ||
| .with_context(|| preflight_failure_hint(&opts.state_dir))?; |
There was a problem hiding this comment.
Resolve relative state-dir paths before rendering failure hints.
opts.state_dir can be relative, but the actual state files live under planned.workspace_root. With --manifest-path /tmp/ws/Cargo.toml, these hints can incorrectly point operators at ./.shipper/events.jsonl instead of /tmp/ws/.shipper/events.jsonl, undermining recovery after a failed publish/resume.
🛠️ Proposed fix
+fn resolve_state_dir(workspace_root: &Path, state_dir: &Path) -> PathBuf {
+ if state_dir.is_absolute() {
+ state_dir.to_path_buf()
+ } else {
+ workspace_root.join(state_dir)
+ }
+}
+
fn preflight_failure_hint(state_dir: &Path) -> String {
format!(
"preflight failed — next steps:\n \ Commands::Preflight => {
+ let state_dir = resolve_state_dir(&planned.workspace_root, &opts.state_dir);
let rep = engine::run_preflight_in_place(&mut planned, &opts, &mut reporter)
- .with_context(|| preflight_failure_hint(&opts.state_dir))?;
+ .with_context(|| preflight_failure_hint(&state_dir))?;
print_preflight(&rep, &cli.format);
}- let receipt = engine::run_publish(¤t_planned, ¤t_opts, &mut reporter)
- .with_context(|| publish_failure_hint(¤t_opts.state_dir))?;
+ let state_dir =
+ resolve_state_dir(¤t_planned.workspace_root, ¤t_opts.state_dir);
+ let receipt = engine::run_publish(¤t_planned, ¤t_opts, &mut reporter)
+ .with_context(|| publish_failure_hint(&state_dir))?;- let receipt = engine::run_resume(¤t_planned, ¤t_opts, &mut reporter)
- .with_context(|| resume_failure_hint(¤t_opts.state_dir))?;
+ let state_dir =
+ resolve_state_dir(¤t_planned.workspace_root, ¤t_opts.state_dir);
+ let receipt = engine::run_resume(¤t_planned, ¤t_opts, &mut reporter)
+ .with_context(|| resume_failure_hint(&state_dir))?;Also applies to: 745-746, 790-791, 1238-1281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/shipper-cli/src/lib.rs` around lines 708 - 709, The failure hints
currently use opts.state_dir which may be a relative path, causing hints to
point at the wrong locations; update the code that builds the hint (invoked
around engine::run_preflight_in_place and preflight_failure_hint) to resolve
opts.state_dir against planned.workspace_root (i.e., if state_dir is relative,
join it with planned.workspace_root and canonicalize/normalize) before passing
it into preflight_failure_hint so generated paths point at the real on-disk
state (apply same change for other call sites that pass opts.state_dir into
failure-hint helpers).
… MVP) Lands the Ergonomics competency MVP from the scout's plan: (1) a stdlib-only build.rs emits SHIPPER_GIT_SHA, SHIPPER_BUILD_PROFILE, and SHIPPER_RUSTC_VERSION so shipper --version --verbose prints a rich supply-chain-auditable identity; (2) preflight/publish/resume failure paths now attach actionable hints that point users at the next recovery step; (3) plan/preflight/publish subcommands gained clap long_about blocks with concrete copy-paste examples, surfaced directly in --help. Snapshots updated to match the new help and version output; no behavior changes in shipper-core.
99b091f to
3099b21
Compare
Summary
shipper --version --verboseprints git SHA + build profile + rustc version so operators can audit exactly what binary they're running.plan,preflight, andpublishsubcommands gained in-help examples (claplong_about) soshipper <cmd> --helpis self-sufficient.Lands the Ergonomics competency MVP from issue #108. Scoped tight: no man pages, no demo workspace, no
cargo-shipperalias — those can follow in separate PRs.What changed
crates/shipper-cli/build.rs(new): stdlib-only build script that emitsSHIPPER_GIT_SHA/SHIPPER_BUILD_PROFILE/SHIPPER_RUSTC_VERSIONviacargo::rustc-env. Re-runs on.git/HEADand.git/refs/headschanges. Novergendep — deliberately keeping the supply chain small.crates/shipper-cli/Cargo.toml: wires up the build script (build = "build.rs").crates/shipper-cli/src/lib.rs: long--versionformatter;long_aboutblocks with worked examples on the three most-used subcommands; hint-attaching error paths for preflight / publish / resume.crates/shipper-cli/tests/{cli_snapshots,e2e_expanded}.rsand snapshot files: updated to cover the new help output, the rich version string, and the new error hints.No behavior changes in
shipper-core; this is a pure CLI-adapter layer change, exactly per the three-crate architecture from #95.Test plan
cargo fmt --all -- --checkcleancargo clippy --workspace --all-targets --all-features -- -D warningscleancargo test -p shipper-cli— one pre-existing Windows cargo file-lock flake (preflight_command_finishability_proven); passes on rerun in isolation, unrelated to this PRcargo test --workspace— same pre-existing Windows packaging flake (preflight_command_snapshot, "failed to rename .crate — os error 2"); passes on rerun in isolation. Documented in CLAUDE.md guidance.cli_snapshots__{plan,preflight,publish}_help,cli_snapshots__version_flag,e2e_expanded__help_{plan,preflight,publish},e2e_expanded__preflight_{json_format_non_git,non_git_directory},e2e_expanded__resume_{corrupted,empty,minimal_json,no,wrong_plan_id}_state_file,e2e_expanded__version_output