Conversation
There was a problem hiding this comment.
Pull request overview
Adds per-custom-agent skill scoping to the SDK surface area (types + docs) and introduces new cross-SDK E2E coverage + replay snapshots for the opt-in behavior.
Changes:
- Added
skillstoCustomAgentConfigacross Node.js, Python, Go, and .NET SDK types. - Updated docs to describe per-agent skill scoping and added
agentNametoskill.invokedevent docs. - Added E2E tests and new replay snapshots for “agent with skills” vs “agent without skills field”.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
nodejs/src/types.ts |
Adds skills?: string[] to CustomAgentConfig with JSdoc explaining opt-in semantics. |
python/copilot/session.py |
Adds skills to the Python CustomAgentConfig TypedDict. |
go/types.go |
Adds Skills []string to Go CustomAgentConfig for JSON serialization. |
dotnet/src/Types.cs |
Adds List<string>? Skills to .NET CustomAgentConfig with XML docs. |
docs/features/custom-agents.md |
Documents the new skills field and adds a dedicated per-agent skills section. |
docs/features/skills.md |
Updates “Skills + Custom Agents” section with per-agent skills example and opt-in note. |
docs/features/streaming-events.md |
Documents agentName on skill.invoked. |
nodejs/test/e2e/skills.test.ts |
Adds Node E2E tests for agent-scoped skills behavior. |
python/e2e/test_skills.py |
Adds Python E2E tests for agent-scoped skills behavior. |
go/internal/e2e/skills_test.go |
Adds Go E2E tests for agent-scoped skills behavior. |
dotnet/test/SkillsTests.cs |
Adds .NET E2E tests for agent-scoped skills behavior. |
test/snapshots/skills/should_allow_agent_with_skills_to_invoke_skill.yaml |
Replay snapshot for the “agent with skills can invoke” scenario. |
test/snapshots/skills/should_not_provide_skills_to_agent_without_skills_field.yaml |
Replay snapshot for the “no skills field => no skills” scenario. |
nodejs/package-lock.json |
Lockfile metadata updates. |
Files not reviewed (1)
- nodejs/package-lock.json: Language not supported
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #995
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Add skills field to Python wire format converter - Explicitly select agents in all E2E tests for deterministic behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
- Add skills field to Python wire format converter - Explicitly select agents in all E2E tests for deterministic behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MackinnonBuck
force-pushed
the
mackinnonbuck/agent-skills-958
branch
from
b6eb0fa to
c1272a1
Compare
This comment has been minimized.
This comment has been minimized.
Add skills: Option<Vec<String>> to CustomAgentConfig, matching the new per-agent skills support in CLI 1.0.23 (SweCustomAgent.skills). Skills are referenced by name from the session's skillDirectories pool and eagerly preloaded into the agent's context at startup. Adds smoke_subagent_with_skills test that creates a skill directory with a SKILL.md containing a known passphrase, configures a subagent with skills: ["trivia-skill"], and verifies the model can access the skill content. Also fixes skill directory structure in smoke_agent_with_skill to use the proper subdirectory/SKILL.md convention. Upstream SDK PR: github/copilot-sdk#995 (draft, not yet merged) Runtime support: CLI 1.0.23 native binary Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a 'skills' field to CustomAgentConfig across all four SDK languages (Node.js, Python, Go, .NET) to support scoping skills to individual subagents. Skills are opt-in: agents get no skills by default. Changes: - Add skills?: string[] to CustomAgentConfig in all SDKs - Update custom-agents.md with skills in config table and new section - Update skills.md with per-agent skills example and opt-in note - Update streaming-events.md with agentName on skill.invoked event - Add E2E tests for agent-scoped skills in all four SDKs - Add snapshot YAML files for new test scenarios Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update type comments, docs, and test descriptions to reflect that per-agent skills are eagerly injected into the agent's context at startup rather than filtered for invocation. Sub-agents do not inherit skills from the parent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The runtime does not emit agentName on the skill.invoked event. The agent name is used only for internal logging during eager skill loading, not as event data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add skills field to Python wire format converter - Explicitly select agents in all E2E tests for deterministic behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The generated_session_events.go on main changed from a flat Data struct to a SessionEventData interface with per-event typed structs. The agent skills test cases added in this PR were using the old message.Data.Content pattern instead of the type assertion pattern used elsewhere. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MackinnonBuck
force-pushed
the
mackinnonbuck/agent-skills-958
branch
from
c1272a1 to
6f3083b
Compare
MackinnonBuck
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #995 · ● 510.2K
| /// When omitted, no skills are injected (opt-in model). | ||
| /// </summary> | ||
| [JsonPropertyName("skills")] | ||
| public List<string>? Skills { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency / .NET convention: This property uses the concrete List<string>? type, but the established convention throughout Types.cs is to use the interface type IList<string>? for public collection properties (see Tools at line 1622, SkillDirectories at line 1865, AvailableTools at line 1792, etc.).
Suggestion:
public IList<string>? Skills { get; set; }This keeps the public API surface consistent with every other nullable list property in the file.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #995 · ● 462.3K
| /// When omitted, no skills are injected (opt-in model). | ||
| /// </summary> | ||
| [JsonPropertyName("skills")] | ||
| public List<string>? Skills { get; set; } |
There was a problem hiding this comment.
Minor: Use IList<string>? instead of List<string>? to match .NET SDK conventions.
Every other nullable string[]-equivalent property in this file uses the IList<string>? interface rather than the concrete List<string> type. For example, the adjacent properties DisabledSkills, SkillDirectories, Tools, etc. all use IList<string>?:
public IList<string>? SkillDirectories { get; set; } // line ~1865
public IList<string>? DisabledSkills { get; set; } // line ~1870
public IList<string>? Tools { get; set; } // line ~1622Suggestion:
public IList<string>? Skills { get; set; }This keeps the public API surface consistent with the rest of the SDK's collection-typed properties.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #995 · ● 468.7K
| /// When omitted, no skills are injected (opt-in model). | ||
| /// </summary> | ||
| [JsonPropertyName("skills")] | ||
| public List<string>? Skills { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency / .NET type convention: This property uses the concrete List<string>? type, but all comparable nullable string-list properties in this file use the interface type IList<string>? (e.g. DisabledSkills, AvailableTools, ExcludedTools, Tools, SkillDirectories).
Suggestion: change to match the established pattern:
public IList<string>? Skills { get; set; }
test/harness/replayingCapiProxy.ts
Outdated
| .replace(/<current_datetime>.*?<\/current_datetime>/g, "") | ||
| .replace(/<reminder>[\s\S]*?<\/reminder>/g, "") | ||
| .replace(/<agent_instructions>[\s\S]*?<\/agent_instructions>/g, "") | ||
| .replace(/<agent_instructions>[\s\S]*?<\/agent_instructions>/g, "") |
There was a problem hiding this comment.
Duplicate replace call: This agent_instructions regex replacement is identical to the one on the line above (line 808). The second call is a no-op but looks like a copy-paste error — worth cleaning up.
The runtime now eagerly injects skill content into <agent_instructions> in the user message instead of using a skill tool call. Update the replay proxy to strip <agent_instructions> during normalization, and simplify the snapshot for agent-with-skills to match the new flow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MackinnonBuck
force-pushed
the
mackinnonbuck/agent-skills-958
branch
from
305dd4e to
276df2f
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #995 · ● 543.4K
| /// When omitted, no skills are injected (opt-in model). | ||
| /// </summary> | ||
| [JsonPropertyName("skills")] | ||
| public List<string>? Skills { get; set; } |
There was a problem hiding this comment.
The property type should be IList<string>? rather than List<string>? to be consistent with the rest of the public API surface in this file. Every other string[]-equivalent property in Types.cs (e.g. Tools, CleanupActions, SkillDirectories, DisabledSkills, ...) uses the interface type IList<string>?.
public IList<string>? Skills { get; set; }Add two regression tests validating that <agent_instructions> blocks are properly stripped during user message normalization, including the case where skill-context is nested inside agent_instructions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅This PR does an excellent job maintaining feature parity — the One minor issue found in the .NET implementation (see inline comment):
Everything else looks consistent across languages. 🎉
|
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #995 · ● 599.7K
| /// When omitted, no skills are injected (opt-in model). | ||
| /// </summary> | ||
| [JsonPropertyName("skills")] | ||
| public List<string>? Skills { get; set; } |
There was a problem hiding this comment.
The Skills property uses List<string>? while every other string-collection property in this file (including Tools right above it in the same class) uses the IList<string>? interface type. This is inconsistent with the established .NET API convention in this codebase.
// Suggested change
public IList<string>? Skills { get; set; }For reference, the Tools property in this same class is declared as:
public IList<string>? Tools { get; set; }And similarly, SkillDirectories, DisabledSkills, AvailableTools, etc. throughout the file all use IList<string>?.
Summary
Adds SDK support for scoping skills to individual subagents, addressing #958.
Changes
Types (all 4 SDKs):
nodejs/src/types.ts): Addedskills?: string[]toCustomAgentConfigpython/copilot/session.py): Addedskillsfield to agent configgo/types.go): AddedSkills []stringto agent config structdotnet/src/Types.cs): AddedList<string>? Skillsto agent config classDocumentation:
docs/features/custom-agents.md: Addedskillsto config reference table and new section on per-agent skillsdocs/features/skills.md: Updated "Skills + Custom Agents" section with per-agent exampledocs/features/streaming-events.md: AddedagentNamefield toskill.invokedevent docsTests (all 4 SDKs):
skillscan invoke listed skillsskillsfield gets no skills (backward compatible)Design decisions
skillsis optional. Omitting it means the agent has no access to skills (not all skills).skillDirectoriespool.Closes #958