Skip to content

feat: configure agent skills as part of sanity init#1079

Closed
jonahsnider wants to merge 15 commits into
mainfrom
aigro-4661/configure-agent-skills
Closed

feat: configure agent skills as part of sanity init#1079
jonahsnider wants to merge 15 commits into
mainfrom
aigro-4661/configure-agent-skills

Conversation

@jonahsnider
Copy link
Copy Markdown
Member

@jonahsnider jonahsnider commented May 13, 2026

Description

Configure agent skills alongside the MCP server when running sanity init. The same logic is reused across both MCP setup and the skills setup for selecting what agents/editors, so that you only get asked once.

Internally this is just shelling out to the skills CLI, which is what Neon's neonctl CLI does as well.

What to review

  • Are the automated tests written well? ie. not too specific
  • Additional questions
    • --mcp and --no-mcp are a little confusing, since they now also decide whether agent skills are added. Should we deprecate these flags and add new ones? Keep same?
    • Should we literally call npx skills or does it make sense to add skills as a dependency of the CLI? Should be a lot faster since we are no longer waiting for npx to download skills, but means we need to stay on top of skills updates via Renovate

Testing

TODO: write a brief guide on how to test locally, including a note about Sanity internal env

Todo list for myself

  • Update sanity init to
  • Add sanity skills command for adding and updating agent skills in existing projects

Note

Medium Risk
Adds best-effort execution of an external npx skills add command during sanity init, which can affect init reliability/UX and write files into the new project directory. MCP setup logic is refactored to handle a new cwd/skills path and updated prompting/telemetry, increasing behavioral surface area.

Overview
sanity init now passes the new project outputPath into setupMCP, which in turn installs Sanity agent skills (via npx skills add sanity-io/agent-toolkit) for the selected/detected editors when a cwd is provided.

MCP setup is updated to track skillsCliAgent mappings per editor, adjust the prompt copy, avoid writing MCP config for already-configured editors while still installing project-local skills during init, and surface skills-install failures separately without aborting MCP setup.

Telemetry/test infrastructure is updated to record installedSkillsCliAgents, add unit tests for the new skills installer and MCP/skills interactions, and switch Vitest “agent” detection to @vercel/detect-agent.

Reviewed by Cursor Bugbot for commit d3b61c2. Bugbot is set up for automated code reviews on this repo. Configure here.

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude Bot commented May 13, 2026

Claude finished @jonahsnider's task in 2m 24s —— View job


Review

  • Read changed files
  • Verify Cursor Bugbot findings
  • Check changeset, tests, and code quality

Findings

