chore(build): centralize the shared build properties in Directory.Build.props

[tylerkron]

What was wrong

The repo had no Directory.Build.props, so all four project files declared the same three build settings by hand — and they had already drifted apart. Daqifi.Mcp, the MCP server we publish as a dotnet tool, was the only project in the repo without TreatWarningsAsErrors. Every other project, including Daqifi.Mcp's own test project, treats warnings as errors; the shipping tool did not, so a new compiler warning in DaqifiAgent.cs or Tools/DaqifiTools.cs would have built green and shipped. Nothing prevented the next drift either: each new project starts life as a copy-paste of the previous one's PropertyGroup, and any repo-wide decision has to be remembered in four places.

How it was fixed

ImplicitUsings, Nullable and TreatWarningsAsErrors now live once in a root Directory.Build.props, and the per-project copies are gone. That closes the Daqifi.Mcp gap as a side effect — it was already warning-clean in both Debug and Release, so nothing had to be fixed to turn the flag on. The CA2007 .editorconfig moves to the repo root as well, with its section changed from [*.cs] to [src/Daqifi.Core/**.cs] so the library keeps the rule and the test-project exemption documented in CONTRIBUTING.md survives verbatim; the root is simply where a repo-wide rule can now go (which is what #484 will need).

Two things a reviewer might expect and won't find, both deliberate:

• TargetFramework(s) is not centralized. Daqifi.Core multi-targets net9.0;net10.0 and Daqifi.Mcp is net9.0 only. That is a real asymmetry with a user-visible consequence (a .NET 10-only machine cannot run dotnet tool install -g Daqifi.Mcp without also installing .NET 9), but resolving it changes what the released package contains, which does not belong in a no-behavior-change cleanup. It is now filed as chore(build): decide whether Daqifi.Mcp should multi-target net9.0;net10.0 like Daqifi.Core #643 and referenced from a comment next to the TargetFramework line in both MCP projects, so it is a recorded decision rather than an omission.
• Package metadata is not centralized. Daqifi.Core sets license, project URL, repository URL, tags and SourceLink; Daqifi.Mcp sets none of them. Only Authors is genuinely common, so hoisting the rest would have silently rewritten the published Daqifi.Mcp package's metadata. Filed as chore(build): the published Daqifi.Mcp package has no license, project URL, repository URL, tags, or SourceLink #644.

Verification

• Before/after evaluated property sets are identical except for the one intended change. Dumped ~55 properties per project with dotnet msbuild <csproj> -getProperty:<Name> for all ten (project, target-framework) pairs, on origin/main and on this branch with restore warm on both sides, and diffed. The only difference anywhere is Daqifi.Mcp's TreatWarningsAsErrors: false → true.
• CA2007 scoping was tested, not assumed. Temporarily dropping one .ConfigureAwait(false) in TcpStreamTransport.cs fails the build with error CA2007, and the test projects — which contain plenty of naked awaits — still build clean.
• Full suite green on both frameworks: net9.0 (3820 Core + 217 MCP) and net10.0 (3820 Core). Release build of the solution: 0 warnings, 0 errors.
• The example CLI at daqifi-core-example-app builds and runs against this branch's Daqifi.Core, and streamed for 5 s from a real Nyquist over /dev/cu.usbmodem1101.

New tests (DirectoryBuildPropsTests) fail the build if any project redeclares a property that Directory.Build.props owns, which is the exact regression that produced this issue. They were confirmed to fail when the drift is reintroduced.

closes #638

---

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Centralize shared MSBuild properties via root Directory.Build.props

⚙️ Configuration changes 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Add root Directory.Build.props to share common build properties across all projects.
• Scope CA2007 rule to Daqifi.Core via root .editorconfig; keep tests exempt.
• Add tests to prevent per-project redeclaration and future config drift.

Diagram

graph TD
  Props["Directory.Build.props"] --> CoreProj["src/Daqifi.Core/Daqifi.Core.csproj"] --> CoreTestsProj["src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj"] --> DBPTests(["DirectoryBuildPropsTests"])
  Props --> McpProj["src/Daqifi.Mcp/Daqifi.Mcp.csproj"] --> McpTestsProj["src/Daqifi.Mcp.Tests/Daqifi.Mcp.Tests.csproj"]
  Editor[".editorconfig"] --> CoreProj
  DBPTests --> Props
  DBPTests --> CoreProj
  DBPTests --> McpProj

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Enforce via MSBuild target (Directory.Build.targets)

• ➕ Fails during build without requiring test execution
• ➕ Can emit precise MSBuild errors tied to project evaluation
• ➖ More MSBuild-specific complexity and harder to maintain than a simple test
• ➖ May be trickier to implement robustly across SDK/project styles

2. Use a dedicated shared props file imported explicitly by each project

• ➕ More explicit per-project control over what is shared
• ➕ Can be versioned/consumed outside the repo root if needed
• ➖ Reintroduces per-project wiring and drift risk (forgetting the import)
• ➖ Doesn't leverage MSBuild's built-in Directory.Build.props discovery

Recommendation: Current approach is a good fit: Directory.Build.props is the idiomatic place for repo-wide defaults, and the added guard tests provide an easy-to-understand backstop against future drift. If the team wants enforcement even when tests are skipped, consider a follow-up to add a lightweight MSBuild target that fails on redeclarations.

Files changed (8) +172 / -15

Tests (1) +115 / -0

DirectoryBuildPropsTests.csAdd tests preventing shared build-property drift +115/-0

Add tests preventing shared build-property drift

• Adds xUnit tests that parse Directory.Build.props to discover centralized property names and ensure no .csproj redeclares them. Includes guardrails to ensure the enumeration covers all expected projects and that TargetFramework(s) remains intentionally uncentralized.

src/Daqifi.Core.Tests/Build/DirectoryBuildPropsTests.cs

Documentation (1) +1 / -1

CONTRIBUTING.mdUpdate CA2007 documentation to reference root .editorconfig section +1/-1

Update CA2007 documentation to reference root .editorconfig section

• Updates contributor guidance to point at the new path-qualified section in the root .editorconfig. Keeps the documented rationale and test exemption intact.

CONTRIBUTING.md

Other (6) +56 / -14

.editorconfigMove analyzer config to repo root and scope CA2007 to Daqifi.Core +12/-3

Move analyzer config to repo root and scope CA2007 to Daqifi.Core

• Promotes .editorconfig to a repo-root file with root=true so repo-wide rules have a home. Changes the CA2007 section from applying to all *.cs to only src/Daqifi.Core/**.cs, preserving the documented test-project exemption.

.editorconfig

Directory.Build.propsAdd centralized repo-wide MSBuild properties +30/-0

Add centralized repo-wide MSBuild properties

• Introduces a root Directory.Build.props defining ImplicitUsings, Nullable, and TreatWarningsAsErrors for all projects. Explicitly documents why TargetFramework(s) is not centralized (tracked separately).

Directory.Build.props

Daqifi.Core.Tests.csprojRely on centralized build props and embed RepositoryRoot metadata +9/-3

Rely on centralized build props and embed RepositoryRoot metadata

• Removes per-project ImplicitUsings/Nullable/TreatWarningsAsErrors in favor of Directory.Build.props. Adds an AssemblyMetadata attribute capturing the repository root so build-configuration tests can locate files reliably.

src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj

Daqifi.Core.csprojRemove duplicated build properties and document Directory.Build.props usage +2/-3

Remove duplicated build properties and document Directory.Build.props usage

• Drops local ImplicitUsings/Nullable/TreatWarningsAsErrors settings now supplied by Directory.Build.props. Adds a comment clarifying the source of these shared properties.

src/Daqifi.Core/Daqifi.Core.csproj

Daqifi.Mcp.Tests.csprojRemove duplicated build properties and document TFM alignment +1/-3

Remove duplicated build properties and document TFM alignment

• Removes local ImplicitUsings/Nullable/TreatWarningsAsErrors settings now supplied by Directory.Build.props. Adds a note that the test project matches the MCP project's single target framework (tracked decision).

src/Daqifi.Mcp.Tests/Daqifi.Mcp.Tests.csproj

Daqifi.Mcp.csprojRemove duplicated build properties and record TFM decision comment +2/-2

Remove duplicated build properties and record TFM decision comment

• Removes local ImplicitUsings/Nullable settings now supplied by Directory.Build.props; TreatWarningsAsErrors is now effectively enabled for this project. Adds a comment calling out the deliberate single-target framework decision and its tracking issue.

src/Daqifi.Mcp/Daqifi.Mcp.csproj

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Case-sensitive property check ✓ Resolved 🐞 Bug ≡ Correctness

Description

DirectoryBuildPropsTests uses case-sensitive comparisons for MSBuild property names, so a project
can redeclare a centralized property using different casing and the test will not flag it, allowing
configuration drift to return unnoticed. This defeats the stated purpose of preventing per-project
overrides of Directory.Build.props values.

Code

src/Daqifi.Core.Tests/Build/DirectoryBuildPropsTests.cs[R37-43]

+    private static IReadOnlyList<string> CentralizedPropertyNames() =>
+        XDocument.Load(DirectoryBuildPropsPath)
+            .Descendants("PropertyGroup")
+            .Elements()
+            .Select(e => e.Name.LocalName)
+            .Distinct(StringComparer.Ordinal)
+            .ToList();

Relevance

●●● Strong

Recent accepted precedents favor case-insensitive identity checks; this is a deterministic MSBuild
correctness gap.

PR-#471
PR-#403

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test is explicitly meant to prevent project-local redeclarations from overriding centralized
properties, but it collects and compares property names using case-sensitive logic
(StringComparer.Ordinal and List.Contains), which can miss a redeclaration if casing differs.

src/Daqifi.Core.Tests/Build/DirectoryBuildPropsTests.cs[12-17]
src/Daqifi.Core.Tests/Build/DirectoryBuildPropsTests.cs[37-43]
src/Daqifi.Core.Tests/Build/DirectoryBuildPropsTests.cs[79-99]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DirectoryBuildPropsTests` intends to prevent projects from redeclaring properties that are centralized in `Directory.Build.props`. However, it builds the centralized property-name list with `StringComparer.Ordinal` and later checks offenders with `centralized.Contains`, which is case-sensitive.

MSBuild property names are case-insensitive, so `<TreatWarningsAsErrors>` and `<treatwarningsaserrors>` refer to the same property and the latter would still override the centralized value — but the current test would not report it.

### Issue Context
The test’s own documentation explicitly says it exists to stop per-project redeclarations from silently overriding shared values.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Build/DirectoryBuildPropsTests.cs[37-43]
- src/Daqifi.Core.Tests/Build/DirectoryBuildPropsTests.cs[79-95]

### Suggested fix
- Build `centralized` as a `HashSet<string>` with `StringComparer.OrdinalIgnoreCase` (or at minimum use `Distinct(StringComparer.OrdinalIgnoreCase)` and ensure the lookup is also ignore-case).
- When scanning each project’s declared properties, compare using the same ignore-case comparer.

Example approach:
- `var centralized = CentralizedPropertyNames().ToHashSet(StringComparer.OrdinalIgnoreCase);`
- In `CentralizedPropertyNames()`, use `Distinct(StringComparer.OrdinalIgnoreCase)` (or just return a `HashSet<string>` directly).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

Good catch on the case-sensitivity — conceded and fixed in 11fb0c8. MSBuild property names are case-insensitive, so <nullable> overrides the centralized <Nullable> exactly as an exact-case redeclaration would, and the ordinal comparison would have let it through. CentralizedPropertyNames() now returns a HashSet<string> built with StringComparer.OrdinalIgnoreCase, which the offender scan uses for its lookup too.

Verified rather than assumed: adding <nullable>enable</nullable> to Daqifi.Mcp.Tests.csproj now fails with src/Daqifi.Mcp.Tests/Daqifi.Mcp.Tests.csproj declares <nullable>, where before the fix it passed. Added CentralizedPropertyNames_AreMatchedCaseInsensitively to pin the comparer so it cannot regress to ordinal.

On the alternative approaches: agreed that a Directory.Build.targets check would fail earlier than a test. Not doing it here — it would add MSBuild machinery to a PR whose whole point is that the build config should be simple to read, and CI runs dotnet test on every PR so the guard does fire. Worth revisiting if the repo ever gains a build path that skips tests.

Full suite re-run green on both frameworks: net9.0 3821 Core + 217 MCP, net10.0 3821 Core.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 11fb0c8

[tylerkron]

Qodo round 2 is clean on head 11fb0c8: Bugs (0), Rule violations (0), the case-sensitivity finding marked Resolved, and no unresolved inline review threads. Re-checked both surfaces after a settle interval and they are still clean on the same SHA. CI green.

Ready for review — not merging.