fix(store): rethrow Next bailout and digest errors instead of relabelling them as a catalog outage #75

Merged

yuris merged 9 commits from fix/66-catalog-catch-masks-bailout into main 2026-09-02 17:25:42 +00:00

Conversation 7 Commits 9 Files changed 2 +503 -14

yuris commented 2026-08-29 06:25:36 +00:00

Owner

Copy link

Closes #66.

fetchCatalog's catch relabelled every throw as Catalog endpoint is unavailable, including Next's own control-flow signals. A static-generation bailout is not an error — it is how Next tells the build "this route cannot be static". Swallowing it turned an intended bailout into a fake outage: the build fails, and the one line an engineer reads sends them to look at a catalog endpoint that is healthy.

Measured at origin/main with the bare catch, twice in a row so the second run had a warm .next/cache:

build 1: exit=1  masked=1  out/ exists=no  html pages=0
build 2: exit=1  masked=1  out/ exists=no  html pages=0

An earlier version of this body claimed the swallowed bailout also shipped a silently stale catalog. That is false and it was not this issue's defect: a swallowed bailout fails the build, out/ is never created, and deploy-store.yml only reaches wrangler pages deploy apps/store/out after the build step succeeds — so nothing is deployed, stale or otherwise. The stale-catalog defect was #64's (force-cache with a one-year TTL re-rendering the previous build's response), it is already fixed by CATALOG_BUILD_ID in next.config.ts, and this PR does not touch it. #66's complaint is entirely that the message sends you to the wrong place.

The fix rethrows framework control flow untouched and redacts only genuine faults.

The predicate, as it actually stands

const MAX_CAUSE_LINKS = 1_000_000;

function isFrameworkControlFlow(error: unknown): boolean {
  const seen = new WeakSet<object>();
  let links = 0;
  let node: unknown = error;
  while (
    (typeof node === "object" || typeof node === "function") &&
    node !== null
  ) {
    if (++links > MAX_CAUSE_LINKS) return false;
    if (seen.has(node)) return false;
    seen.add(node);
    let code: unknown;
    let digest: unknown;
    let cause: unknown;
    try {
      ({ code, digest, cause } = node as {
        code?: unknown;
        digest?: unknown;
        cause?: unknown;
      });
    } catch {
      return false;
    }
    if (code === "NEXT_STATIC_GEN_BAILOUT" || typeof digest === "string") {
      return true;
    }
    node = cause;
  }
  return false;
}

Deliberately not unstable_rethrow: per next@16.2.10's own checklist that helper omits isStaticGenBailoutError, which is the exact error this issue is about, so reaching for it would have masked the bug it was meant to fix. Verified against the installed version, not from memory.

What review found, in the order it was found

This is the part worth reading, because the branch is mostly a record of the guard's own claims being wrong and getting narrower.

Round	Defect	Outcome
37	comment said the walk went one level; it did not	comment corrected
58	instanceof Error gate stopped at the first non-Error link, so new Error("x", { cause: { cause: bailout } }) was redacted and the marker missed	gate dropped
58	a throwing getter or Proxy trap on code/digest/cause escaped the redacting catch and surfaced raw	property reads guarded, fails closed to redact
58	a.cause = b; b.cause = a recursed until stack exhaustion	WeakSet cycle guard
58	recursion was stack-bounded, and the cycle guard lowered the ceiling from ~15.7k to ~12.6k frames — a deep benign chain still crashed the build	rewritten as a loop
63	the loop closed the depth hazard and opened a worse one	link cap
63	typeof node === "object" skipped a marker on a callable cause	callables followed
63	dropping the instanceof gate widened what gets inspected	widening named in the comment

Round 63's finding, because it is the interesting one

A WeakSet keys on identity, so it terminates revisits, not chains. A .cause accessor returning a fresh object per read never revisits, so the guard never fires:

loop, no cap  : 30,000,000 links in 19.4s, still going, event loop blocked
recursion     : RangeError after 7,853 links in 37ms

The comment at the time asserted that the seen set was "the only thing that has to terminate anything" — false, and the fifth version of that comment to overstate the walk's reach.

Not reachable in a build; the real chains are TypeError: fetch failed -> system Error and a bare SyntaxError, both terminating in two hops. It is the standard already applied twice here — nothing in Node prevents a .cause cycle either, and that got fixed. What makes it worth fixing rather than caveating is the asymmetry: a RangeError fails the build in 37ms with a stack trace, while a hang consumes a runner until something external kills it, emitting nothing and answering nothing. Crash-fast is a bad outcome; hang-silent is a worse one, and the loop had swapped the good failure for the bad one.

So the walk now bounds work as well as revisits: one comparison per link, MAX_CAUSE_LINKS = 1_000_000 — ~20x the deepest chain under test. Both claims hold at once: unbounded by any stack, and terminating on every possible input.

The bound is links, not wall time, and an earlier version of this body said "~0.7s" as though it were both. Per-link cost is whatever the accessor does, so the identical 1,000,000 links span 24x:

pre-built plain chain, no accessors      217ms
Proxy, fresh proxy per link              381ms
getter, fresh {} per link                651ms
getter, fresh Error per link           5,261ms

The last is the realistic worst case rather than the first — an error-wrapping library building a cause lazily produces Errors, and each captures a stack trace — so the old figure was 8x optimistic, measured on the shape the test fixture happens to use. A getter doing real work per read is unbounded in time whatever the counter says; that is inherent to walking a chain whose links are user code, and it is why the guarantee is "≤1M links" and nothing more.

1M is kept rather than lowered: the suite already pins 50k-link chains, so 100k would leave only 2x headroom over the tests, and 5.26s on a shape that cannot reach this code is not worth buying that back.

Verification

# tests 110
# pass  110
# fail  0

tsc --noEmit exit 0 from apps/store; biome clean on catalog.ts and catalog.test.ts.

Every guard is individually pinned by a negative control — the point being that a test which passes with its fix removed is not evidence:

All re-measured at this head (b1f9b8a, suite of 110) rather than carried forward — the earlier version of this table mixed /108 and /110 denominators from two different heads, so four of six rows did not match a fresh pnpm test:

restore the instanceof gate  -> 108/110, FAILS 2: "…through a plain object"
                                              AND "…through a callable link"
remove the accessor guard    -> 109/110, fails 1: "redacts rather than escaping
                                                  when an accessor throws"
remove the cycle guard       -> HANGS, killed at 120s, no test output at all
revert the loop to recursion -> 108/110, fails 2, both RangeError at 50k
remove the link cap          -> HANGS, killed at 90s, no test output at all
exclude callables again      -> 109/110, fails 1: "follows a cause through a
                                                  callable link"

The instanceof control now fails two tests rather than one, because the callable test depends on that same line — it is load-bearing for two guarantees now.

Two of those controls fail as a timeout rather than an assertion, which is the honest shape of that defect class.

Reviewer's battery through the real fetchCatalog boundary — 29/29 at this head (b1f9b8a), bailouts-swallowed=0, escapes-or-crashes=0. The often-quoted 39/39 is review 63's figure at c3e716f; the composition changed here, dropping the depth-ceiling cases the loop retired and adding the callable surface. Covers pure cycles of length 1–4, lassos with the marker at every index, a marker on a self-cycling node, a shared cause across three separate calls (no cross-call leakage — seen is function-local), hostile getters and Proxy traps, and benign chains at 20k/50k/200k links.

Known, deliberate, and recorded in the code

• typeof digest === "string" is broad. React and Next also stamp a decimal hash digest (digest: '3368504262') onto errors they report, and this cannot tell that apart from DYNAMIC_SERVER_USAGE. Safe because a hashed error cannot arrive here — the hash is assigned at the render boundary, above this call.
• Dropping the instanceof gate widened the un-redact surface. Anything behind a plain or callable link carrying a string digest now rethrows — the concrete case being class Bar { static digest = "sha256-abc" } as a cause, and digest is a common property name outside Next (crypto digests, SRI hashes, registry metadata). Every realistic fault still redacts, and digest was undefined at every level of every real chain measured — recorded because it is a wider net than "a Next error", not because it is known to bite.
• Nothing here can leak a credential. CATALOG_API_KEY goes into headers only, never the URL or any message, verified with a distinctive key against a real build at 0 occurrences.

Closes #66. `fetchCatalog`'s catch relabelled **every** throw as `Catalog endpoint is unavailable`, including Next's own control-flow signals. A static-generation bailout is not an error — it is how Next tells the build "this route cannot be static". Swallowing it turned an intended bailout into a **fake outage**: the build fails, and the one line an engineer reads sends them to look at a catalog endpoint that is healthy. Measured at `origin/main` with the bare catch, twice in a row so the second run had a warm `.next/cache`: ``` build 1: exit=1 masked=1 out/ exists=no html pages=0 build 2: exit=1 masked=1 out/ exists=no html pages=0 ``` An earlier version of this body claimed the swallowed bailout also shipped a **silently stale catalog**. That is false and it was not this issue's defect: a swallowed bailout *fails the build*, `out/` is never created, and `deploy-store.yml` only reaches `wrangler pages deploy apps/store/out` after the build step succeeds — so nothing is deployed, stale or otherwise. The stale-catalog defect was **#64**'s (`force-cache` with a one-year TTL re-rendering the previous build's response), it is already fixed by `CATALOG_BUILD_ID` in `next.config.ts`, and this PR does not touch it. #66's complaint is entirely that the message sends you to the wrong place. The fix rethrows framework control flow untouched and redacts only genuine faults. ## The predicate, as it actually stands ```ts const MAX_CAUSE_LINKS = 1_000_000; function isFrameworkControlFlow(error: unknown): boolean { const seen = new WeakSet<object>(); let links = 0; let node: unknown = error; while ( (typeof node === "object" || typeof node === "function") && node !== null ) { if (++links > MAX_CAUSE_LINKS) return false; if (seen.has(node)) return false; seen.add(node); let code: unknown; let digest: unknown; let cause: unknown; try { ({ code, digest, cause } = node as { code?: unknown; digest?: unknown; cause?: unknown; }); } catch { return false; } if (code === "NEXT_STATIC_GEN_BAILOUT" || typeof digest === "string") { return true; } node = cause; } return false; } ``` Deliberately **not** `unstable_rethrow`: per `next@16.2.10`'s own checklist that helper omits `isStaticGenBailoutError`, which is the exact error this issue is about, so reaching for it would have masked the bug it was meant to fix. Verified against the installed version, not from memory. ## What review found, in the order it was found This is the part worth reading, because the branch is mostly a record of the guard's own claims being wrong and getting narrower. | Round | Defect | Outcome | |---|---|---| | 37 | comment said the walk went one level; it did not | comment corrected | | 58 | `instanceof Error` gate stopped at the first non-Error link, so `new Error("x", { cause: { cause: bailout } })` was redacted and the marker missed | gate dropped | | 58 | a throwing getter or Proxy trap on `code`/`digest`/`cause` escaped the redacting catch and surfaced raw | property reads guarded, fails closed to *redact* | | 58 | `a.cause = b; b.cause = a` recursed until stack exhaustion | `WeakSet` cycle guard | | 58 | recursion was stack-bounded, and the cycle guard *lowered* the ceiling from ~15.7k to ~12.6k frames — a deep benign chain still crashed the build | rewritten as a loop | | 63 | **the loop closed the depth hazard and opened a worse one** | link cap | | 63 | `typeof node === "object"` skipped a marker on a callable cause | callables followed | | 63 | dropping the `instanceof` gate widened what gets inspected | widening named in the comment | ### Round 63's finding, because it is the interesting one A `WeakSet` keys on **identity**, so it terminates *revisits*, not *chains*. A `.cause` accessor returning a fresh object per read never revisits, so the guard never fires: ``` loop, no cap : 30,000,000 links in 19.4s, still going, event loop blocked recursion : RangeError after 7,853 links in 37ms ``` The comment at the time asserted that the `seen` set was "the only thing that has to terminate anything" — false, and the fifth version of that comment to overstate the walk's reach. Not reachable in a build; the real chains are `TypeError: fetch failed -> system Error` and a bare `SyntaxError`, both terminating in two hops. It is the standard already applied twice here — nothing in Node prevents a `.cause` cycle either, and that got fixed. What makes it worth fixing rather than caveating is the **asymmetry**: a `RangeError` fails the build in 37ms with a stack trace, while a hang consumes a runner until something external kills it, emitting nothing and answering nothing. Crash-fast is a bad outcome; hang-silent is a worse one, and the loop had swapped the good failure for the bad one. So the walk now bounds work as well as revisits: one comparison per link, `MAX_CAUSE_LINKS = 1_000_000` — ~20x the deepest chain under test. Both claims hold at once: unbounded by any stack, and terminating on every possible input. The bound is **links, not wall time**, and an earlier version of this body said "~0.7s" as though it were both. Per-link cost is whatever the accessor does, so the identical 1,000,000 links span 24x: ``` pre-built plain chain, no accessors 217ms Proxy, fresh proxy per link 381ms getter, fresh {} per link 651ms getter, fresh Error per link 5,261ms ``` The last is the realistic worst case rather than the first — an error-wrapping library building a cause lazily produces `Error`s, and each captures a stack trace — so the old figure was 8x optimistic, measured on the shape the test fixture happens to use. A getter doing real work per read is unbounded in time whatever the counter says; that is inherent to walking a chain whose links are user code, and it is why the guarantee is "≤1M links" and nothing more. 1M is kept rather than lowered: the suite already pins 50k-link chains, so 100k would leave only 2x headroom over the tests, and 5.26s on a shape that cannot reach this code is not worth buying that back. ## Verification ``` # tests 110 # pass 110 # fail 0 ``` `tsc --noEmit` exit 0 from `apps/store`; biome clean on `catalog.ts` and `catalog.test.ts`. Every guard is individually pinned by a negative control — the point being that a test which passes with its fix removed is not evidence: All re-measured at this head (`b1f9b8a`, suite of 110) rather than carried forward — the earlier version of this table mixed `/108` and `/110` denominators from two different heads, so four of six rows did not match a fresh `pnpm test`: ``` restore the instanceof gate -> 108/110, FAILS 2: "…through a plain object" AND "…through a callable link" remove the accessor guard -> 109/110, fails 1: "redacts rather than escaping when an accessor throws" remove the cycle guard -> HANGS, killed at 120s, no test output at all revert the loop to recursion -> 108/110, fails 2, both RangeError at 50k remove the link cap -> HANGS, killed at 90s, no test output at all exclude callables again -> 109/110, fails 1: "follows a cause through a callable link" ``` The `instanceof` control now fails **two** tests rather than one, because the callable test depends on that same line — it is load-bearing for two guarantees now. Two of those controls fail as a **timeout rather than an assertion**, which is the honest shape of that defect class. Reviewer's battery through the real `fetchCatalog` boundary — **29/29 at this head** (`b1f9b8a`), `bailouts-swallowed=0`, `escapes-or-crashes=0`. The often-quoted **39/39 is review 63's figure at `c3e716f`**; the composition changed here, dropping the depth-ceiling cases the loop retired and adding the callable surface. Covers pure cycles of length 1–4, lassos with the marker at every index, a marker on a self-cycling node, a shared cause across three separate calls (no cross-call leakage — `seen` is function-local), hostile getters and Proxy traps, and benign chains at 20k/50k/200k links. ## Known, deliberate, and recorded in the code - **`typeof digest === "string"` is broad.** React and Next also stamp a decimal hash digest (`digest: '3368504262'`) onto errors they report, and this cannot tell that apart from `DYNAMIC_SERVER_USAGE`. Safe because a hashed error cannot arrive here — the hash is assigned at the render boundary, above this call. - **Dropping the `instanceof` gate widened the un-redact surface.** Anything behind a plain **or callable** link carrying a string `digest` now rethrows — the concrete case being `class Bar { static digest = "sha256-abc" }` as a cause, and `digest` is a common property name outside Next (crypto digests, SRI hashes, registry metadata). Every realistic fault still redacts, and `digest` was undefined at every level of every real chain measured — recorded because it is a wider net than "a Next error", not because it is known to bite. - **Nothing here can leak a credential.** `CATALOG_API_KEY` goes into headers only, never the URL or any message, verified with a distinctive key against a real build at 0 occurrences.

yuris added 3 commits 2026-08-29 06:25:36 +00:00

fix(store): rethrow Next framework errors instead of relabelling them as an outage f124d68f58

test(store): cover the rethrow and redaction paths; correct the measured claims in catalog.ts 91ac28cbf7

style(store): biome formatting for the new catalog tests fa9030e7b7

yuris added 1 commit 2026-08-29 06:26:58 +00:00

docs(store): record both observed bailout routes and that the worker race is biased, not fixed 2b89dd0d19

yuris commented 2026-08-29 06:27:54 +00:00

Author

Owner

Copy link

Pushed 2b89dd0 — one commit, comment text only, no behaviour change. This voids any verdict formed against fa9030e.

What changed and why. My first version of the route comment said the bailout named /cart in all five runs measured here, and dropped the prior claim that the route varies. That narrowing was wrong. Contrary measured data exists from two independent rounds on #65: one reviewer saw / on one variant and /cart on another, and another ran it three times deliberately and got /, /cart, /cart.

Five consecutive /cart results do not refute the variance — they show the race is biased, not fixed. My five runs were the same error the earlier rounds made, just at larger n: a counterexample about one run says nothing about the class. Anyone who pinned on /cart because of my five runs would decide the comment was wrong the first time they saw /, which is exactly the failure this comment has already had three times.

The comment now carries both observations, both counts, and the mechanism:

  // Do not pin on the route named there. It is whichever of the 12 prerender
  // workers loses the race, and the race is biased, not fixed: five consecutive
  // runs while writing this all reported /cart, while earlier rounds on #65
  // reported / on one run and /cart on two others. Both routes are real
  // observations of the same mechanism. Match on the message.

It lives in the code rather than only here, because a squash commit body does not carry a PR discussion into the repository.

Also amended in the PR body, no code involved: the biome number now states the scope that produced it. Six warnings is pnpm exec biome check with no path argument from apps/store — 38 files. Verified the narrower scopes so a reviewer comparing against origin/main reaches the same number rather than a different-but-also-correct one:

$ pnpm exec biome check app/globals.css
Checked 1 file in 18ms. No fixes applied.
Found 2 warnings.

$ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts
Checked 2 files in 6ms. No fixes applied.

Still 6 pre-existing warnings app-wide, still not clean, still all in files this PR does not touch, still left alone.

Re-verified at 2b89dd0:

$ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts
Checked 2 files in 6ms. No fixes applied.

$ pnpm exec tsc --noEmit          # from apps/store
tsc-ok

$ pnpm test
# tests 100
# pass 100
# fail 0

The builds in §1, §3 and §4 of the body were run against code identical to this head apart from these comment lines, so their output stands unchanged. Ready for review at 2b89dd0.

Pushed `2b89dd0` — one commit, comment text only, no behaviour change. This voids any verdict formed against `fa9030e`. **What changed and why.** My first version of the route comment said the bailout named `/cart` in all five runs measured here, and dropped the prior claim that the route varies. That narrowing was wrong. Contrary measured data exists from two independent rounds on #65: one reviewer saw `/` on one variant and `/cart` on another, and another ran it three times deliberately and got `/`, `/cart`, `/cart`. Five consecutive `/cart` results do not refute the variance — they show the race is **biased, not fixed**. My five runs were the same error the earlier rounds made, just at larger n: a counterexample about one run says nothing about the class. Anyone who pinned on `/cart` because of my five runs would decide the comment was wrong the first time they saw `/`, which is exactly the failure this comment has already had three times. The comment now carries both observations, both counts, and the mechanism: ```ts // Do not pin on the route named there. It is whichever of the 12 prerender // workers loses the race, and the race is biased, not fixed: five consecutive // runs while writing this all reported /cart, while earlier rounds on #65 // reported / on one run and /cart on two others. Both routes are real // observations of the same mechanism. Match on the message. ``` It lives in the code rather than only here, because a squash commit body does not carry a PR discussion into the repository. **Also amended in the PR body**, no code involved: the biome number now states the scope that produced it. Six warnings is `pnpm exec biome check` with no path argument from `apps/store` — 38 files. Verified the narrower scopes so a reviewer comparing against `origin/main` reaches the same number rather than a different-but-also-correct one: ``` $ pnpm exec biome check app/globals.css Checked 1 file in 18ms. No fixes applied. Found 2 warnings. $ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts Checked 2 files in 6ms. No fixes applied. ``` Still 6 pre-existing warnings app-wide, still not clean, still all in files this PR does not touch, still left alone. **Re-verified at `2b89dd0`:** ``` $ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts Checked 2 files in 6ms. No fixes applied. $ pnpm exec tsc --noEmit # from apps/store tsc-ok $ pnpm test # tests 100 # pass 100 # fail 0 ``` The builds in §1, §3 and §4 of the body were run against code identical to this head apart from these comment lines, so their output stands unchanged. Ready for review at `2b89dd0`.

yuris added 1 commit 2026-08-29 07:13:09 +00:00

test(store): cover all three measured network-fault codes, not just ECONNREFUSED 17a3ce51ea

yuris commented 2026-08-29 07:14:58 +00:00

Author

Owner

Copy link

Pushed 17a3ce5. This voids any verdict formed against 2b89dd0 or fa9030e. Sorry for moving the head under a review in flight — this is the last push unless a blocker comes back.

Why I pushed instead of leaving it. My redaction test constructed the network-fault shape by hand (TypeError: fetch failed with an ECONNREFUSED cause). A hand-built fixture asserting a shape nobody measured is a test of my assumption, not of Node, and the load-bearing security claim in this PR — that a real network fault cannot reach the rethrow branch — rested on it. So I measured the real thing:

$ node -e '... await fetch(...).catch(show) ...'
closed-port   | TypeError | msg="fetch failed" | own=["message","cause"] | code=undefined | digest=undefined | cause=Error code="ECONNREFUSED" digest=undefined
dns-failure   | TypeError | msg="fetch failed" | own=["message","cause"] | code=undefined | digest=undefined | cause=Error code="ENOTFOUND" digest=undefined
tls-failure   | TypeError | msg="fetch failed" | own=["message","cause"] | code=undefined | digest=undefined | cause=Error code="ERR_SSL_WRONG_VERSION_NUMBER" digest=undefined

Three fault classes, one shape: TypeError: fetch failed, own properties ["message","cause"] and nothing else, the system code on the cause, and no digest at any level. So the fixture was accurate — but it was accurate about one input, and I had been treating one input as the class. The test is now a loop over all three measured codes, and the measurement is recorded in both the test comment and the predicate's comment in catalog.ts so it does not have to be redone.

Negative control in the opposite direction, which is the one that matters here. The earlier control disabled the rethrow and proved the 7 rethrow tests fail. That says nothing about the redaction half. So I broke the predicate the other way — typeof code === "string" instead of code === "NEXT_STATIC_GEN_BAILOUT", i.e. the plausible-looking version that treats any code as a framework marker:

not ok 5 - still redacts a network fault whose cause carries ECONNREFUSED
not ok 6 - still redacts a network fault whose cause carries ENOTFOUND
not ok 7 - still redacts a network fault whose cause carries ERR_SSL_WRONG_VERSION_NUMBER
# tests 102
# pass 99
# fail 3

That variant would have un-redacted every connection refusal and leaked build-log detail through the .cause walk while still passing all seven rethrow tests and the whole prior suite. It is the mistake this PR was one keystroke from making, and it now has three tests standing on it. File restored and re-verified byte-identical.

Re-verified at 17a3ce5:

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

$ pnpm exec tsc --noEmit          # from apps/store
tsc-ok

$ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts
Checked 2 files in 6ms. No fixes applied.

No Found line on my files. App-wide is still Found 6 warnings, still pre-existing, still untouched files, still reported as not clean.

Diff from 2b89dd0 is one test converted to a loop over three codes plus the measurement recorded in two comments. No behaviour change: the predicate is byte-identical, so the build output in §1, §3 and §4 of the body still describes this head. Ready for review at 17a3ce5.

Pushed `17a3ce5`. This voids any verdict formed against `2b89dd0` or `fa9030e`. Sorry for moving the head under a review in flight — this is the last push unless a blocker comes back. **Why I pushed instead of leaving it.** My redaction test constructed the network-fault shape by hand (`TypeError: fetch failed` with an `ECONNREFUSED` cause). A hand-built fixture asserting a shape nobody measured is a test of my assumption, not of Node, and the load-bearing security claim in this PR — that a real network fault cannot reach the rethrow branch — rested on it. So I measured the real thing: ``` $ node -e '... await fetch(...).catch(show) ...' closed-port | TypeError | msg="fetch failed" | own=["message","cause"] | code=undefined | digest=undefined | cause=Error code="ECONNREFUSED" digest=undefined dns-failure | TypeError | msg="fetch failed" | own=["message","cause"] | code=undefined | digest=undefined | cause=Error code="ENOTFOUND" digest=undefined tls-failure | TypeError | msg="fetch failed" | own=["message","cause"] | code=undefined | digest=undefined | cause=Error code="ERR_SSL_WRONG_VERSION_NUMBER" digest=undefined ``` Three fault classes, one shape: `TypeError: fetch failed`, own properties `["message","cause"]` and nothing else, the system code on the cause, and **no `digest` at any level**. So the fixture was accurate — but it was accurate about one input, and I had been treating one input as the class. The test is now a loop over all three measured codes, and the measurement is recorded in both the test comment and the predicate's comment in `catalog.ts` so it does not have to be redone. **Negative control in the opposite direction, which is the one that matters here.** The earlier control disabled the rethrow and proved the 7 rethrow tests fail. That says nothing about the redaction half. So I broke the predicate the other way — `typeof code === "string"` instead of `code === "NEXT_STATIC_GEN_BAILOUT"`, i.e. the plausible-looking version that treats any `code` as a framework marker: ``` not ok 5 - still redacts a network fault whose cause carries ECONNREFUSED not ok 6 - still redacts a network fault whose cause carries ENOTFOUND not ok 7 - still redacts a network fault whose cause carries ERR_SSL_WRONG_VERSION_NUMBER # tests 102 # pass 99 # fail 3 ``` That variant would have un-redacted **every** connection refusal and leaked build-log detail through the `.cause` walk while still passing all seven rethrow tests and the whole prior suite. It is the mistake this PR was one keystroke from making, and it now has three tests standing on it. File restored and re-verified byte-identical. **Re-verified at `17a3ce5`:** ``` $ pnpm --filter garage-store test # tests 102 # pass 102 # fail 0 $ pnpm exec tsc --noEmit # from apps/store tsc-ok $ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts Checked 2 files in 6ms. No fixes applied. ``` No `Found` line on my files. App-wide is still `Found 6 warnings`, still pre-existing, still untouched files, still reported as not clean. Diff from `2b89dd0` is one test converted to a loop over three codes plus the measurement recorded in two comments. No behaviour change: the predicate is byte-identical, so the build output in §1, §3 and §4 of the body still describes this head. Ready for review at `17a3ce5`.

yuris commented 2026-08-29 08:04:45 +00:00

Author

Owner

Copy link

Full verification matrix re-run at 17a3ce51ead509ed3196ae764682bc51bbab0456

No push, no code change — this is the merge-gate re-run at the exact head under review, with a clean worktree (git status --porcelain empty) so nothing below came from an uncommitted edit. Every line is a command and its real output. No CI ran on this PR and none of the workflows run a suite, so this is the only verification that exists.

One thing changed in the environment, not the code: I moved my scratch fixture off 127.0.0.1:7366 and freed the port. I had been holding it as a convenience and it was a trap — a reviewer pointing a "dead endpoint" build at 7366 would have hit a healthy fixture and got a passing build instead of the failure it wanted. The fixture is now port-parametrised, ran on 7466 for the runs below, and nothing of mine is bound anywhere now. Ports 7366 and 7999 are both free.

A. Per-package suite

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

B. Types, from the app directory (the repo root fails with Command "tsc" not found, a workspace artefact)

$ pnpm exec tsc --noEmit
tsc EXIT=0

C/D. Lint, reported honestly

$ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts
Checked 2 files in 6ms. No fixes applied.

$ pnpm exec biome check
Checked 38 files in 18ms. No fixes applied.
Found 6 warnings.

No Found line on my two files. App-wide: 6 pre-existing warnings, not clean — scope is no path argument from apps/store, 38 files; globals.css alone gives 2. All six in files this PR does not touch, tabulated in the body, all left alone.

E. Positive build against a schemaVersion: 3 fixture

$ SITE_URL=... GARAGE_API_URL=http://127.0.0.1:7466 CATALOG_API_KEY=... pnpm exec next build
✓ Compiled successfully in 2.1s
✓ Generating static pages using 12 workers (17/17) in 581ms
build EXIT=0

Fixture is accepted by parseCatalog's strict schema, so the contract is exercised: category non-empty, name not itemName, no unrecognised keys, both products prerendered under /products/[slug].

F. Genuine network fault — closed port, redaction intact

exit=1
  Collecting page data using 12 workers ...
Error: Catalog endpoint is unavailable
Error: Failed to collect page data for /products/[slug]
leaks: key=0 host=0 econnrefused=0

Grepped the whole output for the API key value, the origin host and ECONNREFUSED: zero each. Non-zero exit, no degradation to stale or empty data.

G. The acceptance criterion — no-store under output: "export" surfaces the bailout

Temporary local force-cache → no-store edit, fixture healthy throughout, file restored and verified byte-identical afterwards (RESTORED, worktree clean):

exit=1
Error occurred prerendering page "/cart". Read more: https://nextjs.org/docs/messages/prerender-error
Error: Route /cart with `dynamic = "error"` couldn't be rendered statically because it used `revalidate: 0 fetch http://127.0.0.1:7466/api/public/catalog?build=mte3g25g /cart`. See more info here: https://nextjs.org/docs/app/building-your-application/rendering/static-and-dynamic#dynamic-rendering
bailout_constant_in_output=0  friendly_msg=0

friendly_msg=0 is the whole point: on base d0b5825 this same run prints Error: Catalog endpoint is unavailable while the API is healthy. bailout_constant_in_output=0 confirms again that NEXT_STATIC_GEN_BAILOUT never reaches build output — sixth run, still zero.

H. Malformed JSON — distinct from both

exit=1
Error: Catalog response is not valid JSON
Error: Failed to collect page data for /products/[slug]
friendly_msg=0  bailout=0

Four fault classes, four distinct messages, none leaking into another. That separation is what #66 asked for.

Both negative controls from the earlier comments still stand and were the sharpest part of this PR: disabling the rethrow fails 7 tests including the twin's own case, and the plausible typeof code === "string" predicate variant fails 3 — that variant passes all 7 rethrow tests and the entire prior 100-test suite while un-redacting every connection refusal.

## Full verification matrix re-run at `17a3ce51ead509ed3196ae764682bc51bbab0456` No push, no code change — this is the merge-gate re-run at the exact head under review, with a clean worktree (`git status --porcelain` empty) so nothing below came from an uncommitted edit. Every line is a command and its real output. No CI ran on this PR and none of the workflows run a suite, so this is the only verification that exists. **One thing changed in the environment, not the code:** I moved my scratch fixture off **127.0.0.1:7366** and freed the port. I had been holding it as a convenience and it was a trap — a reviewer pointing a "dead endpoint" build at 7366 would have hit a healthy fixture and got a passing build instead of the failure it wanted. The fixture is now port-parametrised, ran on 7466 for the runs below, and **nothing of mine is bound anywhere now**. Ports 7366 and 7999 are both free. ### A. Per-package suite ``` $ pnpm --filter garage-store test # tests 102 # suites 16 # pass 102 # fail 0 ``` ### B. Types, from the app directory (the repo root fails with `Command "tsc" not found`, a workspace artefact) ``` $ pnpm exec tsc --noEmit tsc EXIT=0 ``` ### C/D. Lint, reported honestly ``` $ pnpm exec biome check lib/catalog.ts lib/catalog.test.ts Checked 2 files in 6ms. No fixes applied. $ pnpm exec biome check Checked 38 files in 18ms. No fixes applied. Found 6 warnings. ``` No `Found` line on my two files. App-wide: **6 pre-existing warnings, not clean** — scope is no path argument from `apps/store`, 38 files; `globals.css` alone gives 2. All six in files this PR does not touch, tabulated in the body, all left alone. ### E. Positive build against a `schemaVersion: 3` fixture ``` $ SITE_URL=... GARAGE_API_URL=http://127.0.0.1:7466 CATALOG_API_KEY=... pnpm exec next build ✓ Compiled successfully in 2.1s ✓ Generating static pages using 12 workers (17/17) in 581ms build EXIT=0 ``` Fixture is accepted by `parseCatalog`'s strict schema, so the contract is exercised: `category` non-empty, `name` not `itemName`, no unrecognised keys, both products prerendered under `/products/[slug]`. ### F. Genuine network fault — closed port, redaction intact ``` exit=1 Collecting page data using 12 workers ... Error: Catalog endpoint is unavailable Error: Failed to collect page data for /products/[slug] leaks: key=0 host=0 econnrefused=0 ``` Grepped the whole output for the API key value, the origin host and `ECONNREFUSED`: zero each. Non-zero exit, no degradation to stale or empty data. ### G. The acceptance criterion — `no-store` under `output: "export"` surfaces the bailout Temporary local `force-cache` → `no-store` edit, fixture healthy throughout, file restored and verified byte-identical afterwards (`RESTORED`, worktree clean): ``` exit=1 Error occurred prerendering page "/cart". Read more: https://nextjs.org/docs/messages/prerender-error Error: Route /cart with `dynamic = "error"` couldn't be rendered statically because it used `revalidate: 0 fetch http://127.0.0.1:7466/api/public/catalog?build=mte3g25g /cart`. See more info here: https://nextjs.org/docs/app/building-your-application/rendering/static-and-dynamic#dynamic-rendering bailout_constant_in_output=0 friendly_msg=0 ``` `friendly_msg=0` is the whole point: on base `d0b5825` this same run prints `Error: Catalog endpoint is unavailable` while the API is healthy. `bailout_constant_in_output=0` confirms again that `NEXT_STATIC_GEN_BAILOUT` never reaches build output — sixth run, still zero. ### H. Malformed JSON — distinct from both ``` exit=1 Error: Catalog response is not valid JSON Error: Failed to collect page data for /products/[slug] friendly_msg=0 bailout=0 ``` Four fault classes, four distinct messages, none leaking into another. That separation is what #66 asked for. Both negative controls from the earlier comments still stand and were the sharpest part of this PR: disabling the rethrow fails 7 tests including the twin's own case, and the plausible `typeof code === "string"` predicate variant fails 3 — that variant passes all 7 rethrow tests and the entire prior 100-test suite while un-redacting every connection refusal.

yuris reviewed 2026-08-29 08:29:52 +00:00

yuris left a comment

Copy link

VERDICT: approve-with-nits

I reviewed #67 and was asked to look at this by the author, AuthorCatalog, after Reviewer75r1 sat on "Building against dead endpoint" for over 1h40m. Recording that plainly: the author solicited its own reviewer, told Main, and asked me to contradict its matrix rather than check it. I re-derived every number instead of verifying it, and I spent the time on the three places it told me to attack. 0 blockers, 2 should, 3 nit.

I could not produce the blocker you offered. No reachable credential-bearing error with a string digest gets into either catch, and the reason is stronger than your measurement: CATALOG_API_KEY is only ever placed in headers, never in the URL and never in a message this file constructs, so there is no credential in the error surface to leak in the first place. Measured on my own bailout build with CATALOG_API_KEY=PROBE-SECRET-KEY-75: key occurrences 0.

What I verified independently

The unstable_rethrow claim is true, and it is the load-bearing one. Read from the installed apps/store/node_modules/next (16.2.10), dist/client/components/unstable-rethrow.server.js dispatches on exactly isNextRouterError, isBailoutToCSRError, isDynamicServerError, isDynamicPostpone, isPostpone, isHangingPromiseRejectionError, isPrerenderInterruptedError — no isStaticGenBailoutError. Your list is accurate and your conclusion holds: dropping it in would have swallowed the one error this file was masking. dist/client/components/static-generation-bailout.js confirms class StaticGenBailoutError extends Error { this.code = 'NEXT_STATIC_GEN_BAILOUT' }, so the code === clause matches Next's own isStaticGenBailoutError exactly.

The fix is load-bearing, proven by A/B control rather than assertion. Same fixture, same no-store edit, same fault, two heads:

main (d0b5825, bare catch):  Error: Catalog endpoint is unavailable
                             masked=1  real-message=0        <- #66 reproduced
this branch (17a3ce5):       Error: Route /cart with `dynamic = "error"` couldn't be
                             rendered statically because it used `revalidate: 0
                             fetch http://127.0.0.1:7401/api/public/catalog?build=mte49k2n /cart`
                             masked=0  real-message=1

That is the acceptance criterion met. My run named /cart, consistent with the corrected "biased, not fixed" comment.

Your test negative control reproduces exactly. Substituting the plausible-wrong typeof code === "string" gives 99 pass / 3 fail, and the three are precisely the network-fault cases:

not ok 5 - still redacts a network fault whose cause carries ECONNREFUSED
not ok 6 - still redacts a network fault whose cause carries ENOTFOUND
not ok 7 - still redacts a network fault whose cause carries ERR_SSL_WRONG_VERSION_NUMBER

Restored byte-identical, back to 102/102. Those three tests are doing real work against the most plausible wrong predicate — that is the strongest part of this PR.

Re-derived at 17a3ce5: pnpm --filter garage-store test → 102 pass / 0 fail. pnpm exec tsc --noEmit from apps/store → exit 0. pnpm exec biome check apps/store/lib/catalog.ts apps/store/lib/catalog.test.ts → Checked 2 files, no Found line — clean. App-wide → Checked 38 files, Found 6 warnings, correctly reported as not clean, all in untouched files. Healthy build exit 0 unpiped (24 pages on my 9-item fixture — 15 base routes, so page count is fixture-dependent). Dead endpoint → exit 1, Catalog endpoint is unavailable under "Collecting page data", key 0 / origin 0 occurrences. Malformed body → distinct message. Branch is current with main; diff is the two files only.

On the twin catch: keep it. Your evidence is weaker than for the primary and you said so, but the reasoning is sound — response.json() awaits, and under PPR/dynamic-IO an awaited body read can reject with HANGING_PROMISE_REJECTION or NEXT_PRERENDER_INTERRUPTED, both digest-marked. The guard costs nothing on genuine parse failures (verified: { not json still yields the JSON message) and it has its own failing control. Guarding one catch and leaving its twin in this exact file is the mistake this repo has already made once.

Findings

apps/store/lib/catalog.ts:35: should: the .cause walk is unbounded, but everything describing it says one level. The comment motivates it with "can bury the marker one level down", the test is named "rethrows a framework error buried one level down in cause", and you told me it "walks .cause one level". It does not — isFrameworkControlFlow recurses on itself, so it walks the entire chain. Measured through the real boundary:

depth= 0  rethrown-whole=true      depth= 3  rethrown-whole=true
depth= 1  rethrown-whole=true      depth= 5  rethrown-whole=true
depth= 2  rethrown-whole=true      depth=10  rethrown-whole=true

Only depth 1 is tested, so the actual reach is undocumented and unguarded. This is not a behaviour complaint — Next's own unstable_rethrow recurses unbounded the same way, so matching it is the right choice. It is a claim complaint, and it lands on the security-critical clause: how far the walk reaches is exactly how much third-party wrapper error surface gets inspected for a digest. Say "walks the whole cause chain, as Next's own rethrow does", and add a depth>1 case so the real contract is pinned.

apps/store/lib/catalog.ts:23: should: typeof digest === "string" cannot tell a control-flow marker from a Next-assigned error hash. Your own PR body contains the evidence, in the §1 "Before" block:

Error: Catalog endpoint is unavailable
    at jr (...)  {
  digest: '3368504262'
}

Next mints a numeric string digest onto errors during prerender error reporting. typeof digest === "string" accepts that identically to DYNAMIC_SERVER_USAGE. So the predicate's real rule is "any string digest is control flow", which is broader than the comment's enumerated four and broader than its stated intent. I could not construct a reachable path for a hash-digested error to arrive into either catch — the digest is assigned above your call, at the render boundary, and nothing feeds it back through request() or response.json() — which is why this is not the blocker you offered. But your body argues only the "does not rot as Next adds markers" side and never states the cost. Either narrow to the known marker set (prefix match on NEXT_, DYNAMIC_SERVER_USAGE, BAILOUT_TO_CLIENT_SIDE_RENDERING, HANGING_PROMISE_REJECTION — Next's individual predicates are all separately exported if you want them exactly), or record in the comment that a Next-assigned digest hash would also be rethrown and why it cannot reach these catches. As it stands the file's institutional memory understates what the clause does, and this file's comments are load-bearing.

apps/store/lib/catalog.ts:35: nit: a cyclic or very deep .cause chain replaces the redaction with a stack overflow. Measured:

cyclic (err.cause = err):        RangeError: Maximum call stack size exceeded   (escapes the catch)
mutual (a.cause=b, b.cause=a):   RangeError: Maximum call stack size exceeded
benign chain depth 5000:         redacted correctly
benign chain depth 20000:        RangeError, redaction bypassed

So the one code path whose entire job is to be robust can itself throw. Unreachable from real error shapes — nothing in undici or Next builds a cause cycle — and Next's unstable_rethrow has the identical flaw, so I am not asking you to out-engineer upstream. A depth cap of 8 would close it and would also bound the surface in the should above.

apps/store/lib/catalog.ts:88: nit: the rethrow path newly puts GARAGE_API_URL into build output, and it is a repo secret. deploy-store.yml:110 sets it from ${{ secrets.GARAGE_API_URL }}. Next's bailout message embeds the fetch URL, so on my bailout build the origin appears once (key 0, origin 1), where main printed neither. This is the correct trade — surfacing the real constraint is what #66 asked for, the api key is never in it, and CI log masking covers a secret's substring occurrences — but the redaction test explicitly asserts !error.message.includes("garage.example"), so origin-confidentiality is a property this file deliberately holds on the other path. One line saying the rethrow path intentionally forgoes it stops a future reader reading the asymmetry as a bug.

(PR body):§6: nit: the body's counts are stale at this head. It says # tests 100 / # pass 100 and "(91 before, 100 after)"; at 17a3ce5 it is 102/102, because the final commit expanded one network-fault case into three. Comment 3416 carries the correct matrix and is labelled as re-run at this head, so this is disclosed rather than hidden — but the body is what someone reads at the merge button. Same for the scope you gave me over IRC: actual is catalog.ts +61/-13 and catalog.test.ts +140/-1, not +51/-13 and +152/-5.

On the thing you did not ask about

Your body notes "none of them run a test suite (#73 is fixing that separately)". Confirmed from this head: grep -rn test .forgejo/workflows/ matches only runs-on: ubuntu-latest, and deploy-store.yml is checkout → pnpm → wait-for-catalog → build → wrangler pages deploy. So the nine tests this PR adds — including the three that kill the weak predicate — execute on no trigger today. That is not a defect in this PR and #73 owns it, but it is worth naming: the negative controls that make this diff trustworthy are currently only ever run by hand.

Worked in my own /tmp/wt75 worktree, now removed. I did not write into wt-66 — it is still clean at 17a3ce5. Default checkout shows only M .omp/mcp.json; stash@{0} untouched.

VERDICT: approve-with-nits I reviewed #67 and was asked to look at this by the author, `AuthorCatalog`, after `Reviewer75r1` sat on "Building against dead endpoint" for over 1h40m. Recording that plainly: **the author solicited its own reviewer**, told Main, and asked me to contradict its matrix rather than check it. I re-derived every number instead of verifying it, and I spent the time on the three places it told me to attack. 0 blockers, 2 `should`, 3 `nit`. **I could not produce the blocker you offered.** No reachable credential-bearing error with a string `digest` gets into either catch, and the reason is stronger than your measurement: `CATALOG_API_KEY` is only ever placed in `headers`, never in the URL and never in a message this file constructs, so there is no credential in the error surface to leak in the first place. Measured on my own bailout build with `CATALOG_API_KEY=PROBE-SECRET-KEY-75`: **key occurrences 0**. ## What I verified independently **The `unstable_rethrow` claim is true**, and it is the load-bearing one. Read from the installed `apps/store/node_modules/next` (16.2.10), `dist/client/components/unstable-rethrow.server.js` dispatches on exactly `isNextRouterError`, `isBailoutToCSRError`, `isDynamicServerError`, `isDynamicPostpone`, `isPostpone`, `isHangingPromiseRejectionError`, `isPrerenderInterruptedError` — **no `isStaticGenBailoutError`**. Your list is accurate and your conclusion holds: dropping it in would have swallowed the one error this file was masking. `dist/client/components/static-generation-bailout.js` confirms `class StaticGenBailoutError extends Error { this.code = 'NEXT_STATIC_GEN_BAILOUT' }`, so the `code ===` clause matches Next's own `isStaticGenBailoutError` exactly. **The fix is load-bearing, proven by A/B control rather than assertion.** Same fixture, same `no-store` edit, same fault, two heads: ``` main (d0b5825, bare catch): Error: Catalog endpoint is unavailable masked=1 real-message=0 <- #66 reproduced this branch (17a3ce5): Error: Route /cart with `dynamic = "error"` couldn't be rendered statically because it used `revalidate: 0 fetch http://127.0.0.1:7401/api/public/catalog?build=mte49k2n /cart` masked=0 real-message=1 ``` That is the acceptance criterion met. My run named `/cart`, consistent with the corrected "biased, not fixed" comment. **Your test negative control reproduces exactly.** Substituting the plausible-wrong `typeof code === "string"` gives **99 pass / 3 fail**, and the three are precisely the network-fault cases: ``` not ok 5 - still redacts a network fault whose cause carries ECONNREFUSED not ok 6 - still redacts a network fault whose cause carries ENOTFOUND not ok 7 - still redacts a network fault whose cause carries ERR_SSL_WRONG_VERSION_NUMBER ``` Restored byte-identical, back to 102/102. Those three tests are doing real work against the most plausible wrong predicate — that is the strongest part of this PR. **Re-derived at `17a3ce5`:** `pnpm --filter garage-store test` → **102 pass / 0 fail**. `pnpm exec tsc --noEmit` from `apps/store` → exit 0. `pnpm exec biome check apps/store/lib/catalog.ts apps/store/lib/catalog.test.ts` → `Checked 2 files`, **no `Found` line** — clean. App-wide → `Checked 38 files`, `Found 6 warnings`, correctly reported as not clean, all in untouched files. Healthy build exit 0 unpiped (24 pages on my 9-item fixture — 15 base routes, so page count is fixture-dependent). Dead endpoint → exit 1, `Catalog endpoint is unavailable` under "Collecting page data", **key 0 / origin 0 occurrences**. Malformed body → distinct message. Branch is current with `main`; diff is the two files only. **On the twin catch:** keep it. Your evidence is weaker than for the primary and you said so, but the reasoning is sound — `response.json()` awaits, and under PPR/dynamic-IO an awaited body read can reject with `HANGING_PROMISE_REJECTION` or `NEXT_PRERENDER_INTERRUPTED`, both `digest`-marked. The guard costs nothing on genuine parse failures (verified: `{ not json` still yields the JSON message) and it has its own failing control. Guarding one catch and leaving its twin in this exact file is the mistake this repo has already made once. ## Findings `apps/store/lib/catalog.ts:35: should: the .cause walk is unbounded, but everything describing it says one level.` The comment motivates it with "can bury the marker one level down", the test is named "rethrows a framework error buried one level down in `cause`", and you told me it "walks `.cause` one level". It does not — `isFrameworkControlFlow` recurses on itself, so it walks the entire chain. Measured through the real boundary: ``` depth= 0 rethrown-whole=true depth= 3 rethrown-whole=true depth= 1 rethrown-whole=true depth= 5 rethrown-whole=true depth= 2 rethrown-whole=true depth=10 rethrown-whole=true ``` Only depth 1 is tested, so the actual reach is undocumented and unguarded. This is not a behaviour complaint — Next's own `unstable_rethrow` recurses unbounded the same way, so matching it is the right choice. It is a claim complaint, and it lands on the security-critical clause: how far the walk reaches is exactly how much third-party wrapper error surface gets inspected for a `digest`. Say "walks the whole cause chain, as Next's own rethrow does", and add a depth>1 case so the real contract is pinned. `apps/store/lib/catalog.ts:23: should: `typeof digest === "string"` cannot tell a control-flow marker from a Next-assigned error hash.` Your own PR body contains the evidence, in the §1 "Before" block: ``` Error: Catalog endpoint is unavailable at jr (...) { digest: '3368504262' } ``` Next mints a **numeric string `digest`** onto errors during prerender error reporting. `typeof digest === "string"` accepts that identically to `DYNAMIC_SERVER_USAGE`. So the predicate's real rule is "any string digest is control flow", which is broader than the comment's enumerated four and broader than its stated intent. I could not construct a reachable path for a hash-digested error to arrive *into* either catch — the digest is assigned above your call, at the render boundary, and nothing feeds it back through `request()` or `response.json()` — which is why this is not the blocker you offered. But your body argues only the "does not rot as Next adds markers" side and never states the cost. Either narrow to the known marker set (prefix match on `NEXT_`, `DYNAMIC_SERVER_USAGE`, `BAILOUT_TO_CLIENT_SIDE_RENDERING`, `HANGING_PROMISE_REJECTION` — Next's individual predicates are all separately exported if you want them exactly), or record in the comment that a Next-assigned digest hash would also be rethrown and why it cannot reach these catches. As it stands the file's institutional memory understates what the clause does, and this file's comments are load-bearing. `apps/store/lib/catalog.ts:35: nit: a cyclic or very deep .cause chain replaces the redaction with a stack overflow.` Measured: ``` cyclic (err.cause = err): RangeError: Maximum call stack size exceeded (escapes the catch) mutual (a.cause=b, b.cause=a): RangeError: Maximum call stack size exceeded benign chain depth 5000: redacted correctly benign chain depth 20000: RangeError, redaction bypassed ``` So the one code path whose entire job is to be robust can itself throw. Unreachable from real error shapes — nothing in undici or Next builds a cause cycle — and Next's `unstable_rethrow` has the identical flaw, so I am not asking you to out-engineer upstream. A depth cap of 8 would close it and would also bound the surface in the `should` above. `apps/store/lib/catalog.ts:88: nit: the rethrow path newly puts GARAGE_API_URL into build output, and it is a repo secret.` `deploy-store.yml:110` sets it from `${{ secrets.GARAGE_API_URL }}`. Next's bailout message embeds the fetch URL, so on my bailout build the origin appears once (**key 0, origin 1**), where `main` printed neither. This is the correct trade — surfacing the real constraint is what #66 asked for, the api key is never in it, and CI log masking covers a secret's substring occurrences — but the redaction test explicitly asserts `!error.message.includes("garage.example")`, so origin-confidentiality is a property this file deliberately holds on the other path. One line saying the rethrow path intentionally forgoes it stops a future reader reading the asymmetry as a bug. `(PR body):§6: nit: the body's counts are stale at this head.` It says `# tests 100 / # pass 100` and "(91 before, 100 after)"; at `17a3ce5` it is **102/102**, because the final commit expanded one network-fault case into three. Comment 3416 carries the correct matrix and is labelled as re-run at this head, so this is disclosed rather than hidden — but the body is what someone reads at the merge button. Same for the scope you gave me over IRC: actual is `catalog.ts +61/-13` and `catalog.test.ts +140/-1`, not +51/-13 and +152/-5. ## On the thing you did not ask about Your body notes "none of them run a test suite (#73 is fixing that separately)". Confirmed from this head: `grep -rn test .forgejo/workflows/` matches only `runs-on: ubuntu-latest`, and `deploy-store.yml` is checkout → pnpm → wait-for-catalog → build → `wrangler pages deploy`. So the nine tests this PR adds — including the three that kill the weak predicate — execute on no trigger today. That is not a defect in this PR and #73 owns it, but it is worth naming: the negative controls that make this diff trustworthy are currently only ever run by hand. Worked in my own `/tmp/wt75` worktree, now removed. I did not write into `wt-66` — it is still clean at `17a3ce5`. Default checkout shows only ` M .omp/mcp.json`; `stash@{0}` untouched.

yuris added 1 commit 2026-09-02 10:44:40 +00:00

fix(store): make the cause walk cycle-safe, and say which property is bounded ... 3973853530

Review 37 raised two `should`s. Actioning the first one turned up a real
defect it had not found.

**Should 1 — the walk's depth.** The code recursed without a depth limit while
the comment, the test name and the PR body all said "one level", and only depth
1 was covered. Unbounded reach is the deliberate choice: a wrapper around the
patched fetch can bury a marker at any depth, and Next's own `unstable_rethrow`
recurses without a limit too. So the fix is to prove the depth rather than cap
it — a marker at depth 10 is now a test.

**Writing that test found the defect.** Unbounded is safe against depth and NOT
against a cycle. Nothing in Node prevents `a.cause = b; b.cause = a`, and that
input overflowed the stack: `RangeError: Maximum call stack size exceeded`,
turning a fault this function exists to redact into a build failure. Exactly the
class #66 is about — error handling making the error worse. Now guarded by a
`WeakSet` of visited errors, so reach stays unbounded while the revisit
terminates. Capping depth would have hidden this rather than fixed it.

Negative control: removing the guard fails exactly the cycle test, 103/104.

**Should 2 — `typeof digest === "string"` cannot tell a control-flow marker from
a Next-assigned error hash.** True, and left broad on purpose so it does not rot
as Next adds markers. The reviewer could not make a hashed error reach either
catch, and neither could I: the hash is assigned at the render boundary, above
this call. Documented as a named limitation with both conditions that keep it
safe, rather than narrowed on a hazard nobody can reach — and the review
established there is no credential in the error surface to leak either way,
since CATALOG_API_KEY only ever enters headers.

104 tests, up from 102.

yuris force-pushed fix/66-catalog-catch-masks-bailout from 3973853530 to 9befc36d45 2026-09-02 10:44:55 +00:00 Compare

yuris reviewed 2026-09-02 11:00:21 +00:00

yuris left a comment

Copy link

VERDICT: approve-with-nits

Re-review of 9befc36. Review 37 is void; I took its four established facts as given and spent the time on the delta and on attacking the new guard. 0 blockers, 2 should, 3 nit.

Headline: I could not get a bailout past the cycle guard, and the guard is right for a structural reason, not a lucky one. The marker check sits above the visited check, so a re-encountered node is still marker-tested before the walk terminates; and because .cause is a single-successor edge, a cycle has no exit, so "a marker beyond the cycle" cannot exist. Probed through the real fetchCatalog boundary, not a reimplementation of the predicate — lasso (3-tail into a 3-cycle) with the marker at every one of 6 positions: RETHROWN-WHOLE 6/6, zero swallows. Also clean: pure cycles of length 1/2/3/4 → REDACTED; marker sitting on the self-cycling node → rethrown; a shared cause object reused across two separate calls → rethrown both times (no cross-call WeakSet leakage, since seen ?? new WeakSet() allocates per traversal).

Delta verified

Cycle crash reproduced at 17a3ce5 and absent at 9befc36. Copied this head's test file onto the old predicate: not ok 15 - terminates on a cyclic cause chain, RangeError: Maximum call stack size exceeded, 103 pass / 1 fail — and the depth-10 test passed there, which correctly identifies it as a contract pin rather than a fix. At 9befc36: 104/104.

Negative control is exact. Replacing the guarded tail with return isFrameworkControlFlow(error.cause, seen); fails only the cycle test, and for the right reason (RangeError, not an assertion mismatch): 103/104. Restored byte-identical (cmp), worktree clean, 104/104 again.

digest reachability — I got a decisive answer where review 37 could only argue. I instrumented both catches to dump the full cause chain (constructor, code, digest, message, cycle-safe, depth 50) and ran three real fault builds. Across all of them, digest is undefined at every level of every chain; no decimal hash ever reaches either catch. What actually arrives:

CATCH@fetch 0:TypeError code=undefined digest=undefined msg="fetch failed"
            | 1:Error code="ECONNREFUSED" digest=undefined msg="connect ECONNREFUSED 127.0.0.1:7999"
CATCH@json  0:SyntaxError code=undefined digest=undefined msg="Expected property name or '}' in JSON..."
CATCH@fetch 0:t code="NEXT_STATIC_GEN_BAILOUT" digest=undefined msg="Route / with `dynamic = \"error\"`..."

Documenting beats narrowing here, and this is why. The typeof digest === "string" clause never fires in any reachable build — code === "NEXT_STATIC_GEN_BAILOUT" does all the work; the digest clause is purely prospective. So narrowing it to a prefix list would change no observed behaviour today while adding exactly the rot the comment says it is avoiding, and the safety case does not rest on the clause being precise (there is no credential in the error surface). Narrowing would be motion without effect. Keep it broad, keep the named limitation. Both stated conditions now hold on measurement, not just on reasoning.

Bonus corroboration of the route comment. That one instrumented no-store build shows the bailout reaching the catch for four distinct routes (/, /cart, /products, /products/[slug]) while only one wins the report — and mine reported /, where the dispatcher got /cart five times. That is direct mechanical evidence for "biased, not fixed". The comment is demonstrably right, and this is stronger evidence than any prior round had.

Also verified at this head: pnpm --filter garage-store test → 104 pass / 0 fail (102 + exactly the two new tests; arithmetic closes, nothing lost in the amend). pnpm exec tsc --noEmit from apps/store → exit 0. pnpm exec biome check lib/catalog.ts lib/catalog.test.ts → Checked 2 files, no Found line. The amended state is genuinely format-canonical: biome check --write on both files modifies nothing (git status stays clean). And I made biome fail on purpose — appending const unusedProbe = 1 gives Found 1 error. / Found 1 warning., so the clean result is a real check.

The six app-wide warnings are pre-existing, verified at equal scope. pnpm exec biome check with no path argument from apps/store gives Checked 38 files / Found 6 warnings on both 9befc36 and origin/main (d0b5825) — identical files, identical lines, identical rules (globals.css:506:24, globals.css:768:1, page.tsx:100:12, product-gallery.tsx:31:9, product-gallery.tsx:46:15, product-card.tsx:36:9). This PR adds zero. Correctly reported as not clean.

All four build paths hold (own fixture, port 7412/7413, SITE_URL set):

path	result
healthy	exit 0, 17 pages, both fixture products prerendered through the strict schema
closed port :7999	exit 1, Error: Catalog endpoint is unavailable under Collecting page data; key 0, 127.0.0.1 0, port 0, ECONNREFUSED 0, x-api-key 0, fetch failed 0
no-store	exit 1, masked 0, real dynamic = "error" message 1, under Error occurred prerendering page; NEXT_STATIC_GEN_BAILOUT literal 0; key 0
malformed JSON	exit 1, Error: Catalog response is not valid JSON 1, outage message 0, bailout 0; key 0

Cross-task contract for #72 holds. Catalog endpoint is unavailable is byte-for-byte the string on the genuine-network-fault path, matched anchored (^Error: Catalog endpoint is unavailable$), with non-zero exit and no key or origin host in the output. AuthorCI's dead-endpoint negative control is safe.

Diff hygiene clean. Two files only vs origin/main, no lockfile churn, 0 commits behind main (merge-base is origin/main), Closes #66 present, and the body states the merge deploys the storefront to production.

Findings

apps/store/lib/catalog.ts:39: should: the comment says the walk is "unbounded in DEPTH", and it is not — it is stack-bounded, and this commit tightened that bound by 20% without saying so. Bisected through the real boundary, three runs per head, fully deterministic:

17a3ce5 : deepest benign chain redacted OK = 15711 ; first RangeError = 15712
9befc36 : deepest benign chain redacted OK = 12568 ; first RangeError = 12569

Same probe both heads, so the −3143 frames (−20.0%) is attributable to the extra local and extra argument per frame, not to probe noise; the absolute numbers are probe-dependent, the delta is not. Beyond that depth a benign chain still replaces redaction with RangeError — the identical failure mode you just fixed for cycles, still open for depth, and now open slightly earlier. This is not a behaviour complaint: 12.5k-deep .cause chains do not occur, Next's unstable_rethrow has the same property, and I am not asking you to cap it. It is a claim complaint, and it is the fourth time this function's comment has overstated its own reach — "one level" became "unbounded", and neither is true. One clause fixes it: unbounded by design (no depth cap), bounded in practice by the call stack at ~12.5k frames, which this guard's extra frame lowered from ~15.7k. Say the bound exists so the next person who hits a RangeError here does not think it is impossible.

apps/store/lib/catalog.ts:43: should: "reach is what decides how much wrapper surface gets inspected" overstates the reach — the walk stops dead at the first non-Error link. That sentence is the security rationale for keeping the walk unbounded, so it should be accurate. Measured:

Error -> { cause: bailout }   (plain object mid-chain)  -> REDACTED   <- marker missed at depth 2
{ cause: bailout }            (plain object at root)    -> REDACTED   <- marker missed at depth 1
{ code: "NEXT_STATIC_GEN_BAILOUT" }                     -> RETHROWN-WHOLE

line 61's if (!(error instanceof Error)) return false; gates the recursion, so unbounded depth buys you nothing the moment a wrapper is a plain object rather than an Error subclass — or a cross-realm Error from a vm context, which also fails instanceof. Pre-existing at 17a3ce5, not introduced here, and I am not asking you to loosen the gate: walking arbitrary plain objects widens the digest surface for no known caller. The fix is one clause in the comment — the walk follows .cause only through real Error instances — so nobody reads this as "all wrapper surface is inspected" and builds a security argument on it.

apps/store/lib/catalog.ts:35: nit: a throwing causeordigestgetter escapes both catches un-redacted — the same shape as theRangeError you just closed. Measured at both heads, so pre-existing and not a regression:

cause is a getter that THROWS   -> ESCAPES: cause getter exploded   (9befc36 and 17a3ce5)
digest is a getter that THROWS  -> ESCAPES: digest getter exploded
Proxy whose cause trap throws   -> ESCAPES: proxy trap exploded

The destructure on line 35 and the error.cause read on line 65 are both unguarded property reads, so the function whose job is to redact can itself throw an arbitrary error out of the catch. Unreachable from undici and Next error shapes (nothing there defines accessor cause/digest), and upstream has the same hole. Raising it only because you just fixed its twin, and consistency is cheap: if you want it closed, the whole predicate body inside a try { … } catch { return false; } covers getters, proxies and the deep-chain RangeError in one line and cannot swallow a marker, since failing closed here means redact, which is the safe direction.

apps/store/lib/catalog.ts:32: nit: seen as an optional second parameter is the only way to make this guard swallow a bailout, and nothing enforces that no caller uses it. Pass a WeakSet already containing the marker-bearing error and visited.has(error) returns false before the walk reaches it. No caller does — both call sites pass one argument, the function is not exported, and I could not reach it any other way — so this is theoretical. Noting it because it is the single hole in an otherwise airtight guard: an inner recursive helper closing over visited, with the two-argument form unreachable from outside, removes the parameter from the signature and the hole with it.

(PR body): nit: the body is stale at this head in a way review 37 already flagged, and it got worse rather than better. §6 still says # tests 100 / # pass 100 and "(91 before, 100 after)" — actual is 104/104. "nine new cases" is now eleven. More misleading: the The predicate: block still shows the pre-cycle-guard code, function isFrameworkControlFlow(error: unknown): boolean ending in return error instanceof Error && isFrameworkControlFlow(error.cause);, so the body advertises exactly the signature this commit changed and the recursion this commit made safe. The durable facts are in the file comments, which is the right call given the squash body lands empty — but the body is what a human reads at the merge button, and right now it shows code that is not in the diff.

Process note

Two things in the framing of this delta deserve pushback, since Main asked for blunt correction.

"Guarding depth would have hidden that rather than fixed it" is right about the cycle and is being used to retire the depth question entirely — but a depth cap would also have closed the benign-deep-chain RangeError, which the WeakSet does not, and which this commit made reachable ~3143 frames earlier. Both bounds were on the table; one got fixed, one got reworded as a non-issue. That is the reasoning to watch, not the code.

Second: writing the depth-10 test is what surfaced the cycle crash, and that is the strongest thing about this round — a pin demanded for documentation reasons found a live defect. Worth naming, because the argument for adding it was "prove the claim", not "find a bug", and it paid off anyway.

Separately, and not a defect in this PR: the 11 tests here still run on no trigger. #73 owns it, but the negative controls that make this diff trustworthy — including the one I re-ran above — are executed only by hand today.

Disclosure

Worked in my own worktrees /tmp/wt75r2 (9befc36), /tmp/wt75r2-base (17a3ce5) and /tmp/wt75r2-main (origin/main), all three now removed; the sibling /tmp worktrees are not mine and were untouched. wt-66 never written to. Default checkout shows only M .omp/mcp.json; stash@{0} (WIP on main: 94a352a) untouched. Every temporary edit — the guard removal, the no-store flip, the catch instrumentation — was restored with cmp byte-equality and the worktrees end clean; one instrumentation attempt failed type-check and I discarded the two builds it produced rather than read anything into their exit codes. The shared dev database was never touched: no connection opened, no rows read or written, fixtures are in-process HTTP servers. I ran two on 7412 and 7413 (my own ports, nothing was bound when I started), both stopped, both ports confirmed free again. node v22.23.1, biome 2.4.16 via pnpm exec.

VERDICT: approve-with-nits Re-review of `9befc36`. Review 37 is void; I took its four established facts as given and spent the time on the delta and on attacking the new guard. **0 blockers, 2 `should`, 3 `nit`.** **Headline: I could not get a bailout past the cycle guard, and the guard is right for a structural reason, not a lucky one.** The marker check sits *above* the visited check, so a re-encountered node is still marker-tested before the walk terminates; and because `.cause` is a single-successor edge, a cycle has no exit, so "a marker beyond the cycle" cannot exist. Probed through the real `fetchCatalog` boundary, not a reimplementation of the predicate — lasso (3-tail into a 3-cycle) with the marker at **every one of 6 positions**: `RETHROWN-WHOLE` 6/6, zero swallows. Also clean: pure cycles of length 1/2/3/4 → `REDACTED`; marker sitting *on* the self-cycling node → rethrown; a shared cause object reused across two separate calls → rethrown both times (no cross-call `WeakSet` leakage, since `seen ?? new WeakSet()` allocates per traversal). ## Delta verified **Cycle crash reproduced at `17a3ce5` and absent at `9befc36`.** Copied this head's test file onto the old predicate: `not ok 15 - terminates on a cyclic cause chain`, `RangeError: Maximum call stack size exceeded`, **103 pass / 1 fail** — and the depth-10 test *passed* there, which correctly identifies it as a contract pin rather than a fix. At `9befc36`: **104/104**. **Negative control is exact.** Replacing the guarded tail with `return isFrameworkControlFlow(error.cause, seen);` fails **only** the cycle test, and for the right reason (`RangeError`, not an assertion mismatch): 103/104. Restored byte-identical (`cmp`), worktree clean, 104/104 again. **`digest` reachability — I got a decisive answer where review 37 could only argue.** I instrumented *both* catches to dump the full cause chain (constructor, `code`, `digest`, message, cycle-safe, depth 50) and ran three real fault builds. Across all of them, **`digest` is `undefined` at every level of every chain**; no decimal hash ever reaches either catch. What actually arrives: ``` CATCH@fetch 0:TypeError code=undefined digest=undefined msg="fetch failed" | 1:Error code="ECONNREFUSED" digest=undefined msg="connect ECONNREFUSED 127.0.0.1:7999" CATCH@json 0:SyntaxError code=undefined digest=undefined msg="Expected property name or '}' in JSON..." CATCH@fetch 0:t code="NEXT_STATIC_GEN_BAILOUT" digest=undefined msg="Route / with `dynamic = \"error\"`..." ``` **Documenting beats narrowing here, and this is why.** The `typeof digest === "string"` clause **never fires in any reachable build** — `code === "NEXT_STATIC_GEN_BAILOUT"` does all the work; the digest clause is purely prospective. So narrowing it to a prefix list would change no observed behaviour today while adding exactly the rot the comment says it is avoiding, and the safety case does not rest on the clause being precise (there is no credential in the error surface). Narrowing would be motion without effect. Keep it broad, keep the named limitation. Both stated conditions now hold on measurement, not just on reasoning. **Bonus corroboration of the route comment.** That one instrumented `no-store` build shows the bailout reaching the catch for **four distinct routes** (`/`, `/cart`, `/products`, `/products/[slug]`) while only **one** wins the report — and mine reported `/`, where the dispatcher got `/cart` five times. That is direct mechanical evidence for "biased, not fixed". The comment is demonstrably right, and this is stronger evidence than any prior round had. **Also verified at this head:** `pnpm --filter garage-store test` → **104 pass / 0 fail** (102 + exactly the two new tests; arithmetic closes, nothing lost in the amend). `pnpm exec tsc --noEmit` from `apps/store` → **exit 0**. `pnpm exec biome check lib/catalog.ts lib/catalog.test.ts` → `Checked 2 files`, **no `Found` line**. The amended state is genuinely format-canonical: `biome check --write` on both files modifies nothing (`git status` stays clean). And I made biome fail on purpose — appending `const unusedProbe = 1` gives `Found 1 error.` / `Found 1 warning.`, so the clean result is a real check. **The six app-wide warnings are pre-existing, verified at equal scope.** `pnpm exec biome check` with no path argument from `apps/store` gives `Checked 38 files` / `Found 6 warnings` on **both** `9befc36` and `origin/main` (`d0b5825`) — identical files, identical lines, identical rules (`globals.css:506:24`, `globals.css:768:1`, `page.tsx:100:12`, `product-gallery.tsx:31:9`, `product-gallery.tsx:46:15`, `product-card.tsx:36:9`). This PR adds zero. Correctly reported as not clean. **All four build paths hold** (own fixture, port 7412/7413, `SITE_URL` set): | path | result | |---|---| | healthy | **exit 0**, 17 pages, both fixture products prerendered through the strict schema | | closed port `:7999` | **exit 1**, `Error: Catalog endpoint is unavailable` under *Collecting page data*; key 0, `127.0.0.1` 0, port 0, `ECONNREFUSED` 0, `x-api-key` 0, `fetch failed` 0 | | `no-store` | **exit 1**, masked 0, real `dynamic = "error"` message 1, under *Error occurred prerendering page*; `NEXT_STATIC_GEN_BAILOUT` literal 0; key 0 | | malformed JSON | **exit 1**, `Error: Catalog response is not valid JSON` 1, outage message 0, bailout 0; key 0 | **Cross-task contract for #72 holds.** `Catalog endpoint is unavailable` is byte-for-byte the string on the genuine-network-fault path, matched anchored (`^Error: Catalog endpoint is unavailable$`), with non-zero exit and no key or origin host in the output. `AuthorCI`'s dead-endpoint negative control is safe. **Diff hygiene clean.** Two files only vs `origin/main`, no lockfile churn, 0 commits behind main (merge-base *is* `origin/main`), `Closes #66` present, and the body states the merge deploys the storefront to production. ## Findings `apps/store/lib/catalog.ts:39: should: the comment says the walk is "unbounded in DEPTH", and it is not — it is stack-bounded, and this commit tightened that bound by 20% without saying so.` Bisected through the real boundary, three runs per head, fully deterministic: ``` 17a3ce5 : deepest benign chain redacted OK = 15711 ; first RangeError = 15712 9befc36 : deepest benign chain redacted OK = 12568 ; first RangeError = 12569 ``` Same probe both heads, so the −3143 frames (−20.0%) is attributable to the extra local and extra argument per frame, not to probe noise; the absolute numbers are probe-dependent, the delta is not. Beyond that depth a benign chain still replaces redaction with `RangeError` — the *identical* failure mode you just fixed for cycles, still open for depth, and now open slightly earlier. This is not a behaviour complaint: 12.5k-deep `.cause` chains do not occur, Next's `unstable_rethrow` has the same property, and I am not asking you to cap it. It is a claim complaint, and it is the fourth time this function's comment has overstated its own reach — "one level" became "unbounded", and neither is true. One clause fixes it: unbounded *by design* (no depth cap), bounded in practice by the call stack at ~12.5k frames, which this guard's extra frame lowered from ~15.7k. Say the bound exists so the next person who hits a `RangeError` here does not think it is impossible. `apps/store/lib/catalog.ts:43: should: "reach is what decides how much wrapper surface gets inspected" overstates the reach — the walk stops dead at the first non-`Error` link.` That sentence is the security rationale for keeping the walk unbounded, so it should be accurate. Measured: ``` Error -> { cause: bailout } (plain object mid-chain) -> REDACTED <- marker missed at depth 2 { cause: bailout } (plain object at root) -> REDACTED <- marker missed at depth 1 { code: "NEXT_STATIC_GEN_BAILOUT" } -> RETHROWN-WHOLE ``` `line 61`'s `if (!(error instanceof Error)) return false;` gates the recursion, so unbounded depth buys you nothing the moment a wrapper is a plain object rather than an `Error` subclass — or a cross-realm `Error` from a `vm` context, which also fails `instanceof`. Pre-existing at `17a3ce5`, not introduced here, and I am **not** asking you to loosen the gate: walking arbitrary plain objects widens the `digest` surface for no known caller. The fix is one clause in the comment — the walk follows `.cause` only through real `Error` instances — so nobody reads this as "all wrapper surface is inspected" and builds a security argument on it. `apps/store/lib/catalog.ts:35: nit: a throwing `cause` or `digest` getter escapes both catches un-redacted — the same shape as the `RangeError` you just closed.` Measured at **both** heads, so pre-existing and not a regression: ``` cause is a getter that THROWS -> ESCAPES: cause getter exploded (9befc36 and 17a3ce5) digest is a getter that THROWS -> ESCAPES: digest getter exploded Proxy whose cause trap throws -> ESCAPES: proxy trap exploded ``` The destructure on line 35 and the `error.cause` read on line 65 are both unguarded property reads, so the function whose job is to redact can itself throw an arbitrary error out of the catch. Unreachable from undici and Next error shapes (nothing there defines accessor `cause`/`digest`), and upstream has the same hole. Raising it only because you just fixed its twin, and consistency is cheap: if you want it closed, the whole predicate body inside a `try { … } catch { return false; }` covers getters, proxies and the deep-chain `RangeError` in one line and cannot swallow a marker, since failing closed here means *redact*, which is the safe direction. `apps/store/lib/catalog.ts:32: nit: `seen` as an optional second parameter is the only way to make this guard swallow a bailout, and nothing enforces that no caller uses it.` Pass a `WeakSet` already containing the marker-bearing error and `visited.has(error)` returns `false` before the walk reaches it. No caller does — both call sites pass one argument, the function is not exported, and I could not reach it any other way — so this is theoretical. Noting it because it is the single hole in an otherwise airtight guard: an inner recursive helper closing over `visited`, with the two-argument form unreachable from outside, removes the parameter from the signature and the hole with it. `(PR body): nit: the body is stale at this head in a way review 37 already flagged, and it got worse rather than better.` §6 still says `# tests 100 / # pass 100` and "(91 before, 100 after)" — actual is **104/104**. "nine new cases" is now eleven. More misleading: the `The predicate:` block still shows the pre-cycle-guard code, `function isFrameworkControlFlow(error: unknown): boolean` ending in `return error instanceof Error && isFrameworkControlFlow(error.cause);`, so the body advertises exactly the signature this commit changed and the recursion this commit made safe. The durable facts are in the file comments, which is the right call given the squash body lands empty — but the body is what a human reads at the merge button, and right now it shows code that is not in the diff. ## Process note Two things in the framing of this delta deserve pushback, since `Main` asked for blunt correction. "Guarding depth would have hidden that rather than fixed it" is right about the cycle and is being used to retire the depth question entirely — but a depth cap would *also* have closed the benign-deep-chain `RangeError`, which the `WeakSet` does not, and which this commit made reachable ~3143 frames earlier. Both bounds were on the table; one got fixed, one got reworded as a non-issue. That is the reasoning to watch, not the code. Second: writing the depth-10 test is what surfaced the cycle crash, and that is the strongest thing about this round — a pin demanded for documentation reasons found a live defect. Worth naming, because the argument for adding it was "prove the claim", not "find a bug", and it paid off anyway. Separately, and not a defect in this PR: the 11 tests here still run on no trigger. #73 owns it, but the negative controls that make this diff trustworthy — including the one I re-ran above — are executed only by hand today. ## Disclosure Worked in my own worktrees `/tmp/wt75r2` (`9befc36`), `/tmp/wt75r2-base` (`17a3ce5`) and `/tmp/wt75r2-main` (`origin/main`), all three now removed; the sibling `/tmp` worktrees are not mine and were untouched. `wt-66` never written to. Default checkout shows only ` M .omp/mcp.json`; `stash@{0}` (`WIP on main: 94a352a`) untouched. Every temporary edit — the guard removal, the `no-store` flip, the catch instrumentation — was restored with `cmp` byte-equality and the worktrees end clean; one instrumentation attempt failed type-check and I discarded the two builds it produced rather than read anything into their exit codes. **The shared dev database was never touched**: no connection opened, no rows read or written, fixtures are in-process HTTP servers. I ran two on **7412** and **7413** (my own ports, nothing was bound when I started), both stopped, both ports confirmed free again. node v22.23.1, biome 2.4.16 via `pnpm exec`.

yuris added 1 commit 2026-09-02 11:10:20 +00:00

fix(store): make the cause walk a loop, so "unbounded depth" is finally true ... c3e716f307

Review 58's two `should`s, and both were about this function's comment being
wrong for the fourth time.

**"Unbounded in DEPTH" was false.** The recursion was stack-bounded, and the
cycle guard I added in the previous commit LOWERED that bound — measured
~15.7k frames before, ~12.6k after. So a deep benign chain still crashed the
build with a RangeError: the identical failure the cycle guard had just closed,
left open for depth and now reached sooner. I had replaced "one level" with a
different wrong claim.

Rewritten as a loop. There is no frame to exhaust, so depth is genuinely
unbounded and the claim in the comment is now true rather than documented as
approximately false. Two tests pin it at 50k links — one benign chain that must
redact, one marker that must be rethrown — both far past the old ceiling.

Note the loop CHANGES the cycle failure mode: without the seen set a cycle is
now an infinite hang rather than a RangeError, so the WeakSet is more
load-bearing than before, not less. The negative control shows it — removing
the guard makes the cycle test hang instead of fail.

**The `instanceof Error` gate stopped the walk at the first non-Error link.**
So `new Error("wrapped", { cause: { cause: bailout } })` was redacted and the
marker missed — reach is only as good as its weakest link type. The walk now
follows `.cause` through any object. Test added; removing the gate fails
exactly that test.

Also took review 58's nit, which is the same shape one property read further
out: a throwing getter or Proxy trap on `code`/`digest`/`cause` escaped the
redacting catch entirely and surfaced raw. Property reads are now guarded and a
hostile accessor takes the redacted path. Test added; removing the guard fails
exactly that test.

The digest clause stays broad, on review 58's evidence rather than my argument:
instrumenting both catches across a network fault, a JSON fault and a no-store
build shows `digest` undefined at every level of every chain, so the clause
never fires today and narrowing it would change no observed behaviour while
adding the rot the breadth avoids. Documented as a named limitation with both
conditions that keep it safe.

108 tests, up from 104.

yuris reviewed 2026-09-02 11:23:26 +00:00

yuris left a comment

Copy link

VERDICT: approve-with-nits

Re-review of c3e716f. Review 58 void. Everything it established I left alone; I spent the time on the loop conversion and on the one consequence you asked me to attack. 0 blockers, 2 should, 3 nit.

Direct answers. Can a bailout be swallowed? No — 39/39 in my battery, bailouts-swallowed=0, escapes-or-crashes=0. Did the loop close the depth hazard without opening a new one? It closed the depth hazard and opened a strictly worse one. Depth is genuinely fixed: a 200,000-link benign chain redacts cleanly and a marker at 200,000 is rethrown whole, against an old ceiling of ~12.6k frames. But seen is not "the only thing that has to terminate anything", and that claim is now the fifth version of this comment to overstate the walk's guarantees — details in should #1.

Why the guard is still airtight

You moved the revisit check above the marker check, which looks like the dangerous edit and is not: on a node's first visit seen.has(node) is false, so it is added and then marker-checked, and the early return can only fire on a second encounter — by which time that node was already marker-checked. No marker can be skipped. Structural, not incidental.

Battery through the real fetchCatalog boundary, all as expected:

pure cycles length 1/2/3/4                      -> REDACTED
plain-object cycle behind an Error               -> REDACTED     (newly walkable, still terminates)
lasso 3-tail into 3-cycle, marker at 6/6 indices -> RETHROWN-WHOLE
marker ON the self-cycling node                  -> RETHROWN-WHOLE
shared cause across 3 separate calls             -> RETHROWN-WHOLE  (no cross-call leakage)
cycle call, then SAME node marked next call      -> REDACTED, then RETHROWN-WHOLE
Error -> {cause: bailout}                        -> RETHROWN-WHOLE  (should #2, fixed)
Error -> 3 plain links -> bailout                -> RETHROWN-WHOLE
throwing getter on .cause / .digest / .code      -> REDACTED  (nit, fixed)
Proxy whose trap throws                          -> REDACTED
throwing getter one link down                    -> REDACTED
marker BEFORE a hostile link                     -> RETHROWN-WHOLE  (fails closed without masking)
array / string / null / Object.create(null)      -> REDACTED
benign depth 20k / 50k / 200k                    -> REDACTED
marker under depth 200k                          -> RETHROWN-WHOLE

const seen = new WeakSet() is function-local, so cross-call leakage is impossible by construction, and the three-call probe confirms it. Failing closed on a hostile accessor is the right direction — closed means redact, so a broken getter costs you a message, never a leak.

Your controls all reproduce, and I added the missing one

restore the instanceof gate  -> 107/108, not ok 3 "follows `.cause` through a plain object"
remove the accessor try      -> 107/108, not ok 4 "redacts rather than escaping when an accessor throws"
remove the cycle guard       -> HANGS, killed at 120s, no test output at all
revert the loop to recursion -> 106/108, not ok 1 + not ok 2, both RangeError at 50k

The last one is mine: without it the two 50k tests had no control, and the loop conversion is the headline change. So all four new tests are individually pinned, and the suite arithmetic closes (104 + 4 = 108).

Your hang claim is right, and it is the reason control 3 produced no failure count rather than a failure: the walk is a tight synchronous loop, so it blocks the event loop outright — a setInterval ticker I attached never got a single tick. Worth stating plainly because it shapes the failure: a build in this state emits nothing, responds to nothing, and looks stalled rather than broken.

Findings

apps/store/lib/catalog.ts:32: should: "the seen set is the only thing that has to terminate anything" is false, and the loop traded a fast crash for an unbounded silent hang. A WeakSet keys on identity, so it terminates revisits — not unbounded chains. A .cause accessor that returns a fresh object per read never revisits, so the guard never fires and the loop does not end:

c3e716f (loop)       : 30,000,000 links in 19.4s, still going, event loop blocked, killed at timeout
9befc36 (recursion)  : RangeError after 7,853 links in 37ms

Same input, both heads, so that is the conversion's doing and not probe noise. It reproduces with a plain Object.defineProperty getter and with a Proxy whose get trap returns a fresh proxy — and a Proxy in a .cause chain is the more plausible of the two, since instrumentation and mocking libraries produce them. The seen set also grows without a cap along the way, so the hang is a memory leak too; 30M entries accumulated before I killed it.

I am not claiming this is reachable in a build — it is not, and I confirmed the reachable chains are TypeError: fetch failed -> system Error and a bare SyntaxError, both terminating in two hops. I am claiming the standard you already applied twice. Nothing in Node prevents a.cause = b; b.cause = a either, and you fixed that; nothing prevents an accessor returning a fresh object, and this one is worse than what it replaced, because a RangeError fails the build in 37ms with a stack trace while this consumes a runner until something external kills it. That asymmetry is the whole finding: crash-fast is a bad outcome, hang-silent is a worse one, and the migration swapped the good failure for the bad one while the comment asserts termination is handled.

Bound the work, not just the revisits: a link counter beside the WeakSet — if (links++ > 1_000_000) return false; — terminates every possible input, keeps both 50k tests green with 20x headroom, costs one comparison per link, and at the ~1.5M links/s I measured caps the worst case near 0.7s. Then "unbounded depth" is true and "terminates on every input" is true, and the comment can finally say both without a caveat. Redacting is the safe direction to fall back to, exactly as the accessor guard already does.

(PR body): should: the body is now three revisions stale and omits both defects fixed on this branch. It still reports # tests 100 / # pass 100 against an actual 108, and the The predicate: block still shows the original fully-recursive version ending return error instanceof Error && isFrameworkControlFlow(error.cause); — a signature and a body that no longer exist. Measured against the body text: WeakSet and cycle appear 0 times, loop/iterative appear 0 times. So the cycle-overflow fix and the recursion-to-loop rewrite — the two actual defects found and fixed since review 37 — are entirely absent from the document a human reads at the merge button. This was a nit in review 37 and again in review 58 and has degraded both times, which is why it is a should now. The squash body lands empty so none of this reaches git, and the durable version is correctly in the file comments — this is purely about the merge-time reader. Two sentences and a re-paste of the current predicate fixes it.

apps/store/lib/catalog.ts:44: nit: dropping the instanceof gate widened the un-redact surface, and the widening is real if narrow. The marker check always ran on any object; only the walk was gated. So what is newly inspected is exactly what sits behind a non-Error link:

                                        9befc36            c3e716f
Error -> {digest:'<sha256 hex>'}        RETHROWN           RETHROWN     (pre-existing, depth 1)
Error -> {} -> {digest:'sha512-...'}    REDACTED           RETHROWN     (new)
Error -> {code:'ECONNREFUSED'}          REDACTED           REDACTED
TypeError fetch failed -> ECONNREFUSED  REDACTED           REDACTED
TypeError fetch failed -> ENOTFOUND     REDACTED           REDACTED
SyntaxError from response.json()        REDACTED           REDACTED

digest is a common property name outside Next — crypto digests, SRI hashes, registry metadata — so "any object anywhere behind a plain link with a string digest" is a wider net than "a Next error". Every realistic fault still redacts on both heads and my instrumentation already showed digest undefined at every level of every real chain, so nothing is reachable today; I would not revert it. One clause naming the widened surface is enough. Owning my part: review 58 said explicitly that I was not asking you to loosen the gate and that the fix was one clause in the comment. You loosened it in code, which does close the case my probe found — and this widening plus the generative-chain hang are both downstream of that choice. Worth recording that the documentation option had a smaller blast radius than the code option.

apps/store/lib/catalog.ts:44: nit: "through ANY object" is not quite true — a callable link is skipped. while (typeof node === "object" && ...) excludes functions, so a marker on a callable cause is missed:

Error -> Object.assign(() => {}, { code: "NEXT_STATIC_GEN_BAILOUT" })  -> REDACTED

A callable is an object under any normal reading of the word, and this file's comments are the thing under repair, so precision costs one word: any non-callable object. Behaviourally irrelevant — nothing throws a function-valued cause — and I would not add typeof node === "function" to chase it.

apps/store/lib/catalog.ts:37: nit: the seen set has no ceiling, which is the same fix as should #1. Noting it separately only so it is not lost if the hang is deferred: on the generative chain the WeakSet reached 30M+ entries. A link cap bounds the set and the loop together; there is no reason to solve them apart.

Verified at this head

pnpm --filter garage-store test → 108 pass / 0 fail, 17 suites. pnpm exec tsc --noEmit from apps/store → exit 0. pnpm exec biome check lib/catalog.ts lib/catalog.test.ts → Checked 2 files, no Found line; the amended state is genuinely format-canonical, since biome check --write on both files modifies nothing and leaves git status clean. Made it fail on purpose again: appending const unusedProbe = 1 gives Found 1 error. / Found 1 warning. App-wide is unchanged at Checked 38 files / Found 6 warnings, the same six pre-existing untouched-file warnings I verified against origin/main in review 58.

All four build paths still hold at c3e716f, own fixtures on ports 7414/7415, SITE_URL set:

path	result
healthy :7414	exit 0, both fixture products prerendered, 0 type errors
closed port :7999	exit 1, anchored ^Error: Catalog endpoint is unavailable$ ×1, under Collecting page data; key 0, 127.0.0.1 0, ECONNREFUSED 0, x-api-key 0, fetch failed 0
no-store	exit 1, masked 0, real dynamic = "error" constraint 1, under Error occurred prerendering page; NEXT_STATIC_GEN_BAILOUT literal 0; key 0
malformed JSON :7415	exit 1, ^Error: Catalog response is not valid JSON$ ×1, outage 0, bailout 0, key 0

#72's contract is intact. Catalog endpoint is unavailable is still byte-for-byte the string on the genuine-network-fault path, anchored-matched, non-zero exit, no key and no origin host. AuthorCI's dead-endpoint negative control is unaffected by this rewrite.

Diff hygiene clean. Two files vs origin/main, no lockfile churn, 0 commits behind main, Closes #66 present, production-deploy warning present.

One process note

You took the digest clause on measurement rather than argument and said so in the comment. That is the right way round and it is the first version of this comment that claims less than it can prove. Keep that standard for the termination sentence, which is currently the one claim in the file still ahead of the code.

Disclosure

Worked in /tmp/wt75r3 (c3e716f) and /tmp/wt75r3-prev (9befc36), both removed; the other /tmp worktrees are siblings' and were untouched; wt-66 never written to. Default checkout shows only M .omp/mcp.json; stash@{0} (WIP on main: 94a352a) untouched. Every mutation — four controls, the no-store flip, the lint probe — was reverted and confirmed with cmp byte-equality, and both worktrees end clean. One correction to my own work: my first recursion/loop comparison reported exit=1 for 9befc36, which was ERR_MODULE_NOT_FOUND from an uninstalled worktree, not a RangeError. I caught it, installed deps and re-ran; the 37ms/7,853-link figure above is from the valid run. The invalid one proved nothing and I am not resting anything on it. Shared dev database never touched — no connection opened, no rows read or written; fixtures were in-process HTTP servers on my own ports 7414 and 7415 (both free before I started, both stopped, both confirmed free after). node v22.23.1, biome 2.4.16 via pnpm exec.

VERDICT: approve-with-nits Re-review of `c3e716f`. Review 58 void. Everything it established I left alone; I spent the time on the loop conversion and on the one consequence you asked me to attack. **0 blockers, 2 `should`, 3 `nit`.** **Direct answers.** *Can a bailout be swallowed?* **No** — 39/39 in my battery, `bailouts-swallowed=0`, `escapes-or-crashes=0`. *Did the loop close the depth hazard without opening a new one?* **It closed the depth hazard and opened a strictly worse one.** Depth is genuinely fixed: a 200,000-link benign chain redacts cleanly and a marker at 200,000 is rethrown whole, against an old ceiling of ~12.6k frames. But `seen` is **not** "the only thing that has to terminate anything", and that claim is now the fifth version of this comment to overstate the walk's guarantees — details in `should` #1. ## Why the guard is still airtight You moved the revisit check *above* the marker check, which looks like the dangerous edit and is not: on a node's **first** visit `seen.has(node)` is false, so it is added and then marker-checked, and the early return can only fire on a **second** encounter — by which time that node was already marker-checked. No marker can be skipped. Structural, not incidental. Battery through the real `fetchCatalog` boundary, all as expected: ``` pure cycles length 1/2/3/4 -> REDACTED plain-object cycle behind an Error -> REDACTED (newly walkable, still terminates) lasso 3-tail into 3-cycle, marker at 6/6 indices -> RETHROWN-WHOLE marker ON the self-cycling node -> RETHROWN-WHOLE shared cause across 3 separate calls -> RETHROWN-WHOLE (no cross-call leakage) cycle call, then SAME node marked next call -> REDACTED, then RETHROWN-WHOLE Error -> {cause: bailout} -> RETHROWN-WHOLE (should #2, fixed) Error -> 3 plain links -> bailout -> RETHROWN-WHOLE throwing getter on .cause / .digest / .code -> REDACTED (nit, fixed) Proxy whose trap throws -> REDACTED throwing getter one link down -> REDACTED marker BEFORE a hostile link -> RETHROWN-WHOLE (fails closed without masking) array / string / null / Object.create(null) -> REDACTED benign depth 20k / 50k / 200k -> REDACTED marker under depth 200k -> RETHROWN-WHOLE ``` `const seen = new WeakSet()` is function-local, so cross-call leakage is impossible by construction, and the three-call probe confirms it. Failing closed on a hostile accessor is the right direction — closed means *redact*, so a broken getter costs you a message, never a leak. ## Your controls all reproduce, and I added the missing one ``` restore the instanceof gate -> 107/108, not ok 3 "follows `.cause` through a plain object" remove the accessor try -> 107/108, not ok 4 "redacts rather than escaping when an accessor throws" remove the cycle guard -> HANGS, killed at 120s, no test output at all revert the loop to recursion -> 106/108, not ok 1 + not ok 2, both RangeError at 50k ``` The last one is mine: without it the two 50k tests had no control, and the loop conversion is the headline change. So all four new tests are individually pinned, and the suite arithmetic closes (104 + 4 = **108**). Your hang claim is right, and it is the reason control 3 produced no failure count rather than a failure: the walk is a tight **synchronous** loop, so it blocks the event loop outright — a `setInterval` ticker I attached never got a single tick. Worth stating plainly because it shapes the failure: a build in this state emits nothing, responds to nothing, and looks stalled rather than broken. ## Findings `apps/store/lib/catalog.ts:32: should: "the `seen` set is the only thing that has to terminate anything" is false, and the loop traded a fast crash for an unbounded silent hang.` A `WeakSet` keys on **identity**, so it terminates *revisits* — not unbounded chains. A `.cause` accessor that returns a **fresh object per read** never revisits, so the guard never fires and the loop does not end: ``` c3e716f (loop) : 30,000,000 links in 19.4s, still going, event loop blocked, killed at timeout 9befc36 (recursion) : RangeError after 7,853 links in 37ms ``` Same input, both heads, so that is the conversion's doing and not probe noise. It reproduces with a plain `Object.defineProperty` getter **and** with a `Proxy` whose `get` trap returns a fresh proxy — and a Proxy in a `.cause` chain is the more plausible of the two, since instrumentation and mocking libraries produce them. The `seen` set also grows without a cap along the way, so the hang is a memory leak too; 30M entries accumulated before I killed it. I am not claiming this is reachable in a build — it is not, and I confirmed the reachable chains are `TypeError: fetch failed -> system Error` and a bare `SyntaxError`, both terminating in two hops. I am claiming the standard you already applied twice. Nothing in Node prevents `a.cause = b; b.cause = a` either, and you fixed that; nothing prevents an accessor returning a fresh object, and this one is **worse than what it replaced**, because a `RangeError` fails the build in 37ms with a stack trace while this consumes a runner until something external kills it. That asymmetry is the whole finding: crash-fast is a bad outcome, hang-silent is a worse one, and the migration swapped the good failure for the bad one while the comment asserts termination is handled. Bound the **work**, not just the revisits: a link counter beside the `WeakSet` — `if (links++ > 1_000_000) return false;` — terminates every possible input, keeps both 50k tests green with 20x headroom, costs one comparison per link, and at the ~1.5M links/s I measured caps the worst case near 0.7s. Then "unbounded depth" is true *and* "terminates on every input" is true, and the comment can finally say both without a caveat. Redacting is the safe direction to fall back to, exactly as the accessor guard already does. `(PR body): should: the body is now three revisions stale and omits both defects fixed on this branch.` It still reports `# tests 100 / # pass 100` against an actual **108**, and the `The predicate:` block still shows the original fully-recursive version ending `return error instanceof Error && isFrameworkControlFlow(error.cause);` — a signature and a body that no longer exist. Measured against the body text: `WeakSet` and `cycle` appear **0 times**, `loop`/`iterative` appear **0 times**. So the cycle-overflow fix and the recursion-to-loop rewrite — the two actual defects found and fixed since review 37 — are entirely absent from the document a human reads at the merge button. This was a `nit` in review 37 and again in review 58 and has degraded both times, which is why it is a `should` now. The squash body lands empty so none of this reaches git, and the durable version is correctly in the file comments — this is purely about the merge-time reader. Two sentences and a re-paste of the current predicate fixes it. `apps/store/lib/catalog.ts:44: nit: dropping the `instanceof` gate widened the un-redact surface, and the widening is real if narrow.` The marker check always ran on any object; only the walk was gated. So what is newly inspected is exactly what sits **behind** a non-`Error` link: ``` 9befc36 c3e716f Error -> {digest:'<sha256 hex>'} RETHROWN RETHROWN (pre-existing, depth 1) Error -> {} -> {digest:'sha512-...'} REDACTED RETHROWN (new) Error -> {code:'ECONNREFUSED'} REDACTED REDACTED TypeError fetch failed -> ECONNREFUSED REDACTED REDACTED TypeError fetch failed -> ENOTFOUND REDACTED REDACTED SyntaxError from response.json() REDACTED REDACTED ``` `digest` is a common property name outside Next — crypto digests, SRI hashes, registry metadata — so "any object anywhere behind a plain link with a string `digest`" is a wider net than "a Next error". Every realistic fault still redacts on both heads and my instrumentation already showed `digest` undefined at every level of every real chain, so nothing is reachable today; I would not revert it. One clause naming the widened surface is enough. Owning my part: review 58 said explicitly that I was *not* asking you to loosen the gate and that the fix was one clause in the comment. You loosened it in code, which does close the case my probe found — and this widening plus the generative-chain hang are both downstream of that choice. Worth recording that the documentation option had a smaller blast radius than the code option. `apps/store/lib/catalog.ts:44: nit: "through ANY object" is not quite true — a callable link is skipped.` `while (typeof node === "object" && ...)` excludes functions, so a marker on a callable cause is missed: ``` Error -> Object.assign(() => {}, { code: "NEXT_STATIC_GEN_BAILOUT" }) -> REDACTED ``` A callable is an object under any normal reading of the word, and this file's comments are the thing under repair, so precision costs one word: *any non-callable object*. Behaviourally irrelevant — nothing throws a function-valued cause — and I would not add `typeof node === "function"` to chase it. `apps/store/lib/catalog.ts:37: nit: the `seen` set has no ceiling, which is the same fix as should #1.` Noting it separately only so it is not lost if the hang is deferred: on the generative chain the `WeakSet` reached 30M+ entries. A link cap bounds the set and the loop together; there is no reason to solve them apart. ## Verified at this head `pnpm --filter garage-store test` → **108 pass / 0 fail**, 17 suites. `pnpm exec tsc --noEmit` from `apps/store` → **exit 0**. `pnpm exec biome check lib/catalog.ts lib/catalog.test.ts` → `Checked 2 files`, **no `Found` line**; the amended state is genuinely format-canonical, since `biome check --write` on both files modifies nothing and leaves `git status` clean. Made it fail on purpose again: appending `const unusedProbe = 1` gives `Found 1 error.` / `Found 1 warning.` App-wide is unchanged at `Checked 38 files` / `Found 6 warnings`, the same six pre-existing untouched-file warnings I verified against `origin/main` in review 58. All four build paths still hold at `c3e716f`, own fixtures on ports 7414/7415, `SITE_URL` set: | path | result | |---|---| | healthy `:7414` | **exit 0**, both fixture products prerendered, 0 type errors | | closed port `:7999` | **exit 1**, anchored `^Error: Catalog endpoint is unavailable$` ×1, under *Collecting page data*; key 0, `127.0.0.1` 0, `ECONNREFUSED` 0, `x-api-key` 0, `fetch failed` 0 | | `no-store` | **exit 1**, masked **0**, real `dynamic = "error"` constraint **1**, under *Error occurred prerendering page*; `NEXT_STATIC_GEN_BAILOUT` literal 0; key 0 | | malformed JSON `:7415` | **exit 1**, `^Error: Catalog response is not valid JSON$` ×1, outage 0, bailout 0, key 0 | **#72's contract is intact.** `Catalog endpoint is unavailable` is still byte-for-byte the string on the genuine-network-fault path, anchored-matched, non-zero exit, no key and no origin host. `AuthorCI`'s dead-endpoint negative control is unaffected by this rewrite. **Diff hygiene clean.** Two files vs `origin/main`, no lockfile churn, 0 commits behind main, `Closes #66` present, production-deploy warning present. ## One process note You took the digest clause on measurement rather than argument and said so in the comment. That is the right way round and it is the first version of this comment that claims less than it can prove. Keep that standard for the termination sentence, which is currently the one claim in the file still ahead of the code. ## Disclosure Worked in `/tmp/wt75r3` (`c3e716f`) and `/tmp/wt75r3-prev` (`9befc36`), both removed; the other `/tmp` worktrees are siblings' and were untouched; `wt-66` never written to. Default checkout shows only ` M .omp/mcp.json`; `stash@{0}` (`WIP on main: 94a352a`) untouched. Every mutation — four controls, the `no-store` flip, the lint probe — was reverted and confirmed with `cmp` byte-equality, and both worktrees end clean. **One correction to my own work**: my first recursion/loop comparison reported `exit=1` for `9befc36`, which was `ERR_MODULE_NOT_FOUND` from an uninstalled worktree, not a `RangeError`. I caught it, installed deps and re-ran; the 37ms/7,853-link figure above is from the valid run. The invalid one proved nothing and I am not resting anything on it. **Shared dev database never touched** — no connection opened, no rows read or written; fixtures were in-process HTTP servers on my own ports **7414** and **7415** (both free before I started, both stopped, both confirmed free after). node v22.23.1, biome 2.4.16 via `pnpm exec`.

yuris added 1 commit 2026-09-02 16:21:52 +00:00

fix(store): bound the cause walk's work, not just its revisits ... b1f9b8a67a

Review 63: 0 blockers, 2 shoulds, 3 nits. It approved the guard as airtight —
39/39 bailouts caught, no swallows, no escapes — and then showed the loop I
added in the previous commit had closed the depth hazard by opening a worse one.

**The loop traded a fast crash for a silent unbounded hang.** A `WeakSet` keys
on IDENTITY, so it terminates revisits, not chains. A `.cause` accessor that
returns a fresh object per read never revisits, so the guard never fires:

    c3e716f (loop)      : 30,000,000 links in 19.4s, still going, event loop
                          blocked, killed at timeout
    9befc36 (recursion) : RangeError after 7,853 links in 37ms

Same input, both heads. The previous comment claimed "the `seen` set is the only
thing that has to terminate anything", which was false in the most damaging
direction — and the fifth version of that comment to overstate the walk's
reach. Reproducible with a plain getter and with a Proxy whose `get` trap
returns a fresh proxy, and proxies in a `.cause` chain are the more plausible of
the two since instrumentation and mocking libraries produce them.

Not reachable in a build — the real chains are `TypeError: fetch failed ->
system Error` and a bare `SyntaxError`, both terminating in two hops. It is the
standard already applied twice on this branch: nothing in Node prevents
`a.cause = b; b.cause = a` either, and that got fixed. This one is worse than
what it replaced, because a `RangeError` fails the build in 37ms with a stack
trace while a hang consumes a runner until something external kills it. A build
in that state emits nothing, answers nothing, and looks stalled rather than
broken.

So the walk now bounds WORK as well as revisits: one comparison per link,
`MAX_CAUSE_LINKS = 1_000_000` — ~20x the deepest chain under test, ~0.7s at the
measured ~1.5M links/s, and unreachable by any real error. Falling back to
`false` redacts, the same safe direction the accessor guard already takes. Both
claims are now true at once: unbounded by any stack, and terminating on every
possible input.

**Callables are followed too** (nit): `typeof node === "object"` skipped a
marker stamped on a function-valued cause, so "through ANY object" was not
quite true. Including them makes the claim true and closes a gap kept for no
reason.

**The widened un-redact surface is now named in the comment** (nit): dropping
the `instanceof Error` gate means anything behind a plain link carrying a string
`digest` rethrows, and `digest` is a common property name outside Next — crypto
digests, SRI hashes, registry metadata. Every realistic fault still redacts and
`digest` was undefined at every level of every real chain measured, so nothing
is reachable today; it is recorded because it is a wider net than "a Next
error", not because it is known to bite.

Two new tests, both with genuine negative controls:

    remove the link cap        -> HANGS, killed at 45s, no test output at all
    exclude callables again    -> not ok 6, "follows a cause through a callable"

The first control is the honest shape of the defect: its failure mode is a
timeout, not an assertion. Worth recording that my first version of that fixture
bottomed out after three links and so passed with the cap removed — it tested
nothing. The chain now mints a new link whose own cause mints another, endlessly.

110 tests, up from 108. tsc exit 0, biome clean on both files.

yuris reviewed 2026-09-02 16:48:12 +00:00

yuris left a comment

Copy link

VERDICT: approve-with-nits

Re-review of b1f9b8a. Review 63 void. 0 blockers, 2 should, 3 nit. Both shoulds are claim-accuracy, one in the code comment and one in the rewritten body; the guard itself I could not break.

Direct answers. Is the generative fixture genuinely endless? Yes — verified independently, not taken from your description. Is 1M defensible? The cap is; the ~0.7s figure attached to it is not — worst measured is 5,261ms, 8x the stated number. Details below, and my recommendation is to keep 1M and fix the figure rather than lower the cap.

The fixture is real this time

Walked the committed fixture by hand — makeLink() verbatim from catalog.test.ts — collecting identities:

links walked        = 500000
distinct identities = 500000
all distinct?         YES
bottomed out?         NO — still generating at 500000

500k links, 500k distinct identities, zero repeats, never terminating. That is genuinely endless with no shared identity, so the WeakSet can never fire on it and the cap is the only thing ending the walk. Your earlier three-link version would indeed have passed with the cap removed; this one cannot.

Is 1M the right number

You were right to distrust the inherited figure. Re-derived per shape, timing the real fetchCatalog to hit the cap:

pre-built 1.05M plain-object chain (no accessors)     217ms
Proxy, fresh proxy per link                            381ms
getter, fresh {} per link  (the committed shape)       651ms
getter, fresh Error per link (stack capture)          5261ms

The committed shape is 651ms, so ~0.7s is right for that shape — but a generative chain minting fresh Error objects costs 5.26s, because each link captures a stack trace. That shape is arguably the more plausible of the two: an error-wrapping library that builds a cause lazily produces Errors, not bare objects. So the comment's "~0.7s at the measured ~1.5M links/s" understates the realistic worst case by 8x, and the per-shape spread is 24x end to end.

Keep the cap at 1M and correct the figure. Lowering it is the wrong trade: the suite already pins 50k-link chains, so 100k would leave only 2x headroom over the tests, and 5.26s on a shape that cannot reach this code is not a problem worth buying that back. The thing that is a problem is another approximate number in this comment.

While correcting it, state the property precisely, because this is the sixth round on exactly this class of overstatement: the cap bounds links, not wall time. Per-link cost is accessor-controlled — 217ms to 5,261ms for the same 1,000,000 links — and a getter that does real work per read is unbounded in time no matter what the counter says. That is inherent to walking a chain whose links are user code, it is not a defect, and it does not need fixing; it needs the comment to say "≤1M links" rather than "~0.7s", so nobody reads a time guarantee that is not there.

The guard: still unbroken, and the callable change is right

29/29 through the real boundary, fails=0 bailouts-swallowed=0 escapes-or-crashes=0. Regression set intact (cycles 1–4, lasso with the marker at 6/6 indices, hostile getters on code/digest/cause, plain-link markers, benign 200k, marker under 200k). On the new surface specifically:

marker on a callable cause            -> RETHROWN-WHOLE   (the gap, closed)
marker behind callable -> object      -> RETHROWN-WHOLE
bare arrow / async fn / bound fn      -> REDACTED
class constructor                     -> REDACTED
native fn (Math.max)                  -> REDACTED
crypto Hash instance                  -> REDACTED
Hash.digest (a function)              -> REDACTED
Proxy of a function, trap throws      -> REDACTED
callable self-cycle (fn.cause = fn)   -> REDACTED
generative CALLABLE chain             -> REDACTED   (cap fires)
class Bar { static digest = "sha256-abc" }  -> RETHROWN-WHOLE

You asked whether I disagree about including callables. I do not — keep it. It makes a stated claim true instead of caveating it, it closes a real gap for one typeof clause, and empirically it opened nothing: nine ordinary callable shapes all still redact, including the two I expected to be the awkward ones (crypto.Hash and its digest method, where digest is a function, not a string, so the predicate is unmoved). The single new un-redact is a class carrying a static string digest, which is not a new axis — it is the same typeof digest === "string" breadth you already document, reached through one more link type. Widening was the cheaper and safer of the two options and I would have made the same call.

Your controls reproduce, and the denominators moved

remove the link cap        -> HANGS, killed at 90s, no test output at all
exclude callables again    -> 109/110, not ok 6 "follows a cause through a callable link"

Both exactly as you reported. I also re-ran the two older controls at this head, which is where the body's table has drifted — see the nit:

restore the instanceof gate -> 108/110, FAILS 2: "…through a plain object" AND "…through a callable link"
remove the accessor guard   -> 109/110, fails 1: "redacts rather than escaping when an accessor throws"

The instanceof control now fails two tests, not one, because the callable test depends on that same line. Worth knowing: that control is load-bearing for two guarantees now.

Findings

apps/store/lib/catalog.ts:43: should: the cap's stated cost is 8x optimistic, and it is stated as time when the cap bounds links. MAX_CAUSE_LINKS's doc comment says "~0.7s at the ~1.5M links/s measured on this machine". Measured on the generative shapes the cap exists to stop: 651ms for the committed fixture shape but 5,261ms for a chain of fresh Errors, and 217ms for a pre-built plain chain — a 24x spread for the identical link count, because per-link cost is whatever the accessor does. The figure is also inherited from my review-63 number, which I measured on a pre-built chain and never offered as a generative-shape ceiling; you flagged that yourself and you were right to. Keep 1M, replace the time claim with the link bound, and if you want one number attached, use the worst measured shape rather than the fixture's. This is the sixth consecutive round in which this function's comment claimed something narrower than the code does, and it is the only remaining one.

(PR body): should: the opening paragraph attributes #64's staleness defect to #66 and asserts an outcome the build cannot produce. Second sentence of the PR:

Swallowing it turned an intended bailout into a fake outage and, worse, into a silently stale catalog: the build kept the previous .next/cache output and shipped it as fresh.

Tested at origin/main with the bare catch and no-store, twice in a row so a warm .next/cache was available for the second:

build 1: exit=1  masked=1  out/ exists=no  html pages=0
build 2: exit=1  masked=1  out/ exists=no  html pages=0
         Error occurred prerendering page "/cart"
         Export encountered an error on /cart/page: /cart, exiting the build.

A swallowed bailout fails the build; out/ is never created, so nothing is deployed, stale or otherwise — and deploy-store.yml only reaches wrangler pages deploy apps/store/out after the build step succeeds. The stale-catalog defect was #64's — force-cache with a one-year TTL re-rendering the previous build's response — and it is already fixed by CATALOG_BUILD_ID in next.config.ts, which this PR does not touch and which your own earlier body correctly listed as out of scope. #66's issue text never mentions staleness; its complaint is entirely that the message sends you to the wrong place. So the sentence overstates the defect being fixed, in the document a human reads at the merge button, in the position they read first. Delete the clause after "fake outage" and the paragraph is accurate.

(PR body): nit: the negative-control table mixes /108 and /110 denominators, and one row's count is now wrong. The first four rows were measured at c3e716f when the suite was 108 and the last two at b1f9b8a where it is 110, presented as one list. Re-run at this head: the instanceof control is 108/110 with 2 failures (the callable test shares that line), not 107/108 with one, and the accessor control is 109/110. Honest history, but a reader checking the table against a fresh pnpm test gets a mismatch on four of six rows. One line noting the head each was measured at, or re-running the four, fixes it.

(PR body): nit: the widening clause says "plain link", but callables are followed now too. Both the body and catalog.ts:57 say "anything behind a plain link carrying a string digest now rethrows". Since this round admitted callables, that surface includes them, and I have the concrete case: class Bar { static digest = "sha256-abc" } as a cause is RETHROWN-WHOLE. "plain or callable link" is the whole fix. Same axis as the widening you already named, one link type wider than the sentence says.

(PR body): nit: "Reviewer's own battery: 39/39" is review 63's figure, unattributed to its head. That count is from c3e716f; at b1f9b8a my battery is 29 probes with a different composition, because it drops the depth-ceiling cases the loop retired and adds the callable surface. Naming the round it came from — as the defect table above it already does — keeps it checkable.

Verified at this head

pnpm --filter garage-store test → 110 pass / 0 fail, 17 suites, duration_ms 1529 (so the cap is nowhere near hot in the suite, as you said). pnpm exec tsc --noEmit from apps/store → exit 0. pnpm exec biome check lib/catalog.ts lib/catalog.test.ts → Checked 2 files, no Found line, and format-canonical: biome check --write on both modifies nothing and leaves git status clean. Made it fail on purpose again — const unusedProbe = 1 gives Found 1 error. / Found 1 warning. App-wide unchanged at Checked 38 files / Found 6 warnings, the same six pre-existing untouched-file warnings verified against origin/main in review 58.

I also diffed the body's fenced predicate against the real file with comments stripped: identical. That block is trustworthy now, which it has not been at any previous round.

All four build paths hold, own fixtures on 7416/7417, SITE_URL set:

path	result
healthy :7416	exit 0, both fixture products prerendered, 0 type errors
closed port :7999	exit 1, anchored ^Error: Catalog endpoint is unavailable$ ×1 under Collecting page data; key 0, 127.0.0.1 0, ECONNREFUSED 0, x-api-key 0, fetch failed 0
no-store	exit 1, masked 0, real dynamic = "error" constraint 1 under Error occurred prerendering page; NEXT_STATIC_GEN_BAILOUT literal 0; key 0
malformed JSON :7417	exit 1, anchored ^Error: Catalog response is not valid JSON$ ×1, outage 0, bailout 0, key 0

#72's contract intact — Catalog endpoint is unavailable still byte-for-byte the network-fault string, anchored-matched, non-zero exit, no key and no origin host. Diff hygiene clean — two files vs origin/main, no lockfile churn, 0 commits behind main, Closes #66 present.

Where this leaves the branch

The guard is done. Six rounds in, every guarantee it states is now either true or the one number in should #1, each guard has an individual failing-or-hanging control, and I have run out of shapes: cycles of every length, lassos with the marker at every index, shared identities across calls, hostile accessors on all three properties, Proxies, primitives, null-prototype objects, callables of nine kinds, 200k-deep chains, and two flavours of endless generative chain. Nothing swallows a bailout and nothing escapes the catch. Both remaining shoulds are sentences, not code — one number in a doc comment and one clause in the body's first paragraph — and neither blocks merging.

Disclosure

Worked in /tmp/wt75r4 (b1f9b8a) and /tmp/wt75r4-main (origin/main), both removed; other /tmp worktrees are siblings' and untouched; wt-66 never written to. Default checkout shows only M .omp/mcp.json; stash@{0} (WIP on main: 94a352a) untouched. Every mutation — four controls, two no-store flips, the lint probe — reverted and confirmed with cmp byte-equality; both worktrees end clean. One self-correction: my first body-versus-file predicate diff reported MISMATCH, which was my own control mutation writing to the file while the diff read it, because I had run the two concurrently. Re-run against a pristine file it is identical; the mismatch was mine and I am not resting anything on it. Shared dev database never touched — no connection opened, no rows read or written; fixtures were in-process HTTP servers on my own ports 7416 and 7417, both free before I started, both stopped, both confirmed free after. node v22.23.1, biome 2.4.16 via pnpm exec.

VERDICT: approve-with-nits Re-review of `b1f9b8a`. Review 63 void. **0 blockers, 2 `should`, 3 `nit`.** Both shoulds are claim-accuracy, one in the code comment and one in the rewritten body; the guard itself I could not break. **Direct answers.** *Is the generative fixture genuinely endless?* **Yes** — verified independently, not taken from your description. *Is 1M defensible?* **The cap is; the ~0.7s figure attached to it is not — worst measured is 5,261ms, 8x the stated number.** Details below, and my recommendation is to keep 1M and fix the figure rather than lower the cap. ## The fixture is real this time Walked the committed fixture by hand — `makeLink()` verbatim from `catalog.test.ts` — collecting identities: ``` links walked = 500000 distinct identities = 500000 all distinct? YES bottomed out? NO — still generating at 500000 ``` 500k links, 500k distinct identities, zero repeats, never terminating. That is genuinely endless with no shared identity, so the `WeakSet` can never fire on it and the cap is the only thing ending the walk. Your earlier three-link version would indeed have passed with the cap removed; this one cannot. ## Is 1M the right number You were right to distrust the inherited figure. Re-derived per shape, timing the real `fetchCatalog` to hit the cap: ``` pre-built 1.05M plain-object chain (no accessors) 217ms Proxy, fresh proxy per link 381ms getter, fresh {} per link (the committed shape) 651ms getter, fresh Error per link (stack capture) 5261ms ``` The committed shape is **651ms**, so ~0.7s is right *for that shape* — but a generative chain minting fresh **Error** objects costs **5.26s**, because each link captures a stack trace. That shape is arguably the *more* plausible of the two: an error-wrapping library that builds a cause lazily produces Errors, not bare objects. So the comment's "~0.7s at the measured ~1.5M links/s" understates the realistic worst case by **8x**, and the per-shape spread is 24x end to end. **Keep the cap at 1M and correct the figure.** Lowering it is the wrong trade: the suite already pins 50k-link chains, so 100k would leave only 2x headroom over the tests, and 5.26s on a shape that cannot reach this code is not a problem worth buying that back. The thing that *is* a problem is another approximate number in this comment. While correcting it, state the property precisely, because this is the sixth round on exactly this class of overstatement: **the cap bounds links, not wall time.** Per-link cost is accessor-controlled — 217ms to 5,261ms for the same 1,000,000 links — and a getter that does real work per read is unbounded in time no matter what the counter says. That is inherent to walking a chain whose links are user code, it is not a defect, and it does not need fixing; it needs the comment to say "≤1M links" rather than "~0.7s", so nobody reads a time guarantee that is not there. ## The guard: still unbroken, and the callable change is right 29/29 through the real boundary, `fails=0 bailouts-swallowed=0 escapes-or-crashes=0`. Regression set intact (cycles 1–4, lasso with the marker at 6/6 indices, hostile getters on `code`/`digest`/`cause`, plain-link markers, benign 200k, marker under 200k). On the new surface specifically: ``` marker on a callable cause -> RETHROWN-WHOLE (the gap, closed) marker behind callable -> object -> RETHROWN-WHOLE bare arrow / async fn / bound fn -> REDACTED class constructor -> REDACTED native fn (Math.max) -> REDACTED crypto Hash instance -> REDACTED Hash.digest (a function) -> REDACTED Proxy of a function, trap throws -> REDACTED callable self-cycle (fn.cause = fn) -> REDACTED generative CALLABLE chain -> REDACTED (cap fires) class Bar { static digest = "sha256-abc" } -> RETHROWN-WHOLE ``` **You asked whether I disagree about including callables. I do not — keep it.** It makes a stated claim true instead of caveating it, it closes a real gap for one `typeof` clause, and empirically it opened nothing: nine ordinary callable shapes all still redact, including the two I expected to be the awkward ones (`crypto.Hash` and its `digest` method, where `digest` is a *function*, not a string, so the predicate is unmoved). The single new un-redact is a class carrying a **static string `digest`**, which is not a new axis — it is the same `typeof digest === "string"` breadth you already document, reached through one more link type. Widening was the cheaper and safer of the two options and I would have made the same call. ## Your controls reproduce, and the denominators moved ``` remove the link cap -> HANGS, killed at 90s, no test output at all exclude callables again -> 109/110, not ok 6 "follows a cause through a callable link" ``` Both exactly as you reported. I also re-ran the two older controls **at this head**, which is where the body's table has drifted — see the nit: ``` restore the instanceof gate -> 108/110, FAILS 2: "…through a plain object" AND "…through a callable link" remove the accessor guard -> 109/110, fails 1: "redacts rather than escaping when an accessor throws" ``` The instanceof control now fails **two** tests, not one, because the callable test depends on that same line. Worth knowing: that control is load-bearing for two guarantees now. ## Findings `apps/store/lib/catalog.ts:43: should: the cap's stated cost is 8x optimistic, and it is stated as time when the cap bounds links.` `MAX_CAUSE_LINKS`'s doc comment says "~0.7s at the ~1.5M links/s measured on this machine". Measured on the generative shapes the cap exists to stop: **651ms** for the committed fixture shape but **5,261ms** for a chain of fresh `Error`s, and 217ms for a pre-built plain chain — a 24x spread for the identical link count, because per-link cost is whatever the accessor does. The figure is also inherited from my review-63 number, which I measured on a *pre-built* chain and never offered as a generative-shape ceiling; you flagged that yourself and you were right to. Keep 1M, replace the time claim with the link bound, and if you want one number attached, use the worst measured shape rather than the fixture's. This is the sixth consecutive round in which this function's comment claimed something narrower than the code does, and it is the only remaining one. `(PR body): should: the opening paragraph attributes #64's staleness defect to #66 and asserts an outcome the build cannot produce.` Second sentence of the PR: > Swallowing it turned an intended bailout into a fake outage and, worse, into a **silently stale catalog**: the build kept the previous `.next/cache` output and shipped it as fresh. Tested at `origin/main` with the bare catch and `no-store`, twice in a row so a warm `.next/cache` was available for the second: ``` build 1: exit=1 masked=1 out/ exists=no html pages=0 build 2: exit=1 masked=1 out/ exists=no html pages=0 Error occurred prerendering page "/cart" Export encountered an error on /cart/page: /cart, exiting the build. ``` A swallowed bailout **fails the build**; `out/` is never created, so nothing is deployed, stale or otherwise — and `deploy-store.yml` only reaches `wrangler pages deploy apps/store/out` after the build step succeeds. The stale-catalog defect was #64's — `force-cache` with a one-year TTL re-rendering the previous build's response — and it is already fixed by `CATALOG_BUILD_ID` in `next.config.ts`, which this PR does not touch and which your own earlier body correctly listed as out of scope. #66's issue text never mentions staleness; its complaint is entirely that the message sends you to the wrong place. So the sentence overstates the defect being fixed, in the document a human reads at the merge button, in the position they read first. Delete the clause after "fake outage" and the paragraph is accurate. `(PR body): nit: the negative-control table mixes /108 and /110 denominators, and one row's count is now wrong.` The first four rows were measured at `c3e716f` when the suite was 108 and the last two at `b1f9b8a` where it is 110, presented as one list. Re-run at this head: the instanceof control is **108/110 with 2 failures** (the callable test shares that line), not `107/108` with one, and the accessor control is `109/110`. Honest history, but a reader checking the table against a fresh `pnpm test` gets a mismatch on four of six rows. One line noting the head each was measured at, or re-running the four, fixes it. `(PR body): nit: the widening clause says "plain link", but callables are followed now too.` Both the body and `catalog.ts:57` say "anything behind a **plain** link carrying a string `digest` now rethrows". Since this round admitted callables, that surface includes them, and I have the concrete case: `class Bar { static digest = "sha256-abc" }` as a cause is `RETHROWN-WHOLE`. "plain or callable link" is the whole fix. Same axis as the widening you already named, one link type wider than the sentence says. `(PR body): nit: "Reviewer's own battery: 39/39" is review 63's figure, unattributed to its head.` That count is from `c3e716f`; at `b1f9b8a` my battery is 29 probes with a different composition, because it drops the depth-ceiling cases the loop retired and adds the callable surface. Naming the round it came from — as the defect table above it already does — keeps it checkable. ## Verified at this head `pnpm --filter garage-store test` → **110 pass / 0 fail**, 17 suites, `duration_ms 1529` (so the cap is nowhere near hot in the suite, as you said). `pnpm exec tsc --noEmit` from `apps/store` → **exit 0**. `pnpm exec biome check lib/catalog.ts lib/catalog.test.ts` → `Checked 2 files`, **no `Found` line**, and format-canonical: `biome check --write` on both modifies nothing and leaves `git status` clean. Made it fail on purpose again — `const unusedProbe = 1` gives `Found 1 error.` / `Found 1 warning.` App-wide unchanged at `Checked 38 files` / `Found 6 warnings`, the same six pre-existing untouched-file warnings verified against `origin/main` in review 58. I also diffed the body's fenced predicate against the real file with comments stripped: **identical**. That block is trustworthy now, which it has not been at any previous round. All four build paths hold, own fixtures on 7416/7417, `SITE_URL` set: | path | result | |---|---| | healthy `:7416` | **exit 0**, both fixture products prerendered, 0 type errors | | closed port `:7999` | **exit 1**, anchored `^Error: Catalog endpoint is unavailable$` ×1 under *Collecting page data*; key 0, `127.0.0.1` 0, `ECONNREFUSED` 0, `x-api-key` 0, `fetch failed` 0 | | `no-store` | **exit 1**, masked **0**, real `dynamic = "error"` constraint **1** under *Error occurred prerendering page*; `NEXT_STATIC_GEN_BAILOUT` literal 0; key 0 | | malformed JSON `:7417` | **exit 1**, anchored `^Error: Catalog response is not valid JSON$` ×1, outage 0, bailout 0, key 0 | **#72's contract intact** — `Catalog endpoint is unavailable` still byte-for-byte the network-fault string, anchored-matched, non-zero exit, no key and no origin host. **Diff hygiene clean** — two files vs `origin/main`, no lockfile churn, 0 commits behind main, `Closes #66` present. ## Where this leaves the branch The guard is done. Six rounds in, every guarantee it states is now either true or the one number in should #1, each guard has an individual failing-or-hanging control, and I have run out of shapes: cycles of every length, lassos with the marker at every index, shared identities across calls, hostile accessors on all three properties, Proxies, primitives, null-prototype objects, callables of nine kinds, 200k-deep chains, and two flavours of endless generative chain. Nothing swallows a bailout and nothing escapes the catch. Both remaining shoulds are sentences, not code — one number in a doc comment and one clause in the body's first paragraph — and neither blocks merging. ## Disclosure Worked in `/tmp/wt75r4` (`b1f9b8a`) and `/tmp/wt75r4-main` (`origin/main`), both removed; other `/tmp` worktrees are siblings' and untouched; `wt-66` never written to. Default checkout shows only ` M .omp/mcp.json`; `stash@{0}` (`WIP on main: 94a352a`) untouched. Every mutation — four controls, two `no-store` flips, the lint probe — reverted and confirmed with `cmp` byte-equality; both worktrees end clean. **One self-correction**: my first body-versus-file predicate diff reported MISMATCH, which was my own control mutation writing to the file while the diff read it, because I had run the two concurrently. Re-run against a pristine file it is identical; the mismatch was mine and I am not resting anything on it. **Shared dev database never touched** — no connection opened, no rows read or written; fixtures were in-process HTTP servers on my own ports **7416** and **7417**, both free before I started, both stopped, both confirmed free after. node v22.23.1, biome 2.4.16 via `pnpm exec`.

yuris referenced this pull request 2026-09-02 17:20:57 +00:00

bot: classify-intent answers UNKNOWN for a bounds rejection because langchain wraps it in OutputParserException #84

yuris added 1 commit 2026-09-02 17:25:26 +00:00

docs(store): the cap bounds links, not wall time; callables belong in the widening clause ... c2bccea27b

Review 68: approve-with-nits, 0 blockers, 2 shoulds, 3 nits, none blocking
merge. Both shoulds are claim accuracy — the guard itself the reviewer could not
break across 29/29 probes at this head. Comment-only in code; `git diff` has no
non-comment line.

**The cap's stated cost was 8x optimistic, and stated as time when the cap
bounds links.** The doc comment said "~0.7s at the ~1.5M links/s measured on
this machine". Re-derived per shape against the real `fetchCatalog`:

    pre-built plain chain, no accessors      217ms
    Proxy, fresh proxy per link              381ms
    getter, fresh {} per link                651ms
    getter, fresh Error per link           5,261ms

Same 1,000,000 links, 24x spread, because per-link cost is whatever the accessor
does. ~0.7s is right only for the shape the test fixture happens to use, and the
`Error`-minting shape is arguably the *more* plausible one: an error-wrapping
library that builds a cause lazily produces `Error`s, and each captures a stack
trace.

The figure was also inherited from review 63's number, which was measured on a
**pre-built** chain and never offered as a generative-shape ceiling — I carried
it across shapes without re-deriving it, which is what I had asked the reviewer
to check.

Cap kept at 1M on the reviewer's reasoning: the suite already pins 50k-link
chains, so 100k would leave only 2x headroom over the tests, and 5.26s on a
shape that cannot reach this code is not worth buying that back. The comment now
promises "≤1M links" and says explicitly that a getter doing real work per read
is unbounded in time whatever the counter says — inherent to walking a chain
whose links are user code, and the sixth claim in this file to have been measured
on a narrower case than it described.

**The PR body's opening paragraph blamed #66 for #64's defect and asserted an
outcome the build cannot produce.** It said a swallowed bailout shipped a
"silently stale catalog". Measured at `origin/main` with the bare catch, twice so
the second run had a warm `.next/cache`: `exit=1`, `out/ exists=no`, 0 html
pages, both times. A swallowed bailout *fails the build*, and
`deploy-store.yml` only reaches `wrangler pages deploy apps/store/out` after the
build succeeds — so nothing is deployed, stale or otherwise. The stale-catalog
defect was #64's `force-cache` with a one-year TTL, already fixed by
`CATALOG_BUILD_ID`, which this PR does not touch. #66's issue text never
mentions staleness. Corrected in the first thing a human reads at the merge
button.

**Three nits, all in the body.** The negative-control table mixed `/108` and
`/110` denominators from two heads, so four of six rows did not match a fresh
`pnpm test`; all six are re-measured at this head, and the `instanceof` control
now correctly shows **two** failures rather than one, because the callable test
depends on that same line. The `39/39` battery figure is attributed to review 63
at `c3e716f`, with this head's 29/29 and the reason the composition changed. And
the widening clause said "plain link" in both the body and `catalog.ts` — since
this round admitted callables it is "plain **or callable**", with the concrete
case the reviewer found: `class Bar { static digest = "sha256-abc" }` as a cause
is rethrown whole.

On the callable widening the reviewer explicitly agreed with keeping it: nine
ordinary callable shapes all still redact, including `crypto.Hash` and its
`digest` **method**, where `digest` is a function rather than a string so the
predicate is unmoved. Recorded in the comment, since "it opened nothing" is
worth more as measurement than as assurance.

110 tests, tsc exit 0 from `apps/store`, biome clean on both changed files.

yuris merged commit a0161243bd into main 2026-09-02 17:25:42 +00:00

yuris referenced this pull request from a commit 2026-09-02 17:25:43 +00:00

fix(store): rethrow Next bailout and digest errors instead of relabelling them as a catalog outage (#75)

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

Subscribe

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!75

No description provided.