Skip to content

Bug: Tests seed supposedly-valid persisted entities through raw/fake DALs, bypassing schema validation (encodes unstorable fixtures) #1590

Closed
Closed
Bug: Tests seed supposedly-valid persisted entities through raw/fake DALs, bypassing schema validation (encodes unstorable fixtures)#1590
Assignees
JoshuaRowePhantom

Description

@JoshuaRowePhantom
JoshuaRowePhantom
opened on Sep 17, 2026

Summary

Many tests seed entity documents that are presented as valid persisted state, but they write those documents through fakes or a raw store, bypassing the production schema-validating pipeline. Because the fixtures are never validated, tests can (and do) encode persistence contracts that production could never store, giving false confidence.

Production composes writes as MergeProcessingDataAccessLayer → ReferentialIntegrityDataAccessLayer (which extends SchemaValidatingDataAccessLayer) → underlying store (InMemoryDataAccessLayer), built by ValidatedDataAccessLayerFactory.Create. The composed schema enforces unevaluatedProperties = false (see the $comment in JsonSchemas/entity.json), so any property not declared by an entity's composed schema is rejected by production writes.

Proven failure (motivating case). Phantom.Workspaces.Transport.Tests/UserComputerProfileTransportFactoryTests.cs seeds a user-computer-profile document containing a connection-descriptor property through a private EntityLookupDataAccessLayer fake. The production JsonSchemas/user-computer-profile.json schema requires entity-types, computer-reference, and user-reference, and does not declare connection-descriptor; under unevaluatedProperties = false a real write of that document is rejected. The test omits the required computer-reference/user-reference entirely and never runs the validator, so it passes even though production cannot persist the document. The cross-instance scenario (Scenarios/AgentStackCrossInstanceTests.cs) then relies on the same impossible shape to drive routing.

We need a canonical, schema-aware seeding helper used by every test that stands up valid persisted runtime state, an explicit convention for the intentionally-raw exceptions, and an automated guard so new bypasses cannot silently reappear.

Root Cause

There is no shared, schema-aware test seeding primitive. Each test project has independently grown its own way to stand up "persisted" entities:

  • Hand-written IDataAccessLayer fakes whose GetAsync/QueryAsync return EntitySnapshot/QueryEntitySnapshot built from raw JSON (never validated).
  • Direct construction of new InMemoryDataAccessLayer() followed by UpdateAsync of entity documents without wrapping in SchemaValidatingDataAccessLayer/ReferentialIntegrityDataAccessLayer.
  • Scenario harnesses that call SchemaPopulator.Populate() into a raw InMemoryDataAccessLayer and then append additional entity documents by writing directly to that raw store. Populating the built-in schema corpus does not make later writes validated — the appended profile documents still bypass the validator.

Because SchemaValidatingDataAccessLayer.UpdateAsync is the only place composed-schema evaluation happens, any seed that does not pass through it is unverified. SchemaPopulator writes with SchemaValidationPassKind.TrustedEmbeddedSeed on purpose (trusted embedded corpus); tests then reuse the raw store for untrusted runtime entities that should have been validated.

Affected Tests (inventory)

Repo-relative paths. Grouped by mechanism and by shared helper. "By-method" lists are exhaustive where a helper fans out.

Category A — must become schema-aware (entity presented as valid persisted state; behavior-under-test relies on schema validity)

High priority — proven to encode an impossible/forbidden shape (connection-descriptor on user-computer-profile):

# File Class Mechanism Seeded entity types Tests
A1 Phantom.Workspaces.Transport.Tests/UserComputerProfileTransportFactoryTests.cs UserComputerProfileTransportFactoryTests private fake EntityLookupDataAccessLayer : IDataAccessLayer returning EntitySnapshot from raw JSON user-computer-profile (with forbidden connection-descriptor; missing required computer-reference/user-reference) UserComputerProfileTransportFactory_LocalEntity_RoutesToLocalTransport, UserComputerProfileTransportFactory_RemoteEntity_RoutesViaDescriptor, UserComputerProfileTransportFactory_TargetDescriptor_ForwardsThroughRemoteTransport (the 2 no-seed tests _NonProfileDescriptor_ReturnsNull, _DescriptorMissingEntityId_ThrowsTransportException are unaffected)
A2 Phantom.Workspaces.Transport.Tests/Scenarios/AgentStackCrossInstanceTests.cs AgentStackCrossInstanceTests SchemaPopulator.Populate() into raw InMemoryDataAccessLayer, then dataAccessLayer.UpdateAsync(...) of profile docs directly on the raw store (SeedTwoInstanceProfilesAsync / BuildProfileEntity) two user-computer-profile docs, one carrying connection-descriptor every [Theory]/[Fact] cell that calls SeedTwoInstanceProfilesAsync (forward-HTTP and reverse-HTTP profile-routing scenarios)
A3 Phantom.Workspaces.Tests/WorkspacesTransportCompositionTests.cs WorkspacesTransportCompositionTests private fake EntityLookupDataAccessLayer : IDataAccessLayer (CreateComposition) user-computer-profile lookup doc all composition tests that resolve the local profile (e.g. Composition_WithRunningChats_AuthenticatesAndDispatchesAgentSessionLocally, Composition_WithHubFactories_ExposesThemToTransportHost)

