feat(vitess): treat VSchema deletions as unsafe changes

[aparajon]

Removing a vindex, a table routing entry, or a column-vindex association changes Vitess query routing the moment the VSchema is applied: the vindex stops being used for routing, lookup rows stop being maintained, and queries that depended on it can fail or scatter. Deleting a vindex is as dangerous as dropping a table, but VSchema changes previously bypassed the unsafe gate entirely.

• pkg/vschema.Deletions structurally diffs the current and desired VSchema (via the Vitess keyspace proto) and reports removed vindex definitions, table routing entries, and column-vindex associations. Lookup-family vindex removals name the backing table whose rows go stale.
• The PlanetScale engine records removals in plan change metadata, and PlanResponse.UnsafeChanges() surfaces them alongside unsafe table changes, so the existing PR comment and CLI --allow-unsafe gates apply.
• Fail closed: an unparseable VSchema fails the plan, and undecodable deletion metadata renders as an unsafe change instead of passing silently. Additions-only VSchema changes remain safe.
• CLI gate fix: schemabot apply gated on lint error severity (HasErrors()), which never fires for VSchema removals or the DROP-fallback unsafe classification. It now gates on UnsafeChanges(), matching the webhook gates. No weakening: engines already mark every lint-error change unsafe, so UnsafeChanges() is a superset.
• Sharded Issues fix: the plan comment's sharded unsafe view derives from per-shard table changes, which would omit namespace-level VSchema removals; they are now appended in both views.

vschema.json diff
    │
    ├─ additions only ──────────▶ safe, applies normally
    │
    └─ any removal ─────────────▶ ⚠️ unsafe change
         vindex / table entry /     ├─ plan: disclosed in Issues
         column-vindex assoc        └─ apply: blocked without --allow-unsafe

Warnings by removal kind

Each detected removal renders in the ⚠️ Issues section as <keyspace>/vschema.json with one of these reasons (sample identifiers):

Removal	Warning
Lookup vindex (with backing table)	lookup vindex email_lookup is removed: Vitess immediately stops maintaining its rows in backing table email_lookup, queries routed through it can fail or scatter, and the lookup data goes stale
Lookup vindex (no backing table param)	lookup vindex email_lookup is removed: Vitess immediately stops maintaining its lookup rows and queries routed through it can fail or scatter
Functional vindex (hash etc.)	vindex region_idx is removed: Vitess immediately stops using it for routing and lookups, and queries that depend on it can fail or scatter
Table routing entry	table orders is removed from the VSchema: Vitess loses its routing entry and queries against it can fail
Column-vindex association	table users no longer uses vindex email_lookup: routing for queries on its columns changes immediately and lookup rows stop being maintained
Column-vindex reassociation (same table, different columns)	table users moves vindex email_lookup from columns (email) to (phone): routing for queries on the old columns changes immediately and lookup rows for them stop being maintained
Deletion metadata undecodable (fail closed)	VSchema deletions were recorded on this plan but could not be decoded, so the VSchema change is treated as unsafe

Docs: lint-and-safety-levels.md (what unsafe means for VSchema) and namespaces.md (cross-reference). TEMPLATES.md gains a preview scenario (Vitess plan with an unsafe VSchema removal). E2E coverage exercises the full gate: plan disclosure, refusal without the flag, and completion with it.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

This PR extends SchemaBot’s safety gating for Vitess/PlanetScale by classifying VSchema structural removals (vindex definitions, table routing entries, and column-vindex associations) as unsafe changes, so they’re disclosed in plans and blocked on apply unless explicitly acknowledged with --allow-unsafe.

Changes:

• Add structural VSchema diffing (pkg/vschema.Deletions) to detect removals and fail closed on unparseable VSchema.
• Persist detected removals in PlanetScale plan metadata and surface them via PlanResponse.UnsafeChanges() alongside unsafe table DDL.
• Update CLI gating to block applies on UnsafeChanges() (not just lint error severity), plus docs + E2E coverage for the end-to-end unsafe gate.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/vschema/deletions.go	Implements structural deletion detection between current/desired VSchema.
pkg/vschema/deletions_test.go	Unit tests covering deletion detection and fail-closed parsing behavior.
pkg/engine/planetscale/plan.go	Records VSchema deletions into plan metadata during PlanetScale planning.
pkg/engine/planetscale/plan_vschema_test.go	Validates PlanetScale metadata encoding/decoding for VSchema deletions.
pkg/cmd/commands/rollback.go	Adjusts rollback output path for unsafe warnings (now always evaluated).
pkg/cmd/commands/apply.go	Gates apply on UnsafeChanges() and adjusts unsafe-warning display behavior.
pkg/apitypes/vschema.go	Adds metadata encoding/decoding and unsafe-change projection for VSchema deletions.
pkg/apitypes/vschema_test.go	Verifies unsafe-change aggregation includes VSchema deletions and fails closed on corrupt metadata.
pkg/apitypes/apitypes.go	Extends PlanResponse.UnsafeChanges() to include VSchema-derived unsafe changes.
e2e/local/vitess_test.go	E2E test proving plan disclosure + apply refusal without --allow-unsafe + success with it.
docs/namespaces.md	Documents that VSchema removals are unsafe and require --allow-unsafe.
docs/lint-and-safety-levels.md	Defines what “unsafe” means for VSchema changes and how removals are detected.

