yuris/garage
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages 1 Wiki Activity Actions

fix(store): key the catalog fetch on a per-build id so rebuilds cannot serve a stale catalog #65

Merged
yuris merged 5 commits from chore/64-catalog-cache-freshness into main 2026-08-28 03:00:06 +00:00
Conversation 11 Commits 5 Files changed 4 +123 -2
yuris commented 2026-08-28 01:47:09 +00:00
Owner
Copy link

Closes #64

apps/store/lib/catalog.ts fetched the catalog with cache: "force-cache", so a rebuild could render a previous run's catalog. The issue framed this as a verification trap. It reproduced as one — and measuring it turned up a live production bug behind it.

Merging this deploys to production. The diff touches apps/store/**, so push: main fires deploy-store.yml, which builds the store and runs wrangler pages deploy apps/store/out.

Impact: the shop's stock-freshness mechanism has been dead for four days

force-cache was introduced by 6e36a27 ("feat: build static product catalog (#4)", 2026-07-17) — verified: absent in 6e36a27^, present at lib/catalog.ts in 6e36a27, and still present in 9abbe83^, so the later contract extraction preserved it rather than adding it. It was harmless for over a month. The freeze begins at af6ab4c (2026-08-24, "cache the slow steps"), whose diff adds path: apps/store/.next/cache with those restore-keys. Before it, every CI build was cold and fetched fresh.

Successful deploy-store.yml runs from that point (ids below are /actions/tasks .id values, not /actions/runs .index_in_repo — the two are different sequences, and 949/952 are index_in_repo 79/82):

  • 949 — af6ab4c itself, cold. Nothing to restore, so it fetched fresh and saved the cache.
  • 952 (2026-08-24T12:39:16Z, push, bed5b851) — the first frozen deploy. bed5b851 touches apps/store/app/** so the path filter matched, and pnpm-lock.yaml is unchanged between af6ab4c and bed5b851, so its first restore-key next-store-<lockhash>- is a direct prefix hit on 949's saved cache.
  • 6 workflow_dispatch deploys on 2026-08-25 — 953, 960, 963, 967, 968, 973. This is the trigger management fires after publishing an inventory item, whose whole purpose is "so stock edits reach the live site without a commit." None of them could change the site.
  • 7 push deploys on 2026-08-27/28 — 974, 976, 977, 979, 980, 981, 984.

14 deploys republished a frozen catalog, over a window opening 2026-08-24. Credit to the reviewer for dating the window, catching that 952 is the true start, and correcting the force-cache attribution; all re-derived here with git show/git ls-tree and the forge task list rather than relayed.

What production actually needs (measured before touching the fetch)

  • next.config.ts sets output: "export". The fetch must stay cached. cache: "no-store" does not merely change performance, it fails the build. What the build prints is:

    Error occurred prerendering page "/cart".
Error: Catalog endpoint is unavailable

    which names the wrong cause — the bare catch in fetchCatalog rewrites every throw from the fetch. To recover the real one I instrumented that catch with console.error("RAWERR", e) and rebuilt:

    RAWERR Error: Route /cart with `dynamic = "error"` couldn't be rendered statically
because it used `revalidate: 0 fetch .../api/public/catalog /cart`.
    at v (...)
{ code: 'NEXT_STATIC_GEN_BAILOUT' }

    Neither path prints the string NEXT_STATIC_GEN_BAILOUT on its own. It is a property on the thrown error, not part of any message; it appears above only because console.error was handed the error object and Node's inspector appends own enumerable properties. Delete the catch instead of instrumenting it and you get the Route /cart … message alone. Stated precisely because the distinction is the whole point of #66, and because an earlier revision of this body presented the code line as ordinary build output, which it is not.

    So removing the cache was never an option; force-cache is load-bearing. The masking is filed as #66 — it is why this evidence needed a code change to obtain, and why the next person to test this constraint will be told the API is down.

  • deploy-store.yml builds once per run and deploys the export. It also restores apps/store/.next/cache between runs, with restore-keys (next-store-<lockhash>-, next-store-) that match even when sources change.

  • The catalog is read by 6 call sites across ~16 prerendered pages. The cache is what collapses those into one request and one consistent snapshot — a real property worth keeping, not just speed.

The trap reproduces, and it is worse than "fools you once"

Against a local fixture server logging every hit:

  1. Fixture price: 111111 → build → output 111111. Server: HIT #1.
  2. Fixture → 222222, rebuild with .next intact, no source change → output still 111111, zero occurrences of 222222. Server logged no second request.
  3. Touched lib/format.ts (a real source change) and rebuilt → still 111111 (13 occurrences, zero 222222).

The persisted entry:

meta: {"kind":"FETCH","revalidate":31536000,"tags":[]}
cached generatedAt: 2026-08-28T01:00:00.000Z

revalidate: 31536000 is a one-year TTL. This is not a first-rebuild flake; the catalog is frozen until someone clears .next. Step 2 is byte-for-byte the workflow_dispatch path; step 3 shows commit-triggered deploys were exposed too, via restore-keys.

The Wait for a compatible catalog step made this harder to spot: it fetches the API with plain node fetch and confirms a good schemaVersion: 3 payload, then the build ignores that payload and uses the cached one.

Options considered

Option Result
cache: "no-store" Rejected — fails the build under output: export; the thrown error carries code: 'NEXT_STATIC_GEN_BAILOUT' (see above).
next: { revalidate: 60 } Rejected — builds fine, but a rebuild inside the window still rendered the stale fixture with no new request. A time TTL narrows the window; it does not close it.
Exclude fetch-cache from the CI cache Rejected as the primary fix — plausible, but only fixes CI, leaves local verification trapped, and I cannot run Forgejo Actions to prove the exclusion glob works. A correctness guarantee should not rest on an unverifiable CI config.
Key the fetch on a per-build id Adopted — issue option 1, provable locally.

The change

next.config.ts mints a build id once per build; lib/catalog.ts puts it in the query string:

env: { CATALOG_BUILD_ID: Date.now().toString(36) }

url.searchParams.set("build", process.env.CATALOG_BUILD_ID ?? "dev");

Constant within a build, so all ~16 pages still share one snapshot and one request; different across builds, so no build can reuse another's entry. force-cache stays, because the export requires it. The value is inlined at compile time, so every prerender worker agrees on it without env plumbing — verified below that it does not go stale under a warm compile cache.

The API is unaffected by the extra param: apps/management/app/api/[[...route]]/catalog.ts declares no request/query schema and reads only the x-api-key header, so unknown query params are ignored.

One consequence worth stating: the cache is no longer a fallback. A build can no longer borrow an earlier build's catalog, so if the management API is unreachable the build now fails instead of shipping stale data. Measured — endpoint down with a warm cache: main builds successfully off the stale entry, this branch fails with Failed to collect page data for /products/[slug]. That is the intended trade: a failed deploy leaves the previous correct site up. The exposure is also narrower than "an API outage now blocks deploys" suggests — scripts/wait-for-catalog.mts polls for 15 minutes at 15s intervals (TIMEOUT_MS = 15 * 60 * 1000, INTERVAL_MS = 15 * 1000) and treats unreachable as not-ready-yet, so an outage shorter than that never reaches the build step, and a longer one fails at the gate with a purpose-written message. Only the seconds-wide window between the gate passing and the build's own fetch can fail mid-build. Bound established by the reviewer, verified here against the script. Documented in the README.

Verification

Fixture server logs every request; fixture is schemaVersion: 3.

Fix defeats the stale render — warm .next, no source change:

COLD:                     444444=13
WARM (no source change):  555555=13   444444=0

Distinct across builds — 5 consecutive builds, 5 distinct ids, exactly one request each:

HIT #5 /api/public/catalog?build=mtcacko4 -> price 444444
HIT #6 /api/public/catalog?build=mtcacs8l -> price 555555
HIT #7 /api/public/catalog?build=mtcadrso -> price 555555
HIT #8 /api/public/catalog?build=mtcae0rq -> price 555555
HIT #9 /api/public/catalog?build=mtcae8l1 -> price 555555

Constant within a build — a server answering every request with a different price, so any fan-out or torn snapshot would show as more than one price in the export:

$ grep -rho '9000[0-9][0-9]' out/ | sort -u
900001
$ grep -o '<lastmod>[^<]*</lastmod>' out/sitemap.xml | sort -u
<lastmod>2026-08-28T07:00:01.000Z</lastmod>

Server log for that whole build: REQ #1 ?build=mtcaovqq -> price 900001. One request, one price across all 16 routes.

No artifact leak. grep -rEo 'build=[a-z0-9]{8,}|mtca[a-z0-9]+' out/ → no matches. Two builds of identical source do differ, but that is Next's own random per-build buildId, pre-existing: verified at base with this PR reverted (grep -c CATALOG_BUILD_ID → 0, 0) giving 3eDQSLMrvRUTbHQbapAX3 then DDkM6rvM2_ala_YUl8wOr.

Production build path — clean pnpm --filter garage-store build against a schemaVersion: 3 fixture: all 16 routes prerendered, /products/oli-mesin and 6 service pages generated from catalog data.

Provenance marker — the catalog's own generatedAt is already rendered into the sitemap:

fixture generatedAt:   2026-08-28T06:00:00.000Z
built sitemap lastmod: 2026-08-28T06:00:00.000Z

Tests and lint (re-run at 1f077a6):

$ pnpm --filter garage-store test
# tests 91 / # pass 91 / # fail 0

$ pnpm exec biome --version
Version: 2.4.16

$ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts next.config.ts
Checked 3 files in 5ms. No fixes applied.

That is a clean result, not merely a zero exit code — the two are different claims. pnpm exec (never npx, which resolves an unrelated biome@0.3.3 that checks nothing and exits 0); the output has a Checked N files line proving the binary ran; and it has no Found N warnings/errors line, which is what actually means zero findings. Biome exits 0 on warnings, so the exit code proves nothing on its own. Confirmed the binary can fail by injecting const _unused: any = 1; into catalog.ts — it reported Found 1 warning while still exiting 0 — then restored the file.

lib/catalog.test.ts covers the change: the URL assertion now includes the build id, plus a new test that two build ids produce two different URLs — that URL is the cache key, so a shared URL is exactly the old bug.

Audit of the class

Grepped force-cache|no-store|revalidate|cache:\s*"|unstable_cache|dynamic\s*= across apps/, packages/, .forgejo/. One instance — the one fixed here. No twin left behind:

  • store/app/robots.ts, store/app/sitemap.ts: dynamic = "force-static", correct for export.
  • management/(dashboard)/page.tsx, laporan/page.tsx: force-dynamic, runtime rendering, opposite concern.
  • management/api/.../mcp.ts: cache-control: no-store is a response header, not a fetch cache.

Acceptance criteria

  • Verification path provably fresh or documented as needing a defeat — both. Fresh by construction, and apps/store/README.md gains a "Verifying a change against storefront output" section.
  • Anyone reading storefront output can tell which fixture produced it — documented sitemap.xml <lastmod> provenance check, with the caveat that it tracks generatedAt rather than contents, so a fixture edit must bump it or use a sentinel grep.
  • Production caching unchanged or changed with a stated reason grounded in the deploy — changed, grounded: the deploy restores .next/cache, which froze 14 deploys over the four days from 2026-08-24.
  • Cache-defeat procedure sent to Author57 — sent twice: an interim method immediately on request, then the confirmed procedure with measurements.

Noted, deliberately not fixed here

The bare catch around the fetch rewrites any throw into "Catalog endpoint is unavailable" — including the static-generation bailout, which is how a caching misconfiguration currently presents as a network outage. Filed as #66 rather than expanding this diff.

.next/cache/fetch-cache in CI now accumulates one small entry per deploy instead of reusing one. Next prunes its own cache and the entries are ~1 KB; not worth an unverifiable workflow change.

Reviewer note

No CI runs on pull requests here — both workflows are push: main + workflow_dispatch. Nothing has checked this branch automatically. Every result above is local command output, quoted as produced, except the static-generation bailout block, which is explicitly labelled as obtained with the masking catch removed.

Closes #64 `apps/store/lib/catalog.ts` fetched the catalog with `cache: "force-cache"`, so a rebuild could render a previous run's catalog. The issue framed this as a verification trap. It reproduced as one — and measuring it turned up a live production bug behind it. **Merging this deploys to production.** The diff touches `apps/store/**`, so `push: main` fires `deploy-store.yml`, which builds the store and runs `wrangler pages deploy apps/store/out`. ## Impact: the shop's stock-freshness mechanism has been dead for four days `force-cache` was introduced by `6e36a27` ("feat: build static product catalog (#4)", 2026-07-17) — verified: absent in `6e36a27^`, present at `lib/catalog.ts` in `6e36a27`, and still present in `9abbe83^`, so the later contract extraction preserved it rather than adding it. It was harmless for over a month. The freeze begins at `af6ab4c` (2026-08-24, *"cache the slow steps"*), whose diff adds `path: apps/store/.next/cache` with those `restore-keys`. Before it, every CI build was cold and fetched fresh. Successful `deploy-store.yml` runs from that point (ids below are `/actions/tasks` `.id` values, **not** `/actions/runs` `.index_in_repo` — the two are different sequences, and 949/952 are `index_in_repo` 79/82): - **949** — `af6ab4c` itself, **cold**. Nothing to restore, so it fetched fresh and saved the cache. - **952** (2026-08-24T12:39:16Z, push, `bed5b851`) — **the first frozen deploy.** `bed5b851` touches `apps/store/app/**` so the path filter matched, and `pnpm-lock.yaml` is unchanged between `af6ab4c` and `bed5b851`, so its first restore-key `next-store-<lockhash>-` is a direct prefix hit on 949's saved cache. - **6 `workflow_dispatch` deploys** on 2026-08-25 — 953, 960, 963, 967, 968, 973. This is the trigger management fires after publishing an inventory item, whose whole purpose is *"so stock edits reach the live site without a commit."* None of them could change the site. - **7 `push` deploys** on 2026-08-27/28 — 974, 976, 977, 979, 980, 981, 984. **14 deploys republished a frozen catalog, over a window opening 2026-08-24.** Credit to the reviewer for dating the window, catching that 952 is the true start, and correcting the `force-cache` attribution; all re-derived here with `git show`/`git ls-tree` and the forge task list rather than relayed. ## What production actually needs (measured before touching the fetch) - `next.config.ts` sets `output: "export"`. **The fetch must stay cached.** `cache: "no-store"` does not merely change performance, it fails the build. **What the build prints** is: ``` Error occurred prerendering page "/cart". Error: Catalog endpoint is unavailable ``` which names the wrong cause — the bare `catch` in `fetchCatalog` rewrites every throw from the fetch. To recover the real one I instrumented that catch with `console.error("RAWERR", e)` and rebuilt: ``` RAWERR Error: Route /cart with `dynamic = "error"` couldn't be rendered statically because it used `revalidate: 0 fetch .../api/public/catalog /cart`. at v (...) { code: 'NEXT_STATIC_GEN_BAILOUT' } ``` **Neither path prints the string `NEXT_STATIC_GEN_BAILOUT` on its own.** It is a property on the thrown error, not part of any message; it appears above only because `console.error` was handed the error *object* and Node's inspector appends own enumerable properties. Delete the catch instead of instrumenting it and you get the `Route /cart …` message alone. Stated precisely because the distinction is the whole point of #66, and because an earlier revision of this body presented the `code` line as ordinary build output, which it is not. So removing the cache was never an option; `force-cache` is load-bearing. The masking is filed as **#66** — it is why this evidence needed a code change to obtain, and why the next person to test this constraint will be told the API is down. - `deploy-store.yml` builds once per run and deploys the export. It also **restores `apps/store/.next/cache`** between runs, with `restore-keys` (`next-store-<lockhash>-`, `next-store-`) that match even when sources change. - The catalog is read by 6 call sites across ~16 prerendered pages. The cache is what collapses those into one request and one consistent snapshot — a real property worth keeping, not just speed. ## The trap reproduces, and it is worse than "fools you once" Against a local fixture server logging every hit: 1. Fixture `price: 111111` → build → output `111111`. Server: `HIT #1`. 2. Fixture → `222222`, rebuild **with `.next` intact, no source change** → output still `111111`, **zero** occurrences of `222222`. Server logged **no second request**. 3. Touched `lib/format.ts` (a real source change) and rebuilt → **still `111111`** (13 occurrences, zero `222222`). The persisted entry: ``` meta: {"kind":"FETCH","revalidate":31536000,"tags":[]} cached generatedAt: 2026-08-28T01:00:00.000Z ``` `revalidate: 31536000` is a **one-year TTL**. This is not a first-rebuild flake; the catalog is frozen until someone clears `.next`. Step 2 is byte-for-byte the `workflow_dispatch` path; step 3 shows commit-triggered deploys were exposed too, via `restore-keys`. The `Wait for a compatible catalog` step made this harder to spot: it fetches the API with plain `node` fetch and confirms a good `schemaVersion: 3` payload, then the build ignores that payload and uses the cached one. ## Options considered | Option | Result | |---|---| | `cache: "no-store"` | **Rejected** — fails the build under `output: export`; the thrown error carries `code: 'NEXT_STATIC_GEN_BAILOUT'` (see above). | | `next: { revalidate: 60 }` | **Rejected** — builds fine, but a rebuild inside the window still rendered the stale fixture with no new request. A time TTL narrows the window; it does not close it. | | Exclude `fetch-cache` from the CI cache | **Rejected as the primary fix** — plausible, but only fixes CI, leaves local verification trapped, and I cannot run Forgejo Actions to prove the exclusion glob works. A correctness guarantee should not rest on an unverifiable CI config. | | **Key the fetch on a per-build id** | **Adopted** — issue option 1, provable locally. | ## The change `next.config.ts` mints a build id once per build; `lib/catalog.ts` puts it in the query string: ```ts env: { CATALOG_BUILD_ID: Date.now().toString(36) } ``` ```ts url.searchParams.set("build", process.env.CATALOG_BUILD_ID ?? "dev"); ``` Constant within a build, so all ~16 pages still share one snapshot and one request; different across builds, so no build can reuse another's entry. `force-cache` stays, because the export requires it. The value is inlined at compile time, so every prerender worker agrees on it without env plumbing — verified below that it does not go stale under a warm compile cache. The API is unaffected by the extra param: `apps/management/app/api/[[...route]]/catalog.ts` declares no request/query schema and reads only the `x-api-key` header, so unknown query params are ignored. **One consequence worth stating: the cache is no longer a fallback.** A build can no longer borrow an earlier build's catalog, so if the management API is unreachable the build now fails instead of shipping stale data. Measured — endpoint down with a warm cache: `main` builds successfully off the stale entry, this branch fails with `Failed to collect page data for /products/[slug]`. That is the intended trade: a failed deploy leaves the previous correct site up. The exposure is also narrower than "an API outage now blocks deploys" suggests — `scripts/wait-for-catalog.mts` polls for **15 minutes at 15s intervals** (`TIMEOUT_MS = 15 * 60 * 1000`, `INTERVAL_MS = 15 * 1000`) and treats unreachable as not-ready-yet, so an outage shorter than that never reaches the build step, and a longer one fails at the gate with a purpose-written message. Only the seconds-wide window between the gate passing and the build's own fetch can fail mid-build. Bound established by the reviewer, verified here against the script. Documented in the README. ## Verification Fixture server logs every request; fixture is `schemaVersion: 3`. **Fix defeats the stale render** — warm `.next`, no source change: ``` COLD: 444444=13 WARM (no source change): 555555=13 444444=0 ``` **Distinct across builds** — 5 consecutive builds, 5 distinct ids, exactly one request each: ``` HIT #5 /api/public/catalog?build=mtcacko4 -> price 444444 HIT #6 /api/public/catalog?build=mtcacs8l -> price 555555 HIT #7 /api/public/catalog?build=mtcadrso -> price 555555 HIT #8 /api/public/catalog?build=mtcae0rq -> price 555555 HIT #9 /api/public/catalog?build=mtcae8l1 -> price 555555 ``` **Constant within a build** — a server answering **every request with a different price**, so any fan-out or torn snapshot would show as more than one price in the export: ``` $ grep -rho '9000[0-9][0-9]' out/ | sort -u 900001 $ grep -o '<lastmod>[^<]*</lastmod>' out/sitemap.xml | sort -u <lastmod>2026-08-28T07:00:01.000Z</lastmod> ``` Server log for that whole build: `REQ #1 ?build=mtcaovqq -> price 900001`. One request, one price across all 16 routes. **No artifact leak.** `grep -rEo 'build=[a-z0-9]{8,}|mtca[a-z0-9]+' out/` → no matches. Two builds of identical source do differ, but that is Next's own random per-build `buildId`, pre-existing: verified at base with this PR reverted (`grep -c CATALOG_BUILD_ID` → `0`, `0`) giving `3eDQSLMrvRUTbHQbapAX3` then `DDkM6rvM2_ala_YUl8wOr`. **Production build path** — clean `pnpm --filter garage-store build` against a `schemaVersion: 3` fixture: all 16 routes prerendered, `/products/oli-mesin` and 6 service pages generated from catalog data. **Provenance marker** — the catalog's own `generatedAt` is already rendered into the sitemap: ``` fixture generatedAt: 2026-08-28T06:00:00.000Z built sitemap lastmod: 2026-08-28T06:00:00.000Z ``` **Tests and lint** (re-run at `1f077a6`): ``` $ pnpm --filter garage-store test # tests 91 / # pass 91 / # fail 0 $ pnpm exec biome --version Version: 2.4.16 $ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts next.config.ts Checked 3 files in 5ms. No fixes applied. ``` That is a **clean** result, not merely a zero exit code — the two are different claims. `pnpm exec` (never `npx`, which resolves an unrelated `biome@0.3.3` that checks nothing and exits 0); the output has a `Checked N files` line proving the binary ran; and it has **no** `Found N warnings/errors` line, which is what actually means zero findings. Biome exits 0 on warnings, so the exit code proves nothing on its own. Confirmed the binary can fail by injecting `const _unused: any = 1;` into `catalog.ts` — it reported `Found 1 warning` while still exiting 0 — then restored the file. `lib/catalog.test.ts` covers the change: the URL assertion now includes the build id, plus a new test that two build ids produce two different URLs — that URL *is* the cache key, so a shared URL is exactly the old bug. ## Audit of the class Grepped `force-cache|no-store|revalidate|cache:\s*"|unstable_cache|dynamic\s*=` across `apps/`, `packages/`, `.forgejo/`. **One instance** — the one fixed here. No twin left behind: - `store/app/robots.ts`, `store/app/sitemap.ts`: `dynamic = "force-static"`, correct for export. - `management/(dashboard)/page.tsx`, `laporan/page.tsx`: `force-dynamic`, runtime rendering, opposite concern. - `management/api/.../mcp.ts`: `cache-control: no-store` is a response header, not a fetch cache. ## Acceptance criteria - *Verification path provably fresh or documented as needing a defeat* — **both.** Fresh by construction, and `apps/store/README.md` gains a "Verifying a change against storefront output" section. - *Anyone reading storefront output can tell which fixture produced it* — documented `sitemap.xml` `<lastmod>` provenance check, with the caveat that it tracks `generatedAt` rather than contents, so a fixture edit must bump it or use a sentinel grep. - *Production caching unchanged or changed with a stated reason grounded in the deploy* — changed, grounded: the deploy restores `.next/cache`, which froze **14 deploys** over the four days from 2026-08-24. - *Cache-defeat procedure sent to `Author57`* — sent twice: an interim method immediately on request, then the confirmed procedure with measurements. ## Noted, deliberately not fixed here The bare `catch` around the fetch rewrites **any** throw into `"Catalog endpoint is unavailable"` — including the static-generation bailout, which is how a caching misconfiguration currently presents as a network outage. Filed as **#66** rather than expanding this diff. `.next/cache/fetch-cache` in CI now accumulates one small entry per deploy instead of reusing one. Next prunes its own cache and the entries are ~1 KB; not worth an unverifiable workflow change. ## Reviewer note No CI runs on pull requests here — both workflows are `push: main` + `workflow_dispatch`. Nothing has checked this branch automatically. Every result above is local command output, quoted as produced, except the static-generation bailout block, which is explicitly labelled as obtained with the masking `catch` removed.
yuris commented 2026-08-28 01:51:55 +00:00
Author
Owner
Copy link

Additional evidence, at head 83a0876 (unchanged — no new commits, this comment adds measurements only).

The "constant within a build" half, measured directly

The correctness argument has two halves that pull against each other, and the PR body only showed one concretely. The 5-rebuild measurement proves the id differs across builds. Here is the other half — that it is identical within a build, so the snapshot cannot tear and the build does not fan out into ~16 requests.

Test: a fixture server that answers every request with a different price and a different generatedAt. If any two pages, or any two of the 12 prerender workers, made separate requests, the export would contain more than one price.

$ grep -rho '9000[0-9][0-9]' out/ | sort -u
900001

$ grep -o '<lastmod>[^<]*</lastmod>' out/sitemap.xml | sort -u
<lastmod>2026-08-28T07:00:01.000Z</lastmod>

$ grep -rlo '9000[0-9][0-9]' out/ | wc -l
8

Server log for that entire build:

REQ #1 /api/public/catalog?build=mtcaovqq -> price 900001

One request, one distinct price across all 16 rendered routes and 8 emitted files, and the sitemap's generatedAt matches it. A mutating source could not tear the snapshot.

Answering the merge-ordering question: no interaction with Author57

Author57 owns apps/store/app/globals.css; this PR does not touch it, so there is no file overlap. On whether the build id mints into an emitted artifact and could churn a CSS-only PR rebased onto this one — it does not:

$ grep -rEo 'build=[a-z0-9]{8,}|mtca[a-z0-9]+' out/
(no matches)

CATALOG_BUILD_ID exists only in the outbound request URL at build time. It reaches the management API and nothing else.

Two builds of identical source do differ, but for a pre-existing reason unrelated to this change: Next mints its own random buildId per build, which lands in _next/static/<buildId>/ and the "buildId" field of the RSC payload files. Verified at the base commit with this PR's changes reverted (grep -c CATALOG_BUILD_ID next.config.ts lib/catalog.ts → 0, 0):

BASE build 1: "buildId":"3eDQSLMrvRUTbHQbapAX3"
BASE build 2: "buildId":"DDkM6rvM2_ala_YUl8wOr"

So that churn is Next's default behaviour on main today, not something this PR introduces. generateBuildId is not configured, before or after.

Deliberate non-fix now filed

The bare catch masking NEXT_STATIC_GEN_BAILOUT is filed as #66, including the point that it can hide the very constraint that dictates this PR's design — which is the strongest argument for fixing it, and only surfaced because this work hit the bailout.

Additional evidence, at head `83a0876` (unchanged — no new commits, this comment adds measurements only). ## The "constant within a build" half, measured directly The correctness argument has two halves that pull against each other, and the PR body only showed one concretely. The 5-rebuild measurement proves the id *differs across builds*. Here is the other half — that it is *identical within* a build, so the snapshot cannot tear and the build does not fan out into ~16 requests. Test: a fixture server that answers **every request with a different price and a different `generatedAt`**. If any two pages, or any two of the 12 prerender workers, made separate requests, the export would contain more than one price. ``` $ grep -rho '9000[0-9][0-9]' out/ | sort -u 900001 $ grep -o '<lastmod>[^<]*</lastmod>' out/sitemap.xml | sort -u <lastmod>2026-08-28T07:00:01.000Z</lastmod> $ grep -rlo '9000[0-9][0-9]' out/ | wc -l 8 ``` Server log for that entire build: ``` REQ #1 /api/public/catalog?build=mtcaovqq -> price 900001 ``` One request, one distinct price across all 16 rendered routes and 8 emitted files, and the sitemap's `generatedAt` matches it. A mutating source could not tear the snapshot. ## Answering the merge-ordering question: no interaction with `Author57` `Author57` owns `apps/store/app/globals.css`; this PR does not touch it, so there is no file overlap. On whether the build id mints into an emitted artifact and could churn a CSS-only PR rebased onto this one — **it does not**: ``` $ grep -rEo 'build=[a-z0-9]{8,}|mtca[a-z0-9]+' out/ (no matches) ``` `CATALOG_BUILD_ID` exists only in the outbound request URL at build time. It reaches the management API and nothing else. Two builds of identical source *do* differ, but for a pre-existing reason unrelated to this change: Next mints its own random `buildId` per build, which lands in `_next/static/<buildId>/` and the `"buildId"` field of the RSC payload files. Verified at the base commit with this PR's changes reverted (`grep -c CATALOG_BUILD_ID next.config.ts lib/catalog.ts` → `0`, `0`): ``` BASE build 1: "buildId":"3eDQSLMrvRUTbHQbapAX3" BASE build 2: "buildId":"DDkM6rvM2_ala_YUl8wOr" ``` So that churn is Next's default behaviour on `main` today, not something this PR introduces. `generateBuildId` is not configured, before or after. ## Deliberate non-fix now filed The bare `catch` masking `NEXT_STATIC_GEN_BAILOUT` is filed as **#66**, including the point that it can hide the very constraint that dictates this PR's design — which is the strongest argument for fixing it, and only surfaced because this work hit the bailout.
yuris reviewed 2026-08-28 02:00:31 +00:00
yuris left a comment
Copy link

VERDICT: approve-with-nits

Re-derived every load-bearing claim in my own worktrees (/tmp/r65 at 83a0876, /tmp/r65main at origin/main) against my own logging fixture on :8975. I did not reuse any of the author's numbers. The production-bug escalation holds, and the forge has evidence for it that the PR body does not cite.

The escalation holds, and it is worse-evidenced than the PR says

deploy-store.yml:86-93 restores apps/store/.next/cache with restore-keys: next-store-<lockhash>- and a bare next-store-, so a cache is restored on every run regardless of source changes. deploy-store.yml:20-22 is the dispatch trigger with exactly the stated purpose. Reproduced on origin/main, no source change, warm .next:

cold build   -> fixture 111111, HIT #1, out 111111=13, lastmod 2026-01-11
warm rebuild -> fixture 222222, NO new request at all, out 111111=13, 222222=0, lastmod 2026-01-11

Persisted entry: {"revalidate":31536000,"kind":"FETCH","url":".../api/public/catalog"} — one-year TTL, confirmed. Then printf >> apps/store/lib/format.ts (real source change) + rebuild: still 111111=13, 222222=0, still no second request. So both the dispatch path and the commit path were frozen. Item 3 reproduces.

Two things I can add that the PR body leaves out:

  • The window is datable. force-cache has been in lib/catalog.ts since 9abbe83 (2026-07-17), but the .next/cache restore landed in af6ab4c (2026-08-24, "cache the slow steps"). Before that commit every CI build was cold and fetched fresh. So production has been broken for four days, from 2026-08-24 — not since July.
  • It was not theoretical. CLOUDFLARE_PAGES_PROJECT=garage-store is set, and CLOUDFLARE_API_TOKEN/GARAGE_API_URL/CATALOG_API_KEY/SITE_URL all exist, so the if: gate at deploy-store.yml:33 passes and the job really deploys. The task list shows six successful workflow_dispatch deploys on 2026-08-25 (953, 960, 963, 967, 968, 973) plus seven successful push deploys on 2026-08-27/28 — every one of them after af6ab4c. Each of those stock publishes republished a frozen catalog. Worth pasting into the PR body: it turns a local reproduction into an incident.

Constraint check (item 4) — holds, but the pasted output does not

cache: "no-store" does fail the build under output: export, on /cart, exactly as claimed. But at this commit the build prints:

Error occurred prerendering page "/cart".
Error: Catalog endpoint is unavailable

The NEXT_STATIC_GEN_BAILOUT text in the PR body is not what pnpm --filter garage-store build emits. I only got it after deleting the bare catch, and then it matched the quoted block word for word:

Error: Route /cart with `dynamic = "error"` couldn't be rendered statically
because it used `revalidate: 0 fetch http://127.0.0.1:8975/api/public/catalog /cart`.

So the conclusion is right and force-cache is genuinely load-bearing — but the "Reviewer note" says every result is verbatim command output, and that one is not. next: { revalidate: 60 }: also confirmed rejected — cold build rendered 333333, warm rebuild inside the window rendered 333333 again while the fixture said 444444, with no new request.

The fix (item 5) — both directions verified

Six consecutive builds on 83a0876, five of them warm with no source change:

HIT #6  ?build=mtcau7bi -> 444444   (cold)
HIT #7  ?build=mtcaudql -> 555555   warm, out 555555=13 444444=0
HIT #8  ?build=mtcaukpv -> 555555
HIT #9  ?build=mtcaur1d -> 555555
HIT #10 ?build=mtcauxfy -> 555555
HIT #11 ?build=mtcav4br -> 555555

Six builds, six distinct ids, exactly one request each. Both halves hold at once: constant within a build (1 request for 16 routes across 6 call sites, no torn snapshot) and distinct across builds (the id is re-minted even with a warm compile cache — this was my main suspicion and it is empirically dead). No build-time cost either: warm builds 8.2-9.1s on the branch vs 8.3-9.7s on main.

No artifact leak, verified with a control. Neither mtca… nor the string build= appears anywhere in apps/store/out; the id survives only in .next internals, which are not deployed. Two builds against an identical fixture produce 115 differing entries in out/ — and two builds on main produce the same 115. The churn is Next's own per-build _next/static/<id> and is unchanged by this PR.

Provenance (item 6) works and is genuinely per-fixture. sitemap.ts reads generatedAt off the catalog payload, not a clock; changing the fixture's generatedAt moved <lastmod> every time (2026-01-11 → 2026-05-05 → 2026-07-07). In production getPublicCatalog stamps request time (apps/management/lib/catalog.ts:43), which identifies the deploy — also useful. The README's manual defeat also checks out: deleting only .next/cache/fetch-cache on main refreshed the catalog (555555=0 777777=13).

Class audit (item 7) reproduces. My own sweep for force-cache|no-store|revalidate|cache:\s*"|unstable_cache|dynamic\s*=|fetchCache|cache\( over apps/, packages/, .forgejo/ returns one fetch-cache directive — lib/catalog.ts. The rest are force-static (correct for export), force-dynamic (opposite concern), and a cache-control response header in mcp.ts. No twin.

Tests defend the change. pnpm --filter garage-store test → 91/91. biome check on the 3 files → clean. And reverting only lib/catalog.ts to main's version fails exactly the two relevant tests (sends the internal key…, varies the request URL per build…) — so they are a real guard, not decoration. Diff is 4 files, no lockfile churn, Closes #64 present, no globals.css in the diff.

Findings

apps/store/lib/catalog.ts:L13: should: the comment names NEXT_STATIC_GEN_BAILOUT as what a reader will see, but the catch twenty lines below rewrites it — at this commit no-store fails with Error: Catalog endpoint is unavailable and the bailout text is invisible until that catch is removed (I confirmed both). The next person to test this constraint will conclude their fixture server is down. Add the clause: "…reported as 'Catalog endpoint is unavailable' by the catch below", and label the PR body's bailout block as taken with the catch removed rather than as verbatim build output.

apps/store/README.md:L45: nit: "If it does not match the fixture you just set, discard the result" is a one-way check, and the value it compares is the fixture's generatedAt — not its contents. Edit a price and leave generatedAt alone and lastmod matches while proving nothing, which is the same false-confidence shape #64 was filed about. Add: vary generatedAt when you edit a fixture, or use the sentinel grep.

apps/store/README.md:L52: nit: "Clearing the whole directory is the reliable form; deleting only .next/cache/fetch-cache … is enough for the catalog alone" gives two recommendations and implies the cheaper one is less reliable. It isn't — I verified fetch-cache-only defeats the catalog cache while keeping the compile cache. Say that plainly and keep rm -rf .next as the belt-and-braces form.

apps/store/README.md:L22: nit: the "cached, deliberately" paragraph now overstates what the cache does for you. Measured with the endpoint down and a warm cache: main builds successfully off the stale entry, this branch fails with Failed to collect page data for /products/[slug]. That is the right trade — a failed deploy leaves the correct old site up, and wait-for-catalog already gates reachability — but a build now genuinely requires the catalog endpoint, with no cache fallback, and nothing in the diff says so.

On the deferrals

Both are correctly out of scope and I am not filing them. On the bare catch: yes, it needs its own issue, and this review is the argument for it. It cost me a build to discover that the one constraint the whole design rests on presents as a network outage, and it corrupted the author's own evidence block in the PR that fixes the bug it masks. Rethrow anything carrying a Next digest/bailout code and keep the friendly message for real network faults. The ~1KB-per-deploy fetch-cache growth is real but trivial (6 entries / 28K in my run with a one-item fixture); leave it.

Out of scope and not a finding, but noted for whoever touches the workflow next: deploy-store.yml:90 uses brace expansion inside hashFiles('apps/store/**/*.{ts,tsx,css}'). If Forgejo's glob does not expand braces, that hash is constant and the primary cache key never changes — which would make the freeze even stickier than the restore-keys path I measured. Unverifiable without running Forgejo Actions, and this PR fixes the problem either way.

Nothing here blocks the merge. The escalation is real, the fix is provably fresh in both directions, and the constraint that forced this design is genuine.

VERDICT: approve-with-nits Re-derived every load-bearing claim in my own worktrees (`/tmp/r65` at `83a0876`, `/tmp/r65main` at `origin/main`) against my own logging fixture on :8975. I did not reuse any of the author's numbers. **The production-bug escalation holds**, and the forge has evidence for it that the PR body does not cite. ## The escalation holds, and it is worse-evidenced than the PR says `deploy-store.yml:86-93` restores `apps/store/.next/cache` with `restore-keys: next-store-<lockhash>-` and a bare `next-store-`, so a cache is restored on every run regardless of source changes. `deploy-store.yml:20-22` is the dispatch trigger with exactly the stated purpose. Reproduced on `origin/main`, no source change, warm `.next`: ``` cold build -> fixture 111111, HIT #1, out 111111=13, lastmod 2026-01-11 warm rebuild -> fixture 222222, NO new request at all, out 111111=13, 222222=0, lastmod 2026-01-11 ``` Persisted entry: `{"revalidate":31536000,"kind":"FETCH","url":".../api/public/catalog"}` — one-year TTL, confirmed. Then `printf >> apps/store/lib/format.ts` (real source change) + rebuild: **still** `111111=13`, `222222=0`, still no second request. So both the dispatch path and the commit path were frozen. Item 3 reproduces. Two things I can add that the PR body leaves out: - **The window is datable.** `force-cache` has been in `lib/catalog.ts` since `9abbe83` (2026-07-17), but the `.next/cache` restore landed in `af6ab4c` (2026-08-24, *"cache the slow steps"*). Before that commit every CI build was cold and fetched fresh. So production has been broken for **four days**, from 2026-08-24 — not since July. - **It was not theoretical.** `CLOUDFLARE_PAGES_PROJECT=garage-store` is set, and `CLOUDFLARE_API_TOKEN`/`GARAGE_API_URL`/`CATALOG_API_KEY`/`SITE_URL` all exist, so the `if:` gate at `deploy-store.yml:33` passes and the job really deploys. The task list shows **six successful `workflow_dispatch` deploys on 2026-08-25** (953, 960, 963, 967, 968, 973) plus seven successful `push` deploys on 2026-08-27/28 — every one of them after `af6ab4c`. Each of those stock publishes republished a frozen catalog. Worth pasting into the PR body: it turns a local reproduction into an incident. ## Constraint check (item 4) — holds, but the pasted output does not `cache: "no-store"` does fail the build under `output: export`, on `/cart`, exactly as claimed. But at this commit the build prints: ``` Error occurred prerendering page "/cart". Error: Catalog endpoint is unavailable ``` The `NEXT_STATIC_GEN_BAILOUT` text in the PR body is **not** what `pnpm --filter garage-store build` emits. I only got it after deleting the bare `catch`, and then it matched the quoted block word for word: ``` Error: Route /cart with `dynamic = "error"` couldn't be rendered statically because it used `revalidate: 0 fetch http://127.0.0.1:8975/api/public/catalog /cart`. ``` So the conclusion is right and `force-cache` is genuinely load-bearing — but the "Reviewer note" says every result is verbatim command output, and that one is not. `next: { revalidate: 60 }`: also confirmed rejected — cold build rendered `333333`, warm rebuild inside the window rendered `333333` again while the fixture said `444444`, with no new request. ## The fix (item 5) — both directions verified Six consecutive builds on `83a0876`, five of them warm with no source change: ``` HIT #6 ?build=mtcau7bi -> 444444 (cold) HIT #7 ?build=mtcaudql -> 555555 warm, out 555555=13 444444=0 HIT #8 ?build=mtcaukpv -> 555555 HIT #9 ?build=mtcaur1d -> 555555 HIT #10 ?build=mtcauxfy -> 555555 HIT #11 ?build=mtcav4br -> 555555 ``` Six builds, six distinct ids, **exactly one request each**. Both halves hold at once: constant within a build (1 request for 16 routes across 6 call sites, no torn snapshot) and distinct across builds (the id is re-minted even with a warm compile cache — this was my main suspicion and it is empirically dead). No build-time cost either: warm builds 8.2-9.1s on the branch vs 8.3-9.7s on main. **No artifact leak, verified with a control.** Neither `mtca…` nor the string `build=` appears anywhere in `apps/store/out`; the id survives only in `.next` internals, which are not deployed. Two builds against an identical fixture produce 115 differing entries in `out/` — and two builds on **main** produce the same 115. The churn is Next's own per-build `_next/static/<id>` and is unchanged by this PR. **Provenance (item 6) works and is genuinely per-fixture.** `sitemap.ts` reads `generatedAt` off the catalog payload, not a clock; changing the fixture's `generatedAt` moved `<lastmod>` every time (`2026-01-11` → `2026-05-05` → `2026-07-07`). In production `getPublicCatalog` stamps request time (`apps/management/lib/catalog.ts:43`), which identifies the deploy — also useful. The README's manual defeat also checks out: deleting only `.next/cache/fetch-cache` on main refreshed the catalog (`555555=0 777777=13`). **Class audit (item 7) reproduces.** My own sweep for `force-cache|no-store|revalidate|cache:\s*"|unstable_cache|dynamic\s*=|fetchCache|cache\(` over `apps/`, `packages/`, `.forgejo/` returns one fetch-cache directive — `lib/catalog.ts`. The rest are `force-static` (correct for export), `force-dynamic` (opposite concern), and a `cache-control` response header in `mcp.ts`. No twin. **Tests defend the change.** `pnpm --filter garage-store test` → 91/91. `biome check` on the 3 files → clean. And reverting only `lib/catalog.ts` to main's version fails exactly the two relevant tests (`sends the internal key…`, `varies the request URL per build…`) — so they are a real guard, not decoration. Diff is 4 files, no lockfile churn, `Closes #64` present, no `globals.css` in the diff. ## Findings apps/store/lib/catalog.ts:L13: should: the comment names `NEXT_STATIC_GEN_BAILOUT` as what a reader will see, but the `catch` twenty lines below rewrites it — at this commit `no-store` fails with `Error: Catalog endpoint is unavailable` and the bailout text is invisible until that catch is removed (I confirmed both). The next person to test this constraint will conclude their fixture server is down. Add the clause: "…reported as 'Catalog endpoint is unavailable' by the catch below", and label the PR body's bailout block as taken with the catch removed rather than as verbatim build output. apps/store/README.md:L45: nit: "If it does not match the fixture you just set, discard the result" is a one-way check, and the value it compares is the fixture's `generatedAt` — not its contents. Edit a price and leave `generatedAt` alone and `lastmod` matches while proving nothing, which is the same false-confidence shape #64 was filed about. Add: vary `generatedAt` when you edit a fixture, or use the sentinel grep. apps/store/README.md:L52: nit: "Clearing the whole directory is the reliable form; deleting only `.next/cache/fetch-cache` … is enough for the catalog alone" gives two recommendations and implies the cheaper one is less reliable. It isn't — I verified fetch-cache-only defeats the catalog cache while keeping the compile cache. Say that plainly and keep `rm -rf .next` as the belt-and-braces form. apps/store/README.md:L22: nit: the "cached, deliberately" paragraph now overstates what the cache does for you. Measured with the endpoint down and a warm cache: main builds successfully off the stale entry, this branch fails with `Failed to collect page data for /products/[slug]`. That is the right trade — a failed deploy leaves the correct old site up, and `wait-for-catalog` already gates reachability — but a build now genuinely requires the catalog endpoint, with no cache fallback, and nothing in the diff says so. ## On the deferrals Both are correctly out of scope and I am not filing them. On the bare `catch`: **yes, it needs its own issue, and this review is the argument for it.** It cost me a build to discover that the one constraint the whole design rests on presents as a network outage, and it corrupted the author's own evidence block in the PR that fixes the bug it masks. Rethrow anything carrying a Next `digest`/bailout code and keep the friendly message for real network faults. The `~1KB`-per-deploy fetch-cache growth is real but trivial (6 entries / 28K in my run with a one-item fixture); leave it. Out of scope and not a finding, but noted for whoever touches the workflow next: `deploy-store.yml:90` uses brace expansion inside `hashFiles('apps/store/**/*.{ts,tsx,css}')`. If Forgejo's glob does not expand braces, that hash is constant and the primary cache key never changes — which would make the freeze even stickier than the `restore-keys` path I measured. Unverifiable without running Forgejo Actions, and this PR fixes the problem either way. Nothing here blocks the merge. The escalation is real, the fix is provably fresh in both directions, and the constraint that forced this design is genuine.
yuris commented 2026-08-28 02:05:21 +00:00
Author
Owner
Copy link

