From dd1bda0e12763f0e04249623b7e811cbb9cbe238 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Fri, 7 Aug 2026 16:17:54 -0400 Subject: [PATCH 1/3] fix(github): resolve the apply an operator meant when a control command names none MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `schemabot apply` takes no apply ID, so operators reach for `schemabot cancel -e ` the same way. Today every apply-scoped control command answers with a usage line, sending them to `schemabot status` to copy back an identifier SchemaBot already has. A control command that names no apply is now resolved from the PR: acted on when exactly one schema change in that environment could be meant, and answered with the candidates themselves when several could. Nothing is guessed — an ambiguous command still cancels nothing. Stopped applies count as candidates. Stopping leaves a schema change half-applied and cancel is how an operator finishes it, so the state cancel is most often issued against stays reachable. Inference is scoped to what one deployment stores, so an unscoped command on an aggregate repo keeps the usage reply: each participant holds its own slice of the PR's applies, and "exactly one here" is not "exactly one on this PR". Co-Authored-By: Claude Fable 5 --- TEMPLATES.md | 17 ++ pkg/cmd/internal/templates/preview_comment.go | 2 + pkg/webhook/control.go | 159 +++++++++++++++--- pkg/webhook/control_integration_test.go | 139 +++++++++++++++ pkg/webhook/templates/issue_comment.go | 59 ++++++- 5 files changed, 348 insertions(+), 28 deletions(-) diff --git a/TEMPLATES.md b/TEMPLATES.md index 5f202e71d..277220a44 100644 --- a/TEMPLATES.md +++ b/TEMPLATES.md @@ -3751,6 +3751,23 @@ Use `schemabot status -e ` to find the apply ID. +
+Control Command Ambiguous Apply ID + + +## Which Schema Change? + +This PR has more than one schema change in `staging`, so `schemabot cancel` needs to be told which one. + +| Apply | Database | State | +|---|---|---| +| `apply_a1b2c3d4` | `orders` | running | +| `apply_e5f6a7b8` | `customers` | stopped | + +Usage: `schemabot cancel -e staging` + +
+
Volume Changed: Superseded Progress Comment diff --git a/pkg/cmd/internal/templates/preview_comment.go b/pkg/cmd/internal/templates/preview_comment.go index 7a3d2a216..155451c15 100644 --- a/pkg/cmd/internal/templates/preview_comment.go +++ b/pkg/cmd/internal/templates/preview_comment.go @@ -89,6 +89,7 @@ func previewCommentAllOutput() { {"VOLUME COMMAND ACCEPTED", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeCommandAccepted()) }}, {"VOLUME COMMAND INVALID LEVEL", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeInvalidLevel()) }}, {"VOLUME COMMAND MISSING APPLY ID", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeMissingApplyID()) }}, + {"CONTROL COMMAND AMBIGUOUS APPLY ID", func() { fmt.Print(webhooktemplates.PreviewCommentControlAmbiguousApplyID()) }}, {"VOLUME CHANGED: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeSupersededProgress()) }}, {"RESUMED: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentResumeSupersededProgress()) }}, {"REVERT: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentRevertSupersededProgress()) }}, @@ -266,6 +267,7 @@ func previewCommentApplyFlowAllOutput() { {"VOLUME COMMAND ACCEPTED", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeCommandAccepted()) }}, {"VOLUME COMMAND INVALID LEVEL", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeInvalidLevel()) }}, {"VOLUME COMMAND MISSING APPLY ID", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeMissingApplyID()) }}, + {"CONTROL COMMAND AMBIGUOUS APPLY ID", func() { fmt.Print(webhooktemplates.PreviewCommentControlAmbiguousApplyID()) }}, {"VOLUME CHANGED: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentVolumeSupersededProgress()) }}, {"RESUMED: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentResumeSupersededProgress()) }}, {"REVERT: SUPERSEDED PROGRESS COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentRevertSupersededProgress()) }}, diff --git a/pkg/webhook/control.go b/pkg/webhook/control.go index a1f5ae31f..89e552fc5 100644 --- a/pkg/webhook/control.go +++ b/pkg/webhook/control.go @@ -7,6 +7,7 @@ import ( "github.com/block/schemabot/pkg/api" "github.com/block/schemabot/pkg/apitypes" + "github.com/block/schemabot/pkg/state" "github.com/block/schemabot/pkg/storage" "github.com/block/schemabot/pkg/webhook/action" "github.com/block/schemabot/pkg/webhook/templates" @@ -33,20 +34,117 @@ func (h *Handler) logControlCommandError(command, repo string, pr int, applyID, } } -func (h *Handler) loadApplyForPRControl(ctx context.Context, repo string, pr int, installationID int64, requestedBy string, result CommandResult, command string) (*storage.Apply, bool) { - if result.ApplyID == "" { - if h.silentUsageErrorOnUnscopedFanOut(repo, result.Tenant) { - h.logger.Info("skipping missing-apply-id reply for unscoped fan-out control command; the leader posts it once", - "command", command, - "repo", repo, - "pr", pr, - "environment", result.Environment, - "requested_by", requestedBy) - return nil, false +// controllableApplyState reports whether a control command can still act on an +// apply in this state. Stopped is terminal but not settled — stopping leaves the +// schema change half-applied, and cancel is how an operator finishes it — so a +// stopped apply is still something an operator's command can be about. +func controllableApplyState(applyState string) bool { + return !state.IsTerminalApplyState(applyState) || state.IsState(applyState, state.Apply.Stopped) +} + +// inferApplyForPRControl resolves the apply an operator meant when their control +// command named no apply ID. +// +// `schemabot apply` takes no ID, so operators reach for `schemabot cancel -e +// ` the same way; on a PR carrying a single schema change in that +// environment there is only one apply the command can be about, and demanding +// the ID sends them to `schemabot status` to copy back a value SchemaBot already +// knows. Anything less certain than one candidate is answered, not guessed: no +// candidates gets the usage reply, and several get their identifiers listed so +// the operator names the one they mean. +// +// Inference is scoped to what this deployment stores, so it is only sound when +// this deployment is the one answering. An unscoped command on an aggregate repo +// fans out to deployments that each hold their own slice of the PR's applies, +// where "exactly one here" does not mean "exactly one on this PR" — those keep +// the usage reply and its explicit ID. +func (h *Handler) inferApplyForPRControl( + ctx context.Context, applyStore storage.ApplyStore, + repo string, pr int, installationID int64, requestedBy string, + result CommandResult, command string, +) (*storage.Apply, bool) { + if h.silentOnUnscopedFanOut(repo, result.Tenant) { + h.logger.Info("not inferring an apply id for an unscoped fan-out control command; this deployment holds only part of the PR's applies", + "command", command, "repo", repo, "pr", pr, + "environment", result.Environment, "requested_by", requestedBy) + return nil, false + } + + applies, err := applyStore.GetByPR(ctx, repo, pr) + if err != nil { + h.logger.Error("failed to load the PR's applies to infer the apply id for a control command", + "command", command, "repo", repo, "pr", pr, + "environment", result.Environment, "requested_by", requestedBy, "error", err) + h.postCommandError(repo, pr, installationID, command, result.Environment, requestedBy, "Failed to look up this PR's schema changes: "+err.Error()) + return nil, false + } + + var candidates []*storage.Apply + for _, apply := range applies { + if apply.Environment == result.Environment && controllableApplyState(apply.State) { + candidates = append(candidates, apply) + } + } + + if len(candidates) == 1 { + h.logger.Info("inferred the apply id for a control command from the PR's only schema change in this environment", + append(candidates[0].LogAttrs(), "command", command, "requested_by", requestedBy)...) + return candidates[0], true + } + h.logger.Info("control command named no apply id and the PR's applies do not resolve it to one", + "command", command, "repo", repo, "pr", pr, + "environment", result.Environment, "requested_by", requestedBy, + "candidates", len(candidates)) + return nil, false +} + +// replyForUnresolvedApplyID answers a control command whose apply the PR could +// not resolve: the usage line when nothing on the PR matches, and the candidates +// themselves when several do. +func (h *Handler) replyForUnresolvedApplyID( + ctx context.Context, applyStore storage.ApplyStore, + repo string, pr int, installationID int64, requestedBy string, + result CommandResult, command string, +) { + if h.silentUsageErrorOnUnscopedFanOut(repo, result.Tenant) { + h.logger.Info("skipping missing-apply-id reply for unscoped fan-out control command; the leader posts it once", + "command", command, + "repo", repo, + "pr", pr, + "environment", result.Environment, + "requested_by", requestedBy) + return + } + + applies, err := applyStore.GetByPR(ctx, repo, pr) + if err != nil { + // The usage reply stands on its own, so a lookup failure here costs the + // operator the candidate list, not the answer. + h.logger.Warn("failed to load the PR's applies to list control command candidates; replying with usage only", + "command", command, "repo", repo, "pr", pr, + "environment", result.Environment, "requested_by", requestedBy, "error", err) + h.postComment(repo, pr, installationID, templates.RenderControlMissingApplyID(command)) + return + } + + var candidates []templates.ControlApplyCandidate + for _, apply := range applies { + if apply.Environment == result.Environment && controllableApplyState(apply.State) { + candidates = append(candidates, templates.ControlApplyCandidate{ + ApplyID: apply.ApplyIdentifier, + Database: apply.Database, + State: apply.State, + }) } + } + if len(candidates) == 0 { h.postComment(repo, pr, installationID, templates.RenderControlMissingApplyID(command)) - return nil, false + return } + h.postComment(repo, pr, installationID, templates.RenderControlAmbiguousApplyID(command, result.Environment, candidates)) +} + +func (h *Handler) loadApplyForPRControl(ctx context.Context, repo string, pr int, installationID int64, requestedBy string, result CommandResult, command string) (*storage.Apply, bool) { if h.service == nil { h.logger.Error("service not configured for PR control command", "command", command, @@ -82,6 +180,19 @@ func (h *Handler) loadApplyForPRControl(ctx context.Context, repo string, pr int h.postCommandError(repo, pr, installationID, command, result.Environment, requestedBy, "SchemaBot apply storage is not configured for "+command+" commands") return nil, false } + // An operator who names no apply is answered from the PR itself: acted on + // when the PR resolves to exactly one candidate, and asked which one when it + // does not. + if result.ApplyID == "" { + inferred, ok := h.inferApplyForPRControl(ctx, applyStore, repo, pr, installationID, requestedBy, result, command) + if !ok { + h.replyForUnresolvedApplyID(ctx, applyStore, repo, pr, installationID, requestedBy, result, command) + return nil, false + } + h.acknowledgeCommandActPoint(repo, pr, installationID, result) + return inferred, true + } + apply, err := applyStore.GetByApplyIdentifier(ctx, result.ApplyID) if err != nil { h.logger.Error("failed to load apply for PR control command", @@ -159,16 +270,22 @@ func runControlCommand[R any]( h *Handler, ctx context.Context, repo string, pr int, installationID int64, requestedBy string, - result CommandResult, + result *CommandResult, actionName string, execute func(ctx context.Context, req apitypes.ControlRequest) (*R, error), accepted func(*R) bool, errorMessage func(*R) string, ) *R { - apply, ok := h.loadApplyForPRControl(ctx, repo, pr, installationID, requestedBy, result, actionName) + apply, ok := h.loadApplyForPRControl(ctx, repo, pr, installationID, requestedBy, *result, actionName) if !ok { return nil } + // The apply the command acts on is settled here, whether the operator named + // it or the PR resolved it. The result is threaded by pointer so everything + // downstream — the request, the logs, and the caller's acknowledgement + // comment — reports that apply, and an inferred command reads back the + // identifier it acted on rather than a blank. + result.ApplyID = apply.ApplyIdentifier client, blocked := h.actorAuthorizationClient(repo, pr, installationID, requestedBy, apply.Database, result.Environment, actionName) if blocked { return nil @@ -227,7 +344,7 @@ func (h *Handler) handleStopCommand(repo string, pr int, installationID int64, r ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Stop, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Stop, h.service.ExecuteStop, func(r *apitypes.StopResponse) bool { return r.Accepted }, func(r *apitypes.StopResponse) string { return r.ErrorMessage }) @@ -260,7 +377,7 @@ func (h *Handler) handleCancelCommand(repo string, pr int, installationID int64, ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Cancel, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Cancel, h.service.ExecuteCancel, func(r *apitypes.CancelResponse) bool { return r.Accepted }, func(r *apitypes.CancelResponse) string { return r.ErrorMessage }) @@ -293,7 +410,7 @@ func (h *Handler) handleStartCommand(repo string, pr int, installationID int64, ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Start, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Start, h.service.ExecuteStart, func(r *apitypes.StartResponse) bool { return r.Accepted }, func(r *apitypes.StartResponse) string { return r.ErrorMessage }) @@ -327,7 +444,7 @@ func (h *Handler) handleReleaseCommand(repo string, pr int, installationID int64 ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Release, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Release, h.service.ExecuteRelease, func(r *apitypes.ReleaseResponse) bool { return r.Accepted }, func(r *apitypes.ReleaseResponse) string { return r.ErrorMessage }) @@ -356,7 +473,7 @@ func (h *Handler) handleCutoverCommand(repo string, pr int, installationID int64 ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Cutover, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Cutover, h.service.ExecuteCutover, func(r *apitypes.ControlResponse) bool { return r.Accepted }, func(r *apitypes.ControlResponse) string { return r.ErrorMessage }) @@ -423,7 +540,7 @@ func (h *Handler) handleVolumeCommand(repo string, pr int, installationID int64, return } - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Volume, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Volume, func(ctx context.Context, req apitypes.ControlRequest) (*apitypes.VolumeResponse, error) { return h.service.ExecuteVolume(ctx, req, result.VolumeLevel) }, @@ -452,7 +569,7 @@ func (h *Handler) handleSkipRevertCommand(repo string, pr int, installationID in ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.SkipRevert, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.SkipRevert, h.service.ExecuteSkipRevert, func(r *apitypes.ControlResponse) bool { return r.Accepted }, func(r *apitypes.ControlResponse) string { return r.ErrorMessage }) @@ -477,7 +594,7 @@ func (h *Handler) handleRevertCommand(repo string, pr int, installationID int64, ctx, cancel := h.commandContext(context.Background(), commandTimeout) defer cancel() - resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, result, action.Revert, + resp := runControlCommand(h, ctx, repo, pr, installationID, requestedBy, &result, action.Revert, h.service.ExecuteRevert, func(r *apitypes.ControlResponse) bool { return r.Accepted }, func(r *apitypes.ControlResponse) string { return r.ErrorMessage }) diff --git a/pkg/webhook/control_integration_test.go b/pkg/webhook/control_integration_test.go index 3a2de8d4d..2b882888e 100644 --- a/pkg/webhook/control_integration_test.go +++ b/pkg/webhook/control_integration_test.go @@ -1214,3 +1214,142 @@ func (c *stopCommandTernClient) SetPendingObserver(tern.ProgressObserver) {} func (c *stopCommandTernClient) SetObserver(int64, tern.ProgressObserver) {} func (c *stopCommandTernClient) Close() error { return nil } + +// postCancelCommandWithoutApplyID issues the command the way an operator reaches +// for it after `schemabot apply`, which takes no apply ID either. +func postCancelCommandWithoutApplyID(t *testing.T, h *Handler, user string) { + t.Helper() + req := buildWebhookRequest(t, webhookPayloadOpts{ + comment: "schemabot cancel -e staging", + userLogin: user, + isPR: true, + }, nil) + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + require.Equal(t, http.StatusOK, rr.Code) +} + +// captureCommentsAndReactions routes the PR's comment and reaction posts into +// channels a control-command test can read. +func captureCommentsAndReactions(t *testing.T, mux *http.ServeMux) (chan string, chan string) { + t.Helper() + comments := make(chan string, 10) + reactions := make(chan string, 10) + mux.HandleFunc("POST /repos/octocat/hello-world/issues/1/comments", func(w http.ResponseWriter, r *http.Request) { + var body struct { + Body string `json:"body"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + comments <- body.Body + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{"id": 99}) + }) + mux.HandleFunc("POST /repos/octocat/hello-world/issues/comments/42/reactions", func(w http.ResponseWriter, r *http.Request) { + var body struct { + Content string `json:"content"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + reactions <- body.Content + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{"id": 1}) + }) + return comments, reactions +} + +// `schemabot apply` takes no apply ID, so operators reach for `schemabot cancel +// -e ` the same way. On a PR carrying a single schema change in that +// environment there is only one apply the command can be about, so it acts on +// that one instead of sending the operator to `schemabot status` to copy back an +// identifier SchemaBot already has. +func TestE2ECancelCommandWithoutApplyIDActsOnThePRsOnlySchemaChange(t *testing.T) { + ctx := t.Context() + schemabotDB, err := sql.Open("mysql", e2eSchemabotDSN) + require.NoError(t, err) + require.NoError(t, schemabotDB.PingContext(ctx)) + + store := mysqlstore.New(schemabotDB) + applyIdentifier := "apply_1e55c0de" + database := "cancel_inferred_only_db" + cleanupStopCommandTestRows(t, schemabotDB, applyIdentifier, database) + t.Cleanup(func() { + cleanupStopCommandTestRows(t, schemabotDB, applyIdentifier, database) + utils.CloseAndLog(schemabotDB) + }) + applyID := createStopCommandApply(t, store, applyIdentifier, database) + + client, mux := setupGitHubServer(t) + comments, reactions := captureCommentsAndReactions(t, mux) + + service := apiServiceForStopCommandTest(t, store, database) + service.RegisterTernClient(database, "staging", &stopCommandTernClient{remote: true}) + h := &Handler{ + service: service, + ghClients: ghclient.NewSingleClientSet(defaultAppName, &fakeClientFactory{client: ghclient.NewInstallationClient(client, testLogger())}), + logger: testLogger(), + } + + postCancelCommandWithoutApplyID(t, h, "alice") + + comment := readComment(t, comments) + assert.Contains(t, comment, "Cancel Request Accepted") + // The reply names the apply it acted on, so an operator who never typed an + // identifier still knows which schema change they just cancelled. + assert.Contains(t, comment, "`"+applyIdentifier+"`") + + controlReq, err := store.ControlRequests().GetPending(ctx, applyID, storage.ControlOperationCancel) + require.NoError(t, err) + require.NotNil(t, controlReq, "the PR's only schema change was not cancelled") + assert.Equal(t, "github:alice@octocat/hello-world#1", controlReq.RequestedBy) + assertReactionEventually(t, reactions) +} + +// With more than one schema change in the environment the command is genuinely +// ambiguous, and guessing would cancel work the operator did not name. The reply +// lists the candidates so they reissue the command against the one they mean, +// and nothing is cancelled in the meantime. +func TestE2ECancelCommandWithoutApplyIDListsCandidatesWhenThePRHasSeveral(t *testing.T) { + ctx := t.Context() + schemabotDB, err := sql.Open("mysql", e2eSchemabotDSN) + require.NoError(t, err) + require.NoError(t, schemabotDB.PingContext(ctx)) + + store := mysqlstore.New(schemabotDB) + firstIdentifier, secondIdentifier := "apply_a3b1c0de", "apply_b7d2e0fa" + firstDatabase, secondDatabase := "cancel_ambiguous_db_a", "cancel_ambiguous_db_b" + cleanup := func() { + cleanupStopCommandTestRows(t, schemabotDB, firstIdentifier, firstDatabase) + cleanupStopCommandTestRows(t, schemabotDB, secondIdentifier, secondDatabase) + } + cleanup() + t.Cleanup(func() { + cleanup() + utils.CloseAndLog(schemabotDB) + }) + firstApplyID := createStopCommandApply(t, store, firstIdentifier, firstDatabase) + secondApplyID := createStopCommandApply(t, store, secondIdentifier, secondDatabase) + + client, mux := setupGitHubServer(t) + comments, _ := captureCommentsAndReactions(t, mux) + + service := apiServiceForStopCommandTest(t, store, firstDatabase) + service.RegisterTernClient(firstDatabase, "staging", &stopCommandTernClient{remote: true}) + h := &Handler{ + service: service, + ghClients: ghclient.NewSingleClientSet(defaultAppName, &fakeClientFactory{client: ghclient.NewInstallationClient(client, testLogger())}), + logger: testLogger(), + } + + postCancelCommandWithoutApplyID(t, h, "alice") + + comment := readComment(t, comments) + assert.Contains(t, comment, "Which Schema Change?") + assert.Contains(t, comment, "`"+firstIdentifier+"`") + assert.Contains(t, comment, "`"+secondIdentifier+"`") + assert.Contains(t, comment, "`schemabot cancel -e staging`") + + for _, applyID := range []int64{firstApplyID, secondApplyID} { + controlReq, err := store.ControlRequests().GetPending(ctx, applyID, storage.ControlOperationCancel) + require.NoError(t, err) + assert.Nil(t, controlReq, "an ambiguous cancel must not settle on one of the candidates") + } +} diff --git a/pkg/webhook/templates/issue_comment.go b/pkg/webhook/templates/issue_comment.go index 01316f9b8..5edb770be 100644 --- a/pkg/webhook/templates/issue_comment.go +++ b/pkg/webhook/templates/issue_comment.go @@ -88,18 +88,54 @@ type CutoverCommandAcceptedData struct { Status string } -// RenderControlMissingApplyID renders the message posted when an apply-scoped -// control command is invoked without the required apply ID. The usage line -// carries every flag the command requires, so a volume command also shows its -// mandatory `-v` level. -func RenderControlMissingApplyID(command string) string { - usage := fmt.Sprintf("schemabot %s -e ", command) +// controlCommandUsage renders a control command's usage line, carrying every +// flag the command requires — a volume command also shows its mandatory `-v` +// level. +func controlCommandUsage(command, environment string) string { + if environment == "" { + environment = "" + } + usage := fmt.Sprintf("schemabot %s -e %s", command, environment) if command == action.Volume { usage += fmt.Sprintf(" -v <%d-%d>", storage.MinVolume, storage.MaxVolume) } + return usage +} + +// RenderControlMissingApplyID renders the message posted when an apply-scoped +// control command names no apply ID and the PR holds nothing the command could +// have meant. +func RenderControlMissingApplyID(command string) string { return offerSupportChannel(fmt.Sprintf("## Missing Apply ID\n\n"+ "Usage: `%s`\n\n"+ - "Use `schemabot status -e ` to find the apply ID.", usage)) + "Use `schemabot status -e ` to find the apply ID.", controlCommandUsage(command, ""))) +} + +// ControlApplyCandidate is one schema change an apply-scoped control command +// could have meant. +type ControlApplyCandidate struct { + ApplyID string + Database string + State string +} + +// RenderControlAmbiguousApplyID renders the reply to an apply-scoped control +// command that named no apply ID on a PR carrying several the command could +// have meant. +// +// The operator's next step is to reissue the command with one of these +// identifiers, and they are all right here — so unlike the missing-ID reply +// this carries no support-channel link. Sending someone to a support channel to +// read back a list already in front of them is noise. +func RenderControlAmbiguousApplyID(command, environment string, candidates []ControlApplyCandidate) string { + var body strings.Builder + body.WriteString(fmt.Sprintf("## Which Schema Change?\n\n"+ + "This PR has more than one schema change in `%s`, so `schemabot %s` needs to be told which one.\n\n"+ + "| Apply | Database | State |\n|---|---|---|\n", environment, command)) + for _, c := range candidates { + body.WriteString(fmt.Sprintf("| `%s` | `%s` | %s |\n", c.ApplyID, c.Database, c.State)) + } + return body.String() + fmt.Sprintf("\nUsage: `%s`\n", controlCommandUsage(command, environment)) } // RenderVolumeInvalidLevel renders the message posted when a volume command @@ -306,6 +342,15 @@ func PreviewCommentVolumeMissingApplyID() string { return RenderControlMissingApplyID(action.Volume) } +// PreviewCommentControlAmbiguousApplyID renders the reply posted when a control +// command names no apply ID on a PR carrying several it could have meant. +func PreviewCommentControlAmbiguousApplyID() string { + return RenderControlAmbiguousApplyID(action.Cancel, "staging", []ControlApplyCandidate{ + {ApplyID: "apply_a1b2c3d4", Database: "orders", State: "running"}, + {ApplyID: "apply_e5f6a7b8", Database: "customers", State: "stopped"}, + }) +} + // VolumeSupersededProgressData contains data for freezing a progress comment // that a volume change has superseded. type VolumeSupersededProgressData struct { From fc3b8f2bdd17024564b15d07e459d87c3b6f24a0 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Fri, 7 Aug 2026 16:57:29 -0400 Subject: [PATCH 2/3] fix(github): build the candidate table with Fprintf Co-Authored-By: Claude Fable 5 --- pkg/webhook/templates/issue_comment.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/webhook/templates/issue_comment.go b/pkg/webhook/templates/issue_comment.go index 5edb770be..d03ca4104 100644 --- a/pkg/webhook/templates/issue_comment.go +++ b/pkg/webhook/templates/issue_comment.go @@ -129,13 +129,14 @@ type ControlApplyCandidate struct { // read back a list already in front of them is noise. func RenderControlAmbiguousApplyID(command, environment string, candidates []ControlApplyCandidate) string { var body strings.Builder - body.WriteString(fmt.Sprintf("## Which Schema Change?\n\n"+ + fmt.Fprintf(&body, "## Which Schema Change?\n\n"+ "This PR has more than one schema change in `%s`, so `schemabot %s` needs to be told which one.\n\n"+ - "| Apply | Database | State |\n|---|---|---|\n", environment, command)) + "| Apply | Database | State |\n|---|---|---|\n", environment, command) for _, c := range candidates { - body.WriteString(fmt.Sprintf("| `%s` | `%s` | %s |\n", c.ApplyID, c.Database, c.State)) + fmt.Fprintf(&body, "| `%s` | `%s` | %s |\n", c.ApplyID, c.Database, c.State) } - return body.String() + fmt.Sprintf("\nUsage: `%s`\n", controlCommandUsage(command, environment)) + fmt.Fprintf(&body, "\nUsage: `%s`\n", controlCommandUsage(command, environment)) + return body.String() } // RenderVolumeInvalidLevel renders the message posted when a volume command From c05c8e4d3985c6196636ba2a6999c6ac466db49a Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Fri, 7 Aug 2026 17:21:12 -0400 Subject: [PATCH 3/3] fix(github): keep a fan-out reply from presenting one deployment's slice Two ways an unresolved apply id reached the operator with more than it should. A storage lookup failure rendered the raw error into a public PR comment, and on an aggregate repo the candidate table was built from this deployment's own applies, presenting a partial view of the PR as the whole of it. The lookup failure now posts a fixed line and keeps the cause in the server log, and an unscoped fan-out falls back to the usage reply the leader posts once. Co-Authored-By: Claude Fable 5 --- pkg/webhook/control.go | 15 +++++++- pkg/webhook/control_integration_test.go | 47 +++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/pkg/webhook/control.go b/pkg/webhook/control.go index 89e552fc5..9f629f555 100644 --- a/pkg/webhook/control.go +++ b/pkg/webhook/control.go @@ -75,7 +75,11 @@ func (h *Handler) inferApplyForPRControl( h.logger.Error("failed to load the PR's applies to infer the apply id for a control command", "command", command, "repo", repo, "pr", pr, "environment", result.Environment, "requested_by", requestedBy, "error", err) - h.postCommandError(repo, pr, installationID, command, result.Environment, requestedBy, "Failed to look up this PR's schema changes: "+err.Error()) + // The lookup failure is logged above with the identifiers that make it + // triageable. The PR comment is public, so it carries the outcome rather + // than raw storage text naming hosts, addresses, or driver internals. + h.postCommandError(repo, pr, installationID, command, result.Environment, requestedBy, + "Failed to look up this PR's schema changes. Retry, and see server logs if it persists.") return nil, false } @@ -116,6 +120,15 @@ func (h *Handler) replyForUnresolvedApplyID( return } + // On an unscoped fan-out this deployment stores only its own slice of the + // PR's applies, which is why the apply id was not inferred. A candidate list + // built from that slice would present a partial view of the PR as the whole + // of it, so the reply stays the usage line the leader posts once. + if h.silentOnUnscopedFanOut(repo, result.Tenant) { + h.postComment(repo, pr, installationID, templates.RenderControlMissingApplyID(command)) + return + } + applies, err := applyStore.GetByPR(ctx, repo, pr) if err != nil { // The usage reply stands on its own, so a lookup failure here costs the diff --git a/pkg/webhook/control_integration_test.go b/pkg/webhook/control_integration_test.go index 2b882888e..ec1c4c673 100644 --- a/pkg/webhook/control_integration_test.go +++ b/pkg/webhook/control_integration_test.go @@ -1353,3 +1353,50 @@ func TestE2ECancelCommandWithoutApplyIDListsCandidatesWhenThePRHasSeveral(t *tes assert.Nil(t, controlReq, "an ambiguous cancel must not settle on one of the candidates") } } + +// An unscoped control command on an aggregate repo fans out to deployments that +// each store their own slice of the PR's applies, so "exactly one here" is not a +// statement about the PR. The reply stays the usage line rather than a candidate +// table this deployment cannot see the whole of. +func TestE2ECancelCommandWithoutApplyIDDoesNotListCandidatesOnAnAggregateFanOut(t *testing.T) { + ctx := t.Context() + schemabotDB, err := sql.Open("mysql", e2eSchemabotDSN) + require.NoError(t, err) + require.NoError(t, schemabotDB.PingContext(ctx)) + + store := mysqlstore.New(schemabotDB) + applyIdentifier, database := "apply_c9f4a1bb", "cancel_fanout_db" + cleanup := func() { cleanupStopCommandTestRows(t, schemabotDB, applyIdentifier, database) } + cleanup() + t.Cleanup(func() { + cleanup() + utils.CloseAndLog(schemabotDB) + }) + applyID := createStopCommandApply(t, store, applyIdentifier, database) + + client, mux := setupGitHubServer(t) + comments, _ := captureCommentsAndReactions(t, mux) + + service := apiServiceForStopCommandTest(t, store, database) + service.Config().Repos = map[string]api.RepoConfig{ + "octocat/hello-world": {Aggregate: &api.AggregateConfig{Role: api.AggregateRoleLeader}}, + } + service.RegisterTernClient(database, "staging", &stopCommandTernClient{remote: true}) + h := &Handler{ + service: service, + ghClients: ghclient.NewSingleClientSet(defaultAppName, &fakeClientFactory{client: ghclient.NewInstallationClient(client, testLogger())}), + logger: testLogger(), + } + + postCancelCommandWithoutApplyID(t, h, "alice") + + comment := readComment(t, comments) + assert.Contains(t, comment, "Missing Apply ID") + assert.NotContains(t, comment, "Which Schema Change?") + assert.NotContains(t, comment, applyIdentifier, + "a deployment holding one slice of the PR must not present its slice as the PR's candidates") + + controlReq, err := store.ControlRequests().GetPending(ctx, applyID, storage.ControlOperationCancel) + require.NoError(t, err) + assert.Nil(t, controlReq, "a fan-out cancel with no apply id must not settle on this deployment's apply") +}