Skip to content

fix(github): drop the cutover duration promise from progress surfaces - #1240

Merged
aparajon merged 2 commits into
mainfrom
armand/cutover-footer-copy
Sep 1, 2026
Merged

fix(github): drop the cutover duration promise from progress surfaces#1240
aparajon merged 2 commits into
mainfrom
armand/cutover-footer-copy

Conversation

@aparajon

@aparajon aparajon commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Why this matters

The cutting-over comment footer promised "typically completes within seconds", but cutover duration depends on acquiring the table lock under whatever write load the target is carrying — SchemaBot cannot know how long that takes, and on a hot table it can be far longer than seconds. A duration promise that doesn't hold trains operators to distrust the status surface exactly when they are watching it most closely. The honest rendering is the state itself, with no forecast.

What it does

cutting-over rendering
├─ PR comment footer: the "Cutover in progress — typically completes within
│  seconds." line is removed with no replacement — the cutting_over state
│  renders no footer, the same as other states with no operator action
└─ CLI preview: the equivalent "typically completes within seconds" line is
   removed from the defer-cutover preview scenario

Two properties worth calling out:

  • No forecast, anywhere. No surface now claims how long a cutover takes. The status line and per-table glyphs already say what is happening; duration is the one thing SchemaBot cannot promise, so it says nothing.
  • No action prompt lost. The removed footer carried no command — cutover has no operator action while it is in flight — so the footer's disappearance removes only the false claim.
Rendered: cutting-over PR comment (after)

Schema Change Status — Staging

Database: testapp | Apply ID: apply-a1b2c3d4e5f6

Applied by @jackjackbits at 2026-01-01 00:00:00 UTC

Status: Cutting Over

📊 3 cutting over

Schema testapp

orders: 🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦 🔄 Cutting over...

ALTER TABLE `orders` ADD INDEX `idx_user_id`(`user_id`);

users: 🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦 🔄 Cutting over...

ALTER TABLE `users` ADD INDEX `idx_email`(`email`);

