decrating: Phase 1 (medium batch) — collapse dual implementations for environment/storage/git#53
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 1 minutes and 32 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: cd99998b39
ℹ️ 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
33
to
36
| default = ["micro-all"] | ||
| micro-auth = ["shipper/micro-auth"] | ||
| micro-git = ["shipper/micro-git"] | ||
| micro-events = ["shipper/micro-events"] | ||
| micro-lock = ["shipper/micro-lock"] |
There was a problem hiding this comment.
Keep removed micro- feature names CI-compatible*
Removing micro-git, micro-environment, and micro-storage from shipper-cli breaks the current BDD workflow matrix: I checked .github/workflows/ci.yml and it still runs cargo test -p shipper-cli --features <feature-set> with those three values (feature-set list at line 213), which now errors with “the package 'shipper-cli' does not contain this feature”. This makes those matrix jobs fail on every run unless the workflow is updated in the same change or compatibility aliases are kept in Cargo.toml.
Useful? React with 👍 / 👎.
3 tasks
EffortlessSteven
added a commit
that referenced
this pull request
Apr 15, 2026
Stacks on PR #53 (env+storage+git dual-impl cleanup). Move the 2202-LOC shipper-environment crate into crates/shipper/src/runtime/environment/ as crate-private modules (mod.rs + fingerprint.rs + ci.rs, ~2190 LOC total). Preserves the deduped in-tree wrapper logic (normalize_version, EnvironmentInfo::collect delegation with graceful fallback) from PR #53. Split seams: - mod.rs — CiEnvironment, detect_environment, is_ci, collect_environment_fingerprint (the absorbed shim), CiEnvironment snapshot tests. - fingerprint.rs — EnvironmentInfo, get_rust_version, get_cargo_version, get_environment_fingerprint, normalize_tool_version, collect_env_vars, tests + proptests. - ci.rs — get_ci_branch, get_ci_commit_sha, is_pull_request, provider-specific tests. All 13 snapshots preserved (renamed to `shipper__runtime__environment__ snapshot_tests__*.snap`). All 130 absorbed tests pass in-tree. Surgical unblock (precedent: PR #54 policy→engine-parallel inline): shipper-state depended on shipper-environment via one call site in migrate_v1_to_v2. Because shipper-state is an (optional) dep of shipper, it cannot depend on shipper itself — a dep cycle. Inlined an ~20-LOC collect_environment_fingerprint into shipper-state (marked `#[doc(hidden)]`, with a comment pointing back at crate::runtime::environment for the post-shipper-state-absorption replacement), and dropped shipper-environment from shipper-state's Cargo.toml. Will be replaced with a direct call to crate::runtime::environment::collect_environment_fingerprint after shipper-state is itself absorbed into shipper::state. Module is `pub(crate) mod runtime; mod environment;` — the former public `shipper::environment` API had no external consumers in-tree and is now crate-private per the runtime-layer discipline. `#[allow(dead_code)]` is applied at the runtime::environment level because several absorbed items (CI branch/SHA/PR helpers, the short pipe-fingerprint form, EnvironmentInfo::fingerprint) currently have no in-crate callers but retain full test coverage for future wiring. Also updates shipper's `insta` dev-dep to enable the `yaml` feature (required by the moved snapshot_tests module). Per docs/decrating-plan.md section 6 Phase 2.
10 tasks
…m logic) Eliminates the dual-path pattern for the environment subsystem: - Deletes the stale in-tree `environment.rs` (had only `collect_environment_fingerprint` via std::process::Command calls). - Promotes `environment_micro.rs` to canonical `environment.rs`. - The shim's added logic is preserved: it delegates to `shipper_environment::EnvironmentInfo::collect` (reusing CI detection, env-var capture, etc.) and normalizes the `cargo`/`rustc` version strings via `normalize_version` before mapping them into `EnvironmentFingerprint`. It also provides a graceful fallback when collection fails. - Removes `micro-environment` feature from shipper and shipper-cli Cargo.toml, makes `shipper-environment` a non-optional dep, drops the cross-reference from `micro-state` and `micro-all`.
…gic)
Eliminates the dual-path pattern for the storage subsystem:
- Deletes the stale in-tree `storage.rs`.
- Promotes `storage_micro.rs` to canonical `storage.rs`.
- The shim's added logic is preserved:
- `FileStorage` wrapper struct providing the historical
`base_path() -> &PathBuf` accessor (the microcrate exposes `path()`
instead), keeping the old `shipper::storage` API stable.
- `build_storage_backend` short-circuits for `StorageType::File` to
use the wrapped `FileStorage`, delegating to the microcrate only
for cloud backends.
- `config_from_env` accepts extra aliases: `local` (for `file`),
`gs` (for `gcs`), and `blob` (for `azure`).
- Removes `micro-storage` feature from shipper and shipper-cli
Cargo.toml, makes `shipper-storage` a non-optional dep, drops the
cross-reference from `micro-config` and `micro-all`.
Eliminates the dual-path pattern for the git subsystem:
- Deletes the stale in-tree `git.rs` (the large monolithic copy).
- Promotes `git_micro.rs` to canonical `git.rs`.
- The shim's added logic is preserved:
- `SHIPPER_GIT_BIN` environment override: when set, both
`collect_git_context` and `is_git_clean` route through a local
`Command`-based implementation that respects the custom git binary
path. When unset, they delegate to `shipper_git` (the microcrate).
- `git_program()` / `is_repo_root()` / `local_is_git_clean()`
helpers backing the override path.
- `ensure_git_clean` wrapper preserving the historical error
message ("commit/stash changes or use --allow-dirty").
- Removes `micro-git` feature from shipper and shipper-cli Cargo.toml,
makes `shipper-git` a non-optional dep, drops it from `micro-all`.
cd99998 to
294e381
Compare
EffortlessSteven
added a commit
that referenced
this pull request
Apr 15, 2026
…age, backend → shipper::ops::storage Per docs/decrating-plan.md §3.2, shipper-storage is split by concern rather than absorbed wholesale. The crate currently only implements filesystem storage (S3/GCS/Azure bail out as "not yet implemented") — not a finished public product yet. This PR: - Moves StorageType, CloudStorageConfig, and their error types into shipper-types::storage (the stable contract surface that embedders use to declare their storage choice). anyhow is avoided here — the new ParseStorageTypeError and ValidateStorageConfigError keep the types crate free of anyhow. - Moves StorageBackend trait, FileStorage impl, cloud-backend stubs, build_storage_backend factory, and config_from_env env-parsing into shipper/src/ops/storage/ as crate-private (pub(crate)). We do not promise the StorageBackend trait publicly until cloud backends are real. - Introduces shipper::ops as the crate-private operational layer for future internals that should not yet be on the public API surface. - Updates shipper-config to import CloudStorageConfig/StorageType from shipper_types::storage and drops its shipper-storage dependency. - Removes the shipper-storage crate from the workspace and deletes it. - Updates docs/architecture.md and RELEASE_CHECKLIST_v0.3.0.md to drop shipper-storage references. Stacks on PR #53 (env+storage+git dedup intermediate). Per docs/decrating-plan.md §6 Phase 2 (storage SPLIT special case).
Merged
5 tasks
EffortlessSteven
added a commit
that referenced
this pull request
Apr 15, 2026
Stacks on PR #53 (env+storage+git dual-impl cleanup). Move the 2202-LOC shipper-environment crate into crates/shipper/src/runtime/environment/ as crate-private modules (mod.rs + fingerprint.rs + ci.rs, ~2190 LOC total). Preserves the deduped in-tree wrapper logic (normalize_version, EnvironmentInfo::collect delegation with graceful fallback) from PR #53. Split seams: - mod.rs — CiEnvironment, detect_environment, is_ci, collect_environment_fingerprint (the absorbed shim), CiEnvironment snapshot tests. - fingerprint.rs — EnvironmentInfo, get_rust_version, get_cargo_version, get_environment_fingerprint, normalize_tool_version, collect_env_vars, tests + proptests. - ci.rs — get_ci_branch, get_ci_commit_sha, is_pull_request, provider-specific tests. All 13 snapshots preserved (renamed to `shipper__runtime__environment__ snapshot_tests__*.snap`). All 130 absorbed tests pass in-tree. Surgical unblock (precedent: PR #54 policy→engine-parallel inline): shipper-state depended on shipper-environment via one call site in migrate_v1_to_v2. Because shipper-state is an (optional) dep of shipper, it cannot depend on shipper itself — a dep cycle. Inlined an ~20-LOC collect_environment_fingerprint into shipper-state (marked `#[doc(hidden)]`, with a comment pointing back at crate::runtime::environment for the post-shipper-state-absorption replacement), and dropped shipper-environment from shipper-state's Cargo.toml. Will be replaced with a direct call to crate::runtime::environment::collect_environment_fingerprint after shipper-state is itself absorbed into shipper::state. Module is `pub(crate) mod runtime; mod environment;` — the former public `shipper::environment` API had no external consumers in-tree and is now crate-private per the runtime-layer discipline. `#[allow(dead_code)]` is applied at the runtime::environment level because several absorbed items (CI branch/SHA/PR helpers, the short pipe-fingerprint form, EnvironmentInfo::fingerprint) currently have no in-crate callers but retain full test coverage for future wiring. Also updates shipper's `insta` dev-dep to enable the `yaml` feature (required by the moved snapshot_tests module). Per docs/decrating-plan.md section 6 Phase 2.
EffortlessSteven
added a commit
that referenced
this pull request
Apr 15, 2026
…age, backend → shipper::ops::storage Per docs/decrating-plan.md §3.2, shipper-storage is split by concern rather than absorbed wholesale. The crate currently only implements filesystem storage (S3/GCS/Azure bail out as "not yet implemented") — not a finished public product yet. This PR: - Moves StorageType, CloudStorageConfig, and their error types into shipper-types::storage (the stable contract surface that embedders use to declare their storage choice). anyhow is avoided here — the new ParseStorageTypeError and ValidateStorageConfigError keep the types crate free of anyhow. - Moves StorageBackend trait, FileStorage impl, cloud-backend stubs, build_storage_backend factory, and config_from_env env-parsing into shipper/src/ops/storage/ as crate-private (pub(crate)). We do not promise the StorageBackend trait publicly until cloud backends are real. - Introduces shipper::ops as the crate-private operational layer for future internals that should not yet be on the public API surface. - Updates shipper-config to import CloudStorageConfig/StorageType from shipper_types::storage and drops its shipper-storage dependency. - Removes the shipper-storage crate from the workspace and deletes it. - Updates docs/architecture.md and RELEASE_CHECKLIST_v0.3.0.md to drop shipper-storage references. Stacks on PR #53 (env+storage+git dedup intermediate). Per docs/decrating-plan.md §6 Phase 2 (storage SPLIT special case).
EffortlessSteven
added a commit
that referenced
this pull request
May 23, 2026
…-handoff-2026-05-23 merge: backfill shipper handoff-doc sync
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 1 (medium batch) of the decrating effort. The
shippercrate had a dual-implementation pattern for each subsystem: an in-tree<name>.rsand a<name>_micro.rsshim gated behindmicro-<name>features. The CLI defaults tomicro-all, so the shim path was the canonical production path, and the in-tree files were stale duplicates.For these three subsystems, the shim carried non-trivial logic layered on top of the standalone microcrate. This PR:
<name>_micro.rsshim (with its added logic) to canonical<name>.rs.micro-<name>feature flags from bothshipperandshipper-cliCargo.toml, makes the microcrate deps non-optional, and cleans up cross-references inmicro-state,micro-config, andmicro-all.The standalone microcrates (
shipper-environment,shipper-storage,shipper-git) remain as workspace members and are now unconditional dependencies ofshipper. This PR does not absorb them.Net diff vs
origin/main: 9 files changed, 170 insertions(+), 1865 deletions(-) — roughly 1,695 LOC removed.Per-subsystem: what shim logic was preserved
environment (
4fef80b)shipper_environment::EnvironmentInfo::collect, which centralizes CI detection, env-var capture, OS/arch probing, and cargo/rustc version collection.normalize_versionextracts the numeric version token (e.g.,"1.75.0") from command output.EnvironmentFingerprint).CiEnvironment,detect_environment,is_ci,is_pull_request, etc.storage (
e9081a1)FileStoragewrapper struct providing the historicalbase_path() -> &PathBufaccessor (the microcrate exposespath()), keeping the oldshipper::storageAPI stable.build_storage_backendshort-circuitsStorageType::Filethrough the wrappedFileStorageand delegates to the microcrate for cloud backends.config_from_envaccepts extra aliases:local(forfile),gs(forgcs),blob(forazure).git (
cd99998)SHIPPER_GIT_BINenvironment override: when set,collect_git_contextandis_git_cleanroute through a localCommand-based implementation that respects the custom git binary path. When unset, they delegate toshipper_git.git_program(),is_repo_root(),local_is_git_clean()helpers backing the override path.ensure_git_cleanpreserves the historical error message ("commit/stash changes or use --allow-dirty").Validation
All four gates pass locally:
cargo test -p shipper— all tests pass, 0 failed (727 unit + integration tests + 2 schema-version tests + ignored doctests).cargo test -p shipper-cli— all test suites pass, 0 failed.cargo build -p shipper-cli— clean build.cargo check --workspace— clean.Test plan
cargo test -p shipperpasses locallycargo test -p shipper-clipasses locallycargo build -p shipper-clisucceedscargo check --workspacesucceedsshipperagainst a toy workspace post-merge (optional)Notes
default = ["micro-all"]continues to compile inshipper-cli; the now-removedmicro-environment,micro-storage,micro-gitfeature names no longer exist but were not part of any user-facing contract.micro-environment,micro-storage, ormicro-gitindividually will see a compile error; the recommended fix is to drop those feature names (the behavior they selected is now the default and only path).