decrating: absorb shipper-engine-parallel into shipper::engine::parallel#64
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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 49 minutes and 5 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 (19)
📒 Files selected for processing (26)
✨ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 969deaace2
ℹ️ 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".
| ) | ||
| })? | ||
| } else { | ||
| reg.fetch_sparse_index_file(reg.base_url(), crate_name)? |
There was a problem hiding this comment.
Use registry index base for sparse-index fetches
The index readiness path fetches sparse metadata from reg.base_url(), but this client base URL is the registry API host, not necessarily the sparse index host. Registry explicitly supports a separate index_base (e.g., crates.io API vs index.crates.io), so with readiness.method = Index (or Both when index is used) this can poll the wrong endpoint, report false negatives, and incorrectly drive successful publishes into ambiguous/failure handling. Please fetch using the registry’s index base instead of the API base.
Useful? React with 👍 / 👎.
…tion Stacks on top of PR #64 (engine_parallel absorption) — engine-parallel no longer depends on shipper-execution-core externally. Move ~2100 LOC (92 LOC of impl + ~2000 LOC of tests/proptests/snapshots) into crates/shipper/src/runtime/execution/mod.rs as the sole submodule of the new runtime/ layer directory. No in-tree duplicate or _micro.rs shim existed — pure relocation. Update all `use shipper_execution_core::X` to `use crate::runtime::execution::X` across shipper (engine/mod.rs, engine/parallel/publish.rs, engine/parallel/mod.rs, engine/parallel/tests.rs). Fuzz target `fuzz/fuzz_targets/execution_core.rs` now imports from `shipper::runtime::execution::*` (fuzz crate already depends on shipper). Remove the `shipper-execution-core` entry from `fuzz/Cargo.toml`. Move the two BDD tests from `crates/shipper-execution-core/tests/execution_core_bdd.rs` into `crates/shipper/tests/execution_bdd.rs`. Rename snapshot files from `shipper_execution_core__tests__snapshots__*` to `shipper__runtime__execution__tests__snapshots__*` and update their `source:` metadata. Enable the `yaml` feature on shipper's insta dev-dep. Items are exposed as `pub` (rather than `pub(crate)`) because the external fuzz target exercises them directly; this exception is documented in `runtime/execution/CLAUDE.md` and will be tightened in a later pass. Delete crates/shipper-execution-core, drop it from the workspace members, remove the dep from shipper's Cargo.toml, and purge it from docs/architecture.md and RELEASE_CHECKLIST_v0.3.0.md. Per docs/decrating-plan.md §6 Phase 2.
Move the canonical 4826-LOC standalone crate shipper-engine-parallel into crates/shipper/src/engine/parallel/ as the new engine-parallel module, split into mod.rs (public entry + Reporter adapter + inner driver + inline test modules), publish.rs (publish_package + run_publish_level + PackagePublishResult), readiness.rs (is_version_visible_with_backoff + sparse-index fallback), policy.rs (policy_effects), webhook.rs (unchanged engine-specific webhook glue), and tests.rs (full test suite + snapshot submodule). Delete the stale 3237-LOC in-tree duplicate at crates/shipper/src/engine_parallel.rs and the 41-LOC shim at engine_parallel_micro.rs. Remove the standalone crate from workspace members and from shipper's deps. Drop the micro-parallel feature flag. Also moves crates/shipper/src/engine.rs to engine/mod.rs to enable the engine/ layer dir (Rust can't have both engine.rs and engine/mod.rs simultaneously). The engine.rs content is unchanged, only the one call site crate::engine_parallel::run_publish_parallel was rewritten to crate::engine::parallel::run_publish_parallel. The six engine.rs snapshots moved alongside it into engine/snapshots/ with their source: headers updated. Webhook submodule decision: kept as engine/parallel/webhook.rs. It is not a duplicate of the public shipper-webhook crate; rather it is engine-specific glue that defines a parallel-publish-specific WebhookEvent enum and payload builder (PublishStarted, PublishSucceeded, PublishFailed, PublishCompleted) on top of the generic transport provided by shipper-webhook. Because the absorbed module now compiles unconditionally, several microcrate deps that were previously optional (pulled in only when micro-parallel was active) become non-optional in shipper's Cargo.toml: shipper-events, shipper-cargo, shipper-chunking, shipper-plan, shipper-registry, shipper-state, shipper-policy. The corresponding micro-X feature flags now toggle only the X_micro.rs vs X.rs path switch, no longer gating the dep itself. External consumer update: fuzz/Cargo.toml dropped the shipper-engine-parallel dep (the engine_parallel_chunks fuzz target already imported shipper_chunking directly, not shipper_engine_parallel, so no fuzz target source file changes were needed). Public API change: top-level shipper::engine_parallel module is gone; callers must use shipper::engine::parallel. Pre-1.0 (v0.3.0-rc.1), no deprecation shim. Updated docs/architecture.md, docs/HANDOFF.md, .github/copilot-instructions.md, crates/shipper/README.md, and RELEASE_CHECKLIST_v0.3.0.md. The outer shipper::engine::parallel::run_publish_parallel takes the host crate's &crate::registry::RegistryClient and &mut dyn crate::engine::Reporter just like the old shim, so engine::run_publish's call site is trivial (new path instead of old path). Internally it constructs a shipper_registry::RegistryClient from the registry api_base so the absorbed module can run regardless of which registry.rs impl variant is compiled (micro wrapper vs legacy). A tiny PlannedWorkspace type conversion bridges the distinct-but-structurally-identical shipper_plan::PlannedWorkspace and crate::plan::PlannedWorkspace. Internal microcrate deps (shipper_execution_core, shipper_cargo, shipper_events, shipper_plan, shipper_chunking, shipper_state, shipper_registry, shipper_types, shipper_policy, shipper_webhook, shipper_sparse_index) intentionally KEPT in the absorbed module's imports. Subsequent absorption PRs for those microcrates will rewrite these to crate::* paths. This PR unblocks the dep cascade (events, state, plan, chunking, execution-core absorptions that were previously blocked because the standalone shipper-engine-parallel depended on them). Validation: cargo check --workspace clean; cargo test -p shipper all 803 lib tests pass including 65 engine::parallel::* tests (unit + property + snapshot); cargo test -p shipper-cli all passes; cargo clippy -p shipper --all-targets --all-features -D warnings clean. Snapshots passed without accept needed - only path rewrites in source: headers, no behavioral diff. Per docs/decrating-plan.md Phase 2 (special case for engine-parallel).
969deaa to
04f7a8d
Compare
Post-rebase of PR #64 on top of main's policy absorption and registry split, the engine::parallel submodule needed API updates that were not included in the merged PR: - shipper_policy -> crate::runtime::policy (policy was absorbed in main) - shipper_registry::RegistryClient -> HttpRegistryClient alias (the parallel path operates on base-URL, not Registry struct; the full Registry-aware client is named RegistryClient and takes a different constructor after the registry lib split) Fixes compilation of shipper crate on main after PR #64 merge.
…tmerge fix(engine/parallel): restore imports after PR #64 absorption drift
After rebasing PR #70 (plan+levels+chunking absorption replay) onto current main (which has PR #64's engine_parallel absorption), three drift corrections are needed: - engine::parallel::{mod,publish,tests}.rs: `shipper_plan::PlannedWorkspace` -> `crate::plan::PlannedWorkspace`; `shipper_chunking::chunk_by_max_concurrent` -> `crate::plan::chunking::chunk_by_max_concurrent`. The parallel engine was absorbed into shipper itself (PR #64), so it references sibling modules directly instead of the former microcrates. - engine::parallel::run_publish_parallel: drop the shipper_plan reconstruction dance (both sides use the same shipper_types::PlannedWorkspace) and forward the host ws directly to run_publish_parallel_inner. - plan::mod.rs: `crate::state::CURRENT_PLAN_VERSION` -> `crate::state::execution_state::CURRENT_PLAN_VERSION` (the const is re-exported from shipper-state via state::execution_state). - plan::chunking::chunk_by_max_concurrent: promote `pub(crate)` to `pub` so the integration BDD test (`tests/engine_parallel_chunking_bdd.rs`) and the engine::parallel re-export can reach it. - engine::parallel::mod.rs: `pub use crate::plan::chunking::chunk_by_max_concurrent` preserves the former `shipper::engine::parallel::chunk_by_max_concurrent` public surface for consumers.
…tion Stacks on top of PR #64 (engine_parallel absorption) — engine-parallel no longer depends on shipper-execution-core externally. Move ~2100 LOC (92 LOC of impl + ~2000 LOC of tests/proptests/snapshots) into crates/shipper/src/runtime/execution/mod.rs as the sole submodule of the new runtime/ layer directory. No in-tree duplicate or _micro.rs shim existed — pure relocation. Update all `use shipper_execution_core::X` to `use crate::runtime::execution::X` across shipper (engine/mod.rs, engine/parallel/publish.rs, engine/parallel/mod.rs, engine/parallel/tests.rs). Fuzz target `fuzz/fuzz_targets/execution_core.rs` now imports from `shipper::runtime::execution::*` (fuzz crate already depends on shipper). Remove the `shipper-execution-core` entry from `fuzz/Cargo.toml`. Move the two BDD tests from `crates/shipper-execution-core/tests/execution_core_bdd.rs` into `crates/shipper/tests/execution_bdd.rs`. Rename snapshot files from `shipper_execution_core__tests__snapshots__*` to `shipper__runtime__execution__tests__snapshots__*` and update their `source:` metadata. Enable the `yaml` feature on shipper's insta dev-dep. Items are exposed as `pub` (rather than `pub(crate)`) because the external fuzz target exercises them directly; this exception is documented in `runtime/execution/CLAUDE.md` and will be tightened in a later pass. Delete crates/shipper-execution-core, drop it from the workspace members, remove the dep from shipper's Cargo.toml, and purge it from docs/architecture.md and RELEASE_CHECKLIST_v0.3.0.md. Per docs/decrating-plan.md §6 Phase 2.
PR #60 landed shim absorption (re-exports from crate::state::events and crate::state::execution_state pointing at standalone crates) because shipper-store (PR #57) and shipper-engine-parallel (PR #64) had trait- signature deps on them. Both blockers landed via merge train round 2, unblocking this physical removal. Move 2821 LOC (events) + 2725 LOC (state) from standalone crates into crates/shipper/src/state/events/ and crates/shipper/src/state/execution_state/ as absorbed modules. Events split into three files: - mod.rs (production: EventLog, EVENTS_FILE, events_path) - tests.rs (unit + insta snapshot tests) - proptests.rs (property-based tests) Execution-state split into two files: - mod.rs (production: atomic write, migration, encrypted I/O) - tests.rs (unit + snapshot tests; nested proptests + proptests_extended submodules preserved) Snapshots relocated and renamed for the new module path (shipper__state__events__tests__* and shipper__state__execution_state__tests__*). Integration test moved to crates/shipper/tests/state_integration.rs (was crates/shipper-state/tests/state_integration.rs) with imports rewritten to use shipper::state::execution_state::*. shipper-events fuzz target (fuzz/fuzz_targets/event_log_roundtrip.rs) repointed to shipper::state::events::EventLog; its shipper-events dep removed from fuzz/Cargo.toml. Internal shipper imports rewritten: - shipper_events → crate::state::events - shipper_state → crate::state::execution_state - shipper_environment::collect_environment_fingerprint → crate::runtime::environment::collect_environment_fingerprint (also already absorbed; execution_state no longer needs the external dep) Delete crates/shipper-events and crates/shipper-state. Remove from workspace members and shipper's deps. CLAUDE.md files updated to reflect the now- physically-absorbed state. docs/architecture.md updated to remove dangling standalone-crate edges. Per docs/decrating-plan.md §6 Phase 2 follow-up + §6.A R-PR-1..R-PR-5. Validation: cargo check --workspace, cargo test -p shipper (1640 lib + integration tests), cargo test -p shipper-cli, cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo fmt --all -- --check — all green.
Summary
Absorbs the canonical 4826-LOC standalone crate
shipper-engine-parallelintocrates/shipper/src/engine/parallel/, deletes the stale 3237-LOC in-tree duplicate (engine_parallel.rs) and 41-LOC shim (engine_parallel_micro.rs), and drops themicro-parallelfeature flag. Also movesengine.rstoengine/mod.rsto enable theengine/layer directory.Why this PR is the structural unblocker: Phase-2 absorption of
shipper-engine-parallelwas blocking six downstream absorptions (events, state, plan, chunking, execution-core, cargo) because those microcrates were dependencies of the standalone engine-parallel crate. Other agent PRs for events+state, plan+levels+chunking were stopped on this dep cascade and can now proceed after merge.File layout of the absorbed module
engine/parallel/mod.rs— public entry (run_publish_parallel+ inner),Reportertrait,HostReporterAdapter, re-export ofchunk_by_max_concurrent, inline#[cfg(test)] mod property_testsandmod tests;.engine/parallel/publish.rs—publish_package,run_publish_level,PackagePublishResult.engine/parallel/readiness.rs—is_version_visible_with_backoff+ sparse-index fallback.engine/parallel/policy.rs—policy_effectsadapter.engine/parallel/webhook.rs— engine-specific webhook glue (kept as-is).engine/parallel/tests.rs— full test suite includingsnapshot_testssubmodule.engine/parallel/snapshots/— 12 insta snapshots.engine/parallel/CLAUDE.md— module guidance.engine/CLAUDE.md— new layer-dir guidance.Webhook submodule decision
Kept as
engine/parallel/webhook.rs(not dropped in favor ofshipper-webhook). It is engine-specific glue that defines a parallel-publish-specificWebhookEventenum (PublishStarted,PublishSucceeded,PublishFailed,PublishCompleted) and a payload builder that converts those typed events into the genericshipper_webhook::WebhookPayload. It's not a duplicate —shipper-webhookis the transport, this file is the engine's typed event layer on top.Public API breaking change
Top-level
shipper::engine_parallelmodule is gone. External callers must useshipper::engine::parallel::*. Pre-1.0 (v0.3.0-rc.1), no deprecation shim. Updated:docs/architecture.mddocs/HANDOFF.md.github/copilot-instructions.mdcrates/shipper/README.mdRELEASE_CHECKLIST_v0.3.0.md(removed thecargo publish -p shipper-engine-parallel --dry-runstep)Fuzz consumer migration
fuzz/Cargo.tomldropped theshipper-engine-paralleldep. The only fuzz target that referenced this area (engine_parallel_chunks) already importedshipper_chunking::chunk_by_max_concurrentdirectly, so no fuzz target source file changes were needed.Cargo.toml changes
crates/shipper-engine-parallelfrom workspace members.crates/shipper/Cargo.toml:shipper-engine-paralleldep.micro-parallelfeature; removed it frommicro-all.shipper-events,shipper-cargo,shipper-chunking,shipper-plan,shipper-registry,shipper-state,shipper-policy. The correspondingmicro-Xfeatures now only toggle theX_micro.rsvsX.rspath switch.Validation
cargo check --workspace— clean.cargo test -p shipper— 803/803 lib tests pass, including 65engine::parallel::*tests (unit + property + snapshot).cargo test -p shipper-cli— all passes (including BDD tests).cargo clippy -p shipper --all-targets --all-features -- -D warnings— clean.shipper_engine_parallel__tests__*→shipper__engine__parallel__tests__*) andsource:headers updated; the 6 engine snapshots (unrelated to parallel) were relocated fromsrc/snapshots/tosrc/engine/snapshots/becauseengine.rsbecameengine/mod.rswhich changes where insta looks for them.cargo checkon Windows due to worktree cross-reference; the change is mechanical (removed the dep line) and the only fuzz target using this area already usesshipper_chunkingdirectly.Transitional notes
Internal microcrate deps inside the absorbed module (
shipper_execution_core,shipper_cargo,shipper_events,shipper_plan,shipper_chunking,shipper_state,shipper_registry,shipper_types,shipper_policy,shipper_webhook,shipper_sparse_index) are intentionally left as-is. Subsequent absorption PRs for those microcrates will rewrite these tocrate::*paths.This PR is large (4826 LOC moved + 3237 LOC deleted + layer dir created) but atomically necessary. Per
docs/decrating-plan.md§6 Phase 2 (special case for engine-parallel).Test plan
cargo check --workspacecargo test -p shipper engine::parallel— 65 tests passcargo test -p shipper— 803 lib + integration tests passcargo test -p shipper-cli— BDD + integration tests passcargo clippy -p shipper --all-targets --all-features -- -D warnings