feat(engine): disclose at plan time whether an apply continues or discards a copy

[aparajon]

Why this matters

The engine can already predict whether an apply will resume unfinished work on the target or destroy it, but only at apply time — the moment the decision is unavoidable. By then the copy is gone. The point of predicting it at all is to tell someone before they confirm, and that requires the plan to carry the answer.

What it does

Runs the same prediction at plan time, against the target the plan already reads, and carries the result on the plan as an engine-agnostic ExistingCopy per target.

plan
  └─ for each target the plan reads
       └─ does a copy exist for a table in this batch?
            ├─ no  → PlanResult carries nothing
            └─ yes → PlanResult.ExistingCopies += { namespace, disposition, reason, tables, age }

The shape is one entry per target read, not one per plan, so an engine spanning several targets reports which one each disclosure is about. An engine planning each namespace separately produces one entry per namespace that holds a copy; where several namespaces share one connection-scoped database, that database is read once and named once. Either way the name is something an operator can go and look at, which is what a disclosure owes them.

Two safety properties worth calling out:

• A plan is a read. It describes the target and must never fail because of it. A target that cannot be read is logged and the plan carries no disclosure, leaving the plan exactly as it is without the check.
• A plan-time miss is not the last word. The same prediction runs again at apply time against the routing the apply actually took, so neither a target that was unreachable while planning nor a statement the plan expected to route elsewhere escapes evaluation before anything is destroyed.

The type carries no engine-specific fields — namespace, disposition, reason, tables, age — so a second engine that copies tables reports the same shape and every surface renders it one way.

How it moves us toward the northstar

This is the middle link of three. The engineer whose schema change is about to destroy a day of copying learns nothing from an engine's internal state — they read a plan, in a PR comment or on the CLI. The plan is the one object every surface already reads, so putting the disposition there is what lets any of them disclose it without reaching into an engine. #1048 makes the prediction; #1088 renders it.

Opened by Claude (Opus 5).

[Copilot]

Pull request overview

This PR extends the Spirit engine’s planning phase to surface whether applying the planned ALTER batch would adopt (resume) or discard (restart) an unfinished Spirit row copy already present on the target, so that decision is knowable before confirmation.

Changes:

• Adds an engine-agnostic ExistingCopies disclosure field to engine.PlanResult.
• Implements Spirit plan-time detection/prediction of existing copy disposition and attaches it to the returned plan.
• Adds integration tests covering “no copy”, “adopt”, and “discard (statement differs)” plan disclosures.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
pkg/engine/spirit/spirit.go	Populates PlanResult.ExistingCopies during Spirit planning.
pkg/engine/spirit/existing_copy.go	Adds plan-time conversion and prediction helpers to disclose existing-copy disposition.
pkg/engine/spirit/existing_copy_plan_integration_test.go	New integration tests verifying plan-time disclosures across clean/adopt/discard cases.
pkg/engine/engine.go	Introduces PlanResult.ExistingCopies and the engine-agnostic ExistingCopy type.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1087, 4232c06.

Verdict: 8 findings — 2 blocking (namespace mislabeling, dropped multi-namespace disclosure), 5 non-blocking (plan/apply ExecutionMode drift, test-coverage gaps), 1 general suggestion (proto/API wiring).

Blocking

• pkg/engine/spirit/existing_copy.go:140 — plannedExistingCopies labels its ExistingCopy.Namespace with the physical database name pulled from the DSN, not the logical namespace, contradicting PlanResult.ExistingCopies' own doc contract ("one entry per namespace that holds any," engine.go:375-378). For any subdirectory-layout MySQL deployment (e.g. payments/ + payments_audit/ against one DSN), the single disclosed ExistingCopy carries Namespace = "mydb", matching neither SchemaChange.Namespace value the rest of the plan reports — so a future renderer (feat(github): disclose an adopted or discarded copy on the plan comment #1088) can never place the disclosure under the right namespace section.
• pkg/tern/local_client.go:1641 — planMySQLNamespacesWithEngine's aggregation loop merges each namespace's Changes and LintViolations but never nsResult.ExistingCopies, silently dropping the disclosure for every multi-namespace MySQL plan. Any MySQL target with 2+ schema-file namespaces (the exact routing condition at local_client.go:1601-1610) loses the existing-copy disclosure entirely, even if a per-namespace eng.Plan() call computed a real DiscardStatementDiffers for a days-old copy about to be destroyed.

Non-blocking

