decrating: Phase 0 — plan doc + architecture-guard CI#48
Merged
Conversation
Adds docs/decrating-plan.md (the full 11-section migration plan to consolidate 30 microcrates down to 13 published crates while preserving SRP via folder- based modules inside the shipper umbrella crate). Adds .github/workflows/architecture-guard.yml that enforces the one-direction layer rule (engine -> plan -> state -> runtime -> ops) once Phase 3 scaffolds the layer dirs. No-op until then. This is Phase 0 of the plan. Subsequent PRs will execute Phases 1-10.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Summary by CodeRabbit
WalkthroughA GitHub Actions workflow is added to enforce architectural layer import constraints within the shipper crate, rejecting upward dependencies across five defined layers. Simultaneously, a detailed decrating plan document is introduced, outlining a multi-phase microcrate consolidation and internal module architecture targeting version 0.3.0. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~20 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: 6b2ccb2bc0
ℹ️ 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".
| echo "ops/ layer not yet scaffolded — skipping (this is expected before Phase 3)." | ||
| exit 0 | ||
| fi | ||
| if grep -rE 'use crate::(engine|plan|state|runtime)::' crates/shipper/src/ops/; then |
There was a problem hiding this comment.
Catch module imports in upward-layer grep checks
The guard regexes only match paths shaped like use crate::plan::... (note the required trailing ::), so forbidden upward dependencies written as use crate::plan; or grouped imports like use crate::{plan::X}; will pass CI. This repo already uses the module-import form extensively (for example use crate::state;), so once layer folders exist, developers can introduce upward imports without tripping this workflow, which defeats the purpose of the architecture gate.
Useful? React with 👍 / 👎.
Per execution feedback: splitting each subsystem into three separate PRs (eliminate dual impl, then move into folder, then absorb microcrate) creates 50+ PRs with intermediate states that compile but expose meaningless transitional APIs. Each subsystem now lands in ONE atomic PR that does all three operations. Also confirms the audit's "in-tree is stale duplicate" finding as a hard rule: when both an in-tree shipper/src/<name>.rs and a standalone shipper-<name> microcrate exist, the standalone is canonical and the in-tree is deleted. Renumbers downstream phases (was 6-10, now 4-8). Adds wave-based parallelization table for Phase 2 across non-overlapping layers.
This was referenced Apr 15, 2026
Adds the two-ring framing for the 13 published crates: - Ring 1 (5 crates): real supported product surface — shipper, shipper-cli, shipper-config, shipper-types, shipper-schema. The "product story." - Ring 2 (8 crates): published support crates — duration, retry, encrypt, webhook, registry, sparse-index, cargo-failure, output-sanitizer. Useful narrow seams, not product narrative. Reframes the criteria for staying public: a boundary deserves a crate when it's worth a crates.io page + semver promise + permanent name + ongoing support. Ring 1 clears that bar AND carries product narrative; ring 2 clears the support bar but doesn't carry narrative. Also revises shipper-storage from "absorb wholesale into ops/storage/" to SPLIT BY CONCERN: storage config/data types go into shipper-types::storage (the stable contract surface), runtime backend code goes into shipper/src/ops/storage/. Reason: the crate only implements filesystem today (S3/GCS/Azure bail out) — not a finished public product yet.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/architecture-guard.yml:
- Line 36: The grep checks use unanchored patterns and no file-type filter,
causing matches in comments/docs and non-Rust files; update each occurrence of
the grep command (the line using grep -rE 'use
crate::(engine|plan|state|runtime)::') to restrict to Rust files and anchor to
real use statements, e.g. replace with a command that limits files to *.rs and
matches lines starting with optional whitespace then "use", like grep -r
--include='*.rs' -nE "^\s*use\s+crate::(engine|plan|state|runtime)::", so the CI
only flags actual import statements (apply the same change to the four grep
occurrences).
In `@docs/decrating-plan.md`:
- Line 443: The references for the engine_parallel special-case are pointing to
§5.1 (which is the shipper-registry special case) — find the two occurrences
where `engine_parallel` mentions “§5.1” and update them to point to the correct
section that documents the `engine_parallel` special-case (replace the wrong
section number/anchor with the proper § number or anchor ID for the
engine_parallel special-case), and verify the updated references resolve to the
engine_parallel text and that both occurrences are changed (the two places that
mention `engine_parallel`/“§5.1”).
- Line 605: The markdown has "Hazards" subsection headings that lack surrounding
blank lines (triggering MD022); for each occurrence near the "### 7.1 Tests
inside microcrates need a destination" section ensure there is exactly one blank
line before and one blank line after each "Hazards" heading (identify them by
the literal heading text "Hazards" and the enclosing section header "### 7.1
Tests inside microcrates need a destination") so that every "Hazards" subsection
is separated from preceding and following content, making the headings properly
surrounded by blank lines.
- Line 57: Multiple fenced code blocks in the document are missing language
identifiers (MD040) and/or surrounding blank lines (MD031); update each fenced
block to add an appropriate language tag (e.g., ```bash, ```json, ```text) and
ensure there is a blank line before and after each fenced block so markdownlint
passes; scan the entire docs/decrating-plan.md for the reported occurrences and
normalize each fenced block accordingly (add language id and surrounding blank
lines) to resolve MD040 and MD031 errors.
- Around line 540-545: The phase numbering in the decrating-plan is inconsistent
(Phase 6 -> Phase 8, later refs to Phase 9/10 and an older 0–10 effort
breakdown) and will confuse execution and gating; update all phase headings and
inline references so phases are sequential and consistent (e.g., renumber Phase
8/9/10 to follow Phase 6), adjust the effort breakdown to match the final phase
count (0–8 if you stop at Phase 8), and ensure all validation gates and
references (any occurrences of "Phase X", the effort table, and the "Validation
gate" lines) reflect the new numbering and counts throughout the document.
🪄 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: 3e5bdadd-7a2c-4a91-8b9f-01206643c307
📒 Files selected for processing (2)
.github/workflows/architecture-guard.ymldocs/decrating-plan.md
| echo "ops/ layer not yet scaffolded — skipping (this is expected before Phase 3)." | ||
| exit 0 | ||
| fi | ||
| if grep -rE 'use crate::(engine|plan|state|runtime)::' crates/shipper/src/ops/; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current guard commands are not Rust-file scoped / anchored.
python - <<'PY'
from pathlib import Path
wf = Path(".github/workflows/architecture-guard.yml")
lines = wf.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "grep -rE" in line:
rs_scoped = "--include='*.rs'" in line or '--include="*.rs"' in line
anchored = "^[[:space:]]*" in line
print(f"Line {i}: rs_scoped={rs_scoped}, anchored={anchored}")
print(f" {line.strip()}")
PYRepository: EffortlessMetrics/shipper
Length of output: 544
🏁 Script executed:
# Check for non-Rust files in the target directories
find crates/shipper/src/ops crates/shipper/src/runtime crates/shipper/src/state crates/shipper/src/plan -type f ! -name "*.rs" 2>/dev/null | head -20Repository: EffortlessMetrics/shipper
Length of output: 51
🏁 Script executed:
# Verify the proposed grep pattern syntax is valid
echo "pub use crate::engine::something" | grep -E '^[[:space:]]*(pub(\([^)]*\))?[[:space:]]+)?use[[:space:]]+crate::.*(engine)::' && echo "✓ Pattern matches valid use statement" || echo "✗ Pattern failed"Repository: EffortlessMetrics/shipper
Length of output: 139
🏁 Script executed:
# Test that current loose pattern would match non-code text
echo "This mentions use crate::engine:: in documentation" | grep -rE 'use crate::(engine)::' && echo "✓ Current pattern has false positive risk" || echo "✗ Pattern did not match"Repository: EffortlessMetrics/shipper
Length of output: 441
Harden import checks to avoid false positives and missed violations.
The current grep -rE commands lack file-type filtering and use unanchored patterns that can match use crate::...:: in doc strings and comments within Rust files, rather than only actual import statements. This reduces CI reliability.
🔧 Proposed fix
- if grep -rE 'use crate::(engine|plan|state|runtime)::' crates/shipper/src/ops/; then
+ if grep -rE --include='*.rs' '^[[:space:]]*(pub(\([^)]*\))?[[:space:]]+)?use[[:space:]]+crate::.*(engine|plan|state|runtime)::' crates/shipper/src/ops/; then
echo "::error::ops/ layer must not import from engine, plan, state, or runtime"
exit 1
fi
@@
- if grep -rE 'use crate::(engine|plan|state)::' crates/shipper/src/runtime/; then
+ if grep -rE --include='*.rs' '^[[:space:]]*(pub(\([^)]*\))?[[:space:]]+)?use[[:space:]]+crate::.*(engine|plan|state)::' crates/shipper/src/runtime/; then
echo "::error::runtime/ layer must not import from engine, plan, or state"
exit 1
fi
@@
- if grep -rE 'use crate::(engine|plan)::' crates/shipper/src/state/; then
+ if grep -rE --include='*.rs' '^[[:space:]]*(pub(\([^)]*\))?[[:space:]]+)?use[[:space:]]+crate::.*(engine|plan)::' crates/shipper/src/state/; then
echo "::error::state/ layer must not import from engine or plan"
exit 1
fi
@@
- if grep -rE 'use crate::engine::' crates/shipper/src/plan/; then
+ if grep -rE --include='*.rs' '^[[:space:]]*(pub(\([^)]*\))?[[:space:]]+)?use[[:space:]]+crate::.*engine::' crates/shipper/src/plan/; then
echo "::error::plan/ layer must not import from engine"
exit 1
fiApplies to lines 36, 48, 60, 72.
📝 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
| if grep -rE 'use crate::(engine|plan|state|runtime)::' crates/shipper/src/ops/; then | |
| - name: Architecture Guard | |
| run: | | |
| if grep -rE --include='*.rs' '^[[:space:]]*(pub(\([^)]*\))?[[:space:]]+)?use[[:space:]]+crate::.*(engine|plan|state|runtime)::' crates/shipper/src/ops/; then | |
| echo "::error::ops/ layer must not import from engine, plan, state, or runtime" | |
| exit 1 | |
| fi | |
| if grep -rE --include='*.rs' '^[[:space:]]*(pub(\([^)]*\))?[[:space:]]+)?use[[:space:]]+crate::.*(engine|plan|state)::' crates/shipper/src/runtime/; then | |
| echo "::error::runtime/ layer must not import from engine, plan, or state" | |
| exit 1 | |
| fi | |
| if grep -rE --include='*.rs' '^[[:space:]]*(pub(\([^)]*\))?[[:space:]]+)?use[[:space:]]+crate::.*(engine|plan)::' crates/shipper/src/state/; then | |
| echo "::error::state/ layer must not import from engine or plan" | |
| exit 1 | |
| fi | |
| if grep -rE --include='*.rs' '^[[:space:]]*(pub(\([^)]*\))?[[:space:]]+)?use[[:space:]]+crate::.*engine::' crates/shipper/src/plan/; then | |
| echo "::error::plan/ layer must not import from engine" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/architecture-guard.yml at line 36, The grep checks use
unanchored patterns and no file-type filter, causing matches in comments/docs
and non-Rust files; update each occurrence of the grep command (the line using
grep -rE 'use crate::(engine|plan|state|runtime)::') to restrict to Rust files
and anchor to real use statements, e.g. replace with a command that limits files
to *.rs and matches lines starting with optional whitespace then "use", like
grep -r --include='*.rs' -nE "^\s*use\s+crate::(engine|plan|state|runtime)::",
so the CI only flags actual import statements (apply the same change to the four
grep occurrences).
|
|
||
| ## 3. Target Public Crate Graph (13 crates) | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Code-fence formatting is lint-failing in multiple blocks.
Several fenced blocks are missing language identifiers (MD040) and/or required surrounding blank lines (MD031). Please normalize these blocks to keep markdownlint clean.
Also applies to: 158-158, 322-322, 420-420, 446-446, 465-465, 476-476, 486-486, 549-549, 585-585
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/decrating-plan.md` at line 57, Multiple fenced code blocks in the
document are missing language identifiers (MD040) and/or surrounding blank lines
(MD031); update each fenced block to add an appropriate language tag (e.g.,
```bash, ```json, ```text) and ensure there is a blank line before and after
each fenced block so markdownlint passes; scan the entire docs/decrating-plan.md
for the reported occurrences and normalize each fenced block accordingly (add
language id and surrounding blank lines) to resolve MD040 and MD031 errors.
|
|
||
| 1. **Determine canonical version.** Per the audit (§5), the standalone microcrate is canonical for nearly all subsystems. The in-tree `<name>.rs` is a stale duplicate. Exceptions: | ||
| - Some shims (`auth_micro.rs`, `git_micro.rs`, `storage_micro.rs`, `environment_micro.rs`) wrap the microcrate with additional logic (credential fallback, env overrides, compatibility wrappers) that must be preserved into the absorbed module. | ||
| - `engine_parallel` has both a 3237-LOC in-tree and a 4826-LOC standalone — the standalone is canonical (larger, has `webhook.rs` submodule, snapshot tests, BDD tests). See §5.1 for special handling. |
There was a problem hiding this comment.
engine_parallel points to the wrong special-case section.
Line 443 and Line 510 reference §5.1, but §5.1 is the shipper-registry special case, not engine_parallel.
Also applies to: 510-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/decrating-plan.md` at line 443, The references for the engine_parallel
special-case are pointing to §5.1 (which is the shipper-registry special case) —
find the two occurrences where `engine_parallel` mentions “§5.1” and update them
to point to the correct section that documents the `engine_parallel`
special-case (replace the wrong section number/anchor with the proper § number
or anchor ID for the engine_parallel special-case), and verify the updated
references resolve to the engine_parallel text and that both occurrences are
changed (the two places that mention `engine_parallel`/“§5.1”).
Comment on lines
+540
to
+545
| ### Phase 6: Resolve `shipper-schema` vs `shipper-types` | ||
|
|
||
| Audit overlap. If schema is purely versioning constants, fold into `shipper-types::schema` and drop `shipper-schema`. Otherwise keep both. Document the decision. | ||
|
|
||
| **Validation gate:** workspace tests pass; `cargo public-api --diff` shows no unintended public-API expansion. | ||
|
|
There was a problem hiding this comment.
Phase numbering is internally inconsistent and makes execution gates ambiguous.
Section 6 ends at Phase 8, but Line 663/Line 664 still reference Phase 9/10, and Line 680+ uses an older 0–10 effort breakdown. This will cause confusion during execution and release gating.
Also applies to: 663-664, 669-690
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/decrating-plan.md` around lines 540 - 545, The phase numbering in the
decrating-plan is inconsistent (Phase 6 -> Phase 8, later refs to Phase 9/10 and
an older 0–10 effort breakdown) and will confuse execution and gating; update
all phase headings and inline references so phases are sequential and consistent
(e.g., renumber Phase 8/9/10 to follow Phase 6), adjust the effort breakdown to
match the final phase count (0–8 if you stop at Phase 8), and ensure all
validation gates and references (any occurrences of "Phase X", the effort table,
and the "Validation gate" lines) reflect the new numbering and counts throughout
the document.
|
|
||
| ## 7. Hazards & Mitigations (Learnings) | ||
|
|
||
| ### 7.1 Tests inside microcrates need a destination |
There was a problem hiding this comment.
Add blank lines around Hazards subsection headings.
The subsection heading sequence in this range triggers MD022 (headings not surrounded by blank lines).
Also applies to: 608-608, 611-611, 614-614, 617-617, 620-620, 625-625, 631-631, 634-634, 637-637, 642-642
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 605-605: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/decrating-plan.md` at line 605, The markdown has "Hazards" subsection
headings that lack surrounding blank lines (triggering MD022); for each
occurrence near the "### 7.1 Tests inside microcrates need a destination"
section ensure there is exactly one blank line before and one blank line after
each "Hazards" heading (identify them by the literal heading text "Hazards" and
the enclosing section header "### 7.1 Tests inside microcrates need a
destination") so that every "Hazards" subsection is separated from preceding and
following content, making the headings properly surrounded by blank lines.
3 tasks
EffortlessSteven
added a commit
that referenced
this pull request
Apr 15, 2026
Fixes two distinct failures that have kept CI red on `main` for weeks
and block every open PR:
1. Snapshot mismatch in `publish_invalid_manifest_content_snapshot`
(`crates/shipper-cli/tests/e2e_expanded.rs`). On Linux CI runners,
`cargo metadata` reports the invalid manifest using a path relative
to the workspace CWD (e.g. `../../../tmp/.tmpXXX/Cargo.toml`). The
tempdir replacement consumed the leading `/`, leaving `..` fragments
that the previous single-pass `.replace("../", "")` could not clean
up (it left `..<TMPDIR>/...`).
`normalize_stderr` now:
- converts backslashes to forward slashes so path separators are
platform-independent in snapshots; and
- iteratively strips `../` and any residual `..<TMPDIR>` fragments
until stable.
Updated the committed snapshot to the canonical forward-slash form.
2. Race in `FileStorage::write` causing
`tests::concurrent_reads_and_writes_same_file` to panic with
"No such file or directory (os error 2)" on macOS and Linux. All
threads writing to the same destination shared a single
`<basename>.tmp` scratch file, so one thread's `rename` could
remove the file while another thread's `rename` was still pending.
Each write now uses a unique temp filename built from pid, thread
id, and subsecond timestamp. Adjusted two local tests that inspected
the old fixed `.tmp` name to observe the more general invariant
("no `.tmp` extension files remain after a successful write") and
relaxed `atomic_write_overwrites_stale_tmp_from_prior_crash` to only
assert the new write succeeds (the new impl intentionally does not
clobber unrelated stale `.tmp` files).
Validated locally on Windows:
- `cargo test -p shipper-storage -- --include-ignored` → 93 lib + 33
integration + 1 doctest pass (including
`concurrent_reads_and_writes_same_file`, previously `#[cfg_attr]`
ignored on Windows; now runs cleanly).
- `cargo test -p shipper-cli --test e2e_expanded` → 126/126 pass.
- `cargo fmt --all -- --check` and
`cargo clippy -p shipper-storage -p shipper-cli --all-targets
--all-features -- -D warnings` both clean.
Coverage-job-only validation (`cargo llvm-cov`) is deferred to CI.
Unblocks green CI for the decrating PR queue (#48-#58).
5 tasks
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
Foundational PR for the decrating effort: consolidating the workspace from 30 microcrates down to 13 published crates while preserving SRP via folder-based modules inside the `shipper` umbrella crate.
This PR contains no functional changes. It adds:
`docs/decrating-plan.md` — full 11-section migration plan covering:
`.github/workflows/architecture-guard.yml` — CI grep-check that enforces the one-direction layer rule. No-op until Phase 3 scaffolds the layer dirs (`engine/`, `plan/`, `state/`, `runtime/`, `ops/`).
What this PR does NOT do
No code is moved. No crates are deleted. No features are removed. `micro-all` remains the CLI default. This PR is purely the planning artifact + CI guard placeholder, so the plan itself can be reviewed before any execution lands.
Test plan