---

💡 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/1075, 24f7071.

Verdict: 2 findings — 1 blocking (column-vindex reassociation evades detection), 1 general suggestion (vindex type/params changes not flagged).

Blocking

1. pkg/vschema/deletions.go:74 — desiredVindexes := columnVindexNames(desiredTable) builds its "still referenced" set keyed only by vindex name (names[cv.GetName()] = struct{}{}), never by column. If table users moves vindex v1 from column email to phone (same table, same vindex name), Deletions() returns an empty slice — the routing-changing removal of email's association is never disclosed and bypasses --allow-unsafe entirely, even though the operationally-identical case of moving v1 to a different table is correctly caught. Verified empirically: Deletions() on this exact current/desired pair returns [].

General suggestions

1. pkg/vschema/deletions.go:53 — for _, name := range sortedKeys(currentKs.Vindexes) { if _, ok := desiredKs.Vindexes[name]; !ok { only checks vindex-name presence, never comparing Type/Params/Owner. A same-name vindex whose type changes (e.g. hash→xxhash, re-sharding every row) or whose lookup params.table is repointed to a different backing table produces zero deletions and no unsafe-change disclosure. This is arguably out of the type's documented scope ("structural removals"), but worth a follow-up since both changes silently alter live routing/lookup behavior with the same blast radius as a removal.

The one thing that could have broken, verified

The riskiest question: can the new structural-removal detection in deletions.go ever silently miss a real removal (a false negative on the safety gate)? Answer: yes, in one concrete case — reassigning a named column-vindex to a different column within the same table (the Blocking finding above), reproduced by direct execution returning deletions: []. Outside that specific case, whole-vindex removal, whole-table removal, and column-vindex removal against a genuinely-vanished vindex are all correctly detected, and vschema.Changed() (pre-existing, untouched) shares the same proto round-trip so the if vschemaChanged {...} gate in plan.go never skips calling Deletions() when a real removal is present.

Verified correct

• vschema.Changed() and the new Deletions()/parseKeyspace both round-trip through the same vschemapb.Keyspace proto type, so the vschemaChanged gate in plan.go never skips a genuine removal that Deletions() would flag.
• apply.go's dropped HasErrors() guard on the allow-unsafe warning branch is safe: WriteUnsafeWarningAllowed already no-ops on an empty change list, so --allow-unsafe on a clean plan prints nothing extra.
• PlanResponse.UnsafeChanges() is correctly a superset of the old table-only view (table changes + sc.VSchemaUnsafeChanges() per namespace change), and webhook/plan.go's sharded-empty fallback avoids double-counting VSchema entries by manually looping sc.TableChanges instead of calling the now-VSchema-inclusive UnsafeChanges().
• Omitting vschema.json from a namespace's desired files skips the VSchema-diff block entirely (pre-existing gate in branch.go), so no live routing change happens and the new deletion detection correctly never runs — distinct from an explicit {} VSchema, which is diffed and correctly flags every vindex/table as removed.
• Whole-table removal continues before the column-vindex-association loop, so a removed table never also gets redundant column-vindex-removal entries; a vindex removal and a column-vindex-association removal for a still-existing table can legitimately both fire for the same underlying vindex (by design, not double-counting).
• New PR-facing removal-reason strings are fixed, code-authored text interpolating only vindex/table names parsed from the VSchema documents — no raw err.Error() leakage; new comments and the e2e test's doc comment describe scenario/outcome, not bug/PR references.

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

[aparajon]

🤖 Both findings addressed:

1. (Blocking) Fixed in aa4947b — column-vindex associations are now identified by vindex name plus the ordered columns they cover, so a same-table reassociation (and a multi-column reorder) is disclosed as a column_vindex removal with a reason naming the old and new columns. Your repro pair now returns one deletion; tests pin the reassociation, reorder, and legacy column/columns equivalence cases.
2. (Suggestion) Agreed it's a real gap, deferring as a follow-up per your framing — same-name vindex type/params mutation detection is a different concept than structural removals and deserves its own PR.

Reply by Claude (Fable 5).