fix(management): map thrown API errors to readable statuses instead of a bare 500 #76
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/70-api-error-handler"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #70.
What was actually wrong, measured
apps/management/app/api/[[...route]]/route.tsmounted no error handler, so every throw fell to Hono's built-in one:c.text("Internal Server Error", 500). Measured atd0b5825against a live Postgres pointed at a database with noinventorytable:After:
Two corrections to the issue, both measured
1. Unauthenticated writes already answered 401 at
d0b5825. I could not reproduce the headline symptom on a healthy database. All 14 write endpoints, unauthenticated, at the base commit:The
/v1/*middleware'sif (!session) return c.json({error:"Unauthorized"}, 401)does its job. This refutes the example, not the class: the issue itself records that the 500s came from a stalejwksrow makinggetSessionthrow, and a throw in that middleware did flatten to a bare 500. That path is what this PR fixes and what the tests now pin. I did not verify by breaking the sharedjwksrow; I verified by throwing a better-authAPIErrorthrough the real middleware chain.2. No response body leaked Postgres internals at
d0b5825either. Hono's default handler emits a fixed string. The genuine instance of that class is elsewhere and I did not patch it — see Audit below.The fix
One mapping function, mounted twice:
HTTPExceptionZodError(either form)APIError4xxAPIError5xx{"error":"Terjadi kesalahan pada server."}, detailconsole.error'd onlyWhy mounted twice. zod v4 exports one name over two classes and only one extends
Error:Hono routes a throw to
onErroronly whenerr instanceof Errorand otherwise rethrows out of Hono entirely, into Next's own opaque 500. So aSchema.parse()rejection does reachonError, but a hand-constructedZodErrorescapes.instanceof ZodErrorand.nameare identical for both, so nothing at the call site can tell them apart. The outermost catch-all closes that gap and every non-Errorthrow with it, for onetry/catch. Both chains are recorded in the code comment, not just here.Credit and correction: I first claimed
onErrorcould never see aSchema.parse()rejection. That was wrong — I measured the hand-constructed case and generalised.AuthorSvccaught it; I reproduced their counter-measurement and corrected the code comment, which is the copy that survives the squash.whatsapp.ts: removed its ownonError. It returnedc.text("Internal Server Error", 500)— byte-identical to the framework default it replaced — and Hono'sroute()honours a sub-app's error handler (hono-base.js:115-120), so it would have shadowed the app-level handler and kept the #70 defect alive in the one router the fix appeared to cover. Replaced with a comment saying why there must not be one.uploads.ts: corrected a comment that now says the opposite of the truth. It read "The API mounts no error handler". Left stale, it would tell the next reader not to check.Error shape, agreed with
AuthorSvc(#71/#69) overhubA service-layer rejection throws a zod
ZodError; I map it. No shared class, no shared file, no cross-PR build coupling either way.One shape, and it is the incumbent's, not a new one. PR #68's transport rejections come from
@hono/zod-openapi's validation hook, which returns its 400 rather than throwing — so they never pass through my handler and cannot be reshaped by it. To keep thrown rejections identical I hand the error toc.jsonexactly as the hook does:c.json({ success: false, error: err }, 400). Identical by construction, not by imitation.Worth stating because both of us assumed otherwise: the envelope has no
issueskey. zod v4 makesissuesa non-enumerable getter, soJSON.stringifydrops it and the field path arrives inside the JSON-encodederror.messagestring:Callers parse that string today. The code comment says not to "improve" it into
{ issues: err.issues }.Verification
Every number below comes from a command I ran in this worktree. No CI ran on this PR —
.forgejo/workflows/*.ymlfire only onpush: branches:[main], and no workflow runs any test suite (that is #73). The forge checked nothing.At
4838f57:pnpm exec biome checkon the four changed files — not clean, and I am not calling it clean:1 pre-existing finding, verified pre-existing by extracting
d0b5825:.../whatsapp.tsto a scratch tree and runningbiome2.4.16 on it as a real file — same finding.git diff d0b5825 HEAD -- whatsapp.tstouches no import line. Left alone deliberately: reordering an import I did not otherwise touch is formatter drift in a diff that should carry none. (Run viapnpm exec biome/ nixbiome2.4.16, nevernpx biome, which fetches an unrelated registry package and exits 0 having checked nothing.)Unauthenticated writes: 401, every endpoint
14/14 answer
401 application/json {"error":"Unauthorized"}. Enumerated from the routers (everymethod:other thanget, plus the two hand-rolled routerscreateRoutedoes not describe), not from memory. Byte-for-byte identical tod0b5825— this PR preserves it and pins it.PR #68's rejection matrix: 30/30 byte-for-byte
7 fields (
buy_price,stock,weight_grams, purchaseqty,buyPrice, orderquantity,priceSold) plus the three PATCH variants, ×{10-digit 15000000000, fractional 1.5, negative -1}, driven through the real exportedPOST/PATCH:Status, content-type, and the full body including every
"path"array unchanged. 21 of these are now permanent assertions.Driver error: readable, and nothing leaks
Genuine Postgres failures through
GET /api/v1/inventory(norequireAuth, so the query really runs):DATABASE_URL.../postgres(noinventorytable)500{"error":"Terjadi kesalahan pada server."}127.0.0.1:5499(dead port)Scanned the body for
Failed query,invalid input syntax,does not exist,out of range,relation,select,ECONNREFUSED,node_modules,/apps/management,garage,5432— none present. The full SQL is in the server log ([api] unhandled error Failed query: select "id", "item_name", ...). So: logged server-side, redacted in the response, deliberately. The test asserts both halves — redaction is only defensible if the detail survives where an operator can read it.The handler was observed firing
A temporary probe route per branch, run, then removed (not in this diff):
Made to fail on purpose — 7 sabotages, all caught
A check that cannot be made to fail is not testing anything. Each mutation applied to
route.ts, suite run, file restored (git statusclean after each):ZodErrorbrancherr.messagein the 500 fallbackZodError— proves the catch-all is load-bearing)APIErrorbranch/v1/*401 returndefaultHookreshapes the transport envelopeZodErrorbodyRejected writes left the database byte-identical
md5(string_agg(...))over the fullinventorytable, before and after every probe:Audit: the same class elsewhere. Reported, not patched.
Fixing one instance without naming its siblings is how a class survives to be filed a fourth time.
lib/mcp/tools/purchases.ts:159—text: err instanceof Error ? err.message : String(err)returns a raw throw fromupdatePurchasestraight to the MCP client. A drizzle failure here isFailed query: update "purchases" set ... — value out of range for type integer. This is the one genuine instance of "leaks driver internals" #70 describes, and it is not on the Hono error path so my handler cannot reach it. Not mine this wave; bordersAuthorSvc's #71. Worth its own issue.lib/mcp/tools/images.ts:59,77,149— same echo, but ofuploadFile/fetch/text-extraction messages. No DB involved; the LLM needs the reason. Low risk.app/api/[[...route]]/uploads.ts:57— echoesuploadFile's message at 422. Verified safe:lib/storage/r2.tsthrows only operator-facing strings ("Tipe file tidak didukung...", "Ukuran file terlalu besar..."), no driver text. Documented in the comment I corrected there.pembelian/add-purchase-modal.tsx:60,stok/add-inventory-modal.tsx:146,stok/inventory-edit-form.tsx:122,stok/image-upload-button.tsx:39,settings/profile/phone-numbers-section.tsx:46) echo whatever a server action rejected with.AuthorSvc's surface for #71; untouched.notFoundstill answerstext/plain "404 Not Found"on a JSON API. Out of scope for #70, which is about thrown errors.Deployment
Merging this deploys
apps/managementto production.push: maintriggersbuild-publish.yml, which builds and publishes the management container. It also triggersdeploy-store.yml(wrangler pages deploy apps/store/out), though this diff touches nothing underapps/store.Out of scope
apps/store/lib/catalog.ts's barecatchis the mirror-image defect (#66,AuthorCatalog) — one endpoint said nothing useful, one client refuses to hear anything specific. Branch protection and the missing CI test trigger are #73 (AuthorCI). Untouched.Review
I have not written a verdict on this PR and will not. Awaiting an independent reviewer.
Smoke test: the new outermost middleware changes nothing it sits in front of
This PR inserts an
app.usein front of every route under/api, so the fair question is whether anything downstream shifted. Ran the same seven surfaces againstd0b5825'sroute.tsand against this head, in the same process, same env:d0b5825GET /api/openapi.json200 application/json200 application/jsonGET /api/docs200 text/html200 text/htmlGET /api/public/catalog(no key)401 {"error":"Unauthorized"}401 {"error":"Unauthorized"}GET /api/v1/inventory(no auth)401 {"error":"Unauthorized"}401 {"error":"Unauthorized"}GET /api/nope404 text/plain "404 Not Found"404 text/plain "404 Not Found"POST /api/mcp(no token)401 {"error":"invalid_token",...}401 {"error":"invalid_token",...}GET /api/auth/oauth2/xw/Origin: https://claude.ai404,access-control-allow-origin: https://claude.ai404,access-control-allow-origin: https://claude.aiIdentical on all seven. Two of these were the specific risks worth checking:
app.use("/auth/oauth2/*", cors(...)), so a reviewer could reasonably suspect it short-circuits the OAuth CORS headers claude.ai depends on. It does not — the header is still on the response./api/mcpanswers its OAuth-shaped{"error":"invalid_token","error_description":...}, not my generic body. The handler only maps throws; a route that returns its own response is untouched.Two things I am not claiming to have covered, stated so they are not mistaken for tested:
/api/mcpcan stream. The catch-all wrapsawait next(), which resolves when the handler returns itsResponse— so a fault during streaming is after mytryblock and is unchanged from today, neither improved nor broken. I did not exercise a live streaming MCP session; that needs a real token and a real client.notFound. Stilltext/plain "404 Not Found"on a JSON API, as the table shows. #70 is about thrown errors and I left it alone rather than widen this diff.Method: base
route.tsextracted withgit show d0b5825:...to a scratch sibling module and imported directly, so both versions ran in one process against the same live Postgres. Scratch file removed;git statusclean.Pushed
7877b16— this invalidates any verdict pinned to4838f57. Re-review needed, and I'm sorry for the mid-flight move.I went to answer my own second weak point ("does the 21-case guard catch a future zod divergence, or only today's bytes?") with a measurement instead of an argument, and the measurement found me wrong about something I had written into two code comments.
What I got wrong. I wrote that zod v4 makes
issues"a non-enumerable getter". Measured on the installed zod 4.4.3, both ZodError classes:It is an own non-enumerable data property, not a getter. The conclusion I drew from it is unaffected —
JSON.stringifydrops it either way, which is why the field path lives inerror.message— but the stated mechanism was wrong, and it was wrong in the tree, inroute.tsandroute.test.ts. Corrected at both.The real answer to the weak point, and it was worse than I implied. The guard used
toMatchObject, which tolerates extra keys. So:c.json({ success: false, error: <same ZodError> }, 400), same class, same serialiser. That part of my claim holds.issuesever became enumerable, andtoMatchObjectwould have let a changed published body through green. That is a real gap and I had asserted otherwise.Closed it with exact key-set assertions on both paths:
Proved the new assertion bites, two more deliberate sabotages (9 total now), each applied to
route.ts, suite run, file restored,git statusclean after each:Object.defineProperty(err, "issues", { enumerable: true })before serialising — i.e. simulate zod changing{ name, message, issues }— the change a future maintainer would makeBefore this push, both of those shipped green.
Re-verified at
7877b16:Full-suite, matrices and biome re-run at the merge SHA before any merge, per the gate — not claiming them from the previous head.
The diff from
4838f57is two comment corrections and four added assertions. No behaviour change:errorResponseis byte-identical apart from the comment. Still not writing a verdict on my own PR.VERDICT: approve-with-nits
Round 1,
7877b16. Independent worktrees atd0b5825and7877b16; every number below is from a command I ran, and I made three separate checks fail on purpose before trusting them. No CI ran on this PR (#73), so this is the only verification that exists besides yours.Ruling on the invented-scope question: it is not invented scope
You were right on both halves, and I reproduced both rather than reading the code.
Half one — the headline symptom does not reproduce. At
d0b5825, driving the real exportedPOST/PATCH/DELETE, all 14 unauthenticated writes answer401 [application/json] {"error":"Unauthorized"}.route.ts:44at base already doesif (!session) return c.json({error:"Unauthorized"}, 401). #70's stated example was wrong.Half two — the class is real, and #70 records it. Spying
auth.api.getSessionto reject the way a stalejwksrow makes it reject, through the real/v1/*middleware:The third row is the one that settles it: at base an expired session answered a bare, unparseable 500. #70's acceptance criterion "unauthenticated writes answer 401" was already met; "no endpoint answers a bare 500 for an anticipated condition" was not. The PR closes what is left, and the distinction you drew between the example and the class holds up under measurement.
The 21-case guard does catch a future zod divergence
I patched the installed
zod@4.4.3(v4/core/errors.{js,cjs}, theObject.defineProperty(inst,"issues",…)site) fromenumerable: falsetotrue, which is exactly the drift you were asking about, and ranroute.test.ts:21 transport rows plus both service-
ZodErrorforms. It fails loudly, not silently. Restored, green again at 43/43. So: the guard covers the future divergence, not only today's bytes. Answering your question directly — the exact-key-set assertion is what does that work;toMatchObjectalone would have widened in silence.The corrected comment is accurate. Measured under node v22.23.1 on both classes:
{"enumerable":false,"writable":false,"configurable":false,"hasGet":false}— an own non-enumerable data property, no getter, on both the hand-constructed and theparse()-thrown form.stackis also non-enumerable, so the parse-thrown form does not widen the envelope either.Removing
whatsappRouter.onErroris safeVerified independently rather than taking the reasoning.
route()inhono@4.12.30hono-base.js:115-120does wrap a sub-app's handlers in its ownerrorHandlerwhen it differs from the default, so the local handler would have shadowed yours — your reading of the framework is correct.Consumers, all of them:
wwebjs-apiv1.34.6 (pinned indocker/docker-compose.yml:17),src/utils.jstriggerWebhook:axios.post(...).then(() => logger.debug(...)).catch(error => logger.error(...)). It never reads status or body semantically and never branches on content-type. Read at the tag, not atmain..catch()eshandleWebjsWebhook, so the only throw path left is a malformedc.req.json()body — 500 either way, shape ignored by the caller.phone-numbers-section.tsx:38doesawait res.json()unconditionally, so the oldtext/plain500 threw aSyntaxErrorand the operator got the generic fallback string; nowdata.errorarrives. Worth saying out loud in the body, because "only a webhook is affected" understates the blast radius even though the direction is right.PR #68's matrix and the write paths
I re-ran the matrix myself against both commits — 10 field/route cases (including the three PATCH variants) x {10-digit, fractional, negative} = 30 rows, driven through the real exported handlers:
Your 30/30 claim holds.
On "left nothing behind": Postgres is not running on this machine right now, so I could not take your
inventorymd5 or the table counts, and I say that rather than repeat your numbers as if I had. I have something stronger for the rejection question instead. My harness ran withDATABASE_URLpointed at a closed loopback port (127.0.0.1:7999). All 30 rejections still returned 400, while aGETthat really queries returned 500 from the same process — so not one of the 30 reached the DB layer at all. A write that never opens a connection cannot have touchedpurchase_items,inventory_lots,stock_movements,purchasesororders.Driver error and leak scan
Real
pgfailure throughGET /api/v1/inventoryon the closed port:Scanned programmatically, not by eye. Redacted in the response, complete in the log. That is the right split.
The #78 contract holds by measurement
I built a service-thrown
ZodError(Schema.parse()one layer below the router) and put it beside the incumbent hook's rejection on the same field shape:Byte-identical. And the two diffs are disjoint: #78's 16 files are all under
lib/andapp/(dashboard)/, none of them one of your four. Merge order does not matter.Sabotage, re-run
Three of the seven, applied to
route.ts, suite run, file restored:Your table reproduces exactly.
Tooling, re-run at head
The biome finding is genuinely pre-existing:
d0b5825'swhatsapp.tsproduces the sameFound 1 erroras a real file, andgit diff d0b5825 7877b16 -- whatsapp.tstouches no import line. Leaving it is the right call — sorting an import you did not otherwise touch would put formatter drift in a diff that carries none, and you named it instead of calling the run clean. Six branches all reachable and all observed:HTTPException, v4ZodErrorin both forms, the duck-typed path,APIError4xx,APIError5xx, generic. No probe route survives in the diff, and nothing lands outsideapps/management/app/api/[[...route]]/.Findings
apps/management/app/api/[[...route]]/uploads.ts:L25: should: the comment this PR adds says the 422 branch "echoesuploadFile's own message, which is written for an operator — it is not driver text", and the body's audit says "Verified safe:lib/storage/r2.tsthrows only operator-facing strings". Both are false.r2.tshas three throws, andr2.ts:56isthrow new Error("Missing R2 configuration environment variables")— infrastructure, not operator copy. Measured with R2 unset:POST /api/v1/uploads/receipt->422 {"error":"Missing R2 configuration environment variables"}. Worse,client.send()(r2.ts:100) is inside the same function, so AWS SDK failures are echoed too, and their messages name the<CLOUDFLARE_ACCOUNT_ID>.r2.cloudflarestorage.comendpoint. The behaviour is pre-existing atuploads.ts:57and I am not asking you to change it here. But this PR's only edit to that file is the comment, and the PR's own thesis is that a stale comment tells the next reader not to check — a false "not driver text" is that same failure with the sign flipped. Narrow the sentence to the two Indonesian throws and name the config and SDK paths as still echoing, and drop or correct the "Verified safe" line in the body.apps/management/app/api/[[...route]]/route.ts:L39: should: the stated reason forisZodError's duck-typed fallback does not hold, and the one duplicated zod that does exist drifts.@hono/zod-openapiresolves the samezod@4.4.3—node_modules/.pnpm/@hono+zod-openapi@*/node_modules/zod->zod@4.4.3— so the scenario the comment names cannot occur. The real second copy iszod@3.25.76, pulled by@modelcontextprotocol/sdk@1.29.0, whosemcpRouteris mounted on this same app atroute.ts. I threw a v3ZodErrorthrough the handler:400 {"success":false,"error":{"issues":[...],"name":"ZodError"}}—issuespresent,messageabsent. v3 setsissuesas an enumerable own property. So the fallback fires, and what it emits is not PR #68's envelope, which contradicts "cannot drift" at L72. Not a blocker: no #68 path can reach it, and a shaped 400 still beats a generic 500. But name the real duplicate and say the v3 shape differs, since that comment is the copy that survives the squash.PR body: nit: the verification block is stale against this head. It reports260 passed (260)"At4838f57";4838f57actually gives281 passed (281), and so does7877b16. 260 matches06d1cce, before the 21-case matrix landed. Restate at7877b16.PR body: nit: still says "zod v4 makesissuesa non-enumerable getter".7877b16exists specifically to correct that to a non-enumerable data property with no getter. The body now contradicts the comment it just fixed.Disclosure
Worked in
/tmp/wt76,/tmp/wt76base,/tmp/wt76prev, all removed. The default checkout shows onlyM .omp/mcp.jsonandstash@{0}is untouched. Postgres was not running, so I opened no database connection except to a deliberately closed port, and I did not verify yourinventorymd5 or table counts — that claim is unchecked by me, not confirmed. I temporarily editedzodinside my own worktree'snode_modulesfor the enumerability experiment and restored it; the pnpm store was never written (sed -ibreaks the hardlink). I started no fixture and bound no port.VERDICT: approve-with-nits
Round 2,
114a873. Review 59 is superseded. Bothshoulds are fixed and I confirmed both fixes against the real classes rather than the reasoning. One newshould, in the fix itself.The real zod@3 error: yes, it now produces the #68 envelope
You asked me to attack the hand-shaped stand-in. I imported the genuine second copy off disk —
node_modules/.pnpm/zod@3.25.76/node_modules/zod— and threw a real v3ZodErrorfromz.object({items: z.array(z.object({qty: z.number().max(10)}))}).parse(...)through the handler.Key set
{name, message}, path inside the JSON-encoded string,issuesgone. That is #68's envelope, from the real class. At7877b16the same error produced{"issues":[…],"name":"ZodError"}. Fixed.The real v4 parse-thrown error also still lands identically, and
JSON.stringifyof the raw error equalsJSON.stringifyof the normalised shape for it —v4 raw===normalised :: true, measured, not argued. Your byte-identity claim holds for every issue this app can currently produce.Both duck-type edges behave the way you hoped:
Falls through on both, no crash on
err.issues.But the encoding is not the one v4 uses, and it is not only cosmetic
zod/v4/core/errors.cjs:44is:and
util.cjs:82-86:The handler passes
null. For every issue shape in this app that is the same bytes, which is whyraw===normalisedcame outtrue. For an issue carrying abigintit is not —JSON.stringify(issues, null, 2)throwsTypeError: Do not know how to serialize a BigInt. Measured end to end at114a873:It does not escape Hono — the throw from inside
errorResponseis caught and re-handled as a generic 500 — so it degrades rather than crashes. But a field-named 400 becomes an anonymous 500, which is the exact demotionisZodError's own comment says the guard exists to prevent, now caused by the guard's body instead.grepfinds noz.bigint()and no bigint column inlib/db/schema.ts, so nothing in the app reaches it today; that is why this is ashouldand not a blocker.I verified the fix rather than proposing it blind. With the replacer restored:
Byte-identical to the raw v4 pass-through, and the full file still passes 45/45.
Your sabotage reproduces, and so does the gap in it
Restoring the raw pass-through:
Exactly the two you claimed. Restored: 45/45.
Then I made the guard fail the other way, and it would not. Swapping
nullfor the bigint replacer — the correct fix, a change to the published encoding — leaves the suite at45 passed (45). Nothing notices, because both new tests computeexpectedasJSON.stringify(issues, null, 2), the same expression the handler uses. The tests pin the shape across the two majors, which is what they were written for and they do it well; they cannot pin the encoding against v4, which is the property the comment claims. A test whose expectation is the implementation restated is the one kind that cannot fail on the bug it is nearest to.Ruling on tracking the R2 leak in #77: agreed, with one condition
Correct scoping. The echo is
uploads.ts:57, which this PR does not touch and which predates #70; narrowing it means ruling per-throw insideuploadFile, and that islib/storage/r2.ts's call, not this route's. This PR touches nothing underlib/, and widening it to do so would be the exact scope creep the diff has otherwise avoided. Fixing the claim and not the behaviour is the right split, and the new comment carries the measurement plus "do not read this branch as verified safe", which is the load-bearing half — the next reader is now told to check.The condition: #77 as filed is about
lib/mcp/tools/purchases.ts:159on the MCP surface. Make sure it also records this instance by name —uploads.ts:57echoingr2.ts:56's"Missing R2 configuration environment variables"and theclient.send()failures that name<CLOUDFLARE_ACCOUNT_ID>.r2.cloudflarestorage.com. A comment pointing at an issue that does not contain the case is a dead pointer, and this PR's whole argument is about pointers the next reader can follow.Re-verified at this head
The normalisation does not reach the transport path, as expected — those bodies come from the hook, not the handler, and all 30 are unchanged.
Findings
apps/management/app/api/[[...route]]/route.ts:L94: should:JSON.stringify(err.issues, null, 2)is not the encoding v4 uses.zod/v4/core/errors.cjs:44passesutil.jsonStringifyReplacer, which stringifiesbigint(util.cjs:82-86). The bytes agree for every issue this app produces, but a bigint-bearing issue makesJSON.stringifythrow and the handler answers500 {"error":"Terjadi kesalahan pada server."}where the raw pass-through answered a field-named 400 — measured at this head. Not reachable through the app's own schemas today (noz.bigint(), no bigint column), which is why this is not a blocker. Fix:JSON.stringify(err.issues, (_k, v) => (typeof v === "bigint" ? v.toString() : v), 2)— I ran it: bigint case back to 400 with bytes identical to the raw v4 output, suite still 45/45.apps/management/app/api/[[...route]]/route.ts:L73: nit: "v4'smessagegetter" is the same mistake7877b16existed to correct, one property along. Measured on a parse-thrown v4 error:messageis{"enumerable":true,"writable":true,"hasGet":false}— an own data property, assigned byinst.message = …in the initializer, not a getter.issuesis the non-enumerable one.route.test.ts:L397repeats the same wording. Say "ownmessageproperty, assigned asJSON.stringify(issues, jsonStringifyReplacer, 2)" in both and the sentence becomes true and the reason for the fix above at the same time.apps/management/app/api/[[...route]]/route.test.ts:L416: nit: both new tests buildexpectedasJSON.stringify(issues, null, 2), the same expression the handler runs, so they cannot detect an encoding divergence from v4 — I swapped the handler's replacer and the suite stayed at 45/45. They pin the cross-major shape well; if you want them to pin the encoding too, deriveexpectedfrom a real v4ZodError's ownmessagerather than recomputing it.PR body: nit: still260 passed(283 at this head) and still "non-enumerable getter". You are right that these are body-only and never reach git — fold them into the same push as the replacer fix rather than spending a round. Add the whatsapp non-webhook-routes line you agreed to while you are in there.Disclosure
Worked in
/tmp/w2hand/tmp/w2b, both removed and pruned. Default checkout shows onlyM .omp/mcp.json;stash@{0}untouched. Postgres is still not running here, so I opened no database connection except to a deliberately closed loopback port (127.0.0.1:7999) and took nomd5or counts — I am relying on the dispatcher's baseline verification, not repeating it as my own. Everyroute.tsmutation above was applied inside my own worktree and restored;git statusclean after each. No fixture started, no port bound, nonode_modulespatched this round.VERDICT: approve-with-nits
Round 3,
30b89bc. Review 61 superseded by sequencing. 0 blockers, 1 should, 1 nit. Both of round 2's items are genuinely fixed and I confirmed them against real errors from both majors. Theshouldis the premise underneath them, and part of it is mine.1. The bigint claim survives real errors from both majors
Your fixture was right in substance, but you were right to want it attacked. I threw genuine bigint-bearing errors from the real classes —
z.bigint().max(BigInt(10)).parse(BigInt(20))on v4, and the same on the realzod@3.25.76loaded by absolute path so it cannot dedupe:Non-bigint controls from the real classes too:
v4 plain handler === raw pass-through :: true, v3 plain lands in the same envelope. So the encoding now matches v4 exactly, on the real class, for both the bigint and the ordinary path. The claim holds.Your negative controls reproduce, and one is stronger than you said:
2. Both zod majors are NOT still reachable — and they were not when I told you they were
This is the finding, and I have to lead with my own part in it. In review 59 I wrote that the second copy is "zod@3.25.76, pulled by
@modelcontextprotocol/sdk@1.29.0, whose router is mounted on this same app". I read that off apnpm-lock.yamlline and did not resolve the link. You took my word, and it is now the durable comment inroute.tsand the doc comment on the test. It is wrong in exactly the way the@hono/zod-openapiclaim it replaced was wrong. Third iteration of one mistake: naming a plausible second-copy route without resolving it on disk.Resolved from the actual module graph this time, starting at the file that mounts the SDK:
There are two peer-resolved SDK instances in the store. The app links the zod@4.4.3 one. The
zod@3.25.76instance is real but lives undershadcn@4.13.0's private tree —apps/management/package.json:53, a CLI, andgrepfinds noimport … from "shadcn"anywhere inlib/orapp/.pnpm-lock.yaml:6493is theshadcnentry that pulls@modelcontextprotocol/sdk@1.29.0(zod@3.25.76); the app never reaches it.So: no second zod major is reachable from the request path. Nothing collapsed between rounds — it was never true.
This does not make the normalisation wrong, and I am not asking you to revert it. It is still the right code for a reason that is actually live: the catch-all exists precisely to catch throws from callers this module does not control,
isZodErroraccepts on duck-type, and a hand-constructed or foreign-shaped error is reachable today in a way a second zod major is not. Building the envelope fromissuesmakes the published body independent of whoever threw. Keep the code; fix the story. Concretely — say the guard is against any error that duck-types as aZodError(which is whatisZodErroractually accepts), notezod@3.25.76exists in the tree undershadcnbut is not on the request path, and drop "this app mounts TWO zod majors", because it does not.The two
across zod majorstests are fine as behaviour pins and I would keep them — they assert the handler is indifferent to the two serialisation shapes, which is true and useful. Just rename the premise: they are cross-shape, not cross-major.3. Leaving
whatsapp.ts:2alone is still rightorigin/main'swhatsapp.tsline 2 isimport { Hono, type Context } from "hono";— the finding is on main, not introduced here, andgit difftouches no import line. Sorting it would put formatter drift in a diff that otherwise carries none, for a file whose only change is a comment. Confirmed: leave it.One precision on your status line.
biome checkon the two files this push touched is clean, exit 0. On the PR's full four-file changed set it is stillFound 1 error, exit 1. Both true, different scopes — worth saying which, so nobody records "biome clean" as a property of the PR and stops checking.Re-verified at this head
Not re-derived, as you asked: field-path preservation, 400-not-500, the
isAPIError401/403 reasoning, and leaving@hono/zod-openapi's hook return unreshaped. All settled in reviews 59 and 61 and untouched by this diff.Findings
apps/management/app/api/[[...route]]/route.ts:L80: should: "The real second copy is zod@3.25.76 reached through@modelcontextprotocol/sdk, whosemcpRouteris mounted on this very app" is false, and so is L77's "this app mounts TWO zod majors". Resolved from the module graph:lib/mcp/server.tsloads the SDK instance peer-resolved against zod@4.4.3, that SDK's ownzodresolves to the samezod@4.4.3/index.cjsthe app uses, and the onlyzodlink underapps/management/node_modulesiszod@4.4.3.zod@3.25.76reaches the tree throughshadcn@4.13.0(apps/management/package.json:53,pnpm-lock.yaml:6493), a CLI no app file imports. Origin of the error is review 59, mine — I named the route from a lockfile line without resolving the link. Keep the normalisation; it is justified byisZodError's duck-type acceptance, which is live. Rewrite L77-L82 to say the guard covers any error duck-typing as aZodError, and notezod@3.25.76is present undershadcnbut off the request path. Same sentence atroute.test.ts:L393.PR body / status: nit: "biome clean on the changed files" holds for the two files this push touched (exit 0) but not for the PR's four-file changed set, which is stillFound 1 erroratwhatsapp.ts:2. State the scope, since the whole point of naming it pre-existing was that the next reader keeps checking rather than inherits "clean".Disclosure
Worked in
/tmp/w3, removed and pruned. Default checkout shows onlyM .omp/mcp.json;stash@{0}untouched. Postgres still not running here; opened no database connection except a deliberately closed loopback port (127.0.0.1:7999); took nomd5or counts. Bothroute.tsmutations applied inside my own worktree and restored,git statusclean after each. No fixture started, no port bound, nonode_modulespatched this round.Review 66: approve-with-nits, 0 blockers, 1 should. The should is that my own replacement premise was wrong, and it was the third revision of this comment to assert a resolution nobody had resolved. Review 59 correctly disproved the original claim — that `@hono/zod-openapi` was a second zod copy, so the envelope could not drift. I replaced it with "`zod@3.25.76` reaches this app through `@modelcontextprotocol/sdk`, whose `mcpRouter` is mounted on this very app", taken off a pnpm-lock line without following the link. That is false. Verified from the module graph rather than the lockfile: apps/management/node_modules/zod -> zod@4.4.3 (the only zod link) @modelcontextprotocol/sdk/node_modules -> no nested zod; peer-resolves 4.4.3 zod@3.25.76 -> under shadcn (package.json:53), a CLI no app file imports So no second zod major is on the request path. Nothing collapsed — it was never true. **The code is unchanged, because it is right for a reason that IS live.** `isZodError` accepts on duck-type and this handler is the catch-all for throws the module does not control, so building the envelope from `issues` makes the body independent of whoever threw. The guard is cross-SHAPE, not cross-major. Both cross-shape tests keep their value under the honest premise and are renamed rather than deleted: a `ZodError`-shaped object from anywhere keeps `issues` enumerable with no own `message`, and passed raw that emits `{"error":{"issues":[…],"name":"ZodError"}}` — issues present, message absent, not the envelope PR #68 established. The test names already said "v4-shaped" and "v3-shaped"; only the section premise claimed provenance it could not support. The reviewer's independent check of the bigint fix, which this push does not change: a real `z.bigint().max(BigInt(10))` failure from the genuine v4 class answers 400 with `typeof issues[0].maximum === "bigint"`, and the handler's bytes are identical to the raw v4 pass-through. Controls reproduce — dropping the replacer fails the bigint test, passing raw fails 3. **Scope correction on my own reporting** (nit): I have been writing "biome clean" without saying over what. Precisely, at this head: the 2 files this push touches -> Checked 2 files, no findings, exit 0 the PR's full 4-file set -> Found 1 error That one error is the pre-existing `organizeImports` finding at `whatsapp.ts:2`, present on `origin/main`, on no import line this PR touches, and deliberately left alone rather than widening the diff — confirmed as the right call. Stating the scope so "biome clean" stops reading as a property of the PR. 284 tests, tsc exit 0.