Fake IDataAccessLayer returning snapshots from raw JSON:

# File Class / fake Seeded entity types Affected tests
A4 Phantom.Workspaces.Tests/AutoResumeServiceTests.cs FakeQueryDataAccessLayer via CreateAgentSessionSnapshot* agent-session FindMatchingSessionsAsync_WithMatchingLocalExecutor_ReturnsSession, _WithCustomResumePrompt_UsesCustomPrompt, _WithNonMatchingExecutor_ReturnsEmpty, _WithNoAutoResume_ReturnsEmpty, _WithMultipleSessions_ReturnsOnlyMatching
A5 Phantom.Workspaces.Tests/UsageMetricsServiceTests.cs FakeDataAccessLayer, MutableFakeDataAccessLayer via CreateUserAccountEntity user-account all seeding tests, incl. UsageMetricsService_AccountAdded_WhenProviderReturnsMetrics, _AccountNotAdded_WhenProviderReturnsEmptyMetrics, _AccountRemoved_WhenProviderSubsequentlyReturnsEmptyMetrics, _AccountRetained_WhenProviderThrowsOnRefresh, _IgnoresAccounts_WithNoMatchingProvider, _GitHubAccount_RoutesToBothCopilotAndActionsProviders, _AccountCreatedAfterStartup_IsDiscoveredOnNextPoll, _CachedPreviousPeriodUsage_InvalidatedOnPeriodChange (and the remaining timer/lifecycle tests that build the fake)
A6 Phantom.Workspaces.Tests/AgentDefinitionResolverTests.cs FakeDataAccessLayer agent-definition / agent-manifest all resolver tests seeding definition/manifest docs
A7 Phantom.Workspaces.Tests/AgentDefinitionToolExtractorTests.cs raw Parse(...) manifest docs (no DAL) agent-definition manifest tool-extraction tests parsing a manifest entity (see also Category C note)
A8 Phantom.Workspaces.Tests/EntityBrokerTests.cs CallbackDataAccessLayer + CreateSnapshot/SeedSnapshotAsync task, user, actionable, relationship, agent-session CreateSubscriptionAsync_LoadsBindableEntities, RefreshAsync_UpdatesBindableEntityObject, RefreshAsync_SkipsCollectedSubscriptions, UpdateAsync_UpdatesSubscribedEntityWithoutRefreshAsync, SubscribeGetAsync_LoadsInitialResults, RefreshAsync_SubscribedGetRerunsGetAndReplacesResultCollection, RefreshAsync_SubscribedGet_UsesIncrementalCollectionNotifications, RefreshAsync_SubscribedGet_DoesNotClearAndRecreateCollection_WhenMembershipUnchanged, SubscribeQueryAsync_ReturnsActionableInterestTargetsForUser, SubscribeQueryAsync_RefreshesAutomaticallyWhenMatchingEntityAdded, SubscribeQueryAsync_RefreshesAutomaticallyWhenMatchingEntityDeleted

Raw new InMemoryDataAccessLayer() + direct UpdateAsync of runtime entities (no validating wrapper):

