Fix QA nits from PR #8: error unwrapping, dead code, nil guard #10

New issue

Closed

opened 2026-06-14 17:22:19 +00:00 by ldraney · 1 comment

ldraney commented

2026-06-14 17:22:19 +00:00

Owner

Type

Bug

What Broke

QA review of PR #8 (integration tests) found 5 non-blocking nits. Merged as-is to unblock progress; this ticket tracks the fixes.

Repro Steps

Review PR #8 QA comment

Expected Behavior

All nits resolved:

errors.As inconsistency — dns_test.go: 404 checks after retryOn429 use direct type assertion getErr.(*APIError) which won't match wrapped errors. Use errors.As consistently (affects TestAddAndDeleteRecords and TestReplaceRecordsByType).
Dead code — dns_test.go TestReplaceRecordsByType: otherTXT variable declared, populated via no-op, discarded. Remove or add TODO.
Slice append gotcha — dns_test.go TestReplaceAllRecords: append(existing, DNSRecord{...}) may mutate backing array. Use copy pattern.
Nil dereference risk — domains_test.go TestGetDomain: accesses domain.Domain without nil check.
TXT record preservation gap — dns_test.go TestReplaceRecordsByType: only preserves @ TXT records. Add comment documenting risk.

Environment

Go test files in pkg/godaddy/

Lineage

Parent: none
Source: QA review of PR #8

Repo

godaddy-tofu

Acceptance Criteria

All 404 checks use errors.As instead of direct type assertion
Dead otherTXT code removed or marked TODO
TestReplaceAllRecords uses safe slice copy pattern
TestGetDomain has nil guard before field access
TestReplaceRecordsByType documents TXT preservation risk

PR #8, issue #4

### Type Bug ### What Broke QA review of PR #8 (integration tests) found 5 non-blocking nits. Merged as-is to unblock progress; this ticket tracks the fixes. ### Repro Steps 1. Review PR #8 QA comment ### Expected Behavior All nits resolved: 1. **`errors.As` inconsistency** — `dns_test.go`: 404 checks after `retryOn429` use direct type assertion `getErr.(*APIError)` which won't match wrapped errors. Use `errors.As` consistently (affects `TestAddAndDeleteRecords` and `TestReplaceRecordsByType`). 2. **Dead code** — `dns_test.go` `TestReplaceRecordsByType`: `otherTXT` variable declared, populated via no-op, discarded. Remove or add TODO. 3. **Slice append gotcha** — `dns_test.go` `TestReplaceAllRecords`: `append(existing, DNSRecord{...})` may mutate backing array. Use copy pattern. 4. **Nil dereference risk** — `domains_test.go` `TestGetDomain`: accesses `domain.Domain` without nil check. 5. **TXT record preservation gap** — `dns_test.go` `TestReplaceRecordsByType`: only preserves `@` TXT records. Add comment documenting risk. ### Environment Go test files in `pkg/godaddy/` ### Lineage Parent: none Source: QA review of PR #8 ### Repo godaddy-tofu ### Acceptance Criteria - [ ] All 404 checks use `errors.As` instead of direct type assertion - [ ] Dead `otherTXT` code removed or marked TODO - [ ] `TestReplaceAllRecords` uses safe slice copy pattern - [ ] `TestGetDomain` has nil guard before field access - [ ] `TestReplaceRecordsByType` documents TXT preservation risk ### Related PR #8, issue #4

ldraney commented

2026-06-14 17:48:37 +00:00

Author

Owner

Scope Review -- Dottie

Reviewed the issue body against the actual source files at HEAD (pkg/godaddy/dns_test.go, pkg/godaddy/domains_test.go, pkg/godaddy/client.go).

1. Scope clarity and actionability

All 5 nits reference specific files (dns_test.go, domains_test.go) and specific functions. File targets are correct -- both files exist in pkg/godaddy/. A dev agent can pick this up without additional context.

2. Nit-by-nit verification

