peter
8c51062e5b
Round 6 converted three of four `is_demo` 403s to a structured detail and wrote a docstring saying it
had converted all the sites there are. The account-delete route in `routes/me.py` still answered a
hand-written English sentence, which `notifyActionError` discards in favour of the caller's
translated fallback — so the demo user gets no hint that the demo account is the reason. It is
UI-gated today, so the string is unreachable; what was reachable was the false completeness claim,
which is precisely what stops the next reader from looking. The site is converted, and the list is
now DERIVED from the source: a new test walks `app/` for every `is_demo`-guarded 403, asserts each
raises a structured detail, and fails when the count moves — so this file cannot fall behind the code
the way it just did.
The same round's frontend contract guard was failing open. Its pattern required a bare identifier
before `.detail` and excluded `.detail` used as a last call argument, so `setErr(e.detail)`,
`(e as HttpError).detail`, `String(e.detail)` and four more idiomatic forms were invisible — and
because the file set is compared with `toEqual`, a file the pattern cannot see is simply never listed
and the gate stays green over a fresh copy of the contract. Widened to cover cast receivers, optional
chains and call-argument reads; the pinned set is unchanged, so the narrowing had removed no false
positive. A destructured `detail` stays out of reach (a pattern loose enough for it fires on
react-query's own `const { data: detail }`), and that hole is written into the test rather than
papered over with two innocent files.
Its anti-vacuity guard also did not guard anything: the comment says the glob must have matched
something, but the assertion under it read a sibling file by its own URL, which resolves whether or
not the glob enumerated anything. That only mattered while the pinned list was non-empty — the moment
the deferral sweep lands and it becomes [], the whole file would pass over a glob matching zero
files. The scan size is asserted now.
110 lines
6.3 KiB
TypeScript
110 lines
6.3 KiB
TypeScript
import { readFileSync } from "node:fs";
|
|
import { describe, expect, it } from "vitest";
|
|
|
|
// One contract, one implementation — `notifyActionError` (see actionErrors.ts).
|
|
//
|
|
// `req` deliberately leaves 403/404/429 to the caller, and the routes behind those statuses
|
|
// hand-write English. Round 5 taught the shared helper to discard a plain-string `detail` in favour
|
|
// of the caller's already-translated fallback. Round 6 then found `ChatThread` carrying a SECOND,
|
|
// hand-written copy of the same contract (`e.detail || t("messages.sendFailed")`) — so the central
|
|
// fix could not reach it and the English sentence stayed in a Hungarian UI. A fix that lands in one
|
|
// place while a copy of the rule lives in another is this repo's recurring smell; this is the check
|
|
// that names the copies.
|
|
//
|
|
// The assertion pins the expected SET, never a count: a guard that counts is green the moment its
|
|
// glob stops matching (E10 review round 4 learned that the hard way).
|
|
|
|
// Reads a raw `HttpError.detail` and uses it as the message a user sees. Matches `e.detail ||`,
|
|
// `err.detail ? err.detail :`, `(e instanceof HttpError ? e.detail : undefined) ??`, a cast
|
|
// receiver (`(e as HttpError).detail`), an optional chain (`e?.detail`) and a `.detail` passed
|
|
// straight as a call argument (`setErr(e.detail)`).
|
|
//
|
|
// The receiver alternation and the absence of a `)` lookahead are both load-bearing. The first
|
|
// version required a bare identifier before `.detail` and excluded `.detail)` — so `setErr(e.detail)`,
|
|
// `String(e.detail)` and `(e as HttpError).detail` were all invisible, and because the file SET is
|
|
// compared with `toEqual` a file the pattern cannot see is simply never listed: the guard stayed
|
|
// green over a fresh copy of the contract written in any of those shapes (round 7). The `)`
|
|
// exclusion bought nothing — the only `.detail)` in the frontend is inside `lib/api/core.ts`, which
|
|
// `IMPLEMENTATION` skips wholesale — and re-scanning with the wider pattern returns the same five
|
|
// files, so it removes no false positive.
|
|
const RAW_DETAIL = /(?:\)|[A-Za-z_$][\w$]*)\s*\??\.detail\b(?!\s*=[^=])/;
|
|
// KNOWN LIMIT: a `detail` pulled out by destructuring (`const { detail } = err`) is out of reach
|
|
// of a line-level scan — by then there is no receiver left to match, and a pattern loose enough
|
|
// to catch it fires on every unrelated `detail` field too (measured: `const { data: detail } =
|
|
// useQuery(...)` in VideoInfoCard and an options destructure in plexPlayerHooks, neither of which
|
|
// touches an HttpError). Naming the hole beats pinning two innocent files into the list below as
|
|
// though they were copies of the contract.
|
|
|
|
// Every place that still prefers the server's raw string over a translated fallback. These are
|
|
// INLINE surfaces — a form field's error line, a setup step's banner — not toasts, and each shows a
|
|
// reason the user is mid-flow on, so converting them is a sweep of its own (with the ~60 backend
|
|
// routes that hand-write those strings). Logged on ROADMAP.md under the bug-hunt deferrals. What
|
|
// must not happen is a NEW one, or a toast path growing one again.
|
|
const KNOWN_RAW_DETAIL_CONSUMERS = [
|
|
"components/Feed.tsx", // the live-YouTube search error line
|
|
"components/KeyGate.tsx", // the passphrase field's own refusal
|
|
"components/SettingsPanel.tsx", // the change-password field
|
|
"components/SetupWizard.tsx", // the first-run wizard's banner
|
|
"components/Welcome.tsx", // the sign-in card's refusal panel
|
|
];
|
|
|
|
// Where the rule itself lives, plus the request layer that produces and localizes the value.
|
|
const IMPLEMENTATION = ["lib/actionErrors.ts", "lib/api/core.ts"];
|
|
|
|
function consumers(): { hits: string[]; scanned: number } {
|
|
const files = import.meta.glob("../**/*.{ts,tsx}", {
|
|
query: "?raw",
|
|
import: "default",
|
|
eager: true,
|
|
});
|
|
const hits: string[] = [];
|
|
for (const [rel, source] of Object.entries(files)) {
|
|
// Vite resolves the glob against THIS file, so same-directory hits come back as "./x".
|
|
const path = rel.startsWith("./") ? `lib/${rel.slice(2)}` : rel.replace(/^\.\.\//, "");
|
|
if (/\.test\.tsx?$|\.d\.ts$/.test(path)) continue;
|
|
if (IMPLEMENTATION.includes(path)) continue;
|
|
const lines = String(source)
|
|
.split("\n")
|
|
.filter((l) => !/^\s*(\/\/|\*|\/\*)/.test(l)); // a comment about the rule is not a copy of it
|
|
if (lines.some((l) => RAW_DETAIL.test(l))) hits.push(path);
|
|
}
|
|
return { hits: hits.sort(), scanned: Object.keys(files).length };
|
|
}
|
|
|
|
describe("the refused-write contract", () => {
|
|
it("has no copies beyond the ones on the deferral list", () => {
|
|
const { hits, scanned } = consumers();
|
|
// The GLOB must have matched something, or this whole file is a green no-op — and the old
|
|
// version of this guard did not check that: it read `actionErrors.ts` by its own URL, which
|
|
// resolves whether or not `import.meta.glob` enumerated a single file. That only mattered
|
|
// because the pinned set was non-empty; the moment the deferral sweep below lands and the list
|
|
// becomes `[]`, `expect([]).toEqual([])` would pass over a glob that matched nothing (round 7).
|
|
expect(scanned).toBeGreaterThan(100);
|
|
expect(readFileSync(new URL("./actionErrors.ts", import.meta.url), "utf8")).toContain(
|
|
"detailData",
|
|
);
|
|
expect(hits).toEqual(KNOWN_RAW_DETAIL_CONSUMERS);
|
|
});
|
|
|
|
it("sees a raw detail written in any of the shapes this codebase uses", () => {
|
|
// The pattern is the whole guard, so pin the pattern itself: each of these is a real form
|
|
// found in this frontend or in its sibling PRs, and every one of the first five was invisible
|
|
// to the original regex.
|
|
for (const line of [
|
|
" setErr(e.detail);",
|
|
" reportError(err.detail);",
|
|
" notify({ message: (e as HttpError).detail });",
|
|
" toast(String(e.detail));",
|
|
" notify({ message: (err as HttpError).detail ?? fallback });",
|
|
' setError(e.detail ?? t("x"));',
|
|
' const msg = err.detail || t("x");',
|
|
" setErr(state.err?.detail);",
|
|
])
|
|
expect(RAW_DETAIL.test(line), line).toBe(true);
|
|
// And it must not fire on the places that PRODUCE or localize the value, which is why
|
|
// `IMPLEMENTATION` exists rather than a cleverer pattern: an assignment is not a read.
|
|
for (const line of [" detail = await r.json();", " this.detail = detail;"])
|
|
expect(RAW_DETAIL.test(line), line).toBe(false);
|
|
});
|
|
});
|