1. Prompt wording is misleading in sanity mcp configurepromptForMCPSetup.ts:30 (matches Bugbot finding #2)
The shared prompt now says "Configure Sanity MCP and agent skills for your editor?", but skills only install when cwd is set (setupMCP.ts:212), and sanity mcp configure never passes one (configure.ts:43). Users running sanity mcp configure are promised something they won't get.

Suggest either parameterizing the message (pass through setupMCPpromptForMCPSetup) so each call site picks the right wording, or having sanity mcp configure opt-in to installing skills (with appropriate cwd handling). The PR description already flags this UX ambiguity in the --mcp flag question — same root cause.

2. Bugbot finding #1 is stale
Bugbot claims configure.test.ts asserts mockSetupSkills was called once, but the test file at HEAD (commit d3b61c2) only sets up the mock with mockResolvedValue(...) and never asserts it was called. Safe to dismiss this one.

3. setupSkills.ts:80 creates the project dir before skills succeed
await fs.mkdir(cwd, {recursive: true}) runs before execa('npx', …). If skills fails (e.g. timeout, network error, user Ctrl-C during the 90s window), the empty project directory is left behind on disk. Init then continues; if the rest of init aborts later, the user has a stray empty dir.

The comment says this is because the cwd may not exist yet during init. A safer pattern: let init create the dir as part of normal project bootstrap, and have setupSkills no-op (or skip) if cwd doesn't exist instead of creating it. Or at minimum, document the side effect more loudly.

4. Init now has a soft external dependency on npm registry availability
npx -y skills add sanity-io/agent-toolkit cold-installs the skills package (~5–30s on first run, possibly more on slow connections) and reaches GitHub. Handled as best-effort, which is correct, but it does add a new failure surface and adds visible latency to every sanity init for users who hit it for the first time. Worth a brief note in the release/changeset description so users aren't surprised.

5. Test coverage gap — setupSkills.ts:90-93
The error.stdout / error.stderr warning branches (ux.warn(stdout) / ux.warn(stderr)) aren't exercised by setupSkills.test.ts. CLAUDE.md emphasizes coverage; adding one error-with-stdout/stderr case would close this.

6. Minor: dead mock in setupMCP.test.ts:196
mockPromptForMCPSetup.mockImplementation(async (eds) => eds) is set up but mode: 'auto' is used in that test, so the prompt is never called. Remove the line to keep the test clean.

7. Changeset
@sanity/cli: minor is appropriate (new feature behavior in sanity init). Summary "Configure agent skills as part of sanity init" is concise and developer-facing, which matches Sanity product copy conventions. ✓

Direct answer to the PR question

--mcp and --no-mcp are a little confusing… deprecate or keep?

I'd recommend keeping --no-mcp as the single opt-out for both MCP and skills (they're conceptually tied — both are agent-editor setup), but renaming the prompt + flag docs to reflect that scope. Introducing --no-agent-skills separately doubles the surface area for limited benefit. If you do split them, do it before this ships, not after — splitting flags later is a breaking change.
· aigro-4661/configure-agent-skills

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 13, 2026

📦 Bundle Stats — @sanity/cli

Compared against main (f0906a04)

@sanity/cli

Metric Value vs main (f0906a0)
Internal (raw) 2.1 KB -
Internal (gzip) 799 B -
Bundled (raw) 10.97 MB -98 B, -0.0%
Bundled (gzip) 2.06 MB -24 B, -0.0%
Import time 837ms -12ms, -1.5%

bin:sanity

Metric Value vs main (f0906a0)
Internal (raw) 975 B -
Internal (gzip) 460 B -
Bundled (raw) 9.84 MB -
Bundled (gzip) 1.77 MB -
Import time 2.01s +6ms, +0.3%

🗺️ View treemap · Artifacts

Details
  • Import time regressions over 10% are flagged with ⚠️
  • Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.

📦 Bundle Stats — @sanity/cli-core

Compared against main (f0906a04)

Metric Value vs main (f0906a0)
Internal (raw) 95.5 KB -721 B, -0.7%
Internal (gzip) 22.5 KB -40 B, -0.2%
Bundled (raw) 21.60 MB -721 B, -0.0%
Bundled (gzip) 3.42 MB -93 B, -0.0%
Import time 792ms +4ms, +0.5%

🗺️ View treemap · Artifacts

Details
  • Import time regressions over 10% are flagged with ⚠️
  • Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.

📦 Bundle Stats — create-sanity

Compared against main (f0906a04)

Metric Value vs main (f0906a0)
Internal (raw) 976 B -
Internal (gzip) 507 B -
Bundled (raw) 50.7 KB -
Bundled (gzip) 12.6 KB -
Import time ❌ ChildProcess denied: node -
Details
  • Import time regressions over 10% are flagged with ⚠️
  • Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.

jonahsnider and others added 8 commits May 13, 2026 14:15
Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
@jonahsnider jonahsnider force-pushed the aigro-4661/configure-agent-skills branch from 8576c1f to f8d67c8 Compare May 13, 2026 21:15
squiggler-app Bot and others added 2 commits May 13, 2026 21:15
Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
@jonahsnider jonahsnider marked this pull request as ready for review May 13, 2026 21:17
@jonahsnider jonahsnider requested a review from a team as a code owner May 13, 2026 21:17
Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit fffa215. Configure here.

Comment thread packages/@sanity/cli/src/commands/mcp/__tests__/configure.test.ts Outdated
Comment thread packages/@sanity/cli/src/actions/mcp/promptForMCPSetup.ts
…x or Claude

Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
@runeb
Copy link
Copy Markdown
Member

