From 7771cd2287fe9665d2d0027ef3bc1eb9088eeec6 Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Mon, 5 Oct 2026 12:39:46 -0700 Subject: [PATCH 1/2] Improve skills and evals Remove query-pipeline skill as it's consistently failing evals even after tweaks --- .agents/skills/change-tracking/SKILL.md | 6 +- .agents/skills/code-review/SKILL.md | 7 +- .agents/skills/make-custom-agent/SKILL.md | 12 +- .../make-github-actions-workflow/SKILL.md | 37 +- .agents/skills/make-skill/SKILL.md | 159 ++------ .../make-skill/references/anti-patterns.md | 351 ------------------ .../make-skill/references/testing-patterns.md | 301 --------------- .agents/skills/migrations/SKILL.md | 6 +- .agents/skills/model-building/SKILL.md | 16 +- .agents/skills/query-pipeline/SKILL.md | 21 -- .agents/skills/scaffolding/SKILL.md | 4 + .agents/skills/sqlite-adonet/SKILL.md | 14 +- .agents/skills/tooling/SKILL.md | 8 + .agents/skills/update-pipeline/SKILL.md | 4 + .github/copilot-instructions.md | 6 +- eng/harness-evaluation/README.md | 3 + .../copilot-instructions/eval.yaml | 153 ++++++-- .../query-pipeline/fallback-and-tests.cs | 82 ++++ .../query-pipeline/projection-retry.cs | 92 +++++ .../query-pipeline/split-enumerables.cs | 137 +++++++ .../skills/change-tracking/eval.yaml | 28 +- .../skills/code-review/eval.yaml | 84 +++-- .../ModelValidator.cs | 105 ++++++ .../ModelValidatorTest.cs | 60 +++ .../skills/cosmos-provider/eval.yaml | 2 +- .../skills/make-custom-agent/eval.yaml | 49 ++- .../make-github-actions-workflow/eval.yaml | 34 +- .../skills/make-instructions/eval.yaml | 52 ++- .../skills/make-skill/eval.yaml | 57 +-- .../skills/migrations/eval.yaml | 55 ++- .../legacy-snapshot-compatibility/Migrator.cs | 69 ++++ .../SnapshotModelProcessor.cs | 74 ++++ .../SnapshotModelProcessorTest.cs | 50 +++ .../migrations/fixtures/snapshot-harness.cs | 44 +++ .../migrations/fixtures/upgrade-report.txt | 30 ++ .../skills/model-building/eval.yaml | 66 +++- .../InversePropertyAttributeConvention.cs | 81 ++++ .../NavigationAttributeConventionTest.cs | 44 +++ .../skills/query-pipeline/eval.yaml | 51 --- .../skills/run-apichief/eval.yaml | 2 +- .../skills/scaffolding/eval.yaml | 25 +- .../skills/servicing-pr/eval.yaml | 2 +- .../skills/sqlite-adonet/eval.yaml | 22 +- .../skills/tooling/eval.yaml | 24 +- .../skills/triage/eval.yaml | 4 +- .../skills/update-pipeline/eval.yaml | 41 +- 46 files changed, 1509 insertions(+), 1065 deletions(-) delete mode 100644 .agents/skills/make-skill/references/anti-patterns.md delete mode 100644 .agents/skills/make-skill/references/testing-patterns.md delete mode 100644 .agents/skills/query-pipeline/SKILL.md create mode 100644 eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/fallback-and-tests.cs create mode 100644 eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/projection-retry.cs create mode 100644 eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/split-enumerables.cs create mode 100644 eng/harness-evaluation/skills/code-review/fixtures/model-validator-relocation/ModelValidator.cs create mode 100644 eng/harness-evaluation/skills/code-review/fixtures/model-validator-relocation/ModelValidatorTest.cs create mode 100644 eng/harness-evaluation/skills/migrations/fixtures/legacy-snapshot-compatibility/Migrator.cs create mode 100644 eng/harness-evaluation/skills/migrations/fixtures/legacy-snapshot-compatibility/SnapshotModelProcessor.cs create mode 100644 eng/harness-evaluation/skills/migrations/fixtures/legacy-snapshot-compatibility/SnapshotModelProcessorTest.cs create mode 100644 eng/harness-evaluation/skills/migrations/fixtures/snapshot-harness.cs create mode 100644 eng/harness-evaluation/skills/migrations/fixtures/upgrade-report.txt create mode 100644 eng/harness-evaluation/skills/model-building/fixtures/inverse-ambiguity/InversePropertyAttributeConvention.cs create mode 100644 eng/harness-evaluation/skills/model-building/fixtures/inverse-ambiguity/NavigationAttributeConventionTest.cs delete mode 100644 eng/harness-evaluation/skills/query-pipeline/eval.yaml diff --git a/.agents/skills/change-tracking/SKILL.md b/.agents/skills/change-tracking/SKILL.md index 5ce64693ddb..3ec731a1788 100644 --- a/.agents/skills/change-tracking/SKILL.md +++ b/.agents/skills/change-tracking/SKILL.md @@ -23,6 +23,10 @@ Manages entity states and detects changes for `SaveChanges()`. - Snapshot and notification strategies both call `SetPropertyModified()` to keep property flags and entity state consistent. - `SetEntityState()` validates values, updates flags and complex entries, changes state, then runs manager bookkeeping hooks. +## Shared-Identity Replacement + +`SharedIdentityEntry` is the invariant that distinguishes an old deleted entry from its replacement with the same identity. When attaching a replacement graph, a deleted nested entry should be reconsidered only when its source entry has a replacement; do not broadly process every deleted node. Likewise, cascading from the old replaced principal must not delete dependents that belong to the replacement graph. Keep synchronous and asynchronous graph-attach paths equivalent and preserve unrelated values in replacement tests so they isolate the identity transition. + ## Testing Unit tests: `test/EFCore.Tests/ChangeTracking/`. Functional tests: `test/EFCore.Specification.Tests/GraphUpdates/`. @@ -31,7 +35,7 @@ Unit tests: `test/EFCore.Tests/ChangeTracking/`. Functional tests: `test/EFCore. | Pitfall | Solution | |---------|----------| -| There is a failure when there is shared identity entry (Added and Deleted) | Add code that checks `SharedIdentityEntry` | +| State-based checks treat every Added/Deleted entry as a replacement | Scope replacement behavior through `SharedIdentityEntry` and the graph source | ## Validation diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index 3b7913e516b..0c6cb7ab925 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -30,8 +30,9 @@ Before reading the author's narrative: 1. Read each changed implementation beyond its diff hunk. Diff-only review is insufficient evidence for a finding. For very large files, read the containing members, their contracts, and relevant state. When many files are changed, use subagents to break down the review into manageable parts and gather context incrementally. 2. Search callers, implementations, overrides, sibling provider paths, and nearby tests. Read shared helpers whose contracts control the changed behavior. -3. Use focused history when it can explain an invariant, revert, or prior fix attempt. -4. Describe the old and new behavior, likely motivation, affected callers/providers, and initial concerns in your own words. +3. When logic moves, compare the old and new input domains to ensure coverage is preserved. +4. Use focused history when it can explain an invariant, revert, or prior fix attempt. +5. Describe the old and new behavior, likely motivation, affected callers/providers, and initial concerns in your own words. ### 3. Reconcile Pull Request Context @@ -54,6 +55,8 @@ For each non-trivial production change, identify: Check correctness, error handling, async behavior, concurrency, state and resource lifetime, compatibility, provider hierarchy behavior, NativeAOT constraints, and performance only where the changed path plausibly makes them relevant. +For exception cleanup around shared or concurrent state, distinguish whether this invocation acquired each resource from whether the resource currently appears held. Construct a concrete interleaving and require focused coverage that exercises ownership without weakening existing reentrancy or nesting behavior. + For new functionality consider other feature areas that might be affected but are not directly touched by the change. Do not request tests for documentation-only changes, comments, formatting, or demonstrably behavior-preserving mechanical changes. Do not flag issues that the compiler, formatter, or existing analyzers deterministically report. diff --git a/.agents/skills/make-custom-agent/SKILL.md b/.agents/skills/make-custom-agent/SKILL.md index 8c77ab0fa00..f81b8455e81 100644 --- a/.agents/skills/make-custom-agent/SKILL.md +++ b/.agents/skills/make-custom-agent/SKILL.md @@ -207,7 +207,16 @@ Key considerations: - Use the [GitHub Copilot Extensions documentation](https://docs.github.com/en/copilot/building-copilot-extensions/about-building-copilot-extensions) for the full integration guide - For VS Code-specific features (editor access, file trees, command buttons), prefer an extension-based participant instead -### Step 7: Validate +### Step 7: Author the harness evaluation + +Create `eng/harness-evaluation/agents//eval.yaml` and follow `eng/harness-evaluation/README.md`. + +- Declare only the agent file in the eval's root `agent_environment.files`; the experiment removes it from the control. +- Use realistic stimuli that exercise behavior distinctive to the agent without stating the expected diagnosis or implementation in the prompt. +- Combine deterministic output checks, a focused semantic rubric, bounded resources, token grading, and a committed scoring threshold. +- Keep the eval name and directory-derived component ID identical to the agent ID. + +### Step 8: Validate After creating or modifying an agent, verify: @@ -220,6 +229,7 @@ After creating or modifying an agent, verify: - [ ] Agent does not duplicate functionality of built-in agents (`@workspace`, `@vscode`, `@terminal`) - [ ] Handoff agent names match existing agents - [ ] Agent instructions don't include secrets, tokens, or internal URLs +- [ ] The paired harness eval targets only this agent and is meant to meaningfully distinguish treatment from control ## Common Pitfalls diff --git a/.agents/skills/make-github-actions-workflow/SKILL.md b/.agents/skills/make-github-actions-workflow/SKILL.md index f56e03a43b9..961db7e39cb 100644 --- a/.agents/skills/make-github-actions-workflow/SKILL.md +++ b/.agents/skills/make-github-actions-workflow/SKILL.md @@ -106,6 +106,15 @@ Add conditions to avoid unnecessary runs: - Early `return` in scripts for PR author, draft status, etc. - Idempotency checks (e.g., hidden HTML comments as tags to prevent duplicate actions) +For `pull_request_target` policy workflows that react to `opened`, `edited`, and `reopened`: + +- On `edited`, return unless `context.payload.changes?.base` is present; title or body edits must not reapply a target-branch policy. +- Check permissions for `context.actor`, not only the PR author. A maintainer may edit or reopen an external PR, and that action must use the maintainer's authority. +- For `opened`, the actor and author are the same, so perform one permission lookup and reuse it for policy and labeling decisions. +- Exclude automation when either the actor or author is a bot. +- Encode the prohibited branch class directly (for example, `release/*`) instead of requiring a single allowed branch such as `main`. +- If permission lookup fails, stop without taking a destructive action; unknown access is not evidence that the contributor is external. + ### Step 5: Validate After creating the workflow: @@ -152,19 +161,37 @@ const { data: permissions } = await github.rest.repos.getCollaboratorPermissionL const hasWriteAccess = ['admin', 'write'].includes(permissions.permission); ``` -### Preventing Duplicate Comments (SHA Tag Pattern) +### Updating Bot-Owned Comments Idempotently + +A hidden marker identifies the workflow's comment but does not establish ownership: contributors can place the same marker in their own comments. Match the marker only on a bot-authored comment, then update that comment or create a new one. Preserve the workflow's pinned action SHA and never execute code from a pull-request fork in a `pull_request_target` job. ```javascript -const shaTag = ``; +const marker = ''; const { data: comments } = await github.rest.issues.listComments({ owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, per_page: 100 }); -if (comments.some(c => c.body?.includes(shaTag))) { - console.log('Already processed'); - return; + +const body = `${marker}\nComment text`; +const existing = comments.find(comment => + comment.user?.type === 'Bot' && comment.body?.includes(marker)); + +if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body + }); +} else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body + }); } ``` diff --git a/.agents/skills/make-skill/SKILL.md b/.agents/skills/make-skill/SKILL.md index 6bff1870284..b16f1242c80 100644 --- a/.agents/skills/make-skill/SKILL.md +++ b/.agents/skills/make-skill/SKILL.md @@ -1,139 +1,54 @@ --- name: make-skill -description: 'Create and evaluate new Agent Skills for GitHub Copilot. Use when asked to create, scaffold, or add a skill. Generates SKILL.md, optional resources, and a paired Vally harness eval.' +description: 'Create, review, or fix Agent Skills for GitHub Copilot. Use when asked to create, scaffold, improve, or repair a SKILL.md and its paired harness evaluation.' --- -# Create Skill +# Create Or Fix A Skill -This skill helps you scaffold new agent skills that conform to the Agent Skills specification. Agent Skills are a lightweight, open format for extending AI agent capabilities with specialized knowledge and workflows. +Produce concise, stable task guidance that adds knowledge an agent cannot reliably infer from source alone, together with a paired evaluation that proves the guidance changes behavior. -## When Not to Use +## Choose The Right Primitive -- Creating custom agents (use the agents/ directory pattern) -- Adding language-specific, framework-specific, or module-specific coding guidelines (use file-based instructions instead) - -### Key Principles - -- **Frontmatter is critical**: `name` and `description` determine when the skill triggers—be clear and comprehensive -- **Concise is key**: Only include what agents don't already know; context window is shared -- **Useful instructions**: Only include information that's stable, not easily searchable and can be used for any task within the skill's scope -- **No duplication**: Information lives in SKILL.md OR reference files, not both +- Use a skill for a task-specific workflow or body of knowledge loaded on demand. +- Use file instructions for background guidance scoped to paths. +- Use a custom agent when context isolation or distinct tool restrictions are required. +- Prefer an existing skill over creating an overlapping one. ## Workflow -### Step 1: Investigate the Topic - -Build deep understanding of the relevant topics using the repository content, existing documentation, and any linked external resources. - -After investigating, verify: -- [ ] Can explain what the skill does in one paragraph -- [ ] Can list 3-5 specific scenarios where the skill is applicable -- [ ] Can identify common pitfalls or misconceptions about the topic -- [ ] Can outline a step-by-step skill workflow with clear validation steps -- [ ] Have search queries for deeper topics -- [ ] Can determine if the skill should be user-invocable or background knowledge only - -If there are any ambiguities, gaps in understanding, or multiple valid approaches, ask the user for clarification before proceeding to skill creation. Also, evaluate whether the task might be better handled by a custom agent, agentic workflow, an existing skill or multiple narrower skills, and discuss this with the user if relevant. - -### Step 2: Create the skill directory - -``` -.agents/skills// -├── SKILL.md # Required: instructions + metadata -``` - -### Step 3: Generate SKILL.md with frontmatter - -Create the file with required YAML frontmatter: - -```yaml ---- -name: -description: -user-invocable: -argument-hint: -disable-model-invocation: -compatibility: -metadata: -allowed-tools: ---- -``` - -### Step 4: Add body content sections +1. Read the relevant implementation, tests, repository instructions, and neighboring customizations. +2. Identify repeated mistakes or non-obvious invariants. Do not turn source inventories, line counts, or facts easily rediscovered from code into skill content. +3. Create `.agents/skills//SKILL.md`. The frontmatter `name` must exactly match the directory; the description must state what the skill does and when natural user requests should trigger it. +4. Mark background domain guidance `user-invocable: false`. Keep user-invocable workflows focused on an outcome a user would deliberately request. +5. Write actionable guidance with observable validation. Explain why edge cases matter; avoid generic engineering advice already present in repository instructions. +6. Create `eng/harness-evaluation/skills//eval.yaml` and follow `eng/harness-evaluation/README.md`. +7. Lint the customization and inspect both control and treatment artifacts before accepting it. -Include these recommended sections, following this file's structure: +## Paired Evaluation -1. ****: One paragraph describing the outcome beyond what's already in the description -2. **When Not to Use**: Bullet list of exclusions, optional -3. **Inputs and Outputs**: Example inputs and expected outputs, if applicable -4. **Workflow**: Numbered steps with checkpoints -5. **Testing**: Instructions for how to create automated tests for the skill output, if applicable -6. **Validation**: How to confirm the skill worked correctly -7. **Common Pitfalls**: Known traps and how to avoid them, optional +- The eval root `agent_environment.skills` contains only the target skill. Stimulus inputs belong under each stimulus's `agent_environment.files`. +- The runner removes the root skill for control and separately verifies treatment activation. Do not add a `skill-invocation` grader and do not tell the agent to invoke the skill in the shared prompt. +- Prompts may identify fixtures, proposed code, fixed IDs, and output paths, but must not reveal the expected diagnosis, owning mechanism, or regression-test design. +- Use deterministic graders for observable artifacts, a narrow semantic rubric for behavior, and a `token-budget` grader. Define bounded turns, tokens, and duration plus a committed scoring threshold. +- The task must exercise guidance distinctive to the skill. A control that consistently matches treatment means the skill or eval is not useful enough. -### Step 5: Add and populate optional directories if needed +## Validation -``` -.agents/skills// -├── SKILL.md -├── scripts/ # Optional: executable code that agents can run -├── references/ # Optional: REFERENCE.md (Detailed technical reference), FORMS.md (Form templates or structured data formats), domain-specific instruction files -└── assets/ # Optional: templates, resources and other data files that aren't executable or Markdown -``` - -### 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: Author and validate the harness evaluation - -Create `eng/harness-evaluation/skills//eval.yaml` and follow the authoring and validation rules in `eng/harness-evaluation/README.md`. Do not add a `skill-invocation` grader; the runner separately requires exact invocation of `` in every treatment trial so control and treatment share the same quality score. The eval must meaningfully distinguish the skilled treatment from the unskilled control. - -Also verify: - -- [ ] The skill name does not start or end with a hyphen, contain consecutive hyphens, or exceed 64 characters -- [ ] YAML frontmatter name matches the directory name exactly and all frontmatter fields are valid -- [ ] SKILL.md is under 500 lines and 5000 tokens, splitting stable detail into references when needed -- [ ] File references are relative and instructions are actionable and specific -- [ ] Instructions do not duplicate `.github/copilot-instructions.md` or `.github/instructions/` -- [ ] The workflow has numbered steps and observable success criteria -- [ ] No secrets, tokens, or internal URLs are included -- [ ] Optional directories are used appropriately -- [ ] Scripts handle edge cases, fail closed, and return structured, helpful errors -- [ ] The paired Vally comparison demonstrates distinctive value over the unskilled control - -### 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. +- Frontmatter parses and the name matches the directory. +- The skill is concise, stable, and does not duplicate root or scoped instructions. +- File references are relative and exist. +- Validation steps are observable and task-specific. +- The paired eval targets only the skill, has complete bounded grading, and uses a solution-neutral prompt. +- `npm run lint` and the harness inventory tests pass. ## Common Pitfalls -| Pitfall | Solution | -|---------|----------| -| Description is vague | Include what it does AND when to use it | -| Instructions are ambiguous | Use numbered steps with concrete actions | -| Missing validation steps | Add checkpoints that verify success | -| Hardcoded environment assumptions | Document requirements in `compatibility` field | -| Key files section lists files previously mentioned | Avoid duplication, only include in one place and rename section to "Other Key Files" | -| Testing section lists test folders that are obvious from the repo structure | Remove the section if it doesn't add value | - -## References - -- [Agent Skills Specification](https://agentskills.io/specification) -- Repository guidance: `.github/copilot-instructions.md` and `.github/CONTRIBUTING.md` +| Pitfall | Correction | +|---|---| +| Copying implementation inventories into the skill | Keep only non-obvious invariants and routing decisions | +| Exact line counts or volatile metrics | Remove them; point to stable symbols or behavior | +| Generic test-folder guidance | Describe the distinctive test shape or omit it | +| Eval prompt tells the agent what to find | Move expected facts into the hidden rubric | +| Eval root contains repository fixtures | Keep only the target skill at root; stage fixtures per stimulus | +| Prompt instructs skill invocation | Let the treatment-only runner gate enforce activation | +| Initial scaffold launches multi-model review | First produce and lint the requested artifacts; run expensive behavioral validation separately | \ No newline at end of file diff --git a/.agents/skills/make-skill/references/anti-patterns.md b/.agents/skills/make-skill/references/anti-patterns.md deleted file mode 100644 index 03fdf5a6c96..00000000000 --- a/.agents/skills/make-skill/references/anti-patterns.md +++ /dev/null @@ -1,351 +0,0 @@ -# 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 deleted file mode 100644 index f8acfa899fd..00000000000 --- a/.agents/skills/make-skill/references/testing-patterns.md +++ /dev/null @@ -1,301 +0,0 @@ -# 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. - -## Vally Evaluation - -For repeatable, quantitative skill testing, author the repository's paired Vally eval. It provides: - -- **Structured eval suites** — define repository-grounded stimuli, expected outputs, and graders -- **Unskilled controls** — compare identical tasks with and without the target skill -- **Quality and efficiency comparison** — report judge preference and token, turn, tool-call, time, and error deltas -- **CI integration** — require the treatment threshold and fail statistically significant regressions - -Use `node eng/harness-evaluation/src/cli.mjs eval --workers 1 --require-pass` to measure whether a skill improves behavior with its configured trial count. Use multi-model review (above) for 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) -- [ ] `eng/harness-evaluation/skills//eval.yaml` passes treatment/control comparison -- [ ] 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/.agents/skills/migrations/SKILL.md b/.agents/skills/migrations/SKILL.md index c7ecd80c0e2..4670f7fd983 100644 --- a/.agents/skills/migrations/SKILL.md +++ b/.agents/skills/migrations/SKILL.md @@ -18,8 +18,12 @@ user-invocable: false - `SnapshotModelProcessor.Process()` is used at design-time to fixup older model snapshots for backward compatibility. - `MigrationsModelDiffer` uses provider-agnostic structural comparison between relational models to determine what migration operations are necessary. +For an upgrade-only regression, derive a concrete old `ModelSnapshot`, process its model with `SnapshotModelProcessor.Process(..., resetVersion: true)`, and compare it with the equivalent current design-time model through `MigrationsModelDiffer.GetDifferences()`. Preserve the property-bag CLR types emitted by old snapshots and omit APIs that would explicitly configure metadata that was absent in that version. The regression succeeds only when the resulting operation list is empty. + +Legacy snapshot normalization belongs in `SnapshotModelProcessor`, not in runtime migration application. Scope a compatibility branch to the exact released versions that could emit the affected snapshot shape; versions from before the feature existed should not be included merely because they are numerically older. Prefer the mutable metadata interface when it exposes the required setter rather than casting to a concrete metadata implementation. Add focused coverage for every version family named by the gate, plus the first unaffected version. + ## Testing Migration operation tests: `test/EFCore.Relational.Tests/Migrations/`. Functional tests: `test/EFCore.{Provider}.FunctionalTests/Migrations/`. Model differ tests: `test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest*.cs`. -To simulate a snapshot model use `ModelBuilder` calls without conventions. +To simulate a snapshot model use `ModelBuilder` calls without conventions. \ No newline at end of file diff --git a/.agents/skills/model-building/SKILL.md b/.agents/skills/model-building/SKILL.md index df2b49a9a3f..062cf76cb7c 100644 --- a/.agents/skills/model-building/SKILL.md +++ b/.agents/skills/model-building/SKILL.md @@ -20,12 +20,26 @@ Covers model construction (conventions, fluent API, metadata hierarchy) and mode Override `ConfigureConventions(ModelConfigurationBuilder)` to add/remove conventions. +Convention fixes must preserve dispatcher behavior and ambiguity information. For inverse-navigation attributes, do not solve recursion by permanently ignoring an ambiguous navigation: removing one candidate can make another ambiguity appear unique and configure the wrong relationship. `InversePropertyAttributeConvention` records inverse candidates and uses `AddInverseNavigation`, `TryRemoveIfAmbiguous`, and `FindAmbiguousInverse` while still allowing convention processing to continue. + +Test the state transitions, not only the initially ambiguous model: first verify that multiple interacting candidates configure no relationship; then genuinely remove ambiguity both by ignoring/removing one competing navigation and by removing a competing entity type, and verify that the remaining inverse relationship is configured. These cases distinguish candidate bookkeeping from a permanent-ignore workaround. + ## Metadata Interface Hierarchy `IReadOnly*` → `IMutable*` → `IConvention*` → `IRuntime*` Applies to: Model, EntityType, Property, Key, ForeignKey, Navigation, Index, etc. Builders follow: `*Builder` → `IConvention*Builder`. +## Adding A Configurable Metadata Facet + +Follow the nearest existing facet end to end rather than stopping at the public interface: + +1. Add the read-only, mutable, and convention metadata contracts, concrete storage, default, configuration source, convention setter, and builder precedence checks. Preserve the facet when metadata is replaced or a relationship is rebuilt. +2. If changes are observable to conventions, add the convention interface, every `ConventionSet` add/replace/remove path, and immediate plus delayed dispatcher scopes and nodes. +3. Carry the value into the optimized runtime metadata through `RuntimeModelConvention`, runtime constructors/builders, and `CSharpRuntimeModelCodeGenerator` so compiled models preserve it. +4. If snapshots persist the facet, update the fluent builder API and `CSharpSnapshotGenerator`; keep the old absence equivalent to the historical default. +5. Update public API baselines and cover configuration-source precedence, convention dispatch, runtime/compiled model propagation, and snapshot round trips. + ## Model Lifecycle 1. **Mutable Model** — built by `ModelBuilder` during `OnModelCreating`, made read-only by `FinalizeModel()` @@ -91,4 +105,4 @@ Inspect `CSharpSnapshotGenerator` (what gets written) and `MigrationsModelDiffer - All new API is covered by tests - Compiled model baselines update cleanly with `EF_TEST_REWRITE_BASELINES=1` - `ToString()` on metadata objects shows concise contents without throwing exceptions -- No spurious migration is generated against a project with an existing snapshot +- No spurious migration is generated against a project with an existing snapshot \ No newline at end of file diff --git a/.agents/skills/query-pipeline/SKILL.md b/.agents/skills/query-pipeline/SKILL.md deleted file mode 100644 index fdc65de0577..00000000000 --- a/.agents/skills/query-pipeline/SKILL.md +++ /dev/null @@ -1,21 +0,0 @@ ---- -name: query-pipeline -description: 'Implementation details for EF Core LINQ query translation, SQL generation, and bulk operations (ExecuteUpdate/ExecuteDelete). Use when changing expression visitors, SqlExpressions, QuerySqlGenerator, ShaperProcessingExpressionVisitor, UpdateExpression, DeleteExpression, or related classes.' -user-invocable: false ---- - -# Query Pipeline - -## Stages - -1. **Preprocessing** -2. **Translation** -3. **Postprocessing** -4. **Compilation** -5. **SQL Generation** - -## Validation - -- `ToQueryString()` shows generated SQL without executing -- `ExpressionPrinter` dumps expression trees at any pipeline stage -- SQL baselines verified via `AssertSql()` in provider functional tests and the generated SQL corresponds to the LINQ query in the base method diff --git a/.agents/skills/scaffolding/SKILL.md b/.agents/skills/scaffolding/SKILL.md index fcf6b74fbae..b0707232120 100644 --- a/.agents/skills/scaffolding/SKILL.md +++ b/.agents/skills/scaffolding/SKILL.md @@ -16,3 +16,7 @@ Generates C# code from database schemas (reverse engineering). Pipeline: `IDatabaseModelFactory` (reads schema) → `IScaffoldingModelFactory` (builds EF model) → `IModelCodeGenerator` (generates C#) - `IReverseEngineerScaffolder` — orchestrates full pipeline + +## Schema Query Compatibility + +Provider schema readers often query one-to-many metadata tables. Preserve anti-semi semantics when adapting SQL for a limited engine: replacing `NOT EXISTS` with a `LEFT JOIN` plus a null filter is incorrect when an object can have multiple metadata rows, even when the excluded-property predicate is in the join condition. The excluded row does not join but an unrelated row does; its non-null joined key keeps the object in the result. Prefer an equivalent non-correlated anti-set form, such as `NOT IN (SELECT key ... WHERE excluded-condition)`, when nullability and key semantics permit it. diff --git a/.agents/skills/sqlite-adonet/SKILL.md b/.agents/skills/sqlite-adonet/SKILL.md index 3e3f977cb72..ef6d7d07ae8 100644 --- a/.agents/skills/sqlite-adonet/SKILL.md +++ b/.agents/skills/sqlite-adonet/SKILL.md @@ -11,4 +11,16 @@ Standalone ADO.NET provider in `src/Microsoft.Data.Sqlite.Core/`, independent of ## Notable Implementation Details - Static constructor calls `SQLitePCL.Batteries_V2.Init()` reflectively -- `CreateFunction()`/`CreateAggregate()` overloads generated from T4 templates (`.tt` files) \ No newline at end of file +- `CreateFunction()`/`CreateAggregate()` overloads generated from T4 templates (`.tt` files) + +## Command Lifecycle + +`SqliteConnection` tracks associated commands as weak references. Closing an open connection disposes every live tracked command before closing the native connection; command disposal releases prepared statements and unregisters the command. Diagnose close/reopen reports from this ownership path and the `System.Data.Common` contract, not from EF Core provider abstractions. + +Closing does not erase the command's managed configuration or connection reference. If the same command object is used after reopening its connection, execution prepares fresh native statements against the new handle. Add a regression test only when the reported behavior remains reachable after following this lifecycle. + +## Transaction Completion + +A failed `COMMIT` can leave the SQLite transaction active and retryable. Do not call `Complete()` or add transaction-level retries; `SqliteCommand` already retries busy/locked results through the connection's `ExecuteNonQuery` path. + +Rollback during disposal must detach the managed transaction even when rollback throws, so keep `Complete()` in its `finally`. Test failed commit separately from failed `Dispose()`/`DisposeAsync()`: preserve the original exception and verify disposal permits a subsequent transaction. \ No newline at end of file diff --git a/.agents/skills/tooling/SKILL.md b/.agents/skills/tooling/SKILL.md index 4b642c444ff..b1d416eb21c 100644 --- a/.agents/skills/tooling/SKILL.md +++ b/.agents/skills/tooling/SKILL.md @@ -20,6 +20,14 @@ PowerShell module: `Add-Migration`, `Update-Database`, `Scaffold-DbContext`, `Op NuGet package `Microsoft.EntityFrameworkCore.Tasks` provides build/publish-time compiled model and precompiled query generation. Targets in `buildTransitive/Microsoft.EntityFrameworkCore.Tasks.targets`. +## Command Option Ownership + +Trace an option to the code that consumes it before changing command or operation contracts. The outer `dotnet-ef` launcher builds the project and forwards remaining command-line arguments to `ef.dll`; the inner command under `src/ef/Commands/` parses command-specific options. + +- Keep presentation-only behavior in the inner command. For example, suppressing the completion message from `MigrationsAddCommand.Execute()` should guard its `Reporter.WriteInformation()` call and must not change `IOperationExecutor.AddMigration()`. +- Forward a value through `OperationExecutorBase` only when the design-time operation in `Microsoft.EntityFrameworkCore.Design` needs it. +- Treat PMC and `EFCore.Tasks` as separate entry points. Change them only when the requested behavior applies to those surfaces too. + ## Testing - CLI tests: `test/dotnet-ef.Tests/`, `test/ef.Tests/` diff --git a/.agents/skills/update-pipeline/SKILL.md b/.agents/skills/update-pipeline/SKILL.md index cf5181e671c..b21044f7f78 100644 --- a/.agents/skills/update-pipeline/SKILL.md +++ b/.agents/skills/update-pipeline/SKILL.md @@ -21,6 +21,10 @@ Converts tracked entity changes into database INSERT/UPDATE/DELETE commands duri Concurrency tokens → WHERE conditions on UPDATE/DELETE. `AffectedCountModificationCommandBatch` checks affected rows. Throws `DbUpdateConcurrencyException` on mismatch. +## Shared-Row Value Access + +Column accessors may need values from a `SharedIdentityEntry` when one CLR entry replaces another mapped to the same row. Select the effective entry before resolving the column's property mapping: entries in a shared-identity pair can have different entity types, so a property from the original type must not be used against the replacement. Regression tests should exercise the resulting command dependency ordering, not just the accessor in isolation. + ## Validation - `SaveChanges()` returns expected affected row count diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 09445b3b67a..a56d669b23f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -29,12 +29,14 @@ Set `EF_TEST_REWRITE_BASELINES=1` to rewrite SQL and compiled-model baselines. P ## Repository conventions -- Put provider-independent behavior in core or relational specification tests, then override new virtual tests in inheriting provider classes, adding specific assertions such as `AssertSql` for providers that produce SQL. Preserve existing `Check_all_tests_overridden` guards, but do not add one to a provider class that intentionally overrides only a subset of tests. +- Put provider-independent behavior in core or relational specification tests, then override new virtual tests in inheriting provider classes, adding specific assertions such as `AssertSql` for providers that produce SQL. Preserve existing `Check_all_tests_overridden` guards. A new guard may be added only if it doesn't force many redundant overrides. - Prefer existing test infrastructure: `TestHelpers` for services/models, `NonSharedModelTestBase` for both the tests that share a model as well as those that do not. +- Group specification tests by a cohesive query or model trait. Do not create a generic bucket merely because tests use custom models; choose a trait-specific base such as owned-entity queries so the suite remains navigable as regressions accumulate. - Preserve public API and binary compatibility. Prefer overloads over changing shipped signatures. If you need to break a public API, add a new API instead and mark the old one as obsolete. Use `ObsoleteAttribute` with the message pointing to the new API - Types are public by default. Types under `.Internal` or marked `[EntityFrameworkInternal]` must use the repository's internal-API XML documentation pattern on all members and they don't need to preserve compatibility. - User-facing messages come from the owning project's `.resx` resource and generated `*Strings.Designer.cs`. -- Configure asynchronous calls using `ConfigureAwait(false)`. Avoid reflection or runtime code generation where NativeAOT-compatible alternatives exist; otherwise use the established annotations/guards. +- Configure asynchronous calls using `ConfigureAwait(false)`. +- For APIs that activate or construct types, plan NativeAOT-safe activation explicitly; prefer strongly typed factories or generic construction over reflection. When reflection or runtime generation is unavoidable, use the established annotations and guards. - Package versions belong in `eng\Versions.props` or `Directory.Packages.props`, never inline in project files. - Do not edit `eng\common`; it is mirrored from dotnet/arcade and overwritten by automation. - Follow `.editorconfig` for formatting. diff --git a/eng/harness-evaluation/README.md b/eng/harness-evaluation/README.md index 39dddcd8357..2688a309167 100644 --- a/eng/harness-evaluation/README.md +++ b/eng/harness-evaluation/README.md @@ -23,6 +23,7 @@ Every stimulus must: 3. Use stimulus-level `agent_environment.files` when repository inputs are needed. `src` is relative to the eval file; `dest` is relative to the isolated workspace. 4. Add a `token-budget` grader to every stimulus and define its weight in `scoring.weights`. 5. Use deterministic graders where possible and a narrow `prompt` rubric only for semantic quality. +6. Present a realistic task without disclosing its solution. Prompts may name input files, proposed code or diffs, fixed artifact IDs, and required output paths, but must not state the expected diagnosis, owning symbol or stage, implementation mechanism, or regression-test design. Do not tell the agent to invoke the evaluated skill; treatment activation is runner-owned. Keep expected facts in the rubric and grade observable output that the prompt did not supply. The runner passes declared input files from the evaluated commit to Vally, which validates and stages them in an isolated workspace. Inputs are optional. @@ -97,6 +98,8 @@ The runner defaults to one active trial per component. GitHub Actions paralleliz Before accepting a new or materially changed eval, inspect both arms and `comparison.jsonl`. A control that consistently matches or beats the treatment means the eval is not discriminating enough. +Repository skills must demonstrate comparative value, not merely pass their treatment rubric. If a skill consistently fails five-run comparison evals and can't be reasonably improved, remove both the skill directory rather than lowering thresholds, weights, or semantic requirements. Move its paired eval stimuli to `eng\harness-evaluation\instructions\copilot-instructions\`. + ## Adding components - **Skill:** add `.agents/skills//SKILL.md` and `eng/harness-evaluation/skills//eval.yaml`. diff --git a/eng/harness-evaluation/instructions/copilot-instructions/eval.yaml b/eng/harness-evaluation/instructions/copilot-instructions/eval.yaml index 6822b83d060..8f289c6b9a2 100644 --- a/eng/harness-evaluation/instructions/copilot-instructions/eval.yaml +++ b/eng/harness-evaluation/instructions/copilot-instructions/eval.yaml @@ -1,5 +1,5 @@ name: copilot-instructions -description: Evaluates repository-wide Copilot guidance for EF Core implementation conventions and provider test hierarchies. +description: Evaluates repository-wide Copilot guidance for EF Core implementation conventions, provider test hierarchies, and query-pipeline review. type: capability defaults: runs: ${RUNS=5} @@ -12,32 +12,28 @@ agent_environment: - src: ../../../../.github/copilot-instructions.md dest: .github/copilot-instructions.md stimuli: - - name: add-a-query-test-through-provider-hierarchies + - name: move-provider-query-regression-to-specifications prompt: |- - Inspect test/EFCore.Specification.Tests/Query/QueryTestBase.cs, test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs, test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs, and test/EFCore.Sqlite.FunctionalTests/Query/NorthwindWhereQuerySqliteTest.cs. Review a proposal that adds Where_with_parameterized_limit only to the SQL Server class, puts AssertSql in the specification base, copies a separate test into SQLite, and adds a test override that only calls base. Write test-plan.md with the corrected implementation and focused validation plan. Do not edit source. + Inspect test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs and test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs. A proposed fix for nullable-to-non-nullable conversion adds `Convert_to_nullable_on_nullable_value_is_ignored` only to the SQL Server class, where it executes the LINQ query and asserts the result. Write test-plan.md reviewing the test placement and validation. Do not edit source. constraints: max_turns: 40 max_tokens: 750000 max_duration: 5m agent_environment: files: - - src: ../../../../test/EFCore.Specification.Tests/Query/QueryTestBase.cs - dest: test/EFCore.Specification.Tests/Query/QueryTestBase.cs - - src: ../../../../test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs - dest: test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs - - src: ../../../../test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs - dest: test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs - - src: ../../../../test/EFCore.Sqlite.FunctionalTests/Query/NorthwindWhereQuerySqliteTest.cs - dest: test/EFCore.Sqlite.FunctionalTests/Query/NorthwindWhereQuerySqliteTest.cs + - src: ../../../../test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs + dest: test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs + - src: ../../../../test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs + dest: test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs rubric: - - The plan moves provider-independent behavior and result assertions into the specification base rather than duplicating tests, and identifies both staged provider overrides. - - SQL Server and SQLite overrides call the base test before their own AssertSql baselines; the existing SQL Server Check_all_tests_overridden guard remains covered without adding an exhaustive guard to the intentionally selective SQLite class. - - The commands restore and activate the repository SDK, then build or run focused tests for each provider without relying on stale --no-build output. + - The plan moves the LINQ query and result assertion into a virtual test on NorthwindMiscellaneousQueryTestBase so all applicable providers inherit the behavior. + - The SQL Server override calls the base test and contains only its provider SQL baseline through AssertSql. + - The validation runs the shared behavior and SQL Server baseline with focused Microsoft Testing Platform filters and does not rely on stale --no-build output. graders: - type: file-matches config: path: test-plan.md - pattern: (?i)(?=[\s\S]*Where_with_parameterized_limit)(?=[\s\S]*(NorthwindWhereQueryTestBase|specification base))(?=[\s\S]*AssertSql)(?=[\s\S]*SqlServer)(?=[\s\S]*Sqlite)[\s\S]+ + pattern: (?i)(?=[\s\S]*NorthwindMiscellaneousQueryTestBase)(?=[\s\S]*AssertSql)(?=[\s\S]*(shared|specification|provider.independent))[\s\S]+ - type: prompt config: scoring: binary @@ -48,7 +44,22 @@ stimuli: max: 750000 - name: plan-a-provider-independent-async-api prompt: |- - Inspect src/EFCore/DbContext.cs, src/EFCore/Properties/CoreStrings.resx and src/EFCore/EFCore.csproj. Review a proposal for a DbContext async API used by all providers: it is placed in EFCore.Relational, takes `bool isEnabled`, activates a CLR Type with reflection, awaits a callback normally, throws a hard-coded English error, hand-edits CoreStrings.Designer.cs, adds an inline package version to EFCore.csproj, and puts its build hook under eng/common. Write implementation-plan.md with the corrected implementation and focused validation plan. Do not edit source. + Inspect src/EFCore/DbContext.cs and src/EFCore/Properties/CoreStrings.resx. Review this proposed provider-independent DbContext member and write implementation-plan.md with the corrected API, implementation, and focused validation. Do not edit source. + + public virtual async Task ActivateAsync( + Type type, + bool isEnabled, + Func callback) + { + if (!isEnabled) + { + throw new InvalidOperationException("Activation is disabled."); + } + + var instance = Activator.CreateInstance(type)!; + await callback(instance); + return instance; + } constraints: max_turns: 50 max_tokens: 1000000 @@ -59,13 +70,11 @@ stimuli: dest: src/EFCore/DbContext.cs - src: ../../../../src/EFCore/Properties/CoreStrings.resx dest: src/EFCore/Properties/CoreStrings.resx - - src: ../../../../src/EFCore/EFCore.csproj - dest: src/EFCore/EFCore.csproj rubric: - - The plan places provider-independent behavior in EFCore, renames the bool parameter without an is/are prefix, and preserves public API compatibility with complete XML documentation. + - The plan places provider-independent behavior in EFCore, renames the bool parameter without an is/are prefix, treats the supplied member as an unshipped proposal whose API shape may be corrected, and includes complete XML documentation. - It avoids reflection/runtime generation where possible or applies appropriate NativeAOT annotations, and requires ConfigureAwait(false) on the callback. - - It adds the error to CoreStrings.resx, regenerates rather than hand-edits the designer, and centralizes the package version in eng/Versions.props or Directory.Packages.props. - - It places repository build logic outside mirrored eng/common and includes focused behavior plus EFCore.ApiBaseline.Tests validation. + - It adds the error to CoreStrings.resx and regenerates rather than hand-edits the designer. + - It includes focused enabled/disabled, activation-failure, callback, and API-baseline validation. graders: - type: file-matches config: @@ -79,9 +88,9 @@ stimuli: - type: token-budget config: max: 1000000 - - name: design-a-nonshared-provider-query-regression + - name: place-custom-model-query-regression-by-trait prompt: |- - Inspect test/EFCore.Specification.Tests/NonSharedModelTestBase.cs, test/EFCore.Specification.Tests/Query/AdHocMiscellaneousQueryTestBase.cs, test/EFCore.SqlServer.FunctionalTests/Query/AdHocMiscellaneousQuerySqlServerTest.cs, and test/EFCore.Sqlite.FunctionalTests/Query/AdHocMiscellaneousQuerySqliteTest.cs. Review a regression-test proposal that needs a dynamically configured model and runs for SQL Server and SQLite. It creates a standalone xUnit class with two tests, builds services and models manually, and copies the full tests into both providers. Write regression-plan.md with the corrected implementation and focused validation plan. Do not edit source. + Inspect test/EFCore.Specification.Tests/NonSharedModelTestBase.cs, test/EFCore.Specification.Tests/Query/OwnedEntityQueryTestBase.cs, test/EFCore.SqlServer.FunctionalTests/Query/OwnedEntityQuerySqlServerTest.cs, and test/EFCore.Sqlite.FunctionalTests/Query/OwnedEntityQuerySqliteTest.cs. A proposed regression test queries projections containing owned types and introduces `NonSharedModelQueryTestBase`, derived from `NonSharedModelTestBase`, with provider subclasses for SQL Server and SQLite. Write regression-plan.md reviewing the proposed hierarchy, test ownership, and focused validation. Do not edit source. constraints: max_turns: 60 max_tokens: 1200000 @@ -90,21 +99,21 @@ stimuli: files: - src: ../../../../test/EFCore.Specification.Tests/NonSharedModelTestBase.cs dest: test/EFCore.Specification.Tests/NonSharedModelTestBase.cs - - src: ../../../../test/EFCore.Specification.Tests/Query/AdHocMiscellaneousQueryTestBase.cs - dest: test/EFCore.Specification.Tests/Query/AdHocMiscellaneousQueryTestBase.cs - - src: ../../../../test/EFCore.SqlServer.FunctionalTests/Query/AdHocMiscellaneousQuerySqlServerTest.cs - dest: test/EFCore.SqlServer.FunctionalTests/Query/AdHocMiscellaneousQuerySqlServerTest.cs - - src: ../../../../test/EFCore.Sqlite.FunctionalTests/Query/AdHocMiscellaneousQuerySqliteTest.cs - dest: test/EFCore.Sqlite.FunctionalTests/Query/AdHocMiscellaneousQuerySqliteTest.cs + - src: ../../../../test/EFCore.Specification.Tests/Query/OwnedEntityQueryTestBase.cs + dest: test/EFCore.Specification.Tests/Query/OwnedEntityQueryTestBase.cs + - src: ../../../../test/EFCore.SqlServer.FunctionalTests/Query/OwnedEntityQuerySqlServerTest.cs + dest: test/EFCore.SqlServer.FunctionalTests/Query/OwnedEntityQuerySqlServerTest.cs + - src: ../../../../test/EFCore.Sqlite.FunctionalTests/Query/OwnedEntityQuerySqliteTest.cs + dest: test/EFCore.Sqlite.FunctionalTests/Query/OwnedEntityQuerySqliteTest.cs rubric: - - The plan uses NonSharedModelTestBase and its initialization/context factory infrastructure rather than manual service/model construction. - - Provider-independent behavior lives in the staged specification base; SQL Server and SQLite overrides call base and use provider-local AssertSql without copying behavior. - - The plan uses existing TestHelpers/fixture infrastructure, preserves the existing SQL Server Check_all_tests_overridden coverage without adding an exhaustive guard to the intentionally selective SQLite class, and validates both providers with repository setup and focused Microsoft Testing Platform commands. + - The plan rejects an unbounded generic custom-model query bucket and places the regression in the cohesive OwnedEntityQueryTestBase model-trait hierarchy. + - The specification base derives from NonSharedModelTestBase and owns model setup plus provider-independent assertions; provider classes inherit it, with SQL baselines remaining provider-local. + - The plan validates the shared owned-entity test through focused provider commands and does not duplicate its behavior in provider classes. graders: - type: file-matches config: path: regression-plan.md - pattern: (?i)(?=[\s\S]*(NonSharedModelTestBase|InitializeNonSharedTest))(?=[\s\S]*AdHocMiscellaneousQueryTestBase)(?=[\s\S]*AssertSql)(?=[\s\S]*SqlServer)(?=[\s\S]*Sqlite)[\s\S]+ + pattern: (?i)(?=[\s\S]*OwnedEntityQueryTestBase)(?=[\s\S]*NonSharedModelTestBase)(?=[\s\S]*(cohesive|model trait|owned entit))(?=[\s\S]*(SqlServer|SQLite))[\s\S]+ - type: prompt config: scoring: binary @@ -113,6 +122,84 @@ stimuli: - type: token-budget config: max: 1200000 + - name: review-projection-retry-change + prompt: |- + Inspect the staged projection-retry.cs snapshot. Write analysis.md reviewing the implementation and focused validation. Do not edit source. + constraints: &query_review_constraints + max_turns: 35 + max_tokens: 600000 + max_duration: 10m + agent_environment: + files: + - src: fixtures/query-pipeline/projection-retry.cs + dest: projection-retry.cs + rubric: + - The analysis identifies that TryTranslateToServerProjection does not assign `_rootIsTransparentIdentifier` even though the reused visitor's normal Translate entry point does, so state from a prior translation can affect gating and marker remapping. + - It requires initializing `_rootIsTransparentIdentifier` with IsTransparentIdentifierProjection(expression) for every retry invocation before Visit. + - Focused validation exercises retry after a prior transparent-identifier-rooted translation and verifies both transparent and non-transparent retry inputs. + graders: + - type: file-matches + config: + path: analysis.md + pattern: (?i)(?=[\s\S]*_rootIsTransparentIdentifier)(?=[\s\S]*TryTranslateToServerProjection)(?=[\s\S]*IsTransparentIdentifierProjection)(?=[\s\S]*(reuse|stale|previous))[\s\S]+ + - type: prompt + config: + scoring: binary + threshold: 1.0 + evidence: [ trajectory, repo ] + - type: token-budget + config: + max: 600000 + - name: review-split-query-count-change + prompt: |- + Inspect the staged fallback-and-tests.cs snapshot. Write analysis.md reviewing the implementation and focused validation. Do not edit source. + constraints: *query_review_constraints + agent_environment: + files: + - src: fixtures/query-pipeline/fallback-and-tests.cs + dest: fallback-and-tests.cs + rubric: + - The analysis rejects the bare catch because it converts unexpected translation defects into a silent streaming fallback and requires fallback to handle only established unsupported-shape failures. + - It identifies that the static factory and mutation flags are shared process-wide across parallel InlineData rows and inheriting provider suites, and requires test-scoped or async-flow-scoped coordination. + - Focused validation proves expected fallback and unexpected exception propagation while running sync and async concurrency cases without cross-test state interference. + graders: + - type: file-matches + config: + path: analysis.md + pattern: (?i)(?=[\s\S]*(bare catch|catch all|unexpected.*exception))(?=[\s\S]*(static|process.wide|shared mutable))(?=[\s\S]*(parallel|AsyncLocal|test.scoped|isolat))[\s\S]+ + - type: prompt + config: + scoring: binary + threshold: 1.0 + evidence: [ trajectory, repo ] + - type: token-budget + config: + max: 600000 + - name: review-split-query-consumers + prompt: |- + Inspect the staged split-enumerables.cs snapshot. Write analysis.md reviewing the implementation and focused validation. Do not edit source. + constraints: *query_review_constraints + agent_environment: + files: + - src: fixtures/query-pipeline/split-enumerables.cs + dest: split-enumerables.cs + rubric: + - The analysis identifies that both enumerable types consume SplitQueryResultCoordinator but only SplitQueryingEnumerable verifies orphaned child rows when outer enumeration completes. + - It requires VerifyNoOrphanedChildRows in the synchronous and asynchronous GroupBySplitQueryingEnumerable paths when the outer stream is exhausted, not merely at an individual group boundary. + - Focused validation covers grouped split queries with orphaned rows in both sync and async execution while preserving ordinary multi-group iteration. + graders: + - type: file-matches + config: + path: analysis.md + pattern: (?i)(?=[\s\S]*GroupBySplitQueryingEnumerable)(?=[\s\S]*VerifyNoOrphanedChildRows)(?=[\s\S]*(sync|synchronous))(?=[\s\S]*(async|asynchronous))(?=[\s\S]*(end|exhaust|outer stream))[\s\S]+ + - type: prompt + config: + scoring: binary + threshold: 1.0 + evidence: [ trajectory, repo ] + - type: token-budget + config: + max: 600000 scoring: weights: file-matches: 0.2 diff --git a/eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/fallback-and-tests.cs b/eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/fallback-and-tests.cs new file mode 100644 index 00000000000..87a969ee065 --- /dev/null +++ b/eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/fallback-and-tests.cs @@ -0,0 +1,82 @@ +static Expression CreateChildCountAccessor( + SelectExpression parentSelectExpression, + List clientProjectionList, + SelectExpression childCountSelectExpression, + ISqlExpressionFactory sqlExpressionFactory, + IAggregateMethodCallTranslatorProvider aggregateMethodCallTranslatorProvider, + QueryCompilationContext queryCompilationContext) +{ + try + { + if (childCountSelectExpression.Limit != null + || childCountSelectExpression.Offset != null + || childCountSelectExpression.IsDistinct + || childCountSelectExpression.GroupBy.Count > 0) + { + childCountSelectExpression.PushdownIntoSubqueryInternal(); + } + + childCountSelectExpression.ClearOrdering(); + var countTranslation = aggregateMethodCallTranslatorProvider.Translate( + queryCompilationContext.Model, + QueryableMethods.CountWithoutPredicate, + new EnumerableExpression(sqlExpressionFactory.Fragment("*")), + arguments: [], + queryCompilationContext.Logger) + ?? throw new UnreachableException("The Count aggregate method must always be translatable."); + childCountSelectExpression.ReplaceProjection( + new List { sqlExpressionFactory.ApplyDefaultTypeMapping(countTranslation) }); + childCountSelectExpression.ApplyProjection(); + } + catch + { + return Constant(-1); + } + + var countIndex = parentSelectExpression.AddToProjection( + new ScalarSubqueryExpression(childCountSelectExpression)); + return new ProjectionBindingExpression(parentSelectExpression, countIndex, typeof(int)); +} + +public abstract class AdHocQuerySplittingQueryTestBase +{ + [Theory] + [InlineData(true)] + [InlineData(false)] + public virtual async Task Split_include_collection_not_dropped_when_unrelated_entity_inserted_concurrently(bool async) + { + var contextFactory = await InitializeNonSharedTest(); + Context33826.ConcurrentContextFactory = contextFactory.CreateDbContext; + try + { + Context33826.InsertConcurrentEntity = true; + using var context = contextFactory.CreateDbContext(); + var query = context.Blogs.Include(b => b.Posts).AsSplitQuery(); + var blogs = async ? await query.ToListAsync() : query.ToList(); + Assert.Equal(2, blogs.Count); + } + finally + { + Context33826.InsertConcurrentEntity = false; + Context33826.ConcurrentContextFactory = null; + } + } + + protected class Context33826(DbContextOptions options) : DbContext(options) + { + public static Func ConcurrentContextFactory { get; set; } + public static bool InsertConcurrentEntity { get; set; } + public static bool DeleteOtherParentsChildren { get; set; } + + private Blog33826(int id, int secondId) + { + if (id == 20 && secondId == 1 && Context33826.InsertConcurrentEntity) + { + Context33826.InsertConcurrentEntity = false; + using var context = Context33826.ConcurrentContextFactory(); + context.Blogs.Add(new Blog33826(15, 3, [])); + context.SaveChanges(); + } + } + } +} \ No newline at end of file diff --git a/eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/projection-retry.cs b/eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/projection-retry.cs new file mode 100644 index 00000000000..0430bbe7d28 --- /dev/null +++ b/eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/projection-retry.cs @@ -0,0 +1,92 @@ +public class RelationalProjectionBindingExpressionVisitor : ExpressionVisitor +{ + private bool _indexBasedBinding; + private bool _rootIsTransparentIdentifier; + private readonly Stack _projectionMembers = new(); + private readonly Dictionary _projectionMapping = []; + private SelectExpression _selectExpression = null!; + + public virtual Expression Translate(SelectExpression selectExpression, Expression expression) + { + _selectExpression = selectExpression; + _indexBasedBinding = false; + _rootIsTransparentIdentifier = IsTransparentIdentifierProjection(expression); + _projectionMembers.Push(new ProjectionMember()); + + var result = Visit(expression); + if (result == QueryCompilationContext.NotTranslatedExpression) + { + _indexBasedBinding = true; + result = Visit(expression); + } + + _selectExpression = null!; + _projectionMembers.Clear(); + return MatchTypes(result, expression.Type); + } + + internal virtual Expression? TryTranslateToServerProjection( + SelectExpression selectExpression, + Expression expression) + { + _selectExpression = selectExpression; + _indexBasedBinding = false; + _projectionMembers.Push(new ProjectionMember()); + + expression = new MarkerNullCheckSimplifyingExpressionVisitor().Visit(expression); + var result = Visit(expression); + if (result == QueryCompilationContext.NotTranslatedExpression) + { + result = null; + } + else + { + _selectExpression.ReplaceProjection(_projectionMapping); + result = MatchTypes(result, expression.Type); + } + + _selectExpression = null!; + _projectionMapping.Clear(); + _projectionMembers.Clear(); + return result; + } + + protected override Expression VisitNew(NewExpression newExpression) + { + var hasNullabilityMarker = _selectExpression.TryGetNonEntityNullabilityMarker( + newExpression, + out var markerBinding); + var visited = newExpression.Update(newExpression.Arguments.Select(Visit)); + + if (hasNullabilityMarker && _rootIsTransparentIdentifier) + { + var reboundMarker = BindNullabilityMarker(markerBinding!); + _selectExpression.RemapNonEntityNullabilityMarker(newExpression, visited, reboundMarker); + } + + return hasNullabilityMarker + ? GateNonEntityOnNullabilityMarker(visited, markerBinding!, newExpression.Type) + : visited; + } + + private static bool IsTransparentIdentifierProjection(Expression expression) + => expression is NewExpression newExpression + && TransparentIdentifierFactory.IsTransparentIdentifierType(newExpression.Type); + + private Expression GateNonEntityOnNullabilityMarker( + Expression visited, + Expression markerBinding, + Type objectType) + { + if (_rootIsTransparentIdentifier) + { + return visited; + } + + var boundMarker = BindNullabilityMarker(markerBinding); + return Expression.Condition( + Expression.Equal(boundMarker, Expression.Constant(null, boundMarker.Type)), + Expression.Default(objectType), + visited); + } +} \ No newline at end of file diff --git a/eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/split-enumerables.cs b/eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/split-enumerables.cs new file mode 100644 index 00000000000..77bfc4cd75e --- /dev/null +++ b/eng/harness-evaluation/instructions/copilot-instructions/fixtures/query-pipeline/split-enumerables.cs @@ -0,0 +1,137 @@ +public class SplitQueryingEnumerable +{ + private sealed class Enumerator + { + public bool MoveNext() + { + var hasNext = _dataReader!.Read(); + if (hasNext) + { + Current = _shaper(_relationalQueryContext, _dbDataReader!, _resultCoordinator); + _relatedDataLoaders?.Invoke( + _relationalQueryContext, + _relationalQueryContext.ExecutionStrategy, + _resultCoordinator!); + } + else + { + _resultCoordinator!.VerifyNoOrphanedChildRows(); + Current = default!; + } + + return hasNext; + } + } + + private sealed class AsyncEnumerator + { + public async ValueTask MoveNextAsync() + { + var hasNext = await _dataReader!.ReadAsync(_cancellationToken).ConfigureAwait(false); + if (hasNext) + { + Current = _shaper(_relationalQueryContext, _dbDataReader!, _resultCoordinator); + if (_relatedDataLoaders != null) + { + await _relatedDataLoaders( + _relationalQueryContext, + _relationalQueryContext.ExecutionStrategy, + _resultCoordinator!) + .ConfigureAwait(false); + } + } + else + { + _resultCoordinator!.VerifyNoOrphanedChildRows(); + Current = default!; + } + + return hasNext; + } + } +} + +public class GroupBySplitQueryingEnumerable +{ + private sealed class Enumerator + { + public bool MoveNext() + { + var hasNext = _resultCoordinator!.HasNext ?? _dataReader!.Read(); + if (hasNext) + { + var group = new InternalGrouping(_keySelector(_relationalQueryContext, _dbDataReader!)); + do + { + var element = _elementSelector( + _relationalQueryContext, + _dbDataReader!, + _resultCoordinator.ResultContext, + _resultCoordinator); + _relatedDataLoaders?.Invoke( + _relationalQueryContext, + _relationalQueryContext.ExecutionStrategy, + _resultCoordinator); + group.Add(element); + + if (!(_resultCoordinator.HasNext ?? _dbDataReader!.Read())) + { + Current = group; + break; + } + } + while (true); + } + else + { + Current = default!; + } + + return hasNext; + } + } + + private sealed class AsyncEnumerator + { + public async ValueTask MoveNextAsync() + { + var hasNext = _resultCoordinator!.HasNext + ?? await _dataReader!.ReadAsync(_cancellationToken).ConfigureAwait(false); + if (hasNext) + { + var group = new InternalGrouping(_keySelector(_relationalQueryContext, _dbDataReader!)); + do + { + var element = _elementSelector( + _relationalQueryContext, + _dbDataReader!, + _resultCoordinator.ResultContext, + _resultCoordinator); + if (_relatedDataLoaders != null) + { + await _relatedDataLoaders( + _relationalQueryContext, + _relationalQueryContext.ExecutionStrategy, + _resultCoordinator) + .ConfigureAwait(false); + } + group.Add(element); + + if (!(_resultCoordinator.HasNext + ?? await _dbDataReader!.ReadAsync(_cancellationToken).ConfigureAwait(false))) + { + Current = group; + break; + } + } + while (true); + } + else + { + Current = default!; + } + + return hasNext; + } + } +} \ No newline at end of file diff --git a/eng/harness-evaluation/skills/change-tracking/eval.yaml b/eng/harness-evaluation/skills/change-tracking/eval.yaml index 5aaa1227436..e4f1045c936 100644 --- a/eng/harness-evaluation/skills/change-tracking/eval.yaml +++ b/eng/harness-evaluation/skills/change-tracking/eval.yaml @@ -11,30 +11,32 @@ agent_environment: skills: - ../../../../.agents/skills/change-tracking stimuli: - - name: diagnose-temporary-value-snapshot-transition - prompt: >- - Before analyzing files, use the available repository skill that applies to this task. Inspect src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs, src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs, and test/EFCore.Tests/ChangeTracking/TemporaryValuesTest.cs. A proposed change causes a temporary property to lose its modified state when an Added entry becomes Modified. Write analysis.md identifying the controlling code path, the shared-identity edge case that must remain safe, and the focused regression test you would add. Do not edit source. + - name: diagnose-replaced-owned-graph-corruption + prompt: |- + Inspect the staged change-tracking implementation and JSON update tests. Replacing a JSON-owned root with a new instance that reuses its nested leaf persists correctly, but after SaveChanges the replacement graph's leaf is null in memory. + + A proposed fix makes `EntityGraphAttacher` process every entry in `Deleted` state and makes `StateManager.CascadeDelete` return early whenever the entry is `Deleted`. Write analysis.md with the corrected implementation and focused regression-test plan. Do not edit source. constraints: max_turns: 80 max_tokens: 2000000 max_duration: 30m agent_environment: files: - - src: ../../../../src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs - dest: src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs - - src: ../../../../src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs - dest: src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs - - src: ../../../../test/EFCore.Tests/ChangeTracking/TemporaryValuesTest.cs - dest: test/EFCore.Tests/ChangeTracking/TemporaryValuesTest.cs + - src: ../../../../src/EFCore/ChangeTracking/Internal/EntityGraphAttacher.cs + dest: src/EFCore/ChangeTracking/Internal/EntityGraphAttacher.cs + - src: ../../../../src/EFCore/ChangeTracking/Internal/StateManager.cs + dest: src/EFCore/ChangeTracking/Internal/StateManager.cs + - src: ../../../../test/EFCore.Relational.Specification.Tests/Update/JsonUpdateTestBase.cs + dest: test/EFCore.Relational.Specification.Tests/Update/JsonUpdateTestBase.cs rubric: - - The analysis distinguishes snapshot comparison from state-transition handling in InternalEntityEntry. - - It explicitly considers SharedIdentityEntry when Added and Deleted entries can represent the same identity. - - The proposed test is located in TemporaryValuesTest.cs and verifies observable property/state behavior. + - The analysis uses `SharedIdentityEntry`, not broad Added/Deleted state checks, to identify an old entry and its replacement. + - It allows a deleted nested entry through EntityGraphAttacher only when the graph source has a shared-identity replacement, and skips cascading from the replaced old principal; synchronous and asynchronous paths remain equivalent. + - The regression replaces the root while reusing a nested reference, preserves unrelated branch values, and verifies the same nested object/value remains intact in memory after SaveChanges. graders: - type: file-matches config: path: analysis.md - pattern: (InternalEntityEntry|SetEntityState)[\s\S]*(TemporaryValuesTest|temporary) + pattern: (?i)(?=[\s\S]*SharedIdentityEntry)(?=[\s\S]*EntityGraphAttacher)(?=[\s\S]*CascadeDelete)(?=[\s\S]*(nested|leaf|replacement))[\s\S]+ - type: prompt config: scoring: binary diff --git a/eng/harness-evaluation/skills/code-review/eval.yaml b/eng/harness-evaluation/skills/code-review/eval.yaml index f3c35656587..262cd910b38 100644 --- a/eng/harness-evaluation/skills/code-review/eval.yaml +++ b/eng/harness-evaluation/skills/code-review/eval.yaml @@ -11,20 +11,9 @@ agent_environment: skills: - ../../../../.agents/skills/code-review stimuli: - - name: review-an-internal-public-api-change + - name: review-validation-relocation-across-inheritance prompt: |- - Before reviewing, use the available repository skill that applies to this task. Inspect src/EFCore/Metadata/Internal/EntityType.cs and review this proposed member as if it were added to EntityType: - - public virtual async Task ActivateAsync( - Type type, - Func callback) - { - var instance = Activator.CreateInstance(type)!; - await callback(instance); - return instance; - } - - Write review.md with findings and requirements grounded by at least two relevant existing EntityType members or annotations. Do not access GitHub. + Review the staged ModelValidator.cs and ModelValidatorTest.cs. Write review.md. Do not edit source or access GitHub. constraints: &review_constraints max_turns: 50 max_tokens: 1000000 @@ -33,17 +22,19 @@ stimuli: files: - src: ../../../../.github/copilot-instructions.md dest: .github/copilot-instructions.md - - src: ../../../../src/EFCore/Metadata/Internal/EntityType.cs - dest: src/EFCore/Metadata/Internal/EntityType.cs + - src: fixtures/model-validator-relocation/ModelValidator.cs + dest: src/EFCore/Infrastructure/ModelValidator.cs + - src: fixtures/model-validator-relocation/ModelValidatorTest.cs + dest: test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs rubric: - - The review applies the standard internal-API warning XML documentation and does not treat the member as supported public API merely because it is public. - - It addresses reflection and NativeAOT using concrete EntityType annotations or an alternative design and requires ConfigureAwait(false) on the callback. - - It grounds the advice in at least two staged EntityType members or annotations. + - The review identifies that a not-auto-loaded property declared on a base type is no longer checked against a derived type constructor binding because the relocated validation only examines the declaring structuralType. + - It requires checking constructor bindings across the declaring type and its derived types, using the consuming type in the diagnostic. + - It calls out both the stale ValidateAutoLoaded XML summary and the missing regression test for a base property consumed by a derived constructor. graders: - type: file-matches config: path: review.md - pattern: (?i)(?=[\s\S]*internal(?:-| )API)(?=[\s\S]*(NativeAOT|RequiresDynamicCode|DynamicallyAccessedMembers))(?=[\s\S]*ConfigureAwait\(false\))[\s\S]+ + pattern: (?i)(?=[\s\S]*(derived|inherit))(?=[\s\S]*(constructor.?binding|ConstructorBinding))(?=[\s\S]*(base|declar))(?=[\s\S]*(test|coverage))[\s\S]+ - type: prompt config: scoring: binary @@ -52,39 +43,52 @@ stimuli: - type: token-budget config: max: 1000000 - - name: catch-pooled-context-reset-race + - name: catch-concurrency-detector-flag-ownership-race prompt: |- - Before reviewing, use the available repository skill that applies to this task. Inspect the staged pooling implementation and tests. Review a proposed change that rewrites the successful branch of DbContextPool.ReturnAsync as follows: + Inspect the staged concurrency detector and tests. A proposed exception-safety change rewrites `EnterCriticalSection` as follows so an allocation failure cannot leave the detector locked: - if (Interlocked.Increment(ref _count) <= _maxSize) + if (Interlocked.CompareExchange(ref _inCriticalSection, 1, 0) == 1 + && ThreadAcquiredLocksCount.Value == 0) { - _pool.Enqueue(context); - await context.ResetStateAsync(cancellationToken).ConfigureAwait(false); + throw new InvalidOperationException(CoreStrings.ConcurrentMethodInvocation); } - The proposed test awaits ReturnAsync to completion and then asserts that Rent returns a reset context. Write review.md. Focus on concrete correctness and whether the test exercises the risky interleaving. Do not edit source or access GitHub. + try + { + ThreadAcquiredLocksCount.Value++; + } + catch + { + if (_currentContextRefCount == 0) + { + _inCriticalSection = 0; + } + + throw; + } + + _currentContextRefCount++; + + Write review.md. Do not edit source or access GitHub. constraints: *review_constraints agent_environment: files: - src: ../../../../.github/copilot-instructions.md dest: .github/copilot-instructions.md - - src: ../../../../src/EFCore/Internal/DbContextPool.cs - dest: src/EFCore/Internal/DbContextPool.cs - - src: ../../../../src/EFCore/Internal/IDbContextPoolable.cs - dest: src/EFCore/Internal/IDbContextPoolable.cs - - src: ../../../../src/EFCore/DbContext.cs - dest: src/EFCore/DbContext.cs - - src: ../../../../test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs - dest: test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs + - src: ../../../../src/EFCore/Infrastructure/Internal/ConcurrencyDetector.cs + dest: src/EFCore/Infrastructure/Internal/ConcurrencyDetector.cs + - src: ../../../../test/EFCore.Tests/Infrastructure/Internal/ConcurrencyDetectorTest.cs + dest: test/EFCore.Tests/Infrastructure/Internal/ConcurrencyDetectorTest.cs rubric: - - The review identifies that enqueueing before reset completes allows a concurrent renter to observe or use stale or concurrently resetting context state. - - It requires enqueueing only after ResetStateAsync completes and explains why the proposed await-then-rent test cannot expose the race. - - It proposes a focused interleaving test that blocks reset while another operation attempts to rent the same context. + - >- + The review identifies that `_currentContextRefCount == 0` does not prove this invocation acquired `_inCriticalSection`: another thread can be between CompareExchange and its later ref-count increment. + - It requires capturing whether this invocation's CompareExchange acquired the flag and clearing it in the catch only when that boolean is true. + - It explains a concrete interleaving where clearing another thread's flag permits concurrent entry, and requests focused ownership/interleaving coverage without disturbing the nested-section protocol. graders: - type: file-matches config: path: review.md - pattern: (?i)(?=[\s\S]*(race|concurren))(?=[\s\S]*(stale|resetting|ResetStateAsync))(?=[\s\S]*(block|interleav|rent))[\s\S]+ + pattern: (?i)(?=[\s\S]*(CompareExchange|acquired))(?=[\s\S]*(ownership|other thread|interleav|race))(?=[\s\S]*(boolean|this (?:call|invocation)|only clear))[\s\S]+ - type: prompt config: scoring: binary @@ -130,7 +134,7 @@ stimuli: max: 1000000 - name: review-breaking-dbset-public-api prompt: |- - Before reviewing, use the available repository skill that applies to this task. Inspect the staged DbSet and DbContext Find/FindAsync API families. Review a proposed replacement of this DbSet member: + Inspect the staged DbSet and DbContext Find/FindAsync API families. Review a proposed replacement of this DbSet member: public virtual ValueTask FindAsync( object?[]? keyValues, @@ -142,7 +146,7 @@ stimuli: IEnumerable keyValues, CancellationToken cancellationToken = default) - Write review.md. Review the supported API shape and compatibility. Do not edit source or access GitHub. + Write review.md. Do not edit source or access GitHub. constraints: *review_constraints agent_environment: files: @@ -172,7 +176,7 @@ stimuli: max: 1000000 - name: accept-documentation-only-change prompt: |- - Before reviewing, use the available repository skill that applies to this task. Inspect DbContext and review a proposed XML documentation-only correction that changes the visible link text `Changing tracking` to `Change tracking` while preserving the existing `https://aka.ms/efcore-docs-change-tracking` href. No signature or implementation changes are proposed. Write review.md. Do not edit source or access GitHub. + Inspect DbContext and review a proposed XML documentation correction that changes the visible link text `Changing tracking` to `Change tracking` while preserving the existing `https://aka.ms/efcore-docs-change-tracking` href. Write review.md. Do not edit source or access GitHub. constraints: *review_constraints agent_environment: files: diff --git a/eng/harness-evaluation/skills/code-review/fixtures/model-validator-relocation/ModelValidator.cs b/eng/harness-evaluation/skills/code-review/fixtures/model-validator-relocation/ModelValidator.cs new file mode 100644 index 00000000000..0f963822511 --- /dev/null +++ b/eng/harness-evaluation/skills/code-review/fixtures/model-validator-relocation/ModelValidator.cs @@ -0,0 +1,105 @@ +namespace Microsoft.EntityFrameworkCore.Infrastructure; + +public class ModelValidator +{ + protected virtual void ValidateEntityType( + IEntityType entityType, + IDiagnosticsLogger logger) + { + ValidateEntityClrType(entityType, logger); + ValidateChangeTrackingStrategy(entityType, logger); + ValidateIgnoredMembers(entityType, logger); + ValidatePropertyMapping(entityType, logger); + ValidateOwnership(entityType, logger); + ValidateNonNullPrimaryKey(entityType, logger); + ValidateInheritanceMapping(entityType, logger); + ValidateFieldMapping(entityType, logger); + ValidateQueryFilters(entityType, logger); + + foreach (var property in entityType.GetDeclaredProperties()) + { + ValidateProperty(property, entityType, logger); + } + + foreach (var complexProperty in entityType.GetDeclaredComplexProperties()) + { + ValidateComplexProperty(complexProperty, logger); + } + + LogShadowProperties(entityType, logger); + } + + protected virtual void ValidateProperty( + IProperty property, + ITypeBase structuralType, + IDiagnosticsLogger logger) + { + ValidateTypeMapping(property, logger); + ValidatePrimitiveCollection(property, logger); + ValidateAutoLoaded(property, structuralType, logger); + } + + /// + /// Validates that a property configured as not auto-loaded is not a key, foreign key, concurrency token or discriminator. + /// + protected virtual void ValidateAutoLoaded( + IProperty property, + ITypeBase structuralType, + IDiagnosticsLogger logger) + { + if (property.IsAutoLoaded) + { + return; + } + + var typeName = structuralType.DisplayName(); + + if (property.IsKey()) + { + throw new InvalidOperationException(CoreStrings.AutoLoadedKeyProperty(property.Name, typeName)); + } + + if (property.IsForeignKey()) + { + throw new InvalidOperationException(CoreStrings.AutoLoadedForeignKeyProperty(property.Name, typeName)); + } + + if (property.IsConcurrencyToken) + { + throw new InvalidOperationException(CoreStrings.AutoLoadedConcurrencyTokenProperty(property.Name, typeName)); + } + + if (structuralType is IEntityType entityType + && entityType.FindDiscriminatorProperty() == property) + { + throw new InvalidOperationException(CoreStrings.AutoLoadedDiscriminatorProperty(property.Name, typeName)); + } + + if (structuralType.ConstructorBinding is not null + && structuralType.ConstructorBinding.ParameterBindings + .SelectMany(p => p.ConsumedProperties) + .Contains(property)) + { + throw new InvalidOperationException( + CoreStrings.AutoLoadedConstructorProperty(property.Name, typeName)); + } + } + + protected virtual void ValidateComplexProperty( + IComplexProperty complexProperty, + IDiagnosticsLogger logger) + { + var complexType = complexProperty.ComplexType; + ValidateChangeTrackingStrategy(complexType, logger); + + foreach (var property in complexType.GetDeclaredProperties()) + { + ValidateProperty(property, complexType, logger); + } + + foreach (var nestedComplexProperty in complexType.GetDeclaredComplexProperties()) + { + ValidateComplexProperty(nestedComplexProperty, logger); + } + } +} \ No newline at end of file diff --git a/eng/harness-evaluation/skills/code-review/fixtures/model-validator-relocation/ModelValidatorTest.cs b/eng/harness-evaluation/skills/code-review/fixtures/model-validator-relocation/ModelValidatorTest.cs new file mode 100644 index 00000000000..c1d44829987 --- /dev/null +++ b/eng/harness-evaluation/skills/code-review/fixtures/model-validator-relocation/ModelValidatorTest.cs @@ -0,0 +1,60 @@ +namespace Microsoft.EntityFrameworkCore.Infrastructure; + +public partial class ModelValidatorTest : ModelValidatorTestBase +{ + [ConditionalFact] + public virtual void Allows_non_key_property_not_auto_loaded() + { + var modelBuilder = CreateConventionModelBuilder(); + modelBuilder.Entity( + eb => + { + eb.Property(e => e.Id); + eb.Property(e => e.Name); + }); + + var model = modelBuilder.Model; + var property = model.FindEntityType(typeof(AutoLoadEntity))!.FindProperty(nameof(AutoLoadEntity.Name))!; + property.IsAutoLoaded = false; + + Validate(modelBuilder); + } + + [ConditionalFact] + public virtual void Detects_constructor_bound_property_not_auto_loaded() + { + var modelBuilder = CreateConventionModelBuilder(); + modelBuilder.Entity( + eb => + { + eb.Property(e => e.Id); + eb.Property(e => e.Name); + }); + + var model = modelBuilder.Model; + var property = model.FindEntityType(typeof(AutoLoadEntityWithConstructor))! + .FindProperty(nameof(AutoLoadEntityWithConstructor.Name))!; + property.IsAutoLoaded = false; + + VerifyError( + CoreStrings.AutoLoadedConstructorProperty( + nameof(AutoLoadEntityWithConstructor.Name), + nameof(AutoLoadEntityWithConstructor)), + modelBuilder); + } + + protected class AutoLoadEntity + { + public int Id { get; set; } + public string Name { get; set; } = null!; + } + + protected class AutoLoadEntityWithConstructor + { + public AutoLoadEntityWithConstructor(string name) + => Name = name; + + public int Id { get; set; } + public string Name { get; set; } + } +} \ No newline at end of file diff --git a/eng/harness-evaluation/skills/cosmos-provider/eval.yaml b/eng/harness-evaluation/skills/cosmos-provider/eval.yaml index 4ab73facc49..0d26d8177ce 100644 --- a/eng/harness-evaluation/skills/cosmos-provider/eval.yaml +++ b/eng/harness-evaluation/skills/cosmos-provider/eval.yaml @@ -13,7 +13,7 @@ agent_environment: stimuli: - name: explain-readitem-discriminator-selection prompt: >- - Before analyzing files, use the available repository skill that applies to this task. Inspect src/EFCore.Cosmos/Query/Internal/CosmosReadItemAndPartitionKeysExtractor.cs, test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryTestBase.cs, and test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryTest.cs. Write analysis.md explaining why ReadItem_with_single_explicit_discriminator_mapping emits ReadItem, while ReadItem_with_single_explicit_incorrect_discriminator_mapping and ReadItem_with_single_explicit_parameterized_discriminator_mapping emit general queries. Identify the exact extractor condition responsible, explain why retaining each predicate is required for correctness, and outline the assertions that preserve all three behaviors during a refactor. Do not edit source. Include the exact focused EFCore.Cosmos.FunctionalTests command and state whether these tests require the emulator or a Linux-emulator condition. + Inspect src/EFCore.Cosmos/Query/Internal/CosmosReadItemAndPartitionKeysExtractor.cs, test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryTestBase.cs, and test/EFCore.Cosmos.FunctionalTests/Query/ReadItemPartitionKeyQueryTest.cs. Write analysis.md explaining why ReadItem_with_single_explicit_discriminator_mapping emits ReadItem, while ReadItem_with_single_explicit_incorrect_discriminator_mapping and ReadItem_with_single_explicit_parameterized_discriminator_mapping emit general queries. Identify the exact extractor condition responsible, explain why retaining each predicate is required for correctness, and outline the assertions that preserve all three behaviors during a refactor. Do not edit source. Include the exact focused EFCore.Cosmos.FunctionalTests command and state whether these tests require the emulator or a Linux-emulator condition. constraints: max_turns: 80 max_tokens: 2000000 diff --git a/eng/harness-evaluation/skills/make-custom-agent/eval.yaml b/eng/harness-evaluation/skills/make-custom-agent/eval.yaml index 41601157a80..6d4686e6bcd 100644 --- a/eng/harness-evaluation/skills/make-custom-agent/eval.yaml +++ b/eng/harness-evaluation/skills/make-custom-agent/eval.yaml @@ -13,10 +13,10 @@ agent_environment: stimuli: - name: create-an-efcore-api-review-agent prompt: >- - Before creating files, use the available repository skill that applies to this task. Inspect .github/workflows/api-review-baselines.yml, src/EFCore/EFCore.baseline.json, and .github/copilot-instructions.md. - Create a read-only declarative custom agent named efcore-api-review at .github/agents/efcore-api-review.agent.md that reviews EF Core ApiChief baseline deltas, explains compatibility impact, and points reviewers to evidence from the staged workflow and baseline. - Also create its paired harness evaluation at eng/harness-evaluation/agents/efcore-api-review/eval.yaml following eng/harness-evaluation/README.md. Do not create a skill or duplicate the root instructions. - constraints: + Inspect .github/workflows/api-review-baselines.yml, src/EFCore/EFCore.baseline.json, and .github/copilot-instructions.md. + Create `efcore-api-review` at `.github/agents/efcore-api-review.agent.md` to review EF Core ApiChief baseline deltas, explain compatibility impact, and point reviewers to evidence from the staged workflow and baseline without modifying the workspace or GitHub. + Also create its paired evaluation at `eng/harness-evaluation/agents/efcore-api-review/eval.yaml`. Do not create a skill or duplicate the root instructions. + constraints: &authoring_constraints max_turns: 50 max_tokens: 1000000 max_duration: 10m @@ -28,8 +28,6 @@ stimuli: dest: src/EFCore/EFCore.baseline.json - src: ../../../../.github/copilot-instructions.md dest: .github/copilot-instructions.md - - src: ../../../../eng/harness-evaluation/README.md - dest: eng/harness-evaluation/README.md rubric: - The output is .github/agents/efcore-api-review.agent.md with valid frontmatter whose name is efcore-api-review. - The output includes eng/harness-evaluation/agents/efcore-api-review/eval.yaml with a matching eval name and the required harness structure. @@ -39,7 +37,7 @@ stimuli: - type: file-matches config: path: .github/agents/efcore-api-review.agent.md - pattern: '(?m)^---[\s\S]*^name:\s*["'']?efcore-api-review["'']?\s*$[\s\S]*^description:' + pattern: '(?im)(?=^---[\s\S]*^name:\s*["'']?efcore-api-review["'']?\s*$)(?=[\s\S]*^description:)(?=[\s\S]*(ApiChief|API baseline))(?=[\s\S]*compatib)[\s\S]+' - type: file-matches config: path: eng/harness-evaluation/agents/efcore-api-review/eval.yaml @@ -54,6 +52,43 @@ stimuli: - type: token-budget config: max: 1000000 + - name: review-read-only-agent-boundaries + prompt: |- + Inspect the staged API review workflow and root instructions. Review this proposal for `.github/agents/efcore-api-review.agent.md` and write agent-review.md; do not create or edit the agent. + + --- + name: efcore-api-review + description: Reviews EF Core API changes. + tools: ['*'] + --- + + Check out the requested PR branch, regenerate or edit API baselines as needed, run commands, and post an APPROVE or REQUEST_CHANGES review with your findings. + + The proposal's paired eval declares both the agent file and `.github/copilot-instructions.md` in its root `agent_environment.files`, and every stimulus begins by telling the agent to invoke `efcore-api-review`. + constraints: *authoring_constraints + agent_environment: + files: + - src: ../../../../.github/workflows/api-review-baselines.yml + dest: .github/workflows/api-review-baselines.yml + - src: ../../../../.github/copilot-instructions.md + dest: .github/copilot-instructions.md + rubric: + - The review keeps this as a declarative agent but restricts its tools and behavior to read-only repository evidence; it forbids branch mutation, baseline edits, command execution, and APPROVE/REQUEST_CHANGES events. + - It requires a discovery description that says when to use the agent and domain instructions grounded in ApiChief baseline compatibility review without copying root instructions. + - The paired eval root contains only the agent file, its prompts are solution-neutral and do not instruct activation, and it includes bounded deterministic, semantic, and token grading with a committed threshold. + graders: + - type: file-matches + config: + path: agent-review.md + pattern: (?i)(?=[\s\S]*(declarative|\.github/agents))(?=[\s\S]*(read[ -]?only|restrict.*tools))(?=[\s\S]*(APPROVE|REQUEST_CHANGES))(?=[\s\S]*(root agent_environment|only the agent|single.*file))(?=[\s\S]*(activation|invoke))[\s\S]+ + - type: prompt + config: + scoring: binary + threshold: 1 + evidence: [ trajectory, repo ] + - type: token-budget + config: + max: 1000000 scoring: weights: file-matches: 0.4 diff --git a/eng/harness-evaluation/skills/make-github-actions-workflow/eval.yaml b/eng/harness-evaluation/skills/make-github-actions-workflow/eval.yaml index 90000391d34..6004d68f27e 100644 --- a/eng/harness-evaluation/skills/make-github-actions-workflow/eval.yaml +++ b/eng/harness-evaluation/skills/make-github-actions-workflow/eval.yaml @@ -13,7 +13,7 @@ agent_environment: stimuli: - name: extend-the-release-target-validation-workflow prompt: >- - Inspect .github/workflows/validate-pr-target-branch.yml and .github/workflows/label-and-milestone-issues.yml, and inspect .github/CONTRIBUTING.md for contributor context. Modify the staged validate-pr-target-branch.yml so an invalid external PR targeting a release branch also receives one idempotent explanatory comment. Preserve its existing policy, keep the pull_request_target path safe, preserve the existing immutable actions/github-script SHA reference, and do not execute or check out fork code. Use a stable hidden HTML marker and verify the matching comment's bot author before updating the existing bot comment across reruns. + Inspect .github/workflows/validate-pr-target-branch.yml and .github/workflows/label-and-milestone-issues.yml, and inspect .github/CONTRIBUTING.md for contributor context. Modify the staged validate-pr-target-branch.yml so an invalid external PR targeting a release branch receives an explanatory comment. Reruns must leave exactly one workflow-owned comment and must never edit a contributor-authored comment. Preserve its existing policy and immutable action reference, and do not execute or check out fork code. constraints: &short_constraints max_turns: 40 max_tokens: 750000 @@ -34,7 +34,37 @@ stimuli: - type: file-matches config: path: .github/workflows/validate-pr-target-branch.yml - pattern: (?=[\s\S]*actions/github-script@)(?=[\s\S]*'; -const { data: comments } = await github.rest.issues.listComments({ +const comments = await github.paginate(github.rest.issues.listComments, { owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, diff --git a/eng/harness-evaluation/README.md b/eng/harness-evaluation/README.md index 2688a309167..c2481932ae5 100644 --- a/eng/harness-evaluation/README.md +++ b/eng/harness-evaluation/README.md @@ -98,7 +98,7 @@ The runner defaults to one active trial per component. GitHub Actions paralleliz Before accepting a new or materially changed eval, inspect both arms and `comparison.jsonl`. A control that consistently matches or beats the treatment means the eval is not discriminating enough. -Repository skills must demonstrate comparative value, not merely pass their treatment rubric. If a skill consistently fails five-run comparison evals and can't be reasonably improved, remove both the skill directory rather than lowering thresholds, weights, or semantic requirements. Move its paired eval stimuli to `eng\harness-evaluation\instructions\copilot-instructions\`. +Repository skills must demonstrate comparative value, not merely pass their treatment rubric. If a skill consistently fails five-run comparison evals and can't be reasonably improved, remove both the skill directory and its paired eval rather than lowering thresholds, weights, or semantic requirements. Move its paired eval stimuli to `eng\harness-evaluation\instructions\copilot-instructions\`. ## Adding components diff --git a/eng/harness-evaluation/skills/scaffolding/eval.yaml b/eng/harness-evaluation/skills/scaffolding/eval.yaml index 5ad9a3a411c..0973a79d216 100644 --- a/eng/harness-evaluation/skills/scaffolding/eval.yaml +++ b/eng/harness-evaluation/skills/scaffolding/eval.yaml @@ -28,8 +28,8 @@ stimuli: dest: test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs rubric: - >- - The analysis identifies that LEFT JOIN plus a null filter is not equivalent when an object has multiple extended-property rows: a non-matching row survives even if another row has the excluded property. - - It proposes a non-correlated anti-set equivalent such as `object_id NOT IN (SELECT major_id ... WHERE the full extended-property predicate)`. + The analysis recognizes that a LEFT JOIN containing the full excluded-property predicate in `ON`, followed by `ep.major_id IS NULL`, preserves anti-semi semantics when `major_id` is non-null; unrelated property rows do not join. + - It verifies that the complete minor-id, class, and property-name predicates remain in `ON`, or proposes an equivalent non-correlated anti-set such as `NOT IN`. - It updates the schema-reader and database-creator occurrences plus their SQL baselines, and adds/identifies coverage with multiple extended properties on one object. graders: - type: file-matches