feat: implement gRPC IaC ResourceDriver for Hover DNS provider#2
Merged
Conversation
Implements the full typed IaC provider surface (pb.IaCProviderRequiredServer + pb.IaCProviderFinalizerServer + pb.IaCProviderDriftDetectorServer) for Hover DNS, modeled on workflow-plugin-digitalocean. Provider uses a browser-session client (CSRF + TOTP) since Hover has no official API. Key additions: - internal/hover/client.go: public Login method + conditional TOTP (skipped when account has MFA disabled; probed via /signin/totp GET) - internal/drivers/dns.go: ResourceDriver for infra.dns (Create/Read/ Update/Delete/Diff/HealthCheck); structpb-safe Outputs ([]any, not []hover.DNSRecord) - internal/provider.go: HoverProvider implementing interfaces.IaCProvider with platform.ComputePlan delegation - internal/iacserver.go: hoverIaCServer wiring pb RPCs → HoverProvider, ComputePlanVersion "v2", FinalizeApply no-op - go.mod: pin github.com/GoCodeAlone/workflow v0.57.1 - 42 tests, all passing; GOWORK=off go build ./... clean Hover API endpoints used (undocumented; derived from pjslauta/hover-dyn-dns): GET https://www.hover.com/api/domains/<domain>/dns POST https://www.hover.com/api/dns PUT https://www.hover.com/api/dns/<id> DELETE https://www.hover.com/api/dns/<id> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements a typed gRPC IaC provider plugin for Hover DNS, including provider/server wiring, a DNS ResourceDriver for infra.dns, and a Hover portal client with conditional TOTP (MFA) handling.
Changes:
- Add
hoverIaCServer+HoverProviderimplementing the typedpb.IaCProvider*Serversurfaces and delegating to driver logic. - Implement
infra.dnsdriver and Hover client DNS record CRUD + login flow (conditional MFA probe). - Update docs/examples and pin Go module dependencies.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents Hover auth flow (conditional MFA) and updates record field name to content. |
| internal/serve.go | Starts the typed IaC plugin server via the workflow SDK. |
| internal/provider.go | Implements interfaces.IaCProvider for Hover and wires resource drivers. |
| internal/iacserver.go | Implements typed gRPC services and JSON marshalling helpers for config/outputs. |
| internal/iacserver_test.go | Tests basic RPC behavior/capabilities and some initialization validation. |
| internal/hover/client.go | Adds public Login plus conditional TOTP probing; implements Hover DNS record API calls. |
| internal/hover/client_test.go | Adds MFA/no-MFA login tests and DNS record CRUD tests using an httptest stub. |
| internal/drivers/dns.go | Implements ResourceDriver for infra.dns with outputs encoded in structpb-safe primitives. |
| internal/drivers/dns_test.go | Unit tests for driver create/update/diff/read/healthcheck/scale behavior. |
| go.mod | Adds/pins dependencies needed for typed plugin + protobuf usage. |
| go.sum | Records updated dependency checksums. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+214
to
+220
| if err := d.client.UpdateRecord(ctx, ex.ID, dr); err != nil { | ||
| return fmt.Errorf("hover dns update %s/%s %q: %w", dr.Type, dr.Name, domain, err) | ||
| } | ||
| } else { | ||
| created, err := d.client.CreateRecord(ctx, domain, dr) | ||
| if err != nil { | ||
| return fmt.Errorf("hover dns create %s/%s %q: %w", dr.Type, dr.Name, domain, err) |
Comment on lines
+151
to
+153
| if candidates[0].Content != dr.Content { | ||
| return &interfaces.DiffResult{NeedsUpdate: true}, nil | ||
| } |
Comment on lines
+369
to
+379
| for i, m := range items { | ||
| typ, _ := m["type"].(string) | ||
| name, _ := m["name"].(string) | ||
| content, _ := m["content"].(string) | ||
| ttl, _ := optionalInt(m, "ttl") | ||
| id, _ := m["id"].(string) | ||
| if typ == "" || name == "" { | ||
| continue | ||
| } | ||
| _ = i | ||
| recs = append(recs, hover.DNSRecord{ |
Comment on lines
+145
to
+159
| func (c *Client) probeTOTPPage(ctx context.Context) (string, bool, error) { | ||
| req, _ := http.NewRequestWithContext(ctx, http.MethodGet, hoverHost+"/signin/totp", nil) | ||
| req.Header.Set("User-Agent", c.UserAgent) | ||
| resp, err := c.http.Do(req) | ||
| if err != nil { | ||
| return "", false, fmt.Errorf("hover: probe TOTP page: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) | ||
| m := csrfRe.FindSubmatch(body) | ||
| if len(m) < 2 { | ||
| // No CSRF token on the page — MFA is not enabled on this account. | ||
| return "", false, nil | ||
| } | ||
| return string(m[1]), true, nil |
| sdk "github.com/GoCodeAlone/workflow/plugin/external/sdk" | ||
| ) | ||
|
|
||
| // Serve starts the Hover IaC plugin gRPC server. Called from cmd/main.go. |
| 3. GET `/signin/totp` to probe for MFA: | ||
| - If the page contains a `_token`: account has MFA enabled → POST `/signin/totp` | ||
| with `code` (RFC 6238 TOTP) + `_token`. | ||
| - If no `_token`: MFA is disabled → skip step 3. |
Comment on lines
+5
to
+8
| require ( | ||
| github.com/GoCodeAlone/workflow v0.60.8 | ||
| google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af | ||
| ) |
Comment on lines
+115
to
+116
| // Destroy removes DNS records for the given refs. | ||
| func (p *HoverProvider) Destroy(ctx context.Context, resources []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { |
Comment on lines
+214
to
+220
| if err := d.client.UpdateRecord(ctx, ex.ID, dr); err != nil { | ||
| return fmt.Errorf("hover dns update %s/%s %q: %w", dr.Type, dr.Name, domain, err) | ||
| } | ||
| } else { | ||
| created, err := d.client.CreateRecord(ctx, domain, dr) | ||
| if err != nil { | ||
| return fmt.Errorf("hover dns create %s/%s %q: %w", dr.Type, dr.Name, domain, err) |
| } | ||
| if candidates[0].Content != dr.Content { | ||
| return &interfaces.DiffResult{NeedsUpdate: true}, nil | ||
| } |
Comment on lines
+138
to
+155
| // Index current records by (type, name) for O(1) lookup. | ||
| currentByKey := make(map[string][]hover.DNSRecord) | ||
| for _, r := range currentRecs { | ||
| key := recordKey(r.Type, r.Name) | ||
| currentByKey[key] = append(currentByKey[key], r) | ||
| } | ||
|
|
||
| for _, dr := range desiredRecs { | ||
| key := recordKey(dr.Type, dr.Name) | ||
| candidates := currentByKey[key] | ||
| if len(candidates) == 0 { | ||
| return &interfaces.DiffResult{NeedsUpdate: true}, nil | ||
| } | ||
| if candidates[0].Content != dr.Content { | ||
| return &interfaces.DiffResult{NeedsUpdate: true}, nil | ||
| } | ||
| currentByKey[key] = candidates[1:] | ||
| } |
Comment on lines
+145
to
+159
| func (c *Client) probeTOTPPage(ctx context.Context) (string, bool, error) { | ||
| req, _ := http.NewRequestWithContext(ctx, http.MethodGet, hoverHost+"/signin/totp", nil) | ||
| req.Header.Set("User-Agent", c.UserAgent) | ||
| resp, err := c.http.Do(req) | ||
| if err != nil { | ||
| return "", false, fmt.Errorf("hover: probe TOTP page: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) | ||
| m := csrfRe.FindSubmatch(body) | ||
| if len(m) < 2 { | ||
| // No CSRF token on the page — MFA is not enabled on this account. | ||
| return "", false, nil | ||
| } | ||
| return string(m[1]), true, nil |
Comment on lines
+5
to
+8
| require ( | ||
| github.com/GoCodeAlone/workflow v0.60.8 | ||
| google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af | ||
| ) |
Eight findings; this commit addresses every substantive one: 1. (CRITICAL) DNSDriver.upsertRecords passed the apex domain NAME into hover.Client.CreateRecord, but Hover's POST /api/dns endpoint requires the hover-assigned `domain_id` (numeric ID). Live calls would have been rejected. Added a Client.GetDomain method that returns the full Domain struct including ID; upsertRecords now resolves the ID up front and passes it to CreateRecord. Bonus: GetDomain returns the embedded record set, so we drop the separate ListRecords round-trip inside upsertRecords. 2. Diff compared only Content, ignoring TTL. A TTL-only change in desired config would never trigger NeedsUpdate=true even though upsertRecords already supports applying a new TTL. Now: when the desired record specifies a TTL (>0), Diff also compares TTL. TTL == 0 still means "leave the existing TTL alone". 3. probeTOTPPage discarded HTTP status codes and io.ReadAll errors and treated everything as "no MFA". A redirect / login failure / Cloudflare gate / body-read error would have been silently misclassified, letting Login appear to succeed before failing on the next API call. Non-2xx + read errors are now surfaced; only a clean 200 with no _token is treated as "MFA disabled". 4. dnsRecordsFromOutput declared an index variable then discarded it with `_ = i`. Replaced with a bare `range` over the slice. 5. internal/serve.go godoc pointed to cmd/main.go; the actual entrypoint is cmd/workflow-plugin-hover/main.go. Updated. 6. README auth-flow step 3 said "skip step 3", but step 3 is the GET probe itself; only the POST submission is conditional. Reworded to "skip the TOTP submission (the GET probe itself still runs)". 7. (was: PR description / go.mod version mismatch — handled in PR description, no code change required.) 8. HoverProvider.Destroy godoc said "removes DNS records"; the driver Delete is a no-op (Hover has no zone-delete API). Updated the godoc to spell out the actual semantics: state is marked destroyed, upstream records remain. New tests: - TestUpsertRecords_UsesDomainIDNotName (regresses finding 1) - TestDiff_TTLChange_DetectedAsUpdate (regresses finding 2) fakeClient extended with GetDomain implementation + a `lastCreateDomainID` capture field for assertions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The superpowers hook writes a transient .claude/superpowers-state/in-progress.jsonl during agent sessions; it leaked into the prior commit. Ignore it going forward and untrack the existing file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
internal/hover/client.go:181
- fetchCSRF does not check resp.StatusCode. A non-2xx response (redirect/403/5xx) will currently fall through to CSRF parsing and may produce a misleading "token not found" error instead of surfacing the HTTP failure.
func (c *Client) fetchCSRF(ctx context.Context, urlStr string) (string, error) {
req, _ := http.NewRequestWithContext(ctx, http.MethodGet, urlStr, nil)
req.Header.Set("User-Agent", c.UserAgent)
resp, err := c.http.Do(req)
if err != nil {
return "", err
}
defer resp.Body.Close()
internal/drivers/dns.go:360
- optionalInt casts float64/int64 to int without range or fractional checks. Since config will often arrive via JSON as float64, values like 300.5 get truncated and negative TTLs are accepted; both can cause non-converging plans because Hover UpdateRecord only applies TTL when > 0. Consider rejecting non-integer/negative TTLs and checking for overflow before converting to int.
func optionalInt(m map[string]any, key string) (int, bool) {
v, ok := m[key]
if !ok {
return 0, false
}
switch n := v.(type) {
case int:
return n, true
case int64:
return int(n), true
case float64:
return int(n), true
}
return 0, false
internal/drivers/dns.go:223
- upsertRecords updates the first existing record for a (type,name) key (ex := candidates[0]) even if another candidate already matches the desired content. For multi-value records this can cause unnecessary updates and can destroy the intended set of values depending on ordering. Prefer selecting an existing record that matches desired content/ttl (if present) before deciding to update/create.
key := recordKey(dr.Type, dr.Name)
candidates := existingByKey[key]
if len(candidates) > 0 {
ex := candidates[0]
existingByKey[key] = candidates[1:]
Comment on lines
+5
to
+8
| require ( | ||
| github.com/GoCodeAlone/workflow v0.60.8 | ||
| google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af | ||
| ) |
Comment on lines
+313
to
+327
| func recordFromMap(index int, m map[string]any) (hover.DNSRecord, error) { | ||
| typ, err := requiredString(m, "type", index) | ||
| if err != nil { | ||
| return hover.DNSRecord{}, err | ||
| } | ||
| name, err := requiredString(m, "name", index) | ||
| if err != nil { | ||
| return hover.DNSRecord{}, err | ||
| } | ||
| content, err := requiredString(m, "content", index) | ||
| if err != nil { | ||
| return hover.DNSRecord{}, err | ||
| } | ||
| ttl, _ := optionalInt(m, "ttl") | ||
| return hover.DNSRecord{ |
Comment on lines
+148
to
+152
| candidates := currentByKey[key] | ||
| if len(candidates) == 0 { | ||
| return &interfaces.DiffResult{NeedsUpdate: true}, nil | ||
| } | ||
| cur := candidates[0] |
PR #2 round-2 Copilot findings: 1. recordFromMap previously discarded the error from optionalInt, so a string TTL ("300"), a non-integral float (300.5), or a negative value would silently coerce to 0 — unset — and let an apparently-typed config produce surprising diffs. Added optionalNonNegativeInt which rejects wrong-type, non-integral, and negative values with a typed error pointing at the offending records[N].ttl field. 2. Diff matched current records by (type, name) and consumed candidates[0] without checking Content. With multiple records sharing the same (type, name) — common for multi-A or multi-TXT on the apex — current and desired in different orders would falsely report NeedsUpdate. Replaced with multiset matching: each desired record consumes the first candidate whose Content (and TTL, when specified) match. 3. Three new tests: - TestDiff_MultipleARecords_OrderingDoesNotMatter - TestRecordFromMap_InvalidTTL_Rejected (negative) - TestRecordFromMap_NonIntegralTTL_Rejected (300.5) - TestRecordFromMap_StringTTL_Rejected (The go.mod / PR description version mismatch finding is addressed in the PR description, not code — go.mod's v0.60.8 is the intended pin.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/drivers/dns.go:253
- upsertRecords only creates/updates records; it never deletes records that exist in Hover but are no longer present in the desired config. That makes record removals (and cleanup of stale records) impossible via IaC even if Diff is updated to detect them. Consider deleting any remaining unmatched records from dom.Records after desired reconciliation (or explicitly documenting non-pruning behavior and adding a flag to opt in).
}
}
}
return nil
}
Comment on lines
+130
to
+132
| if len(desiredRecs) == 0 { | ||
| return &interfaces.DiffResult{NeedsUpdate: false}, nil | ||
| } |
Comment on lines
+182
to
+186
| _, err = d.Read(ctx, ref) | ||
| if err != nil { | ||
| if isNotFound(err) { | ||
| results = append(results, interfaces.DriftResult{ | ||
| Name: ref.Name, Type: ref.Type, Drifted: true, |
PR #2 round-3 Copilot findings: 1. Diff only verified desired ⊆ current. Removing a record from config silently went undetected — Plan reported NoOp, then reality stayed drifted forever. Diff now: - returns NeedsUpdate=true when desired is empty but current has records; - after matching every desired record, sweeps for leftover unmatched current candidates and returns NeedsUpdate=true if any remain. Caveat documented inline: upsertRecords today does not prune extras (only adds/updates). The drift signal is still the right Plan output — operators want to see the discrepancy in the plan. Adding the prune path is a separate follow-up. 2. DetectDrift's isNotFound() used err.Error() substring matching to recognise missing resources. The driver wraps these with interfaces.ErrResourceNotFound, so errors.Is is the right check. Kept the substring fallback for raw Client errors that reach DetectDrift unwrapped, but the sentinel path is checked first. Tests: - TestDiff_ExtraCurrentRecord_DetectedAsUpdate - TestDiff_EmptyDesired_WithCurrentRecords_NeedsUpdate Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+251
to
+263
| for _, dr := range desired { | ||
| key := recordKey(dr.Type, dr.Name) | ||
| candidates := existingByKey[key] | ||
| if len(candidates) > 0 { | ||
| ex := candidates[0] | ||
| existingByKey[key] = candidates[1:] | ||
| if ex.Content == dr.Content && (dr.TTL == 0 || ex.TTL == dr.TTL) { | ||
| continue // already matches | ||
| } | ||
| if err := d.client.UpdateRecord(ctx, ex.ID, dr); err != nil { | ||
| return fmt.Errorf("hover dns update %s/%s %q: %w", dr.Type, dr.Name, domain, err) | ||
| } | ||
| } else { |
| // are not in the desired set. Treat that as drift so the engine | ||
| // surfaces it during Plan, even though upsertRecords does not | ||
| // currently prune the extras (an explicit prune path is a | ||
| // separate follow-up; see README "Replace semantics" caveat). |
Comment on lines
+12
to
+14
| // - Only the Required + Drift services are registered; Optional services | ||
| // that Hover cannot satisfy (state backend, sizing, migration repair, | ||
| // enumeration) are left as Unimplemented. |
PR #2 round-4 Copilot findings: 1. upsertRecords matched candidates[0] when same-key candidates existed, even when one of the OTHER candidates was already an exact match. With multiple records sharing (type, name) — e.g., two A records on the apex — apply could update record A to record B's content, producing a transient state with two identical records. Split into two passes: - Pass 1: skip-on-match. Every desired record that already has an exact (Content + TTL-when-specified) match upstream consumes that candidate and is treated as no-op. - Pass 2: update-or-create. Remaining desired records consume the next leftover candidate at the same key as an Update target, or fall through to Create if none remain. This matches the multiset semantics introduced in round-3 for Diff and prevents the transient-duplicate state. 2. Diff comment referenced a README "Replace semantics" caveat that didn't exist. Added a "Limitations" section to README covering the no-prune-on-apply gap (and the no-zone-delete caveat); updated the comment to point there. 3. iacserver.go header claimed "Only the Required + Drift services are registered". Misleading — sdk.ServeIaCPlugin registers every service for which the struct embeds an Unimplemented server, and hoverIaCServer embeds the full set. Replaced the line with an accurate statement: every service IS registered; unimplemented methods return typed Unimplemented gRPC status, which is the cross-plugin-uniform shape (clients must check capabilities before calling optional methods). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
intel352
added a commit
that referenced
this pull request
May 21, 2026
* docs: design infra.dns_delegation for hover plugin
New resource type adds registrar-level nameserver management
alongside the existing infra.dns record management. Endpoint
captured from the Hover web UI: PUT /api/control_panel/domains/
domain-<name> with X-CSRF-Token header + {field, value} JSON.
Decisions locked via brainstorming:
- Resource type: infra.dns_delegation (matches AWS/GCP naming
room for equivalents; semantically distinct from infra.dns).
- Delete: reset to Hover defaults [ns1.hover.com, ns2.hover.com].
- CSRF: fetch fresh per PUT.
- Field test: gocodealone.tech end-to-end via
workflow_dispatch GHA inside gocodealone-multisite.
Assumptions, rollback, and top-3 doubts captured in the doc for
adversarial-review attack.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: revise dns_delegation design — adversarial review round 1
Addresses 2 Critical + 3 Important + 3 Minor findings from
adversarial review:
CRITICAL:
- Read endpoint uncertainty (A6): switched primary Read from
/api/domains/<name>/dns to /api/control_panel/domains/domain-<name>
(same API family as the PUT — far more likely to surface the
nameservers field reliably). First implementation task =
curl-verify the response shape.
- Outputs["nameservers"] encoding: explicitly spec'd as []any
(not []string) per the structpb boundary invariant. Helper
+ round-trip JSON test.
IMPORTANT:
- CSRF source ambiguity: documented the two distinct regexes
(form token on /signin; meta tag on /control_panel/). Cited
that both shapes coexist in the captured browser session.
- ensureLogin + fetchControlPanelCSRF concurrency: new *Locked
helpers; SetNameservers holds c.mu across both the CSRF GET
and the PUT. Eliminates the race window.
- Delete hardcoded defaults: primary path stashes
previous_nameservers at Create; Delete restores from state.
Hardcoded fallback only for state-less resources.
MINOR:
- Sequencing: deferred registry manifest + multisite YAML to a
separate session post-goreleaser.
- Validation: relaxed to >=1 nameserver.
- Field test: success = Hover UI shows new NS; dig is separate.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: revise design — adversarial review round 2
Addresses 1 Critical + 2 Important from round 2:
CRITICAL:
- TOCTOU between ensureLogin + c.mu.Lock in SetNameservers:
new ensureLoginLocked helper; SetNameservers holds c.mu for
the entire auth -> CSRF -> PUT sequence. No interleaving
window remains.
IMPORTANT:
- Domain struct dual-population ambiguity: introduced distinct
DomainDelegation type returned by GetDomainDelegation.
Existing Domain struct unchanged.
- Silent Apply thrash on empty nameservers: GetDomainDelegation
returns typed ErrEmptyNameservers on zero entries. Loud
failure at first plan instead of silent re-apply loop.
Minor findings tracked for plan-phase pickup.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: round-3 inline clarifications per reviewer recommendation
Round 3 found 0 Critical + 2 Important + 3 Minor; reviewer
explicitly recommended doc-level inline clarifications rather
than a round-4 dispatch. Three applied:
1. Data-flow diagram fixed: lock acquired BEFORE
ensureLoginLocked (was misleading; implementer could re-
introduce TOCTOU by following the diagram).
2. A6 tentative JSON envelope spec'd: flat object, not
{"domains":[...]} wrapper. Curl verification gate remains
the first implementation task.
3. I/O-under-lock trade-off documented: SetNameservers holds
c.mu across two HTTP round-trips. Acknowledged as
correctness-over-throughput; production hardening path
(session-scoped CSRF cache) deferred to a follow-up if
field-test reveals contention.
Status flipped to PASS (per reviewer guidance).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: implementation plan for infra.dns_delegation v0.2.0
13-task plan with TDD per-task, structpb-safe Outputs, lock-held
SetNameservers, Read via control-panel endpoint with
ErrEmptyNameservers sentinel, Delete restores stashed
previous_nameservers (fallback to Hover defaults).
Single PR (feat/dns-delegation). Out of scope: workflow-registry
manifest bump + gocodealone-multisite field-test artifact +
live field test (deferred to a separate session post-goreleaser).
Rollback notes per task. Scope Manifest authored.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: plan revision per adversarial review round 1
Addresses 1 Critical + 3 Important + 3 Minor:
CRITICAL: ResourceRef has no InputSnapshot field (verified
workflow/interfaces/iac_provider.go:183-187). Adopted reviewer
option #2: v0.2.0 ships Delete = always reset to Hover defaults
[ns1.hover.com, ns2.hover.com]. Stash-and-restore deferred to
v0.3.0 follow-up requiring interfaces change. Design + plan
updated; previous_nameservers dropped from Outputs entirely.
IMPORTANT: Task 3 newTestClient/rewritingTransport conflicted
with existing newStubClient/rewriteTransport helpers. Rewrote
to use the existing helpers.
IMPORTANT: Task 2 cited line numbers that shift after Task 1.
Switched to function-name references throughout.
IMPORTANT: Task 11 test used nonexistent newTestIaCServerInitialized
helper. Replaced with provider-level TestHoverProvider_Capabilities_
IncludesDelegation against HoverProvider.Capabilities() directly
(pure function; no gRPC harness needed).
MINOR: Added []string-vs-[]any clarifying comment to
putNameserversLocked.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: plan revision round 2 — fix stale stash refs + Red-step tests
Addresses 3 Important + 3 Minor from plan-phase adversarial
review round 2:
IMPORTANT:
- Existing TestHoverIaCServer_Capabilities asserts len==1;
added explicit Step 4b in Task 11 to update it to len==2 +
multi-type assertion. Task 11 commit list now includes
iacserver_test.go.
- Task 3/4/5 Red-step test code rewritten to use existing
newStubClient + rewriteTransport directly (was: newTestClient
references that would compile-error before the "re-author"
note was reached).
- DelegationDriver struct comment + PR body bullet rewritten
to match v0.2.0 spec (no previous_nameservers stash;
fallback-only Delete). Was contradicting Task 7's
delegationOutput which already omits the field.
MINOR (design doc cleanup):
- Removed stale "stash current NS as previous_nameservers"
line from data-flow diagram.
- Removed "Pre-change GetDomainDelegation fails during Create"
row from error-handling table (dead-code path).
- Updated return line: dnsDelegationOutputFromDesired(...,
previous_ns) -> delegationOutput(domain, ns).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: lock scope for hover dns_delegation (alignment passed)
* feat(hover): DomainDelegation type + ErrEmptyNameservers sentinel
* refactor(hover): split ensureLogin into Locked variant
* feat(hover): fetchControlPanelCSRFLocked + csrfMetaRe regex
* feat(hover): GetDomainDelegation method (loud on empty)
* feat(hover): SetNameservers + putNameserversLocked
* feat(drivers): DelegationDriver skeleton + interface
* feat(provider): register DelegationDriver + update Capabilities
* feat: plugin.json declares infra.dns_delegation
* test(drivers): defend Update Outputs shape + ErrEmptyNameservers wrap + HealthCheck ctx
Round-1 review findings:
- TestDelegationDriver_Update_HappyPath now asserts
out.Outputs["nameservers"] is []any (structpb-safe).
Previously discarded the output via `_ = out`.
- TestDelegationDriver_CtxCanceled_AllMethods extended to
cover HealthCheck (which returns (result, nil) on
cancellation; Healthy=false carries the signal).
- New TestDelegationDriver_Read_PropagatesErrEmptyNameservers
verifies errors.Is(driverErr, hover.ErrEmptyNameservers)
survives the driver's error-wrap chain. Callers using the
sentinel to distinguish "Hover returned 0 nameservers"
from other failures now have a regression harness.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(dns_delegation): case-insensitive dedup in parseDelegationSpec
Copilot review finding: parseDelegationSpec used exact-string dedup
(seen[s]) while Update + Diff use strings.EqualFold. DNS hostnames
are case-insensitive, so ["NS1.example.com","ns1.example.com"]
should be rejected as duplicates but slipped through. Lowercase
the key when checking + storing in `seen`; preserve the original
casing in the parsed output for display.
Regression test: TestDelegationDriver_Create_CaseInsensitiveDuplicate_Rejected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: sameNameserverSet normalizes before sort; SetNameservers lock-hold comment
PR #5 round-2 Copilot findings:
1. sameNameserverSet sorted case-sensitively then compared with
strings.EqualFold. Mixed-case multisets could falsely diverge
because sort positions disagreed with the case-insensitive
compare. Lowercased all entries before sort; direct == compare
suffices after normalize. Regression test:
TestDelegationDriver_Diff_CaseInsensitiveMatch.
2. SetNameservers lock-hold comment understated worst-case
duration: ensureLoginLocked may run GET /signin + POST /signin
+ GET /signin/totp + (optional) POST /signin/totp on stale
sessions. Updated comment to enumerate the full request list
and note ~180s worst-case vs ~60s warm-session.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: csrfMeta attr-order/quote-style agnostic + Diff sets NeedsUpdate
PR #5 round-3 Copilot findings:
1. csrfMetaRe assumed name=before-content + double quotes. Hover/
Rails may emit content=before-name or single quotes. Replaced
with two patterns covering both attribute orders and both
quote styles, wrapped in extractCSRFMeta() helper.
Test: TestExtractCSRFMeta_AttributeOrders covers all 4
shapes + the missing case.
2. DelegationDriver.Diff returned NeedsReplace=true with
NeedsUpdate at zero. DNSDriver pattern sets both; some
planner paths gate on NeedsUpdate, risking a skipped
replace. Now sets both for domain-change.
Test: TestDelegationDriver_Diff_DomainChange_SetsBothNeedsUpdateAndReplace.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the real gRPC IaC ResourceDriver for
workflow-plugin-hover. Replaces the scaffold stub with:internal/hover/client.go— undocumented Hover control-panel client modeled on pjslauta/hover-dyn-dns. CSRF + session-cookie auth; conditional RFC 6238 TOTP (skips POST when MFA disabled). NewGetDomainmethod resolves hover-assigned numeric domain IDs (required by CreateRecord).internal/drivers/dns.go—ResourceDriverforinfra.dns. Plan via Diff (multiset matching for multi-A/multi-AAAA on the apex); Apply via Create/Update/Delete; structpb-safe Outputs.internal/iacserver.go—pb.IaCProviderRequired/Finalizer/DriftDetectorservers wiring the driver into wfctl.ComputePlanVersion: "v2".Workflow dep
Pinned to
github.com/GoCodeAlone/workflow v0.60.8(the current latest tag). Earlier scaffold notes mentioned v0.57.1; the implementation tracked latest while landing.Hover API endpoints (undocumented; scraped from pjslauta/hover-dyn-dns)
GET/POST /signin(form + CSRF_token)GET/POST /signin/totp(MFA-conditional; probe + submit when_tokenpresent)GET /api/domains/<name>/dns(returns{domains:[{id, domain_name, entries:[...]}]})POST /api/dns← form:domain_id, name, type, content, ttlPUT /api/dns/<id>← form:content, ttlDELETE /api/dns/<id>Caveats
Delete(infra.dns) is a no-op; Hover has no zone-delete API. Records can still be deleted individually via Update path.Test plan
GOWORK=off go test ./...— all green (45+ tests acrossinternal,internal/drivers,internal/hover)GOWORK=off go build ./...cleangofmt -l .clean🤖 Generated with Claude Code