feat(iac): IaCProviderFinalizer + OnPlanComplete hook (#695 Phase 2.5)#697
Merged
Conversation
…y RPC (Phase 2.5)
Adds optional IaCProviderFinalizer service with FinalizeApply RPC plus
FinalizeApplyRequest{plan_id} and FinalizeApplyResponse{repeated
ActionError errors} messages. Plugins opt in by registering the service;
absence = no finalize hook (ADR 0024). Errors preserve per-driver
attribution shape used by the DigitalOcean v1 wrapper (ADR 0040).
Regenerated iac.pb.go + iac_grpc.pb.go via buf generate; bundled with
proto edit per Phase 2 cycle-1 I-1 precedent (split would create
broken intermediate commit).
Phase 2.5 of workflow#640 / workflow#695.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…y wire-status invariant - Add `reserved 2;` directive inside FinalizeApplyResponse (protoc-level enforcement; comment-only reservation gave no protection). Raw descriptor now encodes reserved_range [2,3) — future allocator will fail at codegen. - Replace cross-repo line pins (provider.go:295-307 and :301-306) with function/identifier references (DOProvider.Apply + deferredUpdater + FlushDeferredUpdates) that survive DO-side refactors. - Add wire-status invariant on FinalizeApplyResponse: gRPC status MUST be OK when errors[] is populated (mirrors ADR 0040 invariant 2); non-OK status means errors[] is ignored and gRPC error surfaced directly. Eliminates ambiguity between transport failure and per- driver finalize errors. Regen mechanical (godoc + raw descriptor); no struct/symbol drift. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…n apply (Phase 2.5) Adds ApplyPlanHooks.OnPlanComplete field + deferred-closure invocation inside applyPlanWithEnvProviderAndHooks. Hook fires once after the per-action loop reaches its natural success completion (apply.go:336), matching the v1 DOProvider.Apply wrapper's deferred-flush placement. Changes: - Add OnPlanComplete field with godoc enumerating fire/no-fire exit paths. - Change applyPlanWithEnvProviderAndHooks signature to named returns (result *interfaces.ApplyResult, err error) so the deferred closure can read err for v1 semantic gating and append to result.Errors / reassign err on finalize failure. - Add loopReached flag set immediately before the per-action loop opens; pre-loop preflight failures (apply.go:192) skip finalize because loopReached stays false (no cloud work happened). - Deferred closure gates on err == nil per cycle-1 plan-review C-3 v1 semantic preservation — outer-error exits at per-action hook fatalErr (apply.go:319-324) and post-loop length-invariant violation (333) skip finalize, matching DOProvider.Apply's "return without flushing on top-level err" at internal/provider.go:276-282. - Finalize-side hook errors surface to caller's err (wrapped via fmt.Errorf with %w) AND append result.Errors entry with Resource="<plan-finalize>"/Action="finalize" so per-driver attribution is preserved alongside the finalize-attributed failure. Tests (apply_hooks_test.go, +5 tests, all PASS race-clean): - FiresOnCleanSuccess: success-exit fires finalize. - FiresOnEmptyPlan: zero-iteration plan still fires finalize (regression guard — v1 wrapper flushes stale-queued state for empty plans too). - SurfacesErrorToCaller: finalize hookErr wraps via %w and adds "<plan-finalize>" entry to result.Errors. - SkippedOnOuterError: OnResourceApplied hook returning err triggers outer fatalErr at 324; finalize skipped (C-3 invariant). - DoesNotFireOnPreloopError: replace + ResourceReplacer + delete-hook active triggers preflight rejection at 192; loopReached=false; finalize skipped. Backward compat: ApplyPlanHooks zero value (no OnPlanComplete) → defer short-circuits on `hooks.OnPlanComplete == nil`. Existing callers using positional `return result, ...` continue to work unchanged with named returns. Per workflow#695 Phase 2.5 / ADR 0024 / ADR 0040. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…e panics + drop redundant attribution prefix Three follow-ups on the OnPlanComplete deferred closure (50e3feb): 1. Replace ~10 stale within-file line refs in ApplyPlanHooks.OnPlanComplete godoc + apply.go inline comments + apply_hooks_test.go test docs with symbol/structural anchors. The original commit drafted refs against pre-insertion plan-doc line numbers; the +95 LOC of inserted code shifted everything down ~80-90 lines, leaving every ref misleading. Now anchors to identifier names that survive line shifts: - "apply.go:336" → "the natural success-exit return at the end of applyPlanWithEnvProviderAndHooks" - "apply.go:192" → "the preflightProviderOwnedReplaceWithDeleteHooks early-return" - "apply.go:319-324" → "the per-action loop's `if fatalErr != nil { return ... }` early-return" - "apply.go:333" → "the post-loop length-invariant check" - "apply.go:367" → "preflightProviderOwnedReplaceWithDeleteHooks's engine-side ResourceReplacer rejection error" - "internal/provider.go:276-282" → "the `if err != nil { return ... }` guard immediately after the wrapped ApplyPlan call in workflow-plugin-digitalocean internal/provider.go" (Pre-existing ref at apply_hooks_test.go:270 left alone — not part of this commit's scope.) 2. Add recover() around hooks.OnPlanComplete(ctx) for symmetry with the drift defer's recover() posture on caller-provided closures. A panic inside the finalize hook would otherwise propagate past the defer chain and prevent the caller from observing result/err. On recover, set hookErr to "OnPlanComplete panicked: <panic value>" and surface through the existing result.Errors + outer-err pathways. 3. Drop redundant "plan finalize:" prefix from the ActionError.Error field. The entry already carries Resource="<plan-finalize>" + Action="finalize" — embedding the prefix inside Error caused double-attribution when callers format as "<Resource>/<Action>: <Error>". Outer err keeps the prefix because the outer-err path lacks the structured fields and needs the attribution inline; errors.Is round-trip on the wrapped hookErr is preserved. Tests: - All 5 prior OnPlanComplete tests still PASS (no behavior change for the non-panic, non-attribution-prefix paths). - New TestApplyPlanWithHooks_OnPlanComplete_RecoversFromPanic asserts panic → recovered err mentions "OnPlanComplete panicked" + carries the panic value + appends "<plan-finalize>" entry. - Full ./iac/wfctlhelpers/ -race -count=1 PASS. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… 2.5) Adds the IaCProviderFinalizer service to registerIaCServicesOnly's optional-services block, placed immediately before the ResourceDriver registration and after IaCProviderDriftConfigDetector — same opt-in posture as every other IaCProvider* optional (per-interface type-assert, skip on miss; absence of registration is the negative signal per ADR 0024). Tests (iacserver_test.go, +2 tests + 1 stub): - TestRegisterAll_RegistersIaCProviderFinalizer: provider embedding pb.UnimplementedIaCProviderFinalizerServer → service in grpcSrv.GetServiceInfo()["workflow.plugin.external.iac.IaCProviderFinalizer"]. - TestRegisterAll_SkipsIaCProviderFinalizerWhenNotImplemented: provider without the finalizer embed → service absent. Locks the negative- signal contract that wfctl-side adapter.Finalizer() (Task 4) relies on. Per workflow#695 Phase 2.5 / ADR 0024. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rop roadmap-task IDs Three follow-ups on the IaCProviderFinalizer auto-registration (09fb61a): 1. Extend the "all optionals" coverage contract for the 7th optional. allCapabilitiesStub previously embedded 6 optionals; under T3 it silently shrank from "every optional" to "6-of-7". Adds: - pb.UnimplementedIaCProviderFinalizerServer embed to allCapabilitiesStub. - "workflow.plugin.external.iac.IaCProviderFinalizer" to wantServices in TestRegisterAllIaCProviderServices_AllOptionals_AllRegistered. - Godoc update: "all 7 typed services (Required + 6 optional)" → "all 8 typed services (Required + 7 optional)". AllOptionals_AllRegistered now also locks the auto-registration contract for the new optional (alongside the dedicated positive + negative tests). 2. Move finalizerProviderStub from between the SkipsIaCProviderFinalizerWhenNotImplemented test and the serviceNames helper into the stubs block (right after enumeratorOnlyStub, where single-optional-capability stubs live). Restores the file's stubs-grouped-together convention. 3. Drop "(Task 4 + Task 5)" parenthetical from the registration block comment in iacserver.go. Roadmap-task IDs become historical artifacts after the cascade lands; replaced with stable pointers — the wfctl-side typed adapter file path (cmd/wfctl/iac_typed_adapter.go) and Finalizer() accessor symbol name. Same anchor-by-identifier pattern applied in T1/T2 fix-ups. Tests still 3/3 PASS (AllOptionals + Registers + Skips); full package race-clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…lizer optional service (Phase 2.5)
Adds 4 sub-changes to cmd/wfctl/iac_typed_adapter.go mirroring the
existing optional-client pattern (Enumerator/DriftDetector/etc.):
(a) iacServiceFinalizer constant
"workflow.plugin.external.iac.IaCProviderFinalizer" — placed in the
iacService* constant block between iacServiceDriftConfigDetect and
iacServiceResourceDriver to keep the block's registration-order
ordering stable with the SDK's auto-registration block.
(b) finalizer pb.IaCProviderFinalizerClient field on typedIaCAdapter
struct, placed in the optional-clients section between driftCfg
and resourceDriv (matches accessor + constructor ordering).
(c) Constructor branch in newTypedIaCAdapter:
if registered[iacServiceFinalizer] {
a.finalizer = pb.NewIaCProviderFinalizerClient(conn)
}
Same shape as the 6 sibling optional-client conditionals.
(d) Finalizer() pb.IaCProviderFinalizerClient accessor returning
a.finalizer. Godoc enumerates the wfctl-side consumer
(statePersistenceHooks helper in cmd/wfctl/infra_apply.go) that
gates OnPlanComplete wiring on a non-nil return, citing ADR 0024's
negative-signal contract and workflow#695 Phase 2.5.
Tests (cmd/wfctl/iac_typed_adapter_test.go, +2 tests + 1 helper):
- TestTypedAdapter_Finalizer_PopulatedWhenRegistered: exercises the
constructor branch (map[iacServiceFinalizer]=true → accessor non-nil).
- TestTypedAdapter_Finalizer_NilWhenNotRegistered: locks the negative
signal (map without finalizer key → accessor nil) — contract the
downstream OnPlanComplete wiring relies on.
- dialLazyConn helper: returns a real *grpc.ClientConn from
grpc.NewClient targeting an unreachable loopback port. The
underlying handshake is deferred to first RPC; the conn is valid
for adapter field-wiring assertions without spawning a server.
t.Cleanup drains the dial pool between tests.
Per workflow#695 Phase 2.5 / ADR 0024.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…l + harden dialLazyConn against grpc-go behavior drift Three follow-up Minors on the Finalizer accessor (3d88d70) — no blockers, all author-discretion quality fixes consistent with T1/T2/T3 cleanup posture: 1. Finalizer accessor godoc: "Used by the v2 apply path..." → "Intended for use by the v2 apply path...". statePersistenceHooks in cmd/wfctl/infra_apply.go does NOT yet call adapter.Finalizer() — that wiring lands in the next implement task. Future-tense closes the per-commit-window where the claim is verifiably false. 2. TestTypedAdapter_Finalizer_NilWhenNotRegistered godoc: dropped "(Task 5)" roadmap-task ID; anchored to "statePersistenceHooks (cmd/wfctl/infra_apply.go)" — stable symbol + file path that survives cascade closure (same fix applied to T3's iacserver.go godoc in 09b84f9). 3. dialLazyConn helper: switched from grpc.NewClient against an unreachable port (relying on grpc-go's NewClient-defers-dial semantic, which is documented but not stability-guaranteed) to a real net.Listen + grpc.NewServer with zero services registered. Conn now dials a live but service-empty server, so field-wiring tests survive a future grpc-go release switching to eager-dial. t.Cleanup drains both server + conn for test isolation. Tests still 2/2 PASS; full package race-clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…red statePersistenceHooks helper (Phase 2.5)
Extends the SHARED statePersistenceHooks helper in cmd/wfctl/infra_apply.go
to wire the workflow#695 Phase 2.5 OnPlanComplete hook through the v2
dispatch path. Per cycle-1 plan-review I-2, the single helper edit
covers BOTH v2 dispatch sites — both call sites pass plan.ID through
the new planID string parameter.
Changes:
- Add planID string parameter to statePersistenceHooks (positioned
between providerType and hydratedOut to minimize positional churn).
- Add OnPlanComplete closure to the returned ApplyPlanHooks. The
closure:
* Type-asserts provider to *typedIaCAdapter; no-ops on non-adapter
shapes (in-process fakes, legacy providers) for backward compat.
* Calls adapter.Finalizer() — returns nil when the plugin did not
register IaCProviderFinalizer (ADR 0024 negative signal). No-op
in that case preserves pre-Phase-2.5 behavior for plugins that
don't opt in.
* Invokes FinalizeApply RPC with the plan ID. Wraps gRPC transport
errors with "FinalizeApply gRPC: %w".
* Aggregates per-driver errors from FinalizeApplyResponse.errors
into a single err message preserving the v1 wrapper's
Resource/Action/Error attribution shape (per ADR 0040). The
engine-side defer in apply.go's OnPlanComplete handler appends
the err to result.Errors as the "<plan-finalize>" entry and
surfaces wrapped to outer caller err.
- Update both v2-dispatch call sites to pass plan.ID through the new
parameter.
- Add pb "github.com/GoCodeAlone/workflow/plugin/external/proto"
import.
Verification:
- GOWORK=off go build ./... → clean
- GOWORK=off go test ./iac/wfctlhelpers/ ./plugin/external/sdk/ ./cmd/wfctl/ -race -count=1 → 3/3 packages PASS
- GOWORK=off golangci-lint run --timeout=10m → 0 issues
Per workflow#695 Phase 2.5 / ADR 0024 / ADR 0040.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new optional IaC plugin finalization surface so v2 apply dispatch can preserve post-apply-loop behaviors (e.g., provider-side deferred flush) without routing through the legacy IaCProvider.Apply path.
Changes:
- Introduces optional gRPC service
IaCProviderFinalizerwithFinalizeApply(plan_id)and regenerates typed proto Go code. - Extends
wfctlhelpers.ApplyPlanHookswithOnPlanCompleteand wires wfctl’s v2 apply path to callFinalizeApplywhen the plugin registers the service. - Updates the external plugin SDK auto-registration and adds tests across sdk, wfctl adapter wiring, and apply hook semantics.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plugin/external/sdk/iacserver.go | Auto-registers the new optional IaCProviderFinalizer service when implemented by a plugin provider. |
| plugin/external/sdk/iacserver_test.go | Extends registration coverage tests to include Finalizer positive/negative cases and all-optionals list. |
| plugin/external/proto/iac.proto | Defines IaCProviderFinalizer service and FinalizeApply* request/response messages. |
| plugin/external/proto/iac.pb.go | Regenerated protobuf message code for the new Finalizer messages. |
| plugin/external/proto/iac_grpc.pb.go | Regenerated gRPC client/server scaffolding for IaCProviderFinalizer. |
| iac/wfctlhelpers/apply.go | Adds ApplyPlanHooks.OnPlanComplete and deferred invocation logic after successful loop completion. |
| iac/wfctlhelpers/apply_hooks_test.go | Adds tests for OnPlanComplete firing rules, error surfacing, and panic recovery. |
| cmd/wfctl/infra_apply.go | Wires OnPlanComplete in v2 dispatch to call FinalizeApply via typed adapter when present. |
| cmd/wfctl/iac_typed_adapter.go | Adds typed client wiring + accessor for IaCProviderFinalizer. |
| cmd/wfctl/iac_typed_adapter_test.go | Adds tests validating the Finalizer accessor is populated only when the service is advertised. |
Files not reviewed (2)
- plugin/external/proto/iac.pb.go: Language not supported
- plugin/external/proto/iac_grpc.pb.go: Language not supported
Comment on lines
+180
to
+193
| defer func() { | ||
| if !loopReached || hooks.OnPlanComplete == nil { | ||
| return | ||
| } | ||
| if err != nil { | ||
| // v1 semantic preservation: outer-error exits (the per-action | ||
| // loop's `if fatalErr != nil { return ... }` early-return and | ||
| // the post-loop length-invariant check) skip finalize, | ||
| // matching DOProvider.Apply's "return without flushing on | ||
| // top-level err" behavior (the `if err != nil { return ... }` | ||
| // guard immediately after the wrapped ApplyPlan call in | ||
| // workflow-plugin-digitalocean internal/provider.go). | ||
| return | ||
| } |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…odoc + comment per-driver aggregator field order Three follow-ups on T5 (461de75): 1. Add cmd/wfctl/infra_apply_finalizer_test.go (~225 LOC) with 6 branch-coverage tests for the statePersistenceHooks OnPlanComplete closure — the closure carries load-bearing logic with a wire-visible error-format contract that no prior test exercised: - NonAdapterProviderNoOps: provider not *typedIaCAdapter → nil (Branch A, backward compat for in-process fakes / legacy provider shapes). - NilFinalizerNoOps: adapter.Finalizer() returns nil → nil (Branch B, ADR 0024 negative-signal preservation). - GRPCTransportError: FinalizeApply RPC fails → wrapped "FinalizeApply gRPC: %w" + status.FromError round-trip recovers codes.Internal (Branch C, retry/classifier contract). - AggregatesPerDriverErrors: response with errors[] → aggregated "plugin finalize: N driver(s) failed: R1/A1: E1; R2/A2: E2" with "; " separator (Branch D, ADR 0040 per-driver attribution + locked consumer-visible format string). - SuccessReturnsNil: empty errors[] response → nil (Branch E, clean success exit). - GRPCErrorPreservesErrorsIs: separate sanity-lock for errors.Is round-trip on the gRPC wrap (callers may classify via errors.Is in addition to status.FromError). Test scaffolding: - stubFinalizerServer satisfies pb.IaCProviderFinalizerServer with a caller-provided handler. - newFinalizerAdapter spins up an in-process gRPC server (Required + Finalizer registered) and returns a real *typedIaCAdapter wired via newTypedIaCAdapter. t.Cleanup drains server + conn. - Reuses dialLazyConn (defined in iac_typed_adapter_test.go) for Branch B which only needs a no-Finalizer adapter shape. 2. Flip Finalizer() accessor godoc from T4's interim "Intended for use by..." back to "Used by..." now that T5 has landed the statePersistenceHooks wiring. Closes the per-commit-window where the forward-tense claim made the godoc technically inaccurate. 3. Add inline comment at the OnPlanComplete closure's per-driver aggregator (Resource/Action field order) noting the pre-existing file-level inconsistency with the older applyWithProviderAndStore aggregator (Action/Resource order). The newer site matches proto field declaration order + apply.go's ActionError construction; reconciling the older site is tracked separately. Comment prevents a future contributor from "fixing" the new site back to the inconsistent ordering. Verification: - GOWORK=off go test ./cmd/wfctl/ -run 'TestStatePersistenceHooks_OnPlanComplete' -v -count=1 → 6/6 PASS - GOWORK=off go test ./iac/wfctlhelpers/ ./plugin/external/sdk/ ./cmd/wfctl/ -race -count=1 → 3/3 packages PASS race-clean (cmd/wfctl 96.5s) - GOWORK=off golangci-lint run --timeout=10m → 0 issues Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- plugin/external/proto/iac.pb.go: Language not supported
- plugin/external/proto/iac_grpc.pb.go: Language not supported
CI surfaced TestIaCServiceChecks_CoversEveryProtoService failure on PR #697: the regression-gate test walks iac.proto + asserts every service has a row in wftest/bdd/strict_iac.go's iacServiceChecks list. Phase 2.5 adds IaCProviderFinalizer (PR1 Task 1 proto extend) but the gate row was missed. One-line addition restores invariant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- plugin/external/proto/iac.pb.go: Language not supported
- plugin/external/proto/iac_grpc.pb.go: Language not supported
intel352
added a commit
that referenced
this pull request
May 20, 2026
#743) * feat(iac): workflow#640 Phase 5 — remove legacy wfctlhelpers.ApplyPlan Per docs/migrations/2026-05-16-v2-lifecycle-phase1-inventory.md. Phase 1 (PR #691) + Phase 2 (PR #694) + Phase 2.5 (PR #697) + Phase 3 (per-plugin migration) all shipped. Production code has been on ApplyPlanWithHooks for weeks. This PR closes the loop by removing the deprecated symbol. Changes: iac/wfctlhelpers/apply.go: - Delete func ApplyPlan (the 2-line wrapper that delegated to applyPlanWithEnvProvider with nil hooks). - Keep applyPlanWithEnvProvider as the postcondition-test seam — retained for apply_postcondition_test.go's panicky-env injection. iac/wfctlhelpers/*_test.go: - 10 test files migrated from `ApplyPlan(ctx, p, plan)` to `ApplyPlanWithHooks(ctx, p, plan, ApplyPlanHooks{})`. Empty-hooks form is semantically identical to the pre-deletion ApplyPlan call. docs/migrations/2026-05-16-v2-lifecycle-phase1-inventory.md: - Phase 5 marker flipped to SHIPPED. Verification: GOWORK=off go test ./... -count=1 (all green) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(iac): close v2 lifecycle migration notes * docs(iac): clarify manifest lifecycle metadata --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2.5 of workflow#640 cascade. Adds
IaCProviderFinalizeroptional gRPC service +ApplyPlanHooks.OnPlanCompletehook so plugins with post-Apply-loop work (DO plugin's databasetrusted_sourcesdeferred-flush) can declareComputePlanVersion="v2"dispatch without losing the post-loop behavior.Changes (5 commits, atomic per task)
feat(proto): IaCProviderFinalizer service + FinalizeApply RPC— new optional gRPC service +FinalizeApplyRequest{plan_id}+FinalizeApplyResponse{repeated ActionError errors}withreserved 2;for Phase 2.3 compensation evidence (protoc-level enforcement, not comment-only). Regeneratediac.pb.go+iac_grpc.pb.goatomic with proto edit (Phase 2 cycle-1 I-1 precedent). Response godoc carries wire-status invariant matching ADR 0040 invariant 2.feat(engine): OnPlanComplete hook + deferred-closure invocation—ApplyPlanHooks.OnPlanCompletefield; signature change to named returns; deferred closure withloopReachedflag +err == nilgate (cycle-1 plan-review C-3 v1 semantic preservation — finalize fires ONLY on natural success-exit return, matching DOProvider.Apply's "return without flushing on top-level err"). Recover() on hook panic for symmetry with drift defer. 6 tests in apply_hooks_test.go: clean-success-fires, empty-plan-fires, surfaces-error-to-caller, skipped-on-outer-error, no-fire-on-preflight-error, recovers-from-panic.feat(sdk): auto-register IaCProviderFinalizer— optional auto-registration inregisterIaCServicesOnlymatching existing 6 IaCProvider* optionals (Enumerator/DriftDetector/CredentialRevoker/MigrationRepairer/Validator/DriftConfigDetector). 3 tests: positive, negative-signal, all-optionals coverage extended for new optional.feat(wfctl): adapter Finalizer() accessor—typedIaCAdapterconstant + field + constructor branch + accessor matching existing optional-client patterns. ContractRegistry detection at handle-open; nil return when plugin didn't register. 2 tests: populated-when-registered, nil-when-not.feat(wfctl): wire OnPlanComplete via shared statePersistenceHooks helper— SHARED helper edit (per cycle-1 I-2 — single edit covers BOTH v2 dispatch sites). OnPlanComplete closure type-asserts provider to*typedIaCAdapter, callsadapter.Finalizer().FinalizeApply(), aggregates per-driver errors preserving v1 wrapper's Resource/Action/Error attribution shape. Both call sites updated to passplan.ID.ADR alignment
repeated ActionError; per-action statuses unaltered by finalize-side errors.FinalizeApplyResponsegodoc mandateserrors[]populated ⇒ gRPC OK; non-OK status ⇒errors[]ignored, gRPC error surfaces directly. No graceful-fallback ambiguity.Test plan
iac/wfctlhelpers/apply_hooks_test.go(TDD: write-fail-implement-pass)plugin/external/sdk/iacserver_test.go(incl. AllOptionals coverage extended to 7 optionals + dedicated positive/negative tests)cmd/wfctl/iac_typed_adapter_test.go(with real in-process gRPC serverdialLazyConnhelper for sturdy field-wiring)GOWORK=off go build ./...→ cleanGOWORK=off go test ./iac/wfctlhelpers/ ./plugin/external/sdk/ ./cmd/wfctl/ -race -count=1→ 3/3 PASSGOWORK=off golangci-lint run --timeout=10m→ 0 issuesCascade context
After merge → cut
v0.55.0tag from main HEAD → unblocksworkflow-plugin-digitaloceanPR (v1.3.0) which implementsFinalizeApplyserver-side + flipsComputePlanVersionto"v2"+ bumps workflow pin to v0.55.0.Rollback ordering (per cycle-1 plan-review I-6)
CRITICAL: Task 5 closure body (this PR's
statePersistenceHooksedit) MUST revert BEFORE Task 2 engine struct field (ApplyPlanHooks.OnPlanComplete) if rollback is needed. Reason: the helper closure references the engine struct field; reverting the engine struct first leaves the helper referencing an undefined field — compile break. Revert order: commit 5 → commit 4 → commit 3 → commit 2 → commit 1 (reverse of land order). Or revert this PR as a whole.Per ADR 0040 matched-pair rollback policy: workflow v0.55.0 stays at this branch's HEAD; DO v1.2.0 stays as the production plugin until v1.3.0 ships. Cascade rollback = (close DO PR + cut workflow v0.55.1 reverting whichever commit broke).
Plan:
docs/plans/2026-05-17-v2-lifecycle-phase2.5-onplancomplete.md(locked sha256 76374a1c8e29...).Design:
docs/plans/2026-05-17-v2-lifecycle-phase2.5-onplancomplete-design.md.Closes nothing yet — workflow#695 closes after DO v1.3.0 also ships (cascade closeout).
🤖 Generated with Claude Code