From f638995a81511125320f3c4347021a371222a912 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Sun, 6 Sep 2026 15:27:12 -0500 Subject: [PATCH] fix(retention): clear the lint findings in the retention plugin golangci-lint reports ten findings in plugins/retention on main, which fails the Lint job on every open PR regardless of what that PR touches. Worth knowing that CI's output understates the problem. golangci-lint defaults to max-same-issues: 3, so it printed three bodyclose findings where there were twenty-two. Fixing only the named lines surfaces the next three. What changed: bodyclose, 22 sites. fakeResp builds a synthetic *http.Response for classifyHTTPError to inspect. It has no Body, because classification reads only the status and headers and nothing came from a real transport. The linter flags the call sites because the function returns *http.Response, so they carry a directive; closing a nil Body would panic. The reasoning lives on fakeResp itself rather than being repeated at each site. errcheck, store_models.go. Job.Payload is a map[string]string, which has no channel, func or cyclic value that could make json.Marshal fail. Documented rather than given an error path that can never run. If Payload ever holds something richer, this needs revisiting. govet shadow, provider_hubspot.go. The inner err in the PATCH branch shadowed the one from findContactByEmail; renamed to patchErr. revive unused-parameter, 6 handlers. Renamed r to _ where the handler ignores the request. Only those; the other handlers in these files do read r. unparam, consent_test.go. Every caller passed purpose as "marketing", so the parameter carried no information. Dropped it and bound the value as a local const. Take a parameter again if a test needs a second purpose. unused, provider_generic.go. Dropped the classifierPolicyDecided constant, which no longer gated anything, and kept its rationale as a comment. The point it makes is worth keeping: an unwritten retry policy that only surfaced at the first CRM error would decide by accident whether a transient 503 permanently drops a customer's sync. Full-repo lint is clean and all 98 packages pass. --- plugins/retention/consent_test.go | 17 +++--- plugins/retention/provider_generic.go | 16 +++--- .../provider_generic_classify_test.go | 53 ++++++++++--------- plugins/retention/provider_generic_test.go | 6 +-- plugins/retention/provider_hubspot.go | 4 +- plugins/retention/store_models.go | 5 +- 6 files changed, 55 insertions(+), 46 deletions(-) diff --git a/plugins/retention/consent_test.go b/plugins/retention/consent_test.go index c21ef6d4..3bbc68a8 100644 --- a/plugins/retention/consent_test.go +++ b/plugins/retention/consent_test.go @@ -11,7 +11,12 @@ import ( ) // policy builds a fixed consentPolicy for tests. -func policy(require bool, purpose string) func(context.Context, id.AppID) (bool, string) { +// policy builds a consent policy for the tests. The purpose is fixed at +// "marketing" because that is the only purpose these tests exercise; take a +// parameter again if a test needs a second one. +func policy(require bool) func(context.Context, id.AppID) (bool, string) { + const purpose = "marketing" + return func(context.Context, id.AppID) (bool, string) { return require, purpose } } @@ -28,7 +33,7 @@ func (s *stubConsent) HasConsent(context.Context, id.UserID, id.AppID, string) ( func TestAllowSendPassesWhenGateDisabled(t *testing.T) { p := New() - p.consentPolicy = policy(false, "marketing") + p.consentPolicy = policy(false) p.consent = &stubConsent{granted: false} ok, _, err := p.allowSend(context.Background(), &Job{}) @@ -39,7 +44,7 @@ func TestAllowSendPassesWhenGateDisabled(t *testing.T) { func TestAllowSendBlocksWithoutGrant(t *testing.T) { p := New() - p.consentPolicy = policy(true, "marketing") + p.consentPolicy = policy(true) p.consent = &stubConsent{granted: false} ok, reason, err := p.allowSend(context.Background(), &Job{}) @@ -50,7 +55,7 @@ func TestAllowSendBlocksWithoutGrant(t *testing.T) { func TestAllowSendPassesWithGrant(t *testing.T) { p := New() - p.consentPolicy = policy(true, "marketing") + p.consentPolicy = policy(true) p.consent = &stubConsent{granted: true} ok, _, err := p.allowSend(context.Background(), &Job{}) @@ -60,7 +65,7 @@ func TestAllowSendPassesWithGrant(t *testing.T) { func TestAllowSendBlocksWhenGateOnButConsentUnavailable(t *testing.T) { p := New() - p.consentPolicy = policy(true, "marketing") + p.consentPolicy = policy(true) p.consent = nil // consent plugin not registered ok, reason, err := p.allowSend(context.Background(), &Job{}) @@ -72,7 +77,7 @@ func TestAllowSendBlocksWhenGateOnButConsentUnavailable(t *testing.T) { func TestAllowSendReportsLookupFailureAsLocalError(t *testing.T) { p := New() - p.consentPolicy = policy(true, "marketing") + p.consentPolicy = policy(true) p.consent = &stubConsent{err: assert.AnError} ok, reason, err := p.allowSend(context.Background(), &Job{}) diff --git a/plugins/retention/provider_generic.go b/plugins/retention/provider_generic.go index fa02afaa..547fa2ef 100644 --- a/plugins/retention/provider_generic.go +++ b/plugins/retention/provider_generic.go @@ -27,16 +27,12 @@ type GenericProvider struct { client *http.Client } -// classifierPolicyDecided marks that the retry-classification policy in -// classifyHTTPError has been written and recorded in the spec (see -// "Retry classification" in -// docs/superpowers/specs/2026-09-03-crm-retention-delivery-design.md). It -// used to gate construction while the policy was still an open decision; now -// that it is decided, the constant just documents that the guard was here -// and why: an unwritten policy that only surfaced at the first CRM error -// would decide, by accident, whether a transient 503 permanently drops a -// customer's sync. -const classifierPolicyDecided = true +// The retry-classification policy in classifyHTTPError is written down in the +// spec, under "Retry classification" in +// docs/superpowers/specs/2026-09-03-crm-retention-delivery-design.md. It +// matters that it stays written down: an unwritten policy that only surfaced +// at the first CRM error would decide, by accident, whether a transient 503 +// permanently drops a customer's sync. // NewGenericProvider builds a Provider from a ProviderConfig. ContactURL is // required: Capabilities always advertises CapContacts, so a provider that diff --git a/plugins/retention/provider_generic_classify_test.go b/plugins/retention/provider_generic_classify_test.go index bff048d9..242b68c5 100644 --- a/plugins/retention/provider_generic_classify_test.go +++ b/plugins/retention/provider_generic_classify_test.go @@ -21,6 +21,11 @@ import ( // fakeResp builds a *http.Response carrying just what classifyHTTPError and // retryAfter look at: status, status code, and headers. No network involved. +// fakeResp builds a response for classifyHTTPError to inspect. It has no +// Body: classification reads only the status and headers, and nothing here +// came from a real transport, so there is no reader to drain. bodyclose +// still flags the call sites because the function returns *http.Response, +// hence the directives there. Closing a nil Body would panic. func fakeResp(status int, header http.Header) *http.Response { if header == nil { header = http.Header{} @@ -57,105 +62,105 @@ func TestClassifyHTTPError_Table(t *testing.T) { }, { name: "429 with Retry-After delta-seconds", - resp: fakeResp(http.StatusTooManyRequests, headerWithRetryAfter("120")), + resp: fakeResp(http.StatusTooManyRequests, headerWithRetryAfter("120")), //nolint:bodyclose // synthetic response, Body is nil retryable: true, retryAfter: 2 * time.Minute, }, { name: "429 with Retry-After clamped to 30m ceiling", - resp: fakeResp(http.StatusTooManyRequests, headerWithRetryAfter("99999")), + resp: fakeResp(http.StatusTooManyRequests, headerWithRetryAfter("99999")), //nolint:bodyclose // synthetic response, Body is nil retryable: true, retryAfter: 30 * time.Minute, }, { name: "429 with Retry-After floored to 1s", - resp: fakeResp(http.StatusTooManyRequests, headerWithRetryAfter("0")), + resp: fakeResp(http.StatusTooManyRequests, headerWithRetryAfter("0")), //nolint:bodyclose // synthetic response, Body is nil retryable: true, retryAfter: time.Second, }, { name: "429 with garbage Retry-After falls back to backoff", - resp: fakeResp(http.StatusTooManyRequests, headerWithRetryAfter("banana")), + resp: fakeResp(http.StatusTooManyRequests, headerWithRetryAfter("banana")), //nolint:bodyclose // synthetic response, Body is nil retryable: true, retryAfter: 0, }, { name: "429 with no Retry-After header", - resp: fakeResp(http.StatusTooManyRequests, nil), + resp: fakeResp(http.StatusTooManyRequests, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: true, retryAfter: 0, }, { name: "401 unauthorized", - resp: fakeResp(http.StatusUnauthorized, nil), + resp: fakeResp(http.StatusUnauthorized, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: true, retryAfter: 2 * time.Minute, }, { name: "403 forbidden", - resp: fakeResp(http.StatusForbidden, nil), + resp: fakeResp(http.StatusForbidden, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: true, retryAfter: 2 * time.Minute, }, { name: "404 drops the ref", - resp: fakeResp(http.StatusNotFound, nil), + resp: fakeResp(http.StatusNotFound, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: true, dropRef: true, }, { name: "500 internal server error", - resp: fakeResp(http.StatusInternalServerError, nil), + resp: fakeResp(http.StatusInternalServerError, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: true, }, { name: "502 bad gateway", - resp: fakeResp(http.StatusBadGateway, nil), + resp: fakeResp(http.StatusBadGateway, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: true, }, { name: "503 service unavailable", - resp: fakeResp(http.StatusServiceUnavailable, nil), + resp: fakeResp(http.StatusServiceUnavailable, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: true, }, { name: "504 gateway timeout", - resp: fakeResp(http.StatusGatewayTimeout, nil), + resp: fakeResp(http.StatusGatewayTimeout, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: true, }, { name: "501 not implemented is terminal", - resp: fakeResp(http.StatusNotImplemented, nil), + resp: fakeResp(http.StatusNotImplemented, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: false, }, { name: "400 bad request is terminal", - resp: fakeResp(http.StatusBadRequest, nil), + resp: fakeResp(http.StatusBadRequest, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: false, }, { name: "422 unprocessable entity is terminal", - resp: fakeResp(http.StatusUnprocessableEntity, nil), + resp: fakeResp(http.StatusUnprocessableEntity, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: false, }, { name: "413 payload too large is terminal", - resp: fakeResp(http.StatusRequestEntityTooLarge, nil), + resp: fakeResp(http.StatusRequestEntityTooLarge, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: false, }, { name: "408 request timeout", - resp: fakeResp(http.StatusRequestTimeout, nil), + resp: fakeResp(http.StatusRequestTimeout, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: true, }, { name: "409 conflict", - resp: fakeResp(http.StatusConflict, nil), + resp: fakeResp(http.StatusConflict, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: true, }, { name: "418 unrecognised status is terminal", - resp: fakeResp(http.StatusTeapot, nil), + resp: fakeResp(http.StatusTeapot, nil), //nolint:bodyclose // synthetic response, Body is nil retryable: false, }, } @@ -180,7 +185,7 @@ func TestClassifyHTTPError_NilResponsePreservesTransportError(t *testing.T) { func TestRetryAfter_HTTPDateForm(t *testing.T) { when := time.Now().Add(90 * time.Second).UTC() - resp := fakeResp(http.StatusTooManyRequests, headerWithRetryAfter(when.Format(http.TimeFormat))) + resp := fakeResp(http.StatusTooManyRequests, headerWithRetryAfter(when.Format(http.TimeFormat))) //nolint:bodyclose // synthetic response, Body is nil got := retryAfter(resp) assert.InDelta(t, 90*time.Second, got, float64(5*time.Second), @@ -189,7 +194,7 @@ func TestRetryAfter_HTTPDateForm(t *testing.T) { func TestClassifyHTTPError_429HTTPDateRetryAfter(t *testing.T) { when := time.Now().Add(90 * time.Second).UTC() - resp := fakeResp(http.StatusTooManyRequests, headerWithRetryAfter(when.Format(http.TimeFormat))) + resp := fakeResp(http.StatusTooManyRequests, headerWithRetryAfter(when.Format(http.TimeFormat))) //nolint:bodyclose // synthetic response, Body is nil pe := classifyHTTPError(resp, nil, nil) require.NotNil(t, pe) @@ -214,7 +219,7 @@ func TestTruncate(t *testing.T) { // ────────────────────────────────────────────────── func TestGenericProvider_UpsertContact_404EndToEnd(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"error":"contact not found"}`)) })) @@ -234,7 +239,7 @@ func TestGenericProvider_UpsertContact_404EndToEnd(t *testing.T) { } func TestGenericProvider_UpsertContact_429WithRetryAfterEndToEnd(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Retry-After", "45") w.WriteHeader(http.StatusTooManyRequests) _, _ = w.Write([]byte(`slow down`)) @@ -255,7 +260,7 @@ func TestGenericProvider_UpsertContact_429WithRetryAfterEndToEnd(t *testing.T) { } func TestGenericProvider_UpsertContact_400IsTerminalEndToEnd(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusBadRequest) _, _ = w.Write([]byte(`{"error":"invalid email"}`)) })) diff --git a/plugins/retention/provider_generic_test.go b/plugins/retention/provider_generic_test.go index df213446..eac224c5 100644 --- a/plugins/retention/provider_generic_test.go +++ b/plugins/retention/provider_generic_test.go @@ -67,7 +67,7 @@ func TestGenericProviderName(t *testing.T) { } func TestGenericProviderUpsertContact_Success(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(`{"id":"remote-501"}`)) @@ -170,7 +170,7 @@ func TestGenericProviderUpsertContact_FieldMapRenamesOutgoingFields(t *testing.T } func TestGenericProviderUpsertContact_RemoteIDDefaultsToID(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(`{"id":"default-id-1","other":"ignored"}`)) })) @@ -185,7 +185,7 @@ func TestGenericProviderUpsertContact_RemoteIDDefaultsToID(t *testing.T) { } func TestGenericProviderUpsertContact_RemoteIDFromConfiguredPath(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(`{"result":{"contact_id":"nested-777"}}`)) })) diff --git a/plugins/retention/provider_hubspot.go b/plugins/retention/provider_hubspot.go index 16078a14..fff25246 100644 --- a/plugins/retention/provider_hubspot.go +++ b/plugins/retention/provider_hubspot.go @@ -152,8 +152,8 @@ func (h *HubSpotProvider) UpsertContact(ctx context.Context, c *Contact) (Remote if remoteID != "" { path := hubspotContactsPath + "/" + remoteID - if _, err := h.request(ctx, http.MethodPatch, path, map[string]interface{}{"properties": props}); err != nil { - return RemoteRef{}, err + if _, patchErr := h.request(ctx, http.MethodPatch, path, map[string]interface{}{"properties": props}); patchErr != nil { + return RemoteRef{}, patchErr } return RemoteRef{Provider: h.name, ObjectType: "contact", ID: remoteID}, nil } diff --git a/plugins/retention/store_models.go b/plugins/retention/store_models.go index f5638d74..8f5e9a0b 100644 --- a/plugins/retention/store_models.go +++ b/plugins/retention/store_models.go @@ -47,7 +47,10 @@ type jobModel struct { } func fromJob(j *Job) *jobModel { - payload, _ := json.Marshal(j.Payload) + // Payload is a map[string]string, which has no channel, func or cyclic + // value that could make Marshal fail, so there is no error path here to + // handle. Marshalling anything richer would need this revisited. + payload, _ := json.Marshal(j.Payload) //nolint:errcheck // cannot fail for map[string]string m := &jobModel{ ID: j.ID.String(), AppID: j.AppID.String(), EnvID: j.EnvID.String(), UserID: j.UserID.String(), Provider: j.Provider, Kind: j.Kind,