products: 🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦 🔄 Cutting over...

ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`);

Last updated: 2026-01-01 00:00:00 UTC

A second commit fixes a pre-existing severity-glyph lint finding in the same package (the sharded table failure label now uses glyph.Failed); the rendered output is byte-identical.

How it moves us toward the northstar

The northstar is a status surface an operator can trust during an incident without cross-checking the database. Every claim the surface makes has to be one SchemaBot can actually stand behind; removing the one it can't keeps the cutting-over comment a statement of fact rather than a guess.

Opened by Claude (Fable 5).

aparajon and others added 2 commits September 1, 2026 11:27
Cutover duration depends on lock acquisition under live load, so the
comment footer and CLI preview no longer claim it completes within
seconds; the cutting-over state now stands on its own.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The severity-glyph analyzer requires the pkg/glyph constants so failure
markers stay consistent across every PR-facing surface.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 1, 2026 15:39

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

Removes an unreliable “typically completes within seconds” duration claim from cutover-in-progress status surfaces, ensuring operator-facing progress output remains factual and trustworthy.

Changes:

  • Dropped the “typically completes within seconds” footer from the PR progress comment when an apply is in cutting_over.
  • Removed the equivalent duration sentence from the CLI “defer cutover” preview scenario.
  • Fixed a pre-existing severity-glyph lint issue by using glyph.Failed for the sharded table failure label.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
TEMPLATES.md Updates rendered template examples to remove the cutover duration promise from PR comment + CLI preview.
pkg/webhook/templates/sharded_apply.go Uses glyph.Failed instead of a literal failure glyph for sharded table “Failed” status.
pkg/webhook/templates/apply.go Removes the CuttingOver footer text that previously promised a short duration.
pkg/cmd/internal/templates/preview_defer.go Removes the “typically completes within seconds” line from the defer-cutover preview output.

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

@aparajon
aparajon marked this pull request as ready for review September 1, 2026 16:12

@morgo morgo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Approved on Morgan's behalf by his AI agent.

+1/-10, green CI, and the whole change is removing a promise the system can't keep. "Cutover in progress — typically completes within seconds" is exactly the kind of reassurance that reads fine until a cutover sits behind an MDL wait and the operator concludes something is broken because the UI told them to expect seconds. Removing it from all three surfaces (the doc, the CLI defer preview, and the webhook template) leaves the state word doing the work, which is honest. Nothing functional rides on it.

One thing to sequence, though: this PR also converts the literal in pkg/webhook/templates/sharded_apply.go to glyph.Failed — and #910 carries the identical one-line fix to the same file. Both are open. Whichever lands second will need that hunk dropped.

That's not a criticism of either PR; it's the predictable outcome of the thing I flagged on #910 yesterday. The severityglyphs analyzer blocks any commit touching pkg/webhook/... because of that one pre-existing literal on main, so every PR that goes near the package independently discovers it and fixes it in passing. Two have now done so. A single sweep running the analyzer repo-wide and fixing everything in one commit would stop the trail — and would also surface whatever other pre-existing violations are sitting in packages nobody has staged yet, rather than letting them ambush the next unrelated PR.

@aparajon

aparajon commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Addressing morgo's review — agreed on the framing: the state word is the only thing this surface can say honestly, so it now says only that.

On the duplicated glyph hunk: #1239 landed first, so this PR is the "lands second" case you predicted. No action turned out to be needed — the hunk is byte-identical to what's now on main, so GitHub still reports the PR mergeable and clean; it just melts into a no-op at merge.

On the repo-wide sweep — I ran it. Analyzer over every package on current main, three passes (untagged, integration, e2e build tags). Result: exactly one violation left in the codebase — a literal ℹ️ in writeShardedTableCopyProgress (pkg/webhook/templates/sharded_apply.go:453), which arrived with #1191 in today's merge batch and will ambush the next PR that stages this package, exactly as you described. The sweep also surfaced a defect in the hook itself: it passes -tags=integration to the checker binary, but the singlechecker's -tags flag is deprecated and a no-op, so the integration pass has been silently re-analyzing the same untagged files (build tags have to go in via GOFLAGS). Both fixes — the one remaining literal and the hook's tag handling — are a natural single sweep PR; that's the author's call to open, but the inventory is done and it's short.

Reply generated by Claude Code (Claude Fable 5).

@aparajon
aparajon merged commit adc3747 into main Sep 1, 2026
39 checks passed
@aparajon
aparajon deleted the armand/cutover-footer-copy branch September 1, 2026 18:00
Kiran01bm added a commit that referenced this pull request Sep 1, 2026
…dcolumn-ddl-seam

* origin/main: (28 commits)
  docs: document the PostgreSQL support envelope (#1144)
  fix(engine): report why a Vitess schema change failed (#1242)
  feat(ddl): detect statements whose cost scales with table size (#1237)
  fix(operator): keep a multi-table apply running while tables are queued behind a cutover (#1241)
  fix(storage): index the webhook inbox claim ordering (#1196)
  fix(github): drop the cutover duration promise from progress surfaces (#1240)
  fix(github): render row-copy progress percentages at their true precision (#1239)
  fix(observability): do not report a shutdown as a claim failure (#1233)
  fix(github): tell an operator why a refused apply's database is busy (#1224)
  fix(engine): do not mark an apply failed when its driver shuts down (#1234)
  feat(github): render live row-copy progress on sharded table lines (#1191)
  feat(ui): add approximate row and byte formatters (#1236)
  fix(planetscale): delete the branch an apply created when it fails before its deploy request (#963)
  feat(api): app grouping field on database config (#1226)
  feat(cli): filter pulled tables with --table (#1235)
  fix(github): refuse a Vitess foreign key at plan time instead of at apply time (#966)
  feat(lint): add severityglyphs analyzer to keep the severity vocabulary in pkg/glyph (#1153)
  feat: remove the volume control operation end to end in favor of autoscaling (#1225)
  ci: give the k8s e2e job budget room for setup plus go test's timeout (#1232)
  fix(storage): canonicalize lock and check identity keys (#1216)
  ...

# Conflicts:
#	pkg/ddl/parser.go
#	pkg/ddl/parser_test.go
Kiran01bm added a commit that referenced this pull request Sep 2, 2026
…ditive-convergence

* origin/main: (33 commits)
  feat(postgres): add ADD COLUMN synthesis to the statement parser seam (#1212)
  feat(cli): add storage canonicalize-identity-keys admin subcommand (#1231)
  fix(storage): canonicalize remaining identity keys (#1218)
  fix(storage): canonicalize apply and task identity keys (#1217)
  fix(webhook): canonicalize repository identity at ingress (#1213)
  docs: document the PostgreSQL support envelope (#1144)
  fix(engine): report why a Vitess schema change failed (#1242)
  feat(ddl): detect statements whose cost scales with table size (#1237)
  fix(operator): keep a multi-table apply running while tables are queued behind a cutover (#1241)
  fix(storage): index the webhook inbox claim ordering (#1196)
  fix(github): drop the cutover duration promise from progress surfaces (#1240)
  fix(github): render row-copy progress percentages at their true precision (#1239)
  fix(observability): do not report a shutdown as a claim failure (#1233)
  fix(github): tell an operator why a refused apply's database is busy (#1224)
  fix(engine): do not mark an apply failed when its driver shuts down (#1234)
  feat(github): render live row-copy progress on sharded table lines (#1191)
  feat(ui): add approximate row and byte formatters (#1236)
  fix(planetscale): delete the branch an apply created when it fails before its deploy request (#963)
  feat(api): app grouping field on database config (#1226)
  feat(cli): filter pulled tables with --table (#1235)
  ...

# Conflicts:
#	docs/configuration.md
#	pkg/ddl/postgres_parser.go
#	pkg/ddl/postgres_parser_test.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