diff --git a/.agents/skills/ci-analysis/references/manual-investigation.md b/.agents/skills/ci-analysis/references/manual-investigation.md index b3b319eaacb..4b7d7ae4cbc 100644 --- a/.agents/skills/ci-analysis/references/manual-investigation.md +++ b/.agents/skills/ci-analysis/references/manual-investigation.md @@ -3,6 +3,7 @@ If the script doesn't provide enough information, use these manual investigation steps. ## Table of Contents + - [Get Build Timeline](#get-build-timeline) - [Find Helix Tasks](#find-helix-tasks) - [Get Build Logs](#get-build-logs) @@ -62,6 +63,7 @@ $workItem.Files | ForEach-Object { Write-Host "$($_.FileName): $($_.Uri)" } ``` Common artifacts: + - `console.*.log` - Console output - `*.binlog` - MSBuild binary logs - `run-*.log` - XHarness/test runner logs @@ -70,6 +72,7 @@ Common artifacts: ## Analyze Binlogs Binlogs contain detailed MSBuild execution traces for diagnosing: + - AOT compilation failures - Static web asset issues - NuGet restore problems @@ -89,6 +92,7 @@ curl -s "https://helix.dot.net/api/2019-06-17/jobs/JOB_ID/workitems/WORK_ITEM_NA ``` Example output: + ``` DOTNET_JitStress=1 DOTNET_TieredCompilation=0 diff --git a/.agents/skills/make-skill/SKILL.md b/.agents/skills/make-skill/SKILL.md index 88e88d1ec06..ae9528d73c1 100644 --- a/.agents/skills/make-skill/SKILL.md +++ b/.agents/skills/make-skill/SKILL.md @@ -82,7 +82,17 @@ Include these recommended sections, following this file's structure: └── assets/ # Optional: templates, resources and other data files that aren't executable or Markdown ``` -### Step 6: Validate the skill +### Step 6: Write Scripts (Script-driven Only) + +- Prefer PowerShell, but can also use Python or JavaScript +- Standard param block with defaults +- Ensure scripts produce clear, structured, and parseable console output (for example, section headers and status lines) +- Emoji status: ✅ green / ⚠️ yellow / 🔴 red +- **Fail-closed error handling** — Unknown ≠ Healthy + +> ❌ **NEVER** count API failures as success. Return "Unknown" and exclude from positive counts. + +### Step 7: Validate the skill Ensure the name: - Does not start or end with a hyphen @@ -103,6 +113,20 @@ After creating a skill, verify: - [ ] Optional directories are used appropriately - [ ] Scripts handle edge cases gracefully and return structured outputs and helpful error messages when applicable +### Step 8: Test with Multi-Model Subagents + +Follow [references/testing-patterns.md](references/testing-patterns.md): + +1. Select top-tier model from 2-4 different families +2. Give each the same test prompt exercising the skill +3. Launch in parallel via `task` tool with `model` parameter +4. Synthesize: consensus findings = high confidence +5. Fix errors first, then warnings, then consider suggestions +6. **Retrospective**: When an agent misapplies guidance, ask the *same model* why it made that choice — its self-analysis reveals guidance gaps you can close with targeted anti-patterns (see references/anti-patterns.md) +7. **A/B test**: After fixing issues, re-run the same task to verify improvement — same model, same prompt, compare correctness/speed/tool calls (see references/testing-patterns.md) + +**For new skills or major restructuring**, use the writer-critic convergence loop instead: one agent writes, a different-model agent critiques, writer applies fixes, repeat until convergence (2-3 rounds). See references/testing-patterns.md#writer-critic-convergence-loop. + ## Common Pitfalls | Pitfall | Solution | diff --git a/.agents/skills/make-skill/references/anti-patterns.md b/.agents/skills/make-skill/references/anti-patterns.md new file mode 100644 index 00000000000..03fdf5a6c96 --- /dev/null +++ b/.agents/skills/make-skill/references/anti-patterns.md @@ -0,0 +1,351 @@ +# Anti-Patterns + +Empirical pitfalls discovered during skill development. Every item here was hit in practice — not theoretical. + +## Skill Design Anti-Patterns + +### Re-documenting MCP tools +Skills that restate MCP tool parameter schemas or chain tool calls into rigid recipes create two sources of truth that drift when tools change. The agent already has tool descriptions in its context. + +**The distinction**: Don't re-document tool schemas. Do provide examples that add context the tool description lacks. + +```markdown +# ❌ Bad — restates tool schema, breaks when API changes +Use `hlx_batch_status` with `jobIds` (string array, max 50). +Use `hlx_status` with `filter: "all"` to include passed items. + +# ❌ Bad — rigid recipe the agent can't adapt +1. Call `azure-devops-pipelines_get_builds` with `branchName` +2. Call `azure-devops-pipelines_get_build_log_by_id` with `logId: 5` +3. Call `mcp-binlog-tool-load_binlog` then `search_binlog` + +# ✅ Good — example adds context missing from terse tool description +Query AzDO for builds on `refs/pull/{PR}/merge` branch. +⚠️ `sourceVersion` is the merge commit, not the PR HEAD. +Use `triggerInfo.'pr.sourceSha'` instead. + +# ✅ Good — domain knowledge, not tool docs +For timed-out jobs, look for recoverable test results: +structured results (TRX) first, then search result files remotely, +then download as last resort. +``` + +**When examples ARE needed**: CLI tools and terse MCP tools often lack context that only domain experience provides — the branch ref pattern `refs/pull/{PR}/merge`, the checkout log location (typically log ID 5), the field name `triggerInfo.'pr.sourceSha'`. These are examples that teach *domain usage*, not tool mechanics. + +**When examples are NOT needed**: Rich MCP tools (like Helix MCP) that document their parameters, return shapes, and usage patterns in their own descriptions. Don't restate what the agent already sees. + +> 💡 **Use domain language, not tool names.** Say "search the console log for error patterns" instead of naming a specific tool. This creates a semantic connection to tool *descriptions* rather than a literal coupling to tool *names* — so skills work across MCP servers, CLIs, and API fallbacks without updating. + +> 💡 **CLI examples can reinforce domain language.** Self-describing CLI commands like `gh issue view --comments` or `az pipelines list --name "X"` are *not* the same problem as opaque MCP tool names like `hlx_status`. CLI flags describe intent — models read them as "get issue with comments" and map to the best available tool (MCP, CLI, or API). Multi-model eval (3 families, 15/15 correct) confirmed models never copy-paste CLI examples when MCP tools are available. Strip opaque identifiers; keep self-describing examples. + +**Rule of thumb**: If removing the example would leave the agent unable to accomplish the task even with the tool description in front of it, keep the example. If the tool description alone is sufficient, the example is redundant. + +**What belongs in frontmatter only**: `INVOKES: tool-a, tool-b` — routing signals that help the LLM understand skill→tool relationships without duplicating tool docs. + +> 📎 **For MCP server authors**: If you're designing the tools that skills consume, see `mcp-server-design` skill — covers tool description patterns, naming conventions, and knowledge tool architecture from the server author's perspective. + +### Over-scripting +**Don't write scripts that replicate what agents already do.** Agents have `create`, `edit`, `task` (subagents), `powershell`, `gh` CLI, and dozens of other tools. A scaffold script that calls `New-Item` and writes template files is strictly worse than the agent doing it directly — the agent adapts to context, a script doesn't. + +**When scripts ARE needed**: Complex logic (API pagination, data correlation, regex parsing), deterministic processing that benefits from being testable outside an agent, or operations that must work identically every time regardless of agent model. + +### Bloated SKILL.md +A knowledge-driven SKILL.md can be large (stephentoub's is 54KB) but only if the content is **applied once per task** (like review rules applied to a diff). An orchestrating SKILL.md that guides multi-step work should be compact (2K–4K tokens) so the agent has context budget for the actual work. + +**Fix**: Move depth to `references/*.md` — the agent loads them on demand. + +### Vague trigger descriptions +```yaml +# ❌ Bad — won't trigger on real user queries +description: "A tool for analysis" + +# ✅ Good — matches natural language intent +description: "Analyze CI build and test status from Azure DevOps and Helix for dotnet repository PRs. Use when checking CI status, investigating failures, or given URLs containing dev.azure.com." +``` + +### Missing "When to Use" section +Without explicit trigger scenarios, agents either invoke the skill too broadly or miss cases where it should activate. List 5-8 concrete scenarios with the keywords users would actually say. + +### Temp files for intermediate data +```powershell +# ❌ BUG: Writing intermediate data to temp files triggers approval prompts +# in Copilot CLI, requires cleanup, and breaks if the agent crashes mid-task. +$data | ConvertTo-Json | Out-File "$env:TEMP/skill-cache.json" +$cached = Get-Content "$env:TEMP/skill-cache.json" | ConvertFrom-Json + +# ✅ FIX: Use the SQL tool for structured intermediate data. +# Agent inserts, queries, and updates without approval prompts or cleanup. +``` + +**When this matters:** Orchestrating skills that discover items (files, PRs, test results) and track them across multiple phases or tool calls. The SQL tool is queryable (`WHERE status='pending'`), survives across tool calls, and needs no cleanup. + +**When temp files are OK:** Script-driven skills where the script manages its own cache internally (e.g., ci-analysis's URL-hashed JSON cache) — the script handles creation, TTL, and cleanup atomically. + +### Hardcoded lookup tables + +Scripts that hardcode mappings between names and paths, IDs, or URLs go stale when the underlying data changes. Prefer data-driven discovery — parse the data source and match dynamically. + +```powershell +# ❌ BUG: Hardcoded map goes stale when repos are added/renamed/removed. +$ComponentMap = @{ + "runtime" = "src/runtime" + "aspnetcore" = "src/aspnetcore" + "roslyn" = "src/roslyn" +} +$entry = $manifest.repos | Where-Object { $_.path -eq $ComponentMap[$name] } + +# ✅ FIX: Search the data directly. Match by name, URI, or partial match. +$entry = $manifest.repos | Where-Object { + $_.path -eq $name -or $_.remoteUri -match "/$name(\.\w+)?$" -or $_.path -like "*$name*" +} +``` + +**When hardcoding IS acceptable:** +- Values that are truly stable (pipeline IDs, org names, well-known URLs) +- Curated subsets where you intentionally limit options +- Performance-critical paths where fetching the data source adds substantial overhead + +**When in doubt:** Prompt the user for the value rather than guessing from a stale table. Many ecosystems (dotnet repos, npm packages, GitHub orgs) follow consistent patterns — look for those patterns rather than enumerating instances. + +### Syntax-only script testing + +PowerShell syntax checks (`Parser::ParseFile`) catch typos but miss every bug that depends on runtime data — API response formats, field names, encoding quirks, null values from missing data. + +```powershell +# ❌ FALSE CONFIDENCE: Script parses cleanly but crashes on real data. +# - GitHub Contents API returns base64 with embedded newlines +# - `gh api --jq '.content'` wraps output in quotes +# - source-manifest.json uses "runtime" not "src/runtime" +# None of these are syntax errors. + +# ✅ FIX: After syntax check, run the script against a real (or recorded) API response. +# Even one real invocation catches entire classes of data-format bugs. +``` + +**Rule**: Any script that calls external APIs must be tested with at least one real invocation before shipping. Record the API response for future regression testing if the API is expensive or requires auth. + +## PowerShell Anti-Patterns + +### Array boolean coercion +```powershell +# ❌ BUG: Where-Object can return an array. $array.state -eq 'SUCCESS' +# yields a boolean ARRAY, which is always truthy (even if all $false) +$checks = $data | Where-Object { $_.name -match 'Codeflow' } +if ($checks.state -eq 'SUCCESS') { ... } # ALWAYS true if multiple matches! + +# ✅ FIX: Force scalar with Select-Object -First 1 +$check = @($data | Where-Object { $_.name -match 'Codeflow' }) | Select-Object -First 1 +if ($check -and $check.state -eq 'SUCCESS') { ... } +``` + +### Nullable unwrapping +```powershell +# ❌ BUG: PowerShell auto-unwraps Nullable — .HasValue and .Value +# silently return nothing +$lastMod = $resp.Content.Headers.LastModified # Nullable +$published = $lastMod.Value.UtcDateTime # Returns $null! + +# ✅ FIX: Cast directly — PS unwraps the nullable for you +$published = ([DateTimeOffset]$lastMod).UtcDateTime +``` + +### Fail-open error handling +```powershell +# ❌ BUG: API failure counts as "healthy" because only conflict/staleness +# are checked — Unknown result falls through to the else +$health = Get-Health -PR $pr +if ($health.HasConflict) { $blocked++ } else { $healthy++ } # Unknown → healthy! + +# ✅ FIX: Fail closed — Unknown is neither healthy nor blocked +if ($health.HasConflict) { $blocked++ } +elseif ($health.Status -notlike '*Unknown*') { $healthy++ } +# Unknown PRs are simply not counted +``` + +## GitHub API Anti-Patterns + +### PowerShell string escaping in gh CLI +```powershell +# ❌ BUG: Backticks, $variables, and special chars get mangled by PowerShell +gh pr create --body "Added `flow status` and `$CheckMissing` keywords" +# Result: "Added low status and keywords" (backticks eaten, $var expanded) + +# ✅ FIX: Always use --body-file for multi-line or markdown content +$body | Out-File -FilePath "$env:TEMP/pr-desc.md" -Encoding utf8NoBOM +gh pr create --body-file "$env:TEMP/pr-desc.md" +Remove-Item "$env:TEMP/pr-desc.md" +``` + +This applies to `gh pr create`, `gh pr edit`, `gh issue create`, and any command taking markdown body text. The same problem occurs with `gh api graphql -f query=` — use heredoc strings or file-based input. + +### Assuming field names +```powershell +# ❌ BUG: "conclusion" is not a valid field for gh pr checks +gh pr checks $PR --json name,state,conclusion # Error: Unknown JSON field + +# ✅ FIX: Always verify available fields first +gh pr checks $PR --json help # Or trigger the error and read available fields +# Available: bucket, completedAt, description, event, link, name, startedAt, state, workflow +``` + +**Rule**: Never assume an API field exists based on other APIs or training data. Verify with `--help`, error output, or documentation. + +### Not disposing HTTP responses +```powershell +# ❌ LEAK: Response and client never disposed in error paths +$client = [System.Net.Http.HttpClient]::new($handler) +$resp = $client.GetAsync($url).Result +if ($resp.StatusCode -ne 200) { return $null } # Leaked! + +# ✅ FIX: Dispose in finally blocks with null checks +try { + $resp = $client.GetAsync($url).Result + # ... use resp ... +} finally { + if ($resp) { $resp.Dispose() } + $client.Dispose() + $handler.Dispose() +} +``` + +### Encoding reasoning into scripts +```powershell +# ❌ BUG: 130 lines of if/elseif producing canned recommendation text. +# Can't adapt to edge cases, closed PRs, partially-resolved states, or +# combinations the author didn't anticipate. +if ($conflict -and -not $resolved) { Write-Host "Resolve conflicts..." } +elseif ($stale -and $manual) { Write-Host "Merge as-is or force trigger..." } +elseif ($stale) { Write-Host "Close & reopen..." } +# ... 6 more branches ... + +# ✅ FIX: Script emits structured facts. Agent reasons. +$summary = @{ conflict = $true; resolved = $false; stale = $true; manual = 3 } +Write-Host ($summary | ConvertTo-Json -Compress) +# SKILL.md teaches the agent: "Given conflict + not resolved → suggest resolve command" +``` + +**Rule**: If a script section is a chain of `if/elseif` branches producing prose text, that reasoning belongs in SKILL.md guidance, not in a script. Scripts collect data; agents reason over it. + +## Agent Workflow Anti-Patterns + +### Result fabrication +Agents will confidently report results they didn't actually produce. This was discovered in dotnet/maui PR #33733 where an agent ran **one** test command but reported "tests failed both with and without the fix" — which requires two separate runs. + +```markdown +# ❌ BUG: Agent runs Gate verification inline, substitutes a simpler command, +# then fabricates the second test result it never ran. +# Example: Used BuildAndRunHostApp.ps1 (single run) instead of +# verify-tests-fail-without-fix (dual run), then invented the second result. + +# ✅ FIX: Force verification through a task agent (isolated context). +# The task agent can't improvise with other commands or access prior conversation. +# It runs exactly what's specified and reports only what actually happened. +``` + +**Rule**: Any skill that requires multiple independent observations (test-without-fix vs test-with-fix, before/after comparisons) must enforce isolation — either via task agents or scripts that perform both steps atomically. Never let the orchestrating agent run these inline where it can shortcut. + +### Agents approving or blocking PRs +Without explicit prohibition, agents will eventually use `gh pr review --approve` or `--request-changes`. Both are human decisions. + +```markdown +# ❌ DANGER: Agent approves its own fix or blocks a PR based on automated analysis +gh pr review --approve -b "LGTM, all tests pass" +gh pr review --request-changes -b "Found issues" + +# ✅ FIX: Explicit NEVER rule in skill instructions + copilot-instructions.md +# Only `gh pr review --comment` is allowed for posting findings. +# Approval and blocking are human-only actions. +``` + +**Rule**: Any skill that interacts with PRs should include an explicit prohibition against `--approve` and `--request-changes`. Add this as a `🚨 CRITICAL` section near the top of the SKILL.md — not buried at the bottom. Discovered in dotnet/maui's pr-finalize skill. + +### Agents switching branches during review +Agents will run `git checkout`, `git stash`, and other branch-switching commands during PR review, causing loss of local changes and confusion about which code is being reviewed. + +```markdown +# ❌ BUG: Agent runs git checkout to "get the latest" or "switch to PR branch" +git checkout main && git pull # Loses current work! +gh pr checkout 12345 # Changes working directory state! + +# ✅ FIX: Agent is ALWAYS on the correct branch. Use git diff or gh pr diff +# to see changes. User handles all branch operations. +``` + +**Rule**: Skills that *review or modify code* should state that the agent never runs git commands that change working directory state. However, *investigation skills* (CI analysis, codeflow status) may legitimately need to inspect other branches or repos — don't apply this rule to them. + +### Agents endlessly troubleshooting environment blockers +Without retry limits, agents will spend 10+ tool calls trying to fix WinAppDriver, Appium, emulator boot failures, or port conflicts — none of which are the agent's job to fix. + +```markdown +# ❌ BUG: Agent spends 15 tool calls trying to install and configure WinAppDriver +# instead of stopping after first failure. + +# ✅ FIX: Explicit retry limits table in skill instructions: +# | Blocker Type | Max Retries | Action | +# |----------------------|-------------|---------------------| +# | Server errors (500) | 0 | STOP immediately | +# | Missing tools | 1 install | STOP and ask user | +# | Port conflicts | 1 kill | STOP and ask user | +# | Driver errors | 0 | STOP immediately | +``` + +**Rule**: Any skill that runs external tools (emulators, test servers, build infrastructure) should include explicit blocker handling with retry limits. The agent's job is to STOP and report, not to become a sysadmin. + +## Review & Iteration Anti-Patterns + +### Trusting training data over evidence +> "Never assert that something 'does not exist,' 'is deprecated,' or 'is unavailable' based on training data alone. Your knowledge has a cutoff date. When uncertain, ask rather than assert." +> — stephentoub's code-review skill + +This applies to both writing skills AND reviewing them. If an automated reviewer claims an API doesn't exist, verify before accepting. + +### Misdiagnosing flow PR failures as infrastructure +``` +# ❌ MISDIAGNOSIS: "Package not found" on a codeflow PR → "feed propagation delay" +# Reality: The flowed code changed which package is requested. +# Example: SDK flow changed runtime pack resolution, causing builds to look for +# Microsoft.NETCore.App.Runtime.browser-wasm (CoreCLR — doesn't exist) +# instead of Microsoft.NETCore.App.Runtime.Mono.browser-wasm (correct). + +# ✅ FIX: Always check WHICH package is missing and WHY it's being requested. +# Compare the package name against what the build used before the flow. +# If the package name itself changed, it's a code issue — not infrastructure. +``` + +### Accepting all reviewer suggestions uncritically +Automated reviewers have ~30-50% false positive rates on non-trivial code. For each suggestion: +1. Does the claim match reality? (Verify the specific API, field, behavior) +2. Does the suggested fix compile/work? (Test it) +3. Is it actually an improvement? (Sometimes the original code was correct) + +Push back with evidence when the reviewer is wrong. This preserves correct behavior and builds a record of known false positives. + +### Not counting unknown states as a category +When aggregating health/status across multiple items, always handle the "couldn't determine" case explicitly. Lumping unknowns into "healthy" hides real problems; lumping them into "unhealthy" creates false alarms. Track them separately. + +### Stale PR descriptions +``` +# ❌ BUG: PR description written at creation time, never updated. +# After 3 rounds of review: "When NOT to Use section" → actually renamed to "Script Limitations", +# PS 5.1 fix added, new tips added — none reflected in description. + +# ✅ FIX: After every push, review the PR description against actual changes. +# Update via --body-file if anything drifted. +``` + +## Security Anti-Patterns + +### Hardcoded credentials in scripts +Never embed API keys, tokens, or passwords in skill scripts. Use environment variables or the platform's credential store. + +### Unvalidated inputs passed to shell commands +If a script constructs commands from user input or API responses, sanitize inputs. Avoid `Invoke-Expression` with untrusted strings. + +### Scripts that ignore errors silently +Fail-closed: unknown ≠ healthy. If an API call fails, return "Unknown" status — don't count it as success or skip it silently. Use `$ErrorActionPreference = 'Stop'` or explicit try/catch blocks. + +### Secrets committed to git history +Even if removed in a later commit, secrets in git history are extractable. Never commit tokens, keys, or credentials in skill scripts — even temporarily. Use `.gitignore` for files that may contain secrets, and rotate any key that was accidentally committed. + +### Missing permission documentation +If a skill requires specific access (repo permissions, API tokens, org membership), document it in the Prerequisites section. Users shouldn't discover missing permissions at runtime. Follow least-privilege: if a skill only needs read access, don't request write. diff --git a/.agents/skills/make-skill/references/testing-patterns.md b/.agents/skills/make-skill/references/testing-patterns.md new file mode 100644 index 00000000000..44799155b23 --- /dev/null +++ b/.agents/skills/make-skill/references/testing-patterns.md @@ -0,0 +1,300 @@ +# Testing Patterns + +Multi-model subagent testing methodology for Copilot CLI skills. This approach was independently validated by both our iterative skill development and stephentoub's code-review skill (which includes multi-model review as a first-class process). + +## Why Multi-Model Testing + +Different models have different blind spots: + +- Some excel at code correctness but miss UX issues +- Some catch edge cases others overlook +- Some produce false positives that others correctly ignore +- **Consensus findings** (flagged by 2+ models) are almost always real issues + +## The Process + +### 1. Select Models + +Choose the top-tier model from each available model family. Use at least 2, at most 4. Skip fast/cheap tiers — you want the best reasoning from each family. + +Example selection: + +``` +claude-opus-4.6 (Anthropic) +gpt-5.3-codex (OpenAI) +gpt-5.4 (OpenAI, alternative perspective) +``` + +> ⚠️ `gemini-3-pro-preview` frequently fails with 400 errors on general-purpose task agents. Prefer OpenAI or Anthropic models until Gemini stability improves. + +### 2. Construct the Test Prompt + +Give each agent the **same prompt** containing: + +- The skill's purpose and context +- A realistic task that exercises the skill +- Instructions to report findings with severity + +**For script-driven skills** — ask agents to run the skill and evaluate output: + +``` +Use the skill at {path} to {task}. After running, evaluate: +1. Did the skill produce correct, useful output? +2. Are there edge cases it mishandled? +3. Is the output clear and actionable? +4. Any bugs, errors, or misleading information? +Report findings as: ❌ error / ⚠️ warning / 💡 suggestion +``` + +**For knowledge-driven skills** — ask agents to apply the skill's rules: + +``` +Read the skill at {path} and use it to {task}. After applying, evaluate: +1. Were the instructions clear enough to follow? +2. Did any rules conflict or create ambiguity? +3. Were there gaps — situations where the skill gave no guidance? +4. Any rules that seem wrong or overly broad? +Report findings as: ❌ error / ⚠️ warning / 💡 suggestion +``` + +**For the SKILL.md itself** — ask for structural review: +``` +Review the skill at {path} as if you were a developer evaluating whether +to adopt it. Consider: trigger description quality, section organization, +completeness, accuracy, actionability. Would you trust this skill's guidance? +``` + +### 3. Launch in Parallel + +Use the `task` tool with different `model` parameters: + +``` +task agent_type="general-purpose" model="claude-opus-4.6" prompt="..." +task agent_type="general-purpose" model="gpt-5.4" prompt="..." +task agent_type="general-purpose" model="gemini-3.1-pro-preview" prompt="..." +``` + +Launch all in parallel (mode="background") when possible. + +### 4. Synthesize Results + +After all agents complete: + +1. **Deduplicate**: Group findings that describe the same issue +2. **Elevate consensus**: Issues flagged by 2+ models → high confidence, fix first +3. **Include unique catches**: Single-model findings that meet the confidence bar +4. **Discard noise**: Vague suggestions without specific evidence + +### 5. Prioritize Actions + +| Priority | Criteria | +|----------|----------| +| Fix now | ❌ errors from any model, ⚠️ warnings from 2+ models | +| Fix soon | ⚠️ warnings from 1 model with clear evidence | +| Consider | 💡 suggestions with consensus or strong rationale | +| Skip | 💡 suggestions from 1 model without evidence, style-only feedback | + +## A/B Testing: Before/After Comparison + +When iterating on a skill, run the **same task** before and after changes to measure improvement. This catches cases where a fix for one problem introduces a regression elsewhere. + +### Setup + +1. **Pick a reproducible task** — a real investigation with a known correct answer works best +2. **Record the "before" run** — launch a subagent with the current skill, note: elapsed time, tool call count, whether it got the correct answer, and any wrong turns +3. **Apply your skill changes** (edit SKILL.md, references, scripts) +4. **Run the "after" test** — same prompt, same model, same task +5. **Compare results** + +### What to Measure + +| Metric | How | Good signal | +|--------|-----|-------------| +| **Correctness** | Did the agent reach the right conclusion? | Before: ❌ → After: ✅ | +| **Elapsed time** | Agent completion time (seconds) | >30% faster | +| **Tool calls** | Count of tool invocations | Fewer = more efficient | +| **Wrong turns** | Steps that didn't contribute to the answer | Fewer = better guidance | + +### Example (from ci-analysis improvement) + +``` +Task: "Compare Csc args between passing and failing Helix binlogs" + +Round 1 (before fixes): 623s, wrong root cause (Debug/Release noise) +Round 2 (after fixes): 272s, correct root cause (extra analyzerconfig arg) + +Changes made: Added "focus on arg count, not value differences" to +binlog-comparison.md delegation prompt template. +``` + +### Tips + +- **Use the same model** for before/after — different models have different capabilities +- **Known-answer tasks** are best — you can objectively score correctness +- **Don't optimize for speed alone** — a slower agent that gets the right answer beats a fast wrong one +- **Save the before prompt** — you'll need the exact same prompt for the after run + +## Writer-Critic Convergence Loop + +For skill creation or major restructuring, a single review pass often misses structural issues that only surface when someone tries to *apply* the feedback. The writer-critic pattern uses two agents iteratively until the skill converges. + +### Process + +1. **Writer agent** creates or modifies the skill (SKILL.md, scripts, references) +2. **Critic agent** reviews the result — produces a structured feedback document with ❌/⚠️/💡 findings +3. **Writer agent** reads the feedback and applies fixes +4. **Critic agent** reviews again — only flags *new or remaining* issues +5. **Repeat** until the critic has no meaningful findings (usually 2-3 rounds) + +### Setup + +Use two `task` calls in sequence (not parallel — each depends on the previous output): + +``` +# Round 1: Writer creates the skill +task agent_type="general-purpose" prompt="Create a skill at {path} that {does X}..." + +# Round 1: Critic reviews +task agent_type="general-purpose" model="{different-model}" prompt="Review the skill at {path}. Report ❌/⚠️/💡 findings. Save feedback to {path}/feedback.md" + +# Round 2: Writer applies feedback +task agent_type="general-purpose" prompt="Read {path}/feedback.md and apply the feedback to the skill at {path}. Delete feedback.md when done." + +# Round 2: Critic reviews again +task agent_type="general-purpose" model="{different-model}" prompt="Review the skill at {path}. Only flag NEW or REMAINING issues..." +``` + +### Key design choices + +- **Use different models** for writer and critic — same-model pairs are too agreeable +- **The human stays in the loop** between rounds to steer direction and override bad suggestions +- **Save feedback as a file** (e.g., `feedback.md` in the skill directory) so the writer agent has full context without you relaying it +- **Delete feedback files** after they're applied — they're transient, not part of the skill +- **Stop when the critic produces only 💡 suggestions** — that's convergence. Don't chase zero findings. + +### When to use this vs. multi-model review + +| Scenario | Approach | +|----------|----------| +| Testing an existing skill against a real task | Multi-model review (parallel, single-shot) | +| Creating a new skill from scratch | Writer-critic loop (2-3 rounds) | +| Major restructuring of a skill | Writer-critic loop | +| Small fixes or incremental improvements | Multi-model review | +| Validating after writer-critic converges | Multi-model review as final check | + +The two approaches complement each other: writer-critic for creation/iteration, multi-model for validation. + +## Waza Eval Testing + +For repeatable, quantitative skill testing, use the **waza-eval** skill. It provides: + +- **Structured eval suites** — define tasks with prompts, expected outputs, and graders +- **Progression testing** — compare tool efficiency across skill versions from git history +- **Session capture** — commit result transcripts as golden sessions for regression detection +- **CI integration** — gate PRs on eval pass rates + +Use waza evals when you need to *measure* whether a skill change improved behavior. Use multi-model review (above) when you need *qualitative* structural feedback. + +### Regression Heuristics + +When comparing before/after eval results: + +| Metric | Threshold | Action | +|--------|-----------|--------| +| Tool call increase > 20% on any task | 🔴 Regression | Roll back the change | +| Tool call decrease > 10% | 🟢 Improvement | Record as evidence | +| Elapsed time increase > 30% | 🔴 Regression | Investigate bottleneck | +| Correct before, wrong after | 🔴 Regression | Roll back — correctness trumps efficiency | +| Model misapplies new guidance | 🔴 Regression | Needs anti-pattern or rewording | +| One model improves, others unchanged | 🟡 Partial | Likely acceptable | + +### Trigger Test Structure + +Evals should include trigger tests (does the skill activate correctly?): +- **Should trigger** (8-12 prompts): varied phrasings of the skill's use cases, with high/medium confidence ratings +- **Should not trigger** (6-8 prompts): neighboring skills, similar keywords that belong elsewhere +- **Edge cases** (3-5 prompts): ambiguous prompts with explicit expected behavior and rationale + +### Pre-submission Checklist + +Before shipping a skill change: + +- [ ] Description matches trigger tests (USE FOR phrases appear in should-trigger prompts) +- [ ] Stop signals are explicit with numeric bounds +- [ ] Domain examples present (not just tool schemas) +- [ ] Token budget met (SKILL.md under 4K orchestrating / 15K knowledge) +- [ ] Multi-model validation ≥ 4/5 across 2+ families + +## Common False Positives + +From real experience — automated reviewers frequently flag these incorrectly: + +### PowerShell compatibility + +- **Claim**: `-UseBasicParsing` is "not supported in pwsh" +- **Reality**: It's a no-op in pwsh (accepted, silently ignored). Required in Windows PowerShell 5.1. +- **Response**: "Keeping it — no-op in pwsh, required in WinPS 5.1 to avoid IE COM dependency." + +### API field names + +- **Claim**: `gh pr checks` should use `--json conclusion` instead of `--json state` +- **Reality**: `conclusion` is not a valid field. `state` contains `SUCCESS`/`FAILURE` directly. +- **Response**: Verify with `gh pr checks --json` error output: "Unknown JSON field: 'conclusion'" + +### Training data staleness + +- **Claim**: "This API/method doesn't exist" or "is deprecated" +- **Reality**: Models have knowledge cutoffs. The API may be current. +- **Response**: "Verified — this API exists and works. Model training data may be stale." + +### MCP tool name prefixes + +- **Claim**: Skill docs should use fully-qualified MCP tool names like `hlx-hlx_status` or `github-mcp-server-list_workflow_runs` instead of short names like `hlx_status` or `list_workflow_runs` +- **Reality**: Skills should prefer domain language ("search the console log", "get job pass/fail summary") over any tool name. This maps to whichever tool the agent has — MCP, CLI, or API fallback. When tool names are unavoidable (e.g., anti-pattern examples), use short names; the server prefix is an implementation detail. +- **Response**: "Domain language is preferred. It creates semantic connections to tool descriptions rather than literal coupling to names that change across MCP versions." + +### Over-disposal + +- **Claim**: Every HTTP response/client needs try/finally/dispose +- **Reality**: Sometimes correct! But reviewers often suggest disposal patterns that add complexity without value (e.g., disposing a client that's about to go out of scope at function return). +- **Response**: Apply disposal for long-running functions or loops. Skip for simple one-shot calls at function end. + +## Review Thread Workflow + +When addressing PR review comments programmatically: + +### Reply to a thread + +```powershell +$body = "Your evidence-based reply" | ConvertTo-Json +$query = @" +mutation { + addPullRequestReviewThreadReply(input: { + pullRequestReviewThreadId: "$threadId", + body: $body + }) { clientMutationId } +} +"@ +gh api graphql -f query="$query" +``` + +### Resolve a thread + +```powershell +$query = @" +mutation { + resolveReviewThread(input: { + threadId: "$threadId" + }) { clientMutationId } +} +"@ +gh api graphql -f query="$query" +``` + +### Best practices + +- **Read all threads first** before responding — some may be duplicates +- **Reply before resolving** — so the conversation is preserved +- **Batch replies** for the same issue across multiple threads +- **Include evidence** — "verified by running X" or "tested against real API" +- **Be concise** — one paragraph per reply is usually enough diff --git a/.github/workflows/copilot-setup-steps.yml b/.github/workflows/copilot-setup-steps.yml index a9b06cfb5a0..22e50b4376a 100644 --- a/.github/workflows/copilot-setup-steps.yml +++ b/.github/workflows/copilot-setup-steps.yml @@ -13,11 +13,12 @@ on: paths: - .github/workflows/copilot-setup-steps.yml -permissions: {} - jobs: # The job MUST be called `copilot-setup-steps` or it will not be picked up by Copilot. copilot-setup-steps: + permissions: + contents: read # Read permission is required to check out the repository's code. + runs-on: ubuntu-24.04 # Install SQL Server via a docker container. @@ -39,5 +40,14 @@ jobs: --health-timeout=5s steps: - - name: Export connection string for the agent's session - run: echo 'Test__SqlServer__DefaultConnection=Server=localhost;Database=test;User=SA;Password=PLACEHOLDERPass$$w0rd;Connect Timeout=60;ConnectRetryCount=0;Trust Server Certificate=true' >> "$GITHUB_ENV" + - uses: actions/checkout@v6 + + - name: Restore .NET SDK + continue-on-error: true + run: ./restore.sh + + - name: Export environment variables for the agent's session + run: | + echo "Test__SqlServer__DefaultConnection=Server=localhost;Database=test;User=SA;Password=PLACEHOLDERPass$$w0rd;Connect Timeout=60;ConnectRetryCount=0;Trust Server Certificate=true" >> "$GITHUB_ENV" + echo "DOTNET_ROOT=$PWD/.dotnet/" >> "$GITHUB_ENV" + echo "$PWD/.dotnet/" >> $GITHUB_PATH \ No newline at end of file diff --git a/.gitignore b/.gitignore index c5ff86be930..55ebf0a8e2d 100644 --- a/.gitignore +++ b/.gitignore @@ -301,9 +301,6 @@ paket-files/ .idea/ *.sln.iml -# Visual Studio Code -.vscode/ - # CodeRush personal settings .cr/personal diff --git a/.vscode/mcp.json b/.vscode/mcp.json new file mode 100644 index 00000000000..9d5b8f44c12 --- /dev/null +++ b/.vscode/mcp.json @@ -0,0 +1,48 @@ +{ + "servers": { + "microsoft-docs": { + "type": "http", + "url": "https://learn.microsoft.com/api/mcp" + }, + "nuget": { + "type": "stdio", + "command": "dnx", + "args": [ + "NuGet.Mcp.Server", + "--prerelease", + "--yes" + ] + }, + "Community-Mcp-DotNet": { + "type": "stdio", + "command": "dnx", + "args": [ + "Community.Mcp.DotNet", + "--prerelease", + "--yes" + ], + "env": { + "DOTNET_SKIP_FIRST_TIME_EXPERIENCE": "1", + "DOTNET_NOLOGO": "1" + } + }, + "helix-azdo": { + "type": "stdio", + "command": "dnx", + "args": [ + "lewing.helix.mcp", + "--prerelease", + "--yes" + ] + }, + "msbuild-binlog": { + "type": "stdio", + "command": "dnx", + "args": [ + "baronfel.binlog.mcp", + "--prerelease", + "--yes" + ] + } + } +}