fix(github): keep the PR progress comment updating between operation dispatch waves

[aparajon]

Why this matters

Every GitHub side effect from the comment observer is gated on a valid parent apply lease. That matches the whole-deployment drive, where one driver holds the parent lease end to end — but operation-keyed applies claim the parent only transiently per dispatch wave and drive their operations under operation leases. Between waves no parent lease exists, so every progress edit was skipped fail-closed and the PR progress comment froze at its initial text for the entire apply. An operator watching the PR saw a stale comment while the apply was actively making progress — the worst kind of wrong, because the fail-closed skip looks identical to "nothing is happening."

What it does

Gives the observer a second, equally fail-closed way to earn the right to edit: a durable claim on the tracked comment's database row. The observer writes its own name onto the row with a conditional update — the write only succeeds if no other observer holds the row, or the holder has gone quiet past the staleness window. Exactly one observer across all pods wins at a time; everyone else sees the row is taken and skips. If the claim can't be confirmed for any reason, the observer skips the edit — it never guesses.

Before: parent apply lease is the only edit authority
─────────────────────────────────────────────────────
 dispatch wave        gap between waves       dispatch wave
 [parent lease]       (no parent lease)       [parent lease]
 progress edits ✓     every edit skipped ✗    progress edits ✓
                      comment frozen while
                      operations still run

After: an unheld lease falls through to a durable comment-row claim
────────────────────────────────────────────────────────────────────
 dispatch wave        gap between waves       dispatch wave
 [parent lease]       [comment-row claim]     [parent lease]
 progress edits ✓     one observer wins the   progress edits ✓
                      row → edits ✓, all
                      others skip

• The lease gate stays authoritative whenever a parent lease exists; the terminal summary keeps its existing single-writer authority.
• The claim path is admitted only for the shapes that genuinely drive under operation leases, mirroring the operator's drive-mode split: a multi-operation rollout with non-terminal keyed work, or a generation manifest still expecting undispatched keys. Single-operation and whole-deployment applies never take it: their drives hold the parent lease, so an unheld lease still means no driver and the lease checks remain the only authority.
• The authority is decided once per observer callback against freshly read rows — including a re-read of the parent lease columns, so a dispatch wave that re-claims the parent (or a projection that settles the apply terminal) mid-tick denies the claim instead of racing the new owner. One decision serves all of the callback's side-effect checks, replacing a per-check storage scan and claim write.
• The claim lives in two new columns on the tracked comment row (observer_owner, observer_heartbeat_at), renewed once per admitted callback, with takeover allowed only after the apply-lease staleness window. Losing the claim, or any storage uncertainty, skips the GitHub side effect and logs with the apply's triage attributes — never a spurious edit. The expected peer-pod losses log at debug; genuine uncertainty logs at error.
• Ownership is process-scoped, so concurrent observers on different pods converge on exactly one editor; comment rotation leaves the authority columns untouched.
• The claim's fresh-row decisions run against both storage dialects through the parity suite, and each dialect suite proves the stale-heartbeat takeover against an aged row.

Operational note

The PostgreSQL bootstrapper creates missing tables but does not alter existing ones, so an already-bootstrapped PostgreSQL storage database needs the new columns before this deploys — otherwise startup fails with storage table "apply_comments" is missing expected columns:

ALTER TABLE apply_comments
  ADD COLUMN observer_owner varchar(255) DEFAULT NULL,
  ADD COLUMN observer_heartbeat_at timestamp DEFAULT NULL;

MySQL needs no action: EnsureSchema diffs and applies the addition on startup.

How it fits

Operation-keyed dispatch is the fan-out foundation for multi-deployment and sharded applies, and those are exactly the applies whose progress comments matter most — long-running, many moving parts, watched from the PR. This makes the comment observer's authority model match the drive model: whichever shape holds the work, exactly one observer keeps the PR current, and uncertainty always resolves to silence rather than a wrong edit.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

