feat(remediate): plan-yank --starting-crate graph mode (#98 PR 4)#138
Conversation
Adds the second \`plan-yank\` selector mode: **given a broken crate name, walk the workspace's dependency graph to find every crate that transitively depends on it, and emit a containment plan scoped to that affected chain.** Complements the existing \`--compromised-only\` (marker-based) selector. ## What's new - **\`plan_yank::build_plan_from_starting_crate\`**: takes a receipt, a dep-graph (\`BTreeMap<String, Vec<String>>\`), a starting crate, and an optional \`reason\`. Returns a \`YankPlan\` in reverse-topo order covering the starting crate + every transitive dependent. Errors if the starting crate isn't in the receipt. - **CLI**: \`--starting-crate <CRATE>\` and \`--reason <TEXT>\` on \`plan-yank\`. clap \`conflicts_with = \"starting_crate\"\` on \`--compromised-only\` rejects combinations at parse time. - CLI handler reads \`planned.plan.dependencies\` from the current workspace metadata for the graph walk. ## Algorithm Reverse BFS through the dep map: for each currently-reachable crate, scan the map for crates whose dep list contains it; add those to the frontier. Final set = starting crate + all transitive dependents. Then filter the receipt's topo-ordered packages to that set, keep only \`Published\` entries (Failed/never-shipped yank is a no-op), and reverse to get dependents-first order. ## Tests 5 new unit tests (10 total in \`plan_yank::tests\`): - walks all transitive dependents in reverse topo - ignores unrelated crates (no edge → not in plan) - skips non-Published entries (dependent that Failed doesn't yank) - \`--reason\` applies to every entry (falls back to receipt \`compromised_by\` otherwise) - errors when starting crate isn't in receipt Regenerated \`plan-yank --help\` snapshot. ## Closes half of #98's remaining gap \`plan-yank --starting-crate <name> --reason <text>\` now matches #98's issue spec verbatim. Combined with the earlier \`--compromised-only\` flow (#132), plan-yank fully satisfies the graph vs receipt-filter ask. Plan execution wrappers for yank/fix-forward are the remaining piece (#98 PR 5).
|
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 53 minutes and 54 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 (2)
✨ 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.
Code Review
This pull request introduces a "graph mode" for the plan-yank command, allowing users to generate a yank plan starting from a specific crate and including all its transitive dependents. It adds the --starting-crate and --reason flags to the CLI and implements the logic to walk the dependency graph backwards. A review comment points out that the --reason flag is currently ignored in the default and --compromised-only modes, suggesting it should be applied as a general override across all selection modes for consistency.
| } else { | ||
| PlanYankFilter::AllPublished | ||
| let filter = if compromised_only { | ||
| PlanYankFilter::CompromisedOnly | ||
| } else { | ||
| PlanYankFilter::AllPublished | ||
| }; | ||
| plan_yank::build_plan(&receipt, filter) | ||
| }; |
There was a problem hiding this comment.
The --reason flag is currently ignored when --starting-crate is not provided. According to the help text and the intended behavior of the flag as a general override, it should be applied to the plan entries even in the default or --compromised-only modes. This ensures consistency across all plan-yank selection modes.
} else {
let filter = if compromised_only {
PlanYankFilter::CompromisedOnly
} else {
PlanYankFilter::AllPublished
};
let mut plan = plan_yank::build_plan(&receipt, filter);
if let Some(ref r) = reason {
for entry in &mut plan.entries {
entry.reason = Some(r.clone());
}
}
plan
};There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50cb43072e
ℹ️ 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".
| plan_yank::build_plan_from_starting_crate( | ||
| &receipt, | ||
| &planned.plan.dependencies, | ||
| starting, |
There was a problem hiding this comment.
Guard graph mode from mismatched historical receipts
In plan-yank --starting-crate mode, the affected chain is computed from planned.plan.dependencies (current workspace metadata) even when the user supplies --from-receipt for an older release. If dependency edges changed since that receipt was produced, this can silently omit published dependents that should be yanked, yielding an incomplete containment plan. This path should either validate that the current graph matches the receipt’s package set/plan context or fail fast when they diverge.
Useful? React with 👍 / 👎.
| }; | ||
| plan_yank::build_plan(&receipt, filter) |
There was a problem hiding this comment.
Honor --reason when not using --starting-crate
The CLI advertises --reason as a per-entry override for the yank plan, but in the non-graph branch (default and --compromised-only) the parsed reason is dropped and build_plan is called without any override. As a result, shipper plan-yank --reason ... does not actually apply the provided reason unless --starting-crate is also set, which makes the option behavior inconsistent and can produce missing/incorrect operator context in generated plans.
Useful? React with 👍 / 👎.
Closes the remaining plan-execution gap in #98. \`shipper plan-yank --format json\` emits a YankPlan; \`shipper yank --plan <PATH>\` walks that plan and invokes \`cargo yank\` per entry. ## What's new - \`YankEntry\` / \`YankPlan\` now derive \`Deserialize\` so plans round-trip via \`--format json\` → file → \`--plan\`. The \`filter\` field moves from \`&'static str\` to \`Cow<'static, str>\` so owned strings coming off disk don't require allocation in the construction path. - \`plan_yank::load_plan_from_path\` — thin helper mirroring \`load_receipt_from_path\`. - CLI: \`shipper yank --plan <PATH>\`. Mutually exclusive with \`--crate\` / \`--version\` / \`--reason\` (clap \`conflicts_with\`); those become optional in plan mode. - Handler: iterates entries in order, emits \`PackageYanked\` event per invocation, halts on first non-zero cargo exit (reverse-topo plan means every downstream entry would be failing to yank a dependent of a crate we just failed on — continuing is damage). ## Scope — what this PR does and doesn't **Does:** - Execute yank plans end-to-end. - Correct halt-on-failure semantics. - Events-as-truth: every invocation writes a PackageYanked event (with real exit code) to events.jsonl, including failures. **Does not:** - Resume interrupted plan runs. First yank is destructive on the registry; re-running the plan with the same reason is idempotent (cargo yank of an already-yanked version is a no-op), so the simplest fix is to just re-run the plan, and operator discipline picks up the rest. - Add a \`fix-forward\` executor. That requires Cargo.toml version bumps which are workspace-edit territory — deserves its own PR with --dry-run / --apply / git-cleanliness guards. ## Tests 3 new unit tests (8 total in \`plan_yank::tests\`): - YankPlan JSON round-trips: serialize, write, load → entries + order preserved, per-entry reason preserved. - \`load_plan_from_path\` errors cleanly on missing file. - \`load_plan_from_path\` errors cleanly on malformed JSON. Regenerated \`yank --help\` snapshot. ## Closes the rest of #98's open gap With PR #138 (starting-crate) and this one, #98's acceptance checklist is fulfilled: - [x] Receipt fields (compromised_at/compromised_by/superseded_by) — PR #132 - [x] plan-yank --from-receipt + --starting-crate + --reason — PR #132 + #138 - [x] shipper yank executes a plan — **this PR** - [ ] shipper fix-forward --plan executes a plan — follow-on (needs Cargo.toml edit tooling) - [x] Docs distinguish yank semantics — PR #134 The fix-forward executor is the last remaining #98 piece. It's cleanly deferred because it adds workspace-edit scope; this PR keeps the execution story for yank self-contained.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Closes the remaining plan-execution gap in #98. \`shipper plan-yank --format json\` emits a YankPlan; \`shipper yank --plan <PATH>\` walks that plan and invokes \`cargo yank\` per entry. - \`YankEntry\` / \`YankPlan\` now derive \`Deserialize\` so plans round-trip via \`--format json\` → file → \`--plan\`. The \`filter\` field moves from \`&'static str\` to \`Cow<'static, str>\` so owned strings coming off disk don't require allocation in the construction path. - \`plan_yank::load_plan_from_path\` — thin helper mirroring \`load_receipt_from_path\`. - CLI: \`shipper yank --plan <PATH>\`. Mutually exclusive with \`--crate\` / \`--version\` / \`--reason\` (clap \`conflicts_with\`); those become optional in plan mode. - Handler: iterates entries in order, emits \`PackageYanked\` event per invocation, halts on first non-zero cargo exit (reverse-topo plan means every downstream entry would be failing to yank a dependent of a crate we just failed on — continuing is damage). **Does:** - Execute yank plans end-to-end. - Correct halt-on-failure semantics. - Events-as-truth: every invocation writes a PackageYanked event (with real exit code) to events.jsonl, including failures. **Does not:** - Resume interrupted plan runs. First yank is destructive on the registry; re-running the plan with the same reason is idempotent (cargo yank of an already-yanked version is a no-op), so the simplest fix is to just re-run the plan, and operator discipline picks up the rest. - Add a \`fix-forward\` executor. That requires Cargo.toml version bumps which are workspace-edit territory — deserves its own PR with --dry-run / --apply / git-cleanliness guards. 3 new unit tests (8 total in \`plan_yank::tests\`): - YankPlan JSON round-trips: serialize, write, load → entries + order preserved, per-entry reason preserved. - \`load_plan_from_path\` errors cleanly on missing file. - \`load_plan_from_path\` errors cleanly on malformed JSON. Regenerated \`yank --help\` snapshot. With PR #138 (starting-crate) and this one, #98's acceptance checklist is fulfilled: - [x] Receipt fields (compromised_at/compromised_by/superseded_by) — PR #132 - [x] plan-yank --from-receipt + --starting-crate + --reason — PR #132 + #138 - [x] shipper yank executes a plan — **this PR** - [ ] shipper fix-forward --plan executes a plan — follow-on (needs Cargo.toml edit tooling) - [x] Docs distinguish yank semantics — PR #134 The fix-forward executor is the last remaining #98 piece. It's cleanly deferred because it adds workspace-edit scope; this PR keeps the execution story for yank self-contained.
…#139) Closes the remaining plan-execution gap in #98. \`shipper plan-yank --format json\` emits a YankPlan; \`shipper yank --plan <PATH>\` walks that plan and invokes \`cargo yank\` per entry. - \`YankEntry\` / \`YankPlan\` now derive \`Deserialize\` so plans round-trip via \`--format json\` → file → \`--plan\`. The \`filter\` field moves from \`&'static str\` to \`Cow<'static, str>\` so owned strings coming off disk don't require allocation in the construction path. - \`plan_yank::load_plan_from_path\` — thin helper mirroring \`load_receipt_from_path\`. - CLI: \`shipper yank --plan <PATH>\`. Mutually exclusive with \`--crate\` / \`--version\` / \`--reason\` (clap \`conflicts_with\`); those become optional in plan mode. - Handler: iterates entries in order, emits \`PackageYanked\` event per invocation, halts on first non-zero cargo exit (reverse-topo plan means every downstream entry would be failing to yank a dependent of a crate we just failed on — continuing is damage). **Does:** - Execute yank plans end-to-end. - Correct halt-on-failure semantics. - Events-as-truth: every invocation writes a PackageYanked event (with real exit code) to events.jsonl, including failures. **Does not:** - Resume interrupted plan runs. First yank is destructive on the registry; re-running the plan with the same reason is idempotent (cargo yank of an already-yanked version is a no-op), so the simplest fix is to just re-run the plan, and operator discipline picks up the rest. - Add a \`fix-forward\` executor. That requires Cargo.toml version bumps which are workspace-edit territory — deserves its own PR with --dry-run / --apply / git-cleanliness guards. 3 new unit tests (8 total in \`plan_yank::tests\`): - YankPlan JSON round-trips: serialize, write, load → entries + order preserved, per-entry reason preserved. - \`load_plan_from_path\` errors cleanly on missing file. - \`load_plan_from_path\` errors cleanly on malformed JSON. Regenerated \`yank --help\` snapshot. With PR #138 (starting-crate) and this one, #98's acceptance checklist is fulfilled: - [x] Receipt fields (compromised_at/compromised_by/superseded_by) — PR #132 - [x] plan-yank --from-receipt + --starting-crate + --reason — PR #132 + #138 - [x] shipper yank executes a plan — **this PR** - [ ] shipper fix-forward --plan executes a plan — follow-on (needs Cargo.toml edit tooling) - [x] Docs distinguish yank semantics — PR #134 The fix-forward executor is the last remaining #98 piece. It's cleanly deferred because it adds workspace-edit scope; this PR keeps the execution story for yank self-contained.
Summary
Adds the second `plan-yank` selector mode: given a broken crate name, walk the workspace's dependency graph to find every crate that transitively depends on it, and emit a containment plan scoped to that affected chain. Complements the existing `--compromised-only` receipt-filter mode.
Matches #98's issue spec verbatim:
What's new
Algorithm
Reverse BFS: for each reachable crate, scan the dep map for crates whose dep list contains it; add those to the frontier. Final set = starting crate ∪ transitive dependents. Filter receipt.packages (topo order) to that set, keep only `Published`, reverse to get dependents-first.
Example
```
$ shipper plan-yank --from-receipt .shipper/receipt.json
--starting-crate my-lib --reason "CVE-2026-0001"
yank plan (reverse topological) — registry=crates-io, plan_id=abc, filter=starting_crate
3 entries
```
(app depends on mid, mid depends on my-lib)
Tests
5 new unit tests, 10 total in `plan_yank::tests`:
Closes half of #98's remaining gap
Combined with `--compromised-only` (#132), plan-yank fully satisfies the graph + receipt-filter modes #98 asks for. Plan execution wrappers for yank/fix-forward are the remaining piece (follow-on PR).
Test plan