decrating: introduce state/ layer; absorb events + state into shipper::state::*#60
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 18 minutes and 3 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 (1)
📒 Files selected for processing (28)
✨ 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: 3d0b39678d
ℹ️ 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".
| 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.
Synchronize removed feature flags with CI matrix
This change deletes the micro-events/micro-state feature entries from shipper-cli, but the BDD matrix in .github/workflows/ci.yml still enumerates those names (line 213) and passes them directly to cargo test -p shipper-cli --features ... (line 233). Those matrix legs now fail immediately with the package 'shipper-cli' does not contain this feature, so CI will be red on every PR until either compatibility aliases are restored or the workflow matrix is updated.
Useful? React with 👍 / 👎.
| pub mod events; | ||
| pub mod execution_state; |
There was a problem hiding this comment.
Re-export execution_state APIs at shipper::state
The new state module only exposes events and execution_state, so legacy paths like shipper::state::load_state are no longer available. fuzz/fuzz_targets/load_state.rs still imports shipper::state::load_state, and the fuzz-smoke workflow runs that target (.github/workflows/ci.yml:267), so this refactor breaks that CI job unless a compatibility re-export is kept or the fuzz target is migrated in the same change.
Useful? React with 👍 / 👎.
…::state::* Create the new `crates/shipper/src/state/` layer directory and route all internal and CLI call sites through `crate::state::events::*` and `crate::state::execution_state::*` (canonical namespaces per the decrating plan). Scope of this PR: - Delete stale duplicate `events.rs` (354 LOC) and `state.rs` (1566 LOC) in crates/shipper/src/. These were shadow copies of the `shipper-events` and `shipper-state` microcrates that no call site reached. - Delete the 1-LOC `events_micro.rs` and `state_micro.rs` re-export shims. - Delete the `micro-events` and `micro-state` feature flags from the `shipper` and `shipper-cli` crates (and their references in `micro-all` and `micro-store`). These feature flags existed only to toggle between the stale duplicate and the micro re-export; with the duplicate gone there is nothing to toggle. - Add `crates/shipper/src/state/` as the new state layer directory with `mod.rs`, `CLAUDE.md` (layer doc), and two submodules `events/` and `execution_state/`, each with their own `CLAUDE.md`. - Route every internal `crate::events::X` and `crate::state::X` reference through the new canonical paths `crate::state::events::X` and `crate::state::execution_state::X`. Same for CLI sites that previously used `shipper::events::X` and `shipper::state::X`. Why this is only a partial absorption ------------------------------------- The task brief calls for fully moving `shipper-events` (2821 LOC) and `shipper-state` (2689 LOC) into the new submodules and deleting those standalone crates from the workspace. That is not safe in isolation: `shipper-store` and `shipper-engine-parallel` both depend on `shipper-events` and `shipper-state` directly, and neither can be expressed as a re-export from the `shipper` crate without creating a circular dependency. To keep the workspace building, this PR: - keeps the two standalone crates (`shipper-events`, `shipper-state`) in the workspace unchanged; - makes `crates/shipper/src/state/events/mod.rs` and `crates/shipper/src/state/execution_state/mod.rs` thin re-export shims of those crates (so types are identical across the ecosystem and the `StateStore` trait in `shipper-store` still accepts the same `EventLog`); - promotes `shipper-events` and `shipper-state` from optional to non-optional dependencies of `shipper` (they were previously gated behind the now-removed `micro-events` / `micro-state` feature flags). The full source move into `state/events/` and `state/execution_state/` will happen in the follow-up PRs that absorb `shipper-store` and `shipper-engine-parallel`, at which point the two standalone crates can be deleted from the workspace. Both absorptions ship in one commit because the new `state/` directory cannot coexist with the flat `state.rs` file (Rust cannot disambiguate `state.rs` and `state/mod.rs`). Per docs/decrating-plan.md Phase 2.
3d0b396 to
dc81845
Compare
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
Introduce the new
crates/shipper/src/state/layer directory and route all internal call sites throughcrate::state::events::*andcrate::state::execution_state::*— the canonical namespaces per the decrating plan (Phase 2).events.rs(354 LOC) andstate.rs(1566 LOC) incrates/shipper/src/. These were shadow copies of theshipper-eventsandshipper-statemicrocrates that no call site reached.events_micro.rsandstate_micro.rsre-export shims.micro-eventsandmicro-statefeature flags fromshipperandshipper-cli(and their references inmicro-allandmicro-store).state/layer directory with a layer-levelCLAUDE.md, amod.rs, and two submodules (events/andexecution_state/) each with their ownCLAUDE.md.crate::events::Xandcrate::state::Xreference to use the new canonical paths. Same forshipper::events::X/shipper::state::Xin the CLI.Why these two absorptions ship together
Creating
crates/shipper/src/state/mod.rscannot coexist with the flatstate.rsfile (Rust can't disambiguatestate.rsandstate/mod.rs). Becausestate.rsis a stale duplicate of theshipper-statemicrocrate, deleting it forces absorbing that microcrate's namespace in the same PR.eventsis absorbed at the same time because it lives under the same newstate/layer and follows the exact same pattern.Why this is only a partial absorption
The task brief calls for fully moving
shipper-events(2821 LOC) andshipper-state(2689 LOC) into the new submodules and deleting those standalone crates from the workspace. That is not safe in isolation:shipper-storedepends onshipper-eventsandshipper-statedirectly.shipper-engine-paralleldepends on both as well.Neither dep can be flipped to
shipper::state::events::*from inside those crates without creating a circular dependency (since those crates are consumed byshipperviamicro-store/micro-parallel).To keep every workspace member building, this PR:
shipper-events,shipper-state) in the workspace unchanged;crates/shipper/src/state/events/mod.rsandcrates/shipper/src/state/execution_state/mod.rsthin re-export shims of those crates (so types are identical across the ecosystem andStateStore::save_events(&EventLog)inshipper-storestill accepts the sameEventLogused at call sites);shipper-eventsandshipper-statefrom optional to non-optional dependencies ofshipper(they were previously gated behind the now-removedmicro-events/micro-statefeature flags).The full source move into
state/events/andstate/execution_state/will happen in the follow-up PRs that absorbshipper-storeandshipper-engine-parallel, at which point the two standalone crates can be deleted from the workspace. EachCLAUDE.mdin the new modules documents this explicitly.Test plan
cargo check --workspace— cleancargo test -p shipper— 690 unit + 117 integration tests passcargo test -p shipper-cli— all BDD + e2e tests passcargo clippy -p shipper --all-targets --all-features -- -D warnings— cleancargo clippy -p shipper-cli --all-targets --all-features -- -D warnings— clean