refactor([shared-tolerant-fetch-json-helper]): share a tolerant Response→JSON parser across fetch clients#614
Merged
Conversation
…nse→JSON parser across fetch clients
Promote cosRunnerClient's local readRunnerJson into a shared
server/lib/readResponseJson.js: reads the body via text() and parses
tolerantly via safeJSONParse so a non-JSON/HTML error page no longer
crashes with 'Unexpected token <'. Object callers need no options
(non-JSON falls back to {}, so existing data.foo || [] defaults take
over instead of throwing on null.foo); array callers pass
{ fallback: [], emptyValue: [] }; the function-fallback form surfaces
the raw error text (cosRunnerClient keeps its { error: text } shape).
Migrated the unguarded await response.json() crash sites in
aiProvider, browserService, memoryEmbeddings, huggingFaceCatalog,
messageTokenExtractor, and ollamaManager. aiToolkit/providers.js is
intentionally left alone — it must stay self-contained (no imports out
to PortOS modules per CLAUDE.md) and its .catch(() => ({ data: [] }))
band-aids are already non-crashing.
…fix barrel order, defer remaining clients
/simplify cleanup: hoist the per-call Symbol('parse-failed') sentinel to
module scope; move the readResponseJson barrel export into alphabetical
position (after peerUrl); capture the additional unguarded response.json()
clients surfaced by the reuse review as a follow-up PLAN item.
…ings on malformed-body handling PR #614 review (codex/agy/claude): - aiProvider: a non-JSON 200 body now surfaces an error instead of an empty-but-successful { text: '' } (codex HIGH — don't let a malformed body masquerade as a valid empty completion). - browserService getCdpVersion + findOrOpenPage: pass { fallback: null, emptyValue: null } so a malformed body stays falsy and the /version route's !version -> 503 guard still fires, instead of a truthy {} (agy). - readResponseJson: emptyValue now defaults to a plain-value fallback, so array callers need only { fallback: [] }; a function fallback still maps a blank body to {}. Trim before the empty check so a whitespace-only body is treated as empty, not a parse failure (agy). JSDoc notes the object/array-body expectation and the null-contract pattern (claude). - PLAN.md: expanded the deferred follow-up with civitai.js and telegramClient.js sites found in the reuse sweep.
…a malformed CDP body PR #614 iteration 2 (codex HIGH): navigateToUrl returned a truthy {} with undefined id/url on a malformed 200 body, which a caller could mistake for a successful navigation. Mirror the getCdpVersion/findOrOpenPage fix — read with a null fallback and throw a CDP malformed-body error when the target has no id, consistent with the existing !ok throw.
…-fetch-json-helper # Conflicts: # .changelog/NEXT.md # PLAN.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Promotes
cosRunnerClient.js's localreadRunnerJsoninto a sharedserver/lib/readResponseJson.jsand migrates the fetch-based clients that were doing unguardedawait response.json()off that crash path. A non-JSON / HTML error body (a 500 while a backend restarts, a proxy/captive-portal error) no longer throwsUnexpected token <— which, since most of these callers run outside the Express request lifecycle, would crash the Node process.readResponseJson(response, { fallback, emptyValue }):text()and parses tolerantly throughsafeJSONParse;emptyValue(default{}); a non-JSON body returnsfallback(default{});fallbackmay be a function(rawText) => valueto surface the raw error text (cosRunnerClientkeeps its{ error: text }shape);{}, so existingdata.foo || []defaults take over instead of throwing onnull.foo); array-shaped callers pass{ fallback: [], emptyValue: [] }.Migrated the unguarded sites in
aiProvider,browserService,memoryEmbeddings,huggingFaceCatalog,messageTokenExtractor, andollamaManager. Registered in theserver/libbarrel + README.server/lib/aiToolkit/providers.jsis intentionally left untouched — it must stay self-contained (no imports out to PortOS modules, per CLAUDE.md), and its.catch(() => ({ data: [] }))band-aids are already non-crashing. A reuse-review sweep found a handful of additional fetch clients with the same pattern; those are captured as a scoped follow-up item ([tolerant-fetch-json-additional-clients]) in PLAN.md rather than expanding this PR beyond its stated file list.Test plan
server/lib/readResponseJson.test.jscovers valid object/array bodies, empty body (default + customemptyValue), non-JSON body (default + array fallback), function-fallback receiving raw text, and that the function fallback is not invoked on success or empty body.ollamaManager.test.js'sversionResponsestub to exposetext()(a realResponsehas bothjson()andtext()) now thatollamaRequestreads via the tolerant helper.browserService,ollamaManager,huggingFaceCatalog,aiProvider,cosRunnerClient) pass.