runeb commented May 13, 2026

@jonahsnider could you address CI checks and bot review findings first?

…s to `skills` agents

Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
Signed-off-by: Jonah Snider <jonah@jonahsnider.com>
@socket-security
Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addednpm/​@​vercel/​detect-agent@​1.2.39910010094100

View full report

Copy link
Copy Markdown
Contributor

@jwoods02 jwoods02 left a comment

Choose a reason for hiding this comment

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

Nice job, I've got a couple of questions around this but mainly the UX:

The proposed flow of sanity init would still show something like this as I understand it:
? Configure Sanity MCP and agent skills for your editor?
◉ Cursor
◉ Claude Code
◯ VS Code (missing credentials)

A few issues:

a) The prompt conflates two things with very different scopes.
MCP writes to global config (~/.cursor/mcp.json, ~/.claude.json). Skills writes to the project dir. The user can't tell from "Configure Sanity MCP and agent skills for your editor?" that one of these is happening to their home directory and the other to their new project. No way to separate them either — it's one checkbox per editor.

b) Already-MCP-configured editors silently re-appear, with no explanation.
The "actionable" predicate now includes editors that already have MCP set up globally, because they still need project-local skills. Those editors show up in the prompt with no label distinguishing them from unconfigured ones — getEditorLabel only handles auth expired / missing credentials, not "MCP done, skills pending". So a user who already configured MCP last week sees Cursor in the list and doesn't know what selecting it will do.

c) No success/preview text explains what happened.
After install you see:
✓ MCP configured for Cursor
✓ Installed Sanity agent skills for cursor
Two lines, lowercase agent name on the second one, no indication that skills landed in ./my-project/ vs MCP landing in ~/.cursor/. Users probably puzzle out the global-vs-local distinction once they look around.

d) --mcp / --no-mcp now controls both, as you've already noted.
Doubles down on the same conflation.

I would split these out to 2 separate questions and make skills more generic, MCP questions stays as is and "Install Sanity skills for your editors" is a single yes or no question, if yes we install so they'll work in all detected editors (it's all project level anyway) not options to install skills for each individual editor.

Also to answer the other question in your PR description about adding npx skills as a dependency:
I think we should add skills as a dependency here. We are directly calling it, so we should pin it, manage supply-chain risk, avoid "cold start" issue of downloading it during init etc It is fairly chunky 433 kB addition to our current 3.87 MB bundle but yaml is already a dep of ours so it dedupes making it a little smaller.

If we need it we should add it as a proper dep though IMO.

Happy to discuss this sync or in Slack.

Also not pulled down and tested but will do when everything is green :)

Comment thread package.json
"@commitlint/types": "^20.4.0",
"@eslint/compat": "catalog:",
"@sanity/eslint-config-cli": "workspace:*",
"@vercel/detect-agent": "^1.2.3",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I can only see this being used in the vitest config, do we need to add a new dep for this? (It's only dev dep so nit)

try {
// The cwd may not exist yet when called during `sanity init` (project
// bootstrap happens later). Create it so `npx` doesn't bail with ENOENT.
await fs.mkdir(cwd, {recursive: true})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I understand this correctly, we're installing the skills "project" level not global and doing this work before pulling the template down?

If we create the directory here and add skills, could this conflict / break the template pulling down? I thought you got an error if the directory wasn't empty.

Apologies if tested or misunderstood but worth double checking this

I think we should probably invert this and ad the skills after the project has been pulled down and inited

@jonahsnider
Copy link
Copy Markdown
Member Author

Marking this a draft for now since I don't have time to follow up on the review feedback - @jwoods02 if you could create a followup PR with your implementation that'd be great (feel free to base it on this one if that's helpful)

@jonahsnider jonahsnider marked this pull request as draft May 20, 2026 21:16
@jwoods02
Copy link
Copy Markdown
Contributor

Closing in favour of #1124

@jwoods02 jwoods02 closed this May 21, 2026
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