feat: workflow#640 Phase 2 — v2 hooks-over-gRPC contract (ApplyResult.Actions + ActionStatus + engine populate + decoder)#694
Merged
Conversation
Phase 2 v2 action lifecycle (workflow#640): add `repeated ActionResult actions = 7` to ApplyResult plus the ActionResult message and ActionStatus enum (UNSPECIFIED=0, SUCCESS=1, ERROR=2, DELETE_FAILED=3; tags 4-5 RESERVED for Phase 2.3 compensation). Per ADR 0040 invariants 1-2. Regenerated iac.pb.go in same commit to keep proto + bindings atomic (per cycle-1 plan-review I-1: split-commits create a broken intermediate that fails CI). Tests: proto marshal/unmarshal round trip + enum tag pinning. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Compiler-enforce ActionStatus tag-reuse prevention: replace comment-only reservation with `reserved 4, 5;` directive. Emits no Go symbol (no phantom literal — ADR 0024/0040 forbid that, not this) but prevents Phase 2.3 from accidentally reusing the tags. Regenerate iac.pb.go. Broaden TestApplyResultActionsRoundTrip to four subcases: nil_actions, empty_actions, unspecified_status, mixed_statuses. Use proto.Equal for canonical comparison so future ActionResult fields are covered without test edits. UNSPECIFIED subcase ensures the wire layer encodes/decodes losslessly so T3's reject path receives the expected zero-tag. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…t.Actions
Phase 2 v2 action lifecycle (workflow#640): introduce Go-side mirrors of
pb.ActionStatus + pb.ActionResult — ActionStatus (uint8 enum with four
constants Unspecified/Success/Error/DeleteFailed matching wire tags
0/1/2/3) and ActionOutcome struct {ActionIndex, Status, Error}. Extend
ApplyResult with `Actions []ActionOutcome` (omitempty, so plugins on the
v1 capability shim that emit no actions stay clean in persisted JSON).
Per ADR 0040 invariants 1-2. Tags 4-5 reserved in the proto for
Phase 2.3 compensation; intentionally not declared on the Go side yet
to keep this surface minimal until that work lands.
Tests cover: zero-value-is-Unspecified, constant tags 0-3, struct
field assignment, Actions JSON round-trip, omitempty when nil.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Phase 2 v2 action lifecycle (workflow#640): extend applyResultFromPB to translate pb.ActionResult entries into interfaces.ActionOutcome, populating the new ApplyResult.Actions field added in T2. Reject ACTION_STATUS_UNSPECIFIED at the decode boundary so a plugin that forgets to populate a status never reaches v2 hook dispatch — wfctl returns an explicit error naming the offending action_index. Per ADR 0040 invariant 2 (strict cutover, no graceful fallback). Added mapPBActionStatusToInterface helper covering SUCCESS / ERROR / DELETE_FAILED. The default branch returns ActionStatusUnspecified defensively; in practice UNSPECIFIED is filtered upstream by the reject loop and the default is unreachable. No length validation here — that's engine-side per T4. TDD tests: - TestApplyResultFromPB_DecodesActions: 3 statuses mapped + Error pass-through - TestApplyResultFromPB_RejectsUNSPECIFIED: error mentions UNSPECIFIED + offending action_index - TestApplyResultFromPB_EmptyActionsRoundTrip: plugins on v1 capability shim (no Actions) decode cleanly with empty slice Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Code-reviewer Important on T3 b57c5a9: the original reject loop only caught ACTION_STATUS_UNSPECIFIED. A Phase 2.3+ plugin emitting a reserved tag (4 or 5 for COMPENSATED / COMPENSATION_FAILED) against an older wfctl would silently degrade to ActionStatusUnspecified — the "graceful fallback" ADR 0040 invariant 2 forbids. proto3 preserves unknown enum integer values as-is, so the `reserved 4, 5;` directive added in T1 only prevents tag-reuse at proto compile time; it does NOT block wire-level drift between newer plugins and older wfctl. Change mapPBActionStatusToInterface signature to `(ActionStatus, bool)`, returning ok=false on any unknown wire value. applyResultFromPB converts the `!ok` signal into an explicit error naming the wire integer and offending action_index. UNSPECIFIED is now explicitly in the known set (ok=true) — the UNSPECIFIED-sent rejection lives at the higher policy level in the decode loop, not in the mapper. Minor: pin TestApplyResultFromPB_EmptyActionsRoundTrip on non-nil empty slice (matches sibling Resources/Errors convention). Add direct unit tests of the helper covering 4 known + 1 unknown wire value. Update mapper godoc to call out wire-drift defense as primary purpose. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Phase 2 v2 action lifecycle (workflow#640): the engine-side dispatch loop now records an ActionOutcome for every plan action, with a post-loop length-validation assert guaranteeing 1-to-1 correspondence between plan.Actions and result.Actions on the best-effort continue-on-error path. Per ADR 0040 invariant 1. Implementation: deferred-closure pattern (cycle-1 plan-review C-1). Each iteration runs the dispatch logic inside an inline func() whose defer unconditionally appends the ActionOutcome, derived from iterErr via the new mapDispatchErrToStatus helper. Two error sentinels carry the iteration's state: - iterErr — "this action failed but the loop continues" (best-effort) - fatalErr — hook / ctx-cancellation error that aborts the whole apply Continue paths (jit-substitution error, driver-resolve error, dispatch error other than hookDispatchError) set iterErr only — the deferred append fires with the appropriate status (Error / DeleteFailed) and the loop proceeds. Early-return paths (ctx cancellation, hookDispatchError, post-delete hook, post-apply hook) set fatalErr — the deferred append still fires for the offending action, then the outer for loop bubbles fatalErr to the caller (skipping the post-loop assert; length-on-fatal is correctly < len(plan.Actions)). mapDispatchErrToStatus: nil → Success; delete-action err → DeleteFailed; all other errs → Error. Compensation paths (Phase 2.3) reserved. TDD tests (use real fakeProvider single-driver API from apply_test.go): - TestApplyPlanWithHooks_PopulatesActions_CleanSuccess (2-action plan, both Success, ActionIndex 0/1) - TestApplyPlanWithHooks_PopulatesActions_PreDispatchDriverError (the CRITICAL C-1 invariant test — driver-resolve error MUST still produce ActionOutcome so length-assert doesn't false-fire) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Extends the IaC plugin gRPC contract with per-action outcome surfacing (ApplyResult.Actions + ActionStatus enum + ActionResult message) so wfctl can dispatch v2 lifecycle hooks correctly. Adds the Go-side mirror types in interfaces, an engine-side populator in iac/wfctlhelpers/apply.go using a deferred-closure pattern to guarantee every dispatch-loop exit path appends an ActionOutcome, and a fail-closed decoder in cmd/wfctl/iac_typed_adapter.go that rejects UNSPECIFIED and unknown wire tags per ADR 0040.
Changes:
- Proto: new
ActionStatusenum (tags 0-3, with 4-5 reserved for Phase 2.3 compensation) andActionResultmessage;ApplyResult.actionsat tag 7. - Engine:
applyPlanWithEnvProviderAndHookswraps per-iteration work in an inner func so a deferred closure unconditionally recordsActionOutcomeon all 7 exit paths, with a post-loop length invariant check. - Decoder:
applyResultFromPBdecodes actions, rejectsUNSPECIFIED, and usesmapPBActionStatusToInterfaceto fail loudly on unknown wire values.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/external/proto/iac.proto | Adds ActionStatus enum (with reserved 4-5) and ActionResult message; new actions field on ApplyResult. |
| plugin/external/proto/iac.pb.go | Regenerated proto bindings reflecting the new enum/message and index shifts. |
| plugin/external/proto/iac_proto_test.go | Round-trip + enum-tag pin tests for the new fields. |
| interfaces/iac_state.go | Go mirror types ActionStatus, ActionOutcome, and ApplyResult.Actions. |
| interfaces/iac_state_test.go | Zero-value, constant-value, and JSON round-trip/omitempty tests. |
| iac/wfctlhelpers/apply.go | Deferred-closure pattern to populate result.Actions on every dispatch-loop exit path; post-loop invariant assert; mapDispatchErrToStatus helper. |
| iac/wfctlhelpers/apply_hooks_test.go | Tests covering clean-success and pre-dispatch driver-error population paths. |
| cmd/wfctl/iac_typed_adapter.go | applyResultFromPB decodes Actions, rejecting UNSPECIFIED and unknown wire values; adds mapPBActionStatusToInterface. |
| cmd/wfctl/iac_typed_adapter_test.go | Decoder tests for success, UNSPECIFIED-rejection, empty-actions, unknown-status, and mapper helper. |
Comment on lines
+296
to
+313
| if action.Action == "delete" { | ||
| if err := actionHooks.OnResourceDeleted(ctx, action); err != nil { | ||
| fatalErr = fmt.Errorf("%s/%s: post-delete hook: %w", action.Resource.Type, action.Resource.Name, err) | ||
| iterErr = err | ||
| return | ||
| } | ||
| } | ||
| if len(result.Resources) > preLen { | ||
| out := result.Resources[len(result.Resources)-1] | ||
| out = fillMissingOutputIdentity(action.Resource, out) | ||
| result.Resources[len(result.Resources)-1] = out | ||
| if hooks.OnResourceApplied != nil { | ||
| if err := hooks.OnResourceApplied(ctx, d, action, out); err != nil { | ||
| fatalErr = fmt.Errorf("%s/%s: post-apply hook: %w", action.Resource.Type, action.Resource.Name, err) | ||
| iterErr = err | ||
| return | ||
| } | ||
| } |
Comment on lines
+1250
to
+1263
| func mapPBActionStatusToInterface(s pb.ActionStatus) (interfaces.ActionStatus, bool) { | ||
| switch s { | ||
| case pb.ActionStatus_ACTION_STATUS_UNSPECIFIED: | ||
| return interfaces.ActionStatusUnspecified, true | ||
| case pb.ActionStatus_ACTION_STATUS_SUCCESS: | ||
| return interfaces.ActionStatusSuccess, true | ||
| case pb.ActionStatus_ACTION_STATUS_ERROR: | ||
| return interfaces.ActionStatusError, true | ||
| case pb.ActionStatus_ACTION_STATUS_DELETE_FAILED: | ||
| return interfaces.ActionStatusDeleteFailed, true | ||
| default: | ||
| return interfaces.ActionStatusUnspecified, false | ||
| } | ||
| } |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Two PR #694 review fixes: 1. **G115 lint BLOCKER** (iac/wfctlhelpers/apply.go:218): the `ActionIndex: uint32(i)` cast on the loop counter trips gosec G115 (signed→unsigned overflow). In practice plan.Actions length is bounded by the user's plan size, never near math.MaxUint32, so the warning is a false positive. Add a targeted //nolint:gosec with a short justification rather than a defensive bounds-check that would add an unreachable branch. 2. **mapPBActionStatusToInterface fail-closed on UNSPECIFIED** (cmd/wfctl/iac_typed_adapter.go:1263): the previous shape returned (Unspecified, true) for ACTION_STATUS_UNSPECIFIED on the theory that applyResultFromPB filters UNSPECIFIED upstream. Copilot correctly flagged this as inconsistent with the documented strict-cutover invariant — the mapper itself should be fail-closed so its contract doesn't depend on caller-side pre-filtering. Now returns (Unspecified, false) for UNSPECIFIED, same as unknown wire values. Production decode path is unchanged because the explicit UNSPECIFIED-check at the top of the reject loop catches it first with a more specific error message; the mapper change is a defense- in-depth invariant. Test split: TestMapPBActionStatusToInterface_KnownValues renamed to _ActionableValues (now covers SUCCESS/ERROR/DELETE_FAILED only); new _UnspecifiedFailsClosed asserts (Unspecified, false) at the mapper level. Lint + tests verified locally: - GOWORK=off go test ./iac/wfctlhelpers/ ./cmd/wfctl/ -race → PASS - GOWORK=off golangci-lint run ./iac/wfctlhelpers/... ./cmd/wfctl/... → 0 issues Copilot's third finding (apply.go:313 post-hook-fail-vs-dispatch-fail conflation) is being deferred to Phase 2.3 — tags 4+5 reserved in proto since b09bced for ACTION_STATUS_COMPENSATED / COMPENSATION_FAILED to distinguish those cases. PR comment with the deferral reasoning follows separately. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Re Copilot's flag at Acknowledged — agreed this is a real semantic gap against ADR 0040 invariant 2. A post-delete hook failure today labels the action Deferred to Phase 2.3 (workflow#640 follow-up) which introduces:
Both tags already reserved in A separate issue will be filed at Phase 2.3 design time to track:
Not blocking this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
iac/wfctlhelpers/apply.go:354
mapDispatchErrToStatuskeys the DeleteFailed bucket offactionType == "delete", but it is also invoked for hook errors raised insidedispatchAction(e.g.,hookDispatchErrorat line 285) and for post-delete / post-apply hook failures (lines 299, 310). For adeleteaction whose post-delete hook fails, the outcome is reported asActionStatusDeleteFailedeven though the cloud delete itself succeeded — wfctl-side optimistic state-removal logic that keys off DeleteFailed would then erroneously preserve a resource that the provider has actually deleted. Consider only mapping to DeleteFailed when the failure originated from the driver Delete call itself, not from hook plumbing.
func mapDispatchErrToStatus(err error, actionType string) interfaces.ActionStatus {
if err == nil {
return interfaces.ActionStatusSuccess
}
if actionType == "delete" {
return interfaces.ActionStatusDeleteFailed
}
return interfaces.ActionStatusError
}
iac/wfctlhelpers/apply.go:314
- For the
hookDispatchErrorbranch the deferred closure storesiterErr = hookErr.err(the unwrapped hook error) intoActionOutcome.Error, while the surrounding apply returnsfatalErr(the type/name-prefixed wrapped error) to the caller. So the same hook failure surfaces with two different messages: the bubbled error has the"<type>/<name>: ..."prefix, but the in-result ActionOutcome.Error does not. Same divergence happens at lines 299-300 and 310-311 (post-delete / post-apply hook). Consider storing the wrapped error consistently so persisted apply-state JSON and the caller-visible error agree.
if err := dispatchAction(ctx, d, action, result, actionHooks, deleteHookActive); err != nil {
var hookErr hookDispatchError
if errors.As(err, &hookErr) {
fatalErr = fmt.Errorf("%s/%s: %w", action.Resource.Type, action.Resource.Name, hookErr.err)
iterErr = hookErr.err
return
}
result.Errors = append(result.Errors, interfaces.ActionError{
Resource: action.Resource.Name,
Action: action.Action,
Error: err.Error(),
})
iterErr = err
return
}
if action.Action == "delete" {
if err := actionHooks.OnResourceDeleted(ctx, action); err != nil {
fatalErr = fmt.Errorf("%s/%s: post-delete hook: %w", action.Resource.Type, action.Resource.Name, err)
iterErr = err
return
}
}
if len(result.Resources) > preLen {
out := result.Resources[len(result.Resources)-1]
out = fillMissingOutputIdentity(action.Resource, out)
result.Resources[len(result.Resources)-1] = out
if hooks.OnResourceApplied != nil {
if err := hooks.OnResourceApplied(ctx, d, action, out); err != nil {
fatalErr = fmt.Errorf("%s/%s: post-apply hook: %w", action.Resource.Type, action.Resource.Name, err)
iterErr = err
return
}
}
Comment on lines
+231
to
235
| if err := ctx.Err(); err != nil { | ||
| iterErr = err | ||
| fatalErr = err | ||
| return | ||
| } |
Comment on lines
+1215
to
+1229
| actions := make([]interfaces.ActionOutcome, 0, len(r.GetActions())) | ||
| for _, a := range r.GetActions() { | ||
| if a.GetStatus() == pb.ActionStatus_ACTION_STATUS_UNSPECIFIED { | ||
| return nil, fmt.Errorf("plugin returned ActionResult with UNSPECIFIED status at action_index=%d (Phase 2 contract violation per ADR 0040)", a.GetActionIndex()) | ||
| } | ||
| mapped, ok := mapPBActionStatusToInterface(a.GetStatus()) | ||
| if !ok { | ||
| return nil, fmt.Errorf("plugin returned unknown ActionStatus=%d at action_index=%d (wfctl version too old for this plugin? Phase 2 contract violation per ADR 0040)", int32(a.GetStatus()), a.GetActionIndex()) | ||
| } | ||
| actions = append(actions, interfaces.ActionOutcome{ | ||
| ActionIndex: a.GetActionIndex(), | ||
| Status: mapped, | ||
| Error: a.GetError(), | ||
| }) | ||
| } |
| } | ||
| mapped, ok := mapPBActionStatusToInterface(a.GetStatus()) | ||
| if !ok { | ||
| return nil, fmt.Errorf("plugin returned unknown ActionStatus=%d at action_index=%d (wfctl version too old for this plugin? Phase 2 contract violation per ADR 0040)", int32(a.GetStatus()), a.GetActionIndex()) |
Comment on lines
+209
to
+212
| // wfctl. Engine populates one entry per IaCPlan.Actions index (T4) so | ||
| // the length-validation assert can pair them 1:1. Empty/nil on plugins | ||
| // using the v1 capability shim (downstream pre-v1.2.0 cascade) — | ||
| // wfctl tolerates absence and skips v2-hook dispatch in that case. |
Two PR #694 r2 review fixes: F3 — `cmd/wfctl/iac_typed_adapter.go:1222`: reword the unknown-status error message to be neutral on which side is the older. Old copy "wfctl version too old for this plugin?" assumes wfctl is the older; the symmetric case (plugin emits a tag this wfctl hasn't seen because the plugin is newer) is equally valid. New copy ends with "either upgrade wfctl or downgrade the plugin" — actionable + neutral. F4 — `interfaces/iac_state.go:208-214`: the prior doc-comment on `ApplyResult.Actions` said "Empty/nil on plugins using the v1 capability shim... wfctl tolerates absence and skips v2-hook dispatch in that case", which contradicts ADR 0024 (no compat shim) + ADR 0040 (no graceful fallback). Pre-v1.2.0 plugins are PERMANENTLY incompatible with workflow v0.54.0+; there is no v2-hook-skip tolerance path. Doc rewritten to state the actual contract: Actions is always populated when the plugin declares ComputePlanVersion="v2"; non-v2 plugins are permanently incompatible. Local verify: - GOWORK=off go test ./interfaces/ ./cmd/wfctl/ -count=1 → PASS - GOWORK=off golangci-lint run ./interfaces/... ./cmd/wfctl/... → 0 issues PR r2 findings F1 (ctx-cancel mapping) and F2 (applyResultFromPB nil,err on reject) are being deferred — F1 to Phase 2.3 alongside the :313 post-hook conflation (same SKIPPED/COMPENSATED enum class); F2 as low-priority ergonomic improvement. PR replies posted separately. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Re Copilot's flag at Acknowledged — ctx-cancel mapping to (Error|DeleteFailed) shares the semantic gap class with the apply.go:313 post-hook conflation already flagged earlier. ctx-cancel means "action never attempted" rather than "action attempted and failed"; conflating those into the same Deferring to Phase 2.3 (workflow#640 follow-up) which scopes a broader ActionStatus extension:
Tags 4 + 5 already Same tracking issue as the :313 deferral will cover this. Not blocking this PR. |
Contributor
Author
|
Re Copilot's flag at Acknowledged — partial-state-on-error pattern is an ergonomic improvement, not a correctness fix. Current callers (cmd/wfctl/infra.go apply path, iac/wfctlhelpers consumers) all treat an Tracking as a Phase 2 followup (low priority). The Not blocking this PR. |
Comment on lines
+297
to
+316
| if action.Action == "delete" { | ||
| if err := actionHooks.OnResourceDeleted(ctx, action); err != nil { | ||
| fatalErr = fmt.Errorf("%s/%s: post-delete hook: %w", action.Resource.Type, action.Resource.Name, err) | ||
| iterErr = err | ||
| return | ||
| } | ||
| } | ||
| if len(result.Resources) > preLen { | ||
| out := result.Resources[len(result.Resources)-1] | ||
| out = fillMissingOutputIdentity(action.Resource, out) | ||
| result.Resources[len(result.Resources)-1] = out | ||
| if hooks.OnResourceApplied != nil { | ||
| if err := hooks.OnResourceApplied(ctx, d, action, out); err != nil { | ||
| fatalErr = fmt.Errorf("%s/%s: post-apply hook: %w", action.Resource.Type, action.Resource.Name, err) | ||
| iterErr = err | ||
| return | ||
| } | ||
| } | ||
| syncedOutputs[out.Name] = flattenOutputs(out) | ||
| } |
Comment on lines
+346
to
+354
| func mapDispatchErrToStatus(err error, actionType string) interfaces.ActionStatus { | ||
| if err == nil { | ||
| return interfaces.ActionStatusSuccess | ||
| } | ||
| if actionType == "delete" { | ||
| return interfaces.ActionStatusDeleteFailed | ||
| } | ||
| return interfaces.ActionStatusError | ||
| } |
Comment on lines
+246
to
+265
| resolved, err := jitsubst.ResolveSpec(action.Resource, result.ReplaceIDMap, syncedOutputs, os.LookupEnv) | ||
| if err != nil { | ||
| result.Errors = append(result.Errors, interfaces.ActionError{ | ||
| Resource: action.Resource.Name, | ||
| Action: action.Action, | ||
| Error: fmt.Sprintf("jit substitution: %v", err), | ||
| }) | ||
| iterErr = fmt.Errorf("jit substitution: %v", err) | ||
| return | ||
| } | ||
| result.Errors = append(result.Errors, interfaces.ActionError{ | ||
| Resource: action.Resource.Name, | ||
| Action: action.Action, | ||
| Error: err.Error(), | ||
| }) | ||
| continue | ||
| } | ||
| if action.Action == "delete" { | ||
| if err := actionHooks.OnResourceDeleted(ctx, action); err != nil { | ||
| return result, fmt.Errorf("%s/%s: post-delete hook: %w", action.Resource.Type, action.Resource.Name, err) | ||
| action.Resource = resolved | ||
| d, err := p.ResourceDriver(action.Resource.Type) | ||
| if err != nil { | ||
| result.Errors = append(result.Errors, interfaces.ActionError{ | ||
| Resource: action.Resource.Name, | ||
| Action: action.Action, | ||
| Error: fmt.Sprintf("resolve driver: %v", err), | ||
| }) | ||
| iterErr = fmt.Errorf("resolve driver: %v", err) | ||
| return |
Comment on lines
+233
to
+302
|
|
||
| // TestApplyPlanWithHooks_PopulatesActions_CleanSuccess verifies the | ||
| // Phase 2 engine-side ActionOutcome population — every successful | ||
| // PlanAction gets a corresponding result.Actions entry with | ||
| // ActionStatusSuccess and ActionIndex matching loop position. Per | ||
| // workflow#640 Phase 2 + ADR 0040 invariant 1. | ||
| func TestApplyPlanWithHooks_PopulatesActions_CleanSuccess(t *testing.T) { | ||
| p := newFakeProvider() | ||
| plan := &interfaces.IaCPlan{ | ||
| ID: "plan-1", | ||
| Actions: []interfaces.PlanAction{ | ||
| {Action: "create", Resource: interfaces.ResourceSpec{Name: "r1", Type: "infra.test"}}, | ||
| {Action: "create", Resource: interfaces.ResourceSpec{Name: "r2", Type: "infra.test"}}, | ||
| }, | ||
| } | ||
| result, err := ApplyPlanWithHooks(t.Context(), p, plan, ApplyPlanHooks{}) | ||
| if err != nil { | ||
| t.Fatalf("top-level err: %v", err) | ||
| } | ||
| if len(result.Actions) != 2 { | ||
| t.Fatalf("expected 2 ActionOutcomes, got %d: %+v", len(result.Actions), result.Actions) | ||
| } | ||
| for i, a := range result.Actions { | ||
| if a.ActionIndex != uint32(i) { | ||
| t.Errorf("action %d: ActionIndex=%d, want %d", i, a.ActionIndex, i) | ||
| } | ||
| if a.Status != interfaces.ActionStatusSuccess { | ||
| t.Errorf("action %d: Status=%v, want Success", i, a.Status) | ||
| } | ||
| if a.Error != "" { | ||
| t.Errorf("action %d: Error=%q, want empty", i, a.Error) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // TestApplyPlanWithHooks_PopulatesActions_PreDispatchDriverError covers | ||
| // the CRITICAL cycle-1 plan-review C-1 invariant: every continue exit | ||
| // path (here, the driver-resolve error at apply.go:228-234) must still | ||
| // append an ActionOutcome so the post-loop length-validation assert | ||
| // never false-fires. Per ADR 0040. | ||
| func TestApplyPlanWithHooks_PopulatesActions_PreDispatchDriverError(t *testing.T) { | ||
| p := &fakeProvider{driverErr: errors.New("driver resolution failed")} | ||
| plan := &interfaces.IaCPlan{ | ||
| ID: "plan-1", | ||
| Actions: []interfaces.PlanAction{ | ||
| {Action: "create", Resource: interfaces.ResourceSpec{Name: "r1", Type: "unknown.resource"}}, | ||
| }, | ||
| } | ||
| result, err := ApplyPlanWithHooks(t.Context(), p, plan, ApplyPlanHooks{}) | ||
| if err != nil { | ||
| t.Fatalf("expected no top-level err on driver-resolve failure (best-effort continue), got: %v", err) | ||
| } | ||
| if len(result.Actions) != 1 { | ||
| t.Fatalf("expected 1 ActionOutcome (length-assert invariant), got %d: %+v", len(result.Actions), result.Actions) | ||
| } | ||
| if result.Actions[0].Status != interfaces.ActionStatusError { | ||
| t.Errorf("driver-resolve-error status: want Error, got %v", result.Actions[0].Status) | ||
| } | ||
| if result.Actions[0].ActionIndex != 0 { | ||
| t.Errorf("ActionIndex: want 0, got %d", result.Actions[0].ActionIndex) | ||
| } | ||
| if result.Actions[0].Error == "" { | ||
| t.Errorf("Error: want non-empty, got empty") | ||
| } | ||
| // Cross-check: existing result.Errors path also populated so the | ||
| // pre-Phase-2 contract is preserved. | ||
| if len(result.Errors) != 1 { | ||
| t.Errorf("expected 1 result.Errors entry (legacy contract), got %d", len(result.Errors)) | ||
| } | ||
| } |
Comment on lines
+290
to
+324
| func TestApplyResult_Actions_RoundTrip(t *testing.T) { | ||
| r := ApplyResult{Actions: []ActionOutcome{ | ||
| {ActionIndex: 0, Status: ActionStatusSuccess}, | ||
| {ActionIndex: 1, Status: ActionStatusError, Error: "boom"}, | ||
| }} | ||
| data, err := json.Marshal(r) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| var got ApplyResult | ||
| if err := json.Unmarshal(data, &got); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if len(got.Actions) != 2 { | ||
| t.Fatalf("Actions len: got %d, want 2", len(got.Actions)) | ||
| } | ||
| if got.Actions[0].Status != ActionStatusSuccess || got.Actions[1].Error != "boom" { | ||
| t.Errorf("Actions round-trip mismatch: %+v", got.Actions) | ||
| } | ||
| } | ||
|
|
||
| // TestApplyResult_Actions_OmitemptyWhenNil ensures a nil Actions slice | ||
| // is absent from the JSON output, matching the omitempty convention used | ||
| // by Errors / InputDriftReport / ReplaceIDMap. Plugins on v1 capability | ||
| // shim emit no actions; the JSON must not carry an empty array. | ||
| func TestApplyResult_Actions_OmitemptyWhenNil(t *testing.T) { | ||
| r := ApplyResult{PlanID: "p"} | ||
| data, err := json.Marshal(r) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if bytes.Contains(data, []byte(`"actions"`)) { | ||
| t.Errorf("nil Actions emitted in JSON: %s", data) | ||
| } | ||
| } |
Contributor
Author
r3 review acknowledgement — proceeding to mergeCopilot's 5 new comments on 776bf94 revisit two clusters already decided: Cluster 1: per-action status semantic gaps (4 of 5 findings)
All four belong to the Phase 2.3 deferral cluster already replied at #issuecomment-4467488513 (apply.go:313) + #issuecomment-4467575644 (apply.go:235). Phase 2.3 will introduce
Implementing this distinction in Phase 2 would require either (a) breaking the strict 1:1 length-invariant assert by elide-on-skip OR (b) introducing 1+ new enum values mid-cascade. Both violate the hard-cutover discipline of ADR 0024 (the plugins are already pinned to v2 with this exact enum set). Phase 2.3 is the correct landing zone — the proto-side The advisory Minor on Test-coverage observation (apply_hooks_test.go:302) was also raised by internal code-review and accepted: the plan-spec'd 2 tests (CleanSuccess + PreDispatchDriverError) cover the C-1 invariant per cycle-1 plan-review. Coverage for the other 5 exit paths is welcome engineering hygiene — tracking for a follow-up PR rather than expanding this PR's scope. Cluster 2: import sanity check (1 of 5 findings)
Verified: local Merge decision: all 5 r3 findings are either same-class deferrals (already replied) or accepted advisory Minors. CI is 27 SUCCESS / 1 SKIPPED / 0 FAILURE. Branch is mergeable. Per ADR 0024 + 0040 cascade discipline, PR1 is the gate for the 4 downstream plugin PRs (aws/gcp/azure/digitalocean v1.2.0). Proceeding to admin-merge → tag v0.54.0 → unblock cascade. Phase 2.3 follow-up tracking: will be opened as a workflow issue after PR1 merges + cascade lands, scoping the SKIPPED + COMPENSATED + COMPENSATION_FAILED enum additions plus the dispatch-side mapping rules. |
This was referenced May 16, 2026
intel352
added a commit
that referenced
this pull request
May 17, 2026
Critical fixes: - C-1 (DOProvider.FlushDeferredUpdates does not exist): §E rewritten to inline per-driver iteration in doIaCServer.FinalizeApply mirror of v1 provider.go:295-307; no new public method on DOProvider. - C-2 (deferred-closure needs named returns): §A.0 added as required prerequisite — explicit applyPlanWithEnvProviderAndHooks signature change to (result *interfaces.ApplyResult, err error). - C-3 (cited line numbers): RE-VERIFIED original cites 167/180/229/240/ 249/255 ARE correct in current code; added explicit per-line classification table in §D. - C-4 (empty-plan case): §D paragraph added with regression-test cite. Important fixes: - I-1 UnimplementedIaCProviderFinalizerServer embed: now mandated in §E. - I-2 per-driver attribution: FinalizeApplyResponse extended with repeated ActionError errors (replacing single string field). - I-3 downstream-consumer pin-reset: §Rollback paragraph added. - I-4 ApplyFinalizer interface diverges: rewritten to use Finalizer() pb.IaCProviderFinalizerClient accessor. - I-5 v0.54.0 SHA cite: header now cites PR #694 sha dd9a313. - I-6 MEMORY.md reference: verified existing. Minor fixes: cite ranges tightened; proto comment cross-ref to Phase 2.3. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
intel352
added a commit
that referenced
this pull request
May 17, 2026
…st (#693) (#696) Phase 2.1 follow-up to workflow#640 Phase 2 cascade (PR #694, v0.54.0). Per ADR 0024 + ADR 0040 hard-cutover discipline + Phase 1 Assumption 8: findIaCPluginDir previously json.Unmarshal'd the matching plugin manifest and returned the raw iacProvider.computePlanVersion string without validation. A typo (V2, v2.0, two, v3) would silently route through the v1 dispatch path via wfctlhelpers.DispatchVersionFor's empty/unknown default — breaking the Phase 2 hard-cutover contract. Validation gate now hard-fails on the matching plugin manifest when computePlanVersion is not in {"", "v1", "v2"}. Operators see the misconfiguration loudly with an actionable error instead of silent fallback. Empty/missing field still permitted (defaults to v1 dispatch). Per workflow#693 issue body's 3-option design space, picked option 3 (lightweight enum check, minimum viable) over option 1 (full pluginmanifest package, ~200 LOC overkill) and option 2 (reuse existing schema/ JSON Schema validator, ~50 LOC). YAGNI applies — the field's domain is a 3-element enum closed by the proto spec. 7 test cases (table-driven): empty, v1, v2 pass; V2 (uppercase typo), v2.0 (decimal typo), two (word typo), v3 (future Phase 2.3 tag pre-introduction) all reject with the actionable error. Closes #693.
7 tasks
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
PR 1 of 5 in the workflow#640 Phase 2 hard-cutover cascade per ADR 0024 + 0040. Extends the IaC plugin gRPC contract with per-action outcome evidence so wfctl-side v2 hooks fire correctly. After this PR ships + tags
v0.54.0, the 4 IaC plugin PRs (aws/gcp/azure/digitalocean each v1.2.0) cascade in to declareComputePlanVersion="v2".Changes (6 commits)
515374c2proto extend —ApplyResult.actionsfield at tag 7 +ActionStatusenum (UNSPECIFIED=0/SUCCESS=1/ERROR=2/DELETE_FAILED=3); regeneratediac.pb.goin same atomic commit.b09bced1proto guard —reserved 4, 5;directive onActionStatus(compile-time guard against Phase 2.3 tag reuse) + broadened round-trip subcases.6441c6cdGo types —interfaces.ActionStatus+ActionOutcomestruct; extendApplyResult.Actions.b57c5a90decoder —cmd/wfctl/iac_typed_adapter.go::applyResultFromPBdecodespb.ActionResult→interfaces.ActionOutcome; rejects UNSPECIFIED explicitly.92f73de9decoder fail-closed — unknown ActionStatus wire values error loudly (operator message: "wfctl version too old for this plugin?") per ADR 0040 invariant 2 (no graceful fallback).3e79c9d6engine populate —iac/wfctlhelpers/apply.go::applyPlanWithEnvProviderAndHookspopulatesresult.Actionsvia deferred-closure pattern (covers ALL 7 dispatch-loop exit paths: ctx-cancel, jit-err, driver-resolve-err, hook-dispatch-err, dispatch-other-err, post-delete-hook-err, post-apply-hook-err, success) + post-loop length-validation invariant assert.ADR alignment
ActionOutcome.Status+DELETE_FAILEDdistinct tag.Test plan
GOWORK=off go test ./plugin/external/proto/... -v— proto round-trip + reserved guardGOWORK=off go test ./interfaces/...— type + JSON round-tripGOWORK=off go test ./cmd/wfctl/ -run 'TestApplyResultFromPB|TestMapPB' -v— 6/6 PASSGOWORK=off go test ./iac/wfctlhelpers/ -race— full package PASS race-cleanCascade context
After merge → tag
v0.54.0→ unblocks 4 plugin PRs (each pins workflow v0.54.0 + bumps minEngineVersion + declaresComputePlanVersion="v2"inCapabilitiesResponse).Rollback
If a downstream plugin reveals a contract gap after Phase 2 cascade lands: cut
v0.54.1reverting whichever commit broke (or full revert if proto change is the issue). Per ADR 0040 matched-pair rollback.Plan:
docs/plans/2026-05-16-v2-lifecycle-phase2.md(locked sha256 46b795ec).🤖 Generated with Claude Code