Repository navigation
Make saved skills name-addressable and upsert by name - #119
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR migrates saved-skill identity from generated IDs to user-scoped unique names. It adds a normalized Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP as MCP(meta_save_skill)
participant Repo as DB (mcp_skills)
participant Vector as VectorStore
Client->>MCP: meta_save_skill({ name, title, code, params, collection })
MCP->>Repo: normalizeName & getMcpSkillByName(userId, name)
alt existing skill found
MCP->>Repo: updateMcpSkill(userId, name, fields)
MCP->>Vector: upsertSkillVector(existing.id, embedText)
Vector-->>MCP: ok / error
alt vector error
MCP->>Repo: revert update to previous stored fields
MCP->>Vector: upsertSkillVector(existing.id, oldEmbedText)
Vector-->>MCP: recovered
MCP-->>Client: error (with hint to call meta_save_skill again)
else
MCP-->>Client: { name, ok: true }
end
else new skill
MCP->>Repo: insertMcpSkill(row with name)
MCP->>Vector: upsertSkillVector(new.id, embedText)
Vector-->>MCP: ok / error
alt vector error
MCP->>Repo: deleteMcpSkill(userId, name)
MCP-->>Client: error (insert rolled back)
else
MCP-->>Client: { name, ok: true }
end
end
sequenceDiagram
participant Client
participant MCP as MCP(meta_run_skill)
participant Repo as DB (mcp_skills)
participant Runner as SkillRunner
Client->>MCP: meta_run_skill({ name, params })
MCP->>Repo: normalizeName & getMcpSkillByName(userId, name)
alt not found
MCP-->>Client: { ok: false, hint: "call meta_get_skill then meta_save_skill with same name" }
else found
MCP->>Runner: execute skill code (row.code, params)
Runner-->>MCP: result
MCP-->>Client: { ok: true, result }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kentcdodds
marked this pull request as ready for review
March 31, 2026 15:23
Contributor
|
🔎 Preview deployed: https://kody-pr-119.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts (1)
5-5:⚠️ Potential issue | 🔴 CriticalMissing import for
getMcpSkillByNamecauses build failure.The function
getMcpSkillByNameis used on line 31 but not imported. This causes the TypeScript compilation to fail.🔧 Proposed fix
-import { deleteMcpSkill } from '#mcp/skills/mcp-skills-repo.ts' +import { deleteMcpSkill, getMcpSkillByName } from '#mcp/skills/mcp-skills-repo.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts` at line 5, The file imports deleteMcpSkill but also uses getMcpSkillByName on line 31; add an import for getMcpSkillByName from the same module (e.g., import { deleteMcpSkill, getMcpSkillByName } from '#mcp/skills/mcp-skills-repo.ts') so the TypeScript compiler can find the symbol and the build will succeed; ensure the import specifier matches existing exports in mcp-skills-repo.ts and update any unused-import linting if necessary.
🧹 Nitpick comments (2)
packages/worker/src/mcp/capabilities/meta/domain.ts (1)
1-10: Remove the unusedmeta-update-skill.tsfile.The
metaUpdateSkillCapabilityis neither imported nor registered indomain.ts, and there are no references to it anywhere in the codebase, confirming thatpackages/worker/src/mcp/capabilities/meta/meta-update-skill.tsis dead code and should be deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/meta/domain.ts` around lines 1 - 10, Remove the dead code file meta-update-skill.ts and any exported symbol metaUpdateSkillCapability since it is unused; locate the file that defines metaUpdateSkillCapability, delete that file, and ensure no imports or registrations reference metaUpdateSkillCapability in the meta capabilities domain (e.g., check defineDomain registration code that lists meta*Capability symbols) so the codebase compiles cleanly without the unused export.packages/worker/src/mcp/skills/mcp-skills-repo.ts (1)
74-128: Clarify thenamefield in the update signature.The
updateMcpSkillfunction now acceptsfields.nameand sets it in the UPDATE statement, which enables both upserting-in-place (same name) and the rollback scenario. However, this also technically allows renaming a skill, which could fail if the new name conflicts with an existing skill.Since the current callers always pass the same name (either from
prep.rowPayload.namewhich matches the lookup, or fromexisting.nameduring rollback), this works correctly. Consider adding a brief comment noting this constraint for future maintainers.📝 Optional: Add clarifying comment
export async function updateMcpSkill( db: D1Database, userId: string, skillName: string, fields: { + /** Must match skillName or be unique for this user to avoid constraint violation */ name: string title: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/skills/mcp-skills-repo.ts` around lines 74 - 128, The updateMcpSkill function currently accepts fields.name and uses it in the UPDATE, which allows renaming a skill (and may conflict if the new name already exists); add a concise comment above the updateMcpSkill signature (or directly above the SQL block) stating that callers must pass the current/original name as skillName and that fields.name is expected to equal skillName in normal flows (callers currently supply prep.rowPayload.name or existing.name), and note that changing fields.name effectively renames the skill and may violate uniqueness constraints if a different name is supplied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/migrations/0014-skill-names.sql`:
- Around line 1-8: The migration adds a nullable column mcp_skills.name and
backfills it but leaves it nullable, so update the migration to enforce NOT NULL
after the backfill: after the UPDATE that populates name, add a step to make the
column NOT NULL (or, if using SQLite, recreate the mcp_skills table with the
same schema but name TEXT NOT NULL and copy the data), and ensure the unique
index idx_mcp_skills_user_name still exists after the change; adjust any up/down
migration logic to maintain this invariant so future inserts cannot create NULL
names.
- Around line 3-5: The SQL backfill in 0014-skill-names.sql uses a simplistic
lower/replace formula that differs from the application's normalizeSkillName (in
skill-names.ts), causing lookup mismatches (e.g.,
meta_run_skill/meta_get_skill/meta_delete_skill); fix by creating a post-deploy
TypeScript migration that imports normalizeSkillName from skill-names.ts,
selects rows from mcp_skills where name IS NULL (or where
normalizeSkillName(title) != name), computes normalized =
normalizeSkillName(title) for each row and updates the name column to that
normalized value, and ensure the migration runs as part of deployment so DB
names exactly match the app normalization logic.
In `@packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts`:
- Around line 12-13: The file has two separate imports from the same module
('#mcp/skills/skill-mutation.ts')—consolidate them into one import statement
that imports both buildSkillEmbedTextFromStoredRow and prepareSkillPersistence
together (replace the two import lines with a single import {
buildSkillEmbedTextFromStoredRow, prepareSkillPersistence } from
'#mcp/skills/skill-mutation.ts') so there is no duplicate import.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 573-605: The TypeScript error occurs because the combined
type-guards don't properly narrow agent for both reading state and calling
setState; to fix, cast agent once to a consistent narrower type (e.g., const
regAgent = agent as McpRegistrationAgent & { setState?: (s: any) => void }) and
use regAgent for all accesses: derive searchConversationIdsWithPreamble from
regAgent.state with proper optional checks and Array.isArray, compute
includePreamble, and then call regAgent.setState if typeof regAgent.setState ===
'function'; this single consistent cast (replace multiple inline casts) will
resolve the narrowing error around searchConversationIdsWithPreamble and the
setState call.
---
Outside diff comments:
In `@packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts`:
- Line 5: The file imports deleteMcpSkill but also uses getMcpSkillByName on
line 31; add an import for getMcpSkillByName from the same module (e.g., import
{ deleteMcpSkill, getMcpSkillByName } from '#mcp/skills/mcp-skills-repo.ts') so
the TypeScript compiler can find the symbol and the build will succeed; ensure
the import specifier matches existing exports in mcp-skills-repo.ts and update
any unused-import linting if necessary.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/meta/domain.ts`:
- Around line 1-10: Remove the dead code file meta-update-skill.ts and any
exported symbol metaUpdateSkillCapability since it is unused; locate the file
that defines metaUpdateSkillCapability, delete that file, and ensure no imports
or registrations reference metaUpdateSkillCapability in the meta capabilities
domain (e.g., check defineDomain registration code that lists meta*Capability
symbols) so the codebase compiles cleanly without the unused export.
In `@packages/worker/src/mcp/skills/mcp-skills-repo.ts`:
- Around line 74-128: The updateMcpSkill function currently accepts fields.name
and uses it in the UPDATE, which allows renaming a skill (and may conflict if
the new name already exists); add a concise comment above the updateMcpSkill
signature (or directly above the SQL block) stating that callers must pass the
current/original name as skillName and that fields.name is expected to equal
skillName in normal flows (callers currently supply prep.rowPayload.name or
existing.name), and note that changing fields.name effectively renames the skill
and may violate uniqueness constraints if a different name is supplied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 088a7669-c13a-477d-a552-574cc0e86644
📒 Files selected for processing (20)
docs/agents/mcp-skills.mdpackages/worker/migrations/0014-skill-names.sqlpackages/worker/src/mcp/capabilities/meta/domain.tspackages/worker/src/mcp/capabilities/meta/meta-delete-skill.tspackages/worker/src/mcp/capabilities/meta/meta-get-skill.tspackages/worker/src/mcp/capabilities/meta/meta-run-skill.tspackages/worker/src/mcp/capabilities/meta/meta-save-skill.tspackages/worker/src/mcp/capabilities/unified-search.tspackages/worker/src/mcp/capabilities/unified-search.workers.test.tspackages/worker/src/mcp/index.tspackages/worker/src/mcp/mcp-server.mcp-e2e.test.tspackages/worker/src/mcp/skills/mcp-skills-repo.tspackages/worker/src/mcp/skills/mcp-skills-types.tspackages/worker/src/mcp/skills/skill-mutation.tspackages/worker/src/mcp/skills/skill-names.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/mcp/tools/tool-call-context.tstools/mcp-test-support.ts
Comment on lines
+3
to
+5
| UPDATE mcp_skills | ||
| SET name = lower(replace(trim(title), ' ', '-')) | ||
| WHERE name IS NULL; |
There was a problem hiding this comment.
Critical: Backfill formula doesn't match normalizeSkillName, causing lookup failures for skills with special characters.
The SQL backfill uses lower(replace(trim(title), ' ', '-')), but the application's normalizeSkillName (in skill-names.ts) applies significantly more transformations:
- NFKD normalization and diacritic stripping
- Converts ALL non-alphanumeric characters (not just spaces) to hyphens
- Collapses consecutive hyphens and trims edge hyphens
For any skill with accents, punctuation, or special characters, the backfilled name will not match what normalizeSkillName produces, causing lookup failures in meta_run_skill, meta_get_skill, and meta_delete_skill.
Example:
- Title:
"Résumé Builder!" - SQL backfill:
"résumé-builder!" normalizeSkillName:"resume-builder"
Proposed fix: Use a backfill that mimics normalizeSkillName more closely
SQLite lacks full Unicode normalization, so consider a two-phase approach:
Option 1: Fix in migration with closer approximation (won't handle all edge cases but handles common ones):
UPDATE mcp_skills
-SET name = lower(replace(trim(title), ' ', '-'))
+SET name = lower(
+ replace(
+ replace(
+ replace(
+ replace(trim(title), ' ', '-'),
+ '_', '-'
+ ),
+ '.', '-'
+ ),
+ '!', ''
+ )
+)
WHERE name IS NULL;Option 2 (recommended): Run a TypeScript migration script post-deploy that re-normalizes all skill names using the actual normalizeSkillName function to ensure consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/migrations/0014-skill-names.sql` around lines 3 - 5, The SQL
backfill in 0014-skill-names.sql uses a simplistic lower/replace formula that
differs from the application's normalizeSkillName (in skill-names.ts), causing
lookup mismatches (e.g., meta_run_skill/meta_get_skill/meta_delete_skill); fix
by creating a post-deploy TypeScript migration that imports normalizeSkillName
from skill-names.ts, selects rows from mcp_skills where name IS NULL (or where
normalizeSkillName(title) != name), computes normalized =
normalizeSkillName(title) for each row and updates the name column to that
normalized value, and ensure the migration runs as part of deployment so DB
names exactly match the app normalization logic.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Unchecked update return value allows silent no-op
- Checked the update result in the existing-skill path and throw when no rows are affected to avoid silent no-op updates and orphaned vectors.
- ✅ Fixed: Accidental leading space in execute tool description
- Removed the unintended leading space from the execute tool description line to keep formatting consistent.
Preview (13fd468018)
diff --git a/docs/agents/mcp-apps-spec-notes.md b/docs/agents/mcp-apps-spec-notes.md
--- a/docs/agents/mcp-apps-spec-notes.md
+++ b/docs/agents/mcp-apps-spec-notes.md
@@ -15,9 +15,8 @@
- The repo exposes a single generic shell via `open_generated_ui`.
- Saved apps are reopened by `app_id`; inline renders are ephemeral.
-- Saved apps are hidden from `search` by default; set
- `hidden: false` in `ui_save_app` only for reusable apps that
- should be discoverable.
+- Saved apps are hidden from `search` by default; set `hidden: false` in
+ `ui_save_app` only for reusable apps that should be discoverable.
- If an OAuth provider requires a callback URL, use a persisted hosted saved app
rather than an inline render.
- For secret-bearing requests and host approval policy, also read
diff --git a/docs/agents/mcp-skills.md b/docs/agents/mcp-skills.md
--- a/docs/agents/mcp-skills.md
+++ b/docs/agents/mcp-skills.md
@@ -15,27 +15,27 @@
(skills only when the MCP caller has user context). Search accepts an optional
`skill_collection` filter and skill hits include `collection` plus
`collectionSlug`.
-- **`meta_save_skill`** — persists code and trust flags; server infers static
- `codemode.*` usage with Acorn (after `normalizeCode` from
- `@cloudflare/codemode`, matching execute). Optional `collection` assigns the
- skill to a first-class user-defined grouping. Optional `uses_capabilities`
- merges explicit names when inference is incomplete.
+- **`meta_save_skill`** — upserts by unique per-user skill `name`, persists code
+ and trust flags, and rewrites the existing skill when that `name` already
+ exists; server infers static `codemode.*` usage with Acorn (after
+ `normalizeCode` from `@cloudflare/codemode`, matching execute). Optional
+ `collection` assigns the skill to a first-class user-defined grouping.
+ Optional `uses_capabilities` merges explicit names when inference is
+ incomplete.
- **`meta_get_skill`**, **`meta_run_skill`**, **`meta_delete_skill`** — load,
execute (same sandbox path as `execute`), or remove skill + vector row.
-- **`meta_update_skill`** — same payload as `meta_save_skill` plus `skill_id`;
- replaces code and metadata in place and re-embeds (D1 + Vectorize).
- **`meta_list_skill_collections`** — returns normalized collection names/slugs
with skill counts for browsing and filter confirmation.
When **`meta_run_skill`** fails (`ok: false`), the structured output includes a
-**`hint`** directing the client to **`meta_get_skill`** then
-**`meta_update_skill`** (or delete + save).
+**`hint`** directing the client to inspect the skill with **`meta_get_skill`**
+and then call **`meta_save_skill`** again with the same `name` to replace it.
## Parameters
-Skills can declare **parameters** when saved or updated. Each parameter includes
-`name`, `description`, `type`, and optional `required`/`default` values. Types
-are: `string`, `number`, `boolean`, or `json`.
+Skills can declare **parameters** when saved. Each parameter includes `name`,
+`description`, `type`, and optional `required`/`default` values. Types are:
+`string`, `number`, `boolean`, or `json`.
When running a skill, pass values via `meta_run_skill` **`params`**. The
codemode receives them as the `params` variable (and as the first function
@@ -43,13 +43,13 @@
rejected; defaults are applied when provided.
Example:
-`meta_run_skill({ "skill_id": "<id>", "params": { "owner": "kentcdodds" } })`
+`meta_run_skill({ "name": "github-pr-summary", "params": { "owner": "kentcdodds" } })`
## Collections
-Skills may include an optional **`collection`** string when saved or updated.
-This is a first-class grouping label for related skills, separate from built-in
-capability domains such as `coding`, `meta`, or `home`.
+Skills may include an optional **`collection`** string when saved. This is a
+first-class grouping label for related skills, separate from built-in capability
+domains such as `coding`, `meta`, or `home`.
The server stores both:
@@ -58,7 +58,8 @@
browsing UX
If no collection is provided, the skill remains ungrouped. Existing skills stay
-valid after migration and simply have `null` collection fields until updated.
+valid after migration and simply have `null` collection fields until they are
+saved again.
Use `meta_list_skill_collections({})` to inspect available groupings before
reusing one, and use `search({ query, skill_collection: "<slug>" })` to narrow
@@ -85,6 +86,5 @@
saved UI artifacts and upserts `ui_artifact_<uuid>` vectors after app-search
embed text changes or any D1/Vectorize drift.
-For broken skills, prefer **`meta_update_skill`** to fix stored code in place;
-alternatively **`meta_delete_skill`** + **`meta_save_skill`**. There is no
-versioning.
+For broken skills, prefer **`meta_save_skill`** with the same `name` to replace
+stored code in place. There is no versioning.
diff --git a/docs/setup-manifest.md b/docs/setup-manifest.md
--- a/docs/setup-manifest.md
+++ b/docs/setup-manifest.md
@@ -91,9 +91,9 @@
under `/client/v4/`.)
- `CAPABILITY_REINDEX_SECRET` (optional Worker secret; bearer auth for
`POST /__maintenance/reindex-capabilities`,
- `POST /__maintenance/reindex-skills`, and
- `POST /__maintenance/reindex-apps` to embed and upsert builtin capabilities,
- all user skills, and discoverable saved apps into Vectorize)
+ `POST /__maintenance/reindex-skills`, and `POST /__maintenance/reindex-apps`
+ to embed and upsert builtin capabilities, all user skills, and discoverable
+ saved apps into Vectorize)
Tests run with `CLOUDFLARE_ENV=test` (set by Playwright) and still read local
secrets from `packages/worker/.env`.
@@ -180,9 +180,9 @@
Cloudflare Browser Rendering `/markdown`)
- Create a Cloudflare API token with the account permissions needed for the
product APIs you want to call. This same secret already powers production
- deploys and can also be used by the `cloudflare_rest` and
- `page_to_markdown` MCP capabilities. For Browser Rendering fallback, include
- the **Browser Rendering - Edit** permission.
+ deploys and can also be used by the `cloudflare_rest` and `page_to_markdown`
+ MCP capabilities. For Browser Rendering fallback, include the **Browser
+ Rendering - Edit** permission.
- `CAPABILITY_REINDEX_SECRET` (optional)
- Generate a long random secret (for example `openssl rand -hex 32`), store it
as the repository secret `CAPABILITY_REINDEX_SECRET`, and let the deploy
diff --git a/packages/mock-servers/cloudflare/src/worker.ts b/packages/mock-servers/cloudflare/src/worker.ts
--- a/packages/mock-servers/cloudflare/src/worker.ts
+++ b/packages/mock-servers/cloudflare/src/worker.ts
@@ -284,16 +284,15 @@
{ status: 404 },
)
}
- const hasUrl = typeof payload.url === 'string' && payload.url.trim().length > 0
+ const hasUrl =
+ typeof payload.url === 'string' && payload.url.trim().length > 0
const hasHtml =
typeof payload.html === 'string' && payload.html.trim().length > 0
if (!hasUrl && !hasHtml) {
return json(
{
success: false,
- errors: [
- { code: 1003, message: 'Either url or html is required.' },
- ],
+ errors: [{ code: 1003, message: 'Either url or html is required.' }],
messages: [],
result: null,
},
diff --git a/packages/worker/client/routes/connect-secret.tsx b/packages/worker/client/routes/connect-secret.tsx
--- a/packages/worker/client/routes/connect-secret.tsx
+++ b/packages/worker/client/routes/connect-secret.tsx
@@ -749,7 +749,8 @@
value={state.name}
placeholder="api-token"
on={{
- input: (event) => setState({ name: event.currentTarget.value }),
+ input: (event) =>
+ setState({ name: event.currentTarget.value }),
}}
css={inputCss}
/>
diff --git a/packages/worker/migrations/0014-skill-names.sql b/packages/worker/migrations/0014-skill-names.sql
new file mode 100644
--- /dev/null
+++ b/packages/worker/migrations/0014-skill-names.sql
@@ -1,0 +1,49 @@
+ALTER TABLE mcp_skills ADD COLUMN name TEXT;
+
+UPDATE mcp_skills
+SET name = lower(replace(trim(title), ' ', '-'))
+WHERE name IS NULL;
+
+CREATE TABLE IF NOT EXISTS mcp_skills_next (
+ id TEXT PRIMARY KEY NOT NULL,
+ user_id TEXT NOT NULL,
+ title TEXT NOT NULL,
+ description TEXT NOT NULL,
+ keywords TEXT NOT NULL,
+ code TEXT NOT NULL,
+ search_text TEXT,
+ uses_capabilities TEXT,
+ inferred_capabilities TEXT NOT NULL,
+ inference_partial INTEGER NOT NULL DEFAULT 0,
+ read_only INTEGER NOT NULL,
+ idempotent INTEGER NOT NULL,
+ destructive INTEGER NOT NULL,
+ created_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ updated_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ parameters TEXT,
+ collection_name TEXT,
+ collection_slug TEXT,
+ name TEXT NOT NULL
+);
+
+INSERT INTO mcp_skills_next (
+ id, user_id, title, description, keywords, code, search_text,
+ uses_capabilities, inferred_capabilities, inference_partial, read_only,
+ idempotent, destructive, created_at, updated_at, parameters,
+ collection_name, collection_slug, name
+)
+SELECT
+ id, user_id, title, description, keywords, code, search_text,
+ uses_capabilities, inferred_capabilities, inference_partial, read_only,
+ idempotent, destructive, created_at, updated_at, parameters,
+ collection_name, collection_slug, name
+FROM mcp_skills;
+
+DROP TABLE mcp_skills;
+ALTER TABLE mcp_skills_next RENAME TO mcp_skills;
+
+CREATE INDEX IF NOT EXISTS idx_mcp_skills_user_id ON mcp_skills(user_id);
+CREATE INDEX IF NOT EXISTS idx_mcp_skills_user_collection_slug
+ON mcp_skills(user_id, collection_slug);
+CREATE UNIQUE INDEX IF NOT EXISTS idx_mcp_skills_user_name
+ON mcp_skills(user_id, name);
diff --git a/packages/worker/src/mcp/capabilities/coding/coding-capabilities.node.test.ts b/packages/worker/src/mcp/capabilities/coding/coding-capabilities.node.test.ts
--- a/packages/worker/src/mcp/capabilities/coding/coding-capabilities.node.test.ts
+++ b/packages/worker/src/mcp/capabilities/coding/coding-capabilities.node.test.ts
@@ -153,7 +153,9 @@
mode: 'url',
})
expect(result.markdown).toContain('# Mock Browser Rendering')
- expect(result.markdown).toContain(`source: ${mock.origin}/__mocks/markdown-error`)
+ expect(result.markdown).toContain(
+ `source: ${mock.origin}/__mocks/markdown-error`,
+ )
})
test('page_to_markdown falls back to Browser Rendering for html pages', async () => {
diff --git a/packages/worker/src/mcp/capabilities/coding/fetch-page-markdown.ts b/packages/worker/src/mcp/capabilities/coding/fetch-page-markdown.ts
--- a/packages/worker/src/mcp/capabilities/coding/fetch-page-markdown.ts
+++ b/packages/worker/src/mcp/capabilities/coding/fetch-page-markdown.ts
@@ -213,4 +213,3 @@
},
}
}
-
diff --git a/packages/worker/src/mcp/capabilities/meta/domain.ts b/packages/worker/src/mcp/capabilities/meta/domain.ts
--- a/packages/worker/src/mcp/capabilities/meta/domain.ts
+++ b/packages/worker/src/mcp/capabilities/meta/domain.ts
@@ -7,19 +7,17 @@
import { metaListSkillCollectionsCapability } from './meta-list-skill-collections.ts'
import { metaRunSkillCapability } from './meta-run-skill.ts'
import { metaSaveSkillCapability } from './meta-save-skill.ts'
-import { metaUpdateSkillCapability } from './meta-update-skill.ts'
export const metaDomain = defineDomain({
name: capabilityDomainNames.meta,
description:
- 'Save, update, list via search, load, run, and delete user-scoped codemode skills. Inspect the current runtime capability registry when search results seem incomplete. Save skills only for reasonably repeatable workflows (reusable patterns), not one-off or highly bespoke tasks.',
+ 'Save, list via search, load, run, and delete user-scoped codemode skills. Skill saves upsert by unique name per user. Inspect the current runtime capability registry when search results seem incomplete. Save skills only for reasonably repeatable workflows (reusable patterns), not one-off or highly bespoke tasks.',
keywords: ['skill', 'meta', 'save', 'recipe', 'codemode', 'capabilities'],
capabilities: [
metaListCapabilitiesCapability,
metaGetHomeConnectorStatusCapability,
metaListSkillCollectionsCapability,
metaSaveSkillCapability,
- metaUpdateSkillCapability,
metaDeleteSkillCapability,
metaGetSkillCapability,
metaRunSkillCapability,
diff --git a/packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts b/packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts
--- a/packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts
+++ b/packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts
@@ -2,7 +2,10 @@
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { type CapabilityContext } from '#mcp/capabilities/types.ts'
-import { deleteMcpSkill } from '#mcp/skills/mcp-skills-repo.ts'
+import {
+ deleteMcpSkill,
+ getMcpSkillByName,
+} from '#mcp/skills/mcp-skills-repo.ts'
import { deleteSkillVector } from '#mcp/skills/skill-vectorize.ts'
import { requireMcpUser } from './require-user.ts'
@@ -20,21 +23,26 @@
idempotent: true,
destructive: true,
inputSchema: z.object({
- skill_id: z
- .string()
- .min(1)
- .describe('Skill id returned by meta_save_skill.'),
+ name: z.string().min(1).describe('Unique lower-kebab-case skill name.'),
}),
outputSchema,
async handler(args, ctx: CapabilityContext) {
const user = requireMcpUser(ctx.callerContext)
+ const existing = await getMcpSkillByName(
+ ctx.env.APP_DB,
+ user.userId,
+ args.name,
+ )
+ if (!existing) {
+ return { deleted: false }
+ }
const removed = await deleteMcpSkill(
ctx.env.APP_DB,
user.userId,
- args.skill_id,
+ args.name,
)
if (removed) {
- await deleteSkillVector(ctx.env, args.skill_id)
+ await deleteSkillVector(ctx.env, existing.id)
}
return { deleted: removed }
},
diff --git a/packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts b/packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts
--- a/packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts
+++ b/packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts
@@ -2,7 +2,7 @@
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { type CapabilityContext } from '#mcp/capabilities/types.ts'
-import { getMcpSkillById } from '#mcp/skills/mcp-skills-repo.ts'
+import { getMcpSkillByName } from '#mcp/skills/mcp-skills-repo.ts'
import {
parseSkillParameters,
skillParameterSchema,
@@ -21,7 +21,7 @@
}
const outputSchema = z.object({
- skill_id: z.string(),
+ name: z.string(),
title: z.string(),
description: z.string(),
collection: z.string().nullable(),
@@ -51,18 +51,15 @@
idempotent: true,
destructive: false,
inputSchema: z.object({
- skill_id: z
- .string()
- .min(1)
- .describe('Skill id returned by meta_save_skill.'),
+ name: z.string().min(1).describe('Unique saved skill name.'),
}),
outputSchema,
async handler(args, ctx: CapabilityContext) {
const user = requireMcpUser(ctx.callerContext)
- const row = await getMcpSkillById(
+ const row = await getMcpSkillByName(
ctx.env.APP_DB,
user.userId,
- args.skill_id,
+ args.name,
)
if (!row) {
throw new Error('Skill not found for this user.')
@@ -72,7 +69,7 @@
const uses = parseStringArray(row.uses_capabilities)
const parameters = parseSkillParameters(row.parameters)
return {
- skill_id: row.id,
+ name: row.name,
title: row.title,
description: row.description,
collection: row.collection_name,
diff --git a/packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts b/packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts
--- a/packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts
+++ b/packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts
@@ -3,7 +3,8 @@
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { type CapabilityContext } from '#mcp/capabilities/types.ts'
-import { getMcpSkillById } from '#mcp/skills/mcp-skills-repo.ts'
+import { getMcpSkillByName } from '#mcp/skills/mcp-skills-repo.ts'
+import { normalizeSkillName } from '#mcp/skills/skill-names.ts'
import {
applySkillParameters,
parseSkillParameters,
@@ -11,7 +12,7 @@
import { requireMcpUser } from './require-user.ts'
const runFailureHint =
- 'If the saved codemode is wrong, use meta_get_skill to inspect it, then meta_update_skill to replace code and metadata in place (same skill_id), or meta_delete_skill followed by meta_save_skill.'
+ 'If the saved codemode is wrong, use meta_get_skill to inspect it, then call meta_save_skill again with the same skill name to replace the stored code and metadata.'
const outputSchema = z.object({
ok: z.boolean(),
@@ -33,16 +34,18 @@
{
name: 'meta_run_skill',
description:
- 'Execute a saved skill\'s codemode in the same sandbox as the MCP execute tool. When the skill defines parameters, pass them in params; the code receives them via the params variable or the first function argument. Example: meta_run_skill({ "skill_id": "<id>", "params": { "owner": "kentcdodds", "days": 3 } }). On failure, the structured result includes a hint for updating the skill (meta_update_skill).',
+ 'Execute a saved skill by name in the same sandbox as the MCP execute tool. Skill names are lower-kebab-case and unique per user. When the skill defines parameters, pass them in params; the code receives them via the params variable or the first function argument. Example: meta_run_skill({ "name": "github-pr-summary", "params": { "owner": "kentcdodds", "days": 3 } }). On failure, the structured result includes a hint for re-saving the skill with corrected code or metadata.',
keywords: ['skill', 'run', 'execute'],
readOnly: false,
idempotent: false,
destructive: false,
inputSchema: z.object({
- skill_id: z
+ name: z
.string()
.min(1)
- .describe('Skill id returned by meta_save_skill.'),
+ .describe(
+ 'Unique lower-kebab-case skill name to execute for the signed-in user.',
+ ),
params: z
.record(z.string(), z.unknown())
.optional()
@@ -53,10 +56,11 @@
outputSchema,
async handler(args, ctx: CapabilityContext) {
const user = requireMcpUser(ctx.callerContext)
- const row = await getMcpSkillById(
+ const skillName = normalizeSkillName(args.name)
+ const row = await getMcpSkillByName(
ctx.env.APP_DB,
user.userId,
- args.skill_id,
+ skillName,
)
if (!row) {
throw new Error('Skill not found for this user.')
diff --git a/packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts b/packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts
--- a/packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts
+++ b/packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts
@@ -2,13 +2,28 @@
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { type CapabilityContext } from '#mcp/capabilities/types.ts'
-import { insertMcpSkill, deleteMcpSkill } from '#mcp/skills/mcp-skills-repo.ts'
-import { prepareSkillPersistence } from '#mcp/skills/skill-mutation.ts'
+import {
+ deleteMcpSkill,
+ getMcpSkillByName,
+ insertMcpSkill,
+ isDuplicateSkillNameError,
+ updateMcpSkill,
+} from '#mcp/skills/mcp-skills-repo.ts'
+import {
+ buildSkillEmbedTextFromStoredRow,
+ prepareSkillPersistence,
+} from '#mcp/skills/skill-mutation.ts'
import { skillParameterSchema } from '#mcp/skills/skill-parameters.ts'
import { upsertSkillVector } from '#mcp/skills/skill-vectorize.ts'
import { requireMcpUser } from './require-user.ts'
const inputSchema = z.object({
+ name: z
+ .string()
+ .min(1)
+ .describe(
+ 'Unique lower-kebab-case skill name for this user. This is the public way to refer to the skill in search, get, run, update, and delete flows.',
+ ),
title: z.string().min(1).describe('Short title for the skill.'),
description: z
.string()
@@ -65,7 +80,7 @@
})
const outputSchema = z.object({
- skill_id: z.string(),
+ name: z.string(),
collection: z.string().nullable(),
collection_slug: z.string().nullable(),
inferred_capabilities: z.array(z.string()),
@@ -81,7 +96,7 @@
{
name: 'meta_save_skill',
description:
- 'Save a reusable codemode skill for the signed-in user when the workflow is reasonably repeatable (a pattern you expect to run again with similar structure or inputs). Do not save one-off tasks or highly bespoke work—use execute for those. To change an existing skill in place, use meta_update_skill instead.',
+ 'Save or replace a reusable codemode skill for the signed-in user by name when the workflow is reasonably repeatable (a pattern you expect to run again with similar structure or inputs). The lower-kebab-case skill name is the public identifier, so calling this again with the same name replaces the stored skill in place. Do not save one-off tasks or highly bespoke work—use execute for those.',
keywords: [
'skill',
'save',
@@ -99,17 +114,44 @@
async handler(args, ctx: CapabilityContext) {
const user = requireMcpUser(ctx.callerContext)
const prep = await prepareSkillPersistence(args)
+ const existing = await getMcpSkillByName(
+ ctx.env.APP_DB,
+ user.userId,
+ prep.rowPayload.name,
+ )
- const skillId = crypto.randomUUID()
+ const skillId = existing?.id ?? crypto.randomUUID()
const now = new Date().toISOString()
- await insertMcpSkill(ctx.env.APP_DB, {
- id: skillId,
- user_id: user.userId,
- ...prep.rowPayload,
- created_at: now,
- updated_at: now,
- })
+ if (existing) {
+ const updated = await updateMcpSkill(
+ ctx.env.APP_DB,
+ user.userId,
+ existing.name,
+ prep.rowPayload,
+ )
+ if (!updated) {
+ throw new Error('Skill not found for this user.')
+ }
+ } else {
+ try {
+ await insertMcpSkill(ctx.env.APP_DB, {
+ id: skillId,
+ user_id: user.userId,
+ ...prep.rowPayload,
+ created_at: now,
+ updated_at: now,
+ })
+ } catch (error) {
+ if (isDuplicateSkillNameError(error)) {
+ throw new Error(
+ `A saved skill named "${prep.rowPayload.name}" already exists for this user.`,
+ )
+ }
+ throw error
+ }
+ }
+
try {
await upsertSkillVector(ctx.env, {
skillId,
@@ -118,12 +160,43 @@
collectionSlug: prep.rowPayload.collection_slug,
})
} catch (cause) {
- await deleteMcpSkill(ctx.env.APP_DB, user.userId, skillId)
+ if (existing) {
+ await updateMcpSkill(ctx.env.APP_DB, user.userId, existing.name, {
+ name: existing.name,
+ title: existing.title,
+ description: existing.description,
+ collection_name: existing.collection_name,
+ collection_slug: existing.collection_slug,
+ keywords: existing.keywords,
+ code: existing.code,
+ search_text: existing.search_text,
+ uses_capabilities: existing.uses_capabilities,
+ parameters: existing.parameters,
+ inferred_capabilities: existing.inferred_capabilities,
+ inference_partial: existing.inference_partial,
+ read_only: existing.read_only,
+ idempotent: existing.idempotent,
+ destructive: existing.destructive,
+ })
+ const oldEmbed = await buildSkillEmbedTextFromStoredRow(existing)
+ await upsertSkillVector(ctx.env, {
+ skillId: existing.id,
+ userId: user.userId,
+ embedText: oldEmbed,
+ collectionSlug: existing.collection_slug,
+ })
+ } else {
+ await deleteMcpSkill(
+ ctx.env.APP_DB,
+ user.userId,
+ prep.rowPayload.name,
+ )
+ }
throw cause
}
return {
- skill_id: skillId,
+ name: prep.rowPayload.name,
collection: prep.rowPayload.collection_name,
collection_slug: prep.rowPayload.collection_slug,
inferred_capabilities: prep.merged,
diff --git a/packages/worker/src/mcp/capabilities/meta/meta-update-skill.ts b/packages/worker/src/mcp/capabilities/meta/meta-update-skill.ts
deleted file mode 100644
--- a/packages/worker/src/mcp/capabilities/meta/meta-update-skill.ts
+++ /dev/null
@@ -1,153 +1,0 @@
-import { z } from 'zod'
-import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
-import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
-import { type CapabilityContext } from '#mcp/capabilities/types.ts'
-import { getMcpSkillById, updateMcpSkill } from '#mcp/skills/mcp-skills-repo.ts'
-import {
- buildSkillEmbedTextFromStoredRow,
- prepareSkillPersistence,
-} from '#mcp/skills/skill-mutation.ts'
-import { skillParameterSchema } from '#mcp/skills/skill-parameters.ts'
-import { upsertSkillVector } from '#mcp/skills/skill-vectorize.ts'
-import { requireMcpUser } from './require-user.ts'
-
-const inputSchema = z.object({
- skill_id: z
- .string()
- .min(1)
- .describe('Existing skill id (same as returned by meta_save_skill).'),
- title: z.string().min(1).describe('Short title for the skill.'),
- description: z
- .string()
- .min(1)
- .describe('What this skill does (shown in search and to users).'),
- collection: z
- .string()
- .optional()
- .describe(
- 'Optional user-defined collection/domain for grouping related saved skills.',
- ),
- keywords: z
- .array(z.string())
- .describe('Extra search keywords for this skill.'),
- code: z
- .string()
- .min(1)
- .describe(
- 'Replacement codemode snippet as accepted by execute (async arrow or equivalent after normalization).',
- ),
- search_text: z.string().optional(),
- uses_capabilities: z.array(z.string()).optional(),
- parameters: z
- .array(skillParameterSchema)
- .optional()
- .describe('Replacement parameter definitions for the skill.'),
- read_only: z.boolean(),
- idempotent: z.boolean(),
- destructive: z.boolean(),
-})
-
-const outputSchema = z.object({
- skill_id: z.string(),
- collection: z.string().nullable(),
- collection_slug: z.string().nullable(),
- inferred_capabilities: z.array(z.string()),
- inference_partial: z.boolean(),
- destructive_derived: z.boolean(),
- read_only_derived: z.boolean().nullable(),
- idempotent_derived: z.boolean().nullable(),
- warnings: z.array(z.string()).optional(),
-})
-
-export const metaUpdateSkillCapability = defineDomainCapability(
- capabilityDomainNames.meta,
- {
- name: 'meta_update_skill',
- description:
- 'Replace fields and codemode for an existing skill (same skill_id). Re-runs inference, validation, D1 update, and Vectorize upsert. Use when meta_run_skill fails due to bad stored code.',
- keywords: [
- 'skill',
- 'update',
- 'edit',
- 'replace',
- 'fix',
- 'codemode',
- 'collection',
- ],
- readOnly: false,
- idempotent: true,
- destructive: false,
- inputSchema,
- outputSchema,
- async handler(args, ctx: CapabilityContext) {
- const user = requireMcpUser(ctx.callerContext)
- const existing = await getMcpSkillById(
- ctx.env.APP_DB,
- user.userId,
- args.skill_id,
- )
- if (!existing) {
- throw new Error('Skill not found for this user.')
- }
-
- const { skill_id, ...rest } = args
- const prep = await prepareSkillPersistence(rest)
-
- const updated = await updateMcpSkill(
- ctx.env.APP_DB,
- user.userId,
- skill_id,
- prep.rowPayload,
- )
- if (!updated) {
- throw new Error('Skill not found for this user.')
- }
-
- try {
- await upsertSkillVector(ctx.env, {
- skillId: skill_id,
- userId: user.userId,
- embedText: prep.embedText,
- collectionSlug: prep.rowPayload.collection_slug,
- })
- } catch (cause) {
- await updateMcpSkill(ctx.env.APP_DB, user.userId, skill_id, {
- title: existing.title,
- description: existing.description,
- collection_name: existing.collection_name,
- collection_slug: existing.collection_slug,
- keywords: existing.keywords,
- code: existing.code,
- search_text: existing.search_text,
- uses_capabilities: existing.uses_capabilities,
- parameters: existing.parameters,
- inferred_capabilities: existing.inferred_capabilities,
- inference_partial: existing.inference_partial,
- read_only: existing.read_only,
- idempotent: existing.idempotent,
- destructive: existing.destructive,
- })
- const oldEmbed = await buildSkillEmbedTextFromStoredRow(existing)
- await upsertSkillVector(ctx.env, {
- skillId: skill_id,
- userId: user.userId,
- embedText: oldEmbed,
- collectionSlug: existing.collection_slug,
- })
- throw cause
- }
-
- return {
- skill_id,
- collection: prep.rowPayload.collection_name,
- collection_slug: prep.rowPayload.collection_slug,
- inferred_capabilities: prep.merged,
- inference_partial: prep.inferencePartial,
- destructive_derived: prep.derived.destructiveDerived,
- read_only_derived: prep.derived.readOnlyDerived,
- idempotent_derived: prep.derived.idempotentDerived,
- ...(prep.warnings.length > 0 ? { warnings: prep.warnings } : {}),
- }
- },
- },
-)
\ No newline at end of file
diff --git a/packages/worker/src/mcp/capabilities/unified-search.ts b/packages/worker/src/mcp/capabilities/unified-search.ts
--- a/packages/worker/src/mcp/capabilities/unified-search.ts
+++ b/packages/worker/src/mcp/capabilities/unified-search.ts
@@ -14,10 +14,8 @@
import { type CapabilitySpec } from './types.ts'
import { buildSkillEmbedText } from '#mcp/skills/skill-embed-and-flags.ts'
import { type McpSkillRow } from '#mcp/skills/mcp-skills-types.ts'
+import { parseSkillParameters } from '#mcp/skills/skill-parameters.ts'
import {
- parseSkillParameters,
-} from '#mcp/skills/skill-parameters.ts'
-import {
type SecretMetadata,
type SecretSearchRow,
} from '#mcp/secrets/types.ts'
@@ -37,8 +35,8 @@
}
}
-function buildSkillUsage(skillId: string): string {
- const runArgs = JSON.stringify({ skill_id: skillId })
+function buildSkillUsage(skillName: string): string {
+ const runArgs = JSON.stringify({ name: skillName })
return `Run with meta_run_skill: ${runArgs}. Optionally include "params": { ... }. To inspect code, call meta_get_skill then execute.`
}
@@ -72,7 +70,7 @@
export type SkillSearchHitSummary = {
type: 'skill'
- skillId: string
+ skillName: string
domain: 'meta'
collection: string | null
collectionSlug: string | null
@@ -145,14 +143,14 @@
const keywords = parseJsonStringArray(row.keywords)
return {
type: 'skill',
- skillId: row.id,
+ skillName: row.name,
domain: 'meta',
collection: row.collection_name,
collectionSlug: row.collection_slug,
title: row.title,
description: row.description,
keywords,
- usage: buildSkillUsage(row.id),
+ usage: buildSkillUsage(row.name),
readOnly: row.read_only === 1,
idempotent: row.idempotent === 1,
destructive: row.destructive === 1,
@@ -413,7 +411,7 @@
}
const capKeys = capResult.matches.map((m) => `c:${m.name}`)
- const skillKeys = skillResult.matches.map((m) => `s:${m.skillId}`)
+ const skillKeys = skillResult.matches.map((m) => `s:${m.skillName}`)
... diff truncated: showing 800 of 1909 linesYou can send follow-ups to this agent here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/setup-manifest.md (1)
94-96: Consider hyphenating "built-in" for consistency with standard documentation style.The adjective form is typically hyphenated as "built-in capabilities" rather than "builtin capabilities."
`POST /__maintenance/reindex-skills`, and `POST /__maintenance/reindex-apps` - to embed and upsert builtin capabilities, all user skills, and discoverable + to embed and upsert built-in capabilities, all user skills, and discoverable saved apps into Vectorize)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/setup-manifest.md` around lines 94 - 96, The phrase "builtin capabilities" should be changed to the hyphenated adjective form "built-in capabilities" for consistency; update the instance in this sentence containing `POST /__maintenance/reindex-skills` and `POST /__maintenance/reindex-apps` (and any other occurrences of the exact token "builtin capabilities" in docs/setup-manifest.md) to read "built-in capabilities".packages/worker/src/mcp/skills/mcp-skills-repo.ts (1)
24-54: Normalizenameon writes inside the repo layer.Line 63 and Line 141 normalize lookup keys, but Line 37 and Line 113 persist
row.name/fields.nameverbatim. One missed normalization upstream will create rows that the name-based APIs can no longer read or delete. Normalizing before binding here keeps the repository contract consistent.♻️ Proposed fix
export async function insertMcpSkill( db: D1Database, row: Omit<McpSkillRow, 'created_at' | 'updated_at'> & { created_at?: string updated_at?: string }, ): Promise<void> { const now = new Date().toISOString() + const normalizedName = normalizeSkillName(row.name) await db .prepare( `INSERT INTO mcp_skills ( id, user_id, name, title, description, keywords, code, search_text, uses_capabilities, parameters, collection_name, collection_slug, inferred_capabilities, inference_partial, read_only, idempotent, destructive, created_at, updated_at ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, ) .bind( row.id, row.user_id, - row.name, + normalizedName, row.title, row.description, row.keywords, row.code, row.search_text ?? null, @@ ): Promise<boolean> { const now = new Date().toISOString() const normalizedSkillName = normalizeSkillName(skillName) + const normalizedNewName = normalizeSkillName(fields.name) const out = await db .prepare( `UPDATE mcp_skills SET name = ?, title = ?, description = ?, keywords = ?, code = ?, search_text = ?, uses_capabilities = ?, parameters = ?, collection_name = ?, collection_slug = ?, @@ ) .bind( - fields.name, + normalizedNewName, fields.title, fields.description, fields.keywords, fields.code, fields.search_text,Also applies to: 101-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/skills/mcp-skills-repo.ts` around lines 24 - 54, The INSERTs are persisting row.name/fields.name verbatim while lookups normalize names elsewhere, so normalize the name before binding in the repo insert code to keep the repository contract consistent; locate the bind calls that include row.name (and the similar one with fields.name) in mcp-skills-repo.ts and replace the direct value with the normalized value using the same normalization helper used for lookups (the normalization function referenced at the lookup sites around lines 63 and 141), ensuring created/updated rows store the normalized name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts`:
- Around line 117-150: The current save flow (getMcpSkillByName, insertMcpSkill,
updateMcpSkill) is racy: concurrent inserts can cause duplicate errors and
updateMcpSkill can silently return 0 changes. Replace this with a single upsert
at the repo level (or implement retry-on-duplicate: on isDuplicateSkillNameError
from insertMcpSkill call, run updateMcpSkill and if that update returns 0
changes abort/throw), and ensure skillId is taken from the actual written row
(returned by the upsert or confirmed update) rather than the stale existing
variable (prep.rowPayload.name and skillId must reflect the final DB row).
- Around line 159-191: The current rollback uses a stale snapshot and
deletes/overwrites by name which can clobber concurrent updates; change the
compensation in the catch so you first fetch the current row (by unique id or
version) and confirm it still matches the exact row written by this request
(e.g., compare existing.id or a row version/timestamp against the id/version you
recorded when writing prep.rowPayload), only then call updateMcpSkill or
deleteMcpSkill; additionally, only call upsertSkillVector to restore the old
vector after that guarded revert/delete succeeds — if the current row no longer
matches, skip the revert and skip restoring the old embed to avoid overwriting
another request’s successful save.
---
Nitpick comments:
In `@docs/setup-manifest.md`:
- Around line 94-96: The phrase "builtin capabilities" should be changed to the
hyphenated adjective form "built-in capabilities" for consistency; update the
instance in this sentence containing `POST /__maintenance/reindex-skills` and
`POST /__maintenance/reindex-apps` (and any other occurrences of the exact token
"builtin capabilities" in docs/setup-manifest.md) to read "built-in
capabilities".
In `@packages/worker/src/mcp/skills/mcp-skills-repo.ts`:
- Around line 24-54: The INSERTs are persisting row.name/fields.name verbatim
while lookups normalize names elsewhere, so normalize the name before binding in
the repo insert code to keep the repository contract consistent; locate the bind
calls that include row.name (and the similar one with fields.name) in
mcp-skills-repo.ts and replace the direct value with the normalized value using
the same normalization helper used for lookups (the normalization function
referenced at the lookup sites around lines 63 and 141), ensuring
created/updated rows store the normalized name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a00bea06-b4b3-4431-b5ec-6e691dd059d4
📒 Files selected for processing (20)
docs/agents/mcp-apps-spec-notes.mddocs/agents/mcp-skills.mddocs/setup-manifest.mdpackages/mock-servers/cloudflare/src/worker.tspackages/worker/client/routes/connect-secret.tsxpackages/worker/migrations/0014-skill-names.sqlpackages/worker/src/mcp/capabilities/coding/coding-capabilities.node.test.tspackages/worker/src/mcp/capabilities/coding/fetch-page-markdown.tspackages/worker/src/mcp/capabilities/meta/meta-delete-skill.tspackages/worker/src/mcp/capabilities/meta/meta-get-skill.tspackages/worker/src/mcp/capabilities/meta/meta-save-skill.tspackages/worker/src/mcp/capabilities/meta/meta-update-skill.tspackages/worker/src/mcp/capabilities/unified-search.tspackages/worker/src/mcp/mcp-server.mcp-e2e.test.tspackages/worker/src/mcp/observability.workers.test.tspackages/worker/src/mcp/skills/mcp-skills-repo.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/mcp/ui-artifact-reindex.tspackages/worker/src/mcp/ui-artifacts-repo.ts
💤 Files with no reviewable changes (2)
- packages/worker/src/mcp/capabilities/coding/fetch-page-markdown.ts
- packages/worker/src/mcp/capabilities/meta/meta-update-skill.ts
✅ Files skipped from review due to trivial changes (10)
- docs/agents/mcp-apps-spec-notes.md
- packages/worker/src/mcp/capabilities/coding/coding-capabilities.node.test.ts
- packages/worker/src/mcp/observability.workers.test.ts
- packages/worker/client/routes/connect-secret.tsx
- packages/mock-servers/cloudflare/src/worker.ts
- packages/worker/src/mcp/ui-artifact-reindex.ts
- packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts
- packages/worker/migrations/0014-skill-names.sql
- packages/worker/src/mcp/ui-artifacts-repo.ts
- docs/agents/mcp-skills.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/worker/src/mcp/capabilities/unified-search.ts
- packages/worker/src/mcp/tools/search.ts
- packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts
- packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts
Comment on lines
+117
to
+150
| const existing = await getMcpSkillByName( | ||
| ctx.env.APP_DB, | ||
| user.userId, | ||
| prep.rowPayload.name, | ||
| ) | ||
|
|
||
| const skillId = crypto.randomUUID() | ||
| const skillId = existing?.id ?? crypto.randomUUID() | ||
| const now = new Date().toISOString() | ||
| await insertMcpSkill(ctx.env.APP_DB, { | ||
| id: skillId, | ||
| user_id: user.userId, | ||
| ...prep.rowPayload, | ||
| created_at: now, | ||
| updated_at: now, | ||
| }) | ||
|
|
||
| if (existing) { | ||
| await updateMcpSkill( | ||
| ctx.env.APP_DB, | ||
| user.userId, | ||
| existing.name, | ||
| prep.rowPayload, | ||
| ) | ||
| } else { | ||
| try { | ||
| await insertMcpSkill(ctx.env.APP_DB, { | ||
| id: skillId, | ||
| user_id: user.userId, | ||
| ...prep.rowPayload, | ||
| created_at: now, | ||
| updated_at: now, | ||
| }) | ||
| } catch (error) { | ||
| if (isDuplicateSkillNameError(error)) { | ||
| throw new Error( | ||
| `A saved skill named "${prep.rowPayload.name}" already exists for this user.`, | ||
| ) | ||
| } | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
Make the write step an actual upsert.
If two requests save the same new name concurrently, both can miss existing; one inserts and the other throws "already exists" instead of replacing it. There is a second race on the update path: updateMcpSkill(...) can return false when the row changes between Line 117 and Line 127, but that result is ignored and skillId still comes from the stale existing row. That can send vectorization down a dead id and return success with no matching DB row. Collapse this into a single write in the repo, or at least retry the duplicate insert as an update and abort when the update reports 0 changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts` around lines
117 - 150, The current save flow (getMcpSkillByName, insertMcpSkill,
updateMcpSkill) is racy: concurrent inserts can cause duplicate errors and
updateMcpSkill can silently return 0 changes. Replace this with a single upsert
at the repo level (or implement retry-on-duplicate: on isDuplicateSkillNameError
from insertMcpSkill call, run updateMcpSkill and if that update returns 0
changes abort/throw), and ensure skillId is taken from the actual written row
(returned by the upsert or confirmed update) rather than the stale existing
variable (prep.rowPayload.name and skillId must reflect the final DB row).
Comment on lines
159
to
+191
| } catch (cause) { | ||
| await deleteMcpSkill(ctx.env.APP_DB, user.userId, skillId) | ||
| if (existing) { | ||
| await updateMcpSkill(ctx.env.APP_DB, user.userId, existing.name, { | ||
| name: existing.name, | ||
| title: existing.title, | ||
| description: existing.description, | ||
| collection_name: existing.collection_name, | ||
| collection_slug: existing.collection_slug, | ||
| keywords: existing.keywords, | ||
| code: existing.code, | ||
| search_text: existing.search_text, | ||
| uses_capabilities: existing.uses_capabilities, | ||
| parameters: existing.parameters, | ||
| inferred_capabilities: existing.inferred_capabilities, | ||
| inference_partial: existing.inference_partial, | ||
| read_only: existing.read_only, | ||
| idempotent: existing.idempotent, | ||
| destructive: existing.destructive, | ||
| }) | ||
| const oldEmbed = await buildSkillEmbedTextFromStoredRow(existing) | ||
| await upsertSkillVector(ctx.env, { | ||
| skillId: existing.id, | ||
| userId: user.userId, | ||
| embedText: oldEmbed, | ||
| collectionSlug: existing.collection_slug, | ||
| }) | ||
| } else { | ||
| await deleteMcpSkill( | ||
| ctx.env.APP_DB, | ||
| user.userId, | ||
| prep.rowPayload.name, | ||
| ) | ||
| } |
There was a problem hiding this comment.
The rollback can clobber another request’s successful save.
The compensation logic uses a stale snapshot and deletes by name, not by the exact row version written by this request. If another save updates the same skill after the DB write but before the vector step fails, the existing branch writes old values back, and the else branch can delete the row that the other request just saved. Only revert/delete when the row still matches the version/id written by this request, and only restore the old vector when that guarded revert succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts` around lines
159 - 191, The current rollback uses a stale snapshot and deletes/overwrites by
name which can clobber concurrent updates; change the compensation in the catch
so you first fetch the current row (by unique id or version) and confirm it
still matches the exact row written by this request (e.g., compare existing.id
or a row version/timestamp against the id/version you recorded when writing
prep.rowPayload), only then call updateMcpSkill or deleteMcpSkill; additionally,
only call upsertSkillVector to restore the old vector after that guarded
revert/delete succeeds — if the current row no longer matches, skip the revert
and skip restoring the old embed to avoid overwriting another request’s
successful save.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
Preview (987d70d95e)diff --git a/docs/agents/mcp-apps-spec-notes.md b/docs/agents/mcp-apps-spec-notes.md
--- a/docs/agents/mcp-apps-spec-notes.md
+++ b/docs/agents/mcp-apps-spec-notes.md
@@ -15,9 +15,8 @@
- The repo exposes a single generic shell via `open_generated_ui`.
- Saved apps are reopened by `app_id`; inline renders are ephemeral.
-- Saved apps are hidden from `search` by default; set
- `hidden: false` in `ui_save_app` only for reusable apps that
- should be discoverable.
+- Saved apps are hidden from `search` by default; set `hidden: false` in
+ `ui_save_app` only for reusable apps that should be discoverable.
- If an OAuth provider requires a callback URL, use a persisted hosted saved app
rather than an inline render.
- For secret-bearing requests and host approval policy, also read
diff --git a/docs/agents/mcp-skills.md b/docs/agents/mcp-skills.md
--- a/docs/agents/mcp-skills.md
+++ b/docs/agents/mcp-skills.md
@@ -15,27 +15,27 @@
(skills only when the MCP caller has user context). Search accepts an optional
`skill_collection` filter and skill hits include `collection` plus
`collectionSlug`.
-- **`meta_save_skill`** — persists code and trust flags; server infers static
- `codemode.*` usage with Acorn (after `normalizeCode` from
- `@cloudflare/codemode`, matching execute). Optional `collection` assigns the
- skill to a first-class user-defined grouping. Optional `uses_capabilities`
- merges explicit names when inference is incomplete.
+- **`meta_save_skill`** — upserts by unique per-user skill `name`, persists code
+ and trust flags, and rewrites the existing skill when that `name` already
+ exists; server infers static `codemode.*` usage with Acorn (after
+ `normalizeCode` from `@cloudflare/codemode`, matching execute). Optional
+ `collection` assigns the skill to a first-class user-defined grouping.
+ Optional `uses_capabilities` merges explicit names when inference is
+ incomplete.
- **`meta_get_skill`**, **`meta_run_skill`**, **`meta_delete_skill`** — load,
execute (same sandbox path as `execute`), or remove skill + vector row.
-- **`meta_update_skill`** — same payload as `meta_save_skill` plus `skill_id`;
- replaces code and metadata in place and re-embeds (D1 + Vectorize).
- **`meta_list_skill_collections`** — returns normalized collection names/slugs
with skill counts for browsing and filter confirmation.
When **`meta_run_skill`** fails (`ok: false`), the structured output includes a
-**`hint`** directing the client to **`meta_get_skill`** then
-**`meta_update_skill`** (or delete + save).
+**`hint`** directing the client to inspect the skill with **`meta_get_skill`**
+and then call **`meta_save_skill`** again with the same `name` to replace it.
## Parameters
-Skills can declare **parameters** when saved or updated. Each parameter includes
-`name`, `description`, `type`, and optional `required`/`default` values. Types
-are: `string`, `number`, `boolean`, or `json`.
+Skills can declare **parameters** when saved. Each parameter includes `name`,
+`description`, `type`, and optional `required`/`default` values. Types are:
+`string`, `number`, `boolean`, or `json`.
When running a skill, pass values via `meta_run_skill` **`params`**. The
codemode receives them as the `params` variable (and as the first function
@@ -43,13 +43,13 @@
rejected; defaults are applied when provided.
Example:
-`meta_run_skill({ "skill_id": "<id>", "params": { "owner": "kentcdodds" } })`
+`meta_run_skill({ "name": "github-pr-summary", "params": { "owner": "kentcdodds" } })`
## Collections
-Skills may include an optional **`collection`** string when saved or updated.
-This is a first-class grouping label for related skills, separate from built-in
-capability domains such as `coding`, `meta`, or `home`.
+Skills may include an optional **`collection`** string when saved. This is a
+first-class grouping label for related skills, separate from built-in capability
+domains such as `coding`, `meta`, or `home`.
The server stores both:
@@ -58,7 +58,8 @@
browsing UX
If no collection is provided, the skill remains ungrouped. Existing skills stay
-valid after migration and simply have `null` collection fields until updated.
+valid after migration and simply have `null` collection fields until they are
+saved again.
Use `meta_list_skill_collections({})` to inspect available groupings before
reusing one, and use `search({ query, skill_collection: "<slug>" })` to narrow
@@ -85,6 +86,5 @@
saved UI artifacts and upserts `ui_artifact_<uuid>` vectors after app-search
embed text changes or any D1/Vectorize drift.
-For broken skills, prefer **`meta_update_skill`** to fix stored code in place;
-alternatively **`meta_delete_skill`** + **`meta_save_skill`**. There is no
-versioning.
+For broken skills, prefer **`meta_save_skill`** with the same `name` to replace
+stored code in place. There is no versioning.
diff --git a/docs/setup-manifest.md b/docs/setup-manifest.md
--- a/docs/setup-manifest.md
+++ b/docs/setup-manifest.md
@@ -91,9 +91,9 @@
under `/client/v4/`.)
- `CAPABILITY_REINDEX_SECRET` (optional Worker secret; bearer auth for
`POST /__maintenance/reindex-capabilities`,
- `POST /__maintenance/reindex-skills`, and
- `POST /__maintenance/reindex-apps` to embed and upsert builtin capabilities,
- all user skills, and discoverable saved apps into Vectorize)
+ `POST /__maintenance/reindex-skills`, and `POST /__maintenance/reindex-apps`
+ to embed and upsert builtin capabilities, all user skills, and discoverable
+ saved apps into Vectorize)
Tests run with `CLOUDFLARE_ENV=test` (set by Playwright) and still read local
secrets from `packages/worker/.env`.
@@ -180,9 +180,9 @@
Cloudflare Browser Rendering `/markdown`)
- Create a Cloudflare API token with the account permissions needed for the
product APIs you want to call. This same secret already powers production
- deploys and can also be used by the `cloudflare_rest` and
- `page_to_markdown` MCP capabilities. For Browser Rendering fallback, include
- the **Browser Rendering - Edit** permission.
+ deploys and can also be used by the `cloudflare_rest` and `page_to_markdown`
+ MCP capabilities. For Browser Rendering fallback, include the **Browser
+ Rendering - Edit** permission.
- `CAPABILITY_REINDEX_SECRET` (optional)
- Generate a long random secret (for example `openssl rand -hex 32`), store it
as the repository secret `CAPABILITY_REINDEX_SECRET`, and let the deploy
diff --git a/packages/mock-servers/cloudflare/src/worker.ts b/packages/mock-servers/cloudflare/src/worker.ts
--- a/packages/mock-servers/cloudflare/src/worker.ts
+++ b/packages/mock-servers/cloudflare/src/worker.ts
@@ -284,16 +284,15 @@
{ status: 404 },
)
}
- const hasUrl = typeof payload.url === 'string' && payload.url.trim().length > 0
+ const hasUrl =
+ typeof payload.url === 'string' && payload.url.trim().length > 0
const hasHtml =
typeof payload.html === 'string' && payload.html.trim().length > 0
if (!hasUrl && !hasHtml) {
return json(
{
success: false,
- errors: [
- { code: 1003, message: 'Either url or html is required.' },
- ],
+ errors: [{ code: 1003, message: 'Either url or html is required.' }],
messages: [],
result: null,
},
diff --git a/packages/worker/client/routes/connect-secret.tsx b/packages/worker/client/routes/connect-secret.tsx
--- a/packages/worker/client/routes/connect-secret.tsx
+++ b/packages/worker/client/routes/connect-secret.tsx
@@ -749,7 +749,8 @@
value={state.name}
placeholder="api-token"
on={{
- input: (event) => setState({ name: event.currentTarget.value }),
+ input: (event) =>
+ setState({ name: event.currentTarget.value }),
}}
css={inputCss}
/>
diff --git a/packages/worker/migrations/0014-skill-names.sql b/packages/worker/migrations/0014-skill-names.sql
new file mode 100644
--- /dev/null
+++ b/packages/worker/migrations/0014-skill-names.sql
@@ -1,0 +1,74 @@
+ALTER TABLE mcp_skills ADD COLUMN name TEXT;
+
+WITH base_names AS (
+ SELECT
+ id,
+ user_id,
+ lower(replace(trim(title), ' ', '-')) AS base_name
+ FROM mcp_skills
+ WHERE name IS NULL
+),
+ranked AS (
+ SELECT
+ id,
+ user_id,
+ base_name,
+ COUNT(*) OVER (PARTITION BY user_id, base_name) AS name_count,
+ ROW_NUMBER() OVER (PARTITION BY user_id, base_name ORDER BY id) AS name_index
+ FROM base_names
+)
+UPDATE mcp_skills
+SET name = (
+ SELECT
+ CASE
+ WHEN ranked.name_count = 1 OR ranked.name_index = 1 THEN ranked.base_name
+ ELSE ranked.base_name || '-' || ranked.name_index
+ END
+ FROM ranked
+ WHERE ranked.id = mcp_skills.id
+)
+WHERE name IS NULL;
+
+CREATE TABLE IF NOT EXISTS mcp_skills_next (
+ id TEXT PRIMARY KEY NOT NULL,
+ user_id TEXT NOT NULL,
+ title TEXT NOT NULL,
+ description TEXT NOT NULL,
+ keywords TEXT NOT NULL,
+ code TEXT NOT NULL,
+ search_text TEXT,
+ uses_capabilities TEXT,
+ inferred_capabilities TEXT NOT NULL,
+ inference_partial INTEGER NOT NULL DEFAULT 0,
+ read_only INTEGER NOT NULL,
+ idempotent INTEGER NOT NULL,
+ destructive INTEGER NOT NULL,
+ created_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ updated_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP),
+ parameters TEXT,
+ collection_name TEXT,
+ collection_slug TEXT,
+ name TEXT NOT NULL
+);
+
+INSERT INTO mcp_skills_next (
+ id, user_id, title, description, keywords, code, search_text,
+ uses_capabilities, inferred_capabilities, inference_partial, read_only,
+ idempotent, destructive, created_at, updated_at, parameters,
+ collection_name, collection_slug, name
+)
+SELECT
+ id, user_id, title, description, keywords, code, search_text,
+ uses_capabilities, inferred_capabilities, inference_partial, read_only,
+ idempotent, destructive, created_at, updated_at, parameters,
+ collection_name, collection_slug, name
+FROM mcp_skills;
+
+DROP TABLE mcp_skills;
+ALTER TABLE mcp_skills_next RENAME TO mcp_skills;
+
+CREATE INDEX IF NOT EXISTS idx_mcp_skills_user_id ON mcp_skills(user_id);
+CREATE INDEX IF NOT EXISTS idx_mcp_skills_user_collection_slug
+ON mcp_skills(user_id, collection_slug);
+CREATE UNIQUE INDEX IF NOT EXISTS idx_mcp_skills_user_name
+ON mcp_skills(user_id, name);
diff --git a/packages/worker/src/mcp/capabilities/coding/coding-capabilities.node.test.ts b/packages/worker/src/mcp/capabilities/coding/coding-capabilities.node.test.ts
--- a/packages/worker/src/mcp/capabilities/coding/coding-capabilities.node.test.ts
+++ b/packages/worker/src/mcp/capabilities/coding/coding-capabilities.node.test.ts
@@ -153,7 +153,9 @@
mode: 'url',
})
expect(result.markdown).toContain('# Mock Browser Rendering')
- expect(result.markdown).toContain(`source: ${mock.origin}/__mocks/markdown-error`)
+ expect(result.markdown).toContain(
+ `source: ${mock.origin}/__mocks/markdown-error`,
+ )
})
test('page_to_markdown falls back to Browser Rendering for html pages', async () => {
diff --git a/packages/worker/src/mcp/capabilities/coding/fetch-page-markdown.ts b/packages/worker/src/mcp/capabilities/coding/fetch-page-markdown.ts
--- a/packages/worker/src/mcp/capabilities/coding/fetch-page-markdown.ts
+++ b/packages/worker/src/mcp/capabilities/coding/fetch-page-markdown.ts
@@ -213,4 +213,3 @@
},
}
}
-
diff --git a/packages/worker/src/mcp/capabilities/meta/domain.ts b/packages/worker/src/mcp/capabilities/meta/domain.ts
--- a/packages/worker/src/mcp/capabilities/meta/domain.ts
+++ b/packages/worker/src/mcp/capabilities/meta/domain.ts
@@ -7,19 +7,17 @@
import { metaListSkillCollectionsCapability } from './meta-list-skill-collections.ts'
import { metaRunSkillCapability } from './meta-run-skill.ts'
import { metaSaveSkillCapability } from './meta-save-skill.ts'
-import { metaUpdateSkillCapability } from './meta-update-skill.ts'
export const metaDomain = defineDomain({
name: capabilityDomainNames.meta,
description:
- 'Save, update, list via search, load, run, and delete user-scoped codemode skills. Inspect the current runtime capability registry when search results seem incomplete. Save skills only for reasonably repeatable workflows (reusable patterns), not one-off or highly bespoke tasks.',
+ 'Save, list via search, load, run, and delete user-scoped codemode skills. Skill saves upsert by unique name per user. Inspect the current runtime capability registry when search results seem incomplete. Save skills only for reasonably repeatable workflows (reusable patterns), not one-off or highly bespoke tasks.',
keywords: ['skill', 'meta', 'save', 'recipe', 'codemode', 'capabilities'],
capabilities: [
metaListCapabilitiesCapability,
metaGetHomeConnectorStatusCapability,
metaListSkillCollectionsCapability,
metaSaveSkillCapability,
- metaUpdateSkillCapability,
metaDeleteSkillCapability,
metaGetSkillCapability,
metaRunSkillCapability,
diff --git a/packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts b/packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts
--- a/packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts
+++ b/packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts
@@ -2,7 +2,10 @@
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { type CapabilityContext } from '#mcp/capabilities/types.ts'
-import { deleteMcpSkill } from '#mcp/skills/mcp-skills-repo.ts'
+import {
+ deleteMcpSkill,
+ getMcpSkillByNameInput,
+} from '#mcp/skills/mcp-skills-repo.ts'
import { deleteSkillVector } from '#mcp/skills/skill-vectorize.ts'
import { requireMcpUser } from './require-user.ts'
@@ -20,21 +23,26 @@
idempotent: true,
destructive: true,
inputSchema: z.object({
- skill_id: z
- .string()
- .min(1)
- .describe('Skill id returned by meta_save_skill.'),
+ name: z.string().min(1).describe('Unique lower-kebab-case skill name.'),
}),
outputSchema,
async handler(args, ctx: CapabilityContext) {
const user = requireMcpUser(ctx.callerContext)
+ const existing = await getMcpSkillByNameInput(
+ ctx.env.APP_DB,
+ user.userId,
+ args.name,
+ )
+ if (!existing) {
+ return { deleted: false }
+ }
const removed = await deleteMcpSkill(
ctx.env.APP_DB,
user.userId,
- args.skill_id,
+ existing.name,
)
if (removed) {
- await deleteSkillVector(ctx.env, args.skill_id)
+ await deleteSkillVector(ctx.env, existing.id)
}
return { deleted: removed }
},
diff --git a/packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts b/packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts
--- a/packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts
+++ b/packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts
@@ -2,7 +2,7 @@
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { type CapabilityContext } from '#mcp/capabilities/types.ts'
-import { getMcpSkillById } from '#mcp/skills/mcp-skills-repo.ts'
+import { getMcpSkillByNameInput } from '#mcp/skills/mcp-skills-repo.ts'
import {
parseSkillParameters,
skillParameterSchema,
@@ -21,7 +21,7 @@
}
const outputSchema = z.object({
- skill_id: z.string(),
+ name: z.string(),
title: z.string(),
description: z.string(),
collection: z.string().nullable(),
@@ -51,18 +51,15 @@
idempotent: true,
destructive: false,
inputSchema: z.object({
- skill_id: z
- .string()
- .min(1)
- .describe('Skill id returned by meta_save_skill.'),
+ name: z.string().min(1).describe('Unique saved skill name.'),
}),
outputSchema,
async handler(args, ctx: CapabilityContext) {
const user = requireMcpUser(ctx.callerContext)
- const row = await getMcpSkillById(
+ const row = await getMcpSkillByNameInput(
ctx.env.APP_DB,
user.userId,
- args.skill_id,
+ args.name,
)
if (!row) {
throw new Error('Skill not found for this user.')
@@ -72,7 +69,7 @@
const uses = parseStringArray(row.uses_capabilities)
const parameters = parseSkillParameters(row.parameters)
return {
- skill_id: row.id,
+ name: row.name,
title: row.title,
description: row.description,
collection: row.collection_name,
diff --git a/packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts b/packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts
--- a/packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts
+++ b/packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts
@@ -3,7 +3,7 @@
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { type CapabilityContext } from '#mcp/capabilities/types.ts'
-import { getMcpSkillById } from '#mcp/skills/mcp-skills-repo.ts'
+import { getMcpSkillByNameInput } from '#mcp/skills/mcp-skills-repo.ts'
import {
applySkillParameters,
parseSkillParameters,
@@ -11,7 +11,7 @@
import { requireMcpUser } from './require-user.ts'
const runFailureHint =
- 'If the saved codemode is wrong, use meta_get_skill to inspect it, then meta_update_skill to replace code and metadata in place (same skill_id), or meta_delete_skill followed by meta_save_skill.'
+ 'If the saved codemode is wrong, use meta_get_skill to inspect it, then call meta_save_skill again with the same skill name to replace the stored code and metadata.'
const outputSchema = z.object({
ok: z.boolean(),
@@ -33,16 +33,18 @@
{
name: 'meta_run_skill',
description:
- 'Execute a saved skill\'s codemode in the same sandbox as the MCP execute tool. When the skill defines parameters, pass them in params; the code receives them via the params variable or the first function argument. Example: meta_run_skill({ "skill_id": "<id>", "params": { "owner": "kentcdodds", "days": 3 } }). On failure, the structured result includes a hint for updating the skill (meta_update_skill).',
+ 'Execute a saved skill by name in the same sandbox as the MCP execute tool. Skill names are lower-kebab-case and unique per user. When the skill defines parameters, pass them in params; the code receives them via the params variable or the first function argument. Example: meta_run_skill({ "name": "github-pr-summary", "params": { "owner": "kentcdodds", "days": 3 } }). On failure, the structured result includes a hint for re-saving the skill with corrected code or metadata.',
keywords: ['skill', 'run', 'execute'],
readOnly: false,
idempotent: false,
destructive: false,
inputSchema: z.object({
- skill_id: z
+ name: z
.string()
.min(1)
- .describe('Skill id returned by meta_save_skill.'),
+ .describe(
+ 'Unique lower-kebab-case skill name to execute for the signed-in user.',
+ ),
params: z
.record(z.string(), z.unknown())
.optional()
@@ -53,10 +55,10 @@
outputSchema,
async handler(args, ctx: CapabilityContext) {
const user = requireMcpUser(ctx.callerContext)
- const row = await getMcpSkillById(
+ const row = await getMcpSkillByNameInput(
ctx.env.APP_DB,
user.userId,
- args.skill_id,
+ args.name,
)
if (!row) {
throw new Error('Skill not found for this user.')
diff --git a/packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts b/packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts
--- a/packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts
+++ b/packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts
@@ -2,13 +2,28 @@
import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
import { type CapabilityContext } from '#mcp/capabilities/types.ts'
-import { insertMcpSkill, deleteMcpSkill } from '#mcp/skills/mcp-skills-repo.ts'
-import { prepareSkillPersistence } from '#mcp/skills/skill-mutation.ts'
+import {
+ deleteMcpSkill,
+ getMcpSkillByName,
+ insertMcpSkill,
+ isDuplicateSkillNameError,
+ updateMcpSkill,
+} from '#mcp/skills/mcp-skills-repo.ts'
+import {
+ buildSkillEmbedTextFromStoredRow,
+ prepareSkillPersistence,
+} from '#mcp/skills/skill-mutation.ts'
import { skillParameterSchema } from '#mcp/skills/skill-parameters.ts'
import { upsertSkillVector } from '#mcp/skills/skill-vectorize.ts'
import { requireMcpUser } from './require-user.ts'
const inputSchema = z.object({
+ name: z
+ .string()
+ .min(1)
+ .describe(
+ 'Unique lower-kebab-case skill name for this user. This is the public way to refer to the skill in search, get, run, update, and delete flows.',
+ ),
title: z.string().min(1).describe('Short title for the skill.'),
description: z
.string()
@@ -65,7 +80,7 @@
})
const outputSchema = z.object({
- skill_id: z.string(),
+ name: z.string(),
collection: z.string().nullable(),
collection_slug: z.string().nullable(),
inferred_capabilities: z.array(z.string()),
@@ -81,7 +96,7 @@
{
name: 'meta_save_skill',
description:
- 'Save a reusable codemode skill for the signed-in user when the workflow is reasonably repeatable (a pattern you expect to run again with similar structure or inputs). Do not save one-off tasks or highly bespoke work—use execute for those. To change an existing skill in place, use meta_update_skill instead.',
+ 'Save or replace a reusable codemode skill for the signed-in user by name when the workflow is reasonably repeatable (a pattern you expect to run again with similar structure or inputs). The lower-kebab-case skill name is the public identifier, so calling this again with the same name replaces the stored skill in place. Do not save one-off tasks or highly bespoke work—use execute for those.',
keywords: [
'skill',
'save',
@@ -99,17 +114,44 @@
async handler(args, ctx: CapabilityContext) {
const user = requireMcpUser(ctx.callerContext)
const prep = await prepareSkillPersistence(args)
+ const existing = await getMcpSkillByName(
+ ctx.env.APP_DB,
+ user.userId,
+ prep.rowPayload.name,
+ )
- const skillId = crypto.randomUUID()
+ const skillId = existing?.id ?? crypto.randomUUID()
const now = new Date().toISOString()
- await insertMcpSkill(ctx.env.APP_DB, {
- id: skillId,
- user_id: user.userId,
- ...prep.rowPayload,
- created_at: now,
- updated_at: now,
- })
+ if (existing) {
+ const updated = await updateMcpSkill(
+ ctx.env.APP_DB,
+ user.userId,
+ existing.name,
+ prep.rowPayload,
+ )
+ if (!updated) {
+ throw new Error('Skill not found for this user.')
+ }
+ } else {
+ try {
+ await insertMcpSkill(ctx.env.APP_DB, {
+ id: skillId,
+ user_id: user.userId,
+ ...prep.rowPayload,
+ created_at: now,
+ updated_at: now,
+ })
+ } catch (error) {
+ if (isDuplicateSkillNameError(error)) {
+ throw new Error(
+ `A saved skill named "${prep.rowPayload.name}" already exists for this user.`,
+ )
+ }
+ throw error
+ }
+ }
+
try {
await upsertSkillVector(ctx.env, {
skillId,
@@ -118,12 +160,43 @@
collectionSlug: prep.rowPayload.collection_slug,
})
} catch (cause) {
- await deleteMcpSkill(ctx.env.APP_DB, user.userId, skillId)
+ if (existing) {
+ await updateMcpSkill(ctx.env.APP_DB, user.userId, existing.name, {
+ name: existing.name,
+ title: existing.title,
+ description: existing.description,
+ collection_name: existing.collection_name,
+ collection_slug: existing.collection_slug,
+ keywords: existing.keywords,
+ code: existing.code,
+ search_text: existing.search_text,
+ uses_capabilities: existing.uses_capabilities,
+ parameters: existing.parameters,
+ inferred_capabilities: existing.inferred_capabilities,
+ inference_partial: existing.inference_partial,
+ read_only: existing.read_only,
+ idempotent: existing.idempotent,
+ destructive: existing.destructive,
+ })
+ const oldEmbed = await buildSkillEmbedTextFromStoredRow(existing)
+ await upsertSkillVector(ctx.env, {
+ skillId: existing.id,
+ userId: user.userId,
+ embedText: oldEmbed,
+ collectionSlug: existing.collection_slug,
+ })
+ } else {
+ await deleteMcpSkill(
+ ctx.env.APP_DB,
+ user.userId,
+ prep.rowPayload.name,
+ )
+ }
throw cause
}
return {
- skill_id: skillId,
+ name: prep.rowPayload.name,
collection: prep.rowPayload.collection_name,
collection_slug: prep.rowPayload.collection_slug,
inferred_capabilities: prep.merged,
diff --git a/packages/worker/src/mcp/capabilities/meta/meta-update-skill.ts b/packages/worker/src/mcp/capabilities/meta/meta-update-skill.ts
deleted file mode 100644
--- a/packages/worker/src/mcp/capabilities/meta/meta-update-skill.ts
+++ /dev/null
@@ -1,153 +1,0 @@
-import { z } from 'zod'
-import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts'
-import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts'
-import { type CapabilityContext } from '#mcp/capabilities/types.ts'
-import { getMcpSkillById, updateMcpSkill } from '#mcp/skills/mcp-skills-repo.ts'
-import {
- buildSkillEmbedTextFromStoredRow,
- prepareSkillPersistence,
-} from '#mcp/skills/skill-mutation.ts'
-import { skillParameterSchema } from '#mcp/skills/skill-parameters.ts'
-import { upsertSkillVector } from '#mcp/skills/skill-vectorize.ts'
-import { requireMcpUser } from './require-user.ts'
-
-const inputSchema = z.object({
- skill_id: z
- .string()
- .min(1)
- .describe('Existing skill id (same as returned by meta_save_skill).'),
- title: z.string().min(1).describe('Short title for the skill.'),
- description: z
- .string()
- .min(1)
- .describe('What this skill does (shown in search and to users).'),
- collection: z
- .string()
- .optional()
- .describe(
- 'Optional user-defined collection/domain for grouping related saved skills.',
- ),
- keywords: z
- .array(z.string())
- .describe('Extra search keywords for this skill.'),
- code: z
- .string()
- .min(1)
- .describe(
- 'Replacement codemode snippet as accepted by execute (async arrow or equivalent after normalization).',
- ),
- search_text: z.string().optional(),
- uses_capabilities: z.array(z.string()).optional(),
- parameters: z
- .array(skillParameterSchema)
- .optional()
- .describe('Replacement parameter definitions for the skill.'),
- read_only: z.boolean(),
- idempotent: z.boolean(),
- destructive: z.boolean(),
-})
-
-const outputSchema = z.object({
- skill_id: z.string(),
- collection: z.string().nullable(),
- collection_slug: z.string().nullable(),
- inferred_capabilities: z.array(z.string()),
- inference_partial: z.boolean(),
- destructive_derived: z.boolean(),
- read_only_derived: z.boolean().nullable(),
- idempotent_derived: z.boolean().nullable(),
- warnings: z.array(z.string()).optional(),
-})
-
-export const metaUpdateSkillCapability = defineDomainCapability(
- capabilityDomainNames.meta,
- {
- name: 'meta_update_skill',
- description:
- 'Replace fields and codemode for an existing skill (same skill_id). Re-runs inference, validation, D1 update, and Vectorize upsert. Use when meta_run_skill fails due to bad stored code.',
- keywords: [
- 'skill',
- 'update',
- 'edit',
- 'replace',
- 'fix',
- 'codemode',
- 'collection',
- ],
- readOnly: false,
- idempotent: true,
- destructive: false,
- inputSchema,
- outputSchema,
- async handler(args, ctx: CapabilityContext) {
- const user = requireMcpUser(ctx.callerContext)
- const existing = await getMcpSkillById(
- ctx.env.APP_DB,
- user.userId,
- args.skill_id,
- )
- if (!existing) {
- throw new Error('Skill not found for this user.')
- }
-
- const { skill_id, ...rest } = args
- const prep = await prepareSkillPersistence(rest)
-
- const updated = await updateMcpSkill(
- ctx.env.APP_DB,
- user.userId,
- skill_id,
- prep.rowPayload,
- )
- if (!updated) {
- throw new Error('Skill not found for this user.')
- }
-
- try {
- await upsertSkillVector(ctx.env, {
- skillId: skill_id,
- userId: user.userId,
- embedText: prep.embedText,
- collectionSlug: prep.rowPayload.collection_slug,
- })
- } catch (cause) {
- await updateMcpSkill(ctx.env.APP_DB, user.userId, skill_id, {
- title: existing.title,
- description: existing.description,
- collection_name: existing.collection_name,
- collection_slug: existing.collection_slug,
- keywords: existing.keywords,
- code: existing.code,
- search_text: existing.search_text,
- uses_capabilities: existing.uses_capabilities,
- parameters: existing.parameters,
- inferred_capabilities: existing.inferred_capabilities,
- inference_partial: existing.inference_partial,
- read_only: existing.read_only,
- idempotent: existing.idempotent,
- destructive: existing.destructive,
- })
- const oldEmbed = await buildSkillEmbedTextFromStoredRow(existing)
- await upsertSkillVector(ctx.env, {
- skillId: skill_id,
- userId: user.userId,
- embedText: oldEmbed,
- collectionSlug: existing.collection_slug,
- })
- throw cause
- }
-
- return {
- skill_id,
- collection: prep.rowPayload.collection_name,
- collection_slug: prep.rowPayload.collection_slug,
- inferred_capabilities: prep.merged,
- inference_partial: prep.inferencePartial,
- destructive_derived: prep.derived.destructiveDerived,
- read_only_derived: prep.derived.readOnlyDerived,
- idempotent_derived: prep.derived.idempotentDerived,
- ...(prep.warnings.length > 0 ? { warnings: prep.warnings } : {}),
- }
- },
- },
-)
\ No newline at end of file
diff --git a/packages/worker/src/mcp/capabilities/unified-search.ts b/packages/worker/src/mcp/capabilities/unified-search.ts
--- a/packages/worker/src/mcp/capabilities/unified-search.ts
+++ b/packages/worker/src/mcp/capabilities/unified-search.ts
@@ -14,10 +14,8 @@
import { type CapabilitySpec } from './types.ts'
import { buildSkillEmbedText } from '#mcp/skills/skill-embed-and-flags.ts'
import { type McpSkillRow } from '#mcp/skills/mcp-skills-types.ts'
+import { parseSkillParameters } from '#mcp/skills/skill-parameters.ts'
import {
- parseSkillParameters,
-} from '#mcp/skills/skill-parameters.ts'
-import {
type SecretMetadata,
type SecretSearchRow,
} from '#mcp/secrets/types.ts'
@@ -37,8 +35,8 @@
}
}
-function buildSkillUsage(skillId: string): string {
- const runArgs = JSON.stringify({ skill_id: skillId })
+function buildSkillUsage(skillName: string): string {
+ const runArgs = JSON.stringify({ name: skillName })
return `Run with meta_run_skill: ${runArgs}. Optionally include "params": { ... }. To inspect code, call meta_get_skill then execute.`
}
@@ -72,7 +70,7 @@
export type SkillSearchHitSummary = {
type: 'skill'
- skillId: string
+ skillName: string
domain: 'meta'
collection: string | null
collectionSlug: string | null
... diff truncated: showing 800 of 2021 linesYou can send follow-ups to this agent here. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/mcp/skills/mcp-skills-repo.ts (1)
140-195:⚠️ Potential issue | 🔴 CriticalUse immutable ids for internal update/delete operations.
Switching these helpers to
WHERE name = ?turns every resolve-then-write call site into a TOCTOU hazard. If the original row is deleted/recreated with the same name between the read and the write, the mutation/delete hits the replacement row instead of the row that was originally resolved. In this PR that can desync vector ids inmeta_save_skilland delete another request’s newer save inmeta_delete_skill.💡 Safer shape
export async function updateMcpSkill( db: D1Database, userId: string, - skillName: string, + skillId: string, fields: { /** Changing this value renames the skill and must remain unique per user. */ name: string @@ }, ): Promise<boolean> { const now = new Date().toISOString() - const normalizedSkillName = normalizeSkillName(skillName) const out = await db .prepare( `UPDATE mcp_skills SET name = ?, title = ?, description = ?, keywords = ?, code = ?, search_text = ?, uses_capabilities = ?, parameters = ?, collection_name = ?, collection_slug = ?, inferred_capabilities = ?, inference_partial = ?, read_only = ?, idempotent = ?, destructive = ?, updated_at = ? - WHERE name = ? AND user_id = ?`, + WHERE id = ? AND user_id = ?`, ) @@ fields.idempotent, fields.destructive, now, - normalizedSkillName, + skillId, userId, ) .run() return (out.meta.changes ?? 0) > 0 } export async function deleteMcpSkill( db: D1Database, userId: string, - skillName: string, + skillId: string, ): Promise<boolean> { - const normalizedSkillName = normalizeSkillName(skillName) const out = await db - .prepare(`DELETE FROM mcp_skills WHERE name = ? AND user_id = ?`) - .bind(normalizedSkillName, userId) + .prepare(`DELETE FROM mcp_skills WHERE id = ? AND user_id = ?`) + .bind(skillId, userId) .run() return (out.meta.changes ?? 0) > 0 }Also applies to: 198-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/skills/mcp-skills-repo.ts` around lines 140 - 195, The updateMcpSkill SQL uses WHERE name = ? which is a TOCTOU hazard; change the mutation to use an immutable unique id (e.g., skill_id / id) in the WHERE clause instead of name so the update targets the exact row resolved earlier (adjust the function to accept or look up the skill's immutable id, replace normalizedSkillName with that id in the WHERE and bind list, and apply the same fix for the corresponding delete helper(s) such as meta_save_skill and meta_delete_skill so all internal update/delete operations use the immutable skill id).
♻️ Duplicate comments (2)
packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts (2)
117-153:⚠️ Potential issue | 🔴 CriticalThis is still a read-then-insert race, not an upsert.
Two concurrent saves for a new
namecan both seeexisting === null; one inserts and the other throws"already exists"instead of replacing in place. To match the capability contract, collapse this into a single repo upsert that returns the committed row, or at least retry the duplicate insert as an update and re-read before vectorizing soskillIdcomes from the row that actually won.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts` around lines 117 - 153, The current logic does a read-then-insert and races; change it to an atomic upsert or, if upsert isn't available, catch the duplicate-insert case (isDuplicateSkillNameError) and then perform an update + re-read to obtain the actual committed row/id before continuing. Specifically, replace the read-then-insert flow around getMcpSkillByName / insertMcpSkill with either a single repository upsert that returns the committed row (preferred), or in the insertMcpSkill catch branch call updateMcpSkill (or a dedicated upsert function) and then call getMcpSkillByName again to set skillId from the winning row (instead of using the locally generated crypto.randomUUID()) so concurrent saves resolve to the committed record.
162-194:⚠️ Potential issue | 🔴 CriticalGuard the rollback against concurrent replacements.
This catch still replays the stale
existingsnapshot / delete-by-name blindly. If another request saves the samenameafter this DB write but before vectorization fails, this branch can restore old data or delete the newer row, then overwrite its vector. Fetch the current row first and only revert/delete when it still matches the exact row/version written by this request; only restore the old vector when that guarded rollback succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts` around lines 162 - 194, The rollback currently replays the stale `existing` snapshot or deletes by name blindly; before calling updateMcpSkill, upsertSkillVector, or deleteMcpSkill in the catch block, re-fetch the current DB row for the same skill name and compare a unique identifier/version (e.g., existing.id or a row version/timestamp from the row you wrote) to ensure it still matches the exact row written by this request (the one referenced by `existing`/`prep.rowPayload`); only perform the restore (updateMcpSkill + upsertSkillVector using `buildSkillEmbedTextFromStoredRow`) or the deleteMcpSkill when that guarded equality check succeeds, and only upsert the old vector if the guarded rollback actually applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts`:
- Around line 5-8: The delete flow currently uses getMcpSkillByNameInput which
allows fallback to normalized-title matching and can resolve aliases; change it
to use the exact-match resolver getMcpSkillByName so deletion only accepts the
unique public identifier; update the lookup call(s) in meta-delete-skill.ts
(replace getMcpSkillByNameInput with getMcpSkillByName) and keep calling
deleteMcpSkill with the resulting exact-match record to ensure deletes never run
on alias/normalized-title matches.
---
Outside diff comments:
In `@packages/worker/src/mcp/skills/mcp-skills-repo.ts`:
- Around line 140-195: The updateMcpSkill SQL uses WHERE name = ? which is a
TOCTOU hazard; change the mutation to use an immutable unique id (e.g., skill_id
/ id) in the WHERE clause instead of name so the update targets the exact row
resolved earlier (adjust the function to accept or look up the skill's immutable
id, replace normalizedSkillName with that id in the WHERE and bind list, and
apply the same fix for the corresponding delete helper(s) such as
meta_save_skill and meta_delete_skill so all internal update/delete operations
use the immutable skill id).
---
Duplicate comments:
In `@packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts`:
- Around line 117-153: The current logic does a read-then-insert and races;
change it to an atomic upsert or, if upsert isn't available, catch the
duplicate-insert case (isDuplicateSkillNameError) and then perform an update +
re-read to obtain the actual committed row/id before continuing. Specifically,
replace the read-then-insert flow around getMcpSkillByName / insertMcpSkill with
either a single repository upsert that returns the committed row (preferred), or
in the insertMcpSkill catch branch call updateMcpSkill (or a dedicated upsert
function) and then call getMcpSkillByName again to set skillId from the winning
row (instead of using the locally generated crypto.randomUUID()) so concurrent
saves resolve to the committed record.
- Around line 162-194: The rollback currently replays the stale `existing`
snapshot or deletes by name blindly; before calling updateMcpSkill,
upsertSkillVector, or deleteMcpSkill in the catch block, re-fetch the current DB
row for the same skill name and compare a unique identifier/version (e.g.,
existing.id or a row version/timestamp from the row you wrote) to ensure it
still matches the exact row written by this request (the one referenced by
`existing`/`prep.rowPayload`); only perform the restore (updateMcpSkill +
upsertSkillVector using `buildSkillEmbedTextFromStoredRow`) or the
deleteMcpSkill when that guarded equality check succeeds, and only upsert the
old vector if the guarded rollback actually applied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f20c4c6d-da8c-4af6-92cb-fbff2047128c
📒 Files selected for processing (8)
packages/worker/migrations/0014-skill-names.sqlpackages/worker/src/mcp/capabilities/meta/meta-delete-skill.tspackages/worker/src/mcp/capabilities/meta/meta-get-skill.tspackages/worker/src/mcp/capabilities/meta/meta-run-skill.tspackages/worker/src/mcp/capabilities/meta/meta-save-skill.tspackages/worker/src/mcp/skills/mcp-skills-repo.tspackages/worker/src/mcp/skills/skill-names.tspackages/worker/src/mcp/tools/execute.ts
✅ Files skipped from review due to trivial changes (1)
- packages/worker/src/mcp/tools/execute.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/worker/src/mcp/capabilities/meta/meta-get-skill.ts
- packages/worker/migrations/0014-skill-names.sql
- packages/worker/src/mcp/skills/skill-names.ts
- packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts
Comment on lines
+5
to
+8
| import { | ||
| deleteMcpSkill, | ||
| getMcpSkillByNameInput, | ||
| } from '#mcp/skills/mcp-skills-repo.ts' |
There was a problem hiding this comment.
Delete should use exact-name resolution only.
getMcpSkillByNameInput falls back to matching normalized titles when no name matches. That is fine for read-style flows, but here it makes a destructive operation accept aliases/titles even though the input contract says name is the unique public identifier. Resolve with getMcpSkillByName instead.
🔒 Safer lookup for delete
import {
deleteMcpSkill,
- getMcpSkillByNameInput,
+ getMcpSkillByName,
} from '#mcp/skills/mcp-skills-repo.ts'
@@
- const existing = await getMcpSkillByNameInput(
+ const existing = await getMcpSkillByName(
ctx.env.APP_DB,
user.userId,
args.name,
)Also applies to: 31-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/meta/meta-delete-skill.ts` around lines
5 - 8, The delete flow currently uses getMcpSkillByNameInput which allows
fallback to normalized-title matching and can resolve aliases; change it to use
the exact-match resolver getMcpSkillByName so deletion only accepts the unique
public identifier; update the lookup call(s) in meta-delete-skill.ts (replace
getMcpSkillByNameInput with getMcpSkillByName) and keep calling deleteMcpSkill
with the resulting exact-match record to ensure deletes never run on
alias/normalized-title matches.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
| .prepare(`DELETE FROM mcp_skills WHERE id = ? AND user_id = ?`) | ||
| .bind(skillId, userId) | ||
| .prepare(`DELETE FROM mcp_skills WHERE name = ? AND user_id = ?`) | ||
| .bind(skillName, userId) |
Contributor
There was a problem hiding this comment.
deleteMcpSkill missing name normalization unlike sibling functions
Low Severity
deleteMcpSkill binds skillName directly to the SQL WHERE clause without calling normalizeSkillName, unlike getMcpSkillByName and updateMcpSkill which both normalize. All current callers happen to pass already-normalized or DB-stored names, so it works today, but the inconsistency is a latent bug for any future caller passing user input directly.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
namevalues instead of hidden idsmeta_save_skillthe single public upsert path for creating or replacing a saved skill by namemeta_update_skillcapability and route replacements through save-by-nameTesting
npm run validatenpm run test -- --run packages/worker/src/mcp/capabilities/unified-search.workers.test.ts packages/worker/src/mcp/tools/search.node.test.tsnpm run test:mcp -- --run packages/worker/src/mcp/mcp-server.mcp-e2e.test.tsSummary by CodeRabbit
New Features
Breaking Changes
Improvements