This PR fixes a gap in SchemaBot’s GitHub progress-comment updates for operation-keyed applies: when the parent apply lease is intentionally unheld between dispatch waves, the comment observer can now still safely edit the tracked PR progress comment by claiming a durable, cross-pod “progress-comment authority” on the apply_comments row.

Changes:

• Add a durable, CAS-style progress-comment authority (owner + heartbeat) that gates GitHub side effects when no parent apply lease exists during operation-scoped work.
• Extend storage with ClaimProgressCommentAuthority and persist authority fields on apply_comments (MySQL + Postgres schemas).
• Add unit + integration tests covering operation-scoped in-flight detection, single-winner authority behavior, and lease-held behavior remaining unchanged.

Reviewed changes

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

Show a summary per file

File	Description
pkg/webhook/comment_observer.go	Adds progress-comment authority fallback when parent apply lease is legitimately unheld; avoids freezing progress comments between dispatch waves.
pkg/webhook/comment_observer_test.go	Adds unit tests for determining when operation-scoped work is considered “in flight.”
pkg/webhook/comment_authority_integration_test.go	Adds integration tests to verify progress edits occur without a parent lease, and that concurrent observers converge on one editor.
pkg/storage/storage.go	Extends ApplyCommentStore interface and defines the staleness window constant for authority takeover.
pkg/storage/internal/sqlstore/apply_comments.go	Implements ClaimProgressCommentAuthority as a conditional update with stale takeover behavior.
pkg/storage/internal/sqlstore/apply_comments_test.go	Adds store-level tests for authority claim/renew/lose and stale takeover behavior.
pkg/schema/postgres/apply_comments.sql	Adds observer_owner and observer_heartbeat_at columns for Postgres schema parity.
pkg/schema/mysql/apply_comments.sql	Adds observer_owner and observer_heartbeat_at columns for MySQL schema parity.

---

💡 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/1104, a41fe14.

Verdict: 11 findings — no blockers; 4 non-blocking (1 deploy hazard, 2 invariant gaps, 1 coverage gap), 7 suggestions.

Non-blocking

1. Postgres column addition needs a pre-deploy ALTER — pkg/schema/postgres/apply_comments.sql:9 adds observer_owner / observer_heartbeat_at, but the PG bootstrapper only creates missing tables and then verifies columns. On an already-bootstrapped PG deployment EnsureSchema returns storage table "apply_comments" is missing expected columns: … (ensure_schema_postgres.go:182) and startup aborts. MySQL is unaffected (Spirit diffs and applies), so this needs a release/runbook note, not a code change.

2. In-flight predicate does not match the operator's drive-mode split — comment_observer.go:841 keys on op.OperationKey != "", but recoverApplyOperation routes to the operation lease only when len(ops) > 1 / manifest keys are missing / the op has no tasks (operator.go:752). A single keyed op with tasks drives under the parent lease, so once that lease is genuinely released with the apply parked non-terminal, a peer observer is admitted under the authority and its comment writes take storage's unfenced no-lease path. The repo already encodes the right predicate as len(ops) > 1 || len(apply.MissingExpectedOperationKeys(ops)) > 0 (operator.go:2328).

3. Authority path never re-reads the apply lease — the comment at comment_observer.go:745-748 justifies re-checking the lease "immediately before each side effect", but progressCommentAuthorityOwnsObserver only queries apply_operations and apply_comments, deciding from a once-per-5s poller snapshot. So an authority holder cannot see a dispatch wave re-claiming the parent lease mid-tick and keeps editing concurrently with the new holder. Bounded to comment bookkeeping (and the pre-existing lease = apply.Lease() fallback already admitted peer observers), so safe to tighten later.

