Repository navigation
Conversation
atlas-oc
added
Enhancement
Azyyyyyy
referenced
this pull request
in Azyyyyyy/Interfold
Jul 10, 2026
The three SimpleStatement construction sites in ScyllaEncryptionStateRepository (:35 SELECT, :61 UPDATE-no-salt, :70 UPDATE-with-salt) were passing the SystemId struct directly into the Cassandra driver's `params object[]` instead of unwrapping to .Value. The driver has no serializer for SystemId; the struct is boxed and only reaches the correct wire representation because `Object.ToString()` ends up being called by the driver's fallback path and `SystemId.ToString()` happens to equal `.Value` today - but that is not a contractual guarantee of the wrapper. A future "safer" `ToString` variant that redacted (as `SocketToken`, `Jti`, `LinkToken`, `RecoveryCode`, `PushToken`, `ImportToken` all do) would silently break every encryption-state lookup and write with no compile error and no runtime exception, just wrong-key CQL - the classic silent-misbind failure mode. Every sibling Scylla repo already binds `.Value` at the driver boundary - `ScyllaNotificationTokenRepository:37,40` and `ScyllaSharedQueries:40-41` are the reference shape - so this call site was drift, not intent. Fix restores the same idiom and adds an inline comment explaining why the unwrap must stay. Round-2 strong-typing rescan Commit 1/14 (Finding #15, HIGH severity). Canvas: canvases/strong-typing-rescan-round-2.canvas.tsx. Api.UnitTests 214/214, Bootstrapper.UnitTests 375/375 (1 platform skip), build clean (0 errors, warnings unchanged from Round-4 baseline). Co-authored-by: Cursor <cursoragent@cursor.com>
Azyyyyyy
referenced
this pull request
in Azyyyyyy/Interfold
Jul 16, 2026
* Fix options consistently drifting and make comments be shorter
* The start of the big cleanup, starting with strong typing
* Round 2 of strong typing
* Round 3 of strong typing
* Round 4 - Infra will be next
* Round 1 for Infa
* Round 2 for Infa
* Round 3 for Infa
* Round 4 - many fixes required
* Fix WebSocket join gate to tolerate scoped-sub JWTs on raw topics
* Fix more issues from infra changes
* Round 4 of strong typing - Steps 1-7 (plan complete)
Ships all 7 steps of the strong-typing rescan plan from
canvases/strong-typing-rescan.canvas.tsx.
- Step 1: SocketToken wired through WebSocketHandler; ToString redacts the
JWT so any incidental log-interpolation along the six-hop socket path
cannot leak the credential. Explicit .Value survives only at the two
survival points that need the raw JWT (JwtSecurityTokenHandler and the
Authorization: Bearer header write).
- Step 2: IScyllaKeyspaceResolver.NormalizeSystemId / ResolveRegionalKeyspace
retyped as SystemId at the interface; 12 Scylla repos + ScyllaSharedQueries
updated. SystemId.Value continues to be the driver boundary; the
extension-method convenience overloads are gone.
- Step 3: IRegionContext.ResolveUserRegion(string) removed; only the SystemId
overload survives. InMemory + Scylla implementations aligned; Scylla keeps
a private string-typed helper so its sibling public methods can share
logic without leaking string back through the interface.
- Step 4: SystemId? kept typed end-to-end through the WebSocketHandler
socket-join gate (:166, :190, :591, :661) - no more raw-string hops.
- Step 5: WebSocket JWT sub parsed via ScopedSystemId.TryParseScoped inside
IsSocketJoinTokenAuthorizedAsync; middleware parity for HTTP vs socket
paths is now type-enforced rather than reimplemented via string.Equals.
- Step 6: RecoveryCodeResolver.TryResolve out-param retyped from string to
RecoveryCode. Plaintext exists as a bare string only inside the private
decrypt frame (which has no logging surface); every external observation
goes through the redacting wrapper. SettingsController SetupEncryption /
RecoverEncryption / ImportSp collapse to pass the wrapper directly.
- Step 7: Jti.From(string?) / LinkToken.From(string?) static factories added
to SecretTokens.cs (return null for null/empty/whitespace, wrap otherwise
- total function, not TryParse-style). AuthController.RevokeToken and
AuthLinkController.Callback null-check on the typed nullable so the raw
string no longer lives across the guard/wrap boundary.
New test files:
- SocketTokenRedactionTests: pins ToString-redacts and .Value-unwraps-verbatim
contracts that WebSocketHandler now leans on.
- SecretTokensFromFactoryTests: 8 tests, 4-input matrix + interpolation
round-trip for both Jti.From and LinkToken.From.
Modified tests:
- IsTokenSubjectAuthorizedForTopicTests: reduced 13 -> 7 tests as raw-sub
scenarios became compile-time-impossible after Step 5. Rejection matrix
migrated to ScopedSystemIdTests where it belongs.
- InMemoryRegionContextTests, InMemoryAccountRepositoryLinkTokenRegressionTests,
RegionContextCachingTests, ScyllaFrontingRepositoryNullTimeStartTests: all
retyped to speak SystemId at the boundaries the interface now demands.
Api.UnitTests 214/214, Bootstrapper.UnitTests 375/375 (1 platform skip).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Fix Scylla misbind: bind SystemId.Value in encryption state repo
The three SimpleStatement construction sites in ScyllaEncryptionStateRepository
(:35 SELECT, :61 UPDATE-no-salt, :70 UPDATE-with-salt) were passing the
SystemId struct directly into the Cassandra driver's `params object[]`
instead of unwrapping to .Value. The driver has no serializer for SystemId;
the struct is boxed and only reaches the correct wire representation because
`Object.ToString()` ends up being called by the driver's fallback path and
`SystemId.ToString()` happens to equal `.Value` today - but that is not a
contractual guarantee of the wrapper. A future "safer" `ToString` variant
that redacted (as `SocketToken`, `Jti`, `LinkToken`, `RecoveryCode`,
`PushToken`, `ImportToken` all do) would silently break every
encryption-state lookup and write with no compile error and no runtime
exception, just wrong-key CQL - the classic silent-misbind failure mode.
Every sibling Scylla repo already binds `.Value` at the driver boundary -
`ScyllaNotificationTokenRepository:37,40` and `ScyllaSharedQueries:40-41`
are the reference shape - so this call site was drift, not intent. Fix
restores the same idiom and adds an inline comment explaining why the
unwrap must stay.
Round-2 strong-typing rescan Commit 1/14 (Finding #15, HIGH severity).
Canvas: canvases/strong-typing-rescan-round-2.canvas.tsx.
Api.UnitTests 214/214, Bootstrapper.UnitTests 375/375 (1 platform skip),
build clean (0 errors, warnings unchanged from Round-4 baseline).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Retype IAvatarStorage to speak AvatarUrl end-to-end
Round-2 strong-typing rescan Commit 2/14 (Findings #1, #2, #8, HIGH severity).
Canvas: canvases/strong-typing-rescan-round-2.canvas.tsx.
IAvatarStorage previously returned raw strings from the two save methods and
took `string? avatarUrl` on the delete method, so every caller was doing
`new AvatarUrl(url)` on the very next line and every delete-site was doing
`currentAvatarUrl?.Value` to strip back to string right before the call.
The AvatarUrl wrapper (and its `FromNullable` factory) has existed the whole
time - this commit rips out the pointless round-trip.
Interface + LocalAvatarStorage:
- `SaveSystemAvatarAsync` / `SaveAlterAvatarAsync` return `Task<AvatarUrl>`.
- `DeleteByUrlAsync` takes `AvatarUrl?` and pattern-matches to extract
`.Value` at the filesystem boundary (still needed for URI parsing).
- Private helper `SaveAsync` retyped from `(string systemId, string targetId
...)` to `(SystemId systemId, string targetId, ...)` so callers stop
pre-unwrapping .Value only for the helper to re-normalize immediately
(Finding #8). targetId legitimately polymorphs between "self" (system
avatar) and alterId.ToString() (alter avatar), so it stays a string
segment - no wrapper covers both shapes. Wrap of the final URL happens
once in the private helper's return statement.
Callers - production:
- SettingsController: 1 save (:290) + 3 delete sites (:332, :392, :424).
`string avatarUrl` local retyped to `AvatarUrl`; `new AvatarUrl(avatarUrl)`
at the CommandEnvelope payload collapses to just `avatarUrl`; every
DeleteByUrlAsync call passes `currentAvatarUrl` (AvatarUrl?) directly.
- AltersController: 1 save (:166) + 3 delete sites (:226, :304, :417).
Same collapse pattern.
- SimplyPluralImportService: 2 saves (:850 system, :857 alter). Removes
`new AvatarUrl(localUrl)` at :851 and `AvatarUrl.FromNullable(localUrl)`
at :861 - localUrl is now AvatarUrl and widens implicitly to AvatarUrl?
at the alter-command payload.
Callers - test/util mocks (interface signature must match):
- SPDump/Program.cs TempAvatarStorage: three signatures aligned; empty-URL
returns wrap `new AvatarUrl("")` (AvatarUrl ctor accepts empty, only null
throws - matches existing behaviour of returning `""` before).
- IntegrationTests/SimplyPluralImport/SpImportTests.cs NullAvatarStorage:
three signatures aligned identically.
Build clean (0 errors, 100 warnings - unchanged from Round-1 baseline).
Api.UnitTests 214/214, Bootstrapper.UnitTests 375/375 (1 platform skip).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Retype EncryptionKey.DeriveKey / DeriveChecksum to speak wrappers
Round-2 strong-typing rescan Commit 3/14 (Findings #12 + #13 + #14, HIGH severity).
Canvas: canvases/strong-typing-rescan-round-2.canvas.tsx.
EncryptionKey.DeriveKey and DeriveChecksum were the funnel through which every
strongly-typed value in the encryption pipeline was stripped back to raw
string. The two handler call sites each shipped a `.Value.Value.Value` unwrap
ladder and every command result re-wrapped the derived key back into
`EncryptionKeyMaterial` on the next line. This commit promotes the API so
the wrappers survive the KDF/checksum round-trip end-to-end.
API changes (Interfold.Domain/EncryptionKey.cs):
- `DeriveKey(string pepper, SystemId systemId, RecoveryCode recoveryCode,
EncryptionSalt salt): EncryptionKeyMaterial` - three of four inputs and
the return value moved from string to their existing wrappers. Only the
pepper stays raw because no wrapper exists for deployment-wide crypto
secrets today (creating one would only force an unnecessary .Value at
every use).
- `DeriveChecksum(EncryptionKeyMaterial key): KeyChecksum` - both directions
typed, so the handler's checksum-equality check no longer needs
`string.Equals(..., Ordinal)` - `KeyChecksum` is a `readonly record
struct` whose auto-generated Equals compares .Value with ordinal string
equality (default `EqualityComparer<string>`), so `checksum != keyChecksum`
is semantically identical to the old comparison.
Call-site cleanup:
- SetupEncryptionCommandHandler (:70-77): three .Value strips and two
wrapper re-constructions (`new KeyChecksum(checksum)`, `new
EncryptionKeyMaterial(key)`) collapse to pass wrappers directly.
- RecoverEncryptionCommandHandler (:65-71): same three .Value strips + one
re-construction collapse; string.Equals replaced by record-struct `!=`.
- WireByteFreezeTests.DeriveKey_IsDeterministic_AndScopeSensitive: string
literals wrapped at the call boundary. The scoped-vs-raw byte-freeze
assertion still passes byte-for-byte - the wire form now enforced by the
wrapper types themselves rather than by convention among callers.
Bridge (removed in Commit 5):
- SimplyPluralImportService.ValidateEncryptionKeyAsync (:758) still takes
`string recoveryCode` because Finding #3 (Commit 5) retypes that method
end-to-end. Bridge inline-wraps `new RecoveryCode(recoveryCode)` at the
call and unwraps `key.Value` at the tuple return; both go away in Commit
5 when the method signature and its two callers flip.
Build clean (0 errors, 100 warnings - unchanged from Commit 2 baseline).
Api.UnitTests 214/214 (WireByteFreezeTests all green -
DeriveKey_IsDeterministic_AndScopeSensitive still pins the region-prefix
sensitivity byte-for-byte), Bootstrapper.UnitTests 375/375 (1 platform skip).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Add Jti.NewJti() mint factory and adopt at IssueDeepLinkTokenAsync + socket revoke check
Round-2 strong-typing rescan Commit 4/14 (Findings #7 + #11, MEDIUM/LOW severity).
Canvas: canvases/strong-typing-rescan-round-2.canvas.tsx.
Two Step-7-adjacent raw-string-local windows kept the JTI alive as an
interpolable string across a mint/wrap or read/wrap boundary. Round-2 Commit 4
closes both by adding a `Jti.NewJti()` mint factory and adopting the existing
`Jti.From(string?)` at the socket-side read site.
New factory (Interfold.Contracts/Ids/SecretTokens.cs):
- `Jti.NewJti()`: mints a fresh `Guid.NewGuid().ToString("N")` and wraps in a
single call. Byte-identical to the pre-Round-2 raw shape (32-char lowercase
hex, no dashes) so every pre-Round-2 issued token still verifies against a
Jti minted today and the revocation-store row key is unchanged. Total
function - never returns default.
Adoption:
- AuthController.IssueDeepLinkTokenAsync (:169): raw string local
`Guid.NewGuid().ToString("N")` replaced by `Jti.NewJti()`. Removes the two
`new Jti(jti)` re-wraps at the CreateToken and RecordTokenAsync call sites.
- WebSocketHandler socket-join JTI-revocation check (:694-705): raw string
local from `principal.FindFirstValue(JwtClaimNames.Jti)` replaced by
`Jti.From(...)`; null check now runs on `Jti?` via `is { } typedJti`
pattern-match. Removes the `new Jti(jti)` re-wrap at ValidateTokenNotRevoked
and - more importantly - the LogWarning at :702 previously interpolated a
bare string local (`{Jti}`) so the structured-log sink emitted the raw JTI
verbatim on every revoked-token socket-join attempt; the typed local now
routes through `Jti.ToString` which redacts.
Tests (Interfold.Api.UnitTests/Ids/SecretTokensFromFactoryTests.cs):
Three new tests added alongside the existing eight `From` tests:
- `JtiNewJti_ProducesUniqueValueEachCall`: two consecutive mints must differ.
A collision would silently merge two independent revocation entries so
revoking one would revoke both - the exact silent-security-failure this
factory is designed to prevent.
- `JtiNewJti_ProducesThirtyTwoCharLowercaseHex`: pins the wire format
byte-for-byte with the pre-Round-2 raw `Guid.NewGuid().ToString("N")`
shape. Regex-anchors `^[0-9a-f]{32}$`. A drift would orphan every
pre-Round-2 issued token on the first NewJti() issuance after deploy.
- `JtiNewJti_ValueRedactsUnderInterpolation`: `$"{jti}"` must not contain the
raw value. Pins the AuthController.IssueDeepLinkTokenAsync safety story
end-to-end - operation-id / redirect logs live in the same span as the
mint and must not leak.
Build clean (0 errors, 100 warnings - unchanged from Commit 3 baseline).
Api.UnitTests 217/217 (+3 from Commit 3's 214), Bootstrapper.UnitTests
375/375 (1 platform skip).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Retype SimplyPluralImportService recovery-code + encryption-key end-to-end
Round-2 strong-typing rescan Commit 5/14 (Findings #3 + #4, HIGH severity).
Canvas: canvases/strong-typing-rescan-round-2.canvas.tsx.
The SP import service was the last consumer of the pre-Commit-3 raw-string
EncryptionKey API. Commit 3 shipped a bridge here (inline
`new RecoveryCode(recoveryCode)` at the DeriveKey call, `.Value` unwrap at the
tuple return) so the API flip could land in isolation. This commit removes
that bridge and retypes all six SP-side signatures so the recovery code and
derived key never appear as bare strings inside the import pipeline.
Six signatures / locals updated in SimplyPluralImportService.cs:
1. `ImportAsync` top-level local (:92): `string? encryptionKey` retyped to
`EncryptionKeyMaterial?`. The "no derived key" case now encoded by null
rather than an empty-string sentinel; the `IsNullOrWhiteSpace` gate at
:131 becomes a `is { } derivedEncryptionKey` pattern-match.
2. `ImportAsync` -> `ValidateEncryptionKeyAsync` call site (:95): drops the
`.Value` on providedRecoveryKey; passes the `RecoveryCode` wrapper.
3. `ValidateEncryptionKeyAsync` signature (:763): `string recoveryCode`
retyped to `RecoveryCode`; return tuple's `string DerivedKey` retyped to
`EncryptionKeyMaterial?` (null replaces `string.Empty` as the failure
sentinel). The Commit-3 inline-wrap bridge (`new RecoveryCode(recoveryCode)`)
is deleted; DeriveKey is called with the typed value directly.
4. `ImportNotesAsync` signature (:636): `string encryptionKey` retyped to
`EncryptionKeyMaterial`.
5. `PrepareNote` signature (:703): `string encryptionKey` retyped to
`EncryptionKeyMaterial`.
6. `TryEncryptForClient` signature (:730): `string base64Key` retyped to
`EncryptionKeyMaterial`. The single `.Value` unwrap survives at the
`Convert.FromBase64String` call - the base64-string form is the API
contract of the crypto primitive, so this is a legitimate driver-boundary
unwrap (same shape as `SystemId.Value` at the Cassandra bind boundary).
Wire behaviour: byte-identical. `EncryptionKeyMaterial` is a
`readonly record struct` whose `.Value` is the raw base64 string, so every
downstream AES-GCM encryption produces the identical ciphertext the pre-Commit-5
code produced for the same recovery code + salt + pepper triplet. The
byte-freeze test in `WireByteFreezeTests` (updated in Commit 3) transitively
covers the derivation input; the SP import ciphertext is unchanged because
the underlying key material bytes are unchanged.
Build clean (0 errors, 100 warnings - unchanged from Commit 4 baseline).
Api.UnitTests 217/217, Bootstrapper.UnitTests 375/375 (1 platform skip).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Delete three ResolveFriendshipLevelAsync private wrappers - inline direct call
Round-2 strong-typing rescan Commit 6/14 (Findings #16 + #17, LOW severity).
Canvas: canvases/strong-typing-rescan-round-2.canvas.tsx.
Three Scylla repositories (Alter, Tag, Fronting) each carried a byte-identical
private wrapper whose sole job was to hop the owner's SystemId string ->
typed at the ScyllaSharedQueries.ResolveFriendshipLevelAsync boundary:
private Task<FriendshipLevel?> ResolveFriendshipLevelAsync(ISession session,
string ownerSystemId,
SystemId? viewerSystemId)
=> ScyllaSharedQueries.ResolveFriendshipLevelAsync(
session, _keyspaceResolver,
new SystemId(ownerSystemId), // <- pure string-obsession
viewerSystemId);
Every caller already held a typed `normalizedSystemId` local (result of
`_keyspaceResolver.NormalizeSystemId(systemId)` a few lines up) and was
passing `normalizedSystemId.Value` into the wrapper only to have the wrapper
immediately `new SystemId(...)` it back. The whole scaffolding existed only
because ScyllaSharedQueries was originally hand-rolled with `string` and the
callers were retrofitted to typed later without cleaning up the bridge.
Fix - one commit:
- Deleted the three private `ResolveFriendshipLevelAsync` wrappers from
ScyllaFrontingRepository (:701-702), ScyllaAlterRepository (:587-588),
and ScyllaTagRepository (:668-669).
- Updated all five call sites (:295 Fronting, :452/:528 Alter, :476/:579
Tag) to call `ScyllaSharedQueries.ResolveFriendshipLevelAsync` directly
with the typed `normalizedSystemId` and the already-injected
`_keyspaceResolver`.
The InMemory-side sibling methods (`InMemoryAlterRepository.
ResolveFriendshipLevelAsync` and equivalents) are legitimate private helpers
with different signatures (system + viewer, no session, hits the in-memory
friendships dict directly). They are not the same anti-pattern and stay.
Build clean (0 errors, 100 warnings - unchanged from Commit 5 baseline).
Api.UnitTests 217/217, Bootstrapper.UnitTests 375/375 (1 platform skip).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Introduce ProviderIdentity union - collapse 3 OAuth-provider re-wrap switches
Round-2 strong-typing rescan Commit 7/14 (Findings #5 + #6, MEDIUM severity).
Canvas: canvases/strong-typing-rescan-round-2.canvas.tsx.
The OAuth callback pipeline had three duplicated `oauthProvider switch`
statements, each of which independently re-wrapped the exact same raw
`identity` string in the exact same three ways:
// AuthController.Callback:114
var resolvedSystemId = oauthProvider switch
{
OAuthProvider.Discord => await _accounts.FindOrCreateSystemIdByDiscordIdAsync(new DiscordId(identity), ...),
OAuthProvider.Google => await _accounts.FindSystemIdByEmailAsync(new Email(identity), ...),
OAuthProvider.Apple => await _accounts.FindSystemIdByAppleIdAsync(new AppleId(identity), ...),
_ => null,
};
// AuthLinkController.Callback:119 - same three wraps, different account API
// AuthLinkController.RedirectWithSocketEventAsync:154 - same three wraps, different event type
The root of the smell was `ExtractProviderIdentityAsync` returning
`string?` even though its three sources (`DiscordOAuthService.
ExchangeCodeForDiscordIdAsync`, `GoogleOAuthService.ExchangeCodeForEmailAsync`,
`AppleOAuthService.ExchangeCodeForAppleIdAsync`) each already return the typed
nullable (`DiscordId?` / `Email?` / `AppleId?`). The wrapper was doing a
typed-to-string-to-typed round-trip and inviting every downstream site to
switch on the enum and rewrap manually.
New type (Interfold.Contracts/Ids/ProviderIdentity.cs):
- `ProviderIdentity` - a `readonly record struct` union with three nullable
properties `Discord`, `Google`, `Apple`. Construction only via the three
static factories `FromDiscord` / `FromGoogle` / `FromApple` that enforce
the "exactly one populated" invariant at compile time.
- Callers dispatch via property-pattern match:
identity switch
{
{ Discord: { } discordId } => ...,
{ Google: { } email } => ...,
{ Apple: { } appleId } => ...,
_ => (unreachable given caller-side presence guard)
}
API changes:
- `OAuthControllerBase.ExtractProviderIdentityAsync`: return type flipped
from `Task<string?>` to `Task<ProviderIdentity?>`. Inner logic
reorganised so each provider branch builds up the typed value and wraps
into the union at the return points (all fallback paths still hit the
query/form untyped extraction and wrap at the end).
- `AuthController.Callback`: `string.IsNullOrWhiteSpace(identity)` guard
replaced with `identity is not { } typedIdentity`; the account-lookup
switch dispatches off the union rather than off `oauthProvider`.
- `AuthLinkController.Callback`: same guard + dispatch shape. The
`oauthProvider switch` used for user-facing error text on AlreadyLinked /
UserExists paths stays as-is (no string-wrap in those switches).
- `AuthLinkController.RedirectWithSocketEventAsync`: signature retyped
from `(ScopedSystemId, OAuthProvider, string, string?)` to
`(ScopedSystemId, ProviderIdentity, string?)`. Each PublishAsync call
keeps its concrete event type so the event bus's generic subscriber
routing continues to dispatch by TEvent as before.
Build clean (0 errors, 100 warnings - unchanged from Commit 6 baseline).
Api.UnitTests 217/217, Bootstrapper.UnitTests 375/375 (1 platform skip).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Introduce ScopedSystemId.RepresentsSameUserAs — replace 8 cross-type self-check byte compares
Round-2 Commit 8 (canvas finding #9). Fixes a latent semantic drift in the eight
controller self-request guards:
FriendRequestsController — Send (line 64, UsernameOrSystemId), Cancel (:87),
Accept (:110), Reject (:133) [SystemId]
FriendsController — Show (:43), Delete (:72), Trust/Untrust (:108) [SystemId]
Pre-Round-2 spelling used one of two shapes, both of which reduced to a byte
compare of the scoped composite ("nam:abcdefg") against the raw route value:
- `principal.Value == id.Value` at the Send site — cross-wrapper raw string
compare of ScopedSystemId.Value vs UsernameOrSystemId.Value.
- `principal == id` at the seven SystemId sites — the implicit
ScopedSystemId→SystemId widen made this `SystemId(scoped) == SystemId(raw)`,
still a byte compare.
Both shapes caught the trivial scoped-route case (`/api/friends/nam:abcdefg`)
and silently missed the raw-route case (`/api/friends/abcdefg`). For the
FriendRequestsController.Send site the Slice-7 documentation on
SendFriendRequestCommandHandler:65-73 correctly noted the downstream resolved-id
self-check picked up the miss, so Send only returned the wrong error code for
the fast path. The seven SystemId sites have NO downstream self-check — a raw
self-cancel / self-accept / self-reject / self-delete / self-trust silently fell
through to the domain handler and returned a generic
friend_request:not_requested or friendship_not_found instead of the crisp
cannot_*_self.
Fix: introduce a semantic primitive on ScopedSystemId and route all eight sites
through it.
ScopedSystemId.RepresentsSameUserAs(SystemId candidate)
- TryParseScoped(candidate.Value) → compare scoped composites (cross-region
shapes are treated as different users per the "scoped composite is
identity" contract on the type's xml-doc).
- Else compare against principal.RawId (raw shape belongs to principal's
region by convention — this is the case pre-Round-2 silently missed).
- Blank / whitespace → false (never throws on defensive inputs).
ScopedSystemId.RepresentsSameUserAs(UsernameOrSystemId candidate)
- Delegates to the SystemId primitive when LookupHandle.TryParse returns
LookupKind.Region or LookupKind.Id (scoped and bare raw shapes).
- Username / Discord shapes return false — the controller can't decide
"am I alice?" without a registry lookup, so the fast-path must let the
downstream resolver's self-check take over (preserves Slice-7 semantics).
- Unparseable prefixes (e.g. "xxx:abcdefg") fall back to a byte compare
against Value so an opaque input still catches the trivial case.
Behaviour delta: the 7 SystemId sites now return the specific cannot_*_self
error code for the raw-route shape. The HTTP status is unchanged (400), so
clients that switch on status don't observe a difference; clients that switch
on error codes get a more precise category. The Send site's fast-path shape is
unchanged (byte-compat for the scoped-route case; the newly-caught raw-route
case is a strict improvement because the downstream handler was already going
to catch and reject the same call).
Audit outcome for the canvas note "audit AltersController / TagsController for
the same idiom": no `principal == id` shape in either controller — cleared.
Tests: Interfold.Api.UnitTests/Ids/ScopedSystemIdRepresentsSameUserAsTests.cs
adds 11 tests covering same-region-scoped, raw, cross-region-scoped, mismatched
raw, blank, and the four UsernameOrSystemId shapes (scoped, raw, username,
Discord, unknown prefix). All 228 API unit tests pass.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Retype ScyllaAccountRepository.LinkTokenEntry.Scoped + InMemoryAccountRepository.EnsureEncryptionSaltForSystem
Round-2 Commit 9 (canvas findings #18 + #21). Two small focused string-leak
closures at the boundary between the account-repository FindOrCreate paths and
their internal state.
#18 — ScyllaAccountRepository.LinkTokenEntry
Retyped the record-struct's value field from `string ScopedSystemId` to
the typed `ScopedSystemId Scoped`. Pre-Round-2 the Scylla adapter was the
string-leak outlier — the InMemory sibling has always carried the typed
shape. The reverse-map read at ResolveSystemIdByLinkTokenAsync no longer
re-wraps through `new SystemId(entry.ScopedSystemId)` — it widens through
the wrapper's AsSystemId instead.
Cache-rehydration audit (per canvas note): confirmed
_linkTokenBySystem / _systemByLinkToken are process-lifetime only. The
ctor only captures the session provider / keyspace resolver / options; no
load-from-Scylla seeding path. Safe to change the entry's value shape.
Dictionary key/value types deliberately UNCHANGED — canvas findings #20 +
#22 are scoped to Commit 12, which retypes the reverse-map keys to
LinkToken and the forward-map values to LinkToken / ScopedSystemId.
Keeping the boundaries split so if Commit 12 needs a follow-up, this
smaller change stands on its own.
#21 — InMemoryAccountRepository.EnsureEncryptionSaltForSystem
Retyped the parameter from `string scopedSystemId` to
`ScopedSystemId scoped`. All three call sites (FindOrCreate for Discord /
Email / Apple) already resolved a typed ScopedSystemId locally and had to
unwrap to `.Value` at the boundary only for the method to re-wrap via
`new SystemId(scopedSystemId)` inside the UpsertAsync call. The wrapper
eliminates that round-trip and pins the "the caller already resolved this
to a scoped composite" invariant at the type level.
Each of the three FindOrCreate methods now holds a typed `scopedNew` local
and returns `scopedNew.AsSystemId()` — removes the pre-Round-2
`.Value` → `new SystemId(...)` round-trip on the return path as a side
benefit. The `_systemByDiscord` / `_systemByEmail` / `_systemByApple`
dicts remain string-valued (Commit 12 territory).
No behaviour change. Build passes with 100 warnings unchanged; all 228
Interfold.Api.UnitTests pass (includes InMemoryAccountRepositoryLinkTokenRegressionTests).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Delete untyped QualifyAvatar overloads — every consumer now speaks AvatarUrl?
Round-2 Commit 10 (canvas #10). Blanket-verified per the canvas note that
every consumer of QualifyAvatar migrated to the typed shape after Round-2
Commit 2 (AvatarStorage retype) promoted every read model and command payload
to AvatarUrl?.
Deletions:
- AvatarUrlQualifier.QualifyAvatar(string? url, AvatarSource?, string scheme, HostString host)
- AvatarUrlQualifier.QualifyAvatar(string? url, AvatarSource?, string? origin)
- InterfoldControllerBase.QualifyAvatar(string? url, AvatarSource?)
The two typed AvatarUrlQualifier overloads used to delegate to the string
siblings via `AvatarUrl.FromNullable(QualifyAvatar(url?.Value, source, ...))`.
Post-Round-2 they own the AvatarSource discrimination directly and only reach
into the string primitive `Qualify` when the source is Local — one less
wrap/unwrap round-trip, and no possibility for a caller to accidentally bind
to the raw-string overload when the compiler had both to choose from.
The `AvatarUrlQualifier.Qualify(string?, ...)` overloads are retained — they
are the primitive URL-qualification helper used by non-avatar callers (and by
the typed `QualifyAvatar` internally). Only the AvatarSource-aware string
overloads went.
Verification:
- Build clean, 100 warnings unchanged, 0 errors.
- All 228 Interfold.Api.UnitTests pass.
- Grep for `QualifyAvatar` returns only typed call sites in
FriendsController / FriendRequestsController / AltersController /
PublicSystemsController and the socket handlers
(AlterSocketEventHandlers, FriendshipSocketEventHandlers,
WebSocketInitialization). Zero string-typed consumers.
Also updated the AvatarServingPolicy xml-doc `<see cref>` to point at the
typed overload so the doc reference resolves post-deletion.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Extract ProviderColumn enum inside ScyllaAccountRepository — retype 4 internal helpers + 10 public entry points
Round-2 Commit 11 (canvas #19). Replaces the magic-string column discriminator
that the 10 public OAuth-provider entry points passed into 4 internal helpers.
Before: every public entry point (Try/FindOrCreateSystemIdBy{Discord,Email,Apple}Id
Async, Link{Discord,Email,Apple}ToUserAsync, Unlink{Discord,Email,Apple}Async)
hand-spelled the Cassandra column name as a literal — 10 sites all repeating
`"discord_id"`, `"email"`, or `"apple_id"`. The four internal helpers
(TryFindSystemIdByRegistryColumnAsync, FindOrCreateSystemIdByRegistryColumnAsync,
LinkIdentityAsync, UnlinkIdentityAsync) accepted `string columnName` and
interpolated it into every CQL fragment. A misspelled literal at any entry
point would compile silently and fail at runtime with a Cassandra "table does
not exist" or "unknown column" error — the exact class of drift the strong-
typing rescan is chasing out.
After: a private `ProviderColumn` enum (`Discord`, `Email`, `Apple`) is the
discriminator through every internal helper. A single `ColumnName(ProviderColumn)`
switch-with-explicit-throw maps the enum onto the CQL literal at the boundary
where each helper enters the SQL-string-interpolation layer. Misspelled provider
literals are now impossible (there's no string to misspell); a future
5th provider surfaces as build-time exhaustiveness warnings on the switch and
on every call site instead of a runtime silent fail.
The CQL strings themselves are unchanged, so wire format / storage schema /
integration-test fixtures are unaffected. Every one of the 10 entry points is
still a one-liner — the enum shifts the concern from "spell the column right"
to "pick the right enum value", which the type system can verify.
Verification: - Build clean (100 warnings unchanged, 0 errors).
- All 228 Interfold.Api.UnitTests pass.
Co-authored-by: Cursor <cursoragent@cursor.com>
* round-2 commit 12: retype the account-repo dictionary state to speak wrappers
Round-2 finding #20 (ScyllaAccountRepository) + #22 (InMemoryAccountRepository)
of the second strong-typing rescan. Internal-only change to runtime cache
hygiene; zero interface / on-wire shape / storage-schema movement.
Scylla changes:
* `_linkTokenBySystem` : `<string, string>` -> `<ScopedSystemId, LinkToken>`
* `_systemByLinkToken` : `<string, LinkTokenEntry>` -> `<LinkToken, LinkTokenEntry>`
The four `GetOrCreate` / `Get` / `Resolve` / `Clear` link-token methods now
hold the typed `ScopedSystemId` from `ScopedSystemId.Compose` directly rather
than immediately unwrapping to `.Value` for the dict key, and the reverse-map
lookup takes the typed `LinkToken` so no accidental `linkToken.ToString()`
call site can poke the dict (the wrapper redacts under `ToString`, which
would have silently corrupted the cache pre-Round-2).
InMemory changes (the wider one -- InMemory holds nine dicts, all typed):
* `_usernameBySystem` value -> `Username`
* `_avatarBySystem` value -> `AvatarUrl`
* `_linkTokenBySystem` value -> `LinkToken`
* `_systemByLinkToken` key -> `LinkToken`
* `_discordBySystem` value -> `DiscordId`
* `_emailBySystem` value -> `Email`
* `_appleBySystem` value -> `AppleId`
* `_systemByDiscord` value -> `ScopedSystemId`
* `_systemByEmail` value -> `ScopedSystemId`
* `_systemByApple` value -> `ScopedSystemId`
`_descriptionBySystem` stays `<string, string>` because there is no
`Description` wrapper (descriptions are genuinely free-form user content,
not identifiers). The `_systemBy{Discord,Email,Apple}` dicts keep their
string-typed KEY with the pre-Round-2
`StringComparer.OrdinalIgnoreCase` comparer -- the identity wrappers do not
self-normalise, so the lookup contract is unchanged.
The `LinkIdentifier(...)` helper is promoted to a generic
`LinkIdentifier<TIdentity>(..., Func<TIdentity, string> extractRawValue)
where TIdentity : struct` so the three call sites (Discord / Email / Apple)
pass their typed identity and a static-lambda accessor for the reverse-map
key; static lambdas mean no per-call allocation. The three `Unlink*` /
`Delete` / `GetPublicProfile` methods now speak typed locals throughout, and
`GetPublicProfileAsync` no longer re-wraps each field with `new Username(u)`
/ `new DiscordId(discord)` etc. before feeding
`AccountPublicProfileReadModel` -- the dict values are already the right
shape.
Round-2 Commit 9 already routed `LinkTokenEntry.Scoped` through
`AsSystemId()` on the read side; Commit 12 finishes the job by ensuring the
scoped composite spends zero time as a bare string local on the write side
either.
Because the caches are process-lifetime only (audited in Commit 9 --
`ScyllaAccountRepository` reads no storage-side rehydration of either dict),
the key-shape / value-shape change is invisible externally. Every test run
that exercises the account repository (auth-link flows, avatar reads,
Discord OAuth callback, encryption-setup gate, and the SP-import fixture
that touches profile writes) is on the same wire and CQL bytes as it was
before this commit -- the only observable difference is that a compiler
error (rather than a `NullReferenceException` at runtime) now surfaces if
someone poke the wrong dict with the wrong wrapper type.
Build: 100 warnings (unchanged from Commit 11 baseline), 0 errors.
Api.UnitTests: 228 passed, 0 failed. No new tests -- Commit 12 is a
representation change with no new observable behaviour to pin.
Co-authored-by: Cursor <cursoragent@cursor.com>
* round-2 commit 13: retype IClusterEventBus.SubscribeAsync to speak ScopedSystemId?
Round-2 finding #24 of the second strong-typing rescan. The canvas rated
this LOW severity ("cost > value until we hit the bug") because it's an
interface change that has to thread through every subscriber, but committed
to it in the recommended-refactor sequence because the asymmetry is a real
bug vector: pre-Round-2 the bus's `Subscription.TargetSystemId` was
`SystemId?` and the publisher-side `ITargetedClusterEvent.TargetSystemId`
was `ScopedSystemId`, and `InProcessEventBus.PublishAsync` reconciled the
two by normalising both sides through `SystemIdNormalization.StripRegionPrefix`
on every publish tick. Any subscriber that ever supplied a scoped-shaped
`SystemId` (e.g. via `ScopedSystemId.AsSystemId`) without also stripping
would silently miss every delivery.
Wire changes:
* `IClusterEventBus.SubscribeAsync<TEvent>(SystemId? targetSystemId, ...)`
becomes `SubscribeAsync<TEvent>(ScopedSystemId? targetSystemId, ...)`.
The parameterless broadcast overload (default-interface-method that
delegates with `targetSystemId: null`) is unchanged.
* `InProcessEventBus.Subscription<TEvent>.TargetSystemId` promoted from
`SystemId?` to `ScopedSystemId?`, matching the wire shape of every
`ITargetedClusterEvent.TargetSystemId`.
* `InProcessEventBus.PublishAsync` filter simplified from the two-sided
strip-then-string-equals shape to a scoped-record-struct-equality
compare. Byte-equivalent to the pre-Round-2 shape when both regions
match; correctly rejects the cross-region false positive that the
strip-then-compare shape used to deliver (canvas #24's stated bug
vector).
Socket subscription flow (the load-bearing subscriber):
* `WebSocketHandler.IsSocketJoinTokenAuthorizedAsync` return tuple gained a
third element `ScopedSystemId? TokenSubject`. The parse already happened
inside the helper at the `TryParseScoped` gate (Step 5 of the first
strong-typing rescan), so returning it is free — the caller no longer
has to re-derive the scoped composite from either the raw topic id (which
would need a region resolver) or by re-parsing the JWT.
* `SocketPushContext` gained a `JoinedScopedSystemId` (`ScopedSystemId?`)
alongside the pre-existing raw `JoinedSystemId` (which is retained
because every socket event handler routes through
`TryGetSystemTopic(SystemId)` and those signatures don't change here).
* `SocketEventPumpRunner.SubscribeAsync` now feeds
`context.JoinedScopedSystemId` to the bus. The pump's ~38 subscriptions
now filter on scoped-to-scoped, matching the wire shape end-to-end.
* `WebSocketHandler.HandleAsync` destructures the three-tuple and threads
the parsed `scopedSub` into the `SocketPushContext` constructor.
Tests:
* `WireByteFreezeTests.InProcessEventBus_RoutesScopedAndRawInterchangeably`
was renamed to `InProcessEventBus_ScopedToScopedMatchDelivers` and
rewritten to reflect the new invariant: subscriber and publisher both
speak `ScopedSystemId`, and the test pins the load-bearing single-region
happy path (which is exercised by every WebSocket push in the codebase).
The pre-Round-2 shape asserted that a raw-subscription received scoped
events via `StripRegionPrefix` tolerance; that tolerance is gone by
design and can't be exercised because the interface no longer accepts a
raw `SystemId` subscription target.
* New test: `InProcessEventBus_CrossRegionScopedTargetsDoNotBleedAcross`
pins that a NAM-scoped subscriber does NOT receive an EUR-scoped publish
even when the raw ids match — the pre-Round-2 strip-then-compare shape
WOULD have delivered this cross-region false positive, so the new
scoped-to-scoped compare is not only a type-safety win but also a real
correctness fix.
* `ImportJobBackgroundServiceTests` in-file test stub updated to the new
interface signature (only touchpoint outside the socket flow — the other
test stubs and integration-test fixtures all subscribe with
`targetSystemId: null` via the parameterless broadcast overload, which
is unchanged).
Cross-region regression pin: the new
`InProcessEventBus_CrossRegionScopedTargetsDoNotBleedAcross` test would
have failed against the pre-Round-2 bus body (it delivered the eur-target
event to the nam-subscriber because both normalised to the same raw id).
The passing behaviour is the correctness improvement the canvas finding
identified.
Build: 100 warnings (unchanged from Commit 12 baseline), 0 errors.
Api.UnitTests: 229 passed (+1 for the new cross-region test), 0 failed.
Integration InProcessEventBusReconnectTests: 2 passed, 0 failed.
Co-authored-by: Cursor <cursoragent@cursor.com>
* round-3 commit 1: retype InMemoryStorageKeys.ForSystem to ScopedSystemId + cascade
Finishes Round-2 Commit 12's work by retyping the key side of every per-system
dictionary in the seven InMemory repositories. The pre-Round-3 InMemoryStorageKeys.ForSystem
returned string (immediately unwrapping the ScopedSystemId it just composed), which forced
every consumer to declare ConcurrentDictionary<string, TValue>. Round-2 Commit 12
tightened the value side of InMemoryAccountRepository but explicitly deferred the key-side
retype because it was cross-cutting. This commit closes that gap.
Round-3 canvas findings #1 + #2 + #3, landed as a single wave:
- #1: InMemoryStorageKeys.ForSystem returns ScopedSystemId. Seven repo-level
GetSystemKey helpers now return ScopedSystemId. ~24 ConcurrentDictionary<string, ...>
declarations across InMemoryAccountRepository / InMemoryAlterRepository /
InMemoryFrontingRepository / InMemoryJournalRepository / InMemoryPollRepository /
InMemorySettingsFieldRepository / InMemoryTagRepository retyped to
ConcurrentDictionary<ScopedSystemId, ...>. Byte-identical lookup semantics — the
record-struct default equality is ordinal on Value.
- #2: InMemoryAlterRepository.AlterState.Fields promoted from
Dictionary<string, string?>(StringComparer.OrdinalIgnoreCase) to
Dictionary<FieldId, string?>. Every field id in the codebase is
Guid.NewGuid().ToString("N") (lowercase hex) so the OrdinalIgnoreCase behaviour is
vacuous — no observable drift.
- #3: InMemoryTagRepository._alterMemberships and InMemoryJournalRepository._entryAlters
keys promoted from hand-concatenated "{systemKey}:{childId}" strings to typed
(ScopedSystemId, TagId) / (ScopedSystemId, EntryId) ValueTuples. Structural equality
for free; the concatenated composite is gone.
Side-effect fix: InMemoryAccountRepository.FindOrCreateSystemIdByDiscordIdAsync /
FindSystemIdByEmailAsync / FindSystemIdByAppleIdAsync had a latent shape mismatch pre-
Round-3 — the write side put the identity value under the RAW newSystemId key while every
downstream reader (LinkIdentifier, Unlink*, GetPublicProfile) looked up by the SCOPED
composite via GetSystemKey. The raw-string dictionary silently accepted both spellings so
the drift was invisible; the retype forces the write side to match the read side and
uses scopedNew as the key.
Verified: `dotnet build Interfold.slnx` clean (0 errors, 100 warnings unchanged);
229/229 Api.UnitTests pass; 374/375 Bootstrapper.UnitTests pass (1 skipped unix-only).
Co-authored-by: Cursor <cursoragent@cursor.com>
* round-3 commit 2: extract UserRegistryLookupColumn enum inside ScyllaUserRegistryRegionContext
Round-3 canvas finding #4. Same shape as Round-2 Commit 11's ProviderColumn extraction
inside ScyllaAccountRepository: the pre-Round-3 LookupAsync had a 5-branch
LookupKind switch dispatching to three magic-string CQL column names ("username",
"discord_id", "user_id"). A new lookup kind added later could easily drift from the
SQL builder if a maintainer forgot to update both sides.
Replace the (column, value) tuple destination with a typed
(UserRegistryLookupColumn, string). The enum is private-nested and reachable only
through a private-static ColumnName helper that produces the on-disk CQL column name at
exactly one call site. The CQL vocabulary stays encapsulated inside this class — no
extension method, no assembly-visible enum, no extra top-level type.
Zero behavioural change: byte-identical CQL statements, byte-identical cache keys,
byte-identical fallback path for unparseable inputs.
Verified: `dotnet build Interfold.slnx` clean (0 errors, 100 warnings unchanged);
229/229 Api.UnitTests pass.
Co-authored-by: Cursor <cursoragent@cursor.com>
* round-4: three residual strong-typing fixes missed by rounds 2 and 3
Round-4 audit refuted the Round-3 "next honest scan returns an empty page" claim
with three real findings. All three ship here as a single small commit — two of them
are the same class of bug already fixed elsewhere in earlier rounds that were missed
during the wide sweep.
Round-4 canvas findings #1 + #2 + #3:
- #1 (MEDIUM · Api): PublicSystemsController.Batch self-check promoted from the
pre-R2C8 `principalId.RawId == SystemIdNormalization.StripRegionPrefix(systemId)`
raw-byte-compare to `principalId.RepresentsSameUserAs(systemId)`. The old spelling
stripped both sides and byte-compared, which falsely self-rejects cross-region
requests where the viewer's `"eur:abcdefg"` asks for `"nam:abcdefg"` (different
user, same raw id, distinct scoped composite). Same substitution R2C8 applied to
8 sites across FriendsController / FriendRequestsController.
- #2 (MEDIUM · InMemory Tag + Fronting): ResolveFriendshipLevelAsync helpers in
InMemoryTagRepository and InMemoryFrontingRepository were doing
`systemId == viewerSystemId` — byte-compare on .Value. The correct reference
implementation lives in InMemoryAlterRepository (which strips region prefixes on
both sides) and the Scylla port has been consolidated onto
ScyllaSharedQueries.ResolveFriendshipLevelAsync since R2C6 (which does the
equivalent normalization). Tag/Fronting drift meant a viewer arriving scoped
("nam:abcdefg") against an owner arriving raw ("abcdefg") would miss the
self-branch and degrade the owner's own-system visibility — the same drift-shape
R3C1 fixed for account lookups. Both helpers now match Alter/Scylla exactly.
- #3 (LOW · Contracts + Api + InMemory): EncryptionSalt.NewRandom() factory added
next to EncryptionSalt in Interfold.Contracts/Ids/EncryptionMaterial.cs; adopted
at AuthController.SetupEncryption and InMemoryAccountRepository.EnsureEncryptionSaltForSystem.
Same shape as R2C4's Jti.NewJti(): the raw base64 string spends zero time as a
bare local (EncryptionSalt.ToString redacts, string.ToString does not) and the
32-byte width lives in the wrapper rather than being hard-coded at each mint site.
Removes the now-unused `using System.Security.Cryptography` from AuthController.
Verified: `dotnet build Interfold.slnx` clean (0 errors, 100 warnings unchanged);
229/229 Api.UnitTests pass.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Round-5 sanity check: post-Round-4 dead-code sweep
Delete three surfaces that Rounds 1–4 orphaned. All grep-verified zero live
callers before deletion. Skipping the fourth candidate (`ExecuteCommandAsync`
on the controller base) — kept as scaffold for a later metrics-migration PR.
Findings:
1. `SocketPushContext.JoinedSystemId` (property + ctor param + `WebSocketHandler`
pass site). R2C13 introduced `JoinedScopedSystemId` and rewired the pump's
`SubscribeAsync` filter over to it. The XML doc preserved the raw sibling
"so socket event handlers don't have to change signatures" — but the
handlers route on `evt.TargetSystemId` fed into `TryGetSystemTopic(SystemId)`
and never read `context.JoinedSystemId`. Only remaining references were the
`WebSocketHandler` ctor pass site and comments in `SocketEventPumpRunner`.
Comments in both files updated to name `JoinedScopedSystemId` instead. The
`WebSocketHandler` `joinedSystemId` local stays (still needed for
`BuildJoinInitPayloadAsync` and `HandleEndpointProxyAsync`).
2. `InterfoldControllerBase.QualifyUrl(string?)`. R2C2 typed every avatar
read-model field to `AvatarUrl?` and R2C10 deleted the untyped
`QualifyAvatar(string?, AvatarSource?)` overload. Every avatar consumer
flows through the typed `QualifyAvatar(AvatarUrl?, AvatarSource?)` today.
Exactly two grep occurrences of `QualifyUrl` in the solution — both the
declaration itself. Non-avatar callers wanting origin qualification without
an `AvatarSource` discriminator can call `AvatarUrlQualifier.Qualify`
directly (that's the shim `QualifyUrl` used to delegate to anyway).
3. Both `InMemoryStorageKeys.NormalizeSystemId` overloads
(`(SystemId) → string` and `(string) → string`). R3C1 introduced
`Normalize(SystemId) → SystemId` so the InMemory repos could key their
dictionaries on the typed wrapper directly. Every InMemory repo migrated
to the typed version. Grep-verified zero references to either overload
across the whole solution — pure migration residue. `SystemIdNormalization`
still calls itself a "public shim" and has 6 live call sites; that seam
is legitimately still in use.
Verified: `dotnet build .\Interfold.slnx` clean at 0 errors, 100 warnings
(unchanged from post-Round-4 baseline). `Interfold.Api.UnitTests` 229/229 pass.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Round-5 sanity check: consolidate InMemory ResolveFriendshipLevelAsync
Finish the Round-2-Commit-6 story on the InMemory port. R2C6 collapsed the
three Scylla `ResolveFriendshipLevelAsync` wrappers onto
`ScyllaSharedQueries.ResolveFriendshipLevelAsync`, leaving the same three-way
duplication on the InMemory side. Round-4 finding #2 fixed the Tag and Fronting
copies to match Alter's reference implementation — that alignment is exactly
what makes the collapse safe now.
Add `InMemoryStorageKeys.ResolveFriendshipLevelAsync(SystemId, SystemId?,
IFriendshipRepository?, CancellationToken)` as the single source of truth for
the InMemory port. The three per-repo private helpers (Alter:369, Tag:281,
Fronting:325) become one-line delegators that thread each repo's nullable
`_friendships` field through the shared static. All five internal call sites
across the three repos remain unchanged — encapsulation of `_friendships` is
preserved.
The R4 attribution comment (StripRegionPrefix self-check on both sides vs the
pre-R4 byte compare that silently missed scoped-vs-raw drift between owner
and viewer) now lives on the shared static's XML doc as the single source of
truth for the invariant.
Verified: `dotnet build .\Interfold.slnx` clean at 0 errors, 100 warnings
(unchanged). `Interfold.Api.UnitTests` 229/229 pass.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Round-5 sanity check: collapse IAccountRepository provider fan-out
Finding 6. Replace the six per-provider public methods on IAccountRepository
with two ProviderIdentity-keyed methods that dispatch inside the repo instead
of at the call site. The pre-Round-5 shape had two coupled footguns that only
became worth fixing after Rounds 2/3 landed:
- The fan-out (FindOrCreateSystemIdByDiscordIdAsync +
FindSystemIdByEmailAsync + FindSystemIdByAppleIdAsync + the three
matching Link* methods) mirrored the same 3-branch pattern-match in
every caller. Both AuthController.Callback and AuthLinkController.Callback
did an identical `typedIdentity switch { { Discord: ... } => ...Discord...,
{ Google: ... } => ...Email..., { Apple: ... } => ...Apple..., _ => ... }`.
R2C7's ProviderIdentity union made the union available; R2C11's
ProviderColumn made the Scylla internal dispatch typed. This commit
finishes that story at the interface itself.
- "FindSystemIdByEmailAsync" and "FindSystemIdByAppleIdAsync" advertised
a "find" verb but their implementations were create-on-miss — only the
Discord method's name (FindOrCreate...) reflected auto-provisioning
truthfully. Consolidating onto FindOrCreateSystemIdAsync honours the
invariant for all three providers and matches the create-on-miss
behaviour the OAuth-login flow always wanted.
TryFindSystemIdByDiscordIdAsync stays. It's the only surviving identity-
specific find method — its Email / Apple siblings never had callers because
the OAuth-login flow always auto-provisions on miss and the friendship
resolver only supports the Discord shape. The XML doc now spells out why it
lives alongside the consolidated FindOrCreateSystemIdAsync rather than
folding in as an "auto-provision?" flag on the union.
Scope:
- IAccountRepository: 6 methods → 2 (FindOrCreateSystemIdAsync +
LinkIdentityToUserAsync), keeping the six existing Unlink* + delete
+ profile methods unchanged.
- ScyllaAccountRepository: 6 line-per-method delegations collapse into 2
ProviderIdentity switch expressions that dispatch into the same
ProviderColumn-typed internal helpers R2C11 already established.
- InMemoryAccountRepository: FindOrCreateSystemIdAsync dispatches to three
private per-branch helpers (each still writes to its distinct pair of
identity/reverse-map dicts, so the write-side logic stays per-provider).
LinkIdentityToUserAsync dispatches via the pre-existing generic
LinkIdentifier helper.
- AuthController.Callback: 3-branch switch → single non-dispatching call.
- AuthLinkController.Callback: 3-branch switch → single non-dispatching call.
- ResolveUserIdDispatchTests.RecordingAccountRepository: 6 NotSupportedException
stubs → 2 (matches the interface shape). Updated the two decision-comment
blocks that referenced the old FindOrCreate* / Find* names.
- SendFriendRequestPrefixTests + ScyllaFriendshipRepository decision-comment
updates to name the new consolidated method.
Verified: dotnet build .\Interfold.slnx clean at 0 errors, 100 warnings
(unchanged from post-Round-4 baseline). Interfold.Api.UnitTests 229/229 pass
(the load-bearing ResolveUserId_DiscordPrefix_UnknownId_ReturnsNull_NoPhantomCreate
pin still holds — it exercises TryFind and the auto-provisioning consolidation
is legibly separate).
Co-authored-by: Cursor <cursoragent@cursor.com>
* Round-5 sanity check: consolidate ResolveMultipartUploadAsync
Finding 7. Extract the byte-identical 59-line multipart-upload helper from
AltersController and SettingsController onto InterfoldControllerBase. Both
controllers inherit from the base already, so the two internal call sites
(AltersController.UploadAvatar and SettingsController.UploadAvatar) bind
unchanged to the newly-inherited protected method.
Honesty note: this is not a typing-unlocked refactor. The duplicated helper
predates Round 1 entirely — its shape reads from HttpRequest.Body and returns
an AvatarUploadPayload, neither of which was touched by the four rounds of
strong-typing work. Included in Round 5 because Category 2 of the sanity
check ("N-copy helpers that can now share one implementation") explicitly
covers cloned controller helpers, and this is the largest verbatim controller
clone still standing in the API layer. Flagged as such in the extracted
method's XML doc so a future reader understands why it landed in this batch
rather than in a strong-typing commit.
Also trims the four `using`s that were only there for the deleted helper's
call surface (MultipartReader / MediaTypeHeaderValue / etc.) — two
per-controller. Net dedup: ~60 lines removed from the two controllers, ~90
added on the base (helper + full XML doc explaining the fold), so line count
is roughly break-even but the drift-risk surface halves.
Verified: dotnet build .\Interfold.slnx clean at 0 errors, 100 warnings
(unchanged from post-Round-4 baseline). Interfold.Api.UnitTests 229/229 pass.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Round-5 sanity check: inline FriendshipSocketEventHandlers.IdMatches
Finding 8. Delete the two-line `private static bool IdMatches(string? left,
string right) => SystemTopic.IdMatches(left, right)` pass-through in
FriendshipSocketEventHandlers and inline the single call site to call
SystemTopic.IdMatches directly. The private wrapper added no behaviour beyond
narrowing the second parameter's nullability annotation — pure residue from
the Round-1 socket-token typing work that never earned its keep as a distinct
symbol.
Verified: dotnet build .\Interfold.slnx clean at 0 errors, 100 warnings
(unchanged). Interfold.Api.UnitTests 229/229 pass.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Simplify code comments across the codebase
Strip internal audit terminology (round / slice / commit / finding / canvas
references) and condense verbose commentary into terse, purposeful comments
that focus on the current invariants, contracts, and rationale readers need.
No production or test behaviour changes; XML docs and inline comments only.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Refactor Scylla repositories to use string for normalizedSystemId
Co-authored-by: Cursor <cursoragent@cursor.com>
* refactor(wrappers): adopt target-typed new() and implicit-widen at call sites
The id/token wrappers in Interfold.Contracts.Ids ship explicit narrow
(`(W)raw`) and — for non-PII wrappers — implicit widen (`W -> primitive`)
operators. This adopts them at ~80 sites across Interfold.Domain, .SPDump,
.Infrastructure.InMemory, .Api, .Api.UnitTests, and .IntegrationTests:
- `new W(x)` -> target-typed `new(x)` at typed sinks (variable declarations,
returns, arguments, initializers). `var` bindings are spelled out
(`SystemId x = new(raw);`) rather than falling back to `(W)raw`.
- `.Value` dropped at string sinks where the widen is direct:
IsNullOrWhiteSpace, TryParseScoped, StripRegionPrefix, IdMatches, dict
lookups, `$"..."` interpolation, and StringComparer.Ordinal-anchored
OrderBy keys via `(string)w`.
PII-redacting wrappers (DiscordId, Email, AppleId, LinkToken, PushToken,
ImportToken, RecoveryCode, Jti, SocketToken, EncryptionKeyMaterial,
KeyChecksum, EncryptionSalt) intentionally have no implicit widen — dropping
`.Value` on those would silently defeat SecretRedaction under any
`Foo(string)` overload. Construction still adopts `new()`, but every unwrap
keeps `.Value`. Guarded by PiiWrapperImplicitWidenAbsenceTests.
* Adopt wrapper conversion operators across Scylla infrastructure
* Back TagId/PollId/EntryId/FrontId/FieldId with Guid instead of string
Wrappers now hold a Guid directly; route binding and JsonConverter reject
non-uuid input at the boundary via UuidString.TryParse, and the wire form
(compact 32-char hex) is preserved so idempotency hashes and response
bodies stay byte-identical. Deletes every per-repo TryParseUuid guard
across Scylla + InMemory, collapses the FieldId UDT-join in
ScyllaSharedQueries to a direct Guid compare, and swaps
Dictionary<string, string?>/Guid.Parse for Dictionary<Guid, string?> in
ScyllaAlterRepository. Boundary consumers that used
string.IsNullOrWhiteSpace on the wrapper switch to Guid.Empty checks.
Sort keys made explicit (.Value.ToString("N")) to keep list ordering
stable.
* Centralize Enum handling and use the built in JsonStringEnumConverter
* [Skip CI] reword some comments
* [Skip CI]
- Removed namespace
- Inlined consts where it made sense.
- Use Mocking in place of some stubs
* Tighten friend-request wire contract to id/username only. Move and rename LookupHandle to UserRegistryLookup for internal registry routing. Update docs and comments to clarify contract and rationale.
* Bind Alter Id check within models, to add for struct itself in a second pass
* Move remaining alter id checks to new logic
* Remove BaseRequest
* Refactor system ID normalization to use ScopedSystemId. Removed SystemIdNormalization in favor of ScopedSystemId's StripRegionPrefix method across various files, ensuring a single source of truth for region prefix stripping. Updated related comments and documentation for clarity.
* Many self code review changes
* BInd UnixSeconds and fix tests failing
* Refactor SimplyPluralImportService color parsing to use HexColor normalization. Introduce TolerantWireEnumJsonConverter for nullable enum handling in PhxJoinPayload, ensuring unknown platform values round-trip to null without exceptions. Add unit tests for AlterId and HexColor to enforce strict validation and JSON serialization behavior.
* Fix remaining unit test issues
* Last set of reviews for this PR
* Don't drop & rename, we are unable to because of the Syclla limits...
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
Azyyyyyy
referenced
this pull request
in Azyyyyyy/Interfold
Jul 21, 2026
- #18 / #19 InMemoryFrontingRepository — drop the nullable-and-defaulted ctor parameters on `_friendships` / `_alters` and their fields. DI wires both (InMemoryServiceCollectionExtensions.AddInMemoryPersistence) so the nullability wasn't buying anything and the two guard branches in ListActiveAsync / ListActiveGuardedAsync would silently hide a DI mis- wiring instead of surfacing it. Collapses the "have `_alters` or fall back to a placeholder" branch back to a single expression. - #24 ScyllaSharedQueries.ResolveAlterFields — swap the FirstOrDefault scan for a Dictionary<Guid, string?> keyed by field id so the per- definition projection is O(N + M) instead of O(N * M). Same visible behaviour on the wire; a system with a handful of alters and a handful of settings-field definitions no longer allocates a fresh iterator per definition. Also reverts the manual `def.Id.Value` unwrap to `def.Id` and lets FieldId's implicit-to-Guid operator do the work — matches wrapper-typed comparison intent from the review. - #15 New AlterCommandFlowExecutionTests — three targeted tests that pin UpdateAlterCommandHandler's reject / mutation-failed / happy-path branches through the same call-count assertion style as PollCommandFlowExecutionTests. The MissingAlter case is the specific coverage gap the review flagged; the other two guard against a future refactor accidentally catching the accept path in the reject branch. Build clean (0 errors). New tests: 3 pass. Neighbour suites (Front*, Alter*) all pass. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Notable changes:
/bot-infoTODO:
/front/friendCheck commits for full changelog.
Fixes OctoconDev/issues#355.
Fixes OctoconDev/issues#474.