feat(forest mcp server): add the token route#1343
feat(forest mcp server): add the token route#1343VincentMolinie merged 4 commits intofeat/forest-mcp/mainfrom
Conversation
|
CU-86c6ur5gj |
2 new issues
This is from Qlty Cloud, the successor to Code Climate Quality. Learn more. |
|
Coverage Impact This PR will not change total coverage. 🚦 See full report on Qlty Cloud »🛟 Help
|
VincentMolinie
force-pushed
the
feat/forest-mcp/token
branch
from
307784d to
5b9bcc6
Compare
VincentMolinie
force-pushed
the
feat/forest-mcp/token
branch
from
5b9bcc6 to
c81a0e5
Compare
| /** | ||
| * Setup superagent mocking to intercept HTTP requests made by forestadmin-client | ||
| */ |
There was a problem hiding this comment.
why do you want to mock inside the forestadmin-client ? just mock the forestadmin-client itself, we don't need to test its inside behaviour
There was a problem hiding this comment.
I wanted to make sure the call was properly made
There was a problem hiding this comment.
By doing so I'm making sure that even when upgrading the forestadmin-client, nothing is broken 😉
There was a problem hiding this comment.
that seems a bit overengineered for a small improvement... we don't need to test the forestadmin-client behavior in these tests, that's a completely different package
There was a problem hiding this comment.
mocking a dependency of a dependency to ensure the first one is doing what it should do isn't a very good pattern, I think
There was a problem hiding this comment.
An integration test must test as much as possible everything, without mock
There was a problem hiding this comment.
yeah, but testing an other package doesn't make sense. If we want to actually test everything, we'll do end-to-end tests that mount a real forest agent and look at its responses.
Integration tests should test the whole package, but not its dependencies
| // Handle ESM/CJS interop: the module may be double-wrapped with default exports | ||
| const createForestAdminClient = | ||
| typeof forestAdminClientModule === 'function' | ||
| ? forestAdminClientModule | ||
| : // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| ((forestAdminClientModule as any).default as typeof forestAdminClientModule); |
There was a problem hiding this comment.
i'm pretty sure we don't need that, you can import '@forestadmin/forestadmin-client' directly
There was a problem hiding this comment.
No I do need it 😅. try without it, it was my first attempt but because of one of the πackages we use (mcp/sdk if I remember well) all the packages are imported as ESM. It's not the same standard
There was a problem hiding this comment.
No it is not as far as I know 😉
| ); | ||
|
|
||
| const token = jsonwebtoken.sign( | ||
| { ...user, serverToken: forestServerAccessToken }, |
There was a problem hiding this comment.
why repeating the userInfo that already are in the serverToken ?
There was a problem hiding this comment.
No user is a call to the forest server 😉
There was a problem hiding this comment.
I mean you can decode the token payload and get the userData from there. You know it is valid because that's the server that issued the token directly
2 participants
Definition of Done
General
Security