decrating: fold in-tree registry logic into shipper-registry crate#61
Merged
Merged
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 14 minutes and 23 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 (30)
📒 Files selected for processing (13)
WalkthroughThis pull request consolidates registry functionality by extracting the registry client implementation into a standalone HTTP-backed module within Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c3457334a
ℹ️ 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".
| reporter: &mut dyn crate::engine::Reporter, | ||
| ) -> anyhow::Result<Vec<PackageReceipt>> { | ||
| let reg = reg.inner(); | ||
| let http_reg = HttpRegistryClient::new(reg.registry().api_base.as_str()); |
There was a problem hiding this comment.
Preserve index cache when constructing parallel HTTP client
This rebuilds a fresh HttpRegistryClient from only api_base, which drops the cache configuration already attached to reg (set by init_registry_client in engine.rs). In parallel runs that use index-based readiness (ReadinessMethod::Index/Both), shipper-engine-parallel polls sparse-index files repeatedly, so losing cache_dir disables ETag/cache reuse and can materially increase request volume, latency, and registry rate-limit failures on larger publish waves.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-registry/src/http.rs`:
- Around line 3-7: Update the module docs to point at the actual registry-aware
client: replace references/links to `crate::HttpRegistryClient` with the correct
symbol `crate::context::RegistryClient` (or use the short link
`crate::context::RegistryClient` in the doc comments) where `HttpRegistryClient`
is mentioned (e.g., the paragraph referencing "For the complete registry
surface, use the [`crate::HttpRegistryClient`] from the [`crate::context`]
module" and the similar occurrence around lines 18-19); ensure the inline doc
links use the exact symbol `RegistryClient` within the `crate::context` module
so rustdoc resolves to the real API.
In `@crates/shipper-registry/src/lib.rs`:
- Around line 32-39: The public Owner and OwnersResponse types are duplicated
between context and http, causing API incompatibility; consolidate them into a
single shared module (e.g., a new models or types module) and export that
module's Owner and OwnersResponse from the crate root, then update both
RegistryClient (context::RegistryClient) and HttpRegistryClient methods
(HttpRegistryClient::get_owners, HttpRegistryClient::list_owners) to return the
shared types instead of http::Owner/http::OwnersResponse, and remove the
duplicate definitions in context and http so both clients use the same DTOs.
In `@crates/shipper/src/engine_parallel_micro.rs`:
- Around line 38-40: The current adaptation creates an HttpRegistryClient from
only reg.registry().api_base, dropping Registry::index_base so index-based
readiness (ReadinessMethod::Index/Both) fetches from the wrong host; update the
construction and call site to thread the original registry index_base through to
micro::run_publish_parallel (or pass the full Registry) so readiness checks use
Registry::index_base. Locate the HttpRegistryClient::new call and the
micro::run_publish_parallel call in engine_parallel_micro.rs and either
construct the client with both api_base and index_base or pass the Registry (not
just api_base) into run_publish_parallel so ReporterAdapter/HttpRegistryClient
retain index_base for readiness paths.
In `@templates/circleci-config.yml`:
- Line 47: The CircleCI BDD matrix loop in templates/circleci-config.yml omits
the valid feature sets "micro-plan" and "micro-policy"; update the for-loop that
iterates over feature_set to include "micro-plan" and "micro-policy" so it
matches the GitHub Actions matrix (see the loop variable feature_set in that
file) and restores per-feature coverage across CI.
🪄 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: 15d54275-5290-473f-a92f-20f401386120
⛔ Files ignored due to path filters (30)
Cargo.lockis excluded by!**/*.lockcrates/shipper-registry/src/snapshots/shipper_registry__context__tests__empty_owner_list_detail.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__context__tests__owners_empty_users.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__context__tests__owners_multiple_with_mixed_names.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__context__tests__owners_response_parsed.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__context__tests__owners_with_teams.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__context__tests__readiness_evidence_multi_attempt.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__context__tests__readiness_evidence_single_attempt.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__context__tests__registry_debug_repr.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__context__tests__registry_debug_repr_no_index.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__crate_info.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__error_connection_refused.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__error_owners_forbidden.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__error_owners_not_found.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__error_unexpected_status.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__owner_all_fields.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__owner_minimal.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__owners_api_user_with_id.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__owners_api_user_without_id.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__owners_response_empty.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__owners_response_multiple.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__sparse_path_1char.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__sparse_path_2char.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__sparse_path_3char.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__sparse_path_4char.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__sparse_path_long.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__url_crate.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__url_custom_registry.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__url_owners.snapis excluded by!**/*.snapcrates/shipper-registry/src/snapshots/shipper_registry__http__tests__url_version.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
.github/workflows/ci.ymlcrates/shipper-cli/Cargo.tomlcrates/shipper-engine-parallel/src/lib.rscrates/shipper-registry/Cargo.tomlcrates/shipper-registry/src/context.rscrates/shipper-registry/src/http.rscrates/shipper-registry/src/lib.rscrates/shipper/Cargo.tomlcrates/shipper/src/engine_parallel_micro.rscrates/shipper/src/lib.rscrates/shipper/src/registry_micro.rsdocs/architecture.mdtemplates/circleci-config.yml
💤 Files with no reviewable changes (2)
- crates/shipper-cli/Cargo.toml
- crates/shipper/src/registry_micro.rs
Comment on lines
+3
to
+7
| //! This module provides [`HttpRegistryClient`] — a thin reqwest wrapper that | ||
| //! takes a bare base-URL string. Intended for callers that do not have a full | ||
| //! [`shipper_types::Registry`] handy (e.g. the parallel engine helper crate). | ||
| //! For the complete registry surface, use the [`crate::HttpRegistryClient`] from | ||
| //! the [`crate::context`] module. |
There was a problem hiding this comment.
Fix the doc links to point at RegistryClient.
These docs currently say the “complete” registry-aware client is crate::HttpRegistryClient in crate::context, but context exposes RegistryClient. As written, the docs send readers to the wrong API.
Suggested doc fix
-//! For the complete registry surface, use the [`crate::HttpRegistryClient`] from
+//! For the complete registry surface, use the [`crate::RegistryClient`] from
//! the [`crate::context`] module.
@@
-/// Use [`crate::HttpRegistryClient`] for the full `Registry`-aware client with
+/// Use [`crate::RegistryClient`] for the full `Registry`-aware client with
/// sparse-index visibility checks, readiness backoff, etc.Also applies to: 18-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/shipper-registry/src/http.rs` around lines 3 - 7, Update the module
docs to point at the actual registry-aware client: replace references/links to
`crate::HttpRegistryClient` with the correct symbol
`crate::context::RegistryClient` (or use the short link
`crate::context::RegistryClient` in the doc comments) where `HttpRegistryClient`
is mentioned (e.g., the paragraph referencing "For the complete registry
surface, use the [`crate::HttpRegistryClient`] from the [`crate::context`]
module" and the similar occurrence around lines 18-19); ensure the inline doc
links use the exact symbol `RegistryClient` within the `crate::context` module
so rustdoc resolves to the real API.
Comment on lines
+38
to
+40
| let http_reg = HttpRegistryClient::new(reg.registry().api_base.as_str()); | ||
| let mut adapter = ReporterAdapter { inner: reporter }; | ||
| micro::run_publish_parallel(ws, opts, st, state_dir, reg, &mut adapter) | ||
| micro::run_publish_parallel(ws, opts, st, state_dir, &http_reg, &mut adapter) |
There was a problem hiding this comment.
Preserve index_base when adapting the registry client.
This drops the original Registry context down to api_base only, but shipper-engine-parallel still does sparse-index readiness checks. In the current flow, ReadinessMethod::Index/Both will end up fetching sparse entries from api_base instead of Registry::index_base (for crates.io: https://crates.io/... instead of https://index.crates.io/...), so index-based readiness can fail after a successful publish. Please thread the original index base through to the micro client, or keep the registry-aware client for readiness paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/shipper/src/engine_parallel_micro.rs` around lines 38 - 40, The
current adaptation creates an HttpRegistryClient from only
reg.registry().api_base, dropping Registry::index_base so index-based readiness
(ReadinessMethod::Index/Both) fetches from the wrong host; update the
construction and call site to thread the original registry index_base through to
micro::run_publish_parallel (or pass the full Registry) so readiness checks use
Registry::index_base. Locate the HttpRegistryClient::new call and the
micro::run_publish_parallel call in engine_parallel_micro.rs and either
construct the client with both api_base and index_base or pass the Registry (not
just api_base) into run_publish_parallel so ReporterAdapter/HttpRegistryClient
retain index_base for readiness paths.
| name: Run BDD compatibility matrix | ||
| command: | | ||
| for feature_set in "" "micro-auth" "micro-git" "micro-events" "micro-lock" "micro-encrypt" "micro-environment" "micro-storage" "micro-cargo" "micro-registry" "micro-process" "micro-webhook" "micro-types" "micro-config" "micro-state" "micro-store" "micro-all"; do | ||
| for feature_set in "" "micro-auth" "micro-git" "micro-events" "micro-lock" "micro-encrypt" "micro-environment" "micro-storage" "micro-cargo" "micro-process" "micro-webhook" "micro-types" "micro-config" "micro-state" "micro-store" "micro-all"; do |
There was a problem hiding this comment.
CircleCI BDD matrix omits valid feature sets (micro-plan, micro-policy).
This line now diverges from the GitHub matrix at Line [213] in .github/workflows/ci.yml, reducing per-feature coverage in CircleCI.
Suggested fix
- for feature_set in "" "micro-auth" "micro-git" "micro-events" "micro-lock" "micro-encrypt" "micro-environment" "micro-storage" "micro-cargo" "micro-process" "micro-webhook" "micro-types" "micro-config" "micro-state" "micro-store" "micro-all"; do
+ for feature_set in "" "micro-auth" "micro-git" "micro-events" "micro-lock" "micro-encrypt" "micro-environment" "micro-storage" "micro-cargo" "micro-plan" "micro-policy" "micro-process" "micro-webhook" "micro-types" "micro-config" "micro-state" "micro-store" "micro-all"; do📝 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.
Suggested change
| for feature_set in "" "micro-auth" "micro-git" "micro-events" "micro-lock" "micro-encrypt" "micro-environment" "micro-storage" "micro-cargo" "micro-process" "micro-webhook" "micro-types" "micro-config" "micro-state" "micro-store" "micro-all"; do | |
| for feature_set in "" "micro-auth" "micro-git" "micro-events" "micro-lock" "micro-encrypt" "micro-environment" "micro-storage" "micro-cargo" "micro-plan" "micro-policy" "micro-process" "micro-webhook" "micro-types" "micro-config" "micro-state" "micro-store" "micro-all"; do |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/circleci-config.yml` at line 47, The CircleCI BDD matrix loop in
templates/circleci-config.yml omits the valid feature sets "micro-plan" and
"micro-policy"; update the for-loop that iterates over feature_set to include
"micro-plan" and "micro-policy" so it matches the GitHub Actions matrix (see the
loop variable feature_set in that file) and restores per-feature coverage across
CI.
Move the canonical 4791-LOC implementation from crates/shipper/src/registry.rs INTO crates/shipper-registry/src/, split into: - lib.rs (facade, top-level fns, constants) - context.rs (canonical Registry-aware RegistryClient + tests + snapshots) - http.rs (lightweight HttpRegistryClient with bare-url API, kept for shipper-engine-parallel; previously the entire public surface) shipper-registry becomes the complete public registry client. shipper depends on it via `pub use shipper_registry as registry;` re-export at lib.rs, preserving the `shipper::registry::*` surface. Delete the in-tree duplicate (registry.rs, 4791 LOC) and the shim (registry_micro.rs, 239 LOC). Drop the `micro-registry` feature flag. Make shipper-registry a non-optional dep of shipper. Move insta snapshots: - crates/shipper/src/snapshots/shipper__registry__tests__*.snap -> crates/shipper-registry/src/snapshots/shipper_registry__context__tests__*.snap - Rename pre-existing snapshots from `shipper_registry__tests__*` to `shipper_registry__http__tests__*` to match new module path. shipper-engine-parallel continues to use the bare-url API via `use shipper_registry::HttpRegistryClient as RegistryClient;` (single-line import change). Update .github/workflows/ci.yml, templates/circleci-config.yml and docs/architecture.md to remove `micro-registry` references. Per docs/decrating-plan.md Phase 4 (registry special case).
8c34573 to
9b4e9c8
Compare
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
Phase 4 of the decrating plan (registry special case). Unlike the other
Phase 2 absorptions,
shipper-registrystays public — the work is theopposite direction: move the canonical in-tree implementation INTO
the public crate so
shipper-registrybecomes a complete, publishableregistry client.
crates/shipper/src/registry.rs(4791 LOC, canonical) intocrates/shipper-registry/src/context.rs(Registry-awareRegistryClient,the full surface with backoff/index/owners/readiness-evidence).
crates/shipper-registry/src/http.rs(renamed struct:HttpRegistryClient) so theshipper-engine-parallelcrate continuesto compile with a single-line
usechange.crates/shipper-registry/src/lib.rsfacade with top-levelconstants (
CRATES_IO_API,DEFAULT_TIMEOUT_SECS,USER_AGENT) andconvenience fns (
is_version_visible,is_crate_visible,sparse_index_path).registry.rs) and the 239-LOC shim(
registry_micro.rs).shipper-registrya non-optional dep ofshipper; drop themicro-registryfeature (and remove it frommicro-parallel/micro-all).shipper::registry::*public surface preserved viapub use shipper_registry as registry;atshipper/src/lib.rs— nobreaking change for external library users.
Sub-file split
Snapshot migration
crates/shipper/src/snapshots/shipper__registry__tests__*.snap→
crates/shipper-registry/src/snapshots/shipper_registry__context__tests__*.snapshipper_registry__tests__*.snaprenamed toshipper_registry__http__tests__*.snapto match new module path.Dependencies added to shipper-registry
chrono(readiness evidence timestamps)rand(jitter for backoff)shipper-types(Registry, ReadinessConfig, ReadinessEvidence,ReadinessMethod)
CI/docs cleanup
micro-registryfrom.github/workflows/ci.ymlmatrix.micro-registryfromtemplates/circleci-config.yml.docs/architecture.md(removed(optional)tag and themicro-registryfeature entry).RELEASE_CHECKLIST_v0.3.0.mdstill listsshipper-registryas apublish target — retained, still correct.
Validation
cargo check --workspace: clean.cargo test -p shipper-registry --lib: 258 tests; all pass inisolation. One timing-sensitive test
(
server_errors_500_502_503_all_classified_as_not_visible_in_backoff)is environment-flaky under parallel load but passes standalone — this
is a pre-existing issue inherited from the in-tree.
cargo test -p shipper --lib: 584 passed / 0 failed.cargo test -p shipper-cli --bins: 16 passed / 0 failed.cargo test --test cli_e2e -p shipper-cli: 22 passed / 0 failed.cargo test -p shipper-engine-parallel: 65 passed / 0 failed.cargo clippy --workspace --all-targets --all-features -- -D warnings:clean.
cargo fmt --all -- --check: clean.LOC delta
shipper: -5030 LOC (registry.rs -4791, registry_micro.rs -239)shipper-registry: +4831 LOC (context.rs +4791, http.rs split/rename~-31 net after removing duplicated constants/convenience fns, lib.rs
facade +71)
Test plan
cargo check --workspacecargo test -p shipper-registrycargo test -p shipper(lib)cargo test -p shipper-cli(bins + cli_e2e)cargo test -p shipper-engine-parallelcargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcargo build -p shipper-cli