From f5c24ed907ff15222b3c7262fd23752b07aa10c9 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Wed, 26 Aug 2026 12:21:04 +0800 Subject: [PATCH 1/2] fix(github): number unsafe findings and count the same unit on every surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The unsafe-changes headings advertise a count, but the list below was unordered and, on the CLI, counted changes while the PR comment counted findings — the same apply could say 3 in the terminal and 4 on the PR. Both surfaces now render one numbered line per finding (the table name repeats instead of nesting a sub-list), and every heading counts findings, so the count always equals the number of lines below it and a finding can be referenced by its number. Co-Authored-By: Claude Fable 5 --- TEMPLATES.md | 40 +++++++++--------- pkg/cmd/internal/templates/plan.go | 41 +++++++++++++------ .../internal/templates/plan_unsafe_test.go | 9 ++-- pkg/webhook/plan_test.go | 28 ++++++------- pkg/webhook/templates/apply_commands.go | 3 +- pkg/webhook/templates/plan.go | 33 +++++++-------- 6 files changed, 82 insertions(+), 72 deletions(-) diff --git a/TEMPLATES.md b/TEMPLATES.md index 53257767f..69052740a 100644 --- a/TEMPLATES.md +++ b/TEMPLATES.md @@ -255,8 +255,8 @@ DROP TABLE `reconcile_state`; A plan diffs this PR's schema files against the live database, so what another PR applied before merging reads here as something to remove. If that is not what you intend, merge that PR, or bring this PR's schema files up to date with it, then re-plan. ⚠️ **Issues**: 2 unsafe changes detected -- `orders`: DROP COLUMN discards the column's data -- `reconcile_state`: DROP TABLE removes all data +1. `orders`: DROP COLUMN discards the column's data +2. `reconcile_state`: DROP TABLE removes all data **Destructive drop guidance:** @@ -812,8 +812,8 @@ schemabot apply -e staging ``` ⚠️ **Issues**: 2 unsafe changes detected -- `commerce_sharded/vschema.json`: lookup vindex `customers_email_lookup` is removed: Vitess immediately stops maintaining its rows in backing table `customers_email_lookup`, queries routed through it can fail or scatter, and the lookup data goes stale -- `commerce_sharded/vschema.json`: table `customers` no longer uses vindex `customers_email_lookup`: routing for queries on its columns changes immediately and lookup rows stop being maintained +1. `commerce_sharded/vschema.json`: lookup vindex `customers_email_lookup` is removed: Vitess immediately stops maintaining its rows in backing table `customers_email_lookup`, queries routed through it can fail or scatter, and the lookup data goes stale +2. `commerce_sharded/vschema.json`: table `customers` no longer uses vindex `customers_email_lookup`: routing for queries on its columns changes immediately and lookup rows stop being maintained 📋 **Plan**: **1** vschema update @@ -1404,7 +1404,7 @@ ALTER TABLE `customers` DROP COLUMN `nickname`; --- **⛔ Apply rejected**: 1 unsafe change detected -- `customers`: Unsafe operation detected: `` DROP COLUMN `nickname` `` +1. `customers`: Unsafe operation detected: `` DROP COLUMN `nickname` `` **Destructive drop guidance:** @@ -1437,7 +1437,7 @@ ALTER TABLE `customers` DROP INDEX `idx_customers_email`; --- **⛔ Apply rejected**: 1 unsafe change detected -- `customers`: Unsafe operation detected: `` DROP INDEX `idx_customers_email` `` +1. `customers`: Unsafe operation detected: `` DROP INDEX `idx_customers_email` `` **Destructive drop guidance:** @@ -1472,10 +1472,9 @@ ALTER TABLE `users` RENAME COLUMN `email` TO `email_address`; --- **⛔ Apply rejected**: 3 unsafe changes detected -- `orders`: - - Primary key column `id` has type `int` - - Column `created_at` uses `TIMESTAMP` which overflows on 2038-01-19. Consider using `DATETIME` instead. -- `users`: Column rename detected in table `users`: `email` to `email_address`. Renaming a column cannot be done atomically across application pods, and ORMs that generate column names at compile time (e.g. jOOQ) will break until code is recompiled +1. `orders`: Primary key column `id` has type `int` +2. `orders`: Column `created_at` uses `TIMESTAMP` which overflows on 2038-01-19. Consider using `DATETIME` instead. +3. `users`: Column rename detected in table `users`: `email` to `email_address`. Renaming a column cannot be done atomically across application pods, and ORMs that generate column names at compile time (e.g. jOOQ) will break until code is recompiled **🚨 To proceed with these destructive changes, re-run with `--allow-unsafe`:** ``` @@ -7657,7 +7656,7 @@ ALTER TABLE `mutes` ``` ⚠️ **Issues**: 1 unsafe change detected -- `mutes` (shard `40-80`): DROP COLUMN removes data and is irreversible +1. `mutes` (shard `40-80`): DROP COLUMN removes data and is irreversible **Destructive drop guidance:** @@ -7906,8 +7905,8 @@ schemabot apply -e production | --- | --- | | `-40` | ✅ completed | | `40-80` | ✅ completed | -| `80-c0` | ⛔ cancelled | -| `c0-` | ⛔ cancelled | +| `80-c0` | 🚫 cancelled | +| `c0-` | 🚫 cancelled | --- @@ -8508,12 +8507,11 @@ Lint violations: Non-blocking warnings during plan/apply Unsafe blocked: Destructive changes require --allow-unsafe -⛔ Apply blocked: 3 unsafe change(s) detected - • users: DROP COLUMN email - • orders: DROP TABLE - • products: - - MODIFY COLUMN price_cents: INT → SMALLINT (potential data loss) - - DROP INDEX idx_category +⛔ Apply blocked: 4 unsafe change(s) detected + 1. users: DROP COLUMN email + 2. orders: DROP TABLE + 3. products: MODIFY COLUMN price_cents: INT → SMALLINT (potential data loss) + 4. products: DROP INDEX idx_category 🚨 To proceed with these destructive changes, re-run with --allow-unsafe: @@ -8534,8 +8532,8 @@ Unsafe allowed: Proceeding with --allow-unsafe flag 🚨 Unsafe Changes (--allow-unsafe enabled) The following unsafe changes will be applied: - • users: DROP COLUMN email - • orders: DROP TABLE + 1. users: DROP COLUMN email + 2. orders: DROP TABLE ``` diff --git a/pkg/cmd/internal/templates/plan.go b/pkg/cmd/internal/templates/plan.go index cdbe9d164..66eca9382 100644 --- a/pkg/cmd/internal/templates/plan.go +++ b/pkg/cmd/internal/templates/plan.go @@ -498,7 +498,7 @@ func WriteUnsafeChangesWarning(changes []UnsafeChange) { // names the blocked apply, not the unsafeness of the changes. func WriteUnsafeChangesBlocked(changes []UnsafeChange, database, environment, schemaDir string) { if len(changes) > 0 { - fmt.Printf(glyph.Refused+" Apply blocked: %d unsafe change(s) detected\n", len(changes)) + fmt.Printf(glyph.Refused+" Apply blocked: %d unsafe change(s) detected\n", countUnsafeFindings(changes)) writeUnsafeChangesList(changes) fmt.Println() } @@ -521,20 +521,37 @@ func WriteUnsafeWarningAllowed(changes []UnsafeChange) { fmt.Println() } -// writeUnsafeChangesList writes the list of unsafe changes, splitting multi-reason entries. +// writeUnsafeChangesList writes the unsafe changes one numbered line per +// finding, the same list shape as the PR plan comment, so a heading's count +// always equals the number of lines below it and a finding can be referenced +// by its number. func writeUnsafeChangesList(changes []UnsafeChange) { + n := 0 for _, c := range changes { reasons := ui.LintReasons(c.Reason) - switch len(reasons) { - case 0: - fmt.Printf(" • %s: %s\n", c.Table, c.ChangeType) - case 1: - fmt.Printf(" • %s: %s\n", c.Table, reasons[0]) - default: - fmt.Printf(" • %s:\n", c.Table) - for _, r := range reasons { - fmt.Printf(" - %s\n", r) - } + if len(reasons) == 0 { + n++ + fmt.Printf(" %d. %s: %s\n", n, c.Table, c.ChangeType) + continue + } + for _, r := range reasons { + n++ + fmt.Printf(" %d. %s: %s\n", n, c.Table, r) + } + } +} + +// countUnsafeFindings sums the individual findings across changes so the +// apply-blocked heading counts exactly what the list below shows; a change +// with no parseable reason still counts once. +func countUnsafeFindings(changes []UnsafeChange) int { + n := 0 + for _, c := range changes { + if reasons := ui.LintReasons(c.Reason); len(reasons) > 0 { + n += len(reasons) + } else { + n++ } } + return n } diff --git a/pkg/cmd/internal/templates/plan_unsafe_test.go b/pkg/cmd/internal/templates/plan_unsafe_test.go index ac7d90971..60981d828 100644 --- a/pkg/cmd/internal/templates/plan_unsafe_test.go +++ b/pkg/cmd/internal/templates/plan_unsafe_test.go @@ -23,11 +23,10 @@ func TestWriteUnsafeChangesWarning_SplitsJoinedReasons(t *testing.T) { }) }) - assert.Contains(t, out, " • orders:\n") - assert.Contains(t, out, " - DROP COLUMN removes data\n") - assert.Contains(t, out, " - Column \"created_at\" uses \"TIMESTAMP\" which overflows on 2038-01-19.\n") - assert.Contains(t, out, " • users: DROP TABLE removes all data\n") - assert.Contains(t, out, " • audit_log: drop\n") + assert.Contains(t, out, " 1. orders: DROP COLUMN removes data\n") + assert.Contains(t, out, " 2. orders: Column \"created_at\" uses \"TIMESTAMP\" which overflows on 2038-01-19.\n") + assert.Contains(t, out, " 3. users: DROP TABLE removes all data\n") + assert.Contains(t, out, " 4. audit_log: drop\n") assert.NotContains(t, out, "data; ") } diff --git a/pkg/webhook/plan_test.go b/pkg/webhook/plan_test.go index 381c19621..8bdeddb02 100644 --- a/pkg/webhook/plan_test.go +++ b/pkg/webhook/plan_test.go @@ -344,8 +344,8 @@ func TestRenderPlanComment_UnsafeWarningSummaryCountsChanges(t *testing.T) { rendered := templates.RenderPlanComment(data) assert.Contains(t, rendered, "⚠️ **Issues**: 2 unsafe changes detected") - assert.Contains(t, rendered, "- `orders`: DROP INDEX without making invisible first") - assert.Contains(t, rendered, "- `customers`: DROP COLUMN is destructive") + assert.Contains(t, rendered, "1. `orders`: DROP INDEX without making invisible first") + assert.Contains(t, rendered, "2. `customers`: DROP COLUMN is destructive") } func TestRenderPlanComment_TenantScopedHints(t *testing.T) { @@ -917,10 +917,9 @@ func TestRenderUnsafeChangesBlocked_SplitsJoinedReasonsIntoBullets(t *testing.T) rendered := templates.RenderUnsafeChangesBlocked(data) assert.Contains(t, rendered, "**⛔ Apply rejected**: 3 unsafe changes detected") - assert.Contains(t, rendered, "- `uploads`:\n") - assert.Contains(t, rendered, " - Column `expires_at` uses `TIMESTAMP` which overflows on 2038-01-19. Consider using `DATETIME` instead.\n") - assert.Contains(t, rendered, " - Column `created_at` uses `TIMESTAMP` which overflows on 2038-01-19. Consider using `DATETIME` instead.\n") - assert.Contains(t, rendered, " - Primary key column `uid` has type `varchar`\n") + assert.Contains(t, rendered, "1. `uploads`: Column `expires_at` uses `TIMESTAMP` which overflows on 2038-01-19. Consider using `DATETIME` instead.\n") + assert.Contains(t, rendered, "2. `uploads`: Column `created_at` uses `TIMESTAMP` which overflows on 2038-01-19. Consider using `DATETIME` instead.\n") + assert.Contains(t, rendered, "3. `uploads`: Primary key column `uid` has type `varchar`\n") assert.NotContains(t, rendered, "instead.; ") } @@ -949,10 +948,9 @@ func TestRenderPlanComment_SplitsJoinedUnsafeReasonsIntoBullets(t *testing.T) { rendered := templates.RenderPlanComment(data) assert.Contains(t, rendered, "3 unsafe changes detected") - assert.Contains(t, rendered, "- `orders`:\n") - assert.Contains(t, rendered, " - DROP COLUMN removes data\n") - assert.Contains(t, rendered, " - Column `created_at` uses `TIMESTAMP` which overflows on 2038-01-19. Consider using `DATETIME` instead.\n") - assert.Contains(t, rendered, "- `users`: DROP TABLE removes all data\n") + assert.Contains(t, rendered, "1. `orders`: DROP COLUMN removes data\n") + assert.Contains(t, rendered, "2. `orders`: Column `created_at` uses `TIMESTAMP` which overflows on 2038-01-19. Consider using `DATETIME` instead.\n") + assert.Contains(t, rendered, "3. `users`: DROP TABLE removes all data\n") assert.NotContains(t, rendered, "data; ") } @@ -978,9 +976,9 @@ func TestRenderUnsafeChangesBlocked_EmptyReasonListsBareTableAndCountsOnce(t *te rendered := templates.RenderUnsafeChangesBlocked(data) assert.Contains(t, rendered, "**⛔ Apply rejected**: 2 unsafe changes detected") - assert.Contains(t, rendered, "- `users`\n") - assert.NotContains(t, rendered, "- `users`:") - assert.Contains(t, rendered, "- `orders`: DROP TABLE removes all data\n") + assert.Contains(t, rendered, "1. `users`\n") + assert.NotContains(t, rendered, "1. `users`:") + assert.Contains(t, rendered, "2. `orders`: DROP TABLE removes all data\n") } // The plan comment's unsafe-issues section handles a reasonless change the @@ -1001,8 +999,8 @@ func TestRenderPlanComment_EmptyUnsafeReasonListsBareTableAndCountsOnce(t *testi rendered := templates.RenderPlanComment(data) assert.Contains(t, rendered, "1 unsafe change detected") - assert.Contains(t, rendered, "- `users`\n") - assert.NotContains(t, rendered, "- `users`:") + assert.Contains(t, rendered, "1. `users`\n") + assert.NotContains(t, rendered, "1. `users`:") } func TestRenderUnsafeChangesBlocked_CustomDatabaseTypeHeader(t *testing.T) { diff --git a/pkg/webhook/templates/apply_commands.go b/pkg/webhook/templates/apply_commands.go index 60355081f..eb5f9ae27 100644 --- a/pkg/webhook/templates/apply_commands.go +++ b/pkg/webhook/templates/apply_commands.go @@ -176,8 +176,9 @@ func RenderUnsafeChangesBlocked(data PlanCommentData) string { sb.WriteString("---\n\n") unsafeCount := countUnsafeFindings(data.UnsafeChanges) fmt.Fprintf(&sb, "**"+glyph.Refused+" Apply rejected**: %d unsafe %s detected\n", unsafeCount, pluralize("change", unsafeCount)) + item := 0 for _, c := range data.UnsafeChanges { - writeUnsafeChangeItem(&sb, "`"+c.Table+"`", c.Reason) + writeUnsafeChangeItem(&sb, &item, "`"+c.Table+"`", c.Reason) } sb.WriteString("\n") writeUnsafeDropGuidance(&sb, data.UnsafeChanges, data.IsMySQL) diff --git a/pkg/webhook/templates/plan.go b/pkg/webhook/templates/plan.go index 09ecb5747..8c2b2c6cb 100644 --- a/pkg/webhook/templates/plan.go +++ b/pkg/webhook/templates/plan.go @@ -1060,35 +1060,32 @@ func writeDirectChanges(sb *strings.Builder, changes []DirectChangeData, databas func writeUnsafeWarning(sb *strings.Builder, changes []UnsafeChangeData, isMySQL bool) { n := countUnsafeFindings(changes) fmt.Fprintf(sb, glyph.Attention+" **Issues**: %d unsafe %s detected\n", n, pluralize("change", n)) + item := 0 for _, c := range changes { table := "`" + c.Table + "`" if len(c.Shards) > 0 { table = fmt.Sprintf("%s (%s)", table, planShardList(c.Shards, c.TotalShards)) } - writeUnsafeChangeItem(sb, table, c.Reason) + writeUnsafeChangeItem(sb, &item, table, c.Reason) } sb.WriteString("\n") writeUnsafeDropGuidance(sb, changes, isMySQL) } -// writeUnsafeChangeItem writes one table's unsafe findings as a list item: -// a single "- table: reason" line for one finding, or a nested list when the -// engine joined several, so each finding reads on its own line. -func writeUnsafeChangeItem(sb *strings.Builder, table, reason string) { +// writeUnsafeChangeItem writes one table's unsafe findings, one numbered line +// per finding, so the rendered list is exactly as long as the heading's count +// and operators can reference a finding by its number. n carries the running +// number across tables; a change with no parseable reason still gets a line. +func writeUnsafeChangeItem(sb *strings.Builder, n *int, table, reason string) { reasons := ui.LintReasons(reason) - for i, r := range reasons { - reasons[i] = ui.CodeQuoteIdentifiers(r) - } - switch len(reasons) { - case 0: - fmt.Fprintf(sb, "- %s\n", table) - case 1: - fmt.Fprintf(sb, "- %s: %s\n", table, reasons[0]) - default: - fmt.Fprintf(sb, "- %s:\n", table) - for _, r := range reasons { - fmt.Fprintf(sb, " - %s\n", r) - } + if len(reasons) == 0 { + *n++ + fmt.Fprintf(sb, "%d. %s\n", *n, table) + return + } + for _, r := range reasons { + *n++ + fmt.Fprintf(sb, "%d. %s: %s\n", *n, table, ui.CodeQuoteIdentifiers(r)) } } From a5162716deaa2013e502ac110350946e775a960f Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Mon, 31 Aug 2026 17:03:03 -0400 Subject: [PATCH 2/2] fix(github): explain a reasonless unsafe finding with its change type The PR comment's zero-reason fallback rendered a bare table name while the CLI rendered the engine's change type; thread ChangeType through the webhook unsafe-change data so both surfaces explain the finding. Cross-reference the two countUnsafeFindings helpers so they don't drift. Co-Authored-By: Claude Fable 5 --- pkg/cmd/internal/templates/plan.go | 4 +++- pkg/webhook/plan.go | 12 +++++++----- pkg/webhook/plan_test.go | 16 ++++++++-------- pkg/webhook/templates/apply_commands.go | 2 +- pkg/webhook/templates/plan.go | 21 ++++++++++++++++----- 5 files changed, 35 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/internal/templates/plan.go b/pkg/cmd/internal/templates/plan.go index 66eca9382..104ccb058 100644 --- a/pkg/cmd/internal/templates/plan.go +++ b/pkg/cmd/internal/templates/plan.go @@ -543,7 +543,9 @@ func writeUnsafeChangesList(changes []UnsafeChange) { // countUnsafeFindings sums the individual findings across changes so the // apply-blocked heading counts exactly what the list below shows; a change -// with no parseable reason still counts once. +// with no parseable reason still counts once. The PR comment's +// countUnsafeFindings in pkg/webhook/templates mirrors this; the two must +// agree so the CLI and PR comment report the same count for the same plan. func countUnsafeFindings(changes []UnsafeChange) int { n := 0 for _, c := range changes { diff --git a/pkg/webhook/plan.go b/pkg/webhook/plan.go index 779ba85ad..3a1559fa4 100644 --- a/pkg/webhook/plan.go +++ b/pkg/webhook/plan.go @@ -666,7 +666,7 @@ func shardedUnsafeChanges(shards []*apitypes.ShardPlanResponse) []templates.Unsa k := key{table: unsafeChange.Table, reason: unsafeChange.Reason} uc := byKey[k] if uc == nil { - uc = &templates.UnsafeChangeData{Table: unsafeChange.Table, Reason: unsafeChange.Reason, TotalShards: total} + uc = &templates.UnsafeChangeData{Table: unsafeChange.Table, Reason: unsafeChange.Reason, ChangeType: unsafeChange.ChangeType, TotalShards: total} byKey[k] = uc order = append(order, k) } @@ -923,8 +923,9 @@ func buildPlanCommentData(schema *ghclient.SchemaRequestResult, planResp *apityp for _, t := range sc.TableChanges { if uc, ok := t.UnsafeChange(); ok { unsafe = append(unsafe, templates.UnsafeChangeData{ - Table: uc.Table, - Reason: uc.Reason, + Table: uc.Table, + Reason: uc.Reason, + ChangeType: uc.ChangeType, }) } } @@ -936,8 +937,9 @@ func buildPlanCommentData(schema *ghclient.SchemaRequestResult, planResp *apityp } for _, uc := range sc.VSchemaUnsafeChanges() { unsafe = append(unsafe, templates.UnsafeChangeData{ - Table: uc.Table, - Reason: uc.Reason, + Table: uc.Table, + Reason: uc.Reason, + ChangeType: uc.ChangeType, }) } } diff --git a/pkg/webhook/plan_test.go b/pkg/webhook/plan_test.go index 8bdeddb02..1b12f6da4 100644 --- a/pkg/webhook/plan_test.go +++ b/pkg/webhook/plan_test.go @@ -955,9 +955,9 @@ func TestRenderPlanComment_SplitsJoinedUnsafeReasonsIntoBullets(t *testing.T) { } // An engine can report an unsafe change without a parseable reason. The -// blocking comment still lists the table — as a bare bullet, no dangling -// colon — and the header counts it as one finding. -func TestRenderUnsafeChangesBlocked_EmptyReasonListsBareTableAndCountsOnce(t *testing.T) { +// blocking comment still lists the table — with the engine's change type as +// the explanation, matching the CLI — and the header counts it as one finding. +func TestRenderUnsafeChangesBlocked_EmptyReasonListsChangeTypeAndCountsOnce(t *testing.T) { data := templates.PlanCommentData{ Database: "testdb", Environment: "staging", @@ -968,7 +968,7 @@ func TestRenderUnsafeChangesBlocked_EmptyReasonListsBareTableAndCountsOnce(t *te }}, HasUnsafeChanges: true, UnsafeChanges: []templates.UnsafeChangeData{ - {Table: "users", Reason: ""}, + {Table: "users", Reason: "", ChangeType: "drop"}, {Table: "orders", Reason: "DROP TABLE removes all data"}, }, } @@ -976,13 +976,13 @@ func TestRenderUnsafeChangesBlocked_EmptyReasonListsBareTableAndCountsOnce(t *te rendered := templates.RenderUnsafeChangesBlocked(data) assert.Contains(t, rendered, "**⛔ Apply rejected**: 2 unsafe changes detected") - assert.Contains(t, rendered, "1. `users`\n") - assert.NotContains(t, rendered, "1. `users`:") + assert.Contains(t, rendered, "1. `users`: drop\n") assert.Contains(t, rendered, "2. `orders`: DROP TABLE removes all data\n") } -// The plan comment's unsafe-issues section handles a reasonless change the -// same way: a bare table bullet that still counts once in the header. +// A change with neither a reason nor a change type still gets a line in the +// plan comment's unsafe-issues section — a bare table entry, no dangling +// colon — and still counts once in the header. func TestRenderPlanComment_EmptyUnsafeReasonListsBareTableAndCountsOnce(t *testing.T) { data := templates.PlanCommentData{ Database: "testdb", diff --git a/pkg/webhook/templates/apply_commands.go b/pkg/webhook/templates/apply_commands.go index eb5f9ae27..d360cbdaf 100644 --- a/pkg/webhook/templates/apply_commands.go +++ b/pkg/webhook/templates/apply_commands.go @@ -178,7 +178,7 @@ func RenderUnsafeChangesBlocked(data PlanCommentData) string { fmt.Fprintf(&sb, "**"+glyph.Refused+" Apply rejected**: %d unsafe %s detected\n", unsafeCount, pluralize("change", unsafeCount)) item := 0 for _, c := range data.UnsafeChanges { - writeUnsafeChangeItem(&sb, &item, "`"+c.Table+"`", c.Reason) + writeUnsafeChangeItem(&sb, &item, "`"+c.Table+"`", c.Reason, c.ChangeType) } sb.WriteString("\n") writeUnsafeDropGuidance(&sb, data.UnsafeChanges, data.IsMySQL) diff --git a/pkg/webhook/templates/plan.go b/pkg/webhook/templates/plan.go index 8c2b2c6cb..73fa80308 100644 --- a/pkg/webhook/templates/plan.go +++ b/pkg/webhook/templates/plan.go @@ -27,6 +27,9 @@ type LintViolationData struct { type UnsafeChangeData struct { Table string Reason string + // ChangeType is the engine's change type (e.g. "drop"), rendered when the + // change carries no parseable reason so the finding still explains itself. + ChangeType string // Shards names the shards this unsafe change applies to, for a sharded plan // where only some shards carry it. Empty for a non-sharded change (applies to // the whole table). @@ -1066,7 +1069,7 @@ func writeUnsafeWarning(sb *strings.Builder, changes []UnsafeChangeData, isMySQL if len(c.Shards) > 0 { table = fmt.Sprintf("%s (%s)", table, planShardList(c.Shards, c.TotalShards)) } - writeUnsafeChangeItem(sb, &item, table, c.Reason) + writeUnsafeChangeItem(sb, &item, table, c.Reason, c.ChangeType) } sb.WriteString("\n") writeUnsafeDropGuidance(sb, changes, isMySQL) @@ -1075,12 +1078,18 @@ func writeUnsafeWarning(sb *strings.Builder, changes []UnsafeChangeData, isMySQL // writeUnsafeChangeItem writes one table's unsafe findings, one numbered line // per finding, so the rendered list is exactly as long as the heading's count // and operators can reference a finding by its number. n carries the running -// number across tables; a change with no parseable reason still gets a line. -func writeUnsafeChangeItem(sb *strings.Builder, n *int, table, reason string) { +// number across tables; a change with no parseable reason still gets a line, +// carrying the engine's change type when one is known so the finding explains +// itself. +func writeUnsafeChangeItem(sb *strings.Builder, n *int, table, reason, changeType string) { reasons := ui.LintReasons(reason) if len(reasons) == 0 { *n++ - fmt.Fprintf(sb, "%d. %s\n", *n, table) + if changeType != "" { + fmt.Fprintf(sb, "%d. %s: %s\n", *n, table, changeType) + } else { + fmt.Fprintf(sb, "%d. %s\n", *n, table) + } return } for _, r := range reasons { @@ -1092,7 +1101,9 @@ func writeUnsafeChangeItem(sb *strings.Builder, n *int, table, reason string) { // countUnsafeFindings sums the individual lint findings across changes, so // headers count what the list below actually shows: a table whose reason // carries several joined violations contributes each of them. A change with -// no parseable reason still counts once. +// no parseable reason still counts once. The CLI's countUnsafeFindings in +// pkg/cmd/internal/templates mirrors this; the two must agree so the PR +// comment and CLI report the same count for the same plan. func countUnsafeFindings(changes []UnsafeChangeData) int { n := 0 for _, c := range changes {