feat(github): mirror remote control-request rejections back to the accepting plane

[aparajon]

Why this matters

Running stop, cutover, or volume on a schema change that a remote data plane is driving gets you an acknowledgement right away, but that only means the command was queued. If the engine then refuses it, the failure stays on the far side and never travels back, so the command looks like it worked when it did not.

What changed

The data plane now reports what happened to every control command it finished with. The accepting plane records that, counts it by operation and engine, and puts a warning on the apply's PR comment. The notice clears once the command is re-issued and succeeds.

BEFORE — the rejection stops at the data plane

  operator ──► control plane ──► data plane ──► engine
   volume 50    accepts, ACKs      forwards      rejects
                                       │
                                       └─► settles in the data plane's
                                           row and goes no further

  PR comment: unchanged. The operator has an ACK and no outcome.


AFTER — the rejection travels back

  operator ──► control plane ──► data plane ──► engine
   volume 50    accepts, ACKs      forwards      rejects
                                       │
                      ┌────────────────┘  next progress poll
                      ▼
                control plane   records it durably, counts it by
                                operation and engine
                      │
                      ▼
  PR comment: ⚠️ volume was not applied (requested by octocat)


  operator re-issues, it succeeds
        │
        └─► data plane reports completed ─► notice clears on next render

The new field is additive, so neither plane has to deploy first, but the warning only appears once both are on a version that carries it.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR adds end-to-end propagation of terminal control-request outcomes (especially remote/data-plane rejections) back to the accepting plane, so operators can see when a queued command ultimately did not take effect, and so the PR comment/summary can surface those rejections safely.

Changes:

• Extend the Tern ProgressResponse to include settled_control_requests, allowing the accepting plane to learn terminal control-request outcomes from the serving plane.
• Persist and mirror remote control-request rejections into control-plane storage, emit apply-log entries, and record an observability counter for attribution.
• Render a “Command not applied” warning section in PR comments/summaries based on stored failed control requests, with sanitization to prevent endpoint leakage and markdown escape.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/webhook/templates/control_rejection.go	Adds a template helper to render rejected control commands into a safe, quoted warning block.
pkg/webhook/templates/control_rejection_test.go	Tests rendering behavior and sanitization/blockquote containment for rejection messages.
pkg/webhook/control_rejection_notice.go	Loads failed settled control requests and appends the warning section when within GitHub comment limits.
pkg/webhook/control_rejection_notice_test.go	Ensures status comments include the rejection notice only for failed requests.
pkg/webhook/comment_observer.go	Appends the rejection notice to status and summary comment bodies.
pkg/webhook/comment_observer_test.go	Extends stub storage to support settled-control-request loading in comment rendering tests.
pkg/tern/local_client.go	Populates SettledControlRequests in Progress responses by reading terminal control requests from storage.
pkg/tern/local_client_test.go	Updates test storage to implement ListSettled and ensure ControlRequests store availability.
pkg/tern/grpc_control_rejection_integration_test.go	Adds integration coverage for Progress reporting settled requests and control-plane mirroring of remote failures.
pkg/tern/grpc_client.go	Mirrors remote control-request failures into control-plane storage, logs, and metrics during polling.
pkg/storage/types.go	Adds validation for control-operation strings crossing plane boundaries.
pkg/storage/storage.go	Extends ControlRequestStore interface with ListSettled and RecordRemoteFailure.
pkg/storage/internal/sqlstore/control_requests.go	Implements ListSettled and RecordRemoteFailure in the SQL store.
pkg/proto/tern.proto	Adds SettledControlRequest and settled_control_requests on ProgressResponse.
pkg/proto/ternv1/tern.pb.go	Regenerates protobuf bindings for the new message/field.
pkg/metrics/metrics.go	Adds a counter for remote control requests rejected by operation/engine/deployment/env attribution.

Files not reviewed (1)

• pkg/proto/ternv1/tern.pb.go: Generated file

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pull/967, 905c36e.

Verdict: 8 findings — 6 non-blocking, 2 suggestions, none blocking; CI is green (31/31 checks pass at head 905c36e: Build, Unit, Integration, Lint x4, all E2E suites including gRPC and gRPC multi-deployment, LocalScale x3) and the PR is still open with head unchanged.

Non-blocking

1. A settled rejection is permanently lost when the drive exits through the stop/cancel path. processPendingCancelOrStopControlRequest runs before the regular poll and completes the cancel via its own Progress reads, so pollForCompletion returns at pkg/tern/grpc_client.go:3758 without ever reaching the sole mirror call site at :3884, and the now-terminal apply is never polled again. A rejection that settles just after the last regular poll on a subsequently-cancelled apply never reaches storage, the PR notice, the apply log, or the metric — the exact invisibility this PR fixes; mirror from those side-path Progress reads too, or before the handled-exit return.

2. The crash-repair summary path drops the rejection notice. Missing-summary reconciliation at pkg/webhook/handler.go:536 composes the terminal summary with failureLogsSection but never appends controlRejectionSection, unlike pkg/webhook/comment_observer.go:684. If the publisher crashes before posting the terminal summary, the repaired comment silently omits the "command not applied" warning — add the section call there.

