Global exception handling and Problem Details (task 58) #18

Merged

rob merged 3 commits from feat/problem-details into main 2026-08-03 12:07:56 +00:00

Conversation 3 Commits 3 Files changed 13 +830 -18

rob commented 2026-08-03 11:33:33 +00:00

Owner

Copy link

Registers UseExceptionHandler immediately inside UseHttpLogging, which fixes ADR-0015's StatusCode: 200 defect. Commenting the handler out fails 10 of 15 API tests, one with exactly the old symptom:

requestLog.State["StatusCode"] should be 500 but was 200

The Development decision

The handler runs in every environment, unguarded, with an identical body everywhere and never any exception detail.

The customary if (!app.Environment.IsDevelopment()) guard would have fixed the logged status in deployed environments and left the 200 locally — where these logs are read most and doubted least — because the developer exception page sits outside all user middleware and cannot be brought inside. That is precisely why ADR-0015 handed this decision here.

The cost is losing that page. What it showed is in the console beside the request, at Error, with the stack trace, under the trace id the response body quotes.

Also rejected: including the exception message in Development only. It makes the failure path the one part of the API never exercised in the shape it ships in, and puts a conditional leak one misconfiguration from production.

Design

ApiExceptionHandler answers its own exception types and declines everything else, so there is no branch rendering an arbitrary exception into a response that could later be widened into one.

Diagnostics suppressed for expected failures — without it a 404 was logged at Error with a stack trace. A test pins that.

Nothing added to PlaceMark.Contracts. The wire shape is the framework's ProblemDetails, which both sides already have; a hand-written copy would be a second definition of a format neither controls. Its traceId is emitted without configuration, which is what makes a detail-free 500 workable.

Tests inject a throwing middleware via a startup filter rather than mapping a route that exists only to fail.

Notes

Criterion 3 ("exception details logged with correlation ID") was verified, not reimplemented — ADR-0015's work already does it.

TestServer rethrows rather than producing a 200-logged response, so the deployed half of the old defect cannot be re-demonstrated through it; the Development test reproduces it verbatim. Recorded in ADR-0022.

Possible ADR collision: this takes 0022. PR #17 is in flight — if it also claims 0022, one needs renumbering.

Registers `UseExceptionHandler` immediately inside `UseHttpLogging`, which **fixes ADR-0015's `StatusCode: 200` defect**. Commenting the handler out fails 10 of 15 API tests, one with exactly the old symptom: ``` requestLog.State["StatusCode"] should be 500 but was 200 ``` ## The Development decision **The handler runs in every environment, unguarded, with an identical body everywhere and never any exception detail.** The customary `if (!app.Environment.IsDevelopment())` guard would have fixed the logged status in deployed environments and left the `200` **locally** — where these logs are read most and doubted least — because the developer exception page sits outside all user middleware and cannot be brought inside. That is precisely why ADR-0015 handed this decision here. The cost is losing that page. What it showed is in the console beside the request, at `Error`, with the stack trace, under the trace id the response body quotes. Also rejected: including the exception message in Development only. It makes the failure path the one part of the API never exercised in the shape it ships in, and puts a conditional leak one misconfiguration from production. ## Design `ApiExceptionHandler` answers its own exception types and **declines everything else**, so there is no branch rendering an arbitrary exception into a response that could later be widened into one. Diagnostics suppressed for expected failures — without it a 404 was logged at `Error` with a stack trace. A test pins that. **Nothing added to `PlaceMark.Contracts`.** The wire shape is the framework's `ProblemDetails`, which both sides already have; a hand-written copy would be a second definition of a format neither controls. Its `traceId` is emitted without configuration, which is what makes a detail-free 500 workable. Tests inject a throwing middleware via a startup filter rather than mapping a route that exists only to fail. ## Notes Criterion 3 ("exception details logged with correlation ID") was **verified, not reimplemented** — ADR-0015's work already does it. `TestServer` rethrows rather than producing a `200`-logged response, so the *deployed* half of the old defect cannot be re-demonstrated through it; the Development test reproduces it verbatim. Recorded in ADR-0022. **Possible ADR collision:** this takes 0022. PR #17 is in flight — if it also claims 0022, one needs renumbering.

rob added 1 commit 2026-08-03 11:33:33 +00:00

Add global exception handling and Problem Details ... 3ac7686e46

Registers UseExceptionHandler immediately inside UseHttpLogging, which is
what fixes ADR-0015's defect: the request log reported an unhandled
exception as StatusCode 200 because nothing wrote the 500 before
HttpLogging read the status. Commenting the handler out fails ten of
fifteen API tests, one with exactly that symptom.

The handler runs in every environment, unguarded, with an identical
response body everywhere and never any exception detail. The customary
IsDevelopment guard would have left the 200 in place locally — where
these logs are read most and doubted least — because the developer
exception page sits outside all user middleware and cannot be brought
inside. The cost is that page; what it showed is in the console beside
the request, at Error, with the stack trace, under the trace id the
response body quotes.

ApiExceptionHandler answers its own exception types and declines
everything else, so there is no branch rendering an arbitrary exception
into a response that could later be widened into one.

Suppresses Error-level diagnostics for expected failures — without it a
404 was logged at Error with a stack trace, which a test now pins.

Nothing added to Contracts: the wire shape is the framework's
ProblemDetails, which both sides already have. Its traceId is emitted
without configuration, which is what makes a detail-free 500 workable.

Tests inject a throwing middleware through a startup filter rather than
mapping a route that exists only to fail.

rob reviewed 2026-08-03 11:46:00 +00:00

rob left a comment

Copy link

Verdict: changes needed

Independent review. Everything below was run against 3ac7686 in a detached worktree, not read off the diff.

What I executed

All four CI commands, on the pristine branch, Release, twice (before and after my experiments):

