From fbed383f657e2aad7fc3bab0cdf3f75ed9fb0b79 Mon Sep 17 00:00:00 2001 From: Josh Smith <6895577+joshsmithxrm@users.noreply.github.com> Date: Fri, 2 Jan 2026 01:23:34 -0600 Subject: [PATCH 1/9] feat: add integration testing infrastructure (Phase 1 of #55) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add three new test projects for comprehensive integration testing: - PPDS.Dataverse.IntegrationTests: FakeXrmEasy v3.8.0 for mocked Dataverse tests - PPDS.Auth.IntegrationTests: Authentication flow validation - PPDS.LiveTests: Live Dataverse connection tests with skip logic Infrastructure includes: - FakeXrmEasyTestsBase with middleware builder (RPL-1.5 license) - LiveTestBase with [Trait("Category", "Integration")] for filtering - SkipIfNoCredentials attributes for graceful credential handling - LiveTestConfiguration reading from environment variables CI/CD changes: - integration-tests.yml: Approach B workflow (trusted PRs get integration tests) - test.yml: Updated to exclude integration tests with --filter - dependabot.yml: Added fakexrmeasy group for package updates 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .github/dependabot.yml | 3 + .github/workflows/integration-tests.yml | 71 ++++++++++++ .github/workflows/test.yml | 4 +- PPDS.Sdk.sln | 45 ++++++++ .../AuthenticationSmokeTests.cs | 39 +++++++ .../PPDS.Auth.IntegrationTests.csproj | 30 ++++++ .../FakeXrmEasySmokeTests.cs | 76 +++++++++++++ .../FakeXrmEasyTestsBase.cs | 68 ++++++++++++ .../PPDS.Dataverse.IntegrationTests.csproj | 32 ++++++ .../Infrastructure/LiveTestBase.cs | 88 +++++++++++++++ .../Infrastructure/LiveTestConfiguration.cs | 101 ++++++++++++++++++ .../SkipIfNoCredentialsAttribute.cs | 61 +++++++++++ .../PPDS.LiveTests/LiveDataverseSmokeTests.cs | 59 ++++++++++ tests/PPDS.LiveTests/PPDS.LiveTests.csproj | 31 ++++++ 14 files changed, 706 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/integration-tests.yml create mode 100644 tests/PPDS.Auth.IntegrationTests/AuthenticationSmokeTests.cs create mode 100644 tests/PPDS.Auth.IntegrationTests/PPDS.Auth.IntegrationTests.csproj create mode 100644 tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasySmokeTests.cs create mode 100644 tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs create mode 100644 tests/PPDS.Dataverse.IntegrationTests/PPDS.Dataverse.IntegrationTests.csproj create mode 100644 tests/PPDS.LiveTests/Infrastructure/LiveTestBase.cs create mode 100644 tests/PPDS.LiveTests/Infrastructure/LiveTestConfiguration.cs create mode 100644 tests/PPDS.LiveTests/Infrastructure/SkipIfNoCredentialsAttribute.cs create mode 100644 tests/PPDS.LiveTests/LiveDataverseSmokeTests.cs create mode 100644 tests/PPDS.LiveTests/PPDS.LiveTests.csproj diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 6562f7f21..41160b21c 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -27,6 +27,9 @@ updates: dataverse: patterns: - "Microsoft.PowerPlatform.Dataverse.*" + fakexrmeasy: + patterns: + - "FakeXrmEasy*" # Widen version constraints when needed (e.g., 1.1.* -> 1.2.*) versioning-strategy: increase diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml new file mode 100644 index 000000000..7fe6f435a --- /dev/null +++ b/.github/workflows/integration-tests.yml @@ -0,0 +1,71 @@ +name: Integration Tests + +on: + push: + branches: [main] + pull_request: + branches: [main] + +jobs: + unit-tests: + name: Unit Tests + runs-on: windows-latest + + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 + + - name: Setup .NET + uses: actions/setup-dotnet@v5 + with: + dotnet-version: | + 8.0.x + 9.0.x + 10.0.x + + - name: Restore dependencies + run: dotnet restore + + - name: Build + run: dotnet build --configuration Release --no-restore + + - name: Run Unit Tests (exclude integration) + run: dotnet test --configuration Release --no-build --verbosity normal --filter "Category!=Integration" + + integration-tests: + name: Integration Tests + runs-on: windows-latest + # Only run on PRs from the same repo (not forks) or on main branch + if: github.event_name == 'push' || github.event.pull_request.head.repo.full_name == github.repository + environment: test-dataverse + needs: unit-tests + + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 + + - name: Setup .NET + uses: actions/setup-dotnet@v5 + with: + dotnet-version: | + 8.0.x + 9.0.x + 10.0.x + + - name: Restore dependencies + run: dotnet restore + + - name: Build + run: dotnet build --configuration Release --no-restore + + - name: Run Integration Tests + env: + DATAVERSE_URL: ${{ secrets.DATAVERSE_URL }} + PPDS_TEST_APP_ID: ${{ secrets.PPDS_TEST_APP_ID }} + PPDS_TEST_CLIENT_SECRET: ${{ secrets.PPDS_TEST_CLIENT_SECRET }} + PPDS_TEST_TENANT_ID: ${{ secrets.PPDS_TEST_TENANT_ID }} + PPDS_TEST_CERT_BASE64: ${{ secrets.PPDS_TEST_CERT_BASE64 }} + PPDS_TEST_CERT_PASSWORD: ${{ secrets.PPDS_TEST_CERT_PASSWORD }} + run: dotnet test --configuration Release --no-build --verbosity normal --filter "Category=Integration" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2e0fbeaf2..debb76ecc 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,5 +29,5 @@ jobs: - name: Build run: dotnet build --configuration Release --no-restore - - name: Test - run: dotnet test --configuration Release --no-build --verbosity normal + - name: Test (Unit Tests Only) + run: dotnet test --configuration Release --no-build --verbosity normal --filter "Category!=Integration" diff --git a/PPDS.Sdk.sln b/PPDS.Sdk.sln index 550693ff1..12aae2d83 100644 --- a/PPDS.Sdk.sln +++ b/PPDS.Sdk.sln @@ -23,6 +23,12 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.Migration", "src\PPDS. EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.Auth", "src\PPDS.Auth\PPDS.Auth.csproj", "{5B3662DC-7BC8-4981-8F0F-30FB12CFD3F7}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.Dataverse.IntegrationTests", "tests\PPDS.Dataverse.IntegrationTests\PPDS.Dataverse.IntegrationTests.csproj", "{4CA09022-5929-44B8-9777-CA824EC9A8EE}" +EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.Auth.IntegrationTests", "tests\PPDS.Auth.IntegrationTests\PPDS.Auth.IntegrationTests.csproj", "{9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}" +EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.LiveTests", "tests\PPDS.LiveTests\PPDS.LiveTests.csproj", "{EF193372-AD19-4918-87F2-5B875AC840E1}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -129,6 +135,42 @@ Global {5B3662DC-7BC8-4981-8F0F-30FB12CFD3F7}.Release|x64.Build.0 = Release|Any CPU {5B3662DC-7BC8-4981-8F0F-30FB12CFD3F7}.Release|x86.ActiveCfg = Release|Any CPU {5B3662DC-7BC8-4981-8F0F-30FB12CFD3F7}.Release|x86.Build.0 = Release|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Debug|Any CPU.Build.0 = Debug|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Debug|x64.ActiveCfg = Debug|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Debug|x64.Build.0 = Debug|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Debug|x86.ActiveCfg = Debug|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Debug|x86.Build.0 = Debug|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Release|Any CPU.ActiveCfg = Release|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Release|Any CPU.Build.0 = Release|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Release|x64.ActiveCfg = Release|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Release|x64.Build.0 = Release|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Release|x86.ActiveCfg = Release|Any CPU + {4CA09022-5929-44B8-9777-CA824EC9A8EE}.Release|x86.Build.0 = Release|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Debug|Any CPU.Build.0 = Debug|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Debug|x64.ActiveCfg = Debug|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Debug|x64.Build.0 = Debug|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Debug|x86.ActiveCfg = Debug|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Debug|x86.Build.0 = Debug|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Release|Any CPU.ActiveCfg = Release|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Release|Any CPU.Build.0 = Release|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Release|x64.ActiveCfg = Release|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Release|x64.Build.0 = Release|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Release|x86.ActiveCfg = Release|Any CPU + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876}.Release|x86.Build.0 = Release|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Debug|Any CPU.Build.0 = Debug|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Debug|x64.ActiveCfg = Debug|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Debug|x64.Build.0 = Debug|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Debug|x86.ActiveCfg = Debug|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Debug|x86.Build.0 = Debug|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Release|Any CPU.ActiveCfg = Release|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Release|Any CPU.Build.0 = Release|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Release|x64.ActiveCfg = Release|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Release|x64.Build.0 = Release|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Release|x86.ActiveCfg = Release|Any CPU + {EF193372-AD19-4918-87F2-5B875AC840E1}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -142,5 +184,8 @@ Global {45DB0E17-0355-4342-8218-2FD8FA545157} = {0AB3BF05-4346-4AA6-1389-037BE0695223} {1642C0BD-0B5B-476D-86EB-73BE3CD9BD67} = {827E0CD3-B72D-47B6-A68D-7590B98EB39B} {5B3662DC-7BC8-4981-8F0F-30FB12CFD3F7} = {827E0CD3-B72D-47B6-A68D-7590B98EB39B} + {4CA09022-5929-44B8-9777-CA824EC9A8EE} = {0AB3BF05-4346-4AA6-1389-037BE0695223} + {9C52B513-E62D-49D4-9D5A-3F8C5BAB2876} = {0AB3BF05-4346-4AA6-1389-037BE0695223} + {EF193372-AD19-4918-87F2-5B875AC840E1} = {0AB3BF05-4346-4AA6-1389-037BE0695223} EndGlobalSection EndGlobal diff --git a/tests/PPDS.Auth.IntegrationTests/AuthenticationSmokeTests.cs b/tests/PPDS.Auth.IntegrationTests/AuthenticationSmokeTests.cs new file mode 100644 index 000000000..067b5b494 --- /dev/null +++ b/tests/PPDS.Auth.IntegrationTests/AuthenticationSmokeTests.cs @@ -0,0 +1,39 @@ +using FluentAssertions; +using Xunit; + +namespace PPDS.Auth.IntegrationTests; + +/// +/// Smoke tests for authentication infrastructure. +/// These tests verify the auth library is correctly referenced and basic types work. +/// +public class AuthenticationSmokeTests +{ + [Fact] + public void ProfileStore_CanBeInstantiated() + { + // This test verifies the PPDS.Auth assembly is correctly referenced + // and basic types can be loaded. + var storeType = typeof(PPDS.Auth.Profiles.ProfileStore); + storeType.Should().NotBeNull(); + } + + [Fact] + public void CredentialProvider_TypesExist() + { + // Verify credential provider types are accessible + var clientSecretType = typeof(PPDS.Auth.Credentials.ClientSecretCredentialProvider); + clientSecretType.Should().NotBeNull(); + } + + [Fact] + [Trait("Category", "Integration")] + public void ProfileStore_DefaultPath_IsValid() + { + // Arrange & Act + var store = new PPDS.Auth.Profiles.ProfileStore(); + + // Assert - just verify it can be created without throwing + store.Should().NotBeNull(); + } +} diff --git a/tests/PPDS.Auth.IntegrationTests/PPDS.Auth.IntegrationTests.csproj b/tests/PPDS.Auth.IntegrationTests/PPDS.Auth.IntegrationTests.csproj new file mode 100644 index 000000000..8343a3c0d --- /dev/null +++ b/tests/PPDS.Auth.IntegrationTests/PPDS.Auth.IntegrationTests.csproj @@ -0,0 +1,30 @@ + + + + net8.0;net9.0;net10.0 + PPDS.Auth.IntegrationTests + enable + enable + false + true + + + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + + + diff --git a/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasySmokeTests.cs b/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasySmokeTests.cs new file mode 100644 index 000000000..e980014d9 --- /dev/null +++ b/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasySmokeTests.cs @@ -0,0 +1,76 @@ +using FluentAssertions; +using Microsoft.Xrm.Sdk; +using Xunit; + +namespace PPDS.Dataverse.IntegrationTests; + +/// +/// Smoke tests to verify FakeXrmEasy infrastructure is working correctly. +/// +public class FakeXrmEasySmokeTests : FakeXrmEasyTestsBase +{ + [Fact] + public void Context_IsInitialized() + { + Context.Should().NotBeNull(); + } + + [Fact] + public void Service_IsInitialized() + { + Service.Should().NotBeNull(); + } + + [Fact] + public void Create_WithValidEntity_ReturnsId() + { + // Arrange + var account = new Entity("account") + { + ["name"] = "Test Account" + }; + + // Act + var id = Service.Create(account); + + // Assert + id.Should().NotBeEmpty(); + } + + [Fact] + public void Retrieve_AfterCreate_ReturnsEntity() + { + // Arrange + var account = new Entity("account") + { + ["name"] = "Test Account" + }; + var id = Service.Create(account); + + // Act + var retrieved = Service.Retrieve("account", id, new Microsoft.Xrm.Sdk.Query.ColumnSet(true)); + + // Assert + retrieved.Should().NotBeNull(); + retrieved.GetAttributeValue("name").Should().Be("Test Account"); + } + + [Fact] + public void InitializeWith_SeedsContext() + { + // Arrange + var accountId = Guid.NewGuid(); + var account = new Entity("account", accountId) + { + ["name"] = "Seeded Account" + }; + + // Act + InitializeWith(account); + var retrieved = Service.Retrieve("account", accountId, new Microsoft.Xrm.Sdk.Query.ColumnSet(true)); + + // Assert + retrieved.Should().NotBeNull(); + retrieved.GetAttributeValue("name").Should().Be("Seeded Account"); + } +} diff --git a/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs b/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs new file mode 100644 index 000000000..ff3b0fe92 --- /dev/null +++ b/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs @@ -0,0 +1,68 @@ +using FakeXrmEasy.Abstractions; +using FakeXrmEasy.Abstractions.Enums; +using FakeXrmEasy.Middleware; +using FakeXrmEasy.Middleware.Crud; +using FakeXrmEasy.Middleware.Messages; +using Microsoft.Xrm.Sdk; + +namespace PPDS.Dataverse.IntegrationTests; + +/// +/// Base class for integration tests using FakeXrmEasy to mock Dataverse operations. +/// Provides an in-memory IOrganizationService for testing CRUD and message operations. +/// +public abstract class FakeXrmEasyTestsBase : IDisposable +{ + /// + /// The FakeXrmEasy context providing the mocked Dataverse environment. + /// + protected IXrmFakedContext Context { get; } + + /// + /// The mocked IOrganizationService for executing Dataverse operations. + /// + protected IOrganizationService Service { get; } + + /// + /// Initializes a new instance of the test base with FakeXrmEasy middleware. + /// + protected FakeXrmEasyTestsBase() + { + Context = MiddlewareBuilder + .New() + .AddCrud() + .AddFakeMessageExecutors() + .UseCrud() + .UseMessages() + .SetLicense(FakeXrmEasyLicense.RPL_1_5) + .Build(); + + Service = Context.GetOrganizationService(); + } + + /// + /// Initializes the context with a set of pre-existing entities. + /// + /// The entities to seed the context with. + protected void InitializeWith(params Entity[] entities) + { + Context.Initialize(entities); + } + + /// + /// Initializes the context with a collection of pre-existing entities. + /// + /// The entities to seed the context with. + protected void InitializeWith(IEnumerable entities) + { + Context.Initialize(entities); + } + + /// + /// Disposes of any resources used by the test. + /// + public virtual void Dispose() + { + GC.SuppressFinalize(this); + } +} diff --git a/tests/PPDS.Dataverse.IntegrationTests/PPDS.Dataverse.IntegrationTests.csproj b/tests/PPDS.Dataverse.IntegrationTests/PPDS.Dataverse.IntegrationTests.csproj new file mode 100644 index 000000000..bd18fbfda --- /dev/null +++ b/tests/PPDS.Dataverse.IntegrationTests/PPDS.Dataverse.IntegrationTests.csproj @@ -0,0 +1,32 @@ + + + + net8.0;net9.0;net10.0 + PPDS.Dataverse.IntegrationTests + enable + enable + false + true + + + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + + + + + diff --git a/tests/PPDS.LiveTests/Infrastructure/LiveTestBase.cs b/tests/PPDS.LiveTests/Infrastructure/LiveTestBase.cs new file mode 100644 index 000000000..cb28540ab --- /dev/null +++ b/tests/PPDS.LiveTests/Infrastructure/LiveTestBase.cs @@ -0,0 +1,88 @@ +using Xunit; + +namespace PPDS.LiveTests.Infrastructure; + +/// +/// Base class for live Dataverse integration tests. +/// Provides shared configuration and setup/teardown support. +/// +[Collection("LiveDataverse")] +[Trait("Category", "Integration")] +public abstract class LiveTestBase : IAsyncLifetime +{ + /// + /// Configuration containing credentials and connection details. + /// + protected LiveTestConfiguration Configuration { get; } + + /// + /// Initializes a new instance of the live test base. + /// + protected LiveTestBase() + { + Configuration = new LiveTestConfiguration(); + } + + /// + /// Async initialization called before each test. + /// Override to perform custom setup. + /// + public virtual Task InitializeAsync() + { + return Task.CompletedTask; + } + + /// + /// Async cleanup called after each test. + /// Override to perform custom teardown. + /// + public virtual Task DisposeAsync() + { + return Task.CompletedTask; + } +} + +/// +/// Collection definition for live Dataverse tests. +/// Tests in this collection run sequentially to avoid overwhelming the API. +/// +[CollectionDefinition("LiveDataverse")] +public class LiveDataverseCollection : ICollectionFixture +{ +} + +/// +/// Shared fixture for live Dataverse tests. +/// Created once per test collection and shared across all tests. +/// +public class LiveDataverseFixture : IAsyncLifetime +{ + /// + /// Configuration for the live tests. + /// + public LiveTestConfiguration Configuration { get; } + + /// + /// Initializes the fixture. + /// + public LiveDataverseFixture() + { + Configuration = new LiveTestConfiguration(); + } + + /// + /// Called once before any tests in the collection run. + /// + public Task InitializeAsync() + { + return Task.CompletedTask; + } + + /// + /// Called once after all tests in the collection have run. + /// + public Task DisposeAsync() + { + return Task.CompletedTask; + } +} diff --git a/tests/PPDS.LiveTests/Infrastructure/LiveTestConfiguration.cs b/tests/PPDS.LiveTests/Infrastructure/LiveTestConfiguration.cs new file mode 100644 index 000000000..fd513406f --- /dev/null +++ b/tests/PPDS.LiveTests/Infrastructure/LiveTestConfiguration.cs @@ -0,0 +1,101 @@ +namespace PPDS.LiveTests.Infrastructure; + +/// +/// Configuration for live Dataverse integration tests. +/// Reads credentials from environment variables. +/// +public sealed class LiveTestConfiguration +{ + /// + /// The Dataverse environment URL (e.g., https://org.crm.dynamics.com). + /// + public string? DataverseUrl { get; } + + /// + /// The Entra ID Application (Client) ID. + /// + public string? ApplicationId { get; } + + /// + /// The client secret for client credential authentication. + /// + public string? ClientSecret { get; } + + /// + /// The Entra ID Tenant ID. + /// + public string? TenantId { get; } + + /// + /// Base64-encoded certificate for certificate-based authentication. + /// + public string? CertificateBase64 { get; } + + /// + /// Password for the certificate. + /// + public string? CertificatePassword { get; } + + /// + /// Gets a value indicating whether client secret credentials are available. + /// + public bool HasClientSecretCredentials => + !string.IsNullOrWhiteSpace(DataverseUrl) && + !string.IsNullOrWhiteSpace(ApplicationId) && + !string.IsNullOrWhiteSpace(ClientSecret) && + !string.IsNullOrWhiteSpace(TenantId); + + /// + /// Gets a value indicating whether certificate credentials are available. + /// + public bool HasCertificateCredentials => + !string.IsNullOrWhiteSpace(DataverseUrl) && + !string.IsNullOrWhiteSpace(ApplicationId) && + !string.IsNullOrWhiteSpace(CertificateBase64) && + !string.IsNullOrWhiteSpace(TenantId); + + /// + /// Gets a value indicating whether any live test credentials are available. + /// + public bool HasAnyCredentials => HasClientSecretCredentials || HasCertificateCredentials; + + /// + /// Initializes a new instance reading from environment variables. + /// + public LiveTestConfiguration() + { + DataverseUrl = Environment.GetEnvironmentVariable("DATAVERSE_URL"); + ApplicationId = Environment.GetEnvironmentVariable("PPDS_TEST_APP_ID"); + ClientSecret = Environment.GetEnvironmentVariable("PPDS_TEST_CLIENT_SECRET"); + TenantId = Environment.GetEnvironmentVariable("PPDS_TEST_TENANT_ID"); + CertificateBase64 = Environment.GetEnvironmentVariable("PPDS_TEST_CERT_BASE64"); + CertificatePassword = Environment.GetEnvironmentVariable("PPDS_TEST_CERT_PASSWORD"); + } + + /// + /// Gets the reason why credentials are not available, for skip messages. + /// + public string GetMissingCredentialsReason() + { + var missing = new List(); + + if (string.IsNullOrWhiteSpace(DataverseUrl)) + missing.Add("DATAVERSE_URL"); + if (string.IsNullOrWhiteSpace(ApplicationId)) + missing.Add("PPDS_TEST_APP_ID"); + if (string.IsNullOrWhiteSpace(TenantId)) + missing.Add("PPDS_TEST_TENANT_ID"); + + if (!HasClientSecretCredentials && !HasCertificateCredentials) + { + if (string.IsNullOrWhiteSpace(ClientSecret)) + missing.Add("PPDS_TEST_CLIENT_SECRET"); + if (string.IsNullOrWhiteSpace(CertificateBase64)) + missing.Add("PPDS_TEST_CERT_BASE64"); + } + + return missing.Count > 0 + ? $"Missing environment variables: {string.Join(", ", missing)}" + : "Unknown reason"; + } +} diff --git a/tests/PPDS.LiveTests/Infrastructure/SkipIfNoCredentialsAttribute.cs b/tests/PPDS.LiveTests/Infrastructure/SkipIfNoCredentialsAttribute.cs new file mode 100644 index 000000000..cc809bac6 --- /dev/null +++ b/tests/PPDS.LiveTests/Infrastructure/SkipIfNoCredentialsAttribute.cs @@ -0,0 +1,61 @@ +using Xunit; + +namespace PPDS.LiveTests.Infrastructure; + +/// +/// Skips the test if live Dataverse credentials are not configured. +/// Use this attribute on test methods that require a real Dataverse connection. +/// +public sealed class SkipIfNoCredentialsAttribute : FactAttribute +{ + private static readonly LiveTestConfiguration Configuration = new(); + + /// + /// Initializes a new instance that skips if credentials are missing. + /// + public SkipIfNoCredentialsAttribute() + { + if (!Configuration.HasAnyCredentials) + { + Skip = Configuration.GetMissingCredentialsReason(); + } + } +} + +/// +/// Skips the test if client secret credentials are not configured. +/// +public sealed class SkipIfNoClientSecretAttribute : FactAttribute +{ + private static readonly LiveTestConfiguration Configuration = new(); + + /// + /// Initializes a new instance that skips if client secret credentials are missing. + /// + public SkipIfNoClientSecretAttribute() + { + if (!Configuration.HasClientSecretCredentials) + { + Skip = "Client secret credentials not configured. Set DATAVERSE_URL, PPDS_TEST_APP_ID, PPDS_TEST_CLIENT_SECRET, and PPDS_TEST_TENANT_ID."; + } + } +} + +/// +/// Skips the test if certificate credentials are not configured. +/// +public sealed class SkipIfNoCertificateAttribute : FactAttribute +{ + private static readonly LiveTestConfiguration Configuration = new(); + + /// + /// Initializes a new instance that skips if certificate credentials are missing. + /// + public SkipIfNoCertificateAttribute() + { + if (!Configuration.HasCertificateCredentials) + { + Skip = "Certificate credentials not configured. Set DATAVERSE_URL, PPDS_TEST_APP_ID, PPDS_TEST_CERT_BASE64, and PPDS_TEST_TENANT_ID."; + } + } +} diff --git a/tests/PPDS.LiveTests/LiveDataverseSmokeTests.cs b/tests/PPDS.LiveTests/LiveDataverseSmokeTests.cs new file mode 100644 index 000000000..0de508f2d --- /dev/null +++ b/tests/PPDS.LiveTests/LiveDataverseSmokeTests.cs @@ -0,0 +1,59 @@ +using FluentAssertions; +using PPDS.LiveTests.Infrastructure; +using Xunit; + +namespace PPDS.LiveTests; + +/// +/// Smoke tests for live Dataverse integration. +/// These tests verify the test infrastructure works and skip gracefully when credentials are missing. +/// +public class LiveDataverseSmokeTests : LiveTestBase +{ + [Fact] + public void Configuration_IsAvailable() + { + Configuration.Should().NotBeNull(); + } + + [Fact] + public void Configuration_ReportsCredentialStatus() + { + // This test always runs and verifies the configuration class works + var hasCredentials = Configuration.HasAnyCredentials; + + // Just verify we can check - the result depends on environment + hasCredentials.Should().Be(hasCredentials); // tautology to avoid unused variable warning + } + + [SkipIfNoCredentials] + [Trait("Category", "Integration")] + public void WhenCredentialsAvailable_CanConnect() + { + // This test only runs when credentials are available + Configuration.HasAnyCredentials.Should().BeTrue(); + Configuration.DataverseUrl.Should().NotBeNullOrWhiteSpace(); + } + + [SkipIfNoClientSecret] + [Trait("Category", "Integration")] + public void WhenClientSecretAvailable_HasAllRequiredFields() + { + Configuration.HasClientSecretCredentials.Should().BeTrue(); + Configuration.ApplicationId.Should().NotBeNullOrWhiteSpace(); + Configuration.ClientSecret.Should().NotBeNullOrWhiteSpace(); + Configuration.TenantId.Should().NotBeNullOrWhiteSpace(); + } + + [Fact] + public void MissingCredentialsReason_DescribesMissingVars() + { + var config = new LiveTestConfiguration(); + var reason = config.GetMissingCredentialsReason(); + + if (!config.HasAnyCredentials) + { + reason.Should().Contain("Missing environment variables"); + } + } +} diff --git a/tests/PPDS.LiveTests/PPDS.LiveTests.csproj b/tests/PPDS.LiveTests/PPDS.LiveTests.csproj new file mode 100644 index 000000000..99af59f7f --- /dev/null +++ b/tests/PPDS.LiveTests/PPDS.LiveTests.csproj @@ -0,0 +1,31 @@ + + + + net8.0;net9.0;net10.0 + PPDS.LiveTests + enable + enable + false + true + + + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + + + + From 9983f70c8f4d26104a9627d19106bbf8602c050a Mon Sep 17 00:00:00 2001 From: Josh Smith <6895577+joshsmithxrm@users.noreply.github.com> Date: Fri, 2 Jan 2026 01:54:37 -0600 Subject: [PATCH 2/9] feat: add code coverage reporting with Codecov (#84) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add coverage collection and Codecov integration for visibility into test coverage across all packages. Changes: - test.yml: Add --collect:"XPlat Code Coverage" and Codecov upload - codecov.yml: Per-package thresholds matching issue #55 targets - .gitignore: Exclude coverage/ directory Per-package targets (informational only, no PR blocking): - PPDS.Plugins: 95% (currently 100%) - PPDS.Auth: 70% (currently 0% - no tests) - PPDS.Dataverse: 60% (currently 38%) - PPDS.Migration: 50% (currently 0% - no tests) - PPDS.Cli: 60% (currently ~12%) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .github/workflows/test.yml | 13 +++++- .gitignore | 3 ++ codecov.yml | 84 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 codecov.yml diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index debb76ecc..47e1517bc 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,5 +29,14 @@ jobs: - name: Build run: dotnet build --configuration Release --no-restore - - name: Test (Unit Tests Only) - run: dotnet test --configuration Release --no-build --verbosity normal --filter "Category!=Integration" + - name: Test with Coverage + run: | + dotnet test --configuration Release --no-build --verbosity normal --filter "Category!=Integration" --collect:"XPlat Code Coverage" --results-directory ./coverage + + - name: Upload coverage to Codecov + uses: codecov/codecov-action@v5 + with: + directory: ./coverage + flags: unittests + fail_ci_if_error: false # Don't fail build if upload fails + verbose: true diff --git a/.gitignore b/.gitignore index 90da92daa..9dee43335 100644 --- a/.gitignore +++ b/.gitignore @@ -55,3 +55,6 @@ nul data.zip data/ schema.xml + +# Code coverage +coverage/ diff --git a/codecov.yml b/codecov.yml new file mode 100644 index 000000000..1ba3bcf31 --- /dev/null +++ b/codecov.yml @@ -0,0 +1,84 @@ +# Codecov Configuration +# https://docs.codecov.com/docs/codecov-yaml + +coverage: + # Overall project coverage status + status: + project: + default: + target: 60% + threshold: 5% # Allow 5% drop before failing + informational: true # Don't block PRs (visibility only for now) + + # Patch coverage - new code should be well-tested + patch: + default: + target: 80% + informational: true # Don't block PRs (visibility only for now) + + # Per-package thresholds using component system + # These match the targets defined in issue #55 + +# Component-based coverage tracking +component_management: + default_rules: + statuses: + - type: project + informational: true + - type: patch + informational: true + + individual_components: + - component_id: plugins + name: PPDS.Plugins + paths: + - src/PPDS.Plugins/** + statuses: + - type: project + target: 95% + + - component_id: auth + name: PPDS.Auth + paths: + - src/PPDS.Auth/** + statuses: + - type: project + target: 70% + + - component_id: dataverse + name: PPDS.Dataverse + paths: + - src/PPDS.Dataverse/** + statuses: + - type: project + target: 60% + + - component_id: migration + name: PPDS.Migration + paths: + - src/PPDS.Migration/** + statuses: + - type: project + target: 50% + + - component_id: cli + name: PPDS.Cli + paths: + - src/PPDS.Cli/** + statuses: + - type: project + target: 60% + +# Ignore test projects and generated files +ignore: + - "tests/**" + - "**/obj/**" + - "**/bin/**" + - "**/*.g.cs" + - "**/*.Designer.cs" + +# PR comment configuration +comment: + layout: "header, diff, components, footer" + behavior: default + require_changes: true # Only comment when coverage changes From 12b6d0339d8891f168c154e08cf3a50d111d8b74 Mon Sep 17 00:00:00 2001 From: Josh Smith <6895577+joshsmithxrm@users.noreply.github.com> Date: Fri, 2 Jan 2026 02:00:39 -0600 Subject: [PATCH 3/9] fix: pre-commit hook error on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Change matcher from "Bash" to "Bash(git commit:*)" so hook only fires on commit commands, not every Bash invocation - Use relative path instead of $CLAUDE_PROJECT_DIR which wasn't expanding correctly on Windows - Simplify Python script since matcher now handles filtering 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .claude/hooks/pre-commit-validate.py | 52 +++++++++++----------------- .claude/settings.json | 4 +-- 2 files changed, 23 insertions(+), 33 deletions(-) diff --git a/.claude/hooks/pre-commit-validate.py b/.claude/hooks/pre-commit-validate.py index 740170b13..5c4f86eb9 100644 --- a/.claude/hooks/pre-commit-validate.py +++ b/.claude/hooks/pre-commit-validate.py @@ -2,47 +2,36 @@ """ Pre-commit validation hook for PPDS SDK. Runs dotnet build and test before allowing git commit. + +Note: This hook is only triggered for 'git commit' commands via the +matcher in .claude/settings.json - no need to filter here. """ -import json -import shlex import subprocess import sys import os +import json -def main(): - try: - input_data = json.load(sys.stdin) - except json.JSONDecodeError: - print("⚠️ pre-commit-validate: Failed to parse input. Skipping validation.", file=sys.stderr) - sys.exit(0) - - tool_name = input_data.get("tool_name", "") - tool_input = input_data.get("tool_input", {}) - command = tool_input.get("command", "") - # Only validate git commit commands (robust check using shlex) - if tool_name != "Bash": - sys.exit(0) +def main(): + # Read stdin (Claude Code sends JSON with tool info) try: - parts = shlex.split(command) - is_git_commit = len(parts) >= 2 and os.path.basename(parts[0]) == "git" and parts[1] == "commit" - except ValueError: - is_git_commit = False - if not is_git_commit: - sys.exit(0) # Allow non-commit commands + json.load(sys.stdin) + except (json.JSONDecodeError, EOFError): + pass # Input parsing is optional - matcher already filtered - # Get project directory + # Get project directory from environment or use current directory project_dir = os.environ.get("CLAUDE_PROJECT_DIR", os.getcwd()) try: + print("🔨 Running pre-commit validation...", file=sys.stderr) + # Run dotnet build - print("Running pre-commit validation...", file=sys.stderr) build_result = subprocess.run( ["dotnet", "build", "-c", "Release", "--nologo", "-v", "q"], cwd=project_dir, capture_output=True, text=True, - timeout=300 # 5 minute timeout + timeout=300 ) if build_result.returncode != 0: @@ -51,16 +40,16 @@ def main(): print(build_result.stdout, file=sys.stderr) if build_result.stderr: print(build_result.stderr, file=sys.stderr) - sys.exit(2) # Block commit + sys.exit(2) - # Run dotnet test (unit tests only - integration tests run on PR) + # Run unit tests only (integration tests run on PR) test_result = subprocess.run( ["dotnet", "test", "--no-build", "-c", "Release", "--nologo", "-v", "q", "--filter", "Category!=Integration"], cwd=project_dir, capture_output=True, text=True, - timeout=300 # 5 minute timeout + timeout=300 ) if test_result.returncode != 0: @@ -69,17 +58,18 @@ def main(): print(test_result.stdout, file=sys.stderr) if test_result.stderr: print(test_result.stderr, file=sys.stderr) - sys.exit(2) # Block commit + sys.exit(2) print("✅ Build and unit tests passed", file=sys.stderr) - sys.exit(0) # Allow commit + sys.exit(0) except FileNotFoundError: - print("⚠️ pre-commit-validate: dotnet not found in PATH. Skipping validation.", file=sys.stderr) + print("⚠️ dotnet not found in PATH. Skipping validation.", file=sys.stderr) sys.exit(0) except subprocess.TimeoutExpired: - print("⚠️ pre-commit-validate: Build/test timed out after 5 minutes. Skipping validation.", file=sys.stderr) + print("⚠️ Build/test timed out. Skipping validation.", file=sys.stderr) sys.exit(0) + if __name__ == "__main__": main() diff --git a/.claude/settings.json b/.claude/settings.json index 436598548..ea8beb4d3 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -2,11 +2,11 @@ "hooks": { "PreToolUse": [ { - "matcher": "Bash", + "matcher": "Bash(git commit:*)", "hooks": [ { "type": "command", - "command": "python \"$CLAUDE_PROJECT_DIR/.claude/hooks/pre-commit-validate.py\"", + "command": "python .claude/hooks/pre-commit-validate.py", "timeout": 120 } ] From fb7766aebe343066c396b6a0b93583ccc1834e3d Mon Sep 17 00:00:00 2001 From: Josh Smith <6895577+joshsmithxrm@users.noreply.github.com> Date: Fri, 2 Jan 2026 02:18:21 -0600 Subject: [PATCH 4/9] fix: address bot review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - FakeXrmEasyTestsBase: Call Context.Dispose() to prevent resource leak - LiveDataverseSmokeTests: Fix tautology, improve assertion logic - LiveDataverseSmokeTests: Remove redundant [Trait] (inherited from base) - AuthenticationSmokeTests: Remove Integration trait (no external deps) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .../AuthenticationSmokeTests.cs | 1 - .../FakeXrmEasyTestsBase.cs | 1 + tests/PPDS.LiveTests/LiveDataverseSmokeTests.cs | 10 +++++----- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/PPDS.Auth.IntegrationTests/AuthenticationSmokeTests.cs b/tests/PPDS.Auth.IntegrationTests/AuthenticationSmokeTests.cs index 067b5b494..1badd8c89 100644 --- a/tests/PPDS.Auth.IntegrationTests/AuthenticationSmokeTests.cs +++ b/tests/PPDS.Auth.IntegrationTests/AuthenticationSmokeTests.cs @@ -27,7 +27,6 @@ public void CredentialProvider_TypesExist() } [Fact] - [Trait("Category", "Integration")] public void ProfileStore_DefaultPath_IsValid() { // Arrange & Act diff --git a/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs b/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs index ff3b0fe92..8ea1590b5 100644 --- a/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs +++ b/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs @@ -63,6 +63,7 @@ protected void InitializeWith(IEnumerable entities) /// public virtual void Dispose() { + Context.Dispose(); GC.SuppressFinalize(this); } } diff --git a/tests/PPDS.LiveTests/LiveDataverseSmokeTests.cs b/tests/PPDS.LiveTests/LiveDataverseSmokeTests.cs index 0de508f2d..d54c78872 100644 --- a/tests/PPDS.LiveTests/LiveDataverseSmokeTests.cs +++ b/tests/PPDS.LiveTests/LiveDataverseSmokeTests.cs @@ -20,14 +20,15 @@ public void Configuration_IsAvailable() public void Configuration_ReportsCredentialStatus() { // This test always runs and verifies the configuration class works - var hasCredentials = Configuration.HasAnyCredentials; + var hasAny = Configuration.HasAnyCredentials; + var hasSecret = Configuration.HasClientSecretCredentials; + var hasCert = Configuration.HasCertificateCredentials; - // Just verify we can check - the result depends on environment - hasCredentials.Should().Be(hasCredentials); // tautology to avoid unused variable warning + // Assert that the aggregate property reflects the state of the specific properties + hasAny.Should().Be(hasSecret || hasCert); } [SkipIfNoCredentials] - [Trait("Category", "Integration")] public void WhenCredentialsAvailable_CanConnect() { // This test only runs when credentials are available @@ -36,7 +37,6 @@ public void WhenCredentialsAvailable_CanConnect() } [SkipIfNoClientSecret] - [Trait("Category", "Integration")] public void WhenClientSecretAvailable_HasAllRequiredFields() { Configuration.HasClientSecretCredentials.Should().BeTrue(); From e517109e20765658a4dcc6a273b71a88f55d3ec0 Mon Sep 17 00:00:00 2001 From: Josh Smith <6895577+joshsmithxrm@users.noreply.github.com> Date: Fri, 2 Jan 2026 02:25:00 -0600 Subject: [PATCH 5/9] docs: add individual comment reply syntax to review-bot-comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add section 5 documenting the correct gh api syntax for replying to individual PR review comments using in_reply_to parameter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .claude/commands/review-bot-comments.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/.claude/commands/review-bot-comments.md b/.claude/commands/review-bot-comments.md index 5a3555b7d..b3bec0578 100644 --- a/.claude/commands/review-bot-comments.md +++ b/.claude/commands/review-bot-comments.md @@ -41,6 +41,25 @@ For each bot comment, determine: | Resource not disposed | Yes - leak | | Generic catch clause | Context-dependent | +### 5. Reply to Each Comment Individually + +After fixing or triaging, reply directly to each bot comment. Do NOT batch responses into a single PR comment. + +```bash +# Reply to a specific review comment +gh api repos/joshsmithxrm/ppds-sdk/pulls/{pr}/comments \ + -f body="Fixed in abc123" \ + -F in_reply_to={comment_id} +``` + +**Important:** The `in_reply_to` parameter must be the comment ID (numeric), not a URL. Get IDs from the fetch step. + +| Verdict | Reply Template | +|---------|----------------| +| Valid (fixed) | `Fixed in {commit_sha} - {brief description}` | +| Declined | `Declining - {reason}` | +| False positive | `False positive - {explanation}` | + ## Output ```markdown From 3d99c566b6b3623a5b2238c6a78143f20550e408 Mon Sep 17 00:00:00 2001 From: Josh Smith <6895577+joshsmithxrm@users.noreply.github.com> Date: Fri, 2 Jan 2026 02:30:36 -0600 Subject: [PATCH 6/9] fix: IXrmFakedContext doesn't implement IDisposable directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini's suggestion was incorrect - IXrmFakedContext interface doesn't declare IDisposable. Use safe cast to dispose if runtime type supports it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs b/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs index 8ea1590b5..d017b21fc 100644 --- a/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs +++ b/tests/PPDS.Dataverse.IntegrationTests/FakeXrmEasyTestsBase.cs @@ -63,7 +63,8 @@ protected void InitializeWith(IEnumerable entities) /// public virtual void Dispose() { - Context.Dispose(); + // IXrmFakedContext doesn't declare IDisposable, but runtime type may implement it + (Context as IDisposable)?.Dispose(); GC.SuppressFinalize(this); } } From fce5b85511d17887d7515696cb9a19ffcc38eae7 Mon Sep 17 00:00:00 2001 From: Josh Smith <6895577+joshsmithxrm@users.noreply.github.com> Date: Fri, 2 Jan 2026 02:36:34 -0600 Subject: [PATCH 7/9] docs: require user approval before implementing bot suggestions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update review-bot-comments command to make the process explicit: 1. Fetch and triage comments 2. Present summary with recommendations 3. WAIT for user approval (do NOT auto-implement) 4. Implement approved changes 5. Build and verify before committing 6. Reply to comments Added warning about bot suggestions being wrong (e.g., Gemini suggested calling Dispose on an interface that doesn't implement IDisposable). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .claude/commands/review-bot-comments.md | 78 +++++++++++++++---------- 1 file changed, 46 insertions(+), 32 deletions(-) diff --git a/.claude/commands/review-bot-comments.md b/.claude/commands/review-bot-comments.md index b3bec0578..8a370f537 100644 --- a/.claude/commands/review-bot-comments.md +++ b/.claude/commands/review-bot-comments.md @@ -15,35 +15,45 @@ gh api repos/joshsmithxrm/ppds-sdk/pulls/[PR]/comments ### 2. Triage Each Comment -For each bot comment, determine: +For each bot comment, determine verdict and rationale: -| Verdict | Action | -|---------|--------| -| **Valid** | Fix the issue, reply "Fixed in [commit]" | -| **False Positive** | Reply with reason, dismiss | -| **Unclear** | Investigate before deciding | +| Verdict | Meaning | +|---------|---------| +| **Valid** | Bot is correct, code should be changed | +| **False Positive** | Bot is wrong, explain why | +| **Unclear** | Need to investigate before deciding | -### 3. Common False Positives +### 3. Present Summary and WAIT FOR APPROVAL -| Bot Claim | Why It's Often Wrong | -|-----------|---------------------| -| "Use .Where() instead of foreach+if" | Preference, not correctness | -| "Volatile needed with Interlocked" | Interlocked provides barriers | -| "OR should be AND" | Logic may be intentionally inverted (DeMorgan) | -| "Static field not thread-safe" | May be set once at startup | +**CRITICAL: Do NOT implement fixes automatically.** -### 4. Common Valid Findings +Present a summary table to the user: -| Pattern | Usually Valid | -|---------|---------------| -| Unused variable/parameter | Yes - dead code | -| Missing null check | Check context | -| Resource not disposed | Yes - leak | -| Generic catch clause | Context-dependent | +```markdown +## Bot Review Triage - PR #XX + +| # | Bot | Finding | Verdict | Recommendation | Rationale | +|---|-----|---------|---------|----------------|-----------| +| 1 | Gemini | Missing Dispose | Valid | Add dispose call | Prevents resource leak | +| 2 | Copilot | Use .Where() | False Positive | Decline | Style preference | +``` + +**STOP HERE. Wait for user to review and approve before making ANY changes.** + +Bot suggestions can be wrong (e.g., suggesting methods that don't exist on an interface). +Always get user approval, then verify changes compile before committing. + +### 4. Implement Approved Changes + +After user approval: +1. Make the approved code changes +2. **Build and verify** - `dotnet build` must succeed +3. Run tests to confirm no regressions +4. Commit with descriptive message ### 5. Reply to Each Comment Individually -After fixing or triaging, reply directly to each bot comment. Do NOT batch responses into a single PR comment. +After changes are committed, reply to each bot comment. Do NOT batch responses into a single PR comment. ```bash # Reply to a specific review comment @@ -60,20 +70,24 @@ gh api repos/joshsmithxrm/ppds-sdk/pulls/{pr}/comments \ | Declined | `Declining - {reason}` | | False positive | `False positive - {explanation}` | -## Output +## Common False Positives -```markdown -## Bot Review Triage - PR #82 +| Bot Claim | Why It's Often Wrong | +|-----------|---------------------| +| "Use .Where() instead of foreach+if" | Preference, not correctness | +| "Volatile needed with Interlocked" | Interlocked provides barriers | +| "OR should be AND" | Logic may be intentionally inverted (DeMorgan) | +| "Static field not thread-safe" | May be set once at startup | +| "Call Dispose on X" | Interface may not actually implement IDisposable | -| # | Bot | Finding | Verdict | Action | -|---|-----|---------|---------|--------| -| 1 | Gemini | Use constants in dict | Valid | Fixed in abc123 | -| 2 | Copilot | Add validation tests | Valid | Fixed in def456 | -| 3 | Copilot | Use .Where() | False Positive | Style preference | -| 4 | CodeQL | Generic catch | Valid (low) | Acceptable for disposal | +## Common Valid Findings -All findings addressed: Yes -``` +| Pattern | Usually Valid | +|---------|---------------| +| Unused variable/parameter | Yes - dead code | +| Missing null check | Check context | +| Resource not disposed | Yes - but verify interface first | +| Generic catch clause | Context-dependent | ## When to Use From 87c1b0e7e2ea4d87c134bef800c966196e92e621 Mon Sep 17 00:00:00 2001 From: Josh Smith <6895577+joshsmithxrm@users.noreply.github.com> Date: Fri, 2 Jan 2026 02:37:43 -0600 Subject: [PATCH 8/9] fix: missing closing paren in settings.local.example.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .claude/settings.local.example.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/settings.local.example.json b/.claude/settings.local.example.json index b0136bde8..0a1427a9a 100644 --- a/.claude/settings.local.example.json +++ b/.claude/settings.local.example.json @@ -79,7 +79,7 @@ "Bash(gh pr status:*)", "Bash(gh pr checkout:*)", "Bash(gh pr diff:*)", - "Bash(gh api:*", + "Bash(gh api:*)", "Bash(gh issue list:*)", "Bash(gh issue view:*)", "Bash(gh issue status:*)", From 8eea551189b054dc77a7f93797902486fb130802 Mon Sep 17 00:00:00 2001 From: Josh Smith <6895577+joshsmithxrm@users.noreply.github.com> Date: Fri, 2 Jan 2026 03:19:21 -0600 Subject: [PATCH 9/9] chore: clean up scripts folder and add certificate generator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename install-local.ps1 to Install-LocalCli.ps1 (PowerShell Verb-Noun convention) - Delete ppds-dev.ps1 (trivial wrapper that just called dotnet run) - Add New-TestCertificate.ps1 for generating Azure App Registration test certificates 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- ...install-local.ps1 => Install-LocalCli.ps1} | 0 scripts/New-TestCertificate.ps1 | 142 ++++++++++++++++++ scripts/ppds-dev.ps1 | 15 -- 3 files changed, 142 insertions(+), 15 deletions(-) rename scripts/{install-local.ps1 => Install-LocalCli.ps1} (100%) create mode 100644 scripts/New-TestCertificate.ps1 delete mode 100644 scripts/ppds-dev.ps1 diff --git a/scripts/install-local.ps1 b/scripts/Install-LocalCli.ps1 similarity index 100% rename from scripts/install-local.ps1 rename to scripts/Install-LocalCli.ps1 diff --git a/scripts/New-TestCertificate.ps1 b/scripts/New-TestCertificate.ps1 new file mode 100644 index 000000000..6aa620844 --- /dev/null +++ b/scripts/New-TestCertificate.ps1 @@ -0,0 +1,142 @@ +<# +.SYNOPSIS + Creates a self-signed certificate for PPDS integration testing. + +.DESCRIPTION + Generates a certificate for Azure App Registration authentication testing. + Outputs: + - .cer file (public key) - upload to Azure App Registration + - .pfx file (private key) - store as GitHub secret (base64 encoded) + - Base64 text file - copy to PPDS_TEST_CERT_BASE64 secret + +.PARAMETER CertName + Name for the certificate. Default: "PPDS-IntegrationTests" + +.PARAMETER Password + Password to protect the PFX file. If not provided, prompts for input. + +.PARAMETER OutputPath + Directory to export files. Default: current directory. + +.PARAMETER ValidYears + How many years the certificate is valid. Default: 2 + +.EXAMPLE + .\New-TestCertificate.ps1 + # Prompts for password, creates cert in current directory + +.EXAMPLE + .\New-TestCertificate.ps1 -Password "MySecurePass123!" -OutputPath C:\certs + # Creates cert with specified password in C:\certs + +.NOTES + After running this script: + 1. Upload the .cer file to your Azure App Registration + 2. Add PPDS_TEST_CERT_BASE64 secret (contents of -base64.txt file) + 3. Add PPDS_TEST_CERT_PASSWORD secret (the password you used) + 4. Delete the local files for security +#> +[CmdletBinding()] +param( + [Parameter()] + [string]$CertName = "PPDS-IntegrationTests", + + [Parameter()] + [string]$Password, + + [Parameter()] + [string]$OutputPath = (Get-Location).Path, + + [Parameter()] + [int]$ValidYears = 2 +) + +$ErrorActionPreference = "Stop" + +# Prompt for password if not provided +if (-not $Password) { + $securePassword = Read-Host -Prompt "Enter password for PFX file" -AsSecureString + $bstr = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($securePassword) + $Password = [System.Runtime.InteropServices.Marshal]::PtrToStringAuto($bstr) + [System.Runtime.InteropServices.Marshal]::ZeroFreeBSTR($bstr) +} + +if ([string]::IsNullOrWhiteSpace($Password)) { + Write-Error "Password cannot be empty" + exit 1 +} + +$securePass = ConvertTo-SecureString -String $Password -Force -AsPlainText + +# Create output directory if needed +if (-not (Test-Path $OutputPath)) { + New-Item -ItemType Directory -Path $OutputPath -Force | Out-Null +} + +# File paths +$pfxPath = Join-Path $OutputPath "$CertName.pfx" +$cerPath = Join-Path $OutputPath "$CertName.cer" +$base64Path = Join-Path $OutputPath "$CertName-base64.txt" + +Write-Host "Creating self-signed certificate..." -ForegroundColor Cyan + +# Create the certificate +$cert = New-SelfSignedCertificate ` + -Subject "CN=$CertName" ` + -DnsName "$CertName.local" ` + -CertStoreLocation "Cert:\CurrentUser\My" ` + -NotAfter (Get-Date).AddYears($ValidYears) ` + -KeySpec KeyExchange ` + -KeyExportPolicy Exportable ` + -KeyLength 2048 ` + -HashAlgorithm SHA256 + +Write-Host "Certificate created with thumbprint: $($cert.Thumbprint)" -ForegroundColor Green + +# Export PFX (with private key) +Write-Host "Exporting PFX (private key)..." -ForegroundColor Cyan +Export-PfxCertificate -Cert $cert -FilePath $pfxPath -Password $securePass | Out-Null +Write-Host " -> $pfxPath" -ForegroundColor Gray + +# Export CER (public key only) +Write-Host "Exporting CER (public key)..." -ForegroundColor Cyan +Export-Certificate -Cert $cert -FilePath $cerPath | Out-Null +Write-Host " -> $cerPath" -ForegroundColor Gray + +# Convert PFX to Base64 +Write-Host "Converting PFX to Base64..." -ForegroundColor Cyan +$pfxBytes = [System.IO.File]::ReadAllBytes($pfxPath) +$pfxBase64 = [System.Convert]::ToBase64String($pfxBytes) +[System.IO.File]::WriteAllText($base64Path, $pfxBase64) +Write-Host " -> $base64Path" -ForegroundColor Gray + +# Copy to clipboard +$pfxBase64 | Set-Clipboard +Write-Host "Base64 copied to clipboard!" -ForegroundColor Green + +# Remove from cert store (optional - keep if you want to test locally) +$removeFromStore = Read-Host "Remove certificate from local store? (y/N)" +if ($removeFromStore -eq 'y') { + Remove-Item "Cert:\CurrentUser\My\$($cert.Thumbprint)" -Force + Write-Host "Removed from cert store" -ForegroundColor Gray +} + +# Summary +Write-Host "" +Write-Host "========================================" -ForegroundColor Cyan +Write-Host "Certificate created successfully!" -ForegroundColor Green +Write-Host "========================================" -ForegroundColor Cyan +Write-Host "" +Write-Host "Next steps:" -ForegroundColor Yellow +Write-Host "1. Upload to Azure:" -ForegroundColor White +Write-Host " Azure Portal -> App Registrations -> Your App -> Certificates & secrets" +Write-Host " Upload: $cerPath" -ForegroundColor Gray +Write-Host "" +Write-Host "2. Add GitHub Secrets:" -ForegroundColor White +Write-Host " PPDS_TEST_CERT_BASE64 = contents of $base64Path" -ForegroundColor Gray +Write-Host " PPDS_TEST_CERT_PASSWORD = $Password" -ForegroundColor Gray +Write-Host "" +Write-Host "3. Delete local files (security):" -ForegroundColor White +Write-Host " Remove-Item '$pfxPath', '$cerPath', '$base64Path'" -ForegroundColor Gray +Write-Host "" +Write-Host "Thumbprint: $($cert.Thumbprint)" -ForegroundColor Cyan diff --git a/scripts/ppds-dev.ps1 b/scripts/ppds-dev.ps1 deleted file mode 100644 index 644aab558..000000000 --- a/scripts/ppds-dev.ps1 +++ /dev/null @@ -1,15 +0,0 @@ -<# -.SYNOPSIS - Run PPDS CLI directly from source (no install needed). -.EXAMPLE - .\scripts\ppds-dev.ps1 env who - .\scripts\ppds-dev.ps1 auth create --name dev - .\scripts\ppds-dev.ps1 data export --schema schema.xml -o data.zip -#> -param( - [Parameter(ValueFromRemainingArguments)] - [string[]]$Arguments -) - -$projectPath = Join-Path $PSScriptRoot "..\src\PPDS.Cli\PPDS.Cli.csproj" -dotnet run --project $projectPath -- @Arguments