feat(profile): thread PublishRegime through preflight to publish (#106 PR 1)#154
Conversation
…PR 1) Threads a new `PublishRegime` enum (`FirstPublish` | `Update`) from preflight onto each `PlannedPackage`, so the publish retry loop no longer re-queries `crate_exists` on rate-limit retries to recover the "is this a brand-new crate?" answer. Preflight already knows; this PR just stops throwing the answer away. Per the scout's plan on issue #106 (PR 1 of 5): - Adds `PublishRegime` to `shipper-types` with `is_new_crate()` helper. - Adds `PlannedPackage.regime: Option<PublishRegime>`, gated by `#[serde(default, skip_serializing_if = "Option::is_none")]` so old `state.json` and plan files deserialize unchanged and snapshots written without the field stay byte-identical. - `engine::run_preflight` now takes `&mut PlannedWorkspace` and stamps the regime onto each package after the existing registry probes. - `publish_package` (sequential and parallel paths) seeds `is_new_crate_cached` from `p.regime` when present, falling back to the legacy lazy-cached behavior when the field is `None`. Backward compatibility: - `regime` is `Option<_>` with `serde(default)` and skipped on serialization when `None`. Plans / state files written by older shippers deserialize cleanly with `regime: None`, and serialization output is unchanged for callers that never set it. - The `None` branch preserves the legacy `is_new_crate_cached` re-query path verbatim, so any embedder constructing `PlannedPackage` directly retains today's behavior. Acceptance criteria (from #106 PR 1): - [x] `PublishRegime` defined with `FirstPublish` and `Update` variants - [x] `PlannedPackage.regime` plumbed through plan -> preflight -> publish - [x] Preflight stamps regime based on registry probes (FirstPublish vs Update) - [x] Publish retry loop consumes regime instead of re-querying when present - [x] Backward-compat: missing `regime` deserializes to `None` and is omitted on serialize - [x] No snapshot regressions in plan-id computation or release-level tests
|
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 56 minutes and 48 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 selected for processing (9)
✨ 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 introduces a PublishRegime classification to distinguish between new crate publishes and version updates. This classification is determined during preflight and stored in the PlannedPackage to optimize the publish retry loop by avoiding redundant registry queries. Feedback includes a potential omission in the CLI's publish command that could bypass this optimization, a suggestion to skip redundant network calls in run_preflight when a version is already known to exist, and the removal of an unnecessary mut keyword in a documentation example to prevent compiler warnings.
| let rep = engine::run_preflight(&mut planned, &opts, &mut reporter)?; | ||
| print_preflight(&rep, &cli.format); | ||
| } | ||
| Commands::Publish => { |
There was a problem hiding this comment.
The Commands::Publish block does not appear to be updated to call run_preflight with a mutable reference (or it may not be calling it at all). Since run_preflight is now the only place where the regime field is stamped onto the plan, and run_publish relies on this field to avoid redundant registry queries during retries, the shipper publish command will miss this optimization and fall back to the legacy re-query behavior. This significantly reduces the impact of this PR for the primary user workflow.
| let already_published = reg.version_exists(&p.name, &p.version)?; | ||
| let is_new_crate = reg.check_new_crate(&p.name)?; |
There was a problem hiding this comment.
If already_published is true, it implies the crate already exists on the registry, so is_new_crate must be false. We can skip the redundant check_new_crate network call in this case to improve efficiency.
| let already_published = reg.version_exists(&p.name, &p.version)?; | |
| let is_new_crate = reg.check_new_crate(&p.name)?; | |
| let already_published = reg.version_exists(&p.name, &p.version)?; | |
| let is_new_crate = if already_published { | |
| false | |
| } else { | |
| reg.check_new_crate(&p.name)? | |
| }; |
| /// | ||
| /// ```ignore | ||
| /// let ws = plan::build_plan(&spec)?; | ||
| /// let mut ws = plan::build_plan(&spec)?; |
There was a problem hiding this comment.
The mut keyword is unnecessary here as ws is passed by immutable reference to run_resume on line 1354. This will trigger a compiler warning.
| /// let mut ws = plan::build_plan(&spec)?; | |
| /// let ws = plan::build_plan(&spec)?; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d00b1888f1
ℹ️ 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".
| } | ||
| Commands::Preflight => { | ||
| let rep = engine::run_preflight(&planned, &opts, &mut reporter)?; | ||
| let rep = engine::run_preflight(&mut planned, &opts, &mut reporter)?; |
There was a problem hiding this comment.
Persist preflight-stamped regimes before exiting
run_preflight now mutates planned via &mut in the Preflight command, but that mutated plan is never written anywhere before the process exits. Because shipper publish is a separate CLI invocation that rebuilds the plan from plan::build_plan (with regime: None), the publish path cannot consume the stamped PublishRegime and still falls back to live registry re-queries on rate-limit retries. This makes the new preflight→publish threading ineffective for normal CLI workflows.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
PR 1 of the 5-PR sequence outlined by the scout on issue #106. Threads a new
PublishRegimeenum (FirstPublish|Update) from preflight onto eachPlannedPackageso the publish retry loop can answer "is this a brand-new crate?" by reading the plan instead of re-querying the registry on every rate-limit retry.This eliminates the
is_new_crate_cachedre-query waste called out by the scout's plan and unblocks PRs 2-5 (RegistryProfile, Retry-After parsing, duration estimation, per-crate tuning).Changes
shipper-types: newPublishRegimeenum (FirstPublish|Update) withis_new_crate()helper and round-trip serde tests.PlannedPackage: gainsregime: Option<PublishRegime>.engine::run_preflight: now takes&mut PlannedWorkspaceand stamps the detected regime onto each package after the existing registry probes; new tests cover both FirstPublish and Update stamping.engine::publish_package(sequential and parallel): seedsis_new_crate_cachedfromp.regimewhen present, falling back to the legacy lazy-cached re-query whenNone.&mut plannedtorun_preflight.PlannedPackage(production + tests + property/stress tests) updated withregime: Noneto remain compilable.Backward compatibility
The new field is gated by
#[serde(default, skip_serializing_if = \"Option::is_none\")]:state.json/ plan files written before this PR deserialize cleanly withregime: None.regimeisNone, the field is omitted entirely (no\"regime\": null), so snapshots stay byte-for-byte identical for callers that never set it.Nonebranch in publish_package preserves the legacyis_new_crate_cachedre-query path verbatim, so any embedder constructingPlannedPackagedirectly retains today's behavior.Three serde tests in
shipper-typeslock this in:planned_package_backward_compat_no_regime_field— legacy JSON deserializes cleanly withregime: None.planned_package_regime_none_is_skipped_in_serialization—Noneis omitted on serialize.planned_package_regime_some_round_trips— both variants round-trip via snake_case wire format.Acceptance criteria (from #106 PR 1)
PublishRegimedefined withFirstPublishandUpdatevariantsPlannedPackage.regimeplumbed through plan -> preflight -> publishregimedeserializes toNoneand is omitted on serializeOut of scope (future PRs in this sequence)
RegistryProfileabstraction encoding crates.io rate-limit constants.shipper.toml(defer if scope creep)PublishRegime::Patchgranularity (open question from the scout's plan)Test plan
cargo fmt --all -- --checkcleancargo clippy --workspace --all-targets --all-features -- -D warningscleancargo test --workspace— all suites pass except one pre-existing Windows-parallelism flake inshipper-cli::cli_e2e::preflight_command_snapshotthat also fails onmain(cargofailed to rename .demo-0.1.0.crate to demo-0.1.0.craterace in shared target dir); the test passes in isolation with this diff applied.planned_package_backward_compat_no_regime_fieldplanned_package_regime_none_is_skipped_in_serializationplanned_package_regime_some_round_tripspublish_regime_is_new_crate_matches_variantrun_preflight_stamps_update_regime_on_existing_craterun_preflight_*first-publish test extended to assertregime == FirstPublishRefs #106