test(coverage): drive storageprovider/{dospaces,s3} to >=95% (#31)

Caps the cross-repo 95% coverage push for `common`. After PRs #25-#30
landed for crypto/nats/plans/readiness/r2/logctx, the two remaining
storage backends sat at 63.6% (dospaces) and 72.2% (s3).

dospaces -> 100.0% via accessor + customerEndpointURL fallback coverage:
all simple getters (MasterAccessKey, MasterSecretKey, Endpoint, PublicURL,
Bucket, Region, UseTLS) plus the scheme-prefix and already-schemed
endpoint branches inside customerEndpointURL().

s3 -> 97.2% via accessor + IssueTenantCredentials default-fallback +
safeSessionName branch coverage: digits/dashes/underscores retained,
RoleSessionName truncation at 56, customerEndpointURL passthrough for
already-schemed endpoints, RevokeTenantCredentials no-op confirmation,
and AssumeRole error propagation through IssueTenantCredentials.

Total: 94.5% -> 98.0%. Every package now >=95%.

Coverage block (rule 17):

  Symptom:        common total 94.5% < 95% target; per-pkg dospaces/s3 below floor
  Enumeration:    go tool cover -func | sort | grep "<100"
  Sites found:    2 (storageprovider/dospaces, storageprovider/s3)
  Sites touched:  both
  Coverage test:  TestRegistry_AllProvidersSatisfyContract + per-backend
                  registry-walk tests already in place; new coverage files
                  add accessor + edge-branch assertions.
  Live verified:  go test ./... -short -coverprofile cov.out; total = 98.0%

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: mastermanas805

storageprovider/dospaces/dospaces_coverage_test.go

@@ -0,0 +1,90 @@
+package dospaces_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"instant.dev/common/storageprovider"
+	"instant.dev/common/storageprovider/dospaces"
+)
+
+// TestGetters_AndCustomerEndpointFallbacks exercises every simple accessor on
+// the Provider plus the fallback paths inside customerEndpointURL() that the
+// happy-path tests don't reach.
+//
+// Why each branch matters:
+//
+//   - MasterAccessKey / MasterSecretKey / Endpoint / Bucket / Region / UseTLS
+//     are read by the api at boot when building the presigner for broker mode;
+//     if any returns "" we ship an unsigned URL into a customer's logs.
+//   - PublicURL() with publicURL=="" must fall through customerEndpointURL()
+//     so we never return an empty string (broker-mode would silently 404).
+//   - customerEndpointURL() with UseTLS=false should prepend "http://", and
+//     when the endpoint already carries a scheme (some operators wire
+//     `https://...`) it should pass through unmodified.
+func TestGetters_AndCustomerEndpointFallbacks(t *testing.T) {
+	t.Run("all_getters_return_configured_values", func(t *testing.T) {
+		p, err := dospaces.New(storageprovider.Config{
+			Endpoint:     "nyc3.digitaloceanspaces.com",
+			PublicURL:    "https://s3.instanode.dev",
+			Region:       "nyc3",
+			Bucket:       "instant-shared",
+			MasterKey:    "MK",
+			MasterSecret: "MS",
+			UseTLS:       true,
+		})
+		require.NoError(t, err)
+		prov := p.(*dospaces.Provider)
+
+		assert.Equal(t, "MK", prov.MasterAccessKey())
+		assert.Equal(t, "MS", prov.MasterSecretKey())
+		assert.Equal(t, "nyc3.digitaloceanspaces.com", prov.Endpoint())
+		assert.Equal(t, "https://s3.instanode.dev", prov.PublicURL())
+		assert.Equal(t, "instant-shared", prov.Bucket())
+		assert.Equal(t, "nyc3", prov.Region())
+		assert.True(t, prov.UseTLS())
+	})
+
+	t.Run("public_url_falls_back_to_scheme_prefixed_endpoint_no_tls", func(t *testing.T) {
+		p, err := dospaces.New(storageprovider.Config{
+			Endpoint:     "minio.local:9000",
+			MasterKey:    "MK",
+			MasterSecret: "MS",
+			UseTLS:       false,
+		})
+		require.NoError(t, err)
+		prov := p.(*dospaces.Provider)
+
+		// PublicURL must fall back to customerEndpointURL ("http://minio.local:9000").
+		assert.Equal(t, "http://minio.local:9000", prov.PublicURL())
+		assert.False(t, prov.UseTLS())
+	})
+
+	t.Run("public_url_falls_back_to_scheme_prefixed_endpoint_tls", func(t *testing.T) {
+		p, err := dospaces.New(storageprovider.Config{
+			Endpoint:     "nyc3.digitaloceanspaces.com",
+			MasterKey:    "MK",
+			MasterSecret: "MS",
+			UseTLS:       true,
+		})
+		require.NoError(t, err)
+		prov := p.(*dospaces.Provider)
+		assert.Equal(t, "https://nyc3.digitaloceanspaces.com", prov.PublicURL())
+	})
+
+	t.Run("endpoint_with_existing_scheme_passes_through", func(t *testing.T) {
+		// Some operator wires OBJECT_STORE_ENDPOINT=https://foo.bar — the
+		// customerEndpointURL must NOT double-prefix.
+		p, err := dospaces.New(storageprovider.Config{
+			Endpoint:     "https://already-schemed.example",
+			MasterKey:    "MK",
+			MasterSecret: "MS",
+			UseTLS:       false,
+		})
+		require.NoError(t, err)
+		prov := p.(*dospaces.Provider)
+		assert.Equal(t, "https://already-schemed.example", prov.PublicURL())
+	})
+}

storageprovider/s3/s3_coverage_test.go

@@ -0,0 +1,216 @@
+package s3_test
+
+import (
+	"context"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"instant.dev/common/storageprovider"
+	"instant.dev/common/storageprovider/s3"
+)
+
+// TestS3_New_RequiresMasterCreds — sts:AssumeRole requires platform creds.
+func TestS3_New_RequiresMasterCreds(t *testing.T) {
+	_, err := s3.New(storageprovider.Config{
+		AWSRoleARN: "arn:aws:iam::123:role/x",
+	})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "OBJECT_STORE_ACCESS_KEY")
+}
+
+// TestS3_Getters_AndCustomerEndpointFallbacks covers Master*/Endpoint/Bucket/
+// Region/PublicURL/Name + the two customerEndpointURL branches (already-schemed
+// and bare-host).
+func TestS3_Getters_AndCustomerEndpointFallbacks(t *testing.T) {
+	t.Run("bare_host_gets_https_prefix", func(t *testing.T) {
+		raw, err := s3.New(storageprovider.Config{
+			AWSRoleARN:   "arn:aws:iam::123:role/x",
+			MasterKey:    "MK",
+			MasterSecret: "MS",
+		})
+		require.NoError(t, err)
+		p := raw.(*s3.Provider)
+
+		assert.Equal(t, "MK", p.MasterAccessKey())
+		assert.Equal(t, "MS", p.MasterSecretKey())
+		assert.Equal(t, "s3.us-east-1.amazonaws.com", p.Endpoint())
+		assert.Equal(t, "instant-shared", p.Bucket())
+		assert.Equal(t, "us-east-1", p.Region())
+		assert.Equal(t, "s3", p.Name())
+		assert.Equal(t, "https://s3.us-east-1.amazonaws.com", p.PublicURL())
+	})
+
+	t.Run("endpoint_already_schemed_passes_through", func(t *testing.T) {
+		raw, err := s3.New(storageprovider.Config{
+			Endpoint:     "https://custom.example",
+			AWSRoleARN:   "arn:aws:iam::123:role/x",
+			MasterKey:    "MK",
+			MasterSecret: "MS",
+		})
+		require.NoError(t, err)
+		p := raw.(*s3.Provider)
+		assert.Equal(t, "https://custom.example", p.PublicURL())
+	})
+
+	t.Run("public_url_set_overrides_endpoint", func(t *testing.T) {
+		raw, err := s3.New(storageprovider.Config{
+			Endpoint:     "s3.amazonaws.com",
+			PublicURL:    "https://cdn.example.dev",
+			AWSRoleARN:   "arn:aws:iam::123:role/x",
+			MasterKey:    "MK",
+			MasterSecret: "MS",
+		})
+		require.NoError(t, err)
+		p := raw.(*s3.Provider)
+		assert.Equal(t, "https://cdn.example.dev", p.PublicURL())
+	})
+
+	t.Run("region_defaults_and_bucket_defaults_apply", func(t *testing.T) {
+		raw, err := s3.New(storageprovider.Config{
+			AWSRoleARN:   "arn:aws:iam::123:role/x",
+			MasterKey:    "MK",
+			MasterSecret: "MS",
+		})
+		require.NoError(t, err)
+		p := raw.(*s3.Provider)
+		assert.Equal(t, "us-east-1", p.Region())
+		assert.Equal(t, "instant-shared", p.Bucket())
+	})
+}
+
+// TestS3_RevokeTenantCredentials_IsNoOp — STS sessions can't be revoked, the
+// method must return nil regardless of input.
+func TestS3_RevokeTenantCredentials_IsNoOp(t *testing.T) {
+	raw, err := s3.New(storageprovider.Config{
+		AWSRoleARN:   "arn:aws:iam::123:role/x",
+		MasterKey:    "MK",
+		MasterSecret: "MS",
+	})
+	require.NoError(t, err)
+	p := raw.(*s3.Provider)
+
+	require.NoError(t, p.RevokeTenantCredentials(context.Background(), ""))
+	require.NoError(t, p.RevokeTenantCredentials(context.Background(), "any-key"))
+}
+
+// TestS3_IssueTenantCredentials_DefaultsTTLAndPrefix covers:
+//   - TTL==0 falls through to the 1-hour default
+//   - Empty Prefix falls back to ResourceToken
+//   - Empty Bucket falls back to provider default
+func TestS3_IssueTenantCredentials_DefaultsTTLAndPrefix(t *testing.T) {
+	raw, err := s3.New(storageprovider.Config{
+		Region:       "us-west-2",
+		Bucket:       "my-default",
+		AWSRoleARN:   "arn:aws:iam::123:role/x",
+		MasterKey:    "MK",
+		MasterSecret: "MS",
+	})
+	require.NoError(t, err)
+	p := raw.(*s3.Provider)
+
+	var captured s3.AssumeRoleInput
+	p.SetAssumeRoleFunc(func(ctx context.Context, in s3.AssumeRoleInput) (*s3.AssumeRoleOutput, error) {
+		captured = in
+		return &s3.AssumeRoleOutput{
+			AccessKeyID:     "AK",
+			SecretAccessKey: "SK",
+			SessionToken:    "TOK",
+			Expiration:      time.Now().Add(time.Hour),
+		}, nil
+	})
+
+	creds, err := p.IssueTenantCredentials(context.Background(), storageprovider.IssueRequest{
+		ResourceToken: "tok-ABC",
+		// No Prefix, no Bucket, no TTL — exercises every fallback line.
+	})
+	require.NoError(t, err)
+	assert.Equal(t, "tok-ABC", creds.Prefix, "prefix must default to resource token")
+	assert.Equal(t, "my-default", creds.Bucket, "bucket must default to provider default")
+	assert.Equal(t, int32(3600), captured.DurationSeconds, "TTL==0 must default to 1h")
+}
+
+// TestS3_SafeSessionName covers the trimming + min-length fallback. RoleSessionName
+// is constrained by AWS to [\w+=,.@-] and length 2..64; safeSessionName must drop
+// disallowed characters AND backfill "instanode-x" when the result is too short
+// to be a valid session name.
+func TestS3_SafeSessionName(t *testing.T) {
+	raw, err := s3.New(storageprovider.Config{
+		AWSRoleARN:   "arn:aws:iam::123:role/x",
+		MasterKey:    "MK",
+		MasterSecret: "MS",
+	})
+	require.NoError(t, err)
+	p := raw.(*s3.Provider)
+
+	cases := []struct {
+		name      string
+		token     string
+		expected  string
+	}{
+		{
+			name:     "all_disallowed_chars_falls_back_to_instanode_x",
+			token:    "!!!@@@###",
+			expected: "instanode-instanode-x",
+		},
+		{
+			name:     "long_token_truncated_to_56",
+			token:    strings.Repeat("a", 200),
+			expected: "instanode-" + strings.Repeat("a", 56),
+		},
+		{
+			name:     "digits_underscores_dashes_kept",
+			token:    "tenant-01_abc",
+			expected: "instanode-tenant-01_abc",
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			var captured s3.AssumeRoleInput
+			p.SetAssumeRoleFunc(func(ctx context.Context, in s3.AssumeRoleInput) (*s3.AssumeRoleOutput, error) {
+				captured = in
+				return &s3.AssumeRoleOutput{
+					AccessKeyID:     "AK",
+					SecretAccessKey: "SK",
+					SessionToken:    "TOK",
+					Expiration:      time.Now().Add(15 * time.Minute),
+				}, nil
+			})
+			_, err := p.IssueTenantCredentials(context.Background(), storageprovider.IssueRequest{
+				ResourceToken: c.token,
+				TTL:           15 * time.Minute,
+			})
+			require.NoError(t, err)
+			assert.Equal(t, c.expected, captured.RoleSessionName)
+		})
+	}
+}
+
+// TestS3_AssumeRoleError_Propagates verifies the error from a failing
+// AssumeRole call surfaces to the caller rather than being swallowed into
+// a "best-effort" silent return.
+func TestS3_AssumeRoleError_Propagates(t *testing.T) {
+	raw, err := s3.New(storageprovider.Config{
+		AWSRoleARN:   "arn:aws:iam::123:role/x",
+		MasterKey:    "MK",
+		MasterSecret: "MS",
+	})
+	require.NoError(t, err)
+	p := raw.(*s3.Provider)
+	p.SetAssumeRoleFunc(func(ctx context.Context, in s3.AssumeRoleInput) (*s3.AssumeRoleOutput, error) {
+		return nil, assertError("aws-sdk: throttled")
+	})
+	_, err = p.IssueTenantCredentials(context.Background(), storageprovider.IssueRequest{
+		ResourceToken: "x",
+		TTL:           15 * time.Minute,
+	})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "throttled")
+}
+
+type assertError string
+
+func (a assertError) Error() string { return string(a) }