4. New claim primitive has MySQL-only coverage — ClaimProgressCommentAuthority is tested only via NewMySQL(testDB) at apply_comments_test.go:112, while its siblings ClaimSummaryComment / ReclaimStaleSummaryClaim are in both the cross-dialect conformance suite and the PG integration tests. It is explicitly dialect-sensitive (the zero-rows re-read exists because "MySQL reports rows changed, not rows matched"; the stale window compares a timestamp to NOW()), so PG renew/takeover/exclusion semantics are unverified.

General suggestions

5. Gate re-runs ~5x per tick — leaseStillOwnsObserver is called at comment_observer.go:253 plus lines 876/893/902/910, and each authority-path call issues a full ListByApply (:833) followed by a row-locking UPDATE apply_comments (:802). That is ~5 ops scans and ~5 writes to the row being edited, per apply per pod per tick, with identical results. OnTerminal already states the intended discipline ("Load the operation rows once … a single time per callback", :455) and OnProgress holds o.mu throughout, so hoist the decision.

6. "Authority not won" is Info on every tick — :813 fires once per 5s per losing pod for the whole rollout, for a case the comment itself calls "Expected on every peer pod polling the same apply" — AGENTS.md routes that to Debug. The observer's logger interface only exposes Info/Error (:35-38), so downgrading means widening the interface and the two capturing test loggers.

7. Use the operation-layer state helper — :841 classifies an apply_operations state with state.IsTerminalApplyState; state.IsApplyOperationTerminal exists for exactly this and is what every other operation-state site uses. Identical today (ApplyOperation = Apply), but it is the only such site in the repo.

8. Terminal-gate subtest is vacuous — comment_observer_test.go:343-347 uses an empty stubApplyOperationStore and no ExpectedOperationKeys, so deleting the IsTerminalApplyState short-circuit at :830 would still return false. Seed a non-terminal keyed op alongside the terminal apply state to make it real.

9. Stale assertion message — comment_observer_test.go:319 asserts "a normal observer with no apply lease must fail closed", which this PR no longer guarantees; it passes only because the fixture is state.Apply.Completed and cfg sets no Storage (flip it to Running and it nil-panics). Reword to name the terminal short-circuit it actually pins.

10. Duplicate capture logger — levelCapturingLogger (comment_authority_integration_test.go:22) is capturingLogger (comment_observer_test.go:230) with a recording Info; both compile into the integration build. Add an infos slice to the existing type instead.

11. Doc comment slightly overclaims — "exactly one observer at a time edits the comment" (:777) holds among authority-path observers but not against a lease-admitted one, which never touches observer_owner. Scope the sentence to the authority path.

The one thing that could have broken, verified

The cross-pod CAS in ClaimProgressCommentAuthority (apply_comments.go:337) is now the only thing stopping two pods from editing the same progress comment. Concurrent claimants serialize on the row lock; the loser re-evaluates the WHERE against the committed row, sees the fresh owner, gets 0 rows, and correctly loses. The subtle part is the MySQL rows-changed=0 fallback returning true on current.String == owner — safe, because the observer_owner = ? disjunct means a row recording the caller always matches, so 0 rows there can only be a same-second identical-value no-op by the current holder (Postgres never reaches the fallback). No double-owner window found.

Verified correct

• Placeholder order (owner, applyID, progress, owner, staleSeconds) matches the five ? after dialect.RelativeTime expands on both dialects.
• OnTerminal still fails closed: a terminal apply short-circuits the in-flight gate, so the check-run OnTerminalHook stays unreachable without a lease — tier-0 check state untouched.
• The deleted "apply lease unavailable" early return is re-established for ordinary and CLI applies: no keyed ops ⇒ inFlight == false ⇒ Error log + false at :796.
• Whole-deployment operations (empty OperationKey) are excluded, keeping the lease authoritative for the single-operation shape.
• contextWithApplyLease's new no-lease branch stays consistent with the gate because notifyObserverUntilTerminal re-reads the lease columns every tick.
• The claim bumps updated_at only on the progress row; every summary staleness query filters comment_state = 'summary', so summary claim/reclaim is unaffected.
• TestLeaseHeldApplyKeepsLeaseAuthoritative is non-vacuous (asserts observer_owner stays NULL), and the concurrent-observer integration test genuinely loses the CAS with a distinct pod identity.
• o.applyID == 0 degrades safely: ListByApply(0) returns no ops, so the gate denies the side effect.

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

