fix: prevent validation errors from being classified as network errors

[awhite0030]

Description

Brief description of what this PR does

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Changeset

• Added a changeset (pnpm changeset) describing this change for the changelog

Docs-only or internal chores need no changeset (or run pnpm changeset --empty to note that intentionally).

Testing

Automated Tests

• New features include passing tests in .spec.ts/tsx files
• All existing tests pass (pnpm test:all completes successfully)
• Tests cover both success and error scenarios

Manual Testing

• Tested with Ollama
• Tested with OpenRouter
• Tested with OpenAI-compatible API
• Tested MCP integration (if applicable)

Checklist

• If this was for an open issue, I was assigned to it
• Code follows project style guidelines
• Self-review completed
• Documentation updated (if needed)
• No breaking changes (or clearly documented)
• Appropriate logging added using structured logging (see CONTRIBUTING.md)

---

Root cause: Error classification in isNetworkError relies on simplistic .includes() checks (like includes('connection')), which causes standard validation errors (e.g. "validation failed for connection") to be misclassified as network errors.
Fix: Updated isNetworkError and isValidationError to use case-insensitive substring matching on error.message. Crucially, we now prevent an error from being flagged as a network error if it is already classified as a validation error (unless its name explicitly denotes it as a Fetch/Network error). Added regression tests to ensure standard error checks (like 'fetch failed' or 'invalid email') continue to function correctly.
Validation: Ran pnpm run build, pnpm test:format, pnpm test:lint, pnpm test:types, pnpm test:knip, and targeted pnpm test:ava source/utils/error-formatter.spec.ts. All passed.

Fixes #1136

Fixes #1136

[github-actions]

nc-review: needs work — 1 blocking, 1 important, 1 nit

@awhite0030 — there is a blocking item below.

The PR correctly fixes the misclassification of validation errors as network errors in source/utils/error-formatter.ts by short-circuiting isNetworkError when isValidationError matches (unless the error name is explicitly FetchError or NetworkError), and adds sensible regression tests. The changeset is present and well-formed. However, this is a near-verbatim duplicate of open PR #1155 ('fix(utils): classify validation errors strictly over network substring heuristics') which addresses the same linked bug; one should be closed in favour of the other.

Possible duplicate of #1155 — worth checking before going further.

🔴 blocking · duplicate · source/utils/error-formatter.ts:142

Open PR #1155 (fix/error-formatter-strict-validation, title 'fix(utils): classify validation errors strictly over network substring heuristics') targets the same issue (#1136) and proposes the same fix shape: route validation-classification ahead of the network substring heuristics so an error like 'validation failed for connection' stops being flagged as a network error. Two PRs solving one bug in the same file is the canonical duplicate. One needs to be closed in favour of the other; otherwise both reviewers will duplicate effort and whichever lands second has a noisy merge.

🟠 important · correctness · source/utils/error-formatter.ts:167

The new guard correctly handles the reported case ('validation failed for connection' must not match network), but isValidationError already returns true for any message containing the word required (and the pre-existing invalid and validation substrings too). A real network error whose message happens to contain 'required' — e.g. 'required authentication header missing on remote' or any future API provider that surfaces 'required' in a transport-failure message — will now be silently classified as validation rather than network, because the new guard suppresses it before the msg.includes('network' | 'fetch' | 'connection') check ever runs. The pre-existing isValidationError is a substring heuristic; layering another substring heuristic on top of it compounds the same class of bug this PR is trying to fix. A tighter guard would key off the explicit error.name ('ValidationError' / 'ZodError') or off a more specific phrase like 'validation failed' rather than the loose includes('validation' | 'invalid' | 'required') check.

⚪ nit · tests · source/utils/error-formatter.spec.ts:244

The 'retains original capability to recognize timeout exceeded' test sets t.true(result.isTimeoutError) but does not assert the negative: a message like 'request timeout exceeded during validation' would now be isTimeoutError=true and isValidationError=true simultaneously, which is fine for this PR but worth a paired assertion (t.false(result.isValidationError) only if you intend timeout to win) or an explicit comment on the intended priority. As written, the test would still pass if timeout and validation both flip true on the same message.

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}

[will-lamerton]

Can you take a look at the bot comments please @awhite0030 :)

[github-actions]

Hi @awhite0030, thanks for this PR! It looks like a maintainer has left feedback
or review activity and there are still some outstanding items to wrap up.

Whenever you get a chance, could you take a look at the open comments?
If anything is unclear or you'd like a hand, just reply here and we'll help you get it across the line.