3. Volume rejections render "(requested by tern-grpc)". Volume rows are created only with RequestedBy = storage.ForwardingControlRequestCaller (pkg/tern/local_control.go:1688), and the template checks only != "" at pkg/webhook/templates/control_rejection.go:35 instead of storage.ControlRequestNamesAnOperator. The PR's headline /volume case thus attributes the command to an internal forwarding path — gate the clause on the operator-name helper.

4. New counter is missing from the metrics README label contract. schemabot.remote_control_requests.rejected_total — the first control metric with an engine label (pkg/metrics/metrics.go:792) — has no row in pkg/metrics/README.md:13, repeating the drift fixed in fix(github): dead-letter webhook deliveries that can never succeed #1023/feat(observability): distinguish transport failures and 404s in GitHub request metrics #1024. Add the row with its engine/operation/database/deployment/environment label set.

5. Silent continue on an unrecognized settled-request status. pkg/tern/grpc_client.go:1697 skips any non-failed status with no log, violating AGENTS.md "No silent branch cases" while the adjacent invalid-operation branch warns. A novel terminal status from a newer data plane (e.g. expired) would drop a rejection with zero trace — log the skip.

6. The terminal summary-comment notice path has no test. The only test (TestStatusCommentSurfacesRejectedControlCommands) exercises formatStatusComment exclusively; nothing drives summaryCommentFromOps with a failed control-request row, so a regression at pkg/webhook/comment_observer.go:684 (dropping the call or breaking the body accounting) would ship untested. Add a summary-path case.

General suggestions

7. Mirrored error messages omit the remote apply/operation ID. pkg/tern/grpc_client.go:1704 calls apply.OperatorFacingMessage(message) without additionalRemoteIDs, unlike failPendingControlRequests via pkg/tern/control_requests.go:91. On an operation-scoped drive the parent apply's ExternalID is deliberately empty, so a data-plane message naming the remote identifier surfaces un-redacted and resolves to nothing for the operator — remoteID is in scope at the mirror call site, pass it through.

8. knownControlOperations omits release. The map at pkg/metrics/metrics.go:735 lacks release while ControlOperation.Valid() accepts it (pkg/storage/types.go:961), so a mirrored release rejection would be counted as operation="unknown". Latent today (nothing fails release rows), but the two allowlists have diverged — add the entry.

The one thing that could have broken, verified

The riskiest mechanism is RecordRemoteFailure mutating the accepting plane's live apply_control_requests rows — inserting mirror-only failed rows and flipping locally-queued completed rows to failed — without corrupting the durable control-request lifecycle that forwards operator commands. Three probes proved it safe: the pending-skip guard at pkg/storage/internal/sqlstore/control_requests.go:268 commits-and-returns without writing when the existing row is pending, serialized against RequestPending by SELECT ... FOR UPDATE on the same unique (apply_id, operation) row (:397), so a re-issued command's pending row cannot be overwritten in either interleaving (pinned by TestGRPCClient_MirrorLeavesAReissuedCommandPending). Every accepting-plane consumer of these rows reads status=pending only, so a completed-to-failed flip is visible solely to the notice/ListSettled surface, never to forwarding logic. And the mirrored_remote_rejection marker is stamped only on mirror-created inserts (:312) with ClearRemoteFailure requiring it (:351), so clear-on-success provably cannot erase a failure the accepting plane recorded through its own FailPending lifecycle. The residual exposure is visibility, not corruption: Progress responses consumed outside pollForCompletion discard SettledControlRequests (finding 1).

Verified correct

• Mirror ordering: runs on every successful regular poll before the terminal check, so the last poll still mirrors and the terminal summary reads fresh rows (pkg/tern/grpc_client.go:3884).
• Pending-row guard: a re-issued operator command can never be overwritten, serialized via FOR UPDATE (pkg/storage/internal/sqlstore/control_requests.go:268).
• Exactly-once surfacing: unchanged-row dedup gates the warn log, metric, and apply-log entry across repeat polls (:291, double-mirror test).
• Clear-on-success cannot erase locally recorded failures — it requires the mirror marker (:351).
• Operator-name preservation and re-attribution in the storage merge, both test-covered (:282).
• Proto compatibility: settled_control_requests=11 is a fresh field; an old data plane yields an empty list and an early return (pkg/tern/grpc_client.go:1673).
• Notice rendering is sanitized and size-capped: fields pass through sanitizeCommentError/quoteBlockLines (pkg/webhook/templates/control_rejection.go:34), and the section self-drops before the comment cap so it cannot jointly overflow with failure logs (pkg/webhook/control_rejection_notice.go:58).
• CI: 31/31 checks pass at head 905c36e (Build, Unit, Integration, Lint x4, all E2E suites including gRPC and gRPC multi-deployment, LocalScale x3); PR still open with head unchanged.

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