feat(forest mcp server): add the token route #1343
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
Definition of Done
General
Security