fix(github): name the operator's apply id in relayed control rejections

[aparajon]

Why this matters

An apply that runs on a remote data-plane deployment exists as two records — one on each plane, each named by the side that created it. The operator addresses the control plane's record by its apply identifier; the data plane's record carries its own identifier, which the control plane stores in external_id. So when the data plane rejects a control command (volume, stop, cancel, start, revert, skip-revert), its rejection copy naturally names its own record — an identifier the operator has never seen.

The result is confusing at the worst moment. The operator just asked to change apply-vol123, and SchemaBot replies about some apply-remote999 — an id that resolves to nothing on the control plane (running progress on it returns nothing). It reads like SchemaBot answered about a different schema change, and a data-plane identifier leaks into public PR markdown.

What it does

Adds Apply.OperatorFacingMessage: before a data-plane rejection is relayed to the caller, stored on a failed control request, or written to the apply history, every remote identifier in it is rewritten to the operator-facing apply identifier.

operator: /schemabot volume 5 apply-vol123

BEFORE  ❌ Schema change apply-remote999 is completed; volume can only be
           adjusted while it is running        <- who is apply-remote999?

AFTER   ❌ Schema change apply-vol123 is completed; volume can only be
           adjusted while it is running        <- the apply you asked about

A rejection can reach the durable record by either of two paths, and both now translate:

                          ┌─ immediate attempt (API) ──┐
operator command ─ queue ─┤                            ├─▶ apply_control_requests
                          └─ driver retry (data plane) ┘      one spelling either way

The helper lives on the apply row so the API relay and the driver's retry share it — a rejection must not name the schema change differently depending on which path happened to reach the data plane. It also takes the claimed operation's remote id, which is the only remote identifier a multi-operation apply has (its parent deliberately carries none).

Alongside the translation, the rejection paths that had no server-side record of the raw text now log it before rewriting, so the data plane's own identifier stays greppable during triage. The immediate-stop apply log carries the outcome instead of the raw transport error, per the repo's rule against rendering driver text on operator-facing surfaces.

Notes on scope:

• Of the six response relays, volume and revert/skip-revert are the ones that carry data-plane rejection text today; stop, cancel, and start are covered for uniformity so a future data plane that starts interpolating an id doesn't reintroduce the leak.
• Remote start failures are stored on apply.ErrorMessage by the driver and rendered in the PR comment error block. That text names the remote id and raw gRPC error detail, so it needs sanitizing rather than translating — separate follow-up work, along with the sibling paths that post err.Error() verbatim.
• Blind whole-string replacement is safe because apply identifiers are fixed-length tokens from a single generator, so one can only contain another if the two are equal. The helper's doc records that invariant for whoever adds an engine that reports identifiers in a different shape.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR improves control-command rejection messaging in hybrid/remote deployments by rewriting data-plane apply IDs (external_id) in rejection text to the operator-facing apply identifier, preventing confusing responses and avoiding leaking internal identifiers into PR-facing output.

Changes:

• Add operatorFacingControlError to rewrite remote apply IDs in control-operation rejection messages.
• Apply the rewrite across stop/cancel/start/volume responses, immediate-stop apply-log messages, and revert/skip-revert failure persistence.
• Add a volume-handler test asserting the operator-facing ID is returned while the data plane is still addressed by its own ID.

Reviewed changes

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

File	Description
pkg/api/control_handlers.go	Introduces and applies operatorFacingControlError so relayed control rejections reference the operator-facing apply identifier.
pkg/api/handlers_test.go	Adds an HTTP-level test ensuring volume rejections rewrite the remote apply ID in the response message.

---

💡 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/905, 0c1264b.

Verdict: 3 findings — the rewrite itself is mechanically safe and at the right altitude, but the driver's retry path still stores the same failure records raw, and the invariant the PR claims doesn't yet hold for /start's dominant failure channel.

Findings