[aparajon]

🤖 All 11 findings addressed in bfe6d40 (gate rework) and 5295aea (cross-dialect tests). Per finding:

1. PG pre-deploy ALTER — added an "Operational note" section to the PR body with the exact ALTER TABLE apply_comments ADD COLUMN … statement and a note that MySQL needs no action. No code change, per the finding.

2. Drive-mode split — the in-flight gate now mirrors the operator's predicate: after the missing-manifest-keys check, len(ops) <= 1 returns not-in-flight, so a single keyed operation (which drives under the parent lease) keeps the lease checks as the only authority. One deliberate divergence: the taskless single-keyed-op shape does drive under an operation lease but still fails closed here — it has no task progress to report, its terminal comment publishes via the aggregate-projection CAS winner, and fail-closed matches its pre-PR behavior. The code comment documents this. Covered by a new subtest ("a single keyed operation drives under the parent lease") plus mutation checks (<= 1 → < 1 fails it).

3. Fresh lease re-read — decideProgressCommentAuthority now re-reads the apply row via Applies().Get and denies when fresh.Lease().Valid() (a dispatch wave re-claimed the parent mid-tick) or the apply settled terminal; storage error and vanished-row cases deny at Error. Unit-tested in TestProgressCommentAuthorityDeniesOnFreshApplyRowChanges (4 subtests) and integration-tested end-to-end in TestParentLeaseReclaimAfterSnapshotDeniesAuthority, which re-claims the lease in SQL after the poller snapshot and asserts no GitHub calls and observer_owner stays NULL.

4. Cross-dialect coverage — 5295aea moves the fresh-row claim semantics (first-claim wins, same-owner renewal including identical values, peer loss, per-apply independence) into storagetest.TestApplyComments, so they run under both TestStorageParity (MySQL) and TestPostgresStorageParity — verified both executed and passed via -v. The aged-row stale takeover is per-dialect with each dialect's own backdating SQL (ApplyCommentProgressAuthorityStaleTakeover on PG; the slimmed MySQL test keeps its aged-row scenario).

5. Once per callback — the decision is now memoized per observer callback (authorityDecided/authorityHeld under a dedicated mutex, reset at the top of OnProgress and OnTerminal), so one ops scan + one apply re-read + one claim write serve all of a tick's side-effect checks. TestProgressCommentAuthorityDecidesOncePerCallback proves it with a counting stub: two gate calls → one ListByApply; after the reset → two.

6. Log level — "authority not won" is now Debug; the observer's logger interface (both the struct field and CommentObserverConfig.Logger) gained Debug(msg string, args ...any), which *slog.Logger already satisfies in production wiring.

7. State helper — the operation loop uses state.IsApplyOperationTerminal.

8. Non-vacuous terminal subtest — the "terminal apply" subtest now seeds two keyed ops with one Running, so only the terminal short-circuit can produce the deny; deleting the short-circuit fails the test (mutation-verified).

9. Assertion message — reworded to name what it pins: "a terminal apply short-circuits the in-flight gate, so a normal observer with no apply lease fails closed".

10. Duplicate logger — levelCapturingLogger is deleted; capturingLogger records debugs/infos/errors and both builds share it.

11. Doc comment — the single-editor sentence is scoped to authority-path observers, and the comment now describes the once-per-callback fresh-row decision.

Verification: build + vet clean (plain and -tags integration), go test -race ./pkg/webhook, the webhook authority integration tests, and the storage parity suites on both dialects all green; 4 targeted mutants (predicate boundary, lease re-read, memo, terminal short-circuit) each fail their pinning test.

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