msteams: add member-info action#57528
Conversation
openclaw-barnacle
bot
added
channel: msteams
Greptile SummaryThis PR adds a The change is well-scoped and follows existing conventions in Findings:
Confidence Score: 5/5This PR is safe to merge; all findings are minor style and test-accuracy suggestions with no functional impact on the happy path. No P0 or P1 issues found. Both open findings are P2: one is a minor type-duplication cleanup and the other is a test-mock accuracy nuance that doesn't affect production behaviour. The implementation follows established patterns, input is encoded properly, and unit tests cover the key scenarios. No files require special attention.
|
| Filename | Overview |
|---|---|
| extensions/msteams/src/graph-members.ts | New module that fetches a Microsoft Graph user by ID with a $select projection; clean and correctly uses encodeURIComponent. Minor: GraphUserProfile duplicates four fields already declared in the shared GraphUser type. |
| extensions/msteams/src/graph-members.test.ts | Unit tests cover happy path, sparse data, and error propagation correctly. Sparse-data mock returns absent fields as undefined rather than null, which may not reflect real Graph API responses for $select-requested fields. |
| extensions/msteams/src/channel.ts | Adds member-info to the action list and implements the handler; validates userId, delegates to the new runtime function, and returns a JSON result. Follows the exact same pattern as neighbouring action handlers. |
| extensions/msteams/src/channel.runtime.ts | Registers getMemberInfoMSTeams in the runtime export map alphabetically; straightforward wiring change with no issues. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/msteams/src/graph-members.ts
Line: 4-11
Comment:
**`GraphUserProfile` partially duplicates `GraphUser`**
The existing `GraphUser` type in `graph.ts` already declares `id`, `displayName`, `mail`, and `userPrincipalName` as optional strings. `GraphUserProfile` redeclares all four of those fields verbatim and then adds `jobTitle` and `officeLocation`.
Consider extending `GraphUser` to avoid the duplication:
```suggestion
type GraphUserProfile = import("./graph.js").GraphUser & {
jobTitle?: string;
officeLocation?: string;
};
```
Or, if you prefer to keep the import explicit, import `GraphUser` at the top of the file and write:
```ts
type GraphUserProfile = GraphUser & {
jobTitle?: string;
officeLocation?: string;
};
```
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/graph-members.test.ts
Line: 62-78
Comment:
**Test mock for sparse data may not reflect real Graph API behaviour**
The mock returns an object with only `id` and `displayName`, leaving the other selected fields absent (JavaScript `undefined`). However, the Microsoft Graph API — when asked for fields via `$select` — typically returns those fields with `null` rather than omitting them entirely. This means:
1. `GetMemberInfoMSTeamsResult` and `GraphUserProfile` typing would more accurately read `string | null | undefined` for those optional fields.
2. The assertion `expect(result).toEqual({ user: { ..., mail: undefined, ... } })` wouldn't hold against a real Graph response where `mail` is `null`.
The inconsistency doesn't cause a runtime error today (falsy checks work for both), but it means the type contract doesn't match what the Graph API actually produces, and a strict-equality check on `undefined` by a future consumer would silently fail. Adjusting the mock to use `null` for absent fields and widening the types to `string | null | undefined` would align with the codebase contract from `fetchGraphJson`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "msteams: add member-info action via Grap..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| type GraphUserProfile = { | ||
| id?: string; | ||
| displayName?: string; | ||
| mail?: string; | ||
| jobTitle?: string; | ||
| userPrincipalName?: string; | ||
| officeLocation?: string; | ||
| }; |
There was a problem hiding this comment.
GraphUserProfile partially duplicates GraphUser
The existing GraphUser type in graph.ts already declares id, displayName, mail, and userPrincipalName as optional strings. GraphUserProfile redeclares all four of those fields verbatim and then adds jobTitle and officeLocation.
Consider extending GraphUser to avoid the duplication:
| type GraphUserProfile = { | |
| id?: string; | |
| displayName?: string; | |
| mail?: string; | |
| jobTitle?: string; | |
| userPrincipalName?: string; | |
| officeLocation?: string; | |
| }; | |
| type GraphUserProfile = import("./graph.js").GraphUser & { | |
| jobTitle?: string; | |
| officeLocation?: string; | |
| }; |
Or, if you prefer to keep the import explicit, import GraphUser at the top of the file and write:
type GraphUserProfile = GraphUser & {
jobTitle?: string;
officeLocation?: string;
};Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/graph-members.ts
Line: 4-11
Comment:
**`GraphUserProfile` partially duplicates `GraphUser`**
The existing `GraphUser` type in `graph.ts` already declares `id`, `displayName`, `mail`, and `userPrincipalName` as optional strings. `GraphUserProfile` redeclares all four of those fields verbatim and then adds `jobTitle` and `officeLocation`.
Consider extending `GraphUser` to avoid the duplication:
```suggestion
type GraphUserProfile = import("./graph.js").GraphUser & {
jobTitle?: string;
officeLocation?: string;
};
```
Or, if you prefer to keep the import explicit, import `GraphUser` at the top of the file and write:
```ts
type GraphUserProfile = GraphUser & {
jobTitle?: string;
officeLocation?: string;
};
```
How can I resolve this? If you propose a fix, please make it concise.| }); | ||
|
|
||
| const result = await getMemberInfoMSTeams({ | ||
| cfg: {} as OpenClawConfig, | ||
| userId: "user-456", | ||
| }); | ||
|
|
||
| expect(result).toEqual({ | ||
| user: { | ||
| id: "user-456", | ||
| displayName: "Bob", | ||
| mail: undefined, | ||
| jobTitle: undefined, | ||
| userPrincipalName: undefined, | ||
| officeLocation: undefined, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The mock returns an object with only id and displayName, leaving the other selected fields absent (JavaScript undefined). However, the Microsoft Graph API — when asked for fields via $select — typically returns those fields with null rather than omitting them entirely. This means:
GetMemberInfoMSTeamsResultandGraphUserProfiletyping would more accurately readstring | null | undefinedfor those optional fields.- The assertion
expect(result).toEqual({ user: { ..., mail: undefined, ... } })wouldn't hold against a real Graph response wheremailisnull.
The inconsistency doesn't cause a runtime error today (falsy checks work for both), but it means the type contract doesn't match what the Graph API actually produces, and a strict-equality check on undefined by a future consumer would silently fail. Adjusting the mock to use null for absent fields and widening the types to string | null | undefined would align with the codebase contract from fetchGraphJson.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/graph-members.test.ts
Line: 62-78
Comment:
**Test mock for sparse data may not reflect real Graph API behaviour**
The mock returns an object with only `id` and `displayName`, leaving the other selected fields absent (JavaScript `undefined`). However, the Microsoft Graph API — when asked for fields via `$select` — typically returns those fields with `null` rather than omitting them entirely. This means:
1. `GetMemberInfoMSTeamsResult` and `GraphUserProfile` typing would more accurately read `string | null | undefined` for those optional fields.
2. The assertion `expect(result).toEqual({ user: { ..., mail: undefined, ... } })` wouldn't hold against a real Graph response where `mail` is `null`.
The inconsistency doesn't cause a runtime error today (falsy checks work for both), but it means the type contract doesn't match what the Graph API actually produces, and a strict-equality check on `undefined` by a future consumer would silently fail. Adjusting the mock to use `null` for absent fields and widening the types to `string | null | undefined` would align with the codebase contract from `fetchGraphJson`.
How can I resolve this? If you propose a fix, please make it concise.
|
Reviewed. Looks good to merge. |
2 participants
Summary
member-infomessage action to the MS Teams extension/users/{id}endpointTest plan
getMemberInfoMSTeams(happy path, sparse data, error propagation)pnpm test -- extensions/msteamspassespnpm checkpasses🤖 Generated with Claude Code