Skip to content

Conversation

ioannisj
Copy link

@ioannisj ioannisj commented Sep 4, 2025

Description

Towards #87

Adds a set of survey-related tools:

  • survey-create: Creates a new survey in the project.
  • survey-get: Get a specific survey by ID.
  • surveys-get-all: Get all surveys in the project with optional filtering.
  • survey-update: Update an existing survey by ID.
  • survey-delete: Delete a survey by ID (soft delete - marks as archived.

Plus:

  • surveys-global-stats: Get aggregated response statistics across all surveys in the project.
  • survey-stats: Get response statistics for a specific survey.

This is just CRUD operations mostly. Much more valuable response analysis tools will soon come from @marandaneto

Tried to follow tools documentation. Fair warning: this is my first time touching these tools and language, so excuse any naive bits of code

Tests?

  • Tested locally with lots of prompts and MCP Inspector tool
  • Added some integration tests

@ioannisj ioannisj requested review from joshsny, JonathanLab and a team September 4, 2025 14:50
@ioannisj ioannisj marked this pull request as draft September 4, 2025 14:53
"category": "Surveys",
"summary": "Update an existing survey by ID."
},
"survey-delete": {
Copy link
Member

Choose a reason for hiding this comment

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

Do we call it 'delete' on the UI?
Or do we have both states? Arquive and really deleted?

Copy link
Author

Choose a reason for hiding this comment

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

In the UI we have "Delete" for all surveys (permanent) and "Archive" for stopped or completed surveys. Afaik, we don't have an actual soft delete.
Had the same concerns as this comment here, so I went with always archive from mcp instead

Copy link
Collaborator

@joshsny joshsny left a comment

Choose a reason for hiding this comment

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

This is looking great! Will give it a test run now locally - left some comments, main question is whether the LLM handles the input complexity well enough

const validatedInput = CreateSurveyInputSchema.parse(data);

const createResponseSchema = z.object({
id: z.string(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably worth including more information about the created survey in the response here and in the other tools?

Copy link
Author

Choose a reason for hiding this comment

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

Done, returning the whole survey object which helps with "next steps" as well

delete: async ({
surveyId,
softDelete = true,
}: { surveyId: string; softDelete?: boolean }): Promise<
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just make this a soft delete only to avoid things going too wrong

Copy link
Author

Choose a reason for hiding this comment

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

Added this optional param just for integration test cleanup (so we are left with a clean project).
The deleteHandler actually omits this param. My understanding is that the agent will always soft delete this way, right?

// Output schema - permissive for API responses
export const SurveyQuestionOutputSchema = z.object({
type: z.string(),
question: z.string().optional(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should all use .nullish() to allow null and undefined values

Copy link
Author

Choose a reason for hiding this comment

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

Done. Thought optional() was doing it but reading up it doesn't actually include null values

Copy link
Collaborator

Choose a reason for hiding this comment

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

These look great and really detailed with some useful error messages - when you were testing this locally, was the LLM able to handle the complexity of the input? We might need to slightly restrict the kinds of surveys that can be made if not.

});

export const SurveyGetAllSchema = z.object({
data: ListSurveysInputSchema.optional(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can probably be just ListSurveysInputSchema directly

Copy link
Author

Choose a reason for hiding this comment

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

Done

annotations: {
destructiveHint: true,
idempotentHint: false,
openWorldHint: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
openWorldHint: false,
openWorldHint: true,

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should put this as true for this and the other tools

Copy link
Author

Choose a reason for hiding this comment

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

Done

}

// Format the surveys with better status display
const formattedSurveys = surveysResult.data.map((survey) => ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we pull this out into a function and then apply it elsewhere as well, so that we are being consistent with the output across tools?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense to include the url here too

Copy link
Author

Choose a reason for hiding this comment

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

Done, pulled into a utils function

const { surveyId, ...data } = params;
const projectId = await context.stateManager.getProjectId();

if (data.questions) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we've got this logic here, it probably should also be in the create tool

Copy link
Author

Choose a reason for hiding this comment

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

Done. Single choice question branching is index based, but agent kept using the option text as the key. So my solution was to use Int in input schema, then map to string before making the call. I'm sure there's a better way though

Copy link
Collaborator

@joshsny joshsny left a comment

Choose a reason for hiding this comment

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

This is looking great! Will give it a test run now locally - left some comments, main question is whether the LLM handles the input complexity well enough

@joshsny
Copy link
Collaborator

joshsny commented Sep 4, 2025

Did some testing locally, a few thoughts:

  • The stats tools were not returning results for me, just the survey name, id and some other details, not sure if I was doing this wrong?
  • Surveys list works great
  • I got 'Permission Denied' whilst trying to use the create survey tool, I think it might just be struggling to get the input right as it's quite complex, I think we might need to simplify inputs here
  • Same for update, we probably need to simplify the input schema the same as the create one
  • Deleting surveys worked well

Let me know if you want any more testing :)

Copy link
Author

@ioannisj ioannisj left a comment

Choose a reason for hiding this comment

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

Hey @joshsny, thanks for reviewing this. I tried to address all the comments here.

Regarding the following

Did some testing locally, a few thoughts:

  • The stats tools were not returning results for me, just the survey name, id and some other details, not sure if I was doing this wrong?

This is based on responses, so for a fresh survey with no responses I guess it's because the response counts were zero? Tried running it for a survey with some responses and got some stats back

  • Surveys list works great
  • I got 'Permission Denied' whilst trying to use the create survey tool, I think it might just be struggling to get the input right as it's quite complex, I think we might need to simplify inputs here
  • Same for update, we probably need to simplify the input schema the same as the create one

When I was testing, Permission Denied was usually trying to create or update a survey to a project that was not in my Personal API key. I'm attaching some screenshots below with some prompts I tried. Looks like it's handing it okay but tbh I have no idea how to test this thoroughly.

CleanShot 2025-09-06 at 02 04 14

This prompt here created the survey correctly with

  • Reasonable title and theme
  • Conditional logic on detractors with a reasonable follow up question
  • Correct display conditions
CleanShot 2025-09-06 at 01 51 02 CleanShot 2025-09-06 at 01 51 09

Then the follow-up prompt to link this to a feature flag seemed to work okay as well

CleanShot 2025-09-06 at 02 04 52 CleanShot 2025-09-06 at 02 03 11

Not sure if my prompts are just a bit too specific and giving it too much guidance though.

const validatedInput = CreateSurveyInputSchema.parse(data);

const createResponseSchema = z.object({
id: z.string(),
Copy link
Author

Choose a reason for hiding this comment

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

Done, returning the whole survey object which helps with "next steps" as well

delete: async ({
surveyId,
softDelete = true,
}: { surveyId: string; softDelete?: boolean }): Promise<
Copy link
Author

Choose a reason for hiding this comment

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

Added this optional param just for integration test cleanup (so we are left with a clean project).
The deleteHandler actually omits this param. My understanding is that the agent will always soft delete this way, right?

"category": "Surveys",
"summary": "Update an existing survey by ID."
},
"survey-delete": {
Copy link
Author

Choose a reason for hiding this comment

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

In the UI we have "Delete" for all surveys (permanent) and "Archive" for stopped or completed surveys. Afaik, we don't have an actual soft delete.
Had the same concerns as this comment here, so I went with always archive from mcp instead

});

export const SurveyGetAllSchema = z.object({
data: ListSurveysInputSchema.optional(),
Copy link
Author

Choose a reason for hiding this comment

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

Done

annotations: {
destructiveHint: true,
idempotentHint: false,
openWorldHint: false,
Copy link
Author

Choose a reason for hiding this comment

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

Done

}

// Format the surveys with better status display
const formattedSurveys = surveysResult.data.map((survey) => ({
Copy link
Author

Choose a reason for hiding this comment

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

Done, pulled into a utils function

const { surveyId, ...data } = params;
const projectId = await context.stateManager.getProjectId();

if (data.questions) {
Copy link
Author

Choose a reason for hiding this comment

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

Done. Single choice question branching is index based, but agent kept using the option text as the key. So my solution was to use Int in input schema, then map to string before making the call. I'm sure there's a better way though

// Output schema - permissive for API responses
export const SurveyQuestionOutputSchema = z.object({
type: z.string(),
question: z.string().optional(),
Copy link
Author

Choose a reason for hiding this comment

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

Done. Thought optional() was doing it but reading up it doesn't actually include null values

@ioannisj ioannisj requested a review from joshsny September 5, 2025 23:32
@joshsny
Copy link
Collaborator

joshsny commented Sep 6, 2025

Awesome - thanks again for this @ioannisj - looks great!

Since you are on paternity from Monday, I can merge this in next week - have a great time!

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