Skip to content

fix(github): fold the progress comment into the terminal summary - #1082

Open
aparajon wants to merge 1 commit into
mainfrom
armand/f3-comment-reclaim
Open

fix(github): fold the progress comment into the terminal summary#1082
aparajon wants to merge 1 commit into
mainfrom
armand/f3-comment-reclaim

Conversation

@aparajon

@aparajon aparajon commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Why this matters

Every apply currently ends with two near-identical "Schema Change Status" comments on the PR: the tracked progress comment edited to its full final rendering, and a same-titled terminal summary posted seconds later. Operators reading the bottom of the PR see the same status twice and have to work out which one is authoritative.

What it does

At terminal, the reader sees what they always did — the progress comment above, the summary as the last word at the bottom — but the progress comment now freezes into a collapsed details block pointing at the summary, the same fold shape every rotation flavor already uses, with the final per-operation status preserved inside. Internally the summary is written before the fold: the fold's body links to the summary comment (so its ID must exist first), and a crash between the two writes must leave the verdict visible on the PR — a briefly unfolded progress comment is safe and re-folds on redelivery, while a folded progress comment with no summary posted would show a finished-looking apply with no outcome anywhere.

before                          after
──────                          ─────
[progress: full final status]   [progress: 🏁 folded → summary]
[summary:  full final status]   [summary:  full final status]

Rendered previews:

Finished: Superseded Progress Comment (the fold)

🏁 Schema change finished — the final status is in the summary comment.

Progress while the schema change ran

Schema Change Status — Staging

Database: testapp | Apply ID: apply-a1b2c3d4e5f6

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

Status: Applied

users: 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Complete

ALTER TABLE `users` ADD INDEX `idx_email_created`(`email`, `created_at`);
Summary: Completed (unchanged by this PR — shown for the pairing)

✅ Schema Change Applied — Staging

Database: testapp

Applied successfully — your schema changes are live!

Apply details (3 tables)

Apply ID: apply-a1b2c3d4e5f6

orders

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

users

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

products

ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`);
  • publishClaimedSummary returns the posted comment ID; on a lost claim it reads the summary marker, so a repeated terminal publish (webhook redelivery, fresh observer) re-folds toward the recorded summary instead of restoring the full rendering and undoing the earlier fold.
  • Stopped applies keep the full frozen rendering: stopped is the only terminal state that returns to active, and the resume rotation owns folding the stop record.
  • When the summary's location is unknown (publish failed, claim in flight, or a per-driver observer deferring to the aggregate publisher on a multi-operation apply), the full rendering stands as before — the fold is strictly best-effort UX on top of the exactly-once summary claim.

How it moves us toward the northstar

The PR timeline is the operator's primary interface for a schema change. Ending every apply with exactly one live status record — the summary at the bottom, history folded above it — keeps that interface unambiguous as more engines and multi-operation applies land on the same comment lifecycle.

Opened by Claude (Fable 5).

Every apply ended with two near-identical "Schema Change Status" comments
on the PR: the tracked progress comment edited to its full final rendering,
and the separate terminal summary posted seconds later. The terminal
publish now posts the summary first, then freezes the progress comment as
a collapsed details block pointing at the summary — one authoritative
final record at the bottom of the PR, with the per-operation history
preserved inside the fold.

A repeated terminal publish (webhook redelivery, fresh observer) that
loses the summary claim reads the recorded summary from the marker and
folds toward it again instead of restoring the full rendering. Stopped
applies keep the full frozen rendering — stopped returns to active, and
the resume rotation folds the stop record itself. When the summary's
location is unknown (publish failed, claim in flight, or a per-driver
observer deferring to the aggregate publisher), the full rendering stands
as before.

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

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

This PR improves the GitHub PR timeline UX by ensuring that terminal applies end with exactly one authoritative “Schema Change Status” record at the bottom of the timeline: the terminal summary comment. The tracked progress comment is “frozen” by folding its final per-operation rendering into a collapsed <details> section that points at the summary, avoiding duplicate near-identical terminal status comments.

Changes:

  • Add a dedicated terminal fold rendering (“Finished: Superseded Progress Comment”) that links to the terminal summary and preserves the final progress body inside a collapsed details block.
  • Reorder terminal publishing to post/resolve the terminal summary first (returning the summary comment ID when known), then fold the tracked progress comment toward that summary (except for stopped applies).
  • Add/update preview templates and integration/unit tests to cover terminal folding and repeated terminal publishes (refold behavior).

Reviewed changes

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

Show a summary per file
File Description
TEMPLATES.md Adds a rendered documentation example for the “Finished: Superseded Progress Comment” fold shape.
pkg/webhook/templates/preview.go Adds a preview renderer for the terminal-summary-superseded progress comment.
pkg/webhook/templates/issue_comment.go Introduces a terminal-summary fold marker/prefix and a shared fold helper parameterized by successor marker.
pkg/webhook/templates/issue_comment_test.go Extends frozen-body detection tests to include the new terminal-summary fold.
pkg/webhook/comment_observer.go Posts terminal summary first (returning its comment ID when known) and folds the tracked progress comment toward it when applicable.
pkg/webhook/apply_comment_integration_test.go Updates an existing flow test and adds new end-to-end integration coverage for terminal folding/refolding and the stopped no-fold invariant.
pkg/cmd/internal/templates/preview_comment.go Adds the new preview output entry to the CLI preview listing.

💡 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 August 19, 2026 04:41
@Kiran01bm

Copy link
Copy Markdown
Collaborator

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1082, 20854dd.

Verdict: 7 findings — 2 blocking (the fold fires on Failed/Cancelled/Reverted, and the freeze it depends on is lease-gated with no durable intent), 3 non-blocking, 2 suggestions.

Blocking

1. A failed apply is headlined "🏁 Schema change finished" with the error collapsed out of view. The fold's only exclusion is Stopped (comment_observer.go:504), so Failed, Cancelled and Reverted all take it: the tracked comment becomes 🏁 Schema change finished — the final status is in [the summary] with **Status**: Failed and > ⚠️ Last error: … hidden under <details>. A rollback apply gets the same "finished" headline that writeRollbackHeader (apply.go:236) exists to prevent. Gate on state.Apply.Succeeded rather than on "not Stopped", or vary the prefix by outcome.

2. The fold is owed but never recorded, and the freeze it depends on now runs after a network round-trip. The summary is published first, then editTrackedComment folds — but that returns early on !o.leaseStillOwnsObserver(…) (comment_observer.go:773), so a lease handover in the window between the two leaves a permanent duplicate pair with nothing to reconcile it. Rotation folds persist PendingFreezeCommentID for exactly this reason, and AGENTS.md § Control Operations requires the signal be "durable and cross-pod safe". Pre-PR the per-operation freeze ran before the publish and its comment said "it always runs"; that comment was deleted along with the guarantee.

Non-blocking

3. The lost-claim branch can fold onto a superseded summary. publishClaimedSummary reads the summary marker with a plain Get (comment_observer.go:1626), and sqlstore/apply_comments.go:78 has no SupersededAt predicate — so a stale stop-summary marker becomes the fold target. The sibling reads at :573 and :850 both guard on it.

4. Two terminal paths were missed by the sweep and still end with the duplicate pair. The deferred-cutover finalizer (finalizeTrackedProgressCommentAtTerminal, :581) and ReconcileMissingSummaryComments (handler.go:545) both reach a terminal summary without folding, so the behavior the PR describes as the terminal shape holds only on the main path.

5. Three of eleven mutations survive, all of them the operator-facing copy. Rewriting the <summary> label, the 🏁 Schema change finished prefix, and the — the final status is in connector all leave the suite green — the strings a reviewer would check are exactly the ones unpinned. The structural mutations are well covered (never-fold, always-fold, drop-Stopped-guard, invert, <details open>, drop marker, drop prefix from the predicate all fail).

General suggestions

6. The cancel test now documents the opposite of what it asserts. apply_comment_integration_test.go:2506 still says the comment is "frozen at its final rendering in place", which the fold makes false, and the test only asserts edited.CommentID — never the body — so it would not have caught the change either way.

7. Nested <details> renders acceptably but is worth a fixture. The frozen block wraps content that already contains collapsed sections; GitHub handles this, but no preview fixture shows the nested case, so a future indentation change to renderFoldWithMarker would go unnoticed.

The one thing that could have broken, verified

renderFoldWithMarker is a genuine extraction, not a second implementation drifting from the rotation fold: the rotation path was rewritten to call it, no duplicate helper remains, and the marker constant is shared. I confirmed the collapsed block still round-trips — the marker survives the edit, so a later observer on a fresh drive claim recognizes an already-folded comment and does not stack a second fold on top.

Verified correct

  • TEMPLATES.md is byte-identical to a fresh scripts/update-templates.sh run against a binary built from the PR head.
  • The Stopped exclusion is right: pkg/state/metadata.go notes Stopped is terminal at the apply level but returns to active, so keeping its full rendering is correct.
  • fix(github) is the correct scope per AGENTS.md — the change is PR UX regardless of living in pkg/webhook/.
  • No "migration" terminology; new tests use t.Context(), testify, and scenario comments.
  • The summary-comment claim path is unchanged — the PR adds a fold on top of it, not a new claim.

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

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