Skip to content

fix(cli): align CLI severity glyphs with the shared vocabulary - #1129

Merged
aparajon merged 3 commits into
mainfrom
armand/glyph-cli-surface
Aug 26, 2026
Merged

fix(cli): align CLI severity glyphs with the shared vocabulary#1129
aparajon merged 3 commits into
mainfrom
armand/glyph-cli-surface

Conversation

@aparajon

Copy link
Copy Markdown
Collaborator

Migrates every raw severity glyph in the CLI (pkg/cmd) to the shared pkg/glyph vocabulary, and corrects the sites whose glyph said the wrong thing:

  • Plan-time unsafe changes now carry ⚠️ (Attention), not ⛔. The same plan already renders ⚠️ on the PR comment and in list-plans; the CLI plan heading was the outlier. Nothing has been refused at plan time — the changes await consent.
  • The apply-blocked heading keeps ⛔ (Refused) but attaches it to the refusal: ⛔ Apply blocked: N unsafe change(s) detected, instead of labeling the changes themselves.
  • The multi-deployment first-failure callout moves from a bare one-cell to ❌ (Failed) — it points at a deployment row already marked ❌, and an actual failure was marked a notch below a plan-time warning.
  • The database-locked conflict moves from ❌ to ⛔ — the apply was refused while the lock is held, not attempted and failed.

Escalation (🚨) and Info (ℹ️) sites are unchanged in rendering and now reference the constants. Apply-operation state glyphs (⏹️/🚫/⏸) are a separate vocabulary and are untouched here.

CLI plan with unsafe changes — before / after

Before:

⛔ Unsafe Changes Detected:
  • users: DROP COLUMN email
  • orders: DROP TABLE

After:

⚠️ Unsafe Changes Detected:
  • users: DROP COLUMN email
  • orders: DROP TABLE
CLI apply blocked without --allow-unsafe — before / after

Before:

⛔ Unsafe Changes Detected:
  • users: DROP COLUMN email
  • orders: DROP TABLE
  • products:
      - MODIFY COLUMN price_cents: INT → SMALLINT (potential data loss)
      - DROP INDEX idx_category

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

  schemabot apply -s ./schema/testapp -e staging --allow-unsafe

After:

⛔ Apply blocked: 3 unsafe change(s) detected
  • users: DROP COLUMN email
  • orders: DROP TABLE
  • products:
      - MODIFY COLUMN price_cents: INT → SMALLINT (potential data loss)
      - DROP INDEX idx_category

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

  schemabot apply -s ./schema/testapp -e staging --allow-unsafe
Apply blocked by a database lock — before / after

Before:

❌ Apply Blocked: Database Locked

┌───────────────────────────────────┐
│  Database:   testapp (mysql)      │
│  Locked by:  alice                │
│  Since:      2 hours ago          │
└───────────────────────────────────┘

After:

⛔ Apply blocked: database locked

┌───────────────────────────────────┐
│  Database:   testapp (mysql)      │
│  Locked by:  alice                │
│  Since:      2 hours ago          │
└───────────────────────────────────┘
Multi-deployment first failure — before / after

Before:

│  Deployments:  1 completed · 1 halted · 1 failed      │
└───────────────────────────────────────────────────────┘

  ⚠ First failure: eu-west — duplicate key name 'idx_orders_source'

After:

│  Deployments:  1 completed · 1 halted · 1 failed      │
└───────────────────────────────────────────────────────┘

  ❌ First failure: eu-west — duplicate key name 'idx_orders_source'

This PR was written by an AI agent (Claude Code, model Claude Fable 5) on behalf of the author.

Migrate every raw severity glyph in pkg/cmd to the pkg/glyph constants
and correct the sites whose glyph misled:

- The plan-time unsafe-changes heading carried Refused before anything
  was refused; it now carries Attention, matching the PR plan comment
  and list-plans.
- The apply-blocked heading keeps Refused but names the refusal itself:
  "Apply blocked: N unsafe change(s) detected".