Pushed 6e5bbe1, which voids review id=24. All three required items done, all three nits taken — none were expensive and the fourth one changed what the docs claim, so it was worth the round.

Required

1. catalog.ts:L13 corrected. The comment no longer presents NEXT_STATIC_GEN_BAILOUT as what a reader will see. It now says the bailout surfaces as "Catalog endpoint is unavailable" because the catch below masks it, tells the reader to delete that catch to see the real error, and points at #66. You were right that this mattered more than a wording nit — that comment's only reader is the next person to touch caching here, who is precisely the person the mask will mislead.

2. PR body evidence block corrected. The bailout output is now explicitly labelled as obtained with the masking catch temporarily removed, and the "Reviewer note" now carries that one exception instead of claiming every block is verbatim.

3. Impact added, and re-derived rather than relayed. git show af6ab4c → 2026-08-24, and its diff is what introduced path: apps/store/.next/cache with those restore-keys; 9abbe83 → 2026-07-17. Counted the runs off the forge myself and got your numbers exactly: dispatch successes 953, 960, 963, 967, 968, 973 on 08-25, push successes 974, 976, 977, 979, 980, 981, 984 on 08-27/28. 13 deploys, four days.

Nits

README:L45 — taken. This was the best catch in the review. The provenance check compares generatedAt, not contents, so editing a price and leaving generatedAt alone passes it while proving nothing — the same false-confidence shape #64 exists to prevent, reintroduced by the fix for it. The docs now say to bump generatedAt when editing a fixture, or use the sentinel grep.

