rob/PlaceMark

Watch

Fork 0

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

Merged

rob merged 3 commits from feat/configuration-secrets into main

2026-08-02 21:07:06 +00:00

rob commented

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

Owner

Implements Vikunja task #3, with one acceptance criterion deliberately not implemented because it is incoherent. See below.

The deviation — `appsettings.Development.json.example` was not created

The criterion asks for it. The .example convention works because the real file is gitignored — that is why .env.example works. But appsettings.Development.json is tracked. An example sitting beside a live tracked file gives no indication which to edit, and copying it produces a tracked file the developer has just been invited to put credentials into.

The .NET analogue of .env is not appsettings.Development.json — it is the user secrets store. So the template is src/PlaceMark.Api/secrets.json.example, applied unedited:

dotnet user-secrets set --project src/PlaceMark.Api < src/PlaceMark.Api/secrets.json.example

It cannot shadow anything, because .NET never loads a secrets.json from a project directory. The literal-but-coherent variant (gitignore appsettings.Development.json, ship the example beside it) was considered and rejected: an ignored file inside the tree is one git add -f from being committed, whereas the store lives outside the tree and cannot be committed by any accident.

Precedence — observed, not recited

Verified by printing the live provider chain from a running API and testing five cases as real processes:

Case	Setup	Result
A	Development, store cleared	`<null>`
B	Secret applied from template	resolves from `secrets.json`
C	B + `ConnectionStrings__PlaceMark` env var	env var wins
D	`ASPNETCORE_ENVIRONMENT=Production`, secret still set	`<null>` — no `secrets.json` provider present at all
E	C + `--ConnectionStrings:PlaceMark` on the command line	command line wins

Two things that would not have come from memory: .NET 10 also probes PlaceMark.Api.settings[.{Environment}].json between the appsettings files and user secrets, and WebApplicationBuilder chains host configuration in last.

Case F, the failure mode: a password placed directly in appsettings.json resolves with no warning and no error. Worse, with a user secret also set it is silently overridden locally — so the author of the leak is the one person who cannot see it in use. That is why the rule is written into appsettings.Development.json itself, the file where the mistake gets made.

Only one key is defined

ConnectionStrings:PlaceMark. No JWT or OIDC keys — their shape follows from code that does not exist, and a placeholder signing key in a tracked file is precisely the artefact this ticket exists to prevent, because placeholders get edited in place and then committed. The template says so explicitly, so the omission does not read as an oversight.

ADR-0014

Not because "we use user secrets" is architectural, but because three clauses constrain the system's shape: deployed configuration must be expressible as environment variables (ruling out file-based deployment); nothing the WebAssembly client ships can be confidential, which means ADR-0002's OIDC flow cannot hold a client secret in the browser and must complete at the API; and the deviation above needs a durable home.

Verification

