Skill over mcp#393
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces legacy plural ChangesSEP‑2640 Skills Extension Migration
Sequence Diagram(s)sequenceDiagram
Client->>DirectClient: listSep2640Skills()
DirectClient->>MCPServer: readResource("skill://index.json")
MCPServer->>SkillRegistry: getSep2640IndexTemplates()/getSep2640IndexArchives()
MCPServer->>SkillRegistry: getSep2640InstructionUris()
MCPServer->>FS: read per-skill SKILL.md (if needed)
MCPServer-->>DirectClient: JSON index document
DirectClient-->>Client: Sep2640IndexEntry[]
sequenceDiagram
Client->>DirectClient: readSkillTextUri("skill://a/b/SKILL.md")
DirectClient->>MCPServer: readResource("skill://a/b/SKILL.md")
MCPServer->>SkillLoader: findAndLoadSkillByPath("a/b")
SkillLoader->>FS: read files / resolved inline content
SkillLoader-->>MCPServer: serialized SKILL.md text
MCPServer-->>DirectClient: text content
DirectClient-->>Client: string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/sdk/src/resource/flows/resources-list.flow.ts (1)
307-313:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid clobbering existing
_meta.uikeys when addingresourceUri.The current merge preserves top-level
_metabut replaces the nesteduiobject entirely. If upstream metadata already setsuifields, they get dropped.Proposed nested merge
if (resource.metadata.mimeType === 'text/html;profile=mcp-app') { item._meta = { ...(item._meta ?? {}), ui: { + ...((item._meta?.ui as Record<string, unknown> | undefined) ?? {}), resourceUri: uri, }, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/resource/flows/resources-list.flow.ts` around lines 307 - 313, When adding resourceUri for HTML MCP apps in the mimeType check (resource.metadata.mimeType === 'text/html;profile=mcp-app'), avoid replacing an existing nested ui object on item._meta; instead perform a nested merge so existing item._meta.ui keys are preserved and only resourceUri is added/overwritten. Update the assignment to item._meta to merge the existing item._meta and its ui object with the new ui.resourceUri (using the existing ui spread), referencing the symbols item._meta, ui, resourceUri and the uri variable so upstream ui fields are retained.
🟡 Minor comments (7)
libs/sdk/src/common/metadata/plugin.metadata.ts-128-130 (1)
128-130:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the SEP-2640 template placeholder form
{+skillPath}.Line 129 currently documents
skill://{skillPath}/SKILL.md, but the documented resource template format in this repo usesskill://{+skillPath}/SKILL.md.Suggested doc fix
- * and loaded via `skill://{skillPath}/SKILL.md` MCP resources (per + * and loaded via `skill://{+skillPath}/SKILL.md` MCP resources (perAs per coding guidelines:
libs/sdk/src/skill/README.mddefines the SEP-2640 template asskill://{+skillPath}/SKILL.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/common/metadata/plugin.metadata.ts` around lines 128 - 130, Update the SEP-2640 resource template in the comment text to use the repository's placeholder form: replace the literal "skill://{skillPath}/SKILL.md" with "skill://{+skillPath}/SKILL.md" in the documentation string inside libs/sdk/src/common/metadata/plugin.metadata.ts (the comment block that currently reads `They can be discovered via the `skill://index.json` MCP resource and loaded via `skill://{skillPath}/SKILL.md` MCP resources (per MCP SEP-2640).`). Ensure only the template placeholder is changed to `{+skillPath}` and preserve the rest of the sentence and formatting.libs/sdk/src/common/metadata/app.metadata.ts-118-120 (1)
118-120:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign URI template docs to
{+skillPath}here as well.Line 119 should use
skill://{+skillPath}/SKILL.mdto match the SEP-2640 template format used by project docs.Suggested doc fix
- * resource and loaded via `skill://{skillPath}/SKILL.md` MCP resources + * resource and loaded via `skill://{+skillPath}/SKILL.md` MCP resourcesAs per coding guidelines:
libs/sdk/src/skill/README.mddocuments the canonical template asskill://{+skillPath}/SKILL.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/common/metadata/app.metadata.ts` around lines 118 - 120, Update the URI template in the doc comment from "skill://{skillPath}/SKILL.md" to "skill://{+skillPath}/SKILL.md" to match SEP-2640 and the canonical template in libs/sdk/src/skill/README.md; locate the occurrence in the app metadata comment that mentions the MCP resource and replace the template token {skillPath} with {+skillPath} so documentation is consistent across the codebase.libs/sdk/src/resource/resource.registry.ts-714-724 (1)
714-724:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrebuilt-record detection should validate
providebefore bypassing normalization.Current detection accepts any object with
kind+metadata. Ifprovideis missing/invalid, registration can proceed into inconsistent state instead of failing fast with a clear validation error.Suggested guard tightening
- const isPrebuiltRecord = - resourceDef && typeof resourceDef === 'object' && 'kind' in resourceDef && 'metadata' in resourceDef; + const isPrebuiltRecord = + resourceDef && + typeof resourceDef === 'object' && + 'kind' in resourceDef && + 'metadata' in resourceDef && + 'provide' in resourceDef; let rec: ResourceRecord | ResourceTemplateRecord; if (isPrebuiltRecord) { rec = resourceDef as ResourceRecord | ResourceTemplateRecord; } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/resource/resource.registry.ts` around lines 714 - 724, The prebuilt-record detection (isPrebuiltRecord) currently only checks for 'kind' and 'metadata' on resourceDef and can skip normalization even if 'provide' is missing/invalid; update the guard (or add a follow-up validation) so that when treating resourceDef as a prebuilt ResourceRecord/ResourceTemplateRecord you also validate the presence and shape of 'provide' (e.g., exists and is an array or object as your schema expects) before assigning rec; if 'provide' is invalid, either fall back to running normalizeResource/normalizeResourceTemplate (using isResourceTemplate to choose) or throw a clear validation error so registration fails fast instead of creating an inconsistent ResourceRecord/ResourceTemplateRecord.libs/sdk/src/skill/skill.registry.ts-972-976 (1)
972-976:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmit a reset when instruction URIs change.
This is the only SEP-2640 mutator that updates registry state without calling
bump('reset'). As written, listeners keep serving the old instructions block until some unrelated registry change happens.🔁 Proposed fix
addSep2640InstructionUri(uri: string): void { if (!this.sep2640InstructionUris.includes(uri)) { this.sep2640InstructionUris.push(uri); + this.bump('reset'); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/skill.registry.ts` around lines 972 - 976, addSep2640InstructionUri currently mutates the sep2640InstructionUris array without notifying listeners; update addSep2640InstructionUri to call this.bump('reset') (or the registry's reset bump method) whenever it actually adds a new URI so observers receive a reset event; reference the addSep2640InstructionUri method and the sep2640InstructionUris array and ensure bump('reset') is only invoked when the URI was pushed to avoid spurious resets.libs/sdk/src/skill/sep-2640/sep-2640.uri.ts-216-224 (1)
216-224:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce the documented 64-character skill-name limit.
The comment says the final segment must satisfy the Agent Skills max-64 rule, but the validator never checks length. Direct callers of
validateSkillPath()can still register overlong names.📏 Proposed fix
if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(finalSegment)) { throw new Error(`skill name "${finalSegment}" violates Agent Skills naming rules`); } + if (finalSegment.length > 64) { + throw new Error(`skill name "${finalSegment}" must be 64 characters or fewer`); + } if (finalSegment.includes('--')) { throw new Error(`skill name "${finalSegment}" must not contain consecutive hyphens`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/sep-2640/sep-2640.uri.ts` around lines 216 - 224, The validator for skill path doesn't enforce the documented 64-character max; update validateSkillPath (the block using finalSegment) to reject names longer than 64 characters by adding a length check (e.g., if finalSegment.length > 64) that throws a clear Error mentioning the skill name and the 64-character limit, or incorporate the length into the existing regex test for finalSegment so overlong names are rejected before allowing registration.libs/sdk/src/skill/sep-2640/__tests__/sep-2640.metadata.spec.ts-44-48 (1)
44-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis case is checking an empty path, not an empty segment.
skillPath: []only exercises the array.min(1)guard. The per-segment.min(1)validator added for each path element is still uncovered, so a regression there would slip through.💡 Suggested test adjustment
it('rejects skillPath with empty segments', () => { const result = skillMetadataSchema.safeParse({ ...baseSkill, - skillPath: [], + skillPath: ['acme', '', 'refunds'], }); expect(result.success).toBe(false); });As per coding guidelines:
**/*.spec.ts?(x): Ensure 95%+ test coverage across all metrics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/sep-2640/__tests__/sep-2640.metadata.spec.ts` around lines 44 - 48, The test currently uses skillPath: [] which only validates the array-level .min(1); change the case to provide an array with an empty segment (e.g. skillPath: ['']) so the per-segment `.min(1)` constraint on each element of skillMetadataSchema is exercised; update the spec assertion accordingly to still expect a failure from skillMetadataSchema.safeParse({ ...baseSkill, skillPath: [''] }).libs/sdk/src/direct/direct-client.ts-474-480 (1)
474-480:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten runtime validation before returning typed SEP-2640 entries
listSep2640Skills()currently allows any stringtype/urlthrough, but the method promisesSep2640IndexEntry[]. Narrowing to known entry types andskill://URLs will prevent malformed index data leaking into typed callers.Suggested change
- return doc.skills.filter( - (e) => - e && - typeof e === 'object' && - typeof (e as Sep2640IndexEntry).type === 'string' && - typeof (e as Sep2640IndexEntry).url === 'string', - ); + const allowedTypes = new Set<Sep2640IndexEntry['type']>(['skill-md', 'mcp-resource-template', 'archive']); + return doc.skills.filter((e): e is Sep2640IndexEntry => { + if (!e || typeof e !== 'object') return false; + const candidate = e as { type?: unknown; url?: unknown }; + return ( + typeof candidate.type === 'string' && + allowedTypes.has(candidate.type as Sep2640IndexEntry['type']) && + typeof candidate.url === 'string' && + candidate.url.startsWith('skill://') + ); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/direct/direct-client.ts` around lines 474 - 480, listSep2640Skills currently returns any object with string type/url; tighten the runtime validation in the filter over doc.skills so it only returns true Sep2640IndexEntry values by checking that (a) the type field equals one of the known entry types (e.g. the allowed set your spec uses) and (b) the url field is a valid skill URL (startsWith "skill://" and optionally further basic validation). Update the predicate inside listSep2640Skills to explicitly check allowed types and url.startsWith("skill://") (and reject otherwise) so callers receive only well-formed Sep2640IndexEntry objects from doc.skills.
🧹 Nitpick comments (3)
libs/sdk/src/skill/sep-2640/resources/index.ts (1)
27-28: ⚡ Quick winReturn a typed constructor list instead of
Function[].
Function[]erases the public contract of this SDK export and accepts non-resource callables. Since this lives inlibs/sdk, downstream callers lose constructor types right at the API boundary.♻️ Suggested type tightening
-export function getSep2640Resources(): Function[] { +export function getSep2640Resources(): Array< + typeof Sep2640SkillIndexResource | typeof Sep2640SkillMdResource | typeof Sep2640SkillFileResource +> { return [Sep2640SkillIndexResource, Sep2640SkillMdResource, Sep2640SkillFileResource]; }As per coding guidelines:
**/*.{ts,tsx}: Follow existing TypeScript patterns and keep strict typing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/sep-2640/resources/index.ts` around lines 27 - 28, The function getSep2640Resources currently returns Function[] which erases constructor types; change its signature to return a typed constructor array instead. Replace Function[] with either a union of the resource constructor types (e.g. Array<typeof Sep2640SkillIndexResource | typeof Sep2640SkillMdResource | typeof Sep2640SkillFileResource>) or define a shared constructor type (e.g. type ResourceCtor = new (...args: any[]) => /*instance type*/; and use ResourceCtor[]) and update getSep2640Resources to return that typed array so callers preserve concrete constructor types for Sep2640SkillIndexResource, Sep2640SkillMdResource, and Sep2640SkillFileResource.libs/sdk/src/skill/index.ts (1)
167-205: ⚡ Quick winPlease confirm the draft docs ship with this public export change.
This barrel removes the legacy
skills://surface and publishes the SEP-2640 APIs instead, so the SDK docs should have the matchingdocs/draft/docs/**update in the same PR.As per coding guidelines:
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/index.ts` around lines 167 - 205, The public API surface was changed to export SEP-2640 symbols (e.g., SEP_2640_EXTENSION_ID, buildSkillIndex, parseSkillUri, getSep2640Resources, Sep2640SkillFileResource, findAndLoadSkillByPath) and removed the legacy skills:// surface; update the draft docs under docs/draft/docs/** in this same PR to reflect the new SEP-2640 APIs and the removal of skills:// (add/modify pages describing the exported functions/types, migration notes, and examples for parseSkillUri/parseSkillUriWithKnownSkill, buildSkillIndex/serializeSkillMd, and resource helpers like findAndLoadSkillByPath/getSep2640Resources) so the SDK docs match the new public exports before merging.apps/e2e/demo-e2e-skills/e2e/skills-resources.e2e.spec.ts (1)
113-117: ⚡ Quick winDrop the non-null assertions here.
expect(reviewPr).toBeDefined()does not narrow the type for TypeScript, so the!operators are still bypassing strict-null checks in this test.Proposed fix
const reviewPr = doc.skills.find((s) => s.name === 'review-pr'); expect(reviewPr).toBeDefined(); - expect(reviewPr!.type).toBe('skill-md'); - expect(reviewPr!.url).toBe('skill://review-pr/SKILL.md'); - expect(reviewPr!.description).toBeTruthy(); + if (!reviewPr) { + throw new Error('Expected review-pr to be present in skill://index.json'); + } + expect(reviewPr.type).toBe('skill-md'); + expect(reviewPr.url).toBe('skill://review-pr/SKILL.md'); + expect(reviewPr.description).toBeTruthy();As per coding guidelines:
{libs,apps}/**/*.ts?(x): Avoid non-null assertions (!) - use proper error handling instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/e2e/demo-e2e-skills/e2e/skills-resources.e2e.spec.ts` around lines 113 - 117, Remove the non-null assertions on reviewPr and add a real runtime guard before accessing its properties: after the existing expect(reviewPr).toBeDefined(), explicitly fail or return if reviewPr is undefined (e.g., if (!reviewPr) return fail('reviewPr missing')), then access reviewPr.type, reviewPr.url and reviewPr.description without using !; alternatively replace the three property assertions with a single assertion that reviewPr matches the expected shape (e.g., expect(reviewPr).toEqual(expect.objectContaining({ type: 'skill-md', url: 'skill://review-pr/SKILL.md' }))) so no non-null operator is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/sdk/src/common/metadata/skill.metadata.ts`:
- Around line 511-522: The current skillPath schema (the z.string().regex on the
skillPath segments) treats '%' as a normal character and thus accepts invalid
percent-encodings; update the regex used in the z.string().regex inside the
skillPath array so that any '%' must be followed by exactly two hex digits
(replace bare '%' acceptance with a percent-encoding pattern like
%[0-9A-Fa-f]{2}) and keep the rest of the allowed character classes from the
existing pattern; modify the regex in the skillPath segment validator (the
z.string().min(1).max(64).regex(...) call) so malformed values like "billing%zz"
or trailing "%" are rejected.
In `@libs/sdk/src/direct/client.types.ts`:
- Around line 598-607: The current readSkillUri(uri: string): Promise<string> is
unsafe for non-text resources; change the API to either (A) narrow it to
text-only by renaming to readSkillTextUri and keep Promise<string>, validating
the scheme and documenting it only accepts text resources (e.g. SKILL.md), or
(B) make it return a typed MCP read result (e.g. Promise<ReadResult |
ReadResponse>) that can carry binary buffers and metadata; update the function
signature in client.types to the chosen type, update the JSDoc to reflect the
new contract and scheme validation, and update all callers of readSkillUri to
handle the new return type or new name accordingly.
- Around line 30-35: The Sep2640IndexEntry interface should be replaced by a
discriminated union to enforce protocol rules: create separate types (e.g.,
Sep2640SkillEntry { type: 'skill-md'; name: string; description: string; url:
string }, Sep2640ResourceTemplateEntry { type: 'mcp-resource-template';
description: string; url: string }, and Sep2640ArchiveEntry { type: 'archive';
name?: string; description: string; url: string }) and export their union as
Sep2640IndexEntry; update any related signatures such as listSep2640Skills() to
return/narrow the new union so callers can safely discriminate on the type field
and rely on name being required for 'skill-md' and absent/irrelevant for
'mcp-resource-template'.
In `@libs/sdk/src/direct/direct-client.ts`:
- Around line 491-499: In readSkillUri, replace the two generic throws with
PublicMcpError so SDK errors remain machine-readable: change the first throw
when uri doesn't start with 'skill://' and the second throw when text is not a
string to throw new PublicMcpError(...) with a suitable JSON‑RPC error code and
clear message; ensure you add the necessary import for PublicMcpError and keep
the checks around mcpClient.readResource and read.contents logic intact.
In `@libs/sdk/src/resource/flows/resources-list.flow.ts`:
- Around line 291-304: The code is using wide `unknown` casts when forwarding
MCP fields `annotations` and `_meta`; replace those ad-hoc casts with concrete
MCP metadata types and narrow checks: declare or import the appropriate
interfaces (e.g., a metadata type that includes `annotations?: Record<string,
unknown>` and `_meta?: Record<string, unknown>`), cast `resource.metadata` to
those concrete types instead of `{ annotations?: unknown }`/`{ _meta?:
Record<string, unknown> }`, then validate and assign to `item` (the
`ResponseResourceItem`) using those typed properties (keep the same runtime
checks like `typeof annotations === 'object'` and
`Object.keys(metaFromMetadata).length > 0`), so the forwarding code for
`annotations` and `_meta` is strongly typed and follows MCP result type
patterns.
In `@libs/sdk/src/resource/resource.utils.ts`:
- Around line 63-71: The current early-return returns an object by checking only
item.kind (in resource.utils.ts) which can let malformed ResourceRecord objects
through; update the guard around item (the branch that checks
ResourceKind.ESM/REMOTE/FUNCTION/CLASS_TOKEN) to also validate the pre-built
ResourceRecord shape before returning: assert that item.provide exists
(non-null), item.metadata exists and is an object, and add kind-specific checks
(e.g. if item.kind === ResourceKind.FUNCTION then typeof item.provide ===
'function'; for REMOTE/ESM ensure provide is a non-empty string/URL; for
CLASS_TOKEN ensure provide is a string token) and only then return item as
ResourceRecord; otherwise fall through (or throw a clear normalization error) so
malformed records are rejected early.
In `@libs/sdk/src/skill/sep-2640/__tests__/sep-2640.builders.spec.ts`:
- Around line 114-116: The test currently uses a non-null assertion fmMatch![1]
after const fmMatch = md.match(/^---\n([\s\S]*?)\n---/); — replace that with an
explicit guard: check fmMatch is truthy (e.g., if (!fmMatch) fail the test or
throw with a clear message) before calling yaml.load on fmMatch[1], then assign
const fm = yaml.load(fmMatch[1]) as Record<string, unknown>; this removes the
non-null assertion while keeping the test strict and giving a clear failure when
frontmatter is missing.
In `@libs/sdk/src/skill/sep-2640/sep-2640.builders.ts`:
- Around line 149-153: The metadata-only stub created by metadataToContentStub
currently omits metadata.skillPath causing buildSkillMdIndexEntry to treat
nested skills as flattened; update metadataToContentStub to include the
skillPath (or skillPathSegments) property from SkillMetadata in the returned
IndexEntryInput so downstream code (e.g., buildSkillMdIndexEntry) can use the
original path info to preserve nested skill structure. Ensure you reference
metadata.skillPath (or derive metadata.skillPathSegments if the index expects
segments) and add it to the returned object alongside name and description.
In `@libs/sdk/src/skill/sep-2640/sep-2640.per-skill.ts`:
- Around line 122-135: The current try/catch around resolveLastModified and
registration causes a thrown error in resolveLastModified to skip registering
that skill; change the flow in the for (const skill of skills) loop to call
resolveLastModified(skill) in its own try/catch (or await it and catch errors)
so that if it throws you log a warning and set lastModified = undefined, then
call buildPerSkillResourceRecord and
resourceRegistry.registerDynamicResource(record) in a separate try/catch so
registration failures are still handled; specifically update usages of
resolveLastModified, buildPerSkillResourceRecord, and
resourceRegistry.registerDynamicResource so resolution errors don't suppress
registration.
In `@libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts`:
- Around line 164-180: readSkillFileByPath currently reads files as UTF-8 text
and corrupts binaries; change it to read raw bytes first (Buffer/Uint8Array) and
use guessMimeType(filePath) to decide how to return the resource: for text-like
MIME types (e.g., text/*, application/json, application/javascript, etc.) decode
the buffer to UTF-8 string and for markdown run parseSkillMdFrontmatter(body) to
strip frontmatter; for non-text MIME types (image/png, image/jpeg,
application/octet-stream, etc.) return the raw bytes in the binary/blob branch
of ReadResourceResult. Also update the execute() method to wrap results
accordingly (use { text: ... } for textual content and the binary/blob field for
buffers) so ReadResourceResult consumers see the correct branch; reference
functions/methods: readSkillFileByPath, execute, parseSkillMdFrontmatter,
guessMimeType, and the ReadResourceResult shape when making these changes.
In `@libs/sdk/src/skill/sep-2640/sep-2640.uri.ts`:
- Around line 103-106: The code currently calls decodeURIComponent directly when
building allSegments from cleanRemainder which throws on malformed
percent-escapes; change the decoder usage to a safe decode helper (e.g.
safeDecode(s): string | undefined) that wraps decodeURIComponent in try/catch
and returns undefined on error, then replace .map((s) => decodeURIComponent(s))
with .map(s => safeDecode(s)) and if any element is undefined short-circuit the
parser to return undefined (same change must be applied to the other parser
block that also calls decodeURIComponent on split segments).
- Around line 159-166: parseSkillUriWithKnownSkill currently uses
remainder.startsWith(knownSkillPath) which allows prefix collisions; change the
match to require a path boundary by verifying either remainder ===
knownSkillPath or the character immediately after knownSkillPath in remainder is
'/' (equivalently use remainder.startsWith(knownSkillPath + '/')). Update the
check around variables remainder and afterSkillPath in
parseSkillUriWithKnownSkill so that only exact skill-path matches or those
followed by a '/' proceed; return undefined for prefix-only matches like
"my-skill-2".
In `@libs/sdk/src/skill/skill-directory-loader.ts`:
- Around line 88-98: The findNestedSkillMd traversal currently caps recursion at
a hardcoded _maxDepth (default 8) and swallows all I/O errors in the walk
helper, allowing nested SKILL.md files to be missed or invalid descendants
silently skipped; update findNestedSkillMd/walk to accept and enforce a
configurable maxDepth (allow Infinity or a larger safe default instead of 8) and
stop using bare catches around readdir/stat — catch and ignore only ENOENT (or
missing-directory) errors but rethrow or propagate other errors (or at minimum
log them) so real filesystem failures surface; reference the walk function
inside findNestedSkillMd and the readdir/stat calls to locate and change the
error handling and depth check.
In `@libs/sdk/src/transport/in-memory-server.ts`:
- Around line 122-155: The in-memory server only extracts
experimental/extensions from scope.skills (via skillsCapabilities and the
single-item loop) so it misses experimental/extensions from other capability
contributors; change the merging logic to iterate over the full set of
capability objects used to build baseCapabilities (e.g., remoteCapabilities,
skillsCapabilities, scope.tools.getCapabilities(),
scope.resources.getCapabilities(), scope.prompts.getCapabilities(),
scope.agents.getCapabilities(), completionsCapability,
computeTaskCapabilities(scope)) and Object.assign any found cap['experimental']
into experimental and cap['extensions'] into extensions before attaching them to
baseCapabilities; keep the existing experimental/extensions objects and the
final conditional assignments to baseCapabilities['experimental'] and
baseCapabilities['extensions'] unchanged.
---
Outside diff comments:
In `@libs/sdk/src/resource/flows/resources-list.flow.ts`:
- Around line 307-313: When adding resourceUri for HTML MCP apps in the mimeType
check (resource.metadata.mimeType === 'text/html;profile=mcp-app'), avoid
replacing an existing nested ui object on item._meta; instead perform a nested
merge so existing item._meta.ui keys are preserved and only resourceUri is
added/overwritten. Update the assignment to item._meta to merge the existing
item._meta and its ui object with the new ui.resourceUri (using the existing ui
spread), referencing the symbols item._meta, ui, resourceUri and the uri
variable so upstream ui fields are retained.
---
Minor comments:
In `@libs/sdk/src/common/metadata/app.metadata.ts`:
- Around line 118-120: Update the URI template in the doc comment from
"skill://{skillPath}/SKILL.md" to "skill://{+skillPath}/SKILL.md" to match
SEP-2640 and the canonical template in libs/sdk/src/skill/README.md; locate the
occurrence in the app metadata comment that mentions the MCP resource and
replace the template token {skillPath} with {+skillPath} so documentation is
consistent across the codebase.
In `@libs/sdk/src/common/metadata/plugin.metadata.ts`:
- Around line 128-130: Update the SEP-2640 resource template in the comment text
to use the repository's placeholder form: replace the literal
"skill://{skillPath}/SKILL.md" with "skill://{+skillPath}/SKILL.md" in the
documentation string inside libs/sdk/src/common/metadata/plugin.metadata.ts (the
comment block that currently reads `They can be discovered via the
`skill://index.json` MCP resource and loaded via `skill://{skillPath}/SKILL.md`
MCP resources (per MCP SEP-2640).`). Ensure only the template placeholder is
changed to `{+skillPath}` and preserve the rest of the sentence and formatting.
In `@libs/sdk/src/direct/direct-client.ts`:
- Around line 474-480: listSep2640Skills currently returns any object with
string type/url; tighten the runtime validation in the filter over doc.skills so
it only returns true Sep2640IndexEntry values by checking that (a) the type
field equals one of the known entry types (e.g. the allowed set your spec uses)
and (b) the url field is a valid skill URL (startsWith "skill://" and optionally
further basic validation). Update the predicate inside listSep2640Skills to
explicitly check allowed types and url.startsWith("skill://") (and reject
otherwise) so callers receive only well-formed Sep2640IndexEntry objects from
doc.skills.
In `@libs/sdk/src/resource/resource.registry.ts`:
- Around line 714-724: The prebuilt-record detection (isPrebuiltRecord)
currently only checks for 'kind' and 'metadata' on resourceDef and can skip
normalization even if 'provide' is missing/invalid; update the guard (or add a
follow-up validation) so that when treating resourceDef as a prebuilt
ResourceRecord/ResourceTemplateRecord you also validate the presence and shape
of 'provide' (e.g., exists and is an array or object as your schema expects)
before assigning rec; if 'provide' is invalid, either fall back to running
normalizeResource/normalizeResourceTemplate (using isResourceTemplate to choose)
or throw a clear validation error so registration fails fast instead of creating
an inconsistent ResourceRecord/ResourceTemplateRecord.
In `@libs/sdk/src/skill/sep-2640/__tests__/sep-2640.metadata.spec.ts`:
- Around line 44-48: The test currently uses skillPath: [] which only validates
the array-level .min(1); change the case to provide an array with an empty
segment (e.g. skillPath: ['']) so the per-segment `.min(1)` constraint on each
element of skillMetadataSchema is exercised; update the spec assertion
accordingly to still expect a failure from skillMetadataSchema.safeParse({
...baseSkill, skillPath: [''] }).
In `@libs/sdk/src/skill/sep-2640/sep-2640.uri.ts`:
- Around line 216-224: The validator for skill path doesn't enforce the
documented 64-character max; update validateSkillPath (the block using
finalSegment) to reject names longer than 64 characters by adding a length check
(e.g., if finalSegment.length > 64) that throws a clear Error mentioning the
skill name and the 64-character limit, or incorporate the length into the
existing regex test for finalSegment so overlong names are rejected before
allowing registration.
In `@libs/sdk/src/skill/skill.registry.ts`:
- Around line 972-976: addSep2640InstructionUri currently mutates the
sep2640InstructionUris array without notifying listeners; update
addSep2640InstructionUri to call this.bump('reset') (or the registry's reset
bump method) whenever it actually adds a new URI so observers receive a reset
event; reference the addSep2640InstructionUri method and the
sep2640InstructionUris array and ensure bump('reset') is only invoked when the
URI was pushed to avoid spurious resets.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-skills/e2e/skills-resources.e2e.spec.ts`:
- Around line 113-117: Remove the non-null assertions on reviewPr and add a real
runtime guard before accessing its properties: after the existing
expect(reviewPr).toBeDefined(), explicitly fail or return if reviewPr is
undefined (e.g., if (!reviewPr) return fail('reviewPr missing')), then access
reviewPr.type, reviewPr.url and reviewPr.description without using !;
alternatively replace the three property assertions with a single assertion that
reviewPr matches the expected shape (e.g.,
expect(reviewPr).toEqual(expect.objectContaining({ type: 'skill-md', url:
'skill://review-pr/SKILL.md' }))) so no non-null operator is needed.
In `@libs/sdk/src/skill/index.ts`:
- Around line 167-205: The public API surface was changed to export SEP-2640
symbols (e.g., SEP_2640_EXTENSION_ID, buildSkillIndex, parseSkillUri,
getSep2640Resources, Sep2640SkillFileResource, findAndLoadSkillByPath) and
removed the legacy skills:// surface; update the draft docs under
docs/draft/docs/** in this same PR to reflect the new SEP-2640 APIs and the
removal of skills:// (add/modify pages describing the exported functions/types,
migration notes, and examples for parseSkillUri/parseSkillUriWithKnownSkill,
buildSkillIndex/serializeSkillMd, and resource helpers like
findAndLoadSkillByPath/getSep2640Resources) so the SDK docs match the new public
exports before merging.
In `@libs/sdk/src/skill/sep-2640/resources/index.ts`:
- Around line 27-28: The function getSep2640Resources currently returns
Function[] which erases constructor types; change its signature to return a
typed constructor array instead. Replace Function[] with either a union of the
resource constructor types (e.g. Array<typeof Sep2640SkillIndexResource | typeof
Sep2640SkillMdResource | typeof Sep2640SkillFileResource>) or define a shared
constructor type (e.g. type ResourceCtor = new (...args: any[]) => /*instance
type*/; and use ResourceCtor[]) and update getSep2640Resources to return that
typed array so callers preserve concrete constructor types for
Sep2640SkillIndexResource, Sep2640SkillMdResource, and Sep2640SkillFileResource.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7207857d-529e-43e0-838d-4cf84d9623e6
⛔ Files ignored due to path filters (2)
libs/cli/src/commands/build/exec/__tests__/schema-extractor.spec.tsis excluded by!**/build/**libs/cli/src/commands/build/exec/cli-runtime/schema-extractor.tsis excluded by!**/build/**
📒 Files selected for processing (68)
CLAUDE.mdapps/demo/src/skilled-openapi-fixtures/README.mdapps/e2e/demo-e2e-cli-exec/e2e/cli-build.e2e.spec.tsapps/e2e/demo-e2e-skills/e2e/load-skill.e2e.spec.tsapps/e2e/demo-e2e-skills/e2e/plugin-skills.e2e.spec.tsapps/e2e/demo-e2e-skills/e2e/search-skills.e2e.spec.tsapps/e2e/demo-e2e-skills/e2e/skills-only-mode.e2e.spec.tsapps/e2e/demo-e2e-skills/e2e/skills-resources.e2e.spec.tsapps/e2e/demo-e2e-skills/e2e/tool-authorization.e2e.spec.tsapps/e2e/demo-e2e-skills/src/apps/skills/skills/docs-skill/SKILL.mdapps/e2e/demo-e2e-skills/src/apps/skills/skills/mcp-only.skill.tsapps/e2e/demo-e2e-skills/src/main.tslibs/guard/project.jsonlibs/sdk/src/common/entries/skill.entry.tslibs/sdk/src/common/interfaces/skill.interface.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/metadata/plugin.metadata.tslibs/sdk/src/common/metadata/resource.metadata.tslibs/sdk/src/common/metadata/skill.metadata.tslibs/sdk/src/common/tokens/resource.tokens.tslibs/sdk/src/common/tokens/skill.tokens.tslibs/sdk/src/common/types/options/skills-http/interfaces.tslibs/sdk/src/common/types/options/skills-http/schema.tslibs/sdk/src/direct/client.types.tslibs/sdk/src/direct/direct-client.tslibs/sdk/src/direct/index.tslibs/sdk/src/resource/flows/resources-list.flow.tslibs/sdk/src/resource/resource.registry.tslibs/sdk/src/resource/resource.utils.tslibs/sdk/src/skill/README.mdlibs/sdk/src/skill/__tests__/skill-resource-helpers.spec.tslibs/sdk/src/skill/__tests__/skill-resources.spec.tslibs/sdk/src/skill/index.tslibs/sdk/src/skill/resources/index.tslibs/sdk/src/skill/resources/skill-content-alias.resource.tslibs/sdk/src/skill/resources/skill-content.resource.tslibs/sdk/src/skill/resources/skill-example-content.resource.tslibs/sdk/src/skill/resources/skill-examples-list.resource.tslibs/sdk/src/skill/resources/skill-reference-content.resource.tslibs/sdk/src/skill/resources/skill-references-list.resource.tslibs/sdk/src/skill/resources/skill-resource.helpers.tslibs/sdk/src/skill/resources/skills-catalog.resource.tslibs/sdk/src/skill/sep-2640/__tests__/sep-2640.builders.spec.tslibs/sdk/src/skill/sep-2640/__tests__/sep-2640.metadata.spec.tslibs/sdk/src/skill/sep-2640/__tests__/sep-2640.no-nesting.spec.tslibs/sdk/src/skill/sep-2640/__tests__/sep-2640.per-skill.spec.tslibs/sdk/src/skill/sep-2640/__tests__/sep-2640.registry.spec.tslibs/sdk/src/skill/sep-2640/__tests__/sep-2640.uri.spec.tslibs/sdk/src/skill/sep-2640/index.tslibs/sdk/src/skill/sep-2640/resources/index.tslibs/sdk/src/skill/sep-2640/resources/skill-file.resource.tslibs/sdk/src/skill/sep-2640/resources/skill-index.resource.tslibs/sdk/src/skill/sep-2640/resources/skill-md.resource.tslibs/sdk/src/skill/sep-2640/sep-2640.builders.tslibs/sdk/src/skill/sep-2640/sep-2640.constants.tslibs/sdk/src/skill/sep-2640/sep-2640.last-modified.tslibs/sdk/src/skill/sep-2640/sep-2640.per-skill.tslibs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.tslibs/sdk/src/skill/sep-2640/sep-2640.uri.tslibs/sdk/src/skill/skill-directory-loader.tslibs/sdk/src/skill/skill-mode.utils.tslibs/sdk/src/skill/skill-scope.helper.tslibs/sdk/src/skill/skill.registry.tslibs/sdk/src/skill/skill.utils.tslibs/sdk/src/transport/adapters/transport.local.adapter.tslibs/sdk/src/transport/in-memory-server.tslibs/skills/catalog/TEMPLATE.md
💤 Files with no reviewable changes (11)
- libs/sdk/src/skill/resources/skill-content-alias.resource.ts
- libs/sdk/src/skill/resources/skill-examples-list.resource.ts
- libs/sdk/src/skill/resources/skill-content.resource.ts
- libs/sdk/src/skill/tests/skill-resource-helpers.spec.ts
- libs/sdk/src/skill/resources/skills-catalog.resource.ts
- libs/sdk/src/skill/resources/skill-reference-content.resource.ts
- libs/sdk/src/skill/resources/skill-references-list.resource.ts
- libs/sdk/src/skill/resources/skill-example-content.resource.ts
- libs/sdk/src/skill/resources/index.ts
- libs/sdk/src/skill/tests/skill-resources.spec.ts
- libs/sdk/src/skill/resources/skill-resource.helpers.ts
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 101 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-05-05T15:08:31.916Z |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
libs/sdk/src/skill/skill-directory-loader.ts (1)
124-126: ⚡ Quick winMake nested-path output deterministic.
Sort
foundbefore returning so validation errors are stable across environments and easier to assert in tests.Proposed patch
- await walk(dirPath, 0, ''); - return found; + await walk(dirPath, 0, ''); + return found.sort((a, b) => a.localeCompare(b));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/skill-directory-loader.ts` around lines 124 - 126, The list collected by walk is non-deterministic; before returning the accumulated found array in the function that calls await walk(dirPath, 0, ''), sort the found array (e.g., by nested-path/string key) so output is deterministic across environments—update the return logic in skill-directory-loader.ts to sort the found collection (reference: found and walk) before returning.libs/sdk/src/transport/in-memory-server.ts (1)
169-175: ⚡ Quick winReplace
Record<string, never>cast with a more accurate type alias for forward-compatibility.The cast
as Record<string, never>is misleading—it claims an empty object type whenbaseCapabilitiesactually contains capability entries. Use a type alias that correctly represents a capabilities object withextensionsfield support:Suggested fix
+type ServerCapabilitiesWithExtensions = Record<string, unknown> & { + experimental?: Record<string, unknown>; + extensions?: Record<string, unknown>; +}; + // The MCP `ServerOptions.capabilities` type doesn't model the // forward-compat `extensions` key yet (it's reserved for SEP-2133). // The runtime passthrough is fine; the cast keeps TS happy. const serverOptions = { instructions: '', - capabilities: baseCapabilities as Record<string, never>, + capabilities: baseCapabilities as ServerCapabilitiesWithExtensions, serverInfo: scope.metadata.info,This improves clarity about the actual object shape and maintains strict TypeScript correctness per the coding guidelines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/transport/in-memory-server.ts` around lines 169 - 175, The current cast of baseCapabilities to Record<string, never> in the serverOptions construction is misleading; create a proper type alias (e.g., CapabilitiesWithExtensions or MCPServerCapabilities) that models a capabilities map and includes an optional extensions field (extensions?: Record<string, unknown> or a more specific shape), replace the cast on baseCapabilities with that type (use baseCapabilities as CapabilitiesWithExtensions) and update the serverOptions.capabilities typing to use the new alias instead of Record<string, never> so TypeScript accurately reflects forward-compatibility for the extensions key.apps/e2e/demo-e2e-skills/e2e/skills-resources.e2e.spec.ts (1)
202-205: ⚡ Quick winAdd a percent-encoded traversal regression test.
This only exercises literal
../. The risky path here is percent-encoded separators/dots, e.g.skill://docs-skill/references%2F..%2FSKILL.md, because the resolver decodes segments before building the filesystem path. A dedicated case will lock in theSKILL.mdblock and the traversal guard together.Suggested test
test('should reject path traversal attempts', async ({ mcp }) => { const result = await mcp.resources.read('skill://docs-skill/../../etc/passwd'); expect(result).toBeError(); }); + + test('should reject percent-encoded traversal and SKILL.md bypass attempts', async ({ mcp }) => { + const result = await mcp.resources.read('skill://docs-skill/references%2F..%2FSKILL.md'); + expect(result).toBeError(); + });As per coding guidelines "Ensure any SKILL.md access via the file resource is blocked (requests targeting
SKILL.mdshould not succeed)`."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/e2e/demo-e2e-skills/e2e/skills-resources.e2e.spec.ts` around lines 202 - 205, Add a new e2e test that verifies percent-encoded path traversal is rejected: create a test (similar to 'should reject path traversal attempts') that calls mcp.resources.read with a percent-encoded traversal URL like 'skill://docs-skill/references%2F..%2FSKILL.md' and assert the result is an error (expect(result).toBeError()), ensuring SKILL.md access via file resources is blocked; place it alongside the existing literal ../ test and reuse mcp.resources.read for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/sdk/src/resource/flows/resources-list.flow.ts`:
- Around line 309-314: Guard the ui shape before spreading: instead of
unconditionally casting item._meta?.['ui'] to Record<string, unknown>, validate
at runtime that item._meta?.['ui'] is a plain object (e.g., typeof === 'object'
&& not null && !Array.isArray) before using it as existingUi; if it isn't an
object, treat existingUi as an empty object, then assign item._meta = {
...(item._meta ?? {}), ui: { ...existingUi, resourceUri: uri } } so spreading
cannot explode when plugins supply non-object ui values (references: existingUi,
item._meta, ui, resourceUri, uri).
In `@libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts`:
- Around line 237-240: The helper currently swallows all errors from
instance.load() by returning undefined, which causes real load failures to be
treated later as ResourceNotFoundError; instead, change the catch around
instance.load() in sep-2640.resource-helpers.ts to not suppress errors — either
rethrow the caught error or only return undefined when the caught error is a
true "not found" condition (e.g., check the error type/message). Update the
catch block around instance.load() so it either throws the original error
(preserving the failure) or inspects the error and returns undefined only for
explicit not-found errors, leaving other errors to propagate to
readSkillFileByPath().
- Around line 126-135: Decode the raw filePath before splitting into path
segments and then validate; currently you call split('/') then
decodeURIComponent which allows encoded separators to bypass the '..' and '.'
checks. Change the flow so you first run decodeURIComponent(filePath) and only
then split('/'), filter, and map; keep the existing traversal check on
segments.some(s => s === '..' || s === '.') and also add a guard rejecting any
segment equal to "SKILL.md" (to enforce the SKILL.md access block) before
proceeding to pathResolve.
- Around line 287-289: The isInside(child: string, parent: string) containment
check currently assumes forward slashes; update it to be OS-agnostic by
normalizing both inputs (from pathResolve calls) before comparison—either
replace the manual startsWith logic with Node's path.relative(parent, child) and
ensure the result does not start with '..' or normalize both paths to POSIX
separators (e.g., replace backslashes with '/') before checking
equality/startsWith; modify the isInside function accordingly and ensure callers
(where pathResolve is used) pass the resolved paths into the updated isInside.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-skills/e2e/skills-resources.e2e.spec.ts`:
- Around line 202-205: Add a new e2e test that verifies percent-encoded path
traversal is rejected: create a test (similar to 'should reject path traversal
attempts') that calls mcp.resources.read with a percent-encoded traversal URL
like 'skill://docs-skill/references%2F..%2FSKILL.md' and assert the result is an
error (expect(result).toBeError()), ensuring SKILL.md access via file resources
is blocked; place it alongside the existing literal ../ test and reuse
mcp.resources.read for consistency.
In `@libs/sdk/src/skill/skill-directory-loader.ts`:
- Around line 124-126: The list collected by walk is non-deterministic; before
returning the accumulated found array in the function that calls await
walk(dirPath, 0, ''), sort the found array (e.g., by nested-path/string key) so
output is deterministic across environments—update the return logic in
skill-directory-loader.ts to sort the found collection (reference: found and
walk) before returning.
In `@libs/sdk/src/transport/in-memory-server.ts`:
- Around line 169-175: The current cast of baseCapabilities to Record<string,
never> in the serverOptions construction is misleading; create a proper type
alias (e.g., CapabilitiesWithExtensions or MCPServerCapabilities) that models a
capabilities map and includes an optional extensions field (extensions?:
Record<string, unknown> or a more specific shape), replace the cast on
baseCapabilities with that type (use baseCapabilities as
CapabilitiesWithExtensions) and update the serverOptions.capabilities typing to
use the new alias instead of Record<string, never> so TypeScript accurately
reflects forward-compatibility for the extensions key.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb60cac7-f8fa-477e-af71-f679e325b251
📒 Files selected for processing (23)
apps/e2e/demo-e2e-skills/e2e/skills-resources.e2e.spec.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/common/metadata/plugin.metadata.tslibs/sdk/src/common/metadata/resource.metadata.tslibs/sdk/src/common/metadata/skill.metadata.tslibs/sdk/src/common/tokens/resource.tokens.tslibs/sdk/src/direct/client.types.tslibs/sdk/src/direct/direct-client.tslibs/sdk/src/direct/index.tslibs/sdk/src/resource/flows/resources-list.flow.tslibs/sdk/src/resource/resource.registry.tslibs/sdk/src/resource/resource.utils.tslibs/sdk/src/skill/sep-2640/__tests__/sep-2640.builders.spec.tslibs/sdk/src/skill/sep-2640/__tests__/sep-2640.metadata.spec.tslibs/sdk/src/skill/sep-2640/resources/index.tslibs/sdk/src/skill/sep-2640/resources/skill-file.resource.tslibs/sdk/src/skill/sep-2640/sep-2640.builders.tslibs/sdk/src/skill/sep-2640/sep-2640.per-skill.tslibs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.tslibs/sdk/src/skill/sep-2640/sep-2640.uri.tslibs/sdk/src/skill/skill-directory-loader.tslibs/sdk/src/skill/skill.registry.tslibs/sdk/src/transport/in-memory-server.ts
✅ Files skipped from review due to trivial changes (3)
- libs/sdk/src/common/metadata/plugin.metadata.ts
- libs/sdk/src/common/metadata/app.metadata.ts
- libs/sdk/src/skill/sep-2640/sep-2640.per-skill.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- libs/sdk/src/direct/index.ts
- libs/sdk/src/resource/resource.registry.ts
- libs/sdk/src/skill/sep-2640/resources/index.ts
- libs/sdk/src/direct/client.types.ts
- libs/sdk/src/common/tokens/resource.tokens.ts
- libs/sdk/src/skill/sep-2640/sep-2640.builders.ts
- libs/sdk/src/direct/direct-client.ts
- libs/sdk/src/skill/skill.registry.ts
- libs/sdk/src/skill/sep-2640/sep-2640.uri.ts
- libs/sdk/src/common/metadata/skill.metadata.ts
- libs/sdk/src/common/metadata/resource.metadata.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
312-316: ⚡ Quick winConsider narrowing the
*.generated.tsignore scope.Line 316 currently excludes all generated TypeScript files repo-wide. Scoping it to known generated locations would reduce the chance of unintentionally skipping review on important committed outputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coderabbit.yaml around lines 312 - 316, The global ignore pattern "!**/*.generated.ts" in .coderabbit.yaml is too broad; narrow it by replacing that single repo-wide pattern with targeted generated-file patterns scoped to known generators (e.g., limit to the specific directories/output names produced by scripts/generate-schema-types.mjs and other generators) so only intended generated files are excluded; update the pattern(s) in .coderabbit.yaml (replace "!**/*.generated.ts") with a small set of directory-scoped glob(s) that match only the actual generated locations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.coderabbit.yaml:
- Around line 312-316: The global ignore pattern "!**/*.generated.ts" in
.coderabbit.yaml is too broad; narrow it by replacing that single repo-wide
pattern with targeted generated-file patterns scoped to known generators (e.g.,
limit to the specific directories/output names produced by
scripts/generate-schema-types.mjs and other generators) so only intended
generated files are excluded; update the pattern(s) in .coderabbit.yaml (replace
"!**/*.generated.ts") with a small set of directory-scoped glob(s) that match
only the actual generated locations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 493a97ae-0bd3-47d1-b308-679edb4fb5d5
📒 Files selected for processing (1)
.coderabbit.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/utils/src/uri/uri-template.ts`:
- Around line 167-171: The encodeReserved function currently preserves existing
%xx triplets which causes expandUriTemplate to emit percent-encoded sequences
that matchUriTemplate later decodes, breaking the “raw params” contract; update
encodeReserved to first percent-decode the incoming value (safely, catching
malformed sequences) and then pass the decoded string through
encodeReservedSegment so expansions always encode from the raw value; mention
and update any helper used (encodeReservedSegment) and ensure
expandUriTemplate/matchUriTemplate semantics remain consistent after the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67868bbd-bd23-480d-b76a-fd6db86b6b48
📒 Files selected for processing (3)
libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.tslibs/utils/src/uri/uri-template.spec.tslibs/utils/src/uri/uri-template.ts
✅ Files skipped from review due to trivial changes (2)
- libs/utils/src/uri/uri-template.spec.ts
- libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts (2)
219-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly map true misses to
ResourceNotFoundError.These catches currently turn every read failure into “not found”. That also hides permission errors, directories passed as files, and other I/O failures as 404s. Restrict the 404 mapping to ENOENT-style misses and surface the rest as server errors.
As per coding guidelines "Use specific error classes with MCP error codes instead of generic Error. Never use non-null assertions (
!) - throw proper errors instead".Also applies to: 301-309
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts` around lines 219 - 223, The catch around readFileBuffer(candidatePath) currently maps every read failure to ResourceNotFoundError; change it to only throw ResourceNotFoundError when the underlying error indicates a real "not found" (e.g., errno ENOENT / code 'ENOENT' or equivalent), and for all other errors either rethrow or wrap them in an appropriate server-side MCP error class (e.g., InternalServerError with an MCP error code) instead of masking them as 404s; update the same pattern found in the similar block around lines 301-309; reference the readFileBuffer call and the thrown ResourceNotFoundError and include instance.getSkillPath()/filePath in error messages while avoiding non-null assertions and using specific error classes per guidelines.
123-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the
SKILL.mdblock actually case-insensitive.Both guards only reject exact
SKILL.md, soskill.mdor mixed-case variants can still reach the generic file route on case-insensitive filesystems and bypass the dedicatedSKILL.mdresource.Also applies to: 149-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts` around lines 123 - 125, Change the exact-match guard to be case-insensitive: replace checks like `if (!filePath || filePath === 'SKILL.md')` with `if (!filePath || filePath.toLowerCase() === 'skill.md')` so mixed-case variants are caught; apply the same change to the other occurrence (the block around the second `SKILL.md` guard), keeping the thrown `ResourceNotFoundError(\`skill://${instance.getSkillPath()}/${filePath}\`)` behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts`:
- Around line 329-337: The code in resolveEntryPath treats absolute paths by
checking startsWith('/') which fails on Windows; update the checks to use Node's
path.isAbsolute (or equivalent platform-aware check) for both the incoming
absolutePath and the configured resource path (from
instance.getResources()?.[kind]) so drive-letter and UNC paths are detected;
ensure the branches that currently test startsWith('/') now use isAbsolute to
return absolute paths directly and to set dir when configured is absolute, while
preserving the existing joinPath/pathResolve logic when configured is relative
and baseDir exists.
---
Duplicate comments:
In `@libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts`:
- Around line 219-223: The catch around readFileBuffer(candidatePath) currently
maps every read failure to ResourceNotFoundError; change it to only throw
ResourceNotFoundError when the underlying error indicates a real "not found"
(e.g., errno ENOENT / code 'ENOENT' or equivalent), and for all other errors
either rethrow or wrap them in an appropriate server-side MCP error class (e.g.,
InternalServerError with an MCP error code) instead of masking them as 404s;
update the same pattern found in the similar block around lines 301-309;
reference the readFileBuffer call and the thrown ResourceNotFoundError and
include instance.getSkillPath()/filePath in error messages while avoiding
non-null assertions and using specific error classes per guidelines.
- Around line 123-125: Change the exact-match guard to be case-insensitive:
replace checks like `if (!filePath || filePath === 'SKILL.md')` with `if
(!filePath || filePath.toLowerCase() === 'skill.md')` so mixed-case variants are
caught; apply the same change to the other occurrence (the block around the
second `SKILL.md` guard), keeping the thrown
`ResourceNotFoundError(\`skill://${instance.getSkillPath()}/${filePath}\`)`
behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4dac9292-4a54-4586-97ad-1206bd802da9
📒 Files selected for processing (3)
libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.tslibs/utils/src/uri/uri-template.spec.tslibs/utils/src/uri/uri-template.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/utils/src/uri/uri-template.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
libs/utils/src/path/browser-path.ts (1)
34-41: ⚡ Quick winAdd a focused test matrix for
isAbsolutebehavior.Please add unit tests for key cases (
'',/foo,\\foo,C:\\foo,C:/foo,C:foo) to lock behavior and protect against regressions.Based on learnings, "Maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)" and "All tests must pass (100% test pass rate required)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/utils/src/path/browser-path.ts` around lines 34 - 41, Add a focused unit test suite for the exported function isAbsolute to lock its behavior and prevent regressions: create tests that call isAbsolute with the cases '' (empty string), '/foo' (POSIX root), '\\foo' (leading backslash), 'C:\\foo' and 'C:/foo' (Windows drive letter with separator), and 'C:foo' (drive letter without separator) and assert the expected boolean outcomes (false for '', true for '/foo', true for '\\foo', true for 'C:\\foo', true for 'C:/foo', false for 'C:foo'). Put tests alongside other utils tests and ensure they run in the project test runner so coverage and pass-rate requirements are met.libs/skills/__tests__/skills-validation.spec.ts (1)
824-842: ⚡ Quick winAlign this suite with TEMPLATE.md’s SEP-2640 resource-access contract.
These assertions hardcode the older
## Accessing This Skill/skill://${name}/SKILL.mdshape. The template for this migration requires aResource Accesssection that documentsskill://index.json, per-skillSKILL.md, the resource subpaths, and nested{skillPath}support. As written, incomplete docs can still pass, and a valid nested skill path would fail.Suggested direction
- '## Accessing This Skill', + '## Resource Access',- const name = frontmatter['name']; + const skillPath = Array.isArray(frontmatter['skillPath']) + ? frontmatter['skillPath'] + .filter((segment): segment is string => typeof segment === 'string' && segment.length > 0) + .join('/') + : typeof frontmatter['name'] === 'string' + ? frontmatter['name'] + : undefined; ... - expect(sectionBody).toContain(`skill://${name}/SKILL.md`); + expect(sectionBody).toContain('skill://index.json'); + expect(sectionBody).toContain(`skill://${skillPath}/SKILL.md`);As per coding guidelines, ensure the “Resource Access” section documents the singular SEP-2640
skill://URIs and that{skillPath}may be a single or nested path whose final segment matches the skill name.Also applies to: 845-860
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/skills/__tests__/skills-validation.spec.ts` around lines 824 - 842, Update the test in skills-validation.spec.ts (the it.each using findAllSkillDirs() and readSkillFile()) to check for the TEMPLATE.md SEP-2640 contract: replace the hardcoded '## Accessing This Skill' check with a '## Resource Access' check and assert that the Resource Access section documents both skill://index.json and per-skill SKILL.md URIs, that it lists allowed resource subpaths, and that any `{skillPath}` examples may be nested but have a final segment equal to the skill directory name; use findAllSkillDirs() to derive the expected skill name and readSkillFile() to parse the body and validate presence/format of skill://index.json, skill://<skillPath>/SKILL.md, and that nested paths end with the skill name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts`:
- Around line 193-205: The code uses configured.startsWith('/') to detect
absolute paths (in the loop over candidateDirs) which fails on Windows; replace
that check with the platform-aware path.isAbsolute (or isAbsolute) call used in
resolveEntryPath so that when computing dir in the loop (reference:
candidateDirs, configured, baseDir, pathResolve) you treat configured as
absolute via path.isAbsolute(configured) before resolving, then compute
candidatePath and validate with isInside as before and throw the same
PublicMcpError for invalid filePath; ensure you import/use the same isAbsolute
helper already used in resolveEntryPath to keep behavior consistent.
In `@libs/sdk/src/skill/skill-md-parser.ts`:
- Around line 109-110: The current numeric check for the frontmatter rating only
uses typeof and can accept NaN/Infinity; update the logic in skill-md-parser
(the block that assigns result.rating from frontmatter['rating']) to only accept
finite numbers within 0..5 by using Number.isFinite(frontmatter['rating']) &&
frontmatter['rating'] >= 0 && frontmatter['rating'] <= 5 before assigning; if
the value fails validation, do not set result.rating (or handle as the existing
codebase expects for invalid frontmatter).
In `@libs/utils/src/path/index.ts`:
- Line 2: Remove the legacy renamed aliases from the barrel export: replace the
aliased exports "resolve as pathResolve" and "join as pathJoin" and export the
canonical symbols "resolve" and "join" instead (keep basename, dirname, extname,
isAbsolute as-is) so the export line only re-exports canonical names from
'#path'; update any local references using pathResolve/pathJoin elsewhere to use
resolve/join or add a separate migration note, but do not keep the aliased names
in this index.ts barrel.
---
Nitpick comments:
In `@libs/skills/__tests__/skills-validation.spec.ts`:
- Around line 824-842: Update the test in skills-validation.spec.ts (the it.each
using findAllSkillDirs() and readSkillFile()) to check for the TEMPLATE.md
SEP-2640 contract: replace the hardcoded '## Accessing This Skill' check with a
'## Resource Access' check and assert that the Resource Access section documents
both skill://index.json and per-skill SKILL.md URIs, that it lists allowed
resource subpaths, and that any `{skillPath}` examples may be nested but have a
final segment equal to the skill directory name; use findAllSkillDirs() to
derive the expected skill name and readSkillFile() to parse the body and
validate presence/format of skill://index.json, skill://<skillPath>/SKILL.md,
and that nested paths end with the skill name.
In `@libs/utils/src/path/browser-path.ts`:
- Around line 34-41: Add a focused unit test suite for the exported function
isAbsolute to lock its behavior and prevent regressions: create tests that call
isAbsolute with the cases '' (empty string), '/foo' (POSIX root), '\\foo'
(leading backslash), 'C:\\foo' and 'C:/foo' (Windows drive letter with
separator), and 'C:foo' (drive letter without separator) and assert the expected
boolean outcomes (false for '', true for '/foo', true for '\\foo', true for
'C:\\foo', true for 'C:/foo', false for 'C:foo'). Put tests alongside other
utils tests and ensure they run in the project test runner so coverage and
pass-rate requirements are met.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 348c2ec2-aa33-4926-a46f-83123ef51113
📒 Files selected for processing (23)
libs/sdk/src/skill/__tests__/skill-md-parser.spec.tslibs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.tslibs/sdk/src/skill/skill-md-parser.tslibs/skills/__tests__/skills-validation.spec.tslibs/skills/catalog/frontmcp-authorities/SKILL.mdlibs/skills/catalog/frontmcp-channels/SKILL.mdlibs/skills/catalog/frontmcp-config/SKILL.mdlibs/skills/catalog/frontmcp-deployment/SKILL.mdlibs/skills/catalog/frontmcp-development/SKILL.mdlibs/skills/catalog/frontmcp-extensibility/SKILL.mdlibs/skills/catalog/frontmcp-guides/SKILL.mdlibs/skills/catalog/frontmcp-observability/SKILL.mdlibs/skills/catalog/frontmcp-production-readiness/SKILL.mdlibs/skills/catalog/frontmcp-setup/SKILL.mdlibs/skills/catalog/frontmcp-testing/SKILL.mdlibs/skills/catalog/skills-manifest.jsonlibs/skills/scripts/generate-manifest.mjslibs/utils/src/index.tslibs/utils/src/path/browser-path.tslibs/utils/src/path/index.tslibs/utils/src/path/node-path.tslibs/utils/src/uri/uri-template.spec.tslibs/utils/src/uri/uri-template.ts
✅ Files skipped from review due to trivial changes (13)
- libs/utils/src/path/node-path.ts
- libs/utils/src/index.ts
- libs/skills/catalog/frontmcp-deployment/SKILL.md
- libs/skills/catalog/frontmcp-authorities/SKILL.md
- libs/skills/catalog/frontmcp-setup/SKILL.md
- libs/skills/catalog/frontmcp-channels/SKILL.md
- libs/skills/catalog/frontmcp-testing/SKILL.md
- libs/skills/catalog/frontmcp-extensibility/SKILL.md
- libs/skills/catalog/frontmcp-production-readiness/SKILL.md
- libs/skills/catalog/frontmcp-development/SKILL.md
- libs/skills/catalog/frontmcp-guides/SKILL.md
- libs/skills/catalog/frontmcp-observability/SKILL.md
- libs/skills/catalog/frontmcp-config/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/utils/src/uri/uri-template.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts (1)
23-26: ⚡ Quick winUse the shared
SkillVisibilitytype instead of hard-coding'mcp'.Typing the visibility value keeps this filter aligned with the metadata model and avoids silent drift if the allowed literals ever change.
As per coding guidelines "Use
SkillVisibilitytype from common/metadata instead of inline string literals for visibility values".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts` around lines 23 - 26, The visibility filter in getSepVisibleSkills uses the hard-coded string 'mcp'; update it to use the shared SkillVisibility type so the value is typed and stays in sync with the metadata model: import SkillVisibility from common/metadata (or the appropriate named export) and change registry.getSkills({ visibility: 'mcp' }) to registry.getSkills({ visibility: SkillVisibility.MCP }) or the corresponding SkillVisibility literal; keep ScopeEntry and SkillEntry usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts`:
- Around line 303-305: The code treats an empty-string inline payload as missing
because it checks truthiness of the `inline` variable; change the check to
detect presence of the content property instead (e.g., test whether `(entry as {
content?: string }).content` is defined or the `content` key exists) so that
empty-string `content: ''` is accepted and returned via
`parseSkillMdFrontmatter(inline).body` instead of falling back to disk; refer to
the `inline` variable, the `entry` object, and the `parseSkillMdFrontmatter`
call to locate and update the conditional.
- Around line 356-359: The resolved path returned by resolveEntryPath currently
uses pathResolve(dir, filename) without checking it stays inside the intended
resource directory; update resolveEntryPath (and any place that calls it such as
readSkillFileByPath) to compute const resolved = pathResolve(dir, filename) and
verify that resolved.startsWith(pathResolve(dir) + pathSep) or is exactly
pathResolve(dir) before returning; if the resolved path falls outside dir (e.g.,
contains "../" that escapes), return undefined (or treat as not found) so
metadata-driven filenames cannot escape the skill resource directory. Ensure you
reference variables dir, filename and use pathResolve/joinPath consistently when
performing the containment check.
In `@libs/sdk/src/skill/skill-md-parser.ts`:
- Around line 118-120: The code currently accepts skillPath entries containing
only whitespace; update the filter to trim each string before checking length so
segments are normalized—e.g., when computing segments from
frontmatter['skillPath'] (the variable segments) trim each candidate (s.trim())
and then filter out empties and non-strings, and assign the cleaned array to
result.skillPath; ensure the type guard and filtering use the trimmed value so
no whitespace-only components are included.
---
Nitpick comments:
In `@libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts`:
- Around line 23-26: The visibility filter in getSepVisibleSkills uses the
hard-coded string 'mcp'; update it to use the shared SkillVisibility type so the
value is typed and stays in sync with the metadata model: import SkillVisibility
from common/metadata (or the appropriate named export) and change
registry.getSkills({ visibility: 'mcp' }) to registry.getSkills({ visibility:
SkillVisibility.MCP }) or the corresponding SkillVisibility literal; keep
ScopeEntry and SkillEntry usage unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8402d5a1-ce61-48e2-9591-9e40dd7a2c1b
📒 Files selected for processing (4)
libs/sdk/src/skill/__tests__/skill-md-parser.spec.tslibs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.tslibs/sdk/src/skill/skill-md-parser.tslibs/utils/src/path/browser-path.spec.ts
✅ Files skipped from review due to trivial changes (1)
- libs/utils/src/path/browser-path.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/skill/tests/skill-md-parser.spec.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/sdk/src/skill/__tests__/skill-md-parser.spec.ts (2)
590-602: ⚡ Quick winAdd a no-leak regression for kebab-case
available-when.You verify alias mapping at Lines 590-602, but the no-leak regression at Lines 604-622 only checks
availableWhencamelCase. Add a kebab-case variant so alias handling cannot regress intospecMetadataleakage.Based on learnings: "Achieve 95%+ code coverage across all metrics (statements, branches, functions, lines) for Jest tests".Suggested additional regression
+it('should not pass available-when through specMetadata', () => { + const result = skillMdFrontmatterToMetadata( + { + id: 'x', + name: 'leak-test-kebab', + description: 'Test', + 'available-when': { platform: ['linux'] }, + }, + 'Body', + ); + expect(result.availableWhen).toEqual({ platform: ['linux'] }); + expect(result.specMetadata).toBeUndefined(); +});Also applies to: 604-622
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/__tests__/skill-md-parser.spec.ts` around lines 590 - 602, Add a second no-leak regression test that mirrors the existing camelCase check but uses the kebab-case key 'available-when' to ensure alias mapping doesn't leak into specMetadata: call skillMdFrontmatterToMetadata with { name: 'mac-skill', description: 'Test', 'available-when': { platform: ['darwin'] } } and assert that the returned object's availableWhen equals { platform: ['darwin'] } and that specMetadata (or the same metadata container checked in the existing no-leak assertions) does not contain the raw 'available-when' key or leaked data; locate this near the existing no-leak test block in skill-md-parser.spec.ts so both camelCase and kebab-case are covered.
517-522: ⚡ Quick winAssert
specMetadatais unset for invalidratingtoo.Line 518 says invalid
ratingshould not be shoved intospecMetadata, but the test currently only assertsresult.ratingisundefined. Add the explicit no-leak assertion so this regression is fully pinned.Based on learnings: "Achieve 95%+ code coverage across all metrics (statements, branches, functions, lines) for Jest tests".Suggested test assertion
it('should not map non-numeric rating', () => { // Mirrors the priority behaviour: a mistyped value is dropped // rather than coerced or shoved into specMetadata. const result = skillMdFrontmatterToMetadata({ name: 'bad-rating', description: 'Test', rating: 'high' }, 'Body'); expect(result.rating).toBeUndefined(); + expect(result.specMetadata).toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/skill/__tests__/skill-md-parser.spec.ts` around lines 517 - 522, Test currently only checks result.rating is undefined; also assert the invalid value doesn't leak into specMetadata by adding an assertion on the result object returned from skillMdFrontmatterToMetadata (variable result) that verifies specMetadata is unset or does not contain a rating key (e.g., expect(result.specMetadata).toBeUndefined() or expect(result.specMetadata).not.toHaveProperty('rating')) so the bad 'rating' isn't stored in specMetadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libs/sdk/src/skill/__tests__/skill-md-parser.spec.ts`:
- Around line 590-602: Add a second no-leak regression test that mirrors the
existing camelCase check but uses the kebab-case key 'available-when' to ensure
alias mapping doesn't leak into specMetadata: call skillMdFrontmatterToMetadata
with { name: 'mac-skill', description: 'Test', 'available-when': { platform:
['darwin'] } } and assert that the returned object's availableWhen equals {
platform: ['darwin'] } and that specMetadata (or the same metadata container
checked in the existing no-leak assertions) does not contain the raw
'available-when' key or leaked data; locate this near the existing no-leak test
block in skill-md-parser.spec.ts so both camelCase and kebab-case are covered.
- Around line 517-522: Test currently only checks result.rating is undefined;
also assert the invalid value doesn't leak into specMetadata by adding an
assertion on the result object returned from skillMdFrontmatterToMetadata
(variable result) that verifies specMetadata is unset or does not contain a
rating key (e.g., expect(result.specMetadata).toBeUndefined() or
expect(result.specMetadata).not.toHaveProperty('rating')) so the bad 'rating'
isn't stored in specMetadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56fc8eec-3178-46b1-8d4a-47d8d9cfa0a3
📒 Files selected for processing (3)
libs/sdk/src/skill/__tests__/skill-md-parser.spec.tslibs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.tslibs/sdk/src/skill/skill-md-parser.ts
✅ Files skipped from review due to trivial changes (2)
- libs/sdk/src/skill/skill-md-parser.ts
- libs/sdk/src/skill/sep-2640/sep-2640.resource-helpers.ts
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
1 participant
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 by CodeRabbit
New Features
Improvements
Chores