fix(content): stop caching blank page bodies from transient GitHub failures

.github/workflows/unit-tests.yml

@@ -0,0 +1,32 @@
+name: Unit tests
+
+# Least privilege: this job only reads the tree.
+permissions:
+  contents: read
+
+on:
+  pull_request:
+  push:
+    branches: [main]
+
+jobs:
+  lib-tests:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          persist-credentials: false
+      - uses: actions/setup-node@v4
+        with:
+          node-version: 24 # matches seo-discovery/menu-labels/egress
+          cache: yarn
+      - name: Install dependencies
+        # --ignore-scripts so a malicious PR dependency cannot run lifecycle code.
+        run: yarn install --frozen-lockfile --ignore-scripts
+      - name: Run library unit tests
+        # Scoped to src/lib deliberately. The two component/hook suites under
+        # src/components and src/hooks import @testing-library/react and
+        # @testing-library/user-event, which are declared in neither
+        # package.json nor yarn.lock, so they cannot run. That is pre-existing
+        # rot; widening this job means declaring those deps first.
+        run: npx jest src/lib --ci

src/lib/__tests__/authAndFetch.test.ts

@@ -0,0 +1,161 @@
+/**
+ * Regression tests for the silent-blank-page class of failure.
+ *
+ * The bug these guard against (#720): a transient GitHub contents-API failure
+ * resolved to `null`, `unstable_cache` stored that `null` under the path, and
+ * `[...slug]/page.tsx` rendered an empty article for it at HTTP 200. The
+ * invariant is therefore narrow and load-bearing — **only a genuine 404 may
+ * produce `null`**. Every other outcome must reject, so nothing is cached and
+ * the next request retries.
+ *
+ * `unstable_cache` is stubbed to a pass-through: these assert the behaviour of
+ * the wrapped function, which is what determines whether a bad value is
+ * cacheable at all.
+ */
+
+const mockGetContent = jest.fn();
+
+jest.mock("octokit", () => ({
+  Octokit: jest.fn().mockImplementation(() => ({
+    rest: { repos: { getContent: mockGetContent } },
+  })),
+}));
+
+jest.mock("next/cache", () => ({
+  unstable_cache: (fn: unknown) => fn,
+  revalidateTag: jest.fn(),
+}));
+
+jest.mock("@/lib/helpers", () => ({
+  getFiles: (data: unknown) =>
+    Array.isArray(data) ? data.map((e: { path: string }) => e.path) : [],
+  transformUri: (uri: string) => uri,
+}));
+
+type Mod = typeof import("../authAndFetch");
+let mod: Mod;
+
+const PATH = "site/Using_Zcash/zimppy.md";
+
+/** An octokit HTTP error carries the status on the error object. */
+const httpError = (status: number, message = `HTTP ${status}`) =>
+  Object.assign(new Error(message), { status });
+
+const fileResponse = (content: string) => ({
+  data: {
+    type: "file",
+    encoding: "base64",
+    content: Buffer.from(content, "utf-8").toString("base64"),
+  },
+});
+
+beforeAll(async () => {
+  process.env.OWNER = "ZecHub";
+  process.env.REPO = "zechub";
+  process.env.BRANCH = "main";
+  jest.spyOn(console, "error").mockImplementation(() => {});
+  jest.spyOn(console, "log").mockImplementation(() => {});
+  mod = await import("../authAndFetch");
+});
+
+beforeEach(() => mockGetContent.mockReset());
+
+describe("getFileContentCached — only a 404 may yield null", () => {
+  it("returns the decoded file on success", async () => {
+    mockGetContent.mockResolvedValueOnce(fileResponse("# Zimppy\n\nBody."));
+    await expect(mod.getFileContentCached(PATH)).resolves.toBe(
+      "# Zimppy\n\nBody.",
+    );
+  });
+
+  it("returns null when the file and its folder are both genuinely absent", async () => {
+    mockGetContent.mockRejectedValue(httpError(404));
+    await expect(mod.getFileContentCached(PATH)).resolves.toBeNull();
+  });
+
+  // The core regression. Before the fix each of these resolved to `null` and
+  // was cached forever, blanking the page until a redeploy.
+  it.each([
+    [403, "secondary rate limit"],
+    [429, "too many requests"],
+    [500, "internal server error"],
+    [502, "bad gateway"],
+  ])("rejects on a transient %i so nothing is cached", async (status) => {
+    mockGetContent.mockRejectedValue(httpError(status));
+    await expect(mod.getFileContentCached(PATH)).rejects.toMatchObject({
+      status,
+    });
+  });
+
+  it("rejects when the folder scan itself fails transiently", async () => {
+    // Exact path is a clean 404, so the scan is reached; the scan then hits a
+    // rate limit. That must not be read as "the page does not exist".
+    mockGetContent
+      .mockRejectedValueOnce(httpError(404))
+      .mockRejectedValueOnce(httpError(403));
+    await expect(mod.getFileContentCached(PATH)).rejects.toMatchObject({
+      status: 403,
+    });
+  });
+});
+
+describe("getFileContentCached — responses that are not a readable file", () => {
+  it("rejects a directory response instead of decoding it to an empty body", async () => {
+    mockGetContent.mockResolvedValueOnce({
+      data: [{ path: "site/Using_Zcash/a.md", type: "file" }],
+    });
+    await expect(mod.getFileContentCached(PATH)).rejects.toThrow(
+      /is a directory/,
+    );
+  });
+
+  it("rejects a blob too large to inline (>1 MB) instead of returning an empty body", async () => {
+    mockGetContent.mockResolvedValueOnce({
+      data: { type: "file", encoding: "none", content: "", size: 1_400_000 },
+    });
+    await expect(mod.getFileContentCached(PATH)).rejects.toThrow(
+      /no inline content/,
+    );
+  });
+
+  it("rejects a submodule or symlink entry", async () => {
+    mockGetContent.mockResolvedValueOnce({
+      data: { type: "symlink", target: "../elsewhere.md" },
+    });
+    await expect(mod.getFileContentCached(PATH)).rejects.toThrow(
+      /is not a file/,
+    );
+  });
+});
+
+describe("getFileContentCached — case-insensitive folder fallback", () => {
+  const REQUESTED = "site/ZFAV_Club/Guides_for_Creators/ai-tools.md";
+
+  it("resolves a file whose real name differs only in case or separators", async () => {
+    mockGetContent
+      .mockRejectedValueOnce(httpError(404))
+      .mockResolvedValueOnce({
+        data: [
+          { path: "site/ZFAV_Club/Guides_for_Creators/AI_tools.md" },
+          { path: "site/ZFAV_Club/Guides_for_Creators/AI_tools_for_offline.md" },
+        ],
+      })
+      .mockResolvedValueOnce(fileResponse("# AI tools"));
+    await expect(mod.getFileContentCached(REQUESTED)).resolves.toBe(
+      "# AI tools",
+    );
+  });
+
+  // Before the fix the scan also accepted `normalize(file).includes(slug)`, so
+  // this served a different article's body under the requested path.
+  it("does not match a sibling that merely contains the slug", async () => {
+    mockGetContent.mockRejectedValueOnce(httpError(404)).mockResolvedValueOnce({
+      data: [
+        { path: "site/ZFAV_Club/Guides_for_Creators/AI_tools_for_offline.md" },
+      ],
+    });
+    await expect(mod.getFileContentCached(REQUESTED)).resolves.toBeNull();
+    // three calls would mean it fetched the wrong file
+    expect(mockGetContent).toHaveBeenCalledTimes(2);
+  });
+});

