Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 42 additions & 42 deletions TEMPLATES.md

Large diffs are not rendered by default.

18 changes: 14 additions & 4 deletions docs/lint-and-safety-levels.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
Expand All @@ -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:

Expand All @@ -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.
4 changes: 2 additions & 2 deletions pkg/webhook/apply_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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")
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/webhook/plan_change_ownership_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
14 changes: 7 additions & 7 deletions pkg/webhook/plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand All @@ -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`:")
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/webhook/sharded_apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
41 changes: 26 additions & 15 deletions pkg/webhook/templates/apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -1581,7 +1592,7 @@ func groupStateEmoji(tables []TableProgressData) string {
}

if states[state.Task.Failed] {
return "❌"
return glyph.Failed
}
Comment thread
aparajon marked this conversation as resolved.
if states["reverted"] {
return "↩️"
Expand Down
Loading
Loading