Skip to content

fix(cert-manager): await profile slug resolution before certificate i…

62686d2
Select commit
Loading
Failed to load commit list.
Open

fix(cert-manager): await profile slug resolution before certificate issuance #175

fix(cert-manager): await profile slug resolution before certificate i…
62686d2
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Apr 10, 2026 in 7m 30s

Code review found 1 important issue

Found 5 candidates, confirmed 4. See review comments for details.

Details

Severity Count
🔴 Important 1
🟡 Nit 2
🟣 Pre-existing 0
Severity File:Line Issue
🔴 Important packages/cmd/agent.go:2527-2531 Misleading retry log and missing error in issuance failure summary
🟡 Nit packages/cmd/agent_cert_resolution_test.go:40-48 require.Equal in HTTP handler goroutine causes misleading test failure
🟡 Nit packages/cmd/agent_cert_resolution_test.go:63-90 TestSlugResolutionCompletesBeforeIssuance does not test the race condition fix

Annotations

Check failure on line 2531 in packages/cmd/agent.go

See this annotation in the file changed.

@claude claude / Claude Code Review

Misleading retry log and missing error in issuance failure summary

The final issuance failure summary log has two bugs: it incorrectly states the certificate 'will retry on next renewal check' (it will not — the cert is permanently stuck in `failed` status until the agent restarts), and it omits `lastErr` so operators see no root cause in the summary. Fix by updating the message to say no automatic retry will occur and adding `.Err(lastErr)` to the log call.

Check warning on line 48 in packages/cmd/agent_cert_resolution_test.go

See this annotation in the file changed.

@claude claude / Claude Code Review

require.Equal in HTTP handler goroutine causes misleading test failure

Using `require.Equal` inside an `httptest.Server` handler goroutine at line 40 of `agent_cert_resolution_test.go` is a Go testing anti-pattern: `require.Equal` calls `t.FailNow()` → `runtime.Goexit()`, which only terminates the handler goroutine, not the test goroutine. This leaves the HTTP response unwritten, causing the client to receive a connection reset, and the test then fails on `require.NoError` with a misleading error instead of the actual `projectId` mismatch message. Replace `require.

Check warning on line 90 in packages/cmd/agent_cert_resolution_test.go

See this annotation in the file changed.

@claude claude / Claude Code Review

TestSlugResolutionCompletesBeforeIssuance does not test the race condition fix

TestSlugResolutionCompletesBeforeIssuance does not actually test the race condition fix — it calls resolveCertificateNameReferences() and api.CallIssueCertificate() sequentially and explicitly in the test body, so the ordering assertion is trivially true by construction. The test would pass identically against the pre-fix buggy code, providing false confidence that the goroutine-ordering race is covered.