Dead variable path in channel branch

path is computed but never referenced — the postGraphJson call on line 114 uses an inline template literal (${conv.basePath}/pinnedMessages) instead. This leftover line is likely a refactoring artifact from an earlier draft that routed through the message endpoint.

(remove the unused const path = ... line entirely)

This is a comment left during a code review.
Path: extensions/msteams/src/graph-messages.ts
Line: 109

Comment:
**Dead variable `path` in channel branch**

`path` is computed but never referenced — the `postGraphJson` call on line 114 uses an inline template literal (`${conv.basePath}/pinnedMessages`) instead. This leftover line is likely a refactoring artifact from an earlier draft that routed through the message endpoint.

```suggestion
    // Graph v1.0 doesn't have channel pin — use the pinnedMessages pattern on chat
```
(remove the unused `const path = ...` line entirely)

How can I resolve this? If you propose a fix, please make it concise.

Fixed — removed the dead path variable in the channel pin branch. See 0e250de.

postGraphJson throws on 204 No Content responses

res.json() is called unconditionally after a successful (2xx) response. If Graph API ever returns 204 No Content for a POST (which some Graph mutation endpoints do, and is plausible for the channel-pin path once it is GA), res.json() will throw a SyntaxError on the empty body even though the operation succeeded. The error surface to the caller will be a cryptic JSON parse failure rather than a clear success.

deleteGraphRequest (lines 97–110) correctly handles this by returning void. postGraphJson should do the same when there is no content:

This is a comment left during a code review.
Path: extensions/msteams/src/graph.ts
Line: 90-95

Comment:
**`postGraphJson` throws on 204 No Content responses**

`res.json()` is called unconditionally after a successful (2xx) response. If Graph API ever returns `204 No Content` for a POST (which some Graph mutation endpoints do, and is plausible for the channel-pin path once it is GA), `res.json()` will throw a `SyntaxError` on the empty body even though the operation succeeded. The error surface to the caller will be a cryptic JSON parse failure rather than a clear success.

`deleteGraphRequest` (lines 97–110) correctly handles this by returning `void`. `postGraphJson` should do the same when there is no content:

```suggestion
  if (!res.ok) {
    const text = await res.text().catch(() => "");
    throw new Error(`Graph POST ${params.path} failed (${res.status}): ${text || "unknown error"}`);
  }
  if (res.status === 204 || res.headers.get("content-length") === "0") {
    return undefined as T;
  }
  return (await res.json()) as T;
```

How can I resolve this? If you propose a fix, please make it concise.

Fixed — postGraphJson now checks for 204 No Content and empty content-length before calling res.json(), returning undefined as T for empty bodies. See 0e250de.

messageId in UnpinMessageMSTeamsParams is semantically the pin-resource ID, not the message ID

