Introduce PlaceMark.Contracts for shared API DTOs (Vikunja task #7) #4

Merged

rob merged 2 commits from feat/contracts-project into main 2026-08-02 19:09:17 +00:00

Conversation 2 Commits 2 Files changed 7 +70 -15

rob commented 2026-08-02 19:00:22 +00:00

Owner

Copy link

Implements Vikunja task #7, raised off the review of the scaffolding PR. The architecture had no home for the request/response types the API returns and the front end consumes, leaving only two bad options: duplicate them on both sides, or have the front end reference server-side projects.

Lands now rather than later because the expensive part to retrofit is the shared home and its wiring — that touches every endpoint and every client call.

Reference graph (read from the csproj files, not assumed)

PlaceMark.Contracts       →  nothing at all
PlaceMark.Domain          →  nothing at all
PlaceMark.Infrastructure  →  Domain
PlaceMark.Api             →  Infrastructure, Contracts
PlaceMark.WebUI           →  Contracts  (+ the two pre-existing WASM packages)

Contracts has no project references and no NuGet packages. Domain still references nothing, so it remains ignorant of the wire format.

Verification

After dotnet clean, run independently of the authoring agent:

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

The project is deliberately empty

No PlaceDto, no GroupDto, not even the settled GroupRole enum. The reasoning, which is worth challenging:

• The retrofit argument applies to the shared home, not the types. Adding a DTO later costs one file and changes nothing else.
• Until the first endpoint exists, any type here is unreferenced public API that reviewers must take on trust.
• GroupRole specifically is less decision-free than it looks: a wire-side role enum implies a second domain-side one, since Domain cannot reference Contracts and entities never cross the wire. Shipping it here would quietly settle the duplicate-versus-shared-enum question that this ticket was not scoped to decide. That belongs to whoever writes the memberships endpoint, with the mapping in front of them.

Convention change the reviewer should scrutinise

The README stated "Every production project has exactly one corresponding test project". A declaration-only project has no behaviour to specify, so PlaceMark.Contracts.Tests would be permanently empty.

Rather than ship a dead project or silently break the stated rule, the convention now reads "Every production project that contains behaviour…", names Contracts as the single exception, and states the condition for ending it: "It acquires PlaceMark.Contracts.Tests on the day it acquires logic, and not before."

Amending a convention to fit the code is exactly the move that deserves suspicion, so please judge it on its merits — the alternative was an empty test project that looks like one whose tests were skipped rather than one that never needed any.

Documentation

• CLAUDE.md — Contracts added to the architecture list with the dependency direction.
• README.md — architecture table, dependency-direction section, repository layout tree, naming conventions.
• CONTRIBUTING.md — slightly beyond the ticket's stated doc scope: one bullet added to "Code expectations" stating that domain entities are never exposed over the wire. Flagged rather than buried; trivially revertible if you would rather this PR touched only the two named files.

The recently-added distinction between what the build enforces and what only review can is preserved and extended. The dependency-direction argument had to be reworded — it previously justified enforcement partly with "PlaceMark.Api references only PlaceMark.Infrastructure", which this change makes false. It is now built on the leaf projects (Domain and Contracts reference nothing, so a back-reference is circular), a framing that survives future additions. The README states plainly that PlaceMark.Api references both Contracts and Domain, so returning an entity straight from an endpoint would build without complaint.

Noted for a later ticket

If the "no entities over the wire" rule should be enforced rather than reviewed, the mechanism is an architecture test (NetArchTest or similar) asserting no PlaceMark.Domain type appears in an endpoint signature. That needs endpoints to exist first.

Serialisation-shape tests are also genuinely valuable, but they test the API's serialisation configuration rather than the DTO declarations, so they belong in PlaceMark.Api.Tests regardless of the convention change above.

Implements Vikunja task #7, raised off the review of the scaffolding PR. The architecture had no home for the request/response types the API returns and the front end consumes, leaving only two bad options: duplicate them on both sides, or have the front end reference server-side projects. Lands now rather than later because the expensive part to retrofit is the shared home and its wiring — that touches every endpoint and every client call. ## Reference graph (read from the csproj files, not assumed) ``` PlaceMark.Contracts → nothing at all PlaceMark.Domain → nothing at all PlaceMark.Infrastructure → Domain PlaceMark.Api → Infrastructure, Contracts PlaceMark.WebUI → Contracts (+ the two pre-existing WASM packages) ``` Contracts has no project references *and* no NuGet packages. Domain still references nothing, so it remains ignorant of the wire format. ## Verification After `dotnet clean`, run independently of the authoring agent: - `dotnet build PlaceMark.sln` — **0 Warning(s), 0 Error(s)** - `dotnet test PlaceMark.sln` — exit 0 ## The project is deliberately empty No `PlaceDto`, no `GroupDto`, not even the settled `GroupRole` enum. The reasoning, which is worth challenging: - The retrofit argument applies to the shared home, not the types. Adding a DTO later costs one file and changes nothing else. - Until the first endpoint exists, any type here is unreferenced public API that reviewers must take on trust. - `GroupRole` specifically is less decision-free than it looks: a wire-side role enum implies a *second* domain-side one, since Domain cannot reference Contracts and entities never cross the wire. Shipping it here would quietly settle the duplicate-versus-shared-enum question that this ticket was not scoped to decide. That belongs to whoever writes the memberships endpoint, with the mapping in front of them. ## Convention change the reviewer should scrutinise The README stated "Every production project has exactly one corresponding test project". A declaration-only project has no behaviour to specify, so `PlaceMark.Contracts.Tests` would be permanently empty. Rather than ship a dead project or silently break the stated rule, the convention now reads "Every production project **that contains behaviour**…", names Contracts as the single exception, and states the condition for ending it: "It acquires `PlaceMark.Contracts.Tests` on the day it acquires logic, and not before." Amending a convention to fit the code is exactly the move that deserves suspicion, so please judge it on its merits — the alternative was an empty test project that looks like one whose tests were skipped rather than one that never needed any. ## Documentation - `CLAUDE.md` — Contracts added to the architecture list with the dependency direction. - `README.md` — architecture table, dependency-direction section, repository layout tree, naming conventions. - `CONTRIBUTING.md` — **slightly beyond the ticket's stated doc scope**: one bullet added to "Code expectations" stating that domain entities are never exposed over the wire. Flagged rather than buried; trivially revertible if you would rather this PR touched only the two named files. The recently-added distinction between what the build enforces and what only review can is preserved and extended. The dependency-direction argument had to be reworded — it previously justified enforcement partly with "`PlaceMark.Api` references only `PlaceMark.Infrastructure`", which this change makes false. It is now built on the leaf projects (`Domain` and `Contracts` reference nothing, so a back-reference is circular), a framing that survives future additions. The README states plainly that `PlaceMark.Api` references both Contracts and Domain, so returning an entity straight from an endpoint **would build without complaint**. ## Noted for a later ticket If the "no entities over the wire" rule should be enforced rather than reviewed, the mechanism is an architecture test (NetArchTest or similar) asserting no `PlaceMark.Domain` type appears in an endpoint signature. That needs endpoints to exist first. Serialisation-shape tests are also genuinely valuable, but they test the API's serialisation configuration rather than the DTO declarations, so they belong in `PlaceMark.Api.Tests` regardless of the convention change above.

rob added 1 commit 2026-08-02 19:00:22 +00:00

Introduce PlaceMark.Contracts for shared API DTOs ... ce148b76ee

The architecture had no home for the request and response types that the
API returns and the front end consumes, leaving only two bad options:
duplicate the types on both sides, or have the front end reference
server-side projects.

PlaceMark.Contracts is a leaf project with no project references and no
NuGet packages. Api and WebUI both reference it; Domain does not, so the
domain stays ignorant of the wire format.

The project is deliberately empty. What is expensive to retrofit is the
shared home and its wiring, since that touches every endpoint and every
client call; the types themselves cost one file each and should be
written when the endpoints that need them are, not guessed at now.

Records the rule that domain entities are never exposed over the wire,
and notes that nothing but review enforces it.

Vikunja task #7.

rob reviewed 2026-08-02 19:05:20 +00:00

rob left a comment

Copy link

Verdict: mergeable

Independent review of ce148b7. Everything below was checked against the working copy, not the diff.

Verification

Re-ran from a clean tree:

dotnet clean PlaceMark.sln    → Build succeeded. 0 Warning(s) 0 Error(s)
dotnet build PlaceMark.sln    → Build succeeded. 0 Warning(s) 0 Error(s)   (all 9 projects)
dotnet build PlaceMark.sln -c Release → Build succeeded. 0 Warning(s) 0 Error(s)
dotnet test  PlaceMark.sln    → exit 0

The claims hold, including in Release, which the PR body did not claim. One caveat on AC 3 worth stating plainly rather than leaving implied: dotnet test exits 0 because the solution contains no tests at all — all four test assemblies report No test is available in .... That was true before this PR and is not a regression, but "dotnet test exits 0" is currently a statement about the build, not about behaviour.

Reference graph, read from the csproj files:

Project	ProjectReference	PackageReference
PlaceMark.Contracts	none	none
PlaceMark.Domain	none	none
PlaceMark.Infrastructure	Domain	none
PlaceMark.Api	Infrastructure, Contracts	none
PlaceMark.WebUI	Contracts	the two pre-existing WASM packages

Matches the PR body exactly. AC 1 and AC 2 satisfied; Domain is still ignorant of the wire format.

Solution file: {E8EEE464-…} carries all twelve configuration mappings in the same Debug/Release × AnyCPU/x64/x86 shape as its neighbours, and NestedProjects maps it to {827E0CD3-…}, which is the src folder ({0AB3BF05-…} is tests) — correctly placed. Nothing unrelated was altered: the diff is seven files, and git ls-files src/PlaceMark.Contracts returns the csproj and nothing else, so no stray Class1.cs. British English is clean — the only -ization hits in the changed docs are Microsoft.AspNetCore.Authorization and a Forgejo token scope, both correctly exempt as third-party symbols.

Worth knowing, unrelated to this PR: the only workflow in .forgejo/workflows/ is the temporary runner-verification job. It does not build or test, so a green CI tick on this PR carries no information. Task #5 covers that.

A. The empty project — reasonable, and I would not hold it back

I went in expecting to argue with this and came out agreeing.

The load-bearing deliverable here is the edge in the reference graph, not the types on it. That edge is fully verifiable today: Contracts compiles with zero references of either kind, both consumers reference it, Domain does not, and the rule that governs it is written down in three places. Adding PlaceRequest later is one file and touches nothing else; discovering in six months that the front end has been referencing Infrastructure is a refactor across every endpoint and every client call. The expensive half is the half that landed.

The GroupRole reasoning is the part that could have been a dodge and is not. Domain cannot reference Contracts, and entities never cross the wire, so a wire-side role enum necessarily implies a second domain-side one. Shipping it here would settle duplicate-versus-shared-enum silently, by commit rather than by decision, in a PR whose ticket does not mention it. Deferring that to whoever writes the memberships endpoint, with the mapping code actually in front of them, is the better call — and CLAUDE.md's "roles are Owner/Editor/Viewer" is a decision about the domain, not about the wire representation, so nothing here re-litigates it.

The residual risk is real but small and I am not going to pretend otherwise: an empty shared project is an unfalsified design claim. Nothing yet demonstrates that Contracts can hold what both sides need under its own no-package rule. I checked that specifically, and the rule looks sustainable — System.Text.Json attributes and DataAnnotations both live in the shared framework, so serialisation shaping and validation metadata do not force a package. The first DTO ticket is where this gets proved. It should not be allowed to sit empty for long; the ticket that adds the first endpoint should populate it.

B. The convention amendment — honest maintenance, not laundering

Amending a rule so the code passes deserves the suspicion, and this survives it, on three grounds:

1. It does not lower any testing bar. The old sentence was about project pairing, not coverage; nothing that has behaviour becomes less tested under the new wording.
2. The exception is drawn narrowly enough that it cannot be stretched. It names one project, binds it to a stated criterion (contains no behaviour), and carries an explicit expiry: "It acquires PlaceMark.Contracts.Tests on the day it acquires logic, and not before." To abuse this later you would have to assert in review that a project containing logic contains none — a visible claim someone has to make out loud, not a quiet gap.
3. The alternative genuinely was worse. A permanently-empty test project is a standing invitation to add the wrong tests to it, and it makes the "every production project has a test project" rule true by ritual rather than by meaning.

One correction to the PR body's reasoning, though the conclusion is unaffected. The argument that an empty PlaceMark.Contracts.Tests "looks like one whose tests were skipped" does not hold in this repository today: all four existing test projects are empty — only .csproj files are tracked under tests/, and every assembly discovers zero tests. A fifth empty one would have been indistinguishable from its neighbours right now. The justification that actually works is prospective: the other four will fill in, and PlaceMark.Contracts.Tests never would. Worth carrying the better argument forward if this is ever revisited.

Non-blocking findings

Nothing here blocks the merge. In rough order of how much I would like to see them addressed.

1. README.md:32–35 — the enforcement claim over-reaches for the new leaf. "Project references enforce the direction itself: PlaceMark.Domain and PlaceMark.Contracts have no project references at all, so anything pointing back at them from the layer above fails the build as a circular dependency."

The rebuilt argument is sound for the cases it describes, and it is a genuine improvement — it survives future project additions in a way the old "PlaceMark.Api references only PlaceMark.Infrastructure" phrasing could not. But two leaves side by side create an edge the build does not close: Domain → Contracts, and Contracts → Domain, are both perfectly acyclic and would compile without complaint. AC 2 of the ticket explicitly requires that Domain does not reference Contracts, so this is not hypothetical — it is now the one architectural rule in the document that is neither build-enforced nor listed at README.md:37–43 among the rules only review can catch. The distinction between enforced and reviewed is otherwise preserved well; this rule just falls through the crack between the two lists. Suggest a third bullet in that section, something like: PlaceMark.Domain and PlaceMark.Contracts stay ignorant of each other. Neither referencing the other is circular, so the build permits it; a domain type in the wire format, or a wire type in the domain, is exactly the coupling the split exists to prevent.

2. README.md:41–43 — "PlaceMark.Api references both projects" is imprecise. Api directly references Infrastructure and Contracts; Domain arrives transitively through Infrastructure. The conclusion is correct — ProjectReference flows transitively, so returning an entity from an endpoint really does compile — but the sentence reads as though a direct Domain reference exists, which contradicts the table and graph a few lines above. "references Contracts directly and Domain transitively through Infrastructure" costs four words and removes the ambiguity.

3. CONTRIBUTING.md:56 — stale dependency arrow, two lines above the new bullet. "Respect the dependency direction Api → Infrastructure → Domain" is now an incomplete picture of a graph the very next bullet depends on. Since the file was being touched anyway, either extend the arrow or defer it to the README section that the bullet above already links to.

4. PlaceMark.sln:26 — cosmetic ordering. The new Project(...) entry is appended after the tests block rather than sitting with the other src entries. This is what dotnet sln add does and it has no functional effect — the folder nesting is correct — but the file no longer reads in the order the README layout tree presents.

5. src/PlaceMark.Api/PlaceMark.Api.csproj:11 — trivial. Contracts appended after Infrastructure; alphabetical would be marginally tidier. Ignore if you like.

On the CONTRIBUTING.md scope creep

Keep it. AC 5 requires that the rule be "written down" and does not name a file; "Code expectations" in CONTRIBUTING is precisely where a developer looks for the constraints their code must satisfy, and a rule the build cannot catch belongs where people read before writing, not only where they read before architecting. It is two lines, on the exact topic of the ticket, consistent with the wording in the other two files, and flagged in the PR body rather than buried. Reverting it would leave the rule absent from the one document aimed at the person most likely to break it. This is the kind of adjacent edit CONTRIBUTING.md:14 is asking people to declare, and it was declared.

Consistency check across the three documents

No contradictions found beyond finding 3. CLAUDE.md:19–20, README.md:20/29/41–43 and CONTRIBUTING.md:58–59 state the same rule in three registers without drifting on substance, and none of them claims the build enforces it — CONTRIBUTING.md:59 says outright "the build will not catch a slip here", which matches README.md:43. README.md:45–48 reconciles "WebUI does not reference the server-side projects" with the new reference inside a single sentence, which is the right place to handle it.

Agreed on both items in "Noted for a later ticket": an architecture test is the right mechanism once endpoints exist, and serialisation-shape tests belong to PlaceMark.Api.Tests because they test the serialiser configuration, not the declarations. Neither is a reason to hold this up.

Verdict: mergeable Independent review of `ce148b7`. Everything below was checked against the working copy, not the diff. ## Verification Re-ran from a clean tree: ``` dotnet clean PlaceMark.sln → Build succeeded. 0 Warning(s) 0 Error(s) dotnet build PlaceMark.sln → Build succeeded. 0 Warning(s) 0 Error(s) (all 9 projects) dotnet build PlaceMark.sln -c Release → Build succeeded. 0 Warning(s) 0 Error(s) dotnet test PlaceMark.sln → exit 0 ``` The claims hold, including in Release, which the PR body did not claim. One caveat on AC 3 worth stating plainly rather than leaving implied: `dotnet test` exits 0 because **the solution contains no tests at all** — all four test assemblies report `No test is available in ...`. That was true before this PR and is not a regression, but "`dotnet test` exits 0" is currently a statement about the build, not about behaviour. Reference graph, read from the csproj files: | Project | ProjectReference | PackageReference | | --- | --- | --- | | `PlaceMark.Contracts` | none | none | | `PlaceMark.Domain` | none | none | | `PlaceMark.Infrastructure` | Domain | none | | `PlaceMark.Api` | Infrastructure, Contracts | none | | `PlaceMark.WebUI` | Contracts | the two pre-existing WASM packages | Matches the PR body exactly. AC 1 and AC 2 satisfied; Domain is still ignorant of the wire format. Solution file: `{E8EEE464-…}` carries all twelve configuration mappings in the same Debug/Release × AnyCPU/x64/x86 shape as its neighbours, and `NestedProjects` maps it to `{827E0CD3-…}`, which is the **src** folder (`{0AB3BF05-…}` is tests) — correctly placed. Nothing unrelated was altered: the diff is seven files, and `git ls-files src/PlaceMark.Contracts` returns the csproj and nothing else, so no stray `Class1.cs`. British English is clean — the only `-ization` hits in the changed docs are `Microsoft.AspNetCore.Authorization` and a Forgejo token scope, both correctly exempt as third-party symbols. Worth knowing, unrelated to this PR: the only workflow in `.forgejo/workflows/` is the temporary runner-verification job. It does not build or test, so a green CI tick on this PR carries no information. Task #5 covers that. ## A. The empty project — reasonable, and I would not hold it back I went in expecting to argue with this and came out agreeing. The load-bearing deliverable here is the edge in the reference graph, not the types on it. That edge is fully verifiable today: Contracts compiles with zero references of either kind, both consumers reference it, Domain does not, and the rule that governs it is written down in three places. Adding `PlaceRequest` later is one file and touches nothing else; discovering in six months that the front end has been referencing Infrastructure is a refactor across every endpoint and every client call. The expensive half is the half that landed. The `GroupRole` reasoning is the part that could have been a dodge and is not. Domain cannot reference Contracts, and entities never cross the wire, so a wire-side role enum necessarily implies a second domain-side one. Shipping it here would settle duplicate-versus-shared-enum silently, by commit rather than by decision, in a PR whose ticket does not mention it. Deferring that to whoever writes the memberships endpoint, with the mapping code actually in front of them, is the better call — and CLAUDE.md's "roles are Owner/Editor/Viewer" is a decision about the *domain*, not about the wire representation, so nothing here re-litigates it. The residual risk is real but small and I am not going to pretend otherwise: an empty shared project is an unfalsified design claim. Nothing yet demonstrates that Contracts can hold what both sides need under its own no-package rule. I checked that specifically, and the rule looks sustainable — `System.Text.Json` attributes and `DataAnnotations` both live in the shared framework, so serialisation shaping and validation metadata do not force a package. The first DTO ticket is where this gets proved. It should not be allowed to sit empty for long; the ticket that adds the first endpoint should populate it. ## B. The convention amendment — honest maintenance, not laundering Amending a rule so the code passes deserves the suspicion, and this survives it, on three grounds: 1. It does not lower any testing bar. The old sentence was about project *pairing*, not coverage; nothing that has behaviour becomes less tested under the new wording. 2. The exception is drawn narrowly enough that it cannot be stretched. It names one project, binds it to a stated criterion (contains no behaviour), and carries an explicit expiry: "It acquires `PlaceMark.Contracts.Tests` on the day it acquires logic, and not before." To abuse this later you would have to assert in review that a project containing logic contains none — a visible claim someone has to make out loud, not a quiet gap. 3. The alternative genuinely was worse. A permanently-empty test project is a standing invitation to add the wrong tests to it, and it makes the "every production project has a test project" rule true by ritual rather than by meaning. One correction to the PR body's reasoning, though the conclusion is unaffected. The argument that an empty `PlaceMark.Contracts.Tests` "looks like one whose tests were skipped" does not hold in this repository *today*: **all four existing test projects are empty** — only `.csproj` files are tracked under `tests/`, and every assembly discovers zero tests. A fifth empty one would have been indistinguishable from its neighbours right now. The justification that actually works is prospective: the other four will fill in, and `PlaceMark.Contracts.Tests` never would. Worth carrying the better argument forward if this is ever revisited. ## Non-blocking findings Nothing here blocks the merge. In rough order of how much I would like to see them addressed. **1. `README.md:32–35` — the enforcement claim over-reaches for the new leaf.** "Project references enforce the direction itself: `PlaceMark.Domain` and `PlaceMark.Contracts` have no project references at all, so anything pointing back at them from the layer above fails the build as a circular dependency." The rebuilt argument is sound for the cases it describes, and it is a genuine improvement — it survives future project additions in a way the old "`PlaceMark.Api` references only `PlaceMark.Infrastructure`" phrasing could not. But two leaves side by side create an edge the build does not close: `Domain → Contracts`, and `Contracts → Domain`, are both perfectly acyclic and would compile without complaint. AC 2 of the ticket explicitly requires that Domain does not reference Contracts, so this is not hypothetical — it is now the one architectural rule in the document that is neither build-enforced *nor* listed at `README.md:37–43` among the rules only review can catch. The distinction between enforced and reviewed is otherwise preserved well; this rule just falls through the crack between the two lists. Suggest a third bullet in that section, something like: *`PlaceMark.Domain` and `PlaceMark.Contracts` stay ignorant of each other. Neither referencing the other is circular, so the build permits it; a domain type in the wire format, or a wire type in the domain, is exactly the coupling the split exists to prevent.* **2. `README.md:41–43` — "`PlaceMark.Api` references both projects" is imprecise.** Api directly references Infrastructure and Contracts; Domain arrives transitively through Infrastructure. The conclusion is correct — `ProjectReference` flows transitively, so returning an entity from an endpoint really does compile — but the sentence reads as though a direct Domain reference exists, which contradicts the table and graph a few lines above. "references Contracts directly and Domain transitively through Infrastructure" costs four words and removes the ambiguity. **3. `CONTRIBUTING.md:56` — stale dependency arrow, two lines above the new bullet.** "Respect the dependency direction `Api → Infrastructure → Domain`" is now an incomplete picture of a graph the very next bullet depends on. Since the file was being touched anyway, either extend the arrow or defer it to the README section that the bullet above already links to. **4. `PlaceMark.sln:26` — cosmetic ordering.** The new `Project(...)` entry is appended after the `tests` block rather than sitting with the other `src` entries. This is what `dotnet sln add` does and it has no functional effect — the folder nesting is correct — but the file no longer reads in the order the README layout tree presents. **5. `src/PlaceMark.Api/PlaceMark.Api.csproj:11` — trivial.** `Contracts` appended after `Infrastructure`; alphabetical would be marginally tidier. Ignore if you like. ## On the CONTRIBUTING.md scope creep Keep it. AC 5 requires that the rule be "written down" and does not name a file; "Code expectations" in CONTRIBUTING is precisely where a developer looks for the constraints their code must satisfy, and a rule the build cannot catch belongs where people read before writing, not only where they read before architecting. It is two lines, on the exact topic of the ticket, consistent with the wording in the other two files, and flagged in the PR body rather than buried. Reverting it would leave the rule absent from the one document aimed at the person most likely to break it. This is the kind of adjacent edit `CONTRIBUTING.md:14` is asking people to declare, and it was declared. ## Consistency check across the three documents No contradictions found beyond finding 3. `CLAUDE.md:19–20`, `README.md:20/29/41–43` and `CONTRIBUTING.md:58–59` state the same rule in three registers without drifting on substance, and none of them claims the build enforces it — `CONTRIBUTING.md:59` says outright "the build will not catch a slip here", which matches `README.md:43`. `README.md:45–48` reconciles "WebUI does not reference the server-side projects" with the new reference inside a single sentence, which is the right place to handle it. Agreed on both items in "Noted for a later ticket": an architecture test is the right mechanism once endpoints exist, and serialisation-shape tests belong to `PlaceMark.Api.Tests` because they test the serialiser configuration, not the declarations. Neither is a reason to hold this up.

rob added 1 commit 2026-08-02 19:06:16 +00:00

Document the leaf-project rule the build cannot enforce ... 4c4d9402ff

Review found a real gap. With two leaf projects rather than one, an edge
between Domain and Contracts in either direction is perfectly acyclic and
compiles without complaint, so the build does not enforce the separation
that this ticket's acceptance criteria require. The README listed the
rule nowhere and implied the project graph covered it.

States the rule explicitly alongside the other two that only review
enforces, and says why the two must be free to differ: the domain models
the problem, contracts model the wire, and they change for different
reasons.

Also corrects the claim that Api references both Contracts and Domain.
The Domain reference is transitive through Infrastructure, which the
architecture table above it already showed, and syncs the dependency
arrow in CONTRIBUTING that had gone stale.

rob reviewed 2026-08-02 19:08:40 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-review at 4c4d940, superseding the verdict at ce148b7. Delta only — the earlier verification of the reference graph, solution file and project contents still stands, and git diff ce148b7..HEAD touches nothing but README.md and CONTRIBUTING.md.

dotnet build PlaceMark.sln → Build succeeded. 0 Warning(s) 0 Error(s)
dotnet test  PlaceMark.sln → exit 0

Findings 1, 2 and 3 are properly closed. Taking your direct question seriously, though, the answer is yes — there is a fourth, and a fifth.

Does the new leaf-project bullet state the rule correctly?

Yes, and I checked it rather than taking it on faith. I temporarily added <ProjectReference Include="..\PlaceMark.Contracts\PlaceMark.Contracts.csproj" /> to PlaceMark.Domain.csproj and rebuilt the solution:

Build succeeded.  0 Warning(s)  0 Error(s)

Reverted immediately; tree is clean at 4c4d940. So README.md:40-43 is factually right, not merely plausible — the edge is acyclic and the build has, as the bullet says, nothing to object to. CONTRIBUTING.md:56-59 states the same thing compatibly ("both are leaves, so either edge compiles"). The "domain models the problem, contracts model the wire, they change for different reasons" justification is the right one to have written down: it explains why the duplication is deliberate, which is what stops someone "helpfully" collapsing it later.

Is the review-only list complete? No — two more, and one of them is the rule this whole PR exists to protect

README.md:37-38 now reads "That is the only part the build enforces. Three further rules matter just as much, and nothing but review enforces any of them". That framing is a real improvement, but it invites the reader to treat the list as the complete inventory of what review must guard, and two properties sitting immediately outside it are stated in the indicative — as descriptions of how things currently are — rather than as standing obligations.

Fourth: PlaceMark.WebUI must never reference the server-side projects. README.md:52 says "PlaceMark.WebUI is a separate deployable and does not reference the server-side projects" — a statement of fact, one paragraph after a list that has just implied it is exhaustive. Nothing enforces it. Tested:

WebUI → Infrastructure   →   Build succeeded.  0 Warning(s)  0 Error(s)

This is the one I would most want promoted into the list, because it is the exact failure mode this PR was raised to prevent. Your own PR body opens by naming the two bad options Contracts exists to close off: "duplicate them on both sides, or have the front end reference server-side projects." The second of those compiles today with zero warnings, and once EF Core and Npgsql land in Infrastructure it will still compile — it will just quietly drag a database driver into the WebAssembly payload. A reader who has internalised "three review-only rules" has not been told to watch for it.

Fifth, milder: PlaceMark.Contracts must stay free of NuGet packages. The claim "PlaceMark.Contracts has no NuGet dependencies either — it is the wire format and nothing else" sits at README.md:34-35, inside the paragraph about what project references enforce, and the very next sentence begins "That is the only part the build enforces." A reader can reasonably attach "that" to the whole preceding paragraph and conclude the package-freedom is enforced too. It is not:

Contracts + PackageReference Microsoft.EntityFrameworkCore   →   Build succeeded.  0 Warning(s)  0 Error(s)

Same class of defect as the EF-Core-in-Domain bullet you already have, and arguably sharper, since Contracts ships into the browser. Either move that clause down into the list or make it explicit where it stands.

Both are two-line edits and neither is a factual error — "three further rules matter just as much" does not literally assert that only three exist. That is why this is not blocking. But you asked whether anything was hiding, and it is the completeness of the framing rather than the accuracy of any sentence that has the gap. If you would rather bank these, the sensible home is the architecture-test ticket you already noted: a NetArchTest assertion covering "no PlaceMark.Domain type in an endpoint signature" is the same mechanism that would cover "no server-side assembly in the WebUI closure" and "no package in Contracts", and all three want endpoints to exist first.

Findings 4 and 5 — agreed, do not action

No disagreement. Tidying PlaceMark.sln ordering ahead of a wholesale .slnx migration is work with a known expiry date, and the csproj reference ordering was explicitly marked ignorable. Both were listed as cosmetic and correctly triaged as such.

On correcting the PR body

The review thread is adequate for the record, but I would spend one minute on it, for a specific reason rather than tidiness: per CLAUDE.md:60-64 your merges carry a fixed override paragraph in the merge commit, so the PR description is the text most likely to end up in git history at merge time — and the review thread is not. If the body ships as-is, the argument preserved permanently is the one we have both now agreed is wrong about this repository, and the argument that actually holds — the other four test projects will fill, PlaceMark.Contracts.Tests never would — survives only in a Forgejo comment. Swapping that one clause costs nothing and puts the durable reasoning in the durable place. Your call; I am not gating on it.

Consistency re-check

No contradictions. README.md:32-35 and :37-50, CONTRIBUTING.md:56-61 and CLAUDE.md:19-20 remain mutually consistent, and the strengthened README wording ("closes a cycle and fails the build") is more precise than what it replaced. The transitivity correction at README.md:47-49 now matches both the table and the actual csproj graph. British English clean throughout the delta.

Acceptance criteria 1-5 all met. Merge when ready.

Verdict: mergeable Re-review at `4c4d940`, superseding the verdict at `ce148b7`. Delta only — the earlier verification of the reference graph, solution file and project contents still stands, and `git diff ce148b7..HEAD` touches nothing but `README.md` and `CONTRIBUTING.md`. ``` dotnet build PlaceMark.sln → Build succeeded. 0 Warning(s) 0 Error(s) dotnet test PlaceMark.sln → exit 0 ``` Findings 1, 2 and 3 are properly closed. Taking your direct question seriously, though, the answer is yes — there is a fourth, and a fifth. ## Does the new leaf-project bullet state the rule correctly? Yes, and I checked it rather than taking it on faith. I temporarily added `<ProjectReference Include="..\PlaceMark.Contracts\PlaceMark.Contracts.csproj" />` to `PlaceMark.Domain.csproj` and rebuilt the solution: ``` Build succeeded. 0 Warning(s) 0 Error(s) ``` Reverted immediately; tree is clean at `4c4d940`. So `README.md:40-43` is factually right, not merely plausible — the edge is acyclic and the build has, as the bullet says, nothing to object to. `CONTRIBUTING.md:56-59` states the same thing compatibly ("both are leaves, so either edge compiles"). The "domain models the problem, contracts model the wire, they change for different reasons" justification is the right one to have written down: it explains why the duplication is deliberate, which is what stops someone "helpfully" collapsing it later. ## Is the review-only list complete? No — two more, and one of them is the rule this whole PR exists to protect `README.md:37-38` now reads "That is the *only* part the build enforces. Three further rules matter just as much, and nothing but review enforces any of them". That framing is a real improvement, but it invites the reader to treat the list as the complete inventory of what review must guard, and two properties sitting immediately outside it are stated in the indicative — as descriptions of how things currently are — rather than as standing obligations. **Fourth: `PlaceMark.WebUI` must never reference the server-side projects.** `README.md:52` says "`PlaceMark.WebUI` is a separate deployable and does not reference the server-side projects" — a statement of fact, one paragraph after a list that has just implied it is exhaustive. Nothing enforces it. Tested: ``` WebUI → Infrastructure → Build succeeded. 0 Warning(s) 0 Error(s) ``` This is the one I would most want promoted into the list, because it is the exact failure mode this PR was raised to prevent. Your own PR body opens by naming the two bad options Contracts exists to close off: "duplicate them on both sides, or have the front end reference server-side projects." The second of those compiles today with zero warnings, and once EF Core and Npgsql land in Infrastructure it will *still* compile — it will just quietly drag a database driver into the WebAssembly payload. A reader who has internalised "three review-only rules" has not been told to watch for it. **Fifth, milder: `PlaceMark.Contracts` must stay free of NuGet packages.** The claim "`PlaceMark.Contracts` has no NuGet dependencies either — it is the wire format and nothing else" sits at `README.md:34-35`, inside the paragraph about what project references *enforce*, and the very next sentence begins "That is the only part the build enforces." A reader can reasonably attach "that" to the whole preceding paragraph and conclude the package-freedom is enforced too. It is not: ``` Contracts + PackageReference Microsoft.EntityFrameworkCore → Build succeeded. 0 Warning(s) 0 Error(s) ``` Same class of defect as the EF-Core-in-Domain bullet you already have, and arguably sharper, since Contracts ships into the browser. Either move that clause down into the list or make it explicit where it stands. Both are two-line edits and neither is a factual error — "three further rules matter just as much" does not literally assert that only three exist. That is why this is not blocking. But you asked whether anything was hiding, and it is the completeness of the framing rather than the accuracy of any sentence that has the gap. If you would rather bank these, the sensible home is the architecture-test ticket you already noted: a NetArchTest assertion covering "no `PlaceMark.Domain` type in an endpoint signature" is the same mechanism that would cover "no server-side assembly in the WebUI closure" and "no package in Contracts", and all three want endpoints to exist first. ## Findings 4 and 5 — agreed, do not action No disagreement. Tidying `PlaceMark.sln` ordering ahead of a wholesale `.slnx` migration is work with a known expiry date, and the csproj reference ordering was explicitly marked ignorable. Both were listed as cosmetic and correctly triaged as such. ## On correcting the PR body The review thread is adequate for the record, but I would spend one minute on it, for a specific reason rather than tidiness: per `CLAUDE.md:60-64` your merges carry a fixed override paragraph in the merge commit, so the PR description is the text most likely to end up in git history at merge time — and the review thread is not. If the body ships as-is, the argument preserved permanently is the one we have both now agreed is wrong about this repository, and the argument that actually holds — the other four test projects will fill, `PlaceMark.Contracts.Tests` never would — survives only in a Forgejo comment. Swapping that one clause costs nothing and puts the durable reasoning in the durable place. Your call; I am not gating on it. ## Consistency re-check No contradictions. `README.md:32-35` and `:37-50`, `CONTRIBUTING.md:56-61` and `CLAUDE.md:19-20` remain mutually consistent, and the strengthened README wording ("closes a cycle and fails the build") is more precise than what it replaced. The transitivity correction at `README.md:47-49` now matches both the table and the actual csproj graph. British English clean throughout the delta. Acceptance criteria 1-5 all met. Merge when ready.

rob merged commit 52e4fd701f into main 2026-08-02 19:09:17 +00:00

rob deleted branch feat/contracts-project 2026-08-02 19:09:17 +00:00

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

Introduce PlaceMark.Contracts for shared API DTOs (#4)

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

Establish ADR structure and backfill resolved decisions (Vikunja Documentation #4) #6

rob referenced this pull request from a commit 2026-08-02 19:59:29 +00:00

Add a Docker Compose Postgres for local development

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:16:03 +00:00

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

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!4

No description provided.