- The multi-deployment first-failure callout marked an actual failure
  with a bare one-cell warning glyph; it now carries Failed, matching
  the deployment row beside it.
- The database-locked conflict is a refusal, not a failure, so its
  heading moves from Failed to Refused.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 24, 2026 09:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Aligns CLI operator-facing severity glyphs with the shared pkg/glyph vocabulary so CLI output matches the PR-comment/UI semantics (plan-time attention vs apply-time refusal vs actual failure), and updates docs/templates/tests accordingly.

Changes:

  • Replaces raw severity glyph literals in CLI templates/commands with pkg/glyph constants and adjusts headings to match intended meanings (attention vs refused vs failed).
  • Updates lock-conflict and multi-deployment “first failure” callouts to use the correct severity glyph.
  • Refreshes templates/docs and adjusts integration/e2e expectations to match the new headings.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
TEMPLATES.md Updates rendered CLI template examples to match new headings/glyphs.
pkg/cmd/internal/templates/progress.go Uses glyph.Failed/glyph.Info in table-progress output.
pkg/cmd/internal/templates/progress_multi.go Uses glyph.Failed for the multi-deployment first-failure callout.
pkg/cmd/internal/templates/plan.go Uses pkg/glyph in plan/apply unsafe-change headings and ignore-namespace notices.
pkg/cmd/internal/templates/plan_unsafe_test.go Adds coverage for plan-vs-apply unsafe heading glyph semantics using pkg/glyph.
pkg/cmd/internal/templates/locks.go Changes lock-conflict heading to refused/blocked wording and uses pkg/glyph.
pkg/cmd/commands/watch_tui_view_multi.go Uses glyph.Failed for “First failure” in the TUI multi-deployment header.
pkg/cmd/commands/watch_tui_test.go Updates expected “First failure” glyph in tests.
pkg/cmd/commands/plan.go Updates comments to reflect plan-time unsafe warning as attention (⚠️).
pkg/cmd/commands/fixlint.go Uses glyph.Failed for unfixable lint issues heading.
integration/cli_test.go Updates apply-blocked output assertions to match new wording.
e2e/local/vitess_test.go Updates apply-blocked output assertions to match new wording.
e2e/local/local_test.go Updates plan/apply output assertions/comments to match new plan/apply semantics.
docs/lint-and-safety-levels.md Documents the updated mapping of glyphs to meanings and where they appear.
Suppressed comments (1)

pkg/cmd/commands/watch_tui_test.go:322

  • Same issue here: the expected string hardcodes ❌. Prefer glyph.Failed in the assertion so tests track the shared severity vocabulary rather than duplicating the raw glyph.
	assert.Contains(t, view, "running (degraded)")
	assert.Contains(t, view, "1 running · 1 failed")
	assert.Contains(t, view, "❌ First failure: eu — duplicate column")
	assert.Contains(t, view, "🔄 us — running table copy")

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

Comment thread pkg/cmd/commands/watch_tui_test.go
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aparajon
aparajon marked this pull request as ready for review August 24, 2026 10:04
@Kiran01bm

Copy link
Copy Markdown
Collaborator

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1129, 7dee414.

Verdict: 3 findings — 0 blocking, 2 non-blocking (stale doc note, cross-surface glyph split), 1 suggestion.

Non-blocking

  • docs/lint-and-safety-levels.md:175 — The Presentation-notes bullet still reads "The CLI uses ⛔ for unsafe changes at both plan and apply time, since the terminal is both the review surface and the gate." That is the exact rule this PR reverses, and it contradicts the ⚠️ table row the same diff rewrote 13 lines above. The stale line is phrased as the rationale, so it's the one a future sweep would trust and re-apply.

  • pkg/webhook/templates/multi_apply.go:166 — The first-failure callout moves to glyph.Failed on the CLI but stays a hardcoded ⚠️ on the comment surface (also :169, and sharded_apply.go:320,322), so the comment now shows ⚠️ First failure: us directly above ❌ us — failed. Fair as a follow-up: pkg/webhook is wholly unmigrated (zero pkg/glyph imports) and the CLI's old glyph was the bare 1-cell (U+26A0), so exact codepoint parity never held on this line.

