decrating: absorb shipper-auth into shipper::ops::auth (full absorption)#63
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 6 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 (43)
📒 Files selected for processing (22)
✨ 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: d9321ef820
ℹ️ 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".
Comment on lines
101
to
103
| let env_var = format!( | ||
| "{}{}_TOKEN", | ||
| CARGO_REGISTRIES_TOKEN_PREFIX, |
There was a problem hiding this comment.
Normalize all non-alnum chars in registry env var names
This resolver only rewrites - to _ when building CARGO_REGISTRIES_<NAME>_TOKEN, so registry names containing other punctuation (for example my.registry) are looked up with the wrong env var (CARGO_REGISTRIES_MY.REGISTRY_TOKEN) and the token is missed. In this commit, shipper::auth::resolve_token is now wired through this absorbed resolver path for the main crate, so users who previously relied on Cargo-style normalization (non-alphanumeric -> _) can regress to false "no token" preflight/publish failures even when the correct env var is set.
Useful? React with 👍 / 👎.
Delete the stale 1212-LOC in-tree `crates/shipper/src/auth.rs` and promote the 333-LOC shim from `auth_micro.rs` to canonical `auth.rs`. The shim was already the production code path (CLI defaults to `micro-all`), and it is more than a thin re-export: it layers credential-file fallback and OIDC detection on top of the `shipper-auth` microcrate. Preserved shim behavior: - Delegation to `shipper_auth::resolve_token` as first-choice path. - Legacy fallback to `\$CARGO_HOME/credentials.toml` and the older `credentials` file when the microcrate returns no token. - crates.io alias handling (`crates.io`, `crates_io`, and the nested-table `[registries.crates.io]` TOML shape). - `detect_auth_type` / `detect_auth_type_from_token` helpers for OIDC trusted-publishing diagnostics. - All `serial_test` unit tests covering resolution order, env precedence, legacy credentials file, and OIDC detection. Public API surface is a superset of the deleted in-tree module (`resolve_token`, `detect_auth_type`, `detect_auth_type_from_token`), so no call-site changes were required. Feature-flag cleanup: - Remove `micro-auth` feature from `crates/shipper/Cargo.toml`; make `shipper-auth` a non-optional dependency. - Drop `micro-auth` from the `micro-all` feature list. - Remove `micro-auth` from `crates/shipper-cli/Cargo.toml` features. - Remove `micro-auth` from the BDD feature-set matrix in `.github/workflows/ci.yml` and `templates/circleci-config.yml`. - Update `docs/architecture.md` and `docs/testing.md` to reflect that `shipper-auth` is now a required dependency. Validation: - `cargo test -p shipper` — all tests pass (incl. 8 auth + 4 facade integration auth tests). - `cargo test -p shipper-cli` — all tests pass. - `cargo build -p shipper-cli` — succeeds. - `cargo check --workspace` — succeeds. - `cargo clippy -p shipper --all-targets --all-features -- -D warnings` — clean. Net LOC removed: ~879 (1212 deleted in-tree file minus 333 promoted shim).
Stacks on PR #51 (which did the dual-implementation cleanup). Move the 1762-LOC standalone shipper-auth crate into crates/shipper/src/ops/auth/ as crate-private modules, split into mod.rs (facade), resolver.rs (env + credentials-file token resolution, AuthInfo/TokenSource, mask_token, cargo_home_path), credentials.rs (credentials.toml parsing, legacy format + crates.io aliases, list_configured_registries), and oidc.rs (trusted-publishing env-var detection). Preserves the 333-LOC credential fallback path that PR #51 promoted to canonical (now the extended lookup in credentials::token_from_credentials_file_extended). Delete the standalone crate, the dedupped in-tree auth.rs, and the shipper-auth dep from shipper. Remove from workspace members. Drop shipper-auth references from .github/workflows/mutation.yml, docs/architecture.md, docs/testing.md, and RELEASE_CHECKLIST_v0.3.0.md. shipper::auth::* public re-export preserved via a thin facade module in lib.rs that re-exports from the new crate-private crate::ops::auth -- no breaking change for external consumers (fuzz targets and facade_integration tests continue to compile unchanged). All snapshot tests moved to crates/shipper/src/ops/auth/snapshots/ with renamed source paths. 3 orphaned hardening_snapshots files (no tests referenced them) are dropped. Test counts after absorption: shipper 824 + shipper-cli full suite all green; clippy clean. Per docs/decrating-plan.md S6 Phase 2.
d9321ef to
f1b06f1
Compare
3 tasks
EffortlessSteven
added a commit
that referenced
this pull request
May 23, 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
Stacks on #51 (the dual-implementation cleanup). Reviewer should merge #51 first.
Full absorption of the
shipper-authmicrocrate into theshipperlibrary:shipper-auth/src/lib.rs+ the 333-LOC post-decrating: Phase 1 (hard) — collapse auth dual implementation, preserve shim fallback #51 shim (crates/shipper/src/auth.rs) intocrates/shipper/src/ops/auth/.mod.rs(facade + top-levelresolve_token/detect_auth_typewith whitespace-trim and legacy-credentials fallback)resolver.rs(env-var + credentials-file resolution;AuthInfo/TokenSource;mask_token,cargo_home_path,has_token)credentials.rs(credentials.tomlparsing — both strict micro-form and extended alias-aware form;list_configured_registries)oidc.rs(trusted-publishing env-var detection)pub(crate) mod ops;scaffold inlib.rs(per decrating plan Phase 2).shipper::auth::*public API via a thin facade module inlib.rsthat re-exports fromcrate::ops::auth.crates/shipper-auth/, the in-treeauth.rs, theshipper-authdep fromshipper, and the workspace-members entry.shipper-authreferences from CI, docs, and release checklist.Split rationale
The module-level CLAUDE.md records the responsibility split:
resolver.rsowns the env-var resolution path and the diagnostic types that surface throughshipper doctor.credentials.rsowns all TOML parsing, including the two forms (strict microcrate form, and the extended form that handlescrates-io/crates.io/crates_io/nested[registries.crates.io]).oidc.rsis a self-contained 4-line check that benefits from being its own test surface.mod.rsis a thin public-to-crate facade that layers whitespace-trim + legacy-credentials-filename fallback on top.Validation
cargo check --workspace --all-targetscargo fmt --all -- --checkcargo clippy -p shipper --all-targets --all-features -- -D warningscargo test -p shipper ops::authcargo test -p shippercargo test -p shipper-clicargo build -p shipper-cliPublic API surface
External consumers (fuzz targets,
facade_integration.rs, and the CLI atshipper::auth::detect_auth_type) continue to compile unchanged thanks to thepub mod auth { pub use crate::ops::auth::...; }facade inlib.rs.Snapshots
All 34 kept snapshots transferred from
crates/shipper-auth/src/snapshots/tocrates/shipper/src/ops/auth/snapshots/with renamed filenames (new crate + module path prefix) and editedsource:headers. 3 orphanedhardening_snapshotsfiles were dropped — no test code in the original microcrate referenced them.Test plan
shipper::auth::*public API unchanged after merge (fuzz targets + facade_integration already confirm this locally)shipper doctoroutput is unchanged (sameAuthInfo/TokenSourcerendering)