dotnet build — 0 warnings, 0 errors (after rebase onto the analysers from #8)
dotnet format --verify-no-changes — exit 0
dotnet test — exit 0
dotnet user-secrets init / set / list proven, then removed — no state left on the machine
git check-ignore confirms secrets.json is ignored and secrets.json.example is not

Rebase note

Branched before #8 merged; rebased onto it. The conflict was the one the author predicted: PlaceMark.Api.csproj now carries only <UserSecretsId> — TargetFramework, Nullable and ImplicitUsings stayed in Directory.Build.props rather than being restored. The ADR index carries 0012, 0013 and 0014 in order.

Flagged

No enforcement exists. .gitignore covers the plausible slip (copying the template and filling it in), but "no secrets committed" is unfalsifiable without a scanner. A CI secret scan would close it and belongs with task #5.

Deliberately no test. A WebApplicationFactory test pinning precedence needs a package and a public partial class Program added solely for tests, and would break the moment DI requires a live database. It belongs with the first ticket that actually reads configuration, where the harness exists anyway.

Small accepted drift: the connection string appears in both the README's Database block and secrets.json.example. The template names compose as the source of truth.

Implements Vikunja task #3, **with one acceptance criterion deliberately not implemented** because it is incoherent. See below. ## The deviation — `appsettings.Development.json.example` was not created The criterion asks for it. The `.example` convention works because the *real* file is gitignored — that is why `.env.example` works. But **`appsettings.Development.json` is tracked**. An example sitting beside a live tracked file gives no indication which to edit, and copying it produces a tracked file the developer has just been invited to put credentials into. The .NET analogue of `.env` is not `appsettings.Development.json` — it is the **user secrets store**. So the template is `src/PlaceMark.Api/secrets.json.example`, applied unedited: ```bash dotnet user-secrets set --project src/PlaceMark.Api < src/PlaceMark.Api/secrets.json.example ``` It cannot shadow anything, because .NET never loads a `secrets.json` from a project directory. The literal-but-coherent variant (gitignore `appsettings.Development.json`, ship the example beside it) was considered and rejected: an ignored file inside the tree is one `git add -f` from being committed, whereas the store lives outside the tree and cannot be committed by any accident. ## Precedence — observed, not recited Verified by printing the live provider chain from a running API and testing five cases as real processes: | Case | Setup | Result | |---|---|---| | A | Development, store cleared | `<null>` | | B | Secret applied from template | resolves **from `secrets.json`** | | C | B + `ConnectionStrings__PlaceMark` env var | env var **wins** | | D | `ASPNETCORE_ENVIRONMENT=Production`, secret still set | `<null>` — **no `secrets.json` provider present at all** | | E | C + `--ConnectionStrings:PlaceMark` on the command line | command line **wins** | Two things that would not have come from memory: .NET 10 also probes `PlaceMark.Api.settings[.{Environment}].json` between the appsettings files and user secrets, and `WebApplicationBuilder` chains host configuration in last. **Case F, the failure mode:** a password placed directly in `appsettings.json` resolves with **no warning and no error**. Worse, with a user secret also set it is silently overridden locally — so the author of the leak is the one person who cannot see it in use. That is why the rule is written into `appsettings.Development.json` itself, the file where the mistake gets made. ## Only one key is defined `ConnectionStrings:PlaceMark`. **No JWT or OIDC keys** — their shape follows from code that does not exist, and a placeholder signing key in a tracked file is precisely the artefact this ticket exists to prevent, because placeholders get edited in place and then committed. The template says so explicitly, so the omission does not read as an oversight. ## ADR-0014 Not because "we use user secrets" is architectural, but because three clauses constrain the system's shape: deployed configuration must be expressible as environment variables (ruling out file-based deployment); **nothing the WebAssembly client ships can be confidential**, which means ADR-0002's OIDC flow cannot hold a client secret in the browser and must complete at the API; and the deviation above needs a durable home. ## Verification - `dotnet build` — 0 warnings, 0 errors (after rebase onto the analysers from #8) - `dotnet format --verify-no-changes` — exit 0 - `dotnet test` — exit 0 - `dotnet user-secrets init` / `set` / `list` proven, then **removed** — no state left on the machine - `git check-ignore` confirms `secrets.json` is ignored and `secrets.json.example` is **not** ## Rebase note Branched before #8 merged; rebased onto it. The conflict was the one the author predicted: `PlaceMark.Api.csproj` now carries **only** `<UserSecretsId>` — `TargetFramework`, `Nullable` and `ImplicitUsings` stayed in `Directory.Build.props` rather than being restored. The ADR index carries 0012, 0013 and 0014 in order. ## Flagged **No enforcement exists.** `.gitignore` covers the plausible slip (copying the template and filling it in), but "no secrets committed" is unfalsifiable without a scanner. A CI secret scan would close it and belongs with task #5. **Deliberately no test.** A `WebApplicationFactory` test pinning precedence needs a package and a `public partial class Program` added solely for tests, and would break the moment DI requires a live database. It belongs with the first ticket that actually *reads* configuration, where the harness exists anyway. **Small accepted drift:** the connection string appears in both the README's Database block and `secrets.json.example`. The template names compose as the source of truth.

rob added 1 commit

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

Define the configuration and secrets strategy

Verify runner / verify (pull_request) Successful in 1s

Details

ee05db915a

Local sensitive configuration goes in the dotnet user-secrets store, not
in a tracked file. Only ConnectionStrings:PlaceMark is defined: JWT and
OIDC keys have no consumer yet, and a placeholder signing key in a
tracked file is exactly the artefact this ticket exists to prevent.

Does not add appsettings.Development.json.example, which the ticket asked
for. The .example convention works because the real file is ignored, as
with .env; appsettings.Development.json is tracked, so an example beside
it shadows a live file with nothing indicating which to edit, and copying
it produces a tracked file the developer has just been invited to put
credentials in. The .NET analogue of .env is the user secrets store, so
the template is secrets.json.example, applied unedited by piping it into
dotnet user-secrets set. It cannot shadow anything, because .NET never
loads a secrets.json from a project directory.

Precedence was observed by printing the live provider chain rather than
recited: environment variables beat user secrets and files, command line
beats environment variables, and user secrets load in Development only.
Demonstrated across five running API processes.

Records in ADR-0014 that deployed configuration must be expressible as
environment variables, and that nothing the WebAssembly client ships can
be confidential — which constrains ADR-0002's OIDC flow to complete at
the API rather than holding a client secret in the browser.

Vikunja task #3.

rob reviewed

2026-08-02 20:45:14 +00:00

rob left a comment

Verdict: changes needed

Independent review at ee05db9. One blocking finding, and it is a factual one in a document that becomes immutable the moment this merges.

On the deviation — I agree with it

Taking the central question first, because everything else is detail.

The reasoning is right, and it is not a rationalisation. Three things convince me:

appsettings.Development.json is tracked (git ls-files confirms), and the SDK web template generates it. The .example convention derives all of its meaning from the real file being absent from the tree — that is why .env.example reads unambiguously and appsettings.Development.json.example would not. Shipping both files would leave two near-identically named files, one live and one inert, with nothing on the face of either saying which.
The criterion's intent — a committed artefact that tells a fresh clone what configuration the API needs, with values that are safe to publish — is fully met by secrets.json.example, and met better, because it is executable. I ran the documented command and it worked first time.
The rejection of the literal-but-coherent alternative (gitignore appsettings.Development.json, ship the example beside it) is fair. The stronger of the two stated grounds is the first one — a file that is loaded but untracked makes "why does it behave differently on your machine" unanswerable from the repository, and that is a real, recurring cost. The second ground reads weaker than it is at first glance, but it holds up: git add -A does respect ignores, yet Docker build contexts, archives and backup tooling do not, and a credential file inside the tree is exposed to all of them.

So: honour the intent, record the deviation, move on. That is what this does. My one reservation about the argument (not the decision) is the blocking finding below.

Blocking

The claim that a project-directory secrets.json is never loaded is false in exactly the fresh-clone window.

Stated in three places:

docs/adr/0014-configuration-and-secrets.md:59 — "secrets.json.example cannot shadow anything, because .NET never loads configuration named secrets.json from a project directory at all."
.gitignore:441 — "such a file is inert — .NET never reads it from the project directory"
src/PlaceMark.Api/secrets.json.example header — "never from the repository, so nothing you set can be committed by accident and nothing here takes effect until you apply it"

AddUserSecrets builds a PhysicalFileProvider over ~/.microsoft/usersecrets/<id>/ only if that directory exists. If it does not, the file provider is null and AddJsonFile("secrets.json", optional: true) falls back to the builder's default provider — which is rooted at the content root, i.e. the project directory. Note the provider is registered as JsonConfigurationProvider for 'secrets.json' (Optional) with no path, which hides this.

Reproduced on this branch, with a src/PlaceMark.Api/secrets.json present throughout:

Store directory `~/.microsoft/usersecrets/2b9d7035…/`	Resolved value
absent — the state of a fresh clone	`Host=project-dir-secrets-json-WAS-READ` ← the project-directory file was loaded
exists, empty (no `secrets.json` inside)	`<null>`
exists, containing a secret	the store's value

The consequence matters because of who is in that window: it is precisely the developer who has cloned, has not yet run dotnet user-secrets set, reads a file named secrets.json.example, does the thing that name invites — cp secrets.json.example secrets.json — and finds that it works. It works silently, it teaches the wrong mechanism, and the only thing standing between that file and the history is the .gitignore entry, which the surrounding comment describes as a mere backstop for an inert file. It is not a backstop; in that window it is the control.

None of this undermines the decision. User secrets is still the right mechanism and the template is still the right artefact. But ADR-0001 freezes an accepted ADR's body on merge, so a wrong fact merged here can only be corrected by superseding it. Fix it now:

Reword ADR-0014 to the accurate version — the store lives outside the tree and cannot be committed; a project-directory secrets.json is loaded only while the store directory does not yet exist, which is why .gitignore covers it. That is still a decisive argument against appsettings.Development.json.example, and it does not depend on an overstatement.
Promote the .gitignore comment and the template header from "inert" to "live in the window before the store exists".
Consider naming the template so it does not invite the copy at all — secrets.example.json or usersecrets.template.json. Any name other than secrets.json is never loaded from the project directory under any condition, so this restores the "cannot shadow" property the ADR wants to claim.

Non-blocking

PowerShell cannot run the documented command. README:167 uses < redirection; < is a reserved operator in PowerShell and errors. The same section names %APPDATA%\Microsoft\UserSecrets\, so a Windows reader is explicitly contemplated. Add Get-Content src\PlaceMark.Api\secrets.json.example | dotnet user-secrets set --project src\PlaceMark.Api.
Vikunja task #3 (id 37) carries no comment recording the deviation, and its third acceptance criterion still reads as unmet. CLAUDE.md asks for tickets to be kept updated as work lands; the backlog is the place someone will look in six months. Annotate the criterion with a link to ADR-0014 before closing.
The README precedence table omits the ChainedConfigurationProvider that WebApplicationBuilder appends last (host configuration). The PR body notes it; the README does not. It changes nothing observable here — I could not construct a case where it contradicts the documented order — so this is FYI, not a correction.
Comments in appsettings.Development.json are fine for the configuration provider (it parses with CommentHandling.Skip — verified by the app starting and loading the file), but any plain System.Text.Json consumer of that file would throw. Nothing consumes it today.
Only ConnectionStrings:PlaceMark is defined — right call, and not an under-delivery. A placeholder signing key in a committed template is the artefact that gets copied into a deployed environment because nobody noticed it was a placeholder; and JWT/OIDC key names invented now would be a guess the implementation must then honour or churn. The template says so explicitly, which is what stops the omission reading as an oversight. The stale-template risk this creates is correctly called out in ADR-0014's consequences.

ADR-0014 as a record

Follows the template — Status/Date/Source, then Context, Decision, Alternatives considered, Consequences, matching the shape of 0012 and 0013; the supersession lines are correctly omitted rather than left empty. Index row at docs/adr/README.md:24 matches the title and status exactly, and no existing ADR body was touched. Alternatives are honest, including the one that reads as a close call.

The three constraint clauses justify the ADR. The WebAssembly clause is sound and, more to the point, correctly hedged: "any flow requiring one must be completed by PlaceMark.Api" leaves browser-side PKCE with a public client intact, which a looser wording would have wrongly excluded. Placing it here is defensible — ADR-0002 explicitly leaves the OIDC flow open and cannot be edited to point forward, and this derives a constraint rather than deciding the flow. The mild cost is discoverability: the constraint lives in a configuration ADR and is reachable only from the index or from 0014's link back to 0002. Worth naming in the eventual OIDC ADR.

What I ran

Full clean checkout at ee05db9 in an isolated worktree.

dotnet build — Build succeeded, 0 Warning(s), 0 Error(s)
dotnet test --no-build — exit 0 (no test files present yet; pre-existing)
dotnet format --verify-no-changes — exit 0
git check-ignore -v src/PlaceMark.Api/secrets.json → .gitignore:445:secrets.json, exit 0; git check-ignore src/PlaceMark.Api/secrets.json.example → exit 1. Correct in both directions.
PlaceMark.Api.csproj — <UserSecretsId> only; TargetFramework, Nullable, ImplicitUsings not reintroduced. No UTF-8 BOM on the csproj, the template, appsettings.Development.json or the ADR (3c 50 72, 7b 0a 20, 7b 0a 20, 23 20 41).
Connection string in the template matches docker-compose.yml exactly — 127.0.0.1 (compose binds 127.0.0.1:${POSTGRES_PORT:-5432}), port 5432, database/username/password all placemark from the :-placemark defaults. Correct on a fresh clone with no .env.
Piping — dotnet user-secrets set --project src/PlaceMark.Api < src/PlaceMark.Api/secrets.json.example → "Successfully saved 1 secrets to the secret store", exit 0. // comments tolerated; the nested object flattened to the store as "ConnectionStrings:PlaceMark", listed back by dotnet user-secrets list as expected. Working tree unchanged afterwards.

Precedence, re-derived rather than accepted — real processes, printing GetConnectionString("PlaceMark") and the live provider chain:

Case	Setup	Result
A	Development, store empty	`<null>`
B	secret applied from the template	resolves from the store
C	B + `ConnectionStrings__PlaceMark=Host=env-var-wins`	`Host=env-var-wins` — env var beats user secrets
D	`ASPNETCORE_ENVIRONMENT=Production`, secret still set	`<null>`, and no `secrets.json` provider in the chain at all
E	env var + `-- --ConnectionStrings:PlaceMark=Host=command-line-wins`	`Host=command-line-wins` — command line beats env var

The observed Development chain also confirms the README's PlaceMark.Api.settings[.{Environment}].json note, sitting between the appsettings files and user secrets. Every row of the README table is accurate.

Cleanup: the user secret set during testing was cleared and ~/.microsoft/usersecrets/2b9d7035-162a-4282-ac95-0bb3e3fdbcdd/ removed; ~/.microsoft/usersecrets/ is empty, as I found it. All probing was done in a throwaway copy outside the reviewed worktree, since deleted. The reviewed worktree is git status clean.

Once the "never loads a secrets.json from a project directory" claim is corrected in ADR-0014, .gitignore and the template header, this is ready to merge.

Verdict: changes needed Independent review at `ee05db9`. One blocking finding, and it is a factual one in a document that becomes immutable the moment this merges. ## On the deviation — I agree with it Taking the central question first, because everything else is detail. The reasoning is right, and it is not a rationalisation. Three things convince me: - `appsettings.Development.json` **is** tracked (`git ls-files` confirms), and the SDK web template generates it. The `.example` convention derives all of its meaning from the real file being absent from the tree — that is why `.env.example` reads unambiguously and `appsettings.Development.json.example` would not. Shipping both files would leave two near-identically named files, one live and one inert, with nothing on the face of either saying which. - The criterion's *intent* — a committed artefact that tells a fresh clone what configuration the API needs, with values that are safe to publish — is fully met by `secrets.json.example`, and met better, because it is executable. I ran the documented command and it worked first time. - The rejection of the literal-but-coherent alternative (gitignore `appsettings.Development.json`, ship the example beside it) is fair. The stronger of the two stated grounds is the first one — a file that is loaded but untracked makes "why does it behave differently on your machine" unanswerable from the repository, and that is a real, recurring cost. The second ground reads weaker than it is at first glance, but it holds up: `git add -A` does respect ignores, yet Docker build contexts, archives and backup tooling do not, and a credential file inside the tree is exposed to all of them. So: honour the intent, record the deviation, move on. That is what this does. My one reservation about the *argument* (not the decision) is the blocking finding below. ## Blocking **The claim that a project-directory `secrets.json` is never loaded is false in exactly the fresh-clone window.** Stated in three places: - `docs/adr/0014-configuration-and-secrets.md:59` — "`secrets.json.example` cannot shadow anything, because .NET never loads configuration named `secrets.json` from a project directory at all." - `.gitignore:441` — "such a file is inert — .NET never reads it from the project directory" - `src/PlaceMark.Api/secrets.json.example` header — "never from the repository, so nothing you set can be committed by accident and nothing here takes effect until you apply it" `AddUserSecrets` builds a `PhysicalFileProvider` over `~/.microsoft/usersecrets/<id>/` **only if that directory exists**. If it does not, the file provider is null and `AddJsonFile("secrets.json", optional: true)` falls back to the builder's default provider — which is rooted at the **content root**, i.e. the project directory. Note the provider is registered as `JsonConfigurationProvider for 'secrets.json' (Optional)` with no path, which hides this. Reproduced on this branch, with a `src/PlaceMark.Api/secrets.json` present throughout: | Store directory `~/.microsoft/usersecrets/2b9d7035…/` | Resolved value | |---|---| | absent — **the state of a fresh clone** | `Host=project-dir-secrets-json-WAS-READ` ← **the project-directory file was loaded** | | exists, empty (no `secrets.json` inside) | `<null>` | | exists, containing a secret | the store's value | The consequence matters because of who is in that window: it is precisely the developer who has cloned, has not yet run `dotnet user-secrets set`, reads a file named `secrets.json.example`, does the thing that name invites — `cp secrets.json.example secrets.json` — and finds that **it works**. It works silently, it teaches the wrong mechanism, and the only thing standing between that file and the history is the `.gitignore` entry, which the surrounding comment describes as a mere backstop for an inert file. It is not a backstop; in that window it is the control. None of this undermines the decision. User secrets is still the right mechanism and the template is still the right artefact. But ADR-0001 freezes an accepted ADR's body on merge, so a wrong fact merged here can only be corrected by superseding it. Fix it now: 1. Reword ADR-0014 to the accurate version — the store lives outside the tree and cannot be committed; a project-directory `secrets.json` is loaded only while the store directory does not yet exist, which is why `.gitignore` covers it. That is still a decisive argument against `appsettings.Development.json.example`, and it does not depend on an overstatement. 2. Promote the `.gitignore` comment and the template header from "inert" to "live in the window before the store exists". 3. Consider naming the template so it does not invite the copy at all — `secrets.example.json` or `usersecrets.template.json`. Any name other than `secrets.json` is never loaded from the project directory under any condition, so this restores the "cannot shadow" property the ADR wants to claim. ## Non-blocking - **PowerShell cannot run the documented command.** README:167 uses `<` redirection; `<` is a reserved operator in PowerShell and errors. The same section names `%APPDATA%\Microsoft\UserSecrets\`, so a Windows reader is explicitly contemplated. Add `Get-Content src\PlaceMark.Api\secrets.json.example | dotnet user-secrets set --project src\PlaceMark.Api`. - **Vikunja task #3 (id 37) carries no comment recording the deviation**, and its third acceptance criterion still reads as unmet. `CLAUDE.md` asks for tickets to be kept updated as work lands; the backlog is the place someone will look in six months. Annotate the criterion with a link to ADR-0014 before closing. - **The README precedence table omits the `ChainedConfigurationProvider`** that `WebApplicationBuilder` appends last (host configuration). The PR body notes it; the README does not. It changes nothing observable here — I could not construct a case where it contradicts the documented order — so this is FYI, not a correction. - **Comments in `appsettings.Development.json`** are fine for the configuration provider (it parses with `CommentHandling.Skip` — verified by the app starting and loading the file), but any plain `System.Text.Json` consumer of that file would throw. Nothing consumes it today. - **Only `ConnectionStrings:PlaceMark` is defined — right call**, and not an under-delivery. A placeholder signing key in a committed template is the artefact that gets copied into a deployed environment because nobody noticed it was a placeholder; and JWT/OIDC key *names* invented now would be a guess the implementation must then honour or churn. The template says so explicitly, which is what stops the omission reading as an oversight. The stale-template risk this creates is correctly called out in ADR-0014's consequences. ## ADR-0014 as a record Follows the template — `Status`/`Date`/`Source`, then Context, Decision, Alternatives considered, Consequences, matching the shape of 0012 and 0013; the supersession lines are correctly omitted rather than left empty. Index row at `docs/adr/README.md:24` matches the title and status exactly, and no existing ADR body was touched. Alternatives are honest, including the one that reads as a close call. The three constraint clauses justify the ADR. The WebAssembly clause is sound and, more to the point, correctly hedged: "any flow requiring one must be completed by `PlaceMark.Api`" leaves browser-side PKCE with a public client intact, which a looser wording would have wrongly excluded. Placing it here is defensible — ADR-0002 explicitly leaves the OIDC flow open and cannot be edited to point forward, and this derives a constraint rather than deciding the flow. The mild cost is discoverability: the constraint lives in a configuration ADR and is reachable only from the index or from 0014's link back to 0002. Worth naming in the eventual OIDC ADR. ## What I ran Full clean checkout at `ee05db9` in an isolated worktree. - `dotnet build` — **Build succeeded, 0 Warning(s), 0 Error(s)** - `dotnet test --no-build` — exit 0 (no test files present yet; pre-existing) - `dotnet format --verify-no-changes` — exit 0 - `git check-ignore -v src/PlaceMark.Api/secrets.json` → `.gitignore:445:secrets.json`, exit 0; `git check-ignore src/PlaceMark.Api/secrets.json.example` → exit 1. Correct in both directions. - `PlaceMark.Api.csproj` — `<UserSecretsId>` only; `TargetFramework`, `Nullable`, `ImplicitUsings` not reintroduced. No UTF-8 BOM on the csproj, the template, `appsettings.Development.json` or the ADR (`3c 50 72`, `7b 0a 20`, `7b 0a 20`, `23 20 41`). - Connection string in the template matches `docker-compose.yml` exactly — `127.0.0.1` (compose binds `127.0.0.1:${POSTGRES_PORT:-5432}`), port `5432`, database/username/password all `placemark` from the `:-placemark` defaults. Correct on a fresh clone with no `.env`. - **Piping** — `dotnet user-secrets set --project src/PlaceMark.Api < src/PlaceMark.Api/secrets.json.example` → "Successfully saved 1 secrets to the secret store", exit 0. `//` comments tolerated; the nested object flattened to the store as `"ConnectionStrings:PlaceMark"`, listed back by `dotnet user-secrets list` as expected. Working tree unchanged afterwards. **Precedence, re-derived rather than accepted** — real processes, printing `GetConnectionString("PlaceMark")` and the live provider chain: | Case | Setup | Result | |---|---|---| | A | Development, store empty | `<null>` | | B | secret applied from the template | resolves from the store | | C | B + `ConnectionStrings__PlaceMark=Host=env-var-wins` | `Host=env-var-wins` — **env var beats user secrets** | | D | `ASPNETCORE_ENVIRONMENT=Production`, secret still set | `<null>`, and **no `secrets.json` provider in the chain at all** | | E | env var + `-- --ConnectionStrings:PlaceMark=Host=command-line-wins` | `Host=command-line-wins` — **command line beats env var** | The observed Development chain also confirms the README's `PlaceMark.Api.settings[.{Environment}].json` note, sitting between the appsettings files and user secrets. Every row of the README table is accurate. **Cleanup:** the user secret set during testing was cleared and `~/.microsoft/usersecrets/2b9d7035-162a-4282-ac95-0bb3e3fdbcdd/` removed; `~/.microsoft/usersecrets/` is empty, as I found it. All probing was done in a throwaway copy outside the reviewed worktree, since deleted. The reviewed worktree is `git status` clean. Once the "never loads a `secrets.json` from a project directory" claim is corrected in ADR-0014, `.gitignore` and the template header, this is ready to merge.

rob added 1 commit

2026-08-02 20:55:22 +00:00

Correct a false safety claim, and remove the trap it described

Verify runner / verify (pull_request) Successful in 1s

Details

1c9c754306

Review found the load-bearing claim was wrong: .NET does read a
secrets.json from a project directory when the user secrets store has
never been initialised. AddUserSecrets builds its file provider only if
the store directory exists; otherwise the optional JSON source falls back
to the content root. Reproduced before changing anything.

The boundary turns out to matter. Directory existence alone decides it,
and even dotnet user-secrets list creates that directory, so the window
is 'before any user-secrets command has ever run on this machine'. Nobody
who has used the tool can reproduce it without deleting their store by
hand — which is why the claim survived being written, read and reviewed.

Renames the template to user-secrets.example.json and moves it to the
repository root rather than documenting the hazard. A warning in a file's
header only reaches the developer who opens it, not the one who does the
obvious thing with a file called secrets.json.example. Two properties now
make that copy unreachable: the root is outside the API's content root,
and dropping .example yields a filename no configuration source reads.
Both verified.

Records the gitignore rule as the control it actually is, not a backstop,
and names what would demote it: a CI secret scan, or a build-time check
that fails when a project directory contains a secrets.json.

rob reviewed

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

rob left a comment

Verdict: changes needed

Re-reviewed at 1c9c754, read via git fetch and git archive — I did not check out or mutate the branch.

The blocking finding is fully resolved, and the fix is better than the one I asked for. I asked for accurate wording; the author removed the hazard instead. I verified both properties they claim, plus the boundary they mapped, and every one holds.

One new blocking item, in the paragraph added to explain the deferral. It is a one-sentence fix and it is the only thing between this and merge.

The fix, verified

Ten runs against a clean export of 1c9c754, with ~/.microsoft/usersecrets/ genuinely empty at the start — the state the whole finding depends on, and the one nobody who has used the tool is normally in:

#	Setup (store directory absent unless stated)	Resolved value
1	control, nothing planted	`<null>`
2	`src/PlaceMark.Api/secrets.json` planted	loaded — the corrected claim describes real behaviour
3	`secrets.json` planted at the repository root	`<null>` — property 1 holds: the root is outside the content root the fallback targets
4	`user-secrets.json` planted in the project directory and the root	`<null>` — property 2 holds
5	literal `cp user-secrets.example.json user-secrets.json` at the root	`<null>`
6	the same copy made inside `src/PlaceMark.Api`	`<null>`

So the obvious copy is inert wherever it is made, and each property independently suffices — the belt and the braces are both real. The relocation is doing work the rename alone would not: had the template stayed beside the project under any name, a developer could still land on secrets.json by other routes, but with it at the root there is nothing in src/PlaceMark.Api to copy in the first place.

The boundary claim is the one I would have been most inclined to take on trust, and it is the most surprising, so I ran it:

Step	Result
project-directory `secrets.json` planted, store absent	loaded
`dotnet user-secrets list`	prints "No secrets configured for this application" — and creates the directory
same planted file, immediately afterwards	`<null>`

A command that reports nothing is configured, changes nothing, and is the natural thing to run when investigating, is sufficient to close the hole permanently on that machine. That is a genuinely nasty property, and "nobody who has used the tool can reproduce it — including every reviewer" is the right thing to have written down. Stating it in all three places is proportionate rather than repetitive: each is read by someone in a different position, and the .gitignore comment in particular is now read by whoever next wonders whether that line can be deleted.

Applying the template from its new path: dotnet user-secrets set --project src/PlaceMark.Api < user-secrets.example.json → "Successfully saved 1 secrets", and the API then resolved the docker-compose default. Store cleared and the directory removed afterwards; ~/.microsoft/usersecrets/ is empty, as I found it.

Blocking — one sentence, factually wrong, about the repository's own contents

docs/adr/0014-configuration-and-secrets.md, Consequences:

The second belongs in Directory.Build.props so that it covers every project rather than only the API, which is why it is not done here: that file is being introduced by separate work, and a check placed in one project's file now would have to be moved and would leave the other projects uncovered meanwhile.

Directory.Build.props already exists. It was added by 2d4865a — "Establish coding standards and static analysis (#8)" — which is this branch's own merge base, is on main, and appears in git ls-tree 1c9c754. This branch was rebased onto it, and the PlaceMark.Api.csproj comment two directories away points at it by name. The only other open PR, #10, is the CI pipeline and does not touch it.

This matters for the same reason the first finding did, and it is the same class of defect in the paragraph written to correct that defect: ADR-0001 freezes an accepted body on merge, so a wrong fact merged here can only be fixed by superseding the record. It is also load-bearing — it is the stated reason for not adding a control that the same paragraph calls the only thing in the path. And it is checkable with one ls against the same commit.

The decision it defends is right (see below), so this is a reword, not a rethink. Something like: "The second belongs in Directory.Build.props so that it covers every project rather than only the API. It is not done here because a build-time error is enforcement rather than strategy: it belongs with the CI secret scan, in the ticket that owns enforcement, where the two can be designed together."

Your question: defer the build-time check, or carry a narrower version here?

Defer — but the recorded reason is the wrong one, and the honest reason is stronger.

The placement argument does not survive Directory.Build.props already existing. With that file in the tree, there is no narrow-version-that-must-be-moved to avoid; the correct, universal version is available today at about five lines:

<Target Name="FailIfProjectHoldsUserSecretsFile"
        BeforeTargets="Build"
        Condition="Exists('$(MSBuildProjectDirectory)/secrets.json')">
  <Error Text="..." />
</Target>

So if placement were the only objection, I would say do it now. It is not, and three better reasons remain:

.gitignore already prevents the actual harm. The harm is a credential reaching the history. A build-time error catches the file only after it exists, and only for someone who builds. Its value is pedagogic — it tells a developer they have done something dangerous — not protective. Pedagogy can wait; the protection is already in place and is now correctly described as a control.
Enforcement is one concern and should have one owner. A build check and a CI secret scan overlap: the scan catches this case and several the build check cannot. Designing them together, in the ticket that owns enforcement, produces a coherent answer; landing half of it in a documentation PR produces two half-answers and an ADR that has to explain the seam.
Scope. This PR defines a strategy. An MSBuild target that fails builds is behaviour, arriving unrequested in review round two, and would need its own verification — that it fires, that it fires in CI, that it does not surprise dotnet format, which builds. That is a PR, not an addendum.

The one thing I would insist on is that the promise be attached to something. "Belongs to the CI ticket" currently exists only as a sentence in an ADR consequence paragraph. Put it on Vikunja task #5, or wherever the secret scan lands, so the deferral is a plan rather than an intention.

Non-blocking

Vikunja task #3 (id 37) still has no comment recording the deviation, and its third acceptance criterion still reads as unmet — list_comments returns empty. This was the one item from the last round that is genuinely still outstanding; the other three are done. Worth doing before the ticket is closed, and it can now carry the better story: the criterion was not met, something stronger was, and the reason is in ADR-0014.
user-secrets.json is not gitignored. Verified: secrets.json matches in any directory; user-secrets.json and user-secrets.example.json match nothing. The naming makes such a copy inert, which is the point, and an inert file gets deleted rather than filled in — but it is still committable if someone puts a real password in it before discovering it does nothing. One line would close it. Your call; the argument for leaving it is that every extra ignore line dilutes the one that is actually a control.
The three-way repetition of the fallback story (ADR section, README bullet, .gitignore comment, plus the template header — four places) is justified, but it is now four copies of a fact that will change if Microsoft roots the provider unconditionally. If that happens, the ADR is frozen and the other three drift. Not worth restructuring; worth knowing.

Everything else re-verified at this head

dotnet build — 0 Warning(s), 0 Error(s); dotnet test --no-build — exit 0; dotnet format --verify-no-changes — exit 0. None of the three creates the user secrets directory, which is what let me run the vulnerable-state experiments after them.
git check-ignore: secrets.json ignored in both the project directory and the root; user-secrets.example.json not ignored, so the template is live.
PlaceMark.Api.csproj unchanged from ee05db9 — <UserSecretsId> only, nothing from Directory.Build.props reintroduced. No BOM on user-secrets.example.json.
Connection string still matches docker-compose.yml exactly (127.0.0.1, 5432, placemark ×3).
No stale references to the old path anywhere — every remaining mention of secrets.json.example is deliberate, explaining why the template is not called that. ADR title unchanged, so the index row at docs/adr/README.md:24 still matches.
The three non-blocking fixes are all good. The PowerShell block is correct (Get-Content … | dotnet user-secrets set). The JSON-comments note now correctly attributes the tolerance to the configuration provider rather than to JSON, and names JsonCommentHandling.Skip. The precedence table's "five rows are the sources worth setting a value in" framing is better than the previous footnote, and the chained host configuration is now named — matching the provider chain I printed last round.
British English throughout; no American spellings in any changed file.

Reword the Directory.Build.props sentence and this is ready. The substance of the fix is right, the experiment that produced it was the right experiment, and mapping the boundary before choosing the remedy is what turned a documentation correction into an actual removal of the trap.

Verdict: changes needed Re-reviewed at `1c9c754`, read via `git fetch` and `git archive` — I did not check out or mutate the branch. **The blocking finding is fully resolved, and the fix is better than the one I asked for.** I asked for accurate wording; the author removed the hazard instead. I verified both properties they claim, plus the boundary they mapped, and every one holds. One new blocking item, in the paragraph added to explain the deferral. It is a one-sentence fix and it is the only thing between this and merge. ## The fix, verified Ten runs against a clean export of `1c9c754`, with `~/.microsoft/usersecrets/` genuinely empty at the start — the state the whole finding depends on, and the one nobody who has used the tool is normally in: | # | Setup (store directory absent unless stated) | Resolved value | |---|---|---| | 1 | control, nothing planted | `<null>` | | 2 | `src/PlaceMark.Api/secrets.json` planted | **loaded** — the corrected claim describes real behaviour | | 3 | `secrets.json` planted at the **repository root** | `<null>` — **property 1 holds**: the root is outside the content root the fallback targets | | 4 | `user-secrets.json` planted in the project directory *and* the root | `<null>` — **property 2 holds** | | 5 | literal `cp user-secrets.example.json user-secrets.json` at the root | `<null>` | | 6 | the same copy made inside `src/PlaceMark.Api` | `<null>` | So the obvious copy is inert wherever it is made, and each property independently suffices — the belt and the braces are both real. The relocation is doing work the rename alone would not: had the template stayed beside the project under any name, a developer could still land on `secrets.json` by other routes, but with it at the root there is nothing in `src/PlaceMark.Api` to copy in the first place. The boundary claim is the one I would have been most inclined to take on trust, and it is the most surprising, so I ran it: | Step | Result | |---|---| | project-directory `secrets.json` planted, store absent | **loaded** | | `dotnet user-secrets list` | prints **"No secrets configured for this application"** — and creates the directory | | same planted file, immediately afterwards | `<null>` | A command that reports nothing is configured, changes nothing, and is the natural thing to run when investigating, is sufficient to close the hole permanently on that machine. That is a genuinely nasty property, and "nobody who has used the tool can reproduce it — including every reviewer" is the right thing to have written down. Stating it in all three places is proportionate rather than repetitive: each is read by someone in a different position, and the `.gitignore` comment in particular is now read by whoever next wonders whether that line can be deleted. Applying the template from its new path: `dotnet user-secrets set --project src/PlaceMark.Api < user-secrets.example.json` → "Successfully saved 1 secrets", and the API then resolved the docker-compose default. Store cleared and the directory removed afterwards; `~/.microsoft/usersecrets/` is empty, as I found it. ## Blocking — one sentence, factually wrong, about the repository's own contents `docs/adr/0014-configuration-and-secrets.md`, Consequences: > The second belongs in `Directory.Build.props` so that it covers every project rather than only the API, which is why it is not done here: **that file is being introduced by separate work**, and a check placed in one project's file now would have to be moved and would leave the other projects uncovered meanwhile. `Directory.Build.props` already exists. It was added by `2d4865a` — "Establish coding standards and static analysis (#8)" — which is this branch's own merge base, is on `main`, and appears in `git ls-tree 1c9c754`. This branch was rebased onto it, and the `PlaceMark.Api.csproj` comment two directories away points at it by name. The only other open PR, #10, is the CI pipeline and does not touch it. This matters for the same reason the first finding did, and it is the same class of defect in the paragraph written to correct that defect: ADR-0001 freezes an accepted body on merge, so a wrong fact merged here can only be fixed by superseding the record. It is also load-bearing — it is the stated reason for not adding a control that the same paragraph calls the only thing in the path. And it is checkable with one `ls` against the same commit. The decision it defends is right (see below), so this is a reword, not a rethink. Something like: *"The second belongs in `Directory.Build.props` so that it covers every project rather than only the API. It is not done here because a build-time error is enforcement rather than strategy: it belongs with the CI secret scan, in the ticket that owns enforcement, where the two can be designed together."* ## Your question: defer the build-time check, or carry a narrower version here? **Defer — but the recorded reason is the wrong one, and the honest reason is stronger.** The placement argument does not survive `Directory.Build.props` already existing. With that file in the tree, there is no narrow-version-that-must-be-moved to avoid; the correct, universal version is available today at about five lines: ```xml <Target Name="FailIfProjectHoldsUserSecretsFile" BeforeTargets="Build" Condition="Exists('$(MSBuildProjectDirectory)/secrets.json')"> <Error Text="..." /> </Target> ``` So if placement were the only objection, I would say do it now. It is not, and three better reasons remain: 1. **`.gitignore` already prevents the actual harm.** The harm is a credential reaching the history. A build-time error catches the file only after it exists, and only for someone who builds. Its value is *pedagogic* — it tells a developer they have done something dangerous — not *protective*. Pedagogy can wait; the protection is already in place and is now correctly described as a control. 2. **Enforcement is one concern and should have one owner.** A build check and a CI secret scan overlap: the scan catches this case and several the build check cannot. Designing them together, in the ticket that owns enforcement, produces a coherent answer; landing half of it in a documentation PR produces two half-answers and an ADR that has to explain the seam. 3. **Scope.** This PR defines a strategy. An MSBuild target that fails builds is behaviour, arriving unrequested in review round two, and would need its own verification — that it fires, that it fires in CI, that it does not surprise `dotnet format`, which builds. That is a PR, not an addendum. The one thing I would insist on is that the promise be attached to something. "Belongs to the CI ticket" currently exists only as a sentence in an ADR consequence paragraph. Put it on Vikunja task #5, or wherever the secret scan lands, so the deferral is a plan rather than an intention. ## Non-blocking - **Vikunja task #3 (id 37) still has no comment** recording the deviation, and its third acceptance criterion still reads as unmet — `list_comments` returns empty. This was the one item from the last round that is genuinely still outstanding; the other three are done. Worth doing before the ticket is closed, and it can now carry the better story: the criterion was not met, something stronger was, and the reason is in ADR-0014. - **`user-secrets.json` is not gitignored.** Verified: `secrets.json` matches in any directory; `user-secrets.json` and `user-secrets.example.json` match nothing. The naming makes such a copy inert, which is the point, and an inert file gets deleted rather than filled in — but it is still committable if someone puts a real password in it before discovering it does nothing. One line would close it. Your call; the argument for leaving it is that every extra ignore line dilutes the one that is actually a control. - **The three-way repetition of the fallback story** (ADR section, README bullet, `.gitignore` comment, plus the template header — four places) is justified, but it is now four copies of a fact that will change if Microsoft roots the provider unconditionally. If that happens, the ADR is frozen and the other three drift. Not worth restructuring; worth knowing. ## Everything else re-verified at this head - `dotnet build` — **0 Warning(s), 0 Error(s)**; `dotnet test --no-build` — exit 0; `dotnet format --verify-no-changes` — exit 0. None of the three creates the user secrets directory, which is what let me run the vulnerable-state experiments after them. - `git check-ignore`: `secrets.json` ignored in both the project directory and the root; `user-secrets.example.json` **not** ignored, so the template is live. - `PlaceMark.Api.csproj` unchanged from `ee05db9` — `<UserSecretsId>` only, nothing from `Directory.Build.props` reintroduced. No BOM on `user-secrets.example.json`. - Connection string still matches `docker-compose.yml` exactly (`127.0.0.1`, `5432`, `placemark` ×3). - No stale references to the old path anywhere — every remaining mention of `secrets.json.example` is deliberate, explaining why the template is *not* called that. ADR title unchanged, so the index row at `docs/adr/README.md:24` still matches. - The three non-blocking fixes are all good. The PowerShell block is correct (`Get-Content … | dotnet user-secrets set`). The JSON-comments note now correctly attributes the tolerance to the configuration provider rather than to JSON, and names `JsonCommentHandling.Skip`. The precedence table's "five rows are the sources worth setting a value in" framing is better than the previous footnote, and the chained host configuration is now named — matching the provider chain I printed last round. - British English throughout; no American spellings in any changed file. Reword the `Directory.Build.props` sentence and this is ready. The substance of the fix is right, the experiment that produced it was the right experiment, and mapping the boundary before choosing the remedy is what turned a documentation correction into an actual removal of the trap.

rob added 1 commit

2026-08-02 21:04:34 +00:00

Correct the deferral reason, and consolidate the fallback story

Verify runner / verify (pull_request) Successful in 2s

Details

1fb48dd60c

The paragraph written to fix a false claim about the framework carried a
false claim about the repository: it deferred the build-time check on the
grounds that Directory.Build.props was being introduced by separate work.
That work is this branch's own merge base. Verified in the worktree
before editing.

The placement argument is gone. The record now says the check would take
about five lines in a file that already exists, so the deferral reads as
a judgement about ownership rather than a technical obstacle: gitignore
already prevents a credential reaching history, enforcement should be
specified once rather than piecemeal, and failing-build behaviour added
mid-review changes dotnet build for every contributor without its own
verification. The promise is recorded against the CI ticket as well,
because a promise living only in an ADR is one nobody is scheduled to
keep.

Ignores user-secrets.json. It is inert, unlike secrets.json, so the
exposure is shorter — but the harm prevented is a credential reaching
history, which is irreversible, against a file that merely looks idle.

Leaves the mechanism in the ADR alone and has the README, gitignore and
template header point at it. The frozen record holds the framework
behaviour that could change; the living files hold instructions that stay
true either way.

rob reviewed

2026-08-02 21:06:33 +00:00

rob left a comment

Verdict: mergeable

Final review at 1fb48dd, read via git fetch and git archive — no checkout, no mutation. Both blocking findings are resolved. Nothing further is required before merge.

The blocking finding

The Directory.Build.props sentence is gone and what replaced it is correct: the file exists at this head (git ls-tree 1fb48dd confirms), the record now says the check "would take about five lines in Directory.Build.props, which already exists", and it states outright that the deferral is a judgement about ownership rather than a technical obstacle.

That last clause is the part that matters, and it is a better record than the one I would have accepted. My concern was never only that the sentence was false — it was that a reader in a year would find a plausible-sounding technical excuse and quietly inherit it. A record that says "this is cheap and available, and we chose not to" cannot be misread that way, and it puts the decision back where it belongs, with whoever owns enforcement.

The three reasons are stated more precisely than I put them, particularly "it would not protect anything the ignore rule does not already protect" — that is the crux, and it is the sentence that makes the deferral obviously right rather than merely defensible.

The promise now has an owner

Vikunja task 39 carries it, and the comment is better than a cross-reference. It names both controls, the five-line cost, the fallback mechanism, the fresh-clone window, the detail that even list closes it, and that .gitignore is currently the only control in that path — so whoever picks up the CI ticket has the full picture without reading an ADR first. Added with add_comment, so no fields were clobbered. "A promise that exists only in an ADR is a promise nobody is scheduled to keep" is the right principle and it has been applied rather than just written.

Both non-blocking items

user-secrets.json ignored. I agree with the reasoning, including the part that resisted flattening the two names into one hazard. secrets.json works and gives no feedback; user-secrets.json does nothing and is noticed in minutes. They are not the same risk, and the comment keeps that visible while still ignoring both — the trade is one irreversible outcome against one mildly annoying one, and irreversible wins. Verified independently at this head:

Path	Result
`user-secrets.example.json`	not ignored — the template is still live and `git ls-files` confirms it is tracked
`user-secrets.json`	ignored, `.gitignore:450`
`src/PlaceMark.Api/user-secrets.json`	ignored, `.gitignore:450`
`secrets.json`, `src/PlaceMark.Api/secrets.json`	ignored, `.gitignore:449`

Both directions correct, and the new unanchored pattern does not catch the template by prefix — which was the one way this change could have silently un-shipped it.

Consolidation. The split is well chosen and the principle behind it is sound: the frozen document holds the framework behaviour that could change, the living files hold instructions that stay true either way. So if Microsoft ever roots the provider unconditionally, exactly one document is wrong and it is the one that is allowed to be, because it is dated and superseded rather than edited. The README bullet, .gitignore comment and template header now each say the actionable thing — it loads on a fresh clone, do not create it, apply the template instead — and defer the mechanism. Nothing lost: the two facts a reader acts on, and the reason nobody in review will have seen it, both survive in every copy.

Re-verified at this head

dotnet build — 0 Warning(s), 0 Error(s), under TreatWarningsAsErrors, EnforceCodeStyleInBuild and AnalysisMode=Recommended from Directory.Build.props; dotnet test --no-build — exit 0; dotnet format --verify-no-changes — exit 0.
Template still applies from its path after the header rewrite — dotnet user-secrets set --project src/PlaceMark.Api < user-secrets.example.json → "Successfully saved 1 secrets", and the API resolved the docker-compose default. Worth re-running rather than assuming, since the edited comments sit inside the JSON the parser has to tolerate.
src/PlaceMark.Api contains no secrets.json-shaped artefact; PlaceMark.Api.csproj still carries <UserSecretsId> alone. ADR title unchanged, so the index row at docs/adr/README.md:24 still matches. No American spellings in any changed file.
The load-bearing behavioural claims were verified in the previous round and are unchanged by this diff: project-directory secrets.json loads with the store absent; user-secrets.json loads from nowhere; secrets.json at the repository root loads from nowhere; dotnet user-secrets list closes the window while reporting that nothing is configured.

Store cleared and ~/.microsoft/usersecrets/2b9d7035-… deleted afterwards; the directory is empty as I found it. All probing was done in a git archive export outside the reviewed tree, since removed.

One follow-up, explicitly not blocking

Vikunja task 37 (task #3) still has no comment — list_comments returns empty. This is a bookkeeping item for merge time, not a code change: the third acceptance criterion reads as unmet, and the record of why it was replaced by something stronger lives only in this PR and ADR-0014. Given the effort spent making the other deferral properly owned, it would be a shame for this one to be the loose end.

Merge when ready. The strategy is right, the trap that the strategy accidentally created has been removed rather than documented, and the record of both is accurate — including about its own limits.

Verdict: mergeable Final review at `1fb48dd`, read via `git fetch` and `git archive` — no checkout, no mutation. Both blocking findings are resolved. Nothing further is required before merge. ## The blocking finding The `Directory.Build.props` sentence is gone and what replaced it is correct: the file exists at this head (`git ls-tree 1fb48dd` confirms), the record now says the check "would take about five lines in `Directory.Build.props`, which already exists", and it states outright that the deferral is a judgement about ownership rather than a technical obstacle. That last clause is the part that matters, and it is a better record than the one I would have accepted. My concern was never only that the sentence was false — it was that a reader in a year would find a plausible-sounding technical excuse and quietly inherit it. A record that says "this is cheap and available, and we chose not to" cannot be misread that way, and it puts the decision back where it belongs, with whoever owns enforcement. The three reasons are stated more precisely than I put them, particularly *"it would not protect anything the ignore rule does not already protect"* — that is the crux, and it is the sentence that makes the deferral obviously right rather than merely defensible. ## The promise now has an owner Vikunja task 39 carries it, and the comment is better than a cross-reference. It names both controls, the five-line cost, the fallback mechanism, the fresh-clone window, the detail that even `list` closes it, and that `.gitignore` is currently the only control in that path — so whoever picks up the CI ticket has the full picture without reading an ADR first. Added with `add_comment`, so no fields were clobbered. "A promise that exists only in an ADR is a promise nobody is scheduled to keep" is the right principle and it has been applied rather than just written. ## Both non-blocking items **`user-secrets.json` ignored.** I agree with the reasoning, including the part that resisted flattening the two names into one hazard. `secrets.json` works and gives no feedback; `user-secrets.json` does nothing and is noticed in minutes. They are not the same risk, and the comment keeps that visible while still ignoring both — the trade is one irreversible outcome against one mildly annoying one, and irreversible wins. Verified independently at this head: | Path | Result | |---|---| | `user-secrets.example.json` | **not ignored** — the template is still live and `git ls-files` confirms it is tracked | | `user-secrets.json` | ignored, `.gitignore:450` | | `src/PlaceMark.Api/user-secrets.json` | ignored, `.gitignore:450` | | `secrets.json`, `src/PlaceMark.Api/secrets.json` | ignored, `.gitignore:449` | Both directions correct, and the new unanchored pattern does not catch the template by prefix — which was the one way this change could have silently un-shipped it. **Consolidation.** The split is well chosen and the principle behind it is sound: the frozen document holds the framework behaviour that could change, the living files hold instructions that stay true either way. So if Microsoft ever roots the provider unconditionally, exactly one document is wrong and it is the one that is allowed to be, because it is dated and superseded rather than edited. The README bullet, `.gitignore` comment and template header now each say the actionable thing — it loads on a fresh clone, do not create it, apply the template instead — and defer the mechanism. Nothing lost: the two facts a reader acts on, and the reason nobody in review will have seen it, both survive in every copy. ## Re-verified at this head - `dotnet build` — **0 Warning(s), 0 Error(s)**, under `TreatWarningsAsErrors`, `EnforceCodeStyleInBuild` and `AnalysisMode=Recommended` from `Directory.Build.props`; `dotnet test --no-build` — exit 0; `dotnet format --verify-no-changes` — exit 0. - Template still applies from its path after the header rewrite — `dotnet user-secrets set --project src/PlaceMark.Api < user-secrets.example.json` → "Successfully saved 1 secrets", and the API resolved the docker-compose default. Worth re-running rather than assuming, since the edited comments sit inside the JSON the parser has to tolerate. - `src/PlaceMark.Api` contains no `secrets.json`-shaped artefact; `PlaceMark.Api.csproj` still carries `<UserSecretsId>` alone. ADR title unchanged, so the index row at `docs/adr/README.md:24` still matches. No American spellings in any changed file. - The load-bearing behavioural claims were verified in the previous round and are unchanged by this diff: project-directory `secrets.json` loads with the store absent; `user-secrets.json` loads from nowhere; `secrets.json` at the repository root loads from nowhere; `dotnet user-secrets list` closes the window while reporting that nothing is configured. Store cleared and `~/.microsoft/usersecrets/2b9d7035-…` deleted afterwards; the directory is empty as I found it. All probing was done in a `git archive` export outside the reviewed tree, since removed. ## One follow-up, explicitly not blocking Vikunja task 37 (task #3) still has no comment — `list_comments` returns empty. This is a bookkeeping item for merge time, not a code change: the third acceptance criterion reads as unmet, and the record of why it was replaced by something stronger lives only in this PR and ADR-0014. Given the effort spent making the *other* deferral properly owned, it would be a shame for this one to be the loose end. Merge when ready. The strategy is right, the trap that the strategy accidentally created has been removed rather than documented, and the record of both is accurate — including about its own limits.

rob merged commit 6d4e7e1a36 into main

2026-08-02 21:07:06 +00:00

rob deleted branch feat/configuration-secrets

2026-08-02 21:07:06 +00:00

rob referenced this pull request from a commit

2026-08-02 21:07:07 +00:00

Define the configuration and secrets strategy (#9)

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

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

No description provided.

Rows
Columns

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

The deviation — appsettings.Development.json.example was not created

Precedence — observed, not recited

Only one key is defined

ADR-0014

Verification

Rebase note

Flagged

On the deviation — I agree with it

Blocking

Non-blocking

ADR-0014 as a record

What I ran

The fix, verified

Blocking — one sentence, factually wrong, about the repository's own contents

Your question: defer the build-time check, or carry a narrower version here?

Non-blocking

Everything else re-verified at this head

The blocking finding

The promise now has an owner

Both non-blocking items

Re-verified at this head

One follow-up, explicitly not blocking

The deviation — `appsettings.Development.json.example` was not created