Repository navigation
fix(sdk): send no numeric skill resource annotation and a compact skills line in the instructions by default - #986
Merged
Conversation
frontegg-david
marked this pull request as ready for review
November 2, 2026 03:43
Contributor
Author
|
@coderabbitai review |
Contributor
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Configuration options and validation libs/sdk/src/common/types/options/skills-http/*, libs/sdk/src/common/decorators/front-mcp.decorator.ts, libs/sdk/src/common/types/options/__tests__/skills-http.options.spec.ts, apps/e2e/demo-e2e-dts-consumer/*, libs/skills/catalog/frontmcp-config/references/configure-skills-http.md, libs/skills/catalog/frontmcp-development/references/decorators-guide.md, libs/skills/catalog/skills-manifest.json |
The options add compact instruction injection and resource annotation policies. Schema defaults are compact and no-priority. FrontMcp throws InvalidDecoratorMetadataError for invalid skillsConfig values. The type-consumer fixture covers supported and unsupported annotation options. |
Compact instruction composition libs/sdk/src/skill/skill-instructions.helper.ts, libs/sdk/src/skill/__tests__/*, libs/sdk/src/transport/adapters/__tests__/transport.local.skill-hints.spec.ts, plugins/plugin-feature-flags/src/__tests__/feature-flag.flows.spec.ts, apps/e2e/demo-e2e-feature-flags/*, apps/e2e/demo-e2e-skills/e2e/skills-only-mode.e2e.spec.ts, libs/skills/catalog/frontmcp-config/examples/configure-skills-http/inject-instructions.md, libs/skills/catalog/frontmcp-deployment/references/protocol-versions.md, libs/skills/catalog/frontmcp-production-readiness/references/common-checklist.md, libs/skills/catalog/skills-manifest.json |
Compact mode adds a skill-count line and discovery pointer based on caller-visible skills. Tests cover resource and method pointers, external providers, filtering, and explicit append behavior. E2E coverage checks compact instructions and append mode. |
Per-skill resource annotations libs/sdk/src/skill/sep-2640/sep-2640.per-skill.ts, libs/sdk/src/skill/skill-scope.helper.ts, libs/sdk/src/skill/sep-2640/__tests__/sep-2640.per-skill.spec.ts, apps/e2e/demo-e2e-skills/src/main.ts, apps/e2e/demo-e2e-skills/e2e/skills-resources.e2e.spec.ts |
Resource registration applies the selected annotation policy. Default annotations include assistant audience, full adds priority, and off omits annotations. Tests cover lastModified behavior and the E2E configuration exercises full mode. |
Estimated code review effort: 4 (Complex) | ~45 minutes
Sequence Diagram(s)
sequenceDiagram
participant MCPClient
participant FrontMcp
participant composeInitializeInstructions
participant SkillRegistry
MCPClient->>FrontMcp: Send initialize request
FrontMcp->>composeInitializeInstructions: Compose caller instructions
composeInitializeInstructions->>SkillRegistry: Select caller-visible skills
SkillRegistry-->>composeInitializeInstructions: Return listed skills
composeInitializeInstructions-->>FrontMcp: Return instructions with count and pointer
FrontMcp-->>MCPClient: Return initialize response
Suggested reviewers: alexmercerpo
Merge Risk: 
ef2da
-
No merge-blocking issue is established by the available evidence.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies both primary changes: removing numeric skill resource annotations and making compact skill instructions the default. It is specific and related to the changeset. Description check The description includes the required What and why, User-facing change, and Tests sections. It explains the defaults, compatibility options, affected documentation, validation behavior, and test resul… Docstring Coverage Docstring coverage is 66.67% which is sufficient. The required threshold is 65.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description includes the required What and why, User-facing change, and Tests sections. It explains the defaults, compatibility options, affected documentation, validation behavior, and test results in sufficient detail.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 66.67% which is sufficient. The required threshold is 65.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 12 functions across 23 files. (6 skipped: 6 unsupported.)
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
frontegg-david
marked this pull request as draft
November 2, 2026 03:51
frontegg-david
marked this pull request as ready for review
November 2, 2026 03:51
Contributor
Action not completed
|
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 101 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-11-02T06:45:32.589Z |
frontegg-david
marked this pull request as draft
November 2, 2026 04:24
frontegg-david
marked this pull request as ready for review
November 2, 2026 04:25
frontegg-david
marked this pull request as draft
November 2, 2026 04:52
frontegg-david
marked this pull request as ready for review
November 2, 2026 04:52
frontegg-david
marked this pull request as draft
November 2, 2026 05:24
frontegg-david
marked this pull request as ready for review
November 2, 2026 05:25
frontegg-david
marked this pull request as draft
November 2, 2026 05:58
frontegg-david
marked this pull request as ready for review
November 2, 2026 05:58
…kills line by default
Each SKILL.md resource in resources/list carried priority: 0.8, and some
clients fail the whole list on a numeric annotation. The new
skillsConfig.resourceAnnotations ('no-priority' by default, 'full', 'off')
keeps audience and lastModified and drops priority unless asked for.
The initialize instructions listed every skill's name and description, and
some clients copy them into each tool's description. The new default
injectInstructions: 'compact' sends one line with the skill count the
caller may see and where to read the skills; 'append', 'prepend' and
'replace' keep the full catalog.
An unknown value for either option, or any invalid skillsConfig, makes
@FrontMcp throw InvalidDecoratorMetadataError at boot.
…ternal provider serves skills A read-only skillsConfig.externalProvider serves skills that skill://index.json lists but the registry does not hold, so the compact line undercounted them, and sent no line at all when every skill came from the provider. It now reads "This server has skills: ..." in that case. The catalog no longer says 'off' adds nothing about skills (the sep2640InInstructions hints are still sent), and protocol-versions names the skills line for server/discover.
frontegg-david
force-pushed
the
fix/06-client-safe-skills
branch
from
November 2, 2026 06:31
1f9763a to
fa583c0
Compare
Contributor
Cherry-pick CreatedA cherry-pick PR to Please review and merge if this change should also be in If the cherry-pick is not needed, close the PR. |
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
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.
Stacked on #985 (base
fix/03-hook-order, the top of #980 → #981 → #983 → #984 → #985).What and why
Proposal item 1.8: two skill defaults that break MCP clients. Both defaults change; how to get the old behaviour back
is under User-facing change.
Numeric annotations on every
SKILL.mdresource. Eachskill://<skillPath>/SKILL.mdresource inresources/listcarried
annotations: { audience: ['assistant'], priority: 0.8 }, hard-coded insep-2640.per-skill.ts, and nothingturned it off. A widely used Rust MCP client (its JSON parser keeps arbitrary-precision numbers) fails the whole
resources/listwith "Unexpected response type" when an annotation is a number, so users deletedpriorityin a hook.skillsConfig.resourceAnnotations: 'no-priority' | 'full' | 'off', default'no-priority'.'no-priority'keepsaudience: ['assistant']andlastModified(file-backed skills) and sends no number.'full'brings backpriority: 0.8.'off'sends noannotationsfield, and skips resolvinglastModified.skill://index.jsoncarry none.The full skills catalog in the
initializeinstructions.skillsConfig.injectInstructionsdefaulted to'append',which adds
- **name**: descriptionfor every skill the caller may see, capped only at 16,000 characters. Some clientscopy the server's instructions into every tool's description: one run held 104 copies, about 970K tokens of tool
documentation.
injectInstructions: 'compact', now the default: the server instructions and channel hints, then one line withthe number of skills the caller may see and where to read them:
This server has 3 skills: the `skill://index.json` resource lists each skill's `SKILL.md` URI.mcpResources: false:This server has 3 skills: find one with the `skills/search` request and load it with `skills/load`.skills:filterflow drops them all).skillsConfig.externalProviderthe line names no number (This server has skills: …): theprovider's skills are listed by
skill://index.jsonbut not held by the registry, so a count would be wrong. Theline is sent even when the server declares no skill itself.
skills:filterflow), so a flag-disabled skill is not counted. It costs nothing extra: that filter already ran for the'append'default.'append','prepend'and'replace'keep the full catalog, and'off'is unchanged (it still sends thesep2640InInstructionshints).composeCallerInstructionsstays the single composition point, so stdio, StreamableHTTP/SSE, in-memory, web-fetch and the 2026-07-28
server/discoverall send the same line.?mode=skills_onlyclient gets the line too: it lists no tools, so nothing copies the line into tool descriptions,and skills are all such a client can use.
Validation. Both options are Zod enums in
skillsConfigOptionsSchema. An unknown value stops@FrontMcpat boot withInvalidDecoratorMetadataErrorforskillsConfig(it was a bareZodErrorbefore, for any invalidskillsConfigfield), and the message names the field and the accepted values.
User-facing change
skillsConfig.resourceAnnotations(new):'no-priority'(default) |'full'|'off'.skillsConfig.injectInstructionsgains'compact', which is now the default. The exportedcomposeInitializeInstructions()andcomposeCallerInstructions()use it too when no policy is set.skillsConfigmakes@FrontMcpthrowInvalidDecoratorMetadataError(@FrontMcp invalid metadata for "skillsConfig": …) instead of aZodError.SKILL.mdresource annotations. Before:{ audience: ['assistant'], priority: 0.8, lastModified? }. After:{ audience: ['assistant'], lastModified? }. To get the old behaviour back:skillsConfig: { resourceAnnotations: 'full' }. A hook that deletedpriorityfromresources/listis no longer needed.initializeinstructions. Before: the full catalog (Available skills (…):and a- **name**: descriptionlineper skill, up to 16 KB). After: one line,
This server has N skills: the `skill://index.json` resource lists each skill's `SKILL.md` URI.(This server has skills: …with a read-onlyexternalProvider). To get the oldbehaviour back:
skillsConfig: { injectInstructions: 'append' }.injectInstructionsexplicitly, including the deploy manifest's(
manifest-to-configsets'append'), see no change.sep2640InInstructions: truestill adds its URI hints.skillsConfigreference (instructions injection, and a new section forSKILL.mdresource annotations), the@FrontMcpinstructions/skillsConfigoption rows, the SEP-2640 skillsresources page, and the production checklist line about the 16 KB catalog.
frontmcp-config/references/configure-skills-http.md: thecompactmode (and its wording with a read-onlyexternalProvider), both default changes with the way back, a new "SKILL.md Resource Annotations" section, theboot error, the decision matrix, and the
offrow (it keeps thesep2640InInstructionshints);frontmcp-config/examples/configure-skills-http/inject-instructions.mdand itsskills-manifest.jsonentry;frontmcp-development/references/decorators-guide.md(instructions?andskillsConfig?rows);frontmcp-production-readiness/references/common-checklist.md(the 16 KB line now applies to the catalog modes);frontmcp-deployment/references/protocol-versions.md(server/discoversends the skills line or catalog).Tests
Red first: the unit specs failed before the change (9 failures, plus the boot-validation cases), and the new e2e
defaults failed against an SDK build of the parent branch (
feature-flags-instructions: the compact line;skills-resources: the compact line and no numeric annotation;skills-only-mode: the compact line), while the'append'and'full'restore paths passed there.libs/sdk):sep-2640.per-skill.spec.ts: the default sendsaudiencewith nopriority;'full'addspriority: 0.8;'off'sends no
annotations;lastModifiedstays under'no-priority'and'full'; registration addslastModifiednext to the audience, brings back
priorityunder'full', and under'off'neither annotates nor resolveslastModified.skill-instructions.helper.spec.ts:'compact'is the default; the exact line; singular for one skill; theskills/*variant withmcpResources: false; it follows the server instructions and channel hints; no line with noMCP-visible skill or no registry; per caller, it counts only what
skills:filterkeeps, and adds no line when thecaller sees none; no count beside a read-only external provider, and a line on its own; the count kept beside a
persistent one. Every policy still never ends in a separator.
skill-external-provider.spec.ts: on a real scope with a read-only provider, the default instructions name nocount, with a registered skill and on a server that declares none.
transport.local.skill-hints.spec.ts:'off'keeps thesep2640InInstructionshints.skills-http.options.spec.ts: both defaults; every accepted value; an unknowninjectInstructionsorresourceAnnotationsmakes@FrontMcpthrowInvalidDecoratorMetadataErrornaming the field.'append'explicitly:skill-filter.flow.spec.ts,skill-surface.spec.ts,skill-context.spec.ts,transport.local.skill-hints.spec.ts, andplugins/plugin-feature-flags/.../feature-flag.flows.spec.ts.demo-e2e-skills/e2e/skills-resources.e2e.spec.ts: everySKILL.mdresource inresources/listhasaudience: ['assistant']and no numeric annotation by default; withSKILLS_RESOURCE_ANNOTATIONS=full(
resourceAnnotations: 'full') each haspriority: 0.8; the default instructions are exactly the compact line,with the count matching
skill://index.json.demo-e2e-skills/e2e/skills-only-mode.e2e.spec.ts: a?mode=skills_onlyclient gets the compact line.demo-e2e-feature-flags/e2e/feature-flags-instructions.e2e.spec.ts: by default the line counts 1 skill (theflag-disabled one is not counted); with
SKILLS_INJECT_INSTRUCTIONS=appendthe full catalog lists the enabled skilland leaves the disabled one out (Feature flags: filter the skill catalog in initialize instructions per caller #603).
demo-e2e-dts-consumer/fixture/skills-must-compile.ts:injectInstructions: 'compact'andresourceAnnotations: 'full'compile on@FrontMcp;resourceAnnotations: 'audience'does not.npx nx run-many -t test build -p sdk skills: sdk 572 suites / 7401 tests, skills 3 suites / 249 tests, builds green.npx nx run-many -t test build -p plugin-feature-flags plugin-skilled-openapi adapters: 239, 433 and 1011 tests,builds green.
npx nx test demo-e2e-skills(192 passed, 2 skipped as before),npx nx test demo-e2e-feature-flags(31 passed),npx nx test demo-e2e-dts-consumer(10 passed).npx nx run-many -t linton the touched projects: 0 errors.tsc --noEmitonlibs/sdk/tsconfig.lib.json,libs/sdk/tsconfig.spec.jsonand both e2e apps: no diagnostics.node scripts/fix-unused-imports.mjs origin/fix/03-hook-order, andprettier --checkon every changed file.Summary by CodeRabbit