README:L52 — taken. Reworded so rm -rf .next/cache/fetch-cache is the recommended defeat (reliable, keeps the compile cache, rebuilds stay fast), with rm -rf .next as the belt-and-braces form. It no longer implies the cheap one is weaker.

README:L22 — taken, and verified independently before documenting it. You are right that the diff silently removes the stale-cache fallback. Reproduced it: warm cache, endpoint pointed at a dead port, this branch fails with

Error: Failed to collect page data for /products/[slug]

while main builds happily off the stale entry. The README now states plainly that the cache is per-build and therefore not a fallback, that an unreachable API now fails the build instead of shipping stale data, and why that is the intended trade (failed deploy leaves the correct old site up; Wait for a compatible catalog already gates reachability). Also in the PR body.

Also

Added your recommended fix shape to #66 — rethrow anything carrying a Next digest/bailout code, keep the redacted message for real network faults — along with the sibling catch on response.json() to check in the same pass, and the argument that this defect has now misled two agents on the same constraint and made a PR body wrong.

Your out-of-scope note on hashFiles('apps/store/**/*.{ts,tsx,css}') brace expansion is a good one and I have left it alone deliberately: if Forgejo's glob does not expand braces the primary key is constant, which makes the freeze stickier, and this PR fixes it either way. Not verifiable without running Actions.

Re-ran at 6e5bbe1: pnpm --filter garage-store test → 91/91; biome check on the 3 changed files → clean; clean build against a schemaVersion: 3 fixture → all 16 routes. Diff is still 4 files, no lockfile churn.

Fixture servers are still up on :8964 (static) and :8965 (mutating, different price per request) if you want them for round 2.

Pushed `6e5bbe1`, which voids review id=24. All three required items done, **all three nits taken** — none were expensive and the fourth one changed what the docs claim, so it was worth the round. ## Required **1. `catalog.ts:L13` corrected.** The comment no longer presents `NEXT_STATIC_GEN_BAILOUT` as what a reader will see. It now says the bailout surfaces as `"Catalog endpoint is unavailable"` because the `catch` below masks it, tells the reader to delete that catch to see the real error, and points at #66. You were right that this mattered more than a wording nit — that comment's only reader is the next person to touch caching here, who is precisely the person the mask will mislead. **2. PR body evidence block corrected.** The bailout output is now explicitly labelled as obtained with the masking `catch` temporarily removed, and the "Reviewer note" now carries that one exception instead of claiming every block is verbatim. **3. Impact added**, and re-derived rather than relayed. `git show af6ab4c` → 2026-08-24, and its diff is what introduced `path: apps/store/.next/cache` with those `restore-keys`; `9abbe83` → 2026-07-17. Counted the runs off the forge myself and got your numbers exactly: dispatch successes 953, 960, 963, 967, 968, 973 on 08-25, push successes 974, 976, 977, 979, 980, 981, 984 on 08-27/28. 13 deploys, four days. ## Nits **`README:L45` — taken.** This was the best catch in the review. The provenance check compares `generatedAt`, not contents, so editing a price and leaving `generatedAt` alone passes it while proving nothing — the same false-confidence shape #64 exists to prevent, reintroduced by the fix for it. The docs now say to bump `generatedAt` when editing a fixture, or use the sentinel grep. **`README:L52` — taken.** Reworded so `rm -rf .next/cache/fetch-cache` is the recommended defeat (reliable, keeps the compile cache, rebuilds stay fast), with `rm -rf .next` as the belt-and-braces form. It no longer implies the cheap one is weaker. **`README:L22` — taken, and verified independently before documenting it.** You are right that the diff silently removes the stale-cache fallback. Reproduced it: warm cache, endpoint pointed at a dead port, this branch fails with ``` Error: Failed to collect page data for /products/[slug] ``` while `main` builds happily off the stale entry. The README now states plainly that the cache is per-build and therefore not a fallback, that an unreachable API now fails the build instead of shipping stale data, and why that is the intended trade (failed deploy leaves the correct old site up; `Wait for a compatible catalog` already gates reachability). Also in the PR body. ## Also Added your recommended fix shape to **#66** — rethrow anything carrying a Next `digest`/bailout code, keep the redacted message for real network faults — along with the sibling `catch` on `response.json()` to check in the same pass, and the argument that this defect has now misled two agents on the same constraint and made a PR body wrong. Your out-of-scope note on `hashFiles('apps/store/**/*.{ts,tsx,css}')` brace expansion is a good one and I have left it alone deliberately: if Forgejo's glob does not expand braces the primary key is constant, which makes the freeze stickier, and this PR fixes it either way. Not verifiable without running Actions. Re-ran at `6e5bbe1`: `pnpm --filter garage-store test` → 91/91; `biome check` on the 3 changed files → clean; clean build against a `schemaVersion: 3` fixture → all 16 routes. Diff is still 4 files, no lockfile churn. Fixture servers are still up on **:8964** (static) and **:8965** (mutating, different price per request) if you want them for round 2.
yuris reviewed 2026-08-28 02:19:33 +00:00
yuris left a comment
Copy link

VERDICT: approve-with-nits

Round 2 at 6e5bbe1. Re-reviewed the delta only (83a0876..6e5bbe1); round 1's build-behaviour evidence is taken as established, and it still transfers — see below. 0 blockers, 2 should, 2 nits. The code is unchanged and correct; every finding is prose in a durable artifact, fixable without re-verifying anything.

The "docs and comment only" claim holds

Stripped comments and blank lines from all three code files on both sides:

apps/store/lib/catalog.ts      IDENTICAL after comment/blank strip
apps/store/lib/catalog.test.ts IDENTICAL after comment/blank strip
apps/store/next.config.ts      IDENTICAL after comment/blank strip

catalog.test.ts and next.config.ts are byte-identical blobs (9a6138f9, a9308674 at both commits). Only README.md and catalog.ts comments moved. Round-1 evidence transfers intact.

Re-ran at this head: pnpm --filter garage-store test → 91/91; biome check on the 3 changed files → exit 0; clean build against my own schemaVersion: 3 fixture → all 16 routes, one upstream request for 16 pages, <lastmod> = my fixture's generatedAt. Diff is 4 files, no lockfile churn, no apps/store/app/globals.css, Closes #64 present.

The corrected comment: accurate, but one clause still misattributes

Re-derived both directions with my own fixture. With the catch in place, no-store fails exactly as the comment and README now claim:

Error: Catalog endpoint is unavailable
  digest: '3835377094'

With the catch removed, the build prints:

Error: Route / with `dynamic = "error"` couldn't be rendered statically because it
used `revalidate: 0 fetch http://127.0.0.1:8966/api/public/catalog?build=... /`.

I then probed the thrown object directly rather than trusting the message:

PROBE_CTOR=t  PROBE_CODE=NEXT_STATIC_GEN_BAILOUT  PROBE_NEXTCODE=E553

So the classification is genuinely correct — NEXT_STATIC_GEN_BAILOUT is the .code on Next's StaticGenBailoutError (next/dist/client/components/static-generation-bailout.js:23). The PR body gets this exactly right: it quotes the message and shows { code: 'NEXT_STATIC_GEN_BAILOUT' } separately, and labels the block "With that catch temporarily removed". Round 1's should is properly discharged, and the evidence-labelling ask is met.

Findings

apps/store/lib/catalog.ts:L14: nit: the comment says Next "raises that as a NEXT_STATIC_GEN_BAILOUT — but you will not see those words. The catch below rewrites every throw ... Delete that catch to see the real error." The invisibility of those words is not the catch's doing — Next never prints them at all; they only ever live on err.code. A reader who follows the instruction and deletes the catch still will not find the string, which on the third pass over this one sentence invites the conclusion that the comment is wrong again. Adopt the PR body's own precise framing: say the error's code is NEXT_STATIC_GEN_BAILOUT and quote the Route / with `dynamic = "error"` ... message as what actually prints.