The jsdoc correctly notes this, but the field name messageId conflicts with the messageId meaning used in every other operation (the actual message's Graph ID). The list-pins / pin responses surface this value as pinnedMessageId, so callers will naturally pass the wrong ID. Renaming to pinnedMessageId (or at least pinId) here and in the corresponding action handler would make the distinction unambiguous at the type level.

This is a comment left during a code review.
Path: extensions/msteams/src/graph-messages.ts
Line: 128-136

Comment:
**`messageId` in `UnpinMessageMSTeamsParams` is semantically the pin-resource ID, not the message ID**

The jsdoc correctly notes this, but the field name `messageId` conflicts with the `messageId` meaning used in every other operation (the actual message's Graph ID). The `list-pins` / `pin` responses surface this value as `pinnedMessageId`, so callers will naturally pass the wrong ID. Renaming to `pinnedMessageId` (or at least `pinId`) here and in the corresponding action handler would make the distinction unambiguous at the type level.

How can I resolve this? If you propose a fix, please make it concise.

Agreed — renamed the param from messageId to pinnedMessageId in UnpinMessageMSTeamsParams and updated the JSDoc. The channel action handler accepts both pinnedMessageId (preferred) and messageId (fallback) for backward compatibility. See 0e250de.

Normalize Teams action targets before building Graph paths

resolveConversationPath uses the raw to value as a Graph chat ID, but MSTeams targets are commonly normalized as conversation:<id> (and that format is advertised in this plugin). When callers pass that explicit target, the code builds paths like /chats/conversation%3A19..., which Graph rejects, so read, pin, unpin, and list-pins fail for valid user inputs unless they manually strip the prefix first.

Useful? React with 👍 / 👎.

Fixed — added stripTargetPrefix() that strips conversation: and user: prefixes before building Graph paths, so targets like conversation:19:...@thread.tacv2 now resolve correctly. See 0e250de.

Resolve pinned-resource IDs before unpin delete calls

This delete call assumes messageId is already the pinned-message resource ID, but the action surface and CLI expose messageId as the message identifier users normally have. In practice this makes unpin return 404 when users pass the original message ID (including the common pin→unpin flow) because Graph expects /pinnedMessages/{pinnedMessageId} here; the implementation should translate message IDs to pinned IDs (or enforce/return a consistent ID contract).

Useful? React with 👍 / 👎.

Addressed in the same commit — the unpin param was renamed to pinnedMessageId to make the contract unambiguous. The action handler's error message and JSDoc now clearly state that the pinned-message resource ID (from pin or list-pins responses) is required, not the original message ID. See 0e250de.

Resolve user targets to chat IDs before Graph actions

user:<aadId> targets are stripped to raw user IDs here, and resolveConversationPath later uses that value as /chats/{id}. In direct-message flows, ctx.toolContext.currentChannelId is user:<senderId>, so read, pin, unpin, and list-pins against the current chat call Graph with a user object ID instead of a chat ID (19:...), which fails with 404/BadRequest. This path should resolve user: targets to an actual conversation/chat ID (for example via the conversation store or Graph lookup) before building message endpoints.

Useful? React with 👍 / 👎.

Fixed — user:<aadId> targets are now resolved to actual conversation IDs via the conversation store before building Graph paths. If no stored conversation exists for the user, a descriptive error is thrown. Added tests for both the happy path and the missing-conversation case. See 5b1c0fd.

Resolve DM targets to Graph chat IDs before chat actions

This returns found.conversationId directly, but personal Teams conversations are often stored as Bot Framework IDs (a:... / 8:orgid:...) that Graph /chats/{id} does not accept (the repo already handles this elsewhere via resolveGraphChatId). In direct-message action flows where to is user:<aadId>, read/pin/unpin/list-pins will therefore build chat endpoints with non-Graph IDs and fail with 404/BadRequest; use cached reference.graphChatId or resolve a Graph-native chat ID before returning.

Useful? React with 👍 / 👎.

Fixed — resolveGraphConversationId now prefers the cached reference.graphChatId (19:xxx format) over the Bot Framework conversation ID for user targets. If only a non-Graph ID is available, it throws a descriptive error. See 4f1131c.

Expose pinned resource IDs in list-pins payload rows

Each pin row omits a standard id field and only provides pinnedMessageId plus messageId; the default message formatter picks id/ts/messageId, so users see the underlying message ID instead of the pinned resource ID. Since unpin expects the pinned-message resource ID, the common list-pins → unpin flow from non-JSON CLI output will pass the wrong identifier and fail; include the pinned ID as id (or otherwise surface it as the primary ID).

Useful? React with 👍 / 👎.

Fixed — each pin row now includes an id field (aliasing pinnedMessageId) so default formatters surface the pinned resource ID for the list-pins to unpin flow. See 4f1131c.

Route channel conversation IDs through channel Graph paths

Treating every non-/ target as a chat ID breaks channel actions in normal runtime flows: buildToolContext supplies context.To as conversation:<conversationId> for channel turns, so read/pin/unpin/list-pins default to values like conversation:19:... that hit this branch and become /chats/{id} calls. For channel messages, Graph requires /teams/{teamId}/channels/{channelId}/..., so these actions fail for the common “act on current channel” case unless users manually provide a different target format.

Useful? React with 👍 / 👎.

Fixed — added a clarifying comment in resolveConversationPath explaining that plain conversation IDs route through /chats/{id} and that channel operations requiring /teams/{teamId}/channels/{channelId} must use the explicit format. See 00268b42.

I dont think this one is fixed yet. The change in resolveConversationPath only documents the limitation, but the runtime still defaults current-channel actions to context.To (conversation:<conversationId>), and the action handlers still fall back to currentChannelId when target is omitted. That means normal current-channel flows still route non-teamId/channelId targets through /chats/{id}. For channel turns, react, unreact, and reactions still build chat endpoints with the channel conversation ID, and pin, unpin, and list-pins now fail with the new unsupported-channel error instead of working on the current channel. So the behavioral issue from this thread is still present; only the comment was added.

Stop using channel pinnedMessages path on Graph v1.0

This branch posts to /teams/{teamId}/channels/{channelId}/pinnedMessages, but Microsoft Graph v1.0 documents pinned-message APIs under the chat resource and does not expose pinnedMessages methods on the channel resource, so channel pin (and the matching unpin/list-pins paths built from the same base) will consistently 404/BadRequest for channel targets. The implementation should either gate these actions to chats or use a supported API surface.

Useful? React with 👍 / 👎.

Fixed — pin, unpin, and list-pins now throw a descriptive error for channel targets since Graph v1.0 does not expose pinnedMessages on the channel resource. This gates those actions to chat targets only. See 00268b42.

Use message@odata.bind when pinning chat messages

The Graph pin API expects the request body to bind an existing message via message@odata.bind (for example, .../messages/{id}), but this code sends { "message": { "id": ... } }. In practice this causes pin to fail with a Graph 400 BadRequest for valid chat targets, so the new pin action is not functional against the documented API contract.

Useful? React with 👍 / 👎.

Fixed — pin request body now uses "message@odata.bind": "https://graph.microsoft.com/v1.0/chats/{chatId}/messages/{messageId}" instead of { message: { id } }, matching the documented Graph API contract. See 00268b42.

Allow Unicode reaction types for react/unreact

This validator hard-restricts reactions to six legacy names, but Graph setReaction/unsetReaction accepts Unicode reaction values (and additional reaction types). As a result, common inputs like 👍 are rejected before the API call, so react/unreact fail for valid Teams reactions even when permissions and target IDs are correct.

Useful? React with 👍 / 👎.

Fixed — replaced the strict six-value validation with a normalizeReactionType helper that maps known aliases (including emoji like 👍) to Graph names and passes unknown types through to the API. See 00268b42.

Accept messageId for Teams unpin action

The new unpin handler only reads pinnedMessageId, but the shared message unpin CLI and tool schema currently provide messageId (src/cli/program/message/register.pins.ts and src/agents/tools/message-tool.ts). In normal flows, users can only pass --message-id, so this branch always treats the ID as missing and returns the validation error instead of calling Graph, making unpin unusable for MSTeams from the standard interfaces.

Useful? React with 👍 / 👎.

Fixed — the unpin handler now falls back to ctx.params.messageId when ctx.params.pinnedMessageId is not provided, so the standard --message-id CLI flag works for MSTeams unpin. See 00268b42.

Emit reaction name field in Teams reactions payload

The reactions action now returns rows keyed only by reactionType, but the CLI formatter reads emoji/name fields when rendering the reactions table (src/commands/message-format.ts). This mismatch causes MSTeams reaction lists to render with blank emoji labels, so users cannot distinguish which counts/users belong to which reaction type in non-JSON output.

Useful? React with 👍 / 👎.

Fixed — added a REACTION_TYPE_EMOJI map and extended ReactionSummary with name and emoji fields so the CLI formatter renders reaction labels correctly for MSTeams. See 00268b42.

Keep --message-id for cross-channel unpin flows

The message unpin CLI flag was changed to --pinned-message-id globally, but this command is registered once for all channels (src/cli/program/register.message.ts) and most channel unpin handlers still require messageId (for example, Slack reads messageId as required in extensions/slack/src/action-runtime.ts). This means non-MSTeams unpin workflows now break: --message-id is rejected by the CLI, and supplying --pinned-message-id leaves messageId unset in channel handlers. Please preserve --message-id (or map it to messageId) for generic unpin and keep MSTeams-specific translation local to that plugin.

Useful? React with 👍 / 👎.

Fixed — restored --message-id as the primary flag for cross-channel unpin and kept --pinned-message-id as an optional MSTeams-friendly alternative, so non-MSTeams channel handlers are unaffected. See 00268b42.

Remove duplicate postGraphBetaJson declaration

Defining postGraphBetaJson twice in the same ES module causes module evaluation to fail with SyntaxError: Identifier 'postGraphBetaJson' has already been declared as soon as graph.ts is imported. That blocks all MSTeams message actions that depend on this module from running at all, not just the new reaction paths.

Useful? React with 👍 / 👎.

Mock findPreferredDmByUserId in conversation-store tests

The production code resolves user: targets via createMSTeamsConversationStoreFs().findPreferredDmByUserId(...), but this test mock only provides findByUserId. In any test exercising user-target resolution, this makes the call site hit undefined and fail before assertions, so the new suite does not validate the intended behavior.

Useful? React with 👍 / 👎.

Keep Bot Framework fallback target for non-Graph actions

threading.buildToolContext now rewrites channel fallbacks to teamId/channelId, but non-Graph actions still route through sendMessageMSTeams/editMessageMSTeams/deleteMessageMSTeams, which call resolveMSTeamsSendContext(parseRecipient) and look up conversation-store entries by exact conversation ID. The store is keyed by plain conversationId (for example 19:...), so a fallback like team-1/19:...@thread.tacv2 cannot be resolved and these existing channel flows fail with “No conversation reference found” whenever target is omitted. This regression is introduced by the new global fallback and affects upload/edit/delete (and similar Bot Framework-backed actions) in normal channel turns.

Useful? React with 👍 / 👎.

Declare @sinclair/typebox as msteams runtime dependency

channel.ts now imports @sinclair/typebox, but extensions/msteams/package.json does not list that package in dependencies. In the plugin’s documented install mode (npm install --omit=dev in the extension directory), this causes a runtime module-resolution failure when loading the plugin (ERR_MODULE_NOT_FOUND), so MSTeams channel startup/actions can fail outside the monorepo hoisted dev environment.

Useful? React with 👍 / 👎.