fix(msteams): use formatUnknownError instead of String(err) for error logging#59321
Conversation
Greptile SummaryThis PR replaces
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler/message-handler.ts
Line: 551
Comment:
**User-facing message may expose verbose JSON for non-Error thrown objects**
The previous code used `err instanceof Error ? err.message : String(err)`, which for non-`Error` thrown objects (plain objects, Axios-like shapes) produced the unhelpful `[object Object]`. The new `formatUnknownError(err)` improves on that — but for non-`Error` objects it falls through to `JSON.stringify(err)`, which could produce a large JSON blob sent directly to end users in Teams.
For internal log calls (e.g. `log.error`, `runtime.error`) this is fine — full detail is desirable in logs. For the `context.sendActivity` user-facing message, you may want a tighter cap:
```suggestion
await context.sendActivity(`⚠️ Agent failed: ${err instanceof Error ? err.message : formatUnknownError(err)}`);
```
This preserves the correct `err.message` path for `Error` instances (which covers Axios errors and Bot Framework SDK errors since they extend `Error`) while still using `formatUnknownError` as a fallback for exotic thrown values — limiting what a plain-object throw would surface to the user.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/msteams/src/monitor.ts
Line: 270-271
Comment:
**`formatUnknownError` drops the error class name for `Error` instances**
`formatUnknownError(err)` returns `err.message` for `Error` instances, while the previous `String(err)` called `err.toString()` which produces `"ErrorName: message"` (e.g. `"TypeError: ..."`, `"Error: listen EADDRINUSE :::3000"`).
For this `httpServer.on("error", ...)` callback and the `httpServer.close` callback below, the thrown values are always Node.js `Error` instances. The error _class_ name is often meaningful context in server-level errors (e.g. distinguishing `EADDRINUSE` from `ENOTFOUND` from a generic `Error`). Node.js system errors do typically include the code in `err.message` (e.g. `"listen EADDRINUSE :::`), so in practice the information is usually preserved — but it is worth being aware of this trade-off across all the changed call sites. No action is strictly required, just flagging the change in semantics.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(msteams): use formatUnknownError ins..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await context.sendActivity( | ||
| `⚠️ Agent failed: ${err instanceof Error ? err.message : String(err)}`, | ||
| ); | ||
| await context.sendActivity(`⚠️ Agent failed: ${formatUnknownError(err)}`); |
There was a problem hiding this comment.
The previous code used err instanceof Error ? err.message : String(err), which for non-Error thrown objects (plain objects, Axios-like shapes) produced the unhelpful [object Object]. The new formatUnknownError(err) improves on that — but for non-Error objects it falls through to JSON.stringify(err), which could produce a large JSON blob sent directly to end users in Teams.
For internal log calls (e.g. log.error, runtime.error) this is fine — full detail is desirable in logs. For the context.sendActivity user-facing message, you may want a tighter cap:
| await context.sendActivity(`⚠️ Agent failed: ${formatUnknownError(err)}`); | |
| await context.sendActivity(`⚠️ Agent failed: ${err instanceof Error ? err.message : formatUnknownError(err)}`); |
This preserves the correct err.message path for Error instances (which covers Axios errors and Bot Framework SDK errors since they extend Error) while still using formatUnknownError as a fallback for exotic thrown values — limiting what a plain-object throw would surface to the user.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler/message-handler.ts
Line: 551
Comment:
**User-facing message may expose verbose JSON for non-Error thrown objects**
The previous code used `err instanceof Error ? err.message : String(err)`, which for non-`Error` thrown objects (plain objects, Axios-like shapes) produced the unhelpful `[object Object]`. The new `formatUnknownError(err)` improves on that — but for non-`Error` objects it falls through to `JSON.stringify(err)`, which could produce a large JSON blob sent directly to end users in Teams.
For internal log calls (e.g. `log.error`, `runtime.error`) this is fine — full detail is desirable in logs. For the `context.sendActivity` user-facing message, you may want a tighter cap:
```suggestion
await context.sendActivity(`⚠️ Agent failed: ${err instanceof Error ? err.message : formatUnknownError(err)}`);
```
This preserves the correct `err.message` path for `Error` instances (which covers Axios errors and Bot Framework SDK errors since they extend `Error`) while still using `formatUnknownError` as a fallback for exotic thrown values — limiting what a plain-object throw would surface to the user.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| httpServer.on("error", (err) => { |
There was a problem hiding this comment.
formatUnknownError drops the error class name for Error instances
formatUnknownError(err) returns err.message for Error instances, while the previous String(err) called err.toString() which produces "ErrorName: message" (e.g. "TypeError: ...", "Error: listen EADDRINUSE :::3000").
For this httpServer.on("error", ...) callback and the httpServer.close callback below, the thrown values are always Node.js Error instances. The error class name is often meaningful context in server-level errors (e.g. distinguishing EADDRINUSE from ENOTFOUND from a generic Error). Node.js system errors do typically include the code in err.message (e.g. "listen EADDRINUSE :::), so in practice the information is usually preserved — but it is worth being aware of this trade-off across all the changed call sites. No action is strictly required, just flagging the change in semantics.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/monitor.ts
Line: 270-271
Comment:
**`formatUnknownError` drops the error class name for `Error` instances**
`formatUnknownError(err)` returns `err.message` for `Error` instances, while the previous `String(err)` called `err.toString()` which produces `"ErrorName: message"` (e.g. `"TypeError: ..."`, `"Error: listen EADDRINUSE :::3000"`).
For this `httpServer.on("error", ...)` callback and the `httpServer.close` callback below, the thrown values are always Node.js `Error` instances. The error _class_ name is often meaningful context in server-level errors (e.g. distinguishing `EADDRINUSE` from `ENOTFOUND` from a generic `Error`). Node.js system errors do typically include the code in `err.message` (e.g. `"listen EADDRINUSE :::`), so in practice the information is usually preserved — but it is worth being aware of this trade-off across all the changed call sites. No action is strictly required, just flagging the change in semantics.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
This PR improves MS Teams extension error logging by replacing String(err) with the existing formatUnknownError() helper, preventing unhelpful [object Object] messages when non-Error values are thrown/caught.
Changes:
- Use
formatUnknownError()in onboarding flow error notes. - Use
formatUnknownError()for provider/server and handler error logs in the monitor runtime. - Use
formatUnknownError()in message dispatch/session-meta error paths and resolve flows.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
extensions/msteams/src/onboarding.ts |
Improves channel lookup failure messaging by formatting unknown error types. |
extensions/msteams/src/monitor.ts |
Formats provider resolve/server lifecycle errors for more actionable logs. |
extensions/msteams/src/monitor-handler/message-handler.ts |
Formats dispatch/session recording errors to avoid [object Object] in logs and user-facing failure messages. |
extensions/msteams/src/monitor-handler.ts |
Formats handler + file upload failure errors consistently in logs and activity replies. |
extensions/msteams/src/channel.ts |
Formats resolve failures when building directory/allowlist results. |
| log.debug?.("file upload failed", { uploadId, error: formatUnknownError(err) }); | ||
| await context.sendActivity(`File upload failed: ${formatUnknownError(err)}`); |
There was a problem hiding this comment.
formatUnknownError(err) is called twice here (structured log + user-facing message). Consider computing it once and reusing the value so the formatting is consistent and you avoid double JSON.stringify work for non-Error objects.
| log.debug?.("file upload failed", { uploadId, error: formatUnknownError(err) }); | |
| await context.sendActivity(`File upload failed: ${formatUnknownError(err)}`); | |
| const formattedError = formatUnknownError(err); | |
| log.debug?.("file upload failed", { uploadId, error: formattedError }); | |
| await context.sendActivity(`File upload failed: ${formattedError}`); |
| } catch (err) { | ||
| log.error("dispatch failed", { error: String(err) }); | ||
| runtime.error?.(`msteams dispatch failed: ${String(err)}`); | ||
| log.error("dispatch failed", { error: formatUnknownError(err) }); | ||
| runtime.error?.(`msteams dispatch failed: ${formatUnknownError(err)}`); | ||
| try { | ||
| await context.sendActivity( | ||
| `⚠️ Agent failed: ${err instanceof Error ? err.message : String(err)}`, | ||
| ); | ||
| await context.sendActivity(`⚠️ Agent failed: ${formatUnknownError(err)}`); |
There was a problem hiding this comment.
formatUnknownError(err) is computed 3 times in this catch block (log, runtime error, and user-facing activity). Consider computing it once (e.g., const errMsg = formatUnknownError(err)) and reusing it to avoid repeated JSON.stringify work for non-Error objects and keep the output consistent across all three call sites.
… logging Replace String(err) with the existing formatUnknownError() utility across the msteams extension to prevent [object Object] in error logs when non-Error objects are thrown (e.g., Axios errors, Bot Framework SDK errors). Fixes openclaw#53910
BradGroux
force-pushed
the
fix/msteams-error-stringification
branch
from
d2bd489 to
68e4ff1
Compare
Replace `String(err)` with `formatErrorMessage()` from plugin-sdk in feishu monitor.account.ts. `String(err)` produces `[object Object]` for non-Error thrown values (e.g., Lark SDK error objects), hiding the actual error details. `formatErrorMessage()` traverses the `.cause` chain and handles all error types properly. This follows the same pattern applied to msteams in openclaw#59321.
… logging (openclaw#59321) Replaces String(err) with the existing formatUnknownError() utility across the msteams extension to prevent [object Object] appearing in error logs when non-Error objects are caught (e.g., Axios errors, Bot Framework SDK error objects). Fixes openclaw#53910 thanks @BradGroux
… logging (openclaw#59321) Replaces String(err) with the existing formatUnknownError() utility across the msteams extension to prevent [object Object] appearing in error logs when non-Error objects are caught (e.g., Axios errors, Bot Framework SDK error objects). Fixes openclaw#53910 thanks @BradGroux
Replaces
String(err)with the existingformatUnknownError()utility across the msteams extension. This prevents[object Object]appearing in error logs when non-Error objects are caught (e.g., Axios errors, Bot Framework SDK error objects).The
formatUnknownError()function already exists inextensions/msteams/src/errors.tsand properly handles Error instances, strings, nulls, and arbitrary objects.Fixes #53910