From b072449fe0e2daf7f38fff353f9ef7f7dd405626 Mon Sep 17 00:00:00 2001 From: Josh Hufford Date: Tue, 22 Sep 2026 11:44:56 -0400 Subject: [PATCH] Limit NuGet lockfiles to shipped applications --- docs/foundation-checklist.md | 16 +- docs/quality/dependency-update-research.md | 124 +++ docs/quality/testing.md | 41 +- docs/setup/development.md | 52 +- .../OpenGameBuilder.AppHost.csproj | 3 - .../packages.linux-x64.lock.json | 704 ---------------- .../packages.win-x64.lock.json | 704 ---------------- tests/Directory.Build.props | 1 - .../packages.lock.json | 302 ------- .../packages.lock.json | 774 ------------------ 10 files changed, 190 insertions(+), 2531 deletions(-) create mode 100644 docs/quality/dependency-update-research.md delete mode 100644 src/OpenGameBuilder.AppHost/packages.linux-x64.lock.json delete mode 100644 src/OpenGameBuilder.AppHost/packages.win-x64.lock.json delete mode 100644 tests/OpenGameBuilder.Api.Client.Tests/packages.lock.json delete mode 100644 tests/OpenGameBuilder.Api.Tests/packages.lock.json diff --git a/docs/foundation-checklist.md b/docs/foundation-checklist.md index daaf8a0..9fdf265 100644 --- a/docs/foundation-checklist.md +++ b/docs/foundation-checklist.md @@ -406,21 +406,25 @@ This step does not block engine development or justify further deployment expans - [x] Replace accidental SDK drift from `global.json`'s `latestMinor` roll-forward with a deliberate supported baseline for formatting, analyzers, and builds. Log the selected SDK and update it through reviewed dependency changes. -- [x] Introduce committed NuGet lockfiles for application entry points and locked +- [x] Introduce committed NuGet lockfiles for shipped application entry points and locked CI restores after SDK selection is settled. Keep npm tools exactly pinned with committed lockfiles. Document how intentional dependency updates refresh them; a library lockfile does not constrain downstream consumers. **Acceptance:** a clean checkout runs the documented commands with the declared -toolchain. Missing prerequisites produce useful diagnostics, CI rejects dependency -drift, and local/CI checks have equivalent scope. A fresh editor rehearsal remains +toolchain. Missing prerequisites produce useful diagnostics, CI rejects missing +or stale shipped-application locks, and local/CI checks have equivalent scope. A fresh editor rehearsal remains the separate acceptance in step 5. See [NuGet lockfiles](https://learn.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#locking-dependencies). **Result (2026-09-22):** The read-only doctor and shared formatting, quick, full, and browser commands are documented in [development setup](setup/development.md) -and used by CI. SDK selection is exact; application and test entry points have -committed locks, including separate Windows/Linux AppHost graphs. Locked restores -also cover CodeQL and packaging. A fresh source snapshot passed the full local +and used by CI. SDK selection is exact; the shipped API and Web Client have +committed locks covering CI, CodeQL, and packaging. Tests and the local-only +AppHost restore normally and remain in the solution build/test gates; their +transitive dependency graphs are not frozen. This avoids maintaining custom +platform locks or a bot that repairs dependency PRs; see the +[dependency-update rationale](quality/dependency-update-research.md). +A fresh source snapshot passed the full local gate with serialized MSBuild: zero build warnings, 72 tests, frontend packaging, smoke-package checks, and all five shell suites. Missing prerequisites, incorrect versions, and missing/stale dependency locks were rejected. The diff --git a/docs/quality/dependency-update-research.md b/docs/quality/dependency-update-research.md new file mode 100644 index 0000000..ec36724 --- /dev/null +++ b/docs/quality/dependency-update-research.md @@ -0,0 +1,124 @@ +# Dependency-update lockfile research + +## Question and observed failure + +Researched on 2026-09-22. + +[PR #115](https://github.com/OpenGameBuilder/opengamebuilder/pull/115) updated +centrally managed OpenTelemetry versions, refreshed the API lock, +and left the API integration-test lock and the two AppHost RID-specific locks +stale. [Locked restore then failed](https://github.com/OpenGameBuilder/opengamebuilder/actions/runs/35745905146/job/106807351851). +A fresh regeneration of those three locks in a temporary copy of the CI merge +commit `a74bd33288f1c6d0c57ca72a99a3c0625f7c4fd4` +followed by locked restore succeeds for both the Windows and Linux-selected +AppHost graphs. The repository now uses a narrower policy that keeps committed +locks for the shipped API and Web Client while allowing tests and the local-only +AppHost to resolve dependencies normally. + +## Evidence + +NuGet documents that a lock captures the resolved dependency graph, including +changes from a dependent project's files, and that locked restore rejects an +inconsistent graph. Its default lock name is `packages.lock.json`; a project can +set `NuGetLockFilePath` or use `--lock-file-path` to select a different path. +[NuGet lock files](https://learn.microsoft.com/nuget/consume-packages/package-references-in-project-files#lock-file-extensibility) + +Dependabot issue [#13950](https://github.com/dependabot/dependabot-core/issues/13950) +is open and describes this exact Central Package Management plus +`ProjectReference` case: it updates some locks but not a downstream project's +lock, which makes `RestoreLockedMode=true` fail with NU1004. As of this research, +the issue has no resolution or linked fix. + +GitHub supports the `nuget` Dependabot ecosystem, but its public configuration +reference has no option to run a post-update restore, select a NuGet lock-file +path, or enumerate custom/RID lock files. +[Dependabot options reference](https://docs.github.com/en/code-security/reference/supply-chain-security/dependabot-options-reference) +This is evidence that there is no documented `dependabot.yml` remedy, rather +than proof that no internal behavior could change. + +The current Dependabot NuGet updater itself runs a forced restore for a project +when it refreshes a lock. +[LockFileUpdater.cs at 098c329](https://github.com/dependabot/dependabot-core/blob/098c32944988dba7a697eda66da30c2a62b1dff6/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/LockFileUpdater.cs) +That does not resolve #13950's incomplete downstream selection or establish +support for this repository's custom AppHost paths. + +Renovate documents NuGet `packages.lock.json` maintenance, central package +versions, and its source builds a `ProjectReference` dependency tree before +restoring dependent projects. +[Renovate NuGet manager](https://docs.renovatebot.com/modules/manager/nuget/) +[artifact updater at 3b70d5f](https://github.com/renovatebot/renovate/blob/3b70d5ffe2c2a0d5284f57d40fa687c751b56f0b/lib/modules/manager/nuget/artifacts.ts) +However, the same source derives only sibling `packages.lock.json` paths. +Therefore current source supports the test's ordinary lock more directly than +Dependabot, but does not establish support for `NuGetLockFilePath` or the +AppHost's `packages.win-x64.lock.json` and `packages.linux-x64.lock.json`. +This is a source-based inference, not a Renovate compatibility guarantee. + +## Implemented policy + +The lock policy is now limited to the shipped API and Web Client. Central +package versions, exact SDK selection, their committed lockfiles, and locked +packaging remain in place. Tests and the local-only AppHost no longer opt into +lockfiles, and their former locks are not retained. The AppHost remains in the +solution build, and both test projects still run in CI. + +When the solution is restored with `--locked-mode`, NuGet rejects a stale or +missing lock for the opted-in API and Web Client. Projects that do not opt into +lockfiles use normal dependency resolution during that same restore. The +repository does not suppress NU1004 or keep ignored development locks. + +The tradeoff is reduced repeatability for test and AppHost transitive dependency +resolution. Their resolved graphs can differ between restores and from the +shipped application graphs. Their builds and tests still run; production +dependency drift still fails locked restore. For this repository's current size, +that is a proportionate tradeoff and does not require a privileged +dependency-repair service. This policy is a project-specific judgment, not a +NuGet rule that test executables should never have lockfiles. + +This removes the lockfile failure patterns demonstrated by PR #115. It does not +guarantee that Dependabot will correctly update every future production lockfile +or that a package or SDK update will pass all checks. A failure in either area +still requires investigation. + +## Automation boundary + +No bot write-back automation is configured. Reconsider complete lock +regeneration only if reproducible test or orchestration graphs become a +demonstrated requirement, or if ordinary shipped-application locks repeatedly +need completion and the manual cost warrants an owned automation. Such a system +would add credential maintenance, changed-head handling, and CI-trigger +behavior. + +GitHub documents that PR events created by a workflow using `GITHUB_TOKEN` +require approval to run the resulting workflows; a GitHub App installation +token can trigger those checks automatically. +[Workflow triggering rules](https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/trigger-a-workflow#triggering-a-workflow-from-a-workflow) +Therefore fully unattended write-back would need an authentication or explicit +dispatch design beyond a lock-refresh script. CI validates the committed result +rather than silently repairing locks inside its workspace. + +## Validation evidence + +A fresh temporary copy of the exact PR #115 CI merge commit applied the narrowed +policy while leaving both shipped-application locks unchanged. On Windows, +`pwsh ./scripts/check.ps1 quick -Serial` passed its restore, C# formatting, +zero-warning Release build, and all 72 tests. Solution restore with +`--locked-mode` also passed with the default Windows SDK RID and with an +explicit `NETCoreSdkRuntimeIdentifier=linux-x64`; no development lockfiles were +regenerated. A deliberately mismatched API dependency still failed with NU1004, +confirming that the shipped-application guard remained active. + +Missing API and Web Client locks were each rejected without regenerating them, +and a stale API lock was rejected. This historical snapshot establishes the +policy's behavior for the failure that motivated the change. It is not a +Linux-host execution or a hosted Dependabot rehearsal. + +The implemented policy subsequently passed the repository's full local check, +including all 72 tests, frontend packaging, content and CI policy regressions, +and all five shell suites. The API and Web Client lockfiles were unchanged. + +## Limits + +This research does not claim that a GitHub App, Renovate deployment, or future +upstream release is configured or suitable here. Re-evaluate the decision when +Dependabot #13950 is resolved or when a dependency manager documents support +for custom NuGet lock paths and multi-RID regeneration. diff --git a/docs/quality/testing.md b/docs/quality/testing.md index 0e94411..c7ad51a 100644 --- a/docs/quality/testing.md +++ b/docs/quality/testing.md @@ -94,7 +94,11 @@ it cannot silently skip validation. The selector logs the paths and decision. | Documentation | Ubuntu runs `check.ps1 content`: first-party formatting, lint, workflow/shell checks, and all local Markdown links/anchors. Checking all documents catches backlinks broken by deletions or renames. No solution build or container runs. | | Full | Windows runs `check.ps1 quick -Serial` for locked restore, C# format, Release build, and tests. Ubuntu runs `check.ps1 full`, then builds and checks the API container. | -Both platforms build the AppHost with their committed platform-specific locks. +Both platforms restore and build the entire solution, including the AppHost, +and run both test projects. Only the shipped API and Web Client have committed +NuGet locks. Tests and the local-only AppHost resolve their dependencies normally; +their transitive graphs are not frozen. See the +[lockfile policy](../setup/development.md#command-line-workflow-start-here). The shared action installs the exact SDK in a fresh runner-temporary directory using [`DOTNET_INSTALL_DIR`](https://github.com/actions/setup-dotnet#environment-variables). This prevents preinstalled Visual Studio workload manifests from selecting an @@ -269,18 +273,37 @@ locked restore, format verification, a Release build with zero warnings, all 72 dependencies and syntax, and all five isolated shell suites. The serialized option avoided a Windows MSBuild task-host failure; it did not omit checks. -Both Windows and Linux AppHost graphs passed locked restore; the Linux graph was -selected explicitly on Windows, not executed on a Linux host. Deliberately -missing entry-point locks and changed package requirements stopped the shared -quick command at restore, before build. Doctor fixtures rejected a missing SDK, +Deliberately missing shipped-application locks and changed package requirements +stop restore before build. Doctor fixtures rejected a missing SDK, wrong SDK selection/policy, Node 24, and non-exact or mismatched Playwright pins. Missing smoke URL inputs failed before launching Chromium. PowerShell/YAML parsing, changed documentation targets/anchors, and diff checks passed. -The shared commands and workflow wiring have local evidence. Hosted `build-test`, -CodeQL, Docker image packaging, and live browser smoke after these changes remain -unverified. No services, deployments, certificate trust changes, or editor -rehearsals were performed for this validation. +These were local command checks. Later hosted platform evidence is recorded in +[platform CI acceptance](#recorded-platform-ci-acceptance). No services, +deployments, certificate trust changes, or editor rehearsals were performed for +this local validation. + +### Shipped-application lock policy validation + +The narrower policy was tested against a fresh copy of the failing +[PR #115 CI revision](https://github.com/OpenGameBuilder/opengamebuilder/actions/runs/35745905146/job/106807351851) +with its OpenTelemetry 1.19.1 update. Removing the test and AppHost lock opt-ins +and their four locks made solution restore pass with both Windows and +Linux-selected SDK RIDs. These restores ran on Windows; they are not Linux-host +execution evidence. The same snapshot passed `check.ps1 quick -Serial`, including +format verification, a Release build with zero warnings, and all 72 tests. + +The implementation also passed `check.ps1 full -Serial` locally: the solution +gate, content checks and regressions, frontend publish/portability checks, +smoke-package checks, and all five isolated shell suites. Both committed +production lockfiles remained unchanged. + +Missing API and Web Client locks were each rejected before restore could +regenerate them. A deliberately stale API dependency was rejected with NU1004. +No test or AppHost lockfiles were regenerated. These checks verify that the +shipped applications retain their lock guards while development graphs restore +normally; they do not guarantee that every future Dependabot update succeeds. ### Browser-smoke dependency updates diff --git a/docs/setup/development.md b/docs/setup/development.md index 13688ff..9a46e05 100644 --- a/docs/setup/development.md +++ b/docs/setup/development.md @@ -43,9 +43,9 @@ access are not required.** Aspire is the local launcher, not the production deployment mechanism. When updating the SDK requirement in `global.json`, review the compatible pinned -SDK image in the API Dockerfile and the generated dependency lockfiles together. -Dependabot SDK updates are reviewed with those files; CI builds the API image -without pushing it. +SDK image in the API Dockerfile and the two shipped-application dependency +lockfiles together. Dependabot SDK updates are reviewed with those files; CI +builds the API image without pushing it. ## Command-line workflow (start here) @@ -65,10 +65,13 @@ Use `-Scope quick`, `full`, `content`, `format`, `browser`, or `development` whe workflow. The normal solution gate is `pwsh ./scripts/check.ps1 quick`: it runs the quick -doctor check, a locked restore, C# formatting verification, a Release build, and -the current 72 solution tests. `check.ps1 format` verifies C# after locked restore -and checks first-party content with Prettier and shfmt. To apply formatter changes -deliberately, run `pwsh ./scripts/check.ps1 format -Fix`, then review the diff. +doctor check, a solution restore with `--locked-mode`, C# formatting +verification, a Release build, and the current 72 solution tests. That restore +enforces the committed API and Web Client graphs; tests and the local-only +AppHost do not opt into lockfiles and resolve dependencies normally. +`check.ps1 format` verifies C# after the same restore and checks first-party +content with Prettier and shfmt. To apply formatter changes deliberately, run +`pwsh ./scripts/check.ps1 format -Fix`, then review the diff. Install the [content-checking prerequisites](../quality/content-checks.md) before using `format`, `content`, or `full`: @@ -102,36 +105,29 @@ test project, replace `--solution opengamebuilder.slnx` with, for example, Direct `dotnet restore`, `build`, `test`, and `format` commands remain useful for focused editor work. Use `--locked-mode` for a manual restore that must reproduce -the committed graph. After an intentional SDK or +the shipped applications' committed graphs. After an intentional SDK or [`Directory.Packages.props`](../../Directory.Packages.props) change, refresh -locks with: +those locks with: ```pwsh dotnet restore opengamebuilder.slnx --force-evaluate -p:RestoreLockedMode=false ``` -Review every generated lockfile and run the full check before committing. See +Review any lockfile changes and run the full check before committing. See [NuGet's lock-file documentation](https://learn.microsoft.com/nuget/consume-packages/package-references-in-project-files#locking-dependencies) -for the restore model. The API, Web Client, and two test entry points commit +for the restore model. The shipped API and Web Client entry points each commit `packages.lock.json`; shared-library locks cannot constrain the graph selected by a downstream consuming application, so shared libraries do not duplicate -them. The AppHost's implicit SDK packages vary by host, so it commits reviewed -`packages.win-x64.lock.json` and `packages.linux-x64.lock.json` baselines. For an -intentional dependency refresh, update the native graph with the solution command -above, then refresh the Linux AppHost graph and confirm the native graph remains -locked: - -```pwsh -dotnet restore src/OpenGameBuilder.AppHost/OpenGameBuilder.AppHost.csproj --force-evaluate -p:RestoreLockedMode=false -p:NETCoreSdkRuntimeIdentifier=linux-x64 -m:1 -dotnet restore opengamebuilder.slnx --locked-mode -``` - -Refresh each AppHost lock on its matching host, or review an explicit -cross-target restore as above. A new host platform needs its own reviewed -AppHost lock before it is supported. `Directory.Build.targets` rejects a missing -entry-point lock before a locked restore can create one; `--locked-mode` then -rejects stale dependency graphs. CI restores with `--locked-mode`, and packaging's -implicit restore is locked as well. +them. Tests and the local-only AppHost use normal dependency resolution, so +their transitive graphs can change between restores. This gives up fixed +development-only graphs while retaining Central Package Management, exact SDK +selection, and locked shipped-application restore and packaging. + +[`Directory.Build.targets`](../../Directory.Build.targets) rejects a missing +lock for an opted-in project before a locked restore can create one; +`--locked-mode` then rejects stale API or Web Client graphs. CI restores the +whole solution with `--locked-mode`, and packaging's implicit restore is locked +as well. No host-specific AppHost lock or lock refresh is required. The root `NuGet.Config` deliberately has one source, `nuget.org`, and clears both inherited package sources and inherited package-source mappings. Its `*` diff --git a/src/OpenGameBuilder.AppHost/OpenGameBuilder.AppHost.csproj b/src/OpenGameBuilder.AppHost/OpenGameBuilder.AppHost.csproj index 3b9471b..a20a942 100644 --- a/src/OpenGameBuilder.AppHost/OpenGameBuilder.AppHost.csproj +++ b/src/OpenGameBuilder.AppHost/OpenGameBuilder.AppHost.csproj @@ -3,9 +3,6 @@ - true - - packages.$(NETCoreSdkRuntimeIdentifier).lock.json Exe true