• pkg/engine/spirit/existing_copy.go:108 — plannedSpiritBatch freezes the Spirit batch/table set from the plan-time ExecutionMode snapshot, but routeAlterStatements (direct.go:306-369, unchanged) deliberately re-derives the same classification live at apply time against fresh target/policy state. If a table's live definition or row count changes between plan and apply, the disclosed plan-time Disposition (e.g. Adopt) can diverge from the real apply-time outcome (e.g. Discard) — see "the one thing" below for the full analysis.
• pkg/engine/spirit/existing_copy.go:174 — The ExecutionModeDirect skip in plannedSpiritBatch means a table refused at plan time is excluded from the disclosure entirely (silence, not a stale value). If the refusal no longer holds at apply time, routeAlterStatements can route that table into Spirit for real, discarding a copy the plan said nothing about — a false negative rather than merely a stale positive.
• pkg/engine/spirit/existing_copy.go:148 — The "plan must never fail on a read" safety property this PR documents (target-unreachable branch returns nil and logs) has zero test coverage anywhere in the package, at plan time or otherwise. A future edit that changes return nil to propagate the error would break plans on any transient target read failure with no test catching it.
• pkg/engine/spirit/existing_copy.go:156 — DiscardCopyIncomplete and DiscardCheckpointExpired are proven at the Disposition() level by pre-existing apply-time tests, but never exercised through the new plan-time pipeline (Plan() → plannedExistingCopies → found.planned()); only DiscardStatementDiffers and CopyAdopt are covered there.
• pkg/engine/spirit/existing_copy.go:165 — plannedSpiritBatch's ExecutionModeDirect-skip, ExecutionModeBlocked-abort, and multi-table-join branches have no assertion anywhere that inspects its return values; a regression swapping the Direct continue for a fallthrough, or the Blocked abort for a continue, would produce byte-identical test results in every existing test.

General suggestions

• pkg/api/proto_helpers.go:183 — Neither the PlanResponse proto message, planResultToProtoChanges, planResponseFromProto, nor planContentFromStorage carry ExistingCopies, so a real value is dropped at both the proto and HTTP-API serialization boundaries. Not urgent since no consumer reads the field yet, but worth tracking now since three separate hand-enumerated sites would need updating before feat(github): disclose an adopted or discarded copy on the plan comment #1088 can ever observe a non-nil value, and none would fail to compile or fail a test if only one is updated.

The one thing that could have broken, verified

The riskiest mechanism in this PR is whether the plan-time ExecutionMode snapshot plannedSpiritBatch relies on can go stale by apply time, given that routeAlterStatements (direct.go:306-311) explicitly re-derives refusal live "so a schema or policy change between plan and apply can never smuggle a refused statement past the policy." Verified: this drift is real and reachable, but it does not break the actual apply — reportExistingCopy always recomputes the true Disposition from the live, apply-time-routed batch (execution.go:353-368), so no existing copy is ever adopted/discarded incorrectly in practice; only the plan-time disclosure shown to a reviewer can be wrong. The PR's own doc comment on plannedExistingCopies already partially caveats this for the general case ("a plan-time miss is not the last word"), but it does not cover the ExecutionModeDirect-skip case, where the plan discloses nothing at all for a table that later gets routed through Spirit for real — a silent false negative rather than a caveated stale value. Net: non-blocking for this PR (apply-time safety is intact and unchanged), but worth calling out explicitly in the doc comment before #1088 builds a PR-comment surface that operators will trust.

Verified correct

• plannedExistingCopies is called from spirit.go:587 only after the loop that resolves every change's ExecutionMode/ModeReason — no ordering bug.
• plannedSpiritBatch's all-or-nothing abort on any ExecutionModeBlocked change correctly mirrors routeAlterStatements's real apply-time all-or-nothing failure mode (direct.go:296-299).
• The new ExistingCopy struct's fields (Namespace, Disposition, Reason, Tables, Age) are all plain engine-agnostic types, satisfying AGENTS.md's "no engine-specific fields in the engine interface" rule.
• checkpointMaxAge is reused as the single source of truth for both the plan-time .planned() call and the apply-time reportExistingCopy/Disposition() call — no duplicated or drifted constant.
• existingCopy.Disposition() and findExistingCopy() are reused unchanged by the new plan-time path; the only new logic is plannedSpiritBatch computing what batch/table list to feed them.
• The 3 new integration tests correctly cover the CopyNone/CopyAdopt/DiscardStatementDiffers outcomes for a single-table plan, reusing pre-existing setupTestMySQL/SeedCopy test helpers rather than reinventing them.

This review was generated by Claude Code (claude-sonnet-5).

[aparajon]

🤖 Thanks — addressed in 360a25e0 and 27db4773.

Namespace labeling: the code was right and the doc contract was the overclaim, so I corrected the contract. Splitting one ExistingCopy per namespace would imply independent copies where there is a single Spirit batch sharing a single checkpoint, and #1088's renderer only labels the disclosure (" in \%s`") rather than grouping by namespace — so the name only has to be something an operator can go and look at, which the physical database is. ExistingCopy.Namespace` now documents exactly that, and the PR body carried the same overclaim and is fixed too.

ExecutionMode drift: agreed the Direct skip is a silent false negative rather than a caveated stale value, and the doc comment now says so alongside the unreadable-target case.

Test gaps: plannedSpiritBatch now has unit coverage for every branch (join order, Direct skip, non-ALTER skip, Blocked abort, empty), and plan-time coverage now includes the unreadable target, DiscardCheckpointExpired, and DiscardCopyIncomplete.

Multi-namespace aggregation and proto/API wiring: both already land in #1088 — planMySQLNamespacesWithEngine appends nsResult.ExistingCopies, and the value crosses the proto and HTTP boundaries via protoExistingCopies / existingCopiesFromProto. The one site I left alone is planContentFromStorage: a copy disposition is a reading of the target at plan time, so replaying a stored plan later would render a copy that may have finished, been destroyed, or aged out.

This reply was generated by Claude Code (Opus 5).