dotnet restore PlaceMark.slnx                                     ok
dotnet build   PlaceMark.slnx --no-restore -c Release             0 Warning(s), 0 Error(s)
dotnet test    PlaceMark.slnx --no-build   -c Release             15/15 Api, 10/10 Infrastructure, 1/1 WebUI
dotnet format  PlaceMark.slnx --verify-no-changes --no-restore    exit 0

Then the API itself, dotnet run, with temporary throwing routes, in Development and in Production (JSON console formatter), against Accept: application/json, Accept: text/html, a browser-shaped Accept, and no Accept at all, each carrying a known traceparent.

The central claim holds

Commenting out app.UseExceptionHandler(); fails 11 of 15, not 10 — every test in ExceptionHandlingTests. One fails with exactly the ADR-0015 symptom:

requestLog.State["StatusCode"] should be 500 but was 200

and the deployed-configuration tests fail on the rethrow, as ADR-0022 predicts. So the ordering claim is real and the tests pin it. (The 10 in the description is worth correcting, since the description invites the check.)

Running for real, both environments, the logged status is now the delivered status:

Error | Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware
      | TraceId:1111…1111  System.InvalidOperationException: connection failed for Host=db.internal;…Password=hunter2
      | + full stack trace
Information | …HttpLoggingMiddleware | {"Path":"/review/boom","StatusCode":500,…}

Nothing leaked in any of the four Accept shapes, in either environment: the body is {type,title,status,traceId} and nothing else. A NotFoundException produced a 404 with its detail and no Error record, only the request log at 404 — so the suppression does what it says, and a genuine 500 still logs at Error with a stack trace under the caller's trace id. Ticket #58's three criteria are met.

---

Blocking

1. NotFoundException + Accept: text/html delivers 500, logs 404, and defeats the suppression

Observed, Production, curl -H 'Accept: text/html':

exception	delivered	logged	Error records
ValidationFailedException	400, empty body	400	none
ConflictException	409, empty body	409	none
NotFoundException	500, empty body	404	two

The 404 case only. TryWriteAsync declines, so the handler returns false having already set Response.StatusCode = 404; the middleware's fallback path then hits AllowStatusCode404Response (false by default) and throws:

System.InvalidOperationException: The exception handler configured on ExceptionHandlerOptions
produced a 404 status response. This InvalidOperationException containing the original exception
was thrown …

Kestrel logs that at Error, the middleware logs the NotFoundException at Error with a stack trace despite SuppressDiagnosticsCallback, and the request log records 404 while the caller receives 500.

That last part is the same class of defect this PR exists to remove — the request log disagreeing with what the caller got — reappearing in a corner. It also makes this consequence in ADR-0022 wrong as written:

A caller that will not accept JSON gets an empty body. … Nothing leaks and the status is still logged correctly

True for the unhandled 500; false for an ApiException. Since an accepted ADR's body is immutable under ADR-0001, this needs correcting before it is frozen.

The fix I verified is one line in ApiExceptionHandler — the handler did handle it, so say so, and treat an unwritten body as the writer's business:

await problemDetailsService.TryWriteAsync(new ProblemDetailsContext { … });

return true;

With that, Accept: text/html gives a clean 404 with an empty body, no Error records, and a request log that agrees with the caller; the unhandled 500 path is unchanged. A test for Accept: text/html would have caught this and is the obvious thing to add — it is the one Accept shape ADR-0022 calls out and the only one no test covers.

2. ADR-0022's rejection of a Contracts type rests on a claim that does not compile

both sides already have one — Microsoft.AspNetCore.Mvc.ProblemDetails, from the framework, on the client as well as the server

PlaceMark.WebUI does not. It is a Microsoft.NET.Sdk.BlazorWebAssembly project referencing Microsoft.AspNetCore.Components.WebAssembly and PlaceMark.Contracts, with no FrameworkReference to Microsoft.AspNetCore.App. Dropping a one-line probe into it:

error CS0234: The type or namespace name 'Mvc' does not exist in the namespace
'Microsoft.AspNetCore' (are you missing an assembly reference?)

I agree with the decision — nothing belongs in Contracts today, when nothing consumes a problem body — but not with this reasoning, and the reasoning is what gets frozen. It matters because the alternative it dismisses may be the right answer later: when the typed HTTP client of ADR-0008 reads a failure, it either takes a dependency on an ASP.NET Core assembly for one DTO (on a payload ADR-0008 says to watch) or hand-writes a reader — and a hand-written reader in WebUI is precisely the "second definition" this section rejects, only in the worse place. On ADR-0009's terms that is an argument for a contract, not against one.

Please either correct the claim and re-argue the rejection on grounds that hold (nothing consumes it yet; decide it alongside the typed client), or change the decision.

3. The developer exception page can be brought inside UseHttpLogging

This is the load-bearing premise of the whole Development argument, in ADR-0015, in ADR-0022 and in the description:

the developer exception page sits outside all user middleware and cannot be brought inside

The middleware WebApplication registers automatically is outside and cannot be moved. An explicitly registered one is not. Observed, Development:

app.UseHttpLogging();
if (app.Environment.IsDevelopment()) { app.UseDeveloperExceptionPage(); }
else { app.UseExceptionHandler(); }

GET /review/boom  ->  500, Content-Type: text/html
HttpLoggingMiddleware … Path: /review/boom  StatusCode: 500

The page and a correctly logged 500. So the choice was never "the page or an honest local log" — that dichotomy is stated three times in this change and it is not the case.

The decision may well survive this. The two independent arguments for it are good ones and I would not want them weakened: one shape of failure exercised in every environment, and no IsDevelopment() branch standing between a misconfiguration and a stack trace on the wire — and the page is, after all, a Development-only leak of exactly the kind the "message in Development only" alternative was rejected for. But this is the strongest alternative available and ## Alternatives considered does not contain it. Add it and reject it on those grounds, so the next reader does not rediscover it and conclude the record was mistaken about the framework.

---

Non-blocking

• traceId format. The body carries the full W3C id, 00-1111…1111-3e5614c94d5f0347-01; the logs carry the bare 32-hex trace id, TraceId:1111…1111. Someone handed a trace id by a user and pasting it whole into a log search finds nothing. ExceptionHandlingTests uses ShouldContain rather than ShouldBe, so it already knows this. One sentence in the README's Failing a request section saying which part to search for would close it; changing the emitted value would cost the "without configuration" property the ADR values, so I would not.
• No ADR collision. PR #17 touches no docs/adr/ file, so 0022 is uncontested and the caveat in the description can go.
• ProblemDetailsContext.Exception = apiException hands the exception into the body-writing pipeline. Nothing reads it today, but it is the one thread by which a future CustomizeProblemDetails could render an exception — a mild tension with "there is no branch here that could be made to render an exception".
• Forward-looking: once endpoints take a CancellationToken, a client disconnect surfaces as OperationCanceledException, which this handler declines — so it becomes an Error record and an attempted 500 for a request nobody is listening to. Worth a ticket rather than a change here.

Judged as asked

1. Unguarded in Development — right call, wrong reason. Endorsed on the uniform-failure-shape and no-conditional-leak arguments, which I think are correct and well made. Not endorsed on "cannot be brought inside", which is false (blocking item 3). The mitigation is real and I checked it: everything the page showed is in the console at Error, with the stack trace, under the trace id the body quotes.
2. Declining unknown exceptions — sound. Verified the middleware's own fallback yields {type,title,status,traceId} and nothing from the exception, in both environments. The "no branch to widen" argument is worth the extra return false. The 404 interaction in blocking item 1 is a consequence of declining after setting a status, not of declining as such.
3. Suppression — nothing lost, one hole. A 500 still logs at Error with a full stack trace in both environments; 400/404/409 leave only the request log at the right status. The hole is item 1, where an ApiException gets logged at Error anyway. The ADR's "if that ever proves too thin, add an Information record" is the right escape hatch.
4. Nothing in Contracts — agree with the outcome, reject the stated reason. See item 2.
5. ThrowingStartupFilter — good. Appending inside Configure puts it inside every middleware Program.cs registers, which is what makes the assertions mean anything, and it keeps a route that exists only to fail out of the API. I confirmed the pipeline shape from the failure stack trace: HttpLoggingMiddleware → ThrowingStartupFilter. Preferable to a [Conditional] endpoint or a test-only MapGet.
6. ADR-0022 vs ADR-0001 — conformant. Number is next and unclaimed, filename slug matches the index link, Status/Date/Source present, supersession lines correctly omitted, all four required sections present with a genuine alternatives section. Index row reads | [0022](0022-problem-details-in-every-environment.md) | Answer every failed request with Problem Details, in every environment | Accepted | — correct, and the title matches the file's H1. British English is clean throughout; the two Customize hits are framework symbols.

The code change is small, correct in its main line, and unusually well evidenced. Fix the 404/text/html path and the two ADR claims that observation contradicts, and this is mergeable.

Verdict: changes needed Independent review. Everything below was run against `3ac7686` in a detached worktree, not read off the diff. ## What I executed All four CI commands, on the pristine branch, Release, twice (before and after my experiments): ``` dotnet restore PlaceMark.slnx ok dotnet build PlaceMark.slnx --no-restore -c Release 0 Warning(s), 0 Error(s) dotnet test PlaceMark.slnx --no-build -c Release 15/15 Api, 10/10 Infrastructure, 1/1 WebUI dotnet format PlaceMark.slnx --verify-no-changes --no-restore exit 0 ``` Then the API itself, `dotnet run`, with temporary throwing routes, in `Development` and in `Production` (JSON console formatter), against `Accept: application/json`, `Accept: text/html`, a browser-shaped `Accept`, and no `Accept` at all, each carrying a known `traceparent`. ## The central claim holds Commenting out `app.UseExceptionHandler();` fails **11** of 15, not 10 — every test in `ExceptionHandlingTests`. One fails with exactly the ADR-0015 symptom: ``` requestLog.State["StatusCode"] should be 500 but was 200 ``` and the deployed-configuration tests fail on the rethrow, as ADR-0022 predicts. So the ordering claim is real and the tests pin it. (The `10` in the description is worth correcting, since the description invites the check.) Running for real, both environments, the logged status is now the delivered status: ``` Error | Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware | TraceId:1111…1111 System.InvalidOperationException: connection failed for Host=db.internal;…Password=hunter2 | + full stack trace Information | …HttpLoggingMiddleware | {"Path":"/review/boom","StatusCode":500,…} ``` Nothing leaked in any of the four `Accept` shapes, in either environment: the body is `{type,title,status,traceId}` and nothing else. A `NotFoundException` produced a 404 with its `detail` and **no** `Error` record, only the request log at 404 — so the suppression does what it says, and a genuine 500 still logs at `Error` with a stack trace under the caller's trace id. Ticket #58's three criteria are met. --- ## Blocking ### 1. `NotFoundException` + `Accept: text/html` delivers 500, logs 404, and defeats the suppression Observed, Production, `curl -H 'Accept: text/html'`: | exception | delivered | logged | `Error` records | | --- | --- | --- | --- | | `ValidationFailedException` | 400, empty body | 400 | none | | `ConflictException` | 409, empty body | 409 | none | | `NotFoundException` | **500**, empty body | **404** | **two** | The 404 case only. `TryWriteAsync` declines, so the handler returns `false` having already set `Response.StatusCode = 404`; the middleware's fallback path then hits `AllowStatusCode404Response` (false by default) and throws: ``` System.InvalidOperationException: The exception handler configured on ExceptionHandlerOptions produced a 404 status response. This InvalidOperationException containing the original exception was thrown … ``` Kestrel logs that at `Error`, the middleware logs the `NotFoundException` at `Error` with a stack trace *despite* `SuppressDiagnosticsCallback`, and the request log records 404 while the caller receives 500. That last part is the same class of defect this PR exists to remove — the request log disagreeing with what the caller got — reappearing in a corner. It also makes this consequence in ADR-0022 wrong as written: > **A caller that will not accept JSON gets an empty body.** … Nothing leaks and the status is still logged correctly True for the unhandled 500; false for an `ApiException`. Since an accepted ADR's body is immutable under ADR-0001, this needs correcting before it is frozen. The fix I verified is one line in `ApiExceptionHandler` — the handler *did* handle it, so say so, and treat an unwritten body as the writer's business: ```csharp await problemDetailsService.TryWriteAsync(new ProblemDetailsContext { … }); return true; ``` With that, `Accept: text/html` gives a clean 404 with an empty body, no `Error` records, and a request log that agrees with the caller; the unhandled 500 path is unchanged. A test for `Accept: text/html` would have caught this and is the obvious thing to add — it is the one `Accept` shape ADR-0022 calls out and the only one no test covers. ### 2. ADR-0022's rejection of a `Contracts` type rests on a claim that does not compile > both sides already have one — `Microsoft.AspNetCore.Mvc.ProblemDetails`, from the framework, on the client as well as the server `PlaceMark.WebUI` does not. It is a `Microsoft.NET.Sdk.BlazorWebAssembly` project referencing `Microsoft.AspNetCore.Components.WebAssembly` and `PlaceMark.Contracts`, with no `FrameworkReference` to `Microsoft.AspNetCore.App`. Dropping a one-line probe into it: ``` error CS0234: The type or namespace name 'Mvc' does not exist in the namespace 'Microsoft.AspNetCore' (are you missing an assembly reference?) ``` I agree with the *decision* — nothing belongs in `Contracts` today, when nothing consumes a problem body — but not with this reasoning, and the reasoning is what gets frozen. It matters because the alternative it dismisses may be the right answer later: when the typed HTTP client of ADR-0008 reads a failure, it either takes a dependency on an ASP.NET Core assembly for one DTO (on a payload ADR-0008 says to watch) or hand-writes a reader — and a hand-written reader in `WebUI` is precisely the "second definition" this section rejects, only in the worse place. On ADR-0009's terms that is an argument *for* a contract, not against one. Please either correct the claim and re-argue the rejection on grounds that hold (nothing consumes it yet; decide it alongside the typed client), or change the decision. ### 3. The developer exception page *can* be brought inside `UseHttpLogging` This is the load-bearing premise of the whole Development argument, in ADR-0015, in ADR-0022 and in the description: > the developer exception page sits outside all user middleware and cannot be brought inside The middleware `WebApplication` registers automatically is outside and cannot be moved. An explicitly registered one is not. Observed, Development: ```csharp app.UseHttpLogging(); if (app.Environment.IsDevelopment()) { app.UseDeveloperExceptionPage(); } else { app.UseExceptionHandler(); } ``` ``` GET /review/boom -> 500, Content-Type: text/html HttpLoggingMiddleware … Path: /review/boom StatusCode: 500 ``` The page *and* a correctly logged 500. So the choice was never "the page or an honest local log" — that dichotomy is stated three times in this change and it is not the case. The decision may well survive this. The two independent arguments for it are good ones and I would not want them weakened: one shape of failure exercised in every environment, and no `IsDevelopment()` branch standing between a misconfiguration and a stack trace on the wire — and the page is, after all, a Development-only leak of exactly the kind the "message in Development only" alternative was rejected for. But this is the strongest alternative available and `## Alternatives considered` does not contain it. Add it and reject it on those grounds, so the next reader does not rediscover it and conclude the record was mistaken about the framework. --- ## Non-blocking - **`traceId` format.** The body carries the full W3C id, `00-1111…1111-3e5614c94d5f0347-01`; the logs carry the bare 32-hex trace id, `TraceId:1111…1111`. Someone handed a trace id by a user and pasting it whole into a log search finds nothing. `ExceptionHandlingTests` uses `ShouldContain` rather than `ShouldBe`, so it already knows this. One sentence in the README's *Failing a request* section saying which part to search for would close it; changing the emitted value would cost the "without configuration" property the ADR values, so I would not. - **No ADR collision.** PR #17 touches no `docs/adr/` file, so 0022 is uncontested and the caveat in the description can go. - **`ProblemDetailsContext.Exception = apiException`** hands the exception into the body-writing pipeline. Nothing reads it today, but it is the one thread by which a future `CustomizeProblemDetails` could render an exception — a mild tension with "there is no branch here that could be made to render an exception". - **Forward-looking:** once endpoints take a `CancellationToken`, a client disconnect surfaces as `OperationCanceledException`, which this handler declines — so it becomes an `Error` record and an attempted 500 for a request nobody is listening to. Worth a ticket rather than a change here. ## Judged as asked 1. **Unguarded in Development — right call, wrong reason.** Endorsed on the uniform-failure-shape and no-conditional-leak arguments, which I think are correct and well made. Not endorsed on "cannot be brought inside", which is false (blocking item 3). The mitigation is real and I checked it: everything the page showed is in the console at `Error`, with the stack trace, under the trace id the body quotes. 2. **Declining unknown exceptions — sound.** Verified the middleware's own fallback yields `{type,title,status,traceId}` and nothing from the exception, in both environments. The "no branch to widen" argument is worth the extra `return false`. The 404 interaction in blocking item 1 is a consequence of declining *after* setting a status, not of declining as such. 3. **Suppression — nothing lost, one hole.** A 500 still logs at `Error` with a full stack trace in both environments; 400/404/409 leave only the request log at the right status. The hole is item 1, where an `ApiException` gets logged at `Error` anyway. The ADR's "if that ever proves too thin, add an `Information` record" is the right escape hatch. 4. **Nothing in `Contracts` — agree with the outcome**, reject the stated reason. See item 2. 5. **`ThrowingStartupFilter` — good.** Appending inside `Configure` puts it inside every middleware `Program.cs` registers, which is what makes the assertions mean anything, and it keeps a route that exists only to fail out of the API. I confirmed the pipeline shape from the failure stack trace: `HttpLoggingMiddleware → ThrowingStartupFilter`. Preferable to a `[Conditional]` endpoint or a test-only `MapGet`. 6. **ADR-0022 vs ADR-0001 — conformant.** Number is next and unclaimed, filename slug matches the index link, `Status`/`Date`/`Source` present, supersession lines correctly omitted, all four required sections present with a genuine alternatives section. Index row reads `| [0022](0022-problem-details-in-every-environment.md) | Answer every failed request with Problem Details, in every environment | Accepted |` — correct, and the title matches the file's H1. British English is clean throughout; the two `Customize` hits are framework symbols. The code change is small, correct in its main line, and unusually well evidenced. Fix the 404/`text/html` path and the two ADR claims that observation contradicts, and this is mergeable.

rob added 1 commit 2026-08-03 11:54:51 +00:00

Action review on Problem Details ... 43c1cddc78

A NotFoundException with Accept: text/html delivered 500 while the
request log said 404, and logged twice past the suppression. Returning
TryWriteAsync's result meant a declined body read as a 404 the handler
had not claimed, so the middleware rethrew. The handler now writes the
body if it can and claims the exception regardless. Four tests hold the
shape, each asserting delivered status equals logged status.

Corrects ADR-0022's claim that ProblemDetails is available on the client:
it is not — PlaceMark.WebUI has no Microsoft.AspNetCore.App reference and
a probe gives CS0234. The conclusion stands but on ADR-0009's terms
instead: Contracts ships empty and each type is written alongside its
consumer, and no client code reads an error body yet. Deferred, not
dismissed, with the cost of a hand-written mirror stated.

Accepts the refutation that the developer exception page cannot be
brought inside UseHttpLogging. It can: registered explicitly after
UseHttpLogging it answers and the request log records 500. It is now the
first alternative, rejected on the response rather than the log — it
makes Development the one environment whose failure body no client is
written against, and returns exception detail to a body, which is the
trade already rejected.

ADR-0015 states that premise and is frozen, so ADR-0022 says outright
that it is wrong and why the correction lives here. The same claim was
repeated in Program.cs and a test doc comment; both rewritten.

Makes the body's traceId the bare 32-hex id so it matches the log
records by eye, and stops handing the exception to the body pipeline.

rob referenced this pull request 2026-08-03 11:57:07 +00:00

Apply schema scripts from a console command, rehearsed in CI (task 45) #19

rob reviewed 2026-08-03 12:01:57 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed at 43c1cdd. All three blocking findings are fixed, and I re-ran the evidence rather than taking the summary on trust. One recommendation below that I am not blocking on.

Re-verified

Four CI commands at 43c1cdd, Release: restore ok, build 0 warnings / 0 errors, test 30 passed (19 Api, 10 Infrastructure, 1 WebUI), format --verify-no-changes exit 0.

Handler commented out: 15 of 19 fail, exactly as stated; the 4 survivors are the pre-existing health and request-logging tests.

Blocking 1 — the 404 defect. Fixed, and pinned. Reverting only return true; back to returning the writer's result fails exactly one test — Send_NotFoundExceptionToACallerThatWillNotTakeJson_DeliversTheStatusItLogs — which is the right one, since 404 is the only status AllowStatusCode404Response guards. Running for real with Accept: text/html, Production and Development, delivered status now equals logged status across the board:

request	delivered	logged	Error records
unhandled, application/json	500 + problem body	500	1, with stack trace
unhandled, text/html	500, empty	500	1, with stack trace
NotFoundException, text/html	404, empty	404	none
ValidationFailedException, text/html	400, empty	400	none
ConflictException, text/html	409, empty	409	none

No Kestrel rethrow, no AllowStatusCode404Response InvalidOperationException, and the suppression is no longer bypassed. Dropping Exception from the ProblemDetailsContext is the right call and closes the nit I raised alongside it.

Blocking 2 — the Contracts reasoning. The rewritten alternative is accurate: I reproduced CS0234 again at this head. Deferring on ADR-0009's own "ships empty, each type written alongside its consumer" terms is a better argument than the one it replaces, and it now states the cost a future decider needs rather than asserting a shared definition that does not exist. Accepted.

Blocking 3 — the developer exception page. The correction is right and the rejection is now made on the correct grounds. I re-measured it, using the two-line reversal recipe the Consequences section prescribes (UseDeveloperExceptionPage() under a Development guard, after UseExceptionHandler()) — the recipe works as written, and the request log records 500 with the page in place. To a JSON client the page returns:

{"type":"…rfc9110#section-15.6.1","title":"System.InvalidOperationException",
 "status":500,"detail":"connection failed for Host=db.internal;…Password=hunter2",
 "exception":{"details":"System.InvalidOperationException: … \n   at Program…"}}

title is the exception type, detail is the message, and exception.details carries the whole stack trace — in application/problem+json, the same content type a deployed environment uses for a body that carries none of it. That is the decisive fact and the ADR is right to rest on it rather than on the log.

traceId. Now byte-identical to the log's TraceId, including the server-generated one when no traceparent is supplied — body 808941eb1d37ec08af98f8360d52b6ac, log TraceId:808941eb1d37ec08af98f8360d52b6ac. It applies to every problem body, not just the handler's: a TypedResults.Problem probe carried the bare id too. Tightening the test to ShouldBe is the right move. The HttpContext.TraceIdentifier fallback is better than it looks — in the configuration that suppresses the Activity there is no TraceId in the logs to match anyway, and TraceIdentifier is exactly what the scopes carry as RequestId, so the fallback stays searchable rather than becoming a dead id.

Nothing leaks, re-checked at this head in both environments across application/json, text/html, */* and no Accept header: {type,title,status,traceId} for a fault, plus detail for a refusal, and nothing else. Genuine 500s still log at Error with a full stack trace.

Recommendation, not blocking

ADR-0015 should gain a Partially superseded by line. ADR-0022 says an accepted ADR "is frozen, so ADR-0015 keeps its text" and puts the correction here alone. That is right about the body and not about the metadata: ADR-0001 expressly permits editing "the Status, Superseded by and Partially superseded by fields, adding whichever lines the template left out — and to the matching row in the index", and provides partial supersession for precisely this case, where the old record otherwise stays Accepted. The repo already does it — ADR-0002 carries

- **Partially superseded by:**
  [ADR-0017](0017-link-external-identities-by-explicit-action.md) — the
  linking-by-verified-email clause in the Decision below, …

with the index row reading Accepted (partly superseded by 0017). ADR-0015 has neither, so a reader who arrives there first — from CLAUDE.md, or from ADR-0022's own Context link — gets the false claim with no pointer to the correction, and ADR-0022's "anyone reading ADR-0015's consequences on this point should read this record next" has no way to reach them.

I am not blocking because, unlike the ADR body, this does not freeze: status metadata stays editable indefinitely, so a follow-up commit fixes it as well as this one would. But it is two lines — one field on ADR-0015 naming the claim it displaces, one index row — and it is cheaper here than as a ticket.

Smaller observations

• The byte figures in ADR-0022 are probe-specific. I measured 20,163 bytes of HTML and 1,063 bytes of JSON against the ADR's 16,618 and 809 — same machine, different throw site and longer file paths in the stack trace. Not a contradiction, and the shape claim is what matters, but since 0022's body is still editable, wording them as approximate (or naming the probe) would stop a later reader treating them as reproducible constants and concluding the record is wrong.
• Three of the four new WillNotTakeJson tests pass against the pre-fix code. Only the 404 one is a regression test; the other three document the intended contract for statuses that never broke. That is worth having and I would keep them — noting it only so nobody assumes all four are load-bearing if the guard behaviour changes.
• OperationCanceledException — I agree with declining it. Nothing throws it today, there are no endpoints and no cancellation tokens, and this record's Source line commits every claim in it to observation. A speculative consequence in a frozen document is worse than a ticket raised when there is something to measure. Right call, and right reason.
• CustomizeProblemDetails was the one thing I said I would not do, on the grounds that it costs the "emitted without configuration" property. Seeing it run, I was wrong to weigh it that way: it is a single expression, it applies uniformly to every problem body the API writes, and it converts the id from something a reader must edit before searching into something they can paste. Worth the line.

Good response to the review — every finding was reproduced before it was fixed, and the ADR is now accurate about the thing it previously got wrong, including where it inherited the error. Mergeable.

Verdict: mergeable Re-reviewed at `43c1cdd`. All three blocking findings are fixed, and I re-ran the evidence rather than taking the summary on trust. One recommendation below that I am not blocking on. ## Re-verified Four CI commands at `43c1cdd`, Release: `restore` ok, `build` 0 warnings / 0 errors, `test` **30 passed** (19 Api, 10 Infrastructure, 1 WebUI), `format --verify-no-changes` exit 0. Handler commented out: **15 of 19 fail**, exactly as stated; the 4 survivors are the pre-existing health and request-logging tests. **Blocking 1 — the 404 defect.** Fixed, and pinned. Reverting only `return true;` back to returning the writer's result fails exactly one test — `Send_NotFoundExceptionToACallerThatWillNotTakeJson_DeliversTheStatusItLogs` — which is the right one, since 404 is the only status `AllowStatusCode404Response` guards. Running for real with `Accept: text/html`, Production and Development, delivered status now equals logged status across the board: | request | delivered | logged | `Error` records | | --- | --- | --- | --- | | unhandled, `application/json` | 500 + problem body | 500 | 1, with stack trace | | unhandled, `text/html` | 500, empty | 500 | 1, with stack trace | | `NotFoundException`, `text/html` | **404**, empty | **404** | none | | `ValidationFailedException`, `text/html` | 400, empty | 400 | none | | `ConflictException`, `text/html` | 409, empty | 409 | none | No Kestrel rethrow, no `AllowStatusCode404Response` `InvalidOperationException`, and the suppression is no longer bypassed. Dropping `Exception` from the `ProblemDetailsContext` is the right call and closes the nit I raised alongside it. **Blocking 2 — the `Contracts` reasoning.** The rewritten alternative is accurate: I reproduced `CS0234` again at this head. Deferring on ADR-0009's own "ships empty, each type written alongside its consumer" terms is a better argument than the one it replaces, and it now states the cost a future decider needs rather than asserting a shared definition that does not exist. Accepted. **Blocking 3 — the developer exception page.** The correction is right and the rejection is now made on the correct grounds. I re-measured it, using the two-line reversal recipe the Consequences section prescribes (`UseDeveloperExceptionPage()` under a Development guard, after `UseExceptionHandler()`) — the recipe works as written, and the request log records 500 with the page in place. To a JSON client the page returns: ```json {"type":"…rfc9110#section-15.6.1","title":"System.InvalidOperationException", "status":500,"detail":"connection failed for Host=db.internal;…Password=hunter2", "exception":{"details":"System.InvalidOperationException: … \n at Program…"}} ``` `title` is the exception type, `detail` is the message, and `exception.details` carries the whole stack trace — in `application/problem+json`, the same content type a deployed environment uses for a body that carries none of it. That is the decisive fact and the ADR is right to rest on it rather than on the log. **`traceId`.** Now byte-identical to the log's `TraceId`, including the server-generated one when no `traceparent` is supplied — body `808941eb1d37ec08af98f8360d52b6ac`, log `TraceId:808941eb1d37ec08af98f8360d52b6ac`. It applies to every problem body, not just the handler's: a `TypedResults.Problem` probe carried the bare id too. Tightening the test to `ShouldBe` is the right move. The `HttpContext.TraceIdentifier` fallback is better than it looks — in the configuration that suppresses the `Activity` there is no `TraceId` in the logs to match anyway, and `TraceIdentifier` is exactly what the scopes carry as `RequestId`, so the fallback stays searchable rather than becoming a dead id. **Nothing leaks**, re-checked at this head in both environments across `application/json`, `text/html`, `*/*` and no `Accept` header: `{type,title,status,traceId}` for a fault, plus `detail` for a refusal, and nothing else. Genuine 500s still log at `Error` with a full stack trace. ## Recommendation, not blocking **ADR-0015 should gain a `Partially superseded by` line.** ADR-0022 says an accepted ADR "is frozen, so ADR-0015 keeps its text" and puts the correction here alone. That is right about the *body* and not about the metadata: ADR-0001 expressly permits editing "the `Status`, `Superseded by` and `Partially superseded by` fields, adding whichever lines the template left out — and to the matching row in the index", and provides partial supersession for precisely this case, where the old record otherwise stays Accepted. The repo already does it — ADR-0002 carries ``` - **Partially superseded by:** [ADR-0017](0017-link-external-identities-by-explicit-action.md) — the linking-by-verified-email clause in the Decision below, … ``` with the index row reading `Accepted (partly superseded by 0017)`. ADR-0015 has neither, so a reader who arrives there first — from `CLAUDE.md`, or from ADR-0022's own Context link — gets the false claim with no pointer to the correction, and ADR-0022's "anyone reading ADR-0015's consequences on this point should read this record next" has no way to reach them. I am not blocking because, unlike the ADR body, this does not freeze: status metadata stays editable indefinitely, so a follow-up commit fixes it as well as this one would. But it is two lines — one field on ADR-0015 naming the claim it displaces, one index row — and it is cheaper here than as a ticket. ## Smaller observations - **The byte figures in ADR-0022 are probe-specific.** I measured 20,163 bytes of HTML and 1,063 bytes of JSON against the ADR's 16,618 and 809 — same machine, different throw site and longer file paths in the stack trace. Not a contradiction, and the shape claim is what matters, but since 0022's body is still editable, wording them as approximate (or naming the probe) would stop a later reader treating them as reproducible constants and concluding the record is wrong. - **Three of the four new `WillNotTakeJson` tests pass against the pre-fix code.** Only the 404 one is a regression test; the other three document the intended contract for statuses that never broke. That is worth having and I would keep them — noting it only so nobody assumes all four are load-bearing if the guard behaviour changes. - **`OperationCanceledException` — I agree with declining it.** Nothing throws it today, there are no endpoints and no cancellation tokens, and this record's Source line commits every claim in it to observation. A speculative consequence in a frozen document is worse than a ticket raised when there is something to measure. Right call, and right reason. - **`CustomizeProblemDetails` was the one thing I said I would not do**, on the grounds that it costs the "emitted without configuration" property. Seeing it run, I was wrong to weigh it that way: it is a single expression, it applies uniformly to every problem body the API writes, and it converts the id from something a reader must edit before searching into something they can paste. Worth the line. Good response to the review — every finding was reproduced before it was fixed, and the ADR is now accurate about the thing it previously got wrong, including where it inherited the error. Mergeable.

rob added 1 commit 2026-08-03 12:05:04 +00:00

Record ADR-0015's partial supersession in both directions ... 13afb13833

The correction to ADR-0015's 'cannot be brought inside' claim belonged in
ADR-0022, but the reason given was wrong: an accepted record's body is
frozen, its status metadata is not, and ADR-0001 expressly permits the
supersession fields. ADR-0002 already carries that pattern.

ADR-0015 now has a Partially superseded by line naming the single claim
corrected and stating that everything else in it stands. The index row
follows the prescribed form, and ADR-0022 carries the reciprocal
Supersedes line.

Removes the byte figures for the developer exception page probe. Review
measured different numbers on the same machine — they describe the throw
site, not the behaviour. What decides the argument is that the exception
message and stack trace are in the body, which is observable anywhere.

rob reviewed 2026-08-03 12:07:30 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed at 13afb13. As asked, I checked the supersession trail rather than re-running behaviour — but I did re-run the four CI commands, because Program.cs is in the diff.

Program.cs is comment-only

One line, 16 KB of stack trace → a page of stack trace. No behaviour change, so the verification at 43c1cdd stands. Confirmed green anyway at this head: build 0 warnings / 0 errors, 30 tests pass, format --verify-no-changes exit 0.

The supersession trail is coherent in both directions

ADR-0015 header	Partially superseded by: ADR-0022 — one claim in the Consequences below…
ADR-0015 body	untouched — the diff against 43c1cdd adds 9 lines and removes 0
Index row 0015	Accepted (partly superseded by 0022)
ADR-0022 header	Supersedes: ADR-0015 **in part** — its claim that … and nothing else.
Index row 0022	Accepted

Checks that actually could have failed, and did not:

• The header names the right place. It says "one claim in the Consequences below", and the sentence is indeed in ADR-0015's Consequences — I parsed the file by section to confirm rather than eyeballing it. The quoted fragment "offers no way to bring it inside" is verbatim from that sentence, so a reader can find it by searching the page.
• The anchor resolves. See [The Development complication](#the-development-complication) matches the new ### The Development complication heading in the same file.
• Every ADR link target exists across both files.
• The index convention is followed exactly. Only the superseded record's row is annotated — 0002 reads Accepted (partly superseded by 0017) while 0017 reads plain Accepted, and 0015/0022 now mirror that pair.
• Metadata order matches the template in both files, with the omitted supersession lines left out rather than filled with placeholders.

ADR-0001's requirements are met on both sides: the old record stays Accepted and gains the field naming the new ADR and the part replaced; the new ADR states precisely what it displaces and no more.

The wrong reason is properly gone

Both places that justified the placement by "the record cannot be touched" now say the opposite and say why:

The correction lives here rather than in ADR-0015 because this is the record that acts on the claim and carries the reasoning that replaces it — not because ADR-0015 cannot be touched. It can … Only its body is frozen, which is why the false sentence is still in its Consequences.

That is the correct reading of ADR-0001, it cites the ADR-0002/0017 precedent so the next person has a worked example, and the Consequences paragraph was rewritten to match rather than left contradicting the header. "A reader who arrives at ADR-0015 first is sent here by its header before they reach it" is now true, which it was not before.

Byte figures

Right call, and the replacement is better evidence than the numbers were. several pages of rendered stack trace and carrying the exception message and stack trace in the same body shape a deployed environment uses for a body that carries neither — both observed, with the exception's message present in each is exactly what I measured independently, and unlike 16,618 it will still be true on someone else's machine. The Program.cs comment was brought into line too, which is the kind of thing that usually gets missed.

One informational note

The reasoning for keeping the three sibling WillNotTakeJson tests is in this thread, not in the tree — no test file changed at this head. I am not asking for it: the doc comment already on Send_ValidationFailedExceptionToACallerThatWillNotTakeJson_DeliversTheStatusItLogs explains the trap and names the 404 as the case that broke, which is enough for a reader who wonders why three near-identical tests exist. Noting it only so the omission is deliberate rather than assumed.

Nothing outstanding. Merge it.

Verdict: mergeable Re-reviewed at `13afb13`. As asked, I checked the supersession trail rather than re-running behaviour — but I did re-run the four CI commands, because `Program.cs` is in the diff. ## `Program.cs` is comment-only One line, `16 KB of stack trace` → `a page of stack trace`. No behaviour change, so the verification at `43c1cdd` stands. Confirmed green anyway at this head: build 0 warnings / 0 errors, **30 tests pass**, `format --verify-no-changes` exit 0. ## The supersession trail is coherent in both directions | | | | --- | --- | | ADR-0015 header | `Partially superseded by: ADR-0022 — one claim in the Consequences below…` | | ADR-0015 body | **untouched** — the diff against `43c1cdd` adds 9 lines and removes 0 | | Index row 0015 | `Accepted (partly superseded by 0022)` | | ADR-0022 header | `Supersedes: ADR-0015 **in part** — its claim that … and nothing else.` | | Index row 0022 | `Accepted` | Checks that actually could have failed, and did not: - **The header names the right place.** It says "one claim in the Consequences below", and the sentence is indeed in ADR-0015's Consequences — I parsed the file by section to confirm rather than eyeballing it. The quoted fragment `"offers no way to bring it inside"` is verbatim from that sentence, so a reader can find it by searching the page. - **The anchor resolves.** `See [The Development complication](#the-development-complication)` matches the new `### The Development complication` heading in the same file. - **Every ADR link target exists** across both files. - **The index convention is followed exactly.** Only the superseded record's row is annotated — 0002 reads `Accepted (partly superseded by 0017)` while 0017 reads plain `Accepted`, and 0015/0022 now mirror that pair. - **Metadata order matches the template** in both files, with the omitted supersession lines left out rather than filled with placeholders. ADR-0001's requirements are met on both sides: the old record stays `Accepted` and gains the field naming the new ADR and the part replaced; the new ADR states precisely what it displaces and no more. ## The wrong reason is properly gone Both places that justified the placement by "the record cannot be touched" now say the opposite and say why: > The correction lives here rather than in ADR-0015 because this is the record that acts on the claim and carries the reasoning that replaces it — not because ADR-0015 cannot be touched. It can … Only its *body* is frozen, which is why the false sentence is still in its Consequences. That is the correct reading of ADR-0001, it cites the ADR-0002/0017 precedent so the next person has a worked example, and the Consequences paragraph was rewritten to match rather than left contradicting the header. "A reader who arrives at ADR-0015 first is sent here by its header before they reach it" is now true, which it was not before. ## Byte figures Right call, and the replacement is better evidence than the numbers were. `several pages of rendered stack trace` and `carrying the exception message and stack trace in the same body shape a deployed environment uses for a body that carries neither — both observed, with the exception's message present in each` is exactly what I measured independently, and unlike `16,618` it will still be true on someone else's machine. The `Program.cs` comment was brought into line too, which is the kind of thing that usually gets missed. ## One informational note The reasoning for keeping the three sibling `WillNotTakeJson` tests is in this thread, not in the tree — no test file changed at this head. I am not asking for it: the doc comment already on `Send_ValidationFailedExceptionToACallerThatWillNotTakeJson_DeliversTheStatusItLogs` explains the trap and names the 404 as the case that broke, which is enough for a reader who wonders why three near-identical tests exist. Noting it only so the omission is deliberate rather than assumed. Nothing outstanding. Merge it.

rob merged commit 67618f2370 into main 2026-08-03 12:07:56 +00:00

rob deleted branch feat/problem-details 2026-08-03 12:07:57 +00:00

rob referenced this pull request from a commit 2026-08-03 12:07:57 +00:00

Global exception handling and Problem Details (#18)

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

No description provided.