decrating: Phase 1 (hard) — collapse auth dual implementation, preserve shim fallback#51
decrating: Phase 1 (hard) — collapse auth dual implementation, preserve shim fallback#51EffortlessSteven wants to merge 1 commit into
Conversation
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).
|
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 3 minutes and 6 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6121b291c
ℹ️ 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".
| let micro_token = shipper_auth::resolve_token(registry_name, None) | ||
| .token | ||
| .and_then(|token| { |
There was a problem hiding this comment.
Ignore whitespace-only default token before delegation
Calling shipper_auth::resolve_token first changes precedence when CARGO_REGISTRY_TOKEN is whitespace-only: the microcrate treats it as present and never checks CARGO_REGISTRIES_CRATES_IO_TOKEN, then this wrapper trims it to None and falls through to credential files. In practice, if CI exports CARGO_REGISTRY_TOKEN=" " but has a valid CARGO_REGISTRIES_CRATES_IO_TOKEN, token resolution now fails instead of using the named token as the previous in-crate implementation did.
Useful? React with 👍 / 👎.
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.
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.
Summary
Phase 1 of the decrating effort. Eliminates the dual
authimplementation inside theshippercrate by deleting the stale 1212-LOC in-treeauth.rsand promoting the 333-LOC shim (previouslyauth_micro.rs) to canonical. The shim was already the production code path (shipper-clidefaults tomicro-all), and it is not a thin re-export — it layers credential-file fallback and OIDC detection on top of theshipper-authmicrocrate. Those behaviors are preserved verbatim.The
shipper-authmicrocrate remains a workspace member and is now a non-optional dependency ofshipper. Absorption ofshipper-authintoshipperis explicitly out of scope (Phase 5).Shim logic preserved
shipper_auth::resolve_tokenas first-choice path.$CARGO_HOME/credentials.tomland the oldercredentialsfile when the microcrate returns no token.crates.io,crates_io, and the unquoted[registries.crates.io]nested-table TOML shape).detect_auth_typeanddetect_auth_type_from_tokenhelpers for OIDC trusted-publishing diagnostics.serial_test-marked unit tests (resolution order, env precedence, legacy credentials file, crates.io aliases, OIDC detection).API surface
No additions were required from the deleted in-tree module. The shim's public API (
resolve_token,detect_auth_type,detect_auth_type_from_token) is a superset — or functionally equivalent — of the in-tree version's public API. Call sites inengine.rs(auth::resolve_token,auth::detect_auth_type_from_token) work unchanged.Feature-flag cleanup
crates/shipper/Cargo.toml: removedoptional = truefromshipper-auth; deletedmicro-authfeature entry; removed"micro-auth"frommicro-all.crates/shipper-cli/Cargo.toml: removedmicro-authfeature entry..github/workflows/ci.ymlandtemplates/circleci-config.yml: removedmicro-authfrom the BDD feature-set matrices.docs/architecture.mdanddocs/testing.md: updated references.crates/shipper/src/lib.rs: collapsed the#[cfg(feature = "micro-auth")]gated module declaration to a singlepub mod auth;(preserving the preceding doc comment).Validation
cargo test -p shipper— pass (18 unit tests incl. auth, plus integration tests).cargo test -p shipper-cli— pass (all suites, including BDD).cargo build -p shipper-cli— pass.cargo check --workspace— pass.cargo clippy -p shipper --all-targets --all-features -- -D warnings— clean.Net LOC delta: ~879 lines removed (1212 deleted - 333 promoted).
Test plan
--features micro-authin scripts outside this repo (internal only; feature has never been externally useful sincemicro-allis the CLI default)auth::resolve_tokenandauth::detect_auth_type_from_tokenstill resolve viaengine.rsat runtime (covered by facade_integration and cross_crate_integration test suites)