Skip to content

Conversation

shsteimer
Copy link

Please ensure your pull request adheres to the following guidelines:

  • make sure to link the related issues in this description. Or if there's no issue created, make sure you
    describe here the problem you're solving.
  • when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

If the PR is changing the API specification:

  • make sure you add a "Not implemented yet" note the endpoint description, if the implementation is not ready
    yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
  • make sure you add at least one example of the request and response.

If the PR is changing the API implementation or an entity exposed through the API:

  • make sure you update the API specification and the examples to reflect the changes.

If the PR is introducing a new audit type:

  • make sure you update the API specification with the type, schema of the audit result and an example

Related Issues

Thanks for contributing!

Copy link
Member

@solaris007 solaris007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't understand why you're adding an HTTP controller. this is only needed if you want to expose your data via our HTTP API, in which case first an OpenAPI spec would have to be submitted and the controller be added to our HTTP routes. MCP alone does not require an HTTP controller. For simplicity reasons we wired existing HTTP controller actions that we also wanted to be available as tool to the controllers, this doesn't mean all new tools/resources have to implement a controller.

@shsteimer
Copy link
Author

i don't understand why you're adding an HTTP controller...

This is good feedback. TBH, I wasn't quite sure if this was the right approach or not, and was kind of stuck in between 2 ideas...

  1. just expose CRUX data directly - MCP is just a pass-though to the google crux API
  2. actually importing CRUX data to space cat, and therefore exposing it as a proper API.

with 2 we do I think need a controller, but perhaps it being a dedicated CRUX controller is wrong, but instead it should be part of the audits controllor? but for 1, I guess no need for controller, fair enough.

Will update this PR to remove controller and go purely with option 1. Then we can follow up with option 2 separately.

Copy link

This PR will trigger a minor release when merged.

@shsteimer shsteimer requested a review from solaris007 June 10, 2025 15:47
@shsteimer
Copy link
Author

@solaris007 hopefully this is more along the lines of what you'd expect.

TBH, I don't love initializing the crux client in index.js, but in order to test the crux MCP tool without testing crux-client, this was seemed like the best way to do that. But if you feel otherwise, let me know and I'll revisit.

Other concern is around API Key, https://developer.chrome.com/docs/crux/api#crux_api_key. We either need to move that to a param on the tool, which means the user needs their own key, or create one for space cat usage and inject to the env (as the code currently expects).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants