Skip to content

fix(web): classify rejected inputValidator requests as 400, not 500 - #4587

Open
geclos wants to merge 3 commits into
developmentfrom
claude/gallant-maxwell-hzufr2
Open

fix(web): classify rejected inputValidator requests as 400, not 500#4587
geclos wants to merge 3 commits into
developmentfrom
claude/gallant-maxwell-hzufr2

Conversation

@geclos

@geclos geclos commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a false-positive noise source in Datadog Error Tracking: server functions that reject via .inputValidator() (zod) are recorded as 500 server faults instead of expected 400 client errors.

Datadog issue addressed: 786d9ad4-8069-11f1-ab43-da7ad0900005web service, error type Error, 240 occurrences over the last 7 days (recurring since first seen ~54 days ago), most recently 2026-09-07T04:18 UTC. Sampled from completeOnboarding's organizationName validation failing with "Please enter an organization name".

Root cause: apps/web/src/middlewares/server-fn-error.ts has a deliberate isExpectedClientError filter so 4xx-class errors (bad input, no access, not logged in) are excluded from Error Tracking, keeping the signal focused on real 5xx bugs. It classifies via errorStatus(), which only recognized HttpError-shaped exceptions (objects with httpStatus/httpMessage).

TanStack Start's execValidator (@tanstack/start-client-core) does not throw an HttpError or ZodError when a Standard Schema inputValidator rejects input — it throws a plain Error whose message is JSON.stringify(issues):

if (result.issues) throw new Error(JSON.stringify(result.issues, void 0, 2));

errorStatus() didn't recognize this shape, so it fell through to the 500 default, and every rejected .inputValidator() call across the app (315+ call sites) got recorded into Datadog as a false server fault — exactly the noise the filter was built to prevent. The client already handles this gracefully via extractFieldErrors/createFormSubmitHandler (inline field errors, no raw error surfaced to the user), so this is purely an observability bug, not a user-facing one.

This isn't specific to completeOnboardingerrorStatus() is the single shared classification point for every server function using .inputValidator(), so the fix resolves the false-positive app-wide, not just for the observed call site.