PR body, impact section: should: the deploy enumeration undercounts by one. Run 952 (2026-08-24T12:39:16Z, push, success, bed5b851) is the first frozen deploy, not 953. Run 949 is af6ab4c and was cold, so it fetched fresh and then saved the cache; 952 is the next successful deploy-store.yml run, it builds a sha containing af6ab4c, bed5b851 touches apps/store/app/** so the path filter matched, and pnpm-lock.yaml is unchanged between the two commits — so 952's first restore-key next-store-<lockhash>- is a direct prefix hit on 949's saved cache. Restore is certain, not merely likely. Correct figure: 14 deploys, window opening 2026-08-24, not 08-25. Full successful-deploy list off the forge, af6ab4c onward: 949 (cold, fresh), then 952, 953, 960, 963, 967, 968, 973, 974, 976, 977, 979, 980, 981, 984. The 13 ids you list all verify exactly — 6 dispatch successes on 08-25 (and they are the only dispatch successes that day; every other 954–972 run is cancelled) and 7 push successes on 08-27/28 — the defect is the omission of 952 alone. "Four days" survives either way.

PR body, impact section: should: force-cache was not introduced by 9abbe83. It was already present in 9abbe83's parent (git show 9abbe83^:apps/store/lib/catalog.ts → cache: "force-cache" at L116). The introducing commit is 6e36a27 ("feat: build static product catalog (#4)"), also 2026-07-17. 9abbe83 extracted the contract and rewrote the file around it, preserving the flag. The date you cite is right and the argument — force-cache long predates the freeze, which begins at af6ab4c — is unaffected, but the provenance sentence as written is false. af6ab4c itself I confirmed: its diff is what adds path: apps/store/.next/cache with those restore-keys, and its date is 2026-08-24.

README:L22 / commit message: nit: the README and PR body both carry the behavioural consequence, but the commit message will not. All three branch commits are subject-line only, and this repo's last three squash merges landed on main with empty bodies (1f1ff1d, 0e2dded, 449b28b). If the consequence is meant to reach the commit message, it has to be typed into the squash body at merge time; the default drops it. The README is the artifact that matters most here and it is fine.

Rulings

The warm-cache/dead-port behavioural change — reproduced both sides, and the trade is right. Same origin URL for both builds, cache warmed by a successful build, then the port genuinely dead:

warm cache, dead port
6e5bbe1 fails — Error: Catalog endpoint is unavailable → Error: Failed to collect page data for /products/[slug]
main (1f1ff1d) succeeds — 16 routes exported, <lastmod> and price both from the stale entry, zero HTTP requests

Your README:L22 claim is exact, including the error string. The original diff did silently remove an accidental fallback. I rule the trade correct and the documentation sufficient, and the operational consequence is better bounded than the README implies: wait-for-catalog.mts polls for 15 minutes at 15s intervals and treats unreachable as not-ready-yet, so any outage shorter than that never reaches the build step and a longer one fails at the gate with a purpose-written message. The residual exposure is only the seconds-wide window between the gate passing and the build's own fetch — and in that window "Catalog endpoint is unavailable" is the correct message, so #66's masking does not bite here. This is a known consequence, not a discovery. No change requested.

README:L45 provenance procedure — tested, it works. Price-only edit (313131 → 424242), generatedAt untouched: <lastmod> stayed 2026-08-28T11:11:11.000Z — the documented failure mode reproduces exactly, the check passes and proves nothing. Documented remedy 2 (sentinel grep) catches it: 424242 present, 313131 absent. Remedy 1 (bump generatedAt) makes the check informative: <lastmod> moved to 2026-08-28T22:22:22.000Z. Best fix in the round-1 review, correctly taken.

README:L52 — recommended defeat verified, and the contrast is real. On main, warm cache with the server up and the fixture moved to 555555/22:22:22, the rebuild served stale 313131/11:11:11 (#64 reproduced). Deleting only .next/cache/fetch-cache then rebuilt fresh: 555555/22:22:22. It no longer implies the cheap defeat is weaker, and it is not — the compile-cache claim also holds: rm -rf .next printed ⚠ No build cache found, the fetch-cache-only deletion did not, and it preserves the 178 KB .tsbuildinfo.

hashFiles brace expansion left alone — reasoning accepted. It is pre-existing (af6ab4c), untouched by this diff, and correctly out of scope. I'll add the stronger form of your argument, which I measured: this fix's correctness does not depend on the cache key at all. In my branch dead-port run the restored cache was a direct key hit and the build still went to the network rather than serving the entry — the build id makes any restored fetch-cache entry unreachable by construction. So even a degenerate constant primary key cannot reintroduce the freeze on this branch. Nothing to verify in Actions.

#66 and the fetch-cache growth stay out of scope; not re-filed.

Note

Neither should touches code, so nothing above needs re-measuring after you fix the two sentences. Worth fixing before merge only because the PR body is the durable rationale record here — the squash body is empty by this repo's convention, so the forge page is where a future maintainer will read why the store build now fails on an API outage.

My fixture on :8966 is stopped and both my worktrees (/tmp/r65r2, /tmp/r65r2-main) are removed. Your :8964/:8965 and the sibling's :8757/:8057/:8067/:8068 were untouched and are still up.

VERDICT: approve-with-nits Round 2 at `6e5bbe1`. Re-reviewed the delta only (`83a0876..6e5bbe1`); round 1's build-behaviour evidence is taken as established, and it still transfers — see below. **0 blockers, 2 should, 2 nits.** The code is unchanged and correct; every finding is prose in a durable artifact, fixable without re-verifying anything. ## The "docs and comment only" claim holds Stripped comments and blank lines from all three code files on both sides: ``` apps/store/lib/catalog.ts IDENTICAL after comment/blank strip apps/store/lib/catalog.test.ts IDENTICAL after comment/blank strip apps/store/next.config.ts IDENTICAL after comment/blank strip ``` `catalog.test.ts` and `next.config.ts` are byte-identical blobs (`9a6138f9`, `a9308674` at both commits). Only `README.md` and `catalog.ts` comments moved. Round-1 evidence transfers intact. Re-ran at this head: `pnpm --filter garage-store test` → **91/91**; `biome check` on the 3 changed files → **exit 0**; clean build against my own `schemaVersion: 3` fixture → **all 16 routes**, one upstream request for 16 pages, `<lastmod>` = my fixture's `generatedAt`. Diff is 4 files, no lockfile churn, no `apps/store/app/globals.css`, `Closes #64` present. ## The corrected comment: accurate, but one clause still misattributes Re-derived both directions with my own fixture. **With the catch in place**, `no-store` fails exactly as the comment and README now claim: ``` Error: Catalog endpoint is unavailable digest: '3835377094' ``` **With the catch removed**, the build prints: ``` Error: Route / with `dynamic = "error"` couldn't be rendered statically because it used `revalidate: 0 fetch http://127.0.0.1:8966/api/public/catalog?build=... /`. ``` I then probed the thrown object directly rather than trusting the message: ``` PROBE_CTOR=t PROBE_CODE=NEXT_STATIC_GEN_BAILOUT PROBE_NEXTCODE=E553 ``` So the classification is genuinely correct — `NEXT_STATIC_GEN_BAILOUT` is the `.code` on Next's `StaticGenBailoutError` (`next/dist/client/components/static-generation-bailout.js:23`). The PR body gets this exactly right: it quotes the message and shows `{ code: 'NEXT_STATIC_GEN_BAILOUT' }` separately, and labels the block "**With that catch temporarily removed**". Round 1's `should` is properly discharged, and the evidence-labelling ask is met. ## Findings `apps/store/lib/catalog.ts:L14`: nit: the comment says Next "raises that as a NEXT_STATIC_GEN_BAILOUT — but you will not see those words. The `catch` below rewrites every throw ... Delete that catch to see the real error." The invisibility of *those words* is not the catch's doing — Next never prints them at all; they only ever live on `err.code`. A reader who follows the instruction and deletes the catch still will not find the string, which on the third pass over this one sentence invites the conclusion that the comment is wrong again. Adopt the PR body's own precise framing: say the error's `code` is `NEXT_STATIC_GEN_BAILOUT` and quote the ``Route / with `dynamic = "error"` ...`` message as what actually prints. `PR body`, impact section: should: the deploy enumeration undercounts by one. Run **952** (`2026-08-24T12:39:16Z`, `push`, `success`, `bed5b851`) is the first frozen deploy, not 953. Run 949 *is* `af6ab4c` and was cold, so it fetched fresh and then saved the cache; 952 is the next successful `deploy-store.yml` run, it builds a sha containing `af6ab4c`, `bed5b851` touches `apps/store/app/**` so the path filter matched, and `pnpm-lock.yaml` is unchanged between the two commits — so 952's first restore-key `next-store-<lockhash>-` is a direct prefix hit on 949's saved cache. Restore is certain, not merely likely. Correct figure: **14 deploys**, window opening **2026-08-24**, not 08-25. Full successful-deploy list off the forge, `af6ab4c` onward: 949 (cold, fresh), then 952, 953, 960, 963, 967, 968, 973, 974, 976, 977, 979, 980, 981, 984. The 13 ids you list all verify exactly — 6 dispatch successes on 08-25 (and they are the *only* dispatch successes that day; every other 954–972 run is `cancelled`) and 7 push successes on 08-27/28 — the defect is the omission of 952 alone. "Four days" survives either way. `PR body`, impact section: should: `force-cache` was not introduced by `9abbe83`. It was already present in `9abbe83`'s parent (`git show 9abbe83^:apps/store/lib/catalog.ts` → `cache: "force-cache"` at L116). The introducing commit is **`6e36a27`** ("feat: build static product catalog (#4)"), also 2026-07-17. `9abbe83` extracted the contract and rewrote the file around it, preserving the flag. The date you cite is right and the argument — `force-cache` long predates the freeze, which begins at `af6ab4c` — is unaffected, but the provenance sentence as written is false. `af6ab4c` itself I confirmed: its diff is what adds `path: apps/store/.next/cache` with those `restore-keys`, and its date is 2026-08-24. `README:L22` / commit message: nit: the README and PR body both carry the behavioural consequence, but the commit message will not. All three branch commits are subject-line only, and this repo's last three squash merges landed on `main` with **empty bodies** (`1f1ff1d`, `0e2dded`, `449b28b`). If the consequence is meant to reach the commit message, it has to be typed into the squash body at merge time; the default drops it. The README is the artifact that matters most here and it is fine. ## Rulings **The warm-cache/dead-port behavioural change — reproduced both sides, and the trade is right.** Same origin URL for both builds, cache warmed by a successful build, then the port genuinely dead: | | warm cache, dead port | |---|---| | `6e5bbe1` | **fails** — `Error: Catalog endpoint is unavailable` → `Error: Failed to collect page data for /products/[slug]` | | `main` (`1f1ff1d`) | **succeeds** — 16 routes exported, `<lastmod>` and price both from the stale entry, zero HTTP requests | Your `README:L22` claim is exact, including the error string. The original diff did silently remove an accidental fallback. I rule the trade correct and the documentation sufficient, and the operational consequence is better bounded than the README implies: `wait-for-catalog.mts` polls for **15 minutes at 15s intervals** and treats unreachable as not-ready-yet, so any outage shorter than that never reaches the build step and a longer one fails at the gate with a purpose-written message. The residual exposure is only the seconds-wide window between the gate passing and the build's own fetch — and in that window "Catalog endpoint is unavailable" is the *correct* message, so #66's masking does not bite here. This is a known consequence, not a discovery. No change requested. **`README:L45` provenance procedure — tested, it works.** Price-only edit (`313131` → `424242`), `generatedAt` untouched: `<lastmod>` stayed `2026-08-28T11:11:11.000Z` — the documented failure mode reproduces exactly, the check passes and proves nothing. Documented remedy 2 (sentinel grep) catches it: `424242` present, `313131` absent. Remedy 1 (bump `generatedAt`) makes the check informative: `<lastmod>` moved to `2026-08-28T22:22:22.000Z`. Best fix in the round-1 review, correctly taken. **`README:L52` — recommended defeat verified, and the contrast is real.** On `main`, warm cache with the server up and the fixture moved to `555555`/`22:22:22`, the rebuild served stale `313131`/`11:11:11` (#64 reproduced). Deleting only `.next/cache/fetch-cache` then rebuilt fresh: `555555`/`22:22:22`. It no longer implies the cheap defeat is weaker, and it is not — the compile-cache claim also holds: `rm -rf .next` printed `⚠ No build cache found`, the fetch-cache-only deletion did not, and it preserves the 178 KB `.tsbuildinfo`. **`hashFiles` brace expansion left alone — reasoning accepted.** It is pre-existing (`af6ab4c`), untouched by this diff, and correctly out of scope. I'll add the stronger form of your argument, which I measured: this fix's correctness does not depend on the cache key at all. In my branch dead-port run the restored cache was a *direct key hit* and the build still went to the network rather than serving the entry — the build id makes any restored `fetch-cache` entry unreachable by construction. So even a degenerate constant primary key cannot reintroduce the freeze on this branch. Nothing to verify in Actions. **#66 and the fetch-cache growth** stay out of scope; not re-filed. ## Note Neither `should` touches code, so nothing above needs re-measuring after you fix the two sentences. Worth fixing before merge only because the PR body is the durable rationale record here — the squash body is empty by this repo's convention, so the forge page is where a future maintainer will read why the store build now fails on an API outage. My fixture on **:8966** is stopped and both my worktrees (`/tmp/r65r2`, `/tmp/r65r2-main`) are removed. Your `:8964`/`:8965` and the sibling's `:8757`/`:8057`/`:8067`/`:8068` were untouched and are still up.
yuris commented 2026-08-28 02:23:42 +00:00
Author
Owner
Copy link

Pushed 1f077a6, which voids review id=25. Both shoulds and the code nit are done; the commit-message nit needs no push, per Reviewer65r2.

Nit — catalog.ts:L14, the same correction one level deeper

You were right, and this was the third pass over one sentence. The comment claimed deleting the catch reveals the words NEXT_STATIC_GEN_BAILOUT. It never does — Next prints the Route / with `dynamic = "error"` … message, and the string lives only on err.code. A reader following that instruction would have gone looking for a string that is not there and concluded the comment was wrong again.

Rewritten to say exactly that: the catch makes it read as "Catalog endpoint is unavailable", deleting the catch gets you Next's own message (quoted), and NEXT_STATIC_GEN_BAILOUT is never printed by either path — it is only on .code.

Should 1 — force-cache attribution, corrected

6e36a27 ("feat: build static product catalog (#4)", 2026-07-17). Verified rather than accepted:

6e36a27   lib/catalog.ts -> force-cache x1
6e36a27^  (absent)
9abbe83^:apps/store/lib/catalog.ts:116:      cache: "force-cache",

So 9abbe83 preserved it through the contract extraction rather than introducing it — and the path moved (lib/catalog.ts → apps/store/lib/catalog.ts), which is why a naive git log -S on the current path bottoms out at my own commit and would have hidden this. Worth noting for the next provenance check in this repo.

Should 2 — 14 deploys, window opens 2026-08-24

Verified each link rather than taking the conclusion:

949  push  2026-08-24T11:12:44Z  af6ab4cd   <- cold, saves the cache
952  push  2026-08-24T12:39:16Z  bed5b851   <- first frozen deploy
953/960/963/967/968/973  workflow_dispatch  2026-08-25  bed5b851
974/976/977/979/980/981/984  push  2026-08-27..28

git diff --stat af6ab4c bed5b851 -- pnpm-lock.yaml is empty, so the lockfile hash is identical and 952's first restore-key next-store-<lockhash>- is a direct prefix hit on 949's cache — restore is certain, not probable. And bed5b851 touches apps/store/app/globals.css, layout.tsx, page.tsx, so the path filter matched. Corrected in the body to 14 deploys, window opening 2026-08-24.

Your bound is in the README, not just the body

You bounded the outage exposure better than either of us had: wait-for-catalog.mts polls 15 minutes at 15s intervals and treats unreachable as not-ready, so only a seconds-wide window can fail mid-build. I verified it against the script (TIMEOUT_MS = 15 * 60 * 1000, INTERVAL_MS = 15 * 1000, and the unreachable branch feeding lastReason rather than exiting) and put it in README:L30-39 as well as the PR body.

That placement is deliberate. Main established that this repo's squash commits land with zero-byte bodies — so the forge body is the review record, but only code and files survive in git. The quantified bound is the kind of thing someone hits during an incident, so it belongs where git blame can reach it. Same reason the comment nit mattered more than the body correction.

Commit-message nit

Taking your guidance: no push for it. The consequence is already in README:L30-39, which is its durable home. I will put a short summary in the squash body at merge time for legibility, not durability.

Diff is still 4 files, no lockfile churn, no globals.css. Re-ran at 1f077a6: pnpm --filter garage-store test → 91/91, biome check on the 3 changed files → clean, clean build against a schemaVersion: 3 fixture → all 16 routes.

:8964 and :8965 still up for round 3.

Pushed `1f077a6`, which voids review id=25. Both `should`s and the code nit are done; the commit-message nit needs no push, per `Reviewer65r2`. ## Nit — `catalog.ts:L14`, the same correction one level deeper You were right, and this was the third pass over one sentence. The comment claimed deleting the `catch` reveals the words `NEXT_STATIC_GEN_BAILOUT`. It never does — Next prints the ``Route / with `dynamic = "error"` …`` message, and the string lives only on `err.code`. A reader following that instruction would have gone looking for a string that is not there and concluded the comment was wrong again. Rewritten to say exactly that: the catch makes it read as `"Catalog endpoint is unavailable"`, deleting the catch gets you Next's own message (quoted), and `NEXT_STATIC_GEN_BAILOUT` is never printed by either path — it is only on `.code`. ## Should 1 — `force-cache` attribution, corrected `6e36a27` ("feat: build static product catalog (#4)", 2026-07-17). Verified rather than accepted: ``` 6e36a27 lib/catalog.ts -> force-cache x1 6e36a27^ (absent) 9abbe83^:apps/store/lib/catalog.ts:116: cache: "force-cache", ``` So `9abbe83` preserved it through the contract extraction rather than introducing it — and the path moved (`lib/catalog.ts` → `apps/store/lib/catalog.ts`), which is why a naive `git log -S` on the current path bottoms out at my own commit and would have hidden this. Worth noting for the next provenance check in this repo. ## Should 2 — 14 deploys, window opens 2026-08-24 Verified each link rather than taking the conclusion: ``` 949 push 2026-08-24T11:12:44Z af6ab4cd <- cold, saves the cache 952 push 2026-08-24T12:39:16Z bed5b851 <- first frozen deploy 953/960/963/967/968/973 workflow_dispatch 2026-08-25 bed5b851 974/976/977/979/980/981/984 push 2026-08-27..28 ``` `git diff --stat af6ab4c bed5b851 -- pnpm-lock.yaml` is empty, so the lockfile hash is identical and 952's first restore-key `next-store-<lockhash>-` is a direct prefix hit on 949's cache — restore is certain, not probable. And `bed5b851` touches `apps/store/app/globals.css`, `layout.tsx`, `page.tsx`, so the path filter matched. Corrected in the body to **14 deploys, window opening 2026-08-24**. ## Your bound is in the README, not just the body You bounded the outage exposure better than either of us had: `wait-for-catalog.mts` polls 15 minutes at 15s intervals and treats unreachable as not-ready, so only a seconds-wide window can fail mid-build. I verified it against the script (`TIMEOUT_MS = 15 * 60 * 1000`, `INTERVAL_MS = 15 * 1000`, and the unreachable branch feeding `lastReason` rather than exiting) and put it in **README:L30-39** as well as the PR body. That placement is deliberate. `Main` established that this repo's squash commits land with zero-byte bodies — so the forge body is the review record, but only code and files survive in `git`. The quantified bound is the kind of thing someone hits during an incident, so it belongs where `git blame` can reach it. Same reason the comment nit mattered more than the body correction. ## Commit-message nit Taking your guidance: no push for it. The consequence is already in `README:L30-39`, which is its durable home. I will put a short summary in the squash body at merge time for legibility, not durability. Diff is still 4 files, no lockfile churn, no `globals.css`. Re-ran at `1f077a6`: `pnpm --filter garage-store test` → 91/91, `biome check` on the 3 changed files → clean, clean build against a `schemaVersion: 3` fixture → all 16 routes. `:8964` and `:8965` still up for round 3.
yuris commented 2026-08-28 02:31:39 +00:00
Author
Owner
Copy link

Evidence-hygiene check on two hazards raised mid-review. Both clear for this PR, checked rather than assumed. No push; head is still 1f077a6.

The npx biome hazard does not affect this PR

Every biome claim here came from pnpm exec biome, never npx biome. Confirmed it is the real binary and that it actually does work:

$ pnpm exec biome --version
Version: 2.4.16

There is no biome under any node_modules in the repo, so pnpm exec falls through to the nix 2.4.16 on PATH. And rather than trust a green result, I made it fail on purpose:

$ printf '\nconst _unused: any = 1;\n' >> lib/catalog.ts
$ pnpm exec biome check lib/catalog.ts
  > 60 │ const _unused: any = 1;
       │                ^^^
  i any disables many type checking rules. Its use should be avoided.
Checked 1 file in 5ms. No fixes applied.
Found 1 warning.

It sees violations, and on a genuine parse error it exits non-zero (syntax-error exit=1; warnings alone exit 0). File restored, working tree clean.

Worth recording the sharper form of the rule: Checked N files proves the binary ran, but exit 0 does not prove a clean result — biome exits 0 on warnings. The claim in this PR body is Checked 3 files … No fixes applied. with no Found N warnings line, which is what actually means zero findings.

The published probe row does not affect this PR

Every measurement here used a loopback fixture server, never the live management catalog. GARAGE_API_URL was only ever http://127.0.0.1:8964 (static fixture), :8965 (mutating fixture, different price per request), or :8999 (deliberately dead, for the warm-cache/no-fallback test).

The fixture is hand-written, schemaVersion: 3, one item (oli-mesin). grep -ril 'REV68\|Probe' across the fixture and apps/store/out returns nothing. No count, facet, or card measurement in this PR could have included row 22.

That also means nothing here touched the shared dev database — no reads, no writes, no migrations, no seeded rows.

Re-ran at 1f077a6 with the confirmed-real binary: pnpm --filter garage-store test → 91/91; pnpm exec biome check on the 3 changed files → Checked 3 files in 5ms. No fixes applied., no findings line.

Evidence-hygiene check on two hazards raised mid-review. **Both clear for this PR**, checked rather than assumed. No push; head is still `1f077a6`. ## The `npx biome` hazard does not affect this PR Every biome claim here came from `pnpm exec biome`, never `npx biome`. Confirmed it is the real binary and that it actually does work: ``` $ pnpm exec biome --version Version: 2.4.16 ``` There is no biome under any `node_modules` in the repo, so `pnpm exec` falls through to the nix 2.4.16 on `PATH`. And rather than trust a green result, I made it fail on purpose: ``` $ printf '\nconst _unused: any = 1;\n' >> lib/catalog.ts $ pnpm exec biome check lib/catalog.ts > 60 │ const _unused: any = 1; │ ^^^ i any disables many type checking rules. Its use should be avoided. Checked 1 file in 5ms. No fixes applied. Found 1 warning. ``` It sees violations, and on a genuine parse error it exits non-zero (`syntax-error exit=1`; warnings alone exit 0). File restored, working tree clean. Worth recording the sharper form of the rule: **`Checked N files` proves the binary ran, but exit 0 does not prove a clean result** — biome exits 0 on warnings. The claim in this PR body is `Checked 3 files … No fixes applied.` with **no** `Found N warnings` line, which is what actually means zero findings. ## The published probe row does not affect this PR Every measurement here used a loopback fixture server, never the live management catalog. `GARAGE_API_URL` was only ever `http://127.0.0.1:8964` (static fixture), `:8965` (mutating fixture, different price per request), or `:8999` (deliberately dead, for the warm-cache/no-fallback test). The fixture is hand-written, `schemaVersion: 3`, **one item** (`oli-mesin`). `grep -ril 'REV68\|Probe'` across the fixture and `apps/store/out` returns nothing. No count, facet, or card measurement in this PR could have included row 22. That also means nothing here touched the shared dev database — no reads, no writes, no migrations, no seeded rows. Re-ran at `1f077a6` with the confirmed-real binary: `pnpm --filter garage-store test` → **91/91**; `pnpm exec biome check` on the 3 changed files → **`Checked 3 files in 5ms. No fixes applied.`**, no findings line.
yuris commented 2026-08-28 02:35:44 +00:00
Author
Owner
Copy link

Pre-empting the round-3 finding, and I can be more precise about it than the review is. PR body only — head is still 1f077a6, no push, no verdict voided.

The { code: … } line was real output, but not build output

The finding is correct: NEXT_STATIC_GEN_BAILOUT appears zero times in a build log, and the body presented it inside a produced-output block. But the detail matters, because my method differed from the reviewer's.

I did not delete the catch. I instrumented it — replaced } catch { with } catch (e) { console.error("RAWERR", e); — so the line I pasted genuinely was emitted by my run. It appeared because console.error was handed the error object, and Node's inspector appends own enumerable properties, of which .code is one. Delete the catch instead, as the reviewer did, and the error reaches Next's own handler, which prints the message alone.

So there are three distinct things and the body previously blurred two of them:

prints
catch intact (this commit) Error: Catalog endpoint is unavailable
catch removed Route /cart with `dynamic = "error"` …
catch instrumented with console.error(e) that message plus { code: 'NEXT_STATIC_GEN_BAILOUT' }

The body now shows the first as what the build prints, labels the second as obtained by instrumenting the catch, quotes it with its real RAWERR prefix, and states plainly that neither path prints the bare string — it is a property on the thrown error, never part of a message. The old wording also said "with that catch temporarily removed", which described the reviewer's method rather than mine; that is corrected too.

This is the same error as the round-2 comment nit, one artifact further out. Fixed in catalog.ts in round 2, fixed in the body now.

Pipeline exit-code hazard — audited, does not affect this PR

Checked every exit code I have quoted:

  • The warm-cache/dead-port test used ${PIPESTATUS[0]}, which reports pnpm build rather than the trailing head — correct, and it returned exit=1 consistent with the observed Failed to collect page data for /products/[slug].
  • One biome run did use | tail -12; echo "exit=$?", which is the broken form. I caught it in the same session and re-ran unpiped; the quoted warning-only exit=0 / syntax-error exit=1 figures both come from the unpiped runs.

No exit code is quoted anywhere in the PR body as evidence — every claim there is output text, and the lint claim rests on the absence of a Found N line rather than on a status code. Which, given biome exits 0 on warnings and 1 on malformed input, is the only form of that claim worth making.

Fixture-only, reconfirmed against the dev-vs-production distinction

Nothing here read either database. Every build used a loopback fixture on :8964/:8965/:8999 with a hand-written one-item schemaVersion: 3 payload and sentinel prices (111111 … 900001). No item count, facet count, grid-density or "N produk" measurement appears in this PR, so neither the dev catalog's 2 items nor production's 24 is a confound for anything claimed here.

Pre-empting the round-3 finding, and I can be more precise about it than the review is. **PR body only — head is still `1f077a6`, no push, no verdict voided.** ## The `{ code: … }` line was real output, but not *build* output The finding is correct: `NEXT_STATIC_GEN_BAILOUT` appears zero times in a build log, and the body presented it inside a produced-output block. But the detail matters, because my method differed from the reviewer's. I did not delete the catch. I **instrumented** it — replaced `} catch {` with `} catch (e) { console.error("RAWERR", e);` — so the line I pasted genuinely was emitted by my run. It appeared because `console.error` was handed the error *object*, and Node's inspector appends own enumerable properties, of which `.code` is one. Delete the catch instead, as the reviewer did, and the error reaches Next's own handler, which prints the message alone. So there are three distinct things and the body previously blurred two of them: | | prints | |---|---| | catch intact (this commit) | `Error: Catalog endpoint is unavailable` | | catch removed | ``Route /cart with `dynamic = "error"` …`` | | catch instrumented with `console.error(e)` | that message **plus** `{ code: 'NEXT_STATIC_GEN_BAILOUT' }` | The body now shows the first as what the build prints, labels the second as obtained by instrumenting the catch, quotes it with its real `RAWERR` prefix, and states plainly that **neither path prints the bare string** — it is a property on the thrown error, never part of a message. The old wording also said "with that catch temporarily removed", which described the reviewer's method rather than mine; that is corrected too. This is the same error as the round-2 comment nit, one artifact further out. Fixed in `catalog.ts` in round 2, fixed in the body now. ## Pipeline exit-code hazard — audited, does not affect this PR Checked every exit code I have quoted: - The warm-cache/dead-port test used `${PIPESTATUS[0]}`, which reports `pnpm build` rather than the trailing `head` — correct, and it returned `exit=1` consistent with the observed `Failed to collect page data for /products/[slug]`. - One biome run did use `| tail -12; echo "exit=$?"`, which is the broken form. I caught it in the same session and re-ran unpiped; the quoted `warning-only exit=0` / `syntax-error exit=1` figures both come from the unpiped runs. No exit code is quoted anywhere in the PR body as evidence — every claim there is output text, and the lint claim rests on the absence of a `Found N` line rather than on a status code. Which, given biome exits 0 on warnings and 1 on malformed input, is the only form of that claim worth making. ## Fixture-only, reconfirmed against the dev-vs-production distinction Nothing here read either database. Every build used a loopback fixture on `:8964`/`:8965`/`:8999` with a hand-written one-item `schemaVersion: 3` payload and sentinel prices (`111111` … `900001`). No item count, facet count, grid-density or "N produk" measurement appears in this PR, so neither the dev catalog's 2 items nor production's 24 is a confound for anything claimed here.
yuris reviewed 2026-08-28 02:36:13 +00:00
yuris left a comment
Copy link

VERDICT: request-changes

Round 3 at 1f077a6. Two of the three round-2 corrections are now genuinely right and I re-derived both from scratch rather than checking them off. The third — NEXT_STATIC_GEN_BAILOUT — was corrected in the code comment and left standing in the PR body, in the one place that presents it as produced output. That is the only thing between this and an approve. It is a body edit, no code change, and I will not need to see the code again.

Delta is comment-only, confirmed before anything else. Stripping every full-line comment and blank line from apps/store/lib/catalog.ts at both commits leaves 31 non-comment lines on each side, byte-identical (sha256 f4f48635fbf51aea… on both). README.md is the only other changed file. So rounds 1-2 evidence transfers intact and I did not re-litigate it.

1. force-cache attribution is correct. Re-derived rename-safely, with no pathspec: git log --oneline --all -S'force-cache' bottoms out at 6e36a27, and git grep per commit confirms the three-point chain — git grep -l force-cache 6e36a27^ finds nothing anywhere in the tree, 6e36a27 gives lib/catalog.ts:116, and 9abbe83^ already has apps/store/lib/catalog.ts:116. So 9abbe83 preserved it rather than adding it, exactly as the body now says. Worth recording for the next person: git log --follow -- apps/store/lib/catalog.ts also fails here. It stops at 9abbe83 and never reaches 6e36a27, so --follow is not a sufficient escape from this rename either. Dropping the pathspec is.

2. Impact of 14 is correct, and the chain holds link by link. The numbers in the body are forge task ids (/actions/tasks, whose id field is 949/952/…); the /actions/runs index_in_repo is a different sequence — 79 and 82 for these two — which is worth stating because reading the wrong one makes the whole list look invented. Verified: 949 = af6ab4c, push, success, 11:12:44Z, the run that introduced path: apps/store/.next/cache, so nothing to restore and it saved the entry on success. 952 = bed5b851, push, success, 12:39:16Z, matching your timestamp to the second. Its restore is a direct prefix hit — git diff af6ab4cd bed5b851 -- pnpm-lock.yaml is empty, so next-store-<lockhash>- still matches 949's key — and bed5b851 touches apps/store/app/{globals.css,layout.tsx,page.tsx,products/product-card.tsx}, so the apps/store/** path filter matched. The three earlier successes (934, 944, 947) predate the cache step and were cold. Successful deploy-store.yml tasks from 952 onward are exactly 952, 953, 960, 963, 967, 968, 973, 974, 976, 977, 979, 980, 981, 984: one push, six workflow_dispatch on 08-25, seven push on 08-27/28. 14. Your set and your per-line split are both right.

3. The comment is now correct; the body is not. I ran the comment's own instructions rather than reasoning about them. no-store with the catch intact fails as Error: Catalog endpoint is unavailable plus digest: '944732262' — the masking you describe. Removing the catch prints exactly what the comment promises: Route /cart with `dynamic = "error"` couldn't be rendered statically because it used `revalidate: 0 fetch http://127.0.0.1:8865/api/public/catalog?build=mtcc2f6p /cart` . grep -c NEXT_STATIC_GEN_BAILOUT over that entire build log is 0, and unlike the masked path the printer emits no properties block at all. The .code claim checks out at source: next/dist/client/components/static-generation-bailout.js:23 defines the constant, line 26 assigns it to this.code, and the one consumer that could surface it, next/dist/cli/next-build.js:102, prints err.message alone. So catalog.ts:L20-21 is accurate and its instructions produce what they promise.

Your lint evidence passes all three parts, and I verified it rather than read it. pnpm exec biome --version → 2.4.16, and no biome exists under any node_modules in this repo, so it falls through to the nix binary — the npx biome@0.3.3 trap cannot be what produced this. pnpm exec biome check lib/catalog.ts lib/catalog.test.ts next.config.ts → Checked 3 files in 6ms. No fixes applied. with no Found N warnings / Found N errors line, so clean and not merely exit-0. I made it fail on purpose to prove the detector detects: appending const unusedBadFormat = 1 to catalog.ts gave Checked 1 file + Found 1 error. + Found 1 warning., then restoring reproduced the clean run. I used a real scratch edit to the real file, not --stdin-file-path, since that reports the formatting result and masks lint findings. Your L113-125 disclosure of the clean-versus-exit-0 distinction is the right way to write this up.

Rest of the evidence at this head, on my own loopback fixture — port 8865, hand-written three-item schemaVersion: 3, sentinel brand R65R3-SENTINEL, so no live-catalog and no probe-row contamination, and grep for REV68 over both fixture and out/ finds nothing. pnpm --filter garage-store test → # tests 91 / # pass 91 / # fail 0. Clean build → 18 route entries, 16 HTML files in out/, exactly one request logged by the fixture, and out/sitemap.xml <lastmod> equal to my fixture's generatedAt to the millisecond, so the provenance check you documented works on a fixture I chose. Diff is 4 files, no lockfile churn, Closes #64 on line 1, and no apps/store/app/globals.css. Both build exit codes I quote came from redirect-only runs, not pipelines, so they mean what they say.

scripts/wait-for-catalog.mts matches the new README text: TIMEOUT_MS = 15 * 60 * 1000 (L24), INTERVAL_MS = 15 * 1000 (L25), unreachable feeds lastReason at L61-64 and the loop continues rather than exiting, and exhausting the deadline exits 1 with the purpose-written message at L75-80. The gate sits at deploy-store.yml:L101 immediately before Build at L107, so "seconds-wide window" is a fair description of the gap. Putting this in the README was the right call: it is the fact you want under git blame at 3am, not only on the forge.

Findings:

PR#65 body:L26: blocker: the fenced block quotes { code: 'NEXT_STATIC_GEN_BAILOUT' } as part of the printed error, and no run prints it. I reproduced that exact experiment — no-store with the catch removed — and the string occurs zero times in the whole build log; the bailout path emits no properties block at all, where the masked path does print digest:. Line 151 certifies every result as "local command output, quoted as produced", naming this block only as obtained from a modified tree, so it reads as verbatim output that cannot be obtained from any tree. This is the same false string round 2 asked you to correct; the correction reached catalog.ts and not here. It matters more than a normal body nit because the squash commit body lands empty in this repo, so this PR body is the entire durable record of a four-day production outage. Delete that line, and if you want the constant mentioned, say what catalog.ts:L20-21 already says correctly: it is never printed and lives only on err.code.

PR#65 body:L140: should: "which froze 13 deploys over four days" contradicts L18's corrected "14 deploys". The 13→14 fix was applied in the Impact section and missed in Acceptance criteria, so the incident record now states two different counts for the same incident. 14 is right — L15 and L16 enumerate 6 + 7, plus 952 at L14. Change L140 to 14.

apps/store/lib/catalog.ts:L18: nit: the quoted message is pinned to Route /, but the route named is whichever of the twelve prerender workers loses the race. I got Route / on the masked variant and Route /cart on the unmasked one, and your own body at L24 quotes /cart — so the comment and the body disagree on the route. Harmless as a substring match, but Route <the route that lost> with `dynamic = "error"` would stop the next reader wondering why theirs differs.

apps/store/README.md:L34: nit: the new text names scripts/wait-for-catalog.mts and drops Wait for a compatible catalog, which the sentence it replaced carried. The script path is the better anchor for git blame, which is your stated reason and a good one, but someone mid-incident is looking at the Forgejo run, where the step name is on screen and the script path is not. Naming both costs four words and connects the paragraph to what the reader is actually staring at. That is the only thing I would change about the README addition; the bound itself is correct and belongs there.

Nothing else. The warm-cache and dead-port trade stays settled, the tests remain a real guard, and once L26 and L140 are corrected this is good to merge without another round from me.

VERDICT: request-changes Round 3 at `1f077a6`. Two of the three round-2 corrections are now genuinely right and I re-derived both from scratch rather than checking them off. The third — `NEXT_STATIC_GEN_BAILOUT` — was corrected in the code comment and left standing in the PR body, in the one place that presents it as produced output. That is the only thing between this and an approve. It is a body edit, no code change, and I will not need to see the code again. **Delta is comment-only, confirmed before anything else.** Stripping every full-line comment and blank line from `apps/store/lib/catalog.ts` at both commits leaves 31 non-comment lines on each side, byte-identical (`sha256` `f4f48635fbf51aea…` on both). `README.md` is the only other changed file. So rounds 1-2 evidence transfers intact and I did not re-litigate it. **1. `force-cache` attribution is correct.** Re-derived rename-safely, with no pathspec: `git log --oneline --all -S'force-cache'` bottoms out at `6e36a27`, and `git grep` per commit confirms the three-point chain — `git grep -l force-cache 6e36a27^` finds nothing anywhere in the tree, `6e36a27` gives `lib/catalog.ts:116`, and `9abbe83^` already has `apps/store/lib/catalog.ts:116`. So `9abbe83` preserved it rather than adding it, exactly as the body now says. Worth recording for the next person: `git log --follow -- apps/store/lib/catalog.ts` *also* fails here. It stops at `9abbe83` and never reaches `6e36a27`, so `--follow` is not a sufficient escape from this rename either. Dropping the pathspec is. **2. Impact of 14 is correct, and the chain holds link by link.** The numbers in the body are forge **task** ids (`/actions/tasks`, whose `id` field is 949/952/…); the `/actions/runs` `index_in_repo` is a different sequence — 79 and 82 for these two — which is worth stating because reading the wrong one makes the whole list look invented. Verified: `949` = `af6ab4c`, push, success, 11:12:44Z, the run that introduced `path: apps/store/.next/cache`, so nothing to restore and it saved the entry on success. `952` = `bed5b851`, push, success, **12:39:16Z**, matching your timestamp to the second. Its restore is a direct prefix hit — `git diff af6ab4cd bed5b851 -- pnpm-lock.yaml` is empty, so `next-store-<lockhash>-` still matches 949's key — and `bed5b851` touches `apps/store/app/{globals.css,layout.tsx,page.tsx,products/product-card.tsx}`, so the `apps/store/**` path filter matched. The three earlier successes (934, 944, 947) predate the cache step and were cold. Successful `deploy-store.yml` tasks from 952 onward are exactly 952, 953, 960, 963, 967, 968, 973, 974, 976, 977, 979, 980, 981, 984: one push, six `workflow_dispatch` on 08-25, seven push on 08-27/28. **14.** Your set and your per-line split are both right. **3. The comment is now correct; the body is not.** I ran the comment's own instructions rather than reasoning about them. `no-store` with the `catch` intact fails as `Error: Catalog endpoint is unavailable` plus `digest: '944732262'` — the masking you describe. Removing the `catch` prints exactly what the comment promises: ``Route /cart with `dynamic = "error"` couldn't be rendered statically because it used `revalidate: 0 fetch http://127.0.0.1:8865/api/public/catalog?build=mtcc2f6p /cart` ``. `grep -c NEXT_STATIC_GEN_BAILOUT` over that entire build log is **0**, and unlike the masked path the printer emits no properties block at all. The `.code` claim checks out at source: `next/dist/client/components/static-generation-bailout.js:23` defines the constant, line 26 assigns it to `this.code`, and the one consumer that could surface it, `next/dist/cli/next-build.js:102`, prints `err.message` alone. So `catalog.ts:L20-21` is accurate and its instructions produce what they promise. **Your lint evidence passes all three parts, and I verified it rather than read it.** `pnpm exec biome --version` → 2.4.16, and no `biome` exists under any `node_modules` in this repo, so it falls through to the nix binary — the `npx biome@0.3.3` trap cannot be what produced this. `pnpm exec biome check lib/catalog.ts lib/catalog.test.ts next.config.ts` → `Checked 3 files in 6ms. No fixes applied.` with **no** `Found N warnings` / `Found N errors` line, so clean and not merely exit-0. I made it fail on purpose to prove the detector detects: appending `const unusedBadFormat = 1` to `catalog.ts` gave `Checked 1 file` + `Found 1 error.` + `Found 1 warning.`, then restoring reproduced the clean run. I used a real scratch edit to the real file, not `--stdin-file-path`, since that reports the formatting result and masks lint findings. Your L113-125 disclosure of the clean-versus-exit-0 distinction is the right way to write this up. **Rest of the evidence at this head, on my own loopback fixture** — port 8865, hand-written three-item `schemaVersion: 3`, sentinel brand `R65R3-SENTINEL`, so no live-catalog and no probe-row contamination, and `grep` for `REV68` over both fixture and `out/` finds nothing. `pnpm --filter garage-store test` → `# tests 91 / # pass 91 / # fail 0`. Clean build → 18 route entries, **16** HTML files in `out/`, exactly **one** request logged by the fixture, and `out/sitemap.xml` `<lastmod>` equal to my fixture's `generatedAt` to the millisecond, so the provenance check you documented works on a fixture I chose. Diff is 4 files, no lockfile churn, `Closes #64` on line 1, and no `apps/store/app/globals.css`. Both build exit codes I quote came from redirect-only runs, not pipelines, so they mean what they say. `scripts/wait-for-catalog.mts` matches the new README text: `TIMEOUT_MS = 15 * 60 * 1000` (L24), `INTERVAL_MS = 15 * 1000` (L25), unreachable feeds `lastReason` at L61-64 and the loop continues rather than exiting, and exhausting the deadline exits 1 with the purpose-written message at L75-80. The gate sits at `deploy-store.yml:L101` immediately before `Build` at L107, so "seconds-wide window" is a fair description of the gap. Putting this in the README was the right call: it is the fact you want under `git blame` at 3am, not only on the forge. Findings: PR#65 body:L26: blocker: the fenced block quotes `{ code: 'NEXT_STATIC_GEN_BAILOUT' }` as part of the printed error, and no run prints it. I reproduced that exact experiment — `no-store` with the `catch` removed — and the string occurs **zero** times in the whole build log; the bailout path emits no properties block at all, where the masked path does print `digest:`. Line 151 certifies every result as "local command output, quoted as produced", naming this block only as obtained from a modified tree, so it reads as verbatim output that cannot be obtained from any tree. This is the same false string round 2 asked you to correct; the correction reached `catalog.ts` and not here. It matters more than a normal body nit because the squash commit body lands empty in this repo, so this PR body is the entire durable record of a four-day production outage. Delete that line, and if you want the constant mentioned, say what `catalog.ts:L20-21` already says correctly: it is never printed and lives only on `err.code`. PR#65 body:L140: should: "which froze 13 deploys over four days" contradicts L18's corrected "**14 deploys**". The 13→14 fix was applied in the Impact section and missed in Acceptance criteria, so the incident record now states two different counts for the same incident. 14 is right — L15 and L16 enumerate 6 + 7, plus 952 at L14. Change L140 to 14. apps/store/lib/catalog.ts:L18: nit: the quoted message is pinned to ``Route /``, but the route named is whichever of the twelve prerender workers loses the race. I got `Route /` on the masked variant and `Route /cart` on the unmasked one, and your own body at L24 quotes `/cart` — so the comment and the body disagree on the route. Harmless as a substring match, but ``Route <the route that lost> with `dynamic = "error"` `` would stop the next reader wondering why theirs differs. apps/store/README.md:L34: nit: the new text names `scripts/wait-for-catalog.mts` and drops `Wait for a compatible catalog`, which the sentence it replaced carried. The script path is the better anchor for `git blame`, which is your stated reason and a good one, but someone mid-incident is looking at the Forgejo run, where the step name is on screen and the script path is not. Naming both costs four words and connects the paragraph to what the reader is actually staring at. That is the only thing I would change about the README addition; the bound itself is correct and belongs there. Nothing else. The warm-cache and dead-port trade stays settled, the tests remain a real guard, and once L26 and L140 are corrected this is good to merge without another round from me.
yuris commented 2026-08-28 02:45:38 +00:00
Author
Owner
Copy link

Pushed 3b2e6ec. The blocker in review 26 was already fixed when that review was submitted — it read a body 31 seconds stale — so this round is the should and both nits.

Should — the count disagreed with itself

L18 said 14, L148 still said 13. Fixed. Then I did what I should have done the first time and grepped the whole body rather than the section I was editing:

  • 13 still appears on three lines — out/ occurrence counts (111111 × 13, 444444=13, 555555=13). Those are unrelated measurements and correct.
  • 2026-08-25 appears once, on the dispatch-run line, and is right: those six runs genuinely were on 08-25. The window still opens 08-24 with run 952.
  • four days appears twice and is consistent with a window opening 2026-08-24.

So the only stale figure was L148. I have now corrected this number twice and been caught twice; the grep is the lesson.

Also added — which id sequence those numbers are

Named it in the body, because reading the wrong one makes a correct list look fabricated: the 9xx figures are /actions/tasks .id values, not /actions/runs .index_in_repo, where the same two runs are 79 and 82.

Nit — the route in the quoted message is not stable

Taken, and it was a good catch. My comment pinned Route /, the PR body quoted /cart, and you observed both. The route named is whichever of the 12 prerender workers throws first, so any fixed route in a comment is wrong two runs out of three — and the reader who sees a third one concludes the comment is wrong again, which is precisely the failure mode this comment has already had twice.

catalog.ts now phrases it as Route <first route to lose the race> and says explicitly that the route varies because the workers race. The README carries the same correction and additionally now states that NEXT_STATIC_GEN_BAILOUT is never printed by either path — it had said "not the underlying static-generation bailout", which was true but left the string's status ambiguous in the one artifact that survives in git.

On the two propagated corrections

--follow is not an escape from the rename — confirmed, git log --follow -- apps/store/lib/catalog.ts stops at 9abbe83 and never reaches 6e36a27. Worth adding the mechanism for whoever hits this next: --follow resolves renames of a pathspec, but this file's history crosses a repo-layout move where the store moved from the root (lib/catalog.ts) into apps/store/, and the similarity detection does not carry across the commit that restructured the tree. Dropping the pathspec entirely (git log --oneline --all -S'force-cache') plus git grep -l force-cache <commit>^ to prove tree-wide absence is the pair that actually works, and it is what produced the 6e36a27 attribution in the body.

Your source-level confirmation is stronger than my experiment and I would rather cite it. static-generation-bailout.js:23 defining the constant, :26 assigning this.code, and next-build.js:102 printing err.message alone proves the string can never appear. My instrumented run only showed that it did not appear once, and in fact showed the opposite-looking thing — the string in my own console.error output. That is the difference between evidence and proof.

Re-ran at 3b2e6ec: pnpm --filter garage-store test → 91/91; pnpm exec biome check on the 3 changed files → Checked 3 files in 5ms. No fixes applied., no findings line; clean build against the schemaVersion: 3 fixture → exit 0, all 16 routes, <lastmod> matching the fixture's generatedAt. Diff is still 4 files, no lockfile churn, no globals.css.

Pushed `3b2e6ec`. The blocker in review 26 was already fixed when that review was submitted — it read a body 31 seconds stale — so this round is the `should` and both nits. ## Should — the count disagreed with itself `L18` said 14, `L148` still said 13. Fixed. Then I did what I should have done the first time and grepped the whole body rather than the section I was editing: - `13` still appears on three lines — `out/` occurrence counts (`111111` × 13, `444444=13`, `555555=13`). Those are unrelated measurements and correct. - `2026-08-25` appears once, on the dispatch-run line, and is right: those six runs genuinely were on 08-25. The window still *opens* 08-24 with run 952. - `four days` appears twice and is consistent with a window opening 2026-08-24. So the only stale figure was `L148`. I have now corrected this number twice and been caught twice; the grep is the lesson. ## Also added — which id sequence those numbers are Named it in the body, because reading the wrong one makes a correct list look fabricated: the `9xx` figures are `/actions/tasks` `.id` values, **not** `/actions/runs` `.index_in_repo`, where the same two runs are 79 and 82. ## Nit — the route in the quoted message is not stable Taken, and it was a good catch. My comment pinned `Route /`, the PR body quoted `/cart`, and you observed both. The route named is whichever of the 12 prerender workers throws first, so any fixed route in a comment is wrong two runs out of three — and the reader who sees a third one concludes the comment is wrong *again*, which is precisely the failure mode this comment has already had twice. `catalog.ts` now phrases it as `Route <first route to lose the race>` and says explicitly that the route varies because the workers race. The README carries the same correction and additionally now states that `NEXT_STATIC_GEN_BAILOUT` is never printed by either path — it had said "not the underlying static-generation bailout", which was true but left the string's status ambiguous in the one artifact that survives in git. ## On the two propagated corrections **`--follow` is not an escape from the rename** — confirmed, `git log --follow -- apps/store/lib/catalog.ts` stops at `9abbe83` and never reaches `6e36a27`. Worth adding the mechanism for whoever hits this next: `--follow` resolves renames of a *pathspec*, but this file's history crosses a repo-layout move where the store moved from the root (`lib/catalog.ts`) into `apps/store/`, and the similarity detection does not carry across the commit that restructured the tree. Dropping the pathspec entirely (`git log --oneline --all -S'force-cache'`) plus `git grep -l force-cache <commit>^` to prove tree-wide absence is the pair that actually works, and it is what produced the `6e36a27` attribution in the body. **Your source-level confirmation is stronger than my experiment and I would rather cite it.** `static-generation-bailout.js:23` defining the constant, `:26` assigning `this.code`, and `next-build.js:102` printing `err.message` alone proves the string *can never* appear. My instrumented run only showed that it did not appear once, and in fact showed the opposite-looking thing — the string in my own `console.error` output. That is the difference between evidence and proof. Re-ran at `3b2e6ec`: `pnpm --filter garage-store test` → **91/91**; `pnpm exec biome check` on the 3 changed files → `Checked 3 files in 5ms. No fixes applied.`, **no findings line**; clean build against the `schemaVersion: 3` fixture → exit 0, all 16 routes, `<lastmod>` matching the fixture's `generatedAt`. Diff is still 4 files, no lockfile churn, no `globals.css`.
yuris reviewed 2026-08-28 02:57:27 +00:00
yuris left a comment
Copy link

VERDICT: approve-with-nits

Round 4, delta only (1f077a6..3b2e6ec, one commit). Re-verified at 3b2e6ec in a throwaway worktree against my own fixture on port 8391/8392; both stopped and the worktree removed.

Round 3's should is closed, and the figure is right this time. L18 and L148 both read 14, window opening 2026-08-24. I did not take the classification of the surviving numbers on trust — I reproduced each one.

1. The delta is prose-only — proven, not assumed

Stripped full-line comments and blank lines from lib/catalog.ts at both commits:

1f077a6  f4f48635fbf51aea702e87ff3fb46f361cea6498858b1316686ef5ea16f975f0
3b2e6ec  f4f48635fbf51aea702e87ff3fb46f361cea6498858b1316686ef5ea16f975f0

31 lines of real code survive the strip, so the comparison is not vacuous, and a deliberately tampered copy (force-cache→no-store) hashes 803f144… — the detector can be made to fail. catalog.test.ts and next.config.ts are not in the delta at all. Rounds 1–3 evidence transfers intact.

2. Stale-figure sweep — every classification checked, all correct

The three surviving 13s are out/ occurrence counts, and they are right: I built against a single-item fixture and got exactly 13 occurrences of the sentinel; the same build with two items gave 26. That is the number a careless grep would have "corrected" to 14 and broken.

2026-08-25 appears once (L15) and is correct — I pulled the forge task list and filtered on workflow_id=="deploy-store.yml" and status=="success":

952  2026-08-24T12:39:16Z  push               <- first frozen
953 960 963 967 968 973    2026-08-25         workflow_dispatch  (6, all on 08-25)
974 976 977 979 980 981    2026-08-27         push
984                        2026-08-28T01:24:51Z push
                                              => 14

The id set matches the body exactly. four days (L7, L148) is consistent with 08-24 → 08-28. No stale figure survives anywhere in the body.

3. The route nit — the race is real, phrasing is accurate

Deleted the catch and set no-store on a scratch copy, clean .next each run, three runs:

run 1 -> Error: Route /      with `dynamic = "error"` … `revalidate: 0 fetch …`
run 2 -> Error: Route /cart  …
run 3 -> Error: Route /cart  …

Two distinct routes in three runs, so the race framing holds and the old pinned-route wording really was wrong most runs. NEXT_STATIC_GEN_BAILOUT appeared 0 times in all three logs with the catch removed — the "never printed by either path" claim is confirmed at runtime, not just at the Next source. The 12 in the comment is literal: Next prints Collecting page data using 12 workers. A reader who hits a third route will not think the comment is wrong.

4. README is now unambiguous

NEVER printed by either path; it is only ever a property on the thrown error (L29–30) settles the string's status in the one artifact that survives in git, and the paragraph gives a mid-incident reader the whole path: wrong-cause message → masking catch → #66 → what you see after removing it. Sufficient.

5. id-sequence note is correct and worth having

tasks .id 949 -> run_number/index_in_repo 79, af6ab4cd, 2026-08-24T11:12:44Z
tasks .id 952 -> run_number/index_in_repo 82, bed5b851, 2026-08-24T12:39:16Z

Confirmed from the other side too: /actions/runs index_in_repo 79/82 carry .id 852/855 — a third sequence. Naming the sequence is real insurance.

Evidence re-run at 3b2e6ec

  • pnpm --filter garage-store test → # tests 91 / # pass 91 / # fail 0.
  • biome check lib/catalog.ts lib/catalog.test.ts next.config.ts → Checked 3 files in 5ms. No fixes applied., no Found N line, exit 0. Bare biome 2.4.16 from nix, never npx. Fail-on-purpose control on a real scratch file (not stdin): injecting const _unused: any = 1; produced Found 1 warning. while still exiting 0 — reproducing the author's own claim and proving my detector works.
  • Clean build against a schemaVersion: 3 fixture: exit 0 unpiped (echo $? directly, no pipeline), all 16 routes (8 static + 2 products + 6 services), one distinct sentinel price in out/, sitemap.xml <lastmod> = fixture generatedAt exactly, and grep -rE 'build=[a-z0-9]{6,}' out/ no matches.
  • Fix defeats the stale render: warm .next, no source change, changed fixture → new fetch. Two builds, two ids, one request each: REQ #1 ?build=mtccugpb -> 900001, REQ #2 ?build=mtccv5cu -> 900002, and 900001 is gone from out/ entirely.
  • wait-for-catalog.mts: TIMEOUT_MS = 15 * 60 * 1000, INTERVAL_MS = 15 * 1000 — the 15min/15s claim is exact.
  • Diff is 4 files, no lockfile churn, no apps/store/app/globals.css, Closes #64 present at L1.

Findings — 0 blockers, 0 shoulds, 2 nits

apps/store/README.md:L30: nit: this delta unpacked a parenthetical into three sentences, so "Next persists that response" now sits four sentences from its antecedent and the nearest preceding noun is "the thrown error". Fix: "Next persists the catalog response in .next/cache/fetch-cache…".

PR body:L121: nit: the evidence block is labelled (re-run at 1f077a6) while head is 3b2e6ec, and nothing in the body says the delta since is comment-only, so a reader cannot tell the results still apply. Fix: extend the label to (re-run at 1f077a6; delta to 3b2e6ec is comment-only). I re-ran tests, lint and the build at 3b2e6ec above, so this is a labelling gap, not a gap in the evidence.

Neither nit blocks. #66 and fetch-cache growth remain correctly deferred and out of scope here.

VERDICT: approve-with-nits Round 4, delta only (`1f077a6..3b2e6ec`, one commit). Re-verified at `3b2e6ec` in a throwaway worktree against my own fixture on port 8391/8392; both stopped and the worktree removed. **Round 3's `should` is closed, and the figure is right this time.** L18 and L148 both read **14**, window opening **2026-08-24**. I did not take the classification of the surviving numbers on trust — I reproduced each one. ## 1. The delta is prose-only — proven, not assumed Stripped full-line comments and blank lines from `lib/catalog.ts` at both commits: ``` 1f077a6 f4f48635fbf51aea702e87ff3fb46f361cea6498858b1316686ef5ea16f975f0 3b2e6ec f4f48635fbf51aea702e87ff3fb46f361cea6498858b1316686ef5ea16f975f0 ``` 31 lines of real code survive the strip, so the comparison is not vacuous, and a deliberately tampered copy (`force-cache`→`no-store`) hashes `803f144…` — the detector can be made to fail. `catalog.test.ts` and `next.config.ts` are not in the delta at all. Rounds 1–3 evidence transfers intact. ## 2. Stale-figure sweep — every classification checked, all correct The three surviving `13`s are `out/` occurrence counts, and they are **right**: I built against a **single-item** fixture and got exactly **13** occurrences of the sentinel; the same build with **two** items gave **26**. That is the number a careless grep would have "corrected" to 14 and broken. `2026-08-25` appears once (L15) and is correct — I pulled the forge task list and filtered on `workflow_id=="deploy-store.yml" and status=="success"`: ``` 952 2026-08-24T12:39:16Z push <- first frozen 953 960 963 967 968 973 2026-08-25 workflow_dispatch (6, all on 08-25) 974 976 977 979 980 981 2026-08-27 push 984 2026-08-28T01:24:51Z push => 14 ``` The id set matches the body exactly. `four days` (L7, L148) is consistent with 08-24 → 08-28. **No stale figure survives anywhere in the body.** ## 3. The route nit — the race is real, phrasing is accurate Deleted the `catch` and set `no-store` on a scratch copy, clean `.next` each run, three runs: ``` run 1 -> Error: Route / with `dynamic = "error"` … `revalidate: 0 fetch …` run 2 -> Error: Route /cart … run 3 -> Error: Route /cart … ``` Two distinct routes in three runs, so the race framing holds and the old pinned-route wording really was wrong most runs. `NEXT_STATIC_GEN_BAILOUT` appeared **0** times in all three logs with the catch removed — the "never printed by either path" claim is confirmed at runtime, not just at the Next source. The `12` in the comment is literal: Next prints `Collecting page data using 12 workers`. A reader who hits a third route will not think the comment is wrong. ## 4. README is now unambiguous `NEVER printed by either path; it is only ever a property on the thrown error` (L29–30) settles the string's status in the one artifact that survives in git, and the paragraph gives a mid-incident reader the whole path: wrong-cause message → masking `catch` → #66 → what you see after removing it. Sufficient. ## 5. id-sequence note is correct and worth having ``` tasks .id 949 -> run_number/index_in_repo 79, af6ab4cd, 2026-08-24T11:12:44Z tasks .id 952 -> run_number/index_in_repo 82, bed5b851, 2026-08-24T12:39:16Z ``` Confirmed from the other side too: `/actions/runs` `index_in_repo` 79/82 carry `.id` **852/855** — a *third* sequence. Naming the sequence is real insurance. ## Evidence re-run at `3b2e6ec` - `pnpm --filter garage-store test` → `# tests 91 / # pass 91 / # fail 0`. - `biome check lib/catalog.ts lib/catalog.test.ts next.config.ts` → `Checked 3 files in 5ms. No fixes applied.`, **no** `Found N` line, exit 0. Bare `biome` 2.4.16 from nix, never `npx`. Fail-on-purpose control on a real scratch file (not stdin): injecting `const _unused: any = 1;` produced `Found 1 warning.` **while still exiting 0** — reproducing the author's own claim and proving my detector works. - Clean build against a `schemaVersion: 3` fixture: **exit 0 unpiped** (`echo $?` directly, no pipeline), all **16** routes (8 static + 2 products + 6 services), one distinct sentinel price in `out/`, `sitemap.xml` `<lastmod>` = fixture `generatedAt` exactly, and `grep -rE 'build=[a-z0-9]{6,}' out/` no matches. - Fix defeats the stale render: warm `.next`, **no source change**, changed fixture → new fetch. Two builds, two ids, one request each: `REQ #1 ?build=mtccugpb -> 900001`, `REQ #2 ?build=mtccv5cu -> 900002`, and `900001` is gone from `out/` entirely. - `wait-for-catalog.mts`: `TIMEOUT_MS = 15 * 60 * 1000`, `INTERVAL_MS = 15 * 1000` — the 15min/15s claim is exact. - Diff is 4 files, no lockfile churn, no `apps/store/app/globals.css`, `Closes #64` present at L1. ## Findings — 0 blockers, 0 shoulds, 2 nits apps/store/README.md:L30: nit: this delta unpacked a parenthetical into three sentences, so "Next persists that response" now sits four sentences from its antecedent and the nearest preceding noun is "the thrown error". Fix: "Next persists the catalog response in `.next/cache/fetch-cache`…". PR body:L121: nit: the evidence block is labelled `(re-run at 1f077a6)` while head is `3b2e6ec`, and nothing in the body says the delta since is comment-only, so a reader cannot tell the results still apply. Fix: extend the label to `(re-run at 1f077a6; delta to 3b2e6ec is comment-only)`. I re-ran tests, lint and the build at `3b2e6ec` above, so this is a labelling gap, not a gap in the evidence. Neither nit blocks. #66 and fetch-cache growth remain correctly deferred and out of scope here.
yuris commented 2026-08-28 02:59:34 +00:00
Author
Owner
Copy link

Merge-gate verification, re-run at the exact merge SHA 3b2e6ec. Both nits declined, deliberately and with reasons recorded below — no push, so review 30 stands.

Verification at 3b2e6ec

$ git rev-parse HEAD
3b2e6ecbd457078fd1629bc0e8a70ccfe222cee1

$ pnpm --filter garage-store test
# tests 91
# suites 16
# pass 91
# fail 0

$ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts next.config.ts
Checked 3 files in 5ms. No fixes applied.

The lint claim is clean, not merely a zero exit: the output has a Checked N files line proving the real 2.4.16 binary ran, and no Found N warnings/errors line. Exit status is not offered as evidence, since biome exits 0 on warnings.

Build, unpiped, against a schemaVersion: 3 fixture:

$ pnpm build
✓ Compiled successfully in 2.1s
  Collecting page data using 12 workers ...
✓ Generating static pages using 12 workers (16/16) in 634ms
  ├ ● /products/[slug]
  │ └ /products/oli-mesin
BUILD_EXIT=0

All 16 routes prerendered, product and service detail pages generated from catalog data.

Nit 1 — evidence block labelled 1f077a6 while head is 3b2e6ec: declined

The label is imprecise, but the record is not ambiguous: review 30 is itself pinned to 3b2e6ec and independently re-ran tests, lint and the build at that head, and the block above re-runs all three again at the merge SHA. A reader has two independent confirmations at the merged commit. Not worth voiding an approval and buying a fifth round.

Nit 2 — antecedent drift on "Next persists that response" (README:L30): declined, and recorded as riding the next touch

This one I am not dismissing. It is real, and it is in the artifact that actually survives in git — which is precisely the argument I have been making for four rounds about why the comment and README matter more than this body.

Declining anyway, on cost: it is a five-word pronoun fix on a PR that closes a live production freeze that has been running for four days, and pushing for it voids the approval and costs a full review round.

Recorded as deferred, not dropped: it rides the next touch of that file. #66 is already filed and will edit apps/store/lib/catalog.ts — and almost certainly this same README paragraph, since #66 concerns the very catch the paragraph exists to explain. Whoever takes #66 should fix the antecedent in passing.

Merging

Squash, head-SHA compare-and-swap on 3b2e6ec, delete_branch_after_merge: true. No force_merge, no merge_when_checks_succeed.

MergeMessageField will be passed explicitly, because this repo's squash commits land with zero-byte bodies — the PR body does not reach git log. The impact line goes in the commit message so a future reader finds it from git log rather than only on the forge.

Merging deploys to production: push: main touching apps/store/** fires deploy-store.yml, which builds the store and runs wrangler pages deploy apps/store/out. Since this PR fixes a defect in the deploy path itself, a green workflow run is not sufficient evidence — a successful deploy serving stale content is the exact failure being fixed. I will confirm the run is green and that the live site reflects the current catalog.

Merge-gate verification, re-run at the exact merge SHA **`3b2e6ec`**. Both nits declined, deliberately and with reasons recorded below — no push, so review 30 stands. ## Verification at `3b2e6ec` ``` $ git rev-parse HEAD 3b2e6ecbd457078fd1629bc0e8a70ccfe222cee1 $ pnpm --filter garage-store test # tests 91 # suites 16 # pass 91 # fail 0 $ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts next.config.ts Checked 3 files in 5ms. No fixes applied. ``` The lint claim is **clean**, not merely a zero exit: the output has a `Checked N files` line proving the real 2.4.16 binary ran, and **no `Found N warnings/errors` line**. Exit status is not offered as evidence, since biome exits 0 on warnings. Build, unpiped, against a `schemaVersion: 3` fixture: ``` $ pnpm build ✓ Compiled successfully in 2.1s Collecting page data using 12 workers ... ✓ Generating static pages using 12 workers (16/16) in 634ms ├ ● /products/[slug] │ └ /products/oli-mesin BUILD_EXIT=0 ``` All 16 routes prerendered, product and service detail pages generated from catalog data. ## Nit 1 — evidence block labelled `1f077a6` while head is `3b2e6ec`: declined The label is imprecise, but the record is not ambiguous: **review 30 is itself pinned to `3b2e6ec` and independently re-ran tests, lint and the build at that head**, and the block above re-runs all three again at the merge SHA. A reader has two independent confirmations at the merged commit. Not worth voiding an approval and buying a fifth round. ## Nit 2 — antecedent drift on "Next persists that response" (`README:L30`): declined, and recorded as riding the next touch This one I am not dismissing. It is real, and it is in the artifact that actually survives in `git` — which is precisely the argument I have been making for four rounds about why the comment and README matter more than this body. Declining anyway, on cost: it is a five-word pronoun fix on a PR that closes a **live production freeze that has been running for four days**, and pushing for it voids the approval and costs a full review round. **Recorded as deferred, not dropped:** it rides the next touch of that file. #66 is already filed and will edit `apps/store/lib/catalog.ts` — and almost certainly this same README paragraph, since #66 concerns the very `catch` the paragraph exists to explain. Whoever takes #66 should fix the antecedent in passing. ## Merging Squash, head-SHA compare-and-swap on `3b2e6ec`, `delete_branch_after_merge: true`. No `force_merge`, no `merge_when_checks_succeed`. `MergeMessageField` will be passed explicitly, because this repo's squash commits land with zero-byte bodies — the PR body does **not** reach `git log`. The impact line goes in the commit message so a future reader finds it from `git log` rather than only on the forge. **Merging deploys to production**: `push: main` touching `apps/store/**` fires `deploy-store.yml`, which builds the store and runs `wrangler pages deploy apps/store/out`. Since this PR fixes a defect *in the deploy path itself*, a green workflow run is not sufficient evidence — a successful deploy serving stale content is the exact failure being fixed. I will confirm the run is green **and** that the live site reflects the current catalog.
yuris merged commit c4d96c88d8 into main 2026-08-28 03:00:06 +00:00
yuris deleted branch chore/64-catalog-cache-freshness 2026-08-28 03:00:07 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
yuris/garage!65
No description provided.