feat(provider): implement DetectDrift with ghost + config classification#46
Merged
feat(provider): implement DetectDrift with ghost + config classification#46
Conversation
…classes Replace stub DetectDrift (returning Drifted: false for everything) with a real implementation driven by each resource driver's Read + Diff methods. Three drift classes (workflow v0.20.5 interfaces.DriftClass enum): - DriftClassGhost: cloud Read returns interfaces.ErrResourceNotFound (404) → state has the resource, DO does not. wfctl infra apply --refresh can safely prune this state entry so the next plan generates a create action. - DriftClassConfig: Read succeeds + Diff.NeedsUpdate || NeedsReplace=true → state and cloud both have the resource but configs differ. Fields/ Expected/Actual are populated from DiffResult.Changes for operator clarity. - DriftClassInSync: Read succeeds + no diff → state and cloud agree. - DriftClassUnknown: driver registry lookup fails for ref.Type → operator must investigate; surfaced as Drifted=true so it's not silently ignored. Production-safety invariant: transient errors (rate-limit, auth, network) propagate WITHOUT a DriftResult so callers cannot accidentally prune state on a bad-day API response. Only genuine ErrResourceNotFound gates the ghost path. drivers.ErrResourceNotFound (app_platform.go) is now aliased to interfaces.ErrResourceNotFound so list-scan not-found returns satisfy errors.Is(err, interfaces.ErrResourceNotFound) cross-package. The local var is retained for backwards compat with existing callers (drivers_test). 6 unit tests (all pass, no live DO API calls): - TestDetectDrift_NotFoundReturnsGhost - TestDetectDrift_DiffReturnsConfig - TestDetectDrift_NoDiffReturnsInSync - TestDetectDrift_TransientErrorPropagates - TestDetectDrift_UnknownTypeReturnsUnknown - TestErrResourceNotFound_AliasedToInterfacesSentinel Unblocks core-dump drift recovery: state thinks coredump-staging-vpc + coredump-staging-db exist; DO returns 404. After this PR + lockfile bump, wfctl infra drift will surface 2 DriftClassGhost entries, and wfctl infra apply --refresh --auto-approve will prune + recreate them. See workflow/docs/plans/2026-05-02-infra-drift-recovery-design.md Section 1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements a non-stub DOProvider.DetectDrift so the DigitalOcean provider can classify drift results using the newer workflow DriftClass enum, and updates the provider dependency accordingly.
Changes:
- Replaces the old always-
Drifted:falseDetectDriftstub with real read/diff-based classification logic. - Aligns
drivers.ErrResourceNotFoundwith the workflow sentinel used for ghost-resource detection. - Adds focused unit tests for drift classification paths and bumps
github.com/GoCodeAlone/workflowtov0.20.5.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/provider_detect_drift_test.go |
Adds unit tests for ghost/config/in-sync/unknown/error drift outcomes. |
internal/provider.go |
Implements the new drift detection and classification flow. |
internal/drivers/app_platform.go |
Aliases the local not-found sentinel to the workflow sentinel. |
go.sum |
Updates dependency checksums for the workflow version bump. |
go.mod |
Bumps the workflow dependency to v0.20.5. |
CHANGELOG.md |
Documents the new drift-detection behavior and dependency update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+466
to
+514
| // Read succeeded — check for config drift via the driver's Diff method. | ||
| // We pass an empty desired spec (only Name+Type from ref) because DetectDrift | ||
| // does not have access to the full declared config at this layer. Drivers' | ||
| // Diff implementations compare against the live state; an empty desired spec | ||
| // means drivers will report drift only for fields they can compare directly | ||
| // from the current output vs. their own reference values. | ||
| // | ||
| // For richer config-drift detection (desired config from the IaC spec), | ||
| // callers should use wfctl infra plan which has access to the full config. | ||
| diff, diffErr := d.Diff(ctx, interfaces.ResourceSpec{Name: ref.Name, Type: ref.Type}, out) | ||
| if diffErr != nil { | ||
| // Diff failure is treated as inconclusive — still record as InSync to | ||
| // avoid false positives; callers can run wfctl infra plan for a full check. | ||
| results = append(results, interfaces.DriftResult{ | ||
| Name: ref.Name, | ||
| Type: ref.Type, | ||
| Drifted: false, | ||
| Class: interfaces.DriftClassInSync, | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| if diff != nil && (diff.NeedsUpdate || diff.NeedsReplace) { | ||
| // Extract changed field paths and build Expected/Actual maps. | ||
| fields := make([]string, 0, len(diff.Changes)) | ||
| expected := make(map[string]any, len(diff.Changes)) | ||
| actual := make(map[string]any, len(diff.Changes)) | ||
| for _, ch := range diff.Changes { | ||
| fields = append(fields, ch.Path) | ||
| expected[ch.Path] = ch.New | ||
| actual[ch.Path] = ch.Old | ||
| } | ||
| results = append(results, interfaces.DriftResult{ | ||
| Name: ref.Name, | ||
| Type: ref.Type, | ||
| Drifted: true, | ||
| Class: interfaces.DriftClassConfig, | ||
| Fields: fields, | ||
| Expected: expected, | ||
| Actual: actual, | ||
| }) | ||
| } else { | ||
| results = append(results, interfaces.DriftResult{ | ||
| Name: ref.Name, | ||
| Type: ref.Type, | ||
| Drifted: false, | ||
| Class: interfaces.DriftClassInSync, | ||
| }) | ||
| } |
Comment on lines
+466
to
+474
| // Read succeeded — check for config drift via the driver's Diff method. | ||
| // We pass an empty desired spec (only Name+Type from ref) because DetectDrift | ||
| // does not have access to the full declared config at this layer. Drivers' | ||
| // Diff implementations compare against the live state; an empty desired spec | ||
| // means drivers will report drift only for fields they can compare directly | ||
| // from the current output vs. their own reference values. | ||
| // | ||
| // For richer config-drift detection (desired config from the IaC spec), | ||
| // callers should use wfctl infra plan which has access to the full config. |
Comment on lines
+477
to
+485
| // Diff failure is treated as inconclusive — still record as InSync to | ||
| // avoid false positives; callers can run wfctl infra plan for a full check. | ||
| results = append(results, interfaces.DriftResult{ | ||
| Name: ref.Name, | ||
| Type: ref.Type, | ||
| Drifted: false, | ||
| Class: interfaces.DriftClassInSync, | ||
| }) | ||
| continue |
…sults on transient error Critical: diffErr now maps to DriftClassUnknown (Drifted=true, error in Fields) instead of silently treating a broken Diff impl as InSync. Operators running `wfctl infra drift` no longer see false green-all-clear when a driver's Diff is incomplete or erroring. Important: transient Read errors now return (nil, err) instead of (partial, err), eliminating the footgun where callers could act on an incomplete drift picture. Both fixes backed by new regression tests (TDD: red → green): - TestDetectDrift_DiffErrorReturnsUnknown - TestDetectDrift_TransientErrorDiscardsPriorResults Minor: merged duplicate ### Added sections in CHANGELOG [Unreleased]. Nit: added DriftClassConfig spec-limitation note to CHANGELOG entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+469
to
+478
| // Read succeeded — check for config drift via the driver's Diff method. | ||
| // We pass an empty desired spec (only Name+Type from ref) because DetectDrift | ||
| // does not have access to the full declared config at this layer. Drivers' | ||
| // Diff implementations compare against the live state; an empty desired spec | ||
| // means drivers will report drift only for fields they can compare directly | ||
| // from the current output vs. their own reference values. | ||
| // | ||
| // For richer config-drift detection (desired config from the IaC spec), | ||
| // callers should use wfctl infra plan which has access to the full config. | ||
| diff, diffErr := d.Diff(ctx, interfaces.ResourceSpec{Name: ref.Name, Type: ref.Type}, out) |
Comment on lines
+84
to
+97
| // TestDetectDrift_DiffReturnsConfig verifies that when Read succeeds and Diff | ||
| // reports drift (NeedsUpdate=true with Changes), the result is classified as | ||
| // DriftClassConfig. | ||
| func TestDetectDrift_DiffReturnsConfig(t *testing.T) { | ||
| p := newProviderWithFakeDriver("infra.vpc", &fakeDriverForDrift{ | ||
| readOutput: &interfaces.ResourceOutput{Name: "test-vpc", Type: "infra.vpc", Status: "active"}, | ||
| diffResult: &interfaces.DiffResult{ | ||
| NeedsUpdate: true, | ||
| Changes: []interfaces.FieldChange{ | ||
| {Path: "region", Old: "nyc1", New: "nyc3"}, | ||
| }, | ||
| }, | ||
| }) | ||
| refs := []interfaces.ResourceRef{{Name: "test-vpc", Type: "infra.vpc"}} |
Comment on lines
+16
to
+19
| - `DriftClassConfig` (`Drifted: true`): `Read` succeeds and `Diff` reports | ||
| `NeedsUpdate || NeedsReplace`. Caller should reconcile via plan/apply. | ||
| Drifted field paths are recorded in `DriftResult.Fields`; `Expected` and | ||
| `Actual` maps contain the `New`/`Old` values from `DiffResult.Changes`. |
Comment on lines
+469
to
+478
| // Read succeeded — check for config drift via the driver's Diff method. | ||
| // We pass an empty desired spec (only Name+Type from ref) because DetectDrift | ||
| // does not have access to the full declared config at this layer. Drivers' | ||
| // Diff implementations compare against the live state; an empty desired spec | ||
| // means drivers will report drift only for fields they can compare directly | ||
| // from the current output vs. their own reference values. | ||
| // | ||
| // For richer config-drift detection (desired config from the IaC spec), | ||
| // callers should use wfctl infra plan which has access to the full config. | ||
| diff, diffErr := d.Diff(ctx, interfaces.ResourceSpec{Name: ref.Name, Type: ref.Type}, out) |
Fixes Copilot Important findings from PR #46: passing an empty ResourceSpec to driver Diff methods causes false-positive drift reports (VPC reads ip_range from spec.Config; AppPlatform canonicalExpose defaults to "public" on empty spec, misreporting any app with expose:internal as drifted). DetectDrift's load-bearing job is ghost detection for wfctl infra apply --refresh ghost-prune. Config-drift detection requires the declared spec, which the IaCProvider interface does not supply at this layer — that path belongs to wfctl infra plan. Remove the d.Diff() call and DriftClassConfig path entirely. Test changes: - Remove: TestDetectDrift_DiffReturnsConfig, TestDetectDrift_NoDiffReturnsInSync, TestDetectDrift_DiffErrorReturnsUnknown (no longer applicable) - Add: TestDetectDrift_ReadOkReturnsInSync (fakeDriverForDrift.Diff panics if called, confirmed red→green) - fakeDriverForDrift: drop diffResult/diffErr fields Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+85
to
+92
| // TestDetectDrift_ReadOkReturnsInSync verifies that when Read succeeds, the result | ||
| // is classified as DriftClassInSync with Drifted=false — even when Diff would | ||
| // return a NeedsUpdate result. DetectDrift must NOT call Diff; config-drift | ||
| // detection is deferred to wfctl infra plan which has access to the declared spec. | ||
| // | ||
| // The fakeDriverForDrift.Diff method panics if called, so any invocation from | ||
| // DetectDrift will cause this test to fail with an explicit panic message. | ||
| func TestDetectDrift_ReadOkReturnsInSync(t *testing.T) { |
Comment on lines
+476
to
+478
| // Read succeeded — classify as InSync. Config-drift detection routes | ||
| // through `wfctl infra plan` which has access to the declared spec. | ||
| results = append(results, interfaces.DriftResult{ |
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
Replaces the stub
DOProvider.DetectDrift(which returnedDrifted: falsefor everything) with a real implementation that classifies drift per the newinterfaces.DriftClassenum (workflow v0.20.5):interfaces.ErrResourceNotFound(cloud says 404)NeedsUpdate || NeedsReplace(cloud config differs from state).Fields,Expected,Actualpopulated fromDiffResult.Changes.Production-safety invariant: transient API errors (non-404) propagate WITHOUT a DriftResult, so they never trigger spurious state-prune in
wfctl infra apply --refresh.drivers.ErrResourceNotFound(app_platform.go) is now aliased tointerfaces.ErrResourceNotFoundso list-scan not-found returns satisfy cross-packageerrors.Is(err, interfaces.ErrResourceNotFound).go.mod bumped to workflow v0.20.5.
Test plan
TestDetectDrift_NotFoundReturnsGhost— fake driver Read returnsinterfaces.ErrResourceNotFound; expectDriftClassGhostTestDetectDrift_DiffReturnsConfig— fake driver Read OK + Diff returnsNeedsUpdate=true; expectDriftClassConfigwith drifted fieldsTestDetectDrift_NoDiffReturnsInSync— fake driver Read OK + Diff no drift; expectDriftClassInSyncTestDetectDrift_TransientErrorPropagates— fake driver Read returns generic error (not 404); expect error propagation, no DriftResultTestDetectDrift_UnknownTypeReturnsUnknown— ref.Type has no driver; expectDriftClassUnknownTestErrResourceNotFound_AliasedToInterfacesSentinel— cross-packageerrors.Isresolves correctlygo test ./...green (no regressions)wfctl infra driftagainst staging surfaces VPC + DB ghostsSequencing
PR-D1 of the drift-recovery chain. After merge + workflow-plugin-digitalocean release tag (v0.8.2):
🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com