General suggestions

  • pkg/cmd/internal/templates/plan.go:499 — The new heading counts len(changes) (UnsafeChange entries) while the GitHub apply-rejection heading counts individual findings via countUnsafeFindings. On the repo's own preview data that renders "3 unsafe change(s) detected" in the terminal and "4 Unsafe Changes Detected" on GitHub. Each surface is self-consistent with its own bullet list, so this is a counting-unit divergence rather than a miscount — worth picking one convention while the vocabulary is being unified.

The one thing that could have broken, verified

Five negative lock assertions in integration/locks_test.go (671, 682, 694, 706, 765) had their needle lengthened from "Database Locked" to "Apply blocked: database locked" — a longer NotContains needle is the classic way to make a guard silently vacuous. It holds: WriteLockConflict (locks.go:52) is the sole renderer of that string, and the same needle is asserted positively at 507/577/719/909, so any future rename fails those four loudly rather than quietly disarming the five guards.

Verified correct

  • grep -rn "Database Locked" returns zero hits repo-wide — the old heading is fully swept.
  • Every glyph swap other than the four heading/first-failure changes is codepoint-identical, so that output is byte-for-byte unchanged.
  • No glyph constant contains %, so the concatenated fmt.Printf(glyph.X+"…", args) format strings stay vet-clean.
  • The five remaining "Unsafe Changes Detected" assertions are all plan-path; writePlanBody skips that heading when isApply is true.
  • "unsafe change(s) detected" appears only in WriteUnsafeChangesBlocked, so the retargeted apply assertions can't be satisfied by the lock heading.
  • The plan-time ⚠️ is right for the other caller too — rollback.go:103 is a pre-confirmation disclosure, nothing refused yet.
  • The bare 1-cell is gone from every output path; the only survivor is the width assertion at pkg/ui/link_test.go:138.
  • No non-test file under pkg/cmd holds a raw severity emoji any more — remaining hits are comments only.
  • TEMPLATES.md was regenerated for all four affected samples; the ⛔ 3 Unsafe Changes Detected at line 1326 is the comment surface, still emitted verbatim.
  • Treating a lock conflict as Refused matches the vocabulary: the situation must change before a retry can succeed.

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

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aparajon

Copy link
Copy Markdown
Collaborator Author

🤖 Thanks — addressed as follows, on Armand's behalf:

  1. Stale presentation note — fixed in fcbad85. The bullet now states the current rule: the CLI and the plan comment share the same severity reading (⚠️ awaiting review at plan time, ⛔ on the refused apply).
  2. Webhook first-failure callouts — already handled by the stacked webhook-surface PR: fix(github): align PR comment severity glyphs with the shared vocabulary #1135 moves all four cited sites (and the adjacent Failure callout) to glyph.Failed.
  3. Counting-unit divergence — leaving as-is, deliberately: each heading counts what its own list renders. The CLI lists one bullet per change with reasons nested beneath, so it counts changes; the comment lists individual findings, so it counts findings. Aligning the numbers without aligning the list shapes would detach each heading from the list it introduces — the real unification is a list-shape decision that belongs with the comment-surface work, not this glyph pass.

This reply was generated by Claude Code (Claude Fable 5).

@aparajon
aparajon merged commit 3b8e721 into main Aug 26, 2026
34 checks passed
@aparajon
aparajon deleted the armand/glyph-cli-surface branch August 26, 2026 02:06
Kiran01bm added a commit that referenced this pull request Aug 26, 2026
…-applies

* origin/main:
  test(storage): add Tasks parity family to the cross-dialect suite (#1138)
  test(storage): add ApplyOperations parity family to the cross-dialect suite (#1140)
  fix(github): release cancelled apply check ownership (#1134)
  fix(github): silence participant discovery noise (#1133)
  fix(cli): align CLI severity glyphs with the shared vocabulary (#1129)

# Conflicts:
#	pkg/storage/storagetest/storagetest.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants