[v4] S3/Valkey helpers reject delimiter characters in tenantID — may break existing key patterns

[gandalf-at-lerian]

Context

In the v4 unification (PR #336), the S3 and Valkey tenant-key helpers added validation that rejects tenant IDs containing delimiter characters.

Where to change

S3 helpers

File: commons/tenant-manager/s3/objectstorage.go (v4.0.0-beta.2)

Line 34 (GetObjectStorageKey):

if strings.Contains(tenantID, "/") {
    return "", fmt.Errorf("tenantID must not contain path delimiter '/': %q", tenantID)
}

Line 81 (StripObjectStoragePrefix):

if strings.Contains(tenantID, "/") {
    return "", fmt.Errorf("tenantID must not contain path delimiter '/': %q", tenantID)
}

Valkey helpers

File: commons/tenant-manager/valkey/keys.go (v4.0.0-beta.2)

Line 26 (GetKey):

if strings.Contains(tenantID, ":") {
    return "", fmt.Errorf("tenantID must not contain delimiter character ':': %q", tenantID)
}

Line 55 (GetPattern):

if strings.Contains(tenantID, ":") {
    return "", fmt.Errorf("tenantID must not contain delimiter character ':': %q", tenantID)
}

Line 84 (StripTenantPrefix):

if strings.Contains(tenantID, ":") {
    return "", fmt.Errorf("tenantID must not contain delimiter character ':': %q", tenantID)
}

Affected consumers

• fetcher (v1.18.0) already uses valkey.GetKeyFromContext and tms3.GetObjectStorageKeyForTenant in multi-tenant code — needs to update all call sites from string to (string, error)

Problem

Breaking API change

v3 functions return string. v4 functions return (string, error). Every call site needs to handle the new error return.

Potential data compatibility issue

Current WorkOS tenant ID format (org_xxx) does not contain / or :, so validation passes. But this constraint is undocumented and could break if tenant ID format ever changes.

Suggestion

• Document tenant ID character constraints in migration guide
• Consider making delimiter validation configurable or a warning during migration period
• Add a ValidateTenantID() function for use at tenant creation time (fail early) rather than at every key operation

Raised by

Jefferson Rodrigues (CTO) during multi-tenant task force review.

[gandalf-at-lerian]

Analysis

After reviewing the tenant-manager codebase, this issue is a won't fix for now.

Why the validation is redundant:
The tenant-manager uses uuid.UUID as the native type for tenant IDs — enforced at the domain model level. UUIDs are hex + hyphens only, so they structurally cannot contain / or :. The validation in the S3 and Valkey helpers will never trigger in practice.

WorkOS org_xxx IDs (used in some contexts) also don't contain these characters.

Why we're not removing it now:
Removing the validation would require changing the function signatures from (string, error) back to string, which is a breaking change for any v4 caller that already handles the error return. That warrants a v5 bump — not justified for a cosmetic cleanup alone.

What we're doing instead:

• Adding inline comments in the S3/Valkey helpers explaining that the validation is redundant given the UUID contract
• Keeping the ValidateTenantID() idea as a candidate for v5, when other breaking changes justify the bump

Closing as won't fix.