From b2c8fe85679c1e697fe146d1b0f90cb44fdcfe91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 08:35:35 +0200 Subject: [PATCH] fix(api): answer the unknown-leaf refusal with a sentence and a code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PUT /api/auth/me/report-selection` threw a bare 422 whose whole `error` string was `report-selection.leaves.unknown` — a machine token in the field a client shows a person, with nothing beside it to branch on and no way to learn which leaves were refused. It now answers a sentence, with the token in `meta.errorCode` and the refused ids in `meta.unknownLeaves`, and the operation's description says so. The site was invisible to the refusal guard because the refusal is decided by `if (!minted.ok)` rather than by a parse result, so the guard now reads the refused string as well as the deciding block: every 422 under `src/app/api` whose `error` is a dotted token is inventoried with the reason it still is one. Eleven remain, each with its reason, and the inventory may only shrink. Removing the fix above turns it red naming that route. The four sibling shape refusals keep their token in `error` on purpose, and the comment now says why that is not the same decision as the seven `invalid_json` sites: those moved their string because their status moved to 400 anyway, while a shape refusal that keeps its 422 would be changing the wire for nothing. Both halves publish `meta.errorCode`. Two loose ends in the same guard: a block calling `returnAllZodIssues` or `apiValidationError` without spelling `422` is now judged (their default status is 422, so a future `refuse422(err)` helper cannot slip past), and the site floor moves from 40 to 200 against the 248 the tree carries — the old floor would have survived losing five matches in six. --- docs/api/openapi.yaml | 30 ++-- .../zod-refusal-issue-list-guard.test.ts | 140 ++++++++++++++++-- src/app/api/auth/me/coach-prefs/route.ts | 8 +- src/app/api/auth/me/report-selection/route.ts | 22 ++- src/app/api/auth/me/source-priority/route.ts | 8 +- .../chat/messages/[id]/feedback/route.ts | 8 +- src/app/api/insights/chat/route.ts | 8 +- src/lib/openapi/error-codes.ts | 1 + src/lib/openapi/routes/profile.ts | 2 +- 9 files changed, 186 insertions(+), 41 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 34ac2cfe8..8d65be753 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -7773,7 +7773,8 @@ paths: - Auth summary: Replace the owner's saved doctor-report selection description: Replace, not merge — the body states the whole scope or it is not a scope; a leaf omitted from the body is - a leaf removed from the saved profile. An unknown leaf id is refused with a 422 that names it. The saved + a leaf removed from the saved profile. An unknown leaf id is refused with a 422 carrying `meta.errorCode` + `report-selection.leaves.unknown` and `meta.unknownLeaves`, the ids this build does not know. The saved selection is also what the FHIR REST face and the MCP doctor-visit surfaces replay for a caller that cannot ask a human, so every write is audit-logged. Auth via cookie or Bearer. requestBody: @@ -29574,19 +29575,20 @@ components: onboarding.step.rateLimited, onboarding.tour.invalid, onboarding.user.notFound; personal_records — personal_records.invalid_query; practitioner — practitioner.invalid; profile — profile.update.emailInUse, profile.update.invalidBody, profile.update.nothingSaved; report-selection — - report-selection.body.invalid_json, report-selection.body.invalid_shape; route — route.retired; - share-link — share-link.document.decryptFailed, share-link.document.notFound, - share-link.document.rateLimited, share-link.documents.invalid, share-link.report.notFound, - share-link.report.rateLimited, share-link.selection.forbidden_leaf, share-link.selection.unknown_leaf; - sharing — sharing.accept.expired, sharing.accept.not_found, sharing.accept.not_pending, - sharing.accept.revoked, sharing.access.denied, sharing.invite.duplicate, sharing.invite.invalid, - sharing.invite.invalid_scope, sharing.invite.manage_browser_only, sharing.invite.refused, - sharing.invite.self, sharing.not_permitted, sharing.renounce.already_ended, sharing.renounce.not_found, - sharing.revoke.already_ended, sharing.revoke.not_found, sharing.session.changed, sharing.switch.invalid, - sharing.switch.wrong_transport; source-priority — source-priority.body.invalid_json, - source-priority.body.invalid_shape; tokens — tokens.measurements.ceiling_reached; vaccination — - vaccination.booster-invalid, vaccination.booster-no-antigen, vaccination.encounter-not-found, - vaccination.identity-required, vaccination.invalid, vaccination.link.invalid, vaccination.not-found, + report-selection.body.invalid_json, report-selection.body.invalid_shape, + report-selection.leaves.unknown; route — route.retired; share-link — share-link.document.decryptFailed, + share-link.document.notFound, share-link.document.rateLimited, share-link.documents.invalid, + share-link.report.notFound, share-link.report.rateLimited, share-link.selection.forbidden_leaf, + share-link.selection.unknown_leaf; sharing — sharing.accept.expired, sharing.accept.not_found, + sharing.accept.not_pending, sharing.accept.revoked, sharing.access.denied, sharing.invite.duplicate, + sharing.invite.invalid, sharing.invite.invalid_scope, sharing.invite.manage_browser_only, + sharing.invite.refused, sharing.invite.self, sharing.not_permitted, sharing.renounce.already_ended, + sharing.renounce.not_found, sharing.revoke.already_ended, sharing.revoke.not_found, + sharing.session.changed, sharing.switch.invalid, sharing.switch.wrong_transport; source-priority — + source-priority.body.invalid_json, source-priority.body.invalid_shape; tokens — + tokens.measurements.ceiling_reached; vaccination — vaccination.booster-invalid, + vaccination.booster-no-antigen, vaccination.encounter-not-found, vaccination.identity-required, + vaccination.invalid, vaccination.link.invalid, vaccination.not-found, vaccination.practitioner-not-found; workout — workout.batch.invalid, workout.batch.payload_too_large, workout.batch.too_large; (unprefixed) — about_me_conflict, ai_response_truncated, coach_prefs_conflict, dashboard_layout_conflict, empty_file, health_score_config_conflict, insights_layout_conflict, diff --git a/src/__tests__/zod-refusal-issue-list-guard.test.ts b/src/__tests__/zod-refusal-issue-list-guard.test.ts index 00031d521..57426b41b 100644 --- a/src/__tests__/zod-refusal-issue-list-guard.test.ts +++ b/src/__tests__/zod-refusal-issue-list-guard.test.ts @@ -18,13 +18,21 @@ * judged — a 400, a 401 or a 403 raised off a parse result is a different * refusal and is left alone. * + * The second half of the same audit item is the dotted token in `error`, and + * it is checked by its own sweep at the bottom of this file rather than by the + * per-site matcher above: `report-selection.leaves.unknown` was decided by + * `if (!minted.ok)` rather than by a parse result, so the block matcher could + * not see it while the string it answered with was the defect itself. The + * token sweep reads the refused string, not the condition that chose it, so a + * site cannot hide behind the shape of its own `if`. + * * Its limits, stated so nobody reads more into a green run than is there: * a refusal built somewhere other than the `if (!x.success)` block that decided * it — a helper called with the error, a status assembled from a variable — is - * invisible here, and so is a route that hands the `ZodError` to shared code - * outside `src/app/api`. It catches the shape this defect actually took, and - * the count assertion below is what keeps it from passing on a matcher that - * silently stopped matching anything. + * invisible to the issue-list half, and so is a route that hands the `ZodError` + * to shared code outside `src/app/api`. It catches the shape this defect + * actually took, and the count assertions below are what keep it from passing + * on a matcher that silently stopped matching anything. */ import { readFileSync, readdirSync } from "node:fs"; import { join, relative, sep } from "node:path"; @@ -106,11 +114,21 @@ function scanRefusalSites(): RefusalSite[] { )) { if (!parsed.has(m[1])) continue; const block = refusalBlock(src, (m.index ?? 0) + m[0].length); - if (!/\b422\b/.test(block)) continue; + // `returnAllZodIssues` and `apiValidationError` answer 422 unless told + // otherwise, so a block that calls one is a 422 site whether or not the + // number is spelled: a future `refuse422(parsed.error)` helper would + // otherwise be invisible for the same reason. + const carriesIssues = /returnAllZodIssues|apiValidationError/.test(block); + if (!/\b422\b/.test(block) && !carriesIssues) continue; + if ( + /\b(?:400|401|403|409|413|415|429|500|502)\b/.test(block) && + !/\b422\b/.test(block) + ) + continue; sites.push({ route: relative(process.cwd(), file).split(sep).join("/"), line: src.slice(0, m.index).split("\n").length, - carriesIssues: /returnAllZodIssues|apiValidationError/.test(block), + carriesIssues, }); } } @@ -122,9 +140,9 @@ describe("Zod shape refusals carry the issue list", () => { it("finds the refusal sites it is meant to judge", () => { // A matcher that stops matching would otherwise report a clean zero it did - // not earn. The tree carried 59 of these when the guard was written; the - // floor is deliberately loose so ordinary route churn does not trip it. - expect(sites.length).toBeGreaterThan(40); + // not earn. Two hundred and forty-eight sites when this was written, so + // the old floor of 40 would have survived losing five matches in six. + expect(sites.length).toBeGreaterThan(200); }); it("answers every 422 with details.issues", () => { @@ -141,3 +159,107 @@ describe("Zod shape refusals carry the issue list", () => { expect(stale).toEqual([]); }); }); + +/** + * The dotted token in `error`, wherever it is refused from. + * + * `error` is the field a client shows a person. A 422 that puts + * `report-selection.leaves.unknown` there hands the UI a string it must not + * render and gives it nothing else to branch on. That site was decided by + * `if (!minted.ok)`, not by a parse result, so the per-site matcher above ran + * past it — which is the argument for reading the refused string directly: + * whatever chose the refusal, the string is the defect. + * + * The inventory below is frozen and may only shrink. Each entry says why the + * token is still the whole `error` string; a new one fails until somebody + * either writes a sentence or writes the reason. + */ +const TOKEN_IN_ERROR_422: Record = { + "src/app/api/auth/me/coach-prefs/route.ts coach-prefs.body.invalid_shape": + "The status does not move for a shape refusal, so moving the string too would be an unforced second wire change; `meta.errorCode` now carries the same token.", + "src/app/api/auth/me/report-selection/route.ts report-selection.body.invalid_shape": + "Same shape refusal, same reason.", + "src/app/api/auth/me/source-priority/route.ts source-priority.body.invalid_shape": + "Same shape refusal, same reason.", + "src/app/api/insights/chat/messages/[id]/feedback/route.ts feedback.body.invalid": + "Same shape refusal, same reason.", + "src/app/api/insights/chat/route.ts coach.request.invalid": + "Same shape refusal, same reason.", + "src/app/api/auth/me/health-score-config/route.ts health-score-config.body.invalid_json": + "The published 422 description spells this exact body out as what a non-JSON request earns. It is the one operation whose contract promises the token, so the token is what it answers with.", + "src/app/api/coach/reminder-suggestions/route.ts coach.suggestion.unknownCadence": + "Predates this sweep and carries no `meta.errorCode` yet; the Coach reminder surfaces move as one piece or not at all.", + "src/app/api/coach/reminders/route.ts coach.reminder.invalidWhen": + "Predates this sweep, same surface.", + "src/app/api/coach/reminders/[id]/route.ts coach.reminder.invalidWhen": + "Predates this sweep, same surface.", + "src/app/api/coach/suggested-actions/route.ts coach.action.unknownInterval": + "Predates this sweep, same surface.", + "src/app/api/coach/suggested-actions/route.ts coach.reminder.invalidWhen": + "Predates this sweep, same surface.", +}; + +/** A dotted machine token: no spaces, at least one dot, no sentence in sight. */ +const DOTTED_TOKEN = /^[a-z][A-Za-z0-9_-]*(?:\.[A-Za-z0-9_-]+)+$/; + +/** Every `.ts` under `src/app/api`, tests excluded — not only `route.ts`. */ +function apiFiles(dir: string): string[] { + const found: string[] = []; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const full = join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === "__tests__") continue; + found.push(...apiFiles(full)); + continue; + } + if (entry.name.endsWith(".ts")) found.push(full); + } + return found; +} + +/** Every 422 whose `error` string is a dotted token, keyed ` `. */ +function tokenRefusals(): string[] { + const found: string[] = []; + for (const file of apiFiles(API_ROOT)) { + const src = readFileSync(file, "utf8"); + const route = relative(process.cwd(), file).split(sep).join("/"); + const shapes = [ + /apiError\(\s*"([^"]+)"\s*,\s*422/g, + /apiValidationError\(\s*"([^"]+)"/g, + /new HttpError\(\s*422\s*,\s*"([^"]+)"/g, + ]; + for (const shape of shapes) { + for (const m of src.matchAll(shape)) { + if (!DOTTED_TOKEN.test(m[1])) continue; + const key = `${route} ${m[1]}`; + if (!found.includes(key)) found.push(key); + } + } + } + return found; +} + +describe("a 422 does not answer with a machine token where the sentence goes", () => { + const refusals = tokenRefusals(); + + it("finds the sites it is meant to judge", () => { + // Eleven when this was written, twelve before the report-selection leaf + // refusal was rewritten. A matcher that stopped matching would agree with + // an empty inventory. + expect(refusals.length).toBeGreaterThan(8); + }); + + it("keeps the token in `error` only where a reason is written down", () => { + const unexplained = refusals + .filter((key) => !(key in TOKEN_IN_ERROR_422)) + .sort(); + expect(unexplained).toEqual([]); + }); + + it("keeps no stale inventory entry", () => { + const stale = Object.keys(TOKEN_IN_ERROR_422) + .filter((key) => !refusals.includes(key)) + .sort(); + expect(stale).toEqual([]); + }); +}); diff --git a/src/app/api/auth/me/coach-prefs/route.ts b/src/app/api/auth/me/coach-prefs/route.ts index 7739a4ea2..4e1bf8593 100644 --- a/src/app/api/auth/me/coach-prefs/route.ts +++ b/src/app/api/auth/me/coach-prefs/route.ts @@ -81,9 +81,11 @@ export const PUT = apiHandler(async (req: Request) => { action: { name: "auth.me.coach-prefs.put.invalid" }, meta: { issues: parsed.error.issues.length }, }); - // The dotted token keeps its place in `error` — clients already match on it - // there — while `meta.errorCode` publishes it in the field a machine code - // belongs in, and `details.issues` names the fields that were refused. + // The dotted token keeps its place in `error`: this refusal keeps its 422, + // so moving the string would be a wire change with nothing forcing it — + // the `invalid_json` siblings moved only because their status moved to 400 + // anyway. `meta.errorCode` publishes the same token in the field a machine + // code belongs in, and `details.issues` names the fields that were refused. return apiValidationError( "coach-prefs.body.invalid_shape", sanitiseZodIssues(parsed.error.issues), diff --git a/src/app/api/auth/me/report-selection/route.ts b/src/app/api/auth/me/report-selection/route.ts index e7aeed85b..bbb85b9c2 100644 --- a/src/app/api/auth/me/report-selection/route.ts +++ b/src/app/api/auth/me/report-selection/route.ts @@ -16,7 +16,7 @@ * Replaces `GET`/`PUT /api/auth/me/doctor-report-prefs`, whose column nothing * outside that route ever read. */ -import { apiHandler, requireAuth, HttpError } from "@/lib/api-handler"; +import { apiHandler, requireAuth } from "@/lib/api-handler"; import { apiError, apiSuccess, @@ -65,9 +65,11 @@ export const PUT = apiHandler(async (req: Request) => { action: { name: "auth.me.report-selection.put.invalid" }, meta: { issues: parsed.error.issues.length }, }); - // The dotted token keeps its place in `error` — clients already match on it - // there — while `meta.errorCode` publishes it in the field a machine code - // belongs in, and `details.issues` names the fields that were refused. + // The dotted token keeps its place in `error`: this refusal keeps its 422, + // so moving the string would be a wire change with nothing forcing it — + // the `invalid_json` siblings moved only because their status moved to 400 + // anyway. `meta.errorCode` publishes the same token in the field a machine + // code belongs in, and `details.issues` names the fields that were refused. return apiValidationError( "report-selection.body.invalid_shape", sanitiseZodIssues(parsed.error.issues), @@ -82,7 +84,17 @@ export const PUT = apiHandler(async (req: Request) => { action: { name: "auth.me.report-selection.put.invalid" }, meta: { unknownLeaves: minted.error.unknownLeaves }, }); - throw new HttpError(422, "report-selection.leaves.unknown"); + // Not a Zod refusal, so there is no issue list to carry — but the same + // rule applies: a sentence in `error`, the machine token in + // `meta.errorCode`, and the leaves that were refused named beside it. + return apiError( + "Report selection names leaves this build does not know", + 422, + { + errorCode: "report-selection.leaves.unknown", + unknownLeaves: minted.error.unknownLeaves, + }, + ); } // Persist the canonical ordering rather than the caller's, so two clients diff --git a/src/app/api/auth/me/source-priority/route.ts b/src/app/api/auth/me/source-priority/route.ts index f1c50337e..d5897cd1d 100644 --- a/src/app/api/auth/me/source-priority/route.ts +++ b/src/app/api/auth/me/source-priority/route.ts @@ -65,9 +65,11 @@ export const PUT = apiHandler(async (req: Request) => { action: { name: "auth.me.source-priority.put.invalid" }, meta: { issues: parsed.error.issues.length }, }); - // The dotted token keeps its place in `error` — clients already match on it - // there — while `meta.errorCode` publishes it in the field a machine code - // belongs in, and `details.issues` names the fields that were refused. + // The dotted token keeps its place in `error`: this refusal keeps its 422, + // so moving the string would be a wire change with nothing forcing it — + // the `invalid_json` siblings moved only because their status moved to 400 + // anyway. `meta.errorCode` publishes the same token in the field a machine + // code belongs in, and `details.issues` names the fields that were refused. return apiValidationError( "source-priority.body.invalid_shape", sanitiseZodIssues(parsed.error.issues), diff --git a/src/app/api/insights/chat/messages/[id]/feedback/route.ts b/src/app/api/insights/chat/messages/[id]/feedback/route.ts index 668a06599..4056d7eb9 100644 --- a/src/app/api/insights/chat/messages/[id]/feedback/route.ts +++ b/src/app/api/insights/chat/messages/[id]/feedback/route.ts @@ -93,9 +93,11 @@ async function handlePost(request: NextRequest, ctx: RouteContext) { action: { name: "insights.coach.message.feedback" }, meta: { outcome: "validation_failed" }, }); - // The dotted token keeps its place in `error` — clients already match on it - // there — while `meta.errorCode` publishes it in the field a machine code - // belongs in, and `details.issues` names the fields that were refused. + // The dotted token keeps its place in `error`: this refusal keeps its 422, + // so moving the string would be a wire change with nothing forcing it — + // the `invalid_json` siblings moved only because their status moved to 400 + // anyway. `meta.errorCode` publishes the same token in the field a machine + // code belongs in, and `details.issues` names the fields that were refused. return apiValidationError( "feedback.body.invalid", sanitiseZodIssues(parsed.error.issues), diff --git a/src/app/api/insights/chat/route.ts b/src/app/api/insights/chat/route.ts index e14cdff7e..9dc19d79d 100644 --- a/src/app/api/insights/chat/route.ts +++ b/src/app/api/insights/chat/route.ts @@ -214,9 +214,11 @@ async function handleChatRequest(request: NextRequest): Promise { action: { name: "insights.coach.invalid" }, meta: { issues: parsed.error.issues.length }, }); - // The dotted token keeps its place in `error` — clients already match on it - // there — while `meta.errorCode` publishes it in the field a machine code - // belongs in, and `details.issues` names the fields that were refused. + // The dotted token keeps its place in `error`: this refusal keeps its 422, + // so moving the string would be a wire change with nothing forcing it — + // the `invalid_json` siblings moved only because their status moved to 400 + // anyway. `meta.errorCode` publishes the same token in the field a machine + // code belongs in, and `details.issues` names the fields that were refused. return apiValidationError( "coach.request.invalid", sanitiseZodIssues(parsed.error.issues), diff --git a/src/lib/openapi/error-codes.ts b/src/lib/openapi/error-codes.ts index 9ba5e0a47..67ef9f148 100644 --- a/src/lib/openapi/error-codes.ts +++ b/src/lib/openapi/error-codes.ts @@ -254,6 +254,7 @@ export const ERROR_CODE_CATALOGUE: Readonly> = "report-selection": [ "report-selection.body.invalid_json", "report-selection.body.invalid_shape", + "report-selection.leaves.unknown", ], route: ["route.retired"], "share-link": [ diff --git a/src/lib/openapi/routes/profile.ts b/src/lib/openapi/routes/profile.ts index 5c02cfcb7..2ce21fc07 100644 --- a/src/lib/openapi/routes/profile.ts +++ b/src/lib/openapi/routes/profile.ts @@ -1540,7 +1540,7 @@ export const profilePaths: NonNullable = { tags: ["Auth"], summary: "Replace the owner's saved doctor-report selection", description: - "Replace, not merge — the body states the whole scope or it is not a scope; a leaf omitted from the body is a leaf removed from the saved profile. An unknown leaf id is refused with a 422 that names it. The saved selection is also what the FHIR REST face and the MCP doctor-visit surfaces replay for a caller that cannot ask a human, so every write is audit-logged. Auth via cookie or Bearer.", + "Replace, not merge — the body states the whole scope or it is not a scope; a leaf omitted from the body is a leaf removed from the saved profile. An unknown leaf id is refused with a 422 carrying `meta.errorCode` `report-selection.leaves.unknown` and `meta.unknownLeaves`, the ids this build does not know. The saved selection is also what the FHIR REST face and the MCP doctor-visit surfaces replay for a caller that cannot ask a human, so every write is audit-logged. Auth via cookie or Bearer.", requestBody: { required: true, content: { "application/json": { schema: savedReportProfile } },