fix: safely handle non-Error thrown values in StoreBackend#418
Conversation
Replace `catch (e: any) { e.message }` with `catch (e: unknown)` and
an `instanceof Error` check. If the caught value is not an Error
instance (e.g., a thrown string or object), accessing `.message`
returns `undefined`, producing unhelpful error messages like
"Error: undefined". The fix safely stringifies any thrown value.
|
There was a problem hiding this comment.
Pull request overview
This PR hardens StoreBackend error handling so that thrown non-Error values (e.g., strings) don’t lead to undefined error messages or unsafe property access, improving the reliability of backend error responses.
Changes:
- Replaces
catch (e: any)withcatch (e: unknown)inStoreBackend.read()andStoreBackend.edit(). - Guards
.messageaccess withe instanceof Error, falling back toString(e).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (e: unknown) { | ||
| return { error: e instanceof Error ? e.message : String(e) }; |
There was a problem hiding this comment.
For non-Error thrown values, String(e) will produce unhelpful output for objects (e.g., throw { foo: 1 } becomes "[object Object]"). If the intent is to preserve a meaningful message for thrown objects, consider handling common "error-like" shapes (e.g., object with a string message property) and/or providing a clearer fallback for null/undefined (e.g., "Unknown error").
| } catch (e: any) { | ||
| return { error: e.message }; | ||
| } catch (e: unknown) { | ||
| return { error: e instanceof Error ? e.message : String(e) }; |
There was a problem hiding this comment.
The new catch-handling behavior isn’t covered by existing unit tests. Consider adding tests that simulate read() failing with non-Error thrown values (e.g., a thrown string/number/object) to verify the returned error string is stable and not undefined.
| return { error: e instanceof Error ? e.message : String(e) }; | |
| let message: string | undefined; | |
| if (e instanceof Error) { | |
| message = e.message; | |
| } else if (typeof e === "string") { | |
| message = e; | |
| } else if ( | |
| e !== null && | |
| typeof e === "object" && | |
| "message" in e && | |
| typeof (e as { message?: unknown }).message === "string" | |
| ) { | |
| message = (e as { message: string }).message; | |
| } else if ( | |
| typeof e === "number" || | |
| typeof e === "boolean" || | |
| typeof e === "bigint" || | |
| typeof e === "symbol" | |
| ) { | |
| message = String(e); | |
| } | |
| return { | |
| error: message && message.length > 0 | |
| ? message | |
| : "Unknown error while reading file", | |
| }; |
| const storeValue = this.convertFileDataToStoreValue(newFileData); | ||
| await store.put(namespace, filePath, storeValue); | ||
| return { path: filePath, filesUpdate: null, occurrences: occurrences }; | ||
| } catch (e: any) { | ||
| return { error: `Error: ${e.message}` }; | ||
| } catch (e: unknown) { | ||
| const msg = e instanceof Error ? e.message : String(e); | ||
| return { error: `Error: ${msg}` }; |
There was a problem hiding this comment.
PR description/test plan mention readFileContent / editFileContent, but the changes are in read() and edit(). Please update the PR description (and any related docs) so reviewers and future readers can match the change to the correct methods.
| } catch (e: any) { | ||
| return { error: `Error: ${e.message}` }; | ||
| } catch (e: unknown) { | ||
| const msg = e instanceof Error ? e.message : String(e); |
There was a problem hiding this comment.
Same as above: String(e) will render thrown objects as "[object Object]" and may also produce "undefined"/"null" strings. If you want actionable error text for non-Error throws, consider extracting a string message property when present and using a friendlier fallback for nullish values.
| const msg = e instanceof Error ? e.message : String(e); | |
| const msg = | |
| e instanceof Error | |
| ? e.message | |
| : typeof e === "string" | |
| ? e | |
| : e && typeof (e as any).message === "string" | |
| ? (e as any).message | |
| : "Unknown error"; |
Summary
catch (e: any)withcatch (e: unknown)in twoStoreBackendmethods (readFileContent,editFileContent).messageaccess withe instanceof Errorcheck, falling back toString(e)for non-Error thrown valuesundefinedwhen a non-Error value (string, number, object) is thrownTest plan
StoreBackend.readFileContentandeditFileContentstill pass