src/lib/authAndFetch.ts

@@ -34,9 +34,60 @@ function normalize(str: string): string {
     .replace(/[-_ ]+/g, "");
 }
 
+/**
+ * A 404 is a real answer: the file is not there, and `null` is the correct
+ * result to cache. Anything else — 403/429 rate limiting, 5xx, a network
+ * blip, an expired token — is transient, and caching `null` for it would
+ * blank the page permanently (these caches are keyed by path and, before
+ * this change, never revalidated). Rethrow so `unstable_cache` stores
+ * nothing and the next request retries.
+ */
+function isMissing(err: any): boolean {
+  return err?.status === 404;
+}
+
+function rethrowIfTransient(err: any, path: string): void {
+  if (isMissing(err)) return;
+  console.error(
+    `[authAndFetch] transient GitHub failure for ${path} (status ${err?.status}): ${err?.response?.data?.message ?? err?.message}`,
+  );
+  throw err;
+}
+
+/**
+ * Decode a contents-API response, or throw if it is not a readable file.
+ *
+ * `getContent` does not only return files: a directory comes back as an
+ * array, and a blob over 1 MB comes back with `encoding: "none"` and an
+ * empty `content`. Blindly decoding `res.data?.content || ""` turns both
+ * into `""`, which `[...slug]/page.tsx` cannot tell apart from a missing
+ * page — the same silent-blank failure this module is being hardened
+ * against. Throwing routes them to the transient path instead, so nothing
+ * is cached and the condition stays visible in the logs.
+ */
+function decodeFileContent(data: any, path: string): string {
+  if (Array.isArray(data)) {
+    throw new Error(`[authAndFetch] ${path} is a directory, not a file`);
+  }
+  if (data?.type !== "file") {
+    throw new Error(`[authAndFetch] ${path} is not a file (type ${data?.type})`);
+  }
+  if (data?.encoding !== "base64" || !data?.content) {
+    // Typically a >1MB blob: the API omits inline content in this case.
+    throw new Error(
+      `[authAndFetch] ${path} has no inline content (encoding ${data?.encoding}, size ${data?.size})`,
+    );
+  }
+  return Buffer.from(data.content, "base64").toString("utf-8");
+}
+
 export const getFileContentCached = unstable_cache(
   async (path: string) => {
-    if (!assertRepoConfig()) return null;
+    // Not `return null`: a missing OWNER/REPO is a deploy-configuration fault,
+    // not evidence that the page is absent. Caching null here would blank every
+    // page requested during a misconfigured window — the exact failure this
+    // function is being hardened against.
+    if (!assertRepoConfig()) throw new Error("[authAndFetch] repo not configured");
     const safePath = cleanPath(path);
 
     try {
@@ -47,13 +98,12 @@ export const getFileContentCached = unstable_cache(
           path: safePath,
           ref: branch,
         });
-        // @ts-ignore
-        return Buffer.from(res.data?.content || "", "base64").toString("utf-8");
+        return decodeFileContent(res.data, safePath);
       } catch (err: any) {
-        console.log({
-          "Error! Status": err.status,
-          Message: err.response?.data?.message,
-        });
+        // Only a genuine 404 may fall through to the case-insensitive folder
+        // scan below; a transient failure must not be mistaken for "the file
+        // is not at this exact path".
+        rethrowIfTransient(err, safePath);
       }
 
       const folderPath = safePath.split("/").slice(0, -1).join("/");
@@ -62,49 +112,61 @@ export const getFileContentCached = unstable_cache(
         const slugPart = safePath.split("/").pop()?.replace(/\.md$/i, "") || "";
         const normalizedSlug = normalize(slugPart);
         for (const file of realFiles) {
-          if (
-            normalize(file) === normalizedSlug ||
-            normalize(file).includes(normalizedSlug)
-          ) {
+          // Compare basenames exactly. The previous `includes()` test matched
+          // any sibling whose name merely *contained* the slug, so `ai-tools`
+          // could resolve to `AI_tools_for_offline.md` and cache the wrong
+          // article's body under this path.
+          const base = file.split("/").pop() ?? file;
+          if (normalize(base) === normalizedSlug) {
             const res = await octokit.rest.repos.getContent({
               owner,
               repo,
               path: cleanPath(file),
               ref: branch,
             });
-            // @ts-ignore
-            return Buffer.from(res.data?.content || "", "base64").toString(
-              "utf-8",
-            );
+            return decodeFileContent(res.data, cleanPath(file));
           }
         }
       }
+      // Every lookup returned a clean 404: the page really does not exist.
+      // (Config faults and non-file responses throw above, so they cannot
+      // reach this line and be cached as "missing".)
       return null;
-    } catch {
-      return null;
+    } catch (err: any) {
+      if (isMissing(err)) return null;
+      // Propagate. Every caller wraps this in a catch that degrades to an
+      // empty render for this one request (`[...slug]/page.tsx` L440 + outer
+      // try, `api/content-md` L63); the route is force-dynamic, so no build
+      // path aborts. One recoverable empty render beats a cached blank page.
+      throw err;
     }
   },
-  ["github-file-content-cache"],
-  { revalidate: false, tags: ["github-content"] },
+  // `owner`/`repo`/`branch` are closed over rather than passed as arguments,
+  // so they must be part of the key — otherwise entries written under one
+  // repo configuration are served after that configuration changes.
+  ["github-file-content-cache", owner, repo, branch],
+  // `revalidate: false` cached forever, so a single bad entry never healed.
+  // A TTL bounds the damage of anything that still slips through.
+  { revalidate: 3600, tags: ["github-content"] },
 );
 
 const getTranslationProbeCached = unstable_cache(
   async (path: string) => {
-    if (!assertRepoConfig()) return null;
+    if (!assertRepoConfig()) throw new Error("[authAndFetch] repo not configured");
     try {
       const res = await octokit.rest.repos.getContent({
         owner,
         repo,
         path: cleanPath(path),
         ref: branch,
       });
-      // @ts-ignore
-      return Buffer.from(res.data?.content || "", "base64").toString("utf-8");
-    } catch {
-      return null;
+      return decodeFileContent(res.data, cleanPath(path));
+    } catch (err: any) {
+      if (isMissing(err)) return null;
+      throw err;
     }
   },
-  ["github-translation-probe-cache"],
+  ["github-translation-probe-cache", owner, repo, branch],
   { revalidate: 300, tags: ["github-content"] },
 );