#	Nit	Verified	Notes
1	`errors.As` inconsistency	Yes	`TestAddAndDeleteRecords` lines 92/97 and `TestReplaceRecordsByType` line 158 use `getErr.(APIError)` / `err.(APIError)` direct assertions. Currently these work because `client.go` returns `*APIError` unwrapped, but `errors.As` is forward-compatible. Two functions affected, 3 assertion sites total.
2	Dead `otherTXT` code	Yes	`TestReplaceRecordsByType` declares `otherTXT`, runs a no-op `retryOn429` that sets it to nil, then discards it with `_ = otherTXT`. Clear dead code -- ~8 lines to remove.
3	Slice append gotcha	Yes	`TestReplaceAllRecords` line 214: `replacement := append(existing, ...)` reuses `existing`'s backing array. Fix is `replacement := make([]DNSRecord, len(existing)); copy(replacement, existing); replacement = append(replacement, ...)`. Note: this test is `t.Skip`'d so the risk is latent, but the fix is still correct practice.
4	Nil dereference in `TestGetDomain`	Yes	`domains_test.go` line 47: `domain.Domain` accessed without checking `domain != nil`. If `GetDomain` returns `(nil, nil)` this panics. Simple `if domain == nil { t.Fatal(...) }` guard needed before field access.
5	TXT preservation gap	Yes	`TestReplaceRecordsByType` only fetches `@` TXT records before the full-type replace. Any non-`@` TXT records (except the test name) would be silently deleted. The ask is just a comment documenting this risk -- appropriate for a test-only concern.

3. Acceptance criteria assessment

All 5 acceptance criteria are testable:

Criteria 1: grep for direct *APIError assertions -- should be zero after fix
Criteria 2: grep for otherTXT -- should be absent
Criteria 3: Code inspection for copy pattern in TestReplaceAllRecords
Criteria 4: domain == nil guard present before domain.Domain access
Criteria 5: Comment present in TestReplaceRecordsByType about TXT preservation limitation

4. Missing items (minor)

No labels assigned (e.g., nit, test, code-quality)
No assignee set
The issue does not specify a branch name convention, but this is covered by platform SOPs so not a blocker

Verdict: APPROVED

The scope is well-defined, all 5 nits are verified against the actual source, file targets are accurate, and acceptance criteria are testable. A dev agent can implement this without ambiguity.

## Scope Review -- Dottie Reviewed the issue body against the actual source files at HEAD (`pkg/godaddy/dns_test.go`, `pkg/godaddy/domains_test.go`, `pkg/godaddy/client.go`). ### 1. Scope clarity and actionability All 5 nits reference specific files (`dns_test.go`, `domains_test.go`) and specific functions. File targets are correct -- both files exist in `pkg/godaddy/`. A dev agent can pick this up without additional context. ### 2. Nit-by-nit verification | # | Nit | Verified | Notes | |---|-----|----------|-------| | 1 | `errors.As` inconsistency | Yes | `TestAddAndDeleteRecords` lines 92/97 and `TestReplaceRecordsByType` line 158 use `getErr.(*APIError)` / `err.(*APIError)` direct assertions. Currently these work because `client.go` returns `*APIError` unwrapped, but `errors.As` is forward-compatible. Two functions affected, 3 assertion sites total. | | 2 | Dead `otherTXT` code | Yes | `TestReplaceRecordsByType` declares `otherTXT`, runs a no-op `retryOn429` that sets it to nil, then discards it with `_ = otherTXT`. Clear dead code -- ~8 lines to remove. | | 3 | Slice append gotcha | Yes | `TestReplaceAllRecords` line 214: `replacement := append(existing, ...)` reuses `existing`'s backing array. Fix is `replacement := make([]DNSRecord, len(existing)); copy(replacement, existing); replacement = append(replacement, ...)`. Note: this test is `t.Skip`'d so the risk is latent, but the fix is still correct practice. | | 4 | Nil dereference in `TestGetDomain` | Yes | `domains_test.go` line 47: `domain.Domain` accessed without checking `domain != nil`. If `GetDomain` returns `(nil, nil)` this panics. Simple `if domain == nil { t.Fatal(...) }` guard needed before field access. | | 5 | TXT preservation gap | Yes | `TestReplaceRecordsByType` only fetches `@` TXT records before the full-type replace. Any non-`@` TXT records (except the test name) would be silently deleted. The ask is just a comment documenting this risk -- appropriate for a test-only concern. | ### 3. Acceptance criteria assessment All 5 acceptance criteria are testable: - Criteria 1: `grep` for direct `*APIError` assertions -- should be zero after fix - Criteria 2: `grep` for `otherTXT` -- should be absent - Criteria 3: Code inspection for copy pattern in `TestReplaceAllRecords` - Criteria 4: `domain == nil` guard present before `domain.Domain` access - Criteria 5: Comment present in `TestReplaceRecordsByType` about TXT preservation limitation ### 4. Missing items (minor) - No labels assigned (e.g., `nit`, `test`, `code-quality`) - No assignee set - The issue does not specify a branch name convention, but this is covered by platform SOPs so not a blocker ### Verdict: APPROVED The scope is well-defined, all 5 nits are verified against the actual source, file targets are accurate, and acceptance criteria are testable. A dev agent can implement this without ambiguity.