# File Class Helper Seeded entity types Affected tests
A9 Phantom.Workspaces.Tests/ScheduledToolsRunningViewModelTests.cs ScheduledToolsRunningViewModelTests (RecordingDataAccessLayer + raw JSON seeds) inline $$"""...""" docs user, computer, user-computer-profile, tool all view-model tests that seed the identity/tool entities
A10 Phantom.Workspaces.Llm.Core.Tests/CurrentSessionContextFactoryTests.cs CurrentSessionContextFactoryTests SeedEntityAsync user, computer, user-computer-profile CreateForHostAsync_ResolvesUserComputerProfileFromDataAccessLayer
A11 Phantom.Workspaces.Llm.Core.Tests/CurrentSessionContextProviderTests.cs CurrentSessionContextProviderTests SeedAgentSessionAsync, SeedEntityAsync, SeedProfileWithComputerReferenceAsync agent-session, user-computer-profile, user, computer, agent-definition all 18 tests calling those helpers: GetCurrentSession_HappyPath_ReturnsAllFourMembers, _ResumeOnDifferentProfile_ReportsHostProfileNotSessionStored, _IgnoresStaleHostProfileEntityId, _NullProfileAndUser_MembersAreNull, _NoDefinitionReference_DefinitionNull, _UnknownSessionId_AgentSessionNull, _IncludeProfileFalse_OmitsProfileAndUser, _IncludeDefinitionFalse_OmitsDefinition, CreateCurrentSessionToolsetFactory_WhenKindMatches_ReturnsCurrentSessionProvider, _WhenKindDoesNotMatch_DefersToUnderlying, _UsesContextCapturedInClosure, GetCurrentSession_HappyPath_ReturnsUserComputerAndProfile, _ReturnedEntities_MatchCurrentIdentity, _ContextWithoutComputer_ResolvesComputerFromProfileReference, _ToolName_IsGetCurrentSession, CurrentSessionToolset_CombinedChain_ExposesGetCurrentSessionTool, GetCurrentSession_CopilotSessionWithResolvedHost_ReturnsPopulatedProfileAndUser, _UnresolvedHost_ReturnsExplicitNullMembersNotEmptyObject
A12 Phantom.Workspaces.Llm.Core.Tests/WorkspaceEntityToolsetFactoryTests.cs WorkspaceEntityToolsetFactoryTests StoreGlobalInstructionNoteAsync, inline seeds note, assigned-to/relationship CreateToolsetAsync_WhenKindMatches_ReturnsWorkspaceEntityTools, WorkspacesEntityUpdate_RelationshipWithReasonNote_Succeeds, ProvideAIContextAsync_LoadsInstructionsFromGlobalNote (the _RelationshipWithoutReasonNote_IsRejectedAndNotWritten test is Category B)
A13 Phantom.Workspaces.Llm.Core.Tests/SubAgentDispatcherPersistenceTests.cs RecordingDataAccessLayer (SeedChild) agent-session (child) RestoreSubAgentsAsync_RebuildsSubAgentsFromChildEntities, PersistSubAgent_OnCreate_WritesChildEntityWithParentReference
A14 Phantom.Workspaces.Tools.Tests/DiscoveryToolsTests.cs DiscoveryToolsTests UpsertEntityAsync discovery/workspace fixtures all discovery-tool tests calling UpsertEntityAsync
A15 Phantom.Workspaces.Tools.Tests/GitHubWorkItemDiscoveryToolTests.cs GitHubWorkItemDiscoveryToolTests CreateDummyEntity, CreateGitRepositoryEntity, UpsertEntityAsync git-repository, work-item fixtures all repo/work-item discovery tests
A16 Phantom.Workspaces.Tools.Tests/GitWorkspaceScanToolTests.cs GitWorkspaceScanToolTests UpsertEntityAsync workspace, computer, user-computer-profile, tool, worktree, filesystem-path all scan/routing tests
A17 Phantom.Workspaces.Tools.Tests/GitWorkspaceUpdateToolTests.cs GitWorkspaceUpdateToolTests CreateSnapshot, UpsertEntityAsync workspace/tool/worktree all update-flow tests
A18 Phantom.Workspaces.Tools.Tests/WorkspaceToolExecutionContextTests.cs WorkspaceToolExecutionContextTests UpsertEntityAsync execution-context workspace entities context-setup tests
A19 Phantom.Workspaces.Tools.Tests/AzureDevOpsPullRequestDiscoveryToolTests.cs, AzureDevOpsRepositoryDiscoveryToolTests.cs, AzureDevOpsWorkItemDiscoveryToolTests.cs respective classes mocks returning new EntitySnapshot { ... } azure-devops-project, external all discovery tests returning seeded snapshots

Category B — intentionally raw / below-validation-boundary (do NOT convert; document as explicit exceptions)

These legitimately exercise the DAL/validator/serializer/query engine themselves, or deliberately test malformed input. They should be marked with a naming/comment/fixture convention (see Design) so future audits skip them.

  • Phantom.Workspaces.Data.Core.Tests: SchemaValidatingDataAccessLayerTests, ReferentialIntegrityDataAccessLayerTests, MergeProcessingDataAccessLayerTests, SchemaPopulatorTests, SchemaAccessorTests, EntitySchemaComposerTests, IDataAccessLayerSerializationTests, BaseUpdateProcessingDataAccessLayerTests, PerInvocationDataAccessLayerTests, ScheduleDataAccessLayerTests, WorkspaceEntitySessionDataAccessLayerTests, DataAccessLayerNonQueryWithoutHistoryTests, and every *EntitySchemaTests / *DocumentationTests (validator/schema authoring suites).
  • Phantom.Workspaces.Data.Offline.Tests: InMemoryDataAccessLayerTests, InMemoryDataAccessLayerQueryContractTests, InMemoryDataAccessLayerQueueEmbeddingsContractTests, InMemoryQueryEvaluatorTests, InMemoryParticipationQueryTests (AddAsync helper), SessionsViewParentFilterTests (AddAsync helper), InMemoryDeletionClearsEmbeddingsTests, OfflineDataAccessLayerTimeProviderTests.
  • Phantom.Workspaces.Data.MongoDB.Tests: MongoDbNativeDataRoundTripTests (SeedRawAsync), MongoDbGetFilterBuilderTests, MongoDbQueryFilterBuilderTests.
  • Phantom.Workspaces.Llm.Core.Tests/WorkspaceEntityToolsetFactoryTests.WorkspacesEntityUpdate_RelationshipWithoutReasonNote_IsRejectedAndNotWritten — malformed-input rejection test.
  • Phantom.Workspaces.Tests/ReconnectingWebDataAccessLayerTests.FakeLayer — connection-health probe fake for reconnection logic; seeds no persisted runtime entity.

Category C — ambiguous (verify before converting)

  • Phantom.Workspaces.Llm.Core.Tests/GitHubAccountUpsertServiceTests.cs (MakeSnapshot, FakeDataAccessLayer, ThrowingDataAccessLayer): verify whether each test asserts persisted-schema validity of a user-account/GitHub-account entity (→ A) or only service dedupe/naming/exception behavior (→ may stay B). UpsertForTokenAsync_ExistingEntityWithCorrectData_DoesNotUpsert, _EntityNamed_FollowsSchemaConvention.
  • Phantom.Workspaces.Llm.Core.Tests/SubAgentDispatcherChatClientTests.cs (FakeDataAccessLayer): verify whether the returned agent-session snapshot is load-bearing persisted state (→ A) or an inert stub for the chat-client transport path (→ B).
  • Phantom.Workspaces.Tests/AgentDefinitionToolExtractorTests.cs (A7 above): confirm whether the manifest is treated as persisted state that must be schema-valid (→ A) or as parser input for extraction logic (→ B).

