diff --git a/TEMPLATES.md b/TEMPLATES.md index 3f52d2d99..f5e7c5c65 100644 --- a/TEMPLATES.md +++ b/TEMPLATES.md @@ -42,7 +42,7 @@ CREATE TABLE `orders` ( ALTER TABLE `products` ADD INDEX `idx_category_price`(`category`, `price`); ``` -๐Ÿ’ก **Lint Warnings**: **2** advisory findings +๐Ÿ’ก **Lint Warnings**: 2 advisory findings - `users`: Column `created_at` uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead. - `products`: Index `idx_category` on columns (category) is redundant - covered by index `idx_category_price` on columns (category, price) @@ -124,7 +124,7 @@ ALTER TABLE `order_events` DROP INDEX `idx_events_archived`; ```
-๐Ÿ’ก Lint Warnings: 6 advisory findings +๐Ÿ’ก Lint Warnings: 6 advisory findings **`orders`** - Primary key column `order_ref` has type `varchar` @@ -170,7 +170,7 @@ ALTER TABLE `orders` ADD CONSTRAINT `fk_orders_user` FOREIGN KEY (`user_id`) REF ALTER TABLE `orders` ADD COLUMN `notes` text; ``` -โ›” **Cannot apply**: **2** changes the schema-change engine refuses to execute +โ›” **Cannot apply**: 2 changes the schema-change engine refuses to execute - `users`: dropping primary key is not supported - `orders`: adding foreign key constraints is not supported @@ -208,7 +208,7 @@ ALTER TABLE `users` ALTER TABLE `orders` ADD COLUMN `notes` text; ``` -โš™๏ธ **Direct execution**: **1** change will run as native MySQL DDL +โš™๏ธ **Direct execution**: 1 change will run as native MySQL DDL - `users`: dropping primary key is not supported; runs as native MySQL DDL on a table with ~1,240 rows These statements run synchronously outside the schema-change engine: writes to each table are blocked while its statement runs, the change is **not revertible**, and `--defer-cutover` does not apply to it. Confirming the apply consents to this. @@ -248,13 +248,13 @@ ALTER TABLE `orders` DROP COLUMN `notes`; DROP TABLE `reconcile_state`; ``` -๐Ÿ›‘ **Check before applying**: **2** destructive changes SchemaBot cannot attribute to this PR +๐Ÿ›‘ **Check before applying**: 2 destructive changes SchemaBot cannot attribute to this PR - `orders`: changed by [block/schemabot#4820](https://github.com/block/schemabot/pull/4820), which is still open - `reconcile_state`: changed by [block/schemabot#4821](https://github.com/block/schemabot/pull/4821), which is still open 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 +โš ๏ธ **Issues**: 2 unsafe changes detected - `orders`: DROP COLUMN discards the column's data - `reconcile_state`: DROP TABLE removes all data @@ -288,7 +288,7 @@ schemabot apply -e staging ALTER TABLE `orders` ADD INDEX `idx_user_id`(`user_id`); ``` -โš ๏ธ **Applying destroys work in progress**: **1** unfinished copy on the target +โš ๏ธ **Applying destroys work in progress**: 1 unfinished copy on the target - `orders` in `testapp` (last progress 3h 12m ago): the schema change differs from the one that started it, which was `ALTER TABLE orders ADD INDEX idx_user_created (user_id, created_at)` Applying restarts the copy from zero rows. To keep the work already done, apply the schema change that started it. @@ -321,7 +321,7 @@ schemabot apply -e staging ALTER TABLE `orders` ADD INDEX `idx_user_id`(`user_id`); ``` -โ„น๏ธ **This apply destroys work in progress**: **1** unfinished copy on the target +โ„น๏ธ **This apply destroys work in progress**: 1 unfinished copy on the target - `orders` in `testapp` (last progress 3h 12m ago): the schema change differs from the one that started it, which was `ALTER TABLE orders ADD INDEX idx_user_created (user_id, created_at)` ๐Ÿ“‹ **Plan**: **1** table to alter @@ -349,7 +349,7 @@ ALTER TABLE `orders` ADD INDEX `idx_user_id`(`user_id`); ALTER TABLE `orders` ADD INDEX `idx_user_id`(`user_id`); ``` -โš ๏ธ **Applying destroys work in progress**: **1** unfinished copy on the target +โš ๏ธ **Applying destroys work in progress**: 1 unfinished copy on the target - `orders` in `testapp` (last progress 3h 12m ago): the schema change differs from the one that started it, which was `ALTER TABLE orders ADD INDEX idx_user_created (user_id, created_at)` Applying restarts the copy from zero rows. To keep the work already done, apply the schema change that started it. @@ -389,7 +389,7 @@ schemabot unlock ALTER TABLE `orders` ADD INDEX `idx_user_id`(`user_id`); ``` -โš ๏ธ **Applying destroys work in progress**: **1** unfinished copy on the target +โš ๏ธ **Applying destroys work in progress**: 1 unfinished copy on the target - `orders` in `testapp` (last progress 3h 12m ago): the schema change differs from the one that started it, which was `ALTER TABLE orders ADD INDEX idx_user_created (user_id, created_at)` Applying restarts the copy from zero rows. To keep the work already done, apply the schema change that started it. @@ -429,7 +429,7 @@ ALTER TABLE `orders` ADD INDEX `idx_user_id`(`user_id`); ALTER TABLE `products` ADD COLUMN `sku` varchar(64); ``` -โ™ป๏ธ **Resuming work in progress**: **1** unfinished copy on the target will be continued +โ™ป๏ธ **Resuming work in progress**: 1 unfinished copy on the target will be continued - `orders`, `products` in `testapp` (last progress 3h 12m ago) Applying picks up where the existing copy stopped rather than starting over. @@ -462,7 +462,7 @@ ALTER TABLE `orders` ADD INDEX `idx_user_id`(`user_id`); ALTER TABLE `products` ADD COLUMN `sku` varchar(64); ``` -โ™ป๏ธ **Work already in progress**: **1** unfinished copy still running on the target +โ™ป๏ธ **Work already in progress**: 1 unfinished copy still running on the target - `orders`, `products` in `testapp` (still copying) Applying joins the copy already running rather than starting a new one: every row copied so far is kept, and no second copy is made. @@ -501,7 +501,7 @@ ALTER TABLE `orders` ADD COLUMN `notes` text; --- -**โ›” Apply rejected**: **1** planned change the schema-change engine refuses to execute +**โ›” Apply rejected**: 1 planned change the schema-change engine refuses to execute - `users`: dropping primary key is not supported; direct execution is enabled but the table has ~2,400,000 rows, above the configured limit of 1,000,000 Fix what each reason names โ€” rewrite an unsupported change, or provision the stated access โ€” or contact your SchemaBot operators for help. @@ -811,7 +811,7 @@ schemabot apply -e staging } ``` -โš ๏ธ **Issues**: **2** unsafe changes detected +โš ๏ธ **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 @@ -1121,7 +1121,7 @@ ALTER TABLE `products` ADD INDEX `idx_category_price`(`category`, `price`); ### Production -> โš ๏ธ **Error:** tern client: resolve DSN for testapp/production: connection refused +> โŒ **Error:** tern client: resolve DSN for testapp/production: connection refused --- @@ -1179,7 +1179,7 @@ ALTER TABLE `products` ADD INDEX `idx_category_price`(`category`, `price`);
-๐Ÿ’ก **Lint Warnings**: **2** advisory findings +๐Ÿ’ก **Lint Warnings**: 2 advisory findings - `users`: Column `created_at` uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead. - `products`: Index `idx_category` on columns (category) is redundant - covered by index `idx_category_price` on columns (category, price) @@ -1368,7 +1368,7 @@ ALTER TABLE `customers` DROP COLUMN `nickname`; --- -**โ›” 1 Unsafe Change Detected:** +**โ›” Apply rejected**: 1 unsafe change detected - `customers`: Unsafe operation detected: DROP COLUMN `nickname` **Destructive drop guidance:** @@ -1401,7 +1401,7 @@ ALTER TABLE `customers` DROP INDEX `idx_customers_email`; --- -**โ›” 1 Unsafe Change Detected:** +**โ›” Apply rejected**: 1 unsafe change detected - `customers`: Unsafe operation detected: DROP INDEX `idx_customers_email` **Destructive drop guidance:** @@ -1436,7 +1436,7 @@ ALTER TABLE `users` RENAME COLUMN `email` TO `email_address`; --- -**โ›” 3 Unsafe Changes Detected:** +**โ›” 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. @@ -1544,7 +1544,7 @@ ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`); ``` -> โš ๏ธ **Error:** lock wait timeout exceeded; try restarting transaction +> โŒ **Error:** lock wait timeout exceeded; try restarting transaction --- @@ -1591,7 +1591,7 @@ CREATE TABLE `orders` ( ALTER TABLE `products` ADD INDEX `idx_category_price`(`category`, `price`); ``` -๐Ÿ’ก **Lint Warnings**: **2** advisory findings +๐Ÿ’ก **Lint Warnings**: 2 advisory findings - `users`: Column `created_at` uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead. - `products`: Index `idx_category` on columns (category) is redundant - covered by index `idx_category_price` on columns (category, price) @@ -2678,7 +2678,7 @@ _Requested by @jackjackbits_ Apply Blocked By Prior Env (Pending) -## โŒ Apply Blocked โ€” Production +## โ›” Apply Blocked โ€” Production **Database**: `testapp` @@ -2695,7 +2695,7 @@ schemabot apply -e staging Apply Blocked By Prior Env (Failed) -## โŒ Apply Blocked โ€” Production +## โ›” Apply Blocked โ€” Production **Database**: `testapp` @@ -2730,7 +2730,7 @@ schemabot apply -e production Apply Blocked: Prior Env Check Missing -## โŒ Apply Blocked +## โ›” Apply Blocked SchemaBot could not find a completed `staging` check for this PR. @@ -2761,7 +2761,7 @@ _See server logs for details._ Apply Blocked: Prior Env Check Untrusted -## โŒ Apply Blocked +## โ›” Apply Blocked A `staging` check named `SchemaBot (staging)` exists on this PR, but it was created by a GitHub App this SchemaBot deployment does not trust: @@ -2782,7 +2782,7 @@ Re-running `schemabot plan -e staging` will not resolve this. Apply Blocked: Environment Not In Promotion Order -## โŒ Apply Blocked โ€” Development +## โ›” Apply Blocked โ€” Development `development` is not in the configured promotion order, so SchemaBot cannot determine which environments must be applied before it and cannot enforce staging-first ordering. @@ -2861,7 +2861,7 @@ Schema changes require approval from an authorized reviewer before applying. Apply Blocked: Checks Not Passing -## โŒ Apply Blocked โ€” Staging +## โ›” Apply Blocked โ€” Staging Cannot apply while PR checks are not passing: @@ -3076,7 +3076,7 @@ ALTER TABLE `users` ADD INDEX `idx_email_created`(`email`, `created_at`); ``` -> โš ๏ธ **Error:** lock wait timeout exceeded; try restarting transaction +> โŒ **Error:** lock wait timeout exceeded; try restarting transaction --- @@ -3886,7 +3886,7 @@ ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`); ``` -> โš ๏ธ **Error:** Error 1061: Duplicate key name 'idx_user_id' +> โŒ **Error:** Error 1061: Duplicate key name 'idx_user_id' --- @@ -3933,7 +3933,7 @@ ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`); ``` -> โš ๏ธ **Error:** lock wait timeout exceeded; try restarting transaction +> โŒ **Error:** lock wait timeout exceeded; try restarting transaction --- @@ -3966,7 +3966,7 @@ schemabot apply -e staging ```sql ALTER TABLE `orders` MODIFY COLUMN `status` enum('NEW','PENDING','SHIPPED','DELIVERED') NOT NULL; ``` -> โš ๏ธ Last error: preflight enumReorder check failed: reordering existing ENUM values on column `status` is unsafe: retained values must keep their relative order and new values must be appended at the end +> โŒ Last error: preflight enumReorder check failed: reordering existing ENUM values on column `status` is unsafe: retained values must keep their relative order and new values must be appended at the end **`users`**: โŠ˜ Cancelled (not started) @@ -3981,7 +3981,7 @@ ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`); ``` -> โš ๏ธ **Error:** table orders failed: preflight enumReorder check failed: reordering existing ENUM values on column `status` is unsafe: retained values must keep their relative order and new values must be appended at the end +> โŒ **Error:** table orders failed: preflight enumReorder check failed: reordering existing ENUM values on column `status` is unsafe: retained values must keep their relative order and new values must be appended at the end --- @@ -4748,7 +4748,7 @@ ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`); *Applied by @jackjackbits at 2026-03-15 14:22:00 UTC* -> โš ๏ธ **Error:** table users failed: schema change failed: unsafe warning: Field 'name' doesn't have a default value +> โŒ **Error:** table users failed: schema change failed: unsafe warning: Field 'name' doesn't have a default value 1 of 3 tables completed before failure. @@ -4997,7 +4997,7 @@ _Apply ID: `apply-a1b2c3d4e5f6`_ *Applied by @jackjackbits at 2026-03-15 11:00:00 UTC* -> โš ๏ธ **Error:** Error 1062: Duplicate entry '12345' for key 'addresses.idx_user_id' +> โŒ **Error:** Error 1062: Duplicate entry '12345' for key 'addresses.idx_user_id' 4 of 8 tables completed before failure. @@ -5075,7 +5075,7 @@ schemabot apply -e staging *Applied by @jackjackbits at 2026-03-15 14:22:00 UTC* -> โš ๏ธ **Error:** table customers.addresses failed: Error 1205: Lock wait timeout exceeded +> โŒ **Error:** table customers.addresses failed: Error 1205: Lock wait timeout exceeded 3 of 5 tables completed before failure. @@ -7004,7 +7004,7 @@ _Last updated: 2026-01-01 00:00:0 **Deployments**: 1 completed, 2 halted, 1 failed -> โš ๏ธ **First failure:** us โ€” lock wait timeout exceeded; try restarting transaction +> โŒ **First failure:** us โ€” lock wait timeout exceeded; try restarting transaction --- @@ -7085,7 +7085,7 @@ ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`); ``` -> โš ๏ธ **Error:** lock wait timeout exceeded; try restarting transaction +> โŒ **Error:** lock wait timeout exceeded; try restarting transaction --- @@ -7365,7 +7365,7 @@ ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`); **Deployments**: 1 completed, 2 halted, 1 failed -> โš ๏ธ **First failure:** us โ€” lock wait timeout exceeded; try restarting transaction +> โŒ **First failure:** us โ€” lock wait timeout exceeded; try restarting transaction --- @@ -7421,7 +7421,7 @@ ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`); *Applied by @aparajon at 2026-03-15 14:22:00 UTC* -> โš ๏ธ **Error:** lock wait timeout exceeded; try restarting transaction +> โŒ **Error:** lock wait timeout exceeded; try restarting transaction 1 of 3 tables completed before failure. @@ -7572,7 +7572,7 @@ ALTER TABLE `mutes` DROP COLUMN `legacy_reason`; ``` -โš ๏ธ **Issues**: **1** unsafe change detected +โš ๏ธ **Issues**: 1 unsafe change detected - `mutes` (shard `40-80`): DROP COLUMN removes data and is irreversible **Destructive drop guidance:** @@ -7628,7 +7628,7 @@ _Last updated: 2026-01-01 00:00:0 **Shards**: 1 failed, 3 halted -> โš ๏ธ **First failure:** shard -40 โ€” resolve shard primary for `-40`: context deadline exceeded +> โŒ **First failure:** shard -40 โ€” resolve shard primary for `-40`: context deadline exceeded #### Keyspace `cdb_resolute_sharded` @@ -7738,7 +7738,7 @@ _Last updated: 2026-01-01 00:00:0 **Shards**: 1 failed, 3 halted -> โš ๏ธ **First failure:** shard -40 โ€” resolve shard primary for `-40`: context deadline exceeded +> โŒ **First failure:** shard -40 โ€” resolve shard primary for `-40`: context deadline exceeded #### Keyspace `cdb_resolute_sharded` diff --git a/docs/lint-and-safety-levels.md b/docs/lint-and-safety-levels.md index 9da507525..d61c0a64c 100644 --- a/docs/lint-and-safety-levels.md +++ b/docs/lint-and-safety-levels.md @@ -133,11 +133,11 @@ comment is the *gate firing*. Same data, two moments in time: 1. **Plan time โ€” โš ๏ธ Issues.** The plan comment lists every unsafe change with its reason. Nothing has been refused yet; this is the review surface. -2. **Apply time โ€” โ›” Unsafe Changes Detected.** If an operator runs +2. **Apply time โ€” โ›” Apply rejected.** If an operator runs `schemabot apply` without `--allow-unsafe` while unsafe changes exist, SchemaBot posts a new comment: the full plan, a - `โ›” N Unsafe Changes Detected` section, and a ๐Ÿšจ instruction to re-run with - `--allow-unsafe`. The apply did not start. + `โ›” Apply rejected: N unsafe changes detected` section, and a ๐Ÿšจ instruction + to re-run with `--allow-unsafe`. The apply did not start. So โš ๏ธ always means "review this before you apply", and โ›” always means "your command was refused". Once an apply *is* acknowledged with `--allow-unsafe`, @@ -158,12 +158,13 @@ rejected up front while they are present. | Icon | Where it appears | Meaning | |---|---|---| -| โ›” | Plan comment (**Cannot apply**), apply-rejection comments (**Unsafe Changes Detected**, **Apply rejected**, **Apply Blocked: PR Is Merged/Closed**), CLI apply-blocked headings (**Apply blocked**) | Refusal: this will not or did not proceed | +| โ›” | Plan comment (**Cannot apply**), unsafe/blocked apply-rejection comments (**Apply rejected**), and the **Apply Blocked** headings where retrying unchanged refuses again (merged/closed PR, failing required checks, missing or untrusted prior-environment check, unlisted environment), plus CLI apply-blocked headings (**Apply blocked**) | Refusal: this will not or did not proceed | | โš ๏ธ | Plan comment (**Issues**), CLI plan output (**Unsafe Changes Detected**) | Caution: unsafe changes to review before applying | | ๐Ÿšจ | Apply-rejection comment; CLI apply output | The `--allow-unsafe` instruction, or (CLI) the banner confirming it was supplied | | โš™๏ธ | Plan and locked apply comments (**Direct execution**) | Consent disclosure for native-DDL statements | | ๐Ÿ’ก | Plan comment and CLI (**Lint Warnings**) | Advisory best-practice findings | | โœ… | Plan comment | No schema changes detected | +| โŒ | Failed apply/rollback headings, error and first-failure callouts | An attempted operation failed | Presentation notes: @@ -174,3 +175,12 @@ Presentation notes: types) render as inline code. - The CLI and the plan comment share the same severity reading: โš ๏ธ marks unsafe changes awaiting review at plan time, and โ›” marks the refused apply. +- Not every **Apply Blocked** or **Apply rejected** heading is a refusal: the + glyph follows the cause. ๐Ÿ”’ marks an apply blocked by a held lock, โณ one + waiting on required checks or another apply (wait, then retry), โŒ one that + fail-closed on a transient verification error (retry unchanged can succeed), + and โš ๏ธ a stale-base rejection cleared by rebasing. +- The severity vocabulary (๐Ÿšจ โ›” โŒ โš ๏ธ โ„น๏ธ) lives in `pkg/glyph`. The other + icons in this table โ€” and state/consent icons such as โœ…, ๐Ÿ’ก, โš™๏ธ, and ๐Ÿ›‘ + (**Check before applying**, the unattributed-destructive-change gate) โ€” are + deliberately outside it: they mark states and disclosures, not severities. diff --git a/pkg/webhook/apply_integration_test.go b/pkg/webhook/apply_integration_test.go index 44c50f0c9..d6c817826 100644 --- a/pkg/webhook/apply_integration_test.go +++ b/pkg/webhook/apply_integration_test.go @@ -1807,7 +1807,7 @@ func TestE2EApplyStaleBaseSchemaOutranksUnsafePrompt(t *testing.T) { assert.Contains(t, body, "Apply rejected โ€” base schema is newer") assert.Contains(t, body, "Merge or rebase") assert.NotContains(t, body, "allow-unsafe", "stale branch must not be coached toward --allow-unsafe") - assert.NotContains(t, body, "Unsafe Change", "unsafe prompt must not outrank the freshness rejection") + assert.NotContains(t, body, "unsafe change", "unsafe prompt must not outrank the freshness rejection") assert.NotContains(t, body, "DROP COLUMN", "no stale-plan DDL may be rendered") case <-time.After(30 * time.Second): t.Fatal("timed out waiting for base-schema rejection") @@ -1932,7 +1932,7 @@ func TestE2EApplyConfirmStaleBaseSchemaAtFinalGateOutranksUnsafePrompt(t *testin case body := <-result.comments: assert.Contains(t, body, "Apply rejected โ€” base schema is newer") assert.NotContains(t, body, "allow-unsafe", "stale branch must not be coached toward --allow-unsafe") - assert.NotContains(t, body, "Unsafe Change", "unsafe prompt must not outrank the freshness rejection") + assert.NotContains(t, body, "unsafe change", "unsafe prompt must not outrank the freshness rejection") case <-time.After(30 * time.Second): t.Fatal("timed out waiting for final-gate base-schema rejection") } diff --git a/pkg/webhook/plan_change_ownership_test.go b/pkg/webhook/plan_change_ownership_test.go index fc0535b58..2dc4abc6f 100644 --- a/pkg/webhook/plan_change_ownership_test.go +++ b/pkg/webhook/plan_change_ownership_test.go @@ -109,7 +109,7 @@ func TestRenderPlanComment_AttributedChangeNamesOwnerAndStillOffersApply(t *test rendered := templates.RenderPlanComment(data) - assert.Contains(t, rendered, "๐Ÿ›‘ **Check before applying**: **1** destructive change SchemaBot cannot attribute to this PR") + assert.Contains(t, rendered, "๐Ÿ›‘ **Check before applying**: 1 destructive change SchemaBot cannot attribute to this PR") assert.Contains(t, rendered, "[block/schemabot#42](https://github.com/block/schemabot/pull/42)") // Reconciling the live database to the declared schema stays the operator's // call, so the attribution informs the decision without removing it. diff --git a/pkg/webhook/plan_test.go b/pkg/webhook/plan_test.go index d627fbf09..21fbd4247 100644 --- a/pkg/webhook/plan_test.go +++ b/pkg/webhook/plan_test.go @@ -313,7 +313,7 @@ func TestRenderPlanComment_ShowsUnsafeWarning(t *testing.T) { rendered := templates.RenderPlanComment(data) - assert.Contains(t, rendered, "**Issues**: **1** unsafe change detected") + assert.Contains(t, rendered, "**Issues**: 1 unsafe change detected") assert.Contains(t, rendered, "`orders`") assert.Contains(t, rendered, "DROP INDEX without making invisible first") } @@ -342,7 +342,7 @@ func TestRenderPlanComment_UnsafeWarningSummaryCountsChanges(t *testing.T) { rendered := templates.RenderPlanComment(data) - assert.Contains(t, rendered, "โš ๏ธ **Issues**: **2** unsafe changes detected") + 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") } @@ -884,7 +884,7 @@ func TestRenderUnsafeChangesBlocked_UsedByApplyFlow(t *testing.T) { rendered := templates.RenderUnsafeChangesBlocked(data) - assert.Contains(t, rendered, "โ›” 1 Unsafe Change Detected") + assert.Contains(t, rendered, "**โ›” Apply rejected**: 1 unsafe change detected") assert.Contains(t, rendered, "`users`") assert.Contains(t, rendered, "DROP TABLE removes all data") assert.Contains(t, rendered, "--allow-unsafe") @@ -913,7 +913,7 @@ func TestRenderUnsafeChangesBlocked_SplitsJoinedReasonsIntoBullets(t *testing.T) rendered := templates.RenderUnsafeChangesBlocked(data) - assert.Contains(t, rendered, "โ›” 3 Unsafe Changes Detected") + 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") @@ -945,7 +945,7 @@ func TestRenderPlanComment_SplitsJoinedUnsafeReasonsIntoBullets(t *testing.T) { rendered := templates.RenderPlanComment(data) - assert.Contains(t, rendered, "**3** unsafe changes detected") + 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") @@ -974,7 +974,7 @@ func TestRenderUnsafeChangesBlocked_EmptyReasonListsBareTableAndCountsOnce(t *te rendered := templates.RenderUnsafeChangesBlocked(data) - assert.Contains(t, rendered, "โ›” 2 Unsafe Changes Detected") + 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") @@ -997,7 +997,7 @@ func TestRenderPlanComment_EmptyUnsafeReasonListsBareTableAndCountsOnce(t *testi rendered := templates.RenderPlanComment(data) - assert.Contains(t, rendered, "**1** unsafe change detected") + assert.Contains(t, rendered, "1 unsafe change detected") assert.Contains(t, rendered, "- `users`\n") assert.NotContains(t, rendered, "- `users`:") } diff --git a/pkg/webhook/sharded_apply_test.go b/pkg/webhook/sharded_apply_test.go index 8cd1dde34..37b592932 100644 --- a/pkg/webhook/sharded_apply_test.go +++ b/pkg/webhook/sharded_apply_test.go @@ -488,7 +488,7 @@ func TestFormatApplySummaryComment_ShardedApplyLevelErrorSurfaced(t *testing.T) out := formatApplySummaryComment(apply, ops, false, nil, nil, nil, nil, "") assert.Contains(t, out, "## โŒ Schema Change Failed โ€” Staging") - assert.Contains(t, out, "> โš ๏ธ **Failure:** finalize vschema: apply vschema to keyspace: context deadline exceeded", + assert.Contains(t, out, "> โŒ **Failure:** finalize vschema: apply vschema to keyspace: context deadline exceeded", "the apply row's error reaches the callout when no shard carries the failure") assert.NotContains(t, out, "First failure:", "no shard failed, so there is no shard failure callout") } diff --git a/pkg/webhook/templates/apply.go b/pkg/webhook/templates/apply.go index 74412095f..7a66c0129 100644 --- a/pkg/webhook/templates/apply.go +++ b/pkg/webhook/templates/apply.go @@ -9,6 +9,7 @@ import ( "github.com/block/schemabot/pkg/apitypes" "github.com/block/schemabot/pkg/ddl" + "github.com/block/schemabot/pkg/glyph" "github.com/block/schemabot/pkg/state" "github.com/block/schemabot/pkg/storage" "github.com/block/schemabot/pkg/ui" @@ -177,9 +178,17 @@ func renderApplyStatusComment(data ApplyStatusCommentData, includeLastUpdated bo // per-table task (a VSchema-only apply has no tables at all). writeVSchemaStatus(&sb, data.VSchemaChanges) - // Error message for apply states that need operator triage. - if state.IsState(data.State, state.Apply.Failed, state.Apply.Stopped) && data.ErrorMessage != "" { - writeErrorBlock(&sb, data.ErrorMessage) + // Error message for apply states that need operator attention. A failed + // apply gets the failure glyph โ€” the system stopped and triage is due; a + // stopped apply gets the attention glyph โ€” the heading already says the + // operator paused it, and the error is context, not a fresh failure. + if data.ErrorMessage != "" { + switch { + case state.IsState(data.State, state.Apply.Failed): + writeErrorBlock(&sb, glyph.Failed, data.ErrorMessage) + case state.IsState(data.State, state.Apply.Stopped): + writeErrorBlock(&sb, glyph.Attention, data.ErrorMessage) + } } // Footer with next actions @@ -220,7 +229,7 @@ func writeApplyHeader(sb *strings.Builder, data ApplyStatusCommentData) { case state.Apply.Completed: writeEnvironmentTitle(sb, "โœ… Schema Change Applied", data.Environment) case state.Apply.Failed: - writeEnvironmentTitle(sb, "โŒ Schema Change Failed", data.Environment) + writeEnvironmentTitle(sb, glyph.Failed+" Schema Change Failed", data.Environment) writeSupportChannelOffer(sb) case state.Apply.Stopped: writeEnvironmentTitle(sb, "โน๏ธ Schema Change Stopped", data.Environment) @@ -242,7 +251,7 @@ func writeRollbackHeader(sb *strings.Builder, data ApplyStatusCommentData) { case state.Apply.Completed: writeEnvironmentTitle(sb, "โช Rollback Complete", data.Environment) case state.Apply.Failed: - writeEnvironmentTitle(sb, "โŒ Rollback Failed", data.Environment) + writeEnvironmentTitle(sb, glyph.Failed+" Rollback Failed", data.Environment) writeSupportChannelOffer(sb) case state.Apply.Stopped: writeEnvironmentTitle(sb, "โน๏ธ Rollback Stopped", data.Environment) @@ -803,15 +812,15 @@ func renderTableProgress(sb *strings.Builder, table TableProgressData, applyStat // at all, so its failure label does not mention one. switch pct := ui.RowCopyDisplayPercent(table.PercentComplete, table.RowsCopied); { case pct > 0: - fmt.Fprintf(sb, "**`%s`**: %s \u274c Failed\n", table.TableName, ui.ProgressBarFailed(pct)) + fmt.Fprintf(sb, "**`%s`**: %s "+glyph.Failed+" Failed\n", table.TableName, ui.ProgressBarFailed(pct)) case table.IsInstant: - fmt.Fprintf(sb, "**`%s`**: \u274c Failed\n", table.TableName) + fmt.Fprintf(sb, "**`%s`**: "+glyph.Failed+" Failed\n", table.TableName) default: - fmt.Fprintf(sb, "**`%s`**: \u274c Failed (before row copy started)\n", table.TableName) + fmt.Fprintf(sb, "**`%s`**: "+glyph.Failed+" Failed (before row copy started)\n", table.TableName) } writeDDLLine(sb, table.DDL) if taskErrorAddsDetail(table.ErrorMessage, applyError) { - writeTableErrorLine(sb, table.ErrorMessage) + writeTableErrorLine(sb, glyph.Failed, table.ErrorMessage) } case state.Task.FailedRetryable: @@ -828,7 +837,9 @@ func renderTableProgress(sb *strings.Builder, table TableProgressData, applyStat } writeDDLLine(sb, table.DDL) if table.ErrorMessage != "" { - writeTableErrorLine(sb, table.ErrorMessage) + // The row above says SchemaBot is retrying on its own, so the + // error is context for the operator, not a failure to triage. + writeTableErrorLine(sb, glyph.Attention, table.ErrorMessage) } case state.Task.Cancelled: @@ -979,7 +990,7 @@ func renderRunningTable(sb *strings.Builder, table TableProgressData) { fmt.Fprintf(sb, "**`%s`**: %s Finalizing copy%s\n", table.TableName, ui.ProgressBarActivity(), throttledSuffix(table)) writeDDLLine(sb, table.DDL) fmt.Fprintf(sb, "- Rows copied: %s so far\n", ui.FormatNumber(table.RowsCopied)) - fmt.Fprintf(sb, "- โ„น๏ธ _%s_\n", ui.EstimateExceededTooltip) + fmt.Fprintf(sb, "- "+glyph.Info+" _%s_\n", ui.EstimateExceededTooltip) return } @@ -1030,10 +1041,10 @@ func writeThrottleTooltip(sb *strings.Builder, table TableProgressData) { // whose signal has no tip renders alone so a new engine signal degrades // to raw text rather than a wrong explanation. if tip := ui.ThrottleTip(table.ThrottleReason); tip != "" { - fmt.Fprintf(sb, "- โ„น๏ธ _Throttled: %s ยท %s ([docs](%s))_\n", escapeInlineMarkdown(table.ThrottleReason), tip, ui.ThrottleDocURL) + fmt.Fprintf(sb, "- "+glyph.Info+" _Throttled: %s ยท %s ([docs](%s))_\n", escapeInlineMarkdown(table.ThrottleReason), tip, ui.ThrottleDocURL) return } - fmt.Fprintf(sb, "- โ„น๏ธ _Throttled: %s_\n", escapeInlineMarkdown(table.ThrottleReason)) + fmt.Fprintf(sb, "- "+glyph.Info+" _Throttled: %s_\n", escapeInlineMarkdown(table.ThrottleReason)) } func recoveringIsCopyingRows(table TableProgressData) bool { @@ -1248,7 +1259,7 @@ func writeSummaryFailed(sb *strings.Builder, data ApplyStatusCommentData, comple writeSummaryMetadata(sb, data) if data.ErrorMessage != "" { - writeErrorBlock(sb, data.ErrorMessage) + writeErrorBlock(sb, glyph.Failed, data.ErrorMessage) } if completedCount > 0 { @@ -1581,7 +1592,7 @@ func groupStateEmoji(tables []TableProgressData) string { } if states[state.Task.Failed] { - return "โŒ" + return glyph.Failed } if states["reverted"] { return "โ†ฉ๏ธ" diff --git a/pkg/webhook/templates/apply_commands.go b/pkg/webhook/templates/apply_commands.go index d8a4ea91b..1716da1cb 100644 --- a/pkg/webhook/templates/apply_commands.go +++ b/pkg/webhook/templates/apply_commands.go @@ -7,6 +7,7 @@ import ( "time" "github.com/block/schemabot/pkg/caller" + "github.com/block/schemabot/pkg/glyph" ) // ApplyLockConflictData contains data for apply lock conflict comments. @@ -175,7 +176,7 @@ func RenderUnsafeChangesBlocked(data PlanCommentData) string { // Unsafe changes blocked section sb.WriteString("---\n\n") unsafeCount := countUnsafeFindings(data.UnsafeChanges) - fmt.Fprintf(&sb, "**โ›” %d Unsafe %s Detected:**\n", unsafeCount, pluralize("Change", unsafeCount)) + fmt.Fprintf(&sb, "**"+glyph.Refused+" Apply rejected**: %d unsafe %s detected\n", unsafeCount, pluralize("change", unsafeCount)) for _, c := range data.UnsafeChanges { writeUnsafeChangeItem(&sb, "`"+c.Table+"`", c.Reason) } @@ -189,7 +190,7 @@ func RenderUnsafeChangesBlocked(data PlanCommentData) string { writeAttributedChanges(&sb, data.AttributedChanges) } - sb.WriteString("**๐Ÿšจ To proceed with these destructive changes, re-run with `--allow-unsafe`:**\n") + sb.WriteString("**" + glyph.Escalation + " To proceed with these destructive changes, re-run with `--allow-unsafe`:**\n") applyCmd := fmt.Sprintf("schemabot apply -e %s", data.Environment) if data.Tenant != "" { applyCmd += fmt.Sprintf(" --tenant %s", data.Tenant) @@ -225,7 +226,7 @@ func RenderBlockedChangesApplyRejected(data PlanCommentData) string { sb.WriteString("---\n\n") n := len(data.BlockedChanges) - fmt.Fprintf(&sb, "**โ›” Apply rejected**: **%d** planned %s the schema-change engine refuses to execute\n", n, pluralize("change", n)) + fmt.Fprintf(&sb, "**"+glyph.Refused+" Apply rejected**: %d planned %s the schema-change engine refuses to execute\n", n, pluralize("change", n)) for _, c := range data.BlockedChanges { table := "`" + c.Table + "`" if len(c.Shards) > 0 { @@ -330,7 +331,7 @@ func RenderApplyBlockedByOtherPR(data ApplyLockConflictData) string { func RenderApplyInProgress(data ApplyLockConflictData) string { var sb strings.Builder - writeEnvironmentTitle(&sb, "โš ๏ธ Apply Already In Progress", data.Environment) + writeEnvironmentTitle(&sb, glyph.Attention+" Apply Already In Progress", data.Environment) writeDBLine(&sb, data.Database) writeRequesterOrTimestamp(&sb, data.RequestedBy) sb.WriteString("\n") @@ -349,14 +350,14 @@ func RenderApplyBlockedClosedPR(environment, requestedBy string, merged bool) st var sb strings.Builder if merged { - writeEnvironmentTitle(&sb, "โ›” Apply Blocked: PR Is Merged", environment) + writeEnvironmentTitle(&sb, glyph.Refused+" Apply Blocked: PR Is Merged", environment) writeRequesterOrTimestamp(&sb, requestedBy) sb.WriteString("\nThis PR is already merged, so applies can no longer run from it. SchemaBot only applies schema changes from open PRs.\n\n") sb.WriteString("If the schema change still needs to be applied, open a new PR with it and apply from there.\n") return sb.String() } - writeEnvironmentTitle(&sb, "โ›” Apply Blocked: PR Is Closed", environment) + writeEnvironmentTitle(&sb, glyph.Refused+" Apply Blocked: PR Is Closed", environment) writeRequesterOrTimestamp(&sb, requestedBy) sb.WriteString("\nThis PR is closed, so its schema changes can never merge. SchemaBot only applies schema changes from open PRs.\n\n") sb.WriteString("Reopen this PR, or open a new PR with the schema change, and apply from there.\n") @@ -390,7 +391,7 @@ func RenderLocksAlreadyReleased() string { func RenderCannotUnlock(database, environment, applyID, applyState string) string { var sb strings.Builder - writeEnvironmentTitle(&sb, "โš ๏ธ Cannot Unlock", environment) + writeEnvironmentTitle(&sb, glyph.Attention+" Cannot Unlock", environment) writeDBLine(&sb, database) sb.WriteString("\n") fmt.Fprintf(&sb, "An apply is currently active (apply ID: `%s`, state: `%s`).\n\n", @@ -429,7 +430,7 @@ type StaleSchemaRejectionData struct { func RenderStaleSchemaRejection(data StaleSchemaRejectionData) string { var sb strings.Builder - writeEnvironmentTitle(&sb, "โš ๏ธ Rejected โ€” new commits since discovery", data.Environment) + writeEnvironmentTitle(&sb, glyph.Attention+" Rejected โ€” new commits since discovery", data.Environment) writeDBLine(&sb, data.Database) sb.WriteString("\n") fmt.Fprintf(&sb, "Schema files were loaded at `%s`, but the current PR HEAD is `%s`. ", data.DiscoverySHA, data.CurrentSHA) @@ -463,7 +464,7 @@ type StalePlanRejectionData struct { func RenderStalePlanRejection(data StalePlanRejectionData) string { var sb strings.Builder - writeEnvironmentTitle(&sb, "โš ๏ธ Rejected โ€” the plan you confirmed is stale", data.Environment) + writeEnvironmentTitle(&sb, glyph.Attention+" Rejected โ€” the plan you confirmed is stale", data.Environment) writeDBLine(&sb, data.Database) sb.WriteString("\n") fmt.Fprintf(&sb, "The confirmation plan was rendered at `%s`, but the current PR HEAD is `%s`. ", data.PlanSHA, data.CurrentSHA) @@ -494,7 +495,7 @@ type BaseSchemaFreshnessRejectionData struct { func RenderBaseSchemaFreshnessRejection(data BaseSchemaFreshnessRejectionData) string { var sb strings.Builder - writeEnvironmentTitle(&sb, "โš ๏ธ Apply rejected โ€” base schema is newer", data.Environment) + writeEnvironmentTitle(&sb, glyph.Attention+" Apply rejected โ€” base schema is newer", data.Environment) writeDBLine(&sb, data.Database) sb.WriteString("\n") if data.VerificationError { @@ -530,7 +531,7 @@ func RenderApplyConfirmNoLock(database, environment string) string { func RenderApplyBlockedByPriorEnv(database, environment, priorEnv, status, action string) string { var sb strings.Builder - writeEnvironmentTitle(&sb, "โŒ Apply Blocked", environment) + writeEnvironmentTitle(&sb, glyph.Refused+" Apply Blocked", environment) writeDBLine(&sb, database) sb.WriteString("\n") fmt.Fprintf(&sb, "%s %s. %s before applying to %s.\n\n", capitalizeFirst(priorEnv), status, action, environment) @@ -554,7 +555,7 @@ type BlockingCheck struct { func RenderApplyBlockedByNonPassingChecks(environment string, notPassing []BlockingCheck) string { var sb strings.Builder - writeEnvironmentTitle(&sb, "โŒ Apply Blocked", environment) + writeEnvironmentTitle(&sb, glyph.Refused+" Apply Blocked", environment) if len(notPassing) == 0 { // Defensive: callers should only invoke this template when at least // one non-passing check has been identified. Render a generic message @@ -593,7 +594,10 @@ type CheckStatusAccessDetails struct { func RenderApplyBlockedByCheckStatusError(environment string, err error, details *CheckStatusAccessDetails) string { var sb strings.Builder - writeEnvironmentTitle(&sb, "โŒ Apply Blocked", environment) + // Failure glyph, not refusal: a check-status read error is transient โ€” an + // unchanged retry can succeed โ€” so this is a failed verification the apply + // fail-closed on, not a request SchemaBot refuses to perform. + writeEnvironmentTitle(&sb, glyph.Failed+" Apply Blocked", environment) if err != nil && strings.Contains(err.Error(), "Resource not accessible") { app := "SchemaBot GitHub App" @@ -691,7 +695,10 @@ func RenderApplyBlockedByInProgressChecks(environment string, inProgress, notRep func RenderApplyBlockedByPriorEnvCheckError(priorEnv, reason string) string { var sb strings.Builder - sb.WriteString("## โŒ Apply Blocked\n\n") + // Failure glyph, not refusal: the prior-environment read is transient โ€” + // an unchanged retry can succeed โ€” so this is a failed verification the + // apply fail-closed on, not a request SchemaBot refuses to perform. + sb.WriteString("## " + glyph.Failed + " Apply Blocked\n\n") fmt.Fprintf(&sb, "Could not verify %s status: failed to %s. Retry the apply command.\n\n", priorEnv, reason) sb.WriteString("_See server logs for details._") @@ -705,7 +712,7 @@ func RenderApplyBlockedByPriorEnvCheckError(priorEnv, reason string) string { func RenderApplyBlockedByMissingPriorEnvCheck(priorEnv string) string { var sb strings.Builder - sb.WriteString("## โŒ Apply Blocked\n\n") + sb.WriteString("## " + glyph.Refused + " Apply Blocked\n\n") fmt.Fprintf(&sb, "SchemaBot could not find a completed `%s` check for this PR.\n\n", priorEnv) fmt.Fprintf(&sb, "SchemaBot must verify `%s` before applying a later environment. Create the missing `%s` status with:\n", priorEnv, priorEnv) fmt.Fprintf(&sb, "```\nschemabot plan -e %s\n```\n\n", priorEnv) @@ -723,7 +730,7 @@ func RenderApplyBlockedByMissingPriorEnvCheck(priorEnv string) string { func RenderApplyBlockedByUntrustedPriorEnvCheck(priorEnv, checkName string, untrustedApps []string) string { var sb strings.Builder - sb.WriteString("## โŒ Apply Blocked\n\n") + sb.WriteString("## " + glyph.Refused + " Apply Blocked\n\n") fmt.Fprintf(&sb, "A `%s` check named `%s` exists on this PR, but it was created by a GitHub App this SchemaBot deployment does not trust:\n\n", priorEnv, checkName) for _, app := range untrustedApps { fmt.Fprintf(&sb, "- `%s`\n", app) @@ -761,7 +768,7 @@ func RenderApplyBlockedByPriorEnvInProgress(database, environment, priorEnv stri func RenderApplyBlockedByUnlistedEnvironment(environment string, promotionOrder []string) string { var sb strings.Builder - writeEnvironmentTitle(&sb, "โŒ Apply Blocked", environment) + writeEnvironmentTitle(&sb, glyph.Refused+" Apply Blocked", environment) fmt.Fprintf(&sb, "`%s` is not in the configured promotion order, so SchemaBot cannot determine which environments must be applied before it and cannot enforce staging-first ordering.\n\n", environment) if len(promotionOrder) > 0 { fmt.Fprintf(&sb, "Configured promotion order: `%s`\n\n", strings.Join(promotionOrder, "` โ†’ `")) diff --git a/pkg/webhook/templates/apply_test.go b/pkg/webhook/templates/apply_test.go index 4d4207c2f..4a54f202d 100644 --- a/pkg/webhook/templates/apply_test.go +++ b/pkg/webhook/templates/apply_test.go @@ -54,7 +54,7 @@ func TestRenderApplyCommentsIncludeEnvironmentInTitle(t *testing.T) { rendered := RenderApplyBlockedByPriorEnv("testapp", "production", "staging", "has pending changes", "Apply staging first") firstLine, _, _ := strings.Cut(rendered, "\n") - assert.Equal(t, "## โŒ Apply Blocked โ€” Production", firstLine) + assert.Equal(t, "## โ›” Apply Blocked โ€” Production", firstLine) }) } @@ -952,7 +952,7 @@ func TestRenderApplyStatusComment_Failed(t *testing.T) { assert.Contains(t, result, "## Schema Change Status โ€” Staging") assert.Contains(t, result, "**Status**: Failed") - assert.Contains(t, result, "โš ๏ธ **Error:**") + assert.Contains(t, result, "โŒ **Error:**") assert.Contains(t, result, "lock wait timeout exceeded") assert.Contains(t, result, "๐ŸŸฅ") // red bar for failed table assert.Contains(t, result, "โŒ Failed") @@ -1106,36 +1106,36 @@ func TestRenderApplyStatusComment_FailedTableErrorLine(t *testing.T) { t.Run("table error distinct from apply error renders below the row", func(t *testing.T) { result := render("1 of 2 tables failed", "preflight enumReorder check failed", 35) - assert.Contains(t, result, "> โš ๏ธ **Error:** 1 of 2 tables failed") - assert.Contains(t, result, "> โš ๏ธ Last error: preflight enumReorder check failed") + assert.Contains(t, result, "> โŒ **Error:** 1 of 2 tables failed") + assert.Contains(t, result, "> โŒ Last error: preflight enumReorder check failed") }) t.Run("table error identical to apply error is not repeated", func(t *testing.T) { result := render("preflight enumReorder check failed", "preflight enumReorder check failed", 35) - assert.Contains(t, result, "> โš ๏ธ **Error:** preflight enumReorder check failed") - assert.NotContains(t, result, "> โš ๏ธ Last error:") + assert.Contains(t, result, "> โŒ **Error:** preflight enumReorder check failed") + assert.NotContains(t, result, "> โŒ Last error:") assert.Equal(t, 1, strings.Count(result, "preflight enumReorder check failed")) }) t.Run("table without an error renders no error line", func(t *testing.T) { result := render("apply-level failure", "", 35) - assert.NotContains(t, result, "> โš ๏ธ Last error:") + assert.NotContains(t, result, "> โŒ Last error:") }) t.Run("table error differing only by whitespace is not repeated", func(t *testing.T) { result := render("preflight enumReorder check failed", "preflight enumReorder check failed\n", 35) - assert.NotContains(t, result, "> โš ๏ธ Last error:") + assert.NotContains(t, result, "> โŒ Last error:") }) t.Run("all-whitespace table error renders no error line", func(t *testing.T) { result := render("apply-level failure", " \n", 35) - assert.NotContains(t, result, "> โš ๏ธ Last error:") + assert.NotContains(t, result, "> โŒ Last error:") }) t.Run("pre-copy failure renders error line without a progress bar", func(t *testing.T) { result := render("1 of 2 tables failed", "preflight enumReorder check failed", 0) assert.Contains(t, result, "**`users`**: โŒ Failed (before row copy started)") - assert.Contains(t, result, "> โš ๏ธ Last error: preflight enumReorder check failed") + assert.Contains(t, result, "> โŒ Last error: preflight enumReorder check failed") assert.NotContains(t, result, "0%") }) } @@ -1333,7 +1333,10 @@ func TestRenderApplyStatusComment_Stopped(t *testing.T) { // Progress summary assert.Contains(t, result, "๐Ÿ“Š 1/2 complete") assert.Contains(t, result, "1 stopped") - assert.Contains(t, result, "remote apply remote-123 remained stopped after start grace period 30s") + // The heading already says the apply is stopped, so the error is context + // with the attention glyph, not a fresh failure. + assert.Contains(t, result, "> โš ๏ธ **Error:** remote apply remote-123 remained stopped after start grace period 30s") + assert.NotContains(t, result, "โŒ") assert.Contains(t, result, "schemabot start") } @@ -1995,7 +1998,7 @@ func TestRenderApplyBlockedByNonPassingChecks(t *testing.T) { result := RenderApplyBlockedByNonPassingChecks("staging", notPassing) - assert.Contains(t, result, "## โŒ Apply Blocked") + assert.Contains(t, result, "## โ›” Apply Blocked") assert.Contains(t, result, "โ€” Staging") assert.Contains(t, result, "Cannot apply while PR checks are not passing") assert.Contains(t, result, "| Check | Status |") @@ -2024,7 +2027,7 @@ func TestRenderApplyBlockedByNonPassingChecks_EmptyList(t *testing.T) { for _, notPassing := range [][]BlockingCheck{nil, {}} { result := RenderApplyBlockedByNonPassingChecks("staging", notPassing) - assert.Contains(t, result, "## โŒ Apply Blocked") + assert.Contains(t, result, "## โ›” Apply Blocked") assert.Contains(t, result, "โ€” Staging") assert.Contains(t, result, "Cannot apply while PR checks are not passing.") assert.Contains(t, result, "Get the checks passing โ€” fix failures and re-run cancelled or stale checks โ€” then retry:\n```\nschemabot apply -e staging\n```", @@ -2125,7 +2128,7 @@ func TestRenderApplyBlockedByPriorEnvCheckError(t *testing.T) { func TestRenderApplyBlockedByMissingPriorEnvCheck(t *testing.T) { result := RenderApplyBlockedByMissingPriorEnvCheck("staging") - assert.Contains(t, result, "## โŒ Apply Blocked") + assert.Contains(t, result, "## โ›” Apply Blocked") assert.Contains(t, result, "could not find a completed `staging` check") assert.Contains(t, result, "schemabot plan -e staging") assert.Contains(t, result, "apply `staging`") @@ -2135,7 +2138,7 @@ func TestRenderApplyBlockedByMissingPriorEnvCheck(t *testing.T) { func TestRenderApplyBlockedByUntrustedPriorEnvCheck(t *testing.T) { result := RenderApplyBlockedByUntrustedPriorEnvCheck("staging", "SchemaBot (staging)", []string{"schemabot-staging"}) - assert.Contains(t, result, "## โŒ Apply Blocked") + assert.Contains(t, result, "## โ›” Apply Blocked") assert.Contains(t, result, "`SchemaBot (staging)`") assert.Contains(t, result, "- `schemabot-staging`") assert.Contains(t, result, "does not trust") @@ -2144,6 +2147,27 @@ func TestRenderApplyBlockedByUntrustedPriorEnvCheck(t *testing.T) { assert.NotContains(t, result, "could not find a completed") } +// An environment missing from the promotion order is a configuration refusal: +// SchemaBot cannot place it in the staging-first sequence, and retrying +// unchanged refuses again, so the heading carries the refusal glyph and the +// body names the fix (add the environment to environment_order). +func TestRenderApplyBlockedByUnlistedEnvironment(t *testing.T) { + result := RenderApplyBlockedByUnlistedEnvironment("canary", []string{"staging", "production"}) + + assert.Contains(t, result, "## โ›” Apply Blocked โ€” Canary") + assert.Contains(t, result, "`canary` is not in the configured promotion order") + assert.Contains(t, result, "Configured promotion order: `staging` โ†’ `production`") + assert.Contains(t, result, "Add `canary` to `environment_order`") + + t.Run("empty promotion order omits the order line", func(t *testing.T) { + result := RenderApplyBlockedByUnlistedEnvironment("canary", nil) + + assert.Contains(t, result, "## โ›” Apply Blocked โ€” Canary") + assert.NotContains(t, result, "Configured promotion order") + assert.Contains(t, result, "Add `canary` to `environment_order`") + }) +} + func TestRenderApplyBlockedByInProgressChecks(t *testing.T) { inProgress := []BlockingCheck{ {Name: "CI / unit-tests", State: "in_progress"}, diff --git a/pkg/webhook/templates/blocked_test.go b/pkg/webhook/templates/blocked_test.go index b3d105f1c..1b6cf6c74 100644 --- a/pkg/webhook/templates/blocked_test.go +++ b/pkg/webhook/templates/blocked_test.go @@ -24,7 +24,7 @@ func TestRenderPlanComment_BlockedShownOnPlanAndApply(t *testing.T) { } plan := RenderPlanComment(data) - assert.Contains(t, plan, "โ›” **Cannot apply**: **1** change the schema-change engine refuses to execute") + assert.Contains(t, plan, "โ›” **Cannot apply**: 1 change the schema-change engine refuses to execute") assert.Contains(t, plan, "`users`: dropping primary key is not supported") assert.Contains(t, plan, "An apply will fail on these statements.") @@ -51,7 +51,7 @@ func TestRenderPlanComment_BlockedForeignKey(t *testing.T) { }, }) - assert.Contains(t, out, "โ›” **Cannot apply**: **1** change the schema-change engine refuses to execute") + assert.Contains(t, out, "โ›” **Cannot apply**: 1 change the schema-change engine refuses to execute") assert.Contains(t, out, "`orders`: adding foreign key constraints is not supported") assert.Contains(t, out, "An apply will fail on these statements.") } diff --git a/pkg/webhook/templates/common.go b/pkg/webhook/templates/common.go index 45ce6f99d..70ea38864 100644 --- a/pkg/webhook/templates/common.go +++ b/pkg/webhook/templates/common.go @@ -298,26 +298,32 @@ func quoteBlockLines(msg string) string { return strings.ReplaceAll(msg, "\n", "\n> ") } -// writeErrorBlock writes an error message as a blockquote with warning emoji. -// The message is sanitized before rendering; a message that sanitizes to -// empty writes nothing. -func writeErrorBlock(sb *strings.Builder, msg string) { +// writeErrorBlock writes an error message as a blockquote marked with the +// caller's severity glyph. The severity comes from the call site because the +// same error text carries different weight by apply state: glyph.Failed when +// the system has stopped and the operator's job is triage, glyph.Attention +// when SchemaBot is still acting on its own (retrying, stopped by request) +// and no triage is due. The message is sanitized before rendering; a message +// that sanitizes to empty writes nothing. +func writeErrorBlock(sb *strings.Builder, severity, msg string) { sanitized := sanitizeCommentError(msg) if sanitized == "" { return } - fmt.Fprintf(sb, "\n> โš ๏ธ **Error:** %s\n", quoteBlockLines(html.EscapeString(sanitized))) + fmt.Fprintf(sb, "\n> %s **Error:** %s\n", severity, quoteBlockLines(html.EscapeString(sanitized))) } // writeTableErrorLine writes a task's last error as a blockquote below its -// progress line. The message is sanitized before rendering; a message that -// sanitizes to empty writes nothing. -func writeTableErrorLine(sb *strings.Builder, msg string) { +// progress line, marked with the caller's severity glyph โ€” glyph.Failed for a +// task the system stopped on, glyph.Attention for one it is still retrying. +// The message is sanitized before rendering; a message that sanitizes to +// empty writes nothing. +func writeTableErrorLine(sb *strings.Builder, severity, msg string) { sanitized := sanitizeCommentError(msg) if sanitized == "" { return } - fmt.Fprintf(sb, "> โš ๏ธ Last error: %s\n", quoteBlockLines(html.EscapeString(sanitized))) + fmt.Fprintf(sb, "> %s Last error: %s\n", severity, quoteBlockLines(html.EscapeString(sanitized))) } // taskErrorAddsDetail reports whether a failed table's own error message adds diff --git a/pkg/webhook/templates/common_test.go b/pkg/webhook/templates/common_test.go index f344eb5df..1a14914c5 100644 --- a/pkg/webhook/templates/common_test.go +++ b/pkg/webhook/templates/common_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/block/schemabot/pkg/apitypes" + "github.com/block/schemabot/pkg/glyph" ) func TestSanitizeCommentError(t *testing.T) { @@ -125,19 +126,19 @@ func TestSanitizeCellError(t *testing.T) { func TestWriteErrorBlock(t *testing.T) { t.Run("multi-line error stays inside the blockquote", func(t *testing.T) { var sb strings.Builder - writeErrorBlock(&sb, "first line\nsecond line") - assert.Equal(t, "\n> โš ๏ธ **Error:** first line\n> second line\n", sb.String()) + writeErrorBlock(&sb, glyph.Failed, "first line\nsecond line") + assert.Equal(t, "\n> โŒ **Error:** first line\n> second line\n", sb.String()) }) t.Run("whitespace-only error writes nothing", func(t *testing.T) { var sb strings.Builder - writeErrorBlock(&sb, " \n ") + writeErrorBlock(&sb, glyph.Failed, " \n ") assert.Empty(t, sb.String()) }) t.Run("HTML markup is escaped so it renders as text", func(t *testing.T) { var sb strings.Builder - writeErrorBlock(&sb, "unexpected in output") + writeErrorBlock(&sb, glyph.Failed, "unexpected in output") assert.Contains(t, sb.String(), "<img src=x>") assert.NotContains(t, sb.String(), " but got ") + writeTableErrorLine(&sb, glyph.Failed, "expected but got ") assert.Contains(t, sb.String(), "<nil>") assert.NotContains(t, sb.String(), "") }) diff --git a/pkg/webhook/templates/direct_test.go b/pkg/webhook/templates/direct_test.go index afd1f3100..f98c343ef 100644 --- a/pkg/webhook/templates/direct_test.go +++ b/pkg/webhook/templates/direct_test.go @@ -24,7 +24,7 @@ func TestRenderPlanComment_DirectShownOnPlanAndApply(t *testing.T) { } plan := RenderPlanComment(data) - assert.Contains(t, plan, "โš™๏ธ **Direct execution**: **1** change will run as native MySQL DDL") + assert.Contains(t, plan, "โš™๏ธ **Direct execution**: 1 change will run as native MySQL DDL") assert.Contains(t, plan, "`users`: dropping primary key is not supported; runs as native MySQL DDL on a table with ~1,240 rows") assert.Contains(t, plan, "the change is **not revertible**") assert.Contains(t, plan, "`--defer-cutover` does not apply") @@ -121,7 +121,7 @@ func TestRenderBlockedChangesApplyRejected(t *testing.T) { }, }) - assert.Contains(t, out, "**โ›” Apply rejected**: **1** planned change the schema-change engine refuses to execute") + assert.Contains(t, out, "**โ›” Apply rejected**: 1 planned change the schema-change engine refuses to execute") assert.Contains(t, out, "`users`: dropping primary key is not supported") assert.Contains(t, out, "above the configured limit of 1,000,000") assert.Contains(t, out, "Fix what each reason names") diff --git a/pkg/webhook/templates/errors.go b/pkg/webhook/templates/errors.go index c504e1454..93eeec310 100644 --- a/pkg/webhook/templates/errors.go +++ b/pkg/webhook/templates/errors.go @@ -5,6 +5,8 @@ import ( "html" "strings" "text/template" + + "github.com/block/schemabot/pkg/glyph" ) // SchemaErrorData contains data for rendering schema request error comments. @@ -78,7 +80,7 @@ func (d SchemaErrorData) Attribution() string { return "*Requested by @" + d.RequestedBy + " at " + d.Timestamp + " UTC*" } -const databaseNotFoundTemplate = `## โš ๏ธ Database Not Found +const databaseNotFoundTemplate = "## " + glyph.Attention + ` Database Not Found **Database**: ` + "`{{.DatabaseName}}`" + `{{with .EnvironmentHeader}} | {{.}}{{end}} @@ -88,7 +90,7 @@ No ` + "`schemabot.yaml`" + ` configuration with ` + "`database: {{.DatabaseName Check that your ` + "`schemabot.yaml`" + ` file has the correct ` + "`database`" + ` field matching the ` + "`-d`" + ` flag value.` -const invalidConfigTemplate = `## โš ๏ธ No Valid SchemaBot Configuration Found +const invalidConfigTemplate = "## " + glyph.Attention + ` No Valid SchemaBot Configuration Found {{with .EnvironmentHeader}}{{.}} @@ -104,7 +106,7 @@ type: mysql - **database** (required): The database name - **type** (required): ` + "`vitess`" + ` or ` + "`mysql`" + `` -const noConfigNoDatabaseTemplate = `## โ„น๏ธ No SchemaBot Configuration Found +const noConfigNoDatabaseTemplate = "## " + glyph.Info + ` No SchemaBot Configuration Found {{with .EnvironmentHeader}}{{.}} @@ -127,7 +129,7 @@ Use the ` + "`-d`" + ` flag to specify which database to {{.CommandName}}: schemabot {{.CommandName}} -e {{.ExampleEnvironment}} -d ` + "```" + `` -const noConfigWithDatabaseTemplate = `## โ„น๏ธ No SchemaBot Configuration Found +const noConfigWithDatabaseTemplate = "## " + glyph.Info + ` No SchemaBot Configuration Found **Database**: ` + "`{{.DatabaseName}}`" + `{{with .EnvironmentHeader}} | {{.}}{{end}} @@ -143,7 +145,7 @@ database: {{.DatabaseName}} type: mysql ` + "```" + `` -const configOutsideAllowedDirsTemplate = `## โš ๏ธ SchemaBot Configuration Not Authorized +const configOutsideAllowedDirsTemplate = "## " + glyph.Attention + ` SchemaBot Configuration Not Authorized **Database**: ` + "`{{.DatabaseName}}`" + `{{with .EnvironmentHeader}} | {{.}}{{end}} @@ -155,7 +157,7 @@ SchemaBot found a ` + "`schemabot.yaml`" + ` configuration, but this SchemaBot i Ask a SchemaBot operator to add this directory to ` + "`databases.{{.DatabaseName}}.allowed_dirs`" + ` in the server config, or move the schema config and files under an allowed directory.` -const unmanagedSchemaConfigsNoticeTemplate = `## โš ๏ธ Schema Changes Not Managed by SchemaBot +const unmanagedSchemaConfigsNoticeTemplate = "## " + glyph.Attention + ` Schema Changes Not Managed by SchemaBot This PR changes schema under the following path(s), which this SchemaBot instance is not configured to manage: @@ -165,7 +167,7 @@ These schema changes will **not** be planned or applied, and the SchemaBot check If SchemaBot should manage them, ask a SchemaBot operator to add the directory to the database's ` + "`allowed_dirs`" + ` in the server config; otherwise remove these schema changes from this PR.` -const multipleConfigsTemplate = `## โš ๏ธ Multiple Databases Detected +const multipleConfigsTemplate = "## " + glyph.Attention + ` Multiple Databases Detected {{with .EnvironmentHeader}}{{.}} @@ -185,7 +187,7 @@ Use the ` + "`-d`" + ` flag: schemabot {{.CommandName}} -e {{.ExampleEnvironment}} -d ` + "```" + `` -const genericErrorTemplate = `## โŒ {{.CommandName}} Failed +const genericErrorTemplate = "## " + glyph.Failed + ` {{.CommandName}} Failed {{with .EnvironmentHeader}}{{.}} @@ -289,7 +291,7 @@ func RenderGenericError(data SchemaErrorData) string { // RenderInvalidCommand generates an error message for unrecognized commands. func RenderInvalidCommand() string { - return offerSupportChannel("## โŒ Invalid Command\n\nThat command wasn't recognized. Available commands:\n\n" + commandReference()) + return offerSupportChannel("## " + glyph.Failed + " Invalid Command\n\nThat command wasn't recognized. Available commands:\n\n" + commandReference()) } // RenderInvalidEnv generates an error message when the -e value does not name @@ -306,7 +308,7 @@ func RenderInvalidEnv(action string, available []string) string { if len(quoted) > 0 { availableLine = "\n**Available environments**: " + strings.Join(quoted, ", ") + "\n" } - return offerSupportChannel(fmt.Sprintf(`## โŒ Invalid Environment + return offerSupportChannel(fmt.Sprintf("## "+glyph.Failed+` Invalid Environment `+"`-e`"+` must name one of the configured environments. %s @@ -323,7 +325,7 @@ func markdownInlineCode(s string) string { // RenderMissingEnv generates an error message when -e flag is missing. func RenderMissingEnv(action string) string { - return offerSupportChannel(fmt.Sprintf(`## โŒ Missing Argument + return offerSupportChannel(fmt.Sprintf("## "+glyph.Failed+` Missing Argument You'll need to specify which environment to target with the `+"`-e`"+` flag. diff --git a/pkg/webhook/templates/existing_copy.go b/pkg/webhook/templates/existing_copy.go index a58dbc744..b20185426 100644 --- a/pkg/webhook/templates/existing_copy.go +++ b/pkg/webhook/templates/existing_copy.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/block/schemabot/pkg/engine" + "github.com/block/schemabot/pkg/glyph" "github.com/block/schemabot/pkg/ui" ) @@ -77,11 +78,11 @@ type ExistingCopyData struct { // Both keep the same verb so the two read as one disclosure rather than two. func writeDiscardedCopies(sb *strings.Builder, copies []ExistingCopyData, alreadyApplying bool) { n := len(copies) - marker, subject := "โš ๏ธ", "Applying" + marker, subject := glyph.Attention, "Applying" if alreadyApplying { - marker, subject = "โ„น๏ธ", "This apply" + marker, subject = glyph.Info, "This apply" } - fmt.Fprintf(sb, "%s **%s destroys work in progress**: **%d** unfinished %s on the target\n", + fmt.Fprintf(sb, "%s **%s destroys work in progress**: %d unfinished %s on the target\n", marker, subject, n, copyNoun(n)) writeExistingCopyEntries(sb, copies) if alreadyApplying { @@ -109,7 +110,7 @@ func writeAdoptedCopies(sb *strings.Builder, copies []ExistingCopyData, alreadyA if alreadyApplying { subject = "This apply picks" } - fmt.Fprintf(sb, "โ™ป๏ธ **Resuming work in progress**: **%d** unfinished %s on the target will be continued\n", + fmt.Fprintf(sb, "โ™ป๏ธ **Resuming work in progress**: %d unfinished %s on the target will be continued\n", n, copyNoun(n)) writeExistingCopyEntries(sb, copies) fmt.Fprintf(sb, "\n%s up where the existing %s stopped rather than starting over.\n\n", subject, copyNoun(n)) @@ -136,7 +137,7 @@ func writeRunningCopies(sb *strings.Builder, copies []ExistingCopyData, alreadyA if alreadyApplying { subject = "This apply joined" } - fmt.Fprintf(sb, "โ™ป๏ธ **Work already in progress**: **%d** unfinished %s still running on the target\n", + fmt.Fprintf(sb, "โ™ป๏ธ **Work already in progress**: %d unfinished %s still running on the target\n", n, copyNoun(n)) writeExistingCopyEntries(sb, copies) fmt.Fprintf(sb, "\n%s the %s already running rather than starting %s: every row copied so far is kept, and no second %s is made.\n\n", diff --git a/pkg/webhook/templates/existing_copy_test.go b/pkg/webhook/templates/existing_copy_test.go index f3b60fcb7..6e0612792 100644 --- a/pkg/webhook/templates/existing_copy_test.go +++ b/pkg/webhook/templates/existing_copy_test.go @@ -32,7 +32,7 @@ func TestRenderPlanComment_DiscardedCopyWarnsWhileTheDecisionIsTheOperators(t *t } plan := RenderPlanComment(data) - assert.Contains(t, plan, "โš ๏ธ **Applying destroys work in progress**: **1** unfinished copy on the target\n") + assert.Contains(t, plan, "โš ๏ธ **Applying destroys work in progress**: 1 unfinished copy on the target\n") assert.Contains(t, plan, "- `orders` in `testapp` (last progress 3h 12m ago): the schema change differs from the one that started it, "+ "which was `ALTER TABLE orders ADD INDEX idx_user_created (user_id, created_at)`", "a cause that is a comparison names the side the operator cannot see from the plan above") @@ -74,7 +74,7 @@ func TestRenderPlanComment_DiscardedCopyReadsAsARecordOnceApplying(t *testing.T) assert.Contains(t, out, "**Applying automatically**", "the fixture is the automatic path, which is what makes the section a record") - assert.Contains(t, out, "โ„น๏ธ **This apply destroys work in progress**: **1** unfinished copy on the target\n") + assert.Contains(t, out, "โ„น๏ธ **This apply destroys work in progress**: 1 unfinished copy on the target\n") assert.Contains(t, out, "- `orders` in `testapp` (last progress 3h 12m ago): the schema change differs from the one that started it") assert.NotContains(t, out, "โš ๏ธ **This apply destroys work in progress**", "a reader with no move to make is being informed, not warned") @@ -181,7 +181,7 @@ func TestRenderPlanComment_AdoptedCopyReadsAsContinuation(t *testing.T) { }, }) - assert.Contains(t, out, "โ™ป๏ธ **Resuming work in progress**: **1** unfinished copy on the target will be continued") + assert.Contains(t, out, "โ™ป๏ธ **Resuming work in progress**: 1 unfinished copy on the target will be continued") assert.Contains(t, out, "- `orders`, `products` in `testapp` (last progress 3h 12m ago)") assert.Contains(t, out, "Applying picks up where the existing copy stopped") assert.NotContains(t, out, "destroys work in progress", "an adopted copy is not a discard warning") @@ -212,7 +212,7 @@ func TestRenderPlanComment_AdoptedCopyReadsAsAnEventOnceApplying(t *testing.T) { assert.Contains(t, out, "**Applying automatically**", "the fixture is the automatic path, which is what makes both sections a record") assert.Contains(t, out, "โ„น๏ธ **This apply destroys work in progress**") - assert.Contains(t, out, "โ™ป๏ธ **Resuming work in progress**: **2** unfinished copies on the target will be continued") + assert.Contains(t, out, "โ™ป๏ธ **Resuming work in progress**: 2 unfinished copies on the target will be continued") assert.Contains(t, out, "This apply picks up where the existing copies stopped rather than starting over.") assert.NotContains(t, out, "Applying picks up", "the hypothetical subject belongs to a comment where applying is still a choice") @@ -236,7 +236,7 @@ func TestRenderPlanComment_RunningCopyReadsAsJoiningWorkInFlight(t *testing.T) { }, }) - assert.Contains(t, out, "โ™ป๏ธ **Work already in progress**: **1** unfinished copy still running on the target") + assert.Contains(t, out, "โ™ป๏ธ **Work already in progress**: 1 unfinished copy still running on the target") assert.Equal(t, "- `orders`, `products` in `testapp` (still copying)", entryLine(t, out, "- `orders`")) assert.Contains(t, out, "Applying joins the copy already running rather than starting a new one: "+ "every row copied so far is kept, and no second copy is made.") @@ -269,11 +269,11 @@ func TestRenderPlanComment_RunningAndStoppedCopiesAreDisclosedApart(t *testing.T }, }) - assert.Contains(t, out, "โ™ป๏ธ **Resuming work in progress**: **1** unfinished copy on the target will be continued") + assert.Contains(t, out, "โ™ป๏ธ **Resuming work in progress**: 1 unfinished copy on the target will be continued") assert.Equal(t, "- `orders` in `orders_a` (last progress 3h 12m ago)", entryLine(t, out, "- `orders` in"), "a copy that stopped is still dated by how stale it is") - assert.Contains(t, out, "โ™ป๏ธ **Work already in progress**: **2** unfinished copies still running on the target") + assert.Contains(t, out, "โ™ป๏ธ **Work already in progress**: 2 unfinished copies still running on the target") assert.Equal(t, "- `products` in `orders_b` (still copying)", entryLine(t, out, "- `products`")) assert.Contains(t, out, "This apply joined the copies already running rather than starting new ones", "an apply under way describes what it did, not what applying would do") @@ -300,7 +300,7 @@ func TestRenderPlanComment_RunningCopyKeepsItsHeartbeatOutOfTheDiscardWarning(t }, }) - assert.Contains(t, out, "โš ๏ธ **Applying destroys work in progress**: **2** unfinished copies on the target", + assert.Contains(t, out, "โš ๏ธ **Applying destroys work in progress**: 2 unfinished copies on the target", "a running copy the apply throws away is still a discard") assert.Equal(t, "- `orders` in `orders_a` (last progress 3h 12m ago): the schema change differs from the one that started it", entryLine(t, out, "- `orders` in"), @@ -324,7 +324,7 @@ func TestRenderPlanComment_DiscardedCopyNamesExpiryCause(t *testing.T) { }, }) - assert.Contains(t, out, "โš ๏ธ **Applying destroys work in progress**: **1** unfinished copy on the target\n") + assert.Contains(t, out, "โš ๏ธ **Applying destroys work in progress**: 1 unfinished copy on the target\n") assert.Contains(t, out, "- `orders` in `testapp` (last progress 9d 4h ago): it is too old to resume") assert.NotContains(t, out, "9d 4h of copying", "an expired checkpoint is stale by construction; naming it as copying time contradicts the cause below it") @@ -359,7 +359,7 @@ func TestRenderPlanComment_SeveralDiscardedCopiesReadAsPlural(t *testing.T) { }, }) - assert.Contains(t, out, "โš ๏ธ **Applying destroys work in progress**: **2** unfinished copies on the target\n") + assert.Contains(t, out, "โš ๏ธ **Applying destroys work in progress**: 2 unfinished copies on the target\n") assert.Contains(t, out, "- `orders` in `orders_a` (last progress 3h 12m ago): the schema change differs from the one that started it") assert.Contains(t, out, "- `orders` in `orders_b` (last progress 9d 4h ago): it is too old to resume") assert.Contains(t, out, "Applying restarts the copies from zero rows.") @@ -381,7 +381,7 @@ func TestRenderPlanComment_DiscardedCopyWithoutAge(t *testing.T) { }, }) - assert.Contains(t, out, "โš ๏ธ **Applying destroys work in progress**: **1** unfinished copy on the target\n") + assert.Contains(t, out, "โš ๏ธ **Applying destroys work in progress**: 1 unfinished copy on the target\n") assert.Contains(t, out, "- `orders` in `testapp`: the schema change differs from the one that started it", "an unknown age is omitted rather than rendered as a bare zero, which would read as a copy that just started") assert.Contains(t, out, "To keep the work already done, apply the schema change that started it.") @@ -418,7 +418,7 @@ func TestRenderMultiEnvPlanComment_IdenticalDDLStillDisclosesOneEnvironmentsCopy assert.Contains(t, out, "### Production\n", "the environment holding the copy keeps its own section") assert.NotContains(t, out, "### Staging & Production", "one section cannot speak for two targets when only one of them holds discardable work") - assert.Contains(t, out, "โš ๏ธ **Applying destroys work in progress**: **1** unfinished copy on the target") + assert.Contains(t, out, "โš ๏ธ **Applying destroys work in progress**: 1 unfinished copy on the target") assert.Contains(t, out, "- `orders` in `testapp` (last progress 9h 40m ago): the schema change differs from the one that started it") } @@ -477,7 +477,7 @@ func TestRenderMultiEnvPlanComment_IdenticalDDLStillDisclosesOneEnvironmentsRunn assert.Contains(t, out, "### Production\n", "the environment holding the running copy keeps its own section") assert.NotContains(t, out, "### Staging & Production", "one section cannot speak for two targets when only one of them holds live work") - assert.Contains(t, out, "โ™ป๏ธ **Work already in progress**: **1** unfinished copy still running on the target") + assert.Contains(t, out, "โ™ป๏ธ **Work already in progress**: 1 unfinished copy still running on the target") assert.Contains(t, out, "- `orders` in `testapp` (still copying)") } diff --git a/pkg/webhook/templates/lint_test.go b/pkg/webhook/templates/lint_test.go index ae9a40dc3..7153a2e9b 100644 --- a/pkg/webhook/templates/lint_test.go +++ b/pkg/webhook/templates/lint_test.go @@ -27,7 +27,7 @@ func TestRenderPlanComment_LintInlineBelowFoldThreshold(t *testing.T) { {Message: `Using "varchar" as primary key is discouraged`, Table: "sessions"}, })) - assert.Contains(t, plan, "๐Ÿ’ก **Lint Warnings**: **2** advisory findings") + assert.Contains(t, plan, "๐Ÿ’ก **Lint Warnings**: 2 advisory findings") assert.Contains(t, plan, "- `users`: Index `idx_email` should be made invisible before dropping") assert.Contains(t, plan, "- `sessions`: Using `varchar` as primary key is discouraged") assert.NotContains(t, plan, "๐Ÿ’ก", "a short list never hides behind a fold") @@ -47,7 +47,7 @@ func TestRenderPlanComment_LintFoldsAndGroupsAboveThreshold(t *testing.T) { {Message: "third users finding", Table: "users"}, })) - assert.Contains(t, plan, "๐Ÿ’ก Lint Warnings: 6 advisory findings") + assert.Contains(t, plan, "๐Ÿ’ก Lint Warnings: 6 advisory findings") assert.Contains(t, plan, "**`users`**\n- first users finding\n- second users finding\n- third users finding") assert.Contains(t, plan, "**`sessions`**\n- first sessions finding\n- second sessions finding") diff --git a/pkg/webhook/templates/multi_apply.go b/pkg/webhook/templates/multi_apply.go index 356aaf7df..8dac37b91 100644 --- a/pkg/webhook/templates/multi_apply.go +++ b/pkg/webhook/templates/multi_apply.go @@ -5,6 +5,7 @@ import ( "html" "strings" + "github.com/block/schemabot/pkg/glyph" "github.com/block/schemabot/pkg/presentation" "github.com/block/schemabot/pkg/state" "github.com/block/schemabot/pkg/ui" @@ -163,10 +164,10 @@ func writeAggregateFirstFailure(sb *strings.Builder, failure *presentation.Deplo name := html.EscapeString(failure.Deployment) msg := SanitizeInlineError(failure.Error) if msg == "" { - fmt.Fprintf(sb, "\n> โš ๏ธ **First failure:** %s\n", name) + fmt.Fprintf(sb, "\n> "+glyph.Failed+" **First failure:** %s\n", name) return } - fmt.Fprintf(sb, "\n> โš ๏ธ **First failure:** %s โ€” %s\n", name, html.EscapeString(msg)) + fmt.Fprintf(sb, "\n> "+glyph.Failed+" **First failure:** %s โ€” %s\n", name, html.EscapeString(msg)) } // writeAggregateNextAction renders the single suggested operator action derived diff --git a/pkg/webhook/templates/multi_apply_test.go b/pkg/webhook/templates/multi_apply_test.go index ec1ee413f..93b5ed99c 100644 --- a/pkg/webhook/templates/multi_apply_test.go +++ b/pkg/webhook/templates/multi_apply_test.go @@ -97,7 +97,7 @@ func TestRenderMultiDeploymentApplyComment_FailedHalt(t *testing.T) { assert.Contains(t, out, "
\nโธ au โ€” halted โ€” us failed") // With no error detail on the failed operation, the first-failure line names // the deployment without a reason. - assert.Contains(t, out, "> โš ๏ธ **First failure:** us\n") + assert.Contains(t, out, "> โŒ **First failure:** us\n") } func TestRenderMultiDeploymentApplyComment_UsesOneRenderTimestamp(t *testing.T) { @@ -183,7 +183,7 @@ func TestRenderMultiDeploymentApplyComment_FirstFailureSurfacesError(t *testing. assert.NotContains(t, out, "Schema Change Failed") // A later deployment is still running while siblings have failed. assert.Contains(t, out, "- ๐Ÿ”„ au โ€” running table copy") - assert.Contains(t, out, "> โš ๏ธ **First failure:** us โ€” Error 1061: Duplicate key name idx\n") + assert.Contains(t, out, "> โŒ **First failure:** us โ€” Error 1061: Duplicate key name idx\n") // Only the earliest failure is lifted to the header. assert.NotContains(t, out, "First failure:** eu") } @@ -217,7 +217,7 @@ func TestRenderMultiDeploymentApplySummaryComment_FirstFailureSurfacesError(t *t Environment: "production", }) - assert.Contains(t, out, "> โš ๏ธ **First failure:** us โ€” boom <script>\n") + assert.Contains(t, out, "> โŒ **First failure:** us โ€” boom <script>\n") } // A deployment name with HTML-significant characters is escaped inside the @@ -233,7 +233,7 @@ func TestRenderMultiDeploymentApplyComment_FirstFailureEscapesName(t *testing.T) Environment: "production", }) - assert.Contains(t, out, "> โš ๏ธ **First failure:** us&ca โ€” boom\n") + assert.Contains(t, out, "> โŒ **First failure:** us&ca โ€” boom\n") } // Each deployment's
body is rendered by the single-deployment renderer, @@ -602,6 +602,6 @@ func TestRenderMultiDeploymentApplyComment_FirstFailureErrorSanitized(t *testing }) assert.NotContains(t, out, "db-primary.internal", "internal endpoints are redacted") - assert.Contains(t, out, "> โš ๏ธ **First failure:** us โ€” dial tcp [endpoint redacted]: refused second line\n", + assert.Contains(t, out, "> โŒ **First failure:** us โ€” dial tcp [endpoint redacted]: refused second line\n", "the first-failure line stays on one line") } diff --git a/pkg/webhook/templates/plan.go b/pkg/webhook/templates/plan.go index dd079105b..b19bd3641 100644 --- a/pkg/webhook/templates/plan.go +++ b/pkg/webhook/templates/plan.go @@ -8,6 +8,7 @@ import ( "github.com/block/schemabot/pkg/caller" "github.com/block/schemabot/pkg/ddl" + "github.com/block/schemabot/pkg/glyph" "github.com/block/schemabot/pkg/storage" "github.com/block/schemabot/pkg/ui" ) @@ -360,7 +361,7 @@ func RenderPlanComment(data PlanCommentData) string { if !data.applyingWithoutConfirmation() { // Automatic apply was downgraded to manual confirmation โ€” show unlock since user needs to act - fmt.Fprintf(&sb, "โš ๏ธ **%s**: %s\n\n", data.downgradeHeading(), data.AutoConfirmDowngradeReason) + fmt.Fprintf(&sb, glyph.Attention+" **%s**: %s\n\n", data.downgradeHeading(), data.AutoConfirmDowngradeReason) sb.WriteString("Review the plan above, then confirm manually:\n") fmt.Fprintf(&sb, "```\n%s\n```\n", applyConfirmCmd) sb.WriteString("\n๐Ÿ”“ To discard this plan and unlock, comment:\n") @@ -422,7 +423,7 @@ func attributionStillActionable(data PlanCommentData) bool { // that last changed it, never the specific column or index. func writeAttributedChanges(sb *strings.Builder, changes []AttributedChangeData) { n := len(changes) - fmt.Fprintf(sb, "๐Ÿ›‘ **Check before applying**: **%d** %s SchemaBot cannot attribute to this PR\n", n, pluralize("destructive change", n)) + fmt.Fprintf(sb, "๐Ÿ›‘ **Check before applying**: %d %s SchemaBot cannot attribute to this PR\n", n, pluralize("destructive change", n)) for _, d := range changes { if d.Unresolved { fmt.Fprintf(sb, "- `%s`: ownership could not be established; see server logs\n", d.Table) @@ -569,7 +570,7 @@ func writeIgnoredNamespaces(sb *strings.Builder, ignored []string) { for i, ns := range ignored { quoted[i] = fmt.Sprintf("`%s`", ns) } - fmt.Fprintf(sb, "โ„น๏ธ Namespaces excluded from this plan by `ignore_namespaces`: %s\n\n", strings.Join(quoted, ", ")) + fmt.Fprintf(sb, glyph.Info+" Namespaces excluded from this plan by `ignore_namespaces`: %s\n\n", strings.Join(quoted, ", ")) } // multiEnvHasIgnoredNamespaces reports whether any environment's plan excluded @@ -623,14 +624,14 @@ func writeMultiEnvIgnoredNamespaces(sb *strings.Builder, data MultiEnvPlanCommen for i, ns := range plan.IgnoredNamespaces { quoted[i] = fmt.Sprintf("`%s`", ns) } - fmt.Fprintf(sb, "โ„น๏ธ **%s**: namespaces excluded from this plan by `ignore_namespaces`: %s\n\n", capitalizeFirst(env), strings.Join(quoted, ", ")) + fmt.Fprintf(sb, glyph.Info+" **%s**: namespaces excluded from this plan by `ignore_namespaces`: %s\n\n", capitalizeFirst(env), strings.Join(quoted, ", ")) } } func writeNoChangesDetected(sb *strings.Builder, data PlanCommentData) { sb.WriteString("โœ… **No schema changes detected**\n") if data.RecoveredApplyOwnedCheckState { - sb.WriteString("\nโ„น๏ธ SchemaBot found stored PR check state for this database/environment that was still marked as an apply in progress. Because this fresh plan shows the target schema already matches this PR, SchemaBot updated the PR check to passing.\n") + sb.WriteString("\n" + glyph.Info + " SchemaBot found stored PR check state for this database/environment that was still marked as an apply in progress. Because this fresh plan shows the target schema already matches this PR, SchemaBot updated the PR check to passing.\n") } } @@ -856,7 +857,7 @@ func writeDeploymentDrift(sb *strings.Builder, drift *DeploymentDriftData) { } if !drift.Computed { - sb.WriteString("โš ๏ธ **Could not verify deployment drift** โ€” the plan check is failing closed until it can be confirmed.\n\n") + sb.WriteString(glyph.Attention + " **Could not verify deployment drift** โ€” the plan check is failing closed until it can be confirmed.\n\n") return } @@ -866,7 +867,7 @@ func writeDeploymentDrift(sb *strings.Builder, drift *DeploymentDriftData) { return } - sb.WriteString("โš ๏ธ **Deployment drift detected** โ€” some deployments no longer match the reviewed plan, so the plan check is failing closed:\n\n") + sb.WriteString(glyph.Attention + " **Deployment drift detected** โ€” some deployments no longer match the reviewed plan, so the plan check is failing closed:\n\n") for _, d := range drift.Deployments { name := "`" + d.Deployment + "`" if d.Primary { @@ -876,9 +877,9 @@ func writeDeploymentDrift(sb *strings.Builder, drift *DeploymentDriftData) { case "match": fmt.Fprintf(sb, "- %s โœ… matches the reviewed plan\n", name) case "diverged": - fmt.Fprintf(sb, "- %s โš ๏ธ diverged%s\n", name, driftDetailSuffix(d.Detail)) + fmt.Fprintf(sb, "- %s "+glyph.Attention+" diverged%s\n", name, driftDetailSuffix(d.Detail)) default: - fmt.Fprintf(sb, "- %s โŒ could not verify%s\n", name, driftDetailSuffix(d.Detail)) + fmt.Fprintf(sb, "- %s "+glyph.Failed+" could not verify%s\n", name, driftDetailSuffix(d.Detail)) } } sb.WriteString("\n") @@ -908,7 +909,7 @@ func joinDeploymentNames(deployments []DeploymentDriftEntry) string { // unsupported shape needs a rewrite, a missing grant needs provisioning. func writeBlockedChanges(sb *strings.Builder, changes []BlockedChangeData) { n := len(changes) - fmt.Fprintf(sb, "โ›” **Cannot apply**: **%d** %s the schema-change engine refuses to execute\n", n, pluralize("change", n)) + fmt.Fprintf(sb, glyph.Refused+" **Cannot apply**: %d %s the schema-change engine refuses to execute\n", n, pluralize("change", n)) for _, c := range changes { table := "`" + c.Table + "`" if len(c.Shards) > 0 { @@ -951,7 +952,7 @@ func directConsentCopy(databaseType string, isMySQL bool) (headerNoun, footer st func writeDirectChanges(sb *strings.Builder, changes []DirectChangeData, databaseType string, isMySQL bool) { headerNoun, footer := directConsentCopy(databaseType, isMySQL) n := len(changes) - fmt.Fprintf(sb, "โš™๏ธ **Direct execution**: **%d** %s will run as %s\n", n, pluralize("change", n), headerNoun) + fmt.Fprintf(sb, "โš™๏ธ **Direct execution**: %d %s will run as %s\n", n, pluralize("change", n), headerNoun) for _, c := range changes { table := "`" + c.Table + "`" if len(c.Shards) > 0 { @@ -968,7 +969,7 @@ func writeDirectChanges(sb *strings.Builder, changes []DirectChangeData, databas func writeUnsafeWarning(sb *strings.Builder, changes []UnsafeChangeData, isMySQL bool) { n := countUnsafeFindings(changes) - fmt.Fprintf(sb, "โš ๏ธ **Issues**: **%d** unsafe %s detected\n", n, pluralize("change", n)) + fmt.Fprintf(sb, glyph.Attention+" **Issues**: %d unsafe %s detected\n", n, pluralize("change", n)) for _, c := range changes { table := "`" + c.Table + "`" if len(c.Shards) > 0 { @@ -1102,7 +1103,7 @@ func writeLintViolations(sb *strings.Builder, warnings []LintViolationData) { n := len(warnings) if n <= lintWarningsFoldThreshold { - fmt.Fprintf(sb, "\U0001f4a1 **Lint Warnings**: **%d** advisory %s\n", n, pluralize("finding", n)) + fmt.Fprintf(sb, "\U0001f4a1 **Lint Warnings**: %d advisory %s\n", n, pluralize("finding", n)) for _, w := range warnings { message := ui.CodeQuoteIdentifiers(w.Message) if w.Table != "" { @@ -1117,7 +1118,7 @@ func writeLintViolations(sb *strings.Builder, warnings []LintViolationData) { // GitHub renders content as HTML, not markdown, so the folded // header bolds with tags instead of asterisks. - fmt.Fprintf(sb, "
\n\U0001f4a1 Lint Warnings: %d advisory %s\n\n", n, pluralize("finding", n)) + fmt.Fprintf(sb, "
\n\U0001f4a1 Lint Warnings: %d advisory %s\n\n", n, pluralize("finding", n)) for _, group := range groupLintWarningsByTable(warnings) { if group.table != "" { fmt.Fprintf(sb, "**`%s`**\n", group.table) @@ -1267,7 +1268,7 @@ func RenderMultiEnvPlanComment(data MultiEnvPlanCommentData) string { fmt.Fprintf(&sb, "### %s\n\n", capitalizeFirst(env)) if errMsg, hasErr := data.Errors[env]; hasErr { - writeErrorBlock(&sb, errMsg) + writeErrorBlock(&sb, glyph.Failed, errMsg) sb.WriteString("\n") continue } @@ -1491,7 +1492,7 @@ func writeMultiEnvFooter(sb *strings.Builder, data MultiEnvPlanCommentData) { if len(envsWithErrors) > 0 { sb.WriteString("\n") for _, env := range envsWithErrors { - fmt.Fprintf(sb, "โš ๏ธ **%s** failed to plan. Resolve the error above and re-run:\n", capitalizeFirst(env)) + fmt.Fprintf(sb, glyph.Attention+" **%s** failed to plan. Resolve the error above and re-run:\n", capitalizeFirst(env)) fmt.Fprintf(sb, "```\n%s\n```\n", tenantCommand("schemabot plan", env, data.Tenant)) } } diff --git a/pkg/webhook/templates/reconciliation.go b/pkg/webhook/templates/reconciliation.go index db8612d49..05d53df9b 100644 --- a/pkg/webhook/templates/reconciliation.go +++ b/pkg/webhook/templates/reconciliation.go @@ -3,6 +3,8 @@ package templates import ( "fmt" "strings" + + "github.com/block/schemabot/pkg/glyph" ) // SchemaChangeReconciliationData contains the apply-owned PR state that must be @@ -71,7 +73,7 @@ func RenderNoManagedSchemaChangesChecksRefreshed(data NoManagedSchemaChangesChec // longer contains a schema change whose apply has already started. func RenderSchemaChangeReconciliationRequired(data SchemaChangeReconciliationData) string { var sb strings.Builder - sb.WriteString("## โš ๏ธ Schema Change Reconciliation Required\n\n") + sb.WriteString("## " + glyph.Attention + " Schema Change Reconciliation Required\n\n") writeReconciliationMetadata(&sb, data.Items) writeRequestedLine(&sb, data.RequestedBy, data.Timestamp) sb.WriteString("\n") diff --git a/pkg/webhook/templates/sharded_apply.go b/pkg/webhook/templates/sharded_apply.go index 1aa4349d9..790177ebd 100644 --- a/pkg/webhook/templates/sharded_apply.go +++ b/pkg/webhook/templates/sharded_apply.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/block/schemabot/pkg/apitypes" + "github.com/block/schemabot/pkg/glyph" "github.com/block/schemabot/pkg/state" ) @@ -315,11 +316,12 @@ func writeShardedFailure(sb *strings.Builder, data ShardedApplyData) { if !isShardFailureState(s.State) { continue } + severity := failureSeverity(s.State == state.ApplyOperation.FailedRetryable) shard := html.EscapeString(s.Shard) if msg := SanitizeInlineError(s.Error); msg == "" { - fmt.Fprintf(sb, "\n> โš ๏ธ **First failure:** shard %s\n", shard) + fmt.Fprintf(sb, "\n> %s **First failure:** shard %s\n", severity, shard) } else { - fmt.Fprintf(sb, "\n> โš ๏ธ **First failure:** shard %s โ€” %s\n", shard, html.EscapeString(msg)) + fmt.Fprintf(sb, "\n> %s **First failure:** shard %s โ€” %s\n", severity, shard, html.EscapeString(msg)) } return } @@ -327,10 +329,20 @@ func writeShardedFailure(sb *strings.Builder, data ShardedApplyData) { return } if msg := SanitizeInlineError(data.ErrorMessage); msg != "" { - fmt.Fprintf(sb, "\n> โš ๏ธ **Failure:** %s\n", html.EscapeString(msg)) + fmt.Fprintf(sb, "\n> %s **Failure:** %s\n", failureSeverity(state.IsState(data.State, state.Apply.FailedRetryable)), html.EscapeString(msg)) } } +// failureSeverity returns the glyph for a surfaced failure: glyph.Attention +// while SchemaBot is still retrying on its own โ€” nothing has stopped and no +// triage is due โ€” and glyph.Failed once the system has stopped on the error. +func failureSeverity(retrying bool) string { + if retrying { + return glyph.Attention + } + return glyph.Failed +} + // writeShardStatusTable renders the per-shard status table for a set of shards. func writeShardStatusTable(sb *strings.Builder, shards []ShardStatus) { if len(shards) == 0 { diff --git a/pkg/webhook/templates/sharded_apply_test.go b/pkg/webhook/templates/sharded_apply_test.go index 4363a2744..09e5a8ab8 100644 --- a/pkg/webhook/templates/sharded_apply_test.go +++ b/pkg/webhook/templates/sharded_apply_test.go @@ -47,7 +47,7 @@ func TestRenderShardedApplyComment_FailedSurfacesError(t *testing.T) { }) assert.Contains(t, out, "## Schema Change Status") - assert.Contains(t, out, "> โš ๏ธ **First failure:** shard -40 โ€” "+failErr) + assert.Contains(t, out, "> โŒ **First failure:** shard -40 โ€” "+failErr) assert.Contains(t, out, failErr, "the error also appears in the failed shard's row") assert.Contains(t, out, "To retry:") } @@ -66,8 +66,9 @@ func TestRenderShardedApplyComment_FailedRetryableSurfacesErrorAndStop(t *testin Cells: []ShardCell{mutesCell("-40"), mutesCell("80-")}, }) - assert.Contains(t, out, "First failure:", "a retrying shard's error is still lifted") - assert.Contains(t, out, retryErr, "the retrying shard's error is shown, not dropped") + assert.Contains(t, out, "> โš ๏ธ **First failure:** shard -40 โ€” "+retryErr, + "a retrying shard's error is still lifted, with the attention glyph โ€” SchemaBot is retrying, nothing has stopped") + assert.NotContains(t, out, "โŒ", "no failure glyph while SchemaBot retries on its own") assert.Contains(t, out, "To stop retrying:") assert.Contains(t, out, "schemabot stop apply-x") } @@ -153,7 +154,7 @@ func TestRenderShardedApplyComment_FailedErrorSanitized(t *testing.T) { }) assert.NotContains(t, out, "db-primary.internal", "internal endpoints are redacted") - assert.Contains(t, out, "> โš ๏ธ **First failure:** shard -40 โ€” dial tcp [endpoint redacted]: connect refused retry | later\n", + assert.Contains(t, out, "> โŒ **First failure:** shard -40 โ€” dial tcp [endpoint redacted]: connect refused retry | later\n", "the first-failure line stays on one line") assert.Contains(t, out, "| `-40` | โŒ failed โ€” dial tcp [endpoint redacted]: connect refused retry / later |", "the status cell neutralizes the cell separator") @@ -200,7 +201,7 @@ func TestRenderShardedApplySummaryComment_FailedSurfacesErrorAndRetry(t *testing assert.Contains(t, out, "## โŒ Schema Change Failed โ€” Staging") assert.NotContains(t, out, "Applied successfully", "a failed apply writes no success line") - assert.Contains(t, out, "> โš ๏ธ **First failure:** shard -40 โ€” "+failErr) + assert.Contains(t, out, "> โŒ **First failure:** shard -40 โ€” "+failErr) assert.Contains(t, out, "| `80-` | โธ halted โ€” -40 failed |", "halted siblings keep their final state in the results") assert.Contains(t, out, "To retry:") } @@ -255,7 +256,7 @@ func TestRenderShardedApplySummaryComment_FailureOutsideShardWorkSurfacesApplyEr }) assert.Contains(t, out, "## โŒ Schema Change Failed โ€” Staging") - assert.Contains(t, out, "> โš ๏ธ **Failure:** finalize vschema: dial tcp [endpoint redacted]: connect refused retry | later second line", + assert.Contains(t, out, "> โŒ **Failure:** finalize vschema: dial tcp [endpoint redacted]: connect refused retry | later second line", "the apply-level error is surfaced when no shard failed, sanitized to one line") assert.NotContains(t, out, "db-primary.internal", "internal endpoints never render in PR comments") assert.NotContains(t, out, "First failure:", "no shard failed, so there is no shard failure callout") @@ -275,7 +276,7 @@ func TestRenderShardedApplyComment_ShardFailureOwnsCallout(t *testing.T) { Cells: []ShardCell{mutesCell("-40")}, }) - assert.Contains(t, out, "> โš ๏ธ **First failure:** shard -40 โ€” "+failErr) + assert.Contains(t, out, "> โŒ **First failure:** shard -40 โ€” "+failErr) assert.NotContains(t, out, "**Failure:** apply failed") } @@ -371,6 +372,24 @@ func TestRenderShardedApplyComment_FailureOutsideShardWorkSurfacesApplyError(t * }) assert.Contains(t, out, "## Schema Change Status โ€” Staging") - assert.Contains(t, out, "> โš ๏ธ **Failure:** finalize vschema: apply vschema to keyspace: context deadline exceeded") + assert.Contains(t, out, "> โŒ **Failure:** finalize vschema: apply vschema to keyspace: context deadline exceeded") assert.NotContains(t, out, "First failure:", "no shard failed, so there is no shard failure callout") } + +// An apply-level error on a still-retrying apply is surfaced with the +// attention glyph, not the failure glyph โ€” SchemaBot is retrying on its own, +// so nothing has stopped and no triage is due yet. +func TestRenderShardedApplyComment_RetryingApplyErrorCarriesAttentionGlyph(t *testing.T) { + out := RenderShardedApplyComment(ShardedApplyData{ + State: state.Apply.FailedRetryable, Environment: "staging", Database: "cdb_resolute", + Keyspace: "cdb_resolute_sharded", ApplyID: "apply-x", + ErrorMessage: "finalize vschema: apply vschema to keyspace: context deadline exceeded", + Shards: []ShardStatus{ + {Shard: "-40", Emoji: "โœ…", Label: "completed", State: state.ApplyOperation.Completed}, + }, + Cells: []ShardCell{mutesCell("-40")}, + }) + + assert.Contains(t, out, "> โš ๏ธ **Failure:** finalize vschema: apply vschema to keyspace: context deadline exceeded") + assert.NotContains(t, out, "โŒ", "no failure glyph while SchemaBot retries on its own") +} diff --git a/pkg/webhook/templates/sharded_plan_test.go b/pkg/webhook/templates/sharded_plan_test.go index c287a9a57..c1050623d 100644 --- a/pkg/webhook/templates/sharded_plan_test.go +++ b/pkg/webhook/templates/sharded_plan_test.go @@ -51,12 +51,12 @@ func TestRenderPlanComment_UnsafeShownOnPlanNotOnApply(t *testing.T) { } plan := RenderPlanComment(data) - assert.Contains(t, plan, "**Issues**: **1** unsafe change detected", "the plan comment surfaces unsafe changes for review") + assert.Contains(t, plan, "**Issues**: 1 unsafe change detected", "the plan comment surfaces unsafe changes for review") assert.Contains(t, plan, "DROP COLUMN is destructive") data.IsLocked = true apply := RenderPlanComment(data) - assert.NotContains(t, apply, "**Issues**: **1** unsafe change detected", "the locked apply comment omits the unsafe warning as noise") + assert.NotContains(t, apply, "**Issues**: 1 unsafe change detected", "the locked apply comment omits the unsafe warning as noise") assert.NotContains(t, apply, "DROP COLUMN is destructive") assert.NotContains(t, apply, "Destructive drop guidance", "the drop guidance rides inside the unsafe block and is omitted with it") assert.Contains(t, apply, "DROP COLUMN `email`", "the DDL itself stays visible on the apply comment") @@ -172,7 +172,7 @@ func TestRenderPlanComment_UnsafeShardChangeShowsShard(t *testing.T) { }}, }) - assert.Contains(t, out, "**Issues**: **1** unsafe change detected") + assert.Contains(t, out, "**Issues**: 1 unsafe change detected") assert.Contains(t, out, "`mutes` (shard `40-80`)", "the unsafe change names the shard it applies to") assert.Contains(t, out, "DROP COLUMN `x`", "the drop is shown in that shard's combined ALTER") }