Merge work/coach-read-locale

src/__tests__/helpers/narrated-locale.ts

@@ -0,0 +1,300 @@
+/**
+ * Detector for the "a narrated sentence's locale stays required" guard.
+ *
+ * Lives beside the guard rather than inside it so the guard's own
+ * counter-tests can run this exact code against disguised sources without
+ * re-running the guard itself. See `narrated-locale-required-guard.test.ts`
+ * for why the rule exists and `narrated-locale-guard-defeat.test.ts` for the
+ * disguises it has to see through.
+ */
+import { readFileSync } from "node:fs";
+import { resolve } from "node:path";
+
+import ts from "typescript";
+
+const REPO_ROOT = resolve(__dirname, "../../..");
+
+/**
+ * The chain. Each entry is a declaration whose output is, or becomes, prose a
+ * person reads. Add to it when a new one joins the chain; never remove one
+ * because the check became inconvenient.
+ */
+export const CHAIN: ReadonlyArray<{ file: string; declaration: string }> = [
+  {
+    file: "src/lib/insights/correlation-discovery.ts",
+    declaration: "discoverCorrelations",
+  },
+  {
+    file: "src/lib/insights/correlation-discovery.ts",
+    declaration: "discoverEmergingCorrelations",
+  },
+  {
+    file: "src/lib/insights/correlation-discovery.ts",
+    declaration: "discoverLabOutcomeCorrelations",
+  },
+  {
+    file: "src/lib/ai/coach/tools/correlations-read.ts",
+    declaration: "readCoachCorrelations",
+  },
+  {
+    file: "src/lib/insights/derived/coach-read.ts",
+    declaration: "buildCoachReadStrip",
+  },
+  {
+    file: "src/lib/insights/metric-correlation-context.ts",
+    declaration: "getRelevantCorrelationsForMetric",
+  },
+  {
+    file: "src/lib/cycle/phase-crosstab.ts",
+    declaration: "discoverPhaseCorrelations",
+  },
+  {
+    file: "src/lib/insights/narrative/period-narrative.ts",
+    declaration: "buildPeriodNarrativeContext",
+  },
+  {
+    file: "src/lib/insights/narrative/period-narrative.ts",
+    declaration: "assemblePeriodNarrativeContext",
+  },
+  {
+    file: "src/lib/ai/coach/correlations-snapshot.ts",
+    declaration: "buildCorrelationsSnapshotBlock",
+  },
+];
+
+export const WHY_NO_DEFAULT =
+  "A locale on this chain must be REQUIRED, with no `?` and no default. " +
+  "These functions return finished sentences that a page prints verbatim. " +
+  "An optional locale lets a caller say nothing and still get a sentence — " +
+  "which is exactly how the metric page's Coach-read strip came to answer a " +
+  "German page in English, under a German first line, for months. A default " +
+  "cannot fail loudly; a missing argument can. Pass the reader's locale " +
+  "instead of restoring the fallback.";
+
+export interface LocaleSite {
+  /** Where it sits, for the failure message. */
+  where: string;
+  optional: boolean;
+  hasInitializer: boolean;
+}
+
+/** The local name `Locale` is bound to in this file (it may be aliased). */
+function localeTypeName(source: ts.SourceFile): string {
+  let name = "Locale";
+  for (const statement of source.statements) {
+    if (!ts.isImportDeclaration(statement)) continue;
+    const bindings = statement.importClause?.namedBindings;
+    if (!bindings || !ts.isNamedImports(bindings)) continue;
+    for (const element of bindings.elements) {
+      if ((element.propertyName ?? element.name).text === "Locale") {
+        name = element.name.text;
+      }
+    }
+  }
+  return name;
+}
+
+/** Does this type node denote the locale type (possibly a union with it)? */
+function isLocaleType(node: ts.TypeNode | undefined, alias: string): boolean {
+  if (!node) return false;
+  if (ts.isTypeReferenceNode(node)) return node.typeName.getText() === alias;
+  if (ts.isUnionTypeNode(node))
+    return node.types.some((t) => isLocaleType(t, alias));
+  return false;
+}
+
+/**
+ * Does the type still let a caller say nothing? `Locale | undefined` and
+ * `Locale | null` carry no `?` and no default, so a check that only looked at
+ * the question mark read them as required — and they are the original bug
+ * wearing a different hat: the caller passes `undefined`, the body picks a
+ * language, and the sentence comes back in English again. `any` and `unknown`
+ * are the same hole with the door taken off.
+ */
+function toleratesAbsence(node: ts.TypeNode | undefined): boolean {
+  if (!node) return false;
+  if (
+    node.kind === ts.SyntaxKind.UndefinedKeyword ||
+    node.kind === ts.SyntaxKind.NullKeyword ||
+    node.kind === ts.SyntaxKind.AnyKeyword ||
+    node.kind === ts.SyntaxKind.UnknownKeyword ||
+    node.kind === ts.SyntaxKind.VoidKeyword
+  ) {
+    return true;
+  }
+  if (ts.isLiteralTypeNode(node)) {
+    return node.literal.kind === ts.SyntaxKind.NullKeyword;
+  }
+  if (ts.isUnionTypeNode(node)) return node.types.some(toleratesAbsence);
+  return false;
+}
+
+/**
+ * Property names given a default inside an object binding pattern —
+ * `function narrate({ locale = "en" }: { locale: Locale })`. The default sits
+ * on the binding, not on the parameter, so `parameter.initializer` is empty
+ * and the signature reads clean.
+ */
+function defaultedBindingNames(
+  parameter: ts.ParameterDeclaration,
+): Set<string> {
+  const names = new Set<string>();
+  if (!ts.isObjectBindingPattern(parameter.name)) return names;
+  for (const element of parameter.name.elements) {
+    if (element.initializer === undefined) continue;
+    names.add((element.propertyName ?? element.name).getText());
+  }
+  return names;
+}
+
+/** Find a top-level function/interface declaration by name. */
+function findDeclaration(
+  source: ts.SourceFile,
+  name: string,
+): ts.FunctionDeclaration | ts.InterfaceDeclaration | undefined {
+  for (const statement of source.statements) {
+    if (
+      (ts.isFunctionDeclaration(statement) ||
+        ts.isInterfaceDeclaration(statement)) &&
+      statement.name?.text === name
+    ) {
+      return statement;
+    }
+  }
+  return undefined;
+}
+
+/**
+ * Every place a locale is accepted by `declaration` — as a parameter in its own
+ * right, as a member of an inline options object, or as a member of a
+ * same-file interface used as a parameter type. Keyed on the TYPE, so renaming
+ * the parameter changes nothing.
+ */
+export function localeSites(
+  source: ts.SourceFile,
+  declarationName: string,
+): LocaleSite[] {
+  const alias = localeTypeName(source);
+  const declaration = findDeclaration(source, declarationName);
+  if (!declaration) {
+    throw new Error(
+      `${WHY_NO_DEFAULT}\n\nGuard could not find \`${declarationName}\` in ` +
+        `${source.fileName}. If it was renamed or moved, update the chain ` +
+        `list — do not delete the entry.`,
+    );
+  }
+
+  const sites: LocaleSite[] = [];
+
+  const collectFromMembers = (
+    members: ts.NodeArray<ts.TypeElement>,
+    owner: string,
+    ownerOptional: boolean,
+    ownerHasInitializer: boolean,
+    defaultedNames: Set<string> = new Set(),
+  ) => {
+    for (const member of members) {
+      if (!ts.isPropertySignature(member)) continue;
+      if (!isLocaleType(member.type, alias)) continue;
+      const name = member.name.getText();
+      sites.push({
+        where: `${owner}.${name}`,
+        // The property itself may be required while the object it lives on is
+        // optional or defaulted — which puts the silence right back. So does a
+        // type that merely admits `undefined` / `null`: no question mark, and a
+        // caller can still hand over nothing.
+        optional:
+          member.questionToken !== undefined ||
+          ownerOptional ||
+          toleratesAbsence(member.type),
+        hasInitializer: ownerHasInitializer || defaultedNames.has(name),
+      });
+    }
+  };
+
+  const visitParameters = (
+    parameters: ts.NodeArray<ts.ParameterDeclaration>,
+  ) => {
+    for (const parameter of parameters) {
+      const owner = parameter.name.getText();
+      const optional = parameter.questionToken !== undefined;
+      const hasInitializer = parameter.initializer !== undefined;
+      const defaultedNames = defaultedBindingNames(parameter);
+      if (isLocaleType(parameter.type, alias)) {
+        sites.push({
+          where: owner,
+          optional: optional || toleratesAbsence(parameter.type),
+          hasInitializer,
+        });
+        continue;
+      }
+      const type = parameter.type;
+      if (type && ts.isTypeLiteralNode(type)) {
+        collectFromMembers(
+          type.members,
+          owner,
+          optional,
+          hasInitializer,
+          defaultedNames,
+        );
+      } else if (type && ts.isTypeReferenceNode(type)) {
+        const referenced = findDeclaration(source, type.typeName.getText());
+        if (referenced && ts.isInterfaceDeclaration(referenced)) {
+          collectFromMembers(
+            referenced.members,
+            type.typeName.getText(),
+            optional,
+            hasInitializer,
+            defaultedNames,
+          );
+        }
+      }
+    }
+  };
+
+  if (ts.isInterfaceDeclaration(declaration)) {
+    collectFromMembers(
+      declaration.members,
+      declaration.name.text,
+      false,
+      false,
+    );
+  } else {
+    visitParameters(declaration.parameters);
+  }
+
+  return sites;
+}
+
+/**
+ * Every `??` / `||` in the file whose left side reads a locale. This is the
+ * "move the default into a variable" escape: `opts.locale ?? fallback` is the
+ * same bug as `opts.locale ?? defaultLocale`, and neither is allowed here.
+ */
+export function localeFallbacks(source: ts.SourceFile): string[] {
+  const found: string[] = [];
+  const visit = (node: ts.Node): void => {
+    if (ts.isBinaryExpression(node)) {
+      const op = node.operatorToken.kind;
+      if (
+        op === ts.SyntaxKind.QuestionQuestionToken ||
+        op === ts.SyntaxKind.BarBarToken
+      ) {
+        const left = node.left.getText();
+        if (/locale/i.test(left)) found.push(node.getText());
+      }
+    }
+    ts.forEachChild(node, visit);
+  };
+  ts.forEachChild(source, visit);
+  return found;
+}
+
+export function parse(file: string, text?: string): ts.SourceFile {
+  return ts.createSourceFile(
+    file,
+    text ?? readFileSync(resolve(REPO_ROOT, file), "utf8"),
+    ts.ScriptTarget.Latest,
+    true,
+  );
+}