Counts (files/groups): Category A ≈ 19 files/groups (100+ individual test methods once helpers are expanded); Category B ≈ 20+ classes across 3 data-layer projects plus 2 targeted exceptions; Category C = 3 groups.

Affected Production / Test Infrastructure Files

File Role Relevance
Phantom.Workspaces.Data.Core/SchemaValidatingDataAccessLayer.cs Composed-schema evaluation on UpdateAsync The only layer that enforces unevaluatedProperties=false; the layer all Category-A seeds bypass
Phantom.Workspaces.Data.Core/ReferentialIntegrityDataAccessLayer.cs Extends SchemaValidatingDataAccessLayer, adds referential checks Production write layer that valid seeds must go through
Phantom.Workspaces.Data.Core/ValidatedDataAccessLayerFactory.cs Builds MergeProcessing → ReferentialIntegrity → store Canonical composition the helper should reuse
Phantom.Workspaces.Data.Core/SchemaPopulator.cs Seeds trusted embedded schema corpus (TrustedEmbeddedSeed) Needed to register schemas before validating runtime seeds; not a substitute for validation of runtime writes
Phantom.Workspaces.Data.Offline/InMemoryDataAccessLayer.cs Raw store Must be wrapped, not written directly, for valid seeds
Phantom.Workspaces.Data.Core/JsonSchemas/entity.json Base schema; documents unevaluatedProperties=false at composed level Establishes why undeclared props are rejected
Phantom.Workspaces.Data.Core/JsonSchemas/user-computer-profile.json Requires entity-types,computer-reference,user-reference; no connection-descriptor The contract the motivating tests violate
Phantom.Workspaces.Transport.Tests/UserComputerProfileTransportFactoryTests.cs Motivating fake seed A1
Phantom.Workspaces.Transport.Tests/Scenarios/AgentStackCrossInstanceTests.cs Populate-then-raw-write harness A2
Phantom.Workspaces.Transport.Tests/Infrastructure/HubRelayHarness.cs Scenario harness (transport-only; no entity seeding) Reference for where a shared seed helper would plug in

