diff --git a/cmd/wfctl/iac_typed_adapter.go b/cmd/wfctl/iac_typed_adapter.go index a5be02ba..bc32276e 100644 --- a/cmd/wfctl/iac_typed_adapter.go +++ b/cmd/wfctl/iac_typed_adapter.go @@ -1274,8 +1274,16 @@ func mapPBActionStatusToInterface(s pb.ActionStatus) (interfaces.ActionStatus, b return interfaces.ActionStatusError, true case pb.ActionStatus_ACTION_STATUS_DELETE_FAILED: return interfaces.ActionStatusDeleteFailed, true + // Phase 2.3 (workflow#698) — engine populates COMPENSATION_FAILED + SKIPPED + // server-side; plugins may emit COMPENSATED if they implement own compensation. + case pb.ActionStatus_ACTION_STATUS_COMPENSATED: + return interfaces.ActionStatusCompensated, true + case pb.ActionStatus_ACTION_STATUS_COMPENSATION_FAILED: + return interfaces.ActionStatusCompensationFailed, true + case pb.ActionStatus_ACTION_STATUS_SKIPPED: + return interfaces.ActionStatusSkipped, true default: - // UNSPECIFIED (tag 0) and any unknown wire value (tags 4+) + // UNSPECIFIED (tag 0) and any unknown wire value (tags 7+) // fall here. Caller surfaces the !ok as an explicit error. return interfaces.ActionStatusUnspecified, false } diff --git a/cmd/wfctl/iac_typed_adapter_test.go b/cmd/wfctl/iac_typed_adapter_test.go index 1b1d2f86..3fd4cef6 100644 --- a/cmd/wfctl/iac_typed_adapter_test.go +++ b/cmd/wfctl/iac_typed_adapter_test.go @@ -602,9 +602,10 @@ func TestApplyResultFromPB_RejectsUnknownStatus(t *testing.T) { } } -// TestMapPBActionStatusToInterface_ActionableValues pins the three -// actionable tags SUCCESS / ERROR / DELETE_FAILED to their -// interfaces.ActionStatus mirrors with ok=true. +// TestMapPBActionStatusToInterface_ActionableValues pins the six +// actionable tags (Phase 2: SUCCESS/ERROR/DELETE_FAILED + Phase 2.3 workflow#698: +// COMPENSATED/COMPENSATION_FAILED/SKIPPED) to their interfaces.ActionStatus +// mirrors with ok=true. func TestMapPBActionStatusToInterface_ActionableValues(t *testing.T) { cases := []struct { name string @@ -614,6 +615,10 @@ func TestMapPBActionStatusToInterface_ActionableValues(t *testing.T) { {"SUCCESS", pb.ActionStatus_ACTION_STATUS_SUCCESS, interfaces.ActionStatusSuccess}, {"ERROR", pb.ActionStatus_ACTION_STATUS_ERROR, interfaces.ActionStatusError}, {"DELETE_FAILED", pb.ActionStatus_ACTION_STATUS_DELETE_FAILED, interfaces.ActionStatusDeleteFailed}, + // Phase 2.3 (workflow#698) — new actionable enum values: + {"COMPENSATED", pb.ActionStatus_ACTION_STATUS_COMPENSATED, interfaces.ActionStatusCompensated}, + {"COMPENSATION_FAILED", pb.ActionStatus_ACTION_STATUS_COMPENSATION_FAILED, interfaces.ActionStatusCompensationFailed}, + {"SKIPPED", pb.ActionStatus_ACTION_STATUS_SKIPPED, interfaces.ActionStatusSkipped}, } for _, c := range cases { got, ok := mapPBActionStatusToInterface(c.in) diff --git a/iac/wfctlhelpers/apply.go b/iac/wfctlhelpers/apply.go index 8ad21189..1ff64e83 100644 --- a/iac/wfctlhelpers/apply.go +++ b/iac/wfctlhelpers/apply.go @@ -297,12 +297,20 @@ func applyPlanWithEnvProviderAndHooks( // pattern below records the outcome unconditionally on every exit // from the inner func; the surrounding for loop then bubbles // fatalErr (hook failures, ctx cancellation) to the caller. - var iterErr error // best-effort error: action failed, continue - var fatalErr error // hook/ctx error: stop the whole apply + var iterErr error // best-effort error: action failed, continue + var iterStatus interfaces.ActionStatus // Phase 2.3 (workflow#698): assigned at each error site + var fatalErr error // hook/ctx error: stop the whole apply func() { defer func() { - status := mapDispatchErrToStatus(iterErr, action.Action) + // Phase 2.3 (workflow#698): success-path default — if no + // error site assigned a phase-specific status, the action + // completed cleanly. Otherwise iterStatus was set by the + // path that raised iterErr to the correct phase-specific + // value (SKIPPED / Error / DeleteFailed / CompensationFailed). + if iterErr == nil { + iterStatus = interfaces.ActionStatusSuccess + } errStr := "" if iterErr != nil { errStr = iterErr.Error() @@ -310,7 +318,7 @@ func applyPlanWithEnvProviderAndHooks( result.Actions = append(result.Actions, interfaces.ActionOutcome{ //nolint:gosec // ActionIndex is loop counter bound by len(plan.Actions); G115 false positive. ActionIndex: uint32(i), - Status: status, + Status: iterStatus, Error: errStr, }) }() @@ -320,9 +328,11 @@ func applyPlanWithEnvProviderAndHooks( // guarantees apply stops between actions even if a driver // happens to ignore ctx. The deferred postcondition still runs // on early return so InputDriftReport is populated even on a - // canceled apply. + // canceled apply. Phase 2.3 (#698): ctx-cancel is pre-dispatch + // — action was never attempted; cloud-side state unchanged. if err := ctx.Err(); err != nil { iterErr = err + iterStatus = statusForPreDispatchSkip() fatalErr = err return } @@ -335,7 +345,8 @@ func applyPlanWithEnvProviderAndHooks( // must not reach the driver. The loop continues to the next // action (best-effort apply contract). os.LookupEnv is the // production env source; nil-safe inside ResolveSpec — refs that - // only need replaceIDMap / syncedOutputs still resolve. + // only need replaceIDMap / syncedOutputs still resolve. Phase 2.3 + // (#698): JIT-fail is pre-dispatch — no driver call yet. resolved, err := jitsubst.ResolveSpec(action.Resource, result.ReplaceIDMap, syncedOutputs, os.LookupEnv) if err != nil { result.Errors = append(result.Errors, interfaces.ActionError{ @@ -344,9 +355,12 @@ func applyPlanWithEnvProviderAndHooks( Error: fmt.Sprintf("jit substitution: %v", err), }) iterErr = fmt.Errorf("jit substitution: %v", err) + iterStatus = statusForPreDispatchSkip() return } action.Resource = resolved + // Phase 2.3 (#698): driver-resolve-fail is pre-dispatch — no + // driver method has been called yet. d, err := p.ResourceDriver(action.Resource.Type) if err != nil { result.Errors = append(result.Errors, interfaces.ActionError{ @@ -355,6 +369,7 @@ func applyPlanWithEnvProviderAndHooks( Error: fmt.Sprintf("resolve driver: %v", err), }) iterErr = fmt.Errorf("resolve driver: %v", err) + iterStatus = statusForPreDispatchSkip() return } // Capture result.Resources length pre-dispatch so we can identify @@ -375,22 +390,35 @@ func applyPlanWithEnvProviderAndHooks( if err := dispatchAction(ctx, d, action, result, actionHooks, deleteHookActive); err != nil { var hookErr hookDispatchError if errors.As(err, &hookErr) { + // Phase 2.3 (#698): hookDispatchError wraps a hook + // (typically driver-layer Delete hook) that ran AFTER + // the cloud-side action — cloud-side work IS done; + // hook failure is post-hook semantically. fatalErr = fmt.Errorf("%s/%s: %w", action.Resource.Type, action.Resource.Name, hookErr.err) iterErr = hookErr.err + iterStatus = statusForPostHookFailure() return } + // Phase 2.3 (#698): generic dispatch error — driver's + // Create/Update/Delete RPC returned err. Action attempted; + // cloud-side state may be partially mutated. result.Errors = append(result.Errors, interfaces.ActionError{ Resource: action.Resource.Name, Action: action.Action, Error: err.Error(), }) iterErr = err + iterStatus = statusForDispatchError(action.Action) return } if action.Action == "delete" { + // Phase 2.3 (#698): post-delete-hook ran AFTER cloud-side + // delete succeeded — cloud-side work IS done; hook failure + // is post-hook semantically. 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 + iterStatus = statusForPostHookFailure() return } } @@ -399,9 +427,13 @@ func applyPlanWithEnvProviderAndHooks( out = fillMissingOutputIdentity(action.Resource, out) result.Resources[len(result.Resources)-1] = out if hooks.OnResourceApplied != nil { + // Phase 2.3 (#698): post-apply-hook ran AFTER cloud-side + // create/update succeeded — cloud-side work IS done; + // hook failure is post-hook semantically. 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 + iterStatus = statusForPostHookFailure() return } } @@ -429,23 +461,46 @@ func applyPlanWithEnvProviderAndHooks( return result, nil } -// mapDispatchErrToStatus translates a per-action dispatch error into the -// Phase 2 ActionStatus value (workflow#640). delete actions that fail map -// to ActionStatusDeleteFailed so wfctl downstream can drop the -// state-removal optimistic-update; all other failures map to -// ActionStatusError. Compensation paths (Phase 2.3 — COMPENSATED / -// COMPENSATION_FAILED) are reserved in iac.proto but not yet emitted -// here. nil err means the dispatch (and any post-hooks) succeeded. -func mapDispatchErrToStatus(err error, actionType string) interfaces.ActionStatus { - if err == nil { - return interfaces.ActionStatusSuccess - } +// Phase 2.3 (workflow#698): replaced single mapDispatchErrToStatus with +// 3 phase-specific helpers — each per-action exit path now assigns its +// status directly from the call site (clearer than late-mapping in defer). +// The single-helper version conflated pre-dispatch failures (ctx-cancel / +// JIT-fail / driver-resolve-fail) and post-hook failures with +// dispatch-level errors, breaking ADR 0040 invariant 2 (failed-delete +// preservation requires distinguishing "delete dispatch failed" from +// "delete never attempted"). + +// statusForPreDispatchSkip returns SKIPPED — action was never attempted +// at the driver. Used for ctx-cancel, JIT-substitution-fail, and +// driver-resolve-fail paths. Cloud-side state unchanged from pre-apply. +// Per workflow#698 Phase 2.3. +func statusForPreDispatchSkip() interfaces.ActionStatus { + return interfaces.ActionStatusSkipped +} + +// statusForDispatchError returns Error (non-delete) or DeleteFailed (delete). +// Used when driver's Create/Update/Delete RPC returned an error. Action was +// attempted; cloud-side state may be partially mutated. For delete actions, +// DELETE_FAILED instructs wfctl to preserve state. +// Per workflow#698 Phase 2.3 (extracted from prior mapDispatchErrToStatus). +func statusForDispatchError(actionType string) interfaces.ActionStatus { if actionType == "delete" { return interfaces.ActionStatusDeleteFailed } return interfaces.ActionStatusError } +// statusForPostHookFailure returns COMPENSATION_FAILED — driver succeeded +// but wfctl-side hook (OnResourceApplied / OnResourceDeleted) failed (or +// the driver's own hook layer returned hookDispatchError after touching +// cloud-side state). Cloud-side work IS done; operator must verify state; +// may need manual compensation. State preservation required regardless of +// action type. +// Per workflow#698 Phase 2.3. +func statusForPostHookFailure() interfaces.ActionStatus { + return interfaces.ActionStatusCompensationFailed +} + func preflightProviderOwnedReplaceWithDeleteHooks(p interfaces.IaCProvider, plan *interfaces.IaCPlan) error { for i := range plan.Actions { action := plan.Actions[i] diff --git a/iac/wfctlhelpers/apply_hooks_test.go b/iac/wfctlhelpers/apply_hooks_test.go index b4aeefec..4743f514 100644 --- a/iac/wfctlhelpers/apply_hooks_test.go +++ b/iac/wfctlhelpers/apply_hooks_test.go @@ -285,8 +285,12 @@ func TestApplyPlanWithHooks_PopulatesActions_PreDispatchDriverError(t *testing.T 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) + // Phase 2.3 (workflow#698) reclassification: driver-resolve-error is + // PRE-DISPATCH — driver's Create/Update/Delete RPC never called; cloud + // state unchanged. Status maps to SKIPPED. Phase 2 mapped this to Error + // which conflated pre-dispatch skip with dispatch-level failure. + if result.Actions[0].Status != interfaces.ActionStatusSkipped { + t.Errorf("driver-resolve-error status: want Skipped (Phase 2.3 pre-dispatch reclassification), got %v", result.Actions[0].Status) } if result.Actions[0].ActionIndex != 0 { t.Errorf("ActionIndex: want 0, got %d", result.Actions[0].ActionIndex) @@ -511,3 +515,23 @@ func TestApplyPlanWithHooks_OnPlanComplete_RecoversFromPanic(t *testing.T) { t.Errorf("expected last result.Errors entry to have Resource=\"\"; got: %+v", result.Errors) } } + +// TestStatusHelpers_Phase23 pins the 3 phase-specific helper functions +// per workflow#698 Phase 2.3 to their plan-spec'd return values. +func TestStatusHelpers_Phase23(t *testing.T) { + if got := statusForPreDispatchSkip(); got != interfaces.ActionStatusSkipped { + t.Errorf("statusForPreDispatchSkip() = %v; want ActionStatusSkipped", got) + } + if got := statusForDispatchError("create"); got != interfaces.ActionStatusError { + t.Errorf("statusForDispatchError(\"create\") = %v; want ActionStatusError", got) + } + if got := statusForDispatchError("update"); got != interfaces.ActionStatusError { + t.Errorf("statusForDispatchError(\"update\") = %v; want ActionStatusError", got) + } + if got := statusForDispatchError("delete"); got != interfaces.ActionStatusDeleteFailed { + t.Errorf("statusForDispatchError(\"delete\") = %v; want ActionStatusDeleteFailed", got) + } + if got := statusForPostHookFailure(); got != interfaces.ActionStatusCompensationFailed { + t.Errorf("statusForPostHookFailure() = %v; want ActionStatusCompensationFailed", got) + } +} diff --git a/interfaces/iac_state.go b/interfaces/iac_state.go index 6729ed1a..6f947fbf 100644 --- a/interfaces/iac_state.go +++ b/interfaces/iac_state.go @@ -143,10 +143,14 @@ type DriftEntry struct { // ActionStatus categorizes per-action outcomes for wfctl-side hook dispatch. // Mirrors pb.ActionStatus (plugin/external/proto/iac.proto) for type-safe Go -// boundary use; constant tags 0/1/2/3 MUST stay in lockstep with the proto. -// Per workflow#640 Phase 2 + ADR 0040 invariants 1-2. Tags 4-5 are reserved -// in the proto for Phase 2.3 compensation (ActionStatusCompensated + -// ActionStatusCompensationFailed) and intentionally not declared here yet. +// boundary use; constant tags 0-6 MUST stay in lockstep with the proto. +// Per workflow#640 Phase 2 + workflow#698 Phase 2.3 + ADR 0040 invariants 1-2. +// +// Phase 2.3 reclassification: the engine now distinguishes pre-dispatch +// (Skipped), dispatch (Error/DeleteFailed), and post-hook (CompensationFailed) +// failures. Consumers that previously checked ActionStatusError alone for +// "action did not produce desired end-state" should now check +// {Error, DeleteFailed, CompensationFailed, Skipped}. type ActionStatus uint8 const ( @@ -157,6 +161,10 @@ const ( ActionStatusSuccess ActionStatusError ActionStatusDeleteFailed + // Phase 2.3 (workflow#698): + ActionStatusCompensated // reserved-for-future-emission; engine v0.56.0 does NOT emit (no auto-compensation); plugins may emit if they implement own compensation + ActionStatusCompensationFailed // post-driver-success hook failed; cloud-side work IS done; operator must verify state; manual compensation may be required + ActionStatusSkipped // action never attempted at driver (ctx-cancel pre-dispatch, JIT-substitution-fail, driver-resolve-fail); cloud-side state unchanged ) // ActionOutcome mirrors pb.ActionResult. Engine populates one entry per diff --git a/interfaces/iac_state_phase23_test.go b/interfaces/iac_state_phase23_test.go new file mode 100644 index 00000000..460f7047 --- /dev/null +++ b/interfaces/iac_state_phase23_test.go @@ -0,0 +1,29 @@ +package interfaces + +import "testing" + +// TestActionStatus_Phase23_ValuesMatchProtoTags asserts ActionStatus iota +// values match plugin/external/proto/iac.proto ActionStatus enum tag numbers. +// Per workflow#698 Phase 2.3. +func TestActionStatus_Phase23_ValuesMatchProtoTags(t *testing.T) { + cases := []struct { + name string + status ActionStatus + wantInt int + }{ + {"Unspecified=0", ActionStatusUnspecified, 0}, + {"Success=1", ActionStatusSuccess, 1}, + {"Error=2", ActionStatusError, 2}, + {"DeleteFailed=3", ActionStatusDeleteFailed, 3}, + {"Compensated=4", ActionStatusCompensated, 4}, + {"CompensationFailed=5", ActionStatusCompensationFailed, 5}, + {"Skipped=6", ActionStatusSkipped, 6}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if int(c.status) != c.wantInt { + t.Errorf("%s: int(%d) != want %d", c.name, c.status, c.wantInt) + } + }) + } +} diff --git a/plugin/external/proto/iac.pb.go b/plugin/external/proto/iac.pb.go index 257fb12f..7ca2e1fa 100644 --- a/plugin/external/proto/iac.pb.go +++ b/plugin/external/proto/iac.pb.go @@ -163,6 +163,12 @@ const ( ActionStatus_ACTION_STATUS_SUCCESS ActionStatus = 1 ActionStatus_ACTION_STATUS_ERROR ActionStatus = 2 ActionStatus_ACTION_STATUS_DELETE_FAILED ActionStatus = 3 + // Phase 2.3 (workflow#698): reserved tags 4+5 from Phase 2 now defined + + // tag 6 for "skipped" — replaces misclassification of pre-dispatch + // ctx-cancel/JIT-fail/driver-resolve-fail as Error/DeleteFailed. + ActionStatus_ACTION_STATUS_COMPENSATED ActionStatus = 4 // reserved-for-future-emission; engine v0.56.0 does NOT emit (no auto-compensation); plugins may emit if they implement own compensation + ActionStatus_ACTION_STATUS_COMPENSATION_FAILED ActionStatus = 5 // post-driver-success hook failed; cloud-side work IS done; operator must verify state; manual compensation may be required + ActionStatus_ACTION_STATUS_SKIPPED ActionStatus = 6 // action never attempted at driver (ctx-cancel pre-dispatch, JIT-substitution-fail, driver-resolve-fail); cloud-side state unchanged ) // Enum value maps for ActionStatus. @@ -172,12 +178,18 @@ var ( 1: "ACTION_STATUS_SUCCESS", 2: "ACTION_STATUS_ERROR", 3: "ACTION_STATUS_DELETE_FAILED", + 4: "ACTION_STATUS_COMPENSATED", + 5: "ACTION_STATUS_COMPENSATION_FAILED", + 6: "ACTION_STATUS_SKIPPED", } ActionStatus_value = map[string]int32{ - "ACTION_STATUS_UNSPECIFIED": 0, - "ACTION_STATUS_SUCCESS": 1, - "ACTION_STATUS_ERROR": 2, - "ACTION_STATUS_DELETE_FAILED": 3, + "ACTION_STATUS_UNSPECIFIED": 0, + "ACTION_STATUS_SUCCESS": 1, + "ACTION_STATUS_ERROR": 2, + "ACTION_STATUS_DELETE_FAILED": 3, + "ACTION_STATUS_COMPENSATED": 4, + "ACTION_STATUS_COMPENSATION_FAILED": 5, + "ACTION_STATUS_SKIPPED": 6, } ) @@ -5996,12 +6008,15 @@ const file_iac_proto_rawDesc = "" + "\x16PlanDiagnosticSeverity\x12\x18\n" + "\x14PLAN_DIAGNOSTIC_INFO\x10\x00\x12\x1b\n" + "\x17PLAN_DIAGNOSTIC_WARNING\x10\x01\x12\x19\n" + - "\x15PLAN_DIAGNOSTIC_ERROR\x10\x02*\x8e\x01\n" + + "\x15PLAN_DIAGNOSTIC_ERROR\x10\x02*\xe3\x01\n" + "\fActionStatus\x12\x1d\n" + "\x19ACTION_STATUS_UNSPECIFIED\x10\x00\x12\x19\n" + "\x15ACTION_STATUS_SUCCESS\x10\x01\x12\x17\n" + "\x13ACTION_STATUS_ERROR\x10\x02\x12\x1f\n" + - "\x1bACTION_STATUS_DELETE_FAILED\x10\x03\"\x04\b\x04\x10\x04\"\x04\b\x05\x10\x052\xc4\t\n" + + "\x1bACTION_STATUS_DELETE_FAILED\x10\x03\x12\x1d\n" + + "\x19ACTION_STATUS_COMPENSATED\x10\x04\x12%\n" + + "!ACTION_STATUS_COMPENSATION_FAILED\x10\x05\x12\x19\n" + + "\x15ACTION_STATUS_SKIPPED\x10\x062\xc4\t\n" + "\x13IaCProviderRequired\x12o\n" + "\n" + "Initialize\x12/.workflow.plugin.external.iac.InitializeRequest\x1a0.workflow.plugin.external.iac.InitializeResponse\x12]\n" + diff --git a/plugin/external/proto/iac.proto b/plugin/external/proto/iac.proto index 8bf88834..b6d7925c 100644 --- a/plugin/external/proto/iac.proto +++ b/plugin/external/proto/iac.proto @@ -313,7 +313,12 @@ enum ActionStatus { ACTION_STATUS_SUCCESS = 1; ACTION_STATUS_ERROR = 2; ACTION_STATUS_DELETE_FAILED = 3; - reserved 4, 5; // Phase 2.3 compensation (ACTION_STATUS_COMPENSATED / COMPENSATION_FAILED) + // Phase 2.3 (workflow#698): reserved tags 4+5 from Phase 2 now defined + + // tag 6 for "skipped" — replaces misclassification of pre-dispatch + // ctx-cancel/JIT-fail/driver-resolve-fail as Error/DeleteFailed. + ACTION_STATUS_COMPENSATED = 4; // reserved-for-future-emission; engine v0.56.0 does NOT emit (no auto-compensation); plugins may emit if they implement own compensation + ACTION_STATUS_COMPENSATION_FAILED = 5; // post-driver-success hook failed; cloud-side work IS done; operator must verify state; manual compensation may be required + ACTION_STATUS_SKIPPED = 6; // action never attempted at driver (ctx-cancel pre-dispatch, JIT-substitution-fail, driver-resolve-fail); cloud-side state unchanged } // ActionResult is the per-action outcome surfacing for Phase 2 v2 hooks.