Conversation
- Introduced InMemoryAlterRepositoryTests to ensure direct repository coverage for alter visibility behavior. - Implemented tests for various visibility levels (Public, Friends Only, Trusted Only, Private) to validate filtering based on viewer friendship levels. - Added helper methods for seeding friendships and creating alters to facilitate comprehensive testing. - Consolidated visibility logic checks to ensure consistency with existing controller tests, addressing potential regression issues. This commit enhances the test suite by closing the gap in repository-level testing for the InMemory backend, ensuring robust coverage for visibility-related functionality.
…ile documenting extensive schema unification and migration plans.
…frastructure services, and migration architecture documentation.
…zing migration and refactoring documentation
… across controllers and command handlers
Azyyyyyy
commented
Jul 18, 2026
…ulations. Also fixed failing tests
DeleteFrontByIdAsync cleared any active front on the alter when the front being deleted was found in history, regardless of whether the active front was the same one. This caused a subsequent active front (F2) to be wiped when the predecessor (F1) was deleted from history. Fix: mirror Scylla's guard — only clear the active entry when its FrontId matches the front being deleted. Adds fronting-delete-parity.trace.json to ReplayParityTests to cover the scenario: start F1, end F1, start F2, delete F1 => F2 must remain active.
…writers [R2-A2] UpdateAlterAsync, DeleteAlterAsync, SetAlterLockedAsync, SetAlterPinnedAsync wrapped their bodies in DatabaseTransientRetry.ExecuteScyllaAsync and then called GetAlterRefAsync which itself uses _scopeResolver.ExecuteAsync (another retry envelope). Transient errors would retry 5x5=25 times. Fix: rewrite all four methods to use _scopeResolver.ExecuteAsync directly, with a private GetAlterRefCoreAsync(ScyllaScope, EntryId, ct) that reads the ref within the already-open scope — eliminating the nested envelope.
…rtJobRunner [R2-A3] Round 1 left PkImportJobRunner returning ImportFailed. The plan wording called for ImportErrorCode.Unimplemented to distinguish 'this importer is not built yet' from 'a real importer ran and returned a graceful failure'. Decision: add Unimplemented (wire: 'unimplemented') to ImportErrorCode and switch the stub runner. ImportFailed retains its 'generic fallback for runners that failed without a specific code' meaning.
…t sites [R2-B1]
Creates TestClient.NoRedirect(IWebFactoryFixture) and NoRedirect(InterfoldWebApplicationFactory)
to eliminate the 42 identical new WebApplicationFactoryClientOptions { AllowAutoRedirect = false }
blocks across 14 test files.
Also bundles D13:
- Add SendAuthedDeleteAsync to TestJson
- Sweep 4 raw HttpRequestMessage(HttpMethod.Delete) sites
- Fix AlterJournalsControllerTests:17 empty-options quirk (inconsistent with all siblings,
normalised to AllowAutoRedirect = false)
…9 handler sites [R2-B3] Sweeps 19 'TryGetSystemTopic' guards across the event handlers in Alter, Fronting, Import, Journal, Poll, Settings, Tag, and Friendship socket handler classes to use the unified SendIfJoinedAsync helper.
… sweep 9 controllers [R2-B2] Introduces DispatchNoContentAsync, DispatchAcceptedAsync, and DispatchCreatedAsync helpers on the base controller, centralising command envelope building, handling, and response mapping. Sweeps ~54 endpoints across all 9 controllers (excluding AuthController) to use the unified dispatchers, making their bodies cleaner and reducing boilerplate.
…sweep 22 sites [R2-B4] Adds ExecuteGlobalAsync overloads to IScyllaScopeResolver to allow executing global (non-regional) queries with transient retry policy. Sweeps 22 DatabaseTransientRetry sites onto the scope resolver: - 12 global sites in ScyllaFriendshipRepository - 3 global sites in ScyllaNotificationTokenRepository - 7 regional/global sites in ScyllaImportOperationRepository
Extracts MapAlterReadModel to AlterRowMappers in Scylla infrastructure, replacing 2 duplicate mapping expressions inside ScyllaAlterRepository. Also extracts private MapAlterReadModel helper to InMemoryAlterRepository as a mirror to remove 2 duplicate mappings in the in-memory driver.
…del [R2-B5-b] Extracts MapFrontHistoryReadModel to a new FrontingRowMappers class, replacing 4 duplicate mapping statements in ScyllaFrontingRepository.
Extracts MapAlterJournalReadModel and MapJournalReadModel to a new JournalRowMappers class, replacing 4 duplicate mapping statements inside ScyllaJournalRepository. Also extracts private MapAlterJournalReadModel and MapJournalReadModel mirrors to InMemoryJournalRepository to clean up 4 duplicate mappings in the in-memory driver.
Extracts MapTagReadModel and MapTagPublicReadModel to TagRowMappers class, replacing 4 duplicate mapping statements inside ScyllaTagRepository. Also extracts private MapTagReadModel and MapTagPublicReadModel mirrors to InMemoryTagRepository to clean up 4 duplicate mappings in the in-memory driver.
…256Validator [R2-B7]
… sites [R2-C1]
Round 2 wave C, item 1. The `BootstrapAsync` / `PublishAsync` helpers added in
Round 1 cover the "top of test body: fresh scratch + primary bootstrap/publish"
shape, but the DinD suite has dozens of *secondary* subcommand invocations
(`install-service`, `update-images`, `backup`, `restore`, `rotate-secrets`,
`rotate-certs`, second `bootstrap`, etc.) against an existing scratch that were
still hand-rolling the same six-item arg list per site.
Add `DinDFixtureBase.RunOnScratchAsync(scratch, testName, command, extraArgs)`
which pre-populates `--config`, `--output-dir`, `--non-interactive` from the
scratch and appends the caller's extra flags. It deliberately does NOT assert
exit code so callers keep flexibility for the exit-non-zero cases
(HealthTimeoutDumpsComposeLogs, RestoreWithoutArchivesFailsClearly,
BackupWithoutComposeFailsClearly, UnsupportedOsTests, etc.).
Sweeps applied:
SystemdInstallTests 7 sites (all install-service invocations)
RestorePhaseTests 6 sites (backup / restore combos)
BackupPhaseTests 5 sites (backup + retention loop + misuse)
UbuntuBootstrapTests 5 sites (rotate-secrets, rotate-certs,
IsIdempotentOnRerun,
RecoversFromInterruptedPublish)
BootstrapIdempotenceTests 4 sites (bootstrap re-run pairs)
UpdateImagesPhaseTests 4 sites (update-images + misuse)
LaunchPhaseTests 2 sites (HealthTimeout, RespectsCustomApiHttpPort)
UpdateImagesCassandraModeTests 2 sites (cassandra-mode update-images)
DbInitFaultRecoveryTests 2 sites (fault-inject halt + resume)
MdnsGateTests 1 site
UnsupportedOsTests 1 site
UbuntuBootstrapTests 2 sites PublishAsync sweep (single-shot pubs)
TrustStoreFalseTests 1 site PublishAsync sweep
The `up` subcommand invocation in LaunchPhaseTests deliberately omits --config
(it operates on the pre-published output dir only) and stays on the raw
RunBootstrapperAsync path; the four PrereqsPhaseTests sites also intentionally
omit --config/--output-dir to exercise the no-config path.
Also folds in two pre-migrated sites (TrustDownloadTests, WebHttpsTests) that
already used BootstrapAsync/PublishAsync from a prior working session -- they're
the same pattern of work.
15 files touched, +136 / -192 (net -56 LOC).
Co-authored-by: Cursor <cursoragent@cursor.com>
Update the round-2 dedup plan to reflect that C1 shipped as commit d863855 under a broader scope than originally spec'd. The bullet's `~59 sites onto BootstrapAsync` framing was based on the raw `RunBootstrapperAsync` mention count; the actual sweep-able set was 3 sites for `BootstrapAsync`/`PublishAsync` plus ~40 sites once a second helper (`RunOnScratchAsync`) was added to cover secondary subcommand invocations against an existing scratch. The full progress banner now lists C1 in the completed set (16 commits, was 15). Co-authored-by: Cursor <cursoragent@cursor.com>
…dentifier generics [R2-C2]
Round-2 C2 closes the B7 tail on the InMemory adapter: the three
FindOrCreateSystemIdBy{Discord,Email,Apple} bodies and the three
Unlink{Discord,Email,Apple}Async bodies were hand-copied six-way. All
six now dispatch to two generics keyed by the identity wrapper type +
a static extractor lambda, matching the shape B7 used for
LinkIdentifier<TIdentity>. DeleteAsync's identity cleanup also routes
through the new UnlinkIdentifier so the three-block copy inside Delete
collapses to three one-line calls.
Scylla side was already factored via UnlinkIdentityAsync(systemId,
ProviderColumn) in an earlier pass, so this bullet touches InMemory
only.
Adds InMemoryAccountRepositoryIdentityRegressionTests covering the
core refactor risk (dict-pair swap - UnlinkDiscord scrubbing the email
map, etc.) plus the round-trip / idempotency / case-insensitivity
invariants that had no direct unit coverage before this bullet. All 8
new tests pass; the 4 existing link-token regression tests still pass;
373/373 unit tests green overall.
Net -42 LOC on InMemoryAccountRepository.cs (79 ins, 121 del).
Co-authored-by: Cursor <cursoragent@cursor.com>
Add PsqlAsync and CqlshAsync to DinDFixtureBase so the 'docker compose -f {composeFile} exec -T [-e PGPASSWORD=...] msg-db psql -U ... -d ... -h ... {sql}' shape and the corresponding 'docker compose -f {composeFile} exec -T scylla cqlsh -u ... -p ... -e {cql} 2>&1 || true' shape land in one place. Password parameters are forwarded verbatim so literal values, $ADMIN_PW-style shell variables, and $(grep ...) subshells all keep working. Quoted SQL/CQL expressions are also passed verbatim so callers keep control of shell quoting.
Sweep 10 sites: DbInitSecurityInvariantsTests (2 cqlsh + 4 psql), DbInitFaultRecoveryTests (2 cqlsh + 1 psql), RestorePhaseTests (4 psql). RestorePhase pre-extracts the admin password once via a single ExecAsync + grep|sed instead of re-deriving ADMIN_PW=\ inside every compound sh -c. Drops one redundant 'using System.Text;' now that ParseEnv is gone from the test.
Co-authored-by: Cursor <cursoragent@cursor.com>
…sts body extract B8: add BaseEndpointTest.SeedVisibilityQuartetAsync(client, prefix) that seeds the four viewer principals (owner + nonFriend + friend + trusted) with a public profile each and an alter on each non-owner (needed so the guarded reads have a row to gate). Deliberately unifies on the 'seed all four' shape that PublicSystemsControllerTests already used — AltersControllerTests had been seeding only the owner, which risked masking a test-side race where the friend-request accept resolves a viewer id with no profile row yet. Sweep 5 sites: 3 in PublicSystemsControllerTests, 2 in AltersControllerTests. C1: with the setup lifted into the shared helper, the two AltersControllerTests methods FieldSecurityLevelByRelationship_AppliesCorrectly and CustomFields_FieldSecurityLevelByRelationship_AppliesCorrectly are byte-identical except for the client type (TestClient.NoRedirect vs fixture.Factory.CreateClient) and the principal prefix. Extract the 60-line shared body into a private static RunFieldVisibilityScenarioAsync(client, prefix) so the mirror pair keeps two entry points (pinning both codepaths) but only one copy of the assertion body. Also fixes the line-45 friend->nonFriend drift bug that existed before this refactor and would silently pass because the assertion set didn't distinguish the two viewers' expected slice. Co-authored-by: Cursor <cursoragent@cursor.com>
…to DockerCompose
Add three shared builders to DockerCompose:
* BuildPostgresExecArgs(composeFile, service, tool, adminUser, database, params trailer) — the 'compose -f X exec -T --env PGPASSWORD service tool -U user -d db {trailer}' preamble that pg_dump, pg_restore, and (future) pg_isready-with-creds all share. The password lands via --env so it never appears on argv, matching the security invariant asserted by BackupCommandBuildingTests.
* BuildContainerCpFromContainer(id, path) — 'docker cp id:path -' (stdout stream).
* BuildContainerCpIntoContainer(id, path) — 'docker cp - id:path' (stdin stream).
Retarget BackupPhase.BuildPostgresDumpArgs / BuildContainerCpArgs and RestorePhase.BuildPgRestoreArgs / BuildContainerCpWriteArgs as one-line delegators so the existing BackupCommandBuildingTests + RestoreCommandBuildingTests argv-shape assertions continue to pin the phase-level contract (all 20 tests still pass). The next phase that needs to exec postgres tools as admin (a future WaitForPostgresAsync variant, an ad-hoc DDL runner) picks up the shared preamble by construction rather than re-hand-rolling the auth-sensitive '--env PGPASSWORD' positioning.
Co-authored-by: Cursor <cursoragent@cursor.com>
The initial B3 landing used a bare 'string column' parameter on SetGlobalFlagAsync and SetAlterFlagAsync, which meant a future caller could still pass an arbitrary column identifier and slip a CQL-injection-adjacent literal into the interpolated UPDATE statement. The plan explicitly called for a closed 'JournalFlag' enum for exactly this reason - callers physically cannot pass anything other than Locked or Pinned. Add a private nested 'JournalFlag' enum (Locked, Pinned) and a private static FlagToColumn(JournalFlag) mapper. Retarget both Set(Global|Alter)FlagAsync signatures onto the enum and update the four public entry points (SetGlobalLockedAsync, SetGlobalPinnedAsync, SetAlterLockedAsync, SetAlterPinnedAsync) to pass JournalFlag.Locked / JournalFlag.Pinned. FlagToColumn is the single place the enum-to-column-name mapping lives, and it throws ArgumentOutOfRangeException on any future enum variant that lacks a mapping so the next hand that adds JournalFlag.Archived (or similar) is forced to update the mapper in the same commit. Co-authored-by: Cursor <cursoragent@cursor.com>
- Refactored the SettingsController to utilize a new DispatchEncryptionKeyAsync method, reducing code duplication in the encryption setup and recovery processes. - Updated FriendshipSocketEventHandlers to streamline friendship retrieval with a retry mechanism, improving readability and maintainability. - Consolidated common logic in FrontingSocketEventHandlers and SettingsSocketEventHandlers for handling socket events, enhancing code clarity. - Introduced a new UntilNotNullAsync method to handle eventual consistency in fetching friendship and friend request data, reducing boilerplate code across multiple handlers. - Added FixedRegionContext to unit tests for consistent region handling in identity-related tests. These changes aim to improve code organization and reduce redundancy across the codebase.
…ffects on missing polls The new PollCommandFlow helper was introduced with hot Task/ValueTask parameters, which C# evaluates eagerly. That meant _repo.UpdateAsync/ DeleteAsync and _bus.PublishAsync ran at the call site inside ExecuteUpdateAsync/ExecuteDeleteAsync, BEFORE the ExistsAsync check inside the helper. Symptoms on a not-found row: * a spurious UPDATE/DELETE against the persistence adapter, and * a spurious PollUpdatedEvent / PollDeletedEvent on the cluster bus. Both are invisible in the HTTP-level integration tests (the handler still returns poll:not_found), which is exactly why the regression slipped through. They corrupt downstream observers and can throw unobserved exceptions in the returned-but-unawaited Task. Fix: change ExecuteExistingPollMutationAsync's mutate/publish params from Task<bool>/ValueTask to Func<CT, Task<bool>>/Func<CT, ValueTask> so they're only invoked after the existence check passes, and publishAccepted only fires on the accepted branch. Also adds a comment at the seam explaining the trap for the next refactor. Adds PollCommandFlowExecutionTests to lock the invariant in with hand-rolled counting fakes: * DeletePoll_MissingPoll_DoesNotMutateOrPublish * DeletePoll_ExistsButMutationReturnsFalse_RejectsWithDeleteFailedAndDoesNotPublish * DeletePoll_HappyPath_CallsMutateOncePublishesOnceAndAccepts * UpdatePoll_MissingPoll_DoesNotMutateOrPublish The Update and Delete branches share the helper; pinning them separately guarantees drift in one branch cannot silently pass in the other. Co-authored-by: Cursor <cursoragent@cursor.com>
…ueries -> 1) The health check used to issue one `SELECT table_name FROM system_schema.tables WHERE keyspace_name = ? AND table_name = ? LIMIT 1` per required table, sequentially per keyspace. On the current schema that is 20+ round-trips per health probe. Replaces that loop with a single keyspace-scoped query and an in-memory HashSet<string> lookup (OrdinalIgnoreCase for Cassandra's case-insensitive table names). Behaviour is unchanged: the returned list of missing tables is a subset of the input in the same order. Also removes the pre-existing TODO that flagged this exact optimisation. Co-authored-by: Cursor <cursoragent@cursor.com>
…) helper Each Scylla repository previously duplicated the same ScyllaExistsQueries.RowExistsAsync(session, keyspace, "table", "id", normalizedSystemId, id.Value) call at every existence check inside ExistsAsync + UpdateAsync + DeleteAsync + related paths. That constructed the same 6-arg call 2-3 times per repository, each carrying the "which table + which id column" wiring inline. Extracts a private static ExistsAsync(ScyllaScope, TId) helper on each repository so the table + id-column encoding lives in one place, and in-scope mutations reduce to `await ExistsAsync(scope, id)`. The public ExistsAsync(SystemId, TId, CT) collapses to _scopeResolver.ExecuteAsync(systemId, scope => ExistsAsync(scope, id), ct). Behaviour unchanged: same query, same params, same result. Just moves the "table X keyed by column Y" wiring off every call site and into one line per repository. Co-authored-by: Cursor <cursoragent@cursor.com>
…repositories
Several InMemory repositories had the same
InMemoryStorageKeys.ForSystem(_regionContext, systemId) +
_bySystem.TryGetValue(systemKey, out var store) pattern repeated
across List / Get / Exists / Update / Delete / Relocate / etc. The
existing pattern in InMemorySettingsFieldRepository, InMemoryPoll,
InMemoryJournal, InMemoryTag already used private TryGetStore
helpers; this brings InMemoryAlterRepository (and refreshes the
others) into line so every store-lookup call site becomes:
if (!TryGetStore(systemId, out var store)) return <empty>;
Also lifts a TryGetAlter/TryGetPoll/TryGetAlterEntry helper where the
same "resolve store then key into it" pair repeated. Behaviour
unchanged: same lock scope, same fallbacks, same friendship-level
resolution.
Co-authored-by: Cursor <cursoragent@cursor.com>
…malization helpers Introduces per-aggregate flow helpers that centralise the recurring "validate -> mutate -> check success -> publish -> return Success" shape that every command handler in these aggregates was open-coding: * Alters/AlterCommandFlow.cs * Tags/TagCommandFlow.cs (+ TagCommandValidation.cs) * Fronting/FrontingCommandFlow.cs * Journals/GlobalJournalCommandFlow.cs * Journals/AlterJournalCommandFlow.cs * Friendships/FriendshipCommandFlow.cs * Friendships/FriendshipEventFlow.cs * Friendships/FriendshipCommandNormalization.cs FriendshipEventFlow centralises the "publish A-side + publish B-side" pair-event dance the 5 friendship handlers (Accept / Cancel / Reject / Remove / Send / SetFriendTrust) were repeating with slightly different ordering. FriendshipCommandNormalization encodes the "peer inherits principal's region" invariant in one place instead of 10 call sites of ScopedSystemId.Compose(principalId.Region, peer). All parameter types that model deferred work (mutate/publish/apply) are Func<CT, ...> rather than hot Task/ValueTask so nothing runs before its guarded call point - matches the discipline restored on PollCommandFlow in the preceding fix commit. Handler bodies collapse from ~30-40 lines of open-coded logic to a single delegating call each. Co-authored-by: Cursor <cursoragent@cursor.com>
…+ inline AuthToken wrappers + tidy misc auth helpers
Three related tidies that all sit at the domain / api boundary:
1. Settings namespace consolidation
- Move SettingsCommandHelper from Interfold.Domain into
Interfold.Domain.Settings so all three sibling helpers
(SettingsCommandHelper, SettingsIdempotentCommandFlow,
SettingsFieldCommandFlow) live in the same namespace. Also
move the physical file into the matching folder.
- Introduce the two lighter helpers alongside the existing
full-fat SettingsCommandHelper:
* SettingsIdempotentCommandFlow - lightweight mutate/publish
body for handlers that already inherit
IdempotentCommandHandler (the base handles idempotency).
* SettingsFieldCommandFlow - dedicated shape for the
SettingsFieldCommandResult path (result carries FieldId).
- Add an XML-doc "type-choice table" on SettingsCommandHelper
that resolves the drift risk: a reader landing on any of the
three sees the discovery rule for picking between them.
- Adopt SettingsCommandHelper.PublishProfileUpdatedAsync at
UpdateUsernameCommandHandler (was constructing the event by
hand). Add the required `using Interfold.Domain.Settings;` to
Accounts + Fronting call sites that live outside the Settings
namespace. (The Fronting `using` addition rode along with the
preceding FrontingCommandFlow commit for the same file - it
became load-bearing here after the namespace move.)
2. Auth-token wrapper inlined
- Delete AuthTokenCommandFlow (a 2-line wrapper around
`await x; return Success(y);` with only 2 callers) and inline
the body in RecordAuthTokenCommandHandler /
RevokeAuthTokenCommandHandler. At two call sites the helper
hurt readability more than it helped.
3. Misc auth tidy
- AuthenticateOAuthCommandHandler / LinkOAuthIdentityCommandHandler:
replace the inline `CommandExecutionResult.Rejected(new ConflictResult(...))`
construction with the newer CommandHandler.RejectInvariant<T>()
helper for consistency with other handlers in the codebase.
- AuthController / OAuthControllerBase: extract the shared
"OAuth identity failure" 403 response as
OAuthControllerBase.OAuthIdentityFailureResponse() so the
message string lives in one place.
Co-authored-by: Cursor <cursoragent@cursor.com>
…ge and delete RefactorTool
Three IntegrationTests files were previously TODO stubs
(`//TODO: MAKE`); this fills them out with real coverage:
* AuthLinkControllerTests - Begin missing-redirect-uri 400, valid
Begin round-trip, unknown-provider handling, etc.
* FriendRequestsControllerTests - request send / list / cancel /
accept / reject / duplicate suppression, principal isolation.
* FriendsControllerTests - list / trust-set / trust-unset / remove
happy paths + rejection edges.
Net +345 / -6. All three files continue to use the standard
InMemoryWebFactoryFixture + ScyllaWebFactoryFixture +
CassandraWebFactoryFixture triple so the same tests run against every
persistence adapter.
Also deletes the one-off csharp/RefactorTool project (Program.cs +
RefactorTool.csproj). Its jobs were completed by the recent
deduplication rounds and it is no longer referenced from
Interfold.slnx or any build path.
Co-authored-by: Cursor <cursoragent@cursor.com>
…on methods
Move the 15 pure "publish a well-known event tuple" helpers off their static
containers and onto IClusterEventBus as extension methods, so command-handler
bodies read as `_eventBus.PublishFriendshipAddedBothWaysAsync(from, to, ct)`
instead of `FriendshipEventFlow.PublishFriendshipAddedBothWaysAsync(_eventBus,
from, to, ct)`. The noun the caller cares about (the bus) sits at the front
and the leading `_eventBus,` argument disappears from every call site.
Splits by aggregate:
- Interfold.Domain.Settings.SettingsEventBusExtensions
· PublishProfileUpdatedAsync (moved out of SettingsCommandHelper)
- Interfold.Domain.Friendships.FriendshipEventBusExtensions
· Publish{Friendship{Added,Removed}BothWays,
Request{RemovedFromThenTo,RemovedToThenFrom}}Async
· FriendshipEventFlow deleted — every method moved
- Interfold.Domain.Fronting.FrontingEventBusExtensions
· PublishStateChangedAsync + 6 "…And…" transition variants
(Started/Ended/Set/BulkUpdated/CommentUpdated/PrimaryChanged)
· PublishDeletedAsync, PublishEndedForAltersAsync,
PublishPrimaryClearedIfNeededAsync
· Removed from FrontingCommandFlow — that class now holds only orchestration
(validation, mutation-or-reject, front-by-id resolution, alter-fanout);
Interfold.Contracts.Events using no longer needed there.
Call-site refactor across 17 handlers:
· 5 Settings: DeleteAvatar, SetupEncryption, UpdateDescription, UploadAvatar,
plus SetPrimaryFront + UpdateUsername
· 5 Friendships: Accept, Cancel, Reject, Remove, Send
· 7 Fronting: BulkUpdate, DeleteFrontById, EndFront, SetFront,
SetPrimaryFront, StartFront, UpdateFrontComment
Same-assembly extension resolution — every caller lives in Interfold.Domain
and either shares the extension's namespace or already has the appropriate
`using` (UpdateUsernameCommandHandler and SetPrimaryFrontCommandHandler both
already have `using Interfold.Domain.Settings;` from prior commits).
Verified: full solution build clean (0 errors), Interfold.Api.UnitTests 377/377
passing, Interfold.Bootstrapper.UnitTests 382 passing + 1 Unix-only skip.
Co-authored-by: Cursor <cursoragent@cursor.com>
Consolidates Round 6 and Round 7 dedup sweeps, fixes three clusters of
pre-existing CI failures uncovered by the sweeps, and closes with a small
QoL pass on InterfoldWebApplicationFactory so tests read the persistence
mode off the shared enum instead of round-tripping magic strings.
Round 6 — dedup
- Contracts: extract StringBackedJsonConverter<T> base and fold 8 JSON
converters (AvatarUrl, EncryptionMaterial, EntityRef, ErrorCode,
IdempotencyKey, IdentityWrappers, OperationId, PollData.Question,
SystemId) onto it. Wire-format contract preserved — WireByteFreezeTests
still green.
- Scylla: delete the standalone ApplySchema method on ScyllaMigrationService
and route its single caller through ApplyTemplatedMigrationPerKeyspace
with SchemaMigration.
- Bootstrapper tests: replace the last four `grep | sed` secrets-JSON
extractions in DbInitSecurityInvariantsTests.cs and
DbInitFaultRecoveryTests.cs with dinD.ReadSecretsFieldAsync so the
helper is the single source of truth for that shape.
- Bootstrapper Aspire: introduce
PublishPhase.EnumerateSharedAspireParameters as the single source of
truth for the 24 shared Aspire params, seeded into both BuildEnvReplacements
(kebab-case .env) and PublishInProcessAsync (upper-snake-case in-memory
config). New unit test in PublishEnvPostProcessingTests pins both the
count and the kebab→upper-snake mapping.
Round 7 — duplicate-type sweep
- Delete Interfold.Domain.Abstractions.SpImportResult and re-type
ISimplyPluralImportService.ImportAsync to return
Interfold.Domain.Abstractions.ImportJobs.ImportJobOutcome directly.
SpImportJobRunner.RunAsync is now a pure pass-through; the terminal-state
contract (graceful failures must populate ErrorCode, throws classify as
`exception`) lives on the service, not translated at the runner boundary.
Callers in SimplyPluralImportService (ImportAsync + ValidateEncryptionKeyAsync)
and the SpImportTests helper updated. Verified with SpImportTests
(23/23) and ImportJobBackgroundServiceTests (5/5).
- Delete the private nested FixedRegionContext in
InMemoryAlterRepositoryTests and adopt the shared Support/FixedRegionContext
helper. Verified with InMemoryAlterRepositoryTests (5/5).
CI fixes uncovered by the sweep
- AuthLinkController.cs: two independent fixes.
· Begin: add up-front redirect_uri guard returning 400 MissingRedirectUri
(was landing on the challenge-issue 400 branch after cookie writes had
already happened, tripping the documented contract).
· Callback: this route is [AllowAnonymous], so BuildEnvelope's PrincipalId
lookup throws InvalidOperationException before the handler ever runs.
Mirror AuthController.Callback and hand-build the CommandEnvelope with
a synthetic `nam:auth` principal — the LinkOAuthIdentityCommand handler
resolves the real system id off the link token itself, so command
PrincipalId is never observed.
- AuthLinkControllerTests.cs: root-cause was WebApplicationFactory<Program>
handing back different IServiceProvider trees for `Factory.Services` vs
an HTTP request scope, so the direct-repo seed wrote to a different
InMemory dictionary than the callback read from. Move both flow tests
onto per-test private factories pinned to NodeGroup=Primary, and seed
the link token via GET /api/settings/link_token (Primary-only write
path) so seed and callback share one SP.
- DinDFixtureBase.CountFilesAsync: change the signature from
`(string globExpr)` to `(DinDScratch scratch, string relativePathOrGlob)`
so callers can no longer forget the `$` on `"{scratch.OutputDir}/…"` (that
bug had five instances across BackupPhaseTests and UpdateImagesPhaseTests
which had been shipping since the previous CI green). Sweep updates all
ten call sites to pass the scratch as a first-class arg.
- UnsupportedOsTests.cs: the "no partial artifacts" assertion was comparing
an ExecResult against `0` instead of parsing the wc -l stdout. Parse the
stdout to int and assert on that.
QoL — typed persistence mode on InterfoldWebApplicationFactory
- Change the InterfoldWebApplicationFactory ctor from `string persistenceType`
to `PersistenceMode persistenceMode`. The wire spelling is produced inside
the ctor via EnumWireExtensions.ToWire<TEnum> — the same single source of
truth every other Interfold enum uses. Expose a public
`PersistenceMode PersistenceMode { get; }` for tests that need to layer
extra config onto a sibling factory. Internal `_persistenceType == "inmemory"`
string comparisons fold to `PersistenceMode != PersistenceMode.InMemory`.
- Update the 3 fixtures (InMemory / Scylla / Cassandra) and 3 controller
tests (ClusterRoleAndOAuthRegistrationTests, InMemorySecretsSeedTests,
TrustControllerTests) to pass the enum instead of `"inmemory"` /
`"scylla-postgres"` magic strings.
- AuthLinkControllerTests: delete the fixture-type-switch
GetPersistenceType() helper and read `fixture.Factory.PersistenceMode`
directly. Merge ApplyBackendConfiguration into a single
CreatePrimaryNodeFactory() helper that lifts the four shared
OCTOCON_SINGLE_SCYLLA_INSTANCE / OCTOCON_DB_RETRY_* calls out of the
Scylla/Cassandra branches — only the two per-fixture values (Postgres
connection + CQL port) live inside the switch, as a tuple projection
(`null` means "InMemory, no external backend").
Verified: dotnet build ./csharp/Interfold.IntegrationTests clean (0 errors);
AuthLinkControllerTests, TrustControllerTests,
ClusterRoleAndOAuthRegistrationTests, InMemorySecretsSeedTests all green
against InMemory, Scylla, and Cassandra fixtures.
Co-authored-by: Cursor <cursoragent@cursor.com>
Azyyyyyy
marked this pull request as ready for review
July 20, 2026 06:55
Azyyyyyy
commented
Jul 20, 2026
Owner
Author
There was a problem hiding this comment.
This file seems to have a lot of functions that could be folded together with code changes
Addresses the low-risk items from the PR #12 review canvas: - #16 delete csharp/Interfold.Domain/refactor.ps1 (obsolete helper, no callers). - OctoconDev#23 swap Array.Empty<AlterPublicFieldReadModel>() for [] in ScyllaSharedQueries.ResolveAlterFields. - #20 shrink the six-line security_level rationale on ScyllaAlterRepository.CreateAsync to a single line pointing at ScyllaAlterRepositoryUdtNullTests. - #25 same treatment for the seven-line duplicate on ScyllaTagRepository.CreateAsync — points at the shared invariant. - #26 / #27 trim the two INTENTIONAL-fallback comments in ScyllaUserRegistryRegionContext.LookupAsync and HandleForLookup to two lines each. No behavioural change. Build clean (0 errors, 102 pre-existing warnings). Co-authored-by: Cursor <cursoragent@cursor.com>
- #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. - OctoconDev#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>
…isers) - #7 / #8 / #9 Convert UpdateAlterCommand from a positional record to a body-only `{ get; init; }` record with `required` on AlterId and UpdatedAt. Everything else is either naturally-null or defaults to false, so callers no longer have to spell out 13-14 nulls positionally just to change one field. - Update every call site to the property-initialiser form: - AltersController.Update (PATCH) — the "meaningful args" path. - AltersController.UploadAvatarMultipart / UploadAvatarByUrl / DeleteAvatar — the three review-flagged sites where a 15-arg positional call with 12 nulls was the specific complaint. - SimplyPluralImportService (two call sites — the initial import upsert and the avatar-rehost post-download update). - InMemoryAlterRepositoryTests (three sites) and the new AlterCommandFlowExecutionTests helper. Declaration order of the properties is preserved, so JSON serialisation of the payload — and therefore idempotency-key hashing — is byte-for- byte identical. Build clean (0 errors). Alter tests all pass (9/9). Co-authored-by: Cursor <cursoragent@cursor.com>
- #11 Add IAccountRepository.GetPublicSystemAsync returning PublicSystemReadModel? directly. Callers of the public wire view (currently just PublicSystemsController.Show) no longer have to fetch the wider AccountPublicProfileReadModel and hand-project 5 of its 8 fields at the controller layer — the repo returns exactly the fields the wire response carries. - InMemory adapter mirrors the existing GetPublicProfileAsync existence rule (a "present" account is one where any identity-bearing field or provider link is set) so the two projections agree — if GetPublicProfileAsync returns non-null, GetPublicSystemAsync must too, otherwise SystemMustExistAttribute would let a request through that Show then 404s. - Scylla adapter uses a narrower SELECT (no discord/email/apple columns) since the public wire projection intentionally drops them. - PublicSystemsController.Show shrinks from a 13-line project-and-null- check to a 5-line qualify-and-return. AccountPublicProfileReadModel and GetPublicProfileAsync are retained for other callers that legitimately need the identity-link surface (WebSocket init, FCM push, SettingsController's avatar helpers). Build clean (0 errors). Account tests pass (3/3). Co-authored-by: Cursor <cursoragent@cursor.com>
Collapses six overlapping surfaces on InterfoldControllerBase (and the matching controller call sites) that the PR review picked apart: - #10 BuildEnvelope null-safe. Introduces a private TryGetPrincipalId that returns nullable, and a private-static AnonymousPrincipalId ("nam:auth") sentinel. BuildEnvelope now stamps the sentinel when the request is anonymous instead of throwing — the OAuth handlers behind AuthController.Callback and AuthLinkController.Callback resolve the real system id off the payload, so command.PrincipalId is never read. Both callback controllers drop their hand-rolled CommandEnvelope constructions and call BuildEnvelope directly. - OctoconDev#2 Combine the two "no bytes uploaded" avatar helpers. Extract the shared read-current / update / cleanup-old-local tail into a private RunAvatarMetadataChangeAsync. HandleAvatarUrlUploadAsync becomes URL validation + delegation; HandleAvatarDeleteAsync is a one-line delegate. - #3 Collapse the three DispatchCreatedAsync overloads. The sync (Func<TResult, TData>) overload had no callers — deleted. The two async overloads merge into one with an optional locationSelector. Every DispatchCreatedAsync call site is rewrapped to the new shape. - #5 Delete the replaySelector plumbing. TResult already carries Replay via ICommandResult, and every existing selector was literally `res => res?.Replay`. Add `where T : ICommandResult` to CommandCreated / CommandCreatedAsync and read result.Result!.Replay directly; drop the Func<T?, bool?>? replaySelector param from every signature and every call site. - #12 Extract generic DispatchOkAsync<TPayload, TResult, TWire>. Wraps the accepted / conflict split around a wire-projection lambda so SettingsController.SetupEncryption / RecoverEncryption stop needing a private per-controller helper. The removed DispatchEncryptionKeyAsync becomes a two-line ToEncryptionKeyResponse projection. - #4 (general "too many functions") is addressed by the above five: 3 fewer public/protected methods on the base, and two fewer replay- selector parameters everywhere. Build clean (0 errors). Api.UnitTests: 380/380 pass. Co-authored-by: Cursor <cursoragent@cursor.com>
…andler (#17) Finishes the migration flagged by review comment #17: every settings-shaped handler now inherits IdempotentCommandHandler<TCmd, SettingsCommandResult> and delegates the mutate → check → publish → success body to the existing SettingsIdempotentCommandFlow.ExecuteMutationAsync helper. SettingsCommandHelper had been documented as the "old path for handlers that don't inherit from IdempotentCommandHandler" — this batch removes the last such handlers, so the type is deleted rather than left as a discovery hazard. Handlers converted (11 total): - Avatar: UploadAvatarCommandHandler, DeleteAvatarCommandHandler - Identity: UnlinkAppleCommandHandler, UnlinkDiscordCommandHandler, UnlinkEmailCommandHandler - Fields: DeleteFieldCommandHandler, RelocateFieldCommandHandler, UpdateFieldCommandHandler - Wipes: WipeTagsCommandHandler, WipeAltersCommandHandler - Delete: DeleteAccountCommandHandler Uniform conversion pattern per handler: - Base: ICommandHandler<TCmd, SettingsCommandResult> → IdempotentCommandHandler<TCmd, SettingsCommandResult> - Constructor forwards IIdempotencyStore to `: base(idempotencyStore)` and drops the private `_idempotencyStore` field. - HandleAsync → ExecuteCoreAsync + `DuplicateEntityRef` override. - Body: SettingsCommandHelper.ExecuteAndPublishAsync[/FieldsChangedAsync] → SettingsIdempotentCommandFlow.ExecuteMutationAsync The field-changed variant folds into the general helper by passing the `SettingsFieldsChangedEvent` publisher lambda inline at the call site. - UploadAvatar keeps its RejectIfBlank guard on Payload.AvatarUrl (now via the base class's protected RejectIfBlank helper, not the static one on CommandHandler). - DeleteAccount keeps its shared `unfriendedIds` list between the mutation and publish lambdas — IdempotentCommandHandler skips ExecuteCoreAsync on replay, which is the same guarantee the previous Result.Replay: false guard on the old helper gave, so the FriendshipRemovedEvent fan-out still only runs on the first-time accept. SettingsCommandHelper.cs deleted. XmlDoc <see cref> references to it in SettingsFieldCommandFlow, SettingsIdempotentCommandFlow, and SettingsEventBusExtensions are re-pointed at the surviving flow helpers so the docs compile clean. Plain-text mentions in ImportSpCommandHandler, EntityRefs.SettingsActionFailed, and the InMemoryAccountRepository regression test's Because() string are rewritten to describe the flow generically. Build clean (0 errors, warnings unchanged). Api.UnitTests: 380/380 pass.
…merge (#6, #14) Two review threads folded together because they touch the same axis (shared shapes across wrapper structs / avatar-carrying read models). #14 — Fold SecretStringJsonConverter<T> into StringBackedJsonConverter<T>. The two abstract JSON converters had byte-for-byte identical bodies (Read via reader.GetString() ?? string.Empty, Write via writer.WriteStringValue). The only "secret" behaviour is on each struct's own ToString() override via SecretRedaction.Redact — the converter itself always emits the raw string at the wire boundary, in both families. Delete SecretStringJsonConverter<T> and re-point the six concrete converters (LinkToken / PushToken / ImportToken / RecoveryCode / Jti / SocketToken) at StringBackedJsonConverter<T>. SecretRedaction stays for the six struct-level ToString() overrides that still redact. StringBackedJsonConverter<T>'s xml-doc is expanded to list all 17 wrapper structs (11 public + 6 secret) it now covers and to note that the "secret" spelling lives on struct ToString(), not the converter. The Guid-backed sibling (GuidIdJsonConverter<T>) stays separate — its wire body is a Guid, not a string. #6 — Introduce IAvatarBearing so QualifyAvatar callsites stop threading the (AvatarUrl?, AvatarSource?) pair by hand. New interface in Interfold.Contracts.Models with AvatarUrl? / AvatarSource? getters. Implemented by six read-shape carriers: BareAlter (and AlterReadModel by inheritance), FriendProfileReadModel, FriendFrontingAlterReadModel, PublicSystemReadModel, AccountPublicProfileReadModel, SocketSelfReadModel. Positional records satisfy the interface's { get; } declarations via their generated { get; init; } properties — no per-record body needed. AvatarUrlQualifier gains two new overloads: QualifyAvatar(IAvatarBearing?, string scheme, HostString host) QualifyAvatar(IAvatarBearing?, string? origin) Both accept a null bearing and pass it through as null so the `profile?...` pattern at socket boundaries stays a one-liner. The three existing (AvatarUrl?, AvatarSource?, ...) overloads stay for the couple of callers that hold the pair without a wrapping bearing (e.g. WebSocketInitialization's SocketSelfReadModel construction, where the "bearing" is being built rather than consumed). Callsites simplified: AltersController.Show, PublicSystemsController.Show / ListAlters / ShowAlter / Batch, InterfoldControllerBase.QualifyAvatar (base helper), AlterSocketEventHandlers.HandleUpsertAsync, FriendshipSocketEventHandlers, WebSocketInitialization's own per-alter qualification and the BuildSelfReadModel `profile?` call. Every `QualifyAvatar(x.AvatarUrl, x.AvatarSource, ...)` becomes `QualifyAvatar(x, ...)`. Build clean (0 errors). Api.UnitTests: 380/380 pass.
Azyyyyyy
commented
Jul 21, 2026
SummarySummary
Coverageinterfold-bootstrap - 64.4%
Interfold.Api - 65.5%
Interfold.AppHost - 44.9%
Interfold.Contracts - 78.8%
Interfold.DatabaseBootstrap - 88.9%
Interfold.Domain - 74.3%
Interfold.Infrastructure - 74.8%
Interfold.Infrastructure.InMemory - 77.3%
Interfold.Infrastructure.Postgres - 73.2%
Interfold.Infrastructure.Scylla - 70.7%
Interfold.ServiceDefaults - 93.4%
|
…k rename Two small cleanups that sit on top of the review-comment batches: - Base/InterfoldControllerBase.HandleAvatarUploadAsync. Post-save tail (read-current → update-metadata → best-effort cleanup of the old Local bytes) was a byte-for-byte duplicate of RunAvatarMetadataChangeAsync, the helper Batch 5 extracted for the URL / delete flows. Route the multipart flow through the same helper via the same `c => updateMetadataAsync(avatarUrl, c)` closure trick HandleAvatarUrlUploadAsync uses. Drops ~15 lines and consolidates the cleanup rule so a future change to it (e.g. also purging External URLs, retry policy on the delete probe) only has to be made in one spot. RunAvatarMetadataChangeAsync's xml-doc is re-summarised to note it now covers all three avatar mutation flows (upload / URL / delete), not just the "no bytes uploaded" pair Batch 5 originally called out. - AuthLinkController: rename RedirectWithSocketEventAsync → RedirectToClient. The old name misrepresented the method on two axes: (1) it's not async (returns IActionResult, no Task) and (2) it doesn't fire a socket event — it validates the caller-supplied redirect_uri and either Redirects or returns BadRequest(MissingRedirectUri). RedirectToClient reflects what the method actually does and matches the terse verb-noun style of Begin / Callback in the same controller. Updates the one call site and the prose reference in Begin's fail-fast comment. Build clean (0 errors). Api.UnitTests: 380/380 pass on top of 3d0377a (tuple→bearing follow-up commit).
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
1 participant
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.
No description provided.