fix(github): align PR comment severity glyphs with the shared vocabulary

[aparajon]

Why this matters

The PR-comment surface spells every severity glyph inline, so the same situation reads differently from comment to comment: a recorded failure renders ⚠️ — one notch below a plan-time warning — safety-gate refusals split between ❌ and ⛔, and the unsafe-changes rejection heading pins ⛔ to the changes themselves rather than the refusal. Operators triage urgency from these glyphs, and pkg/glyph already defines the one-glyph-per-meaning vocabulary the CLI surface uses. This PR moves the PR-comment surface onto it and corrects the sites whose glyph said the wrong thing.

What it does

Migrates every raw severity glyph in pkg/webhook/templates to the pkg/glyph constants, and fixes the misleads:

• The unsafe-changes rejection heading attaches ⛔ to the refusal: **⛔ Apply rejected**: N unsafe changes detected, matching the engine-blocked rejection heading, instead of labeling the changes.
• Error callouts carry the glyph of the failure's standing. The > **Error:** and > Last error: quotes render ❌ once the system has stopped on the failure and triage is due; while SchemaBot is still retrying on its own, or the apply was stopped by request, they render ⚠️ — nothing has stopped that an operator must triage.
• First-failure callouts follow the same rule in multi-deployment and sharded progress comments: ❌ when they point at a deployment/shard the system stopped on, ⚠️ when SchemaBot is still retrying that shard or apply.
• The Apply Blocked headings where retrying unchanged refuses again move from ❌ to ⛔. Prior-environment gate, non-passing PR checks, missing/untrusted prior-environment check, unlisted environment — all are refusals by a safety gate, matching the Apply Blocked: PR Is Merged/Closed headings that already carried ⛔. Check-read errors that fail closed keep ❌: an unchanged retry can succeed, so they are failed verifications, not refusals — the same rendering the required-checks review gate uses for that error class.
• Heading counts are no longer bolded. Section headings already carry a bold label, so the bold count was double emphasis; the plan summary keeps its bold counts, where the number is the line's only emphasis.

Everything else (escalation, info notices, config-error templates) keeps its rendering and now references the constants. Cancelled/stopped state glyphs are a separate vocabulary and are untouched here; the iconography table in docs/lint-and-safety-levels.md now names which icons are severities and which are states.

Apply rejected over unsafe changes — before / after

Before:

⛔ 3 Unsafe Changes Detected:

• users: DROP COLUMN email
• orders: DROP TABLE
• products: MODIFY COLUMN price_cents

🚨 To proceed with these destructive changes, re-run with --allow-unsafe:

After:

⛔ Apply rejected: 3 unsafe changes detected

• users: DROP COLUMN email
• orders: DROP TABLE
• products: MODIFY COLUMN price_cents

🚨 To proceed with these destructive changes, re-run with --allow-unsafe:

Failed apply error callouts — before / after

Before:

⚠️ Error: table orders failed: preflight enumReorder check failed

⚠️ Last error: lock wait timeout exceeded; try restarting transaction

After (apply failed — system stopped):

❌ Error: table orders failed: preflight enumReorder check failed

❌ Last error: lock wait timeout exceeded; try restarting transaction

Unchanged (SchemaBot retrying on its own — no triage due yet):

⚠️ Last error: lock wait timeout exceeded; try restarting transaction

Multi-deployment / sharded first failure — before / after

Before:

⚠️ First failure: us — lock wait timeout exceeded; try restarting transaction

After (shard failed — system stopped):

❌ First failure: us — lock wait timeout exceeded; try restarting transaction

Unchanged (shard retrying):

⚠️ First failure: shard -40 — lock wait timeout exceeded; try restarting transaction

Apply blocked by a safety gate — before / after

Before:

❌ Apply Blocked — Production

Staging has pending changes. Apply staging first before applying to production.

After:

⛔ Apply Blocked — Production

Staging has pending changes. Apply staging first before applying to production.

Unbolded heading counts — before / after

Before:

💡 Lint Warnings: 2 advisory findings

After:

💡 Lint Warnings: 2 advisory findings

How it moves us toward the northstar

This completes the PR-comment half of the shared severity vocabulary: with the CLI surface already migrated, both operator surfaces now read from pkg/glyph, so a glyph can never drift on one surface without the other. That unblocks the final step of the vocabulary work — the analyzer that forbids raw severity glyph literals outside pkg/glyph — which can only be turned on once no surface spells its own.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