src/__tests__/narrated-locale-guard-defeat.test.ts

@@ -0,0 +1,259 @@
+import { describe, expect, it } from "vitest";
+
+import { localeSites, localeFallbacks, parse } from "./helpers/narrated-locale";
+
+/**
+ * The guard checked against a guard.
+ *
+ * A structural check that cannot fail is worse than no check: it reads as
+ * proof and is not. So each case below is a disguise someone would plausibly
+ * reach for while putting the English default back — renaming the parameter,
+ * aliasing the type, hiding the fallback behind a variable, defaulting the
+ * whole options object instead of the field — and each one is asserted to be
+ * caught by the SAME detector the real guard runs.
+ *
+ * These sources are strings on purpose. They are the shapes that must not
+ * ship, so nothing here may live in a file the app can import.
+ */
+
+const FILE = "disguise.ts";
+
+describe("the locale guard cannot be talked around", () => {
+  it("catches the plain optional it was written for", () => {
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export function narrate(opts: { locale?: Locale }) { return opts; }`,
+      ),
+      "narrate",
+    );
+    expect(site.optional).toBe(true);
+  });
+
+  it("catches a renamed parameter — it reads the type, not the name", () => {
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export function narrate(opts: { lang?: Locale }) { return opts; }`,
+      ),
+      "narrate",
+    );
+    expect(site.where).toBe("opts.lang");
+    expect(site.optional).toBe(true);
+  });
+
+  it("catches an aliased type import", () => {
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale as L } from "@/lib/i18n/config";
+         export function narrate(opts: { lang?: L }) { return opts; }`,
+      ),
+      "narrate",
+    );
+    expect(site.optional).toBe(true);
+  });
+
+  it("catches a required field on a DEFAULTED options object", () => {
+    // The field says `locale: Locale`, so a field-only check reads as clean —
+    // but `= {}` means the caller can still say nothing.
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export function narrate(opts: { locale: Locale } = {} as never) { return opts; }`,
+      ),
+      "narrate",
+    );
+    expect(site.hasInitializer).toBe(true);
+  });
+
+  it("catches a required field on an OPTIONAL options object", () => {
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export function narrate(opts?: { locale: Locale }) { return opts; }`,
+      ),
+      "narrate",
+    );
+    expect(site.optional).toBe(true);
+  });
+
+  it("catches an optional locale hidden in a same-file interface", () => {
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export interface Opts { period: string; locale?: Locale }
+         export function narrate(opts: Opts) { return opts; }`,
+      ),
+      "narrate",
+    );
+    expect(site.optional).toBe(true);
+  });
+
+  it("catches a union that merely tolerates undefined", () => {
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export function narrate(opts: { locale?: Locale | undefined }) { return opts; }`,
+      ),
+      "narrate",
+    );
+    expect(site.optional).toBe(true);
+  });
+
+  it("catches `Locale | undefined` written WITHOUT the question mark", () => {
+    // The nastiest disguise, because it reads as required at a glance: no `?`,
+    // no default, and the field is spelled out in the signature. But the caller
+    // can still pass `undefined`, the body picks a language, and the sentence
+    // comes back in English — the original bug, one hat later.
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export function narrate(opts: { locale: Locale | undefined }) {
+           return opts.locale === undefined ? "en" : opts.locale;
+         }`,
+      ),
+      "narrate",
+    );
+    expect(site.optional).toBe(true);
+  });
+
+  it("catches `Locale | null` the same way", () => {
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export function narrate(opts: { locale: Locale | null }) {
+           return opts.locale === null ? "en" : opts.locale;
+         }`,
+      ),
+      "narrate",
+    );
+    expect(site.optional).toBe(true);
+  });
+
+  it("catches a locale widened to `any`", () => {
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export function narrate(opts: { locale: Locale | any }) { return opts; }`,
+      ),
+      "narrate",
+    );
+    expect(site.optional).toBe(true);
+  });
+
+  it("catches a default hidden in the parameter's own binding pattern", () => {
+    // The field is required in the type and the parameter has no initializer,
+    // so both of the earlier checks read clean — the default sits on the
+    // destructuring instead.
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export function narrate({ locale = "en" }: { locale: Locale }) { return locale; }`,
+      ),
+      "narrate",
+    );
+    expect(site.hasInitializer).toBe(true);
+  });
+
+  it("leaves a plainly required locale alone", () => {
+    // The shape the whole chain is supposed to have. If this ever reads as a
+    // violation the guard stops being a guard and starts being noise.
+    const [site] = localeSites(
+      parse(
+        FILE,
+        `import type { Locale } from "@/lib/i18n/config";
+         export function narrate(opts: { locale: Locale }) { return opts.locale; }`,
+      ),
+      "narrate",
+    );
+    expect(site.optional).toBe(false);
+    expect(site.hasInitializer).toBe(false);
+  });
+
+  it("refuses to pass a declaration that dropped the locale entirely", () => {
+    expect(() =>
+      localeSites(
+        parse(
+          FILE,
+          `export function narrate(series: string[]) { return series; }`,
+        ),
+        "narrate",
+      ),
+    ).not.toThrow();
+    expect(
+      localeSites(
+        parse(
+          FILE,
+          `export function narrate(series: string[]) { return series; }`,
+        ),
+        "narrate",
+      ),
+    ).toEqual([]); // the guard's own `length > 0` assertion is what fails on this
+  });
+
+  it("throws rather than quietly passing when the declaration is gone", () => {
+    expect(() =>
+      localeSites(parse(FILE, `export const x = 1;`), "narrate"),
+    ).toThrow(/could not find/);
+  });
+
+  it("catches the default moved into a variable", () => {
+    expect(
+      localeFallbacks(
+        parse(
+          FILE,
+          `const fallback = "en";
+           export function narrate(opts: { locale?: string }) {
+             return opts.locale ?? fallback;
+           }`,
+        ),
+      ),
+    ).not.toEqual([]);
+  });
+
+  it("catches the default written with || instead of ??", () => {
+    expect(
+      localeFallbacks(
+        parse(
+          FILE,
+          `export const l = (o: { locale?: string }) => o.locale || "en";`,
+        ),
+      ),
+    ).not.toEqual([]);
+  });
+
+  it("catches a renamed field's fallback when it still reads like a locale", () => {
+    expect(
+      localeFallbacks(
+        parse(
+          FILE,
+          `export const l = (o: { readerLocale?: string }) => o.readerLocale ?? "en";`,
+        ),
+      ),
+    ).not.toEqual([]);
+  });
+
+  it("does not fire on a fallback that has nothing to do with language", () => {
+    // A guard that flags everything gets silenced, so it has to stay quiet
+    // where it should.
+    expect(
+      localeFallbacks(
+        parse(
+          FILE,
+          `export const n = (o: { minPairs?: number }) => o.minPairs ?? 20;`,
+        ),
+      ),
+    ).toEqual([]);
+  });
+});

src/__tests__/narrated-locale-required-guard.test.ts

@@ -0,0 +1,80 @@
+import { describe, expect, it } from "vitest";
+
+import { queryKeys } from "@/lib/query-keys";
+import {
+  CHAIN,
+  WHY_NO_DEFAULT,
+  localeFallbacks,
+  localeSites,
+  parse,
+} from "./helpers/narrated-locale";
+
+/**
+ * A function that writes a sentence for a person must be told which language.
+ *
+ * The metric page's "Coach read" strip printed its own summary in German and
+ * the pattern line under it in English, one under the other, for months. The
+ * translations existed in all six languages the whole time. What broke was the
+ * shape of the call: the discovery engine took `locale?: Locale` and fell back
+ * to English, and the route that knew who was reading simply never said. Nobody
+ * had to write anything wrong — staying quiet was enough, and the result read
+ * like a finished sentence, so nothing looked broken from the code's side.
+ *
+ * A default is what made silence survivable. So on this chain there is none:
+ * every locale a narrated sentence depends on is a REQUIRED parameter, and a
+ * caller that cannot name one does not compile. This guard is what keeps that
+ * true, because "optional with a sensible default" reads like a kindness every
+ * time someone adds the next caller.
+ *
+ * It reads types, not names. Renaming the parameter, or lifting the default
+ * into a variable, does not get past it — see the disguises in
+ * `narrated-locale-guard-defeat.test.ts`, which run this same detector.
+ */
+
+describe("a narrated sentence's locale stays required", () => {
+  for (const { file, declaration } of CHAIN) {
+    it(`${declaration} (${file}) accepts a locale, and it is required`, () => {
+      const sites = localeSites(parse(file), declaration);
+
+      // Deleting the parameter must not pass as "nothing optional found".
+      expect(
+        sites.length,
+        `${WHY_NO_DEFAULT}\n\n\`${declaration}\` no longer accepts a locale at ` +
+          `all. It writes prose; it has to be told the language.`,
+      ).toBeGreaterThan(0);
+
+      for (const site of sites) {
+        expect(
+          site.optional,
+          `${WHY_NO_DEFAULT}\n\nOptional locale at \`${declaration}\` → ${site.where}.`,
+        ).toBe(false);
+        expect(
+          site.hasInitializer,
+          `${WHY_NO_DEFAULT}\n\nDefaulted locale at \`${declaration}\` → ${site.where}.`,
+        ).toBe(false);
+      }
+    });
+  }
+
+  for (const file of [...new Set(CHAIN.map((entry) => entry.file))]) {
+    it(`${file} falls back to no language of its own`, () => {
+      const fallbacks = localeFallbacks(parse(file));
+      expect(
+        fallbacks,
+        `${WHY_NO_DEFAULT}\n\nA locale fallback in ${file}: ` +
+          `${fallbacks.join(", ")}. Naming the fallback something else does ` +
+          `not change what it does.`,
+      ).toEqual([]);
+    });
+  }
+
+  it("keys the strip's browser cache by language", () => {
+    // The server-side fix is invisible to anyone who switches language rather
+    // than reloads if the cached entry is shared across languages.
+    const german = queryKeys.insightsCoachRead("WEIGHT", "de");
+    const english = queryKeys.insightsCoachRead("WEIGHT", "en");
+    expect(german).not.toEqual(english);
+    expect(german).toContain("de");
+    expect(english).toContain("en");
+  });
+});