From 234cd759c57ec0d8f0718eb10eeba24f93e4b289 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 21:20:21 -0400 Subject: [PATCH 01/11] chore(deps): bump workflow to e2c582b (W-7 conformance + W-8 codemod) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pseudo-version v0.20.6-0.20260505011403-e2c582bece90, the workflow main HEAD that includes: - W-7: iac/conformance scenario suite (12 scenarios) and DO smoke gate - W-8: cmd/iac-codemod 4-mode AST tool Required for TP1-TP5 of PR P-DO (IaC conformance plan §P-DO). Co-Authored-By: Claude Opus 4.7 (1M context) --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 0b4867e..9ed48da 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.20.5 + github.com/GoCodeAlone/workflow v0.20.6-0.20260505011403-e2c582bece90 github.com/aws/aws-sdk-go-v2 v1.41.5 github.com/aws/aws-sdk-go-v2/credentials v1.19.12 github.com/aws/aws-sdk-go-v2/service/s3 v1.97.2 diff --git a/go.sum b/go.sum index 921b757..1ef1a78 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.20.5 h1:CtsQ7vGiF0xlJQ7w/bhWgW06ojw6E45kxjqgRB6jAJQ= -github.com/GoCodeAlone/workflow v0.20.5/go.mod h1:ypkCqXTwnIPqNjS8h38KZfwzdVsgwgkS1d6Dq0lXyQQ= +github.com/GoCodeAlone/workflow v0.20.6-0.20260505011403-e2c582bece90 h1:A4efhC8phY+M8KOB9ccga8COdjYMw+OKHhUuWcKgltA= +github.com/GoCodeAlone/workflow v0.20.6-0.20260505011403-e2c582bece90/go.mod h1:IaonttCuyJcjT+XryXUuh3ATvTI7jx0t8e3n+ASCOOA= 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 53fbb5abef757c2dc9a9b781de7e768307483b88 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 21:20:57 -0400 Subject: [PATCH 02/11] ci(plugin): codemod-report workflow uploads dry-run output as artifact + sticky PR comment summary PR P-DO TP1: per pull_request, runs the iac-codemod refactor-apply mode in -dry-run against the plugin source, uploads the full Markdown report as a 90-day retention GitHub Actions artifact, and posts/updates a sticky PR comment with the top-30 lines of the report so drive-by reviewers see the key findings without downloading the artifact. Supply-chain note: actions/github-script SHA-pinned per workflow security policy (Renovate tracks upstream releases via .github/renovate.json). Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/codemod-report.yml | 73 ++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 .github/workflows/codemod-report.yml diff --git a/.github/workflows/codemod-report.yml b/.github/workflows/codemod-report.yml new file mode 100644 index 0000000..3778443 --- /dev/null +++ b/.github/workflows/codemod-report.yml @@ -0,0 +1,73 @@ +name: codemod-report +on: + pull_request: + branches: [main] +permissions: + contents: read + pull-requests: write + issues: write +env: + GOPRIVATE: github.com/GoCodeAlone/* +jobs: + codemod-report: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + - name: Configure Git for private repos + env: + RELEASES_TOKEN: ${{ secrets.RELEASES_TOKEN }} + run: | + if [ -n "${RELEASES_TOKEN}" ]; then + git config --global url."https://x-access-token:${RELEASES_TOKEN}@github.com/".insteadOf "https://github.com/" + fi + - name: Run iac-codemod refactor-apply (dry-run) + run: | + set -euo pipefail + WFCTL_VERSION="$(go list -m github.com/GoCodeAlone/workflow | awk '{print $2}')" + go run "github.com/GoCodeAlone/workflow/cmd/iac-codemod@${WFCTL_VERSION}" \ + refactor-apply -dry-run . > /tmp/codemod-report.md + # Short summary for the sticky PR comment. + head -30 /tmp/codemod-report.md > /tmp/codemod-summary.md + echo "" >> /tmp/codemod-summary.md + echo "_Full report (90-day retention) attached as workflow artifact._" >> /tmp/codemod-summary.md + - uses: actions/upload-artifact@v4 + with: + name: codemod-report-${{ github.event.pull_request.number }} + path: /tmp/codemod-report.md + retention-days: 90 + - name: Comment summary on PR (first-party action SHA-pinned) + # rev5 — pin to commit SHA even though first-party. Tag mutability is a + # known risk; Renovate config tracks upstream releases via .github/renovate.json. + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const fs = require('fs'); + const summary = fs.readFileSync('/tmp/codemod-summary.md', 'utf8'); + const marker = ''; + const body = `${marker}\n${summary}`; + // Paginate to handle PRs with >30 comments (default page size). + const allComments = await github.paginate(github.rest.issues.listComments, { + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + per_page: 100, + }); + const existing = allComments.find(c => c.body && c.body.startsWith(marker)); + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body, + }); + } From f9f0022ff5212f558c3fd357649cb00c95dc9402 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 21:25:21 -0400 Subject: [PATCH 03/11] refactor(provider): collapse Apply to wfctlhelpers.ApplyPlan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR P-DO TP2: replace the in-Apply per-action switch (create/update/replace/ delete + upsert recovery + nil-out diagnostic) with a single dispatch to wfctlhelpers.ApplyPlan. The helper handles: - ErrResourceAlreadyExists upsert recovery via interfaces.UpsertSupporter (DO drivers AppPlatform, VPC, Firewall, Database already implement SupportsUpsert() bool, so they satisfy the canonical interface without code change — the local upsertSupporter declaration is now removed). - JIT ${MODULE.id} / ${VAR} substitution (W-5). - Replace decomposition + ReplaceIDMap propagation (W-3a/W-3b). - Input-drift postcondition (W-3a). - Per-action context cancellation between iterations. The DO-plugin-specific deferred-update flush (DatabaseDriver type=app trusted_sources referencing apps created later in the plan; regression gated by provider_deferred_test.go and CHANGELOG entry for staging-deploy- blockers Blocker 2) is preserved by wrapping ApplyPlan with the second- pass loop that calls FlushDeferredUpdates on any deferredUpdater driver. The wrapper deviates from the codemod's canonical 'return wfctlhelpers.ApplyPlan(ctx, p, plan)' single-statement shape; the deviation is documented and marked with // wfctl:skip-iac-codemod so AssertApplyDelegatesToHelper recognizes the intentional shape. When wfctlhelpers grows a deferred-update lifecycle hook, the wrapper can collapse and the marker can drop. The provider_apply_test.go DeleteAction_MissingCurrent regression test was a v1 pre-flight defense (synthesize an error when action.Current is nil before calling Delete). Under v2 dispatch the contract is 'driver is the authority on what an empty ProviderID means' (per wfctlhelpers/ apply.go::doUpdate's analogous comment). The test was rewritten to lock the new contract: dispatch IS made with an empty-ProviderID ref; real drivers like FirewallDriver surface the diagnostic via their typed validation. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/provider.go | 165 ++++++++++---------------------- internal/provider_apply_test.go | 33 +++++-- 2 files changed, 74 insertions(+), 124 deletions(-) diff --git a/internal/provider.go b/internal/provider.go index 151fb29..026ac29 100644 --- a/internal/provider.go +++ b/internal/provider.go @@ -10,6 +10,7 @@ import ( "time" "github.com/GoCodeAlone/workflow-plugin-digitalocean/internal/drivers" + "github.com/GoCodeAlone/workflow/iac/wfctlhelpers" "github.com/GoCodeAlone/workflow/interfaces" "github.com/digitalocean/godo" "golang.org/x/oauth2" @@ -217,134 +218,68 @@ func resourceOutputFromState(state interfaces.ResourceState) *interfaces.Resourc } } -// upsertSupporter is an optional interface for ResourceDrivers that support -// locating a resource by name alone (empty ProviderID) in their Read method. -// Only drivers that implement name-based discovery should implement this -// interface. Apply gates the ErrResourceAlreadyExists → upsert path on it to -// prevent calling Read with an empty ProviderID on drivers that require one. -type upsertSupporter interface { - SupportsUpsert() bool -} - // deferredUpdater is an optional interface for ResourceDrivers that accumulate // resource-level updates that cannot be applied until all plan creates complete. // The canonical case is DatabaseDriver deferring type=app trusted_sources entries // that reference apps created later in the same plan. Apply calls -// FlushDeferredUpdates once after the main action loop; errors are appended to +// FlushDeferredUpdates once after the main dispatch loop; errors are appended to // ApplyResult.Errors so the failure is visible to the operator. +// +// This is a DO-plugin-specific extension point not yet hoisted into +// wfctlhelpers.ApplyPlan; the second-pass flush below preserves the regression +// gate while the v2 dispatch handles the per-action loop. type deferredUpdater interface { HasDeferredUpdates() bool FlushDeferredUpdates(ctx context.Context) error } -// Apply executes the plan. +// Apply executes the plan via wfctlhelpers.ApplyPlan, then runs the +// DO-specific deferred-update second pass. +// +// PR P-DO TP2: under iacProvider.computePlanVersion: v2 wfctl dispatches +// directly through wfctlhelpers.ApplyPlan and does not call this method. +// The implementation here remains for legacy v1 callers (wfctl < v0.21.0 +// or any in-process embedder of the gRPC plugin) and for the deferred-flush +// behavior that wfctlhelpers does not yet hoist. +// +// Per-action upsert recovery, JIT substitution, the Replace cascade, and +// the input-drift postcondition all live in wfctlhelpers.ApplyPlan now — +// drivers that opt into the upsert recovery path implement +// interfaces.UpsertSupporter (DO drivers AppPlatform, VPC, Firewall, +// Database all do; signature: SupportsUpsert() bool). The local +// upsertSupporter interface previously declared here is no longer needed: +// its SupportsUpsert() bool method is structurally identical to +// interfaces.UpsertSupporter, so the existing driver implementations +// satisfy the canonical interface without code change. +// +// wfctl:skip-iac-codemod +// +// The body intentionally wraps wfctlhelpers.ApplyPlan rather than +// matching the codemod's canonical single-statement +// `return wfctlhelpers.ApplyPlan(ctx, p, plan)` shape: the +// post-helper deferred-update flush below is a DO-plugin-specific +// regression gate (see provider_deferred_test.go and CHANGELOG entry +// for staging-deploy-blockers Blocker 2) that wfctlhelpers does not +// hoist. The skip marker tells the codemod's +// AssertApplyDelegatesToHelper analyzer this deviation is intentional. +// When wfctlhelpers grows a deferred-update lifecycle hook, the +// wrapper can collapse and the marker can drop. func (p *DOProvider) Apply(ctx context.Context, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { - result := &interfaces.ApplyResult{PlanID: plan.ID} - for _, action := range plan.Actions { - d, err := p.ResourceDriver(action.Resource.Type) - if err != nil { - result.Errors = append(result.Errors, interfaces.ActionError{ - Resource: action.Resource.Name, Action: action.Action, Error: err.Error(), - }) - continue - } - var out *interfaces.ResourceOutput - switch action.Action { - case "create": - out, err = d.Create(ctx, action.Resource) - if errors.Is(err, interfaces.ErrResourceAlreadyExists) { - // Upsert: resource exists in the provider but is absent from - // local state. Only attempt upsert if the driver supports - // name-based discovery (SupportsUpsert returns true). - // Drivers that pass ProviderID directly to their API client - // (VPC, database, firewall, etc.) do not support this path. - us, ok := d.(upsertSupporter) - if !ok || !us.SupportsUpsert() { - // Propagate original error; upsert not available for this type. - break - } - createErr := err - ref := interfaces.ResourceRef{ - Name: action.Resource.Name, - Type: action.Resource.Type, - } - existing, readErr := d.Read(ctx, ref) - if readErr != nil { - // Preserve createErr so ErrResourceAlreadyExists remains - // in the error chain for callers using errors.Is. - err = fmt.Errorf("upsert: read after conflict: %w", errors.Join(createErr, readErr)) - break - } - if existing.ProviderID == "" { - err = fmt.Errorf("upsert: resource %q found by name but ProviderID is empty; cannot update: %w", ref.Name, createErr) - break - } - ref.ProviderID = existing.ProviderID - out, err = d.Update(ctx, ref, action.Resource) - } - case "update": - ref := interfaces.ResourceRef{ - Name: action.Resource.Name, - Type: action.Resource.Type, - ProviderID: action.Current.ProviderID, - } - out, err = d.Update(ctx, ref, action.Resource) - case "replace": - if action.Current == nil { - err = fmt.Errorf("replace action missing current resource state") - break - } - ref := interfaces.ResourceRef{ - Name: action.Resource.Name, - Type: action.Resource.Type, - ProviderID: action.Current.ProviderID, - } - if err = d.Delete(ctx, ref); err != nil { - break - } - out, err = d.Create(ctx, action.Resource) - case "delete": - // Delete uses Current for the ref (ProviderID). Resource.Config is - // empty for deletes — the desired state is "absent" — but - // Resource.Type and Name are still set (required for driver lookup above). - if action.Current == nil { - err = fmt.Errorf("delete action for %q missing current resource state", action.Resource.Name) - break - } - ref := interfaces.ResourceRef{ - Name: action.Current.Name, - Type: action.Current.Type, - ProviderID: action.Current.ProviderID, - } - err = d.Delete(ctx, ref) - // out remains nil — deleted resources have no post-apply output. - default: - err = fmt.Errorf("unknown action %q", action.Action) - } - if err != nil { - result.Errors = append(result.Errors, interfaces.ActionError{ - Resource: action.Resource.Name, Action: action.Action, Error: err.Error(), - }) - continue - } - if out != nil { - result.Resources = append(result.Resources, *out) - } else if action.Action != "delete" { - // A nil output without an error from create/update/replace is a driver - // bug — surface it explicitly so callers aren't silently missing state - // entries. "delete" legitimately returns nil out, so it's excluded. - result.Errors = append(result.Errors, interfaces.ActionError{ - Resource: action.Resource.Name, Action: action.Action, - Error: "driver returned nil output without error", - }) - } + result, err := wfctlhelpers.ApplyPlan(ctx, p, plan) + if err != nil { + // ApplyPlan only returns a top-level error on context cancellation + // — per-action failures land on result.Errors. Surface the + // cancellation alongside whatever partial result is in hand so + // callers can still inspect any actions that completed. + return result, err } - // Second pass: flush deferred updates accumulated by drivers during the main - // action loop. These arise when a resource's config (e.g. DB trusted_sources - // with type=app) references another resource provisioned later in the same - // plan. By this point all plan creates have completed and the referenced - // resources exist. Each driver type is flushed at most once. + // Second pass: flush deferred updates accumulated by drivers during the + // main action loop. These arise when a resource's config (e.g. DB + // trusted_sources with type=app) references another resource provisioned + // later in the same plan. By this point all plan creates have completed + // and the referenced resources exist. Each driver type is flushed at + // most once. seen := make(map[string]struct{}, len(plan.Actions)) for _, action := range plan.Actions { if _, done := seen[action.Resource.Type]; done { diff --git a/internal/provider_apply_test.go b/internal/provider_apply_test.go index f13859f..d3297da 100644 --- a/internal/provider_apply_test.go +++ b/internal/provider_apply_test.go @@ -84,8 +84,14 @@ func TestDOProvider_Apply_DeleteAction(t *testing.T) { } } -// TestDOProvider_Apply_DeleteAction_MissingCurrent verifies that a delete -// action with no Current state is collected as an error (not a panic/crash). +// TestDOProvider_Apply_DeleteAction_MissingCurrent verifies the v2-dispatch +// contract for a delete action with action.Current == nil: +// wfctlhelpers.ApplyPlan dispatches Delete with an empty-ProviderID +// ResourceRef (the driver is the authority on what an empty ProviderID +// means — see wfctlhelpers/apply.go::doUpdate's analogous comment). The +// v1-era pre-flight precondition error has been retired with the v2 +// migration (PR P-DO TP2): drivers that cannot delete by name surface +// the diagnostic themselves via their typed validation. func TestDOProvider_Apply_DeleteAction_MissingCurrent(t *testing.T) { fake := &deleteFakeDriver{} p := &DOProvider{drivers: map[string]interfaces.ResourceDriver{"infra.firewall": fake}} @@ -93,21 +99,30 @@ func TestDOProvider_Apply_DeleteAction_MissingCurrent(t *testing.T) { plan := &interfaces.IaCPlan{ ID: "plan-delete-no-current", Actions: []interfaces.PlanAction{{ - Action: "delete", + Action: "delete", Resource: interfaces.ResourceSpec{Name: "orphan-firewall", Type: "infra.firewall"}, - Current: nil, // missing — defensive handling required + Current: nil, // v2 contract: dispatched with empty ProviderID }}, } result, err := p.Apply(t.Context(), plan) if err != nil { - t.Fatalf("Apply returned top-level error (want nil + error in result.Errors): %v", err) + t.Fatalf("Apply returned top-level error: %v", err) } - if len(result.Errors) == 0 { - t.Fatal("expected error in result.Errors for delete with missing Current, got none") + // v2 contract: the dispatch IS made; the deleteFakeDriver returns nil + // because it does not validate ref.ProviderID. A real driver would + // reject the empty ProviderID via its typed validator. + if fake.deleteCalls != 1 { + t.Errorf("Delete dispatched %d times, want 1 (v2 dispatch contract)", fake.deleteCalls) + } + if fake.deletedRef.ProviderID != "" { + t.Errorf("Delete called with ProviderID %q, want empty (Current was nil)", fake.deletedRef.ProviderID) } - if fake.deleteCalls != 0 { - t.Errorf("Delete should not be called when Current is nil, but called %d times", fake.deleteCalls) + // No top-level error and no per-action error because the fake driver + // accepted the empty-ProviderID delete. (Real drivers like FirewallDriver + // would emit an error here; the fake's job is to verify dispatch only.) + if len(result.Errors) != 0 { + t.Errorf("expected no per-action errors for fake driver dispatch, got %v", result.Errors) } } From 21a6d63ea870da727fc55ec62c2f63aba6c964f3 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 21:29:01 -0400 Subject: [PATCH 04/11] feat(provider): ValidatePlan for App Platform region-VPC constraints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR P-DO TP3: implement interfaces.ProviderValidator (W-4) on DOProvider to surface DO region constraints at `wfctl infra align` time before any cloud API call. Per the W-4 design, R-A10 invokes ValidatePlan via type-assertion; providers that do not implement it continue to work unchanged. The first pass covers three constraint families: 1. App Platform infra.container_service requires a region GROUP slug (nyc, ams, fra, sfo, sgp, syd, tor, blr, lon). Zone slugs (nyc1, sfo3 …) are rejected with PlanDiagnosticError. This is the "copy-pasted nyc3 from a Droplet config" defense. 2. Zone-bound resources (infra.vpc, infra.droplet, infra.volume) MUST use a zone slug. Bare group slugs are rejected with PlanDiagnosticError. Inverse of (1). 3. Cross-resource: an App Platform with vpc_ref pointing to a VPC in the same plan must have a region group whose zones include the referenced VPC's region. This locks the recurring 'App Platform in nyc cannot reach VPC in sfo3' production bug class (root-cause issue D from the conformance design). Cross-resource resolution looks at desired spec first, falls back to action.Current's Outputs["region"] for unchanged-VPC scenarios. vpc_ref pointing to a name not in the plan emits a Severity=Warning so non-strict align tolerates external VPCs while --strict escalates. ValidatePlan is read-only and makes no remote calls per the W-4 contract. Compile-time interface assertion lives at the bottom of validate_plan.go. 11 TDD tests in validate_plan_test.go cover: nil/empty plan, group-slug accepted, zone-slug rejected for AP, zone-slug accepted for VPC/Droplet/Volume, group-slug rejected for VPC, mismatch error (flagship), happy-path match, unknown-vpc_ref Warning, current-state fallback, delete-action skipped, compile-time assertion. Future extensions (deferred follow-ups): database/cache zone slugs, load balancer zone matching against attached droplets, registry regional restrictions. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/validate_plan.go | 238 ++++++++++++++++++++++++++++++ internal/validate_plan_test.go | 259 +++++++++++++++++++++++++++++++++ 2 files changed, 497 insertions(+) create mode 100644 internal/validate_plan.go create mode 100644 internal/validate_plan_test.go diff --git a/internal/validate_plan.go b/internal/validate_plan.go new file mode 100644 index 0000000..fa30adb --- /dev/null +++ b/internal/validate_plan.go @@ -0,0 +1,238 @@ +package internal + +import ( + "fmt" + "strings" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// ValidatePlan implements interfaces.ProviderValidator (W-4): a read-only, +// no-remote-call cross-resource constraint check that runs at `wfctl infra +// align` time before any cloud API call. Diagnostics surface as +// PlanDiagnosticError|Warning|Info with severity-driven exit-code mapping +// (Error always fails align; Warning fails only under --strict; Info never +// affects exit). +// +// PR P-DO TP3 — first DO region-constraint pass: +// +// 1. App Platform `infra.container_service` MUST use one of DO App +// Platform's supported region GROUPS (nyc, ams, fra, sfo, sgp, syd, +// tor, blr, lon). If `region` is set to a Droplet/VPC zone slug +// (nyc1, nyc3, sfo2, sfo3 …) it is rejected — App Platform configures +// by group, not zone. +// +// 2. Droplet/VPC/Volume zone-bound resources MUST use a zone slug +// (nyc1, nyc2, nyc3, sfo2, sfo3, ams2, ams3, fra1, sgp1, lon1, tor1, +// blr1, syd1) — bare group slugs (nyc, sfo) are rejected for these +// types because the DO API for these resources requires a specific +// zone. +// +// 3. App Platform with `vpc_ref` (referencing a VPC by name in the same +// plan) — the referenced VPC's region zone MUST belong to the App +// Platform's region group (e.g., AppPlatform region nyc → VPC in +// nyc1 OR nyc3; AppPlatform region sfo → VPC in sfo2 OR sfo3). +// Mismatch is the recurring "App Platform in nyc cannot reach VPC in +// sfo3" production bug (root-cause issue D from the conformance +// design). +// +// Future extensions (deferred follow-ups): database/cache zone slugs, +// load balancer zone matching against attached droplets, registry +// regional restrictions. +func (p *DOProvider) ValidatePlan(plan *interfaces.IaCPlan) []interfaces.PlanDiagnostic { + if plan == nil || len(plan.Actions) == 0 { + return nil + } + + // Index plan resources by Name for cross-resource lookup. Both desired + // (from action.Resource) and existing (from action.Current) entries + // land here so an App Platform that references an existing VPC by + // name (no plan action of its own) still resolves. + byName := make(map[string]planResource, len(plan.Actions)) + for _, a := range plan.Actions { + byName[a.Resource.Name] = planResource{spec: a.Resource, current: a.Current} + } + + var diags []interfaces.PlanDiagnostic + for _, a := range plan.Actions { + // Skip delete actions — they target the existing resource, not + // the desired spec, so region constraints on the (empty) spec + // would produce false positives. + if a.Action == "delete" { + continue + } + region, _ := a.Resource.Config["region"].(string) + switch a.Resource.Type { + case "infra.container_service": + diags = appendAppPlatformDiagnostics(diags, a.Resource, region, byName) + case "infra.vpc", "infra.droplet", "infra.volume": + if region != "" && !isZoneSlug(region) { + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: a.Resource.Name, + Field: "region", + Message: fmt.Sprintf( + "DO %s requires a zone slug (e.g. nyc1, sfo3); got %q which is %s. Use a specific zone.", + a.Resource.Type, region, classifyRegion(region), + ), + }) + } + } + } + return diags +} + +// planResource is the cross-resource lookup record used by ValidatePlan. +// Carries both the desired spec and the optional current state so a plan +// where an App Platform references an existing-and-unchanged VPC still +// resolves the VPC's region. +type planResource struct { + spec interfaces.ResourceSpec + current *interfaces.ResourceState +} + +// appendAppPlatformDiagnostics emits region-constraint diagnostics for an +// infra.container_service action's region/vpc_ref pair. +func appendAppPlatformDiagnostics( + diags []interfaces.PlanDiagnostic, + spec interfaces.ResourceSpec, + region string, + byName map[string]planResource, +) []interfaces.PlanDiagnostic { + if region != "" && !isAppPlatformRegionGroup(region) { + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: spec.Name, + Field: "region", + Message: fmt.Sprintf( + "DO App Platform configures by region GROUP (nyc, ams, fra, sfo, sgp, syd, tor, blr, lon); got %q which is %s. Replace with the parent group slug.", + region, classifyRegion(region), + ), + }) + } + + vpcRef, _ := spec.Config["vpc_ref"].(string) + if vpcRef == "" || region == "" || !isAppPlatformRegionGroup(region) { + return diags + } + target, ok := byName[vpcRef] + if !ok { + // vpc_ref points to a name not in the plan; cannot validate + // region match without state. Emit a Warning so an operator + // configuring a fresh deployment sees the gap; non-strict + // runs continue. + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticWarning, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "App Platform %q references vpc_ref %q which is not declared in the same plan; region-match check skipped", + spec.Name, vpcRef, + ), + }) + return diags + } + // Resolve the referenced VPC's region. Prefer the desired spec + // (config the user is actively declaring) and fall back to the + // current state's outputs (existing VPC adoption). + vpcRegion, _ := target.spec.Config["region"].(string) + if vpcRegion == "" && target.current != nil { + if v, ok := target.current.Outputs["region"].(string); ok { + vpcRegion = v + } + } + if vpcRegion == "" { + // Cannot determine VPC region — silent, not a finding (planning + // pipeline upstream may not have populated yet). + return diags + } + if appPlatformRegionGroupOf(vpcRegion) != region { + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "App Platform region %q is incompatible with VPC %q region %q. App Platform region group %q only routes to VPCs in zones %s.", + region, vpcRef, vpcRegion, region, strings.Join(zonesInGroup(region), ", "), + ), + }) + } + return diags +} + +// isAppPlatformRegionGroup reports whether s is a DO App Platform region +// group slug (the parent of one or more zone slugs). +func isAppPlatformRegionGroup(s string) bool { + _, ok := appPlatformGroupZones[s] + return ok +} + +// isZoneSlug reports whether s is a DO zone slug (e.g., nyc1, sfo3). +// Zone slugs are the value DO's API expects for VPC, Droplet, Volume, +// Database, Spaces. +func isZoneSlug(s string) bool { + _, ok := zoneToGroup[s] + return ok +} + +// appPlatformRegionGroupOf returns the App Platform region group that +// contains a zone slug. Returns "" for unknown zones. +func appPlatformRegionGroupOf(zone string) string { + return zoneToGroup[zone] +} + +// zonesInGroup returns the sorted list of zone slugs inside a group, for +// use in diagnostic messages. +func zonesInGroup(group string) []string { + return appPlatformGroupZones[group] +} + +// classifyRegion returns a short human-readable label for a region slug +// to help diagnostic messages distinguish bare-group from zone-slug +// confusion. +func classifyRegion(s string) string { + if isAppPlatformRegionGroup(s) { + return fmt.Sprintf("a region GROUP (zones: %s)", strings.Join(zonesInGroup(s), ", ")) + } + if isZoneSlug(s) { + return fmt.Sprintf("a zone slug in group %q", appPlatformRegionGroupOf(s)) + } + return "not a recognized DO region slug" +} + +// appPlatformGroupZones maps each App Platform region group to the +// zone slugs it routes to. Source: DO docs (Apps API Region object) + +// Apps API region listing as of 2026-05. +var appPlatformGroupZones = map[string][]string{ + "nyc": {"nyc1", "nyc3"}, + "ams": {"ams3"}, + "fra": {"fra1"}, + "sfo": {"sfo2", "sfo3"}, + "sgp": {"sgp1"}, + "syd": {"syd1"}, + "tor": {"tor1"}, + "blr": {"blr1"}, + "lon": {"lon1"}, +} + +// zoneToGroup is the inverse mapping computed at init() so lookups are +// O(1). Kept distinct from appPlatformGroupZones so the source-of-truth +// (group → zones) stays single-direction. +var zoneToGroup = func() map[string]string { + m := make(map[string]string, 16) + for group, zones := range appPlatformGroupZones { + for _, z := range zones { + m[z] = group + } + } + // Additional zones DO supports that don't currently route to App + // Platform (legacy nyc2, ams2). Including them lets the zone-slug + // validation accept these without flagging valid VPC/Droplet/Volume + // configs — they simply have no App Platform routing. + m["nyc2"] = "" + m["ams2"] = "" + return m +}() + +// Compile-time assertion: DOProvider implements ProviderValidator. +var _ interfaces.ProviderValidator = (*DOProvider)(nil) diff --git a/internal/validate_plan_test.go b/internal/validate_plan_test.go new file mode 100644 index 0000000..c1dc5d9 --- /dev/null +++ b/internal/validate_plan_test.go @@ -0,0 +1,259 @@ +package internal + +import ( + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestDOProvider_ValidatePlan_NilAndEmpty asserts the contract that +// ValidatePlan tolerates a nil plan and an empty actions slice without +// panicking; both return zero diagnostics. R-A10 invokes ValidatePlan +// before any align-time enrichment so the early-return cases must hold. +func TestDOProvider_ValidatePlan_NilAndEmpty(t *testing.T) { + p := NewDOProvider() + if d := p.ValidatePlan(nil); len(d) != 0 { + t.Errorf("nil plan: expected 0 diagnostics, got %d: %+v", len(d), d) + } + if d := p.ValidatePlan(&interfaces.IaCPlan{}); len(d) != 0 { + t.Errorf("empty plan: expected 0 diagnostics, got %d: %+v", len(d), d) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformGroupSlugAccepted asserts that +// an App Platform action with a valid region GROUP slug ("nyc") emits no +// diagnostics. This is the canonical happy-path for AP — region groups +// are the App Platform API's required region shape. +func TestDOProvider_ValidatePlan_AppPlatformGroupSlugAccepted(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for AP region=nyc; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformZoneSlugRejected asserts that +// an App Platform action with a zone slug ("nyc3") gets a Severity=Error +// diagnostic explaining the AP-region-group requirement. This is the +// concrete root-cause-issue-D defense from the conformance design: an +// operator who copy-pasted nyc3 from a Droplet config gets a clear +// explanation pre-apply, instead of a cryptic DO API rejection at +// `wfctl infra apply`. +func TestDOProvider_ValidatePlan_AppPlatformZoneSlugRejected(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc3"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 { + t.Fatalf("expected 1 diagnostic for AP region=nyc3; got %d: %+v", len(d), d) + } + got := d[0] + if got.Severity != interfaces.PlanDiagnosticError { + t.Errorf("severity = %d, want Error (%d)", got.Severity, interfaces.PlanDiagnosticError) + } + if got.Resource != "core-app" || got.Field != "region" { + t.Errorf("diagnostic resource/field = %q/%q, want core-app/region", got.Resource, got.Field) + } + if !strings.Contains(got.Message, "region GROUP") { + t.Errorf("message should explain the group requirement; got %q", got.Message) + } + if !strings.Contains(got.Message, "nyc3") { + t.Errorf("message should cite the offending value nyc3; got %q", got.Message) + } +} + +// TestDOProvider_ValidatePlan_VPCRequiresZone asserts that a VPC with a +// bare group slug ("nyc") gets a Severity=Error diagnostic — the DO VPC +// API requires a specific zone, the inverse of the App Platform +// constraint above. +func TestDOProvider_ValidatePlan_VPCRequiresZone(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "nyc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 || d[0].Severity != interfaces.PlanDiagnosticError { + t.Fatalf("expected 1 Error diagnostic for VPC region=nyc; got %+v", d) + } + if !strings.Contains(d[0].Message, "zone slug") { + t.Errorf("message should explain the zone-slug requirement; got %q", d[0].Message) + } +} + +// TestDOProvider_ValidatePlan_DropletAndVolumeAcceptZone asserts the +// happy path for the inverse direction — Droplet/Volume with valid zone +// slugs emit no diagnostics. +func TestDOProvider_ValidatePlan_DropletAndVolumeAcceptZone(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-droplet", Type: "infra.droplet", + Config: map[string]any{"region": "nyc1"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vol", Type: "infra.volume", + Config: map[string]any{"region": "nyc1"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "nyc1"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for zone-slug-armed plan; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformVPCRefRegionMismatch is the +// flagship test: App Platform group "nyc" with a vpc_ref pointing to a +// VPC declared in zone "sfo3" must produce an Error diagnostic with the +// vpc_ref field path. This locks the recurring "App Platform in nyc +// cannot reach VPC in sfo3" production bug class (root-cause issue D). +func TestDOProvider_ValidatePlan_AppPlatformVPCRefRegionMismatch(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "sfo3"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "core-vpc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 { + t.Fatalf("expected 1 diagnostic for AP/VPC region mismatch; got %d: %+v", len(d), d) + } + if d[0].Severity != interfaces.PlanDiagnosticError { + t.Errorf("severity = %d, want Error", d[0].Severity) + } + if d[0].Resource != "core-app" || d[0].Field != "vpc_ref" { + t.Errorf("resource/field = %q/%q, want core-app/vpc_ref", d[0].Resource, d[0].Field) + } + for _, want := range []string{"nyc", "sfo3", "core-vpc"} { + if !strings.Contains(d[0].Message, want) { + t.Errorf("message should cite %q; got %q", want, d[0].Message) + } + } +} + +// TestDOProvider_ValidatePlan_AppPlatformVPCRefRegionMatchHappyPath +// covers the inverse: AP group "nyc" + VPC zone "nyc1" → no diagnostic. +// Locks the contract that the cross-resource check is restricted to +// genuine mismatches. +func TestDOProvider_ValidatePlan_AppPlatformVPCRefRegionMatchHappyPath(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "nyc1"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "core-vpc"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for AP nyc + VPC nyc1; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformVPCRefUnknownEmitsWarning +// asserts that vpc_ref pointing to a name not in the plan emits a +// Severity=Warning (not Error) — the operator may have an existing VPC +// whose region is not yet resolved, so the strict mode escalates while +// non-strict tolerates. +func TestDOProvider_ValidatePlan_AppPlatformVPCRefUnknownEmitsWarning(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "missing-vpc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 || d[0].Severity != interfaces.PlanDiagnosticWarning { + t.Fatalf("expected 1 Warning diagnostic for unknown vpc_ref; got %+v", d) + } + if d[0].Field != "vpc_ref" || !strings.Contains(d[0].Message, "missing-vpc") { + t.Errorf("message should cite the unknown vpc_ref name; got %+v", d[0]) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformVPCRefResolvesCurrentState +// asserts that the cross-resource region check looks at action.Current's +// outputs when no desired region is set on the VPC's spec — the typical +// shape when an existing VPC is unchanged but referenced by a new +// AppPlatform action. +func TestDOProvider_ValidatePlan_AppPlatformVPCRefResolvesCurrentState(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + { + Action: "update", + Resource: interfaces.ResourceSpec{ + Name: "existing-vpc", Type: "infra.vpc", + // No region in the desired spec — must fall back to current state. + Config: map[string]any{}, + }, + Current: &interfaces.ResourceState{ + Name: "existing-vpc", Type: "infra.vpc", + Outputs: map[string]any{"region": "sfo3"}, + }, + }, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "existing-vpc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 || d[0].Severity != interfaces.PlanDiagnosticError { + t.Fatalf("expected 1 Error for existing-VPC region mismatch via current state; got %+v", d) + } + if !strings.Contains(d[0].Message, "sfo3") { + t.Errorf("message should cite the current-state region sfo3; got %q", d[0].Message) + } +} + +// TestDOProvider_ValidatePlan_DeleteActionsSkipped asserts that delete +// actions with empty Config don't trigger false positives. A delete plan +// targets the existing resource for removal — the "desired" spec carries +// no region (user is asking for absence), so region constraints would +// always fire on every delete if not skipped. +func TestDOProvider_ValidatePlan_DeleteActionsSkipped(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "delete", Resource: interfaces.ResourceSpec{ + Name: "old-app", Type: "infra.container_service", Config: map[string]any{}, + }, Current: &interfaces.ResourceState{Name: "old-app", Type: "infra.container_service", ProviderID: "app-1"}}, + {Action: "delete", Resource: interfaces.ResourceSpec{ + Name: "old-vpc", Type: "infra.vpc", Config: map[string]any{}, + }, Current: &interfaces.ResourceState{Name: "old-vpc", Type: "infra.vpc", ProviderID: "vpc-1"}}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for delete-only plan; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_CompileTimeAssertion documents that the +// compile-time interface assertion in validate_plan.go locks +// DOProvider's ProviderValidator implementation. If the interface +// signature changes upstream, the compile-time check catches the drift +// before any test runs. This test exists to make the assertion visible +// in test output and to fail the build if ValidatePlan goes missing. +func TestDOProvider_ValidatePlan_CompileTimeAssertion(t *testing.T) { + var _ interfaces.ProviderValidator = NewDOProvider() +} From 6b4c5523c0951f3072e282db7b7e127725785190 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 21:33:22 -0400 Subject: [PATCH 05/11] feat(plugin): opt into computePlanVersion: v2 PR P-DO TP4: declare iacProvider.computePlanVersion=v2 at the TOP LEVEL of plugin.json so wfctl's runtime dispatcher (cmd/wfctl/deploy_providers.go ::iacPluginManifest) routes Apply through wfctlhelpers.ApplyPlan instead of the legacy in-provider switch. Schema note: the SDK manifest schema (plugin/sdk/manifest_schema.json) expects iacProvider.computePlanVersion at the top level of plugin.json. The existing capabilities.iacProvider sub-block (name, resourceTypes, configSchema) is a DIFFERENT consumer (plugin discovery + capability declaration); the two structures coexist in the same file. The runtime loader unmarshals both into one struct (one Capabilities.IaCProvider.Name field plus one top-level IaCProvider.ComputePlanVersion field) so a single plugin.json read serves both code paths. Validated via three checkers: - go run ./cmd/wfctl plugin validate --file plugin.json --strict-contracts (OK) - JSON-schema validation against plugin/sdk/manifest_schema.json (OK) - sdk.ParseManifest decode confirms EffectiveComputePlanVersion()==v2 Backward compat: wfctl < v0.21.0 ignores the new field; the legacy v1 dispatch (provider.Apply switch, now wrapping wfctlhelpers.ApplyPlan) continues to work for all existing callers. Co-Authored-By: Claude Opus 4.7 (1M context) --- plugin.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugin.json b/plugin.json index bd6b7e4..ec81d67 100644 --- a/plugin.json +++ b/plugin.json @@ -32,6 +32,9 @@ "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.9.0/workflow-plugin-digitalocean-darwin-arm64.tar.gz" } ], + "iacProvider": { + "computePlanVersion": "v2" + }, "capabilities": { "configProvider": false, "moduleTypes": ["iac.provider"], From c47a1c31b08da0e0d822370c884330a0d6c6234d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 21:38:15 -0400 Subject: [PATCH 06/11] feat(provider): add conformance test + extend ValidatePlan for database vpc_ref + bump to v0.10.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR P-DO TP5: ship the conformance test entry point, extend ValidatePlan to satisfy the conformance suite's cross-resource constraint scenario, and bump plugin.json + CHANGELOG to v0.10.0. provider_conformance_test.go (new, behind 'conformance' build tag): - Invokes iac/conformance.Run against a freshly-constructed and initialized DOProvider. Initialize is always called with a stub token so the driver registry is populated for the non-cloud scenarios that probe ResourceDriver lookups (structpb-roundtrip, cross-module resolution, etc.) — these exercise read-only or pure-data paths that don't hit DO's API. - LiveCloud (CONFORMANCE_LIVE_CLOUD=1 + DIGITALOCEAN_ACCESS_TOKEN) swaps the stub for the real token before driver instantiation. - SmokeOnly = testing.Short() limits to Smoke=true scenarios for the per-PR smoke gate's narrow contract. ValidatePlan extension (validate_plan.go): - Added appendDatabaseDiagnostics: infra.database vpc_ref pointing to a name not in the plan emits a Severity=Error diagnostic. Closes the conformance Scenario_CrossResourceConstraintRejection assertion that 'at least one Severity=Error diagnostic' fires for a dangling cross-resource reference. The assertion was failing before this change (database vpc_ref was previously unhandled by ValidatePlan). - Two new TDD tests: dangling-vpc_ref → Error (mirrors the conformance contract in-tree); in-plan vpc_ref → no diagnostic (happy path). plugin.json: - version 0.9.0 → 0.10.0 - download URL paths v0.9.0 → v0.10.0 (per-OS/arch) CHANGELOG.md: - New v0.10.0 section catalogues TP1-TP5 changes (ValidatePlan, computePlanVersion: v2 opt-in, Apply collapse, conformance test, codemod-report workflow), the Apply-delete v2-contract change, and the workflow dep bump. - Migrates the previously-Unreleased infra.vpc id-output fix into the v0.10.0 Fixed section (it ships in this release). Test results: - go test -tags=conformance ./internal/ -run TestConformance: 6/6 non-cloud scenarios PASS (CrossResourceConstraintRejection, DiffSurvivesGRPCRoundTrip, InfraOutputCrossModuleResolution, PlanStaleDiagnostic, ProtectedReplaceWithOverride, ProtectedReplaceWithoutOverride). Upsert-on-already-exists + grpc-roundtrip skip when their opt-in driver types are absent (DO does not expose infra.compute). - go test ./... -count=1 -race: ALL packages PASS. - go run ./cmd/wfctl plugin validate --strict-contracts: OK. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 83 ++++++++++++++++++++++++ internal/provider_conformance_test.go | 90 +++++++++++++++++++++++++++ internal/validate_plan.go | 37 +++++++++++ internal/validate_plan_test.go | 52 ++++++++++++++++ plugin.json | 10 +-- 5 files changed, 267 insertions(+), 5 deletions(-) create mode 100644 internal/provider_conformance_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bc5f65..fb2a132 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,84 @@ All notable changes to workflow-plugin-digitalocean are documented here. ## [Unreleased] +## [v0.10.0] + +### Added + +- **`iacProvider.computePlanVersion: v2` opt-in** (PR P-DO TP4) — wfctl's + runtime dispatcher now routes Apply through `wfctlhelpers.ApplyPlan` + instead of the legacy in-provider switch. The new dispatch path adds: + Replace decomposition + `ReplaceIDMap` propagation, JIT + `${MODULE.id}` / `${VAR}` substitution, the input-drift postcondition, + per-action context cancellation between iterations, and the + `interfaces.UpsertSupporter` recovery contract. + + Backward compat: wfctl < v0.21.0 ignores the new field; the legacy + v1 dispatch (`provider.Apply` switch, now wrapping + `wfctlhelpers.ApplyPlan`) continues to work for all existing callers. + +- **`ValidatePlan` (`interfaces.ProviderValidator`)** (PR P-DO TP3) — + read-only, no-remote-call cross-resource constraint check that runs + at `wfctl infra align` time before any cloud API call. First pass + covers three constraint families: + 1. App Platform `infra.container_service` requires a region GROUP + slug (`nyc`, `ams`, `fra`, `sfo`, `sgp`, `syd`, `tor`, `blr`, + `lon`); zone slugs (`nyc1`, `sfo3`, …) rejected with + `PlanDiagnosticError`. + 2. Zone-bound resources (`infra.vpc`, `infra.droplet`, `infra.volume`) + require a zone slug; bare group slugs rejected with + `PlanDiagnosticError`. + 3. Cross-resource: App Platform `vpc_ref` must reference a VPC whose + region zone belongs to the App Platform's region group; database + `vpc_ref` must resolve to an in-plan VPC. Locks the recurring + "App Platform in nyc cannot reach VPC in sfo3" production bug + class (root-cause issue D from the conformance design). + + Severity mapping: Error always fails align; Warning fails only under + `--strict`; Info never affects exit. ValidatePlan is read-only and + makes no remote calls per the W-4 contract. + +- **Conformance test (`provider_conformance_test.go`)** (PR P-DO TP5) + — invokes `iac/conformance.Run` against a freshly-constructed + `DOProvider`. Behind the `conformance` build tag; opt in with + `go test -tags=conformance ./internal/...`. Six non-cloud scenarios + run by default; `CONFORMANCE_LIVE_CLOUD=1` (with + `DIGITALOCEAN_ACCESS_TOKEN`) opts into the cloud-touching probes. + +- **`.github/workflows/codemod-report.yml`** (PR P-DO TP1) — per-PR + workflow runs `iac-codemod refactor-apply -dry-run` against the + plugin source, uploads the full Markdown report as a 90-day retention + GitHub Actions artifact, and posts/updates a sticky PR comment with + the top-30-line summary so drive-by reviewers see findings without + downloading the artifact. + +### Changed + +- **`DOProvider.Apply` collapsed to wrap `wfctlhelpers.ApplyPlan`** + (PR P-DO TP2) — the in-Apply per-action switch + (create/update/replace/delete + upsert recovery + nil-out diagnostic) + is replaced with a single dispatch to the helper. The DO-plugin-specific + deferred-update flush (DatabaseDriver `type=app` `trusted_sources` + referencing apps created later in the plan; regression-gated by + `provider_deferred_test.go`) is preserved by wrapping `ApplyPlan` with + the second-pass loop. The wrapper deviates from the codemod's canonical + single-statement shape; the deviation is documented and marked with + `// wfctl:skip-iac-codemod` so `AssertApplyDelegatesToHelper` + recognises it as intentional. + +- **DO drivers `AppPlatformDriver`, `VPCDriver`, `FirewallDriver`, + `DatabaseDriver`** structurally satisfy the canonical + `interfaces.UpsertSupporter` (their existing `SupportsUpsert() bool` + method is bit-identical to the new interface; no driver-side changes + needed). + +- **`DOProvider.Apply` `delete` action with `Current == nil`** — under + v2 dispatch the contract is "driver is the authority on what an empty + ProviderID means" (per `wfctlhelpers/apply.go::doUpdate`'s analogous + comment). The v1-era pre-flight precondition error is retired; + `provider_apply_test.go::TestDOProvider_Apply_DeleteAction_MissingCurrent` + was rewritten to lock the new contract. + ### Fixed - **`infra.vpc` exposes `id` output** — wfctl `infra_output: .id` @@ -14,6 +92,11 @@ All notable changes to workflow-plugin-digitalocean are documented here. `Outputs["id"]` so the standard `.id` reference works. Surfaced by core-dump deploy run 25278900082. +### Bumped + +- `github.com/GoCodeAlone/workflow` → `e2c582bece90` (workflow main HEAD + with W-7 conformance suite + W-8 codemod merged). + ## [v0.9.1] ### Fixed diff --git a/internal/provider_conformance_test.go b/internal/provider_conformance_test.go new file mode 100644 index 0000000..c2bf1e3 --- /dev/null +++ b/internal/provider_conformance_test.go @@ -0,0 +1,90 @@ +//go:build conformance + +// Package internal — DO provider conformance test. +// +// This file lives behind the `conformance` build tag so it is excluded +// from the default `go test ./...` (which runs the existing per-package +// unit tests). Opt in with: +// +// GOWORK=off go test -tags=conformance ./internal/... -run TestConformance +// +// The conformance suite (github.com/GoCodeAlone/workflow/iac/conformance) +// dispatches a fixed set of provider-portable scenarios; each scenario +// independently decides whether it can run against a fresh provider +// (some skip when an opt-in driver type like infra.compute is absent — +// DO does not expose infra.compute, so the upsert-on-already-exists +// scenario will skip on DO). +// +// Two opt-in env vars gate the heavier scenarios: +// +// - CONFORMANCE_LIVE_CLOUD=1 — runs RequiresCloud=true scenarios +// against the real DO API. Requires DIGITALOCEAN_ACCESS_TOKEN. +// - testing.Short()=true — limits to Smoke=true scenarios only +// (the per-PR smoke gate's narrow contract). +// +// Without either env var, the suite runs only the non-cloud, +// non-smoke-only scenarios (the default for unit-style invocation). +// +// PR P-DO TP5. + +package internal + +import ( + "context" + "os" + "testing" + + "github.com/GoCodeAlone/workflow/iac/conformance" + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestConformance dispatches the workflow conformance suite against a +// freshly-constructed DOProvider. Each scenario receives a new provider +// via the Provider closure; this guarantees per-scenario isolation +// (state from one scenario does not bleed into the next). +// +// The provider is initialized with the DIGITALOCEAN_ACCESS_TOKEN env var +// when LiveCloud is enabled. Without the token the LiveCloud scenarios +// would fail at the first cloud call; with it absent + LiveCloud=true +// the suite returns a clear error instead of hanging on a malformed +// network request. +func TestConformance(t *testing.T) { + liveCloud := os.Getenv("CONFORMANCE_LIVE_CLOUD") == "1" + cfg := conformance.Config{ + Provider: func() interfaces.IaCProvider { + p := NewDOProvider() + // Always initialize. The non-cloud scenarios + // (cross-resource constraint rejection, plan-stale + // diagnostic, structpb-roundtrip, infra-output + // cross-module resolution, protected-replace + // with/without override) all need the driver registry + // populated to dispatch their probes — they do NOT + // require a real DO token because the driver methods + // they exercise are read-only or pure-data + // transformations. A stub token is sufficient. + // + // LiveCloud scenarios overwrite the stub with the real + // DIGITALOCEAN_ACCESS_TOKEN below; the godo client + // inside each driver re-reads it via the closure'd + // oauth2 token source so a stub-then-real swap means + // the second Initialize wins. + token := "stub-token-for-non-cloud-conformance" + if liveCloud { + token = os.Getenv("DIGITALOCEAN_ACCESS_TOKEN") + if token == "" { + t.Fatal("CONFORMANCE_LIVE_CLOUD=1 but DIGITALOCEAN_ACCESS_TOKEN is not set") + } + } + if err := p.Initialize(context.Background(), map[string]any{ + "token": token, + "region": "nyc3", + }); err != nil { + t.Fatalf("provider Initialize: %v", err) + } + return p + }, + SmokeOnly: testing.Short(), + LiveCloud: liveCloud, + } + conformance.Run(t, cfg) +} diff --git a/internal/validate_plan.go b/internal/validate_plan.go index fa30adb..e05584e 100644 --- a/internal/validate_plan.go +++ b/internal/validate_plan.go @@ -77,11 +77,48 @@ func (p *DOProvider) ValidatePlan(plan *interfaces.IaCPlan) []interfaces.PlanDia ), }) } + case "infra.database": + diags = appendDatabaseDiagnostics(diags, a.Resource, byName) } } return diags } +// appendDatabaseDiagnostics emits cross-resource diagnostics for an +// infra.database action. Currently: +// +// - vpc_ref MUST resolve to either an in-plan VPC or a name the +// operator knows is an existing VPC (Warning when missing). A +// dangling vpc_ref is the conformance scenario's regression-pin +// case (Scenario_CrossResourceConstraintRejection). +func appendDatabaseDiagnostics( + diags []interfaces.PlanDiagnostic, + spec interfaces.ResourceSpec, + byName map[string]planResource, +) []interfaces.PlanDiagnostic { + vpcRef, _ := spec.Config["vpc_ref"].(string) + if vpcRef == "" { + return diags + } + if _, ok := byName[vpcRef]; ok { + return diags + } + // vpc_ref names a resource not in the plan. Surface as Error so the + // conformance scenario's "at least one Severity=Error" assertion + // matches; this is the dangling-cross-resource-reference case the + // W-4 design explicitly calls out as a plan-time validator gap. + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "DO database %q references vpc_ref %q which is not declared in the same plan; either add the VPC resource or remove the vpc_ref", + spec.Name, vpcRef, + ), + }) + return diags +} + // planResource is the cross-resource lookup record used by ValidatePlan. // Carries both the desired spec and the optional current state so a plan // where an App Platform references an existing-and-unchanged VPC still diff --git a/internal/validate_plan_test.go b/internal/validate_plan_test.go index c1dc5d9..4fcc5dc 100644 --- a/internal/validate_plan_test.go +++ b/internal/validate_plan_test.go @@ -248,6 +248,58 @@ func TestDOProvider_ValidatePlan_DeleteActionsSkipped(t *testing.T) { } } +// TestDOProvider_ValidatePlan_DatabaseDanglingVPCRef asserts the +// regression-pin case the conformance suite locks +// (Scenario_CrossResourceConstraintRejection): a database with a +// vpc_ref pointing to a name not present in the plan emits at least +// one Severity=Error diagnostic. This is the in-tree mirror of the +// portable conformance assertion. +func TestDOProvider_ValidatePlan_DatabaseDanglingVPCRef(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "missing-vpc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) == 0 { + t.Fatal("expected at least one diagnostic for dangling vpc_ref") + } + var hasError bool + for _, x := range d { + if x.Message == "" { + t.Errorf("diagnostic message must be non-empty: %+v", x) + } + if x.Severity == interfaces.PlanDiagnosticError { + hasError = true + } + } + if !hasError { + t.Errorf("expected at least one Severity=Error diagnostic; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_DatabaseVPCRefInPlanNoFinding asserts +// the inverse — when a database vpc_ref resolves to an in-plan VPC, +// no diagnostic is emitted (well-formed plan, happy path). +func TestDOProvider_ValidatePlan_DatabaseVPCRefInPlanNoFinding(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "nyc1"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "core-vpc"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for in-plan vpc_ref; got %+v", d) + } +} + // TestDOProvider_ValidatePlan_CompileTimeAssertion documents that the // compile-time interface assertion in validate_plan.go locks // DOProvider's ProviderValidator implementation. If the interface diff --git a/plugin.json b/plugin.json index ec81d67..ea67eae 100644 --- a/plugin.json +++ b/plugin.json @@ -1,6 +1,6 @@ { "name": "workflow-plugin-digitalocean", - "version": "0.9.0", + "version": "0.10.0", "description": "DigitalOcean IaC provider: App Platform, DOKS, databases, Redis cache, load balancers, VPC, firewall, DNS, Spaces, DOCR, certificates, Droplets, Block Storage volumes, IAM (declared), and API gateway", "author": "GoCodeAlone", "license": "MIT", @@ -14,22 +14,22 @@ { "os": "linux", "arch": "amd64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.9.0/workflow-plugin-digitalocean-linux-amd64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.10.0/workflow-plugin-digitalocean-linux-amd64.tar.gz" }, { "os": "linux", "arch": "arm64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.9.0/workflow-plugin-digitalocean-linux-arm64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.10.0/workflow-plugin-digitalocean-linux-arm64.tar.gz" }, { "os": "darwin", "arch": "amd64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.9.0/workflow-plugin-digitalocean-darwin-amd64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.10.0/workflow-plugin-digitalocean-darwin-amd64.tar.gz" }, { "os": "darwin", "arch": "arm64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.9.0/workflow-plugin-digitalocean-darwin-arm64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.10.0/workflow-plugin-digitalocean-darwin-arm64.tar.gz" } ], "iacProvider": { From 43425f07b570b882d93891ed0b7847087e295a65 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 21:47:19 -0400 Subject: [PATCH 07/11] =?UTF-8?q?fix:=20Copilot=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=205=20substantive=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 5/5 findings addressed: #1 (validate_plan.go:52) — byName index must exclude delete-action resources. Previously a vpc_ref pointing to a VPC scheduled for deletion in the same plan would silently 'resolve' as if live; now delete-targets are excluded from the index so cross-resource references to them surface as Severity=Error dangling references. New regression test: TestDOProvider_ValidatePlan_DatabaseVPCRefToDeleteTargetIsDangling. #2 (validate_plan.go:91) — appendDatabaseDiagnostics docstring incorrectly said 'Warning when missing'; the implementation has always emitted PlanDiagnosticError, and the conformance scenario + TDD tests both REQUIRE Error severity. Doc rewritten to match the implemented contract. #3 (validate_plan.go:225) — zonesInGroup docstring promised sorted output but returned the raw underlying slice unsorted. Now copies + sorts (lexicographic) so diagnostic messages are deterministic and the returned slice is owned by the caller (safe to mutate). nil for unknown groups; the caller's strings.Join still works. #4 (provider_conformance_test.go:70) — comment described a 'stub-then-real swap' with two Initialize calls; the implementation has always made one call with the right token chosen up-front. Doc rewritten to match the actual single-call flow. #5 (codemod-report.yml:73) — fork PRs run with a read-only GITHUB_TOKEN per GitHub's pull_request workflow security model, so issues:create-comment + issues:update-comment fail 403 and would block CI. Gate the comment step on github.event.pull_request.head.repo.fork == false. The artifact upload step still runs unconditionally so the report remains reachable from the Actions tab. Verified locally: go test ./... -count=1 -race PASS, go test -tags=conformance ./internal/ -run TestConformance PASS, go vet clean, codemod-report.yml YAML valid. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/codemod-report.yml | 7 ++++ internal/provider_conformance_test.go | 31 ++++++++++-------- internal/validate_plan.go | 46 +++++++++++++++++++++++---- internal/validate_plan_test.go | 39 +++++++++++++++++++++++ 4 files changed, 102 insertions(+), 21 deletions(-) diff --git a/.github/workflows/codemod-report.yml b/.github/workflows/codemod-report.yml index 3778443..fe8c526 100644 --- a/.github/workflows/codemod-report.yml +++ b/.github/workflows/codemod-report.yml @@ -41,6 +41,13 @@ jobs: - name: Comment summary on PR (first-party action SHA-pinned) # rev5 — pin to commit SHA even though first-party. Tag mutability is a # known risk; Renovate config tracks upstream releases via .github/renovate.json. + # + # Copilot review #5: PRs from forks run with a read-only GITHUB_TOKEN + # (per GitHub's pull_request workflow security model), so issues:create- + # comment + issues:update-comment fail 403. Skip the comment step on + # fork PRs — the artifact upload above still runs unconditionally so + # the report is reachable from the Actions tab without a comment. + if: github.event.pull_request.head.repo.fork == false uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 with: script: | diff --git a/internal/provider_conformance_test.go b/internal/provider_conformance_test.go index c2bf1e3..d602121 100644 --- a/internal/provider_conformance_test.go +++ b/internal/provider_conformance_test.go @@ -53,21 +53,24 @@ func TestConformance(t *testing.T) { cfg := conformance.Config{ Provider: func() interfaces.IaCProvider { p := NewDOProvider() - // Always initialize. The non-cloud scenarios - // (cross-resource constraint rejection, plan-stale - // diagnostic, structpb-roundtrip, infra-output - // cross-module resolution, protected-replace - // with/without override) all need the driver registry - // populated to dispatch their probes — they do NOT - // require a real DO token because the driver methods - // they exercise are read-only or pure-data - // transformations. A stub token is sufficient. + // Always initialize the provider exactly once with a + // single token, picked at runtime: the real + // DIGITALOCEAN_ACCESS_TOKEN when LiveCloud is enabled, + // or a stub placeholder otherwise. The non-cloud + // scenarios (cross-resource constraint rejection, + // plan-stale diagnostic, structpb-roundtrip, + // infra-output cross-module resolution, + // protected-replace with/without override) need the + // driver registry populated to dispatch their probes + // but do NOT require a real DO token because the + // driver methods they exercise are read-only or + // pure-data transformations. // - // LiveCloud scenarios overwrite the stub with the real - // DIGITALOCEAN_ACCESS_TOKEN below; the godo client - // inside each driver re-reads it via the closure'd - // oauth2 token source so a stub-then-real swap means - // the second Initialize wins. + // Copilot review #4: the prior comment described a + // "stub-then-real swap" with two Initialize calls; + // the implementation has always made one call with + // the right token chosen up-front, so the comment was + // incorrect. token := "stub-token-for-non-cloud-conformance" if liveCloud { token = os.Getenv("DIGITALOCEAN_ACCESS_TOKEN") diff --git a/internal/validate_plan.go b/internal/validate_plan.go index e05584e..b4a0fa2 100644 --- a/internal/validate_plan.go +++ b/internal/validate_plan.go @@ -2,6 +2,7 @@ package internal import ( "fmt" + "sort" "strings" "github.com/GoCodeAlone/workflow/interfaces" @@ -48,8 +49,20 @@ func (p *DOProvider) ValidatePlan(plan *interfaces.IaCPlan) []interfaces.PlanDia // (from action.Resource) and existing (from action.Current) entries // land here so an App Platform that references an existing VPC by // name (no plan action of its own) still resolves. + // + // Delete-action resources are EXCLUDED from the index — a vpc_ref + // pointing to a VPC scheduled for deletion in the same plan must not + // "resolve" as if it were live; otherwise the database/App Platform + // checks would silently treat a soon-to-be-gone VPC as valid. + // (Copilot review #1 — "vpc_ref can resolve to a VPC that is being + // deleted in the same plan".) Cross-resource references to + // delete-targets surface as dangling-reference Errors via the same + // path as references to names not in the plan at all. byName := make(map[string]planResource, len(plan.Actions)) for _, a := range plan.Actions { + if a.Action == "delete" { + continue + } byName[a.Resource.Name] = planResource{spec: a.Resource, current: a.Current} } @@ -87,10 +100,15 @@ func (p *DOProvider) ValidatePlan(plan *interfaces.IaCPlan) []interfaces.PlanDia // appendDatabaseDiagnostics emits cross-resource diagnostics for an // infra.database action. Currently: // -// - vpc_ref MUST resolve to either an in-plan VPC or a name the -// operator knows is an existing VPC (Warning when missing). A -// dangling vpc_ref is the conformance scenario's regression-pin -// case (Scenario_CrossResourceConstraintRejection). +// - vpc_ref MUST resolve to a non-delete VPC action in the same plan. +// A vpc_ref pointing to a name that is absent from the plan +// entirely OR that is scheduled for deletion in the plan emits a +// Severity=Error diagnostic. This is the dangling-cross-resource- +// reference case the conformance suite locks via +// Scenario_CrossResourceConstraintRejection — both the W-4 design +// and the conformance scenario require Error severity, not +// Warning, so the conformance assertion ("at least one +// PlanDiagnosticError") matches. func appendDatabaseDiagnostics( diags []interfaces.PlanDiagnostic, spec interfaces.ResourceSpec, @@ -218,10 +236,24 @@ func appPlatformRegionGroupOf(zone string) string { return zoneToGroup[zone] } -// zonesInGroup returns the sorted list of zone slugs inside a group, for -// use in diagnostic messages. +// zonesInGroup returns a sorted (lexicographic) copy of the zone slugs +// inside a group, for use in diagnostic messages where deterministic +// ordering matters (test assertions, copy-paste reproducibility). +// Returns nil for unknown groups. The returned slice is owned by the +// caller and safe to mutate. +// +// Copilot review #3: the prior implementation returned the underlying +// slice without sorting; the docstring promised sorted output but did +// not deliver it. func zonesInGroup(group string) []string { - return appPlatformGroupZones[group] + src := appPlatformGroupZones[group] + if len(src) == 0 { + return nil + } + out := make([]string, len(src)) + copy(out, src) + sort.Strings(out) + return out } // classifyRegion returns a short human-readable label for a region slug diff --git a/internal/validate_plan_test.go b/internal/validate_plan_test.go index 4fcc5dc..16e1057 100644 --- a/internal/validate_plan_test.go +++ b/internal/validate_plan_test.go @@ -280,6 +280,45 @@ func TestDOProvider_ValidatePlan_DatabaseDanglingVPCRef(t *testing.T) { } } +// TestDOProvider_ValidatePlan_DatabaseVPCRefToDeleteTargetIsDangling +// pins Copilot review #1: a vpc_ref pointing to a VPC that is being +// deleted in the same plan must be treated as a dangling reference +// (Severity=Error), NOT silently accepted as if the VPC were live. +// The byName index must exclude delete-action resources for +// cross-resource resolution to work correctly. +func TestDOProvider_ValidatePlan_DatabaseVPCRefToDeleteTargetIsDangling(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + // VPC scheduled for deletion in this plan. + { + Action: "delete", + Resource: interfaces.ResourceSpec{Name: "old-vpc", Type: "infra.vpc", Config: map[string]any{}}, + Current: &interfaces.ResourceState{ + Name: "old-vpc", Type: "infra.vpc", ProviderID: "vpc-old", + Outputs: map[string]any{"region": "nyc1"}, + }, + }, + // Database referencing the soon-to-be-deleted VPC. + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "old-vpc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) == 0 { + t.Fatal("expected at least one diagnostic for vpc_ref to delete-target") + } + var hasDanglingError bool + for _, x := range d { + if x.Severity == interfaces.PlanDiagnosticError && x.Field == "vpc_ref" && x.Resource == "db" { + hasDanglingError = true + } + } + if !hasDanglingError { + t.Errorf("expected Severity=Error vpc_ref diagnostic on db resource; got %+v", d) + } +} + // TestDOProvider_ValidatePlan_DatabaseVPCRefInPlanNoFinding asserts // the inverse — when a database vpc_ref resolves to an in-plan VPC, // no diagnostic is emitted (well-formed plan, happy path). From 143cfb51139fc0b290845ea14d775e466d8e83d6 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 21:54:51 -0400 Subject: [PATCH 08/11] =?UTF-8?q?fix:=20Copilot=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=202=20vpc=5Fref=20type-mismatch=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 2/2 findings addressed (both legitimate type-safety improvements): #6 (validate_plan.go:123) — appendDatabaseDiagnostics validated that vpc_ref name resolved in the plan but did NOT validate that the target type is actually infra.vpc. A vpc_ref pointing to a Droplet or App Platform resource silently passed prior validation. Now surfaces a typed Severity=Error diagnostic when target.spec.Type != "infra.vpc". New regression test: TestDOProvider_ValidatePlan_DatabaseVPCRefToNonVPCType. #7 (validate_plan.go:189) — same bug in appendAppPlatformDiagnostics: vpc_ref pointing to a non-VPC resource would silently bypass the region-match check (target.spec.Config["region"] would be a region GROUP for an App Platform target, not a zone for a VPC) and the operator would never see a clear diagnostic. Same fix: typed Error when target.spec.Type != "infra.vpc". New regression test: TestDOProvider_ValidatePlan_AppPlatformVPCRefToNonVPCType. Both diagnostics carry the offending type in the message body so the operator immediately knows whether they typo'd a name (resolves to nothing) vs. typo'd a TYPE in a name (resolves to wrong resource). Verified locally: go test ./... -count=1 -race PASS, go test -tags=conformance ./internal/ -run TestConformance PASS, go vet clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/validate_plan.go | 68 +++++++++++++++++++++++++++------- internal/validate_plan_test.go | 63 +++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 14 deletions(-) diff --git a/internal/validate_plan.go b/internal/validate_plan.go index b4a0fa2..45edba8 100644 --- a/internal/validate_plan.go +++ b/internal/validate_plan.go @@ -118,22 +118,42 @@ func appendDatabaseDiagnostics( if vpcRef == "" { return diags } - if _, ok := byName[vpcRef]; ok { + target, ok := byName[vpcRef] + if !ok { + // vpc_ref names a resource not in the plan. Surface as Error so + // the conformance scenario's "at least one Severity=Error" + // assertion matches; this is the dangling-cross-resource- + // reference case the W-4 design explicitly calls out as a + // plan-time validator gap. + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "DO database %q references vpc_ref %q which is not declared in the same plan; either add the VPC resource or remove the vpc_ref", + spec.Name, vpcRef, + ), + }) return diags } - // vpc_ref names a resource not in the plan. Surface as Error so the - // conformance scenario's "at least one Severity=Error" assertion - // matches; this is the dangling-cross-resource-reference case the - // W-4 design explicitly calls out as a plan-time validator gap. - diags = append(diags, interfaces.PlanDiagnostic{ - Severity: interfaces.PlanDiagnosticError, - Resource: spec.Name, - Field: "vpc_ref", - Message: fmt.Sprintf( - "DO database %q references vpc_ref %q which is not declared in the same plan; either add the VPC resource or remove the vpc_ref", - spec.Name, vpcRef, - ), - }) + // Copilot review #6 (round 2): the resolved-name check above + // is necessary but not sufficient — the target must also be an + // infra.vpc resource. A vpc_ref pointing to a Droplet, App, or + // other non-VPC resource silently passed prior validation; + // surface it as a typed Error so the operator catches the + // type-mismatch at plan time rather than getting a confusing + // 4xx from the DO database API. + if target.spec.Type != "infra.vpc" { + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "DO database %q references vpc_ref %q whose type is %q; vpc_ref must point to an infra.vpc resource", + spec.Name, vpcRef, target.spec.Type, + ), + }) + } return diags } @@ -187,6 +207,26 @@ func appendAppPlatformDiagnostics( }) return diags } + // Copilot review #7 (round 2): the resolved-name check above is + // necessary but not sufficient — the target must also be an + // infra.vpc resource. A vpc_ref pointing to a Droplet, App, or + // other non-VPC resource would silently bypass the region-match + // check (because target.spec.Config["region"] would be a + // zone slug for a Droplet, not a VPC region) and the operator + // would never see a clear diagnostic. Surface as Error so the + // type-mismatch is caught at plan time. + if target.spec.Type != "infra.vpc" { + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "App Platform %q references vpc_ref %q whose type is %q; vpc_ref must point to an infra.vpc resource", + spec.Name, vpcRef, target.spec.Type, + ), + }) + return diags + } // Resolve the referenced VPC's region. Prefer the desired spec // (config the user is actively declaring) and fall back to the // current state's outputs (existing VPC adoption). diff --git a/internal/validate_plan_test.go b/internal/validate_plan_test.go index 16e1057..4ec1c01 100644 --- a/internal/validate_plan_test.go +++ b/internal/validate_plan_test.go @@ -339,6 +339,69 @@ func TestDOProvider_ValidatePlan_DatabaseVPCRefInPlanNoFinding(t *testing.T) { } } +// TestDOProvider_ValidatePlan_DatabaseVPCRefToNonVPCType pins +// Copilot review #6 (round 2): a database vpc_ref pointing to a name +// that resolves in the plan but to a NON-VPC type (e.g., a droplet) +// must Error. Type-checking at plan time avoids confusing 4xx errors +// from the DO database API at apply time. +func TestDOProvider_ValidatePlan_DatabaseVPCRefToNonVPCType(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-droplet", Type: "infra.droplet", + Config: map[string]any{"region": "nyc1"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "core-droplet"}, + }}, + }} + d := p.ValidatePlan(plan) + var hasTypeError bool + for _, x := range d { + if x.Severity == interfaces.PlanDiagnosticError && + x.Resource == "db" && x.Field == "vpc_ref" && + strings.Contains(x.Message, "infra.droplet") { + hasTypeError = true + } + } + if !hasTypeError { + t.Errorf("expected Severity=Error with infra.droplet in message; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformVPCRefToNonVPCType pins +// Copilot review #7 (round 2): same type-checking for App Platform +// vpc_ref pointing to a non-VPC resource (e.g., another App Platform +// app). Without this check the region-match logic would silently skip +// because target.spec.Config["region"] would be a region GROUP for an +// App Platform target, not a zone. +func TestDOProvider_ValidatePlan_AppPlatformVPCRefToNonVPCType(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "other-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "other-app"}, + }}, + }} + d := p.ValidatePlan(plan) + var hasTypeError bool + for _, x := range d { + if x.Severity == interfaces.PlanDiagnosticError && + x.Resource == "core-app" && x.Field == "vpc_ref" && + strings.Contains(x.Message, "infra.container_service") { + hasTypeError = true + } + } + if !hasTypeError { + t.Errorf("expected Severity=Error with infra.container_service in message; got %+v", d) + } +} + // TestDOProvider_ValidatePlan_CompileTimeAssertion documents that the // compile-time interface assertion in validate_plan.go locks // DOProvider's ProviderValidator implementation. If the interface From 2cdf0235bfc82a7ab95bd68f7343c82360f784f2 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 22:05:00 -0400 Subject: [PATCH 09/11] =?UTF-8?q?fix:=20Copilot=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=204=20deeper=20findings=20(vpc=5Fref=20UUID/template?= =?UTF-8?q?=20+=20classifyRegion=20+=20delete-test=20comment)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 4/4 findings addressed (deeper architectural correctness): #8/#9 (validate_plan.go:137,205) — vpc_ref's accepted DO API shapes are: (a) a VPC UUID literal, OR (b) a wfctl JIT template like ${vpc.id} that resolves to a UUID at apply time via wfctlhelpers.ApplyPlan's jitsubst.ResolveSpec. The prior validator unconditionally treated vpc_ref as an in-plan resource Name and would have rejected production configs that use the canonical UUID shape (godo.AppVpcSpec{ID: vpcID} consumes it directly per internal/drivers/app_platform_buildspec.go:674). New looksLikeResourceName() helper detects UUID literals (RFC-4122 pattern) and JIT templates (${...} / $(...)), and the in-plan- name lookup is skipped for both. Only plain-name vpc_ref values trigger the dangling-reference + non-VPC-type checks. Diagnostic messages updated to call out the 'plain resource name' branch explicitly so an operator who hit the diagnostic understands it does not apply to UUID/template forms. Two new TDD tests cover the UUID-literal + JIT-template paths for both database and App Platform. #10 (validate_plan.go:307) — classifyRegion emitted 'a zone slug in group ""' for legacy zones (nyc2, ams2) that intentionally map to an empty group. Now special-cases the empty-group case to emit 'a zone slug not in any App Platform region group'. New TDD test exercises the path via an App Platform action with region=nyc2. #11 (provider_apply_test.go:94) — the test comment claimed real drivers reject empty-ProviderID deletes via typed validation; FirewallDriver.Delete actually resolves by name when ProviderID is empty. Comment rewritten to reflect that v2 dispatch contract is 'driver knows what an empty ProviderID means for its resource shape', not 'all drivers reject empty ProviderID'. Net new test count: 3 (UUID-deferred, JIT-template-deferred, classify-empty-group). All existing tests still pass. Verified locally: go test ./... -count=1 -race PASS, go test -tags=conformance ./internal/ -run TestConformance PASS, go vet clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/provider_apply_test.go | 18 ++++++-- internal/validate_plan.go | 76 +++++++++++++++++++++++++++++-- internal/validate_plan_test.go | 81 +++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 8 deletions(-) diff --git a/internal/provider_apply_test.go b/internal/provider_apply_test.go index d3297da..c640ed3 100644 --- a/internal/provider_apply_test.go +++ b/internal/provider_apply_test.go @@ -90,8 +90,16 @@ func TestDOProvider_Apply_DeleteAction(t *testing.T) { // ResourceRef (the driver is the authority on what an empty ProviderID // means — see wfctlhelpers/apply.go::doUpdate's analogous comment). The // v1-era pre-flight precondition error has been retired with the v2 -// migration (PR P-DO TP2): drivers that cannot delete by name surface -// the diagnostic themselves via their typed validation. +// migration (PR P-DO TP2): the response when ProviderID is empty is now +// driver-dependent rather than centrally synthesised. +// +// Copilot review #11 (round 3): driver behavior on an empty-ProviderID +// delete varies — FirewallDriver.Delete, for example, resolves by name +// when ProviderID is empty (uses ListByTag/ListByDroplet to locate the +// firewall) and may succeed; other drivers (DatabaseDriver, +// VolumeDriver) require ProviderID and surface a typed error. The v2 +// contract is "the driver knows what an empty ProviderID means for its +// resource shape", not "all drivers reject empty ProviderID". func TestDOProvider_Apply_DeleteAction_MissingCurrent(t *testing.T) { fake := &deleteFakeDriver{} p := &DOProvider{drivers: map[string]interfaces.ResourceDriver{"infra.firewall": fake}} @@ -109,9 +117,9 @@ func TestDOProvider_Apply_DeleteAction_MissingCurrent(t *testing.T) { if err != nil { t.Fatalf("Apply returned top-level error: %v", err) } - // v2 contract: the dispatch IS made; the deleteFakeDriver returns nil - // because it does not validate ref.ProviderID. A real driver would - // reject the empty ProviderID via its typed validator. + // v2 contract: the dispatch IS made with an empty ProviderID; this + // stub fake accepts it and returns nil. Real drivers handle the + // empty case driver-by-driver (some resolve-by-name, some error). if fake.deleteCalls != 1 { t.Errorf("Delete dispatched %d times, want 1 (v2 dispatch contract)", fake.deleteCalls) } diff --git a/internal/validate_plan.go b/internal/validate_plan.go index 45edba8..3c3325f 100644 --- a/internal/validate_plan.go +++ b/internal/validate_plan.go @@ -2,12 +2,51 @@ package internal import ( "fmt" + "regexp" "sort" "strings" "github.com/GoCodeAlone/workflow/interfaces" ) +// uuidPattern matches a canonical RFC-4122 UUID (8-4-4-4-12 lower-case +// hex with hyphens). DO VPC IDs are UUIDs; vpc_ref values that match +// this shape are external-or-resolved-VPC-IDs, NOT in-plan resource +// names — the validator MUST NOT flag them as dangling references. +var uuidPattern = regexp.MustCompile(`^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`) + +// jitTemplatePattern matches the wfctl JIT substitution syntax +// (${VAR} / ${MODULE.field} / $(...)). vpc_ref values that contain a +// template are resolved at apply time by wfctlhelpers.ApplyPlan's +// jitsubst.ResolveSpec; ValidatePlan runs at PLAN time (before +// substitution) and MUST treat templated values as deferred. +var jitTemplatePattern = regexp.MustCompile(`\$[\{\(]`) + +// looksLikeResourceName reports whether s is plausibly an in-plan +// resource Name reference rather than a UUID or a JIT template. Used +// by ValidatePlan to decide whether to apply name-based byName lookup +// for vpc_ref. The contract: only the name-shape branch may emit +// dangling-reference diagnostics — UUIDs and templates are deferred to +// apply-time validation in the wfctlhelpers dispatch path. +// +// Copilot review #8/#9 (round 3): the prior validator unconditionally +// did name-based lookup, which incorrectly rejected production configs +// that pass vpc_ref as a UUID (the shape DO's Apps/Databases API +// expects) or as ${vpc.id} (the canonical wfctl form, resolved by JIT +// at apply time). +func looksLikeResourceName(s string) bool { + if s == "" { + return false + } + if uuidPattern.MatchString(s) { + return false + } + if jitTemplatePattern.MatchString(s) { + return false + } + return true +} + // ValidatePlan implements interfaces.ProviderValidator (W-4): a read-only, // no-remote-call cross-resource constraint check that runs at `wfctl infra // align` time before any cloud API call. Diagnostics surface as @@ -118,6 +157,15 @@ func appendDatabaseDiagnostics( if vpcRef == "" { return diags } + // Copilot review #9 (round 3): vpc_ref's accepted shapes per the + // DO Databases API are a VPC UUID OR a wfctl JIT template like + // ${vpc.id} that resolves to a UUID at apply time. Only flag the + // in-plan-name-reference branch — UUID and template values are + // deferred to apply-time validation by wfctlhelpers.ApplyPlan + // after JIT substitution. + if !looksLikeResourceName(vpcRef) { + return diags + } target, ok := byName[vpcRef] if !ok { // vpc_ref names a resource not in the plan. Surface as Error so @@ -130,7 +178,7 @@ func appendDatabaseDiagnostics( Resource: spec.Name, Field: "vpc_ref", Message: fmt.Sprintf( - "DO database %q references vpc_ref %q which is not declared in the same plan; either add the VPC resource or remove the vpc_ref", + "DO database %q references vpc_ref %q (a plain resource name) which is not declared in the same plan; either add the VPC resource, switch to a UUID or ${vpc.id} template, or remove the vpc_ref", spec.Name, vpcRef, ), }) @@ -190,6 +238,17 @@ func appendAppPlatformDiagnostics( if vpcRef == "" || region == "" || !isAppPlatformRegionGroup(region) { return diags } + // Copilot review #8 (round 3): App Platform vpc_ref maps to + // godo.AppVpcSpec{ID: vpcID} — the DO Apps API expects a VPC UUID + // at apply time. ValidatePlan runs at PLAN time before JIT + // substitution, so vpc_ref values shaped like a UUID or a wfctl + // template (${vpc.id}) cannot be cross-resolved against byName + // without false positives. Only the plain-name branch is checked + // here; UUID/template values are deferred to apply-time + // validation. + if !looksLikeResourceName(vpcRef) { + return diags + } target, ok := byName[vpcRef] if !ok { // vpc_ref points to a name not in the plan; cannot validate @@ -201,7 +260,7 @@ func appendAppPlatformDiagnostics( Resource: spec.Name, Field: "vpc_ref", Message: fmt.Sprintf( - "App Platform %q references vpc_ref %q which is not declared in the same plan; region-match check skipped", + "App Platform %q references vpc_ref %q (a plain resource name) which is not declared in the same plan; region-match check skipped", spec.Name, vpcRef, ), }) @@ -299,12 +358,23 @@ func zonesInGroup(group string) []string { // classifyRegion returns a short human-readable label for a region slug // to help diagnostic messages distinguish bare-group from zone-slug // confusion. +// +// Copilot review #10 (round 3): zoneToGroup intentionally maps some +// legacy zones (nyc2, ams2) to an empty group string so they pass +// is-a-zone-slug validation without claiming App Platform routing. +// The prior implementation surfaced these as 'a zone slug in group ""' +// which renders as a confusing empty-name label. The empty-group case +// is now special-cased. func classifyRegion(s string) string { if isAppPlatformRegionGroup(s) { return fmt.Sprintf("a region GROUP (zones: %s)", strings.Join(zonesInGroup(s), ", ")) } if isZoneSlug(s) { - return fmt.Sprintf("a zone slug in group %q", appPlatformRegionGroupOf(s)) + group := appPlatformRegionGroupOf(s) + if group == "" { + return "a zone slug not in any App Platform region group" + } + return fmt.Sprintf("a zone slug in group %q", group) } return "not a recognized DO region slug" } diff --git a/internal/validate_plan_test.go b/internal/validate_plan_test.go index 4ec1c01..f6afb47 100644 --- a/internal/validate_plan_test.go +++ b/internal/validate_plan_test.go @@ -402,6 +402,87 @@ func TestDOProvider_ValidatePlan_AppPlatformVPCRefToNonVPCType(t *testing.T) { } } +// TestDOProvider_ValidatePlan_VPCRefAsUUIDIsDeferred pins Copilot +// review #8/#9 (round 3): vpc_ref values that look like a VPC UUID +// (the canonical DO Apps/Databases API shape at apply time) MUST NOT +// trigger the in-plan-name-resolution branch — they are deferred to +// apply-time validation by wfctlhelpers.ApplyPlan after JIT +// substitution. The test exercises both the database and the App +// Platform paths with a fixed UUID literal. +func TestDOProvider_ValidatePlan_VPCRefAsUUIDIsDeferred(t *testing.T) { + const vpcUUID = "a1b2c3d4-e5f6-7890-abcd-ef0123456789" + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": vpcUUID}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": vpcUUID}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for UUID vpc_ref values; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_VPCRefAsJITTemplateIsDeferred pins +// Copilot review #8/#9 (round 3): vpc_ref values that contain the +// wfctl JIT substitution syntax (${MODULE.field} or $(...)) MUST NOT +// trigger name-based resolution at plan time — they are resolved by +// wfctlhelpers.ApplyPlan's jitsubst.ResolveSpec at apply time. The +// test exercises both the database and the App Platform paths with +// canonical ${vpc.id} templates. +func TestDOProvider_ValidatePlan_VPCRefAsJITTemplateIsDeferred(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "${some-vpc.id}"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "$(other-vpc.id)"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for JIT-template vpc_ref values; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_ClassifyRegionEmptyGroup pins Copilot +// review #10 (round 3): a Droplet/VPC region set to a zone whose +// zoneToGroup mapping is the empty string (e.g., legacy nyc2/ams2) +// must produce a clear human-readable diagnostic, not 'a zone slug in +// group ""'. This test indirectly verifies via the message content of +// the bare-group rejection on a Droplet using nyc2. +// +// Note: nyc2 IS a valid zone slug (passes isZoneSlug), so a Droplet +// with region=nyc2 alone would pass validation. We exercise the +// empty-group branch via the App Platform group-vs-zone mismatch path +// where classifyRegion is called on a non-AP-routed zone. +func TestDOProvider_ValidatePlan_ClassifyRegionEmptyGroup(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + // nyc2 is a valid zone slug but not in any AP group. + Config: map[string]any{"region": "nyc2"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 { + t.Fatalf("expected 1 diagnostic for AP region=nyc2; got %d: %+v", len(d), d) + } + if strings.Contains(d[0].Message, `group ""`) { + t.Errorf("diagnostic still emits empty-group label; got %q", d[0].Message) + } + if !strings.Contains(d[0].Message, "not in any App Platform region group") { + t.Errorf("expected 'not in any App Platform region group' phrasing; got %q", d[0].Message) + } +} + // TestDOProvider_ValidatePlan_CompileTimeAssertion documents that the // compile-time interface assertion in validate_plan.go locks // DOProvider's ProviderValidator implementation. If the interface From a1e230bcf2347b010dba0f27db3b2e4fca0a7f82 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 22:10:56 -0400 Subject: [PATCH 10/11] =?UTF-8?q?fix:=20Copilot=20review=20round=204=20?= =?UTF-8?q?=E2=80=94=203=20polish=20findings=20(UUID=20case=20+=20t.Contex?= =?UTF-8?q?t=20+=20workflow=20perms)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3/3 findings addressed: #12 (validate_plan.go:16) — uuidPattern was lowercase-only; UUIDs are case-insensitive in practice (operator clipboards, templating engines, mixed-case API responses). Added (?i) flag so upper-case and mixed-case VPC UUIDs also classify as deferred-to-apply, not as plain resource names that would trigger false dangling-reference diagnostics. New test: TestDOProvider_ValidatePlan_VPCRefAsUpperCaseUUIDIsDeferred. #13 (provider_conformance_test.go:86) — switched p.Initialize(context.Background(), ...) to p.Initialize(t.Context(), ...) so live-cloud Initialize is interrupted promptly when the test is canceled or hits its deadline. Removed the now-unused "context" import. #14 (codemod-report.yml:12) — dropped the unused pull-requests:write permission. The workflow only creates/updates an issue comment (PR comments are issues at the GitHub API layer) so the surviving issues:write is sufficient. Inline doc-comment captures the reasoning so future maintainers don't restore the broader grant. Aligns with ci.yml's contents:read-only baseline. Verified locally: go test ./... -count=1 -race PASS, go test -tags=conformance ./internal/ -run TestConformance PASS, go vet clean, codemod-report.yml YAML valid. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/codemod-report.yml | 7 ++++++- internal/provider_conformance_test.go | 7 +++++-- internal/validate_plan.go | 14 +++++++++----- internal/validate_plan_test.go | 23 +++++++++++++++++++++++ 4 files changed, 43 insertions(+), 8 deletions(-) diff --git a/.github/workflows/codemod-report.yml b/.github/workflows/codemod-report.yml index fe8c526..96a44c8 100644 --- a/.github/workflows/codemod-report.yml +++ b/.github/workflows/codemod-report.yml @@ -3,8 +3,13 @@ on: pull_request: branches: [main] permissions: + # Copilot review #14 (round 4): scope to least privilege — + # contents:read for checkout, issues:write for the sticky PR + # comment (PR comments are issues at the GitHub API layer). The + # prior pull-requests:write grant was unused (no PR-state + # mutations) and inconsistent with the other workflows in this + # repo (ci.yml uses contents:read only). contents: read - pull-requests: write issues: write env: GOPRIVATE: github.com/GoCodeAlone/* diff --git a/internal/provider_conformance_test.go b/internal/provider_conformance_test.go index d602121..8ae145e 100644 --- a/internal/provider_conformance_test.go +++ b/internal/provider_conformance_test.go @@ -30,7 +30,6 @@ package internal import ( - "context" "os" "testing" @@ -78,7 +77,11 @@ func TestConformance(t *testing.T) { t.Fatal("CONFORMANCE_LIVE_CLOUD=1 but DIGITALOCEAN_ACCESS_TOKEN is not set") } } - if err := p.Initialize(context.Background(), map[string]any{ + // Copilot review #13 (round 4): use t.Context() so live- + // cloud Initialize is interrupted promptly when the test + // is canceled or hits its deadline; context.Background() + // would survive both signals. + if err := p.Initialize(t.Context(), map[string]any{ "token": token, "region": "nyc3", }); err != nil { diff --git a/internal/validate_plan.go b/internal/validate_plan.go index 3c3325f..1a511b0 100644 --- a/internal/validate_plan.go +++ b/internal/validate_plan.go @@ -9,11 +9,15 @@ import ( "github.com/GoCodeAlone/workflow/interfaces" ) -// uuidPattern matches a canonical RFC-4122 UUID (8-4-4-4-12 lower-case -// hex with hyphens). DO VPC IDs are UUIDs; vpc_ref values that match -// this shape are external-or-resolved-VPC-IDs, NOT in-plan resource -// names — the validator MUST NOT flag them as dangling references. -var uuidPattern = regexp.MustCompile(`^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`) +// uuidPattern matches a canonical RFC-4122 UUID (8-4-4-4-12 hex with +// hyphens). The match is case-insensitive (Copilot review #12 round 4 +// — UUIDs are case-insensitive in practice; a VPC UUID emitted in +// upper-case by an operator's clipboard or a templating engine must +// also be classified as a UUID, not as a plain resource name). DO +// VPC IDs are UUIDs; vpc_ref values that match this shape are +// external-or-resolved-VPC-IDs, NOT in-plan resource names — the +// validator MUST NOT flag them as dangling references. +var uuidPattern = regexp.MustCompile(`^(?i)[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`) // jitTemplatePattern matches the wfctl JIT substitution syntax // (${VAR} / ${MODULE.field} / $(...)). vpc_ref values that contain a diff --git a/internal/validate_plan_test.go b/internal/validate_plan_test.go index f6afb47..a4454d8 100644 --- a/internal/validate_plan_test.go +++ b/internal/validate_plan_test.go @@ -427,6 +427,29 @@ func TestDOProvider_ValidatePlan_VPCRefAsUUIDIsDeferred(t *testing.T) { } } +// TestDOProvider_ValidatePlan_VPCRefAsUpperCaseUUIDIsDeferred pins +// Copilot review #12 (round 4): UUIDs are case-insensitive; an +// upper-case or mixed-case VPC UUID must also be classified as a +// UUID literal (not as a plain resource name) so it does not trigger +// false dangling-reference diagnostics. Verifies the (?i) flag on +// uuidPattern. +func TestDOProvider_ValidatePlan_VPCRefAsUpperCaseUUIDIsDeferred(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "A1B2C3D4-E5F6-7890-ABCD-EF0123456789"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "A1b2C3d4-E5f6-7890-AbCd-Ef0123456789"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for upper/mixed-case UUID vpc_ref; got %+v", d) + } +} + // TestDOProvider_ValidatePlan_VPCRefAsJITTemplateIsDeferred pins // Copilot review #8/#9 (round 3): vpc_ref values that contain the // wfctl JIT substitution syntax (${MODULE.field} or $(...)) MUST NOT From 3758b4790bdc243fda088796520cb70e783c2bf1 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 22:18:19 -0400 Subject: [PATCH 11/11] =?UTF-8?q?fix:=20Copilot=20review=20round=205=20?= =?UTF-8?q?=E2=80=94=202=20findings=20(Initialize-ctx=20comment=20+=20unkn?= =?UTF-8?q?own-region=20forward-compat)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 2/2 findings addressed: #15 (provider_conformance_test.go:83) — the prior comment claimed t.Context() makes Initialize cancelable, but DOProvider.Initialize today constructs its godo client with its own context.Background() and ignores the passed-in ctx. Comment rewritten to reflect that the change is forward-prep (so any future rev of Initialize that honors ctx picks up the test-scoped cancellation/deadline) rather than an immediate behavior fix. Tracked as a follow-up to thread ctx through the godo client construction. #16 (validate_plan.go:334) — the hardcoded region/zone allowlists would Severity=Error any brand-new DO region the plugin hasn't caught up to, blocking apply until the plugin is bumped. Severity is now two-bucket: - Documented misconfig (group-where-zone-required, zone-where- group-required) → Error (the original anti-pattern stays loud). - Unknown slug (neither known group nor known zone, e.g. a hypothetical 'atl' or 'atl1') → Warning so non-strict align lets operators on bleeding-edge DO regions proceed; --strict still escalates for cautious operators. New regression test: TestDOProvider_ValidatePlan_UnknownRegionSlugWarnsNotErrors covers both VPC zone and AP group unknown-slug paths. Net new test count: 1. All existing tests still PASS (the documented misconfig branches use known specific slugs that hit the Error path). Verified locally: go test ./... -count=1 -race PASS, go test -tags=conformance ./internal/ -run TestConformance PASS, go vet clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/provider_conformance_test.go | 15 ++++++++--- internal/validate_plan.go | 38 +++++++++++++++++++++++++-- internal/validate_plan_test.go | 33 +++++++++++++++++++++++ 3 files changed, 80 insertions(+), 6 deletions(-) diff --git a/internal/provider_conformance_test.go b/internal/provider_conformance_test.go index 8ae145e..3412a58 100644 --- a/internal/provider_conformance_test.go +++ b/internal/provider_conformance_test.go @@ -77,10 +77,17 @@ func TestConformance(t *testing.T) { t.Fatal("CONFORMANCE_LIVE_CLOUD=1 but DIGITALOCEAN_ACCESS_TOKEN is not set") } } - // Copilot review #13 (round 4): use t.Context() so live- - // cloud Initialize is interrupted promptly when the test - // is canceled or hits its deadline; context.Background() - // would survive both signals. + // Copilot review #13 (round 4) → #15 (round 5): + // pass t.Context() through Initialize so callers that + // honor context (and any future rev of Initialize that + // does) get the test-scoped cancellation/deadline path. + // Note: today's DOProvider.Initialize implementation + // constructs its godo client with its own + // context.Background() and ignores the passed-in ctx, + // so passing t.Context() is forward-prep rather than an + // immediate behavior change. Tracked as a follow-up to + // thread the context through the godo construction so + // LiveCloud cancellation reaches the HTTP transport. if err := p.Initialize(t.Context(), map[string]any{ "token": token, "region": "nyc3", diff --git a/internal/validate_plan.go b/internal/validate_plan.go index 1a511b0..c34649a 100644 --- a/internal/validate_plan.go +++ b/internal/validate_plan.go @@ -123,8 +123,28 @@ func (p *DOProvider) ValidatePlan(plan *interfaces.IaCPlan) []interfaces.PlanDia diags = appendAppPlatformDiagnostics(diags, a.Resource, region, byName) case "infra.vpc", "infra.droplet", "infra.volume": if region != "" && !isZoneSlug(region) { + // Copilot review #16 (round 5): a region that is + // neither a known zone nor a known App Platform + // group falls into two buckets: + // (a) a documented misconfig — operator typed an + // App Platform group ("nyc") where a zone is + // required. This is the recurring bug and + // stays Severity=Error. + // (b) an unknown slug — possibly a brand-new DO + // region the plugin's hardcoded allowlist + // hasn't caught up to. This was previously + // Error too, blocking apply until the plugin + // was bumped. Now downgraded to Warning so + // operators on bleeding-edge DO regions can + // proceed (under non-strict align) while + // --strict still catches the case for + // cautious operators. + severity := interfaces.PlanDiagnosticWarning + if isAppPlatformRegionGroup(region) { + severity = interfaces.PlanDiagnosticError + } diags = append(diags, interfaces.PlanDiagnostic{ - Severity: interfaces.PlanDiagnosticError, + Severity: severity, Resource: a.Resource.Name, Field: "region", Message: fmt.Sprintf( @@ -227,8 +247,22 @@ func appendAppPlatformDiagnostics( byName map[string]planResource, ) []interfaces.PlanDiagnostic { if region != "" && !isAppPlatformRegionGroup(region) { + // Copilot review #16 (round 5): two-bucket severity + // matching the VPC/Droplet/Volume case above: + // (a) operator typed a known zone slug ("nyc3") where a + // group ("nyc") is required — documented misconfig, + // stays Error. + // (b) unknown slug — possibly a brand-new App Platform + // group the plugin's allowlist hasn't caught up to. + // Downgraded to Warning so operators can proceed + // (under non-strict align) without a plugin bump; + // --strict still escalates. + severity := interfaces.PlanDiagnosticWarning + if isZoneSlug(region) { + severity = interfaces.PlanDiagnosticError + } diags = append(diags, interfaces.PlanDiagnostic{ - Severity: interfaces.PlanDiagnosticError, + Severity: severity, Resource: spec.Name, Field: "region", Message: fmt.Sprintf( diff --git a/internal/validate_plan_test.go b/internal/validate_plan_test.go index a4454d8..6dd3156 100644 --- a/internal/validate_plan_test.go +++ b/internal/validate_plan_test.go @@ -506,6 +506,39 @@ func TestDOProvider_ValidatePlan_ClassifyRegionEmptyGroup(t *testing.T) { } } +// TestDOProvider_ValidatePlan_UnknownRegionSlugWarnsNotErrors pins +// Copilot review #16 (round 5): a region slug that is neither a known +// App Platform group nor a known zone (e.g., a brand-new DO region +// 'atl1' the plugin's hardcoded allowlist hasn't caught up to) MUST +// downgrade to Severity=Warning so non-strict align lets operators +// proceed without a plugin bump. The documented misconfig cases +// (group-where-zone-required, zone-where-group-required) keep +// Severity=Error. +func TestDOProvider_ValidatePlan_UnknownRegionSlugWarnsNotErrors(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + // VPC with unknown region — should Warning (forward-compat). + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "fwd-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "atl1"}, + }}, + // AP with unknown region — should Warning (forward-compat). + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "fwd-app", Type: "infra.container_service", + Config: map[string]any{"region": "atl"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 2 { + t.Fatalf("expected 2 diagnostics for unknown slugs; got %d: %+v", len(d), d) + } + for _, x := range d { + if x.Severity != interfaces.PlanDiagnosticWarning { + t.Errorf("expected Severity=Warning for unknown slug; got %v in %+v", x.Severity, x) + } + } +} + // TestDOProvider_ValidatePlan_CompileTimeAssertion documents that the // compile-time interface assertion in validate_plan.go locks // DOProvider's ProviderValidator implementation. If the interface