Skip to content

fix(migration): expose prepared replacement targets - #729

Merged
redwhisk merged 2 commits into
masterfrom
fix/migration-confirmation-targets
Oct 3, 2026
Merged

redwhisk merged 2 commits into
masterfrom
fix/migration-confirmation-targets

Conversation

@AprilNEA

@AprilNEA AprilNEA commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

PrepareMigration omitted replacement names from executable responses. A client could therefore accept a different replacement plan after preview even when resource counts, the replacement flag, and warnings were unchanged.

Return a typed replacement summary from the same plan saved under plan_id, including an explicitly present empty summary. Keep the full plan and container environments restricted to dry runs. This supports the confirmation fix in arcboxlabs/arcbox-desktop#368.

Validation: make refresh-connect-descriptor, cargo build -p arcbox-protocol -p arcbox-connect, cargo fmt --check, buf breaking against origin/master, and git diff --check passed. Scoped Clippy passed with -D warnings. cargo test -p arcbox-core migration:: passed all 14 tests, including saved-plan consistency, dry-run consistency, and empty-summary wire presence. CI for the updated head is pending.


Note

Medium Risk
Changes the migration prepare contract so clients must rely on the new replacement summary before RunMigration; additive on the wire but affects confirmation UX for destructive replacements.

Overview
PrepareMigration now always returns a typed replacements summary (container/volume/network/image tag names) alongside the existing replacements_required flag, including an explicitly present empty summary on runnable prepares. The summary comes from the same plan stored under plan_id, so clients can verify the exact targets the user confirmed without receiving the full plan (still dry-run only).

Protocol and generated types gain PrepareMigrationResponse.replacements; arcbox-core maps plan.replacements.to_wire() into the response. Tests cover dry-run parity with the embedded plan, empty-summary wire presence after encode/decode, and a shim-driven prepare that reports volume replacements matching the saved plan.

Reviewed by Cursor Bugbot for commit d5e6f9a. Bugbot is set up for automated code reviews on this repo. Configure here.

@greptile-apps

greptile-apps Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Critical risk] Exposes prepared migration replacement targets in public API contract.

The PR appears safe to merge; no actionable issue was established in its changes.

Summary

The PR adds a replacement-target summary to PrepareMigrationResponse, populated from the prepared plan while keeping the full plan limited to dry runs. It also updates the protocol types, Connect descriptor, documentation, and tests.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[PrepareMigration] --> B[Build migration plan]
  B --> C[Return replacement summary]
  B --> D[Save plan under plan_id]
  B --> E[Return full plan only for dry run]
Loading

Reviews (2) · Last reviewed commit: "chore(protocol): merge master and refres..."

pullfrog[bot]
pullfrog Bot previously approved these changes Oct 3, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Full diff reviewed (6 files, 1 commit) against master (af91385).

  • Daemon (app/arcbox-core/src/migration/mod.rs) — prepare_migration now returns replacements, the typed summary projected from the same plan that is stored under plan_id, always present on the wire (including explicitly present when empty); the full plan remains dry-run-only.
  • Protocol (rpc/arcbox-protocol, rpc/arcbox-connect) — adds PrepareMigrationResponse.replacements (MigrationReplacementSummary, field 12) to api.proto, regenerates the prost twin and the Connect descriptor, and documents the client contract in the README.

Verified during review:

  • descriptor/protos.sha256 matches an independent re-hash of the proto sources (per refresh.sh's algorithm), so the committed Connect descriptor is in sync with api.proto.
  • The field is purely additive (tag 12): the Proto Breaking-Change Check gate is green, an old daemon simply omits the summary, and the README/proto comments define the absence semantics.
  • The new REPLACED_VOLUME_SHIM test genuinely exercises the planner path — the shim's replies match DockerCliRunner's exact commands (volume ls - -q, volume inspect <name>), and build_replacements derives volumes: ["pgdata"] from the target-side name collision. The assertions pin the names, replacements_required, plan withholding, and equality with the stored plan, so each would fail under the regression this PR fixes.
  • The encode/decode roundtrip pins the empty-summary wire-presence contract for a runnable prepare with nothing to replace.
  • All in-repo consumers checked (arcbox-api passthrough, arcbox-cli display-only) need no changes for the new field.

Pullfrog  | View workflow run | Using glm-5.3-flash | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Delta since the prior pullfrog review (d5e6f9a): the branch merged master (b5411f2, ~30 commits of machine clone/export/import work landed separately) and refreshed the Connect descriptor. The PR's own diff against master is unchanged from the previously reviewed set — same 6 files, same hunks.

  • Merge resolution — the merge's combined diff touches exactly the three expected files (arcbox_connect.protoset, protos.sh256→protos.sha256, rpc/arcbox-protocol/src/generated/arcbox.v1.rs): the generated file kept both master's machine types and this PR's replacements field 12, with the MigrationReplacementSummary reference resolving to the pre-existing type, and it regenerates byte-identically from the current proto sources (working tree stayed clean after the build).
  • Descriptor refresh — an independent re-hash of the proto sources following refresh.sh's algorithm matches the committed protos.sha256 (ffae49a0…), and the refreshed protoset contains the replacements field plus the merged-in machine.proto definitions.
  • Build — cargo build -p arcbox-protocol passes on the merged state.
  • Proto gates on the new head are green (Proto Breaking-Change Check, Control-plane proto compatibility, SDK TypeScript/Python, crates.io packaging check); Build, Lint, and Test and E2E tests were still pending at review time — worth a glance before merging, but the merged content is a superset of master's green state plus verified regenerated artifacts.

Pullfrog  | View workflow run | Using glm-5.3-flash | 𝕏

@redwhisk
redwhisk merged commit fc1cbca into master Oct 3, 2026
16 checks passed
@redwhisk
redwhisk deleted the fix/migration-confirmation-targets branch October 3, 2026 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants