From 4379f767472e8739f37a25c0d87060ddf0db266a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 30 Oct 2026 23:36:25 +0100 Subject: [PATCH 1/6] feat(i18n): every server refusal is said by a record the contract holds A refusal code or reason with no sentence fell through to the generic "couldn't carry out that action" unnoticed: the parity test is green when EN and HU miss the same key. One did (an edit refused for a file that is not video or audio). - lib/api/refusalCodes.ts: which sentence answers each code, as data; localizeDetail says a refusal through it, unchanged for every reader. - backendIds contract: every detail code the backend sends is in exactly one record, every reason it can carry has its sentence in EN and HU, and no family names a reason the backend cannot send. - The reasons that travel in a variable are Literal types (edit, move, cookies, quota), so the type checker refuses an unknown one too. - downloads.errors.edit.not_media in EN and HU. --- CLAUDE.md | 2 +- backend/app/downloads/credentials.py | 10 ++ backend/app/downloads/quota.py | 12 +- backend/app/downloads/service.py | 12 +- backend/app/reader/service.py | 9 +- backend/app/refusal.py | 6 +- backend/app/routes/downloads.py | 2 +- frontend/src/i18n/backendIds.contract.test.ts | 116 ++++++++++++- frontend/src/i18n/locales/en/downloads.json | 1 + frontend/src/i18n/locales/hu/downloads.json | 1 + frontend/src/i18n/unusedKeys.contract.test.ts | 7 + frontend/src/lib/api/refusalCodes.ts | 104 +++++++++++ frontend/src/lib/api/refusalText.dom.test.ts | 61 +++++++ frontend/src/lib/api/refusalText.ts | 161 +++++++----------- 14 files changed, 381 insertions(+), 123 deletions(-) create mode 100644 frontend/src/lib/api/refusalCodes.ts create mode 100644 frontend/src/lib/api/refusalText.dom.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 0534ad64..d81043fe 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -105,7 +105,7 @@ CQRS, a repository/DI framework, a router library, a generic CRUD-router factory | Ownership check | `userscope.owned_by` (a filter) / `owned_row` (a row by id): a unit's `queries.own_x` returns None and its route answers `not_found`; a core route `owned_or_404`. A composite rule (own or shared, built-in or own) says so in its `queries` | an inline owner filter — `Model.user_id == …`, an alias's, `.in_()`, `filter_by(user_id=…)` (fence-held, every module) — or a `db.get` and a compare where `owned_row` fits | | Admin gate import | `app.auth` | the `app.routes.admin` re-export | | Upstream failure status | `422` with JSON detail | `502` (reserve 502 for raw byte-proxies) | -| User-facing error | structured `detail` (`{code, reason}`) | a bare English string under a bilingual UI | +| User-facing error | structured `detail` (`{code, reason}`), its sentence named in `lib/api/refusalCodes.ts` (contract-held) | a bare English string under a bilingual UI | | Mutation response | `{"ok": true, …}` (`{"deleted": id}`), test-held | a bare `{"unsubscribed"}`/`{"cleared": n}`, no `ok` | | Transaction commit | the handler commits; a unit's `service` and every helper `flush` (fence-held, every module) | a helper that commits — only a listed one (what owns its session, or a core step that must land whatever its caller does next), each with its reason: the fence's `UNIT_COMMITS` / `CORE_COMMITS`, a route's `COMMITTING_HELPERS` (`test_route_conventions`) | | Admin knob storage | `sysconfig` (values) | env / `app_state` / a table of its own (the job intervals had one) | diff --git a/backend/app/downloads/credentials.py b/backend/app/downloads/credentials.py index 587af04f..39e1cd93 100644 --- a/backend/app/downloads/credentials.py +++ b/backend/app/downloads/credentials.py @@ -25,6 +25,7 @@ Security controls enforced here: import os from pathlib import Path +from typing import Literal from sqlalchemy import select from sqlalchemy.orm import Session @@ -163,9 +164,18 @@ def get_credential(db: Session, user_id: int, host_key: str) -> DownloadCredenti ).scalar_one_or_none() +# Every reason a pasted cookies.txt is refused; the UI names each one (`downloads.errors.cookies.`, +# held by the frontend's backendIds contract, which reads this Literal). +CookieReason = Literal["empty", "format"] + + class InvalidCookies(ValueError): """The pasted cookies aren't a usable Netscape cookies.txt — surfaced as a 400 by the route.""" + def __init__(self, reason: CookieReason): + super().__init__(reason) + self.reason = reason + def validate_cookies(raw: str) -> str: """Sanity-check a pasted cookies.txt (Netscape format) and return the cleaned text. yt-dlp reads diff --git a/backend/app/downloads/quota.py b/backend/app/downloads/quota.py index aca55ae0..e8d56f85 100644 --- a/backend/app/downloads/quota.py +++ b/backend/app/downloads/quota.py @@ -20,6 +20,7 @@ their output's real size before it is placed. Edit jobs do not consult it at all """ from dataclasses import dataclass +from typing import Literal from sqlalchemy import and_, func, or_, select from sqlalchemy.orm import Session @@ -50,10 +51,15 @@ class QuotaLimits: unlimited: bool -class QuotaExceeded(Exception): - """Raised by check_enqueue; `reason` is one of max_jobs|max_bytes (i18n key on the client).""" +# Every cap a download can hit; the UI names each one (`downloads.errors.quota.`, held by the +# frontend's backendIds contract, which reads this Literal). +QuotaReason = Literal["max_jobs", "max_bytes"] - def __init__(self, reason: str, limit: int, current: int): + +class QuotaExceeded(Exception): + """Raised by check_enqueue; `reason` names the cap it hit (an i18n key on the client).""" + + def __init__(self, reason: QuotaReason, limit: int, current: int): self.reason = reason self.limit = limit self.current = current diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 13010aeb..ffc40cbf 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -10,6 +10,7 @@ handler); the folder, link and quota writes are `folders`, `links` and `quota`. import uuid from datetime import datetime, timezone +from typing import Literal from sqlalchemy import delete as delete_rows from sqlalchemy import func, select, update @@ -39,10 +40,15 @@ from app.plex import paths as plex_paths from app.userscope import owned_by, owned_row -class EditError(Exception): - """Bad edit request (empty spec / source not ready); `reason` is an i18n-mappable key.""" +# Every reason an edit is refused; the UI names each one (`downloads.errors.edit.`, held by +# the frontend's backendIds contract, which reads this Literal). +EditReason = Literal["source_not_ready", "not_media", "empty_edit"] - def __init__(self, reason: str): + +class EditError(Exception): + """Bad edit request (empty spec / source not ready / not media); `reason` names the sentence.""" + + def __init__(self, reason: EditReason): self.reason = reason super().__init__(reason) diff --git a/backend/app/reader/service.py b/backend/app/reader/service.py index 5402e653..5eb779c0 100644 --- a/backend/app/reader/service.py +++ b/backend/app/reader/service.py @@ -9,7 +9,7 @@ unique key, a row lock taken before a presence check. The comfort shelf's writes from datetime import datetime, timezone -from typing import cast +from typing import Literal, cast from sqlalchemy import Table, bindparam, delete, exists, func, select, text, update from sqlalchemy.dialects.postgresql import insert as pg_insert @@ -301,10 +301,15 @@ def set_progress(db: Session, user: User, video_id: str, position: int) -> None: # --- playlists ------------------------------------------------------------------------------- +# Every reason a move is refused; the UI names each one (`playlists.moveErrors.`, held by the +# frontend's backendIds contract, which reads this Literal). +MoveReason = Literal["same_playlist", "not_in_playlist"] + + class MoveRefused(Exception): """A move between playlists that cannot happen: `reason` is the code the UI translates.""" - def __init__(self, reason: str): + def __init__(self, reason: MoveReason): super().__init__(reason) self.reason = reason diff --git a/backend/app/refusal.py b/backend/app/refusal.py index 95b2cf04..8e6b9ffd 100644 --- a/backend/app/refusal.py +++ b/backend/app/refusal.py @@ -8,8 +8,10 @@ whatever sentence the route happened to hand-write. `refused` is the generic fam (`not_found`, `youtube`, `playlist`, the admin refusals, …) stay, since each is already translated. A test (`test_http_details.py`) reads every `HTTPException` in the app and fails on a string detail, -and the frontend's contract test requires every `refused(…, "")` to be named in both -languages — so the rule is enforced, not described. +and the frontend's contract test (`i18n/backendIds.contract.test.ts`) requires every `code` a detail +carries to be in one record of `lib/api/refusalCodes.ts`, and every reason it can be sent with — a +literal, or a `Literal` type when it travels in a variable — to have its sentence in both languages. +So the rule is enforced, not described. """ from fastapi import HTTPException diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index b872c0e7..b7e672d8 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -208,7 +208,7 @@ def set_credential( try: cleaned = dlcreds.validate_cookies(body.cookies) except dlcreds.InvalidCookies as e: - raise HTTPException(status_code=400, detail={"code": "cookies", "reason": str(e)}) from e + raise HTTPException(status_code=400, detail={"code": "cookies", "reason": e.reason}) from e dlcreds.upsert_credential(db, user.id, host_key, cleaned) audit.record( db, diff --git a/frontend/src/i18n/backendIds.contract.test.ts b/frontend/src/i18n/backendIds.contract.test.ts index c4603e5b..29d7b082 100644 --- a/frontend/src/i18n/backendIds.contract.test.ts +++ b/frontend/src/i18n/backendIds.contract.test.ts @@ -2,6 +2,12 @@ import { readdirSync, readFileSync } from "node:fs"; import { describe, expect, it } from "vitest"; import { RUN_STATUSES } from "../lib/activityUrlState"; +import { + REFUSAL_BY_REASON, + REFUSAL_FAMILIES, + REFUSAL_FIXED, + REFUSAL_UNSHOWN, +} from "../lib/api/refusalCodes"; import { vocabKey } from "../lib/vocab"; // Every id the BACKEND owns and the UI names must have its name in both languages. The UI looks these @@ -87,13 +93,9 @@ const quotaActions = enumValues("quota.py"); // The admin actions' refusal reasons → `errors.admin.`. The codes are read from // `localizeDetail`'s own ADMIN_REFUSALS, so a code added there is scanned here too; a detail dict // ruff wraps over several lines still matches. -const adminCodes = [ - ...( - readFileSync(new URL("../lib/api/refusalText.ts", import.meta.url), "utf-8").match( - /ADMIN_REFUSALS = new Set\(\[([^\]]*)\]/, - )?.[1] ?? "" - ).matchAll(/"(\w+)"/g), -].map((m) => m[1]!); +const adminCodes = Object.entries(REFUSAL_FAMILIES) + .filter(([, f]) => f.prefix === "errors.admin") + .map(([code]) => code); const adminRefusals = [ ...backend("routes/admin.py").matchAll(/\{\s*"code":\s*"(\w+)",\s*"reason":\s*"(\w+)"/g), ].filter((m) => adminCodes.includes(m[1]!)); @@ -214,6 +216,54 @@ const clipReasons = [ ), ]; +// Every refusal code the backend answers: each `"code": ""` of a detail anywhere in the app. A +// subtitle's status payload (`{"state": …, "code": …}`) is the player's own vocabulary, not a refusal. +const backendCodes = [ + ...new Set( + appSources().flatMap((src) => + src + .split("\n") + .filter((line) => !line.includes('"state":')) + .flatMap((line) => [...line.matchAll(/"code":\s*"(\w+)"/g)].map((m) => m[1]!)), + ), + ), +]; +const literalOf = (file: string, name: string) => + [ + ...( + backend(file).match(new RegExp(`^${name} = Literal\\[([^\\]]*)\\]`, "m"))?.[1] ?? "" + ).matchAll(/"(\w+)"/g), + ].map((m) => m[1]!); +const literalCalls = (file: string, re: RegExp) => [ + ...new Set([...backend(file).matchAll(re)].map((m) => m[1]!)), +]; +const detailReasons = (file: string, code: string) => + literalCalls(file, new RegExp(`"code":\\s*"${code}",\\s*"reason":\\s*"(\\w+)"`, "g")); +// What each refusal code can carry as its reason, read where it is decided: a Literal when the reason +// travels in a variable (`e.reason`), the call's own literal otherwise. +const reasonsOf: Record = { + refused: refusedReasons, + youtube: youtubeReasons, + not_found: goneWhats, + clip: clipReasons, + ...Object.fromEntries(adminCodes.map((c) => [c, detailReasons("routes/admin.py", c)])), + playlist: literalCalls("routes/playlists.py", /_refused\(\s*\d+,\s*"(\w+)"/g), + playlist_move: literalOf("reader/service.py", "MoveReason"), + edit: literalOf("downloads/service.py", "EditReason"), + cookies: literalOf("downloads/credentials.py", "CookieReason"), + url_blocked: literalCalls("downloads/urlguard.py", /^REASON_\w+ = "(\w+)"/gm), + login: literalCalls("routes/auth.py", /_login_refused\(\s*\d+,\s*"(\w+)"/g), + quota: literalOf("downloads/quota.py", "QuotaReason"), + reset: detailReasons("routes/auth.py", "reset"), + collection: detailReasons("routes/plex/collections.py", "collection"), +}; +const recordedCodes = [ + ...Object.keys(REFUSAL_FAMILIES), + ...Object.keys(REFUSAL_BY_REASON), + ...Object.keys(REFUSAL_FIXED), + ...Object.keys(REFUSAL_UNSHOWN), +]; + // The closed English vocabularies the backend WRITES into data, named through `vocab.` // (lib/vocab) by their letters-and-digits key. A word the backend adds shows in English until named — // so each list is read from its source here: the TMDB crew jobs kept, every auto-tag the tagger can @@ -301,6 +351,25 @@ describe("backend ids have their names in both languages", () => { expect(adminCodes.filter((c) => !adminRefusals.some((m) => m[1] === c))).toEqual([]); }); + it("every refusal code the backend answers is said by exactly one record, and no record names another", () => { + expect(backendCodes).toEqual(expect.arrayContaining(["refused", "playlist", "too_large"])); + expect(backendCodes.filter((c) => recordedCodes.filter((r) => r === c).length !== 1)).toEqual( + [], + ); + expect(recordedCodes.filter((c) => !backendCodes.includes(c))).toEqual([]); + }); + + it("read every refusal's reasons (a scan that matches nothing is green over nothing)", () => { + const coded = [...Object.keys(REFUSAL_FAMILIES), ...Object.keys(REFUSAL_BY_REASON)]; + expect(Object.keys(reasonsOf).sort()).toEqual(coded.sort()); + expect(Object.entries(reasonsOf).filter(([, rs]) => rs.length === 0)).toEqual([]); + }); + + it("a code whose reasons share sentences names every reason it is sent with, and no other", () => { + for (const [code, { reasons }] of Object.entries(REFUSAL_BY_REASON)) + expect(Object.keys(reasons).sort(), code).toEqual([...reasonsOf[code]!].sort()); + }); + it("the run list's status chips are the server's run statuses", () => { expect([...RUN_STATUSES].sort()).toEqual([...runStatuses].sort()); }); @@ -456,6 +525,39 @@ describe("backend ids have their names in both languages", () => { } }); + it(`${lang}: every refusal the backend can answer has its sentence`, () => { + const wanted = [ + ...Object.entries(REFUSAL_FAMILIES).flatMap(([code, f]) => [ + ...reasonsOf[code]!.map((r) => `${f.prefix}.${r}${"suffix" in f ? f.suffix : ""}`), + f.fallback, + ]), + ...Object.values(REFUSAL_BY_REASON).flatMap((f) => [ + ...Object.values(f.reasons), + f.fallback, + ]), + ...Object.values(REFUSAL_FIXED), + "errors.generic", + ]; + expect(missing([...new Set(wanted)])).toEqual([]); + }); + + it(`${lang}: no sentence for a refusal reason the backend cannot send`, () => { + // A family whose prefix is its own (the shared `errors.admin`, and the sign-in screens, which + // name more than the one-line refusal, are checked elsewhere or not at all). + for (const [code, f] of Object.entries(REFUSAL_FAMILIES)) { + if (f.prefix === "errors.admin" || "suffix" in f) continue; + const fallbackId = f.fallback.startsWith(`${f.prefix}.`) + ? f.fallback.slice(f.prefix.length + 1) + : null; + expect( + [...idsUnder(keys, f.prefix)].filter( + (r) => r !== fallbackId && !reasonsOf[code]!.includes(r), + ), + code, + ).toEqual([]); + } + }); + it(`${lang}: every admin refusal reason`, () => { expect(missing(adminReasons.map((r) => `errors.admin.${r}`))).toEqual([]); expect([...idsUnder(keys, "errors.admin")].filter((r) => !adminReasons.includes(r))).toEqual( diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index 2549e42c..3e924f73 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -219,6 +219,7 @@ "edit": { "empty_edit": "Choose a trim range or crop area first", "source_not_ready": "The source download isn't ready to edit", + "not_media": "Only video and audio can be edited", "generic": "Couldn't create that edit" }, "upload": { diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index 59f90d89..ff26192c 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -219,6 +219,7 @@ "edit": { "empty_edit": "Előbb válassz vágási tartományt vagy körbevágási területet", "source_not_ready": "A forrásletöltés még nem szerkeszthető", + "not_media": "Csak videó és hang szerkeszthető", "generic": "Nem sikerült létrehozni a vágást" }, "upload": { diff --git a/frontend/src/i18n/unusedKeys.contract.test.ts b/frontend/src/i18n/unusedKeys.contract.test.ts index c7905892..70b29661 100644 --- a/frontend/src/i18n/unusedKeys.contract.test.ts +++ b/frontend/src/i18n/unusedKeys.contract.test.ts @@ -3,6 +3,8 @@ import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest"; +import { REFUSAL_FAMILIES, type RefusalFamily } from "../lib/api/refusalCodes"; + // Every locale key is used by the app. The parity test proves EN and HU agree; nothing proved either // one is still READ — 75 keys had outlived the code that showed them (a whole sidebar sort vocabulary, // the old login page, a pager the DataTable replaced), each a string a translator would keep in step @@ -49,9 +51,14 @@ const code = sources(SRC).join("\n"); const literals = new Set([...code.matchAll(/["'`]([\w.-]+)["'`]/g)].map((m) => m[1]!)); // Each dynamic key as a SHAPE, every interpolation a `*`: `plex.sort.*`, `editor.*.hint`, // `config.choices.*.*` — from a template literal that opens with a key head, or a head concatenated. +// A refusal's sentence is built from its record (`lib/api/refusalCodes`) rather than a template the +// scan could read, so each family's prefix is a shape of its own. const shapes = [ ...[...code.matchAll(/`([\w.]+\.(?:\$\{[^}`]*\}|[\w.-])*)`/g)].map((m) => m[1]!), ...[...code.matchAll(/["'`]([\w.]+\.)["'`]\s*\+/g)].map((m) => `${m[1]!}\${}`), + ...Object.values(REFUSAL_FAMILIES).map( + (f: RefusalFamily) => `${f.prefix}.\${reason}${f.suffix ?? ""}`, + ), ] .filter((s) => s.includes("${")) .map((s) => s.replace(/\$\{[^}]*\}/g, "*")); diff --git a/frontend/src/lib/api/refusalCodes.ts b/frontend/src/lib/api/refusalCodes.ts new file mode 100644 index 00000000..bf42be1d --- /dev/null +++ b/frontend/src/lib/api/refusalCodes.ts @@ -0,0 +1,104 @@ +// Which sentence answers each server refusal (`{ code, reason, … }`), as data: `localizeDetail` says a +// refusal through these records, and `i18n/backendIds.contract.test.ts` reads every code and reason the +// backend can send and requires each to land here with its sentence in both languages. A refusal that +// fell through to the generic "couldn't carry out that action" used to stay unnoticed — the language +// parity test is green when EN and HU miss the same key. +// +// No imports: the contract test reads this module in a plain node run, without i18n. + +/** A code whose reasons each name their own sentence: `.`. */ +export type RefusalFamily = { prefix: string; suffix?: string; fallback: string }; + +export const REFUSAL_FAMILIES = { + // Every other refusal (backend `app/refusal.py`): one family, the reason naming the sentence and the + // rest of the detail its values. + refused: { prefix: "errors.refused", fallback: "errors.generic" }, + // An action YouTube refused (routes/_common.youtube_refusal): what the reader can do about it — wait + // for the budget, reconnect, or retry. An unknown reason reads as the retryable one. + youtube: { prefix: "errors.youtube", fallback: "errors.youtube.unavailable" }, + // Any row that is gone — deleted in another tab or device, or never the caller's (a stranger's row + // 404s the same way); `reason` names WHAT is gone. + not_found: { prefix: "errors.notFound", fallback: "errors.notFound.generic" }, + // A clip the server will not cut (downloads/clip): the range, the frame, the source; a cap comes + // with the refusal (`limit_s`). + clip: { prefix: "clip.errors", fallback: "clip.failed" }, + // An admin action on an account or an invite: an address no longer approved, a link or invitation + // with nothing to go to or no mail to go by. The toast's title names the button; this is the WHY, + // which used to be the generic "try again" no retry could fix. One namespace for the three codes. + activate: { prefix: "errors.admin", fallback: "errors.generic" }, + resend: { prefix: "errors.admin", fallback: "errors.generic" }, + invite_send: { prefix: "errors.admin", fallback: "errors.generic" }, + // A playlist action the server refused (Watch later can't be deleted, YouTube said no, …). + playlist: { prefix: "playlists.errors", fallback: "errors.generic" }, + // The one-step "move to another list": both reasons mean the list moved under you, named apart. + playlist_move: { prefix: "playlists.moveErrors", fallback: "playlists.moveFailed" }, + edit: { prefix: "downloads.errors.edit", fallback: "downloads.errors.edit.generic" }, + cookies: { prefix: "downloads.errors.cookies", fallback: "downloads.errors.cookies.generic" }, + // A download URL the server refuses to fetch. The reasons are deliberately distinct — "we could not + // look that host up" is a different thing to tell someone than "we will not fetch from that address". + url_blocked: { + prefix: "downloads.errors.urlBlocked", + fallback: "downloads.errors.urlBlocked.generic", + }, + // Password sign-in refusals, in one line. The sign-in card renders the account-shaped ones as a full + // panel off `detailData`; this is the form every other consumer (and the card's fallback) gets. + login: { + prefix: "welcome.auth.blocked", + suffix: ".short", + fallback: "welcome.auth.genericError", + }, +} satisfies Record; + +/** A code whose few reasons share sentences: each reason the backend sends is named here. */ +export const REFUSAL_BY_REASON = { + quota: { + reasons: { + max_bytes: "downloads.errors.quota.max_bytes", + max_jobs: "downloads.errors.quota.max_jobs", + }, + fallback: "downloads.errors.quota.generic", + }, + // The set-password link. Its failure is one a user meets by simply clicking an old link, so it may + // not answer in hardcoded English under a bilingual UI. + reset: { + reasons: { weak_password: "welcome.auth.weakPassword", invalid: "welcome.auth.deadLink.short" }, + fallback: "welcome.auth.deadLink.short", + }, + // A Plex collection name already taken in that library (the one collection-edit refusal). + collection: { + reasons: { name_taken: "plex.collection.nameTaken" }, + fallback: "plex.collection.editFailed", + }, +} satisfies Record; fallback: string }>; + +/** A code with one sentence, whatever else it carries. */ +export const REFUSAL_FIXED = { + // The email confirmation link — like `reset`, met by clicking an old link. + verify: "welcome.auth.verifyInvalid", + // Local-upload ingest: the per-file cap carries the limit in bytes; empty is a bare code. + upload_too_large: "downloads.errors.upload.tooLarge", + upload_empty: "downloads.errors.upload.empty", + // The EDGE ceiling's refusal (`bodylimit.TOO_LARGE_BODY`), shared by every route and carrying no + // number. On a DEFAULT install it is what an over-cap upload gets, because the ceiling and the + // route's own cap are the same figure and the edge answers first — the numbered `upload_too_large` + // answers only once an admin has LOWERED the knob (bodylimit.ROUTE_MAX_BODY_BYTES). + // A BACKSTOP, not the plan: the middleware answers without draining, so whether the browser reads + // this at all depends on the client having finished sending (readable up to 200 MiB over loopback, + // a bare `onerror` through a dev proxy). That is why the upload dialog checks the file against + // `upload_max_bytes` BEFORE sending. The sentence stays PAYLOAD-NEUTRAL: a JSON form gets it too + // (`debug_sessions` refuses an over-size draft with the same code), and advice about a file told the + // issue composer's user to pick a smaller one — of nothing. + too_large: "errors.tooLarge", + // A manual sync trigger on cooldown: a pace limit rather than an error, and the copy says so. + sync_cooldown: "errors.syncCooldown", +} satisfies Record; + +/** Codes no screen prints, so they read as the generic failure — each with why nobody reads it. */ +export const REFUSAL_UNSHOWN = { + subtitle: "answers a src; the player polls the track's status and never reads this body", + json_required: "answers the client log beacon, which the UI always sends as JSON", + diagnostics_off: "answers the client log beacon; the client stops streaming on it, silently", + rate_limited: "answers the client log beacon over its rate; dropped lines are not reported", + bad_cursor: "a list cursor the UI itself wrote; only a hand-edited request gets this", + invalid_input: "a NUL byte in a request, which no form of the UI can send", +} satisfies Record; diff --git a/frontend/src/lib/api/refusalText.dom.test.ts b/frontend/src/lib/api/refusalText.dom.test.ts new file mode 100644 index 00000000..138228ac --- /dev/null +++ b/frontend/src/lib/api/refusalText.dom.test.ts @@ -0,0 +1,61 @@ +// @vitest-environment jsdom +// +// A server refusal said through the records (`refusalCodes`), in both languages, by the real i18n — +// one case per record shape, so moving the dispatch into data changed what no reader sees. +import { afterEach, describe, expect, it } from "vitest"; +import i18n from "../../i18n"; +import { localizeDetail } from "./refusalText"; + +afterEach(async () => { + await i18n.changeLanguage("en"); +}); + +describe.each(["en", "hu"])("localizeDetail (%s)", (lng) => { + const say = async (d: unknown) => { + await i18n.changeLanguage(lng); + return localizeDetail(d); + }; + + it("names an edit refused for a file that is not video or audio", async () => { + const said = await say({ code: "edit", reason: "not_media" }); + expect(said).toBe(i18n.t("downloads.errors.edit.not_media")); + expect(said).not.toBe(i18n.t("downloads.errors.edit.generic")); + }); + + it("says a family's reason, and its own fallback for a reason it does not know", async () => { + expect(await say({ code: "playlist_move", reason: "same_playlist" })).toBe( + i18n.t("playlists.moveErrors.same_playlist"), + ); + expect(await say({ code: "playlist_move", reason: "nope" })).toBe( + i18n.t("playlists.moveFailed"), + ); + }); + + it("reads a sign-in refusal's one-line form", async () => { + expect(await say({ code: "login", reason: "suspended" })).toBe( + i18n.t("welcome.auth.blocked.suspended.short"), + ); + }); + + it("puts a refusal's numbers into its sentence", async () => { + expect(await say({ code: "quota", reason: "max_jobs", limit: 7 })).toContain("7"); + expect(await say({ code: "reset", reason: "weak_password", min: 12 })).toContain("12"); + expect(await say({ code: "refused", reason: "view_name_taken", name: "Esti" })).toContain( + "Esti", + ); + }); + + it("says a shared-sentence code's fallback, and a fixed code's one sentence", async () => { + expect(await say({ code: "reset", reason: "invalid" })).toBe( + i18n.t("welcome.auth.deadLink.short"), + ); + expect(await say({ code: "sync_cooldown" })).toBe(i18n.t("errors.syncCooldown")); + }); + + it("reads a code no screen shows, or one it does not know, as the generic failure", async () => { + expect(await say({ code: "bad_cursor" })).toBe(i18n.t("errors.generic")); + expect(await say({ code: "never_sent" })).toBe(i18n.t("errors.generic")); + expect(await say({ code: "constructor" })).toBe(i18n.t("errors.generic")); + expect(await say(null)).toBe(i18n.t("errors.generic")); + }); +}); diff --git a/frontend/src/lib/api/refusalText.ts b/frontend/src/lib/api/refusalText.ts index 1a90d127..abdcdb8a 100644 --- a/frontend/src/lib/api/refusalText.ts +++ b/frontend/src/lib/api/refusalText.ts @@ -3,15 +3,52 @@ // moment, and the notice store cannot import the request core that imports it. import i18n from "../../i18n"; import { formatBytes, formatDecimal } from "../format"; +import { + REFUSAL_BY_REASON, + REFUSAL_FAMILIES, + REFUSAL_FIXED, + type RefusalFamily, +} from "./refusalCodes"; -// Localize a STRUCTURED server error ({ code, reason, ... }) into the user's language. FastAPI -// usually returns a plain string `detail`, but some routes (e.g. download quota/edit) return a -// structured object with an i18n reason key + numbers so HU/DE users don't get hardcoded English -// with a dot-decimal separator. Falls back to a generic message for unknown shapes. -// The refusal codes of the admin's account and invite actions (routes/admin.py). Their reasons share -// one namespace, `errors.admin.`, which i18n/backendIds.contract checks against the backend. -const ADMIN_REFUSALS = new Set(["activate", "resend", "invite_send"]); +type Detail = { + code?: string; + reason?: string; + limit?: number; + limit_s?: number; + min?: number; +} | null; +// Which sentence answers which code lives in `refusalCodes` — the records the backendIds contract holds +// against every code and reason the backend can send. A Map, so a code is looked up as data only. +const families = new Map(Object.entries(REFUSAL_FAMILIES)); +const byReason = new Map( + Object.entries(REFUSAL_BY_REASON).map(([code, f]) => [ + code, + { reasons: new Map(Object.entries(f.reasons)), fallback: f.fallback }, + ]), +); +const fixed = new Map(Object.entries(REFUSAL_FIXED)); + +// The numbers a refusal carries, in the reader's number format: a download cap in GB, a clip cap in +// minutes, an upload cap in bytes, a password's minimum length; a generic `refused` interpolates +// whatever values its sentence names (`{{min}}`, `{{name}}`, …). +function valuesOf(o: NonNullable): Record { + const lang = i18n.language; + if (o.code === "refused") return { ...o }; + if (o.code === "clip") + return { minutes: o.limit_s == null ? undefined : formatDecimal(o.limit_s / 60, lang, 1, 0) }; + if (o.code === "quota") + return o.reason === "max_bytes" + ? { size: formatDecimal((o.limit || 0) / 1_073_741_824, lang, 1, 0) } + : { limit: o.limit }; + if (o.code === "reset") return { min: o.limit ?? o.min }; + if (o.code === "upload_too_large") return { size: formatBytes(o.limit, lang) }; + return {}; +} + +// Localize a STRUCTURED server error ({ code, reason, ... }) into the user's language. A reason this +// build does not know reads as its code's fallback sentence rather than a raw key, and a code it does +// not know (or one no screen shows, `REFUSAL_UNSHOWN`) as the generic failure. export function localizeDetail(d: unknown): string { const t = i18n.t.bind(i18n); // FastAPI request-validation errors: `detail` is an ARRAY of { loc, msg, type }. The UI sends @@ -19,102 +56,18 @@ export function localizeDetail(d: unknown): string { // `loc` field names are English, snake_case internal tokens — surface a clean, fully localized // "check your input" message rather than leaking them into a translated sentence. if (Array.isArray(d)) return t("errors.validation"); - const o = d as - | { code?: string; reason?: string; limit?: number; limit_s?: number; min?: number } - | null - | undefined; - if (o?.code === "quota") { - if (o.reason === "max_bytes") { - const gb = formatDecimal((o.limit || 0) / 1_073_741_824, i18n.language, 1, 0); - return t("downloads.errors.quota.max_bytes", { size: gb }); - } - if (o.reason === "max_jobs") return t("downloads.errors.quota.max_jobs", { limit: o.limit }); - return t("downloads.errors.quota.generic"); - } - if (o?.code === "edit") - return t(`downloads.errors.edit.${o.reason}`, t("downloads.errors.edit.generic")); - // A clip the server will not cut (downloads/clip): the range, the frame, the source. An unknown - // reason reads as the plain failure rather than a key; a cap comes with the refusal (`limit_s`). - if (o?.code === "clip") - return t(`clip.errors.${o.reason}`, { - defaultValue: t("clip.failed"), - minutes: o.limit_s == null ? undefined : formatDecimal(o.limit_s / 60, i18n.language, 1, 0), + const o = d as Detail | undefined; + if (!o?.code) return t("errors.generic"); + const family = families.get(o.code); + if (family) + return t(`${family.prefix}.${o.reason}${family.suffix ?? ""}`, { + ...valuesOf(o), + defaultValue: t(family.fallback), }); - if (o?.code === "cookies") - return t(`downloads.errors.cookies.${o.reason}`, t("downloads.errors.cookies.generic")); - // Password sign-in refusals. The card renders the account-shaped ones as a full panel - // off `detailData`; this is the one-line form every other consumer (and the card's own fallback) - // gets. A reason this build doesn't know falls through to the generic message rather than showing - // a raw key, which is what an i18n miss would otherwise put on screen. - if (o?.code === "login") - return t(`welcome.auth.blocked.${o.reason}.short`, t("welcome.auth.genericError")); - // The two email-link confirmations. Their failure is one a user meets by simply clicking an old - // link, so it is the last place that may answer in hardcoded English under a bilingual UI. - if (o?.code === "verify") return t("welcome.auth.verifyInvalid"); - if (o?.code === "reset") - return o.reason === "weak_password" - ? t("welcome.auth.weakPassword", { min: o.limit ?? o.min }) - : t("welcome.auth.deadLink.short"); - // Local-upload ingest: the per-file cap carries the limit in bytes; empty is a bare code. - if (o?.code === "upload_too_large") - return t("downloads.errors.upload.tooLarge", { size: formatBytes(o.limit, i18n.language) }); - if (o?.code === "upload_empty") return t("downloads.errors.upload.empty"); - // The EDGE ceiling's refusal (`bodylimit.TOO_LARGE_BODY`), shared by every route and carrying no - // number. Not a theoretical case: on a DEFAULT install it is what an over-cap upload gets, - // because the ceiling and the route's own cap are the same figure and the edge answers first — - // the route's numbered `upload_too_large` above is the answer only once an admin has LOWERED the - // knob (see bodylimit.ROUTE_MAX_BODY_BYTES). Without this branch that refusal fell through to - // the generic "couldn't carry out that action", the one message no retry can fix. - // - // A BACKSTOP, not the plan. The middleware answers without draining, so whether the browser gets - // to read this at all depends on the client having finished sending: measured readable up to - // 200 MiB over loopback, and measured as a bare `onerror` through a dev proxy. That is why the - // upload dialog checks the file against `upload_max_bytes` BEFORE sending — this branch is what - // happens when that check could not run. - // - // `errors.tooLarge` stays PAYLOAD-NEUTRAL for that reason. It is also what a JSON form gets - // (`debug_sessions` refuses an over-size draft with the same bare code), so wording it as advice - // about a file told the issue composer's user to pick a smaller one — of nothing. The - // file-specific sentence is `downloads.errors.upload.tooLarge`, one branch up, which has a number. - if (o?.code === "too_large") return t("errors.tooLarge"); - // SEC-12: a download URL the server refuses to fetch. The reasons are deliberately distinct — - // "we could not look that host up" is a different thing to tell someone than "we will not fetch - // from that address" — so an unknown reason falls back to the generic refusal rather than a key. - if (o?.code === "url_blocked") - return t(`downloads.errors.urlBlocked.${o.reason}`, t("downloads.errors.urlBlocked.generic")); - // The one-step "move to another list". Both reasons mean the same thing to a reader — - // the list moved under you — but they are different enough to be worth naming separately, and an - // unknown reason falls back to the plain failure rather than printing a key. - if (o?.code === "playlist_move") - return t(`playlists.moveErrors.${o.reason}`, t("playlists.moveFailed")); - // A Plex collection name already taken in that library (the one collection-edit refusal). - if (o?.code === "collection") - return o.reason === "name_taken" - ? t("plex.collection.nameTaken") - : t("plex.collection.editFailed"); - // An action YouTube refused (routes/_common.youtube_refusal): what the reader can do about it — - // wait for the budget, reconnect, or retry. An unknown reason reads as the retryable one. - if (o?.code === "youtube") - return t(`errors.youtube.${o.reason}`, t("errors.youtube.unavailable")); - // Every other refusal (backend `app/refusal.py`): one family, the reason naming the sentence and - // the rest of the detail its values (`{{min}}`, `{{name}}`, …). A reason this build does not know - // reads as the generic failure rather than a key. - if (o?.code === "refused") - return t(`errors.refused.${o.reason}`, { ...o, defaultValue: t("errors.generic") }); - // A playlist action the server refused (Watch later can't be deleted, YouTube said no, …). - if (o?.code === "playlist") return t(`playlists.errors.${o.reason}`, t("errors.generic")); - // An admin action on an account or an invite that the server refused: activating an address that - // is no longer approved, a confirmation link or invitation with nothing to go to or no mail to go - // by. The toast's title already names the button; this is the WHY, and it used to be the generic - // "try again", which no retry could fix. - if (ADMIN_REFUSALS.has(o?.code ?? "")) return t(`errors.admin.${o?.reason}`, t("errors.generic")); - // Any row that is gone — deleted in another tab or device, or never the caller's (a stranger's - // row 404s the same way). `reason` names WHAT is gone; one this build doesn't know says it - // generically rather than printing a key. - if (o?.code === "not_found") - return t(`errors.notFound.${o.reason}`, t("errors.notFound.generic")); - // SEC-14: a manual sync trigger on cooldown. Not an error the user caused so much as a pace - // limit, and the copy says so. - if (o?.code === "sync_cooldown") return t("errors.syncCooldown"); - return t("errors.generic"); + const shared = byReason.get(o.code); + if (shared) { + return t(shared.reasons.get(o.reason ?? "") ?? shared.fallback, valuesOf(o)); + } + const one = fixed.get(o.code); + return one ? t(one, valuesOf(o)) : t("errors.generic"); } From ceadf69e438cc173ed4bc9dcf9610dfce3a4e5fa Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 31 Oct 2026 01:00:26 +0100 Subject: [PATCH 2/6] fix(i18n): hold every refusal reason by type, and refuse what the scan cannot read Review round 1 of the refusal contract: it claimed more than it held. - The other reason carriers get their Literal too (BlockedUrl, ClipError, FolderError, the playlist and sign-in refusal helpers), so the type checker refuses a reason passed by variable or an unknown literal, and the contract reads one shape. - A literal reason in a detail dict built anywhere counts as a reason of its code; a code the scan cannot read, a refused() whose reason it cannot read, and a new reason passed by variable each fail the contract until listed with what holds them. - A sign-in refusal key the backend no longer sends is caught too. - localizeDetail's numbers (download cap in GB, upload cap in bytes) and the shared-sentence fallbacks are tested in both languages. - One Literal parser in the contract; a stale comment fixed. --- backend/app/downloads/clip.py | 18 +- backend/app/downloads/folders.py | 13 +- backend/app/downloads/urlguard.py | 23 ++- backend/app/refusal.py | 3 +- backend/app/routes/auth.py | 11 +- backend/app/routes/playlists.py | 19 +- frontend/src/i18n/backendIds.contract.test.ts | 189 +++++++++++------- frontend/src/lib/api/refusalText.dom.test.ts | 19 ++ 8 files changed, 213 insertions(+), 82 deletions(-) diff --git a/backend/app/downloads/clip.py b/backend/app/downloads/clip.py index 9d3f57d4..af2cb46b 100644 --- a/backend/app/downloads/clip.py +++ b/backend/app/downloads/clip.py @@ -22,6 +22,7 @@ import json import math import re from pathlib import Path +from typing import Literal from app.downloads.edit import AAC_ARGS, EVEN_FRAME, INPUT_GUARD, X264_ARGS from app.plex.sync import HDR_TRANSFERS @@ -57,11 +58,26 @@ _CROP_ASPECTS = (0.25, 4.0) _EDGE_SLACK = 1e-3 +# Every reason a clip is refused; the UI names each one (`clip.errors.`, held by the frontend's +# backendIds contract, which reads this Literal). +ClipReason = Literal[ + "bad_source", + "bad_range", + "too_short", + "too_long", + "bad_audio", + "bad_crop", + "not_found", + "media_missing", + "past_end", +] + + class ClipError(Exception): """A clip request that cannot be cut; `reason` is an i18n-mappable key, `params` the numbers its message names (the cap itself, so the client never says a stale one).""" - def __init__(self, reason: str, **params: float): + def __init__(self, reason: ClipReason, **params: float): self.reason = reason self.params = params super().__init__(reason) diff --git a/backend/app/downloads/folders.py b/backend/app/downloads/folders.py index f38eb8ce..fffb75c0 100644 --- a/backend/app/downloads/folders.py +++ b/backend/app/downloads/folders.py @@ -24,6 +24,7 @@ kept it — and its `ready_at`, the moment it finished (`stamp_done`). from datetime import datetime, timedelta, timezone from enum import StrEnum +from typing import Literal from urllib.parse import urlsplit from sqlalchemy import select @@ -181,10 +182,20 @@ def stamp_done(db: Session, job: DownloadJob) -> None: job.expiry_warned = False +# Every folder change the rules refuse, answered as a generic refusal; the UI names each one +# (`errors.refused.`, held by the frontend's backendIds contract, which reads this Literal). +FolderReason = Literal[ + "file_folder_gone", + "file_folder_name_taken", + "file_folder_not_its_source", + "file_folder_not_own", +] + + class FolderError(Exception): """A folder change the rules refuse; `reason` is the refusal the route answers with.""" - def __init__(self, reason: str) -> None: + def __init__(self, reason: FolderReason) -> None: super().__init__(reason) self.reason = reason diff --git a/backend/app/downloads/urlguard.py b/backend/app/downloads/urlguard.py index 3ca5a946..575c7836 100644 --- a/backend/app/downloads/urlguard.py +++ b/backend/app/downloads/urlguard.py @@ -33,22 +33,31 @@ user-controlled destination to guard. import ipaddress import socket +from typing import Literal from urllib.parse import urlparse # Reason codes travel to the client inside the structured 422 detail, so they are part of the API -# vocabulary (frontend `localizeDetail`) — keep them stable. -REASON_SCHEME = "scheme" # not http(s) -REASON_NO_HOST = "no_host" # URL carries no hostname -REASON_MALFORMED = "malformed" # unparseable authority or port; urlparse raises late, so guard it -REASON_UNRESOLVABLE = "unresolvable" # DNS said nothing (honest: not the same as "blocked") -REASON_PRIVATE = "private" # resolves into a network the server must not fetch from +# vocabulary — keep them stable. The UI names each one (`downloads.errors.urlBlocked.`, held by +# the frontend's backendIds contract, which reads this Literal). +UrlBlockedReason = Literal["scheme", "no_host", "malformed", "unresolvable", "private"] +REASON_SCHEME: UrlBlockedReason = "scheme" # not http(s) +REASON_NO_HOST: UrlBlockedReason = "no_host" # URL carries no hostname +REASON_MALFORMED: UrlBlockedReason = ( + "malformed" # unparseable authority or port; urlparse raises late +) +REASON_UNRESOLVABLE: UrlBlockedReason = ( + "unresolvable" # DNS said nothing (not the same as "blocked") +) +REASON_PRIVATE: UrlBlockedReason = ( + "private" # resolves into a network the server must not fetch from +) class BlockedUrl(Exception): """A user-supplied URL that must not be fetched server-side. ``reason`` is one of the REASON_* codes above.""" - def __init__(self, reason: str): + def __init__(self, reason: UrlBlockedReason): super().__init__(reason) self.reason = reason diff --git a/backend/app/refusal.py b/backend/app/refusal.py index 8e6b9ffd..c092bb55 100644 --- a/backend/app/refusal.py +++ b/backend/app/refusal.py @@ -11,7 +11,8 @@ A test (`test_http_details.py`) reads every `HTTPException` in the app and fails and the frontend's contract test (`i18n/backendIds.contract.test.ts`) requires every `code` a detail carries to be in one record of `lib/api/refusalCodes.ts`, and every reason it can be sent with — a literal, or a `Literal` type when it travels in a variable — to have its sentence in both languages. -So the rule is enforced, not described. +A code or reason that scan cannot read fails it until it is listed with what holds it. So the rule is +enforced, not described. """ from fastapi import HTTPException diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index a36f4584..92c6a4ef 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -976,7 +976,16 @@ def _resend_verify_if_eligible(email: str) -> None: send_verification_link(db, user) -def _login_refused(status: int, reason: str, user: User | None = None, **extra) -> HTTPException: +# Every refusal a password sign-in answers with; the UI names each one (`welcome.auth.blocked.`, +# held by the frontend's backendIds contract, which reads this Literal). +LoginRefusal = Literal[ + "rate_limited", "invalid", "suspended", "email_unverified", "pending_approval" +] + + +def _login_refused( + status: int, reason: LoginRefusal, user: User | None = None, **extra +) -> HTTPException: """Every refusal password_login can answer with, in the codebase's structured `{code, reason}` shape so the SPA can say it in the user's own language (the old bare English strings were unreadable under a bilingual UI, and unusable as anything but a one-line error). diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 076409a5..f27ea526 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -3,7 +3,7 @@ CRUD (create / rename / delete, add / remove / reorder items) plus YouTube two-w mirror the user's own YouTube playlists in (`/sync-youtube`) and push/revert local edits back out (`/{id}/push-plan`, `/{id}/revert-youtube`), behind the optional write scope.""" -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Literal from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel, Field @@ -53,7 +53,22 @@ def _check_pull_cooldown(user: User) -> None: raise HTTPException(status_code=429, detail={"code": "sync_cooldown"}) -def _refused(status: int, reason: str) -> HTTPException: +# Every playlist action the server refuses; the UI names each one (`playlists.errors.`, held by +# the frontend's backendIds contract, which reads this Literal). +PlaylistRefusal = Literal[ + "name_required", + "not_on_youtube", + "quota_exceeded", + "sync_all_busy", + "sync_busy", + "watch_later_not_pushable", + "watch_later_undeletable", + "youtube_editing_off", + "youtube_read_scope", +] + + +def _refused(status: int, reason: PlaylistRefusal) -> HTTPException: """A playlist action the server refuses, as a reason the UI translates (`playlists.errors.`) — a bare English detail reached the Hungarian UI verbatim.""" return HTTPException(status_code=status, detail={"code": "playlist", "reason": reason}) diff --git a/frontend/src/i18n/backendIds.contract.test.ts b/frontend/src/i18n/backendIds.contract.test.ts index 29d7b082..524ed7db 100644 --- a/frontend/src/i18n/backendIds.contract.test.ts +++ b/frontend/src/i18n/backendIds.contract.test.ts @@ -90,9 +90,9 @@ const runEnum = (name: string) => const runStatuses = runEnum("RunStatus"); const runTriggers = runEnum("RunTrigger"); const quotaActions = enumValues("quota.py"); -// The admin actions' refusal reasons → `errors.admin.`. The codes are read from -// `localizeDetail`'s own ADMIN_REFUSALS, so a code added there is scanned here too; a detail dict -// ruff wraps over several lines still matches. +// The admin actions' refusal reasons → `errors.admin.`. The codes are the refusal records +// under that prefix (lib/api/refusalCodes), so a code recorded there is scanned here too; a detail +// dict ruff wraps over several lines still matches. const adminCodes = Object.entries(REFUSAL_FAMILIES) .filter(([, f]) => f.prefix === "errors.admin") .map(([code]) => code); @@ -172,29 +172,38 @@ const goneWhats = [ ), ), ]; -// Every generic refusal → `errors.refused.`: each `refused(, "")` anywhere in -// the backend (not `_refused`, a router's own family), plus the values of the settings map, which -// `setting_refused` picks from at runtime, and every `FolderError` reason. -const appSources = (dir = ""): string[] => +// A backend `Name = Literal["a", "b"]` alias, by name: the reasons a typed parameter accepts, which +// basedpyright then holds at every call site — a reason passed by variable included. +const literalOf = (file: string, name: string) => + [ + ...( + backend(file).match(new RegExp(`^${name} = Literal\\[([^\\]]*)\\]`, "m"))?.[1] ?? "" + ).matchAll(/"(\w+)"/g), + ].map((m) => m[1]!); +// Every backend source, by its path under app/. +const appPaths = (dir = ""): string[] => readdirSync(new URL(`../../../backend/app/${dir}`, import.meta.url), { withFileTypes: true, }).flatMap((e) => e.isDirectory() - ? appSources(`${dir}${e.name}/`) + ? e.name === "__pycache__" + ? [] + : appPaths(`${dir}${e.name}/`) : e.name.endsWith(".py") - ? [backend(`${dir}${e.name}`)] + ? [`${dir}${e.name}`] : [], ); +const appSources = (): string[] => appPaths().map(backend); +// Every generic refusal → `errors.refused.`: each `refused(, "")` anywhere in +// the backend (not `_refused`, a router's own family), the values of the settings map, which +// `setting_refused` picks from at runtime, and the folder rules' `FolderReason`, answered with +// `refused(409, e.reason)`. const refusedReasons = [ ...new Set([ ...appSources().flatMap((src) => [...src.matchAll(/(? m[1]!), ), - // A folder change the rules refuse travels as `FolderError("")` and is answered with - // `refused(409, e.reason)`, which the pattern above cannot read. - ...appSources().flatMap((src) => - [...src.matchAll(/FolderError\("(\w+)"\)/g)].map((m) => m[1]!), - ), + ...literalOf("downloads/folders.py", "FolderReason"), ...[ ...(backend("refusal.py").split("_SETTING_REASONS = {")[1]?.split("}")[0] ?? "").matchAll( /:\s*"(\w+)"/g, @@ -202,61 +211,90 @@ const refusedReasons = [ ].map((m) => m[1]!), ]), ]; -// A refused YouTube action → `errors.youtube.`, the reasons read from their Literal. -const youtubeReasons = [ - ...( - backend("youtube/client.py").match(/^YouTubeRefusal = Literal\[([^\]]*)\]/m)?.[1] ?? "" - ).matchAll(/"(\w+)"/g), -].map((m) => m[1]!); -// A clip the server will not cut → `clip.errors.`: every `ClipError(""…)` in the backend -// (a reason may carry its numbers after it: `ClipError("too_long", limit_s=…)`). -const clipReasons = [ - ...new Set( - appSources().flatMap((src) => [...src.matchAll(/ClipError\("(\w+)"[,)]/g)].map((m) => m[1]!)), - ), -]; +// A `refused(…)` whose reason the scan above cannot read. Only two may: a folder rule's `e.reason` +// (the nearest `except` above it catches `FolderError`, typed `FolderReason`) and the settings map. +const opaqueRefused = appPaths().flatMap((f) => { + const src = backend(f); + return [...src.matchAll(/(? { + const arg = m[1]!.trim(); + if (f === "refusal.py" && arg.startsWith("_SETTING_REASONS[")) return false; + const caught = [...src.slice(0, m.index).matchAll(/except ([^\n:]*):/g)].pop()?.[1] ?? ""; + return !(arg === "e.reason" && /FolderError as e$/.test(caught)); + }) + .map((m) => `${f}: ${m[0]}`); +}); +const youtubeReasons = literalOf("youtube/client.py", "YouTubeRefusal"); +const clipReasons = literalOf("downloads/clip.py", "ClipReason"); -// Every refusal code the backend answers: each `"code": ""` of a detail anywhere in the app. A -// subtitle's status payload (`{"state": …, "code": …}`) is the player's own vocabulary, not a refusal. -const backendCodes = [ +// Every `"code": …` a backend dict carries, with the `"reason": …` that follows it. Two modules use a +// `code` key for something that is not a refusal; everything else is one. +const NOT_REFUSALS: Record = { + "plex/subtitles.py": + "the player's subtitle status vocabulary (`/status`), read by the player itself", + "plex/tmdb.py": "a production country's ISO code", +}; +const detailSites = appPaths() + .filter((f) => !(f in NOT_REFUSALS)) + .flatMap((f) => + [ + ...backend(f).matchAll( + /"code":\s*(?:"(\w+)"|([^\s,}]+))(?:,\s*"reason":\s*(?:"(\w+)"|([^\s,}]+)))?/g, + ), + ].map((m) => ({ f, code: m[1], codeExpr: m[2], reason: m[3], reasonExpr: m[4] })), + ); +const backendCodes = [...new Set(detailSites.flatMap((s) => (s.code ? [s.code] : [])))]; +// A code the scan cannot read (`"code": SOME_CONSTANT`) is a refusal no record is checked for. +const opaqueCodes = detailSites.filter((s) => !s.code).map((s) => `${s.f}: "code": ${s.codeExpr}`); +// A reason passed by variable, each pinned to what holds it, so a new one has to say how it is held. +const OPAQUE_REASONS: Record = { + "refusal.py: refused ← reason": "a literal at each refused() call, else opaqueRefused", + "routes/auth.py: login ← reason": "_login_refused's LoginRefusal", + "routes/downloads.py: url_blocked ← urlguard.REASON_MALFORMED": "UrlBlockedReason", + "routes/downloads.py: url_blocked ← e.reason": "BlockedUrl's UrlBlockedReason", + "routes/downloads.py: cookies ← e.reason": "InvalidCookies' CookieReason", + "routes/downloads.py: edit ← e.reason": "EditError's EditReason", + "routes/downloads.py: clip ← e.reason": "ClipError's ClipReason", + "routes/files.py: quota ← e.reason": "QuotaExceeded's QuotaReason", + "routes/playlists.py: playlist ← reason": "_refused's PlaylistRefusal", + "routes/playlists.py: playlist_move ← e.reason": "MoveRefused's MoveReason", + "routes/plex/playback.py: subtitle ← code": "REFUSAL_UNSHOWN: no screen reads it", + "routes/_common.py: not_found ← what": "opaqueWhats", + "routes/_common.py: youtube ← reason": "YouTubeRefusal", +}; +const opaqueReasons = [ ...new Set( - appSources().flatMap((src) => - src - .split("\n") - .filter((line) => !line.includes('"state":')) - .flatMap((line) => [...line.matchAll(/"code":\s*"(\w+)"/g)].map((m) => m[1]!)), - ), + detailSites + .filter((s) => s.code && s.reasonExpr) + .map((s) => `${s.f}: ${s.code} ← ${s.reasonExpr}`), ), ]; -const literalOf = (file: string, name: string) => - [ - ...( - backend(file).match(new RegExp(`^${name} = Literal\\[([^\\]]*)\\]`, "m"))?.[1] ?? "" - ).matchAll(/"(\w+)"/g), - ].map((m) => m[1]!); -const literalCalls = (file: string, re: RegExp) => [ - ...new Set([...backend(file).matchAll(re)].map((m) => m[1]!)), -]; -const detailReasons = (file: string, code: string) => - literalCalls(file, new RegExp(`"code":\\s*"${code}",\\s*"reason":\\s*"(\\w+)"`, "g")); -// What each refusal code can carry as its reason, read where it is decided: a Literal when the reason -// travels in a variable (`e.reason`), the call's own literal otherwise. -const reasonsOf: Record = { - refused: refusedReasons, - youtube: youtubeReasons, - not_found: goneWhats, - clip: clipReasons, - ...Object.fromEntries(adminCodes.map((c) => [c, detailReasons("routes/admin.py", c)])), - playlist: literalCalls("routes/playlists.py", /_refused\(\s*\d+,\s*"(\w+)"/g), - playlist_move: literalOf("reader/service.py", "MoveReason"), - edit: literalOf("downloads/service.py", "EditReason"), - cookies: literalOf("downloads/credentials.py", "CookieReason"), - url_blocked: literalCalls("downloads/urlguard.py", /^REASON_\w+ = "(\w+)"/gm), - login: literalCalls("routes/auth.py", /_login_refused\(\s*\d+,\s*"(\w+)"/g), - quota: literalOf("downloads/quota.py", "QuotaReason"), - reset: detailReasons("routes/auth.py", "reset"), - collection: detailReasons("routes/plex/collections.py", "collection"), -}; +// A reason written as a literal in a detail dict, wherever that dict is built. +const literalReasons = (code: string) => + detailSites.filter((s) => s.code === code && s.reason).map((s) => s.reason!); +// What each refusal code can carry as its reason: the Literal its typed parameter accepts, or the +// call's own literal — and every literal any detail dict anywhere gives it. +const reasonsOf: Record = Object.fromEntries( + Object.entries({ + refused: refusedReasons, + youtube: youtubeReasons, + not_found: goneWhats, + clip: clipReasons, + ...Object.fromEntries(adminCodes.map((c) => [c, []])), + playlist: literalOf("routes/playlists.py", "PlaylistRefusal"), + playlist_move: literalOf("reader/service.py", "MoveReason"), + edit: literalOf("downloads/service.py", "EditReason"), + cookies: literalOf("downloads/credentials.py", "CookieReason"), + url_blocked: literalOf("downloads/urlguard.py", "UrlBlockedReason"), + login: literalOf("routes/auth.py", "LoginRefusal"), + quota: literalOf("downloads/quota.py", "QuotaReason"), + reset: [], + collection: [], + } as Record).map(([code, rs]) => [ + code, + [...new Set([...rs, ...literalReasons(code)])], + ]), +); const recordedCodes = [ ...Object.keys(REFUSAL_FAMILIES), ...Object.keys(REFUSAL_BY_REASON), @@ -359,6 +397,14 @@ describe("backend ids have their names in both languages", () => { expect(recordedCodes.filter((c) => !backendCodes.includes(c))).toEqual([]); }); + it("every refusal code and reason is one the scans can read, or says what holds it", () => { + // The modules set aside as not refusals still exist and still use the key. + for (const f of Object.keys(NOT_REFUSALS)) expect(backend(f), f).toContain('"code":'); + expect(opaqueCodes).toEqual([]); + expect(opaqueRefused).toEqual([]); + expect(opaqueReasons.sort()).toEqual(Object.keys(OPAQUE_REASONS).sort()); + }); + it("read every refusal's reasons (a scan that matches nothing is green over nothing)", () => { const coded = [...Object.keys(REFUSAL_FAMILIES), ...Object.keys(REFUSAL_BY_REASON)]; expect(Object.keys(reasonsOf).sort()).toEqual(coded.sort()); @@ -542,16 +588,21 @@ describe("backend ids have their names in both languages", () => { }); it(`${lang}: no sentence for a refusal reason the backend cannot send`, () => { - // A family whose prefix is its own (the shared `errors.admin`, and the sign-in screens, which - // name more than the one-line refusal, are checked elsewhere or not at all). + // A family whose prefix is its own (the shared `errors.admin` is checked below). A suffixed + // family shares its prefix with more than its refusals — the sign-in screens' `gate`, + // `addressLabel` — so only an id with the refusal's own key counts as a reason there. for (const [code, f] of Object.entries(REFUSAL_FAMILIES)) { - if (f.prefix === "errors.admin" || "suffix" in f) continue; + if (f.prefix === "errors.admin") continue; + const suffix = "suffix" in f ? f.suffix : ""; const fallbackId = f.fallback.startsWith(`${f.prefix}.`) ? f.fallback.slice(f.prefix.length + 1) : null; expect( [...idsUnder(keys, f.prefix)].filter( - (r) => r !== fallbackId && !reasonsOf[code]!.includes(r), + (r) => + keys.has(`${f.prefix}.${r}${suffix}`) && + r !== fallbackId && + !reasonsOf[code]!.includes(r), ), code, ).toEqual([]); diff --git a/frontend/src/lib/api/refusalText.dom.test.ts b/frontend/src/lib/api/refusalText.dom.test.ts index 138228ac..0a22de0e 100644 --- a/frontend/src/lib/api/refusalText.dom.test.ts +++ b/frontend/src/lib/api/refusalText.dom.test.ts @@ -4,6 +4,7 @@ // one case per record shape, so moving the dispatch into data changed what no reader sees. import { afterEach, describe, expect, it } from "vitest"; import i18n from "../../i18n"; +import { formatBytes } from "../format"; import { localizeDetail } from "./refusalText"; afterEach(async () => { @@ -45,6 +46,24 @@ describe.each(["en", "hu"])("localizeDetail (%s)", (lng) => { ); }); + it("puts a download cap in GB and an upload cap in bytes, in the reader's number format", async () => { + expect(await say({ code: "quota", reason: "max_bytes", limit: 1.5 * 2 ** 30 })).toContain( + lng === "hu" ? "1,5" : "1.5", + ); + const upload = await say({ code: "upload_too_large", limit: 5_242_880 }); + expect(upload).toContain(formatBytes(5_242_880, lng)); + expect(upload).not.toBe(i18n.t("downloads.errors.upload.tooLarge", { size: "" })); + }); + + it("says a shared-sentence code's own fallback for a reason it does not know", async () => { + expect(await say({ code: "quota", reason: "nope" })).toBe( + i18n.t("downloads.errors.quota.generic"), + ); + expect(await say({ code: "collection", reason: "nope" })).toBe( + i18n.t("plex.collection.editFailed"), + ); + }); + it("says a shared-sentence code's fallback, and a fixed code's one sentence", async () => { expect(await say({ code: "reset", reason: "invalid" })).toBe( i18n.t("welcome.auth.deadLink.short"), From 5e8589bbf85ce8ea57788c26eb2d62d574080614 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 31 Oct 2026 15:07:38 +0100 Subject: [PATCH 3/6] fix(i18n): read every refusal from the backend's syntax tree, not by regex The contract read Python with regexes, and each guard added for a missed shape had shapes of its own it missed: a reason after another key, a status passed by name, a second `e.reason` handler with the same text, a pin naming a type nothing checked. backend/tests/refusal_sites.py (stdlib `ast`, run by the contract on the host's Python) reads every `{"code": ...}` detail and follows each reason to its values: literals, both arms of a conditional, module constants, the `Literal` on the parameter or attribute it travels in (on the class an `except` catches), every call of a `str` helper with its defaults, a map's values with its keys held to the index's type. What it cannot follow fails the contract, unless no screen shows that code; no pins. The reason attributes are typed with their Literal, so a subclass or an assignment cannot widen them; InvalidSetting takes a SettingReason, and the settings map is held to it (a reason the map lacked was a 500). The per-family reverse checks are one check by prefix (the admin actions included), and the leaf-key filter applies only to the suffixed sign-in family. --- backend/app/downloads/clip.py | 2 +- backend/app/downloads/credentials.py | 2 +- backend/app/downloads/folders.py | 2 +- backend/app/downloads/quota.py | 2 +- backend/app/downloads/service.py | 2 +- backend/app/downloads/urlguard.py | 2 +- backend/app/reader/service.py | 2 +- backend/app/refusal.py | 23 +- backend/app/sysconfig.py | 9 +- backend/tests/refusal_sites.py | 625 ++++++++++++++++++ backend/tests/test_refusal_sites.py | 278 ++++++++ docs/development.md | 3 +- frontend/src/i18n/backendIds.contract.test.ts | 324 +++------ 13 files changed, 1022 insertions(+), 254 deletions(-) create mode 100644 backend/tests/refusal_sites.py create mode 100644 backend/tests/test_refusal_sites.py diff --git a/backend/app/downloads/clip.py b/backend/app/downloads/clip.py index af2cb46b..4b4b1021 100644 --- a/backend/app/downloads/clip.py +++ b/backend/app/downloads/clip.py @@ -78,7 +78,7 @@ class ClipError(Exception): message names (the cap itself, so the client never says a stale one).""" def __init__(self, reason: ClipReason, **params: float): - self.reason = reason + self.reason: ClipReason = reason self.params = params super().__init__(reason) diff --git a/backend/app/downloads/credentials.py b/backend/app/downloads/credentials.py index 39e1cd93..c80a892d 100644 --- a/backend/app/downloads/credentials.py +++ b/backend/app/downloads/credentials.py @@ -174,7 +174,7 @@ class InvalidCookies(ValueError): def __init__(self, reason: CookieReason): super().__init__(reason) - self.reason = reason + self.reason: CookieReason = reason def validate_cookies(raw: str) -> str: diff --git a/backend/app/downloads/folders.py b/backend/app/downloads/folders.py index fffb75c0..c440b9de 100644 --- a/backend/app/downloads/folders.py +++ b/backend/app/downloads/folders.py @@ -197,7 +197,7 @@ class FolderError(Exception): def __init__(self, reason: FolderReason) -> None: super().__init__(reason) - self.reason = reason + self.reason: FolderReason = reason def own_folder(db: Session, user_id: int, folder_id: int, *, hold: bool = True) -> FileFolder: diff --git a/backend/app/downloads/quota.py b/backend/app/downloads/quota.py index e8d56f85..179b3b92 100644 --- a/backend/app/downloads/quota.py +++ b/backend/app/downloads/quota.py @@ -60,7 +60,7 @@ class QuotaExceeded(Exception): """Raised by check_enqueue; `reason` names the cap it hit (an i18n key on the client).""" def __init__(self, reason: QuotaReason, limit: int, current: int): - self.reason = reason + self.reason: QuotaReason = reason self.limit = limit self.current = current super().__init__(f"download quota exceeded: {reason} ({current}/{limit})") diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index ffc40cbf..e2e80910 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -49,7 +49,7 @@ class EditError(Exception): """Bad edit request (empty spec / source not ready / not media); `reason` names the sentence.""" def __init__(self, reason: EditReason): - self.reason = reason + self.reason: EditReason = reason super().__init__(reason) diff --git a/backend/app/downloads/urlguard.py b/backend/app/downloads/urlguard.py index 575c7836..854aa732 100644 --- a/backend/app/downloads/urlguard.py +++ b/backend/app/downloads/urlguard.py @@ -59,7 +59,7 @@ class BlockedUrl(Exception): def __init__(self, reason: UrlBlockedReason): super().__init__(reason) - self.reason = reason + self.reason: UrlBlockedReason = reason def _is_forbidden(ip: ipaddress.IPv4Address | ipaddress.IPv6Address) -> bool: diff --git a/backend/app/reader/service.py b/backend/app/reader/service.py index 5eb779c0..fa651005 100644 --- a/backend/app/reader/service.py +++ b/backend/app/reader/service.py @@ -311,7 +311,7 @@ class MoveRefused(Exception): def __init__(self, reason: MoveReason): super().__init__(reason) - self.reason = reason + self.reason: MoveReason = reason def touch(pl: Playlist) -> None: diff --git a/backend/app/refusal.py b/backend/app/refusal.py index c092bb55..337a1c8a 100644 --- a/backend/app/refusal.py +++ b/backend/app/refusal.py @@ -7,16 +7,19 @@ whatever sentence the route happened to hand-write. `refused` is the generic fam (`errors.refused.`, the params interpolated into the sentence); the older specific codes (`not_found`, `youtube`, `playlist`, the admin refusals, …) stay, since each is already translated. -A test (`test_http_details.py`) reads every `HTTPException` in the app and fails on a string detail, -and the frontend's contract test (`i18n/backendIds.contract.test.ts`) requires every `code` a detail -carries to be in one record of `lib/api/refusalCodes.ts`, and every reason it can be sent with — a -literal, or a `Literal` type when it travels in a variable — to have its sentence in both languages. -A code or reason that scan cannot read fails it until it is listed with what holds it. So the rule is -enforced, not described. +A test (`test_http_details.py`) reads every `HTTPException` in the app and fails on a string detail. +`tests/refusal_sites.py` reads every `{"code": …}` detail from the syntax tree and follows each reason +to its values: a literal, a `Literal` type on the parameter or attribute it travels in, every call of +a helper like `refused` below, a map's values. The frontend's contract test +(`i18n/backendIds.contract.test.ts`) runs it and requires each code in one record of +`lib/api/refusalCodes.ts`, and each reason's sentence in both languages; a code or reason it cannot +follow fails it, unless no screen shows that code. So the rule is enforced, not described. """ from fastapi import HTTPException +from app.sysconfig import InvalidSetting, SettingReason + def refused(status: int, reason: str, **params: object) -> HTTPException: """The refusal to raise: `raise refused(409, "view_name_taken", name=name)`. `params` travel in @@ -24,9 +27,9 @@ def refused(status: int, reason: str, **params: object) -> HTTPException: return HTTPException(status_code=status, detail={"code": "refused", "reason": reason, **params}) -# `sysconfig.InvalidSetting.reason` → the refusal it answers as. A dict of literals, so the frontend's -# contract test can read every reason this can produce. -_SETTING_REASONS = { +# `sysconfig.InvalidSetting.reason` → the refusal it answers as. A dict of literals, so the refusal +# scan reads every reason this can produce, and holds its keys to `SettingReason`. +_SETTING_REASONS: dict[SettingReason, str] = { "not_a_number": "setting_not_a_number", "too_low": "setting_too_low", "too_high": "setting_too_high", @@ -35,6 +38,6 @@ _SETTING_REASONS = { } -def setting_refused(exc) -> HTTPException: +def setting_refused(exc: InvalidSetting) -> HTTPException: """A value the admin entered that `sysconfig.set_value` refused (`exc`: its `InvalidSetting`).""" return refused(400, _SETTING_REASONS[exc.reason], key=exc.key, **exc.params) diff --git a/backend/app/sysconfig.py b/backend/app/sysconfig.py index d5c1adb5..74607b6b 100644 --- a/backend/app/sysconfig.py +++ b/backend/app/sysconfig.py @@ -443,15 +443,20 @@ def _default(s: ConfigSpec): return getattr(settings, s.default_attr) +# Every reason `set_value` refuses with. `refusal.setting_refused` looks each one up in its map, which +# the refusal scan holds to exactly these keys: a reason the map lacks would answer a 500. +SettingReason = Literal["not_a_number", "too_low", "too_high", "not_a_choice", "no_encryption_key"] + + class InvalidSetting(ValueError): """A value `set_value` refuses, as a reason the UI translates (`errors.refused.setting_`) plus what the sentence needs — the English message is for logs only. A ValueError still, so a caller that only needs "refused or not" keeps working.""" - def __init__(self, key: str, reason: str, **params: object): + def __init__(self, key: str, reason: SettingReason, **params: object): super().__init__(f"{key}: {reason} {params}") self.key = key - self.reason = reason + self.reason: SettingReason = reason self.params = params diff --git a/backend/tests/refusal_sites.py b/backend/tests/refusal_sites.py new file mode 100644 index 00000000..edf79344 --- /dev/null +++ b/backend/tests/refusal_sites.py @@ -0,0 +1,625 @@ +"""Every refusal the backend can answer, read from its syntax tree: each `{"code": …, "reason": …}` detail, +with every reason its code can be sent with. + +The frontend's contract (`i18n/backendIds.contract.test.ts`) runs this and requires a sentence for each +code and reason in both languages. A regex over the source read only the shapes it was written for: a +reason after another key, a status passed by name, a second handler with the same text all went past it. +Here a reason is followed to where it comes from, and what cannot be followed is reported, never guessed: + +- a literal; both arms of `a if c else b`, every operand of `a or b`; +- a module constant (`urlguard.REASON_MALFORMED`), a local variable's assignments; +- a parameter or attribute typed with a `Literal` (basedpyright holds every value passed in), the + attribute found on the class an `except … as e` catches, or the class a parameter is typed with; +- a `str` parameter of a helper (`refused`, `not_found`): followed to every call of it, its default + included, and a helper passed around as a value is unread; +- a call: its return annotation; a map lookup (`_SETTING_REASONS[exc.reason]`): the map's values, and + its keys must be exactly what the index can be. + +Anything else is UNREAD, and the contract fails on it unless no screen shows that code. Stdlib only: it +runs on the host, beside the frontend's tests (`python -I backend/tests/refusal_sites.py` → JSON). +""" + +import ast +import json +import pathlib +import sys +from dataclasses import dataclass, field + +APP = pathlib.Path(__file__).resolve().parents[1] / "app" + +# Files whose `"code"` dicts are data, not refusals: a subtitle track's status payload, TMDB country codes. +NOT_REFUSALS = { + "plex/subtitles.py": "a subtitle track's status payload (its own `code`)", + "plex/tmdb.py": "a production country's ISO code", +} + +Node = ast.AST + + +@dataclass +class Res: + values: set[str] = field(default_factory=set) + unread: list[str] = field(default_factory=list) + + def __or__(self, other: "Res") -> "Res": + return Res(self.values | other.values, self.unread + other.unread) + + +@dataclass +class Module: + path: str # relative to app/, posix + name: str # dotted, `app.…` + tree: ast.Module + parents: dict[Node, Node] + imports: dict[ + str, tuple[str, str | None] + ] # local name → (module, symbol or None for the module) + top: dict[str, Node] # module-level bindings: def, class, or the assigned value + + +def _module_name(path: str) -> str: + parts = path.removesuffix(".py").split("/") + if parts[-1] == "__init__": + parts = parts[:-1] + return ".".join(["app", *parts]) + + +def _load(sources: dict[str, str]) -> dict[str, Module]: + mods: dict[str, Module] = {} + for path, src in sources.items(): + tree = ast.parse(src) + parents = {child: node for node in ast.walk(tree) for child in ast.iter_child_nodes(node)} + imports: dict[str, tuple[str, str | None]] = {} + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and node.module and node.level == 0: + for a in node.names: + imports[a.asname or a.name] = (node.module, a.name) + elif isinstance(node, ast.Import): + for a in node.names: + if a.asname: + imports[a.asname] = (a.name, None) + top: dict[str, Node] = {} + for stmt in tree.body: + if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + top[stmt.name] = stmt + elif isinstance(stmt, ast.Assign): + for t in stmt.targets: + if isinstance(t, ast.Name): + top[t.id] = stmt + elif isinstance(stmt, ast.AnnAssign) and isinstance(stmt.target, ast.Name): + top[stmt.target.id] = stmt + name = _module_name(path) + mods[name] = Module(path, name, tree, parents, imports, top) + return mods + + +class Scan: + def __init__(self, sources: dict[str, str]): + self.mods = _load(sources) + self.errors: list[str] = [] + self._following: set[tuple[str, str, str]] = set() + + # --- names --------------------------------------------------------------------------------------- + + def lookup(self, m: Module, name: str) -> tuple[Module, Node] | tuple[str, None] | None: + """What a module-level name is: (its module, its binding), or (a module's name, None).""" + if name in m.top: + return m, m.top[name] + if name in m.imports: + mod, sym = m.imports[name] + if sym is None: + return (mod, None) if mod in self.mods else None + if f"{mod}.{sym}" in self.mods: + return f"{mod}.{sym}", None + if mod in self.mods: + return self.lookup(self.mods[mod], sym) + return None + + def lookup_expr(self, m: Module, e: Node) -> tuple[Module, Node] | tuple[str, None] | None: + if isinstance(e, ast.Name): + return self.lookup(m, e.id) + if isinstance(e, ast.Attribute): + base = self.lookup_expr(m, e.value) + if base and base[1] is None and isinstance(base[0], str): + return self.lookup(self.mods[base[0]], e.attr) + return None + + def at(self, m: Module, e: Node) -> str: + return f"{m.path}:{getattr(e, 'lineno', '?')}: {ast.unparse(e)}" + + def enclosing(self, m: Module, e: Node, kind: type | tuple[type, ...]) -> Node | None: + node = m.parents.get(e) + while node is not None and not isinstance(node, kind): + node = m.parents.get(node) + return node + + # --- types --------------------------------------------------------------------------------------- + + def ann_values(self, m: Module, ann: Node | None) -> set[str] | None: + """The values a `Literal` annotation allows, or None when it is not one (`str`, a class, …).""" + if ann is None: + return None + if isinstance(ann, ast.Constant): + if ann.value is None: + return set() + if isinstance(ann.value, str): + return self.ann_values(m, ast.parse(ann.value, mode="eval").body) + return None + if isinstance(ann, ast.Subscript) and getattr(ann.value, "id", None) == "Literal": + items = ann.slice.elts if isinstance(ann.slice, ast.Tuple) else [ann.slice] + if all(isinstance(i, ast.Constant) and isinstance(i.value, str) for i in items): + return {i.value for i in items} + return None + if isinstance(ann, ast.BinOp) and isinstance(ann.op, ast.BitOr): + left, right = self.ann_values(m, ann.left), self.ann_values(m, ann.right) + return None if left is None or right is None else left | right + found = self.lookup_expr(m, ann) + if found and isinstance(found[1], ast.Assign) and isinstance(found[0], Module): + if isinstance(found[1].value, (ast.Subscript, ast.BinOp, ast.Name, ast.Attribute)): + return self.ann_values(found[0], found[1].value) + return None + + def classes(self, m: Module, e: Node | None) -> list[tuple[Module, ast.ClassDef]] | None: + if e is None: + return None + if isinstance(e, ast.Tuple): + out = [self.classes(m, x) for x in e.elts] + return None if any(o is None for o in out) else [c for o in out for c in o] + found = self.lookup_expr(m, e) + if found and isinstance(found[0], Module) and isinstance(found[1], ast.ClassDef): + return [(found[0], found[1])] + return None + + def attr_values(self, cm: Module, cls: ast.ClassDef, attr: str) -> set[str] | None: + """An attribute's `Literal` values, from its annotation on the class or `self.attr: T` in a method, + else from a base class.""" + for node in ast.walk(cls): + if isinstance(node, ast.AnnAssign): + t = node.target + if (isinstance(t, ast.Name) and t.id == attr and node in cls.body) or ( + isinstance(t, ast.Attribute) + and t.attr == attr + and getattr(t.value, "id", None) == "self" + ): + return self.ann_values(cm, node.annotation) + for base in cls.bases: + for bm, bc in self.classes(cm, base) or []: + v = self.attr_values(bm, bc, attr) + if v is not None: + return v + return None + + # --- reasons ------------------------------------------------------------------------------------- + + def resolve(self, m: Module, e: Node) -> Res: + if isinstance(e, ast.Constant): + if e.value is None: + return Res() + if isinstance(e.value, str): + return Res({e.value}) + if isinstance(e, ast.IfExp): + return self.resolve(m, e.body) | self.resolve(m, e.orelse) + if isinstance(e, ast.BoolOp) and isinstance(e.op, ast.Or): + out = Res() + for v in e.values: + out = out | self.resolve(m, v) + return out + if isinstance(e, ast.Name): + return self.resolve_name(m, e) + if isinstance(e, ast.Attribute): + return self.resolve_attr(m, e) + if isinstance(e, ast.Call): + found = self.lookup_expr(m, e.func) + if ( + found + and isinstance(found[0], Module) + and isinstance(found[1], (ast.FunctionDef, ast.AsyncFunctionDef)) + ): + v = self.ann_values(found[0], found[1].returns) + if v is not None: + return Res(v) + if isinstance(e, ast.Subscript): + return self.resolve_lookup(m, e) + return Res(unread=[self.at(m, e)]) + + def resolve_name(self, m: Module, e: ast.Name) -> Res: + fn = self.enclosing(m, e, (ast.FunctionDef, ast.AsyncFunctionDef)) + while fn is not None: + assert isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)) + if self.caught_as(m, e, e.id) is not None: + return Res(unread=[self.at(m, e)]) # the exception itself, not a reason + params = {a.arg: a for a in [*fn.args.posonlyargs, *fn.args.args, *fn.args.kwonlyargs]} + if e.id in params: + return self.resolve_param(m, fn, params[e.id], e) + if ( + fn.args.vararg + and fn.args.vararg.arg == e.id + or fn.args.kwarg + and fn.args.kwarg.arg == e.id + ): + return Res(unread=[self.at(m, e)]) + assigned = self.assignments(fn, e.id) + if assigned: + out = Res() + for a in assigned: + if ( + isinstance(a, ast.AnnAssign) + and (v := self.ann_values(m, a.annotation)) is not None + ): + out = out | Res(v) + elif ( + isinstance(a, (ast.Assign, ast.AnnAssign)) + and a.value is not None + and not any( + isinstance(t, (ast.Tuple, ast.List)) for t in getattr(a, "targets", []) + ) + ): + out = out | self.resolve(m, a.value) + else: + out = out | Res(unread=[self.at(m, e)]) + return out + fn = self.enclosing(m, fn, (ast.FunctionDef, ast.AsyncFunctionDef)) + found = self.lookup(m, e.id) + if ( + found + and isinstance(found[0], Module) + and isinstance(found[1], (ast.Assign, ast.AnnAssign)) + ): + stmt = found[1] + if stmt.value is not None and isinstance(stmt.value, ast.Constant): + return self.resolve(found[0], stmt.value) + return Res(unread=[self.at(m, e)]) + + def assignments(self, fn: Node, name: str) -> list[Node]: + """The statements that bind `name` in this function's own body (not in a nested one).""" + out: list[Node] = [] + stack = list(ast.iter_child_nodes(fn)) + while stack: + node = stack.pop() + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda, ast.ClassDef)): + continue + if isinstance(node, ast.Assign): + if any( + name in {n.id for n in ast.walk(t) if isinstance(n, ast.Name)} + for t in node.targets + ): + out.append(node) + elif isinstance(node, ast.AnnAssign) and getattr(node.target, "id", None) == name: + out.append(node) + elif isinstance( + node, (ast.For, ast.With, ast.AugAssign, ast.NamedExpr, ast.comprehension) + ): + targets = [getattr(node, "target", None)] + [ + i.optional_vars + for i in getattr(node, "items", []) + if i.optional_vars is not None + ] + if any( + t is not None and name in {n.id for n in ast.walk(t) if isinstance(n, ast.Name)} + for t in targets + ): + out.append( + node + ) # bound by a loop, a `with`, an augmented or a walrus: not followed + stack.extend(ast.iter_child_nodes(node)) + return out + + def caught_as(self, m: Module, e: Node, name: str) -> ast.ExceptHandler | None: + node = m.parents.get(e) + while node is not None and not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + if isinstance(node, ast.ExceptHandler) and node.name == name: + return node + node = m.parents.get(node) + return None + + def resolve_param(self, m: Module, fn: Node, param: ast.arg, e: Node) -> Res: + v = self.ann_values(m, param.annotation) + if v is not None: + return Res(v) + if param.annotation is not None and getattr(param.annotation, "id", None) != "str": + return Res(unread=[self.at(m, e)]) + return self.follow(m, fn, param) + + def follow(self, m: Module, fn: Node, param: ast.arg) -> Res: + """A `str` parameter of a helper: what every call of the helper passes it.""" + assert isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)) + key = (m.name, fn.name, param.arg) + if key in self._following: + return Res() + if m.top.get(fn.name) is not fn: + return Res( + unread=[ + f"{m.path}:{fn.lineno}: {fn.name}({param.arg}) is not a module-level helper" + ] + ) + self._following.add(key) + positional = [a.arg for a in [*fn.args.posonlyargs, *fn.args.args]] + defaults = dict( + zip( + positional[len(positional) - len(fn.args.defaults) :], fn.args.defaults, strict=True + ) + ) + defaults |= { + a.arg: d + for a, d in zip(fn.args.kwonlyargs, fn.args.kw_defaults, strict=True) + if d is not None + } + out = Res() + for cm in self.mods.values(): + for node in ast.walk(cm.tree): + if not isinstance(node, (ast.Name, ast.Attribute)) or isinstance( + cm.parents.get(node), ast.Attribute + ): + continue + found = self.lookup_expr(cm, node) + if not (found and found[0] is m and found[1] is fn): + continue + call = cm.parents.get(node) + if not (isinstance(call, ast.Call) and call.func is node): + out = out | Res(unread=[f"{self.at(cm, node)} (the helper used as a value)"]) + continue + kw = {k.arg: k.value for k in call.keywords} + given = param.arg in positional and positional.index(param.arg) < len(call.args) + if param.arg in kw: + out = out | self.resolve(cm, kw[param.arg]) + elif any(isinstance(a, ast.Starred) for a in call.args): + out = out | Res(unread=[self.at(cm, call)]) + elif given: # given by position, a `**` spread cannot pass it again (a TypeError) + out = out | self.resolve(cm, call.args[positional.index(param.arg)]) + elif None in kw: + out = out | Res(unread=[self.at(cm, call)]) + elif param.arg in defaults: + out = out | self.resolve(m, defaults[param.arg]) + return out + + def resolve_attr(self, m: Module, e: ast.Attribute) -> Res: + base = e.value + if isinstance(base, ast.Name): + handler = self.caught_as(m, e, base.id) + fn = self.enclosing(m, e, (ast.FunctionDef, ast.AsyncFunctionDef)) + typed = None + if handler is not None: + typed = self.classes(m, handler.type) + elif fn is not None: + params = { + a.arg: a for a in [*fn.args.posonlyargs, *fn.args.args, *fn.args.kwonlyargs] + } + if base.id in params: + typed = self.classes(m, params[base.id].annotation) + if typed is not None: + out = Res() + for cm, cls in typed: + v = self.attr_values(cm, cls, e.attr) + out = out | ( + Res(v) + if v is not None + else Res( + unread=[f"{self.at(m, e)} ({cls.name}.{e.attr} has no Literal type)"] + ) + ) + return out + found = self.lookup_expr(m, e) + if ( + found + and isinstance(found[0], Module) + and isinstance(found[1], (ast.Assign, ast.AnnAssign)) + ): + stmt = found[1] + if isinstance(stmt.value, ast.Constant): + return self.resolve(found[0], stmt.value) + return Res(unread=[self.at(m, e)]) + + def resolve_lookup(self, m: Module, e: ast.Subscript) -> Res: + found = self.lookup_expr(m, e.value) + if not ( + found + and isinstance(found[0], Module) + and isinstance(found[1], (ast.Assign, ast.AnnAssign)) + ): + return Res(unread=[self.at(m, e)]) + table = found[1].value + if not isinstance(table, ast.Dict) or not all( + isinstance(k, ast.Constant) and isinstance(k.value, str) for k in table.keys + ): + return Res(unread=[self.at(m, e)]) + out = Res() + for v in table.values: + out = out | self.resolve(found[0], v) + index = self.resolve(m, e.slice) + keys = {k.value for k in table.keys} + if index.unread or index.values != keys: + self.errors.append( + f"{self.at(m, e)}: the map's keys {sorted(keys)} are not what its index can be " + f"{sorted(index.values)}{' + unread ' + str(index.unread) if index.unread else ''}" + ) + return out + + # --- sites --------------------------------------------------------------------------------------- + + def spread_is_safe(self, m: Module, e: Node, src: Node) -> bool: + """A `**x` in a detail, or `detail |= {…}` from it, cannot carry a reason when `x` is the `**kwargs` + of a function that takes `reason` by name: Python binds that one to the parameter.""" + if isinstance(src, ast.Attribute): + return self.spread_is_safe_attr(m, e, src) + fn = self.enclosing(m, e, (ast.FunctionDef, ast.AsyncFunctionDef)) + if fn is None or fn.args.kwarg is None: + return False + names = {a.arg for a in [*fn.args.posonlyargs, *fn.args.args, *fn.args.kwonlyargs]} + kwarg = fn.args.kwarg.arg + if isinstance(src, ast.Name): + return src.id == kwarg and "reason" in names + + if isinstance(src, ast.DictComp): + it = src.generators[0].iter + return ( + isinstance(it, ast.Call) + and isinstance(it.func, ast.Attribute) + and it.func.attr == "items" + and getattr(it.func.value, "id", None) == kwarg + and "reason" in names + ) + return False + + def spread_is_safe_attr(self, m: Module, e: Node, src: ast.Attribute) -> bool: + """`e.params` of a caught exception whose `__init__(self, reason, **params)` keeps `self.params = params`.""" + if not isinstance(src.value, ast.Name): + return False + handler = self.caught_as(m, e, src.value.id) + typed = self.classes(m, handler.type) if handler else None + return bool(typed) and all(self.keeps_kwargs(cls, src.attr) for _, cls in typed or []) + + def keeps_kwargs(self, cls: ast.ClassDef, attr: str) -> bool: + init = next( + (s for s in cls.body if isinstance(s, ast.FunctionDef) and s.name == "__init__"), None + ) + if init is None or init.args.kwarg is None: + return False + names = {a.arg for a in [*init.args.posonlyargs, *init.args.args, *init.args.kwonlyargs]} + kwarg = init.args.kwarg.arg + return "reason" in names and any( + isinstance(n, ast.Assign) + and any( + isinstance(t, ast.Attribute) + and t.attr == attr + and getattr(t.value, "id", None) == "self" + for t in n.targets + ) + and getattr(n.value, "id", None) == kwarg + for n in ast.walk(init) + ) + + def entries(self, d: Node) -> list[tuple[str | None, Node]]: + if isinstance(d, ast.Dict): + return [ + (k.value if isinstance(k, ast.Constant) else None if k is None else "?", v) + for k, v in zip(d.keys, d.values, strict=True) + ] + assert isinstance(d, ast.Call) + return [(k.arg, k.value) for k in d.keywords] + + def site(self, m: Module, d: Node, out: dict) -> None: + items = self.entries(d) + code_expr = next(v for k, v in items if k == "code") + code = self.resolve(m, code_expr) + if code.unread or len(code.values) != 1: + out["unreadCodes"].append(self.at(m, code_expr)) + return + entry = out["codes"].setdefault( + next(iter(code.values)), {"reasons": set(), "unread": [], "sites": 0, "reasonless": []} + ) + entry["sites"] += 1 + res, has_reason = Res(), False + for k, v in items: + if k == "reason": + res, has_reason = res | self.resolve(m, v), True + elif k is None and not self.spread_is_safe(m, d, v): + res = res | Res(unread=[f"{self.at(m, v)} (a spread that may carry a reason)"]) + res, more = self.mutations(m, d, res) + has_reason = has_reason or more + if not has_reason: + entry["reasonless"].append(f"{m.path}:{d.lineno}") + entry["reasons"] |= res.values + entry["unread"] += res.unread + + def mutations(self, m: Module, d: Node, res: Res) -> tuple[Res, bool]: + """`detail |= {…}`, `detail["reason"] = …`, `detail.update(…)` on the variable a detail is put in.""" + stmt = m.parents.get(d) + if not (isinstance(stmt, (ast.Assign, ast.AnnAssign)) and stmt.value is d): + return res, False + target = stmt.targets[0] if isinstance(stmt, ast.Assign) else stmt.target + fn = self.enclosing(m, d, (ast.FunctionDef, ast.AsyncFunctionDef)) + if not isinstance(target, ast.Name) or fn is None: + return res, False + found = False + for node in ast.walk(fn): + if isinstance(node, ast.AugAssign) and getattr(node.target, "id", None) == target.id: + v = node.value + if isinstance(v, ast.Dict): + for k, val in self.entries(v): + if k == "reason": + res, found = res | self.resolve(m, val), True + elif k is None and not self.spread_is_safe(m, node, val): + res = res | Res( + unread=[f"{self.at(m, val)} (a spread that may carry a reason)"] + ) + elif not self.spread_is_safe(m, node, v): + res = res | Res(unread=[f"{self.at(m, node)} (may carry a reason)"]) + elif ( + isinstance(node, ast.Subscript) + and getattr(node.value, "id", None) == target.id + and isinstance(node.ctx, ast.Store) + ): + if isinstance(node.slice, ast.Constant) and node.slice.value != "reason": + continue + assign = m.parents.get(node) + if isinstance(node.slice, ast.Constant) and isinstance(assign, ast.Assign): + res, found = res | self.resolve(m, assign.value), True + else: + res = res | Res(unread=[f"{self.at(m, node)} (may carry a reason)"]) + elif ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and getattr(node.func.value, "id", None) == target.id + and node.func.attr in {"update", "setdefault"} + ): + res = res | Res(unread=[f"{self.at(m, node)} (may carry a reason)"]) + return res, found + + def run(self) -> dict: + out: dict = { + "codes": {}, + "unreadCodes": [], + "errors": self.errors, + "excluded": {}, + "aliases": {}, + "notRefusals": sorted(NOT_REFUSALS), + } + for m in self.mods.values(): + for name, stmt in m.top.items(): + if ( + isinstance(stmt, ast.Assign) + and isinstance(stmt.value, ast.Subscript) + and (v := self.ann_values(m, stmt.value)) + ): + out["aliases"].setdefault(m.path, {})[name] = sorted(v) + for node in ast.walk(m.tree): + is_dict = isinstance(node, ast.Dict) and any( + isinstance(k, ast.Constant) and k.value == "code" for k in node.keys + ) + is_call = ( + isinstance(node, ast.Call) + and getattr(node.func, "id", None) == "dict" + and any(k.arg == "code" for k in node.keywords) + ) + if not (is_dict or is_call): + continue + if m.path in NOT_REFUSALS: + out["excluded"][m.path] = out["excluded"].get(m.path, 0) + 1 + continue + self.site(m, node, out) + for entry in out["codes"].values(): + entry["reasons"] = sorted(entry["reasons"]) + return out + + +def scan(sources: dict[str, str]) -> dict: + return Scan(sources).run() + + +def scan_app(app: pathlib.Path = APP) -> dict: + return scan( + { + p.relative_to(app).as_posix(): p.read_text(encoding="utf-8") + for p in sorted(app.rglob("*.py")) + if "__pycache__" not in p.parts + } + ) + + +if __name__ == "__main__": + json.dump( + scan_app(pathlib.Path(sys.argv[1]) if len(sys.argv) > 1 else APP), + sys.stdout, + indent=1, + sort_keys=True, + ) diff --git a/backend/tests/test_refusal_sites.py b/backend/tests/test_refusal_sites.py new file mode 100644 index 00000000..9c6effbb --- /dev/null +++ b/backend/tests/test_refusal_sites.py @@ -0,0 +1,278 @@ +"""The refusal scan (`refusal_sites.py`) on small sources: every shape a reason travels in is followed to +its values or reported unread. A scan that silently drops a shape is a guard green over nothing, so each +shape that once went past the regex it replaced is here. Pure: no app import, no database.""" + +import textwrap + +from tests.refusal_sites import scan, scan_app + + +def _scan(**files: str) -> dict: + return scan( + {f"{name.replace('__', '/')}.py": textwrap.dedent(src) for name, src in files.items()} + ) + + +def _code(result: dict, code: str) -> dict: + return result["codes"][code] + + +REFUSAL = """ + from fastapi import HTTPException + + def refused(status: int, reason: str, **params: object) -> HTTPException: + return HTTPException(status_code=status, detail={"code": "refused", "reason": reason, **params}) +""" + + +def test_a_literal_wherever_it_sits_in_the_dict(): + r = _scan( + a=""" + def f(x): + raise HTTPException(409, {"code": "clip", "limit_s": 5, "reason": "too_long"}) + def g(x): + raise HTTPException(409, {"reason": "before", "code": "clip"}) + def h(x): + raise HTTPException(409, { + "code": "clip", # a comment between + "reason": "after_comment", + }) + def i(x): + raise HTTPException(409, detail=dict(code="clip", reason="by_call")) + """ + ) + assert _code(r, "clip")["reasons"] == ["after_comment", "before", "by_call", "too_long"] + assert _code(r, "clip")["unread"] == [] + + +def test_every_arm_of_a_conditional_and_every_operand_of_or(): + r = _scan( + a=""" + def f(x, e): + raise HTTPException(409, {"code": "edit", "reason": "a" if x else "b"}) + def g(x): + raise HTTPException(409, {"code": "edit", "reason": x.why or "c"}) + """ + ) + assert _code(r, "edit")["reasons"] == ["a", "b", "c"] + assert [u.split(": ", 1)[1] for u in _code(r, "edit")["unread"]] == ["x.why"] + + +def test_a_helper_is_followed_to_every_call_however_it_is_written(): + r = _scan( + refusal=REFUSAL, + routes__a=""" + from http import HTTPStatus + from app import refusal + from app.refusal import refused + + def f(status): + raise refused(status, "by_name_status") + def g(): + raise refused(HTTPStatus.CONFLICT, reason="by_keyword") + def h(): + raise refusal.refused(409, "qualified") + def i(x): + raise refused(409, "a" if x else "second_arm") + """, + ) + assert _code(r, "refused")["reasons"] == [ + "a", + "by_keyword", + "by_name_status", + "qualified", + "second_arm", + ] + assert _code(r, "refused")["unread"] == [] + + +def test_a_helper_passed_as_a_value_or_spread_into_is_unread(): + r = _scan( + refusal=REFUSAL, + routes__a=""" + from app.refusal import refused + + def f(args): + pick = refused + raise refused(*args) + """, + ) + unread = _code(r, "refused")["unread"] + assert any("the helper used as a value" in u for u in unread) + assert any("refused(*args)" in u for u in unread) + + +def test_a_default_is_what_a_call_that_omits_it_sends(): + r = _scan( + routes___common=""" + def not_found(what: str = "generic"): + return HTTPException(404, {"code": "not_found", "reason": what}) + + def get_or_404(db, id_, *, what: str = "generic"): + raise not_found(what) + """, + routes__a=""" + from app.routes._common import get_or_404, not_found + + def f(db): + get_or_404(db, 1, what="user") + get_or_404(db, 2) + raise not_found("debug_session") + """, + ) + assert _code(r, "not_found")["reasons"] == ["debug_session", "generic", "user"] + + +def test_a_caught_exception_answers_with_its_attribute_type_every_class_it_catches(): + r = _scan( + errors=""" + from typing import Literal + ClipReason = Literal["bad_range", "too_short"] + EditReason = Literal["not_media"] + + class ClipError(Exception): + def __init__(self, reason: ClipReason, **params: float): + self.reason: ClipReason = reason + self.params = params + + class EditError(Exception): + def __init__(self, reason: EditReason): + self.reason: EditReason = reason + + class Untyped(Exception): + def __init__(self, reason: str): + self.reason = reason + + class Narrower(ClipError): + pass + """, + routes__a=""" + from app import errors + + def f(): + try: + pass + except (errors.ClipError, errors.EditError) as e: + raise HTTPException(422, {"code": "clip", "reason": e.reason, **e.params}) + except errors.Untyped as e: + raise HTTPException(422, {"code": "clip", "reason": e.reason}) + except errors.Narrower as e: + raise HTTPException(422, {"code": "edit", "reason": e.reason}) + """, + ) + assert _code(r, "clip")["reasons"] == ["bad_range", "not_media", "too_short"] + unread = _code(r, "clip")["unread"] + assert any("Untyped.reason has no Literal type" in u for u in unread) + # EditError keeps no `**params`, so its `e.params` could be anything + assert any("e.params" in u for u in unread) + assert _code(r, "edit")["reasons"] == ["bad_range", "too_short"] + + +def test_a_typed_parameter_is_its_literal_and_an_untyped_one_is_unread_when_never_called(): + r = _scan( + a=""" + from typing import Literal + Why = Literal["one", "two"] + + def typed(reason: Why): + return HTTPException(409, {"code": "playlist", "reason": reason}) + + class C: + def method(self, reason: str): + return HTTPException(409, {"code": "playlist", "reason": reason}) + """ + ) + assert _code(r, "playlist")["reasons"] == ["one", "two"] + assert any("is not a module-level helper" in u for u in _code(r, "playlist")["unread"]) + + +def test_a_value_the_scan_cannot_follow_is_unread(): + r = _scan( + a=""" + def f(svc): + hit, code = svc.fetch() + raise HTTPException(422, {"code": "subtitle", "reason": code}) + def g(svc): + raise HTTPException(422, {"code": "subtitle", "reason": svc.reason()}) + def h(e): + raise HTTPException(422, {"code": "subtitle", "reason": e.reason}) + """ + ) + assert len(_code(r, "subtitle")["unread"]) == 3 + assert _code(r, "subtitle")["reasons"] == [] + + +def test_a_map_lookup_is_its_values_and_its_keys_must_be_what_the_index_can_be(): + src = """ + from typing import Literal + SettingReason = Literal["too_low", "too_high"] + + class InvalidSetting(ValueError): + def __init__(self, key: str, reason: SettingReason, **params: object): + self.reason: SettingReason = reason + self.params = params + + _MAP: dict[SettingReason, str] = {{{keys}}} + + def refused(status: int, reason: str, **params: object): + return HTTPException(status, {{"code": "refused", "reason": reason, **params}}) + + def setting_refused(exc: InvalidSetting): + return refused(400, _MAP[exc.reason], **exc.params) + """ + whole = _scan(a=src.format(keys='"too_low": "setting_too_low", "too_high": "setting_too_high"')) + assert _code(whole, "refused")["reasons"] == ["setting_too_high", "setting_too_low"] + assert whole["errors"] == [] + short = _scan(a=src.format(keys='"too_low": "setting_too_low"')) + assert len(short["errors"]) == 1 and "not what its index can be" in short["errors"][0] + + +def test_a_detail_built_up_in_steps_is_read_through_its_variable(): + r = _scan( + a=""" + def f(status: int, reason: str, user=None, **extra): + detail: dict = {"code": "login", "reason": "invalid"} + if user is not None: + detail |= {"email": user.email, "reason": "suspended"} + detail |= {k: v for k, v in extra.items() if v is not None} + return HTTPException(status, detail) + + def g(more): + detail = {"code": "login"} + detail["reason"] = "late" + detail.update(more) + detail |= more + return HTTPException(401, detail) + """ + ) + entry = _code(r, "login") + assert entry["reasons"] == ["invalid", "late", "suspended"] + assert sum("may carry a reason" in u for u in entry["unread"]) == 2 + + +def test_a_code_must_be_one_the_scan_can_read(): + r = _scan( + a=""" + BUSY = "busy" + def f(code): + raise HTTPException(409, {"code": BUSY}) + def g(code): + raise HTTPException(409, {"code": code}) + """ + ) + assert "busy" in r["codes"] and r["codes"]["busy"]["reasonless"] + assert len(r["unreadCodes"]) == 1 and r["unreadCodes"][0].endswith(": code") + + +def test_the_data_files_are_excluded_by_name_only(): + r = _scan(plex__tmdb='x = {"code": "HU", "name": "Hungary"}', plex__other='y = {"code": "HU"}') + assert r["excluded"] == {"plex/tmdb.py": 1} + assert "HU" in r["codes"] + + +def test_the_app_scans(): + """The real tree, on the interpreter the backend runs: every refusal it holds is read (the + frontend's contract judges what is read).""" + result = scan_app() + assert {"refused", "not_found", "login", "clip"} <= set(result["codes"]) + assert result["errors"] == [] diff --git a/docs/development.md b/docs/development.md index 465b327c..dcbe382d 100644 --- a/docs/development.md +++ b/docs/development.md @@ -144,7 +144,8 @@ npm run typecheck # tsc x3 (app / node / e2e) npm run lint # eslint (typed rules included; warnings fail) npm run format:check # prettier npm run knip # unused files/exports/deps -npm run test # vitest (incl. the compiler and gate-register contracts) +npm run test # vitest (incl. the compiler and gate-register contracts; the refusal contract runs + # the host's `python` on backend/tests/refusal_sites.py, stdlib only) npx vite build # the production bundle (devOnlyGuard fails it on dev-only code, chunkGuard on a lazy page loaded at boot) npm audit --omit=dev --audit-level=high # what ships to the browser, at high or above ``` diff --git a/frontend/src/i18n/backendIds.contract.test.ts b/frontend/src/i18n/backendIds.contract.test.ts index 524ed7db..436d5ce6 100644 --- a/frontend/src/i18n/backendIds.contract.test.ts +++ b/frontend/src/i18n/backendIds.contract.test.ts @@ -1,4 +1,6 @@ +import { execFileSync } from "node:child_process"; import { readdirSync, readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest"; import { RUN_STATUSES } from "../lib/activityUrlState"; @@ -27,6 +29,25 @@ import { vocabKey } from "../lib/vocab"; const backend = (file: string) => readFileSync(new URL(`../../../backend/app/${file}`, import.meta.url), "utf-8"); +// Every refusal the backend can answer, read from its syntax tree by `backend/tests/refusal_sites.py` +// (stdlib `ast`, on the host's Python, as the ruff lane is): each code with every reason it can be sent +// with — a reason in a variable followed to its `Literal` type, its helper's every call, its map — and +// what it could not follow, which fails below. It also hands back every `Literal` alias it read. +type RefusalScan = { + codes: Record; + unreadCodes: string[]; + errors: string[]; + notRefusals: string[]; + excluded: Record; + aliases: Record>; +}; +const scan = JSON.parse( + execFileSync("python", ["-I", "backend/tests/refusal_sites.py"], { + cwd: fileURLToPath(new URL("../../../", import.meta.url)), + encoding: "utf-8", + }), +) as RefusalScan; + const mods = import.meta.glob("./locales/*/*.json", { eager: true }) as Record< string, { default: Record } @@ -54,12 +75,11 @@ const keysOf = (lang: string): Set => { const specs = [ ...backend("sysconfig.py").matchAll(/ConfigSpec\(\s*"(\w+)",\s*"(\w+)",\s*"(\w+)",\s*"(\w+)"/g), ].map((m) => ({ key: m[1]!, type: m[2]!, group: m[3]!, attr: m[4]! })); -// A choice key's options, as config.py spells them: `: = …` → ` = Literal[…]`. +// A choice key's options, as config.py spells them: `: = …`, the alias's values from the scan. const configPy = backend("config.py"); const choicesOf = (attr: string): string[] => { const alias = configPy.match(new RegExp(`^\\s+${attr}: (\\w+) =`, "m"))?.[1]; - const body = alias && configPy.match(new RegExp(`^${alias} = Literal\\[([^\\]]*)\\]`, "m"))?.[1]; - return [...(body ?? "").matchAll(/"(\w+)"/g)].map((m) => m[1]!); + return (alias && scan.aliases["config.py"]?.[alias]) || []; }; const choiceIds = specs .filter((s) => s.type === "choice") @@ -90,16 +110,10 @@ const runEnum = (name: string) => const runStatuses = runEnum("RunStatus"); const runTriggers = runEnum("RunTrigger"); const quotaActions = enumValues("quota.py"); -// The admin actions' refusal reasons → `errors.admin.`. The codes are the refusal records -// under that prefix (lib/api/refusalCodes), so a code recorded there is scanned here too; a detail -// dict ruff wraps over several lines still matches. +// The admin actions' refusals → `errors.admin.`: the codes recorded under that prefix. const adminCodes = Object.entries(REFUSAL_FAMILIES) .filter(([, f]) => f.prefix === "errors.admin") .map(([code]) => code); -const adminRefusals = [ - ...backend("routes/admin.py").matchAll(/\{\s*"code":\s*"(\w+)",\s*"reason":\s*"(\w+)"/g), -].filter((m) => adminCodes.includes(m[1]!)); -const adminReasons = [...new Set(adminRefusals.map((m) => m[2]!))]; // The download failure catalog's host-specific entries → `downloads.errors.fail.catalog.`. // A code holds a dot (`facebook.login_wall`), so the reverse check strips the prefix and the part // rather than splitting on dots. @@ -120,66 +134,6 @@ const failCategories = [ ]), ]; -// What a structured 404 says is gone → `errors.notFound.`: every `not_found("x")` and every -// `owned_or_404(..., what="x")` / `get_or_404(..., what="x")` under routes/, read from the sources. -const routeFiles = (dir = "routes"): string[] => - readdirSync(new URL(`../../../backend/app/${dir}/`, import.meta.url), { - withFileTypes: true, - }).flatMap((e) => - e.isDirectory() - ? routeFiles(`${dir}/${e.name}`) - : e.name.endsWith(".py") - ? [`${dir}/${e.name}`] - : [], - ); -const routeSources = (): string[] => routeFiles().map(backend); -// The words a job's result gives for not doing its work → `scheduler.skip.` (the Scheduler -// page, the run notice and the audit row name them; lib/jobResult). Read as the `"skipped": "…"` / -// `"reason": "…"` literals of the non-HTTP backend: under routes/ (and main.py) a `reason` is a refusal -// code, which `errors.*` names. A word from an exception's text is not a literal and is not covered. -const appFiles = (dir: string): string[] => - readdirSync(new URL(`../../../backend/app/${dir}`, import.meta.url), { - withFileTypes: true, - }).flatMap((e) => - e.isDirectory() - ? e.name === "routes" || e.name === "__pycache__" - ? [] - : appFiles(`${dir}${e.name}/`) - : e.name.endsWith(".py") && `${dir}${e.name}` !== "main.py" - ? [`${dir}${e.name}`] - : [], - ); -const skipWords = [ - ...new Set( - appFiles("").flatMap((f) => - [...backend(f).matchAll(/"(?:skipped|reason)": "([^"]+)"/g)].map((m) => m[1]!), - ), - ), -]; -// A target the scan cannot read — `what=SOME_CONSTANT` — is a 404 no locale is checked for (one hid -// `debug_session`). Only the helpers' own pass-through in `_common.py` may name it by variable. -const opaqueWhats = routeFiles() - .filter((f) => !f.endsWith("/_common.py")) - .flatMap((f) => - [...backend(f).matchAll(/not_found\((?!"|\))[^)]*\)|what=(?!")\S+/g)].map( - (m) => `${f}: ${m[0]}`, - ), - ); -const goneWhats = [ - ...new Set( - routeSources().flatMap((src) => - [...src.matchAll(/not_found\("(\w+)"\)|what="(\w+)"/g)].map((m) => (m[1] ?? m[2])!), - ), - ), -]; -// A backend `Name = Literal["a", "b"]` alias, by name: the reasons a typed parameter accepts, which -// basedpyright then holds at every call site — a reason passed by variable included. -const literalOf = (file: string, name: string) => - [ - ...( - backend(file).match(new RegExp(`^${name} = Literal\\[([^\\]]*)\\]`, "m"))?.[1] ?? "" - ).matchAll(/"(\w+)"/g), - ].map((m) => m[1]!); // Every backend source, by its path under app/. const appPaths = (dir = ""): string[] => readdirSync(new URL(`../../../backend/app/${dir}`, import.meta.url), { @@ -193,114 +147,32 @@ const appPaths = (dir = ""): string[] => ? [`${dir}${e.name}`] : [], ); -const appSources = (): string[] => appPaths().map(backend); -// Every generic refusal → `errors.refused.`: each `refused(, "")` anywhere in -// the backend (not `_refused`, a router's own family), the values of the settings map, which -// `setting_refused` picks from at runtime, and the folder rules' `FolderReason`, answered with -// `refused(409, e.reason)`. -const refusedReasons = [ - ...new Set([ - ...appSources().flatMap((src) => - [...src.matchAll(/(? m[1]!), - ), - ...literalOf("downloads/folders.py", "FolderReason"), - ...[ - ...(backend("refusal.py").split("_SETTING_REASONS = {")[1]?.split("}")[0] ?? "").matchAll( - /:\s*"(\w+)"/g, - ), - ].map((m) => m[1]!), - ]), -]; -// A `refused(…)` whose reason the scan above cannot read. Only two may: a folder rule's `e.reason` -// (the nearest `except` above it catches `FolderError`, typed `FolderReason`) and the settings map. -const opaqueRefused = appPaths().flatMap((f) => { - const src = backend(f); - return [...src.matchAll(/(? { - const arg = m[1]!.trim(); - if (f === "refusal.py" && arg.startsWith("_SETTING_REASONS[")) return false; - const caught = [...src.slice(0, m.index).matchAll(/except ([^\n:]*):/g)].pop()?.[1] ?? ""; - return !(arg === "e.reason" && /FolderError as e$/.test(caught)); - }) - .map((m) => `${f}: ${m[0]}`); -}); -const youtubeReasons = literalOf("youtube/client.py", "YouTubeRefusal"); -const clipReasons = literalOf("downloads/clip.py", "ClipReason"); - -// Every `"code": …` a backend dict carries, with the `"reason": …` that follows it. Two modules use a -// `code` key for something that is not a refusal; everything else is one. -const NOT_REFUSALS: Record = { - "plex/subtitles.py": - "the player's subtitle status vocabulary (`/status`), read by the player itself", - "plex/tmdb.py": "a production country's ISO code", -}; -const detailSites = appPaths() - .filter((f) => !(f in NOT_REFUSALS)) - .flatMap((f) => - [ - ...backend(f).matchAll( - /"code":\s*(?:"(\w+)"|([^\s,}]+))(?:,\s*"reason":\s*(?:"(\w+)"|([^\s,}]+)))?/g, - ), - ].map((m) => ({ f, code: m[1], codeExpr: m[2], reason: m[3], reasonExpr: m[4] })), - ); -const backendCodes = [...new Set(detailSites.flatMap((s) => (s.code ? [s.code] : [])))]; -// A code the scan cannot read (`"code": SOME_CONSTANT`) is a refusal no record is checked for. -const opaqueCodes = detailSites.filter((s) => !s.code).map((s) => `${s.f}: "code": ${s.codeExpr}`); -// A reason passed by variable, each pinned to what holds it, so a new one has to say how it is held. -const OPAQUE_REASONS: Record = { - "refusal.py: refused ← reason": "a literal at each refused() call, else opaqueRefused", - "routes/auth.py: login ← reason": "_login_refused's LoginRefusal", - "routes/downloads.py: url_blocked ← urlguard.REASON_MALFORMED": "UrlBlockedReason", - "routes/downloads.py: url_blocked ← e.reason": "BlockedUrl's UrlBlockedReason", - "routes/downloads.py: cookies ← e.reason": "InvalidCookies' CookieReason", - "routes/downloads.py: edit ← e.reason": "EditError's EditReason", - "routes/downloads.py: clip ← e.reason": "ClipError's ClipReason", - "routes/files.py: quota ← e.reason": "QuotaExceeded's QuotaReason", - "routes/playlists.py: playlist ← reason": "_refused's PlaylistRefusal", - "routes/playlists.py: playlist_move ← e.reason": "MoveRefused's MoveReason", - "routes/plex/playback.py: subtitle ← code": "REFUSAL_UNSHOWN: no screen reads it", - "routes/_common.py: not_found ← what": "opaqueWhats", - "routes/_common.py: youtube ← reason": "YouTubeRefusal", -}; -const opaqueReasons = [ +// The words a job's result gives for not doing its work → `scheduler.skip.` (the Scheduler +// page, the run notice and the audit row name them; lib/jobResult). Read as the `"skipped": "…"` / +// `"reason": "…"` literals of the non-HTTP backend: under routes/ (and main.py) a `reason` is a refusal +// code, which `errors.*` names. A word from an exception's text is not a literal and is not covered. +const skipWords = [ ...new Set( - detailSites - .filter((s) => s.code && s.reasonExpr) - .map((s) => `${s.f}: ${s.code} ← ${s.reasonExpr}`), + appPaths() + .filter((f) => !f.startsWith("routes/") && f !== "main.py") + .flatMap((f) => + [...backend(f).matchAll(/"(?:skipped|reason)": "([^"]+)"/g)].map((m) => m[1]!), + ), ), ]; -// A reason written as a literal in a detail dict, wherever that dict is built. -const literalReasons = (code: string) => - detailSites.filter((s) => s.code === code && s.reason).map((s) => s.reason!); -// What each refusal code can carry as its reason: the Literal its typed parameter accepts, or the -// call's own literal — and every literal any detail dict anywhere gives it. -const reasonsOf: Record = Object.fromEntries( - Object.entries({ - refused: refusedReasons, - youtube: youtubeReasons, - not_found: goneWhats, - clip: clipReasons, - ...Object.fromEntries(adminCodes.map((c) => [c, []])), - playlist: literalOf("routes/playlists.py", "PlaylistRefusal"), - playlist_move: literalOf("reader/service.py", "MoveReason"), - edit: literalOf("downloads/service.py", "EditReason"), - cookies: literalOf("downloads/credentials.py", "CookieReason"), - url_blocked: literalOf("downloads/urlguard.py", "UrlBlockedReason"), - login: literalOf("routes/auth.py", "LoginRefusal"), - quota: literalOf("downloads/quota.py", "QuotaReason"), - reset: [], - collection: [], - } as Record).map(([code, rs]) => [ - code, - [...new Set([...rs, ...literalReasons(code)])], - ]), -); + +const backendCodes = Object.keys(scan.codes); const recordedCodes = [ ...Object.keys(REFUSAL_FAMILIES), ...Object.keys(REFUSAL_BY_REASON), ...Object.keys(REFUSAL_FIXED), ...Object.keys(REFUSAL_UNSHOWN), ]; +// The codes whose reason picks the sentence, and what each can carry as its reason. +const reasoned = [...Object.keys(REFUSAL_FAMILIES), ...Object.keys(REFUSAL_BY_REASON)]; +const reasonsOf: Record = Object.fromEntries( + reasoned.map((code) => [code, scan.codes[code]?.reasons ?? []]), +); // The closed English vocabularies the backend WRITES into data, named through `vocab.` // (lib/vocab) by their letters-and-digits key. A word the backend adds shows in English until named — @@ -363,10 +235,11 @@ describe("backend ids have their names in both languages", () => { expect(runTriggers).toEqual(expect.arrayContaining(["scheduled", "ad_hoc"])); expect(adminCodes).toEqual(expect.arrayContaining(["activate", "resend"])); expect(failDetails).toEqual(expect.arrayContaining(["facebook.login_wall"])); - expect(goneWhats).toEqual(expect.arrayContaining(["playlist", "download", "job"])); - expect(opaqueWhats).toEqual([]); - expect(youtubeReasons).toEqual(["quota", "reconnect", "unavailable", "gone"]); - expect(clipReasons).toEqual(expect.arrayContaining(["too_long", "past_end", "bad_crop"])); + expect(reasonsOf.not_found).toEqual( + expect.arrayContaining(["playlist", "download", "job", "user", "debug_session"]), + ); + expect(reasonsOf.youtube).toEqual(["gone", "quota", "reconnect", "unavailable"]); + expect(reasonsOf.clip).toEqual(expect.arrayContaining(["too_long", "past_end", "bad_crop"])); expect(crewJobs).toEqual( expect.arrayContaining(["Director", "Creator", "Director of Photography"]), ); @@ -382,11 +255,14 @@ describe("backend ids have their names in both languages", () => { "no service user", ]), ); - expect(refusedReasons).toEqual( - expect.arrayContaining(["weak_password", "setting_too_low", "plex_write_failed"]), + expect(reasonsOf.refused).toEqual( + expect.arrayContaining([ + "weak_password", + "setting_too_low", + "plex_write_failed", + "file_folder_gone", + ]), ); - // Every code the frontend localizes is raised somewhere the scan can see. - expect(adminCodes.filter((c) => !adminRefusals.some((m) => m[1] === c))).toEqual([]); }); it("every refusal code the backend answers is said by exactly one record, and no record names another", () => { @@ -397,12 +273,19 @@ describe("backend ids have their names in both languages", () => { expect(recordedCodes.filter((c) => !backendCodes.includes(c))).toEqual([]); }); - it("every refusal code and reason is one the scans can read, or says what holds it", () => { - // The modules set aside as not refusals still exist and still use the key. - for (const f of Object.keys(NOT_REFUSALS)) expect(backend(f), f).toContain('"code":'); - expect(opaqueCodes).toEqual([]); - expect(opaqueRefused).toEqual([]); - expect(opaqueReasons.sort()).toEqual(Object.keys(OPAQUE_REASONS).sort()); + it("every refusal code and reason is one the scan can follow", () => { + // The modules set aside as not refusals still use the key. + expect(Object.keys(scan.excluded).sort()).toEqual(scan.notRefusals); + expect(scan.unreadCodes).toEqual([]); + expect(scan.errors).toEqual([]); + // A reason it could not follow is a sentence nobody is asked for, unless no screen says that code. + expect( + Object.entries(scan.codes) + .filter(([code]) => !(code in REFUSAL_UNSHOWN)) + .flatMap(([, c]) => c.unread), + ).toEqual([]); + // A code whose reason picks the sentence, sent without one, says only its fallback. + expect(reasoned.flatMap((code) => scan.codes[code]?.reasonless ?? [])).toEqual([]); }); it("read every refusal's reasons (a scan that matches nothing is green over nothing)", () => { @@ -533,33 +416,6 @@ describe("backend ids have their names in both languages", () => { expect([...named].filter((d) => !failDetails.includes(d))).toEqual([]); }); - it(`${lang}: every "that is gone" a 404 names, and every YouTube refusal`, () => { - expect(missing(goneWhats.map((w) => `errors.notFound.${w}`))).toEqual([]); - expect( - [...idsUnder(keys, "errors.notFound")].filter( - (w) => w !== "generic" && !goneWhats.includes(w), - ), - ).toEqual([]); - expect(missing(youtubeReasons.map((r) => `errors.youtube.${r}`))).toEqual([]); - expect( - [...idsUnder(keys, "errors.youtube")].filter((r) => !youtubeReasons.includes(r)), - ).toEqual([]); - }); - - it(`${lang}: every generic refusal the backend can answer, and none it cannot`, () => { - expect(missing(refusedReasons.map((r) => `errors.refused.${r}`))).toEqual([]); - expect( - [...idsUnder(keys, "errors.refused")].filter((r) => !refusedReasons.includes(r)), - ).toEqual([]); - }); - - it(`${lang}: every reason a clip is refused, and none it cannot be`, () => { - expect(missing(clipReasons.map((r) => `clip.errors.${r}`))).toEqual([]); - expect([...idsUnder(keys, "clip.errors")].filter((r) => !clipReasons.includes(r))).toEqual( - [], - ); - }); - it(`${lang}: every crew job, auto-tag and built-in profile the backend writes, and no other`, () => { for (const [group, words] of Object.entries(VOCAB)) { const wanted = words.map((w) => `vocab.${group}.${vocabKey(w)}`); @@ -588,32 +444,32 @@ describe("backend ids have their names in both languages", () => { }); it(`${lang}: no sentence for a refusal reason the backend cannot send`, () => { - // A family whose prefix is its own (the shared `errors.admin` is checked below). A suffixed - // family shares its prefix with more than its refusals — the sign-in screens' `gate`, - // `addressLabel` — so only an id with the refusal's own key counts as a reason there. + // By prefix: the admin actions share `errors.admin`. A suffixed family shares its prefix with more + // than its refusals — the sign-in screens' `gate`, `addressLabel` — so only an id with the + // refusal's own key counts as a reason there. + const byPrefix = new Map< + string, + { suffix: string; reasons: Set; spare: Set } + >(); for (const [code, f] of Object.entries(REFUSAL_FAMILIES)) { - if (f.prefix === "errors.admin") continue; - const suffix = "suffix" in f ? f.suffix : ""; - const fallbackId = f.fallback.startsWith(`${f.prefix}.`) - ? f.fallback.slice(f.prefix.length + 1) - : null; - expect( - [...idsUnder(keys, f.prefix)].filter( - (r) => - keys.has(`${f.prefix}.${r}${suffix}`) && - r !== fallbackId && - !reasonsOf[code]!.includes(r), - ), - code, - ).toEqual([]); + const p = byPrefix.get(f.prefix) ?? { + suffix: "suffix" in f ? f.suffix : "", + reasons: new Set(), + spare: new Set(), + }; + for (const r of reasonsOf[code]!) p.reasons.add(r); + if (f.fallback.startsWith(`${f.prefix}.`)) + p.spare.add(f.fallback.slice(f.prefix.length + 1)); + byPrefix.set(f.prefix, p); } - }); - - it(`${lang}: every admin refusal reason`, () => { - expect(missing(adminReasons.map((r) => `errors.admin.${r}`))).toEqual([]); - expect([...idsUnder(keys, "errors.admin")].filter((r) => !adminReasons.includes(r))).toEqual( - [], - ); + for (const [prefix, { suffix, reasons, spare }] of byPrefix) + expect( + [...idsUnder(keys, prefix)].filter( + (r) => + (!suffix || keys.has(`${prefix}.${r}${suffix}`)) && !spare.has(r) && !reasons.has(r), + ), + prefix, + ).toEqual([]); }); } }); From f8a8e905da8eac45837683a6b1890ea39e042b38 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 31 Oct 2026 18:45:02 +0100 Subject: [PATCH 4/6] fix(i18n): read refusals along a closed list of shapes, report the rest The scan followed many shapes and called the rest unread, but each shape it learned opened a silent branch: a parameter rebound in the body, a second visit of a followed helper, an `import app.x`, a name bound twice or under an `if`, a spread or a store that replaces the code, a detail rebuilt from its variable, a None reason. It now reads only what the app writes and reports everything else: a branch the app never takes is gone (an `or`, a default, a base class's attribute, string and union annotations, a bare module constant), a spread is clean only from an untouched `**kwargs` no call puts a `code` in, and the detail's variable may only grow by `|=` or `["reason"]`. The real tree reads exactly as before; every exit and every kept branch has a self-test that a mutant of it fails. The detail test also fails a dict without a `code`, a JSONResponse sentence and a detail-less HTTPException; the two that are no refusal (the setup gate's 503, an unmatched api path's 404) are listed with why. The contract's assertion that compared the reason map with itself is gone. --- backend/app/refusal.py | 12 +- backend/tests/refusal_sites.py | 740 +++++++++++------- backend/tests/test_http_details.py | 91 +++ backend/tests/test_refusal_sites.py | 511 ++++++++++-- frontend/src/i18n/backendIds.contract.test.ts | 4 +- 5 files changed, 985 insertions(+), 373 deletions(-) diff --git a/backend/app/refusal.py b/backend/app/refusal.py index 337a1c8a..d996cff4 100644 --- a/backend/app/refusal.py +++ b/backend/app/refusal.py @@ -7,10 +7,11 @@ whatever sentence the route happened to hand-write. `refused` is the generic fam (`errors.refused.`, the params interpolated into the sentence); the older specific codes (`not_found`, `youtube`, `playlist`, the admin refusals, …) stay, since each is already translated. -A test (`test_http_details.py`) reads every `HTTPException` in the app and fails on a string detail. -`tests/refusal_sites.py` reads every `{"code": …}` detail from the syntax tree and follows each reason -to its values: a literal, a `Literal` type on the parameter or attribute it travels in, every call of -a helper like `refused` below, a map's values. The frontend's contract test +A test (`test_http_details.py`) reads every `HTTPException` and `JSONResponse` detail in the app and +fails on a sentence, a dict without a `code`, or none at all. `tests/refusal_sites.py` reads every +`{"code": …}` detail from the syntax tree and follows each reason along a closed list of shapes: a +literal, a `Literal` type on the parameter or attribute it travels in, every call of a helper like +`refused` below, a map's values; anything else is reported, never guessed. The frontend's contract test (`i18n/backendIds.contract.test.ts`) runs it and requires each code in one record of `lib/api/refusalCodes.ts`, and each reason's sentence in both languages; a code or reason it cannot follow fails it, unless no screen shows that code. So the rule is enforced, not described. @@ -23,7 +24,8 @@ from app.sysconfig import InvalidSetting, SettingReason def refused(status: int, reason: str, **params: object) -> HTTPException: """The refusal to raise: `raise refused(409, "view_name_taken", name=name)`. `params` travel in - the detail for the sentence to interpolate (`{{name}}`); keep them to values the reader may see.""" + the detail for the sentence to interpolate (`{{name}}`); keep them to values the reader may see, + and never a `code`: it would replace this one (the refusal scan reads such a call as an unread code).""" return HTTPException(status_code=status, detail={"code": "refused", "reason": reason, **params}) diff --git a/backend/tests/refusal_sites.py b/backend/tests/refusal_sites.py index edf79344..0fbe3bbd 100644 --- a/backend/tests/refusal_sites.py +++ b/backend/tests/refusal_sites.py @@ -2,27 +2,37 @@ with every reason its code can be sent with. The frontend's contract (`i18n/backendIds.contract.test.ts`) runs this and requires a sentence for each -code and reason in both languages. A regex over the source read only the shapes it was written for: a -reason after another key, a status passed by name, a second handler with the same text all went past it. -Here a reason is followed to where it comes from, and what cannot be followed is reported, never guessed: +code and reason in both languages. A regex over the source read only the shapes it was written for, and a +scan that tries to follow every shape opens a silent gap with each one it learns. So this one reads a +CLOSED list, the shapes the app writes, and reports everything else, never guesses: -- a literal; both arms of `a if c else b`, every operand of `a or b`; -- a module constant (`urlguard.REASON_MALFORMED`), a local variable's assignments; -- a parameter or attribute typed with a `Literal` (basedpyright holds every value passed in), the - attribute found on the class an `except … as e` catches, or the class a parameter is typed with; -- a `str` parameter of a helper (`refused`, `not_found`): followed to every call of it, its default - included, and a helper passed around as a value is unread; -- a call: its return annotation; a map lookup (`_SETTING_REASONS[exc.reason]`): the map's values, and - its keys must be exactly what the index can be. +- a reason: a string literal (`None` sends none: the site counts as reasonless); both arms of + `a if c else b`; a local variable bound only by plain `x = …`; a module constant reached as `mod.NAME`; + a call's `Literal` return annotation; +- a parameter typed with a `Literal` (basedpyright holds every value passed in), or a `str` one of a + module-level helper (`refused`, `not_found`), followed to every call, each of which must pass it by + position or by name; a parameter its body binds again is unread; +- an attribute: `e.x` of the exception an `except C as e` catches, or of a parameter typed `C`, when a + method of `C` declares `self.x: `; +- a map lookup (`_SETTING_REASONS[exc.reason]`): the map's values, and its keys must be exactly what the + index can be; +- the code: one literal, never changed after. A `**` spread in the detail is the `**kwargs` of a helper or + of an exception's `__init__` that takes `reason` by name, used for nothing but being spread, and no call + of it passes `code=` or a `**` that is not such a spread itself. The detail's variable is changed only by + `|= {…}` or `["reason"] = …`, and goes nowhere but into an `HTTPException` / `JSONResponse`. -Anything else is UNREAD, and the contract fails on it unless no screen shows that code. Stdlib only: it -runs on the host, beside the frontend's tests (`python -I backend/tests/refusal_sites.py` → JSON). +A name bound twice at module level, inside a module-level `if`/`try`/loop, or imported from two places is +not looked up. A reference named like a followed helper that the scan cannot resolve (`import app.x`, a +relative import) is unread. Anything else is UNREAD, or an unread code, and the contract fails on it unless +no screen shows that code. Stdlib only: it runs on the host, beside the frontend's tests +(`python -I backend/tests/refusal_sites.py` → JSON). """ import ast import json import pathlib import sys +from collections.abc import Iterator from dataclasses import dataclass, field APP = pathlib.Path(__file__).resolve().parents[1] / "app" @@ -33,16 +43,34 @@ NOT_REFUSALS = { "plex/tmdb.py": "a production country's ISO code", } +# What a detail held in a variable may be handed to: the response it is sent in, nothing that could change it. +SENDERS = {"HTTPException", "JSONResponse"} +READS = {"get", "items", "keys", "values", "copy"} + Node = ast.AST +FUNCS = (ast.FunctionDef, ast.AsyncFunctionDef) +SCOPES = (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda, ast.ClassDef) +COMPOUND = ( + ast.If, + ast.Try, + ast.TryStar, + ast.For, + ast.AsyncFor, + ast.While, + ast.With, + ast.AsyncWith, + ast.Match, +) @dataclass class Res: values: set[str] = field(default_factory=set) unread: list[str] = field(default_factory=list) + none: bool = False # an arm sends no reason def __or__(self, other: "Res") -> "Res": - return Res(self.values | other.values, self.unread + other.unread) + return Res(self.values | other.values, self.unread + other.unread, self.none or other.none) @dataclass @@ -51,10 +79,45 @@ class Module: name: str # dotted, `app.…` tree: ast.Module parents: dict[Node, Node] - imports: dict[ - str, tuple[str, str | None] - ] # local name → (module, symbol or None for the module) - top: dict[str, Node] # module-level bindings: def, class, or the assigned value + # local name → (module, symbol or None for the module) + imports: dict[str, tuple[str, str | None]] + top: dict[str, Node] # module-level bindings: def, class, or the assignment + ambiguous: set[str] # bound more than once, conditionally, or imported from two places + + +def _own_scope(node: Node) -> Iterator[Node]: + """`node` and what it holds in its own scope: a nested def, class or lambda is itself, not its body.""" + stack = [node] + while stack: + n = stack.pop() + yield n + if n is not node and isinstance(n, SCOPES): + continue + stack.extend(ast.iter_child_nodes(n)) + + +def _stored(target: Node) -> set[str]: + """The names an assignment target binds, not the ones it reads (`LABELS[k] = v` binds none).""" + return { + n.id for n in ast.walk(target) if isinstance(n, ast.Name) and isinstance(n.ctx, ast.Store) + } + + +def _binds(node: Node) -> set[str]: + """The names one node binds in the scope it sits in.""" + if isinstance(node, ast.Assign): + return set().union(*(_stored(t) for t in node.targets)) + if isinstance(node, (ast.AnnAssign, ast.AugAssign, ast.For, ast.AsyncFor, ast.NamedExpr)): + return _stored(node.target) + if isinstance(node, (ast.With, ast.AsyncWith)): + return set().union(*(_stored(i.optional_vars) for i in node.items if i.optional_vars)) + if isinstance(node, (ast.Import, ast.ImportFrom)): + return {(a.asname or a.name).split(".")[0] for a in node.names} + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + return {node.name} + if isinstance(node, (ast.MatchAs, ast.MatchStar, ast.ExceptHandler)) and node.name: + return {node.name} + return set() def _module_name(path: str) -> str: @@ -69,16 +132,18 @@ def _load(sources: dict[str, str]) -> dict[str, Module]: for path, src in sources.items(): tree = ast.parse(src) parents = {child: node for node in ast.walk(tree) for child in ast.iter_child_nodes(node)} - imports: dict[str, tuple[str, str | None]] = {} + origins: dict[str, set[tuple[str, str | None]]] = {} for node in ast.walk(tree): if isinstance(node, ast.ImportFrom) and node.module and node.level == 0: for a in node.names: - imports[a.asname or a.name] = (node.module, a.name) + origins.setdefault(a.asname or a.name, set()).add((node.module, a.name)) elif isinstance(node, ast.Import): for a in node.names: if a.asname: - imports[a.asname] = (a.name, None) + origins.setdefault(a.asname, set()).add((a.name, None)) top: dict[str, Node] = {} + count: dict[str, int] = {} + ambiguous = {name for name, o in origins.items() if len(o) > 1} for stmt in tree.body: if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): top[stmt.name] = stmt @@ -88,8 +153,15 @@ def _load(sources: dict[str, str]) -> dict[str, Module]: top[t.id] = stmt elif isinstance(stmt, ast.AnnAssign) and isinstance(stmt.target, ast.Name): top[stmt.target.id] = stmt + for n in [stmt] if isinstance(stmt, SCOPES) else _own_scope(stmt): + for name in _binds(n): + count[name] = count.get(name, 0) + 1 + if isinstance(stmt, COMPOUND): + ambiguous.add(name) + ambiguous |= {name for name, c in count.items() if c > 1} + imports = {name: next(iter(o)) for name, o in origins.items() if name not in ambiguous} name = _module_name(path) - mods[name] = Module(path, name, tree, parents, imports, top) + mods[name] = Module(path, name, tree, parents, imports, top, ambiguous) return mods @@ -98,11 +170,17 @@ class Scan: self.mods = _load(sources) self.errors: list[str] = [] self._following: set[tuple[str, str, str]] = set() + self._followed: dict[tuple[str, str, str], Res] = {} + self._clean: dict[int, bool] = {} # --- names --------------------------------------------------------------------------------------- - def lookup(self, m: Module, name: str) -> tuple[Module, Node] | tuple[str, None] | None: + def lookup( + self, m: Module, name: str, seen: frozenset[tuple[str, str]] = frozenset() + ) -> tuple[Module, Node] | tuple[str, None] | None: """What a module-level name is: (its module, its binding), or (a module's name, None).""" + if name in m.ambiguous: + return None if name in m.top: return m, m.top[name] if name in m.imports: @@ -111,19 +189,33 @@ class Scan: return (mod, None) if mod in self.mods else None if f"{mod}.{sym}" in self.mods: return f"{mod}.{sym}", None - if mod in self.mods: - return self.lookup(self.mods[mod], sym) + if mod in self.mods and (mod, sym) not in seen: + return self.lookup(self.mods[mod], sym, seen | {(mod, sym)}) return None def lookup_expr(self, m: Module, e: Node) -> tuple[Module, Node] | tuple[str, None] | None: if isinstance(e, ast.Name): - return self.lookup(m, e.id) + return None if self.local(m, e) else self.lookup(m, e.id) if isinstance(e, ast.Attribute): base = self.lookup_expr(m, e.value) if base and base[1] is None and isinstance(base[0], str): return self.lookup(self.mods[base[0]], e.attr) return None + def local(self, m: Module, e: ast.Name) -> bool: + """`e` names a parameter or a variable of a function it sits in.""" + fn = self.enclosing(m, e, FUNCS) + while fn is not None: + a = fn.args + if e.id in { + p.arg for p in [*a.posonlyargs, *a.args, *a.kwonlyargs, a.vararg, a.kwarg] if p + }: + return True + if self.assignments(fn, e.id): + return True + fn = self.enclosing(m, fn, FUNCS) + return False + def at(self, m: Module, e: Node) -> str: return f"{m.path}:{getattr(e, 'lineno', '?')}: {ast.unparse(e)}" @@ -133,60 +225,106 @@ class Scan: node = m.parents.get(node) return node + def assignments(self, fn: Node, name: str) -> list[Node]: + """What binds `name` in this function's own body (not in a nested one).""" + return [n for n in _own_scope(fn) if n is not fn and name in _binds(n)] + + def caught_as(self, m: Module, e: Node, name: str) -> ast.ExceptHandler | None: + node = m.parents.get(e) + while node is not None and not isinstance(node, FUNCS): + if isinstance(node, ast.ExceptHandler) and node.name == name: + return node + node = m.parents.get(node) + return None + + def references( + self, m: Module, target: Node + ) -> Iterator[tuple[Module, Node, ast.Call | None, bool]]: + """Every reference to a module-level def or class: the call it makes (None: not a call), and + whether it resolved. A reference named like it that the scan cannot resolve is yielded too.""" + name = getattr(target, "name", None) + for cm in self.mods.values(): + for node in ast.walk(cm.tree): + if not isinstance(node, (ast.Name, ast.Attribute)) or not isinstance( + node.ctx, ast.Load + ): + continue + if (node.id if isinstance(node, ast.Name) else node.attr) != name: + continue + parent = cm.parents.get(node) + if isinstance(parent, ast.Attribute) and parent.value is node: + continue + found = self.lookup_expr(cm, node) + if found: + if not (found[0] is m and found[1] is target): + continue + elif not self.unresolved(cm, node): + continue + call = parent if isinstance(parent, ast.Call) and parent.func is node else None + yield cm, node, call, found is not None + + def unresolved(self, m: Module, node: Node) -> bool: + """A name, or a dotted path, the app could mean that the scan cannot resolve: not a local, nor an + outside module's (an attribute of an object the function holds is not one).""" + root = node + while isinstance(root, ast.Attribute): + root = root.value + if not isinstance(root, ast.Name) or self.local(m, root): + return False + if root.id in m.ambiguous or (root.id not in m.top and root.id not in m.imports): + return True + if root is node or root.id in m.top: + return False + found = self.lookup(m, root.id) + return ( + found is not None and found[1] is None + ) # an app module whose dotted path went nowhere + # --- types --------------------------------------------------------------------------------------- - def ann_values(self, m: Module, ann: Node | None) -> set[str] | None: - """The values a `Literal` annotation allows, or None when it is not one (`str`, a class, …).""" - if ann is None: - return None - if isinstance(ann, ast.Constant): - if ann.value is None: - return set() - if isinstance(ann.value, str): - return self.ann_values(m, ast.parse(ann.value, mode="eval").body) - return None + def ann_values(self, m: Module, ann: Node | None, depth: int = 0) -> set[str] | None: + """The values a `Literal[…]` annotation, or an alias of one, allows; None when it is not one.""" if isinstance(ann, ast.Subscript) and getattr(ann.value, "id", None) == "Literal": items = ann.slice.elts if isinstance(ann.slice, ast.Tuple) else [ann.slice] if all(isinstance(i, ast.Constant) and isinstance(i.value, str) for i in items): return {i.value for i in items} return None - if isinstance(ann, ast.BinOp) and isinstance(ann.op, ast.BitOr): - left, right = self.ann_values(m, ann.left), self.ann_values(m, ann.right) - return None if left is None or right is None else left | right - found = self.lookup_expr(m, ann) - if found and isinstance(found[1], ast.Assign) and isinstance(found[0], Module): - if isinstance(found[1].value, (ast.Subscript, ast.BinOp, ast.Name, ast.Attribute)): - return self.ann_values(found[0], found[1].value) + if isinstance(ann, (ast.Name, ast.Attribute)) and depth < 8: + found = self.lookup_expr(m, ann) + if found and isinstance(found[0], Module) and isinstance(found[1], ast.Assign): + return self.ann_values(found[0], found[1].value, depth + 1) return None def classes(self, m: Module, e: Node | None) -> list[tuple[Module, ast.ClassDef]] | None: - if e is None: - return None - if isinstance(e, ast.Tuple): - out = [self.classes(m, x) for x in e.elts] - return None if any(o is None for o in out) else [c for o in out for c in o] - found = self.lookup_expr(m, e) + found = self.lookup_expr(m, e) if e is not None else None if found and isinstance(found[0], Module) and isinstance(found[1], ast.ClassDef): return [(found[0], found[1])] return None + def typed(self, m: Module, at: Node, name: str) -> list[tuple[Module, ast.ClassDef]] | None: + """The class `name` holds where `at` sits: the one an `except C as name` catches, or a parameter's + annotation (a parameter its body binds again holds no known class).""" + handler = self.caught_as(m, at, name) + if handler is not None: + return self.classes(m, handler.type) + fn = self.enclosing(m, at, FUNCS) + if fn is None: + return None + params = {a.arg: a for a in [*fn.args.posonlyargs, *fn.args.args, *fn.args.kwonlyargs]} + if name in params and not self.assignments(fn, name): + return self.classes(m, params[name].annotation) + return None + def attr_values(self, cm: Module, cls: ast.ClassDef, attr: str) -> set[str] | None: - """An attribute's `Literal` values, from its annotation on the class or `self.attr: T` in a method, - else from a base class.""" + """An attribute's `Literal` values, from `self.attr: T` in a method of the class.""" for node in ast.walk(cls): - if isinstance(node, ast.AnnAssign): - t = node.target - if (isinstance(t, ast.Name) and t.id == attr and node in cls.body) or ( - isinstance(t, ast.Attribute) - and t.attr == attr - and getattr(t.value, "id", None) == "self" - ): - return self.ann_values(cm, node.annotation) - for base in cls.bases: - for bm, bc in self.classes(cm, base) or []: - v = self.attr_values(bm, bc, attr) - if v is not None: - return v + if ( + isinstance(node, ast.AnnAssign) + and isinstance(node.target, ast.Attribute) + and node.target.attr == attr + and getattr(node.target.value, "id", None) == "self" + ): + return self.ann_values(cm, node.annotation) return None # --- reasons ------------------------------------------------------------------------------------- @@ -194,27 +332,18 @@ class Scan: def resolve(self, m: Module, e: Node) -> Res: if isinstance(e, ast.Constant): if e.value is None: - return Res() + return Res(none=True) if isinstance(e.value, str): return Res({e.value}) if isinstance(e, ast.IfExp): return self.resolve(m, e.body) | self.resolve(m, e.orelse) - if isinstance(e, ast.BoolOp) and isinstance(e.op, ast.Or): - out = Res() - for v in e.values: - out = out | self.resolve(m, v) - return out if isinstance(e, ast.Name): return self.resolve_name(m, e) if isinstance(e, ast.Attribute): return self.resolve_attr(m, e) if isinstance(e, ast.Call): found = self.lookup_expr(m, e.func) - if ( - found - and isinstance(found[0], Module) - and isinstance(found[1], (ast.FunctionDef, ast.AsyncFunctionDef)) - ): + if found and isinstance(found[0], Module) and isinstance(found[1], FUNCS): v = self.ann_values(found[0], found[1].returns) if v is not None: return Res(v) @@ -223,95 +352,31 @@ class Scan: return Res(unread=[self.at(m, e)]) def resolve_name(self, m: Module, e: ast.Name) -> Res: - fn = self.enclosing(m, e, (ast.FunctionDef, ast.AsyncFunctionDef)) + fn = self.enclosing(m, e, FUNCS) while fn is not None: - assert isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)) + assert isinstance(fn, FUNCS) if self.caught_as(m, e, e.id) is not None: return Res(unread=[self.at(m, e)]) # the exception itself, not a reason - params = {a.arg: a for a in [*fn.args.posonlyargs, *fn.args.args, *fn.args.kwonlyargs]} - if e.id in params: - return self.resolve_param(m, fn, params[e.id], e) - if ( - fn.args.vararg - and fn.args.vararg.arg == e.id - or fn.args.kwarg - and fn.args.kwarg.arg == e.id - ): - return Res(unread=[self.at(m, e)]) + a = fn.args + params = {p.arg: p for p in [*a.posonlyargs, *a.args, *a.kwonlyargs]} assigned = self.assignments(fn, e.id) + if e.id in params: + if assigned: + return Res(unread=[f"{self.at(m, e)} (a parameter its body binds again)"]) + return self.resolve_param(m, fn, params[e.id], e) if assigned: out = Res() - for a in assigned: - if ( - isinstance(a, ast.AnnAssign) - and (v := self.ann_values(m, a.annotation)) is not None + for s in assigned: + if isinstance(s, ast.Assign) and all( + isinstance(t, ast.Name) for t in s.targets ): - out = out | Res(v) - elif ( - isinstance(a, (ast.Assign, ast.AnnAssign)) - and a.value is not None - and not any( - isinstance(t, (ast.Tuple, ast.List)) for t in getattr(a, "targets", []) - ) - ): - out = out | self.resolve(m, a.value) + out = out | self.resolve(m, s.value) else: out = out | Res(unread=[self.at(m, e)]) return out - fn = self.enclosing(m, fn, (ast.FunctionDef, ast.AsyncFunctionDef)) - found = self.lookup(m, e.id) - if ( - found - and isinstance(found[0], Module) - and isinstance(found[1], (ast.Assign, ast.AnnAssign)) - ): - stmt = found[1] - if stmt.value is not None and isinstance(stmt.value, ast.Constant): - return self.resolve(found[0], stmt.value) + fn = self.enclosing(m, fn, FUNCS) return Res(unread=[self.at(m, e)]) - def assignments(self, fn: Node, name: str) -> list[Node]: - """The statements that bind `name` in this function's own body (not in a nested one).""" - out: list[Node] = [] - stack = list(ast.iter_child_nodes(fn)) - while stack: - node = stack.pop() - if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda, ast.ClassDef)): - continue - if isinstance(node, ast.Assign): - if any( - name in {n.id for n in ast.walk(t) if isinstance(n, ast.Name)} - for t in node.targets - ): - out.append(node) - elif isinstance(node, ast.AnnAssign) and getattr(node.target, "id", None) == name: - out.append(node) - elif isinstance( - node, (ast.For, ast.With, ast.AugAssign, ast.NamedExpr, ast.comprehension) - ): - targets = [getattr(node, "target", None)] + [ - i.optional_vars - for i in getattr(node, "items", []) - if i.optional_vars is not None - ] - if any( - t is not None and name in {n.id for n in ast.walk(t) if isinstance(n, ast.Name)} - for t in targets - ): - out.append( - node - ) # bound by a loop, a `with`, an augmented or a walrus: not followed - stack.extend(ast.iter_child_nodes(node)) - return out - - def caught_as(self, m: Module, e: Node, name: str) -> ast.ExceptHandler | None: - node = m.parents.get(e) - while node is not None and not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): - if isinstance(node, ast.ExceptHandler) and node.name == name: - return node - node = m.parents.get(node) - return None - def resolve_param(self, m: Module, fn: Node, param: ast.arg, e: Node) -> Res: v = self.ann_values(m, param.annotation) if v is not None: @@ -322,70 +387,45 @@ class Scan: def follow(self, m: Module, fn: Node, param: ast.arg) -> Res: """A `str` parameter of a helper: what every call of the helper passes it.""" - assert isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)) + assert isinstance(fn, FUNCS) key = (m.name, fn.name, param.arg) + if key in self._followed: + return self._followed[key] + where = f"{m.path}:{fn.lineno}: {fn.name}({param.arg})" if key in self._following: - return Res() - if m.top.get(fn.name) is not fn: - return Res( - unread=[ - f"{m.path}:{fn.lineno}: {fn.name}({param.arg}) is not a module-level helper" - ] - ) + return Res(unread=[f"{where} reached again while it is followed"]) + if m.top.get(fn.name) is not fn or fn.name in m.ambiguous: + return Res(unread=[f"{where} is not a module-level helper"]) self._following.add(key) positional = [a.arg for a in [*fn.args.posonlyargs, *fn.args.args]] - defaults = dict( - zip( - positional[len(positional) - len(fn.args.defaults) :], fn.args.defaults, strict=True - ) - ) - defaults |= { - a.arg: d - for a, d in zip(fn.args.kwonlyargs, fn.args.kw_defaults, strict=True) - if d is not None - } + by_name = param not in fn.args.posonlyargs out = Res() - for cm in self.mods.values(): - for node in ast.walk(cm.tree): - if not isinstance(node, (ast.Name, ast.Attribute)) or isinstance( - cm.parents.get(node), ast.Attribute - ): - continue - found = self.lookup_expr(cm, node) - if not (found and found[0] is m and found[1] is fn): - continue - call = cm.parents.get(node) - if not (isinstance(call, ast.Call) and call.func is node): - out = out | Res(unread=[f"{self.at(cm, node)} (the helper used as a value)"]) - continue - kw = {k.arg: k.value for k in call.keywords} - given = param.arg in positional and positional.index(param.arg) < len(call.args) - if param.arg in kw: - out = out | self.resolve(cm, kw[param.arg]) - elif any(isinstance(a, ast.Starred) for a in call.args): - out = out | Res(unread=[self.at(cm, call)]) - elif given: # given by position, a `**` spread cannot pass it again (a TypeError) - out = out | self.resolve(cm, call.args[positional.index(param.arg)]) - elif None in kw: - out = out | Res(unread=[self.at(cm, call)]) - elif param.arg in defaults: - out = out | self.resolve(m, defaults[param.arg]) + for cm, node, call, resolved in self.references(m, fn): + if not resolved: + out = out | Res( + unread=[f"{self.at(cm, node)} (a reference the scan cannot resolve)"] + ) + continue + if call is None: + out = out | Res(unread=[f"{self.at(cm, node)} (the helper used as a value)"]) + continue + kw = {k.arg: k.value for k in call.keywords} + given = param.arg in positional and positional.index(param.arg) < len(call.args) + if by_name and param.arg in kw: + out = out | self.resolve(cm, kw[param.arg]) + elif any(isinstance(a, ast.Starred) for a in call.args): + out = out | Res(unread=[self.at(cm, call)]) + elif given: + out = out | self.resolve(cm, call.args[positional.index(param.arg)]) + else: + out = out | Res(unread=[f"{self.at(cm, call)} (does not pass {param.arg})"]) + self._followed[key] = out # once followed, its key answers from here: never reached again return out def resolve_attr(self, m: Module, e: ast.Attribute) -> Res: base = e.value if isinstance(base, ast.Name): - handler = self.caught_as(m, e, base.id) - fn = self.enclosing(m, e, (ast.FunctionDef, ast.AsyncFunctionDef)) - typed = None - if handler is not None: - typed = self.classes(m, handler.type) - elif fn is not None: - params = { - a.arg: a for a in [*fn.args.posonlyargs, *fn.args.args, *fn.args.kwonlyargs] - } - if base.id in params: - typed = self.classes(m, params[base.id].annotation) + typed = self.typed(m, e, base.id) if typed is not None: out = Res() for cm, cls in typed: @@ -426,144 +466,248 @@ class Scan: for v in table.values: out = out | self.resolve(found[0], v) index = self.resolve(m, e.slice) - keys = {k.value for k in table.keys} - if index.unread or index.values != keys: + keys = {k.value for k in table.keys if isinstance(k, ast.Constant)} + if index.unread or index.none or index.values != keys: self.errors.append( f"{self.at(m, e)}: the map's keys {sorted(keys)} are not what its index can be " f"{sorted(index.values)}{' + unread ' + str(index.unread) if index.unread else ''}" ) return out - # --- sites --------------------------------------------------------------------------------------- - - def spread_is_safe(self, m: Module, e: Node, src: Node) -> bool: - """A `**x` in a detail, or `detail |= {…}` from it, cannot carry a reason when `x` is the `**kwargs` - of a function that takes `reason` by name: Python binds that one to the parameter.""" - if isinstance(src, ast.Attribute): - return self.spread_is_safe_attr(m, e, src) - fn = self.enclosing(m, e, (ast.FunctionDef, ast.AsyncFunctionDef)) - if fn is None or fn.args.kwarg is None: - return False - names = {a.arg for a in [*fn.args.posonlyargs, *fn.args.args, *fn.args.kwonlyargs]} - kwarg = fn.args.kwarg.arg - if isinstance(src, ast.Name): - return src.id == kwarg and "reason" in names + # --- spreads ------------------------------------------------------------------------------------- + def clean_spread(self, m: Module, at: Node, src: Node) -> bool: + """`**src` cannot carry a `reason` or a `code`: it is a function's `**kwargs` (or a caught + exception's `e.params` kept from its `__init__`'s), or `{k: … for k, v in that.items()}`: a + value may change, a key may not.""" if isinstance(src, ast.DictComp): - it = src.generators[0].iter + g = src.generators + if len(g) != 1 or not isinstance(g[0].target, ast.Tuple) or len(g[0].target.elts) != 2: + return False + k, v = g[0].target.elts + it = g[0].iter return ( - isinstance(it, ast.Call) + isinstance(k, ast.Name) + and isinstance(v, ast.Name) + and getattr(src.key, "id", None) == k.id + and isinstance(it, ast.Call) and isinstance(it.func, ast.Attribute) and it.func.attr == "items" - and getattr(it.func.value, "id", None) == kwarg - and "reason" in names + and not it.args + and self.clean_spread(m, at, it.func.value) + ) + if isinstance(src, ast.Attribute) and isinstance(src.value, ast.Name): + typed = self.typed(m, at, src.value.id) + return bool(typed) and all(self.kept(cm, cls, src.attr) for cm, cls in typed or []) + if isinstance(src, ast.Name): + fn = self.enclosing(m, at, FUNCS) + return ( + fn is not None + and fn.args.kwarg is not None + and src.id == fn.args.kwarg.arg + and self.kwargs_clean(m, fn, fn) ) return False - def spread_is_safe_attr(self, m: Module, e: Node, src: ast.Attribute) -> bool: - """`e.params` of a caught exception whose `__init__(self, reason, **params)` keeps `self.params = params`.""" - if not isinstance(src.value, ast.Name): - return False - handler = self.caught_as(m, e, src.value.id) - typed = self.classes(m, handler.type) if handler else None - return bool(typed) and all(self.keeps_kwargs(cls, src.attr) for _, cls in typed or []) - - def keeps_kwargs(self, cls: ast.ClassDef, attr: str) -> bool: + def kept(self, cm: Module, cls: ast.ClassDef, attr: str) -> bool: + """`self.attr` is only ever the `**kwargs` of the class's `__init__`, and that one is clean.""" init = next( (s for s in cls.body if isinstance(s, ast.FunctionDef) and s.name == "__init__"), None ) if init is None or init.args.kwarg is None: return False - names = {a.arg for a in [*init.args.posonlyargs, *init.args.args, *init.args.kwonlyargs]} - kwarg = init.args.kwarg.arg - return "reason" in names and any( - isinstance(n, ast.Assign) - and any( - isinstance(t, ast.Attribute) - and t.attr == attr - and getattr(t.value, "id", None) == "self" - for t in n.targets - ) - and getattr(n.value, "id", None) == kwarg - for n in ast.walk(init) + stores = [ + n + for n in ast.walk(cls) + if isinstance(n, ast.Attribute) + and n.attr == attr + and getattr(n.value, "id", None) == "self" + and not isinstance(n.ctx, ast.Load) + ] + kept = [ + s + for s in stores + if isinstance(p := cm.parents.get(s), ast.Assign) + and getattr(p.value, "id", None) == init.args.kwarg.arg + and self.enclosing(cm, p, FUNCS) is init + ] + return len(stores) == 1 and len(kept) == 1 and self.kwargs_clean(cm, init, cls) + + def kwargs_clean(self, m: Module, fn: Node, target: Node) -> bool: + """`fn`'s `**kwargs` holds no `reason` (`fn` takes it by name) and no `code`: it is used for + nothing but `**kwargs`, `kwargs.items()`, `self.x = kwargs` or an f-string, and every call of + `target` (the helper, or the class `fn` is the `__init__` of) passes no `code=` and no unclean + `**`.""" + assert isinstance(fn, FUNCS) and fn.args.kwarg is not None + if id(fn) in self._clean: + return self._clean[id(fn)] + self._clean[id(fn)] = False # a call that reaches it again while it is checked is not clean + kwarg = fn.args.kwarg.arg + clean = "reason" in {a.arg for a in [*fn.args.args, *fn.args.kwonlyargs]} and not ( + self.assignments(fn, kwarg) ) + for n in _own_scope(fn): + if not clean: + break + if isinstance(n, ast.Name) and n.id == kwarg: + p = m.parents.get(n) + clean = ( + isinstance(p, ast.FormattedValue) # printed in a message + or isinstance(p, ast.keyword) + and p.arg is None + or isinstance(p, ast.Dict) + and any(k is None and v is n for k, v in zip(p.keys, p.values, strict=True)) + or isinstance(p, ast.Attribute) + and p.attr == "items" + and isinstance(m.parents.get(p), ast.Call) + or isinstance(p, ast.Assign) + and p.value is n + and all( + isinstance(t, ast.Attribute) and getattr(t.value, "id", None) == "self" + for t in p.targets + ) + ) + refs = list(self.references(m, target)) if clean else [] + for cm, node, call, resolved in refs: + if call is not None: + clean = not any( + k.arg == "code" or k.arg is None and not self.clean_spread(cm, call, k.value) + for k in call.keywords + ) + elif not resolved or not isinstance(target, ast.ClassDef): + clean = False # a helper passed around, or a name the scan cannot resolve + else: + # `except C`, an annotation: not a construction; a subclass is built some other way + cls = self.enclosing(cm, node, ast.ClassDef) + clean = not (cls is not None and any(node in ast.walk(b) for b in cls.bases)) + if not clean: + break + self._clean[id(fn)] = clean + return clean + + # --- sites --------------------------------------------------------------------------------------- def entries(self, d: Node) -> list[tuple[str | None, Node]]: + """A dict's (key, value) pairs: None for a `**` spread, "?" for a key that is not a literal.""" if isinstance(d, ast.Dict): return [ - (k.value if isinstance(k, ast.Constant) else None if k is None else "?", v) + ( + k.value + if isinstance(k, ast.Constant) and isinstance(k.value, str) + else None + if k is None + else "?", + v, + ) for k, v in zip(d.keys, d.values, strict=True) ] assert isinstance(d, ast.Call) - return [(k.arg, k.value) for k in d.keywords] + return [(None, a) for a in d.args] + [(k.arg, k.value) for k in d.keywords] def site(self, m: Module, d: Node, out: dict) -> None: items = self.entries(d) - code_expr = next(v for k, v in items if k == "code") - code = self.resolve(m, code_expr) - if code.unread or len(code.values) != 1: - out["unreadCodes"].append(self.at(m, code_expr)) + code = Res() + for v in (v for k, v in items if k == "code"): + code = code | self.resolve(m, v) + problems = [ + f"{self.at(m, v)} (a key the scan cannot read)" for k, v in items if k == "?" + ] + [ + f"{self.at(m, v)} (a spread that may carry the code)" + for k, v in items + if k is None and not self.clean_spread(m, d, v) + ] + res, has_reason, changed = self.mutations(m, d) + problems += changed + if code.unread or code.none or len(code.values) != 1 or problems: + out["unreadCodes"] += problems or [self.at(m, d)] return entry = out["codes"].setdefault( next(iter(code.values)), {"reasons": set(), "unread": [], "sites": 0, "reasonless": []} ) entry["sites"] += 1 - res, has_reason = Res(), False for k, v in items: if k == "reason": res, has_reason = res | self.resolve(m, v), True - elif k is None and not self.spread_is_safe(m, d, v): - res = res | Res(unread=[f"{self.at(m, v)} (a spread that may carry a reason)"]) - res, more = self.mutations(m, d, res) - has_reason = has_reason or more - if not has_reason: + if not has_reason or res.none: entry["reasonless"].append(f"{m.path}:{d.lineno}") entry["reasons"] |= res.values entry["unread"] += res.unread - def mutations(self, m: Module, d: Node, res: Res) -> tuple[Res, bool]: - """`detail |= {…}`, `detail["reason"] = …`, `detail.update(…)` on the variable a detail is put in.""" - stmt = m.parents.get(d) - if not (isinstance(stmt, (ast.Assign, ast.AnnAssign)) and stmt.value is d): - return res, False - target = stmt.targets[0] if isinstance(stmt, ast.Assign) else stmt.target - fn = self.enclosing(m, d, (ast.FunctionDef, ast.AsyncFunctionDef)) - if not isinstance(target, ast.Name) or fn is None: - return res, False - found = False - for node in ast.walk(fn): - if isinstance(node, ast.AugAssign) and getattr(node.target, "id", None) == target.id: - v = node.value - if isinstance(v, ast.Dict): - for k, val in self.entries(v): + def mutations(self, m: Module, d: Node) -> tuple[Res, bool, list[str]]: + """A detail put in a variable: what `|= {…}` and `["reason"] = …` add, and every other use that + could change it or send it elsewhere.""" + holder, stmt = d, m.parents.get(d) + while isinstance(stmt, ast.IfExp) and holder is not stmt.test: + holder, stmt = stmt, m.parents.get(stmt) + if not (isinstance(stmt, (ast.Assign, ast.AnnAssign)) and stmt.value is holder): + return Res(), False, [] + targets = stmt.targets if isinstance(stmt, ast.Assign) else [stmt.target] + fn = self.enclosing(m, d, FUNCS) + if fn is None or len(targets) != 1 or not isinstance(targets[0], ast.Name): + return ( + Res(), + False, + [f"{self.at(m, stmt)} (a detail put where the scan cannot follow it)"], + ) + name = targets[0].id + res, found, problems = Res(), False, [] + for n in _own_scope(fn): + if n is stmt: + continue + if name in _binds(n): + if not (isinstance(n, ast.AugAssign) and isinstance(n.op, ast.BitOr)): + problems.append(f"{self.at(m, n)} (binds the detail's variable again)") + elif isinstance(n.value, ast.Dict): + for k, v in self.entries(n.value): if k == "reason": - res, found = res | self.resolve(m, val), True - elif k is None and not self.spread_is_safe(m, node, val): - res = res | Res( - unread=[f"{self.at(m, val)} (a spread that may carry a reason)"] - ) - elif not self.spread_is_safe(m, node, v): - res = res | Res(unread=[f"{self.at(m, node)} (may carry a reason)"]) - elif ( - isinstance(node, ast.Subscript) - and getattr(node.value, "id", None) == target.id - and isinstance(node.ctx, ast.Store) - ): - if isinstance(node.slice, ast.Constant) and node.slice.value != "reason": + res, found = res | self.resolve(m, v), True + elif k in ("code", "?") or k is None and not self.clean_spread(m, n, v): + problems.append(f"{self.at(m, v)} (may change the code)") + elif not self.clean_spread(m, n, n.value): + problems.append(f"{self.at(m, n)} (may change the code)") + continue + if not (isinstance(n, ast.Name) and n.id == name and isinstance(n.ctx, ast.Load)): + continue + p = m.parents.get(n) + if isinstance(p, ast.AugAssign) and p.target is n: + continue + if isinstance(p, ast.Subscript) and p.value is n: + key = p.slice.value if isinstance(p.slice, ast.Constant) else None + assign = m.parents.get(p) + if isinstance(p.ctx, ast.Load): continue - assign = m.parents.get(node) - if isinstance(node.slice, ast.Constant) and isinstance(assign, ast.Assign): + if ( + key == "reason" + and isinstance(p.ctx, ast.Store) + and isinstance(assign, ast.Assign) + and assign.targets == [p] + ): res, found = res | self.resolve(m, assign.value), True - else: - res = res | Res(unread=[f"{self.at(m, node)} (may carry a reason)"]) - elif ( - isinstance(node, ast.Call) - and isinstance(node.func, ast.Attribute) - and getattr(node.func.value, "id", None) == target.id - and node.func.attr in {"update", "setdefault"} + elif not ( + isinstance(key, str) + and key not in ("code", "reason") + and isinstance(p.ctx, ast.Store) + ): + problems.append(f"{self.at(m, p)} (may change the code or drop the reason)") + continue + if isinstance(p, ast.Attribute) and p.attr in READS: + continue + sender = ( + p + if isinstance(p, ast.Call) + else m.parents.get(p) + if isinstance(p, ast.keyword) + else None + ) + func = getattr(sender, "func", None) + if ( + isinstance(sender, ast.Call) + and (getattr(func, "id", None) or getattr(func, "attr", None)) in SENDERS + and (n in sender.args or isinstance(p, ast.keyword) and p.arg == "detail") ): - res = res | Res(unread=[f"{self.at(m, node)} (may carry a reason)"]) - return res, found + continue + problems.append(f"{self.at(m, n)} (the detail goes where the scan cannot follow it)") + return res, found, problems def run(self) -> dict: out: dict = { diff --git a/backend/tests/test_http_details.py b/backend/tests/test_http_details.py index 4b896bd4..5bc98728 100644 --- a/backend/tests/test_http_details.py +++ b/backend/tests/test_http_details.py @@ -51,3 +51,94 @@ def test_the_scan_sees_a_sentence_when_there_is_one(): sample = ast.parse('HTTPException(status_code=400, detail=f"bad {x}")').body[0].value assert _is_text(sample.keywords[1].value) assert not _is_text(ast.parse('{"code": "refused", "reason": "x"}').body[0].value) + + +# Answers that are no refusal of the app's, by the function they are sent from: the client shows a +# detail only when it is an object (`lib/api/core.ts`), so these read as its generic line, and no screen +# needs more from them. +NO_REFUSAL = { + ("main.py", "setup_gate"): "the 503 before setup: the setup screen is what the user sees", + ("main.py", "spa_fallback"): "a bare 404 for an api path no route matches: no screen calls one", +} + + +def _detail_offenders(source: str, path: str) -> list[str]: + """Each detail the client could not say: a dict without a `code`, a sentence, or none at all. + + `HTTPException(status, detail)` and `JSONResponse({"detail": …})`; a variable or a call is the + refusal scan's to read (`refusal_sites.py`, which follows each `code` dict).""" + tree = ast.parse(source) + parents = {child: node for node in ast.walk(tree) for child in ast.iter_child_nodes(node)} + out = [] + for node in ast.walk(tree): + name = isinstance(node, ast.Call) and getattr( + node.func, "id", getattr(node.func, "attr", "") + ) + if name == "HTTPException": + kw = {k.arg: k.value for k in node.keywords} + detail = kw.get("detail", node.args[1] if len(node.args) > 1 else None) + elif name == "JSONResponse": + content = {k.arg: k.value for k in node.keywords}.get( + "content", node.args[0] if node.args else None + ) + if not isinstance(content, ast.Dict) or not any( + isinstance(k, ast.Constant) and k.value == "detail" for k in content.keys + ): + continue # an answer, not a refusal + detail = next( + v + for k, v in zip(content.keys, content.values, strict=True) + if isinstance(k, ast.Constant) and k.value == "detail" + ) + else: + continue + fn = parents.get(node) + while fn is not None and not isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)): + fn = parents.get(fn) + if (path, getattr(fn, "name", None)) in NO_REFUSAL: + continue + keys = ( + [k.value for k in detail.keys if isinstance(k, ast.Constant)] + if isinstance(detail, ast.Dict) + else [k.arg for k in detail.keywords] + if isinstance(detail, ast.Call) and getattr(detail.func, "id", "") == "dict" + else None + ) + if detail is None or _is_text(detail) or (keys is not None and "code" not in keys): + out.append(f"{path}:{node.lineno}") + return out + + +def test_every_detail_is_one_the_client_can_say(): + offenders = [ + o + for p in sorted(APP.rglob("*.py")) + for o in _detail_offenders(p.read_text(encoding="utf-8"), p.relative_to(APP).as_posix()) + ] + assert offenders == [], ( + "a refusal the client can only answer with its generic line — send `{code, reason}` " + f"(`refused(status, reason)`), or list it in NO_REFUSAL with why: {offenders}" + ) + + +def test_the_detail_scan_sees_each_shape_the_client_cannot_say(): + src = """ +def f(x): + raise HTTPException(409, detail={"reason": "view_name_taken"}) +def g(x): + raise HTTPException(409, dict(reason="taken")) +def h(x): + raise HTTPException(status_code=404) +def i(x): + return JSONResponse({"detail": "This instance isn't set up yet."}, status_code=503) +def j(x): + return JSONResponse(content={"detail": {"reason": "x"}}, status_code=409) +def ok(x): + raise HTTPException(409, {"code": "refused", "reason": "x"}) + raise HTTPException(409, detail(x)) + return JSONResponse({"detail": {"code": "too_large"}}, status_code=413) + return JSONResponse({"ok": True}) +def setup_gate(x): + return JSONResponse({"detail": "listed"}, status_code=503) +""" + assert _detail_offenders(src, "main.py") == [f"main.py:{n}" for n in (3, 5, 7, 9, 11)] diff --git a/backend/tests/test_refusal_sites.py b/backend/tests/test_refusal_sites.py index 9c6effbb..e9b74afc 100644 --- a/backend/tests/test_refusal_sites.py +++ b/backend/tests/test_refusal_sites.py @@ -1,6 +1,7 @@ -"""The refusal scan (`refusal_sites.py`) on small sources: every shape a reason travels in is followed to -its values or reported unread. A scan that silently drops a shape is a guard green over nothing, so each -shape that once went past the regex it replaced is here. Pure: no app import, no database.""" +"""The refusal scan (`refusal_sites.py`) on small sources: each shape on its closed list is followed to its +values, and each exit from that list is reported, unread or as an unread code. A scan that silently drops a +shape is a guard green over nothing, so every exit is pinned here, not only the paths the app takes. Pure: +no app import, no database.""" import textwrap @@ -17,6 +18,10 @@ def _code(result: dict, code: str) -> dict: return result["codes"][code] +def _unread(result: dict, code: str) -> list[str]: + return [u.split(": ", 1)[1] for u in _code(result, code)["unread"]] + + REFUSAL = """ from fastapi import HTTPException @@ -24,6 +29,14 @@ REFUSAL = """ return HTTPException(status_code=status, detail={"code": "refused", "reason": reason, **params}) """ +# The same helper without a `**params` spread: what a call passes it is about the reason alone. +PLAIN = """ + from fastapi import HTTPException + + def refused(status: int, reason: str) -> HTTPException: + return HTTPException(status_code=status, detail={"code": "refused", "reason": reason}) +""" + def test_a_literal_wherever_it_sits_in_the_dict(): r = _scan( @@ -45,17 +58,38 @@ def test_a_literal_wherever_it_sits_in_the_dict(): assert _code(r, "clip")["unread"] == [] -def test_every_arm_of_a_conditional_and_every_operand_of_or(): +def test_both_arms_of_a_conditional_and_a_none_arm_makes_the_site_reasonless(): r = _scan( a=""" - def f(x, e): + def f(x): raise HTTPException(409, {"code": "edit", "reason": "a" if x else "b"}) def g(x): - raise HTTPException(409, {"code": "edit", "reason": x.why or "c"}) + raise HTTPException(409, {"code": "playlist", "reason": "lit" if x else None}) + def h(x): + raise HTTPException(409, {"code": "busy", "reason": None}) """ ) - assert _code(r, "edit")["reasons"] == ["a", "b", "c"] - assert [u.split(": ", 1)[1] for u in _code(r, "edit")["unread"]] == ["x.why"] + assert _code(r, "edit")["reasons"] == ["a", "b"] and _code(r, "edit")["reasonless"] == [] + assert _code(r, "playlist")["reasons"] == ["lit"] + assert _code(r, "playlist")["reasonless"] == ["a.py:5"] + assert _code(r, "busy")["reasonless"] == ["a.py:7"] + + +def test_what_the_list_does_not_hold_is_unread(): + r = _scan( + a=""" + def f(x, svc): + raise HTTPException(409, {"code": "edit", "reason": x.why or "c"}) + def g(svc): + raise HTTPException(409, {"code": "edit", "reason": svc.reason()}) + def h(): + raise HTTPException(409, {"code": "edit", "reason": 5}) + def i(e): + raise HTTPException(409, {"code": "edit", "reason": f"x{e}"}) + """ + ) + assert _unread(r, "edit") == ["x.why or 'c'", "svc.reason()", "5", "f'x{e}'"] + assert _code(r, "edit")["reasons"] == [] def test_a_helper_is_followed_to_every_call_however_it_is_written(): @@ -86,23 +120,43 @@ def test_a_helper_is_followed_to_every_call_however_it_is_written(): assert _code(r, "refused")["unread"] == [] -def test_a_helper_passed_as_a_value_or_spread_into_is_unread(): +def test_a_call_the_helper_cannot_be_followed_through_is_unread(): r = _scan( - refusal=REFUSAL, + refusal=PLAIN, routes__a=""" from app.refusal import refused - def f(args): + def f(args, kw): pick = refused raise refused(*args) + def g(kw): + raise refused(409, **kw) + def h(): + raise refused(409) + """, + routes__b=""" + import app.refusal + + def f(): + raise app.refusal.refused(409, "dotted") + """, + routes__c=""" + from ..refusal import refused + + def f(): + raise refused(409, "relative") """, ) - unread = _code(r, "refused")["unread"] - assert any("the helper used as a value" in u for u in unread) - assert any("refused(*args)" in u for u in unread) + unread = _unread(r, "refused") + assert len(unread) == 6 + assert sum("the helper used as a value" in u for u in unread) == 1 + assert sum("a reference the scan cannot resolve" in u for u in unread) == 2 + assert sum("does not pass reason" in u for u in unread) == 2 # `**kw`, and nothing at all + assert any(u == "refused(*args)" for u in unread) + assert _code(r, "refused")["reasons"] == [] -def test_a_default_is_what_a_call_that_omits_it_sends(): +def test_a_default_is_not_followed_a_call_must_pass_the_reason(): r = _scan( routes___common=""" def not_found(what: str = "generic"): @@ -118,12 +172,191 @@ def test_a_default_is_what_a_call_that_omits_it_sends(): get_or_404(db, 1, what="user") get_or_404(db, 2) raise not_found("debug_session") + def g(): + raise not_found() """, ) - assert _code(r, "not_found")["reasons"] == ["debug_session", "generic", "user"] + assert _code(r, "not_found")["reasons"] == ["debug_session", "user"] + assert [u.endswith("(does not pass what)") for u in _unread(r, "not_found")] == [True, True] -def test_a_caught_exception_answers_with_its_attribute_type_every_class_it_catches(): +def test_a_positional_only_reason_passed_by_name_is_not_bound_to_it(): + r = _scan( + a=""" + def deny(reason: str, /, **params): + return HTTPException(409, {"code": "playlist", "reason": reason}) + + def f(): + raise deny("by_position") + def g(): + raise deny(reason="lands_in_params") + """ + ) + assert _code(r, "playlist")["reasons"] == ["by_position"] + assert _unread(r, "playlist") == ["deny(reason='lands_in_params') (does not pass reason)"] + + +def test_a_helper_reaching_two_sites_gives_both_its_reasons_and_one_reached_again_is_unread(): + r = _scan( + a=""" + def deny(reason: str, admin): + if admin: + return HTTPException(409, {"code": "activate", "reason": reason}) + return HTTPException(409, {"code": "playlist", "reason": reason}) + + def again(reason: str, n): + if n: + return again(reason, n - 1) + return HTTPException(409, {"code": "edit", "reason": reason}) + + def f(): + raise deny("foo", True) + def g(): + raise again("bar", 3) + """ + ) + assert _code(r, "activate")["reasons"] == ["foo"] + assert _code(r, "playlist")["reasons"] == ["foo"] + assert _code(r, "edit")["reasons"] == ["bar"] + assert any("reached again while it is followed" in u for u in _unread(r, "edit")) + + +def test_a_parameter_its_body_binds_again_is_unread(): + r = _scan( + a=""" + from typing import Literal + + def refused(status, reason: str = ""): + reason = reason or "fallback" + return HTTPException(status, {"code": "refused", "reason": reason}) + + def typed(reason: Literal["a"]): + if not reason: + reason = "b" + return HTTPException(409, {"code": "edit", "reason": reason}) + + def f(): + raise refused(409, "x") + """ + ) + assert _code(r, "refused")["reasons"] == [] + assert _unread(r, "refused") == ["reason (a parameter its body binds again)"] + assert _unread(r, "edit") == ["reason (a parameter its body binds again)"] + + +def test_a_local_is_followed_only_through_plain_assignments(): + r = _scan( + a=""" + from typing import Literal + + def f(x): + why = "first" + if x: + why = "second" if x > 1 else "third" + raise HTTPException(409, {"code": "edit", "reason": why}) + def g(svc): + hit, why = svc.fetch() + raise HTTPException(409, {"code": "playlist", "reason": why}) + def h(items): + for why in items: + pass + raise HTTPException(409, {"code": "playlist", "reason": why}) + def i(): + why: Literal["typed"] = pick() + raise HTTPException(409, {"code": "playlist", "reason": why}) + def j(items): + LABELS[why] = "NotAReason" + raise HTTPException(409, {"code": "playlist", "reason": why}) + """ + ) + assert _code(r, "edit")["reasons"] == ["first", "second", "third"] + assert _code(r, "playlist")["reasons"] == [] + assert _unread(r, "playlist") == [ + "why", + "why", + "why", + "why", + ] # unpacked, a loop, annotated, unbound + + +def test_a_call_answers_with_its_literal_return_type(): + r = _scan( + a=""" + from typing import Literal + + def why(x) -> Literal["quota", "gone"]: + return "quota" + def how(x) -> str: + return "x" + + def f(x): + raise HTTPException(409, {"code": "youtube", "reason": why(x)}) + def g(x): + raise HTTPException(409, {"code": "youtube", "reason": how(x)}) + """ + ) + assert _code(r, "youtube")["reasons"] == ["gone", "quota"] + assert _unread(r, "youtube") == ["how(x)"] + + +def test_a_module_constant_is_read_only_as_mod_name_and_only_when_bound_once(): + r = _scan( + urlguard=""" + REASON_MALFORMED = "malformed" + TWICE = "a" + TWICE = "b" + if FLAG: + LATER = "c" + """, + routes__a=""" + from app import urlguard + + BARE = "bare" + + def f(): + raise HTTPException(409, {"code": "url_blocked", "reason": urlguard.REASON_MALFORMED}) + def g(): + raise HTTPException(409, {"code": "url_blocked", "reason": urlguard.TWICE}) + def h(): + raise HTTPException(409, {"code": "url_blocked", "reason": urlguard.LATER}) + def i(): + raise HTTPException(409, {"code": "url_blocked", "reason": BARE}) + """, + ) + assert _code(r, "url_blocked")["reasons"] == ["malformed"] + assert _unread(r, "url_blocked") == ["urlguard.TWICE", "urlguard.LATER", "BARE"] + + +def test_a_name_imported_from_two_places_or_under_an_if_is_not_looked_up(): + r = _scan( + m1='R = Literal["one"]', + m2='R = Literal["two"]', + a=""" + def f(): + from app.m1 import R + def g(): + from app.m2 import R + def h(reason: R): + return HTTPException(409, {"code": "edit", "reason": reason}) + """, + b=""" + if FLAG: + from app.m1 import R + def h(reason: R): + return HTTPException(409, {"code": "playlist", "reason": reason}) + """, + c=""" + from app.m1 import R + def h(reason: R): + return HTTPException(409, {"code": "clip", "reason": reason}) + """, + ) + assert _code(r, "edit")["reasons"] == [] and _unread(r, "edit") == ["reason"] + assert _code(r, "playlist")["reasons"] == [] and _unread(r, "playlist") == ["reason"] + assert _code(r, "clip")["reasons"] == ["one"] + + +def test_a_caught_exception_answers_with_the_literal_its_class_declares_on_self(): r = _scan( errors=""" from typing import Literal @@ -136,8 +369,7 @@ def test_a_caught_exception_answers_with_its_attribute_type_every_class_it_catch self.params = params class EditError(Exception): - def __init__(self, reason: EditReason): - self.reason: EditReason = reason + reason: EditReason class Untyped(Exception): def __init__(self, reason: str): @@ -152,57 +384,80 @@ def test_a_caught_exception_answers_with_its_attribute_type_every_class_it_catch def f(): try: pass + except errors.ClipError as e: + raise HTTPException(422, {"code": "clip", "reason": e.reason}) except (errors.ClipError, errors.EditError) as e: - raise HTTPException(422, {"code": "clip", "reason": e.reason, **e.params}) + raise HTTPException(422, {"code": "clip", "reason": e.reason}) + except errors.EditError as e: + raise HTTPException(422, {"code": "clip", "reason": e.reason}) except errors.Untyped as e: raise HTTPException(422, {"code": "clip", "reason": e.reason}) except errors.Narrower as e: - raise HTTPException(422, {"code": "edit", "reason": e.reason}) + raise HTTPException(422, {"code": "clip", "reason": e.reason}) + except errors.ClipError as e: + raise HTTPException(422, {"code": "clip", "reason": e}) """, ) - assert _code(r, "clip")["reasons"] == ["bad_range", "not_media", "too_short"] - unread = _code(r, "clip")["unread"] - assert any("Untyped.reason has no Literal type" in u for u in unread) - # EditError keeps no `**params`, so its `e.params` could be anything - assert any("e.params" in u for u in unread) - assert _code(r, "edit")["reasons"] == ["bad_range", "too_short"] + assert _code(r, "clip")["reasons"] == ["bad_range", "too_short"] + assert _unread(r, "clip") == [ + "e.reason", # a tuple of classes + "e.reason (EditError.reason has no Literal type)", # declared on the class body only + "e.reason (Untyped.reason has no Literal type)", + "e.reason (Narrower.reason has no Literal type)", # a subclass: not looked up the bases + "e", # the exception itself + ] -def test_a_typed_parameter_is_its_literal_and_an_untyped_one_is_unread_when_never_called(): +def test_a_parameter_answers_with_its_literal_or_its_class_and_any_other_type_is_unread(): r = _scan( a=""" from typing import Literal Why = Literal["one", "two"] + class Err(Exception): + def __init__(self, reason: Literal["three"]): + self.reason: Literal["three"] = reason + def typed(reason: Why): return HTTPException(409, {"code": "playlist", "reason": reason}) + def inline(reason: Literal["four"]): + return HTTPException(409, {"code": "playlist", "reason": reason}) + def by_class(exc: Err): + return HTTPException(409, {"code": "playlist", "reason": exc.reason}) + def rebound(exc: Err): + exc = other() + return HTTPException(409, {"code": "playlist", "reason": exc.reason}) + def optional(reason: Literal["five"] | None = None): + return HTTPException(409, {"code": "playlist", "reason": reason}) + def quoted(reason: "Why"): + return HTTPException(409, {"code": "playlist", "reason": reason}) + def number(reason: int): + return HTTPException(409, {"code": "playlist", "reason": reason}) class C: def method(self, reason: str): return HTTPException(409, {"code": "playlist", "reason": reason}) """ ) - assert _code(r, "playlist")["reasons"] == ["one", "two"] - assert any("is not a module-level helper" in u for u in _code(r, "playlist")["unread"]) + assert _code(r, "playlist")["reasons"] == ["four", "one", "three", "two"] + unread = _unread(r, "playlist") + # a parameter bound again, `| None`, a quoted annotation, `int` + assert unread[:4] == ["exc.reason", "reason", "reason", "reason"] + assert "method(reason) is not a module-level helper" in unread[4] + assert len(unread) == 5 -def test_a_value_the_scan_cannot_follow_is_unread(): +def test_a_str_helper_nobody_calls_sends_nothing(): r = _scan( a=""" - def f(svc): - hit, code = svc.fetch() - raise HTTPException(422, {"code": "subtitle", "reason": code}) - def g(svc): - raise HTTPException(422, {"code": "subtitle", "reason": svc.reason()}) - def h(e): - raise HTTPException(422, {"code": "subtitle", "reason": e.reason}) + def helper(reason: str): + return HTTPException(409, {"code": "playlist", "reason": reason}) """ ) - assert len(_code(r, "subtitle")["unread"]) == 3 - assert _code(r, "subtitle")["reasons"] == [] + assert _code(r, "playlist") == {"reasons": [], "unread": [], "sites": 1, "reasonless": []} -def test_a_map_lookup_is_its_values_and_its_keys_must_be_what_the_index_can_be(): +def test_a_map_lookup_is_its_values_and_its_keys_must_be_exactly_what_the_index_can_be(): src = """ from typing import Literal SettingReason = Literal["too_low", "too_high"] @@ -218,13 +473,114 @@ def test_a_map_lookup_is_its_values_and_its_keys_must_be_what_the_index_can_be() return HTTPException(status, {{"code": "refused", "reason": reason, **params}}) def setting_refused(exc: InvalidSetting): - return refused(400, _MAP[exc.reason], **exc.params) + return refused(400, _MAP[{index}], **exc.params) """ - whole = _scan(a=src.format(keys='"too_low": "setting_too_low", "too_high": "setting_too_high"')) + both = '"too_low": "setting_too_low", "too_high": "setting_too_high"' + whole = _scan(a=src.format(keys=both, index="exc.reason")) assert _code(whole, "refused")["reasons"] == ["setting_too_high", "setting_too_low"] - assert whole["errors"] == [] - short = _scan(a=src.format(keys='"too_low": "setting_too_low"')) - assert len(short["errors"]) == 1 and "not what its index can be" in short["errors"][0] + assert whole["errors"] == [] and whole["unreadCodes"] == [] + for keys, index in [ + ('"too_low": "setting_too_low"', "exc.reason"), # a key short + (both + ', "gone": "setting_gone"', "exc.reason"), # a key no index can be + (both, "exc.key"), # an index the scan cannot read + ]: + r = _scan(a=src.format(keys=keys, index=index)) + assert len(r["errors"]) == 1 and "not what its index can be" in r["errors"][0] + + +def test_a_lookup_in_anything_but_a_module_dict_of_literal_keys_is_unread(): + r = _scan( + a=""" + BUILT = make() + NUMBERED = {1: "one"} + + def f(table, k): + raise HTTPException(409, {"code": "edit", "reason": table[k]}) + def g(k): + raise HTTPException(409, {"code": "edit", "reason": BUILT[k]}) + def h(k): + raise HTTPException(409, {"code": "edit", "reason": NUMBERED[k]}) + """ + ) + assert _unread(r, "edit") == ["table[k]", "BUILT[k]", "NUMBERED[k]"] + assert r["errors"] == [] + + +def test_a_code_must_be_one_literal(): + r = _scan( + a=""" + BUSY = "busy" + def f(code): + raise HTTPException(409, {"code": "busy"}) + def g(code): + raise HTTPException(409, {"code": code}) + def h(): + raise HTTPException(409, {"code": BUSY}) + def i(x): + raise HTTPException(409, {"code": "a" if x else "b"}) + def j(x): + raise HTTPException(409, {"code": "a" if x else None}) + def k(key): + raise HTTPException(409, {"code": "edit", key: "late"}) + """ + ) + assert r["codes"]["busy"]["reasonless"] == ["a.py:4"] + assert set(r["codes"]) == {"busy"} + assert len(r["unreadCodes"]) == 5 + assert r["unreadCodes"][-1].endswith("(a key the scan cannot read)") + + +def test_a_spread_is_clean_only_from_an_untouched_kwargs_no_call_puts_a_code_in(): + def unread_codes(helper: str, call: str) -> list[str]: + return _scan(a=helper + "\n" + f"def caller(x, more):\n raise {call}\n")["unreadCodes"] + + helper = """ +def deny(status, reason: str, **params): + return HTTPException(status, {"code": "refused", "reason": reason, **params}) +""" + assert unread_codes(helper, 'deny(409, "a", limit=3)') == [] + assert unread_codes(helper, 'deny(409, "a", code=x)') # the spread puts its own code + assert unread_codes(helper, 'deny(409, "a", **more)') # a spread nothing vouches for + assert unread_codes(helper + "pick = deny\n", 'deny(409, "a")') # passed around + posonly = helper.replace("reason: str,", "reason: str, /,") # `reason=` lands in params + assert unread_codes(posonly, 'deny(409, "a")') + mutated = helper.replace(" return", ' params["reason"] = "zz"\n return') + assert unread_codes(mutated, 'deny(409, "a")') + renamed = helper.replace("**params})", "**{k.lstrip('_'): v for k, v in params.items()}})") + assert unread_codes(renamed, 'deny(409, "a")') + filtered = helper.replace("**params})", "**{k: v for k, v in params.items() if v}})") + assert unread_codes(filtered, 'deny(409, "a")') == [] + + +def test_a_caught_exceptions_params_are_clean_only_when_kept_from_its_init(): + def unread_codes(errors: str, construct: str = 'Err("a", limit=1)') -> list[str]: + return _scan( + errors=errors, + a=f""" + from app.errors import Err + def f(): + raise {construct} + def g(): + try: + pass + except Err as e: + raise HTTPException(422, {{"code": "clip", "reason": e.reason, **e.params}}) + """, + )["unreadCodes"] + + kept = """ + from typing import Literal + class Err(Exception): + def __init__(self, reason: Literal["a"], **params: float): + super().__init__(f"{reason} {params}") + self.reason: Literal["a"] = reason + self.params = params + """ + assert unread_codes(kept) == [] + assert unread_codes(kept, 'Err("a", code="other")') + assert unread_codes(kept + "\n class Sub(Err):\n pass\n") + assert unread_codes(kept.replace("self.params = params", "self.params = {'code': 'x'}")) + assert unread_codes(kept + "\n def more(self):\n self.params = {}\n") def test_a_detail_built_up_in_steps_is_read_through_its_variable(): @@ -235,37 +591,58 @@ def test_a_detail_built_up_in_steps_is_read_through_its_variable(): if user is not None: detail |= {"email": user.email, "reason": "suspended"} detail |= {k: v for k, v in extra.items() if v is not None} + detail["email"] = "x" return HTTPException(status, detail) - def g(more): - detail = {"code": "login"} + def g(x): + detail = {"code": "login"} if x else {"code": "login", "reason": "other"} detail["reason"] = "late" - detail.update(more) - detail |= more - return HTTPException(401, detail) + return HTTPException(401, detail=detail) """ ) - entry = _code(r, "login") - assert entry["reasons"] == ["invalid", "late", "suspended"] - assert sum("may carry a reason" in u for u in entry["unread"]) == 2 + assert r["unreadCodes"] == [] + assert _code(r, "login")["reasons"] == ["invalid", "late", "other", "suspended"] -def test_a_code_must_be_one_the_scan_can_read(): - r = _scan( - a=""" - BUSY = "busy" - def f(code): - raise HTTPException(409, {"code": BUSY}) - def g(code): - raise HTTPException(409, {"code": code}) - """ +def test_a_detail_changed_or_sent_any_other_way_is_an_unread_code(): + changes = [ + "detail.update(more)", + "detail |= more", + 'detail |= {"code": "other"}', + 'detail["code"] = "other"', + 'del detail["reason"]', + "detail[key] = more", + 'detail = {**detail, "reason": "b"}', + "detail = rebuilt()", + "copy = detail", + "add_extra(detail)", + "return detail", + 'return {**detail, "reason": "b"}', + 'return dict(detail, reason="b")', + ] + for change in changes: + r = _scan( + a=f""" + def f(key, more): + detail = {{"code": "login", "reason": "a"}} + {change} + raise HTTPException(401, detail) + """ + ) + assert r["unreadCodes"], change + assert "login" not in r["codes"], change + held = _scan(a='class C:\n detail = {"code": "login"}\n') + assert ( + held["unreadCodes"] + and "a detail put where the scan cannot follow it" in held["unreadCodes"][0] ) - assert "busy" in r["codes"] and r["codes"]["busy"]["reasonless"] - assert len(r["unreadCodes"]) == 1 and r["unreadCodes"][0].endswith(": code") def test_the_data_files_are_excluded_by_name_only(): - r = _scan(plex__tmdb='x = {"code": "HU", "name": "Hungary"}', plex__other='y = {"code": "HU"}') + r = _scan( + plex__tmdb='def f():\n return {"code": "HU", "name": "Hungary"}', + plex__other='def f():\n return {"code": "HU"}', + ) assert r["excluded"] == {"plex/tmdb.py": 1} assert "HU" in r["codes"] @@ -275,4 +652,4 @@ def test_the_app_scans(): frontend's contract judges what is read).""" result = scan_app() assert {"refused", "not_found", "login", "clip"} <= set(result["codes"]) - assert result["errors"] == [] + assert result["errors"] == [] and result["unreadCodes"] == [] diff --git a/frontend/src/i18n/backendIds.contract.test.ts b/frontend/src/i18n/backendIds.contract.test.ts index 436d5ce6..e060932c 100644 --- a/frontend/src/i18n/backendIds.contract.test.ts +++ b/frontend/src/i18n/backendIds.contract.test.ts @@ -288,9 +288,7 @@ describe("backend ids have their names in both languages", () => { expect(reasoned.flatMap((code) => scan.codes[code]?.reasonless ?? [])).toEqual([]); }); - it("read every refusal's reasons (a scan that matches nothing is green over nothing)", () => { - const coded = [...Object.keys(REFUSAL_FAMILIES), ...Object.keys(REFUSAL_BY_REASON)]; - expect(Object.keys(reasonsOf).sort()).toEqual(coded.sort()); + it("every code whose reason picks the sentence is read with a reason (a scan that matches nothing is green over nothing)", () => { expect(Object.entries(reasonsOf).filter(([, rs]) => rs.length === 0)).toEqual([]); }); From 31a75f49856c51ead21e6c48a85374ace1d7d441 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 31 Oct 2026 23:54:55 +0100 Subject: [PATCH 5/6] fix(i18n): write each refusal where it is sent, and read only that form A refusal's detail now stands where it is sent (HTTPException's detail, a JSONResponse's "detail") or is returned by a factory that only ever is, with any spread before its code and reason, so no spread can replace them. The scan follows no variable and no kwargs any more; what it cannot read in a reason (a lambda's or comprehension's name, a nonlocal or global, a local that reaches itself, a constant or map changed elsewhere, an app import it cannot resolve) is reported. The detail guard reads the same slots and fails on anything but a code dict or a factory's call. --- backend/app/refusal.py | 18 +- backend/app/routes/auth.py | 18 +- backend/app/routes/downloads.py | 5 +- backend/tests/refusal_sites.py | 509 +++++++++++++--------------- backend/tests/test_http_details.py | 178 ++++------ backend/tests/test_refusal_sites.py | 333 ++++++++++++------ 6 files changed, 546 insertions(+), 515 deletions(-) diff --git a/backend/app/refusal.py b/backend/app/refusal.py index d996cff4..1e47f547 100644 --- a/backend/app/refusal.py +++ b/backend/app/refusal.py @@ -7,11 +7,13 @@ whatever sentence the route happened to hand-write. `refused` is the generic fam (`errors.refused.`, the params interpolated into the sentence); the older specific codes (`not_found`, `youtube`, `playlist`, the admin refusals, …) stay, since each is already translated. -A test (`test_http_details.py`) reads every `HTTPException` and `JSONResponse` detail in the app and -fails on a sentence, a dict without a `code`, or none at all. `tests/refusal_sites.py` reads every -`{"code": …}` detail from the syntax tree and follows each reason along a closed list of shapes: a -literal, a `Literal` type on the parameter or attribute it travels in, every call of a helper like -`refused` below, a map's values; anything else is reported, never guessed. The frontend's contract test +A test (`test_http_details.py`) reads every place an `HTTPException` or a `JSONResponse` sends a detail +and fails on one the client cannot say: a sentence, a dict without a `code`, a variable, none at all. +`tests/refusal_sites.py` reads every `{"code": …}` detail from the syntax tree, written where it is sent +(or returned by a factory that only ever is), with any `**` spread before its `code` and `reason`, and +follows each reason along a closed list of shapes: a literal, a `Literal` type on the parameter or +attribute it travels in, every call of a helper like `refused` below, a map's values; anything else is +reported, never guessed. The frontend's contract test (`i18n/backendIds.contract.test.ts`) runs it and requires each code in one record of `lib/api/refusalCodes.ts`, and each reason's sentence in both languages; a code or reason it cannot follow fails it, unless no screen shows that code. So the rule is enforced, not described. @@ -24,9 +26,9 @@ from app.sysconfig import InvalidSetting, SettingReason def refused(status: int, reason: str, **params: object) -> HTTPException: """The refusal to raise: `raise refused(409, "view_name_taken", name=name)`. `params` travel in - the detail for the sentence to interpolate (`{{name}}`); keep them to values the reader may see, - and never a `code`: it would replace this one (the refusal scan reads such a call as an unread code).""" - return HTTPException(status_code=status, detail={"code": "refused", "reason": reason, **params}) + the detail for the sentence to interpolate (`{{name}}`); keep them to values the reader may see. + They are spread first, so a `code` or `reason` among them could never replace these two.""" + return HTTPException(status_code=status, detail={**params, "code": "refused", "reason": reason}) # `sysconfig.InvalidSetting.reason` → the refusal it answers as. A dict of literals, so the refusal diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 92c6a4ef..b90791dd 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -1001,15 +1001,15 @@ def _login_refused( `email_verified` is false, and vice versa — the SPA renders whichever is actually outstanding rather than assuming a sequence. """ - detail: dict = {"code": "login", "reason": reason} - if user is not None: - detail |= { - "email": user.email, - "email_verified": user.email_verified, - "approved": user.is_active, - } - detail |= {k: v for k, v in extra.items() if v is not None} - return HTTPException(status_code=status, detail=detail) + account = ( + {"email": user.email, "email_verified": user.email_verified, "approved": user.is_active} + if user is not None + else {} + ) + given = {k: v for k, v in extra.items() if v is not None} + return HTTPException( + status_code=status, detail={**account, **given, "code": "login", "reason": reason} + ) @router.post("/password-login") diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index b7e672d8..380bb337 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -359,8 +359,9 @@ def enqueue_clip( job = service.enqueue_clip(db, user.id, spec, display_name=display_name) except clipmod.ClipError as e: status = 404 if e.reason == "not_found" else 422 - detail = {"code": "clip", "reason": e.reason, **e.params} - raise HTTPException(status_code=status, detail=detail) from e + raise HTTPException( + status_code=status, detail={**e.params, "code": "clip", "reason": e.reason} + ) from e except quota.QuotaExceeded as e: raise HTTPException(status_code=422, detail=quota_detail(e)) from e db.commit() diff --git a/backend/tests/refusal_sites.py b/backend/tests/refusal_sites.py index 0fbe3bbd..9f8453e9 100644 --- a/backend/tests/refusal_sites.py +++ b/backend/tests/refusal_sites.py @@ -2,12 +2,19 @@ with every reason its code can be sent with. The frontend's contract (`i18n/backendIds.contract.test.ts`) runs this and requires a sentence for each -code and reason in both languages. A regex over the source read only the shapes it was written for, and a -scan that tries to follow every shape opens a silent gap with each one it learns. So this one reads a -CLOSED list, the shapes the app writes, and reports everything else, never guesses: +code and reason in both languages. A scan that tries to follow every shape opens a silent gap with each one +it learns, and following how a value flows through Python (variables, scopes, aliases, a dict changed in +place) never closes. So the CODE keeps to a narrow form, and this reads a CLOSED list of it, reporting +everything else, never guessing: +- where a detail stands: where it is sent — `HTTPException`'s second argument or `detail=`, the `"detail"` + value of a `JSONResponse`'s content dict — through the arms of `a if c else b`, or as what a module-level + factory returns when every return of it is a detail and every reference to it is a call sent as one. A + detail held in a variable is not followed; +- the code: one literal. A `**` spread, or a key that is not a literal, stands BEFORE a literal `code` and + `reason`, both of which the detail then carries: the later key wins, so no spread can change them; - a reason: a string literal (`None` sends none: the site counts as reasonless); both arms of - `a if c else b`; a local variable bound only by plain `x = …`; a module constant reached as `mod.NAME`; + `a if c else b`; a function's local bound only by plain `x = …`; a module constant reached as `mod.NAME`; a call's `Literal` return annotation; - a parameter typed with a `Literal` (basedpyright holds every value passed in), or a `str` one of a module-level helper (`refused`, `not_found`), followed to every call, each of which must pass it by @@ -15,17 +22,15 @@ CLOSED list, the shapes the app writes, and reports everything else, never guess - an attribute: `e.x` of the exception an `except C as e` catches, or of a parameter typed `C`, when a method of `C` declares `self.x: `; - a map lookup (`_SETTING_REASONS[exc.reason]`): the map's values, and its keys must be exactly what the - index can be; -- the code: one literal, never changed after. A `**` spread in the detail is the `**kwargs` of a helper or - of an exception's `__init__` that takes `reason` by name, used for nothing but being spread, and no call - of it passes `code=` or a `**` that is not such a spread itself. The detail's variable is changed only by - `|= {…}` or `["reason"] = …`, and goes nowhere but into an `HTTPException` / `JSONResponse`. + index can be. -A name bound twice at module level, inside a module-level `if`/`try`/loop, or imported from two places is -not looked up. A reference named like a followed helper that the scan cannot resolve (`import app.x`, a -relative import) is unread. Anything else is UNREAD, or an unread code, and the contract fails on it unless -no screen shows that code. Stdlib only: it runs on the host, beside the frontend's tests -(`python -I backend/tests/refusal_sites.py` → JSON). +A name bound twice at module level, inside a module-level `if`/`try`/loop, named by a `global`, or imported +from two places is not looked up; nor is a local a lambda or a comprehension binds, or one a `nonlocal` or +`global` names. A module constant or map that anything stores into, deletes from or (a map) uses but by +`map[k]` or a read method is unread. A reference named like a followed helper that the scan cannot resolve +(`import app.x`, a relative import, an app module that does not bind it) is unread. Anything else is UNREAD, +or an unread code, and the contract fails on it unless no screen shows that code. Stdlib only: it runs on +the host, beside the frontend's tests (`python -I backend/tests/refusal_sites.py` → JSON). """ import ast @@ -43,13 +48,13 @@ NOT_REFUSALS = { "plex/tmdb.py": "a production country's ISO code", } -# What a detail held in a variable may be handed to: the response it is sent in, nothing that could change it. -SENDERS = {"HTTPException", "JSONResponse"} -READS = {"get", "items", "keys", "values", "copy"} +# What a map may be used by besides `map[k]`: a method that only reads it. +READS = {"get", "items", "keys", "values"} Node = ast.AST FUNCS = (ast.FunctionDef, ast.AsyncFunctionDef) SCOPES = (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda, ast.ClassDef) +COMPREHENSIONS = (ast.ListComp, ast.SetComp, ast.DictComp, ast.GeneratorExp) COMPOUND = ( ast.If, ast.Try, @@ -82,7 +87,10 @@ class Module: # local name → (module, symbol or None for the module) imports: dict[str, tuple[str, str | None]] top: dict[str, Node] # module-level bindings: def, class, or the assignment - ambiguous: set[str] # bound more than once, conditionally, or imported from two places + ambiguous: set[ + str + ] # bound more than once, conditionally, by a `global`, or imported from two places + slots: set[int] # the ids of the nodes sent as a detail def _own_scope(node: Node) -> Iterator[Node]: @@ -120,6 +128,39 @@ def _binds(node: Node) -> set[str]: return set() +def _sender(call: ast.Call) -> str | None: + return getattr(call.func, "id", None) or getattr(call.func, "attr", None) + + +def detail_slots(tree: Node) -> Iterator[tuple[ast.Call, Node | None]]: + """Every place a detail is sent from, with what it holds: `HTTPException`'s second argument or + `detail=` (None: sent without one), and the `"detail"` value of a `JSONResponse`'s content dict (a + content without one is an answer, not a refusal).""" + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + kw = {k.arg: k.value for k in node.keywords} + if _sender(node) == "HTTPException": + yield node, kw.get("detail", node.args[1] if len(node.args) > 1 else None) + elif _sender(node) == "JSONResponse": + content = kw.get("content", node.args[0] if node.args else None) + if isinstance(content, ast.Dict): + for k, v in zip(content.keys, content.values, strict=True): + if isinstance(k, ast.Constant) and k.value == "detail": + yield node, v + + +def _code_dict(node: Node) -> bool: + """A `{"code": …}` dict, or a `dict(code=…)` call.""" + if isinstance(node, ast.Dict): + return any(isinstance(k, ast.Constant) and k.value == "code" for k in node.keys) + return ( + isinstance(node, ast.Call) + and getattr(node.func, "id", None) == "dict" + and any(k.arg == "code" for k in node.keywords) + ) + + def _module_name(path: str) -> str: parts = path.removesuffix(".py").split("/") if parts[-1] == "__init__": @@ -133,6 +174,7 @@ def _load(sources: dict[str, str]) -> dict[str, Module]: tree = ast.parse(src) parents = {child: node for node in ast.walk(tree) for child in ast.iter_child_nodes(node)} origins: dict[str, set[tuple[str, str | None]]] = {} + ambiguous: set[str] = set() for node in ast.walk(tree): if isinstance(node, ast.ImportFrom) and node.module and node.level == 0: for a in node.names: @@ -141,9 +183,11 @@ def _load(sources: dict[str, str]) -> dict[str, Module]: for a in node.names: if a.asname: origins.setdefault(a.asname, set()).add((a.name, None)) + elif isinstance(node, ast.Global): + ambiguous |= set(node.names) top: dict[str, Node] = {} count: dict[str, int] = {} - ambiguous = {name for name, o in origins.items() if len(o) > 1} + ambiguous |= {name for name, o in origins.items() if len(o) > 1} for stmt in tree.body: if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): top[stmt.name] = stmt @@ -160,8 +204,9 @@ def _load(sources: dict[str, str]) -> dict[str, Module]: ambiguous.add(name) ambiguous |= {name for name, c in count.items() if c > 1} imports = {name: next(iter(o)) for name, o in origins.items() if name not in ambiguous} + slots = {id(d) for _, d in detail_slots(tree) if d is not None} name = _module_name(path) - mods[name] = Module(path, name, tree, parents, imports, top, ambiguous) + mods[name] = Module(path, name, tree, parents, imports, top, ambiguous, slots) return mods @@ -171,7 +216,9 @@ class Scan: self.errors: list[str] = [] self._following: set[tuple[str, str, str]] = set() self._followed: dict[tuple[str, str, str], Res] = {} - self._clean: dict[int, bool] = {} + self._resolving: set[int] = set() + self._factory: dict[int, bool] = {} + self._changed: dict[int, bool] = {} # --- names --------------------------------------------------------------------------------------- @@ -195,26 +242,31 @@ class Scan: def lookup_expr(self, m: Module, e: Node) -> tuple[Module, Node] | tuple[str, None] | None: if isinstance(e, ast.Name): - return None if self.local(m, e) else self.lookup(m, e.id) + return None if self.binder(m, e) is not None else self.lookup(m, e.id) if isinstance(e, ast.Attribute): base = self.lookup_expr(m, e.value) if base and base[1] is None and isinstance(base[0], str): return self.lookup(self.mods[base[0]], e.attr) return None - def local(self, m: Module, e: ast.Name) -> bool: - """`e` names a parameter or a variable of a function it sits in.""" - fn = self.enclosing(m, e, FUNCS) - while fn is not None: - a = fn.args - if e.id in { - p.arg for p in [*a.posonlyargs, *a.args, *a.kwonlyargs, a.vararg, a.kwarg] if p - }: - return True - if self.assignments(fn, e.id): - return True - fn = self.enclosing(m, fn, FUNCS) - return False + def binder(self, m: Module, e: ast.Name) -> Node | None: + """The innermost scope `e` is bound in: a function (a parameter, or a binding in its own body), a + lambda (a parameter), a comprehension (its target); None: a module-level name.""" + node = m.parents.get(e) + while node is not None: + if isinstance(node, (*FUNCS, ast.Lambda)): + a = node.args + params = [*a.posonlyargs, *a.args, *a.kwonlyargs, a.vararg, a.kwarg] + if e.id in {p.arg for p in params if p}: + return node + if isinstance(node, FUNCS) and self.assignments(node, e.id): + return node + elif isinstance(node, COMPREHENSIONS) and any( + e.id in _stored(g.target) for g in node.generators + ): + return node + node = m.parents.get(node) + return None def at(self, m: Module, e: Node) -> str: return f"{m.path}:{getattr(e, 'lineno', '?')}: {ast.unparse(e)}" @@ -269,17 +321,56 @@ class Scan: root = node while isinstance(root, ast.Attribute): root = root.value - if not isinstance(root, ast.Name) or self.local(m, root): + if not isinstance(root, ast.Name) or self.binder(m, root) is not None: return False if root.id in m.ambiguous or (root.id not in m.top and root.id not in m.imports): return True - if root is node or root.id in m.top: + if root.id in m.top: return False + if root is node: # imported from an app module that does not bind it where the scan can see + mod = m.imports[root.id][0] + return (mod == "app" or mod.startswith("app.")) and self.lookup(m, root.id) is None found = self.lookup(m, root.id) return ( found is not None and found[1] is None ) # an app module whose dotted path went nowhere + def changed(self, m: Module, stmt: Node, name: str, *, only_read: bool) -> bool: + """Something in the app stores into, deletes or rebinds the module-level `name` that `stmt` binds, + or (`only_read`, a map) uses it but by `map[k]` or a method that only reads it.""" + key = id(stmt) * 2 + only_read + if key in self._changed: + return self._changed[key] + own = stmt.targets if isinstance(stmt, ast.Assign) else [getattr(stmt, "target", None)] + out = False + for cm in self.mods.values(): + for node in ast.walk(cm.tree): + if out: + break + if not ( + isinstance(node, ast.Name) + and node.id == name + or isinstance(node, ast.Attribute) + and node.attr == name + ) or (cm is m and node in own): + continue + found = self.lookup_expr(cm, node) + if not (found and found[0] is m and found[1] is stmt): + continue + if not isinstance(node.ctx, ast.Load): + out = True + elif only_read: + p = cm.parents.get(node) + out = not ( + isinstance(p, ast.Subscript) + and p.value is node + and isinstance(p.ctx, ast.Load) + or isinstance(p, ast.Attribute) + and p.attr in READS + ) + self._changed[key] = out + return out + # --- types --------------------------------------------------------------------------------------- def ann_values(self, m: Module, ann: Node | None, depth: int = 0) -> set[str] | None: @@ -352,30 +443,33 @@ class Scan: return Res(unread=[self.at(m, e)]) def resolve_name(self, m: Module, e: ast.Name) -> Res: - fn = self.enclosing(m, e, FUNCS) - while fn is not None: - assert isinstance(fn, FUNCS) - if self.caught_as(m, e, e.id) is not None: - return Res(unread=[self.at(m, e)]) # the exception itself, not a reason - a = fn.args - params = {p.arg: p for p in [*a.posonlyargs, *a.args, *a.kwonlyargs]} - assigned = self.assignments(fn, e.id) - if e.id in params: - if assigned: - return Res(unread=[f"{self.at(m, e)} (a parameter its body binds again)"]) - return self.resolve_param(m, fn, params[e.id], e) + if self.caught_as(m, e, e.id) is not None: + return Res(unread=[self.at(m, e)]) # the exception itself, not a reason + fn = self.binder(m, e) + if fn is None: + return Res(unread=[self.at(m, e)]) + if not isinstance(fn, FUNCS): + return Res(unread=[f"{self.at(m, e)} (bound by a lambda or a comprehension)"]) + if any(isinstance(n, (ast.Nonlocal, ast.Global)) and e.id in n.names for n in ast.walk(fn)): + return Res(unread=[f"{self.at(m, e)} (named by a nonlocal or a global)"]) + a = fn.args + params = {p.arg: p for p in [*a.posonlyargs, *a.args, *a.kwonlyargs]} + assigned = self.assignments(fn, e.id) + if e.id in params: if assigned: - out = Res() - for s in assigned: - if isinstance(s, ast.Assign) and all( - isinstance(t, ast.Name) for t in s.targets - ): - out = out | self.resolve(m, s.value) - else: - out = out | Res(unread=[self.at(m, e)]) - return out - fn = self.enclosing(m, fn, FUNCS) - return Res(unread=[self.at(m, e)]) + return Res(unread=[f"{self.at(m, e)} (a parameter its body binds again)"]) + return self.resolve_param(m, fn, params[e.id], e) + out = Res(unread=[] if assigned else [self.at(m, e)]) # `*args`, `**kwargs` + for s in assigned: + if not (isinstance(s, ast.Assign) and all(isinstance(t, ast.Name) for t in s.targets)): + out = out | Res(unread=[self.at(m, e)]) + elif id(s) in self._resolving: + out = out | Res(unread=[f"{self.at(m, e)} (reached again while it is resolved)"]) + else: + self._resolving.add(id(s)) + out = out | self.resolve(m, s.value) + self._resolving.discard(id(s)) + return out def resolve_param(self, m: Module, fn: Node, param: ast.arg, e: Node) -> Res: v = self.ann_values(m, param.annotation) @@ -443,10 +537,11 @@ class Scan: found and isinstance(found[0], Module) and isinstance(found[1], (ast.Assign, ast.AnnAssign)) + and isinstance(found[1].value, ast.Constant) ): - stmt = found[1] - if isinstance(stmt.value, ast.Constant): - return self.resolve(found[0], stmt.value) + if self.changed(found[0], found[1], e.attr, only_read=False): + return Res(unread=[f"{self.at(m, e)} (bound again or deleted elsewhere)"]) + return self.resolve(found[0], found[1].value) return Res(unread=[self.at(m, e)]) def resolve_lookup(self, m: Module, e: ast.Subscript) -> Res: @@ -462,6 +557,9 @@ class Scan: isinstance(k, ast.Constant) and isinstance(k.value, str) for k in table.keys ): return Res(unread=[self.at(m, e)]) + name = e.value.attr if isinstance(e.value, ast.Attribute) else getattr(e.value, "id", "") + if self.changed(found[0], found[1], name, only_read=True): + return Res(unread=[f"{self.at(m, e)} (the map is changed after it is built)"]) out = Res() for v in table.values: out = out | self.resolve(found[0], v) @@ -474,119 +572,57 @@ class Scan: ) return out - # --- spreads ------------------------------------------------------------------------------------- - - def clean_spread(self, m: Module, at: Node, src: Node) -> bool: - """`**src` cannot carry a `reason` or a `code`: it is a function's `**kwargs` (or a caught - exception's `e.params` kept from its `__init__`'s), or `{k: … for k, v in that.items()}`: a - value may change, a key may not.""" - if isinstance(src, ast.DictComp): - g = src.generators - if len(g) != 1 or not isinstance(g[0].target, ast.Tuple) or len(g[0].target.elts) != 2: - return False - k, v = g[0].target.elts - it = g[0].iter - return ( - isinstance(k, ast.Name) - and isinstance(v, ast.Name) - and getattr(src.key, "id", None) == k.id - and isinstance(it, ast.Call) - and isinstance(it.func, ast.Attribute) - and it.func.attr == "items" - and not it.args - and self.clean_spread(m, at, it.func.value) - ) - if isinstance(src, ast.Attribute) and isinstance(src.value, ast.Name): - typed = self.typed(m, at, src.value.id) - return bool(typed) and all(self.kept(cm, cls, src.attr) for cm, cls in typed or []) - if isinstance(src, ast.Name): - fn = self.enclosing(m, at, FUNCS) - return ( - fn is not None - and fn.args.kwarg is not None - and src.id == fn.args.kwarg.arg - and self.kwargs_clean(m, fn, fn) - ) - return False - - def kept(self, cm: Module, cls: ast.ClassDef, attr: str) -> bool: - """`self.attr` is only ever the `**kwargs` of the class's `__init__`, and that one is clean.""" - init = next( - (s for s in cls.body if isinstance(s, ast.FunctionDef) and s.name == "__init__"), None - ) - if init is None or init.args.kwarg is None: - return False - stores = [ - n - for n in ast.walk(cls) - if isinstance(n, ast.Attribute) - and n.attr == attr - and getattr(n.value, "id", None) == "self" - and not isinstance(n.ctx, ast.Load) - ] - kept = [ - s - for s in stores - if isinstance(p := cm.parents.get(s), ast.Assign) - and getattr(p.value, "id", None) == init.args.kwarg.arg - and self.enclosing(cm, p, FUNCS) is init - ] - return len(stores) == 1 and len(kept) == 1 and self.kwargs_clean(cm, init, cls) - - def kwargs_clean(self, m: Module, fn: Node, target: Node) -> bool: - """`fn`'s `**kwargs` holds no `reason` (`fn` takes it by name) and no `code`: it is used for - nothing but `**kwargs`, `kwargs.items()`, `self.x = kwargs` or an f-string, and every call of - `target` (the helper, or the class `fn` is the `__init__` of) passes no `code=` and no unclean - `**`.""" - assert isinstance(fn, FUNCS) and fn.args.kwarg is not None - if id(fn) in self._clean: - return self._clean[id(fn)] - self._clean[id(fn)] = False # a call that reaches it again while it is checked is not clean - kwarg = fn.args.kwarg.arg - clean = "reason" in {a.arg for a in [*fn.args.args, *fn.args.kwonlyargs]} and not ( - self.assignments(fn, kwarg) - ) - for n in _own_scope(fn): - if not clean: - break - if isinstance(n, ast.Name) and n.id == kwarg: - p = m.parents.get(n) - clean = ( - isinstance(p, ast.FormattedValue) # printed in a message - or isinstance(p, ast.keyword) - and p.arg is None - or isinstance(p, ast.Dict) - and any(k is None and v is n for k, v in zip(p.keys, p.values, strict=True)) - or isinstance(p, ast.Attribute) - and p.attr == "items" - and isinstance(m.parents.get(p), ast.Call) - or isinstance(p, ast.Assign) - and p.value is n - and all( - isinstance(t, ast.Attribute) and getattr(t.value, "id", None) == "self" - for t in p.targets - ) - ) - refs = list(self.references(m, target)) if clean else [] - for cm, node, call, resolved in refs: - if call is not None: - clean = not any( - k.arg == "code" or k.arg is None and not self.clean_spread(cm, call, k.value) - for k in call.keywords - ) - elif not resolved or not isinstance(target, ast.ClassDef): - clean = False # a helper passed around, or a name the scan cannot resolve - else: - # `except C`, an annotation: not a construction; a subclass is built some other way - cls = self.enclosing(cm, node, ast.ClassDef) - clean = not (cls is not None and any(node in ast.walk(b) for b in cls.bases)) - if not clean: - break - self._clean[id(fn)] = clean - return clean - # --- sites --------------------------------------------------------------------------------------- + def is_detail(self, node: Node | None) -> bool: + if isinstance(node, ast.IfExp): + return self.is_detail(node.body) and self.is_detail(node.orelse) + return _code_dict(node) + + def factory(self, m: Module, fn: Node) -> bool: + """A module-level function each return of which is a detail, and each reference to which is a + call sent as one (`quota_detail`).""" + assert isinstance(fn, FUNCS) + if id(fn) in self._factory: + return self._factory[id(fn)] + returns = [n for n in _own_scope(fn) if isinstance(n, ast.Return)] + ok = ( + m.top.get(fn.name) is fn # a name bound twice: its calls are unresolved below + and bool(returns) + and all(self.is_detail(r.value) for r in returns) + ) + for cm, _, call, resolved in self.references(m, fn) if ok else []: + ok = resolved and id(call) in cm.slots # a bare reference is no call: never sent + if not ok: + break + self._factory[id(fn)] = ok + return ok + + def placed(self, m: Module, d: Node) -> bool: + """`d` is sent as a detail: from a sender's slot, through the arms of `a if c else b`, or as what a + factory returns.""" + holder, p = d, m.parents.get(d) + while isinstance(p, ast.IfExp) and holder is not p.test: + holder, p = p, m.parents.get(p) + if id(holder) in m.slots: + return True + fn = self.enclosing(m, p, FUNCS) if isinstance(p, ast.Return) else None + return fn is not None and self.factory(m, fn) + + def sayable(self, m: Module, d: Node | None) -> bool: + """What a slot holds is a detail the client can say: a code dict, or a factory's call.""" + if isinstance(d, ast.IfExp): + return self.sayable(m, d.body) and self.sayable(m, d.orelse) + if _code_dict(d): + return True + found = self.lookup_expr(m, d.func) if isinstance(d, ast.Call) else None + return bool( + found + and isinstance(found[0], Module) + and isinstance(found[1], FUNCS) + and self.factory(found[0], found[1]) + ) + def entries(self, d: Node) -> list[tuple[str | None, Node]]: """A dict's (key, value) pairs: None for a `**` spread, "?" for a key that is not a literal.""" if isinstance(d, ast.Dict): @@ -610,14 +646,16 @@ class Scan: for v in (v for k, v in items if k == "code"): code = code | self.resolve(m, v) problems = [ - f"{self.at(m, v)} (a key the scan cannot read)" for k, v in items if k == "?" - ] + [ - f"{self.at(m, v)} (a spread that may carry the code)" + f"{self.at(m, v)} (a spread in dict())" for k, v in items - if k is None and not self.clean_spread(m, d, v) + if k is None and isinstance(d, ast.Call) + ] + [ + f"{self.at(m, v)} (a spread or a computed key the code and the reason do not follow)" + for i, (k, v) in enumerate(items) + if k in (None, "?") and not {"code", "reason"} <= {k2 for k2, _ in items[i + 1 :]} ] - res, has_reason, changed = self.mutations(m, d) - problems += changed + if not self.placed(m, d): + problems.append(f"{self.at(m, d)} (a detail put where the scan cannot follow it)") if code.unread or code.none or len(code.values) != 1 or problems: out["unreadCodes"] += problems or [self.at(m, d)] return @@ -625,6 +663,7 @@ class Scan: next(iter(code.values)), {"reasons": set(), "unread": [], "sites": 0, "reasonless": []} ) entry["sites"] += 1 + res, has_reason = Res(), False for k, v in items: if k == "reason": res, has_reason = res | self.resolve(m, v), True @@ -633,81 +672,17 @@ class Scan: entry["reasons"] |= res.values entry["unread"] += res.unread - def mutations(self, m: Module, d: Node) -> tuple[Res, bool, list[str]]: - """A detail put in a variable: what `|= {…}` and `["reason"] = …` add, and every other use that - could change it or send it elsewhere.""" - holder, stmt = d, m.parents.get(d) - while isinstance(stmt, ast.IfExp) and holder is not stmt.test: - holder, stmt = stmt, m.parents.get(stmt) - if not (isinstance(stmt, (ast.Assign, ast.AnnAssign)) and stmt.value is holder): - return Res(), False, [] - targets = stmt.targets if isinstance(stmt, ast.Assign) else [stmt.target] - fn = self.enclosing(m, d, FUNCS) - if fn is None or len(targets) != 1 or not isinstance(targets[0], ast.Name): - return ( - Res(), - False, - [f"{self.at(m, stmt)} (a detail put where the scan cannot follow it)"], - ) - name = targets[0].id - res, found, problems = Res(), False, [] - for n in _own_scope(fn): - if n is stmt: - continue - if name in _binds(n): - if not (isinstance(n, ast.AugAssign) and isinstance(n.op, ast.BitOr)): - problems.append(f"{self.at(m, n)} (binds the detail's variable again)") - elif isinstance(n.value, ast.Dict): - for k, v in self.entries(n.value): - if k == "reason": - res, found = res | self.resolve(m, v), True - elif k in ("code", "?") or k is None and not self.clean_spread(m, n, v): - problems.append(f"{self.at(m, v)} (may change the code)") - elif not self.clean_spread(m, n, n.value): - problems.append(f"{self.at(m, n)} (may change the code)") - continue - if not (isinstance(n, ast.Name) and n.id == name and isinstance(n.ctx, ast.Load)): - continue - p = m.parents.get(n) - if isinstance(p, ast.AugAssign) and p.target is n: - continue - if isinstance(p, ast.Subscript) and p.value is n: - key = p.slice.value if isinstance(p.slice, ast.Constant) else None - assign = m.parents.get(p) - if isinstance(p.ctx, ast.Load): + def other_details(self) -> list[tuple[str, int, str | None]]: + """Each slot that sends what the client cannot say — nothing, or anything but a code dict or a + factory's call: (its file, its line, the function it is sent from).""" + out = [] + for m in self.mods.values(): + for call, d in detail_slots(m.tree): + if self.sayable(m, d): continue - if ( - key == "reason" - and isinstance(p.ctx, ast.Store) - and isinstance(assign, ast.Assign) - and assign.targets == [p] - ): - res, found = res | self.resolve(m, assign.value), True - elif not ( - isinstance(key, str) - and key not in ("code", "reason") - and isinstance(p.ctx, ast.Store) - ): - problems.append(f"{self.at(m, p)} (may change the code or drop the reason)") - continue - if isinstance(p, ast.Attribute) and p.attr in READS: - continue - sender = ( - p - if isinstance(p, ast.Call) - else m.parents.get(p) - if isinstance(p, ast.keyword) - else None - ) - func = getattr(sender, "func", None) - if ( - isinstance(sender, ast.Call) - and (getattr(func, "id", None) or getattr(func, "attr", None)) in SENDERS - and (n in sender.args or isinstance(p, ast.keyword) and p.arg == "detail") - ): - continue - problems.append(f"{self.at(m, n)} (the detail goes where the scan cannot follow it)") - return res, found, problems + fn = self.enclosing(m, call, FUNCS) + out.append((m.path, call.lineno, getattr(fn, "name", None))) + return sorted(out) def run(self) -> dict: out: dict = { @@ -727,15 +702,7 @@ class Scan: ): out["aliases"].setdefault(m.path, {})[name] = sorted(v) for node in ast.walk(m.tree): - is_dict = isinstance(node, ast.Dict) and any( - isinstance(k, ast.Constant) and k.value == "code" for k in node.keys - ) - is_call = ( - isinstance(node, ast.Call) - and getattr(node.func, "id", None) == "dict" - and any(k.arg == "code" for k in node.keywords) - ) - if not (is_dict or is_call): + if not _code_dict(node): continue if m.path in NOT_REFUSALS: out["excluded"][m.path] = out["excluded"].get(m.path, 0) + 1 @@ -750,14 +717,16 @@ def scan(sources: dict[str, str]) -> dict: return Scan(sources).run() +def app_sources(app: pathlib.Path = APP) -> dict[str, str]: + return { + p.relative_to(app).as_posix(): p.read_text(encoding="utf-8") + for p in sorted(app.rglob("*.py")) + if "__pycache__" not in p.parts + } + + def scan_app(app: pathlib.Path = APP) -> dict: - return scan( - { - p.relative_to(app).as_posix(): p.read_text(encoding="utf-8") - for p in sorted(app.rglob("*.py")) - if "__pycache__" not in p.parts - } - ) + return scan(app_sources(app)) if __name__ == "__main__": diff --git a/backend/tests/test_http_details.py b/backend/tests/test_http_details.py index 5bc98728..b9cda470 100644 --- a/backend/tests/test_http_details.py +++ b/backend/tests/test_http_details.py @@ -1,57 +1,16 @@ -"""No refusal answers in a hand-written English sentence. +"""No refusal answers in a way the client cannot say. The UI is bilingual and translates a structured `detail` (`{code, reason, …}`), but a string detail it can only print as it came — so ~150 of them reached the Hungarian UI verbatim, in the blocking error dialog and on every inline surface that shows `HttpError.detail`. `app/refusal.py` is the family they -became; this test is what keeps a new one from being written the old way. Pure: it reads the sources. +became; this test is what keeps a new one from being written the old way. It reads the same slots the +refusal scan does (`refusal_sites.detail_slots`), so the two cannot disagree on where a detail is sent. +Pure: it reads the sources. """ -import ast -import pathlib - -APP = pathlib.Path(__file__).resolve().parents[1] / "app" - - -def _is_text(node: ast.AST) -> bool: - """A detail that is a sentence: a literal, an f-string, `str(exc)`, or `"…" + x`.""" - if isinstance(node, ast.Constant) and isinstance(node.value, str): - return True - if isinstance(node, ast.JoinedStr): - return True - if isinstance(node, ast.Call) and getattr(node.func, "id", "") == "str": - return True - if isinstance(node, ast.BoolOp): - return any(_is_text(v) for v in node.values) - if isinstance(node, ast.BinOp): - return _is_text(node.left) or _is_text(node.right) - return False - - -def test_no_http_exception_carries_a_sentence(): - offenders = [] - for path in sorted(APP.rglob("*.py")): - for node in ast.walk(ast.parse(path.read_text(encoding="utf-8"))): - if not ( - isinstance(node, ast.Call) - and getattr(node.func, "id", getattr(node.func, "attr", "")) == "HTTPException" - ): - continue - kw = {k.arg: k.value for k in node.keywords} - detail = kw.get("detail", node.args[1] if len(node.args) > 1 else None) - if detail is not None and _is_text(detail): - offenders.append(f"{path.relative_to(APP).as_posix()}:{node.lineno}") - assert offenders == [], ( - "a refusal with an English sentence for a detail — raise `refused(status, reason)` " - f"(app/refusal.py) and name the reason in both locales: {offenders}" - ) - - -def test_the_scan_sees_a_sentence_when_there_is_one(): - """A guard that matches nothing is green over nothing.""" - sample = ast.parse('HTTPException(status_code=400, detail=f"bad {x}")').body[0].value - assert _is_text(sample.keywords[1].value) - assert not _is_text(ast.parse('{"code": "refused", "reason": "x"}').body[0].value) +import textwrap +from tests.refusal_sites import Scan, app_sources # Answers that are no refusal of the app's, by the function they are sent from: the client shows a # detail only when it is an object (`lib/api/core.ts`), so these read as its generic line, and no screen @@ -59,62 +18,21 @@ def test_the_scan_sees_a_sentence_when_there_is_one(): NO_REFUSAL = { ("main.py", "setup_gate"): "the 503 before setup: the setup screen is what the user sees", ("main.py", "spa_fallback"): "a bare 404 for an api path no route matches: no screen calls one", + ("main.py", "_validation_error"): "FastAPI's own 422 list: a body the client itself got wrong", + ("routes/diagnostics.py", "_parsed_events"): "the client-events beacon's 422: nothing shows it", } -def _detail_offenders(source: str, path: str) -> list[str]: - """Each detail the client could not say: a dict without a `code`, a sentence, or none at all. - - `HTTPException(status, detail)` and `JSONResponse({"detail": …})`; a variable or a call is the - refusal scan's to read (`refusal_sites.py`, which follows each `code` dict).""" - tree = ast.parse(source) - parents = {child: node for node in ast.walk(tree) for child in ast.iter_child_nodes(node)} - out = [] - for node in ast.walk(tree): - name = isinstance(node, ast.Call) and getattr( - node.func, "id", getattr(node.func, "attr", "") - ) - if name == "HTTPException": - kw = {k.arg: k.value for k in node.keywords} - detail = kw.get("detail", node.args[1] if len(node.args) > 1 else None) - elif name == "JSONResponse": - content = {k.arg: k.value for k in node.keywords}.get( - "content", node.args[0] if node.args else None - ) - if not isinstance(content, ast.Dict) or not any( - isinstance(k, ast.Constant) and k.value == "detail" for k in content.keys - ): - continue # an answer, not a refusal - detail = next( - v - for k, v in zip(content.keys, content.values, strict=True) - if isinstance(k, ast.Constant) and k.value == "detail" - ) - else: - continue - fn = parents.get(node) - while fn is not None and not isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)): - fn = parents.get(fn) - if (path, getattr(fn, "name", None)) in NO_REFUSAL: - continue - keys = ( - [k.value for k in detail.keys if isinstance(k, ast.Constant)] - if isinstance(detail, ast.Dict) - else [k.arg for k in detail.keywords] - if isinstance(detail, ast.Call) and getattr(detail.func, "id", "") == "dict" - else None - ) - if detail is None or _is_text(detail) or (keys is not None and "code" not in keys): - out.append(f"{path}:{node.lineno}") - return out +def _offenders(sources: dict[str, str]) -> list[str]: + return [ + f"{path}:{line}" + for path, line, fn in Scan(sources).other_details() + if (path, fn) not in NO_REFUSAL + ] def test_every_detail_is_one_the_client_can_say(): - offenders = [ - o - for p in sorted(APP.rglob("*.py")) - for o in _detail_offenders(p.read_text(encoding="utf-8"), p.relative_to(APP).as_posix()) - ] + offenders = _offenders(app_sources()) assert offenders == [], ( "a refusal the client can only answer with its generic line — send `{code, reason}` " f"(`refused(status, reason)`), or list it in NO_REFUSAL with why: {offenders}" @@ -122,23 +40,49 @@ def test_every_detail_is_one_the_client_can_say(): def test_the_detail_scan_sees_each_shape_the_client_cannot_say(): - src = """ -def f(x): - raise HTTPException(409, detail={"reason": "view_name_taken"}) -def g(x): - raise HTTPException(409, dict(reason="taken")) -def h(x): - raise HTTPException(status_code=404) -def i(x): - return JSONResponse({"detail": "This instance isn't set up yet."}, status_code=503) -def j(x): - return JSONResponse(content={"detail": {"reason": "x"}}, status_code=409) -def ok(x): - raise HTTPException(409, {"code": "refused", "reason": "x"}) - raise HTTPException(409, detail(x)) - return JSONResponse({"detail": {"code": "too_large"}}, status_code=413) - return JSONResponse({"ok": True}) -def setup_gate(x): - return JSONResponse({"detail": "listed"}, status_code=503) -""" - assert _detail_offenders(src, "main.py") == [f"main.py:{n}" for n in (3, 5, 7, 9, 11)] + """A guard that matches nothing is green over nothing.""" + src = textwrap.dedent( + """ + def f(x): + raise HTTPException(409, detail={"reason": "view_name_taken"}) + def g(x): + raise HTTPException(409, dict(reason="taken")) + def h(x): + raise HTTPException(status_code=404) + def i(x): + return JSONResponse({"detail": "This instance isn't set up yet."}, status_code=503) + def j(x): + return JSONResponse(content={"detail": {"reason": "x"}}, status_code=409) + def k(x): + raise HTTPException(400, detail=f"bad {x}") + def l(x): + raise HTTPException(404, None) + def n(x, msg): + raise HTTPException(400, detail=msg) + def o(x): + raise HTTPException(400, detail=["a sentence"]) + def p(x): + raise HTTPException(400, detail=describe(x)) + def q(x): + raise HTTPException(400, detail={"code": "a"} if x else "text") + def r(x): + raise HTTPException(400, detail=nothing(x)) + def nothing(x): + pass + def ok(x): + raise HTTPException(409, {"code": "refused", "reason": "x"}) + raise HTTPException(409, detail=quota(x)) + raise HTTPException(409, detail={"code": "a"} if x else {"code": "b"}) + return JSONResponse({"detail": {"code": "too_large"}}, status_code=413) + return JSONResponse({"ok": True}) + def quota(x): + return {"code": "quota", "reason": "files"} + def setup_gate(x): + return JSONResponse({"detail": "listed"}, status_code=503) + """ + ) + lines = (3, 5, 7, 9, 11, 13, 15, 17, 19, 21, 23, 25) + assert _offenders({"main.py": src}) == [f"main.py:{n}" for n in lines] + # A file whose code dicts are data still answers its refusals in a way the client can say. + data = 'def f():\n raise HTTPException(400, "text")\n' + assert _offenders({"plex/tmdb.py": data}) == ["plex/tmdb.py:2"] diff --git a/backend/tests/test_refusal_sites.py b/backend/tests/test_refusal_sites.py index e9b74afc..1c3592fd 100644 --- a/backend/tests/test_refusal_sites.py +++ b/backend/tests/test_refusal_sites.py @@ -26,15 +26,7 @@ REFUSAL = """ from fastapi import HTTPException def refused(status: int, reason: str, **params: object) -> HTTPException: - return HTTPException(status_code=status, detail={"code": "refused", "reason": reason, **params}) -""" - -# The same helper without a `**params` spread: what a call passes it is about the reason alone. -PLAIN = """ - from fastapi import HTTPException - - def refused(status: int, reason: str) -> HTTPException: - return HTTPException(status_code=status, detail={"code": "refused", "reason": reason}) + return HTTPException(status_code=status, detail={**params, "code": "refused", "reason": reason}) """ @@ -109,6 +101,12 @@ def test_a_helper_is_followed_to_every_call_however_it_is_written(): def i(x): raise refused(409, "a" if x else "second_arm") """, + routes__b=""" + from other.lib import refused + + def f(): + refused("an outside library's, not the app's") + """, ) assert _code(r, "refused")["reasons"] == [ "a", @@ -122,7 +120,7 @@ def test_a_helper_is_followed_to_every_call_however_it_is_written(): def test_a_call_the_helper_cannot_be_followed_through_is_unread(): r = _scan( - refusal=PLAIN, + refusal=REFUSAL, routes__a=""" from app.refusal import refused @@ -267,6 +265,8 @@ def test_a_local_is_followed_only_through_plain_assignments(): def j(items): LABELS[why] = "NotAReason" raise HTTPException(409, {"code": "playlist", "reason": why}) + def k(*why): + raise HTTPException(409, {"code": "playlist", "reason": why}) """ ) assert _code(r, "edit")["reasons"] == ["first", "second", "third"] @@ -276,7 +276,8 @@ def test_a_local_is_followed_only_through_plain_assignments(): "why", "why", "why", - ] # unpacked, a loop, annotated, unbound + "why", + ] # unpacked, a loop, annotated, unbound, `*args` def test_a_call_answers_with_its_literal_return_type(): @@ -470,7 +471,7 @@ def test_a_map_lookup_is_its_values_and_its_keys_must_be_exactly_what_the_index_ _MAP: dict[SettingReason, str] = {{{keys}}} def refused(status: int, reason: str, **params: object): - return HTTPException(status, {{"code": "refused", "reason": reason, **params}}) + return HTTPException(status, {{**params, "code": "refused", "reason": reason}}) def setting_refused(exc: InvalidSetting): return refused(400, _MAP[{index}], **exc.params) @@ -527,115 +528,229 @@ def test_a_code_must_be_one_literal(): assert r["codes"]["busy"]["reasonless"] == ["a.py:4"] assert set(r["codes"]) == {"busy"} assert len(r["unreadCodes"]) == 5 - assert r["unreadCodes"][-1].endswith("(a key the scan cannot read)") + assert r["unreadCodes"][-1].endswith( + "(a spread or a computed key the code and the reason do not follow)" + ) -def test_a_spread_is_clean_only_from_an_untouched_kwargs_no_call_puts_a_code_in(): - def unread_codes(helper: str, call: str) -> list[str]: - return _scan(a=helper + "\n" + f"def caller(x, more):\n raise {call}\n")["unreadCodes"] +def test_a_spread_or_a_computed_key_stands_before_both_the_code_and_the_reason(): + def unread_codes(detail: str) -> list[str]: + return _scan(a=f"def f(x, more, key):\n raise HTTPException(409, {detail})\n")[ + "unreadCodes" + ] - helper = """ -def deny(status, reason: str, **params): - return HTTPException(status, {"code": "refused", "reason": reason, **params}) + assert unread_codes('{**more, "code": "refused", "reason": "a"}') == [] + assert unread_codes('{key: 1, **more, "reason": "a", "code": "refused"}') == [] + for late in [ + '{"code": "refused", "reason": "a", **more}', # the spread may replace both + '{"code": "refused", **more, "reason": "a"}', # … the code + '{"reason": "a", **more, "code": "refused"}', # … the reason + '{**more, "code": "refused"}', # no reason after it: the spread may carry one + '{"code": "refused", "reason": "a", key: "x"}', # a key that may be either + 'dict(**more, code="refused", reason="a")', # dict() raises on a repeated key + 'dict(more, code="refused", reason="a")', + ]: + assert unread_codes(late), late + + +def test_a_detail_stands_only_where_it_is_sent(): + def result(body: str, extra: str = "") -> dict: + return _scan(a=f"def f(x, cached, fix):\n{textwrap.indent(body, ' ')}\n{extra}") + + for body in [ + 'detail = {"code": "login", "reason": "a"}\nraise HTTPException(401, detail)', + 'if (detail := {"code": "login", "reason": "a"}):\n raise HTTPException(401, detail)', + 'raise HTTPException(401, {"code": "login", "reason": "a"}.copy())', + 'raise HTTPException(401, fix({"code": "login", "reason": "a"}))', + 'raise HTTPException(401, cached or {"code": "login", "reason": "a"})', + 'return JSONResponse({"code": "login", "reason": "a"}, status_code=401)', # the whole body + 'raise HTTPException(401, None, {"code": "login", "reason": "a"})', # the headers + 'raise HTTPException(401, headers={"code": "login", "reason": "a"})', + 'raise HTTPException(401, "a" if {"code": "login", "reason": "a"} else "b")', + ]: + r = result(body) + assert r["unreadCodes"] and "login" not in r["codes"], body + assert "(a detail put where the scan cannot follow it)" in r["unreadCodes"][0], body + held = _scan(a='class C:\n detail = {"code": "login"}\n') + assert "(a detail put where the scan cannot follow it)" in held["unreadCodes"][0] + + sent = result( + 'raise HTTPException(401, {"code": "login", "reason": "a"} if x else {"code": "login"})\n' + 'raise HTTPException(401, detail=dict(code="login", reason="b"))\n' + 'return JSONResponse(content={"detail": {"code": "login", "reason": "c"}})' + ) + assert sent["unreadCodes"] == [] and _code(sent, "login")["reasons"] == ["a", "b", "c"] + + +def test_a_factory_is_a_detail_only_when_each_return_is_one_and_each_call_is_sent(): + factory = """ +def quota(e) -> dict: + return {"code": "quota", "reason": "files"} """ - assert unread_codes(helper, 'deny(409, "a", limit=3)') == [] - assert unread_codes(helper, 'deny(409, "a", code=x)') # the spread puts its own code - assert unread_codes(helper, 'deny(409, "a", **more)') # a spread nothing vouches for - assert unread_codes(helper + "pick = deny\n", 'deny(409, "a")') # passed around - posonly = helper.replace("reason: str,", "reason: str, /,") # `reason=` lands in params - assert unread_codes(posonly, 'deny(409, "a")') - mutated = helper.replace(" return", ' params["reason"] = "zz"\n return') - assert unread_codes(mutated, 'deny(409, "a")') - renamed = helper.replace("**params})", "**{k.lstrip('_'): v for k, v in params.items()}})") - assert unread_codes(renamed, 'deny(409, "a")') - filtered = helper.replace("**params})", "**{k: v for k, v in params.items() if v}})") - assert unread_codes(filtered, 'deny(409, "a")') == [] + sent = "def f(e):\n raise HTTPException(422, detail=quota(e))\n" + assert _scan(a=factory + sent)["codes"]["quota"]["reasons"] == ["files"] + for change in [ + "def g(e):\n d = quota(e)\n d['code'] = 'other'\n raise HTTPException(422, d)\n", + "def g(e):\n raise HTTPException(422, detail=quota(e).copy())\n", + "pick = quota\n", + ]: + r = _scan(a=factory + sent + change) + assert r["unreadCodes"] and "quota" not in r["codes"], change + twice = _scan(a="quota = None\n" + factory + sent) # the def is the last of two bindings + assert twice["unreadCodes"] and "quota" not in twice["codes"] + for other in [ + " if e:\n return None\n", + " if e:\n return cached\n", + " if e:\n return {'code': 'quota', 'reason': 'x'} if e > 1 else None\n", + " log({'code': 'quota', 'reason': 'logged'})\n", # a detail it does not return + ]: + bent = factory.replace("-> dict:\n", "-> dict:\n" + other) + assert _scan(a=bent + sent)["unreadCodes"], other + method = "class C:\n def quota(self):\n return {'code': 'quota', 'reason': 'm'}\n" + assert _scan(a=method)["unreadCodes"] + dotted = _scan( + a=factory, + routes__b="import app.a\n\ndef g(e):\n raise HTTPException(422, detail=app.a.quota(e))\n", + ) + assert dotted["unreadCodes"] and "quota" not in dotted["codes"] -def test_a_caught_exceptions_params_are_clean_only_when_kept_from_its_init(): - def unread_codes(errors: str, construct: str = 'Err("a", limit=1)') -> list[str]: - return _scan( - errors=errors, - a=f""" - from app.errors import Err - def f(): - raise {construct} - def g(): - try: - pass - except Err as e: - raise HTTPException(422, {{"code": "clip", "reason": e.reason, **e.params}}) - """, - )["unreadCodes"] - - kept = """ - from typing import Literal - class Err(Exception): - def __init__(self, reason: Literal["a"], **params: float): - super().__init__(f"{reason} {params}") - self.reason: Literal["a"] = reason - self.params = params - """ - assert unread_codes(kept) == [] - assert unread_codes(kept, 'Err("a", code="other")') - assert unread_codes(kept + "\n class Sub(Err):\n pass\n") - assert unread_codes(kept.replace("self.params = params", "self.params = {'code': 'x'}")) - assert unread_codes(kept + "\n def more(self):\n self.params = {}\n") - - -def test_a_detail_built_up_in_steps_is_read_through_its_variable(): +def test_a_name_a_lambda_a_comprehension_a_nonlocal_or_a_global_binds_is_unread(): r = _scan( a=""" - def f(status: int, reason: str, user=None, **extra): - detail: dict = {"code": "login", "reason": "invalid"} - if user is not None: - detail |= {"email": user.email, "reason": "suspended"} - detail |= {k: v for k, v in extra.items() if v is not None} - detail["email"] = "x" - return HTTPException(status, detail) - - def g(x): - detail = {"code": "login"} if x else {"code": "login", "reason": "other"} - detail["reason"] = "late" - return HTTPException(401, detail=detail) + def f(svc): + why = "invalid" + make = lambda why: HTTPException(401, {"code": "login", "reason": why}) + raise make(svc.state) + def g(svc): + why = "invalid" + raise [HTTPException(401, {"code": "login", "reason": why}) for why in svc.all][0] + def h(svc): + why = "invalid" + def inner(): + nonlocal why + why = svc.reason + inner() + raise HTTPException(401, {"code": "login", "reason": why}) + def i(svc): + global why + why = "invalid" + raise HTTPException(401, {"code": "login", "reason": why}) """ ) - assert r["unreadCodes"] == [] - assert _code(r, "login")["reasons"] == ["invalid", "late", "other", "suspended"] - - -def test_a_detail_changed_or_sent_any_other_way_is_an_unread_code(): - changes = [ - "detail.update(more)", - "detail |= more", - 'detail |= {"code": "other"}', - 'detail["code"] = "other"', - 'del detail["reason"]', - "detail[key] = more", - 'detail = {**detail, "reason": "b"}', - "detail = rebuilt()", - "copy = detail", - "add_extra(detail)", - "return detail", - 'return {**detail, "reason": "b"}', - 'return dict(detail, reason="b")', + assert _code(r, "login")["reasons"] == [] + assert sorted(_unread(r, "login")) == [ + "why (bound by a lambda or a comprehension)", + "why (bound by a lambda or a comprehension)", + "why (named by a nonlocal or a global)", + "why (named by a nonlocal or a global)", ] - for change in changes: - r = _scan( - a=f""" - def f(key, more): - detail = {{"code": "login", "reason": "a"}} - {change} - raise HTTPException(401, detail) - """ - ) - assert r["unreadCodes"], change - assert "login" not in r["codes"], change - held = _scan(a='class C:\n detail = {"code": "login"}\n') - assert ( - held["unreadCodes"] - and "a detail put where the scan cannot follow it" in held["unreadCodes"][0] + + +def test_a_local_that_reaches_itself_is_unread(): + r = _scan( + a=""" + def f(x): + why = "a" + if x: + why = why if x > 1 else "b" + raise HTTPException(409, {"code": "edit", "reason": why}) + def g(x): + a = "z" + b = a + a = b + raise HTTPException(409, {"code": "playlist", "reason": a}) + def h(x): + why = "c" + if x: + raise HTTPException(409, {"code": "busy", "reason": why}) + raise HTTPException(409, {"code": "busy", "reason": why}) + """ ) + assert _code(r, "busy")["reasons"] == ["c"] and _code(r, "busy")["unread"] == [] + assert _code(r, "edit")["reasons"] == ["a", "b"] + assert _unread(r, "edit") == ["why (reached again while it is resolved)"] + assert _code(r, "playlist")["reasons"] == ["z"] + assert any("reached again while it is resolved" in u for u in _unread(r, "playlist")) + + +def test_a_module_constant_or_a_map_changed_elsewhere_is_unread(): + def reasons(urlguard: str, route: str = "", **more: str) -> tuple[list[str], list[str]]: + r = _scan( + **more, + urlguard="REASON = 'malformed'\n_MAP = {'a': 'map_a'}\n" + urlguard, + routes__a="from app import urlguard\n" + + route + + "\ndef f(k: Literal['a']):\n" + + " raise HTTPException(409, {'code': 'url_blocked', 'reason': urlguard.REASON})\n" + + " raise HTTPException(409, {'code': 'url_blocked', 'reason': urlguard._MAP[k]})\n", + ) + return _code(r, "url_blocked")["reasons"], _unread(r, "url_blocked") + + assert reasons("def get(k):\n return _MAP.get(k)\n") == (["malformed", "map_a"], []) + other = "from app import other\ndef g():\n other.REASON = 'x'\n" # another module's REASON + assert reasons("", other, other="REASON = 'y'\n") == (["malformed", "map_a"], []) + shadow = ( + "def h(*urlguard):\n" + " raise HTTPException(409, {'code': 'url_blocked', 'reason': urlguard.REASON})\n" + ) + assert reasons("", shadow)[1] == ["urlguard.REASON"] + for urlguard, route in [ + ("def reset():\n global REASON\n REASON = 'x'\n", ""), + ("", "def g():\n urlguard.REASON = 'x'\n"), + ("", "def g():\n del urlguard.REASON\n"), + ]: + assert reasons(urlguard, route)[0] == ["map_a"], (urlguard, route) + for urlguard in [ + "_MAP['a'] = compute()\n", + "def init(x):\n _MAP['a'] = x\n", + "_MAP.update(a='other')\n", + "def drop():\n del _MAP['a']\n", + "register(_MAP)\n", + ]: + got, unread = reasons(urlguard) + assert got == ["malformed"] and "(the map is changed after it is built)" in unread[0] + + +def test_a_helper_bound_twice_is_not_followed(): + r = _scan( + refusal="refused = None\n" + textwrap.dedent(REFUSAL), + routes__a="from app.refusal import refused\n\ndef f():\n raise refused(409, 'lost')\n", + ) + assert _code(r, "refused")["reasons"] == [] + assert _unread(r, "refused")[0].endswith("refused(reason) is not a module-level helper") + + +def test_a_dotted_path_through_an_app_module_that_goes_nowhere_is_unread(): + r = scan( + { + "__init__.py": "", + "refusal.py": textwrap.dedent(REFUSAL), + "routes/a.py": "import app as a\n\ndef f():\n raise a.refusal.refused(409, 'dotted')\n", + } + ) + assert _unread(r, "refused") == ["a.refusal.refused (a reference the scan cannot resolve)"] + + +def test_a_helper_reached_through_an_app_module_that_does_not_bind_it_is_unread(): + for shim in [ + "from app.refusal import *\n", + "try:\n from app.refusal import refused\nexcept ImportError:\n refused = None\n", + ]: + r = _scan( + refusal=REFUSAL, + shim=shim, + routes__a="from app.shim import refused\n\ndef f():\n raise refused(409, 'hidden')\n", + ) + assert _unread(r, "refused") == ["refused (a reference the scan cannot resolve)"], shim + package = scan( + { + "pkg/refusal.py": textwrap.dedent(REFUSAL), + "pkg/__init__.py": "from .refusal import refused\n", + "routes/a.py": "from app.pkg import refused\n\ndef f():\n raise refused(409, 'hidden')\n", + } + ) + assert _unread(package, "refused") == ["refused (a reference the scan cannot resolve)"] def test_the_data_files_are_excluded_by_name_only(): From be38d652a5a881fe7129ec2de83d01ec53dfc3ac Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 1 Nov 2026 00:18:59 +0100 Subject: [PATCH 6/6] chore(release): 0.114.3 notes --- CHANGELOG.md | 6 ++++++ VERSION | 2 +- frontend/src/content/release-notes/en.json | 8 ++++++++ frontend/src/content/release-notes/hu.json | 8 ++++++++ 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05796f1d..85cd6411 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ edit the release notes and re-run the generator. Upgrading, pinning a version and rolling back: [docs/self-hosting.md](docs/self-hosting.md#upgrading). +## 0.114.3 — 2026-11-01 + +### Fixed + +- Editing a file that is neither video nor audio now says why it can't be done, instead of a generic error. + ## 0.114.2 — 2026-10-30 ### Fixed diff --git a/VERSION b/VERSION index 484129bc..c672d9c2 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.114.2 +0.114.3 diff --git a/frontend/src/content/release-notes/en.json b/frontend/src/content/release-notes/en.json index 4a0fba84..a1cf90ee 100644 --- a/frontend/src/content/release-notes/en.json +++ b/frontend/src/content/release-notes/en.json @@ -1,4 +1,12 @@ [ + { + "version": "0.114.3", + "date": "2026-11-01", + "new": [], + "fixed": [ + "Editing a file that is neither video nor audio now says why it can't be done, instead of a generic error." + ] + }, { "version": "0.114.2", "date": "2026-10-30", diff --git a/frontend/src/content/release-notes/hu.json b/frontend/src/content/release-notes/hu.json index 7dc11d2d..ee918e94 100644 --- a/frontend/src/content/release-notes/hu.json +++ b/frontend/src/content/release-notes/hu.json @@ -1,4 +1,12 @@ [ + { + "version": "0.114.3", + "date": "2026-11-01", + "new": [], + "fixed": [ + "Ha nem videó- vagy hangfájlt próbálsz szerkeszteni, az alkalmazás most megmondja, miért nem lehet, általános hibaüzenet helyett." + ] + }, { "version": "0.114.2", "date": "2026-10-30",