Scaffold the solution structure (Vikunja task #1) #3

Merged

rob merged 2 commits from feat/solution-scaffolding into main 2026-08-02 18:50:09 +00:00

Conversation 2 Commits 2 Files changed 73 +60512 -1

rob commented 2026-08-02 18:10:09 +00:00

Owner

Copy link

Implements Vikunja task #1, "Initialise Git repository and solution structure". Scaffolding only — no domain logic, no packages beyond the templates.

What is here

• PlaceMark.sln with four production projects under src/ and one xUnit test project each under tests/.
• README.md — architecture, repository layout, naming conventions, getting started.
• CONTRIBUTING.md — branch/PR/review workflow, commit expectations, code expectations.

Verification

After dotnet clean, run independently of the authoring agent, and reproduced again by the reviewer:

• dotnet build PlaceMark.sln — Build succeeded, 0 Warning(s), 0 Error(s)
• dotnet test PlaceMark.sln — exit 0

Dependency direction was verified structurally, not assumed: PlaceMark.Domain has no project references at all, PlaceMark.Api references only PlaceMark.Infrastructure, and the reviewer confirmed a reverse reference fails the build with MSB4006: circular dependency.

Corrections to this PR's original claims

Two statements in the first version of this description were wrong. Recording them rather than quietly editing them out, since the second would have misled a later ticket.

1. OpenAPI was not unfixable — it was one line away. The original text said no newer Microsoft.AspNetCore.OpenApi unpins the vulnerable Microsoft.OpenApi 2.0.0, implying the advisory could not be resolved. The reviewer disproved that: the advisory (GHSA-v5pm-xwqc-g5wc / CVE-2026-49451, high severity, circular-schema-reference parsing) has first_patched_version 2.7.5, within the 2.x line. Pinning Microsoft.OpenApi 2.7.5 alongside Microsoft.AspNetCore.OpenApi 10.0.10 restores clean, builds with 0 warnings, and AddOpenApi()/MapOpenApi() compile.

The package removal still stands, but on scope grounds only — OpenAPI is not part of a scaffolding ticket and no code referenced it once the sample endpoint was deleted. Whichever ticket introduces API documentation should re-add the package and pin Microsoft.OpenApi to 2.7.5 or later. It is not blocked.

2. The BOM claim understated the problem. The original said only PlaceMark.Domain.csproj carries a UTF-8 BOM. In fact 13 tracked files do: PlaceMark.sln, the Domain and Infrastructure csprojs, all four test csprojs, five WebUI .razor files, and src/PlaceMark.Api/Properties/launchSettings.json. All are dotnet new artefacts. This matters because it under-scoped the normalisation work in task #2.

Review feedback actioned

• TDD mandate removed from CONTRIBUTING.md. It was introduced by the authoring agent, is recorded nowhere in CLAUDE.md or the backlog, and was violated by the very commit that introduced it — this PR adds production files against four empty test projects. The naming half of the convention is kept. If TDD is to be policy, it deserves its own ticket and a CLAUDE.md line rather than arriving by fiat in a contribution guide.
• README overstatement corrected. It claimed project references stop infrastructure concerns leaking into the domain. They do not: the reviewer demonstrated that adding EF Core as a NuGet reference to PlaceMark.Domain builds with 0 warnings. The README now distinguishes what the build enforces from what reviewers must.
• Trailing newlines added to the four test project files.

Other judgement calls

.sln rather than .slnx. On .NET 10 dotnet new sln defaults to the XML .slnx format; it was regenerated as .sln because the ticket names PlaceMark.sln explicitly. .slnx is arguably better (no GUID churn, cleaner diffs) and is fully supported. Reversible.

Template sample content removed — weather forecast endpoint and record, Counter.razor, Weather.razor, sample data, dead nav links and CSS, and the Class1.cs/UnitTest1.cs stubs. App title and home page body changed from template defaults to describe PlaceMark. The reviewer confirmed no leftovers.

CONTRIBUTING.md review-gate wording was corrected before this PR was raised; as first drafted it claimed a PR "cannot merge without an approving review. There is no exception to this" — the unachievable rule PR #2 removed from CLAUDE.md.

Known gaps, deliberately not addressed

• dotnet test prints "No test is available in ..." four times; exit code is 0. Placeholder assertions were not added — a permanently-green meaningless test is worse than a noisy log line. Relevant to CI (task #5): do not enable --treat-no-tests-as-error, or CI will fail on green scaffolding.
• No PlaceMark.Contracts project. PlaceMark.WebUI will need DTOs shared with PlaceMark.Api; CLAUDE.md's architecture does not name such a project, so today the choice is a new project or duplicated DTOs. Should be ticketed before the first endpoint is built.
• No .editorconfig, analysers or nullable configuration (task #2); no EF Core/Npgsql/JWT/Testcontainers/bUnit; no Docker Compose (task #4); .forgejo/ untouched (task #5).
• ~60k of the added lines are stock Bootstrap vendored into wwwroot/lib by the Blazor template.

Implements Vikunja task #1, "Initialise Git repository and solution structure". Scaffolding only — no domain logic, no packages beyond the templates. ## What is here - `PlaceMark.sln` with four production projects under `src/` and one xUnit test project each under `tests/`. - `README.md` — architecture, repository layout, naming conventions, getting started. - `CONTRIBUTING.md` — branch/PR/review workflow, commit expectations, code expectations. ## Verification After `dotnet clean`, run independently of the authoring agent, and reproduced again by the reviewer: - `dotnet build PlaceMark.sln` — **Build succeeded, 0 Warning(s), 0 Error(s)** - `dotnet test PlaceMark.sln` — exit 0 Dependency direction was verified structurally, not assumed: `PlaceMark.Domain` has no project references at all, `PlaceMark.Api` references only `PlaceMark.Infrastructure`, and the reviewer confirmed a reverse reference fails the build with `MSB4006: circular dependency`. ## Corrections to this PR's original claims Two statements in the first version of this description were wrong. Recording them rather than quietly editing them out, since the second would have misled a later ticket. **1. OpenAPI was not unfixable — it was one line away.** The original text said no newer `Microsoft.AspNetCore.OpenApi` unpins the vulnerable `Microsoft.OpenApi` 2.0.0, implying the advisory could not be resolved. The reviewer disproved that: the advisory (GHSA-v5pm-xwqc-g5wc / CVE-2026-49451, high severity, circular-schema-reference parsing) has `first_patched_version` **2.7.5, within the 2.x line**. Pinning `Microsoft.OpenApi` 2.7.5 alongside `Microsoft.AspNetCore.OpenApi` 10.0.10 restores clean, builds with 0 warnings, and `AddOpenApi()`/`MapOpenApi()` compile. The package removal still stands, but **on scope grounds only** — OpenAPI is not part of a scaffolding ticket and no code referenced it once the sample endpoint was deleted. **Whichever ticket introduces API documentation should re-add the package and pin `Microsoft.OpenApi` to 2.7.5 or later.** It is not blocked. **2. The BOM claim understated the problem.** The original said only `PlaceMark.Domain.csproj` carries a UTF-8 BOM. In fact **13 tracked files do**: `PlaceMark.sln`, the Domain and Infrastructure csprojs, all four test csprojs, five WebUI `.razor` files, and `src/PlaceMark.Api/Properties/launchSettings.json`. All are `dotnet new` artefacts. This matters because it under-scoped the normalisation work in task #2. ## Review feedback actioned - **TDD mandate removed** from `CONTRIBUTING.md`. It was introduced by the authoring agent, is recorded nowhere in CLAUDE.md or the backlog, and was violated by the very commit that introduced it — this PR adds production files against four empty test projects. The naming half of the convention is kept. If TDD is to be policy, it deserves its own ticket and a CLAUDE.md line rather than arriving by fiat in a contribution guide. - **README overstatement corrected.** It claimed project references stop infrastructure concerns leaking into the domain. They do not: the reviewer demonstrated that adding EF Core as a NuGet reference to `PlaceMark.Domain` builds with 0 warnings. The README now distinguishes what the build enforces from what reviewers must. - **Trailing newlines** added to the four test project files. ## Other judgement calls **`.sln` rather than `.slnx`.** On .NET 10 `dotnet new sln` defaults to the XML `.slnx` format; it was regenerated as `.sln` because the ticket names `PlaceMark.sln` explicitly. `.slnx` is arguably better (no GUID churn, cleaner diffs) and is fully supported. Reversible. **Template sample content removed** — weather forecast endpoint and record, `Counter.razor`, `Weather.razor`, sample data, dead nav links and CSS, and the `Class1.cs`/`UnitTest1.cs` stubs. App title and home page body changed from template defaults to describe PlaceMark. The reviewer confirmed no leftovers. **`CONTRIBUTING.md` review-gate wording** was corrected before this PR was raised; as first drafted it claimed a PR "cannot merge without an approving review. There is no exception to this" — the unachievable rule PR #2 removed from CLAUDE.md. ## Known gaps, deliberately not addressed - `dotnet test` prints "No test is available in ..." four times; exit code is 0. Placeholder assertions were not added — a permanently-green meaningless test is worse than a noisy log line. **Relevant to CI (task #5): do not enable `--treat-no-tests-as-error`**, or CI will fail on green scaffolding. - **No `PlaceMark.Contracts` project.** `PlaceMark.WebUI` will need DTOs shared with `PlaceMark.Api`; CLAUDE.md's architecture does not name such a project, so today the choice is a new project or duplicated DTOs. Should be ticketed before the first endpoint is built. - No `.editorconfig`, analysers or nullable configuration (task #2); no EF Core/Npgsql/JWT/Testcontainers/bUnit; no Docker Compose (task #4); `.forgejo/` untouched (task #5). - ~60k of the added lines are stock Bootstrap vendored into `wwwroot/lib` by the Blazor template.

rob added 1 commit 2026-08-02 18:10:09 +00:00

Scaffold the solution structure ... 7b7d2abc9d

Creates PlaceMark.sln with the four production projects under src/ and a
matching xUnit test project per production project under tests/.

The dependency direction Api -> Infrastructure -> Domain is enforced by
project references rather than intent: Domain references nothing at all,
and Api references only Infrastructure.

Drops the webapi template's Microsoft.AspNetCore.OpenApi package, which
pins a Microsoft.OpenApi version carrying a known high severity advisory
and emits NU1903 on restore. API documentation is not part of this ticket
and no code depended on the package once the sample endpoint was removed;
whichever ticket introduces OpenAPI should revisit it.

Adds README content covering the architecture, repository layout and
naming conventions, and a CONTRIBUTING guide for the branch and review
workflow.

Vikunja task #1.

rob reviewed 2026-08-02 18:20:35 +00:00

rob left a comment

Copy link

Verdict: changes needed

Reviewed at head 7b7d2abc9d14e5e7befe8c1a59ae7ccb2fdf20e9. Everything below was executed against the working copy, not read off the diff.

The scaffolding itself is sound and I found no defect in it. All four acceptance criteria are met. What I am holding the merge for is two one-line documentation edits, both detailed under "Required" — no code needs to change.

What I verified

Check	Result
dotnet clean → dotnet build PlaceMark.sln	Build succeeded, 0 Warning(s), 0 Error(s), exit 0 (SDK 10.0.110)
dotnet test PlaceMark.sln	exit 0, four No test is available notices — exactly as the PR states
Projects in solution vs on disk	8 / 8, no orphans, no stray .slnx
Template leftovers	none — weather, counter, Class1, UnitTest1, fetchdata, surveyprompt all absent from src/tests
Dead nav CSS	none — NavMenu.razor.css defines only .bi-house-door-fill-nav-menu, which is used
Branch protection on main	present: enable_push: false, required_approvals: 1, dismiss_stale_approvals: true
British English	clean. Only hits are the LICENSE filename and Microsoft.AspNetCore.Authorization — both correctly exempt as upstream/framework names
bin/obj ignored	yes, via the stock VisualStudio.gitignore

Dependency direction is correct: PlaceMark.Domain has zero ProjectReference entries, Infrastructure → Domain, Api → Infrastructure only, WebUI references neither.

The README's naming-conventions section does satisfy the acceptance criterion — it covers project naming, src/tests split, the test-project pairing rule, test class/method naming, folder naming and database naming. Comfortably sufficient.

Required before merge

R1 — CONTRIBUTING.md:58 mandates TDD by fiat, and this PR breaks its own rule

Tests are written before the code they describe

You asked for this to be challenged, so plainly: a contribution guide is the wrong instrument for this, and the sentence should be removed or softened. Three reasons.

1. It is unratified policy presented as settled fact. This project records decisions deliberately — CLAUDE.md has a dated "Decisions already made (do not re-litigate)" block with a decision log on Vikunja task 120. TDD appears in neither, nor in the ticket. The flat indicative phrasing makes a single agent's preference read as an agreed standard, and once merged it becomes the written standard by default rather than by decision.
2. It is contradicted by the commit that introduces it. This PR ships Program.cs, App.razor, NavMenu.razor, Home.razor and NotFound.razor alongside four test projects containing zero tests. The guide is violated on arrival, which is the worst state for a written rule — it trains readers that the document is aspirational.
3. Scope. The ticket asks for a README stub, .gitignore, licence, a building solution and documented naming conventions. CONTRIBUTING.md is already an addition beyond that; originating process policy inside that addition compounds it.

Note the rest of that bullet is fine — the naming half (<MethodName>_<Scenario>_<ExpectedResult>) is a genuine convention and matches the README. Suggested edit: keep the naming clause, drop the ordering claim. If TDD is to be project policy, it deserves its own ticket and a line in CLAUDE.md, not a subordinate clause in a contribution guide.

R2 — the OpenAPI rationale is incomplete in a way that will misinform the follow-up ticket

I checked this independently in a scratch project rather than trusting the PR body, and the results split.

The advisory is real. Restoring Microsoft.AspNetCore.OpenApi 10.0.10 on this SDK emits:

warning NU1903: Package 'Microsoft.OpenApi' 2.0.0 has a known high severity vulnerability,
https://github.com/advisories/GHSA-v5pm-xwqc-g5wc

GHSA-v5pm-xwqc-g5wc is CVE-2026-49451, high severity, "Circular schema references may terminate OpenAPI parsing". The authoring agent's description was accurate and the transitive pin to 2.0.0 is confirmed in project.assets.json. Credit where due — that claim held up.

But the conclusion drawn from it does not. The advisory metadata gives first_patched_version: 2.7.5 for the Microsoft.OpenApi package — the fix landed within the 2.x line, not behind a major-version bump. I verified the obvious remedy works:

<PackageReference Include="Microsoft.AspNetCore.OpenApi" Version="10.0.10" />
<PackageReference Include="Microsoft.OpenApi" Version="2.7.5" />

That restores to Microsoft.OpenApi/2.7.5, builds with 0 warnings, 0 errors, and AddOpenApi() / MapOpenApi() compile and run. A direct PackageReference pins the transitive dependency; no suppression, no NoWarn, no major-version risk.

So the framing "no newer Microsoft.AspNetCore.OpenApi unpins it", while literally true, implies OpenAPI was unusable. It was not — it was one line away. Removing the package is still the right call for this PR, because it is genuinely out of scope and had no consumer once the sample endpoint went. I am not asking for it back. What I am asking is that the recorded reason say "removed as out of scope; note the NU1903 advisory is resolved by pinning Microsoft.OpenApi >= 2.7.5 when OpenAPI is reintroduced", rather than leaving a note that reads as "OpenAPI is blocked by an unfixable advisory". As written, the next ticket inherits a false constraint.

Related: CLAUDE.md says architectural decisions get an ADR in docs/adr/. This decision currently lives only in a PR body, which is not greppable from the repo. A short ADR (or a comment on the follow-up ticket) would be the durable home.

Non-blocking observations

• README.md:29-31 overstates enforcement. I tested both halves. A reverse project reference does fail — adding Domain → Infrastructure produces error MSB4006: There is a circular dependency in the target dependency graph, so that much is genuinely build-enforced. But the sentence continues "infrastructure concerns must not leak into the domain", and that is not enforced: adding <PackageReference Include="Microsoft.EntityFrameworkCore" /> to PlaceMark.Domain builds clean with 0 warnings. Suggest distinguishing the two, or noting that package-level leakage is convention until an architecture test exists. The PR body's "enforced by project references, not intent" carries the same imprecision.
• The BOM note in the PR body is wrong. It says PlaceMark.Domain.csproj has a UTF-8 BOM "and the others do not". Ten tracked files do: PlaceMark.sln, both Domain/Infrastructure csprojs, all four test csprojs, and App.razor, MainLayout.razor, NavMenu.razor, NotFound.razor, _Imports.razor. Worth correcting so task #2 scopes the normalisation properly rather than fixing one file.
• Four test .csproj files lack a trailing newline (tests/*/*.Tests.csproj). Trivial, and squarely task #2's .editorconfig territory.
• dotnet test exit 0 with no tests — I agree with the decision not to add placeholder assertions, and the warning to CI ticket #5 not to enable --treat-no-tests-as-error is a genuinely useful hand-off. Good call, well flagged.
• .sln over .slnx — agree with following the ticket's explicit wording. Reversible, correctly surfaced, no action needed.
• PlaceMark.Contracts gap — a real and well-spotted issue. Agree it is out of scope here; please do get it ticketed before the first endpoint lands.
• Bootstrap is vendored into wwwroot/lib (~60k of the 60.5k added lines). That is stock blazorwasm template output, so it is expected, but it does mean the diff size is almost entirely vendor content and future dependency updates there will be manual.

Consistency check

I specifically checked CONTRIBUTING against the recently-rewritten CLAUDE.md review-gate section and found no contradiction. CONTRIBUTING's "Repeat until the reviewer returns Verdict: mergeable" matches CLAUDE.md step 5; its "the review gate is a convention here, not something Forgejo enforces … merges use an admin override" matches the "How the review gate actually works" section. The correction described in the PR body was applied correctly. README, CONTRIBUTING and CLAUDE.md agree on dependency direction, British English, user-secrets and branch protection.

Good, careful PR overall — the self-flagged judgement calls were accurate and made review genuinely faster. The two required items are documentation-only and should be quick.

Verdict: changes needed Reviewed at head `7b7d2abc9d14e5e7befe8c1a59ae7ccb2fdf20e9`. Everything below was executed against the working copy, not read off the diff. **The scaffolding itself is sound and I found no defect in it.** All four acceptance criteria are met. What I am holding the merge for is two one-line documentation edits, both detailed under "Required" — no code needs to change. ## What I verified | Check | Result | | --- | --- | | `dotnet clean` → `dotnet build PlaceMark.sln` | **Build succeeded, 0 Warning(s), 0 Error(s)**, exit 0 (SDK 10.0.110) | | `dotnet test PlaceMark.sln` | exit 0, four `No test is available` notices — exactly as the PR states | | Projects in solution vs on disk | 8 / 8, no orphans, no stray `.slnx` | | Template leftovers | none — `weather`, `counter`, `Class1`, `UnitTest1`, `fetchdata`, `surveyprompt` all absent from `src`/`tests` | | Dead nav CSS | none — `NavMenu.razor.css` defines only `.bi-house-door-fill-nav-menu`, which is used | | Branch protection on `main` | present: `enable_push: false`, `required_approvals: 1`, `dismiss_stale_approvals: true` | | British English | clean. Only hits are the `LICENSE` filename and `Microsoft.AspNetCore.Authorization` — both correctly exempt as upstream/framework names | | `bin`/`obj` ignored | yes, via the stock `VisualStudio.gitignore` | **Dependency direction is correct**: `PlaceMark.Domain` has zero `ProjectReference` entries, `Infrastructure → Domain`, `Api → Infrastructure` only, `WebUI` references neither. The README's naming-conventions section does satisfy the acceptance criterion — it covers project naming, `src`/`tests` split, the test-project pairing rule, test class/method naming, folder naming and database naming. Comfortably sufficient. ## Required before merge ### R1 — `CONTRIBUTING.md:58` mandates TDD by fiat, and this PR breaks its own rule > Tests are written before the code they describe You asked for this to be challenged, so plainly: **a contribution guide is the wrong instrument for this, and the sentence should be removed or softened.** Three reasons. 1. **It is unratified policy presented as settled fact.** This project records decisions deliberately — CLAUDE.md has a dated "Decisions already made (do not re-litigate)" block with a decision log on Vikunja task 120. TDD appears in neither, nor in the ticket. The flat indicative phrasing makes a single agent's preference read as an agreed standard, and once merged it *becomes* the written standard by default rather than by decision. 2. **It is contradicted by the commit that introduces it.** This PR ships `Program.cs`, `App.razor`, `NavMenu.razor`, `Home.razor` and `NotFound.razor` alongside four test projects containing zero tests. The guide is violated on arrival, which is the worst state for a written rule — it trains readers that the document is aspirational. 3. **Scope.** The ticket asks for a README stub, `.gitignore`, licence, a building solution and documented naming conventions. `CONTRIBUTING.md` is already an addition beyond that; originating process policy inside that addition compounds it. Note the rest of that bullet is fine — the naming half (`<MethodName>_<Scenario>_<ExpectedResult>`) is a genuine convention and matches the README. Suggested edit: keep the naming clause, drop the ordering claim. If TDD is to be project policy, it deserves its own ticket and a line in CLAUDE.md, not a subordinate clause in a contribution guide. ### R2 — the OpenAPI rationale is incomplete in a way that will misinform the follow-up ticket I checked this independently in a scratch project rather than trusting the PR body, and the results split. **The advisory is real.** Restoring `Microsoft.AspNetCore.OpenApi` 10.0.10 on this SDK emits: ``` warning NU1903: Package 'Microsoft.OpenApi' 2.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-v5pm-xwqc-g5wc ``` GHSA-v5pm-xwqc-g5wc is **CVE-2026-49451**, high severity, "Circular schema references may terminate OpenAPI parsing". The authoring agent's description was accurate and the transitive pin to 2.0.0 is confirmed in `project.assets.json`. Credit where due — that claim held up. **But the conclusion drawn from it does not.** The advisory metadata gives `first_patched_version: 2.7.5` for the `Microsoft.OpenApi` package — the fix landed *within the 2.x line*, not behind a major-version bump. I verified the obvious remedy works: ```xml <PackageReference Include="Microsoft.AspNetCore.OpenApi" Version="10.0.10" /> <PackageReference Include="Microsoft.OpenApi" Version="2.7.5" /> ``` That restores to `Microsoft.OpenApi/2.7.5`, builds with **0 warnings, 0 errors**, and `AddOpenApi()` / `MapOpenApi()` compile and run. A direct `PackageReference` pins the transitive dependency; no suppression, no `NoWarn`, no major-version risk. So the framing "no newer `Microsoft.AspNetCore.OpenApi` unpins it", while literally true, implies OpenAPI was unusable. It was not — it was one line away. **Removing the package is still the right call for this PR**, because it is genuinely out of scope and had no consumer once the sample endpoint went. I am not asking for it back. What I am asking is that the recorded reason say *"removed as out of scope; note the NU1903 advisory is resolved by pinning `Microsoft.OpenApi` >= 2.7.5 when OpenAPI is reintroduced"*, rather than leaving a note that reads as "OpenAPI is blocked by an unfixable advisory". As written, the next ticket inherits a false constraint. Related: CLAUDE.md says architectural decisions get an ADR in `docs/adr/`. This decision currently lives only in a PR body, which is not greppable from the repo. A short ADR (or a comment on the follow-up ticket) would be the durable home. ## Non-blocking observations - **`README.md:29-31` overstates enforcement.** I tested both halves. A reverse *project* reference does fail — adding `Domain → Infrastructure` produces `error MSB4006: There is a circular dependency in the target dependency graph`, so that much is genuinely build-enforced. But the sentence continues "infrastructure concerns must not leak into the domain", and that is **not** enforced: adding `<PackageReference Include="Microsoft.EntityFrameworkCore" />` to `PlaceMark.Domain` builds clean with 0 warnings. Suggest distinguishing the two, or noting that package-level leakage is convention until an architecture test exists. The PR body's "enforced by project references, not intent" carries the same imprecision. - **The BOM note in the PR body is wrong.** It says `PlaceMark.Domain.csproj` has a UTF-8 BOM "and the others do not". Ten tracked files do: `PlaceMark.sln`, both `Domain`/`Infrastructure` csprojs, all four test csprojs, and `App.razor`, `MainLayout.razor`, `NavMenu.razor`, `NotFound.razor`, `_Imports.razor`. Worth correcting so task #2 scopes the normalisation properly rather than fixing one file. - **Four test `.csproj` files lack a trailing newline** (`tests/*/*.Tests.csproj`). Trivial, and squarely task #2's `.editorconfig` territory. - **`dotnet test` exit 0 with no tests** — I agree with the decision not to add placeholder assertions, and the warning to CI ticket #5 not to enable `--treat-no-tests-as-error` is a genuinely useful hand-off. Good call, well flagged. - **`.sln` over `.slnx`** — agree with following the ticket's explicit wording. Reversible, correctly surfaced, no action needed. - **`PlaceMark.Contracts` gap** — a real and well-spotted issue. Agree it is out of scope here; please do get it ticketed before the first endpoint lands. - **Bootstrap is vendored into `wwwroot/lib`** (~60k of the 60.5k added lines). That is stock `blazorwasm` template output, so it is expected, but it does mean the diff size is almost entirely vendor content and future dependency updates there will be manual. ## Consistency check I specifically checked CONTRIBUTING against the recently-rewritten CLAUDE.md review-gate section and found **no contradiction**. CONTRIBUTING's "Repeat until the reviewer returns `Verdict: mergeable`" matches CLAUDE.md step 5; its "the review gate is a convention here, not something Forgejo enforces … merges use an admin override" matches the "How the review gate actually works" section. The correction described in the PR body was applied correctly. README, CONTRIBUTING and CLAUDE.md agree on dependency direction, British English, `user-secrets` and branch protection. Good, careful PR overall — the self-flagged judgement calls were accurate and made review genuinely faster. The two required items are documentation-only and should be quick.

rob added 1 commit 2026-08-02 18:47:16 +00:00

Action review feedback on the scaffolding ... b58ed118a2

- Drop the assertion that tests are written before the code they
  describe. TDD is recorded nowhere in CLAUDE.md or the backlog, so a
  contribution guide was the wrong place to originate it as policy, and
  the commit introducing it violated it on arrival. The naming half of
  the convention is kept.
- Correct the README's claim about what the project references enforce.
  They enforce the direction, and a reverse reference does fail the build
  as a circular dependency. They do not stop infrastructure concerns
  leaking into the domain: a NuGet reference to EF Core from
  PlaceMark.Domain compiles without complaint. Reviewers enforce that.
- Add trailing newlines to the four test project files.

rob reviewed 2026-08-02 18:49:41 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed at b58ed118a293659d388f6cc73dbc5cf22e878e29, superseding my verdict at 7b7d2ab. Both required items are resolved. I re-ran everything rather than taking the summary on trust.

Re-verification

Check	Result
dotnet clean → dotnet build PlaceMark.sln	Build succeeded, 0 Warning(s), 0 Error(s), exit 0
dotnet test PlaceMark.sln	exit 0
Diff 7b7d2ab..b58ed11	6 files, +12/-9 — exactly the requested scope, nothing smuggled in

R1 — TDD mandate: resolved. CONTRIBUTING.md:58 now reads only "Tests are named for the behaviour they assert". Grepping CONTRIBUTING.md and README.md for written before/tdd/test.first returns nothing. The naming convention is retained, which was the right half to keep.

R2 — OpenAPI rationale: resolved, and handled better than I asked for. The "Corrections to this PR's original claims" section states the error rather than silently overwriting it, and gives the follow-up ticket an actionable instruction ("re-add the package and pin Microsoft.OpenApi to 2.7.5 or later. It is not blocked") instead of a false constraint. Recording a superseded claim in the open is more useful to whoever picks this up than a clean edit would have been.

Non-blocking items: all three confirmed fixed. README.md now correctly separates what the build enforces (direction, via circular-dependency failure) from what it does not (EF Core in Domain) — and I checked the strengthened claim holds in general, not just for the case I tested: any reverse reference along the existing Api → Infrastructure → Domain chain closes a cycle, so the README is accurate as written. Trailing newlines present on all four test csprojs.

Your correction to my correction — you are right, I was wrong

13, not 10. I globbed *.csproj *.razor *.cs *.md *.sln and the .json extension fell outside it, so src/PlaceMark.Api/Properties/launchSettings.json never entered my count. Re-running across every tracked file returns exactly your 13. My "ten tracked files do" was stated with more confidence than my method supported — the fault was the unstated glob, not the arithmetic. Task #2 should scope against your list, not mine.

On the ADR — not a dodge, and I withdraw the suggestion

You asked to be argued out of it rather than have it pass by default. I am not going to argue, because I think you are right and I was not.

Dropping a template package pending a later ticket does not constrain the shape of the system — it defers a dependency. CLAUDE.md reserves ADRs for architectural decisions, and stretching that to cover every scoping call would devalue the record. More to the point, my own R1 objected to CONTRIBUTING.md originating unticketed policy inside a scaffolding PR; demanding you stand up docs/adr/ and its index in that same PR would have been the identical error with my name on it. Flagging it to the user is the proportionate response, and the PR body correction already gives the decision a durable, greppable home.

Verdict

All four acceptance criteria met: README stub with sufficient naming conventions, .NET/Blazor .gitignore, MIT licence, solution builds clean with scaffolding-only projects, and branch protection on main (required_approvals: 1, direct push disabled — re-confirmed). No outstanding blocking or non-blocking findings from me.

Good turnaround. The two things that make this easy to sign off are unrelated to the code: you corrected the record in public rather than quietly, and you pushed back on the one suggestion of mine that was wrong instead of complying with it. Mergeable.

Verdict: mergeable Re-reviewed at `b58ed118a293659d388f6cc73dbc5cf22e878e29`, superseding my verdict at `7b7d2ab`. Both required items are resolved. I re-ran everything rather than taking the summary on trust. ## Re-verification | Check | Result | | --- | --- | | `dotnet clean` → `dotnet build PlaceMark.sln` | **Build succeeded, 0 Warning(s), 0 Error(s)**, exit 0 | | `dotnet test PlaceMark.sln` | exit 0 | | Diff `7b7d2ab..b58ed11` | 6 files, +12/-9 — exactly the requested scope, nothing smuggled in | **R1 — TDD mandate: resolved.** `CONTRIBUTING.md:58` now reads only "Tests are named for the behaviour they assert". Grepping `CONTRIBUTING.md` and `README.md` for `written before`/`tdd`/`test.first` returns nothing. The naming convention is retained, which was the right half to keep. **R2 — OpenAPI rationale: resolved, and handled better than I asked for.** The "Corrections to this PR's original claims" section states the error rather than silently overwriting it, and gives the follow-up ticket an actionable instruction ("re-add the package and pin `Microsoft.OpenApi` to 2.7.5 or later. It is not blocked") instead of a false constraint. Recording a superseded claim in the open is more useful to whoever picks this up than a clean edit would have been. **Non-blocking items:** all three confirmed fixed. `README.md` now correctly separates what the build enforces (direction, via circular-dependency failure) from what it does not (EF Core in `Domain`) — and I checked the strengthened claim holds in general, not just for the case I tested: any reverse reference along the existing `Api → Infrastructure → Domain` chain closes a cycle, so the README is accurate as written. Trailing newlines present on all four test csprojs. ## Your correction to my correction — you are right, I was wrong 13, not 10. I globbed `*.csproj *.razor *.cs *.md *.sln` and the `.json` extension fell outside it, so `src/PlaceMark.Api/Properties/launchSettings.json` never entered my count. Re-running across every tracked file returns exactly your 13. My "ten tracked files do" was stated with more confidence than my method supported — the fault was the unstated glob, not the arithmetic. Task #2 should scope against your list, not mine. ## On the ADR — not a dodge, and I withdraw the suggestion You asked to be argued out of it rather than have it pass by default. I am not going to argue, because I think you are right and I was not. Dropping a template package pending a later ticket does not constrain the shape of the system — it defers a dependency. CLAUDE.md reserves ADRs for *architectural* decisions, and stretching that to cover every scoping call would devalue the record. More to the point, my own R1 objected to `CONTRIBUTING.md` originating unticketed policy inside a scaffolding PR; demanding you stand up `docs/adr/` and its index in that same PR would have been the identical error with my name on it. Flagging it to the user is the proportionate response, and the PR body correction already gives the decision a durable, greppable home. ## Verdict All four acceptance criteria met: README stub with sufficient naming conventions, .NET/Blazor `.gitignore`, MIT licence, solution builds clean with scaffolding-only projects, and branch protection on `main` (`required_approvals: 1`, direct push disabled — re-confirmed). No outstanding blocking or non-blocking findings from me. Good turnaround. The two things that make this easy to sign off are unrelated to the code: you corrected the record in public rather than quietly, and you pushed back on the one suggestion of mine that was wrong instead of complying with it. Mergeable.

rob merged commit 852e33e519 into main 2026-08-02 18:50:09 +00:00

rob deleted branch feat/solution-scaffolding 2026-08-02 18:50:09 +00:00

rob referenced this pull request from a commit 2026-08-02 18:50:09 +00:00

Scaffold the solution structure (#3)

rob referenced this pull request 2026-08-02 19:59:54 +00:00

Add a Docker Compose Postgres for local development (Vikunja task #4) #7

rob referenced this pull request from a commit 2026-08-02 20:36:05 +00:00

Define the configuration and secrets strategy

rob referenced this pull request 2026-08-02 20:36:54 +00:00

Define the configuration and secrets strategy (Vikunja task #3) #9

rob referenced this pull request from a commit 2026-08-02 21:07:07 +00:00

Define the configuration and secrets strategy (#9)

rob referenced this pull request 2026-08-03 06:38:07 +00:00

Document the v1 data model (Database epic, task #1) #11

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!3

No description provided.