MSTeams: harden channel integration and readable focus labels#48659
Closed
hddevteam wants to merge 27 commits intoopenclaw:mainfrom
Closed
MSTeams: harden channel integration and readable focus labels#48659hddevteam wants to merge 27 commits intoopenclaw:mainfrom
hddevteam wants to merge 27 commits intoopenclaw:mainfrom
Conversation
…inlined
Graph API's /hostedContents collection endpoint does not return contentBytes
inline in the response — the field is always null/absent. The previous code
would skip every item with an empty contentBytes, causing all image/file
attachments sent via Teams to be silently dropped.
Fix: when contentBytes is absent but the item has an id, fall back to fetching
the actual bytes via the individual /{id}/$value endpoint, which always returns
the binary content. The content-type header from that response is also used to
set itemContentType for accurate MIME detection downstream.
Verified working against Microsoft Teams (channel messages) with both image
and file attachments.
…already string | undefined)
When a Teams channel message contains a mix of image and html attachments, every() required ALL attachments to be html before trying the Graph path. This caused image attachments to be silently skipped. Use some() so the Graph path is attempted whenever at least one attachment has a text/html content type.
…LIST DM image attachments in Microsoft Teams use a direct download URL on smba.trafficmanager.net (the Bot Framework attachment service, SMBA = Service Messaging Bot Attachments). This domain was absent from the auth token allowlist, so download requests were sent without a Bearer token, resulting in 401 responses and silent download failures. Add "trafficmanager.net" so that Bot Framework tokens are automatically attached to requests targeting this domain.
…lback for channels
Two related fixes for buildMSTeamsGraphMessageUrls:
1. DM/group chat: prefer channelData.chatId (the proper Graph chat GUID)
over conversationId, which may be a Bot Framework conversation ID in
the form "a:xxx" that Graph does not recognise.
2. Channel messages: Teams channel IDs surfaced in channelData.team.id
are sometimes in thread format ("19:xxx@thread.tacv2") rather than a
GUID. The /teams/{guid}/channels/ Graph endpoint rejects these with
400. /chats/{threadId}/messages/ accepts thread-format IDs, so append
/chats/ candidate URLs as a fallback after the /teams/ URLs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- archive-store: tolerate malformed JSONL lines (skip + warn instead of throw) - archive-store: bounded top-N accumulator in searchMessages/searchAttachments avoids O(n) memory with large archives; O(limit) memory instead - channel-cleanup: cache Graph team/channel resolution across archives in one sweep to avoid N× full-tenant Graph scan per archived channel - graph: check content-length header before materialising hosted-content arrayBuffer to prevent large allocations beyond maxBytes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…er context Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ctivities Some Bot Framework clients (e.g. certain MS Teams clients) send activity payloads that contain invalid JSON escape sequences such as bare backslashes followed by characters not defined in RFC 8259 (e.g. \p, \q, \c). The previous `express.json()` middleware uses body-parser's strict JSON parser which throws `SyntaxError: Bad escaped character in JSON` on such payloads. This causes the webhook to return a non-200 response, putting Azure Bot Service into exponential backoff and dropping subsequent messages. This fix replaces `express.json()` with `express.raw()` (raw Buffer body), then adds a two-pass JSON parse middleware: 1. First tries standard JSON.parse on the raw body. 2. If that fails (SyntaxError), attempts to repair the payload by double-escaping bare backslashes: /\\([^"\\/bfnrtu])/g → \\\\$1 3. If the repaired payload parses successfully, logs a warning and continues normally so the activity is processed. 4. If the payload cannot be repaired, responds with HTTP 200 to prevent Azure Bot Service backoff, and logs the error for investigation. Fixes: SyntaxError: Bad escaped character in JSON at position N Tested: conversationUpdate and message activities from MS Teams clients
…iddleware Covers the bug where express.json()'s strict parser throws SyntaxError on Bot Framework activities containing invalid escape sequences (e.g. \\p, \\q), causing Azure Bot Service to enter exponential backoff and drop messages. Tests verify: 1. Original JSON.parse() fails on bad-escape payloads (proving the bug) 2. Two-pass repair middleware recovers such payloads 3. Normal well-formed payloads still work (no regression) 4. Valid escape sequences (\\n \\t \\\\ etc.) are not double-escaped 5. Completely unrecoverable JSON is re-thrown (caller returns HTTP 200)"
Greptile code review (PR comment) identified an edge case in the
repair regex: /\\([^\"\\\/bfnrtu])/g incorrectly matches the second \
of a valid \\\\ pair followed by an invalid-escape char.
Example of the bug:
Input JSON text: \"C:\\\\q\" (valid: literal string C:\\q)
Old regex transforms \\q → \\\\q producing \"C:\\\\\\\\q\" (C:\\\\q) — wrong!
Fix: use alternation so valid \\\\ pairs are consumed first before
the invalid-escape branch can inspect their constituent backslashes.
/(\\\\\\\\)+|\\\\([^\"\\\\\\\/bfnrtu])/g
^^^^^^^^^ consume valid \\\\ pairs (leave unchanged)
^^^^^^^^^^^^^^^^^^^^^^^^^ only then repair lone bare escapes
This ensures that:
\\\\q → unchanged (valid: single literal backslash + q)
\\q → \\\\q (repaired: invalid bare escape)
\\\\\\q → \\\\\\\\q (repaired: two-backslash pair OK, trailing lone \\q repaired)
Also adds a regression test for the consecutive-backslash edge case."
Copilot Reviewer identified that the express mock in monitor.lifecycle.test.ts exported json() but not raw(). Since monitor.ts now uses express.raw() instead of express.json(), the test suite would throw 'express.raw is not a function' at startup. Add raw() as a no-op middleware factory alongside json() in the mock, matching the pattern already used for json().
CI 'Check types and lint and oxfmt' step failed because our new files were not formatted with oxfmt. Run oxfmt --write on both files to fix.
- viewer-client.ts: import RegisteredCustomThemes and implement ensurePierreThemeLoaders to register theme loaders. - monitor.ts: add a newline for better readability before messageHandler definition. - remove outdated openclaw-2026.3.9.tgz file.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Microsoft Teams integration (webhook JSON/media ingestion, threading defaults, and DM “recent channel focus” labeling) and adds a new persistent Teams channel archive extension that can prune archived data when channels are deleted.
Changes:
- Add a new
channel_deletedplugin hook and wire MS TeamsconversationUpdate(channelDeleted) events to it. - Improve MS Teams webhook robustness (JSON escape repair middleware, cross-tenant Graph token usage, improved attachment/media fallback) and default channel replies to threads.
- Introduce
msteams-channel-archiveextension for durable message/attachment archiving plus cleanup service + query tools.
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/wired-hooks-message.test.ts | Adds coverage for runChannelDeleted hook runner behavior. |
| src/plugins/types.ts | Registers new channel_deleted hook type and handler signature. |
| src/plugins/hooks.ts | Adds runChannelDeleted to the hook runner API. |
| src/plugin-sdk/msteams.ts | Exposes additional SDK exports needed by Teams extensions (sessions + global hook runner + service type). |
| src/hooks/message-hook-mappers.ts | Extends canonical inbound context mapping with Teams-specific metadata/media arrays. |
| src/hooks/message-hook-mappers.test.ts | Updates tests for new inbound context fields. |
| src/auto-reply/templating.ts | Adds ProviderMetadata to message context typing for templating/tooling. |
| pnpm-lock.yaml | Adds new workspace importer deps for extensions/msteams-channel-archive. |
| extensions/msteams/src/policy.ts | Adds alternate team id matching + defaults channel reply style to thread. |
| extensions/msteams/src/policy.test.ts | Tests legacy vs alternate team id matching + new reply-style defaults. |
| extensions/msteams/src/monitor.ts | Switches to raw JSON parsing with escape repair; improves error handling/backoff behavior. |
| extensions/msteams/src/monitor.lifecycle.test.ts | Updates express mock to include raw() middleware. |
| extensions/msteams/src/monitor-handler/message-handler.ts | Persists recent channel focus sidecar; resolves readable labels via Graph; enriches ProviderMetadata. |
| extensions/msteams/src/monitor-handler/message-handler.focus.test.ts | Adds focused tests for sidecar persistence, Graph label resolution, and DM injection. |
| extensions/msteams/src/monitor-handler/inbound-media.ts | Forwards conversation tenant id to Graph media fallback; broadens HTML attachment detection. |
| extensions/msteams/src/monitor-handler/inbound-media.test.ts | Adds regression test for tenant forwarding to Graph media download. |
| extensions/msteams/src/monitor-handler.ts | Forwards Teams channelDeleted events to channel_deleted plugin hooks via global hook runner. |
| extensions/msteams/src/monitor-handler.file-consent.test.ts | Adds coverage ensuring channelDeleted events reach hooks; updates mocks for hook runner. |
| extensions/msteams/src/graph.ts | Adds conversation-tenant Graph token support + getTeamById. |
| extensions/msteams/src/conversation-store.ts | Stores additional mutable display label fields + runtime/Graph team ids. |
| extensions/msteams/src/channel-focus-store.ts | New sidecar store for “recent channel focus” metadata keyed by main session. |
| extensions/msteams/src/attachments/shared.ts | Tightens allowlists for Bot Framework attachment hosts. |
| extensions/msteams/src/attachments/html.ts | Avoids counting non-downloadable HTML/card attachments as documents. |
| extensions/msteams/src/attachments/graph.ts | Adds /chats URL fallback, hosted content $value fetching, and cross-tenant token support. |
| extensions/msteams/src/attachments.test.ts | Adds placeholder regression cases for HTML-only attachments. |
| extensions/msteams/src/tests/monitor-json-repair.test.ts | Adds regression tests for JSON escape repair logic. |
| extensions/msteams-channel-archive/src/types.ts | Defines archive index/message/attachment types. |
| extensions/msteams-channel-archive/src/tools.ts | Registers archive query tools with TypeBox schemas. |
| extensions/msteams-channel-archive/src/channel-cleanup.ts | Adds periodic Graph-based “deleted channel” cleanup sweep + pruning. |
| extensions/msteams-channel-archive/src/channel-cleanup.test.ts | Tests pruning rules, failure behavior, and token/team-id caching. |
| extensions/msteams-channel-archive/src/archive-store.ts | Implements on-disk JSONL archive storage + attachment copying + search/prune APIs. |
| extensions/msteams-channel-archive/src/archive-store.test.ts | Tests archiving, dedupe, search, and prune/orphan attachment cleanup. |
| extensions/msteams-channel-archive/package.json | Adds new private extension package manifest. |
| extensions/msteams-channel-archive/openclaw.plugin.json | Declares plugin id/channels/config schema. |
| extensions/msteams-channel-archive/index.ts | Wires hooks (message_received, channel_deleted), tools, and cleanup service. |
| extensions/msteams-channel-archive/index.test.ts | Verifies hook registration and mediaTypes alignment behavior. |
| extensions/diffs/src/viewer-client.ts | Registers Pierre theme loaders via RegisteredCustomThemes. |
| CHANGELOG.md | Documents Teams channel archive + hook runner hardening entries. |
| .github/labeler.yml | Adds labeler rule for the new msteams-channel-archive extension. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comment on lines
+52
to
+55
| export async function resolveGraphToken(params: { | ||
| cfg: unknown; | ||
| conversationTenantId?: string; | ||
| }): Promise<string> { |
|
|
||
| export async function getTeamById(token: string, teamId: string): Promise<GraphGroup | null> { | ||
| const path = `/teams/${encodeURIComponent(teamId)}?$select=id,displayName`; | ||
| return await fetchGraphJson<GraphGroup>({ token, path }); |
Contributor
Greptile SummaryThis is a substantial MS Teams integration hardening PR that stacks several related improvements: channel archive persistence (new Key findings:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/msteams/src/graph.ts
Line: 88-92
Comment:
**`getTeamById` return type is misleading**
The function signature promises `GraphGroup | null`, but since `fetchGraphJson` throws a `new Error(...)` on any non-OK HTTP response (including 404), this function will never actually return `null` — it either returns the group or throws. All call-sites already use try-catch, so this is functionally fine, but the `| null` annotation is dead code and could mislead a future caller into skipping the try-catch wrapper in favour of a null check.
Consider updating to match the real contract:
```suggestion
export async function getTeamById(token: string, teamId: string): Promise<GraphGroup> {
const path = `/teams/${encodeURIComponent(teamId)}?$select=id,displayName`;
return await fetchGraphJson<GraphGroup>({ token, path });
}
```
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/attachments/graph.ts
Line: 307-335
Comment:
**Cross-tenant token acquisition via private SDK internals**
The cast `params.tokenProvider as unknown as { connectionSettings?: ...; getAccessToken(authConfig, scope) }` accesses internal properties and an overloaded signature that are not part of the public `tokenProvider` contract. If the SDK refactors `MsalTokenProvider` internals, this silently falls back to the bot-tenant token (because `_provider.connectionSettings?.clientId` will be `undefined` and the cross-tenant branch is skipped). This is fail-safe behaviour today, but makes the cross-tenant path invisible in test coverage and fragile across SDK upgrades.
The cleaner, SDK-version-proof approach already used elsewhere in this PR is `resolveGraphToken({ cfg, conversationTenantId })` — that function explicitly constructs the right auth config via `sdk.getAuthConfigWithDefaults(...)`. Consider replacing this block with a call to `resolveGraphToken` (or an equivalent helper) so the cross-tenant override doesn't rely on private properties.
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-channel-archive/src/archive-store.ts
Line: 224-234
Comment:
**`console.warn` bypasses the injected logger**
`parseJsonLine` uses `console.warn` directly rather than the `Logger` that is injected via `MSTeamsChannelArchiveStore`. This means malformed JSONL warnings won't flow through the host's configured log handler (level filtering, structured-log sinks, etc.).
One option is to thread the logger through as a parameter. Alternatively, since this is a static parse helper, it can return a sentinel and let the calling method emit the warning through `this.logger`:
```suggestion
function parseJsonLine<T>(line: string): T | null {
try {
return JSON.parse(line) as T;
} catch {
return null;
}
}
```
And in the callers (e.g. `readMessagesFromFile`), emit the warning via `this.logger.warn(...)` when `null` is returned, passing `filePath` and `lineNumber` from the calling context.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 6679564 |
Comment on lines
88
to
+92
| return res.value ?? []; | ||
| } | ||
|
|
||
| export async function getTeamById(token: string, teamId: string): Promise<GraphGroup | null> { | ||
| const path = `/teams/${encodeURIComponent(teamId)}?$select=id,displayName`; |
Contributor
There was a problem hiding this comment.
getTeamById return type is misleading
The function signature promises GraphGroup | null, but since fetchGraphJson throws a new Error(...) on any non-OK HTTP response (including 404), this function will never actually return null — it either returns the group or throws. All call-sites already use try-catch, so this is functionally fine, but the | null annotation is dead code and could mislead a future caller into skipping the try-catch wrapper in favour of a null check.
Consider updating to match the real contract:
Suggested change
| return res.value ?? []; | |
| } | |
| export async function getTeamById(token: string, teamId: string): Promise<GraphGroup | null> { | |
| const path = `/teams/${encodeURIComponent(teamId)}?$select=id,displayName`; | |
| export async function getTeamById(token: string, teamId: string): Promise<GraphGroup> { | |
| const path = `/teams/${encodeURIComponent(teamId)}?$select=id,displayName`; | |
| return await fetchGraphJson<GraphGroup>({ token, path }); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/graph.ts
Line: 88-92
Comment:
**`getTeamById` return type is misleading**
The function signature promises `GraphGroup | null`, but since `fetchGraphJson` throws a `new Error(...)` on any non-OK HTTP response (including 404), this function will never actually return `null` — it either returns the group or throws. All call-sites already use try-catch, so this is functionally fine, but the `| null` annotation is dead code and could mislead a future caller into skipping the try-catch wrapper in favour of a null check.
Consider updating to match the real contract:
```suggestion
export async function getTeamById(token: string, teamId: string): Promise<GraphGroup> {
const path = `/teams/${encodeURIComponent(teamId)}?$select=id,displayName`;
return await fetchGraphJson<GraphGroup>({ token, path });
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
307
to
335
| const messageUrl = params.messageUrl; | ||
| let accessToken: string; | ||
| try { | ||
| accessToken = await params.tokenProvider.getAccessToken("https://graph.microsoft.com"); | ||
| // Cross-tenant: if conversation tenant differs from bot tenant, use conversation tenant token | ||
| const _provider = params.tokenProvider as unknown as { | ||
| connectionSettings?: { clientId?: string; clientSecret?: string; tenantId?: string }; | ||
| getAccessToken(authConfig: Record<string, unknown>, scope: string): Promise<string>; | ||
| getAccessToken(scope: string): Promise<string>; | ||
| }; | ||
| const _convTenantId = params.conversationTenantId; | ||
| if ( | ||
| _convTenantId && | ||
| _provider.connectionSettings?.clientId && | ||
| _provider.connectionSettings?.clientSecret && | ||
| _convTenantId !== _provider.connectionSettings.tenantId | ||
| ) { | ||
| accessToken = await _provider.getAccessToken( | ||
| { | ||
| clientId: _provider.connectionSettings.clientId, | ||
| clientSecret: _provider.connectionSettings.clientSecret, | ||
| tenantId: _convTenantId, | ||
| }, | ||
| "https://graph.microsoft.com", | ||
| ); | ||
| } else { | ||
| accessToken = await params.tokenProvider.getAccessToken("https://graph.microsoft.com"); | ||
| } | ||
| } catch { | ||
| return { media: [], messageUrl, tokenError: true }; |
Contributor
There was a problem hiding this comment.
The cast params.tokenProvider as unknown as { connectionSettings?: ...; getAccessToken(authConfig, scope) } accesses internal properties and an overloaded signature that are not part of the public tokenProvider contract. If the SDK refactors MsalTokenProvider internals, this silently falls back to the bot-tenant token (because _provider.connectionSettings?.clientId will be undefined and the cross-tenant branch is skipped). This is fail-safe behaviour today, but makes the cross-tenant path invisible in test coverage and fragile across SDK upgrades.
The cleaner, SDK-version-proof approach already used elsewhere in this PR is resolveGraphToken({ cfg, conversationTenantId }) — that function explicitly constructs the right auth config via sdk.getAuthConfigWithDefaults(...). Consider replacing this block with a call to resolveGraphToken (or an equivalent helper) so the cross-tenant override doesn't rely on private properties.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/attachments/graph.ts
Line: 307-335
Comment:
**Cross-tenant token acquisition via private SDK internals**
The cast `params.tokenProvider as unknown as { connectionSettings?: ...; getAccessToken(authConfig, scope) }` accesses internal properties and an overloaded signature that are not part of the public `tokenProvider` contract. If the SDK refactors `MsalTokenProvider` internals, this silently falls back to the bot-tenant token (because `_provider.connectionSettings?.clientId` will be `undefined` and the cross-tenant branch is skipped). This is fail-safe behaviour today, but makes the cross-tenant path invisible in test coverage and fragile across SDK upgrades.
The cleaner, SDK-version-proof approach already used elsewhere in this PR is `resolveGraphToken({ cfg, conversationTenantId })` — that function explicitly constructs the right auth config via `sdk.getAuthConfigWithDefaults(...)`. Consider replacing this block with a call to `resolveGraphToken` (or an equivalent helper) so the cross-tenant override doesn't rely on private properties.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+224
to
+234
| threadId?: string; | ||
| rootMessageId?: string; | ||
| limit?: number; | ||
| }): Promise<ArchiveMessageRecord[]> { | ||
| const threadKey = params.rootMessageId?.trim() || params.threadId?.trim(); | ||
| if (!threadKey) { | ||
| throw new Error("threadId or rootMessageId is required"); | ||
| } | ||
|
|
||
| const messages = await this.readArchiveMessages(params.conversationId); | ||
| return (await Promise.all(messages.map((message) => this.withAttachmentStatus(message)))) |
Contributor
There was a problem hiding this comment.
console.warn bypasses the injected logger
parseJsonLine uses console.warn directly rather than the Logger that is injected via MSTeamsChannelArchiveStore. This means malformed JSONL warnings won't flow through the host's configured log handler (level filtering, structured-log sinks, etc.).
One option is to thread the logger through as a parameter. Alternatively, since this is a static parse helper, it can return a sentinel and let the calling method emit the warning through this.logger:
Suggested change
| threadId?: string; | |
| rootMessageId?: string; | |
| limit?: number; | |
| }): Promise<ArchiveMessageRecord[]> { | |
| const threadKey = params.rootMessageId?.trim() || params.threadId?.trim(); | |
| if (!threadKey) { | |
| throw new Error("threadId or rootMessageId is required"); | |
| } | |
| const messages = await this.readArchiveMessages(params.conversationId); | |
| return (await Promise.all(messages.map((message) => this.withAttachmentStatus(message)))) | |
| function parseJsonLine<T>(line: string): T | null { | |
| try { | |
| return JSON.parse(line) as T; | |
| } catch { | |
| return null; | |
| } | |
| } |
And in the callers (e.g. readMessagesFromFile), emit the warning via this.logger.warn(...) when null is returned, passing filePath and lineNumber from the calling context.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams-channel-archive/src/archive-store.ts
Line: 224-234
Comment:
**`console.warn` bypasses the injected logger**
`parseJsonLine` uses `console.warn` directly rather than the `Logger` that is injected via `MSTeamsChannelArchiveStore`. This means malformed JSONL warnings won't flow through the host's configured log handler (level filtering, structured-log sinks, etc.).
One option is to thread the logger through as a parameter. Alternatively, since this is a static parse helper, it can return a sentinel and let the calling method emit the warning through `this.logger`:
```suggestion
function parseJsonLine<T>(line: string): T | null {
try {
return JSON.parse(line) as T;
} catch {
return null;
}
}
```
And in the callers (e.g. `readMessagesFromFile`), emit the warning via `this.logger.warn(...)` when `null` is returned, passing `filePath` and `lineNumber` from the calling context.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6679564015
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+323
to
+330
| accessToken = await _provider.getAccessToken( | ||
| { | ||
| clientId: _provider.connectionSettings.clientId, | ||
| clientSecret: _provider.connectionSettings.clientSecret, | ||
| tenantId: _convTenantId, | ||
| }, | ||
| "https://graph.microsoft.com", | ||
| ); |
There was a problem hiding this comment.
Call token provider with scope-first signature
In the cross-tenant branch, _provider.getAccessToken is invoked with an auth-config object as the first argument and the Graph scope second, but the provider used in this path is the same MsalTokenProvider/MSTeamsAccessTokenProvider that is otherwise called as getAccessToken(scope). When that single-argument implementation is used (the normal case), this passes a non-scope object where the scope is expected, causing token acquisition to fail and downloadMSTeamsGraphMedia to return tokenError, so cross-tenant inbound media extraction silently stops working.
Useful? React with 👍 / 👎.
| if (!sourcePath) { | ||
| continue; | ||
| } | ||
| const content = await fs.promises.readFile(sourcePath); |
There was a problem hiding this comment.
Skip unreadable attachments instead of aborting archive writes
Archiving currently reads each sourcePath without error handling, so a single missing or unreadable media file throws and aborts archiveMessage before the message record is appended. In that case the whole channel message is lost from the archive (not just the bad attachment), which makes archive persistence fragile whenever media files are cleaned up or inaccessible.
Useful? React with 👍 / 👎.
Member
|
Thanks for the contribution. I am closing this because the branch appears dirty and too broad for the core queue: the stated MS Teams hardening spans 39 files, adds a new |
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
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
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Security Impact (required)
Yes)Yes)Yes)No)Yes)Yes, explain risk + mitigation:Team.ReadBasic.AllandChannel.ReadBasic.Allconsented. The code keeps DM/channel isolation intact, uses stable IDs for persistence keys, and treats labels as cached diagnostics/display metadata instead of authority.Repro + Verification
Environment
Steps
Expected
Actual
teamName/channelNameand recent focus sidecar stores graph-backed labels.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
teamLabelSource=graphandchannelLabelSource=graph.teamNameandchannelNameafter conversation-tenant consent.moltbot-main,oc-pm, andoc-tlsuccessfully.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes)Yes)Yes)Team.ReadBasic.AllandChannel.ReadBasic.Allon the bot enterprise app.Failure Recovery (if this breaks)
Risks and Mitigations