refactor(webhook): split apply command into error-returning core

[Kiran01bm]

Summary

Splits handleApplyCommand into a thin goSafe wrapper over a new error-returning core, applyCommandCore, that classifies every outcome as retryable-transient or terminal. This is the first prerequisite for durable issue_comment/apply: a later durable driver needs a retry-vs-terminal signal to decide whether to re-drive a delivery. Behavior is unchanged in this PR — the wrapper discards the disposition.

What

• handleApplyCommand becomes the synchronous entry point; it calls applyCommandCore and discards the returned (retry, err).
• applyCommandCore returns (retry bool, err error):
  • retry=true, err≠nil — transient infrastructure failure (command bootstrap, a GitHub read, a storage op, plan execution) the same delivery could clear on a later attempt.
  • retry=false, err=nil — a terminal outcome that is the command's answer (silent skips, gate blocks, config/stale rejections, no-op plans, or a dispatched apply).
• Adds focused contract tests for the new core's disposition on both axes.
• apply-confirm is intentionally left for a follow-up PR to keep this change small.

Why

Durable command dispatch must distinguish "re-drive this" from "this is the final answer." Extracting the classification now, without wiring a consumer, keeps the behavior-preserving refactor isolated from the durable-routing change that follows — smaller blast radius, reviewable independently.

Note: some retryable paths already post an operator-facing error comment before returning. A future durable retry would re-post those; that duplication is a known concern for the durable-driver slice, not introduced here.

Observability note: the two new typed schema-request errors shift the schema-request failure metric reason from unexpected to database_not_configured / environment_not_configured and the corresponding log level from Error to Warn — for every command routed through handleSchemaRequestError, not just apply.

Before / after

Before:
handleApplyCommand(...)  ──▶  returns on error/blocked/dispatch (void)

After:
handleApplyCommand(...)                 (goSafe wrapper, behavior unchanged)
└─ _, _ = applyCommandCore(...)   discards disposition
│
├─ transient infra failure   ─▶ (retry=true,  err≠nil)
└─ terminal command answer    ─▶ (retry=false, err=nil)

[Copilot]

Pull request overview

Refactors the webhook apply command handler by extracting an error-returning core (applyCommandCore) that reports a durability disposition (retry, err) intended for a future durable issue_comment driver, while keeping the current webhook behavior unchanged by discarding that disposition in handleApplyCommand.

Changes:

• Split handleApplyCommand into a thin wrapper that calls applyCommandCore.
• Introduced (retry bool, err error) returns from applyCommandCore and updated early returns throughout to map outcomes to retryable-vs-terminal dispositions.
• Added contract tests covering key retryable and terminal disposition cases.

Reviewed changes

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

File	Description
pkg/webhook/apply_handlers.go	Extracts applyCommandCore and adds retry/terminal disposition returns across apply flow.
pkg/webhook/apply_error_contract_test.go	Adds tests to pin the disposition contract for selected core outcomes.

---

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

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 7f8dd93.

Verdict: behavior-preserving and safe to land. Every old return maps to a classified return with identical side effects, and the head commit's handleSchemaRequestError classification correctly closes the transient-config-read hole Copilot flagged. The two findings below both misclassify in the safe direction (under-retry — never a double-apply), so none of this blocks the PR; it's about the contract the durable driver will inherit.

Findings

1. Gate-internal operational failures are terminal, but the posted comment says "retry". Every gate mapped to (retry=false, err=nil) fails closed on its own infrastructure failures: enforceOpenPR returns blocked=true when it can't read the PR (and posts "Retry, and see server logs if it persists."); same for a checkReviewGate error, a GetPRCheckStatuses failure, and an actor-authorization error. So a GitHub 500 at the FetchPullRequest call outside enforcePassingChecks re-drives, while the identical 500 inside it is "the command's final answer." The doc comment's claim that terminal = "a re-drive would only reproduce it" overclaims.

2. The plan-failure split treats transient target-DB failures as deterministic. isTransientRemotePlanError covers only transport (RemoteDeploymentUnavailableError, gRPC Unavailable). A plan failing because the target database is briefly unreachable (RDS failover, dial/ping blip; on remote engines this arrives as a non-Unavailable application error) is classified terminal under a comment claiming "planning failures are deterministic" — planning against a live DB isn't.

Action items

1. (Finding 1) Update the applyCommandCore doc contract to state that gate-internal operational failures are deliberately terminal (fail-closed, gate posts its own retry guidance) — or, if unintended, track a follow-up to make gates return (blocked, err). Doc-only is fine for this PR; same resolution as the equivalent finding on refactor(webhook): extract error-returning aggregate fold core #785.
2. (Finding 2) Narrow the comment at the plan-failure branch to what the predicate actually checks ("failures other than remote-deployment unavailability are treated as deterministic"), or widen the classification in the durable slice.
3. (PR body) Note the observability change: the two new typed errors shift the schema-request metric reason from unexpected to environment_not_configured/database_not_configured and the log level Error→Warn — for every command using handleSchemaRequestError, not just apply.
4. (optional) Add a test for the new ErrLockHeld-at-acquire branch — the only genuinely new classification logic without one. (The logic is correct: I verified Acquire returns ErrLockHeld only for another-owner conflict; same-owner re-acquire succeeds.)

Verified (tried to break, couldn't)

Behavior preservation at every exit (comments/logs/acks/lock-release identical; handleSchemaRequestError still always posts — the bool only classifies, so its void callers in plan/apply-confirm are untouched); the two typed errors are drop-in (byte-identical messages, errors.As works through the wrap); terminal-after-lock is the correct at-most-once boundary (re-driving past acquire could double-dispatch); retryable-with-lock-held re-drives safely re-enter the active-apply guard; (true, nil)/(false, err≠nil) are unrepresentable; bootstrap-retryable matches the #780/#790 durable processors; contract tests pass locally at head and need no build tag.

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

[Kiran01bm]

🤖 Thanks for the review - here is the response summary

#	Concern	Status
1	Gate-internal operational failures are terminal while the gate's posted comment says "retry" — doc contract overclaims "a re-drive would only reproduce it"	fixed — doc-only, per the reviewer's suggested resolution (same as the equivalent finding on #785): the applyCommandCore contract now states gate blocks are deliberately terminal — gates fail closed and post their own retry guidance; recovery is the user re-issuing the command, not a driver re-drive
2	Plan-failure branch comment claims "planning failures are deterministic", but planning runs against a live target DB that can be briefly unreachable	fixed — comment narrowed to what isTransientRemotePlanError actually checks ("failures other than remote-deployment unavailability are treated as deterministic"), explicitly noting the not-truly-deterministic subset recovers via user re-issue
3	PR body doesn't note the observability shift (schema-request metric reason unexpected → database_not_configured/environment_not_configured, log Error→Warn, for every handleSchemaRequestError caller)	fixed — observability note added to the PR body's Why section
4	(optional) No test for the new ErrLockHeld-at-acquire branch	reply — the branch is only reachable in the race window between the lock pre-check (Get sees no lock) and Acquire; no seam exists in applyCommandCore to inject that interleaving, the unit layer can't drive past plan execution (h.service is a concrete *api.Service), and the integration harness can only pre-seed the lock, which exercises the pre-check branch instead (TestE2EApplyLockConflictDifferentPR). The reviewer verified the classification logic correct; adding a seam solely for this test isn't warranted