This PR standardizes severity glyphs across GitHub PR-comment templates by switching hard-coded emoji to the shared pkg/glyph vocabulary, and adjusts a few headings/callouts so the glyph meaning matches the underlying event (refusal vs failure vs attention).

Changes:

• Replace raw severity glyphs in pkg/webhook/templates/* with pkg/glyph constants (e.g., Attention, Failed, Refused, Info, Escalation).
• Update “unsafe changes” apply rejection and “Apply Blocked” headings to use the refusal glyph (⛔) and update error/first-failure callouts to use the failure glyph (❌).
• Refresh template previews and documentation to reflect the new glyph semantics.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
TEMPLATES.md	Updates rendered preview snippets to match the new glyph vocabulary and corrected headings/callouts.
pkg/webhook/templates/sharded_apply.go	Switches sharded failure callouts to glyph.Failed.
pkg/webhook/templates/sharded_apply_test.go	Updates expectations for sharded failure callouts.
pkg/webhook/templates/reconciliation.go	Uses glyph.Attention for the reconciliation-required heading.
pkg/webhook/templates/plan.go	Replaces severity glyph literals with glyph.* constants in plan comment rendering.
pkg/webhook/templates/multi_apply.go	Switches multi-deployment first-failure callout to glyph.Failed.
pkg/webhook/templates/multi_apply_test.go	Updates expectations for multi-deployment first-failure callout.
pkg/webhook/templates/existing_copy.go	Uses glyph.Attention / glyph.Info for the discarded-copies disclosure marker.
pkg/webhook/templates/errors.go	Converts error templates to use glyph.Attention / glyph.Info / glyph.Failed constants.
pkg/webhook/templates/common.go	Changes error blockquote markers from ⚠️ to glyph.Failed for Error: / Last error: lines.
pkg/webhook/templates/common_test.go	Updates expectations for the new error blockquote glyph.
pkg/webhook/templates/apply.go	Uses glyph.Failed in headings / grouped emoji selection, plus glyph.Info in informational bullets.
pkg/webhook/templates/apply_test.go	Updates expectations for Apply Blocked headings and error markers.
pkg/webhook/templates/apply_commands.go	Updates unsafe-change rejection copy and apply-blocked headings to use glyph.Refused / glyph.Escalation / glyph.Attention.
pkg/webhook/sharded_apply_test.go	Updates sharded summary expectation for the apply-level failure callout.
pkg/webhook/plan_test.go	Updates unsafe-changes rejection copy expectations.
docs/lint-and-safety-levels.md	Updates documentation to reflect revised glyph semantics and adds ❌ to the legend.

Suppressed comments (1)

pkg/webhook/templates/common.go:322

• writeTableErrorLine now always prefixes with the failure glyph (❌), but apply progress rendering calls this for retryable interruptions (state.Task.FailedRetryable in apply.go). That makes an in-progress retry read like a terminal failure; consider letting the caller choose glyph.Attention vs glyph.Failed (or split into two helpers) based on task/apply state.

	if sanitized == "" {
		return
	}
	fmt.Fprintf(sb, "> "+glyph.Failed+" Last error: %s\n", quoteBlockLines(html.EscapeString(sanitized)))

---

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

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1135, e473cee.

Verdict: 6 findings — 3 non-blocking (glyph overshoot on auto-retrying paths, doc overclaim, ⛔ on transient read errors), 3 suggestions.

Non-blocking

1. The ⚠️→❌ migration overshoots on auto-retrying paths: ❌ now marks failures the system has not stopped on. — pkg/webhook/templates/sharded_apply.go:321, :331, pkg/webhook/templates/common.go:322

pkg/glyph defines the constant this PR is adopting as:

// Failed marks an attempted operation that failed. The system already
// stopped on its own; the operator's job is triage.
Failed = "❌"

Three of the migrated sites render on states where the system has not stopped and no triage is due:

• sharded_apply.go:305 — return opState == state.ApplyOperation.Failed || opState == state.ApplyOperation.FailedRetryable — so the lifted first-failure callout fires for an auto-retrying shard. The repo's own test proves the pairing: sharded_apply_test.go:63 builds {Shard: "-40", Emoji: "🔁", Label: "retrying", State: state.ApplyOperation.FailedRetryable, ...} and :69 asserts assert.Contains(t, out, "First failure:", "a retrying shard's error is still lifted"). The shard row reads 🔁 retrying; the callout above it now reads ❌.
• sharded_apply.go:327 — if !state.IsState(data.State, state.Apply.Failed, state.Apply.FailedRetryable) { return } guards the > ❌ **Failure:** … headline, so a whole apply that is still retrying gets a "Failure" banner.
• apply.go:832 — writeTableErrorLine(sb, table.ErrorMessage) inside the case state.Task.FailedRetryable: branch. This one is visible in the snapshot the PR itself regenerated, TEMPLATES.md:3769-3780: **Status**: Retrying, row 🔄 Interrupted — retrying automatically (attempt 2/10), then > ❌ Last error: lock wait timeout exceeded, footer SchemaBot retries automatically.

Failure scenario: an apply hits a transient lock-wait timeout, SchemaBot retries on its own, and the live progress comment an operator is watching now carries ❌. Nothing has stopped and there is nothing to triage, but the glyph asserts both. This also contradicts the PR body's own stated criterion for the first-failure move ("they point at a deployment/shard row already marked ❌") — for the sharded retryable case the row is marked 🔁, not ❌. Note the multi-deployment sibling is fine: presentation.go:259 restricts firstFailure to state.ApplyOperation.Failed only, so writeAggregateFirstFailure never fires on a retrying deployment. Fix: pass the severity in from the call site (glyph.Failed for Failed, glyph.Attention for FailedRetryable) rather than hard-coding it inside the shared helpers.

Secondary, weaker instance of the same shape: apply.go:182 — if state.IsState(data.State, state.Apply.Failed, state.Apply.Stopped) && data.ErrorMessage != "" — puts ❌ **Error:** under the ⏹️ Schema Change Stopped heading (apply.go:227). No preview fixture reaches it (the Stopped section of TEMPLATES.md carries no error block), so reachability is unproven, but the branch is live.

2. The generalized ⛔ row in the vocabulary table now overclaims — four live templates render those exact headings with a different glyph. — docs/lint-and-safety-levels.md:161

| ⛔ | Plan comment (**Cannot apply**), apply-rejection comments (**Apply rejected**, **Apply Blocked**), CLI apply-blocked headings (**Apply blocked**) | Refusal: this will not or did not proceed |

The pre-PR row named specific headings (Apply Blocked: PR Is Merged/Closed) and was therefore accurate; broadening it to bare **Apply Blocked** / **Apply rejected** makes it false for:

• apply_commands.go:300 — writeEnvironmentTitle(&sb, "🔒 Apply Blocked", data.Environment)
• apply_commands.go:644 and :747 — writeEnvironmentTitle(&sb, "⏳ Apply Blocked", environment)
• apply_commands.go:498 — writeEnvironmentTitle(&sb, glyph.Attention+" Apply rejected — base schema is newer", data.Environment)

Failure scenario: a contributor adding a new apply-rejection heading consults the table, reads that Apply Blocked carries ⛔, and picks ⛔ for a wait-and-retry case that the existing code spells ⏳. Doc-only, no runtime effect, but this is the table the vocabulary work exists to keep honest.

3. ⛔ on transient check-read errors conflicts with pkg/glyph's own rule and diverges from the review gate's fail-closed path. — pkg/webhook/templates/apply_commands.go:597, :695

glyph.go:14-18 states the boundary this PR is enforcing: "Refused is reserved for requests SchemaBot understood and will not perform, where retrying unchanged refuses again." Both templates the PR moved to ⛔ tell the operator the opposite — apply_commands.go:696 renders Could not verify %s status: failed to %s. Retry the apply command. and :614 renders …Retry the command; if it keeps failing, inspect the SchemaBot logs for the exact GitHub API error:. A transient GitHub read failure can succeed on an unchanged retry, so it is not a refusal.

The divergence this creates is concrete: review_gate.go:47 — h.postCommandError(repo, pr, installationID, commandName, environment, requestedBy, reviewGateErrorDetail(err)) — handles the structurally identical case (a GitHub read inside the gate fails, apply stops fail-closed, retry advised) and renders errors.go:190 const genericErrorTemplate = "## " + glyph.Failed + " {{.CommandName}} Failed" → ## ❌ Apply Failed. Failure scenario: an operator triaging two refusals caused by the same class of GitHub App permission problem sees ⛔ for one gate and ❌ for the other. Because both are already glyph constants, the planned raw-literal analyzer will never surface the drift. Either move these two to glyph.Failed, or move the review gate to glyph.Refused — but not one of each.

General suggestions

4. Two safety-gate guards went vacuous with the heading rename. — pkg/webhook/apply_integration_test.go:1810 and :1935

assert.NotContains(t, body, "Unsafe Change", "unsafe prompt must not outrank the freshness rejection")

RenderUnsafeChangesBlocked now emits **⛔ Apply rejected**: 1 unsafe change detected (apply_commands.go:179, rendered at TEMPLATES.md:1371), and assert.NotContains is case-sensitive — the capitalized Unsafe Change no longer appears anywhere in that comment, so the assertion passes regardless of whether the unsafe prompt regressed. The invariant survives only because the sibling assert.NotContains(t, body, "allow-unsafe", …) on the preceding line still fires. Retarget to "unsafe change" or "Apply rejected" so the assertion tests what its failure message claims.

5. The one ❌→⛔ flip with no unit test. — pkg/webhook/templates/apply_commands.go:765

writeEnvironmentTitle(&sb, glyph.Refused+" Apply Blocked", environment)

Every other blocked-apply template got its assertion updated in this PR (apply_test.go:57, :1998, :2027, :2045, :2062, :2094, :2106, :2128, :2138). A repo-wide grep for RenderApplyBlockedByUnlistedEnvironment returns only the caller check_prior_env.go:69, the preview registration preview.go:872, and the definition — no _test.go. Its only coverage is the TEMPLATES.md snapshot, and scripts/update-templates.sh is referenced only by the pre-commit hook and scripts/preview-gist.sh; no workflow under .github/ regenerates or diffs the file, so a future revert here would be caught by nothing.

6. The "no raw severity glyph" northstar still has un-migrated members of the same vocabulary. — pkg/webhook/templates/plan.go:426, :955

fmt.Fprintf(sb, "🛑 **Check before applying**: %d %s SchemaBot cannot attribute to this PR\n", …) and ⚙️ **Direct execution** (plus 💡 for lint warnings and ✅ for no-changes) stay raw literals. Three of those four sit in the very vocabulary table this PR edits at docs/lint-and-safety-levels.md:163-166, and 🛑 is not in the table at all. Worth deciding before the analyzer lands whether these join pkg/glyph or are explicitly declared out of the severity vocabulary — otherwise the analyzer ships with an allowlist nobody can justify from the docs.

The one thing that could have broken, verified

The blanket rewrite of two shared helpers, writeErrorBlock and writeTableErrorLine (common.go:311, :322). They fan out to every apply-status, apply-summary, sharded and multi-deployment comment, so a wrong glyph there is wrong on every live progress comment SchemaBot posts, and the two format strings splice a constant into the format argument of Fprintf rather than an operand.

Mechanically it is safe. None of the five pkg/glyph constants contains a %, and they are untyped string constants, so every rewritten site is a constant expression with an unchanged verb count and ordering — go vet's printf checker runs in all four green lint (…) jobs, and Build / Unit Tests / Integration Tests are green on e473cee. Byte-identity of the substituted literals is proved indirectly by the snapshot: unrelated ℹ️ lines at TEMPLATES.md:3281 and ⚠️ lines at TEMPLATES.md:221 render through rewritten call sites yet do not appear in the diff at all — any variation-selector or codepoint drift in glyph.Info / glyph.Attention would have moved them.

Semantically it is not safe at two of the five call sites, which is finding 1: apply.go:832 calls writeTableErrorLine from the state.Task.FailedRetryable branch whose own row renders 🔄 Interrupted — retrying automatically, and apply.go:182 calls writeErrorBlock for state.Apply.Stopped as well as Failed. The reachable result is committed in this PR's own snapshot at TEMPLATES.md:3780.

Verified correct

• No stale pre-PR string survives. grep for ⚠️ **Error, ⚠️ Last error, ⚠️ **First failure, ❌ Apply Blocked and Unsafe Changes Detected over TEMPLATES.md returns zero hits; the only repo-wide match is the analyzer fixture pkg/analyzers/webhookheaders/testdata/src/example/example.go, which is never rendered.
• The raw-glyph sweep of pkg/webhook/templates is complete for the five constants. No raw or \u-escaped ⚠️/❌/⛔/ℹ️/🚨 literal remains in any non-test file in that package — only two prose comments at existing_copy.go:68 and :74.
• The \"## \" + glyph.Refused + \" Apply Blocked\" concatenations do not evade the webhookheaders analyzer. Its package doc at webhookheaders.go:7-11 states callers pass ./pkg/webhook/... excluding ./pkg/webhook/templates, so the "## "-prefix rule never applied to apply_commands.go:695, :709, :727. All four lint (…) jobs are green.
• No consumer parses the changed heading text. The only body-matching code in pkg/webhook is stripLeadingHeading (templates/multi_apply.go:278), which tests strings.HasPrefix(body, "## ") and cuts at the first newline — glyph-agnostic. Comment upsert/dedup never matches Apply Blocked, Unsafe Change, or **Error:**.
• const template concatenation is legal and output-preserving. errors.go:190 and the ## + glyph.Attention config-error templates compile as constant expressions (untyped string consts) and reproduce the old literals exactly — the Database Not Found / Invalid Env fixtures in TEMPLATES.md are untouched by the diff.
• Count-unbolding is consistent and the exception is the stated one. Every unbolded heading still carries a bold label (plan.go:426, :912, :955, :972, :1106, :1121; apply_commands.go:179, :229), and the only surviving **%d** verbs are the four plan-summary parts at plan.go:538-547, which the PR body explicitly keeps bold.
• The updated assertions still kill a revert. assert.Contains(t, rendered, "3 unsafe changes detected") (plan_test.go:948) does not match the pre-PR **3** unsafe changes detected, since the ** sits between the digit and the space; same for the unbolded strings in lint_test.go and existing_copy_test.go.
• pluralize("Change", n) → pluralize("change", n) is case-transparent. func pluralize(singular string, count int) string at plan.go:1182 returns singular for n == 1 and singular+"s" otherwise, so 1 unsafe change detected / 3 unsafe changes detected render as asserted.
• The two rejection comments remain distinguishable after RenderUnsafeChangesBlocked adopted the Apply rejected heading RenderBlockedChangesApplyRejected already used: blocked_gate_integration_test.go:74-77 pins the engine-blocked path by asserting ⛔ Apply rejected and the schema-change engine refuses to execute and assert.NotContains(t, body, "--allow-unsafe", "a guaranteed failure must not coach an unsafe override").
• The CLI surface was correctly left alone. pkg/cmd/internal/templates/plan_unsafe_test.go:41 still asserts glyph.Attention+" Unsafe Changes Detected:", consistent with the ⚠️ row the PR keeps at docs/lint-and-safety-levels.md:162. No e2e or integration test asserts any of the changed PR-comment strings.
• The PR follows the repo's UX convention at AGENTS.md:51 — five collapsed <details> before/after renderings in the summary rather than TEMPLATES.md anchors.

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

[aparajon]

🤖 All six findings addressed in d500591:

1. ⚠️→❌ overshoot on auto-retrying paths — the shared error helpers now take the severity from the call site. Retryable task rows, retrying shard/apply callouts, and the stopped-apply error block render ⚠️ (SchemaBot is still acting or was stopped by request — no triage due); ❌ is reserved for failures the system stopped on. failureSeverity() in sharded_apply.go names the rule.
2. Docs ⛔ row overclaimed — the iconography table now scopes ⛔ to the Apply Blocked headings where retrying unchanged refuses again, and a presentation note spells out that the glyph follows the cause (🔒 lock, ⏳ wait-then-retry, ❌ transient verification fail-closed, ⚠️ stale base).
3. ⛔ on transient check-read errors — reverted to ❌ for both RenderApplyBlockedByCheckStatusError and RenderApplyBlockedByPriorEnvCheckError: an unchanged retry can succeed, so these are failed verifications the apply fail-closed on, not refusals — matching the review gate's rendering of the same error class.
4. Vacuous NotContains("Unsafe Change") guards — both integration-test assertions now check the lowercase copy actually rendered ("unsafe change"), verified against the stale-base template containing no such text.
5. Missing unit test for the unlisted-environment ⛔ flip — added TestRenderApplyBlockedByUnlistedEnvironment covering the heading glyph, the promotion-order line, the remediation hint, and the empty-order case.
6. Un-migrated 🛑/⚙️/💡/✅ literals in plan.go — documented as deliberately outside the severity vocabulary: the docs table now names them state/consent icons, so the analyzer's scope is settled before it lands (severity glyphs only).

This reply was written by Claude (Fable 5).