From 69718efc732c660bca51e7e8ca47cc7932036d32 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 02:39:09 -0400 Subject: [PATCH 01/12] deps(workflow): bump to v0.26.0 for EnumeratorAll + RotationResult types (PR4b prep) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the Task 15 Step 1 go.mod bump landed AHEAD of Tasks 11-14 so that their failing-test commits compile against the new workflow surface area (`interfaces.EnumeratorAll`, `cmd/wfctl.RotationResult`, `bootstrapSecrets` storage-filter behavior). Without this prep commit the rest of the PR4b stack would fail to compile against the prior v0.25.0 pin. Per plan section "PR4b — DO plugin infra.spaces_key driver + EnumeratorAll": > Sequencing: PR4b — but Task 15 go.mod bump must run at the START of > PR4b before any code in Tasks 11-14 references workflow types from > the new tag (won't compile against old workflow tag). `go test ./...` and `go build ./...` both green against v0.26.0 (no existing call sites broke; the bump is purely additive on the workflow side). Co-Authored-By: Claude Opus 4.7 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 92223c4..2364fe8 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/GoCodeAlone/workflow-plugin-digitalocean go 1.26.0 require ( - github.com/GoCodeAlone/workflow v0.25.0 + github.com/GoCodeAlone/workflow v0.26.0 github.com/aws/aws-sdk-go-v2 v1.41.6 github.com/aws/aws-sdk-go-v2/credentials v1.19.15 github.com/aws/aws-sdk-go-v2/service/s3 v1.97.2 diff --git a/go.sum b/go.sum index 9f6b8a5..9b380d3 100644 --- a/go.sum +++ b/go.sum @@ -56,8 +56,8 @@ github.com/GoCodeAlone/modular/modules/jsonschema v1.15.0 h1:xb1mI4NZkzvNKQ2F6nk github.com/GoCodeAlone/modular/modules/jsonschema v1.15.0/go.mod h1:hhGouwAVsonmJ4Lain4jINZ9nZCoc9l9eF3BHbmR8eE= github.com/GoCodeAlone/modular/modules/reverseproxy/v2 v2.8.0 h1:cvdLHbM/vzvygQTcAWSJsy+dAPzzwWyjzKMmTBFcFIo= github.com/GoCodeAlone/modular/modules/reverseproxy/v2 v2.8.0/go.mod h1:/9ipMG4qM2CHQ14BfXKdVlYRJelef6M8MFI5TbZv67M= -github.com/GoCodeAlone/workflow v0.25.0 h1:m+Y+daZswMqUxzq64PBBGrLkoX4dwV1QcLS+jM97sCM= -github.com/GoCodeAlone/workflow v0.25.0/go.mod h1:Ue+5YDScTZgtA36q6r/kDaIRxGJFkyxXbeyJVNVJ0Cc= +github.com/GoCodeAlone/workflow v0.26.0 h1:IiDb92K7o3pP7g4p9JSEHUY6h9Slj1hkgC6IQM8IjPg= +github.com/GoCodeAlone/workflow v0.26.0/go.mod h1:Ue+5YDScTZgtA36q6r/kDaIRxGJFkyxXbeyJVNVJ0Cc= github.com/GoCodeAlone/yaegi v0.17.2 h1:WK6Y6e0t1a6U7r+S2dN3CGWW1PizYD3zO0zneToZPxM= github.com/GoCodeAlone/yaegi v0.17.2/go.mod h1:z5Pr6Wse6QJcQvpgxTxzMAevFarH0N37TG88Y9dprx0= github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.32.0 h1:rIkQfkCOVKc1OiRCNcSDD8ml5RJlZbH/Xsq7lbpynwc= From 21250dbf2586b399dd62b60ac87371df7b7b6dcf Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 02:41:19 -0400 Subject: [PATCH 02/12] test(do): failing test for SpacesKeyDriver.Create happy path --- internal/drivers/spaces_key_test.go | 164 ++++++++++++++++++++++++++++ 1 file changed, 164 insertions(+) create mode 100644 internal/drivers/spaces_key_test.go diff --git a/internal/drivers/spaces_key_test.go b/internal/drivers/spaces_key_test.go new file mode 100644 index 0000000..0de4eb3 --- /dev/null +++ b/internal/drivers/spaces_key_test.go @@ -0,0 +1,164 @@ +package drivers + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" + "github.com/GoCodeAlone/workflow/secrets" + "github.com/digitalocean/godo" +) + +// TestSpacesKeyDriver_Create_HappyPath is the contract test for +// SpacesKeyDriver.Create. It pins the split-storage pattern that is the whole +// point of the driver: +// +// - ResourceOutput.ProviderID == access_key (the godo identifier; matches +// the sister bucket driver in spaces.go). +// - State outputs contain the metadata needed for downstream resources +// (name, access_key, created_at) but NEVER secret_key — the secret +// belongs in the secrets provider, not in IaC state files. +// - The provided secrets.Provider receives BOTH the access_key and +// secret_key as `_access_key` / `_secret_key` +// so consumers (apps, runners) can read them by canonical name. +// - SensitiveKeys() includes "access_key" so wfctl masks it in plan/diff +// output. secret_key isn't in Outputs so doesn't need masking there. +// +// This test is the failing-side of the Task 11/12 TDD pair (PR4b). Until +// Task 12 lands NewSpacesKeyDriver in spaces_key.go, the file fails to +// compile with `undefined: NewSpacesKeyDriver`. Per ADR 0020. +func TestSpacesKeyDriver_Create_HappyPath(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys" && r.Method == http.MethodPost { + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{ + "key": map[string]any{ + "name": "test-key", + "access_key": "AKIATEST123", + "secret_key": "SK_secret_data_here", + "created_at": "2026-05-08T11:00:00Z", + "grants": []any{map[string]any{"permission": "fullaccess"}}, + }, + }) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + })) + defer srv.Close() + + client := godoClientForTest(t, srv.URL) + fakeGHSecrets := &fakeSecretsProvider{stored: map[string]string{}} + + driver := NewSpacesKeyDriver(client, fakeGHSecrets) + spec := interfaces.ResourceSpec{ + Name: "test-key", + Type: "infra.spaces_key", + Config: map[string]any{ + "name": "test-key", + "grants": []any{map[string]any{"permission": "fullaccess"}}, + }, + } + + out, err := driver.Create(context.Background(), spec) + if err != nil { + t.Fatalf("Create: %v", err) + } + + // ResourceOutput per workflow interfaces/iac_resource_driver.go. + // ProviderID == access_key (the godo identifier; matches sister bucket + // driver pattern at internal/drivers/spaces.go). + if out.ProviderID != "AKIATEST123" { + t.Errorf("ProviderID: want AKIATEST123 (access_key as godo ID), got %q", out.ProviderID) + } + + // State outputs must contain {name, access_key, created_at}, NOT secret_key. + if got, _ := out.Outputs["access_key"].(string); got != "AKIATEST123" { + t.Errorf("access_key: want AKIATEST123, got %v", out.Outputs["access_key"]) + } + if got, _ := out.Outputs["created_at"].(string); got != "2026-05-08T11:00:00Z" { + t.Errorf("created_at: want 2026-05-08T11:00:00Z, got %v", out.Outputs["created_at"]) + } + if _, ok := out.Outputs["secret_key"]; ok { + t.Errorf("secret_key MUST NOT be in state outputs (must live in GH Secrets only)") + } + + // Both sub-keys must reach the secrets.Provider under the canonical + // `_access_key` / `_secret_key` names. + if got := fakeGHSecrets.stored["test-key_access_key"]; got != "AKIATEST123" { + t.Errorf("GH Secret test-key_access_key: want AKIATEST123, got %q", got) + } + if got := fakeGHSecrets.stored["test-key_secret_key"]; got != "SK_secret_data_here" { + t.Errorf("GH Secret test-key_secret_key: want SK_secret_data_here, got %q", got) + } + + // SensitiveKeys returns access_key (so wfctl masks it in logs). + // secret_key is NOT in Outputs, so doesn't need state-side masking; + // the secrets.Provider handles its sensitivity separately. + sensitive := driver.SensitiveKeys() + found := false + for _, k := range sensitive { + if k == "access_key" { + found = true + break + } + } + if !found { + t.Errorf("SensitiveKeys must include access_key; got %v", sensitive) + } +} + +// godoClientForTest builds a godo client whose BaseURL points at the given +// httptest server. Mirrors the pattern in `internal/provider_enumerator_test.go` +// (`newProviderForEnumeratorTest`) — godo uses BaseURL for every request, so +// rewriting it is the standard hermetic test stub. +func godoClientForTest(t *testing.T, serverURL string) *godo.Client { + t.Helper() + client := godo.NewClient(http.DefaultClient) + base, err := url.Parse(serverURL + "/") + if err != nil { + t.Fatalf("parse httptest URL %q: %v", serverURL, err) + } + client.BaseURL = base + return client +} + +// fakeSecretsProvider is a minimal in-memory secrets.Provider that records +// every Set so tests can assert which sub-keys were stored. Get/Delete/List +// implement the interface contract; the Spaces key driver only uses Set. +type fakeSecretsProvider struct { + stored map[string]string +} + +func (f *fakeSecretsProvider) Name() string { return "fake" } + +func (f *fakeSecretsProvider) Get(_ context.Context, key string) (string, error) { + if v, ok := f.stored[key]; ok { + return v, nil + } + return "", secrets.ErrNotFound +} + +func (f *fakeSecretsProvider) Set(_ context.Context, key, value string) error { + if f.stored == nil { + f.stored = map[string]string{} + } + f.stored[key] = value + return nil +} + +func (f *fakeSecretsProvider) Delete(_ context.Context, key string) error { + delete(f.stored, key) + return nil +} + +func (f *fakeSecretsProvider) List(_ context.Context) ([]string, error) { + keys := make([]string, 0, len(f.stored)) + for k := range f.stored { + keys = append(keys, k) + } + return keys, nil +} From d876e7fb6e84eea5baf724fabb4068483c6726f3 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 02:44:59 -0400 Subject: [PATCH 03/12] test(do): failing test for EnumeratorAll on infra.spaces_key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds TestProvider_EnumerateAll_SpacesKeys + supporting newDOProviderForTest helper to internal/provider_test.go, per Task 14 of the spaces-key-iac-resource plan (docs/plans/2026-05-08-spaces-key-iac-resource.md, commit 316559f7). The test fakes a paginated DO API response (page 1 → 2 keys + next link; page 2 → 1 key) and asserts that *DOProvider implements interfaces.EnumeratorAll (added in workflow v0.26.0), that EnumerateAll(ctx, "infra.spaces_key") returns 3 *ResourceOutput entries (pagination handled inside the provider, not punted to caller), and that each output has Type, ProviderID (= access_key), and Outputs.{access_key, created_at} populated so audit-keys / prune CLIs can filter without re-reading. Currently FAILS with `Provider must implement EnumeratorAll` — the failing-side signal Task 14 is supposed to produce. Task 15's EnumerateAll implementation will make it PASS. Stacks on top of Task 11's failing test for SpacesKeyDriver.Create (commit 21250db); both share the workflow v0.26.0 pin from commit 69718ef. Co-Authored-By: Claude Opus 4.7 --- internal/provider_test.go | 106 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/internal/provider_test.go b/internal/provider_test.go index cbf45fb..aac8ec3 100644 --- a/internal/provider_test.go +++ b/internal/provider_test.go @@ -2,9 +2,12 @@ package internal import ( "context" + "encoding/json" "fmt" "io" "net/http" + "net/http/httptest" + "net/url" "os" "strings" "sync" @@ -1096,3 +1099,106 @@ func TestDOProvider_Apply_UpsertAllDrivers(t *testing.T) { } } } + +// newDOProviderForTest builds a *DOProvider whose godo client points at the +// given httptest server URL. Mirrors newProviderForEnumeratorTest in +// provider_enumerator_test.go but takes a raw URL instead of a typed mock so +// the EnumeratorAll spaces_key test can drive its own paginated handler. +// +// godo.NewClient + BaseURL override is the standard hermetic pattern used +// elsewhere in this package (see provider_enumerator_test.go:144 and +// internal/drivers/spaces_key_test.go:118). +func newDOProviderForTest(t *testing.T, serverURL string) *DOProvider { + t.Helper() + client := godo.NewClient(http.DefaultClient) + base, err := url.Parse(serverURL + "/") + if err != nil { + t.Fatalf("parse httptest URL %q: %v", serverURL, err) + } + client.BaseURL = base + return &DOProvider{client: client, region: "nyc3"} +} + +// TestProvider_EnumerateAll_SpacesKeys is the contract test for the +// EnumeratorAll interface implementation on the DO provider, scoped to the +// "infra.spaces_key" resource type. Spaces keys live outside the DO tag +// system so the existing EnumerateByTag path cannot reach them; the audit / +// prune CLIs (Tasks 17, 19, 21) need a tag-free enumeration path that +// returns every key in the account, paginated transparently. +// +// The test fakes a paginated DO API response (page 1 returns 2 keys with a +// next-page link; page 2 returns 1 key) and asserts: +// +// - DOProvider implements interfaces.EnumeratorAll (added in workflow +// v0.26.0; see iac_provider.go). +// - EnumerateAll(ctx, "infra.spaces_key") returns 3 *ResourceOutput +// entries — pagination must be handled inside the provider, not punted +// to the caller. +// - Each output has Type="infra.spaces_key", non-empty ProviderID +// (= access_key, matching the SpacesKeyDriver.Create contract), and +// Outputs.access_key + Outputs.created_at populated so audit-keys / prune +// can filter by age without re-reading every key. +// +// Until Task 15 lands the EnumerateAll method on *DOProvider, this test +// fails with `Provider must implement EnumeratorAll` — which is the +// failing-side signal Task 14 is supposed to produce. +func TestProvider_EnumerateAll_SpacesKeys(t *testing.T) { + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/v2/spaces/keys" { + w.Header().Set("Content-Type", "application/json") + page := r.URL.Query().Get("page") + if page == "" || page == "1" { + _ = json.NewEncoder(w).Encode(map[string]any{ + "keys": []any{ + map[string]any{"name": "key-a", "access_key": "AK_A", "created_at": "2026-05-01T00:00:00Z"}, + map[string]any{"name": "key-b", "access_key": "AK_B", "created_at": "2026-05-02T00:00:00Z"}, + }, + "links": map[string]any{ + "pages": map[string]any{"next": srv.URL + "/v2/spaces/keys?page=2"}, + }, + }) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "keys": []any{ + map[string]any{"name": "key-c", "access_key": "AK_C", "created_at": "2026-05-03T00:00:00Z"}, + }, + }) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + })) + defer srv.Close() + + p := newDOProviderForTest(t, srv.URL) + enumerator, ok := interfaces.IaCProvider(p).(interfaces.EnumeratorAll) + if !ok { + t.Fatalf("Provider must implement EnumeratorAll") + } + + outs, err := enumerator.EnumerateAll(context.Background(), "infra.spaces_key") + if err != nil { + t.Fatalf("EnumerateAll: %v", err) + } + if len(outs) != 3 { + t.Fatalf("expected 3 keys (paginated), got %d: %+v", len(outs), outs) + } + // Each *ResourceOutput must have Name + Type + ProviderID populated, plus + // the full Outputs map (name, access_key, created_at, ...) so downstream + // filter use cases (wfctl infra audit-keys, prune) don't have to re-read. + for _, o := range outs { + if o.Type != "infra.spaces_key" { + t.Errorf("expected Type=infra.spaces_key, got %q", o.Type) + } + if o.ProviderID == "" { + t.Errorf("ProviderID must be populated (= access_key); got empty for %q", o.Name) + } + if o.Outputs["access_key"] == nil { + t.Errorf("Outputs.access_key must be populated for %q", o.Name) + } + if o.Outputs["created_at"] == nil { + t.Errorf("Outputs.created_at must be populated for %q", o.Name) + } + } +} From 0f2e0623c535d277ef1f7b0759840067c8fb028f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 02:50:57 -0400 Subject: [PATCH 04/12] feat(do): EnumeratorAll for infra.spaces_key + grantsToMaps helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements Task 15 of the spaces-key-iac-resource plan (docs/plans/2026-05-08-spaces-key-iac-resource.md, commit 316559f7). Adds three things to internal/provider.go: 1. (p *DOProvider) EnumerateAll(ctx, resourceType) — implements the interfaces.EnumeratorAll opt-in interface (added in workflow v0.26.0; see iac_provider.go). Switches on resourceType; spaces_key is the only supported type today (other DO resources have native tag enumeration via EnumerateByTag and don't need the all-resources path). Unknown resourceType returns a clear error so audit-keys/prune CLIs surface a precise diagnostic. 2. (p *DOProvider) enumerateAllSpacesKeys(ctx) — paginates GET /v2/spaces/keys via godo SpacesKeys.List using ListOptions{Page, PerPage:200}; loop terminates when godo signals last page (Links.Pages == nil || Pages.Next == ""). Each *ResourceOutput carries name, access_key, created_at, grants so the audit/prune CLIs can filter by age or grant scope without re-reading every key. ProviderID is set to AccessKey to match SpacesKeyDriver.Create's contract (sister driver in internal/drivers/spaces_key.go). 3. grantsToMaps([]*godo.Grant) helper — converts the typed grants slice into []map[string]any so it can live in ResourceOutput.Outputs (which is map[string]any, not a typed slice). Returns nil for empty/nil input. Verification: - GOWORK=off go test ./internal -run "TestProvider_EnumerateAll" -v → TestProvider_EnumerateAll_SpacesKeys (Task 14's failing test) now PASS - GOWORK=off go test ./internal -count=1 → all internal-package tests PASS - GOWORK=off go build ./cmd/plugin → produces a Mach-O binary (994KB). Full `wfctl plugin list | grep infra.spaces_key` smoke depends on Task 12's driver registration in DOProvider.drivers + Capabilities() entry — that lands separately in implementer-3's Task 12 commit. Adaptations from plan: - Plan code uses `(p *Provider)` receiver; actual type is `*DOProvider`. - Added p.client nil guard at top of EnumerateAll (mirrors EnumerateByTag's uninitialised-provider check, which is the in-tree pattern). - Added k != nil and g != nil defensive checks in the loops. Per Task 15 Step 6, after PR4b merges: tag DO plugin v0.14.0 and push. That step happens post-merge, not in this commit. Co-Authored-By: Claude Opus 4.7 --- internal/provider.go | 92 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/internal/provider.go b/internal/provider.go index c6be57f..815b019 100644 --- a/internal/provider.go +++ b/internal/provider.go @@ -679,6 +679,98 @@ func (p *DOProvider) RevokeProviderCredential(ctx context.Context, source string return nil } +// EnumerateAll lists every resource of resourceType in the DO account, +// regardless of tag. Used for resource types that don't support tags +// (e.g. spaces_key, where the DO API has no tag surface). Paginates +// transparently using godo.Response.Links so callers don't have to. +// +// Returns []*ResourceOutput per the workflow EnumeratorAll contract +// (interfaces/iac_provider.go) — full metadata is populated so +// downstream callers (wfctl infra audit-keys, prune, rotate-and-prune) +// can filter without re-reading each resource individually. +// +// Implements interfaces.EnumeratorAll (workflow v0.26.0+; opt-in +// interface, type-asserted by the host's IaCProvider proxy). +func (p *DOProvider) EnumerateAll(ctx context.Context, resourceType string) ([]*interfaces.ResourceOutput, error) { + if p.client == nil { + return nil, fmt.Errorf("digitalocean: EnumerateAll called on provider that is not initialized — call Initialize first") + } + switch resourceType { + case "infra.spaces_key": + return p.enumerateAllSpacesKeys(ctx) + default: + return nil, fmt.Errorf("digitalocean: EnumerateAll: resource type %q not supported", resourceType) + } +} + +// enumerateAllSpacesKeys paginates GET /v2/spaces/keys via godo's SpacesKeys.List +// using ListOptions{Page,PerPage:200}; loop terminates when godo signals the +// last page (Links.Pages == nil || Pages.Next == ""). Each *ResourceOutput +// carries the name + access_key + created_at + grants so the audit/prune CLIs +// can filter by age or grant scope without an extra Get per key. +// +// ProviderID is set to AccessKey to match SpacesKeyDriver.Create's contract +// (sister driver in internal/drivers/spaces_key.go) — keeps Read/Delete +// dispatch consistent across the lifecycle. +func (p *DOProvider) enumerateAllSpacesKeys(ctx context.Context) ([]*interfaces.ResourceOutput, error) { + var all []*interfaces.ResourceOutput + opt := &godo.ListOptions{Page: 1, PerPage: 200} + for { + keys, resp, err := p.client.SpacesKeys.List(ctx, opt) + if err != nil { + return nil, fmt.Errorf("digitalocean: EnumerateAll spaces_key: list page=%d: %w", opt.Page, err) + } + for _, k := range keys { + if k == nil { + continue + } + all = append(all, &interfaces.ResourceOutput{ + Name: k.Name, + Type: "infra.spaces_key", + ProviderID: k.AccessKey, // godo identifier — used by Read/Delete + Outputs: map[string]any{ + "name": k.Name, + "access_key": k.AccessKey, + "created_at": k.CreatedAt, + "grants": grantsToMaps(k.Grants), + }, + Sensitive: map[string]bool{"access_key": true}, + Status: "running", + }) + } + if resp == nil || resp.Links == nil || resp.Links.Pages == nil || resp.Links.Pages.Next == "" { + break + } + page, err := resp.Links.CurrentPage() + if err != nil { + // godo couldn't parse the next-page link — bail rather than loop forever. + break + } + opt.Page = page + 1 + } + return all, nil +} + +// grantsToMaps converts a []*godo.Grant to []map[string]any so it can live in +// ResourceOutput.Outputs (which is map[string]any, not a typed grants slice). +// Returns nil for empty/nil input so the Outputs entry is omitted-rather-than-empty. +func grantsToMaps(grants []*godo.Grant) []map[string]any { + if len(grants) == 0 { + return nil + } + out := make([]map[string]any, 0, len(grants)) + for _, g := range grants { + if g == nil { + continue + } + out = append(out, map[string]any{ + "bucket": g.Bucket, + "permission": string(g.Permission), + }) + } + return out +} + // Close is a no-op; the godo client has no persistent connection to close. func (p *DOProvider) Close() error { return nil } From f203b1508d9137b660d1db8c724580d91ec62f17 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 02:59:18 -0400 Subject: [PATCH 05/12] chore(test): hermetic godo client in spaces_key_test helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switches `godoClientForTest` from `http.DefaultClient` to `srv.Client()` and takes `*httptest.Server` directly instead of just the URL string. This matches the sister-code pattern in `internal/provider_enumerator_test.go::newProviderForEnumeratorTest` (line 149: `godo.NewClient(srv.Client())`) — every godo request rides the test server's own transport rather than the package-global default client. Why it matters: another test could mutate `http.DefaultClient.Transport` via `defer`-restore around an httptest swap (the workflow secrets package does exactly this in `generators_test.go`). If that other test panicked mid-swap, our DO plugin tests would inherit a leaked transport. Using `srv.Client()` keeps each test self-contained. Closes Copilot 3212656317 carry-over from Task 11 quality review. No behavior change beyond test-isolation hardening — the `undefined: NewSpacesKeyDriver` failure mode is preserved (Task 12 still makes it pass). Co-Authored-By: Claude Opus 4.7 --- internal/drivers/spaces_key_test.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/internal/drivers/spaces_key_test.go b/internal/drivers/spaces_key_test.go index 0de4eb3..13421db 100644 --- a/internal/drivers/spaces_key_test.go +++ b/internal/drivers/spaces_key_test.go @@ -50,7 +50,7 @@ func TestSpacesKeyDriver_Create_HappyPath(t *testing.T) { })) defer srv.Close() - client := godoClientForTest(t, srv.URL) + client := godoClientForTest(t, srv) fakeGHSecrets := &fakeSecretsProvider{stored: map[string]string{}} driver := NewSpacesKeyDriver(client, fakeGHSecrets) @@ -112,15 +112,18 @@ func TestSpacesKeyDriver_Create_HappyPath(t *testing.T) { } // godoClientForTest builds a godo client whose BaseURL points at the given -// httptest server. Mirrors the pattern in `internal/provider_enumerator_test.go` -// (`newProviderForEnumeratorTest`) — godo uses BaseURL for every request, so -// rewriting it is the standard hermetic test stub. -func godoClientForTest(t *testing.T, serverURL string) *godo.Client { +// httptest server and whose underlying http.Client is the test server's own +// client. Mirrors `internal/provider_enumerator_test.go::newProviderForEnumeratorTest` +// (line 149: `godo.NewClient(srv.Client())`). Using `srv.Client()` rather than +// `http.DefaultClient` keeps the test hermetic — every request rides the +// test server's transport, so an ambient transport mutation in another test +// can't leak in. Standard httptest discipline. +func godoClientForTest(t *testing.T, srv *httptest.Server) *godo.Client { t.Helper() - client := godo.NewClient(http.DefaultClient) - base, err := url.Parse(serverURL + "/") + client := godo.NewClient(srv.Client()) + base, err := url.Parse(srv.URL + "/") if err != nil { - t.Fatalf("parse httptest URL %q: %v", serverURL, err) + t.Fatalf("parse httptest URL %q: %v", srv.URL, err) } client.BaseURL = base return client From 7ad1f0254830c20e96ab62fb847ec4873382d7df Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 03:05:07 -0400 Subject: [PATCH 06/12] chore(do): hermetic test helper + tighter created_at assertion + propagate pagination error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundles three code-reviewer follow-ups on Tasks 14+15 (PR4b) into a single commit: 1. Task 15 (Important / security) — `internal/provider.go`, `enumerateAllSpacesKeys`: replace `break` after a `resp.Links.CurrentPage()` parse error with `return nil, fmt.Errorf(...)`. Silent break would cause prune/audit-keys to miss orphan keys past the failed page boundary, leaving valid credentials live against DO. Sister paginators in `EnumerateByTag` (droplets, volumes, databases) all return-on-error; this matches that contract. 2. Task 14 (Minor / hermeticity) — `internal/provider_test.go`, `newDOProviderForTest`: refactor signature from `(t, serverURL string)` to `(t, srv *httptest.Server)` so the helper can use `srv.Client()` instead of `http.DefaultClient`. Mirrors the sister helper at `provider_enumerator_test.go:149` and `internal/drivers/spaces_key_test.go` (post-f203b15). Hermetic — the server's client trusts its own TLS cert if/when tests move to TLS and never mutates the global http.DefaultClient state. Updated the `TestProvider_EnumerateAll_SpacesKeys` call site accordingly. 3. Task 14 (Minor / structpb safety) — same test, `created_at` and `access_key` assertions: change from `o.Outputs[k] == nil` (which passes for any non-nil interface{} value, including time.Time) to `if got, _ := o.Outputs[k].(string); got == ""`. ResourceOutput.Outputs is map[string]any but the gRPC-side proto roundtrip (structpb) only accepts string/number/bool/list/map values; a regression that stored time.Time would pass a bare nil-check but fail proto marshal. Locking the structpb-safe string shape at the test boundary catches that. Verification: - GOWORK=off go test ./internal -run "TestProvider_EnumerateAll" -v → PASS - GOWORK=off go test ./internal -count=1 → all internal/ tests PASS - GOWORK=off go build ./... → clean Co-Authored-By: Claude Opus 4.7 --- internal/provider.go | 9 +++++++-- internal/provider_test.go | 39 ++++++++++++++++++++++++--------------- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/internal/provider.go b/internal/provider.go index 815b019..549e985 100644 --- a/internal/provider.go +++ b/internal/provider.go @@ -743,8 +743,13 @@ func (p *DOProvider) enumerateAllSpacesKeys(ctx context.Context) ([]*interfaces. } page, err := resp.Links.CurrentPage() if err != nil { - // godo couldn't parse the next-page link — bail rather than loop forever. - break + // Propagate the error rather than break-with-partial-result. + // Silent break would cause prune/audit-keys to miss orphan keys + // past the failed page boundary, leaving valid credentials in + // place against DO indefinitely. Sister paginators (droplets, + // volumes, databases) in EnumerateByTag take this same return- + // on-error stance — match that contract. + return nil, fmt.Errorf("digitalocean: EnumerateAll spaces_key: parse next page: %w", err) } opt.Page = page + 1 } diff --git a/internal/provider_test.go b/internal/provider_test.go index aac8ec3..fef5364 100644 --- a/internal/provider_test.go +++ b/internal/provider_test.go @@ -1101,19 +1101,21 @@ func TestDOProvider_Apply_UpsertAllDrivers(t *testing.T) { } // newDOProviderForTest builds a *DOProvider whose godo client points at the -// given httptest server URL. Mirrors newProviderForEnumeratorTest in -// provider_enumerator_test.go but takes a raw URL instead of a typed mock so -// the EnumeratorAll spaces_key test can drive its own paginated handler. +// given httptest server. Mirrors newProviderForEnumeratorTest in +// provider_enumerator_test.go but takes the server (not just a URL) so the +// EnumeratorAll spaces_key test can drive its own paginated handler while +// still using the server's hermetic http client. // -// godo.NewClient + BaseURL override is the standard hermetic pattern used -// elsewhere in this package (see provider_enumerator_test.go:144 and -// internal/drivers/spaces_key_test.go:118). -func newDOProviderForTest(t *testing.T, serverURL string) *DOProvider { +// Why srv.Client() and not http.DefaultClient: matches the sister helpers +// at provider_enumerator_test.go:149 and internal/drivers/spaces_key_test.go +// (post-f203b15). srv.Client() trusts the server's TLS cert if/when the +// test moves to TLS, and never mutates the global http.DefaultClient state. +func newDOProviderForTest(t *testing.T, srv *httptest.Server) *DOProvider { t.Helper() - client := godo.NewClient(http.DefaultClient) - base, err := url.Parse(serverURL + "/") + client := godo.NewClient(srv.Client()) + base, err := url.Parse(srv.URL + "/") if err != nil { - t.Fatalf("parse httptest URL %q: %v", serverURL, err) + t.Fatalf("parse httptest URL %q: %v", srv.URL, err) } client.BaseURL = base return &DOProvider{client: client, region: "nyc3"} @@ -1171,7 +1173,7 @@ func TestProvider_EnumerateAll_SpacesKeys(t *testing.T) { })) defer srv.Close() - p := newDOProviderForTest(t, srv.URL) + p := newDOProviderForTest(t, srv) enumerator, ok := interfaces.IaCProvider(p).(interfaces.EnumeratorAll) if !ok { t.Fatalf("Provider must implement EnumeratorAll") @@ -1187,6 +1189,13 @@ func TestProvider_EnumerateAll_SpacesKeys(t *testing.T) { // Each *ResourceOutput must have Name + Type + ProviderID populated, plus // the full Outputs map (name, access_key, created_at, ...) so downstream // filter use cases (wfctl infra audit-keys, prune) don't have to re-read. + // + // access_key + created_at are asserted as non-empty STRINGS rather than + // just non-nil interface{}: ResourceOutput.Outputs is map[string]any but + // the gRPC-side proto roundtrip (structpb) only accepts string/number/ + // bool/list/map values. A regression that stored time.Time would pass a + // bare nil-check but fail the proto marshal — type-assert to string here + // to lock the structpb-safe shape. for _, o := range outs { if o.Type != "infra.spaces_key" { t.Errorf("expected Type=infra.spaces_key, got %q", o.Type) @@ -1194,11 +1203,11 @@ func TestProvider_EnumerateAll_SpacesKeys(t *testing.T) { if o.ProviderID == "" { t.Errorf("ProviderID must be populated (= access_key); got empty for %q", o.Name) } - if o.Outputs["access_key"] == nil { - t.Errorf("Outputs.access_key must be populated for %q", o.Name) + if got, _ := o.Outputs["access_key"].(string); got == "" { + t.Errorf("Outputs.access_key must be populated as non-empty string for %q; got %T %v", o.Name, o.Outputs["access_key"], o.Outputs["access_key"]) } - if o.Outputs["created_at"] == nil { - t.Errorf("Outputs.created_at must be populated for %q", o.Name) + if got, _ := o.Outputs["created_at"].(string); got == "" { + t.Errorf("Outputs.created_at must be populated as non-empty string for %q; got %T %v", o.Name, o.Outputs["created_at"], o.Outputs["created_at"]) } } } From c620f658bced5f99a0c1f964fe50091813669cde Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 13:57:01 -0400 Subject: [PATCH 07/12] chore(deps): bump workflow to v0.27.0 (engine-side sensitive routing) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit workflow v0.27.0 ships the iac/sensitive package which routes ResourceOutput.Sensitive[k]==true fields through the engine's configured secrets.Provider. This unblocks the post-pivot SpacesKeyDriver landing — drivers no longer take a secrets.Provider parameter; the engine handles routing per-call based on the Sensitive map. Per ADR 0015/0020 and workflow PR #588 (commit ffae1409). --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 2364fe8..5dbf73e 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/GoCodeAlone/workflow-plugin-digitalocean go 1.26.0 require ( - github.com/GoCodeAlone/workflow v0.26.0 + github.com/GoCodeAlone/workflow v0.27.0 github.com/aws/aws-sdk-go-v2 v1.41.6 github.com/aws/aws-sdk-go-v2/credentials v1.19.15 github.com/aws/aws-sdk-go-v2/service/s3 v1.97.2 diff --git a/go.sum b/go.sum index 9b380d3..d2b571a 100644 --- a/go.sum +++ b/go.sum @@ -56,8 +56,8 @@ github.com/GoCodeAlone/modular/modules/jsonschema v1.15.0 h1:xb1mI4NZkzvNKQ2F6nk github.com/GoCodeAlone/modular/modules/jsonschema v1.15.0/go.mod h1:hhGouwAVsonmJ4Lain4jINZ9nZCoc9l9eF3BHbmR8eE= github.com/GoCodeAlone/modular/modules/reverseproxy/v2 v2.8.0 h1:cvdLHbM/vzvygQTcAWSJsy+dAPzzwWyjzKMmTBFcFIo= github.com/GoCodeAlone/modular/modules/reverseproxy/v2 v2.8.0/go.mod h1:/9ipMG4qM2CHQ14BfXKdVlYRJelef6M8MFI5TbZv67M= -github.com/GoCodeAlone/workflow v0.26.0 h1:IiDb92K7o3pP7g4p9JSEHUY6h9Slj1hkgC6IQM8IjPg= -github.com/GoCodeAlone/workflow v0.26.0/go.mod h1:Ue+5YDScTZgtA36q6r/kDaIRxGJFkyxXbeyJVNVJ0Cc= +github.com/GoCodeAlone/workflow v0.27.0 h1:oufPjwWTuwbZw5PckBEDrIah+w7JJcj51nKQzLe5io0= +github.com/GoCodeAlone/workflow v0.27.0/go.mod h1:Ue+5YDScTZgtA36q6r/kDaIRxGJFkyxXbeyJVNVJ0Cc= github.com/GoCodeAlone/yaegi v0.17.2 h1:WK6Y6e0t1a6U7r+S2dN3CGWW1PizYD3zO0zneToZPxM= github.com/GoCodeAlone/yaegi v0.17.2/go.mod h1:z5Pr6Wse6QJcQvpgxTxzMAevFarH0N37TG88Y9dprx0= github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.32.0 h1:rIkQfkCOVKc1OiRCNcSDD8ml5RJlZbH/Xsq7lbpynwc= From fd5a1b0114ebbcdfa92e89218ea18034d3b41629 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 13:57:14 -0400 Subject: [PATCH 08/12] refactor(spaces_key): pivot test to engine-routing pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-pivot: driver took a secrets.Provider parameter and called Set() itself; test asserted secret_key was NOT in Outputs and that fakeSecretsProvider.stored received both sub-keys. Post-pivot (workflow v0.27.0): driver returns Outputs containing every field including secret_key, with Sensitive[access_key|secret_key]=true. The engine's iac/sensitive.Route routes flagged fields through the configured secrets.Provider before persisting state — the driver itself is platform-agnostic. Test changes: - Switch package drivers -> drivers_test (black-box, addresses Copilot #2) - Drop fakeSecretsProvider + secrets import (no longer used) - NewSpacesKeyDriver(client) — single-arg constructor - Validate request payload reaching DO (name + grants), addresses Copilot #4 - Return 500 on unexpected request path (deterministic failure surface), addresses Copilot #5 - Assert Outputs.secret_key == "SK_secret_data_here" (driver returns it, engine routes after) - Assert Sensitive[access_key]=true, Sensitive[secret_key]=true, Sensitive[created_at] unset - Assert SensitiveKeys() includes both access_key + secret_key - Updated header comment to reflect engine-routing per ADR 0015/0020 --- internal/drivers/spaces_key_test.go | 150 +++++++++++++--------------- 1 file changed, 71 insertions(+), 79 deletions(-) diff --git a/internal/drivers/spaces_key_test.go b/internal/drivers/spaces_key_test.go index 13421db..e991abb 100644 --- a/internal/drivers/spaces_key_test.go +++ b/internal/drivers/spaces_key_test.go @@ -1,4 +1,4 @@ -package drivers +package drivers_test import ( "context" @@ -6,34 +6,56 @@ import ( "net/http" "net/http/httptest" "net/url" + "reflect" "testing" "github.com/GoCodeAlone/workflow/interfaces" - "github.com/GoCodeAlone/workflow/secrets" "github.com/digitalocean/godo" + + "github.com/GoCodeAlone/workflow-plugin-digitalocean/internal/drivers" ) -// TestSpacesKeyDriver_Create_HappyPath is the contract test for -// SpacesKeyDriver.Create. It pins the split-storage pattern that is the whole -// point of the driver: +// TestSpacesKeyDriver_Create_HappyPath pins the engine-routing contract for +// SpacesKeyDriver.Create per workflow v0.27.0 + ADR 0015/0020: // // - ResourceOutput.ProviderID == access_key (the godo identifier; matches -// the sister bucket driver in spaces.go). -// - State outputs contain the metadata needed for downstream resources -// (name, access_key, created_at) but NEVER secret_key — the secret -// belongs in the secrets provider, not in IaC state files. -// - The provided secrets.Provider receives BOTH the access_key and -// secret_key as `_access_key` / `_secret_key` -// so consumers (apps, runners) can read them by canonical name. -// - SensitiveKeys() includes "access_key" so wfctl masks it in plan/diff -// output. secret_key isn't in Outputs so doesn't need masking there. +// the sister bucket driver in spaces.go and the EnumerateAll path in +// internal/provider.go::enumerateAllSpacesKeys). +// - Outputs contains EVERY field including access_key AND secret_key — +// the workflow engine's iac/sensitive package routes them through the +// configured secrets.Provider before persisting state. The driver is +// platform-agnostic: it does NOT touch secrets.Provider itself. +// - Sensitive["access_key"]==true and Sensitive["secret_key"]==true — +// these flags drive the engine's per-call routing. +// - Sensitive["created_at"] is unset/false — created_at is metadata, not +// a secret. +// - SensitiveKeys() returns both "access_key" and "secret_key" — used by +// wfctl plan/diff display masking, separate from per-call routing. // -// This test is the failing-side of the Task 11/12 TDD pair (PR4b). Until -// Task 12 lands NewSpacesKeyDriver in spaces_key.go, the file fails to -// compile with `undefined: NewSpacesKeyDriver`. Per ADR 0020. +// The test also validates the request payload reaching DO so the spec→godo +// translation (name + grants) is pinned, and returns 500 on any unexpected +// path so godo-level errors surface deterministically. func TestSpacesKeyDriver_Create_HappyPath(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/v2/spaces/keys" && r.Method == http.MethodPost { + // Validate the request payload — the driver must translate + // spec.Config{name, grants} into the godo.SpacesKeyCreateRequest + // faithfully. Without this assertion, the test would still pass + // even if Create dropped/garbled the spec. + var got godo.SpacesKeyCreateRequest + if err := json.NewDecoder(r.Body).Decode(&got); err != nil { + http.Error(w, "decode body: "+err.Error(), http.StatusBadRequest) + return + } + if got.Name != "test-key" { + http.Error(w, "request name="+got.Name+" want test-key", http.StatusBadRequest) + return + } + if len(got.Grants) != 1 || got.Grants[0] == nil || + got.Grants[0].Permission != godo.SpacesKeyFullAccess { + http.Error(w, "request grants mismatch", http.StatusBadRequest) + return + } w.WriteHeader(http.StatusCreated) _ = json.NewEncoder(w).Encode(map[string]any{ "key": map[string]any{ @@ -46,14 +68,17 @@ func TestSpacesKeyDriver_Create_HappyPath(t *testing.T) { }) return } + // Unexpected path — return 500 so godo surfaces it as an HTTP error + // rather than the test silently waiting for a response that never + // matches. t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) })) defer srv.Close() client := godoClientForTest(t, srv) - fakeGHSecrets := &fakeSecretsProvider{stored: map[string]string{}} - driver := NewSpacesKeyDriver(client, fakeGHSecrets) + driver := drivers.NewSpacesKeyDriver(client) spec := interfaces.ResourceSpec{ Name: "test-key", Type: "infra.spaces_key", @@ -68,46 +93,50 @@ func TestSpacesKeyDriver_Create_HappyPath(t *testing.T) { t.Fatalf("Create: %v", err) } - // ResourceOutput per workflow interfaces/iac_resource_driver.go. // ProviderID == access_key (the godo identifier; matches sister bucket // driver pattern at internal/drivers/spaces.go). if out.ProviderID != "AKIATEST123" { t.Errorf("ProviderID: want AKIATEST123 (access_key as godo ID), got %q", out.ProviderID) } - // State outputs must contain {name, access_key, created_at}, NOT secret_key. + // All four output fields populated — the engine routes Sensitive ones + // through secrets.Provider after this method returns. if got, _ := out.Outputs["access_key"].(string); got != "AKIATEST123" { t.Errorf("access_key: want AKIATEST123, got %v", out.Outputs["access_key"]) } + if got, _ := out.Outputs["secret_key"].(string); got != "SK_secret_data_here" { + t.Errorf("secret_key: want SK_secret_data_here (engine routes after return), got %v", out.Outputs["secret_key"]) + } if got, _ := out.Outputs["created_at"].(string); got != "2026-05-08T11:00:00Z" { t.Errorf("created_at: want 2026-05-08T11:00:00Z, got %v", out.Outputs["created_at"]) } - if _, ok := out.Outputs["secret_key"]; ok { - t.Errorf("secret_key MUST NOT be in state outputs (must live in GH Secrets only)") + if _, ok := out.Outputs["name"]; !ok { + t.Errorf("name MUST be in Outputs; got %v", out.Outputs) } - // Both sub-keys must reach the secrets.Provider under the canonical - // `_access_key` / `_secret_key` names. - if got := fakeGHSecrets.stored["test-key_access_key"]; got != "AKIATEST123" { - t.Errorf("GH Secret test-key_access_key: want AKIATEST123, got %q", got) + // Sensitive flags MUST mark both access_key and secret_key — the + // engine's iac/sensitive.Route consults this map per-call to decide + // what to push through secrets.Provider. + if !out.Sensitive["access_key"] { + t.Errorf("Sensitive[access_key]: want true, got %v", out.Sensitive["access_key"]) + } + if !out.Sensitive["secret_key"] { + t.Errorf("Sensitive[secret_key]: want true, got %v", out.Sensitive["secret_key"]) } - if got := fakeGHSecrets.stored["test-key_secret_key"]; got != "SK_secret_data_here" { - t.Errorf("GH Secret test-key_secret_key: want SK_secret_data_here, got %q", got) + // created_at is metadata, not sensitive — must NOT be flagged. + if out.Sensitive["created_at"] { + t.Errorf("Sensitive[created_at]: want false/unset, got true") } - // SensitiveKeys returns access_key (so wfctl masks it in logs). - // secret_key is NOT in Outputs, so doesn't need state-side masking; - // the secrets.Provider handles its sensitivity separately. - sensitive := driver.SensitiveKeys() - found := false - for _, k := range sensitive { - if k == "access_key" { - found = true - break - } + // SensitiveKeys (display masking) must include both keys — distinct + // from the per-call Sensitive map but parallel in content here. + wantKeys := map[string]bool{"access_key": true, "secret_key": true} + gotKeys := map[string]bool{} + for _, k := range driver.SensitiveKeys() { + gotKeys[k] = true } - if !found { - t.Errorf("SensitiveKeys must include access_key; got %v", sensitive) + if !reflect.DeepEqual(gotKeys, wantKeys) { + t.Errorf("SensitiveKeys: want %v, got %v", wantKeys, gotKeys) } } @@ -117,7 +146,7 @@ func TestSpacesKeyDriver_Create_HappyPath(t *testing.T) { // (line 149: `godo.NewClient(srv.Client())`). Using `srv.Client()` rather than // `http.DefaultClient` keeps the test hermetic — every request rides the // test server's transport, so an ambient transport mutation in another test -// can't leak in. Standard httptest discipline. +// can't leak in. func godoClientForTest(t *testing.T, srv *httptest.Server) *godo.Client { t.Helper() client := godo.NewClient(srv.Client()) @@ -128,40 +157,3 @@ func godoClientForTest(t *testing.T, srv *httptest.Server) *godo.Client { client.BaseURL = base return client } - -// fakeSecretsProvider is a minimal in-memory secrets.Provider that records -// every Set so tests can assert which sub-keys were stored. Get/Delete/List -// implement the interface contract; the Spaces key driver only uses Set. -type fakeSecretsProvider struct { - stored map[string]string -} - -func (f *fakeSecretsProvider) Name() string { return "fake" } - -func (f *fakeSecretsProvider) Get(_ context.Context, key string) (string, error) { - if v, ok := f.stored[key]; ok { - return v, nil - } - return "", secrets.ErrNotFound -} - -func (f *fakeSecretsProvider) Set(_ context.Context, key, value string) error { - if f.stored == nil { - f.stored = map[string]string{} - } - f.stored[key] = value - return nil -} - -func (f *fakeSecretsProvider) Delete(_ context.Context, key string) error { - delete(f.stored, key) - return nil -} - -func (f *fakeSecretsProvider) List(_ context.Context) ([]string, error) { - keys := make([]string, 0, len(f.stored)) - for k := range f.stored { - keys = append(keys, k) - } - return keys, nil -} From 108108fb7dbcb059da95bb277b78a4fa8e1da8d6 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 13:57:35 -0400 Subject: [PATCH 09/12] feat(spaces_key): implement driver + register (PR4b Tasks 12+13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit internal/drivers/spaces_key.go — full ResourceDriver implementation per the engine-routing contract (workflow v0.27.0): - Create: builds godo.SpacesKeyCreateRequest from spec.Config, returns ResourceOutput with all fields populated (name, access_key, secret_key, created_at, grants) and Sensitive[access_key|secret_key]=true so the engine's iac/sensitive.Route handles routing through secrets.Provider per ADR 0015. - Read: refreshes via godo.SpacesKeys.Get(accessKey); falls back to enumerate-and-match-by-name when ProviderID is empty (upsert recovery path). secret_key is omitted — DO API only returns it on Create; the cached value lives in the engine's secrets.Provider. - Update: in-place rename + grant updates via godo.SpacesKeys.Update. - Delete: idempotent (404 -> success); matches RevokeProviderCredential stance for sister Spaces credentials. - Diff: compares mutable fields (name + grants); name divergence is an in-place Update for Spaces keys (no NeedsReplace). - HealthCheck: report healthy iff Get succeeds. - Scale: not supported (access keys have no replica concept). - SupportsUpsert: true (Read can locate by name when ProviderID empty). - ProviderIDFormat: IDFormatFreeform (DO access keys are not UUIDs). - SensitiveKeys: returns both access_key + secret_key for plan/diff display masking. internal/provider.go: - Register infra.spaces_key driver with NewSpacesKeyDriver(p.client) (single-arg, plugin-agnostic — the engine handles secrets routing). - Add infra.spaces_key to Capabilities (Tier 1, noScale ops). - enumerateAllSpacesKeys: only set Outputs["grants"] when non-empty so the docstring matches reality (Copilot #7). --- internal/drivers/spaces_key.go | 386 +++++++++++++++++++++++++++++++++ internal/provider.go | 25 ++- 2 files changed, 403 insertions(+), 8 deletions(-) create mode 100644 internal/drivers/spaces_key.go diff --git a/internal/drivers/spaces_key.go b/internal/drivers/spaces_key.go new file mode 100644 index 0000000..e9fc3bd --- /dev/null +++ b/internal/drivers/spaces_key.go @@ -0,0 +1,386 @@ +// Package drivers — SpacesKeyDriver provisions DigitalOcean Spaces access +// keys (resource type `infra.spaces_key`) via godo's SpacesKeysService. +// +// Engine-routing pattern (workflow v0.27.0+): +// +// - Create returns ResourceOutput.Outputs containing every field including +// access_key and secret_key, and flags both as Sensitive via +// ResourceOutput.Sensitive[k]==true. The workflow engine's iac/sensitive +// package routes those flagged values through the configured +// secrets.Provider (keyed `_`) and replaces them with +// `secret_ref://...` placeholders before persisting state. The driver +// itself NEVER touches secrets.Provider — it stays platform-agnostic. +// +// - Read returns Outputs WITHOUT secret_key (the DO API doesn't return it +// on List/Get; the secret value lives in the secrets.Provider after +// Create routed it). Audit/refresh paths reconcile against access_key, +// name, created_at, grants — the only fields the API can re-read. +// +// - Diff compares mutable fields (grants). Spaces keys can't be renamed +// server-side, so name divergence sets NeedsReplace=true. +// +// - SensitiveKeys() returns access_key + secret_key for plan/diff display +// masking; this is independent from the per-call routing trigger above. +// +// ProviderID == access_key — matches the sister bucket driver +// (internal/drivers/spaces.go) and the EnumerateAll path in +// internal/provider.go::enumerateAllSpacesKeys. Per ADR 0015/0020. +package drivers + +import ( + "context" + "errors" + "fmt" + "reflect" + + "github.com/GoCodeAlone/workflow/interfaces" + "github.com/digitalocean/godo" +) + +// spacesKeysAPI is the subset of godo.SpacesKeysService that this driver +// uses. Extracted as an interface so tests can inject a fake without +// constructing a full *godo.Client. +type spacesKeysAPI interface { + Create(context.Context, *godo.SpacesKeyCreateRequest) (*godo.SpacesKey, *godo.Response, error) + Get(context.Context, string) (*godo.SpacesKey, *godo.Response, error) + List(context.Context, *godo.ListOptions) ([]*godo.SpacesKey, *godo.Response, error) + Update(context.Context, string, *godo.SpacesKeyUpdateRequest) (*godo.SpacesKey, *godo.Response, error) + Delete(context.Context, string) (*godo.Response, error) +} + +// SpacesKeyDriver manages DigitalOcean Spaces access keys (infra.spaces_key). +// Constructed with a *godo.Client only — the secrets.Provider is the +// engine's concern under the v0.27.0 engine-side routing contract. +type SpacesKeyDriver struct { + client spacesKeysAPI +} + +// NewSpacesKeyDriver returns a driver bound to the given godo client. +func NewSpacesKeyDriver(client *godo.Client) *SpacesKeyDriver { + return &SpacesKeyDriver{client: client.SpacesKeys} +} + +// ResourceType is the canonical resource-type string this driver serves. +func (d *SpacesKeyDriver) ResourceType() string { return "infra.spaces_key" } + +// SensitiveKeys returns the output keys whose values should be masked in +// plan/diff display. The engine ALSO consults ResourceOutput.Sensitive +// per-call to decide routing through secrets.Provider; SensitiveKeys is +// strictly the display-side signal. +func (d *SpacesKeyDriver) SensitiveKeys() []string { + return []string{"access_key", "secret_key"} +} + +// ProviderIDFormat declares ProviderIDs are freeform (DO access keys are +// not UUIDs). Disables UUID validation while still enforcing non-empty. +func (d *SpacesKeyDriver) ProviderIDFormat() interfaces.ProviderIDFormat { + return interfaces.IDFormatFreeform +} + +// Create provisions a new Spaces key and returns its full outputs flagged +// for engine-side sensitive routing. +func (d *SpacesKeyDriver) Create(ctx context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + req, err := spacesKeyCreateRequest(spec) + if err != nil { + return nil, fmt.Errorf("spaces_key create %q: %w", spec.Name, err) + } + key, _, err := d.client.Create(ctx, req) + if err != nil { + return nil, fmt.Errorf("spaces_key create %q: %w", spec.Name, WrapGodoError(err)) + } + if key == nil { + return nil, fmt.Errorf("spaces_key create %q: godo returned nil key", spec.Name) + } + return spacesKeyOutput(key, /*includeSecret*/ true), nil +} + +// Read refreshes metadata for an existing Spaces key. secret_key is NOT +// returned (the DO API only exposes it on Create); the cached value lives +// in the secrets.Provider after Create routed it. +func (d *SpacesKeyDriver) Read(ctx context.Context, ref interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { + if ref.ProviderID == "" { + // Name-only fallback for upsert / state-heal: enumerate and match + // by name. Spaces keys have no native name lookup. + key, err := d.findByName(ctx, ref.Name) + if err != nil { + return nil, fmt.Errorf("spaces_key read %q: %w", ref.Name, err) + } + return spacesKeyOutput(key, /*includeSecret*/ false), nil + } + key, _, err := d.client.Get(ctx, ref.ProviderID) + if err != nil { + return nil, fmt.Errorf("spaces_key read %q: %w", ref.Name, WrapGodoError(err)) + } + if key == nil { + return nil, fmt.Errorf("spaces_key read %q: godo returned nil key", ref.Name) + } + return spacesKeyOutput(key, /*includeSecret*/ false), nil +} + +// SupportsUpsert advertises that Read can locate by Name when ProviderID +// is empty, so wfctl ApplyPlan can recover from ErrResourceAlreadyExists. +func (d *SpacesKeyDriver) SupportsUpsert() bool { return true } + +// Update mutates a Spaces key in place. The DO API only allows changing +// grants and name — name change is treated as in-place rename via +// SpacesKeyUpdateRequest. +func (d *SpacesKeyDriver) Update(ctx context.Context, ref interfaces.ResourceRef, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + grants, err := grantsFromConfig(spec.Config) + if err != nil { + return nil, fmt.Errorf("spaces_key update %q: %w", ref.Name, err) + } + name := strFromConfig(spec.Config, "name", spec.Name) + if ref.ProviderID == "" { + return nil, fmt.Errorf("spaces_key update %q: ProviderID is required", ref.Name) + } + key, _, err := d.client.Update(ctx, ref.ProviderID, &godo.SpacesKeyUpdateRequest{ + Name: name, + Grants: grants, + }) + if err != nil { + return nil, fmt.Errorf("spaces_key update %q: %w", ref.Name, WrapGodoError(err)) + } + if key == nil { + return nil, fmt.Errorf("spaces_key update %q: godo returned nil key", ref.Name) + } + // secret_key is NOT returned by Update — only Create. Mirror Read's shape. + return spacesKeyOutput(key, /*includeSecret*/ false), nil +} + +// Delete removes the Spaces key by access_key. 404 is treated as success +// (idempotent delete) — sister provider RevokeProviderCredential takes +// the same stance. +func (d *SpacesKeyDriver) Delete(ctx context.Context, ref interfaces.ResourceRef) error { + if ref.ProviderID == "" { + return fmt.Errorf("spaces_key delete %q: ProviderID is required", ref.Name) + } + resp, err := d.client.Delete(ctx, ref.ProviderID) + if err != nil { + if resp != nil && resp.StatusCode == 404 { + return nil + } + return fmt.Errorf("spaces_key delete %q: %w", ref.Name, WrapGodoError(err)) + } + return nil +} + +// Diff compares desired spec against current state. Spaces keys support +// in-place rename + grant updates; both go via Update (NeedsUpdate=true), +// no replace path. +func (d *SpacesKeyDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, current *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { + if current == nil { + return &interfaces.DiffResult{NeedsUpdate: true}, nil + } + desiredGrants, err := grantsFromConfig(desired.Config) + if err != nil { + return nil, fmt.Errorf("spaces_key diff %q: %w", desired.Name, err) + } + desiredName := strFromConfig(desired.Config, "name", desired.Name) + curName, _ := current.Outputs["name"].(string) + + var changes []interfaces.FieldChange + if curName != desiredName { + changes = append(changes, interfaces.FieldChange{ + Path: "name", Old: curName, New: desiredName, + }) + } + + desiredGrantsMap := grantsToMaps(desiredGrants) + curGrants := normalizeGrantsForDiff(current.Outputs["grants"]) + if !reflect.DeepEqual(curGrants, desiredGrantsMap) { + changes = append(changes, interfaces.FieldChange{ + Path: "grants", Old: curGrants, New: desiredGrantsMap, + }) + } + return &interfaces.DiffResult{NeedsUpdate: len(changes) > 0, Changes: changes}, nil +} + +// HealthCheck has no provider-side concept for Spaces keys; report healthy +// if the key still exists. Errors here are non-fatal at the call site. +func (d *SpacesKeyDriver) HealthCheck(ctx context.Context, ref interfaces.ResourceRef) (*interfaces.HealthResult, error) { + if ref.ProviderID == "" { + return &interfaces.HealthResult{Healthy: false, Message: "ProviderID empty"}, nil + } + if _, _, err := d.client.Get(ctx, ref.ProviderID); err != nil { + return &interfaces.HealthResult{Healthy: false, Message: err.Error()}, nil + } + return &interfaces.HealthResult{Healthy: true}, nil +} + +// Scale is not supported (access keys have no replica concept). +func (d *SpacesKeyDriver) Scale(_ context.Context, _ interfaces.ResourceRef, _ int) (*interfaces.ResourceOutput, error) { + return nil, errors.New("spaces_key does not support scale operation") +} + +// findByName paginates SpacesKeys.List looking for an entry with the given +// Name. Used by Read when ProviderID is empty (name-only upsert path). +func (d *SpacesKeyDriver) findByName(ctx context.Context, name string) (*godo.SpacesKey, error) { + opt := &godo.ListOptions{Page: 1, PerPage: 200} + for { + keys, resp, err := d.client.List(ctx, opt) + if err != nil { + return nil, fmt.Errorf("list spaces_keys page=%d: %w", opt.Page, WrapGodoError(err)) + } + for _, k := range keys { + if k != nil && k.Name == name { + return k, nil + } + } + if resp == nil || resp.Links == nil || resp.Links.Pages == nil || resp.Links.Pages.Next == "" { + break + } + page, err := resp.Links.CurrentPage() + if err != nil { + return nil, fmt.Errorf("list spaces_keys: parse next page: %w", err) + } + opt.Page = page + 1 + } + return nil, fmt.Errorf("%w: spaces_key %q", interfaces.ErrResourceNotFound, name) +} + +// spacesKeyCreateRequest builds godo.SpacesKeyCreateRequest from spec.Config. +func spacesKeyCreateRequest(spec interfaces.ResourceSpec) (*godo.SpacesKeyCreateRequest, error) { + name := strFromConfig(spec.Config, "name", spec.Name) + if name == "" { + return nil, fmt.Errorf("spec.Config[\"name\"] (or spec.Name) is required") + } + grants, err := grantsFromConfig(spec.Config) + if err != nil { + return nil, err + } + return &godo.SpacesKeyCreateRequest{Name: name, Grants: grants}, nil +} + +// grantsFromConfig parses spec.Config["grants"] into []*godo.Grant. Accepts +// both raw map shape (from YAML/JSON) and pre-typed []*godo.Grant. +// +// Each grant entry must carry a non-empty `permission`; `bucket` is +// optional (empty bucket means "all buckets" — DO's account-wide grant). +func grantsFromConfig(config map[string]any) ([]*godo.Grant, error) { + raw, ok := config["grants"] + if !ok || raw == nil { + return nil, nil + } + switch v := raw.(type) { + case []*godo.Grant: + return v, nil + case []any: + out := make([]*godo.Grant, 0, len(v)) + for i, item := range v { + m, ok := item.(map[string]any) + if !ok { + return nil, fmt.Errorf("grants[%d]: expected object, got %T", i, item) + } + perm, _ := m["permission"].(string) + if perm == "" { + return nil, fmt.Errorf("grants[%d]: permission is required", i) + } + bucket, _ := m["bucket"].(string) + out = append(out, &godo.Grant{ + Bucket: bucket, + Permission: godo.SpacesKeyPermission(perm), + }) + } + return out, nil + default: + return nil, fmt.Errorf("grants: expected array, got %T", raw) + } +} + +// spacesKeyOutput renders the standard ResourceOutput for a *godo.SpacesKey. +// includeSecret controls whether secret_key is populated (true on Create, +// false on Read/Update where the API doesn't return it). +func spacesKeyOutput(k *godo.SpacesKey, includeSecret bool) *interfaces.ResourceOutput { + outputs := map[string]any{ + "name": k.Name, + "access_key": k.AccessKey, + "created_at": k.CreatedAt, + "grants": grantsToMaps(k.Grants), + } + sensitive := map[string]bool{ + "access_key": true, + } + if includeSecret { + outputs["secret_key"] = k.SecretKey + sensitive["secret_key"] = true + } + return &interfaces.ResourceOutput{ + Name: k.Name, + Type: "infra.spaces_key", + ProviderID: k.AccessKey, + Outputs: outputs, + Sensitive: sensitive, + Status: "active", + } +} + +// normalizeGrantsForDiff coerces a stored Outputs["grants"] value (which +// can round-trip as []map[string]any in-process or []any of map[string]any +// after JSON decode) to the canonical []map[string]any shape produced by +// grantsToMaps. Returns nil when the input is nil or empty so DeepEqual +// against grantsToMaps's nil-on-empty result succeeds symmetrically. +func normalizeGrantsForDiff(v any) []map[string]any { + switch t := v.(type) { + case nil: + return nil + case []map[string]any: + if len(t) == 0 { + return nil + } + return t + case []any: + if len(t) == 0 { + return nil + } + out := make([]map[string]any, 0, len(t)) + for _, item := range t { + if m, ok := item.(map[string]any); ok { + out = append(out, map[string]any{ + "bucket": stringOr(m["bucket"], ""), + "permission": stringOr(m["permission"], ""), + }) + } + } + if len(out) == 0 { + return nil + } + return out + } + return nil +} + +// stringOr coerces v to string or returns def. Used in +// normalizeGrantsForDiff so JSON-round-tripped state values match the +// canonical map shape produced by grantsToMaps. +func stringOr(v any, def string) string { + if s, ok := v.(string); ok { + return s + } + return def +} + +// grantsToMaps converts a []*godo.Grant to []map[string]any so it can live +// in ResourceOutput.Outputs (which is map[string]any, not a typed grants +// slice). Returns nil for empty/nil input so the Outputs entry is +// omitted-rather-than-empty. Mirrors the helper of the same name in +// internal/provider.go (kept package-local here to avoid a tight coupling +// from drivers up to the parent internal package). +func grantsToMaps(grants []*godo.Grant) []map[string]any { + if len(grants) == 0 { + return nil + } + out := make([]map[string]any, 0, len(grants)) + for _, g := range grants { + if g == nil { + continue + } + out = append(out, map[string]any{ + "bucket": g.Bucket, + "permission": string(g.Permission), + }) + } + if len(out) == 0 { + return nil + } + return out +} diff --git a/internal/provider.go b/internal/provider.go index 549e985..901aecf 100644 --- a/internal/provider.go +++ b/internal/provider.go @@ -99,6 +99,7 @@ func (p *DOProvider) Initialize(ctx context.Context, config map[string]any) erro "infra.firewall": drivers.NewFirewallDriver(p.client), "infra.dns": drivers.NewDNSDriver(p.client), "infra.storage": drivers.NewSpacesDriver(p.client, p.region, spacesAccessKey, spacesSecretKey), + "infra.spaces_key": drivers.NewSpacesKeyDriver(p.client), "infra.registry": drivers.NewRegistryDriver(p.client), "infra.certificate": drivers.NewCertificateDriver(p.client), "infra.droplet": drivers.NewDropletDriver(p.client, p.region), @@ -123,6 +124,7 @@ func (p *DOProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { {ResourceType: "infra.firewall", Tier: 1, Operations: ops}, {ResourceType: "infra.dns", Tier: 1, Operations: ops}, {ResourceType: "infra.storage", Tier: 1, Operations: ops}, + {ResourceType: "infra.spaces_key", Tier: 1, Operations: noScale}, {ResourceType: "infra.registry", Tier: 2, Operations: ops}, {ResourceType: "infra.certificate", Tier: 1, Operations: ops}, {ResourceType: "infra.droplet", Tier: 1, Operations: ops}, @@ -724,18 +726,25 @@ func (p *DOProvider) enumerateAllSpacesKeys(ctx context.Context) ([]*interfaces. if k == nil { continue } + outputs := map[string]any{ + "name": k.Name, + "access_key": k.AccessKey, + "created_at": k.CreatedAt, + } + // grantsToMaps returns nil for empty/nil input — omit the key + // entirely in that case so the docstring matches reality and + // downstream consumers can use map-presence as the "has any + // grants" signal. + if g := grantsToMaps(k.Grants); g != nil { + outputs["grants"] = g + } all = append(all, &interfaces.ResourceOutput{ Name: k.Name, Type: "infra.spaces_key", ProviderID: k.AccessKey, // godo identifier — used by Read/Delete - Outputs: map[string]any{ - "name": k.Name, - "access_key": k.AccessKey, - "created_at": k.CreatedAt, - "grants": grantsToMaps(k.Grants), - }, - Sensitive: map[string]bool{"access_key": true}, - Status: "running", + Outputs: outputs, + Sensitive: map[string]bool{"access_key": true}, + Status: "running", }) } if resp == nil || resp.Links == nil || resp.Links.Pages == nil || resp.Links.Pages.Next == "" { From ee2896754f8444f3668f662c44e85ce3094f3e42 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 13:57:40 -0400 Subject: [PATCH 10/12] docs(test): update stale Task 14/15 comment in EnumerateAll test The "Until Task 15 lands ... this test fails" preamble is stale once the EnumerateAll method already lands on *DOProvider in this branch. Replace with a forward-looking pin description (Copilot #9). --- internal/provider_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/provider_test.go b/internal/provider_test.go index fef5364..6541d26 100644 --- a/internal/provider_test.go +++ b/internal/provider_test.go @@ -1141,9 +1141,10 @@ func newDOProviderForTest(t *testing.T, srv *httptest.Server) *DOProvider { // Outputs.access_key + Outputs.created_at populated so audit-keys / prune // can filter by age without re-reading every key. // -// Until Task 15 lands the EnumerateAll method on *DOProvider, this test -// fails with `Provider must implement EnumeratorAll` — which is the -// failing-side signal Task 14 is supposed to produce. +// Pins the contract post Tasks 14+15: DOProvider implements EnumeratorAll +// and enumerateAllSpacesKeys returns the full paginated set with the same +// ProviderID convention (access_key) and Outputs shape that +// SpacesKeyDriver.Read produces. func TestProvider_EnumerateAll_SpacesKeys(t *testing.T) { var srv *httptest.Server srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 6861817d626452f8236f066413a3db8e3013052d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 14:10:26 -0400 Subject: [PATCH 11/12] fix(spaces_key): address Copilot round 2 findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five Copilot inline findings on PR #84: #1 (Critical) spaces_key.go package doc claimed name changes force NeedsReplace=true, but godo's SpacesKeysService.Update accepts both Name and Grants — the impl was correct (in-place rename via Update), the doc was wrong. Updated doc to match the impl. #2 (Critical) provider_test.go TestProvider_EnumerateAll_SpacesKeys unexpected-request branch only called t.Errorf; godo could observe an empty 200 body and proceed. Added http.Error(...,500) so godo surfaces the failure as an HTTP error. Mirrors the same fix already in internal/drivers/spaces_key_test.go. #3 (Important) enumerateAllSpacesKeys returned Status="running" while SpacesKeyDriver.Create returns Status="active". Spaces keys have no lifecycle status in the godo API; "active" matches Driver.Create AND the sister bucket driver (internal/drivers/spaces.go). Switched enumerateAllSpacesKeys to "active" and added a comment explaining the canonical choice. #4 (Important) spacesKeyOutput unconditionally set Outputs["grants"] to grantsToMaps result (nil when empty). enumerateAllSpacesKeys omits the key entirely when grants is empty. Aligned spacesKeyOutput to the same omit-when-empty convention so both code paths produce identical state-record shapes. #5 (Minor) Tightened package doc claims for Read/Update/Delete: - Read AND Update return Outputs without secret_key (was previously only stating Read; Update also routes through includeSecret=false). - Delete idempotency on 404 stated explicitly in the package doc. go build ./... + go test ./... pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/spaces_key.go | 28 +++++++++++++++++++++------- internal/provider.go | 6 +++++- internal/provider_test.go | 5 +++++ 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/internal/drivers/spaces_key.go b/internal/drivers/spaces_key.go index e9fc3bd..b2714a9 100644 --- a/internal/drivers/spaces_key.go +++ b/internal/drivers/spaces_key.go @@ -11,13 +11,21 @@ // `secret_ref://...` placeholders before persisting state. The driver // itself NEVER touches secrets.Provider — it stays platform-agnostic. // -// - Read returns Outputs WITHOUT secret_key (the DO API doesn't return it -// on List/Get; the secret value lives in the secrets.Provider after -// Create routed it). Audit/refresh paths reconcile against access_key, -// name, created_at, grants — the only fields the API can re-read. +// - Read and Update return Outputs WITHOUT secret_key (the DO API only +// exposes secret_key on Create; Get/List/Update never re-emit it). +// The secret value lives in the secrets.Provider after Create routed +// it. Audit/refresh paths reconcile against access_key, name, +// created_at, grants — the only fields the API can re-read. // -// - Diff compares mutable fields (grants). Spaces keys can't be renamed -// server-side, so name divergence sets NeedsReplace=true. +// - Delete is idempotent on 404: when the underlying DELETE responds +// 404 (key already gone), the driver returns nil so reapply / cleanup +// pipelines don't error on prior partial success. Sister provider +// RevokeProviderCredential takes the same stance. +// +// - Diff compares mutable fields (name, grants). godo's +// SpacesKeysService.Update accepts both Name and Grants, so name +// divergence is a NeedsUpdate (in-place rename), NOT NeedsReplace. +// A FieldChange entry is appended for each differing field. // // - SensitiveKeys() returns access_key + secret_key for plan/diff display // masking; this is independent from the per-call routing trigger above. @@ -295,7 +303,13 @@ func spacesKeyOutput(k *godo.SpacesKey, includeSecret bool) *interfaces.Resource "name": k.Name, "access_key": k.AccessKey, "created_at": k.CreatedAt, - "grants": grantsToMaps(k.Grants), + } + // grantsToMaps returns nil for empty/nil input — omit the key entirely + // in that case so this output shape matches enumerateAllSpacesKeys (in + // internal/provider.go) and downstream consumers can use map-presence + // as the "has any grants" signal symmetrically across both code paths. + if g := grantsToMaps(k.Grants); g != nil { + outputs["grants"] = g } sensitive := map[string]bool{ "access_key": true, diff --git a/internal/provider.go b/internal/provider.go index 901aecf..8c0952b 100644 --- a/internal/provider.go +++ b/internal/provider.go @@ -744,7 +744,11 @@ func (p *DOProvider) enumerateAllSpacesKeys(ctx context.Context) ([]*interfaces. ProviderID: k.AccessKey, // godo identifier — used by Read/Delete Outputs: outputs, Sensitive: map[string]bool{"access_key": true}, - Status: "running", + // Spaces keys don't have a lifecycle status in the godo API; + // use "active" as the canonical "exists and usable" marker + // to match SpacesKeyDriver.Create (internal/drivers/spaces_key.go) + // and the sister bucket driver (internal/drivers/spaces.go). + Status: "active", }) } if resp == nil || resp.Links == nil || resp.Links.Pages == nil || resp.Links.Pages.Next == "" { diff --git a/internal/provider_test.go b/internal/provider_test.go index 6541d26..ff0510f 100644 --- a/internal/provider_test.go +++ b/internal/provider_test.go @@ -1170,7 +1170,12 @@ func TestProvider_EnumerateAll_SpacesKeys(t *testing.T) { }) return } + // Unexpected path — return 500 so godo surfaces it as an HTTP error + // rather than the test silently observing an empty 200 body. Mirrors + // the same hermetic-handler pattern in + // internal/drivers/spaces_key_test.go. t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) })) defer srv.Close() From 4e3f0ed2dfa645be6e1fa3e7586af2a94015a5ec Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 9 May 2026 14:33:21 -0400 Subject: [PATCH 12/12] test(spaces_key): add Read/Update/Delete/Diff/HealthCheck lifecycle coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the regression-detection gap flagged by code-reviewer on PR #84: the existing TestSpacesKeyDriver_Create_HappyPath only pinned Create. For a destructive credential-lifecycle driver, the rest of the ResourceDriver methods needed coverage before merge. 15 new tests, all using stdlib testing + httptest (no testify): Read (4): - Read_HappyPath: GET /v2/spaces/keys/{access_key}; secret_key NOT in Outputs; Sensitive[secret_key] unset (DO API never re-emits secret). - Read_NotFound: 404 → errors.Is(err, ErrResourceNotFound) via WrapGodoError mapping. - Read_FindByName: empty ProviderID drives findByName fallback; paginated List response exercised. - Read_FindByName_NotFound: name-fallback exhaustion → ErrResourceNotFound. Update (2): - Update_RenameInPlace: PUT carries Name+Grants; secret_key absent in response Outputs (Update never re-issues secret). - Update_GrantsChange: grants-only update; request payload + Outputs shape pinned for read/readwrite permissions across two buckets. Delete (3): - Delete_HappyPath: 204 No Content → nil error. - Delete_Idempotent_404: 404 → nil (matches RevokeProviderCredential). - Delete_OtherError_Propagates: 500 → errors.Is(err, ErrTransient); guards against silent swallow of transient API failures. Diff (5): - Diff_NilCurrent_NeedsUpdate: nil current → NeedsUpdate=true; doesn't panic. - Diff_NoChanges: desired==current → 0 changes, no update, no replace. - Diff_NameChange: name divergence → NeedsUpdate=true (NOT NeedsReplace); godo Update accepts Name → in-place rename per ADR 0015. - Diff_GrantsChange: grants divergence → FieldChange{Path: grants} with canonical []map[string]any Old/New shapes (post-normalize). - Diff_StructpbRoundtripResilience: current.Outputs[grants] as []any{map[string]any} (post-structpb shape) MUST NOT false-positive drift; pins the gRPC plugin boundary contract per workspace memory feedback_workflow_plugin_structpb_boundary. HealthCheck (1): - HealthCheck: existence == healthy; spaces keys have no provider-side health concept. Verified: GOWORK=off go test ./internal/drivers ./internal -count=1 → ok. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/spaces_key_test.go | 655 ++++++++++++++++++++++++++++ 1 file changed, 655 insertions(+) diff --git a/internal/drivers/spaces_key_test.go b/internal/drivers/spaces_key_test.go index e991abb..5938114 100644 --- a/internal/drivers/spaces_key_test.go +++ b/internal/drivers/spaces_key_test.go @@ -3,6 +3,7 @@ package drivers_test import ( "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "net/url" @@ -140,6 +141,660 @@ func TestSpacesKeyDriver_Create_HappyPath(t *testing.T) { } } +// TestSpacesKeyDriver_Read_HappyPath pins the Read contract: with a non-empty +// ProviderID, the driver issues GET /v2/spaces/keys/{access_key} and returns +// the metadata fields ONLY — secret_key MUST NOT appear in Outputs (the DO +// API never re-emits it after Create), and Sensitive[secret_key] MUST be +// unset/false. access_key remains flagged sensitive for masking only. +func TestSpacesKeyDriver_Read_HappyPath(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys/AKIATEST123" && r.Method == http.MethodGet { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "key": map[string]any{ + "name": "test-key", + "access_key": "AKIATEST123", + "created_at": "2026-05-08T11:00:00Z", + "grants": []any{map[string]any{"permission": "fullaccess", "bucket": ""}}, + }, + }) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + })) + defer srv.Close() + + driver := drivers.NewSpacesKeyDriver(godoClientForTest(t, srv)) + out, err := driver.Read(context.Background(), interfaces.ResourceRef{ + Name: "test-key", + Type: "infra.spaces_key", + ProviderID: "AKIATEST123", + }) + if err != nil { + t.Fatalf("Read: %v", err) + } + if out.ProviderID != "AKIATEST123" { + t.Errorf("ProviderID: want AKIATEST123, got %q", out.ProviderID) + } + if name, _ := out.Outputs["name"].(string); name != "test-key" { + t.Errorf("Outputs[name]: want test-key, got %v", out.Outputs["name"]) + } + if ak, _ := out.Outputs["access_key"].(string); ak != "AKIATEST123" { + t.Errorf("Outputs[access_key]: want AKIATEST123, got %v", out.Outputs["access_key"]) + } + if ca, _ := out.Outputs["created_at"].(string); ca != "2026-05-08T11:00:00Z" { + t.Errorf("Outputs[created_at]: want 2026-05-08T11:00:00Z, got %v", out.Outputs["created_at"]) + } + if _, ok := out.Outputs["grants"]; !ok { + t.Errorf("Outputs[grants]: want present, got absent") + } + // secret_key MUST NOT appear in Outputs on Read — the API never re-emits it. + if _, ok := out.Outputs["secret_key"]; ok { + t.Errorf("Outputs[secret_key]: must be absent on Read; got %v", out.Outputs["secret_key"]) + } + // Sensitive[secret_key] must be unset/false on Read (only access_key is flagged). + if out.Sensitive["secret_key"] { + t.Errorf("Sensitive[secret_key]: must be unset/false on Read, got true") + } + if !out.Sensitive["access_key"] { + t.Errorf("Sensitive[access_key]: want true (display masking), got false") + } +} + +// TestSpacesKeyDriver_Read_NotFound asserts that a 404 from godo Get is +// converted to interfaces.ErrResourceNotFound (via WrapGodoError → +// sentinelForStatus mapping) so callers can errors.Is for typed handling. +func TestSpacesKeyDriver_Read_NotFound(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys/AK_GONE" && r.Method == http.MethodGet { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"id":"not_found","message":"key not found"}`)) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + })) + defer srv.Close() + + driver := drivers.NewSpacesKeyDriver(godoClientForTest(t, srv)) + _, err := driver.Read(context.Background(), interfaces.ResourceRef{ + Name: "test-key", + ProviderID: "AK_GONE", + }) + if err == nil { + t.Fatal("Read: expected ErrResourceNotFound, got nil") + } + if !errors.Is(err, interfaces.ErrResourceNotFound) { + t.Errorf("Read: want errors.Is(err, ErrResourceNotFound), got %v", err) + } +} + +// TestSpacesKeyDriver_Read_FindByName covers the upsert / state-heal fallback: +// when ResourceRef.ProviderID is empty, Read paginates the List endpoint and +// matches by Name. The httptest server serves two pages so the pagination +// loop in findByName is exercised. +func TestSpacesKeyDriver_Read_FindByName(t *testing.T) { + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys" && r.Method == http.MethodGet { + w.Header().Set("Content-Type", "application/json") + page := r.URL.Query().Get("page") + if page == "" || page == "1" { + _ = json.NewEncoder(w).Encode(map[string]any{ + "keys": []any{ + map[string]any{"name": "key-other", "access_key": "AK_OTHER", "created_at": "2026-05-01T00:00:00Z"}, + }, + "links": map[string]any{ + "pages": map[string]any{"next": srv.URL + "/v2/spaces/keys?page=2"}, + }, + }) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "keys": []any{ + map[string]any{ + "name": "target-key", + "access_key": "AK_TARGET", + "created_at": "2026-05-02T00:00:00Z", + "grants": []any{map[string]any{"permission": "read", "bucket": "bk"}}, + }, + }, + }) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + })) + defer srv.Close() + + driver := drivers.NewSpacesKeyDriver(godoClientForTest(t, srv)) + out, err := driver.Read(context.Background(), interfaces.ResourceRef{ + Name: "target-key", + // ProviderID intentionally empty — drives findByName fallback. + }) + if err != nil { + t.Fatalf("Read (findByName): %v", err) + } + if out.ProviderID != "AK_TARGET" { + t.Errorf("ProviderID: want AK_TARGET, got %q", out.ProviderID) + } + if name, _ := out.Outputs["name"].(string); name != "target-key" { + t.Errorf("Outputs[name]: want target-key, got %v", out.Outputs["name"]) + } +} + +// TestSpacesKeyDriver_Read_FindByName_NotFound asserts that when the name +// fallback exhausts pagination without a match, ErrResourceNotFound is +// returned (wrapped). This is the negative-path companion to FindByName. +func TestSpacesKeyDriver_Read_FindByName_NotFound(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys" && r.Method == http.MethodGet { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "keys": []any{ + map[string]any{"name": "other-1", "access_key": "AK_O1"}, + map[string]any{"name": "other-2", "access_key": "AK_O2"}, + }, + // No links.pages.next → list ends after this page. + }) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + })) + defer srv.Close() + + driver := drivers.NewSpacesKeyDriver(godoClientForTest(t, srv)) + _, err := driver.Read(context.Background(), interfaces.ResourceRef{Name: "missing-key"}) + if err == nil { + t.Fatal("Read (findByName): expected ErrResourceNotFound, got nil") + } + if !errors.Is(err, interfaces.ErrResourceNotFound) { + t.Errorf("Read (findByName): want errors.Is(err, ErrResourceNotFound), got %v", err) + } +} + +// TestSpacesKeyDriver_Update_RenameInPlace pins the in-place rename contract: +// godo Update accepts both Name and Grants, so a name change is NeedsUpdate +// (not NeedsReplace). The test asserts the request payload reaches DO with +// both fields, the response is mapped to Outputs, and secret_key is NOT in +// Outputs (Update never re-issues the secret). +func TestSpacesKeyDriver_Update_RenameInPlace(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys/AKIATEST123" && r.Method == http.MethodPut { + var got godo.SpacesKeyUpdateRequest + if err := json.NewDecoder(r.Body).Decode(&got); err != nil { + http.Error(w, "decode body: "+err.Error(), http.StatusBadRequest) + return + } + if got.Name != "renamed-key" { + http.Error(w, "request name="+got.Name+" want renamed-key", http.StatusBadRequest) + return + } + if len(got.Grants) != 1 || got.Grants[0] == nil || + got.Grants[0].Permission != godo.SpacesKeyFullAccess { + http.Error(w, "request grants mismatch", http.StatusBadRequest) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "key": map[string]any{ + "name": "renamed-key", + "access_key": "AKIATEST123", + "created_at": "2026-05-08T11:00:00Z", + "grants": []any{map[string]any{"permission": "fullaccess", "bucket": ""}}, + }, + }) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + })) + defer srv.Close() + + driver := drivers.NewSpacesKeyDriver(godoClientForTest(t, srv)) + out, err := driver.Update(context.Background(), + interfaces.ResourceRef{Name: "test-key", ProviderID: "AKIATEST123"}, + interfaces.ResourceSpec{ + Name: "renamed-key", + Type: "infra.spaces_key", + Config: map[string]any{ + "name": "renamed-key", + "grants": []any{map[string]any{"permission": "fullaccess"}}, + }, + }, + ) + if err != nil { + t.Fatalf("Update: %v", err) + } + if name, _ := out.Outputs["name"].(string); name != "renamed-key" { + t.Errorf("Outputs[name]: want renamed-key, got %v", out.Outputs["name"]) + } + // secret_key MUST NOT appear in Update Outputs — Update doesn't re-issue. + if _, ok := out.Outputs["secret_key"]; ok { + t.Errorf("Outputs[secret_key]: must be absent on Update; got %v", out.Outputs["secret_key"]) + } + if out.Sensitive["secret_key"] { + t.Errorf("Sensitive[secret_key]: must be unset/false on Update, got true") + } +} + +// TestSpacesKeyDriver_Update_GrantsChange pins the grants-only update path: +// name unchanged, grants list rewritten. Asserts request payload faithfully +// translates spec.Config["grants"] to the godo wire format. +func TestSpacesKeyDriver_Update_GrantsChange(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys/AKIATEST123" && r.Method == http.MethodPut { + var got godo.SpacesKeyUpdateRequest + if err := json.NewDecoder(r.Body).Decode(&got); err != nil { + http.Error(w, "decode body: "+err.Error(), http.StatusBadRequest) + return + } + if got.Name != "test-key" { + http.Error(w, "request name="+got.Name+" want test-key", http.StatusBadRequest) + return + } + if len(got.Grants) != 2 { + http.Error(w, "request grants len mismatch", http.StatusBadRequest) + return + } + if got.Grants[0].Bucket != "bk-1" || got.Grants[0].Permission != godo.SpacesKeyRead { + http.Error(w, "request grants[0] mismatch", http.StatusBadRequest) + return + } + if got.Grants[1].Bucket != "bk-2" || got.Grants[1].Permission != godo.SpacesKeyReadWrite { + http.Error(w, "request grants[1] mismatch", http.StatusBadRequest) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "key": map[string]any{ + "name": "test-key", + "access_key": "AKIATEST123", + "created_at": "2026-05-08T11:00:00Z", + "grants": []any{ + map[string]any{"permission": "read", "bucket": "bk-1"}, + map[string]any{"permission": "readwrite", "bucket": "bk-2"}, + }, + }, + }) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + })) + defer srv.Close() + + driver := drivers.NewSpacesKeyDriver(godoClientForTest(t, srv)) + out, err := driver.Update(context.Background(), + interfaces.ResourceRef{Name: "test-key", ProviderID: "AKIATEST123"}, + interfaces.ResourceSpec{ + Name: "test-key", + Type: "infra.spaces_key", + Config: map[string]any{ + "name": "test-key", + "grants": []any{ + map[string]any{"permission": "read", "bucket": "bk-1"}, + map[string]any{"permission": "readwrite", "bucket": "bk-2"}, + }, + }, + }, + ) + if err != nil { + t.Fatalf("Update: %v", err) + } + grants, ok := out.Outputs["grants"].([]map[string]any) + if !ok { + t.Fatalf("Outputs[grants]: want []map[string]any, got %T", out.Outputs["grants"]) + } + if len(grants) != 2 { + t.Fatalf("Outputs[grants] len: want 2, got %d", len(grants)) + } + if grants[0]["bucket"] != "bk-1" || grants[0]["permission"] != "read" { + t.Errorf("Outputs[grants][0]: want {bk-1, read}, got %v", grants[0]) + } +} + +// TestSpacesKeyDriver_Delete_HappyPath asserts a 204 No Content from godo +// Delete is treated as success (nil error). +func TestSpacesKeyDriver_Delete_HappyPath(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys/AKIATEST123" && r.Method == http.MethodDelete { + w.WriteHeader(http.StatusNoContent) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + })) + defer srv.Close() + + driver := drivers.NewSpacesKeyDriver(godoClientForTest(t, srv)) + err := driver.Delete(context.Background(), interfaces.ResourceRef{ + Name: "test-key", ProviderID: "AKIATEST123", + }) + if err != nil { + t.Fatalf("Delete: want nil, got %v", err) + } +} + +// TestSpacesKeyDriver_Delete_Idempotent_404 pins the idempotent-delete contract: +// a 404 response from DO (key already absent) is treated as success — matches +// sister provider RevokeProviderCredential and ADR 0015. +func TestSpacesKeyDriver_Delete_Idempotent_404(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys/AK_GONE" && r.Method == http.MethodDelete { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"id":"not_found","message":"key not found"}`)) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + })) + defer srv.Close() + + driver := drivers.NewSpacesKeyDriver(godoClientForTest(t, srv)) + err := driver.Delete(context.Background(), interfaces.ResourceRef{ + Name: "test-key", ProviderID: "AK_GONE", + }) + if err != nil { + t.Fatalf("Delete (404 idempotent): want nil, got %v", err) + } +} + +// TestSpacesKeyDriver_Delete_OtherError_Propagates asserts that non-404 +// errors (e.g. 500) propagate as errors rather than being silently swallowed. +// Without this guard, transient API failures on Delete could leak resources +// (the engine would believe the delete succeeded). +func TestSpacesKeyDriver_Delete_OtherError_Propagates(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys/AKIATEST123" && r.Method == http.MethodDelete { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"id":"server_error","message":"upstream failure"}`)) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + })) + defer srv.Close() + + driver := drivers.NewSpacesKeyDriver(godoClientForTest(t, srv)) + err := driver.Delete(context.Background(), interfaces.ResourceRef{ + Name: "test-key", ProviderID: "AKIATEST123", + }) + if err == nil { + t.Fatal("Delete (500): expected non-nil error, got nil") + } + // Per WrapGodoError: 5xx maps to ErrTransient. + if !errors.Is(err, interfaces.ErrTransient) { + t.Errorf("Delete (500): want errors.Is(err, ErrTransient), got %v", err) + } +} + +// TestSpacesKeyDriver_Diff_NilCurrent_NeedsUpdate covers the resource-not-yet- +// created path: Diff(spec, nil) must return NeedsUpdate=true and not panic. +// Per the impl docstring at internal/drivers/spaces_key.go:179, nil current +// means "treat as initial create". +func TestSpacesKeyDriver_Diff_NilCurrent_NeedsUpdate(t *testing.T) { + driver := drivers.NewSpacesKeyDriver(godo.NewClient(nil)) + result, err := driver.Diff(context.Background(), + interfaces.ResourceSpec{ + Name: "test-key", + Type: "infra.spaces_key", + Config: map[string]any{ + "name": "test-key", + "grants": []any{map[string]any{"permission": "fullaccess"}}, + }, + }, + nil, + ) + if err != nil { + t.Fatalf("Diff(nil current): %v", err) + } + if !result.NeedsUpdate { + t.Errorf("Diff(nil current): want NeedsUpdate=true, got false") + } +} + +// TestSpacesKeyDriver_Diff_NoChanges asserts that when desired matches current +// (same name, same grants), Diff reports no changes, no update, no replace. +func TestSpacesKeyDriver_Diff_NoChanges(t *testing.T) { + driver := drivers.NewSpacesKeyDriver(godo.NewClient(nil)) + result, err := driver.Diff(context.Background(), + interfaces.ResourceSpec{ + Name: "test-key", + Type: "infra.spaces_key", + Config: map[string]any{ + "name": "test-key", + "grants": []any{map[string]any{"permission": "fullaccess", "bucket": ""}}, + }, + }, + &interfaces.ResourceOutput{ + Name: "test-key", + ProviderID: "AKIATEST123", + Outputs: map[string]any{ + "name": "test-key", + "access_key": "AKIATEST123", + "grants": []map[string]any{ + {"permission": "fullaccess", "bucket": ""}, + }, + }, + }, + ) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if result.NeedsUpdate { + t.Errorf("Diff: want NeedsUpdate=false, got true (changes=%v)", result.Changes) + } + if result.NeedsReplace { + t.Errorf("Diff: want NeedsReplace=false, got true") + } + if len(result.Changes) != 0 { + t.Errorf("Diff: want 0 changes, got %d (%v)", len(result.Changes), result.Changes) + } +} + +// TestSpacesKeyDriver_Diff_NameChange asserts that a name divergence yields +// NeedsUpdate=true (NOT NeedsReplace), since godo Update accepts Name → the +// driver does an in-place rename. Pins ADR 0015 + spaces_key.go:25-28 +// docstring contract. +func TestSpacesKeyDriver_Diff_NameChange(t *testing.T) { + driver := drivers.NewSpacesKeyDriver(godo.NewClient(nil)) + result, err := driver.Diff(context.Background(), + interfaces.ResourceSpec{ + Name: "renamed-key", + Type: "infra.spaces_key", + Config: map[string]any{ + "name": "renamed-key", + "grants": []any{map[string]any{"permission": "fullaccess", "bucket": ""}}, + }, + }, + &interfaces.ResourceOutput{ + Name: "test-key", + ProviderID: "AKIATEST123", + Outputs: map[string]any{ + "name": "test-key", + "access_key": "AKIATEST123", + "grants": []map[string]any{ + {"permission": "fullaccess", "bucket": ""}, + }, + }, + }, + ) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !result.NeedsUpdate { + t.Errorf("Diff: want NeedsUpdate=true (name change), got false") + } + if result.NeedsReplace { + t.Errorf("Diff: want NeedsReplace=false (rename is in-place), got true") + } + // Find the name FieldChange. + var nameChange *interfaces.FieldChange + for i := range result.Changes { + if result.Changes[i].Path == "name" { + nameChange = &result.Changes[i] + break + } + } + if nameChange == nil { + t.Fatalf("Diff: want FieldChange{Path: name}, got %v", result.Changes) + } + if nameChange.Old != "test-key" || nameChange.New != "renamed-key" { + t.Errorf("Diff: name FieldChange Old=%v New=%v, want test-key→renamed-key", + nameChange.Old, nameChange.New) + } + if nameChange.ForceNew { + t.Errorf("Diff: name FieldChange.ForceNew: want false, got true") + } +} + +// TestSpacesKeyDriver_Diff_GrantsChange asserts that grants-list divergence +// yields NeedsUpdate=true with a grants FieldChange (Old/New populated with +// the normalized canonical map shape). +func TestSpacesKeyDriver_Diff_GrantsChange(t *testing.T) { + driver := drivers.NewSpacesKeyDriver(godo.NewClient(nil)) + result, err := driver.Diff(context.Background(), + interfaces.ResourceSpec{ + Name: "test-key", + Type: "infra.spaces_key", + Config: map[string]any{ + "name": "test-key", + "grants": []any{ + map[string]any{"permission": "readwrite", "bucket": "bk-1"}, + }, + }, + }, + &interfaces.ResourceOutput{ + Name: "test-key", + ProviderID: "AKIATEST123", + Outputs: map[string]any{ + "name": "test-key", + "access_key": "AKIATEST123", + "grants": []map[string]any{ + {"permission": "read", "bucket": "bk-1"}, + }, + }, + }, + ) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !result.NeedsUpdate { + t.Errorf("Diff: want NeedsUpdate=true (grants change), got false") + } + var grantsChange *interfaces.FieldChange + for i := range result.Changes { + if result.Changes[i].Path == "grants" { + grantsChange = &result.Changes[i] + break + } + } + if grantsChange == nil { + t.Fatalf("Diff: want FieldChange{Path: grants}, got %v", result.Changes) + } + // Old/New must be the canonical []map[string]any shape (post-normalize). + wantOld := []map[string]any{{"permission": "read", "bucket": "bk-1"}} + wantNew := []map[string]any{{"permission": "readwrite", "bucket": "bk-1"}} + if !reflect.DeepEqual(grantsChange.Old, wantOld) { + t.Errorf("Diff: grants Old: want %v, got %v", wantOld, grantsChange.Old) + } + if !reflect.DeepEqual(grantsChange.New, wantNew) { + t.Errorf("Diff: grants New: want %v, got %v", wantNew, grantsChange.New) + } +} + +// TestSpacesKeyDriver_Diff_StructpbRoundtripResilience pins the gRPC plugin +// boundary contract: when Outputs round-trips through structpb, typed slices +// are decoded as []any with each element being map[string]any. The driver's +// normalizeGrantsForDiff helper MUST coerce that shape to the canonical +// []map[string]any so DeepEqual against grantsToMaps's output succeeds and +// Diff does NOT false-positive on a no-op refresh. +// +// Per workspace memory feedback_workflow_plugin_structpb_boundary — +// without this, drivers running over the gRPC plugin boundary erroneously +// report "drift" on every plan even when nothing changed. +func TestSpacesKeyDriver_Diff_StructpbRoundtripResilience(t *testing.T) { + driver := drivers.NewSpacesKeyDriver(godo.NewClient(nil)) + // Construct current.Outputs["grants"] as the post-structpb-roundtrip shape: + // []any of map[string]any (NOT []map[string]any). This is what comes back + // from structpb.NewStruct → AsMap on the consumer side. + result, err := driver.Diff(context.Background(), + interfaces.ResourceSpec{ + Name: "test-key", + Type: "infra.spaces_key", + Config: map[string]any{ + "name": "test-key", + "grants": []any{map[string]any{"permission": "fullaccess", "bucket": ""}}, + }, + }, + &interfaces.ResourceOutput{ + Name: "test-key", + ProviderID: "AKIATEST123", + Outputs: map[string]any{ + "name": "test-key", + "access_key": "AKIATEST123", + // Note: []any (not []map[string]any) — the structpb-roundtripped shape. + "grants": []any{ + map[string]any{"permission": "fullaccess", "bucket": ""}, + }, + }, + }, + ) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if result.NeedsUpdate { + t.Errorf("Diff (structpb roundtrip): want NeedsUpdate=false (no real drift), got true; changes=%v", + result.Changes) + } + if len(result.Changes) != 0 { + t.Errorf("Diff (structpb roundtrip): want 0 changes, got %d (%v)", + len(result.Changes), result.Changes) + } +} + +// TestSpacesKeyDriver_HealthCheck asserts the HealthCheck contract: with a +// non-empty ProviderID and a successful godo Get, it returns Healthy=true. +// Spaces keys have no provider-side health concept, so existence == healthy. +func TestSpacesKeyDriver_HealthCheck(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/spaces/keys/AKIATEST123" && r.Method == http.MethodGet { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "key": map[string]any{ + "name": "test-key", + "access_key": "AKIATEST123", + "created_at": "2026-05-08T11:00:00Z", + }, + }) + return + } + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + })) + defer srv.Close() + + driver := drivers.NewSpacesKeyDriver(godoClientForTest(t, srv)) + result, err := driver.HealthCheck(context.Background(), interfaces.ResourceRef{ + Name: "test-key", ProviderID: "AKIATEST123", + }) + if err != nil { + t.Fatalf("HealthCheck: %v", err) + } + if result == nil { + t.Fatal("HealthCheck: result is nil") + } + if !result.Healthy { + t.Errorf("HealthCheck: want Healthy=true, got false (msg=%q)", result.Message) + } +} + // godoClientForTest builds a godo client whose BaseURL points at the given // httptest server and whose underlying http.Client is the test server's own // client. Mirrors `internal/provider_enumerator_test.go::newProviderForEnumeratorTest`