1. The driver's durable retry path stores the same failure records raw — the other half of the split the PR body says it closed. The body claims the rewrite covers messages "stored on a failed control request" (the revert/skip-revert failure records). That's true only for the API-side immediate attempt. When the immediate gRPC call transport-fails, the request stays pending (control_handlers.go#L2102-L2108) and the driver retries it; a rejection on that retry stores the raw data-plane text into the very same apply_control_requests.error_message column via failPendingControlRequests (control_requests.go#L79, UPDATE at mysqlstore/control_requests.go#L182) — revert "revert was not accepted: %s" (grpc_client.go#L749-L754), skip-revert (grpc_client.go#L705-L710), and cutover/start write raw through the same helper (#L643-L648, #L2083-L2093). Failure scenario: operator requests a revert, the immediate call times out, the driver's retry receives the rejection — the stored record names apply-remote999 while the identical rejection delivered on the immediate path would have been stored as apply-vol123; same event, two spellings, chosen by transport luck. This is latent today — I verified there is currently no operator-facing reader of failed rows (GetPending filters to pending at mysqlstore/control_requests.go#L94, RequestPending NULLs error_message on reset at #L75) — but the record is durable, so the first feature that surfaces failed control requests inherits the inconsistency. The clean fix is one layer down, inside failPendingControlRequests (so both writers converge); note pkg/api imports pkg/tern, so the helper has to move down rather than be imported up, and the driver side additionally needs op-level ExternalOperationID handling for multi-op applies.

2. The stated invariant — the new test's comment says "no internal identifier reaches PR-facing markdown" — still fails for /start's real-world failure mode, via a channel this PR doesn't touch. When a remote start fails, the driver builds "remote start failed for remote apply %s: %v" naming the remote id and the raw gRPC error (grpc_client.go#L2083, stored into apply.ErrorMessage at #L2088), and that text renders verbatim in the PR-comment error block for Failed/Stopped applies (templates/apply.go#L181-L182) and in progress/status output (progress_handlers.go#L574-L575). For start — one of the six commands the PR covers — the response relay it rewrites never carries data-plane text today (see Notes), so the dominant remote-id leak for that command is this untouched channel. Same family: webhook/control.go#L192 posts err.Error() verbatim to the PR on command errors, and the sibling branch at control_handlers.go#L835-L836 — in the very function this PR edited — relays a transport error's %v into the apply log unrewritten. All pre-existing, none introduced here; flagging because the PR body's claim reads stronger than what's delivered, and a follow-up should route these through the same rewrite (the start-failure text also carries raw gRPC error detail, which the repo's own rule says shouldn't render in PR markdown at all).

3. 1 of 9 rewrite sites is tested, and the helper has no unit test. The new test (handlers_test.go#L4125-L4158) covers only the volume response relay. The two FailPending stores (the "failure records" the body specifically calls out), the immediate-stop apply-log line, and the stop/cancel/start/revert/skip-revert responses are untested, and there is no table-driven unit test for operatorFacingControlError's guards (nil apply, empty ExternalID, empty message, multiple occurrences of the id). A refactor that reverts any one argument back to resp.ErrorMessage passes CI silently.

The one thing that could have broken, verified

The blind strings.ReplaceAll (control_handlers.go#L467): replacing every occurrence of one apply id with another inside arbitrary message text is only safe if (a) the id being replaced is byte-identical to what the data plane interpolates, and (b) it can never appear as a substring of some other token. Both hold. (a) The rejection copy interpolates the data plane's own ApplyIdentifier (e.g. local_control.go#L1629), and that value is exactly the control plane's stored ExternalID: the API addresses the data plane by apply.ExternalID (control_handlers.go#L448-L453), the data plane resolves its apply row by that same id, and ExternalID has a single writer that persists the remote's reported id verbatim (grpc_client.go#L1507) — a byte-identical round trip. (b) Every apply identifier in the system comes from one generator, "apply-" + uuid-hex[:16] (plan_handlers.go#L911, local_client.go#L2034) — fixed-length 22-char tokens, so one id containing another as a substring implies equality; a false-positive replacement inside an unrelated identifier is structurally impossible with today's generators. The guards (empty message / nil apply / empty ExternalID → passthrough) close the remaining degenerate cases, including the mass-deletion hazard of replacing the empty string.

Notes (not blocking)

• The helper's godoc overstates one guarantee: "the remote id stays triageable from … the server logs, which keep the raw message" (control_handlers.go#L461-L462). True for stop (raw error_message logged at #L859), but the volume and revert/skip-revert rejection paths on the control plane log nothing with the raw text — triage there relies on the apply row's external_id and the data plane's own logs. Worth softening the comment or adding the log lines.
• Three of the six response-relay rewrites (stop, cancel, start) are defensive no-ops today: I traced their data-plane response constructors and none interpolate an apply id into ErrorMessage for the gRPC path. Harmless — and reasonable belt-and-braces — but the PR body's "all six response relays" is uniformity, not six live fixes; volume and revert/skip-revert are the live ones.
• The substring-safety proof above is generator-dependent. If a future engine ever reports variable-length or operator-influenced remote ids, blind ReplaceAll acquires a corruption risk. A cheap future-proofing option: skip the rewrite unless the ExternalID matches the apply-[0-9a-f]{16} shape.
• Multi-op applies: the parent's ExternalID is deliberately never set (op-level ids live on apply_operations.external_id), so the helper is an identity function for them — and I verified every relay this PR touches skips multi-op applies anyway. Op-level remote ids can still leak through driver-stored text (finding 1's layer), not through anything rewritten here.

Verified correct

• The premise is real and end-to-end: a remote rejection names an id the operator can't resolve, and the byte-identical round trip (verified above) means the rewrite always fires when it should.
• All nine documented call sites are present and correctly argumented (#L778, #L863, #L996, #L1403, #L1992, #L2117, #L2123, #L2248, #L2254); every downstream consumer of these responses (webhook command posts, CLI, already-requested rebuilds) sees only the rewritten text.
• The "six response relays" enumeration is accurate: cutover and release relays never carry data-plane rejection text at the API layer, so nothing in scope was missed on the response side.
• Local-mode and PlanetScale untouched, as claimed: ExternalID is only ever written by the remote gRPC dispatch path; local applies hit the nil/empty guard.
• The new test is a genuine end-to-end discriminator: it proves the data plane is still addressed by the remote id (mock.volumeReq.ApplyId == "apply-remote999") while the operator response carries only the operator id — the exact two-sided contract of the change.
• Altitude is right. Control rejections are schemabot-authored templated copy (trusted), which the repo's own rule distinguishes from untrusted raw errors (those get a fixed sanitized line, e.g. apply_execute.go); the tern proto has no structured {code, apply_id} alternative to rewrite against, so a boundary rewrite at the relay is the correct depth, and fix(github) is the correct scope per the repo's commit conventions.
• Build, go vet, gofmt, and the full pkg/api test suite pass in the review worktree at 0c1264b; CI is fully green on the head SHA.

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

[aparajon]

🤖 Follow-up on these findings, posted on Armand's behalf. All three are addressed at the current head:

1. Driver retry path stores raw text — fixed in 8a705f2. The helper moved down to pkg/storage as Apply.OperatorFacingMessage (it couldn't be imported up, as you noted), and failPendingControlRequests now rewrites before persisting, taking the remote ids the caller addressed. All gRPC driver call sites pass scope.remoteApplyID(apply), which resolves the claimed operation's external_id (with its EngineResumeContext fallback — also the remote apply id) for multi-operation drives, so the op-level case is covered too. A rejection now reads the same whichever path reached the data plane, and the driver-side test pins both spellings.

2. /start's dominant leak channel (apply.ErrorMessage → PR error block) — deliberately out of scope, now stated honestly: the PR body's scope notes call out that remote start failures carry the remote id and raw gRPC error detail, so that channel needs sanitizing (fixed line + server logs) rather than translating — separate follow-up along with the sibling err.Error() posts.

3. Test coverage — closed: a table-driven unit test for the helper (nil apply, empty ids, multi-occurrence, op-level ids, local passthrough), API-level tests proving the durable revert/skip-revert records and the stop apply-log line store the translated text while the data plane is still addressed by its own id, and the driver retry-path test above.

Your non-blocking notes also landed: the raw-text logging gap you flagged is fully closed as of 06641c7 (the API paths got their warn logs earlier; that commit adds them to the driver's revert/skip-revert rejection paths, and scopes the helper's triage note to what the paths actually guarantee), the stop/cancel/start relays are documented in the body as uniformity rather than live fixes, and the substring-safety invariant is recorded in the helper's godoc for whoever adds an engine with differently-shaped identifiers.

This reply was posted by Claude Code (claude-fable-5).