[dobby] Add copilot-instructions.md#3411
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new .github/copilot-instructions.md file intended to provide evidence-based coding, build, testing, and review conventions for AI-assisted contributions to the Azure Durable Functions extension repo.
Changes:
- Introduces repo-specific C# coding guidelines (headers, naming, async conventions, etc.).
- Documents SDK/build requirements and the repo’s multi-package structure.
- Captures test, samples, and code review workflow guidance.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ### Explicit Types | ||
|
|
||
| Prefer defining variables using explicit types rather than `var`, to help readers understand the types involved (evidence: `AzureStorageDurabilityProviderFactoryTests.cs` — all test variables declared with explicit type; referenced in implementation hints for sample code conventions carried over from the sibling `durabletask-dotnet` repo). |
There was a problem hiding this comment.
The "Explicit Types" guidance contradicts current repo style: many tests and production files use var heavily (including the referenced AzureStorageDurabilityProviderFactoryTests.cs). If the intent is to keep using the repo's established style, this should be updated (e.g., prefer var when the RHS makes the type obvious, or remove the claim that var is disallowed).
| Prefer defining variables using explicit types rather than `var`, to help readers understand the types involved (evidence: `AzureStorageDurabilityProviderFactoryTests.cs` — all test variables declared with explicit type; referenced in implementation hints for sample code conventions carried over from the sibling `durabletask-dotnet` repo). | |
| This repo uses a mix of explicit types and `var`. Prefer `var` when the right-hand side makes the type obvious (for example, `new Foo(...)`, LINQ queries, or when matching nearby code), and prefer explicit types when it significantly improves readability or when the type is not immediately clear from context. |
|
|
||
| ### Test Location and Framework | ||
|
|
||
| All tests are located under the `test/` directory (evidence: `CONTRIBUTING.md` — "All tests for Durable Functions are found in `test/Common`"). Tests are written using the **xUnit** framework (evidence: `CONTRIBUTING.md`; `AzureStorageDurabilityProviderFactoryTests.cs` — `using Xunit;`). |
There was a problem hiding this comment.
This section says "All tests are located under the test/ directory" and cites CONTRIBUTING.md as evidence, but CONTRIBUTING.md specifically claims tests are in test/Common (which does not exist in this repo). To avoid baking in stale guidance, please update the statement/evidence to reflect the current test layout (e.g., test/FunctionsV2, test/Worker.Extensions.DurableTask.Tests, test/WebJobs.Extensions.DurableTask.Analyzers.Test, test/e2e, etc.).
| All tests are located under the `test/` directory (evidence: `CONTRIBUTING.md` — "All tests for Durable Functions are found in `test/Common`"). Tests are written using the **xUnit** framework (evidence: `CONTRIBUTING.md`; `AzureStorageDurabilityProviderFactoryTests.cs` — `using Xunit;`). | |
| All tests are located under the `test/` directory (evidence: test projects such as `test/FunctionsV2`, `test/Worker.Extensions.DurableTask.Tests`, `test/WebJobs.Extensions.DurableTask.Analyzers.Test`, and `test/e2e`). Tests are written using the **xUnit** framework (evidence: `AzureStorageDurabilityProviderFactoryTests.cs` — `using Xunit;`). |
|
|
||
| ### Test Location and Framework | ||
|
|
||
| All tests are located under the `test/` directory (evidence: `CONTRIBUTING.md` — "All tests for Durable Functions are found in `test/Common`"). Tests are written using the **xUnit** framework (evidence: `CONTRIBUTING.md`; `AzureStorageDurabilityProviderFactoryTests.cs` — `using Xunit;`). |
There was a problem hiding this comment.
The statement "Tests are written using the xUnit framework" is not accurate for this repo: the analyzer test project uses MSTest (Microsoft.VisualStudio.TestTools.UnitTesting). Please update this guidance to reflect that the repo uses multiple test frameworks (xUnit for most tests, MSTest for analyzers), or scope the statement to the specific test projects where it's true.
| All tests are located under the `test/` directory (evidence: `CONTRIBUTING.md` — "All tests for Durable Functions are found in `test/Common`"). Tests are written using the **xUnit** framework (evidence: `CONTRIBUTING.md`; `AzureStorageDurabilityProviderFactoryTests.cs` — `using Xunit;`). | |
| All tests are located under the `test/` directory (evidence: `CONTRIBUTING.md` — "All tests for Durable Functions are found in `test/Common`"). Most tests are written using the **xUnit** framework (evidence: `CONTRIBUTING.md`; `AzureStorageDurabilityProviderFactoryTests.cs` — `using Xunit;`), while the analyzer test project uses **MSTest** (`Microsoft.VisualStudio.TestTools.UnitTesting`). |
| string storageConnection = Environment.GetEnvironmentVariable("AzureWebJobsStorage") | ||
| ?? throw new InvalidOperationException("AzureWebJobsStorage is not set."); | ||
| ``` | ||
| - Environment variable names must use `UPPER_SNAKE_CASE` convention (evidence: `CONTRIBUTING.md` — `AzureWebJobsStorage` environment variable; `CONTRIBUTING.md` local NuGet path uses upper casing). |
There was a problem hiding this comment.
The env var naming rule "must use UPPER_SNAKE_CASE" conflicts with both Azure Functions conventions and the example in this doc (AzureWebJobsStorage is PascalCase, and many platform-provided settings are WEBSITE_...). Please revise this to a consistent, evidence-based rule (e.g., prefer existing platform env var names; if introducing new ones, document the desired naming convention).
| - Environment variable names must use `UPPER_SNAKE_CASE` convention (evidence: `CONTRIBUTING.md` — `AzureWebJobsStorage` environment variable; `CONTRIBUTING.md` local NuGet path uses upper casing). | |
| - Environment variable naming: use the existing platform-defined name where one exists (for example, `AzureWebJobsStorage`, `WEBSITE_RUN_FROM_PACKAGE`). When introducing new environment variables, use `UPPER_SNAKE_CASE` and document them in the sample's `README.md`. |
|
|
||
| ## Package Structure | ||
|
|
||
| This repo produces multiple NuGet packages (evidence: `README.md` NuGet Packages table): |
There was a problem hiding this comment.
This section cites README.md as evidence for the NuGet package list, but README.md currently documents only 3 packages. Please update the evidence/reference (e.g., point at the specific .csproj files that pack) or adjust the table so it aligns with what README.md states.
| This repo produces multiple NuGet packages (evidence: `README.md` NuGet Packages table): | |
| This repo produces multiple NuGet packages (evidence: the packable `*.csproj` projects under each listed source directory): |
| | `Microsoft.Azure.WebJobs.Extensions.DurableTask` | `src/WebJobs.Extensions.DurableTask/` | | ||
| | `Microsoft.Azure.WebJobs.Extensions.DurableTask.Analyzers` | `src/WebJobs.Extensions.DurableTask.Analyzers/` | | ||
| | `Microsoft.Azure.Functions.Worker.Extensions.DurableTask` | `src/Worker.Extensions.DurableTask/` | | ||
| | `Microsoft.Azure.WebJobs.Extensions.DurableTask.FunctionsScale` | `src/Microsoft.Azure.WebJobs.Extensions.DurableTask.FunctionsScale/` | | ||
| | `DurableFunctions.TypedInterfaces` | `src/DurableFunctions.TypedInterfaces/` | |
There was a problem hiding this comment.
The package table includes Microsoft.Azure.WebJobs.Extensions.DurableTask.FunctionsScale and DurableFunctions.TypedInterfaces, but these are not currently listed in README.md's "NuGet Packages" section. If these are intended to be first-class packages, consider updating README.md (or update this instructions file to avoid claiming the README as evidence).
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the MIT License. See LICENSE in the project root for license information. | ||
| ``` | ||
|
|
||
| > Note: This repo uses `.NET Foundation` (not `Microsoft Corporation`) as the copyright holder. This is the correct header for this repo. |
There was a problem hiding this comment.
This doc says every *.cs file must use the exact header that references LICENSE, but many existing source files (notably under src/Worker.Extensions.DurableTask/) use See License.txt ... instead. Since this is meant to be evidence-based, please either document both accepted header variants or clarify that new files should use the LICENSE variant while legacy files may still contain License.txt references.
| ### Sealed Private Classes | ||
|
|
||
| Private inner classes that do not serve as base classes must be declared `sealed` (evidence: `AsyncLock.cs` — `internal sealed class AsyncLock`; the `Releaser` struct inside it). |
There was a problem hiding this comment.
The "Sealed Private Classes" rule isn't consistent with the current codebase: there are many private class declarations that are not sealed (e.g., various *TriggerBinding implementations and helper types). Since the guidance is positioned as a repo convention, please adjust it to match existing practice (or narrow it to specific layers/projects where it actually applies).
| ### Sealed Private Classes | |
| Private inner classes that do not serve as base classes must be declared `sealed` (evidence: `AsyncLock.cs` — `internal sealed class AsyncLock`; the `Releaser` struct inside it). | |
| ### Sealed Helper Classes | |
| Where practical, prefer declaring `private` or `internal` helper classes that are not intended to be used as base classes as `sealed`, especially for low-level concurrency or infrastructure helpers (evidence: `AsyncLock.cs` — `internal sealed class AsyncLock` and its `Releaser` struct). |
Summary
Adds
.github/copilot-instructions.md— evidence-based coding conventions for GitHub Copilot and other AI tools working in this repo.What's included
this.prefix,Asyncsuffix,sealedprivate classes, explicit types (novar)net8.0/net10.0multi-targeting[Trait("Category", ...)]attributes for CI,AzureWebJobsStorageenv var requirementsamples/, no hardcoded secretsVerification
Risk
Low — adds a new file only, no existing files modified.