Note on prior art: an earlier draft PR (#4488) proposed the identical fix back on 2026-08-21 but was never merged, so the issue kept accumulating occurrences for another ~17 days (74 → 240). This PR carries the same fix, independently re-verified against the current development HEAD, with tests re-run rather than assumed. Recommend closing #4488 as superseded once this merges.

Fix: duck-type the Standard Schema issues array shape ({ message: string, path: array }[]) in errorStatus() and classify it as 400, the same way NotFoundError/UnauthorizedError are already excluded. Also routed recordServerFnError's status through the shared errorStatus() helper instead of a narrower inline httpError ? e.httpStatus : 500 check, so both call sites (recordServerFnError and recordRequestError) stay in sync.

No behavior change for users: the client-bound error message is untouched, so extractFieldErrors continues to parse field errors identically. This only changes what gets reported to Datadog (skipped) and the log level (warn instead of error).

Related issue (if applicable)

N/A — found via scheduled Datadog Error Tracking triage, not a filed issue.

How was this tested?

  • Added two unit tests to server-fn-error.test.ts:
    • A rejected-validator-shaped Error is classified 400/isClientError: true and not recorded to the span, while the client-bound message still round-trips correctly for extractFieldErrors.
    • A JSON-array error message that doesn't match the Standard Schema issue shape is still correctly reported as 500 (no over-broad matching).
  • Confirmed the new tests fail without the fix (reverted the source change, reran) and pass with it.
  • pnpm --filter @app/web test — full suite green except 2 pre-existing, unrelated failures in phone-countries.test.ts (ICU timezone data differences in this sandbox; untouched by this change).
  • pnpm --filter @app/web typecheck (tsgo) — clean.
  • pnpm exec biome check on changed files — clean.

Verification after deploy: occurrences of issue 786d9ad4-8069-11f1-ab43-da7ad0900005 (and the equivalent pattern on any other .inputValidator()-guarded server function) should stop appearing in Error Tracking going forward, since new occurrences will be classified as 400 at ingestion.

Checklist

  • Lint, type-checking, and tests pass locally
  • PR title follows Conventional Commits
  • I have signed the CLA

🤖 Generated with Claude Code

https://claude.ai/code/session_01KGQjNxrRnKpLJ22HRJTgpB


Generated by Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

TanStack Start's execValidator throws a plain Error whose message is the
JSON-encoded Standard Schema issues array when a .inputValidator() (zod)
rejects input, instead of an HttpError/ZodError. errorStatus() only
recognized HttpError-shaped exceptions, so every rejected .inputValidator()
call across the app fell through to the 500 default and was recorded as a
false server fault in Datadog Error Tracking, defeating the
isExpectedClientError filter built specifically to keep this noise out.

Duck-type the Standard Schema issues shape in errorStatus() and classify
it as 400, and route recordServerFnError's status through the shared
errorStatus() helper instead of a narrower inline check.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGQjNxrRnKpLJ22HRJTgpB
@vercel

vercel Bot commented Sep 7, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
design-system Skipped Skipped Sep 14, 2026 8:32pm UTC

Request Review

@geclos
geclos marked this pull request as ready for review September 14, 2026 20:19

geclos commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator Author

Scheduled Datadog triage (2026-09-14): this fix is still correct and the underlying Datadog issue (786d9ad4-8069-11f1-ab43-da7ad0900005) keeps recurring while this sits unmerged — most recent occurrence today at 06:09 UTC, ~250+ total now. All CI checks are green (test-unit, test-integration, typecheck, check, knip, CodeQL) and the base is current. Marked ready for review — no new code change needed, this just needs a merge.

Once merged, #4488 can be closed as superseded (same fix, opened first, never merged).


Generated by Claude Code

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-14T20:23:44.333923Z 57a78cd Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Comment thread apps/web/src/middlewares/server-fn-error.ts Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Review summary

Reviewed the diff (apps/web/src/middlewares/server-fn-error.ts + its test file) against the stated goal: classify TanStack's rejected-inputValidator Error as 400 instead of the 500 default in errorStatus().

What I checked:

  • Correctness of the new isInputValidationError/isStandardSchemaIssue duck-typing and its placement in errorStatus().
  • Whether this changes the actual HTTP response status sent to clients (it doesn't — recordServerFnError/recordRequestError only gate Datadog reporting and log level; the wire-level response for a thrown Error is unaffected here, so the "no user-facing behavior change" claim in the PR body holds).
  • Consistency with the existing 4xx-exclusion pattern (isHttpError, isMissingServerFnError → 404) and that recordServerFnError now delegates to the shared errorStatus() instead of its own narrower inline check.
  • New/updated tests: both new cases (validator-shaped error → 400/not reported; look-alike JSON array → still 500/reported) are meaningful and would fail without the fix.
  • No tenancy/authz surface here — this is purely an observability classification helper, org-scoped concerns don't apply.

Findings: one inline note (non-blocking) — isStandardSchemaIssue requires path to be an array on every issue, but the type declares path? as optional per the Standard Schema spec (a root-level issue may omit path). Zod always populates path so this PR's fix and tests are unaffected, but as written the helper wouldn't catch that edge case for other Standard-Schema-compliant validators. Suggested a one-line tweak to accept a missing path.

No correctness, security, or architecture-boundary issues found otherwise. The change is narrowly scoped, well-tested, and matches the file's existing "classify status, gate on it" pattern.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57a78cd7d1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread apps/web/src/middlewares/server-fn-error.ts
The Standard Schema spec allows an issue to omit path entirely for a
root-level error, not just supply an empty array. isStandardSchemaIssue
required Array.isArray(path) unconditionally, so such an issue fell
through the duck-type check to the 500 default — the exact
misclassification this PR fixes, for that case.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGQjNxrRnKpLJ22HRJTgpB
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

📄 Generated an HTML diff-review artifact explaining this PR's change (background, intuition, code walkthrough, and a 5-question quiz).

🤖 Generated with Claude Code

…uest

ZodError.message is JSON.stringify(issues, ..., 2) too, since it subclasses
Error, so the duck-typed check matched any schema.parse() failure anywhere
in a handler (e.g. a repository mapper validating a persisted row) as an
expected 400 client error and hid it from Datadog. TanStack's execValidator
throws a literal `new Error(...)`, not a subclass, so require an exact
Error constructor match to exclude ZodError and other Error subclasses.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGQjNxrRnKpLJ22HRJTgpB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Inbox

Development

Successfully merging this pull request may close these issues.

2 participants