Add azure-hosted-copilot-sdk skill with test infrastructure and local dev tooling#880
Add azure-hosted-copilot-sdk skill with test infrastructure and local dev tooling#880jongio wants to merge 42 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new github-copilot skill (Copilot SDK + Copilot Extensions, Azure deployment pathing) and expands the test and local-dev infrastructure to support multi-turn deployment scenarios, plus documentation improvements for validate/deploy workflows.
Changes:
- Introduces
plugin/skills/github-copilotwith references for Copilot SDK agent and Copilot Extensions. - Adds new integration tests for multi-turn GHCP SDK → Azure deployment scenarios, including regression detectors.
- Enhances local dev scripts to register MCP servers from
plugin/.mcp.jsonand adds a live “local test” command; updates azure-validate/azure-deploy docs/checklists.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/regression-detectors.ts | Adds event-scanning detectors for common failure/regression patterns in deployment conversations. |
| tests/utils/ghcp-sdk-workspace.ts | Adds helper to scaffold an azd Copilot SDK agent template into a test workspace. |
| tests/utils/agent-runner.ts | Adds runConversation() multi-turn runner and minor filename sanitization fix. |
| tests/github-copilot/unit.test.ts | Adds unit tests validating the new skill’s metadata/content/frontmatter. |
| tests/github-copilot/triggers.test.ts | Adds trigger/anti-trigger tests and snapshots for the new skill. |
| tests/github-copilot/integration.test.ts | Adds integration tests intended to validate real session behavior for the new skill. |
| tests/github-copilot/fixtures/sample.json | Adds sample prompts/expectations and regression limits for scenarios. |
| tests/github-copilot/snapshots/triggers.test.ts.snap | Adds snapshots for extracted trigger keywords/description. |
| tests/github-copilot/README.md | Adds a README (currently template-style) under the new skill test folder. |
| tests/ghcp-sdk-foundry-agent/integration.test.ts | Adds multi-turn end-to-end integration test for Foundry-hosted agent scenario. |
| tests/ghcp-sdk-deploy/integration.test.ts | Adds multi-turn end-to-end integration test for GHCP SDK Azure deployment scenario. |
| tests/_template/unit.test.ts | Updates the test template with frontmatter-format validation checks. |
| tests/README.md | Updates the skill coverage grid with additional skill rows. |
| scripts/src/local/commands/verify.ts | Adds MCP server registration checks and validates skill SKILL.md frontmatter existence. |
| scripts/src/local/commands/test.ts | Adds a live smoke-test command that runs Copilot CLI prompts to validate plugin + MCP connectivity. |
| scripts/src/local/commands/setup.ts | Extends setup to merge plugin/.mcp.json into ~/.copilot/mcp-config.json. |
| scripts/src/local/cli.ts | Adds npm run local test command wiring. |
| plugin/skills/github-copilot/references/copilot-sdk-agent.md | Adds Copilot SDK agent reference links and workflow guidance. |
| plugin/skills/github-copilot/references/copilot-extensions.md | Adds Copilot Extensions reference links and operational guidance (SSE/signature verification). |
| plugin/skills/github-copilot/SKILL.md | Adds the new github-copilot skill with path detection + deployment orchestration rules. |
| plugin/skills/azure-validate/references/recipes/azd/README.md | Inserts explicit “Build Verification” step before packaging validation. |
| plugin/skills/azure-validate/SKILL.md | Updates validate steps to include build verification before proceeding. |
| plugin/skills/azure-deploy/references/recipes/azd/errors.md | Adds troubleshooting guidance for duplicate azd-service-name tagged resources. |
| plugin/skills/azure-deploy/references/recipes/azd/README.md | Adds guidance to stream azd up output visibly to the user. |
| plugin/skills/azure-deploy/references/pre-deploy-checklist.md | Updates duplicate-tag resolution guidance (prefer new env, else delete with confirmation). |
| plugin/skills/azure-deploy/SKILL.md | Adds guidance to stream deployment output visibly. |
| plugin/.mcp.json | Adds additional MCP servers (context7, playwright, microsoft-learn). |
| .github/skills/sensei/references/SCORING.md | Updates frontmatter format guidance, especially around USE FOR/DO NOT USE FOR parsing. |
| .github/skills/sensei/references/LOOP.md | Adds detection/fix guidance for unsafe frontmatter formatting cases. |
| .github/skills/sensei/references/EXAMPLES.md | Updates examples to prefer quoted descriptions for USE FOR/DO NOT USE FOR content. |
| .github/skills/sensei/SKILL.md | Updates the Sensei loop/template guidance accordingly. |
|
Please address copilot comments and checks are failing. Had one comment on the yaml description. Please add screenshot of integration test result |
tmeschter
left a comment
There was a problem hiding this comment.
Approved with some comments.
|
Please fix the type errors reported by the PR Checks. |
Feedback on Sensei Pipe Block Scalar ChangesThe sensei changes in this PR convert all This conflicts with existing guidance and specs
What the repo looks like todayWe currently have a mix of formats across skills:
The pipe-format skills are currently working, which means either the parser bug is intermittent or has been fixed. RecommendationThe fix here is a workaround, not the right solution. A 600+ character description on a single quoted line is hard to read, review, and maintain -- which is exactly why we recommended the pipe format in our guidance doc. The right approach:
Happy to discuss -- just want to make sure we are not encoding a parser bug as permanent guidance. cc @jongio |
|
Update: Filed the parser bug as github/copilot-cli#1427 — the YAML frontmatter parser misinterprets pipe block scalar continuation lines containing |
- Fix integration test imports (use useAgentRunner pattern) - Fix event handler leak in runConversation (register once outside loop) - Remove unused countToolCalls function - Remove redundant microsoft-learn MCP server from .mcp.json - Fix MCP server type default from 'local' to 'stdio' - Fix require→ESM imports in template unit test - Remove tests for non-existent skills (ghcp-sdk-deploy, ghcp-sdk-foundry-agent) - Fix negative trigger test prompts to avoid keyword overlap - Replace generic README template with skill-specific docs - Fix unit test assertion to match trimmed SKILL.md content
8acd3eb to
01d7c39
Compare
- Fix integration test imports (use useAgentRunner pattern) - Fix event handler leak in runConversation (register once outside loop) - Remove unused countToolCalls function - Remove redundant microsoft-learn MCP server from .mcp.json - Fix MCP server type default from 'local' to 'stdio' - Fix require→ESM imports in template unit test - Remove tests for non-existent skills (ghcp-sdk-deploy, ghcp-sdk-foundry-agent) - Fix negative trigger test prompts to avoid keyword overlap - Replace generic README template with skill-specific docs - Fix unit test assertion to match trimmed SKILL.md content
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
tests/README.md:438
- The Skills Coverage Grid in tests/README.md is missing an entry for the new 'github-copilot' skill. Since this PR adds comprehensive test coverage for this skill (unit.test.ts, triggers.test.ts, integration.test.ts), it should be added to the grid with appropriate checkmarks. Add a row like:
| github-copilot | ✅ | ✅ | ✅ | ✅ | - |
| Skill | Tests | Unit | Triggers | Integration | Coverage |
|-------|-------|------|----------|-------------|----------|
| appinsights-instrumentation | ✅ | ✅ | ✅ | ✅ | - |
| azure-ai | ✅ | ✅ | ✅ | ✅ | - |
| azure-aigateway | ✅ | ✅ | ✅ | ✅ | - |
| azure-cost-optimization | ✅ | ✅ | ✅ | ✅ | - |
| azure-create-app | ❌ | - | - | - | - |
| azure-deploy | ✅ | ✅ | ✅ | ✅ | - |
| azure-deployment-preflight | ❌ | - | - | - | - |
| azure-diagnostics | ✅ | - | - | ✅ | - |
| azure-functions | ✅ | - | - | ✅ | - |
| azure-keyvault-expiration-audit | ✅ | ✅ | ✅ | ✅ | - |
| azure-kusto | ✅ | - | - | ✅ | - |
| azure-nodejs-production | ✅ | - | - | ✅ | - |
| azure-observability | ✅ | - | - | ✅ | - |
| azure-postgres | ✅ | - | - | ✅ | - |
| azure-prepare | ✅ | - | - | ✅ | - |
| azure-quick-review | ✅ | ✅ | ✅ | ✅ | - |
| azure-resource-visualizer | ✅ | - | - | ✅ | - |
| azure-role-selector | ✅ | ✅ | ✅ | ✅ | - |
| azure-security | ✅ | - | - | ✅ | - |
| azure-security-hardening | ✅ | - | - | ✅ | - |
| azure-storage | ✅ | - | - | ✅ | - |
| azure-validate | ✅ | ✅ | ✅ | ✅ | - |
| entra-app-registration | ✅ | ✅ | ✅ | ✅ | - |
| microsoft-foundry | ✅ | ✅ | ✅ | ✅ | - |
plugin/skills/github-copilot/references/existing-project-integration.md
Outdated
Show resolved
Hide resolved
- Revert jest.setup.ts, tsconfig.json, skill-loader.ts, jest.config to upstream ESM versions - Delete broken tests/github-copilot/ (referenced nonexistent skill) - Create tests/azure-hosted-github-sdk/ with correct unit and trigger tests - Remove unused ghcp-sdk-workspace.ts - All 601 tests pass (29 suites, 100% pass rate) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename skill to match the product name (Copilot SDK) rather than the vendor (GitHub). Updates skill directory, test directory, spec doc, frontmatter name field, cross-skill references, and all test constants. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Condense Step 2B and 2C bodies into single-line references (detail already in reference files). Trim 4 redundant triggers from description. 835 → ~498 tokens (limit: 500). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Updated skill description to clarify usage scenarios and avoid confusion. - Simplified routing table in Step 1 for better clarity. - Enhanced model configuration section with clearer paths and examples. - Improved deployment instructions for existing SDK apps. - Revised integration steps for existing projects to streamline BYOM support. - Adjusted test prompts and expectations to align with updated SDK functionalities. - Updated references in unit tests to reflect changes in environment variables and SDK templates.
…en-level streaming
- Add PR microsoft#880 metadata (branch, state, commit/file count) - Add local template support section (azure-dev CLI work) - Update PR conflict check: microsoft#938 merged - Update test status labels: Updated → Complete - Add P2 future work: plugin-side template source config - Add known issues: stale PR title, merged PR microsoft#938 - Update invocation rate: explicitly 0% - Update file inventory with azure-dev CLI files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tion test imports
- Re-add setupMcpServers() to setup.ts with MCP config helpers - Re-add checkMcpServers() and checkSkills() to verify.ts - Fix cli.ts: smoke.js -> test.js import, add smoke as alias for test - Both setup and verify now handle MCP server lifecycle Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove hardcoded C:\code\azure-dev path from spec doc - Remove local azure-dev CLI file listing from spec doc - Fix azure-validate recipe step numbering (was 8,9,8 → now 7,8,9) - Regenerate triggers.test.ts.snap to match current SKILL.md description Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Convert single quotes to double quotes in scripts/src/local/ per ESLint rule. Keep single quotes only where inner double quotes require avoidEscape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4561556 to
85dd054
Compare
|
|
||
| const nameMatch = frontmatter.match(/^name:\s*(.+)/m); | ||
| if (nameMatch && nameMatch[1].trim() !== skill) { | ||
| invalid.push({ name: skill, error: `name "${nameMatch[1].trim()}" doesn"t match directory` }); |
There was a problem hiding this comment.
There's a typo in the error message. "doesn"t" should be "doesn't" (with proper apostrophe). This will appear in validation error output when a skill name doesn't match its directory name.
| invalid.push({ name: skill, error: `name "${nameMatch[1].trim()}" doesn"t match directory` }); | |
| invalid.push({ name: skill, error: `name "${nameMatch[1].trim()}" doesn't match directory` }); |
| "context7": { | ||
| "command": "npx", | ||
| "args": ["-y", "@upstash/context7-mcp@latest"] | ||
| }, | ||
| "playwright": { |
There was a problem hiding this comment.
The "context7" and "playwright" MCP server entries are missing the required "type" field. Looking at the existing entries, "azure" has command-based configuration (stdio type), while "foundry-mcp" explicitly sets "type": "http". For consistency and proper configuration, stdio-based servers should include "type": "stdio".
Add the type field:
"context7": {
"type": "stdio",
"command": "npx",
"args": ["-y", "@upstash/context7-mcp@latest"]
},
"playwright": {
"type": "stdio",
"command": "npx",
"args": ["-y", "@playwright/mcp@latest"]
}| "context7": { | |
| "command": "npx", | |
| "args": ["-y", "@upstash/context7-mcp@latest"] | |
| }, | |
| "playwright": { | |
| "context7": { | |
| "type": "stdio", | |
| "command": "npx", | |
| "args": ["-y", "@upstash/context7-mcp@latest"] | |
| }, | |
| "playwright": { | |
| "type": "stdio", |
Instead of reading template via GitHub MCP and reconstructing infra, scaffold to a temp dir with azd init --template, then copy infra/ scripts/azure.yaml into the user's project. Avoids overwriting existing files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Added mandatory Step 0 prompt-based technology check before Phase 1 - Created references/specialized-routing.md with full routing table - Fixes routing issue where copilot SDK prompts never delegated to azure-hosted-copilot-sdk - Fixed azure-hosted-copilot-sdk unit test: deploy ref -> deploy existing ref - Updated snapshots for both skills Score: azure-prepare Medium-High (unchanged), azure-hosted-copilot-sdk Medium-High (unchanged) Tokens: azure-prepare 1864/500, azure-hosted-copilot-sdk 656/500 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Step 0 section in SKILL.md was being skipped because the agent jumps straight to the Phase 1 table and loads analyze.md (Step 1). The delegation table was at the bottom of analyze.md - too late. Moved the specialized technology delegation check to the very top of analyze.md with MANDATORY/STOP markers so the agent sees it before choosing a mode. Removed the duplicate section from the bottom. Verified by live test: all 3 runs loaded analyze.md first but never reached the delegation table at line 70+. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace 4 prompt-only invocation tests with 5 scenario-based tests: 1. Greenfield + explicit mention (no existing code) 2. Existing Express app + add copilot SDK (Express workspace) 3. Existing copilot SDK app + deploy (SDK in package.json, no SDK keyword) 4. Existing copilot SDK app + modify (SDK in package.json, no SDK keyword) 5. Greenfield + vague mention (copilot-powered) Scenarios 2-4 use setup() callback to populate workspace with Express+TypeScript mock files. Scenario 3 tests scan.md file-based detection when the prompt doesn't mention copilot SDK. Added content-quality test for existing copilot SDK app deployment. Extracted measureInvocationRate helper to reduce test boilerplate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes 6 issues discovered during live Repo Rater session:
1. wireApi: 'responses' → 'completions' (responses + store:false breaks tool calls)
2. apiVersion must be nested under azure: {}, not top-level (caused CAPIError 404)
3. Added AZURE_CLIENT_ID to required env vars (needed for user-assigned MI)
4. Added 3 new errors to table: CAPIError 404, 400 tool calls, 500 missing client ID
5. Added nginx proxy_read_timeout 300s guidance (default 60s causes 504)
6. Added gpt-4o BYOM warning to deploy-existing.md (template default is unsupported)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| test("BYOM routing row in step 1 table", () => { | ||
| expect(skill.content).toContain("Use Azure/own model"); | ||
| expect(skill.content).toContain("Step 2C"); | ||
| }); |
There was a problem hiding this comment.
The test expects the routing table to say "Use Azure/own model → Step 2C" but the actual SKILL.md correctly routes it to "Step 3 (BYOM config)". The test assertion on line 95 should expect "Step 3" instead of "Step 2C" to match the actual routing logic in the skill.
| "version": "4.0.18", | ||
| "dev": true, | ||
| "license": "MIT", | ||
| "peer": true, |
There was a problem hiding this comment.
The package-lock.json file has added "peer": true flags to several dependencies (@types/node, @typescript-eslint/ast-spec, acorn, eslint-plugin-deprecation, fast-glob, typescript, typescript-eslint, vitest). The "peer" field in package-lock.json is used to indicate peer dependency metadata. These additions appear correct for properly tracking peer dependencies, though it's worth verifying that these are intentional changes and not accidental modifications from a dependency resolution change.
| "peer": true, |
CR-001 [Both]: Add copilot-powered and copilot-sdk-service to analyze.md delegation table — was missing from the primary enforcement point, causing Scenario 5 (vague copilot-powered prompt) to bypass routing. CR-002 [Codex]: Align env var name to MODEL_NAME (not AZURE_DEPLOYMENT_NAME) in azure-model-config.md code sample and env vars table to match what the template actually reads. CR-003 [Codex]: Fix mock SDK version from ^1.0.0 to ^0.1.22 in integration test workspace setup to match the real package version. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the
azure-hosted-copilot-sdkskill that guides users through building apps with the GitHub Copilot SDK (@github/copilot-sdk) and deploying them to Azure. Uses a single template (azure-samples/copilot-sdk-service) with three model paths: GitHub default, GitHub specific, and Azure BYOM (Bring Your Own Model).Feature spec:
docs/spec/azure-hosted-copilot-sdk.mdWhat's Changed
New Skill:
azure-hosted-copilot-sdkDefaultAzureCredentialTests:
azure-hosted-copilot-sdktriggers.test.ts— 23 tests (10 trigger, 8 negative, 5 edge cases)unit.test.ts— 17 tests (metadata, content validation, BYOM patterns, frontmatter)integration.test.ts— 4 invocation rate + 2 content quality testsTest Infrastructure (
tests/utils/)agent-runner.ts— Enhanced withrunConversation()for multi-turn test scenarios, per-turn and aggregate metadata trackingregression-detectors.ts— Reusable detector functions for common deployment anti-patterns: secret leaks, ACR auth spirals, port-binding confusion, hosting thrashingevaluate.ts— Shared evaluation helpers extracted from test filesgit-clone.ts— Git clone utility for test fixturesTest Templates (
tests/_template/)integration.test.tstemplate with improved patternsunit.test.tstemplate for consistent test structure across skillsLocal Dev Tooling (
scripts/src/local/)testcommand — Live validation of the local plugin against real agent sessionssetupcommand — Enhanced with MCP server registration (mcp-config.json)verifycommand — Expanded health checks for plugin and MCP serversReference Validation (
scripts/src/references/)cli.ts— Validates markdown references, detects broken links, orphaned refs, and out-of-place filesreferences.test.ts— Tests for reference validation logicOther Changes
plugin/.mcp.json— Added MCP server entries for Microsoft Learn.gitignore— Updated with test output patternstest-all-integration.yml,test-all-skills.yml, updatedpr.ymlTest Plan
npm test -- tests/azure-hosted-copilot-sdk/npm test -- tests/npm run local test