Design / Fix

  1. Add a canonical schema-aware seeding fixture/helper (e.g. ValidatingEntitySeedFixture / SeededDataAccessLayer) in a shared test support library referenced by all test projects (e.g. Phantom.Workspaces.Testing). It must:
    • Build a real store (InMemoryDataAccessLayer), run SchemaPopulator.Populate() to register the trusted embedded schema corpus, then expose a validating front (ValidatedDataAccessLayerFactory.Create(store)), so all subsequent seeds go through ReferentialIntegrityDataAccessLayer/SchemaValidatingDataAccessLayer.
    • Expose explicit APIs:
      • Task<EntityId> SeedValidEntityAsync(...) — writes a runtime entity through the validating pipeline and fails the test if validation reports any error (surfacing UpdateErrors).
      • Task SeedManyValidAsync(...) / relationship-aware overloads — batch seeding with referential links so multi-entity fixtures (profiles + computers + users, sessions + children) validate together.
      • Task SeedRawEntityForMalformedInputTestAsync(...) — the only sanctioned way to write below the validator, for Category-B malformed-input tests; named so intent is unmistakable.
    • Require complete entities: callers must provide entity-id, entity-types (including base entity), and all required composed fields. The helper must not silently inject or relax schema to keep an impossible fixture alive.
  2. Convert Category-A tests to use SeedValidEntityAsync. This will force the motivating fixtures to become storable: user-computer-profile seeds must include computer-reference and user-reference, and must not include connection-descriptor unless/until the production schema is deliberately extended (a separate decision, out of scope here — do not add a schema allowance merely to preserve the current tests).
  3. Document Category-B exceptions with a naming + comment + fixture convention (e.g. tests using SeedRawEntityForMalformedInputTestAsync, or classes annotated // below-validation-boundary: <reason>), so audits and the guard can distinguish them.
  4. Account for Bug: Eager in-memory repository startup validates trusted embedded seed twice #1576 (CLOSED). Bug: Eager in-memory repository startup validates trusted embedded seed twice #1576 introduced a narrowly scoped trusted-embedded-seed path (SchemaValidationPassKind.TrustedEmbeddedSeed) that retains merge/referential processing while bypassing schema composition for the built-in corpus. Runtime-entity tests must still validate through the normal pass; only tests specifically exercising the trusted embedded seed may intentionally run below the validator, and must be tagged Category B.
  5. Forbid the populate-then-raw-write anti-pattern. In AgentStackCrossInstanceTests, populate through the store but perform all subsequent profile writes through the validating front, not the raw InMemoryDataAccessLayer.

Automated Guard Strategy

Prefer a code-level guard over grep-only enforcement. Evaluate and implement in this order:

  1. Central helper adoption (primary): route all valid seeds through the shared fixture; make the fixture the path of least resistance.
  2. Architecture/analyzer test (enforcement): an xUnit "architecture" test (or Roslyn analyzer) that scans test assemblies and fails when a test type declares its own IDataAccessLayer implementation that returns EntitySnapshot/QueryEntitySnapshot, or constructs new InMemoryDataAccessLayer() and calls UpdateAsync with an entity change, unless the type is on an explicit allow-list (the documented Category-B classes) or uses the sanctioned raw helper. Allow-list membership is the auditable record of exceptions.
  3. Canonical-fixture schema test: a test that enumerates a set of canonical fixture documents (one per commonly-seeded entity type: user-computer-profile, agent-session, user, computer, tool, agent-definition, user-account, relationship) and asserts each validates through ValidatedDataAccessLayerFactory, so the fixtures the helper hands out are provably storable.
  4. Helper naming convention (SeedValidEntityAsync vs SeedRawEntityForMalformedInputTestAsync) as the human-readable signal backing the analyzer allow-list.

Expected Tests

Following Subject_Scenario_ExpectedOutcome:

Subject Scenario Expected Outcome
ValidatingEntitySeedFixture_SeedValidEntity_PersistsThroughValidatingPipeline seed a complete user-computer-profile (with computer-reference+user-reference) write succeeds; entity is retrievable; no UpdateError
ValidatingEntitySeedFixture_SeedProfileWithConnectionDescriptor_IsRejected seed a user-computer-profile carrying connection-descriptor (the motivating shape) seed fails with a schema validation error (unevaluatedProperties), proving the old fixture was impossible
ValidatingEntitySeedFixture_SeedProfileMissingRequiredReferences_IsRejected seed a user-computer-profile without computer-reference/user-reference seed fails with a required-property validation error
ValidatingEntitySeedFixture_SeedRawForMalformedInputTest_BypassesValidator use SeedRawEntityForMalformedInputTestAsync with a deliberately invalid doc document is written to the raw store unvalidated (Category-B path works)
ValidatingEntitySeedFixture_SeedManyValid_WithRelationships_AllValidate batch-seed user+computer+profile+agent-session with references all entities validate and referential links resolve
CanonicalFixtures_AllSeededEntityTypes_ValidateThroughProductionPipeline enumerate one canonical doc per seeded entity type each validates via ValidatedDataAccessLayerFactory
SeedingArchitectureGuard_TestDeclaresAdHocDataAccessLayerFake_FailsUnlessAllowListed a test type returns EntitySnapshot from its own IDataAccessLayer and is not allow-listed guard test fails
SeedingArchitectureGuard_TestWritesEntityToRawInMemory_FailsUnlessAllowListed a test constructs raw InMemoryDataAccessLayer and UpdateAsynces an entity, not allow-listed guard test fails
UserComputerProfileTransportFactory_RemoteEntity_RoutesViaDescriptor (converted) profile seeded via SeedValidEntityAsync; routing metadata supplied where production actually carries it routing still works using a storable profile shape
AgentStackCrossInstance_ProfileRouting_UsesValidatedProfiles (converted) both profiles seeded through the validating front, not the raw store scenario passes with storable profiles

Dependencies / Related Issues

Implementation Commit Plan (bounded batches)

  1. Fixture + guard scaffolding: add ValidatingEntitySeedFixture (with SeedValidEntityAsync, SeedManyValidAsync, SeedRawEntityForMalformedInputTestAsync) to shared test support; add the canonical-fixture schema test and the architecture-guard test with an initial allow-list = current Category-B classes. Establishes the contract before mass conversion.
  2. Motivating transport batch: convert A1 (UserComputerProfileTransportFactoryTests), A2 (AgentStackCrossInstanceTests), A3 (WorkspacesTransportCompositionTests) to SeedValidEntityAsync; remove connection-descriptor from seeded profiles (or make the routing input come from where production actually supplies it) and add the required references.
  3. Main-app fakes batch: convert A4–A8 (AutoResumeServiceTests, UsageMetricsServiceTests, AgentDefinitionResolverTests, EntityBrokerTests, and A9 ScheduledToolsRunningViewModelTests); delete per-test fake DALs in favor of the fixture; resolve A7/C for the tool extractor.
  4. Llm.Core batch: convert A10–A13 (CurrentSessionContextFactoryTests, CurrentSessionContextProviderTests, WorkspaceEntityToolsetFactoryTests, SubAgentDispatcherPersistenceTests); resolve C for GitHubAccountUpsertServiceTests, SubAgentDispatcherChatClientTests.
  5. Tools batch: convert A14–A19 (discovery/scan/update/context/Azure DevOps tool tests) to seed through the fixture.
  6. Lock the guard: remove any temporary allow-list entries that were only converted-in-progress; leave only genuine Category-B classes; document the exception convention in test-support README/XML docs.

Each batch keeps the guard green and the canonical-fixture test authoritative, so no batch can reintroduce an unstorable fixture.

Activity

  1. JoshuaRowePhantom
    added
    bugSomething isn't working
    diagnosedRoot cause identified
    on Sep 17, 2026
  2. JoshuaRowePhantom
    self-assigned this
    on Sep 17, 2026

  3. JoshuaRowePhantom commented on Sep 17, 2026

    @JoshuaRowePhantom
    JoshuaRowePhantom
    on Sep 17, 2026
    OwnerAuthor

    Design

    Root cause

    Tests that model valid persisted runtime state currently either return hand-built EntitySnapshot values from ad-hoc IDataAccessLayer fakes or write entity changes directly to raw InMemoryDataAccessLayer instances. Those paths never cross the production composition (MergeProcessingDataAccessLayer → ReferentialIntegrityDataAccessLayer/schema validation), so unstorable fixture shapes can pass.

    Approach

    • Add one shared ValidatingEntitySeedFixture in Phantom.Workspaces.Testing. It owns a raw in-memory store, populates the trusted embedded schema corpus once, exposes the production validating front, and offers only clearly separated SeedValidEntityAsync/SeedManyValidAsync and SeedRawEntityForMalformedInputTestAsync APIs. Valid seeds fail immediately with the real UpdateError details; the raw API remains explicit and narrow for rejection tests.
    • Add test-first contract coverage for valid profile/reference graphs, forbidden profile properties, missing required references, explicit malformed raw seeding, batch relationship resolution, and representative canonical entity types.
    • Convert the issue's Category-A call sites across Transport, main app, Llm.Core, and Tools tests to the shared validating path. Complete fixtures rather than weakening schemas, and move routing metadata to its production owner instead of persisting connection-descriptor on profiles. Preserve only genuine validator/DAL/parser-isolation cases as documented below-boundary exceptions.
    • Add an architecture guard with a small auditable Category-B allow-list plus focused guard self-tests. It will reject new ad-hoc test IDataAccessLayer snapshot fakes and raw in-memory entity writes outside the sanctioned helper/allow-list.
    • Keep Bug: Eager in-memory repository startup validates trusted embedded seed twice #1576's single-owner contract: SchemaPopulator uses the trusted embedded-seed pass; every runtime fixture write uses the normal external validation pass.

    Alternatives considered

    • Duplicating schema checks in test helpers was rejected because it would drift from production.
    • A caller-controlled “skip validation” flag was rejected because it makes bypasses easy and unauditable.
    • Extending user-computer-profile merely to preserve existing fixtures was rejected; the current schema remains authoritative.
    • Broadly rewriting intentionally malformed DAL/schema tests was rejected; those remain explicit Category-B exceptions.

    Expected files

    Phantom.Workspaces.Testing (fixture and project references), fixture/architecture tests in Phantom.Workspaces.Data.Core.Tests, affected Category-A files named in this issue under Phantom.Workspaces.Transport.Tests, Phantom.Workspaces.Tests, Phantom.Workspaces.Llm.Core.Tests, and Phantom.Workspaces.Tools.Tests, plus only the test-project references needed to consume the shared fixture.

  4. JoshuaRowePhantom commented on Sep 18, 2026

    @JoshuaRowePhantom
    JoshuaRowePhantom
    on Sep 18, 2026
    OwnerAuthor

    Implementation notes

    Commits: 508cbc04950c443525aa73e02cd59eb3b6ca58ed

    Deviations from proposed design

    • The validating conversions exposed a production composition defect: inherited $ref schemas did not contribute evaluated properties to the final unevaluatedProperties: false boundary. The authoritative composer now flattens applicable schema roots while retaining missing inheritance requirements; existing schema-validation rejection coverage remains green.
    • task.status now conditionally leaves pull-request/work-item vocabularies to their specialized schemas rather than broadening the generic task enum.
    • Remote profile routing no longer reads forbidden persisted connection-descriptor data. Transient descriptors are accepted at the transport boundary, with production reverse-HTTP hub URLs supplied by WorkspacesTransportComposition as the fallback route.
    • Category-B DAL wrappers/parser-isolation tests remain explicit, auditable architecture-guard exceptions; no validation-bypass flag was added.

    Scope changes

    • Added tightly coupled production corrections for GitHub/Azure DevOps work-item type ancestry (task) because validation proved the emitted documents were otherwise unstorable.
    • Added a shared validated Tools test-context helper to avoid repeating fixture setup.

    Verification

    • Targeted union: 328 passed, 0 failed.
    • Stability: 10/10 consecutive runs passed, 328 tests per run, -NoBuild after the successful targeted build.
    • No crash dumps; worktree clean. No push.

    Files changed

    • Phantom.Workspaces.Testing/{ValidatingEntitySeedFixture.cs,SeedingArchitectureGuard.cs,Phantom.Workspaces.Testing.csproj}
    • Phantom.Workspaces.Data.Core.Tests/{ValidatingEntitySeedFixtureTests.cs,SeedingArchitectureGuardTests.cs}
    • Phantom.Workspaces.Data.Core/{SchemaValidatingDataAccessLayer.cs,JsonSchemas/task.json}
    • Phantom.Workspaces.Transport/{UserComputerProfileTransportFactory.cs}
    • Phantom.Workspaces/Services/WorkspacesTransportComposition.cs
    • Phantom.Workspaces/Tools/{GitHub/GitHubWorkItemDiscoveryTool.cs,AzureDevOps/AzureDevOpsWorkItemDiscoveryTool.cs}
    • Phantom.Workspaces.Transport.Tests/{Phantom.Workspaces.Transport.Tests.csproj,UserComputerProfileTransportFactoryTests.cs,Scenarios/AgentStackCrossInstanceTests.cs}
    • Phantom.Workspaces.Tests/{WorkspacesTransportCompositionTests.cs,AutoResumeServiceTests.cs,UsageMetricsServiceTests.cs,AgentDefinitionResolverTests.cs,AgentDefinitionToolExtractorTests.cs,ScheduledToolsRunningViewModelTests.cs}
    • Phantom.Workspaces.Llm.Core.Tests/{CurrentSessionContextFactoryTests.cs,CurrentSessionContextProviderTests.cs,WorkspaceEntityToolsetFactoryTests.cs,SubAgentDispatcherPersistenceTests.cs,GitHubAccountUpsertServiceTests.cs}
    • Phantom.Workspaces.Tools.Tests/{ValidatedWorkspaceToolTestContext.cs,DiscoveryToolsTests.cs,GitHubWorkItemDiscoveryToolTests.cs,GitWorkspaceScanToolTests.cs,GitWorkspaceUpdateToolTests.cs,WorkspaceToolExecutionContextTests.cs,AzureDevOpsPullRequestDiscoveryToolTests.cs,AzureDevOpsRepositoryDiscoveryToolTests.cs,AzureDevOpsWorkItemDiscoveryToolTests.cs}

  5. JoshuaRowePhantom commented on Sep 18, 2026

    @JoshuaRowePhantom
    JoshuaRowePhantom
    on Sep 18, 2026
    OwnerAuthor

    Integration reconciliation notes

    Integration commit: fb6091c8b2ae06e06d05f46bcdf8a0c63061ab3a

    Parents:

    Reconciliation

    Verification

    Manually reconciled files

    • Phantom.Workspaces.Tools.Tests/GitWorkspaceUpdateToolTests.cs
    • Phantom.Workspaces.Tools.Tests/GitWorkspaceScanToolTests.cs

  6. JoshuaRowePhantom commented on Sep 18, 2026

    @JoshuaRowePhantom
    JoshuaRowePhantom
    on Sep 18, 2026
    OwnerAuthor

    Verification result

    Status: ✅ Verified
    Worktree: fix/1590-validated-test-entity-seeding @ fb6091c8b2ae06e06d05f46bcdf8a0c63061ab3a
    Commits checked:

    fb6091c8 Merge github/features into fix/1590 validation work
508cbc04 Fix #1590: validate persisted test entity seeds

    Merge parents = 508cbc04 (fix) + d5913892 (current features); both are ancestors of the tip. Verification performed independently and read-only (implementation/integration notes were not trusted).

    Root-contract checks (all satisfied)

    1. Single authoritative validation boundary. Phantom.Workspaces.Testing/ValidatingEntitySeedFixture.cs builds a raw InMemoryDataAccessLayer, exposes the production front via ValidatedDataAccessLayerFactory.Create (→ MergeProcessing → ReferentialIntegrity/SchemaValidating), and populates the trusted corpus through that same front — consistent with Bug: Eager in-memory repository startup validates trusted embedded seed twice #1576's single validation-owner (SchemaPopulator uses the trusted-embedded-seed pass; runtime seeds use the normal pass). SeedValidEntityAsync/SeedManyValidAsync surface real UpdateErrors and fail the test on any validation error; they require complete entities (entity-id + base entity type) and never inject/relax schema.
    2. Narrow raw path only. Intentionally-invalid writes are possible solely through the explicitly named SeedRawEntityForMalformedInputTestAsync (writes to the raw store); no general caller-controlled bypass, no production exposure.
    3. Fake/raw DAL behaviour preserved; no schema duplication / silent normalisation / weakened assertions. Converted Category-A files (A1–A19) route valid seeds through the fixture or ValidatedWorkspaceToolTestContext; audited — none re-introduce ad-hoc snapshot fakes or hide expected validation failures.
    4. Motivating fixtures fixed (fail-on-base / pass-on-branch). ValidatingEntitySeedFixture_SeedProfileWithConnectionDescriptor_IsRejected proves the old user-computer-profile shape now fails (unevaluatedProperties); _SeedProfileMissingRequiredReferences_IsRejected proves the missing computer-reference/user-reference case fails (required). A1 (UserComputerProfileTransportFactoryTests), A2 (AgentStackCrossInstanceTests), A3 (WorkspacesTransportCompositionTests) now seed storable profiles (with both references, no connection-descriptor); routing metadata moved to the transient transport boundary + a configured reverse-http hub fallback. The populate-then-raw-write anti-pattern is gone.
    5. Static guard prevents recurrence. SeedingArchitectureGuard + SeedingArchitectureGuard_AllRuntimeEntityTestSeeds_UseValidatedOrDocumentedRawPath scan the 4 test projects and fail on un-allow-listed ad-hoc IDataAccessLayer snapshot fakes or raw InMemoryDataAccessLayer entity writes. The Category-B allow-list is auditable and documented; entries are legitimate (e.g. EntityBrokerTests seeds through the validated EntityRepository DAL — a decorator that trips the heuristic; the remainder seed production-persisted entity types — mcp-server/scheduler/note — outside the issue's A1–A19 conversion scope). Canonical-fixture test validates one representative doc per seeded entity type through the production pipeline.
    6. Production changes are in-scope, correct, and regression-guarded. The conversions exposed a real composition defect: SchemaValidatingDataAccessLayer.BuildComposedSchemaText now flattens applicable schema roots (dropping redundant sibling $refs, resolving relative refs) so inherited properties are evaluated by the composed unevaluatedProperties: false boundary. task.json leaves pull-request/work-item status vocab to specialised schemas; the two work-item tools add task ancestry so emitted docs are storable. All entity-schema/composition/validation suites pass unchanged (no regression).
    7. Merge fidelity. Git workspace tool never records or reconciles filesystem existence for existing entities: deleted worktrees stay visible forever #1586 (all 12 reconciliation methods) and Git workspace scan silently narrows discovery to the wrong subtree when a fixed-drive root is trimmed to a drive-relative path #1589 (NormalizeRepositoryPath_TrailingSeparatorVariants_ProduceSameDeterministicId) are present in the tip; the combined (--cc) diff is limited to the two reconciled test files — every other file merged cleanly, so both sides are preserved with no evil-merge regression. Reconciled files seed through the validated path and do not trip the guard.
    8. No shortcuts. No timing/polling/sleep-based synchronisation, .Result/.Wait()/.GetAwaiter().GetResult(), disabled/skipped tests, or unbacked TODOs were introduced. (The one Task.Delay(50) in ScheduledToolsRunningViewModelTests is pre-existing from ScheduledToolsRunningViewModel: Tools ObservableCollection mutated off UI thread, causing 'Collection was modified' crash in scheduled task viewer #616, untouched by this diff; UsageMetricsServiceTests uses WhenAny(tcs, Task.Delay(timeout)) failsafe with a TaskCompletionSource as the real synchroniser.)

    Tests

    Run via scripts\run-tests.ps1 only.

    • Build: succeeded.
    • Targeted union (full mode): the changed test classes plus the data-core schema/composition/entity-schema regression suites → 491 executed, 491 passed, 0 failed (superset of the ~347 change-footprint union; additionally regression-guards the SchemaValidatingDataAccessLayer/task.json production change with zero failures).
    • Stability: run-tests-stability.ps1 -Mode full -NoBuild over the changed-class footprint → 10/10 consecutive runs passed, 252 tests per run.
    • Crash dumps: none. Worktree: clean. HEAD: unchanged (fb6091c8…). No source edits, commits, pushes, ref/tag moves.
    === Test Run Summary (targeted union) ===
Executed : 491
Passed   : 491
Failed   : 0

    needs-slow-tests: YES — the diff modifies real filesystem/Git tests (GitWorkspaceScanToolTests/GitWorkspaceUpdateToolTests use LibGit2Sharp + temp git repos) and carries a broad schema-composition production change; checkin should run the full suite.

    Outstanding gaps

    None — all criteria satisfied.

  7. JoshuaRowePhantom
    added
    verified-locallyImplementation has been verified locally
    needs-slow-testsRequires full test suite including slow Git tests at checkin
    on Sep 18, 2026

  8. JoshuaRowePhantom commented on Sep 18, 2026

    @JoshuaRowePhantom
    JoshuaRowePhantom
    on Sep 18, 2026
    OwnerAuthor

    Check-in gate diagnosis: unrelated known failures

    The required full/slow gate at fb6091c8b2ae06e06d05f46bcdf8a0c63061ab3a executed 7,731 tests (7,730 passed, 1 failed) and encountered two previously documented, unrelated failures:

    features...HEAD contains no changes to either failing test or its implementation path. Dump analysis reproduced #1591 exactly: testhost awaited ProcessRunner.ReadLinesAsync / RunWindowsContainedAsync; child PowerShell thread 12 was blocked in Kernel32.CreateProcess → Process.StartWithCreateProcess → NativeCommandProcessor.InitNativeProcess. Four collector copies were archived as dumps\1591\13-* through 16-* and removed from the worktree. Existing bugs #1591/#1592 were left untouched as requested.

    The branch-specific build, 425-test schema/data target, 10×252 stability gate, and 7,505-test fast gate all passed with zero failures. These full-gate failures are unrelated under the check-in policy.

  9. JoshuaRowePhantom
    added a commit that references this issue on Sep 18, 2026

    Fix #1590: validate persisted test entity seeds

    508cbc0

  10. JoshuaRowePhantom commented on Sep 18, 2026

    @JoshuaRowePhantom
    JoshuaRowePhantom
    on Sep 18, 2026
    OwnerAuthor

    Checked in

    Commit: fb6091c8b2ae06e06d05f46bcdf8a0c63061ab3a
    Branch fast-forwarded: features → fb6091c8b2ae06e06d05f46bcdf8a0c63061ab3a
    Remote confirmed: github/features = fb6091c8b2ae06e06d05f46bcdf8a0c63061ab3a

    Gates

    Merge ancestry is intact: verified fix 508cbc04 and exact prior github/features d5913892 are the two parents of the pushed merge commit. No rebase/amend/history rewrite was performed.

  11. JoshuaRowePhantom
    removed
    bugSomething isn't working
    verified-locallyImplementation has been verified locally
    diagnosedRoot cause identified
    needs-slow-testsRequires full test suite including slow Git tests at checkin
    on Sep 18, 2026
  12. JoshuaRowePhantom
    closed this as completedon Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions