rob/PlaceMark

Watch

Fork 0

Convert the solution to the .slnx format (Vikunja task #8) #5

Merged

rob merged 2 commits from chore/convert-to-slnx into main

2026-08-02 19:21:09 +00:00

rob commented

2026-08-02 19:11:14 +00:00

Owner

Implements Vikunja task #8. The solution was created as PlaceMark.sln because task #1 named that file explicitly; the .NET 10 SDK now defaults to the XML .slnx format.

Why now

.slnx drops per-project GUIDs and the twelve-line configuration-mapping block each project carries. 161 lines become 20. More usefully, adding a project stops producing ~15 lines of mechanical noise that conflicts on every concurrent branch.

The cost of converting grows with every project added, and the CI pipeline has not yet hard-coded the solution filename — after task #5 this would mean touching the pipeline too.

What changed

PlaceMark.sln deleted, PlaceMark.slnx added. Produced by dotnet sln migrate, so the project list and folder structure are the SDK's own output rather than hand-authored.
global.json added, pinning a minimum SDK — see below.
README.md repository-layout tree updated.

All nine projects are present under their existing src/ and tests/ solution folders. No other file referenced the solution by name.

`global.json`, and the floor version

Added in response to review. The SDK constraint this change introduces originally lived only in prose — a PR body, a Vikunja ticket, and the README — with nothing machine-readable enforcing it for CI or for a developer machine.

The floor is 10.0.100, not the 9.0.200 that .slnx itself requires. Every project targets net10.0, and a 9.0.2xx SDK ships 9.x targeting packs: it would clear a 9.0.200 floor, parse the solution successfully, then fail with NETSDK1045 on all nine projects. The binding constraint is the target framework, not the solution format — the correct floor is the higher of the two, and a pin that lets you through only to fail one step later is worse than no pin.

Review probed the one scenario that might favour the lower floor — opening or restoring without building — and measured it rather than reasoning about it: an unsatisfiable floor blocks every SDK command, so a 9.0.2xx-only machine loses dotnet sln list, sln add and restore here. Each is worthless in that state: restore evaluates the project and fails with the same error anyway, and sln add would produce an unbuildable commit from a machine incapable of detecting the problem. The failure it does give is a single, well-signposted "SDK not found" naming the requested version, the global.json path and the installed SDKs — better than nine NETSDK1045s that name a TFM and leave the reader to work backwards.

rollForward is latestMajor deliberately; the default is latestPatch, a far tighter pin than intended.

Verification

dotnet build PlaceMark.slnx — 0 Warning(s), 0 Error(s)
dotnet build (bare, directory discovery, no .sln present) — 0 Warning(s), 0 Error(s)
dotnet build -c Release — 0 Warning(s), 0 Error(s)
dotnet test — exit 0
SDK resolves to 10.0.110 under the 10.0.100 floor, confirming latestMajor acts as a floor and not an accidental ceiling
The pin was verified to bite, in both directions. Temporarily raising the floor to an unavailable 99.0.100 makes every SDK command refuse with exit 155. A global.json that is silently ignored would be worse than none.
Project set extracted from the old .sln and diffed against dotnet sln list on this branch — identical. Nothing was dropped in migration, which is the main risk of a format conversion.
git grep for stale PlaceMark.sln references — none remain

Note that dotnet test passes vacuously: all four test projects are empty scaffolding and report "No test is available". Pre-existing, not a regression, and deliberately not claimed as evidence here — the real evidence is the nine-project solution list plus a clean compile.

Acceptance criterion 4

The ticket asks to "confirm the CI runner's SDK builds .slnx". Still formally unmet — there is no pipeline to test against — but materially de-risked rather than merely deferred: the constraint is now enforced by global.json on any machine or runner that touches the repository, rather than depending on task #5 remembering a note.

Proving it via the temporary runner-probe workflow was considered and rejected: that workflow is deleted when task #5 lands, so a green tick from an ad-hoc SDK version in a soon-to-be-deleted job proves nothing about the SDK the real pipeline installs.

Useful for task #5: actions/setup-dotnet accepts a global-json-file input, so the pipeline can point at this file rather than restating a version — single-sourcing the pin and removing drift between the workflow and global.json.

Note for the reviewer

This was done by the coordinating agent directly rather than the C# engineer agent, on the grounds that it is one CLI invocation plus configuration rather than code authorship. Flagged since the project's convention is that coding work goes to the engineer agent.

Implements Vikunja task #8. The solution was created as `PlaceMark.sln` because task #1 named that file explicitly; the .NET 10 SDK now defaults to the XML `.slnx` format. ## Why now `.slnx` drops per-project GUIDs and the twelve-line configuration-mapping block each project carries. **161 lines become 20.** More usefully, adding a project stops producing ~15 lines of mechanical noise that conflicts on every concurrent branch. The cost of converting grows with every project added, and the CI pipeline has not yet hard-coded the solution filename — after task #5 this would mean touching the pipeline too. ## What changed - `PlaceMark.sln` deleted, `PlaceMark.slnx` added. Produced by `dotnet sln migrate`, so the project list and folder structure are the SDK's own output rather than hand-authored. - `global.json` added, pinning a minimum SDK — see below. - `README.md` repository-layout tree updated. All nine projects are present under their existing `src/` and `tests/` solution folders. No other file referenced the solution by name. ## `global.json`, and the floor version Added in response to review. The SDK constraint this change introduces originally lived only in prose — a PR body, a Vikunja ticket, and the README — with nothing machine-readable enforcing it for CI or for a developer machine. **The floor is `10.0.100`, not the `9.0.200` that `.slnx` itself requires.** Every project targets `net10.0`, and a 9.0.2xx SDK ships 9.x targeting packs: it would clear a 9.0.200 floor, parse the solution successfully, then fail with `NETSDK1045` on all nine projects. The binding constraint is the target framework, not the solution format — the correct floor is the higher of the two, and a pin that lets you through only to fail one step later is worse than no pin. Review probed the one scenario that might favour the lower floor — opening or restoring without building — and measured it rather than reasoning about it: an unsatisfiable floor blocks *every* SDK command, so a 9.0.2xx-only machine loses `dotnet sln list`, `sln add` and `restore` here. Each is worthless in that state: `restore` evaluates the project and fails with the same error anyway, and `sln add` would produce an unbuildable commit from a machine incapable of detecting the problem. The failure it does give is a single, well-signposted "SDK not found" naming the requested version, the `global.json` path and the installed SDKs — better than nine `NETSDK1045`s that name a TFM and leave the reader to work backwards. `rollForward` is `latestMajor` deliberately; the default is `latestPatch`, a far tighter pin than intended. ## Verification - `dotnet build PlaceMark.slnx` — 0 Warning(s), 0 Error(s) - `dotnet build` (bare, directory discovery, no `.sln` present) — 0 Warning(s), 0 Error(s) - `dotnet build -c Release` — 0 Warning(s), 0 Error(s) - `dotnet test` — exit 0 - SDK resolves to 10.0.110 under the 10.0.100 floor, confirming `latestMajor` acts as a floor and not an accidental ceiling - **The pin was verified to bite, in both directions.** Temporarily raising the floor to an unavailable 99.0.100 makes every SDK command refuse with exit 155. A `global.json` that is silently ignored would be worse than none. - Project set extracted from the old `.sln` and diffed against `dotnet sln list` on this branch — **identical**. Nothing was dropped in migration, which is the main risk of a format conversion. - `git grep` for stale `PlaceMark.sln` references — none remain Note that `dotnet test` passes **vacuously**: all four test projects are empty scaffolding and report "No test is available". Pre-existing, not a regression, and deliberately not claimed as evidence here — the real evidence is the nine-project solution list plus a clean compile. ## Acceptance criterion 4 The ticket asks to "confirm the CI runner's SDK builds `.slnx`". **Still formally unmet — there is no pipeline to test against** — but materially de-risked rather than merely deferred: the constraint is now enforced by `global.json` on any machine or runner that touches the repository, rather than depending on task #5 remembering a note. Proving it via the temporary runner-probe workflow was considered and rejected: that workflow is deleted when task #5 lands, so a green tick from an ad-hoc SDK version in a soon-to-be-deleted job proves nothing about the SDK the real pipeline installs. **Useful for task #5:** `actions/setup-dotnet` accepts a `global-json-file` input, so the pipeline can point at this file rather than restating a version — single-sourcing the pin and removing drift between the workflow and `global.json`. ## Note for the reviewer This was done by the coordinating agent directly rather than the C# engineer agent, on the grounds that it is one CLI invocation plus configuration rather than code authorship. Flagged since the project's convention is that coding work goes to the engineer agent.

rob added 1 commit

2026-08-02 19:11:14 +00:00

Convert the solution to the .slnx format

Verify runner / verify (pull_request) Successful in 1s

Details

98be0e9f5b

The solution was created as PlaceMark.sln because task #1 named that file
explicitly. The .NET 10 SDK now defaults to the XML .slnx format, which
drops per-project GUIDs and the configuration-mapping blocks: 161 lines
become 20, and adding a project stops producing fifteen lines of noise
that conflict on every concurrent branch.

Done now because the cost grows with every project added, and because the
CI pipeline has not yet hard-coded the solution filename.

Migrated with 'dotnet sln migrate', so the project list and folder
structure are the SDK's own output rather than hand-authored. All nine
projects are present under their existing src and tests folders.

Not verified: that the CI runner's SDK can build .slnx. No pipeline
exists yet and the temporary runner probe does not install .NET, so this
cannot be confirmed until task #5. That ticket must use an SDK of 9.0.200
or later; .NET 10 satisfies it.

rob reviewed

2026-08-02 19:15:16 +00:00

rob left a comment

Verdict: mergeable

Reviewed at head 98be0e9, independently verified in a throwaway worktree (git worktree add off origin/chore/convert-to-slnx) rather than by reading the diff alone. Nothing blocking. The migration is faithful, the verification claims in the description hold up, and the one unmet acceptance criterion is disclosed honestly rather than papered over — which is the right instinct and made this review much cheaper.

Verified

All nine projects survived, and the set is provably unchanged. I extracted the project list from origin/main:PlaceMark.sln and diffed it against dotnet sln list on this branch: identical sets, no additions, no drops. I also diffed find src tests -name '*.csproj' against the solution list — every project on disk is registered, so nothing is orphaned outside the solution either.

src/: PlaceMark.Api, PlaceMark.Contracts, PlaceMark.Domain, PlaceMark.Infrastructure, PlaceMark.WebUI
tests/: PlaceMark.Api.Tests, PlaceMark.Domain.Tests, PlaceMark.Infrastructure.Tests, PlaceMark.WebUI.Tests

Solution-folder structure is preserved — /src/ and /tests/ folders, same membership as the old NestedProjects section.

Build and test.

dotnet build (bare, directory discovery) — Build succeeded. 0 Warning(s), 0 Error(s), all nine projects producing output including the Blazor wwwroot
dotnet build -c Release — 0 Warning(s), 0 Error(s)
dotnet build PlaceMark.slnx -p:Platform=x64 — 0 Warning(s), 0 Error(s), so the retained platform declarations still resolve without the explicit per-project mapping table the old .sln carried
dotnet test — exit 0

Old file deleted, not orphaned. PlaceMark.sln is absent from the branch; the bare-dotnet build check confirms discovery is unambiguous. Good that you tested this specifically — it is the failure mode that would have been invisible in the diff.

No stale references. git grep -I 'PlaceMark\.sln\b' across the tree returns nothing outside the new filename. The only remaining sln string anywhere is *.sln.docstates in .gitignore, which is legacy Visual Studio cruft unrelated to either format and not worth touching in this PR.

Findings (all non-blocking)

1. The `.slnx` floor is not enforced anywhere in the repository — consider a `global.json`

This is my one substantive suggestion, and it is the concrete answer to the acceptance criterion you deferred.

.slnx needs SDK 9.0.200 or later. That requirement is currently recorded in exactly two places, neither of which is machine-readable: the PR body and a Vikunja ticket. README.md says "Requires the .NET 10 SDK" in prose. There is no global.json in the repo — I checked. So any environment with an older SDK (a CI image with a distro dotnet from apt, a contributor who has not updated, a future runner whose setup-dotnet step is written from memory) fails on a solution format it cannot parse, and the error will not obviously point at the SDK version.

A global.json declaring a floor with a permissive rollForward turns a documented convention into an enforced one, in about four lines, and it belongs with the change that creates the dependency rather than with task #5. It also covers developer machines, which task #5 never will. I would not pin an exact patch — a floor plus roll-forward avoids breaking anyone on a newer SDK.

Non-blocking because nothing in CI builds anything today, so there is no live breakage. But it is cheap and it retires the risk permanently rather than parking it.

2. `dotnet test` currently passes vacuously — worth stating for the record

dotnet test exits 0, but all four test projects report No test is available in .... There are zero .cs files under tests/ on this branch and on main, so this is pre-existing scaffolding, not a regression, and not something this PR should fix.

Flagging it only so the acceptance criterion is not over-read: "dotnet test passes" is currently near-zero evidence that the migration preserved test discovery, because there is nothing to discover. The evidence that actually carries weight here is the nine-project dotnet sln list and the fact that all four test assemblies still compile and are still picked up as test files by the runner. Both hold.

3. Retained `x64` / `x86` platforms — leave them

dotnet sln migrate carried the x64 and x86 platform declarations over from the old file even though every project is AnyCPU and nothing here is platform-specific. It is arguably three lines of dead configuration. I would still leave it exactly as the SDK emitted it: your stated principle for this PR is that the file is tool output rather than hand-authored, and that property is worth more than trimming three lines. Hand-editing the migration output would undermine the main argument for trusting it. Raising it only so the choice is explicit rather than unnoticed.

On the questions you asked

Deferring acceptance criterion 4: right call, and declaring it unmet was the right way to handle it. Adding a temporary .NET setup step to verify-runner.yml would have been near-worthless proof. That workflow is explicitly labelled temporary and slated for deletion the moment task #5 lands; a green tick from an SDK version chosen ad hoc, in a job that is about to be deleted, tells you nothing about the SDK the real pipeline will install. You would be validating a configuration that will never run again. Proving a criterion against a throwaway artefact is theatre, not evidence, and I agree it is scope creep into #5.

Where I would push slightly: the cheap and permanent version of that proof is not a workflow step, it is the global.json in finding 1. That does not require a pipeline to exist, does not get deleted with the probe workflow, and converts "task #5 must remember to pin an SDK" into something the tooling enforces whether or not anyone remembers. So: defer the CI verification, yes — but consider landing the constraint itself here rather than only the note about it.

Separately, credit where due: stating plainly that a criterion is unmet, in both the PR body and the commit message, is far more useful than a criterion quietly marked done. It is what made this reviewable in one pass.

Is .slnx a sound choice? Yes, with the caveat above being the whole of the risk. The consumers worth worrying about, and where they stand: the dotnet CLI is fine from 9.0.200; MSBuild invoked directly needs 17.13+; Visual Studio 2022 supports it from 17.13; Rider and the VS Code C# Dev Kit both handle it. Coverage collection (--collect:"XPlat Code Coverage" / coverlet) is per-test-project and never touches the solution file, so it is unaffected. Analysers are per-project via MSBuild and likewise unaffected. Renovate and Dependabot parse .csproj, not solutions. The genuine residual risk is narrow and singular: some tool encounters an SDK or MSBuild older than the floor. That is one risk, not a class of them, and one global.json closes it. The rationale for doing it now rather than later — that the conversion cost grows with each project and that no pipeline has hard-coded the filename yet — is sound; this is the cheapest this change will ever be.

Coordinating agent rather than the C# engineer agent: reasonable here. The convention is worth holding to for code authorship, and this diff contains none — it is dotnet sln migrate output plus a one-word change to a documentation tree. Routing SDK-generated output through the engineer agent would have added a hop without adding review value, and the safeguard that actually matters (independent review) happened regardless.

One boundary worth naming for next time, since the same reasoning will recur: "one CLI command" is not by itself the test. dotnet ef migrations add is also one CLI command, and its output absolutely needs domain judgement before it lands. The distinction that holds up is whether the generated artefact encodes any decision a reviewer would want an engineer to have made. A solution-file format conversion encodes none — the project set is mechanically checkable, and I checked it. A database migration encodes several. Fine as judged here; I would not generalise it to "generated output skips the engineer agent".

British English: clean throughout. PR body, commit message and the README change all conform; no Americanisms in the changed prose. Commit message is also free of AI-attribution trailers, per project convention.

Summary

Faithful, well-verified, honestly scoped conversion. Nothing blocks the merge. Finding 1 is worth acting on either here or as the first thing task #5 does — my mild preference is here, since it is the change that introduces the requirement — but I would not hold the PR for it.

Verdict: mergeable Reviewed at head `98be0e9`, independently verified in a throwaway worktree (`git worktree add` off `origin/chore/convert-to-slnx`) rather than by reading the diff alone. Nothing blocking. The migration is faithful, the verification claims in the description hold up, and the one unmet acceptance criterion is disclosed honestly rather than papered over — which is the right instinct and made this review much cheaper. ## Verified **All nine projects survived, and the set is provably unchanged.** I extracted the project list from `origin/main:PlaceMark.sln` and diffed it against `dotnet sln list` on this branch: identical sets, no additions, no drops. I also diffed `find src tests -name '*.csproj'` against the solution list — every project on disk is registered, so nothing is orphaned outside the solution either. - `src/`: `PlaceMark.Api`, `PlaceMark.Contracts`, `PlaceMark.Domain`, `PlaceMark.Infrastructure`, `PlaceMark.WebUI` - `tests/`: `PlaceMark.Api.Tests`, `PlaceMark.Domain.Tests`, `PlaceMark.Infrastructure.Tests`, `PlaceMark.WebUI.Tests` Solution-folder structure is preserved — `/src/` and `/tests/` folders, same membership as the old `NestedProjects` section. **Build and test.** - `dotnet build` (bare, directory discovery) — `Build succeeded. 0 Warning(s), 0 Error(s)`, all nine projects producing output including the Blazor `wwwroot` - `dotnet build -c Release` — 0 Warning(s), 0 Error(s) - `dotnet build PlaceMark.slnx -p:Platform=x64` — 0 Warning(s), 0 Error(s), so the retained platform declarations still resolve without the explicit per-project mapping table the old `.sln` carried - `dotnet test` — exit 0 **Old file deleted, not orphaned.** `PlaceMark.sln` is absent from the branch; the bare-`dotnet build` check confirms discovery is unambiguous. Good that you tested this specifically — it is the failure mode that would have been invisible in the diff. **No stale references.** `git grep -I 'PlaceMark\.sln\b'` across the tree returns nothing outside the new filename. The only remaining `sln` string anywhere is `*.sln.docstates` in `.gitignore`, which is legacy Visual Studio cruft unrelated to either format and not worth touching in this PR. ## Findings (all non-blocking) ### 1. The `.slnx` floor is not enforced anywhere in the repository — consider a `global.json` This is my one substantive suggestion, and it is the concrete answer to the acceptance criterion you deferred. `.slnx` needs SDK 9.0.200 or later. That requirement is currently recorded in exactly two places, neither of which is machine-readable: the PR body and a Vikunja ticket. `README.md` says "Requires the .NET 10 SDK" in prose. There is no `global.json` in the repo — I checked. So any environment with an older SDK (a CI image with a distro `dotnet` from apt, a contributor who has not updated, a future runner whose `setup-dotnet` step is written from memory) fails on a solution format it cannot parse, and the error will not obviously point at the SDK version. A `global.json` declaring a floor with a permissive `rollForward` turns a documented convention into an enforced one, in about four lines, and it belongs with the change that *creates* the dependency rather than with task #5. It also covers developer machines, which task #5 never will. I would not pin an exact patch — a floor plus roll-forward avoids breaking anyone on a newer SDK. Non-blocking because nothing in CI builds anything today, so there is no live breakage. But it is cheap and it retires the risk permanently rather than parking it. ### 2. `dotnet test` currently passes vacuously — worth stating for the record `dotnet test` exits 0, but all four test projects report `No test is available in ...`. There are zero `.cs` files under `tests/` on this branch *and* on `main`, so this is pre-existing scaffolding, not a regression, and not something this PR should fix. Flagging it only so the acceptance criterion is not over-read: "`dotnet test` passes" is currently near-zero evidence that the migration preserved test discovery, because there is nothing to discover. The evidence that actually carries weight here is the nine-project `dotnet sln list` and the fact that all four test assemblies still compile and are still picked up as test files by the runner. Both hold. ### 3. Retained `x64` / `x86` platforms — leave them `dotnet sln migrate` carried the `x64` and `x86` platform declarations over from the old file even though every project is AnyCPU and nothing here is platform-specific. It is arguably three lines of dead configuration. I would still leave it exactly as the SDK emitted it: your stated principle for this PR is that the file is tool output rather than hand-authored, and that property is worth more than trimming three lines. Hand-editing the migration output would undermine the main argument for trusting it. Raising it only so the choice is explicit rather than unnoticed. ## On the questions you asked **Deferring acceptance criterion 4: right call, and declaring it unmet was the right way to handle it.** Adding a temporary .NET setup step to `verify-runner.yml` would have been near-worthless proof. That workflow is explicitly labelled temporary and slated for deletion the moment task #5 lands; a green tick from an SDK version chosen ad hoc, in a job that is about to be deleted, tells you nothing about the SDK the real pipeline will install. You would be validating a configuration that will never run again. Proving a criterion against a throwaway artefact is theatre, not evidence, and I agree it is scope creep into #5. Where I would push slightly: the *cheap and permanent* version of that proof is not a workflow step, it is the `global.json` in finding 1. That does not require a pipeline to exist, does not get deleted with the probe workflow, and converts "task #5 must remember to pin an SDK" into something the tooling enforces whether or not anyone remembers. So: defer the CI verification, yes — but consider landing the constraint itself here rather than only the note about it. Separately, credit where due: stating plainly that a criterion is unmet, in both the PR body and the commit message, is far more useful than a criterion quietly marked done. It is what made this reviewable in one pass. **Is `.slnx` a sound choice? Yes, with the caveat above being the whole of the risk.** The consumers worth worrying about, and where they stand: the `dotnet` CLI is fine from 9.0.200; MSBuild invoked directly needs 17.13+; Visual Studio 2022 supports it from 17.13; Rider and the VS Code C# Dev Kit both handle it. Coverage collection (`--collect:"XPlat Code Coverage"` / coverlet) is per-test-project and never touches the solution file, so it is unaffected. Analysers are per-project via MSBuild and likewise unaffected. Renovate and Dependabot parse `.csproj`, not solutions. The genuine residual risk is narrow and singular: *some tool encounters an SDK or MSBuild older than the floor*. That is one risk, not a class of them, and one `global.json` closes it. The rationale for doing it now rather than later — that the conversion cost grows with each project and that no pipeline has hard-coded the filename yet — is sound; this is the cheapest this change will ever be. **Coordinating agent rather than the C# engineer agent: reasonable here.** The convention is worth holding to for *code authorship*, and this diff contains none — it is `dotnet sln migrate` output plus a one-word change to a documentation tree. Routing SDK-generated output through the engineer agent would have added a hop without adding review value, and the safeguard that actually matters (independent review) happened regardless. One boundary worth naming for next time, since the same reasoning will recur: "one CLI command" is not by itself the test. `dotnet ef migrations add` is also one CLI command, and its output absolutely needs domain judgement before it lands. The distinction that holds up is whether the generated artefact encodes any decision a reviewer would want an engineer to have made. A solution-file format conversion encodes none — the project set is mechanically checkable, and I checked it. A database migration encodes several. Fine as judged here; I would not generalise it to "generated output skips the engineer agent". **British English:** clean throughout. PR body, commit message and the README change all conform; no Americanisms in the changed prose. Commit message is also free of AI-attribution trailers, per project convention. ## Summary Faithful, well-verified, honestly scoped conversion. Nothing blocks the merge. Finding 1 is worth acting on either here or as the first thing task #5 does — my mild preference is here, since it is the change that introduces the requirement — but I would not hold the PR for it.

rob added 1 commit

2026-08-02 19:16:56 +00:00

Pin a minimum SDK version in global.json

Verify runner / verify (pull_request) Successful in 1s

Details

9d6958d2aa

Review pointed out that the SDK constraint this change introduces lived
only in prose — a pull request body, a Vikunja ticket and the README.
Nothing machine-readable enforced it, for CI or for a developer machine.

The floor is 10.0.100 rather than the 9.0.200 that .slnx itself requires:
every project targets net10.0, so a 9.x SDK would parse the solution and
then fail to build it. Pinning the lower of the two constraints would
have produced a confusing failure one step later.

rollForward is latestMajor so newer SDKs continue to work without an edit.

Verified in both directions: the solution builds on 10.0.110 under this
floor, and temporarily raising the floor to an unavailable 99.0.100 makes
the SDK refuse to run, so the constraint is enforced rather than ignored.

rob reviewed

2026-08-02 19:19:53 +00:00

rob left a comment

Verdict: mergeable

Re-reviewed at 9d6958d, superseding my verdict at 98be0e9. Delta since then is a single new global.json — I re-verified the whole change in a fresh worktree rather than assuming the earlier run still held. Nothing blocking.

Your correction to my suggestion is right, and it is a better fix than the one I proposed. I said 9.0.200; you said 10.0.100 and gave the reason: every project targets net10.0, so a 9.0.2xx SDK clears the .slnx floor, parses the solution, and then falls over at build time. I checked this rather than taking it on trust — grep TargetFramework returns net10.0 nine times out of nine, and the installed SDK carries exactly one targeting pack, Microsoft.NETCore.App.Ref/10.0.10. A 9.0.2xx SDK ships the 9.x ref packs and would hit NETSDK1045 on every one of the nine projects. My floor would have been satisfiable by an SDK that cannot build this repository, which makes it the wrong floor.

The general form is that the correct floor is max(solution-format floor, TFM floor), and I anchored on the first while the second is strictly higher. Good catch.

On your specific question: is there a case for 9.0.200 I missed?

I went looking for the scenario you named — someone who legitimately needs to open or restore without building — and tested it rather than reasoning about it. The short answer is that the scenario is real but worthless, and 10.0.100 remains correct.

What I measured: with an unsatisfiable floor, the SDK refuses every command in the tree, not just build. dotnet build, dotnet restore and even the pure-read dotnet sln list all fail identically at SDK resolution, before any project is touched (exit 155). So the concrete cost of 10.0.100 over 9.0.200 is that a 9.0.2xx-only machine loses dotnet sln list / dotnet sln add / dotnet restore in this tree, which a 9.0.200 floor would have permitted.

Taking those one at a time:

dotnet restore gains nothing. Restore evaluates the project, which is where NETSDK1045 is raised, so restore on net10.0 under a 9.0.2xx SDK fails anyway. A 9.0.200 floor buys you the right to reach a failure one step later.
dotnet sln list / dotnet sln add genuinely would work under a 9.0.200 floor and are blocked under yours. But the value of enumerating or mutating a project list you cannot restore or build is close to nil — and sln add in that state actively produces an unbuildable commit from a machine that could never have discovered it was unbuildable. Blocking it is the better outcome.
IDE open. VS 17.13+ and Rider parse .slnx themselves, so the format floor is not what gates opening; project load needs the SDK. Under your floor an IDE reports one "compatible SDK not found" up front. Under mine it opens the solution and then fails nine project loads individually. The first is a better experience.

So every branch of the "read without building" case resolves the same way: the 9.0.200 floor does not enable any useful action, and it defers the failure to a point where it is harder to read. That is the argument you made in the commit message and it survives scrutiny.

Worth adding, because it strengthens your case beyond what you claimed: the diagnostic is genuinely good. It names the requested version, the exact global.json path, the installed SDKs, and links aka.ms/dotnet/sdk-not-found. Compare that against nine NETSDK1045s, which name a target framework and leave the reader to work backwards to an SDK version. Failing once, early, with the version printed is worth more than failing late nine times.

One residual nuance, not an objection: the pin now encodes the TFM constraint and the .slnx format constraint is invisible in the file — nobody reading global.json learns that 9.0.200 is the format floor. It is recorded in your commit message, which is the right place. I'd specifically recommend not adding a comment to global.json to capture it; it is strict JSON in enough consumers that a comment is a needless risk for a fact that is already written down.

Verified at `9d6958d`

global.json is valid JSON, committed, not caught by any .gitignore rule
SDK resolves to 10.0.110 under the 10.0.100 floor — so rollForward: latestMajor is doing what you intend and the pin is not accidentally a ceiling
dotnet build — Build succeeded. 0 Warning(s), 0 Error(s)
dotnet test — exit 0
dotnet sln list — 9 projects, and I re-diffed that set against the project list extracted from origin/main:PlaceMark.sln: still identical, nothing dropped by either commit
Floor genuinely bites — independently reproduced your 99.0.100 experiment; all SDK commands refuse, exit 155

rollForward: latestMajor is the right policy for this project. It means a future .NET 11 or 12 SDK is picked up silently, which is a real tradeoff — but the alternative is an annual edit, CI will pin its own SDK explicitly anyway, and for a hobby-scale project the cost of the edit outweighs the risk. Note also that the default when rollForward is omitted is latestPatch, which would have been a much tighter pin than you want; specifying it was necessary, not decorative.

Findings 2 and 3

Agreed on both, and agreed with not actioning them.

The vacuous dotnet test is pre-existing (zero .cs files under tests/ on main too) and recording it against the CI ticket so the pipeline is not built to trust it is exactly the right disposition — better than fixing it here. On the x64/x86 declarations, you have my own reasoning back at me and I still hold it.

Non-blocking: the PR description is now stale

Worth a quick edit before merge, since the body outlives the branch as the record of what happened. The "One acceptance criterion I could NOT verify" section still reads as though the constraint lives only in prose, and still says "task #5 must pin a .NET 10 SDK in the workflow". That was true at 98be0e9; at 9d6958d the repository enforces it. The section also still cites 9.0.200 as the operative floor, which is now the number you deliberately rejected.

Criterion 4 is still not met — no CI exists, so nothing has been observed — but it is materially de-risked rather than merely deferred, and the body should say so.

Concretely useful for task #5: actions/setup-dotnet accepts a global-json-file input, so the pipeline can point at this file instead of restating a version. That single-sources the pin and removes the failure mode where the workflow and global.json drift apart. Worth putting on the ticket.

Summary

The .slnx conversion is faithful and independently verified; the SDK floor is correct, better-reasoned than my original suggestion, and verified to bite in both directions. Only outstanding item is the stale PR body, which is documentation and does not need another review round.

Verdict: mergeable Re-reviewed at `9d6958d`, superseding my verdict at `98be0e9`. Delta since then is a single new `global.json` — I re-verified the whole change in a fresh worktree rather than assuming the earlier run still held. Nothing blocking. **Your correction to my suggestion is right, and it is a better fix than the one I proposed.** I said 9.0.200; you said 10.0.100 and gave the reason: every project targets `net10.0`, so a 9.0.2xx SDK clears the `.slnx` floor, parses the solution, and then falls over at build time. I checked this rather than taking it on trust — `grep TargetFramework` returns `net10.0` nine times out of nine, and the installed SDK carries exactly one targeting pack, `Microsoft.NETCore.App.Ref/10.0.10`. A 9.0.2xx SDK ships the 9.x ref packs and would hit `NETSDK1045` on every one of the nine projects. My floor would have been satisfiable by an SDK that cannot build this repository, which makes it the wrong floor. The general form is that the correct floor is `max(solution-format floor, TFM floor)`, and I anchored on the first while the second is strictly higher. Good catch. ## On your specific question: is there a case for 9.0.200 I missed? I went looking for the scenario you named — someone who legitimately needs to open or restore without building — and tested it rather than reasoning about it. The short answer is that the scenario is **real but worthless**, and 10.0.100 remains correct. What I measured: with an unsatisfiable floor, the SDK refuses **every** command in the tree, not just build. `dotnet build`, `dotnet restore` and even the pure-read `dotnet sln list` all fail identically at SDK resolution, before any project is touched (exit 155). So the concrete cost of 10.0.100 over 9.0.200 is that a 9.0.2xx-only machine loses `dotnet sln list` / `dotnet sln add` / `dotnet restore` in this tree, which a 9.0.200 floor would have permitted. Taking those one at a time: - **`dotnet restore`** gains nothing. Restore evaluates the project, which is where `NETSDK1045` is raised, so restore on `net10.0` under a 9.0.2xx SDK fails anyway. A 9.0.200 floor buys you the right to reach a failure one step later. - **`dotnet sln list` / `dotnet sln add`** genuinely would work under a 9.0.200 floor and are blocked under yours. But the value of enumerating or mutating a project list you cannot restore or build is close to nil — and `sln add` in that state actively produces an unbuildable commit from a machine that could never have discovered it was unbuildable. Blocking it is the better outcome. - **IDE open.** VS 17.13+ and Rider parse `.slnx` themselves, so the format floor is not what gates opening; project *load* needs the SDK. Under your floor an IDE reports one "compatible SDK not found" up front. Under mine it opens the solution and then fails nine project loads individually. The first is a better experience. So every branch of the "read without building" case resolves the same way: the 9.0.200 floor does not enable any *useful* action, and it defers the failure to a point where it is harder to read. That is the argument you made in the commit message and it survives scrutiny. Worth adding, because it strengthens your case beyond what you claimed: the diagnostic is genuinely good. It names the requested version, the exact `global.json` path, the installed SDKs, and links `aka.ms/dotnet/sdk-not-found`. Compare that against nine `NETSDK1045`s, which name a target framework and leave the reader to work backwards to an SDK version. Failing once, early, with the version printed is worth more than failing late nine times. One residual nuance, not an objection: the pin now encodes the TFM constraint and the `.slnx` format constraint is invisible in the file — nobody reading `global.json` learns that 9.0.200 is the format floor. It is recorded in your commit message, which is the right place. I'd specifically recommend **not** adding a comment to `global.json` to capture it; it is strict JSON in enough consumers that a comment is a needless risk for a fact that is already written down. ## Verified at `9d6958d` - `global.json` is valid JSON, committed, not caught by any `.gitignore` rule - SDK resolves to 10.0.110 under the 10.0.100 floor — so `rollForward: latestMajor` is doing what you intend and the pin is not accidentally a ceiling - `dotnet build` — `Build succeeded. 0 Warning(s), 0 Error(s)` - `dotnet test` — exit 0 - `dotnet sln list` — 9 projects, and I re-diffed that set against the project list extracted from `origin/main:PlaceMark.sln`: still **identical**, nothing dropped by either commit - Floor genuinely bites — independently reproduced your 99.0.100 experiment; all SDK commands refuse, exit 155 `rollForward: latestMajor` is the right policy for this project. It means a future .NET 11 or 12 SDK is picked up silently, which is a real tradeoff — but the alternative is an annual edit, CI will pin its own SDK explicitly anyway, and for a hobby-scale project the cost of the edit outweighs the risk. Note also that the default when `rollForward` is omitted is `latestPatch`, which would have been a much tighter pin than you want; specifying it was necessary, not decorative. ## Findings 2 and 3 Agreed on both, and agreed with not actioning them. The vacuous `dotnet test` is pre-existing (zero `.cs` files under `tests/` on `main` too) and recording it against the CI ticket so the pipeline is not built to trust it is exactly the right disposition — better than fixing it here. On the `x64`/`x86` declarations, you have my own reasoning back at me and I still hold it. ## Non-blocking: the PR description is now stale Worth a quick edit before merge, since the body outlives the branch as the record of what happened. The "One acceptance criterion I could NOT verify" section still reads as though the constraint lives only in prose, and still says "**task #5 must pin a .NET 10 SDK in the workflow**". That was true at `98be0e9`; at `9d6958d` the repository enforces it. The section also still cites 9.0.200 as the operative floor, which is now the number you deliberately rejected. Criterion 4 is still not *met* — no CI exists, so nothing has been observed — but it is materially de-risked rather than merely deferred, and the body should say so. Concretely useful for task #5: `actions/setup-dotnet` accepts a `global-json-file` input, so the pipeline can point at this file instead of restating a version. That single-sources the pin and removes the failure mode where the workflow and `global.json` drift apart. Worth putting on the ticket. ## Summary The `.slnx` conversion is faithful and independently verified; the SDK floor is correct, better-reasoned than my original suggestion, and verified to bite in both directions. Only outstanding item is the stale PR body, which is documentation and does not need another review round.

rob merged commit 60b93a0bbf into main

2026-08-02 19:21:09 +00:00

rob deleted branch chore/convert-to-slnx

2026-08-02 19:21:09 +00:00

rob referenced this pull request from a commit

2026-08-02 19:21:11 +00:00

Convert the solution to the .slnx format (#5)

rob referenced this pull request

2026-08-02 20:07:32 +00:00

Establish coding standards and static analysis (Vikunja task #2) #8

rob referenced this pull request from a commit

2026-08-02 20:36:24 +00:00