mirror of
https://github.com/dotnet/skills.git
synced 2026-09-20 09:49:54 +08:00
62ff166f2bf806137fe121b5328d68bc7db4a743
653 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 github-actions[bot]
|
62ff166f2b | Weekly plugin version sync | ||
|
Amaury Levé
|
2124a6e351 |
Flag reference skills that carry a direct-activation eval (#899) (#983)
`disable-model-invocation: true` drops a skill from the CLI's `<available_skills>` menu, so the experiment's skilled arm -- which loads exactly one skill -- cannot reach it either. The gate already reasoned about this, but `report_uncovered()` skipped any skill that had an eval, so it only reported the *better* case (no eval, visibly zero evidence) and stayed silent on the worse one (an eval scoring baseline against baseline and labelling the result a pass or a fail). Two evals landed in that blind spot after the reasoning was written down: tests/dotnet-test/platform-detection (#974) and tests/dotnet-test/filter-syntax (#976). Both READMEs still described them as deliberately un-evaluated. - report reference skills that carry a direct eval, naming the spec - self-test both sides of the trigger via a new silent_case helper - correct eng/eval-quality/README.md and plugins/dotnet-test/README.md Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3d666b74-a00f-4ed0-a3ce-2d3427cf0d23 |
||
|
Amaury Levé
|
38bf797c27 |
Capture the repo's hard-won skill-authoring lessons as guidance (#979)
* Capture the repo's hard-won skill-authoring lessons as guidance Mines ~35 merged PRs (#830-#976), issue #899 and eng/eval-quality/README.md into reusable guidance so contributors do not have to rediscover it. - Add .agents/skills/improve-skill-quality: a triage playbook for evals that regress, return no verdict, or fail to activate. Classify before rewriting - broken fixtures, underpowered trial counts, forced tools, stale spec keys and harness errors have all masqueraded as skill regressions. Two reference docs carry the content patterns and the symptom/cause/fix catalogue with citations. - Rewrite .agents/skills/create-skill-test. It documented a pre-Vally schema (scenarios:/assertions:/setup.copy_test_files) that no longer loads: all 97 eval specs use stimuli:/graders:/environment. Anyone following it authored a broken eval. Also folds in the trial floor, dormancy-guard and fixture rules. - Extend .agents/skills/create-skill with the description-as-router rules and a "write for delta over the baseline model" section. - CONTRIBUTING.md: add the quality playbook, document eval sizing, and correct the eval snippet and the stale "raise runs in dotnet-skills.experiment.yaml" advice - an experiment-level runs overwrites every eval's own value. - .gitignore: .agents/skills/ was silently swallowing the repo's own authoring skills. Replaced with .agents/skills/* plus negations for the five owned directories, so installed third-party skills stay ignored. Validated: markdownlint clean, check_eval_quality.py clean, all relative links resolve, frontmatter parses, descriptions under 1024 chars, bodies under 500 lines. Claims fact-checked against the repo by a review pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f * Address multi-model review: correct the sign-test arithmetic and eval exceptions Three review rounds (Copilot, Claude Opus 4.8, GPT-5.6 Sol/Terra) against the implementation in eng/vally-adapter/adapt.mjs. Statistics. The docs conflated the gate's two independent bars. adapt.mjs gates on (1) counted trials >= 5, else `underpowered`, and (2) p <= 0.05 on a sign test over the *discordant* (non-tie) trials. So "at 5-7 trials one tie makes a pass unreachable" was wrong: at 6 trials 5W/1T/0L is 5 discordant and passes at p = 0.031, and at 7 trials two ties are survivable. Every trial table is now keyed on discordant trials. eng/eval-quality/README.md carried the same over-generalisation and is corrected too, since these docs restate it. Eval coverage. "Every skill needs an eval" and "no direct eval for a disable-model-invocation skill" contradicted each other. Reference skills are now explicitly exempt and covered through their consumers, and filter-syntax is no longer described as the sole answer-graded exception - platform-detection uses the same pattern. Fixtures. "Every buildable fixture must build" forbade the intentionally broken fixtures the same document demonstrates. Now: healthy fixtures build, deliberately broken ones fail only for the reason their stimulus is about. Triage routing. The first-match classifier sent fixture failures to the harness row before the fixture row could match, and treated any positive-but-failing record as a power problem even though adapt.mjs requires `conclusive` first. Added an inconclusive row, moved the fixture row up, and scoped "quote a losing trial" to content fixes so it no longer blocks the no-results path. Also: expect_tools does not assert activation (the harness reports it separately); CONTRIBUTING claimed evaluations run automatically on plugin changes when a maintainer must trigger /evaluate against a bound commit; the minimal eval example is labelled as a sub-floor skeleton; create-skill scaffolds into plugins/<plugin>/skills/ rather than a non-existent top-level skills/. Validated: markdownlint clean, check_eval_quality.py clean, adapt.test.mjs 21 pass, links and anchors resolve. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f * Address review: document that agent evals sit outside the verdict flow Abhitej's review found one actionable gap plus three nits; all four are small enough to land here rather than as follow-ups. Agent evals. create-skill-test told authors to write tests/<plugin>/agent.<name>/ eval.yaml, size it for the trial floor, and validate with run-skill-evals.sh -- none of which applies. The canonical experiment declares `evals: tests/*/!(agent.*)/eval.yaml`, so agent specs are excluded: no verdict is computed, the floor does not apply, and the runner drops them even when named explicitly because --eval-filter is intersected with that glob. Step 1 now says so, Step 10 shows the EXPERIMENT_FILE route for actually exercising one, and the checklist and pitfalls are scoped accordingly. Nits: aligned eng/eval-quality/README.md check 8 on "stimuli" (it still said "scenarios" while every authoring doc says stimuli); added a .gitignore comment so the next repo-owned authoring skill gets a negation instead of being silently untracked; and noted in improve-skill-quality that eng/skill-validator/src/docs/InvestigatingResults.md documents the retired skill-validator evaluate schema, so the vally-adapter one is the guide to read. Validated: markdownlint clean, check_eval_quality.py clean, 22/22 eval-quality self-tests, gitignore negations still exact. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f * Generalize two triage rows that were written in test-skill vocabulary The triage catalogue is meant to apply to every plugin, but two rows stated their rule in dotnet-test terms, so an author on another plugin would read past them: - the split-brain fixture row was framed entirely around Cobertura `line-rate`; the rule is that a fixture must never state the same fact twice in two places that disagree, whatever the format. Cobertura is now the named example. - the volume-comparison row said "both arms write tests" / "do not reward test count"; the rule is that when both arms produce the same kind of artifact the judge falls back on comparing quantity. Also generalized the one-line summary of the first rule in SKILL.md Step 4 so the detail and the summary do not disagree. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f * Correct the environment.skills guidance and a stale cross-reference Two suppressed findings from the Copilot review summaries, both real. environment.skills. The Step 5 skeleton documented it as "extra skills loaded in the isolated arm only", which is wrong in both halves. The experiment declares `vary: /environment/skills` and supplies the value for each arm itself (`[]` for baseline, `plugins/<plugin>/skills/<skill>` for skilled), so whatever a skill eval declares is replaced in every arm -- it cannot add a skill to one arm, and in a skill eval it does nothing at all. The key is meaningful only in `agent.*` evals, which the experiment does not vary. Removed it from the generic skeleton, documented where it does apply, and pointed at an existing agent eval to copy rather than reproducing a spelling: the three agent specs in this repo disagree about whether entries are bare names or relative paths, so the doc should not assert one. Stale cross-reference. Moving the fixture row to the top of the triage table last round left the sentence below it still saying a fixture failure "also matches the two rows above it", when it is now the first row. Reworded to say why it leads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f |
||
|
Abhitej John
|
ad0cfe8735 |
Merge pull request #977 from dotnet/abhitejjohn-studious-dollop
Fix invalid XML in version-tooling nuget.config (unblocks automated version bumps) |
||
|
Abhitej John
|
3eb9c87254 |
Fix invalid XML in version-tooling nuget.config
The header comment contained `--configfile`, i.e. a doubled hyphen `--`, which XML forbids inside a comment. That made eng/version/nuget.config invalid XML, so `dotnet tool restore --configfile eng/version/nuget.config` failed with "NuGet.Config is not valid XML" on every run of BOTH versioning workflows (weekly-version-sync and version-bump-command). As a result no automated version bumps have ever been produced since the infra merged in #813. Reword the comment to avoid any doubled-hyphen sequence and move it inside <configuration>. Verified: the file now parses as XML and `dotnet tool restore` succeeds; nbgv get-version computes correctly against the plugins. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70b3727e-c27d-4a3e-aff9-f4eb65eaf25c |
||
|
Amaury Levé
|
573938df09 |
Close the remaining dotnet-test eval follow-ups (#899) (#971)
* Close the remaining dotnet-test eval follow-ups from #899 Every non-agent dotnet-test eval now clears the 5-trial floor, the two cost P1s are addressed, and the reference-skill coverage gap is recorded as a decision instead of a standing warning. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 024b3241-d7af-418a-b9cd-3bb9ee9bf0ee * Remove the duplicate block that cloned a grade-tests scenario, and gate it The 'production code available' scenario carried a leftover tail from the edit that moved the 'production code unavailable' one. YAML keeps the last duplicate key, so every field of the new scenario was silently overwritten: it shipped as a byte-identical rerun of its predecessor and never loaded the dotnet-production-available fixture it was built around. Parsing the spec and counting stimuli - which is what verified this PR - returns the intended 5 names either way, so only the parser can see it. The eval-quality gate now loads specs with a duplicate-key-strict loader (failing check 9), with a self-test case and the incident recorded. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 024b3241-d7af-418a-b9cd-3bb9ee9bf0ee --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 024b3241-d7af-418a-b9cd-3bb9ee9bf0ee |
||
|
Amaury Levé
|
030493de5a |
Fix migrate-mstest-v1v2-to-v3 activation and test-skill eval quality (#974)
* Fix migrate-mstest-v1v2-to-v3 skill activation The frontmatter description said DO NOT USE FOR: ... projects already on MSTest v3+, which blocked the skill on every scenario where the packages had already been bumped to 3.x and only the source or settings still needed the v1/v2-to-v3 fixes (Assert object overloads, DataRow strict typing, .testsettings -> .runsettings). It also gated the whole skill behind "the user asks to upgrade MSTest", so a standalone .testsettings conversion never matched. - Rewrite the description around both entry points (pre-upgrade migration and post-upgrade breaking-change fixes) and add the concrete trigger keywords those prompts contain: CS1501/CS1503/CS0121, MSTEST0014, LegacySettings, DeploymentEnabled, per-test TestTimeout, net5.0. Note that the current runner is preserved so "migrate to v3 but keep VSTest" isn't poached by migrate-vstest-to-mtp. - Narrow migrate-mstest-v3-to-v4, which claimed the generic "tests don't compile after upgrading MSTest" phrasing and competed for the same prompts. - Widen the Boundary Gate: a 3.x package version alone no longer ends the migration when v1/v2-era settings or errors remain, so the skill actually performs the requested edits instead of reporting "already migrated". - Add a routing row to the test-migration agent for the same case. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad6ff32a-d441-4a7b-b474-2bfaee764740 * Clear the eval-quality gate's test-skill findings The gate reported three classes of debt against the dotnet-test and dotnet-test-migration plugins. All three are addressed here; the four ERRORS it also reports are dotnet-maui allowlist lines and are untouched. Underpowered evals (5). Below five trials the pass gate's sign test cannot reach p <= 0.05 at any effect size, so these five evals could never return a verdict. Each is now at or above the floor and its allowlist line is deleted in the same change, as the ledger's shrink-only rule requires: - coverage-analysis 3 -> 5: adds a refactoring-safety question (the "is this safe to change?" use case named in the skill's Purpose but never exercised) and a branch-vs-line coverage question. Both reuse the existing partial-coverage fixture. - find-untested-sources 4 -> 5: adds a mixed C#/TypeScript repository, which is the only case that exercises the documented engine choice - polyglot tree-sitter rather than the C#-only Roslyn engine. Composed from the two existing fixtures. - generate-testability-wrappers 4 -> 5: adds the ambient-context path (Step 5) for a project with no DI container, where AsyncLocal<T> and scoped disposal are the distinguishing content. - grade-tests 4 -> 5: adds a C# case with the production code present. Every prior C# scenario hides it, so "Unverified" was never tested as a negative, and the D band and the swallowed-exception F were never graded at all. New production-available fixture. - code-testing-agent 3 -> 6 via defaults.runs=2. Scenarios are preferred over runs, but each of these drives a full generate-build-test pipeline (npm ci plus two Vitest runs, pip install plus pytest, a dotnet test build) under a 60m budget, which is the documented case for buying trials with runs. Orphaned fixtures (5). v3-sealed-timeout, mtp-mstest-sdk9, mtp-mstest-sdk10, mtp-mstest-hotreload-installed and vstest-mstest are all superseded first- generation copies: their per-scenario successors differ only in whitespace, a dropped rollForward, or a package version. Both evals are already well above the floor, so wiring them up would add no power. Deleted. Skills with no eval (2 of 4). platform-detection and filter-syntax carry real checkable rules that nothing measured, and several are counterintuitive enough that a baseline is likely to get them wrong - global.json test.runner outranking TestingPlatformDotnetTestSupport on .NET 10+, Microsoft.NET.Test.Sdk not being a VSTest signal, MTP properties living in Directory.Build.props, xUnit v3 dropping VSTest --filter while MSTest on MTP keeps it. Both get a 5-scenario eval with small fixtures and no build step. code-testing-extensions and test-analysis-extensions are left flagged on purpose: their bodies are tables of paths to extension files, so a head-to-head eval would score path recall rather than user value. The content those files hold is already exercised through code-testing-agent's three-language pipeline. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad6ff32a-d441-4a7b-b474-2bfaee764740 * Fix the two skill defects behind the v1v2-to-v3 eval losses The first eval run reached 7W/2T/2L, p=0.090, short of the p<=0.05 gate. Both losses trace to skill content that actively misled the agent, and the session transcripts show exactly how. Loss 1 -- 'Migrate MSTest v1 project with assembly reference', skilled scored 0.00 against a 4.17 baseline. The transcript shows the skill loading correctly and the agent then replying, in full: 'To give you specific migration steps, I need to see your project file. Could you share the path to your .csproj?' The project was already in the working directory. Cause: the Inputs table marked 'Project or solution path' as Required=Yes, which reads as a precondition the agent must obtain before doing anything. This is the worst kind of failure for a real user - they describe their project in prose and get a question back instead of an answer. Path is now optional and discovered by globbing, Step 1 leads with locating the project, and a note forbids opening with a request for the path. The same Required=Yes trap was present in migrate-mstest-v3-to-v4 and migrate-vstest-to-mtp, so both are corrected too. Loss 2 -- 'Fix DataRow type mismatch errors', skilled 3.96 against a 5.00 baseline. The skill's breaking-change table said the 16-argument DataRow cap was 'fixed in later v3 versions' and suggested 'refactor test / wrap extra params in array'. On a project already at MSTest 3.8, the agent concluded the valid 17-argument row exceeded the limit and rewrote it - first as new object[] { 17 }, which failed, then second-guessing itself mid-run ('let me check if the latest 3.x actually fixed the 16-arg limit'), finally settling on a (object)17 cast. Churn plus wasted turns on code that was already correct. The vague wording was the problem, so it is replaced with the fact: the cap was introduced in 3.0.1 and removed again in 3.0.3 (microsoft/testfx#1554 and the maintainer's 'please feel free to update to 3.0.3'). On 3.0.3+ a longer row is valid and must be left alone. A general guideline is added alongside it - confirm the diagnostic before editing, because rewriting valid code to dodge a limit the project is not subject to is a defect rather than caution. Both fixes are about what the skill tells a real user, not about the graders; no eval prompt, fixture, or grader is touched. Skill grows ~480 tokens and stays in the 'standard' tier, below the 5,000-token warning threshold. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad6ff32a-d441-4a7b-b474-2bfaee764740 * Correct the Assert/DataRow facts and make the fixtures reproduce them The two eval runs on this PR compared byte-identical skill content: run 1 ( |
||
|
Amaury Levé
|
5ed5ed8202 |
Close the remaining dotnet-test skill-coverage gaps (#976)
The coverage report left four dotnet-test skills with uncovered teaching
points. All four are now at 100%.
filter-syntax had no eval at all, so every point it teaches was uncovered
by construction. Add one with five stimuli - a VSTest category filter with
an exclusion, an MTP project on the .NET 9 SDK where the filter has to be
forwarded past the SDK's own command line, xUnit v3's framework-specific
filter options (including the query language, whose trait selector is the
`[trait]` point the report flagged), TUnit's path-shaped treenode filter,
and a VSTest-to-xUnit-v3 translation of four CI filter arguments. Five
stimuli is also the trial floor: a new eval cannot be allowlisted.
generate-testability-wrappers was missing the whole no-DI branch of the
skill. Add an ambient-context stimulus (a shipped library whose public
static API cannot take constructor injection) covering Step 5, the
`AsyncLocal<T>` validation item, the `[ThreadStatic]` pitfall, and the
`readonly` field. Add a `sealed` grader on the custom-wrapper stimulus, a
lifetime rubric item for the AddSingleton/AddTransient choice, and
assertion graders so the TimeProvider and MockFileSystem samples have to
actually assert rather than only construct the fake.
grade-tests was never exercised on an input large enough to trigger the
row cap, so the cap, the collapsed overflow, the summary-leads rule and
the report-once rule were all untested. Add a 62-test fixture (44 sound,
11 null-check-only, 7 assertion-free) and a stimulus that asks for a
report that stays readable in a PR comment.
writing-mstest-tests covered the collection half of the assertion table
but not the null/identity/emptiness/absence half. Add a stimulus that
rewrites four generic IsTrue checks, covering Assert.AreSame,
Assert.IsNull, Assert.IsEmpty and Assert.DoesNotContain.
Both generate-testability-wrappers and grade-tests cross the five-trial
floor with their new stimulus, so their allowlist lines go with them. The
four dotnet-maui entries were already stale on main -
|
||
|
Amaury Levé
|
2c22b6c815 |
Fix the underpowered eval gate: score direction, not magnitude (#952) (#965)
The skill evaluation gate flipped ~27% of its verdicts between runs on byte-identical inputs. Reproducing all 22 verdicts from the A/A test in #952 from the formula alone isolates two independent defects. The gate scored the judge's *magnitude*. Trial scores are a five-point ordinal scale (much-better +1.0, slightly-better +0.4, equal 0, ...), and a Student-t interval over those reads the 0.4 -> 1.0 step as variance, so a skill is punished for winning more decisively. Four wins and three ties over seven trials passes at ciLow +0.031 when every win is "slightly-better" and fails at -0.021 when one is "much-better" - same record, better outcome, reversed verdict. coverage-analysis failed five consecutive runs while winning 100% of its trials, then passed on a sixth with the same 3W/0T/0L record. The gate had no minimum sample size. vally returns ciLow = mean below two trials, so a single lucky judgment passed outright; a neutral skill passes ~30% of the time at one trial, and 18 of 94 gated evals sit there. adapt.mjs now reads only each trial's winner and decides with an exact one-sided sign test at 5%. The verdict is a deterministic function of the win/tie/loss record, so identical records always produce identical results. Collapsing to direction alone is not enough: a t-interval over win/tie/loss disagrees with the exact test on 12 records up to n=10 and is the permissive side in every one, passing 4W/0T/0L, 4W/3T/0L and 6W/0T/1L at p = 0.0625. MIN_CREDIBLE_TRIALS = 5 falls out of the same test rather than being chosen: the sign test cannot reach 5% below five discordant trials, and discordant trials can never exceed counted trials. Below it no record can pass, so the verdict is reported as underpowered - never a pass, never a regression - which is what stops "won every trial, failed anyway" being rediagnosed each run. check_eval_quality.py enforces the same floor on specs before they run, counting trials as scenarios x defaults.runs. The 60 existing evals below it are grandfathered in a ledger that can only shrink: the gate errors on a stale, duplicated or unnecessary entry, and --base-ref rejects entries new relative to the base branch, so a PR cannot add a below-floor eval and exempt it in the same change. Renames are read from git and are not growth. The experiment file no longer sets `runs` in `overrides:`. Precedence is CLI > experiment overrides > eval defaults and the merge is a plain spread, so that line did not default anything - it overwrote every eval's own defaults.runs and made per-eval trial counts impossible to express. No eval declares `defaults:` today, so this changes nothing until one opts in, and it is what makes raising a thin eval possible at all. Also fixes two latent bugs found while testing: --vally paths containing a space were split into separate argv entries (which made the adapter suite unrunnable on Windows, where process.execPath is under "Program Files"), and importing adapt.mjs made it parse its importer's command line. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29c41814-d553-40fd-b50c-6590c88c9540 |
||
|
Amaury Levé
|
f0a0b919bf |
Close the dotnet-test skill-coverage gaps from the PR 945 report (#949)
* Close the dotnet-test skill-coverage gaps from the PR 945 report The coverage comment reported 0% for seven dotnet-test skills. Most of that was a measurement artifact: Measure-SkillCoverage.ps1 parses an eval schema that does not exist in this repo. It looks for `assertions:` blocks with sibling `pattern:`/`value:` keys, but all 97 tests/**/eval.yaml files use `graders:` with a nested `config:` map, and it only captured rubric entries that were single-line quoted scalars while nearly every rubric item here is an unquoted plain scalar wrapped across lines. The result was `Evidence items: 0` and every teaching point reported as uncovered. Rewrite Get-EvalEvidence as an indentation-driven parser that reads the real shape: graders with nested config (pattern, substring, value, command, stdout_contains, stdout_matches, and path for file-exists graders), plus block and folded scalars for both grader values and rubric items. Literal values are regex-escaped before code-pattern matching so a substring is not treated as a regex. Cross-checked against a real YAML parser: evidence counts now agree exactly for all 73 skills that have an eval spec. With the measurement fixed the true gaps were 24 teaching points, all of which are now covered: - detect-static-dependencies: new accounting-discipline stimulus for file:line locations, one authoritative total, and pure helpers such as Path.Combine excluded from the needs-wrapping total. - migrate-static-to-wrapper: new with-tests fixture (custom IClock seam plus an MSTest project) and a stimulus covering the required using directive, test doubles in the updated tests, and scope discipline. - test-smell-detection: wired up the orphaned skip-and-magic fixture (added its missing production class) to cover reasoned vs bare skips, contextually obvious numbers, justified severities, and concrete fixes. - test-tagging: wired up the orphaned go-report-only fixture for the report-only no-source-edits rule, plus a grader for the NUnit [Category] attribute. - assertion-quality, test-anti-patterns, test-gap-analysis: rubric items for metric arithmetic, per-finding locations, severity prioritisation, empirical survivor confirmation, mutation category labels, and settled verdicts. Also fix crap-score, whose refactor-required stimuli referenced a coverage.cobertura.xml that .gitignore silently swallowed via `coverage*.xml`. Add the fixture, un-ignore fixture coverage reports, and align the two rubric numbers with what the file records (60% line coverage, complexity 14, CRAP ~26). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c217f3c4-fd5a-4ede-bbc6-5d916c1182e6 * Address review feedback on the eval.yaml evidence parser Remove-YamlQuoting now applies each quoting style's escape rules, so single-quoted scalars collapse '' to ' (the corpus has user''s and xUnit''s) and double-quoted scalars honour \" and \\ instead of only \\. Parsed evidence now matches what a real YAML parser produces. Read-YamlScalar switches to indentation-only consumption once a | or > header is seen. The structural breaks meant for plain scalars would otherwise truncate a block scalar at its first blank, #, "- " or key:-shaped line and silently drop the evidence. The same treatment applies to a quoted scalar until its closing quote is reached, which fixes the one real truncation in the corpus today: the double-quoted rubric item in dotnet-upgrade/migrate-nullable-references whose continuation line starts with #nullable disable. The stimulus boundary is now anchored to the indent of the first "- name:" seen and only fires outside a graders/rubric block, so a deeper rubric bullet that happens to start with name: can no longer be mistaken for a new scenario and reset the section. Coverage is unchanged across all 75 skills, so these fixes close latent foot-guns rather than move numbers. Cross-checked against PyYAML over all 98 eval.yaml files. Also re-adds the two test-tagging points that the rebase onto main dropped and closes two migrate-static-to-wrapper points that main's newer SKILL.md introduced: the [Category] code pattern, the report-only "no source files modified" rule, the pitfall that static classes cannot take constructors, and the Step 7 migration summary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94603bab-d29b-4bed-b5a6-1b0b1f50547d --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c217f3c4-fd5a-4ede-bbc6-5d916c1182e6 Copilot-Session: 94603bab-d29b-4bed-b5a6-1b0b1f50547d |
||
|
Abhitej John
|
7d86b4e5e8 |
Merge pull request #947 from kubaflo/kubaflo/dotnet-maui-skills-eval-fixes
Fix dotnet-maui skill regressions and sharpen decisiveness |
||
|
Jakub Florkowski
|
b02c5a20ed |
Revert accidental repo-wide runs: 1 -> 3 change in the experiment config
|
||
|
leslierichardson95
|
85cd103400 |
Retire mcp-csharp-create, mcp-csharp-test, and mcp-csharp-publish skills (#943)
Removes the remaining three mcp-csharp-* skills from the dotnet-ai plugin. Following the cut of mcp-csharp-debug (#938), this retires the rest of the family based on the cross-family skills-eval (#889) and the 30-day telemetry review (https://gist.github.com/AbhitejJohn/db57f38051ebf4e7de5556a4bd98ec15): - mcp-csharp-test: STRENGTHEN 0/5 families; 2 real users, 0 with >=5 invocations. - mcp-csharp-publish: STRENGTHEN 0/5 families; 0 real users. - mcp-csharp-create: wins only 2/5 families (Sonnet, Haiku) and REGRESSES both frontier models; impact 0.40 at avgN 2.6 (directional only); absent from the telemetry report. Validating it would require a full 5-family eval sweep with no evidence of real demand to justify the investment. Changes: - Delete plugins/dotnet-ai/skills/mcp-csharp-{create,test,publish}/ (SKILL.md + references) - Delete tests/dotnet-ai/mcp-csharp-{create,test,publish}/ (eval.yaml) - Remove the dotnet-ai CODEOWNERS block for the three skills and their tests - Repoint the skill-validator doc example to a surviving skill (technology-selection) dotnet-ai retains technology-selection. plugin.json auto-discovers ./skills/, so no manifest change is needed. Verified no remaining mcp-csharp-* references repo-wide (git grep). Refs dotnet/skills#889. Supersedes #939, #940, #941. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6a8ce111-58e5-48a7-9f7f-8de972bdc693 |
||
|
Amaury Levé
|
9d1ccf3094 |
Make the coverage-analysis plateau fixture self-consistent (#964)
`fixtures/plateau/coverage.cobertura.xml` declared a 75% line-rate against a payload that enumerates 22 of 47 lines (47%), so a skill that trusts the attribute and one that recomputes from `<lines>` read different inputs from the same file — the split-brain class the eval quality gate exists to stop. 75% is unreachable from this payload: `CalculateGpa` contributes 24 of the 47 lines at 0% and the rubric requires it to stay the blocker, capping the achievable rate at 23/47 = 48.9%. Restate the plateau at 47% instead, which the scenario does not depend on — only on one method dominating the shortfall: - declared line rates 0.75 -> 0.47 at file, package and class level, with lines-covered/lines-valid = 22/47 - branch rates 0.60 -> 0.44 with branches-covered/branches-valid = 7/16, which is what the `condition-coverage` data actually sums to - prompt reworded to "stuck at 47%" Every level of the fixture now agrees with the lines beneath it, so the aggregate-vs-payload check has no remaining offenders and is promoted from warning to error, with a self-test case covering the exact shape (methods and file totals self-consistent, only the aggregates contradicting the payload) and the README updated. Fixes #958 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 57b65d53-dc7a-4d0a-b5c6-d17687018f01 |
||
|
Amaury Levé
|
f2eb897a12 |
Fix dotnet-test findings from the refreshed cross-family eval (#899) (#945)
* Fix dotnet-test findings from the refreshed cross-family eval (#899) Every change below is driven by judge evidence from the losing trials of the refreshed 5-family dotnet-test matrix (runs 30108473397 + recovery runs), not by style preference. Eval measurement fix — the "discovery" P2s were an artifact: - assertion-quality, test-gap-analysis, test-smell-detection, and test-tagging each have a decline stimulus with `constraints.reject_skills: ["*"]`, so the skill cannot activate there by construction. Without `expect_activation: false` the adapter counted those dormant runs as missed activations, which is exactly the 75-88% invocation rates reported in the scorecard. Annotating them (the convention already used by agent.test-quality-auditor) removes the false signal; the non-activations were the only ones observed for these skills. Skill fixes: - test-gap-analysis: baselines won by actually running the suite while the skill reasoned statically and reported survivors that the tests in fact kill. Added Step 4b: confirm every reported survivor by applying it, re-running the covering tests, and reverting; fall back to reasoning only when the suite cannot run, labelled unverified. Calibrated severity down for strong suites. - test-anti-patterns: baselines won on depth, not polish. Added a depth bar — account for every test in scope, give exact expected values in fixes, name the adjacent error-path/boundary gaps, and keep counts consistent. Trimmed three pitfall rows that duplicated the calibration step so the skill stays under the profiler's "comprehensive" threshold. - detect-static-dependencies: losses were all counting accuracy. One authoritative total (no findings parked outside it), classify by the resource touched rather than by the `static` keyword, exclude pure helpers such as Path.Combine from the needs-wrapping total, require file:line, and add the missing randomness/culture/serialization categories. - test-smell-detection: the calibration rule told models to downgrade Sleepy Test for integration tests, which is what lost both losing scenarios. Fixed sleeps now stay High in any category; Mystery Guest and Eager Test still downgrade. - crap-score: losses came from estimating coverage after collection failed. Added the dotnet-coverage/ReportGenerator recovery path and a hard rule never to publish a CRAP score built on assumed coverage. - coverage-analysis: answer the asked question first, reconcile every number against the script output, and list every below-threshold member instead of declaring one method the entire gap. - migrate-static-to-wrapper: migrate exactly what was requested (no adjacent DateTime.Now rewrites, respect intentional-use comments) and never report "build succeeded" when the build or restore failed. - code-testing-agent: quote each requirement verbatim in the evidence table so multi-condition requirements map to a test that covers the whole combination, and cite a clean run rather than a coverage attempt that exited non-zero. Validation: skill-validator check passes (20 skills, 10 agents); markdownlint clean; eval specs parse and the adapter now reports all four decline stimuli as expect-dormant. Refs #899 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa * Strengthen underpowered dotnet-test evals from the PR 945 eval run The PR eval reported 5 of 10 skills as "no credible improvement". Reproducing the gate arithmetic from the artifacts shows the dominant cause is statistical power, not skill quality. The gate is `mean > 0 AND ci_low > 0` with a t-based CI over per-trial scores, which reduces to `sqrt(n) * (mean/sd) > t(n-1)`. The required mean/sd ratio is brutal at small n: n=2 -> 8.98 n=3 -> 2.48 n=4 -> 1.59 n=5 -> 1.24 n=6 -> 1.05 n=8 -> 0.84 Recomputing each reported CI from the per-trial scores reproduces the published numbers exactly, which confirms the mechanism: crap-score [0.4,1.0,0.4] n=3 CI [-0.261, 1.461] migrate-static-to-wrapper [1.0,0.4,0.4] n=3 CI [-0.261, 1.461] test-gap-analysis [0.4,0,0.4,0.4] n=4 CI [-0.018, 0.618] test-anti-patterns [0.4,0,0.4,0,0,0.4] n=6 CI [-0.030, 0.430] code-testing-agent [0,0.4] n=2 CI [-2.341, 2.741] crap-score and migrate-static-to-wrapper won 100% of their trials (3W/0T/0L) and still failed: at n=3 nothing short of three identically-sized wins can clear the gate. That is a property of a thin eval, not of the skill. Scenario counts are raised with discriminating cases, four of them by wiring up fixtures that were already committed but had no stimulus referencing them: - test-gap-analysis 4 -> 6, using the orphaned `report-quality` fixture (trivial auto-properties and an auto-generated .g.cs to skip, private helpers reachable only through the public API, and a deliberately weak Assert.IsTrue that cannot kill arithmetic mutations) and the orphaned `rust-error-propagation` fixture (an untested `?` propagation path and an untested `<=` boundary). - test-anti-patterns 6 -> 8, using the orphaned `pytest-mixed` fixture (which also checks the calibration rule that pytest's bare `assert` must not be flagged) and the orphaned `assertion-problems` fixture (which separates Critical false-confidence assertions from a Low-severity message nit). - crap-score 3 -> 6, with a new `refactor-required` fixture whose numbers are self-consistent: ApplySurcharges has complexity 13 behind a stale "Complexity: 4" comment (CRAP 28.4, needs 77.2% coverage), ClassifyAccount has complexity 17 so coverage alone can never reach CRAP < 15, and RoundToCurrency is 100% covered so its CRAP equals its complexity exactly. - migrate-static-to-wrapper 3 -> 5, adding a DateTimeKind-preservation scenario over the existing fixture and a new `static-helper` fixture where a static class must gain an ambient TimeProvider seam without breaking its callers. - code-testing-agent 2 -> 3, with a compact C# fixture that must extend an existing suite to the untested method only. This eval stays the weakest: each scenario is expensive, so raising `runs` is a better lever than adding more heavyweight scenarios. Verification: - every eval spec parses and all 254 fixture references resolve - the three new fixtures compile; the shipping-quotes fixture restores, builds and its three seed tests pass under `dotnet test` in a clean workspace - skill-validator check passes (20 skills, 10 agents) - markdownlint clean Refs #899 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa * Address review feedback on fixture and counting wording - BillableWeightTests: the ZeroOrNegative test only asserted the zero case, so its name overstated what it covered. Made it data-driven over 0 and -1 so the name matches the assertions. This matters more than usual here: the file is the seed suite for a test-quality eval, and a misleading test name is exactly what these skills are supposed to flag. - detect-static-dependencies: the Step 3 lead-in said to count each "static call pattern", which contradicted the rule immediately below it that instance members reaching the same untestable resource must also be counted. Reworded to "call site" and made the instance-member inclusion explicit. Verified: the fixture restores, builds and now passes 4 tests (was 3); skill-validator check passes; markdownlint clean. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1947263a-0ef9-47bd-ac53-5af5afa3ddaa |
||
|
Abhitej John
|
e93e52b7db |
Improve /evaluate visibility and run one evaluation per PR (#961)
* Fix /evaluate not-found path capturing gh api error body When a commented SHA does not resolve, 'gh api commits/<sha>' can emit an error body onto stdout, so FULL_SHA held a JSON payload instead of being empty. That slipped past the not-found check and surfaced the raw API error in the reachability message. Accept only a real 40-char hex object id; anything else is treated as not found (fail-closed, clean message). Found while validating the gates on a dummy PR after #956 merged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29ef3316-14f6-4dc1-a838-40f8be09f915 * Add /evaluate PR visibility and per-PR single-flight Tightens the automated evaluation workflow so a maintainer's /evaluate always gets a clear, prompt response on the PR and a PR is evaluated one run at a time. - Give every /evaluate trigger a unique concurrency group so its gate job always starts and can respond, instead of being starved behind an in-flight evaluation (the cause of a /evaluate sometimes going quiet). - Acknowledge authorized comment triggers early with a reaction, right after the write-access check, regardless of outcome. - Add a best-effort single-flight in the gate: defer to an in-flight run on the same commit (posting a link to it) and supersede an older run on a stale commit. Sibling identity is keyed on (head repository, head branch) so it can never affect a run from an unrelated repository, and a numeric run-id tiebreak makes the race deterministic. It is not a security control -- each run stays bound to its own validated commit. - Cap cheap gate fan-out per PR with a job-level concurrency group so a burst of /evaluate cannot spin up unbounded runners; the expensive evaluation jobs are excluded and are never interrupted by it. - Restore workflow-level cancel-in-progress: true (now only affects the lightweight status/noop groups). Harden the gate step under set -euo pipefail so a transient runs/jobs API error fails open rather than aborting the gate. gate gains actions: write, used only after the write-access check to supersede a stale run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29ef3316-14f6-4dc1-a838-40f8be09f915 * Harden /evaluate single-flight and clear stale acknowledgements Review follow-ups on the gate's single-flight step: * Narrow sibling selection. `(head repository, head branch)` is not a PR identity, so restrict candidates to this workflow's evaluation entry points and, when the run payload lists associated pull requests, require this PR's number to be among them. That drops `schedule`/`push` runs and the `pull_request` status-only runs (whose jobs are still undecided in the first moments of a run and would otherwise read as a real evaluation), and stops one head branch feeding two PRs with different bases from cross-cancelling. Fork runs report an empty PR list and still fall back to the repo/branch key. * Fix the jq event filter. `[...] | index(.event)` evaluates `.event` against the array, not the run, so it errored out; with the surrounding `|| true` the candidate list would silently come back empty and disable single-flight entirely. Use an explicit `or` chain instead. * Never let an archaeological run kill head coverage. `/evaluate <old-sha>` produced a newer run id, so it superseded (cancelled) the in-flight evaluation of the PR head and left that commit's required check pending with nothing left to resolve it. A run bound to a non-head commit now neither cancels, nor yields to, a run covering the head. Run-id ordering still breaks every symmetric race, so exactly one run survives. * Clear the eyes reaction on gate-terminal paths. The acknowledgement now fires for every authorized `/evaluate`, but `report-status` (which removes it) only runs when `should_eval == 'true'` -- so bare `/evaluate` guidance, an unresolvable commit, and defer/yield left the comment marked in-progress forever. * Paginate the jobs lookup, matching `eval_run_exists_for_head`, and document the gate concurrency trade-off (GitHub keeps one pending job per group, so the middle request of a same-PR burst misses its own ack). Validated with actionlint and a bash test covering the decision matrix, both symmetric races, and the jq filter against run-payload fixtures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 449babf9-dc33-4164-b226-e7bdc0318e49 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Amaury Levé <amauryleve@microsoft.com> Copilot-Session: 29ef3316-14f6-4dc1-a838-40f8be09f915 Copilot-Session: 449babf9-dc33-4164-b226-e7bdc0318e49 |
||
|
Jakub Florkowski
|
a1eec661a1 | Merge branch 'main' into kubaflo/dotnet-maui-skills-eval-fixes | ||
|
Jakub Florkowski
|
fcb7e5602d |
Fix SetAppTheme example: wrong BindableProperty owner (Copilot review)
The Copilot reviewer flagged this twice and was right both times:
label.SetAppTheme<FileImageSource>(Image.SourceProperty,
"logo_light.png", "logo_dark.png");
Two defects in one line:
- Image.SourceProperty is applied to a *Label* instance. The BindableProperty
must belong to the object it is set on.
- T is FileImageSource while Image.SourceProperty is ImageSource-typed, and the
arguments are bare strings.
Worse, it COMPILES - verified. SetAppTheme<T> takes a BindableObject and a
BindableProperty with no compile-time check that the property belongs to the
object, so a model copying this produces code that builds and silently does
nothing useful. That is precisely the failure class this PR exists to fix.
I introduced this. The earlier compile check used `var img = new Image();
img.SetAppTheme<FileImageSource>(...)` - I verified a variant rather than the
exact text I shipped, and the `label.` receiver slipped through.
Corrected to the right owner and type, with ImageSource.FromFile:
var image = new Image();
image.SetAppTheme<ImageSource>(Image.SourceProperty,
ImageSource.FromFile("logo_light.png"),
ImageSource.FromFile("logo_dark.png"));
This time the exact block was extracted programmatically from SKILL.md and
compiled verbatim in a net11.0-maccatalyst app rather than retyped. Also swept
the other seven skills and the theming reference file for the same
wrong-owner pattern - none found.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469
|
||
|
Abhitej John
|
6a98798417 |
Own CI/CD paths myself instead of auto-requesting Viktor (#962)
Replace @ViktorHofer with @AbhitejJohn on the /eng/ and /.github/workflows/ CODEOWNERS rules, keeping @JanKrivanek, so routine engineering and workflow PRs no longer auto-request Viktor as a reviewer. Other ownership rules are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29ef3316-14f6-4dc1-a838-40f8be09f915 |
||
|
Jakub Florkowski
|
cf626334ee |
Add regression-guard scenarios to theming, shell-navigation and DI evals
The gate keeps flapping on these three, and the artifacts show why: with 4 scenarios, ONE tie is mathematically disqualifying. 4 scenarios, 3W/1T/0L -> mean +30%, CI [-1.8%, +61.8%] FAIL 5 scenarios, 4W/1T/0L -> mean +32%, CI [+9.8%, +54.2%] PASS maui-collectionview already demonstrates this: it scored 4W/1T/0L on the last run and PASSED, because the regression-guard scenario added earlier gave it a 5th data point. The other three had no such margin. That the flapping is sampling noise rather than content is now directly evidenced - verified with git diff that the content was byte-identical across these runs: maui-theming |
||
|
Jakub Florkowski
|
6d1201c3ef |
Eliminate the last two tying scenarios in shell-navigation and DI
Latest gate run: maui-theming PASSED (+40%, 4/0/0) - the MergedDictionaries fix worked. But maui-dependency-injection and maui-shell-navigation flipped to ❌, both at 3W/1T/0L with ZERO losses: at 4 trials a single tie puts the CI lower bound at -1.8%, so any tie fails. Rather than argue noise, I diagnosed the two specific tying scenarios. maui-shell-navigation, "Set up Shell navigation with tabs and flyout": Both answers satisfied all four rubric items and were near-identical - except baseline set an explicit Route= on each ShellContent and the skill never mentioned Route at all (zero occurrences in the file). That omission is a real defect. Routing.cs:134 generates "D_FAULT_{TypeName}{++s_routeCount}" when Route is unset. Verified at runtime in a net11.0-maccatalyst app with three ShellContent elements: ShellContent Title='' Route='D_FAULT_ShellContent2' ShellContent Title='Active' Route='D_FAULT_ShellContent5' ShellContent Title='Archived' Route='archived' <- explicit The numbers are not sequential (2 and 5) because the counter is shared across all Shell element types, so they shift when pages are reordered or added. No stable absolute route ("//dashboard") or deep link can target them. Added the rule to the workflow plus the explanation, and set Route= on every ShellContent in the AppShell example. maui-dependency-injection, "Shell navigation auto-resolves DI-registered pages": baseline covered IQueryAttributable and the skill did not (the skill covered ActivatorUtilities and ContentTemplate, which baseline lacked - so it was trading one gap for another). Added a "Passing parameters to a DI-resolved ViewModel" section showing the actual combination: constructor injection for dependencies, IQueryAttributable for navigation parameters, and the fact that Shell applies query attributes to the BindingContext so no page wiring is needed. Measured (n=4/scenario, 32 trials across both skills, executor claude-opus-4.6, judge gpt-5.5): 32W / 0T / 0L mean +0.55 [95% CI +0.45, +0.65] 100% win rate Both previously-tying scenarios now win 4/4, and neither skill has a single tie left. Per-skill mean is +0.55 for each. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469 |
||
|
Jakub Florkowski
|
714b5975ea |
maui-theming: never Clear() MergedDictionaries when swapping a theme
The gate had maui-theming at 3W/1T/0L, +30% [-1.8%, +61.8%] - one tie away
from passing, on "Avoid common theming mistakes". Comparing the trajectories,
baseline and skilled were near-identical and both satisfied all four rubric
items, so there was nothing to win on... except that BOTH emitted the same
real bug:
var merged = Application.Current.Resources.MergedDictionaries;
merged.Clear();
merged.Add(new DarkTheme());
The default MAUI template merges Resources/Styles/Colors.xaml and Styles.xaml
into Application.Resources. Clear() removes those too, so every implicit style,
brush and colour in the app silently disappears.
Verified at runtime in a net11.0-maccatalyst app built from the default
template:
before Clear(): merged=2 'Primary' resolves=True value=[Color Red=0.317...]
after Clear(): merged=1 'Primary' resolves=False value=<GONE>
The skill previously had a weak comment acknowledging this ("assumes theme
dictionaries are the only merged dictionaries... move them to
Application.Resources directly instead"), which is both easy to miss and poor
advice. Replaced across SKILL.md (both occurrences) and theming-api.md with a
tracked-reference swap that removes only the previous theme:
static ResourceDictionary? _currentTheme;
if (_currentTheme is not null) merged.Remove(_currentTheme);
merged.Add(theme);
_currentTheme = theme;
Also verified at runtime: app styles survive across two consecutive theme
swaps and the dictionary count stays flat (2 -> 3 -> 3, 'Primary' OK
throughout), so it neither leaks nor unstyles the app.
Measured (n=4/scenario, 16 trials, executor claude-opus-4.6, judge gpt-5.5):
before 14W/1T/1L +0.33 [+0.21, +0.44] 88% win
after 15W/1T/0L +0.38 [+0.32, +0.43] 94% win
"Avoid common theming mistakes" - the scenario that tied on the gate - now
wins 4/4, and the remaining single tie is on a different scenario. Tightest
confidence interval of any arm measured in this PR.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469
|
||
|
Jakub Florkowski
|
4d61dd9e29 | Merge remote-tracking branch 'upstream/main' into kubaflo/dotnet-maui-skills-eval-fixes | ||
|
Jakub Florkowski
|
1db8796dc4 |
maui-theming: recommend a single-source palette instead of inline literals
maui-theming was the last skill not passing the gate: 1W/3T/0L, +10% [-21.8%, +41.8%]. No losses - three ties. I had put that down to a ceiling effect (its baseline Quality is 5.0/5) and suggested accepting it. That was wrong; the artifacts show a real, fixable gap. What the eval artifacts actually show: - Skilled answers were already LONGER than baseline on all three tied scenarios (2466 vs 2100, 2195 vs 1972, 1797 vs 1590 chars), so this was not the terseness problem fixed in the previous commit. - On "Detect and respond to system theme changes" the skilled answer covered strictly more than baseline (adding UiMode and Preferences) and still tied. More words were not converting into wins. - On "Add light/dark mode support using AppThemeBinding" BOTH answers scattered hardcoded hex literals across every element - while that scenario's rubric explicitly says "Does not suggest hardcoded color values as a final solution". Baseline at least mentioned moving them to App.xaml; the skill never demonstrated it. So the differentiator was structural, not volume. Added a "Define the palette once - don't scatter literals" section that makes the recommended final shape a single App.xaml source of truth: a raw Color palette plus implicit Styles (TargetType="ContentPage" ApplyToDerivedTypes="True", TargetType="Label") whose setters carry the AppThemeBinding. Pages then need no theming markup at all. Inline AppThemeBinding is reframed as the one-off case, and even then pointed at StaticResource keys rather than literal hex. The pattern is compile-verified in a net11.0-maccatalyst app (implicit styles with nested AppThemeBinding + StaticResource in a merged ResourceDictionary). Measured (n=4/scenario, 16 trials, executor claude-opus-4.6, judge gpt-5.5): before this commit 10W/5T/1L +0.22 [+0.09, +0.36] 63% win after 14W/1T/1L +0.33 [+0.21, +0.44] 88% win Ties collapse from 5 to 1, and all four scenarios now win predominantly - including "Add light/dark mode support using AppThemeBinding", which was the tie that motivated the change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469 |
||
|
Sayed Ibrahim Hashimi
|
7d5106946f |
adding scaffolding skill and test (#329)
* adding scaffolding skill and test * Update tests/dotnet-data/scaffold-generate-aspnet/fixtures/blazor-crud/Components/Routes.razor Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * updating pr based on copilot feedback * updating pr based on copilot feedback * Update tests/dotnet-data/scaffold-generate-aspnet/fixtures/blazor-crud/Components/Layout/ReconnectModal.razor.css Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update tests/dotnet-data/scaffold-generate-aspnet/fixtures/blazor-crud/Components/Layout/ReconnectModal.razor.css Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update tests/dotnet-data/scaffold-generate-aspnet/fixtures/blazor-crud/Components/Layout/ReconnectModal.razor.css Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update plugins/dotnet-data/skills/scaffold-generate-aspnet/SKILL.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update tests/dotnet-data/scaffold-generate-aspnet/fixtures/minimal-api/Program.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fixing warnings in test projects * fixing ci build issues * fixing eval.yaml * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Fix unresolved Copilot review comments on PR #329 - Add CODEOWNERS entries for scaffold-generate-aspnet skill and tests - Fix navbar toggler aria-controls/id mismatch in razor-pages-crud fixture Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * updating code owners for scaffold-generate-aspnet * Update tests/dotnet-data/scaffold-generate-aspnet/fixtures/blazor-crud/Components/Pages/Error.razor Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update tests/dotnet-data/scaffold-generate-aspnet/eval.yaml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update tests/dotnet-data/scaffold-generate-aspnet/fixtures/blazor-crud/Components/Pages/Error.razor Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update plugins/dotnet-data/skills/scaffold-generate-aspnet/SKILL.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update tests/dotnet-data/scaffold-generate-aspnet/eval.yaml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update tests/dotnet-data/scaffold-generate-aspnet/eval.yaml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @webreidi * Apply suggestion from @webreidi * Apply suggestion from @webreidi * Add Blazor CRUD application structure and initial components - Created Home, NotFound, and Weather pages with basic layout and functionality. - Implemented routing in the Router component. - Added models for Department and Employee with data annotations for validation. - Set up Program.cs for Blazor server-side rendering and configured HTTP request pipeline. - Included launch settings and appsettings for development environment. - Established CSS styles for the application and added favicon. - Created minimal API with TodoItem model and basic weather forecast endpoint. - Developed Razor Pages CRUD structure with models for Category and Product. - Implemented error handling and privacy policy pages in Razor Pages. * Remove SKILL.md documentation for ASP.NET Core scaffolding * Refactor eval.yaml to enhance scaffolding scenarios and improve grading configurations * Add scaffolding for Minimal API and MVC with existing DbContext, including models, DbContexts, and configuration files * Refine evaluation criteria for create-datadriven-aspnetcore skill by updating file paths and adding checks for Razor Pages and DbContext presence --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Dan Moseley <danmose@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Abhitej John <aajohn@microsoft.com> Co-authored-by: Wendy Breiding <55603905+webreidi@users.noreply.github.com> Co-authored-by: Wendy Breiding (She/Her) <webreidi@microsoft.com> |
||
|
Jakub Florkowski
|
5812d5af17 |
Address review feedback: guard the regression, pin versions, fix cross-skill tension
Follows @AbhitejJohn's review and the cross-family run (actions/runs/30313685452: 23/35 pass, no regressions). Guard the P0 regression (the review's highest-value item - "nothing stops the broken markup from creeping back"): - Added an `ItemSizingStrategy placement for uniform items` stimulus to tests/dotnet-maui/maui-collectionview/eval.yaml with an `output-not-matches` grader on `<(Linear|Grid)ItemsLayout[^>]*ItemSizingStrategy`. Verified the pattern catches both broken forms (LinearItemsLayout and GridItemsLayout) and passes the corrected form, so the exact MAUIX2002 build break this PR fixed is now regression-guarded. A regex guard rather than a real compile: the eval runner has no MAUI workload, and this catches the specific defect deterministically. - The new scenario initially LOST (-0.4, 0, -0.4): the guard passed in both arms, but baseline gave richer sizing guidance. Enriched the Performance Tips entry with the default (MeasureAllItems), a full XAML example, and the four cases where MeasureFirstItem is wrong (variable heights, DataTemplateSelector, atypical first item, size-depends-on-late-data). The scenario now wins 3/3. maui-collectionview, n=3, 15 trials: 12W/2T/1L, mean +0.29 [95% CI +0.16, +0.42] - up from +0.16 [-0.00, +0.32] before the enrichment. Minor items from the review: - Cross-skill tension: the CollectionView "Basic Setup" page sets BindingContext by instantiating the ViewModel inline, which reads oddly next to maui-dependency-injection's constructor-injection guidance. Added the one-liner pointing DI apps at constructor injection. - Version-pinned the SDK-band-sensitive claims: the XC0022-XC0025 table now states it is verified against .NET 10/11 with a note to re-check BuildException.cs on newer SDKs; the MauiEnableXamlCBindingWithSourceCompilation default is pinned to .NET 10/11; the ListView [Obsolete] note now says explicitly that it does not apply to .NET 9 and earlier, so a model checks the target framework first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469 |
||
|
Abhitej John
|
ce49205f11 |
Merge pull request #956 from dotnet/abhitejjohn-bind-evaluate-to-reviewed-sha
Bind PR evaluation to the reviewed commit |
||
|
AbhitejJohn
|
618d19de8d |
Address review: robust SHA parse + accurate dispatch doc
Parse '/evaluate <sha>' from issue comments with a bash regex instead of a sed BRE, avoiding word-boundary escape ambiguity across sed implementations and matching the validation used elsewhere in the gate. Reject over-length hex strings to the guidance path (fail-safe). Clarify the workflow_dispatch docs: the PR head travels in the head_sha input, while a dispatched run's github.sha is the default-branch tip. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29ef3316-14f6-4dc1-a838-40f8be09f915 |
||
|
Abhitej John
|
9d08a0a31d |
Bind PR evaluation to the reviewed commit
Harden the /evaluate pipeline in evaluation.yml so a run is always tied to one specific reviewed commit captured when the trigger fires, instead of resolving the live branch head downstream. Every job (gate, discover, evaluate, report-status) now consumes the single gate-bound SHA. - Add a pull_request_review trigger (recommended path): the run binds to review.commit_id, so no SHA needs to be typed. Files changed -> Review changes -> Submit review with /evaluate in the body. - Comment path (/evaluate <sha>) now requires an explicit SHA that must belong to the PR; a bare /evaluate posts copy-paste-ready guidance and points to the review flow. - Label and workflow_dispatch paths bind to the head carried in the event payload / passed by the triage worker. - Warn (log + PR comment) when the bound commit is not the current head. - Only write-access triggers (label, dispatch) may cancel an in-progress run; comment/review triggers queue so an authorized run finishes. - Reachability check tolerates PRs >250 commits via a compare-ancestry fallback, staying fail-closed. - discover fails closed if the bound commit can't be checked out. - Pass inputs.plugin and actor logins via env instead of inlining. - Update docs/design/pr-triage-workflows.md (4 entry points, commit binding). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29ef3316-14f6-4dc1-a838-40f8be09f915 |
||
|
Jakub Florkowski
|
9c02b41315 |
Fix the 3 skills that failed the official evaluation
The maintainer-triggered /evaluate run (actions/runs/30246707498) passed 4 of 7 skills. maui-collectionview - the P0 this PR exists to fix - passed 4W/0T/0L, alongside maui-safe-area (+100%), maui-app-lifecycle (+55%) and maui-data-binding (+40%). This commit addresses the other three. Only those three files change; the four passing skills are untouched. Diagnosis from the eval artifacts: in every failing scenario the skilled answer was SHORTER than baseline (shell 1804 vs 2433 chars, theming 1347 vs 1541). The scope-control wording added earlier in this PR - needed to stop maui-collectionview over-applying - had over-corrected the other skills into answering too narrowly. Baseline kept winning by including the follow-through code that makes a fix actionable. maui-shell-navigation (was 2W/2T/0L, +35% not credible): - Added "answer narrowly, but completely": show AppShell markup *and* RegisterRoute, GoToAsync *and* the receiving IQueryAttributable, and present both parameter-passing approaches with when each fits. - Local n=4: 16W/0T/0L, mean +0.66. maui-dependency-injection (was 1W/2T/1L, +0.0%): Two claims the judge repeatedly called "misleading"/"overconfident", both verified wrong against source: - "Unregistered page silently skips injection - dependencies are silently null, no exception is thrown". Routed pages go through ActivatorUtilities.GetServiceOrCreateInstance (Routing.cs:336), which injects registered dependencies and THROWS when one cannot be resolved. ContentTemplate pages go through Activator.CreateInstance (ElementTemplate.cs:33) - no DI at all, and a MissingMethodException rather than null dependencies. Section rewritten around that real distinction. - "must be AddTransient" softened to "prefer", with the case where Singleton is defensible. - Also added the alternatives the judge rewarded baseline for: explicit IServiceScopeFactory.CreateScope() and the factory pattern. - Local n=4: 15W/1T/0L, mean +0.49 (the 3 consistent losses are gone). maui-theming (was 3W/0T/1L, +20% not credible): - All losses were the same complaint: baseline also showed how to swap merged dictionaries and hook RequestedThemeChanged. An instruction to "be complete" did not stick, so the code now lives inline in the DynamicResource pitfall where the model actually reads it. - Scoped the Android UiMode rule to runtime/system theme switching - it was being bolted onto narrow AppThemeBinding questions and judged "ancillary and slightly overstated as required". - Expanded the C# helpers so both SetAppThemeColor and generic SetAppTheme<T> are shown; all three overloads compile-verified in a net11.0-maccatalyst app. - Local n=4: 10W/5T/1L, mean +0.22 [95% CI +0.09, +0.36] - CI now above zero. All three were measured with the same harness as the official run (vally, executor claude-opus-4.6, judge gpt-5.5), at n=4 per scenario rather than the official n=1, since single-trial cells cannot separate signal from noise - maui-shell-navigation originally "failed" with zero losses purely on CI width. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469 |
||
|
Jakub Florkowski
|
df0806a4dc |
Resolve Copilot review comment: conflicting XC0022 guidance
The data-binding "Common Pitfalls" row said "enable XC0022 as error" while the "Enforce binding warnings as errors" section above recommends XC0022;XC0025 paired with MauiEnableXamlCBindingWithSourceCompilation - two different prescriptions for the same task. The pitfall row now points at that section as the single source of truth instead of restating a partial version. Copilot's four earlier comments on this PR (theming escaped pipe, ObservableCollection contradiction, ShellContent eager form, QueryProperty example) were already addressed in previous commits; the ShellContent and QueryProperty findings matched gpt-5.6-sol's and gpt-5.5's independently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469 |
||
|
Jakub Florkowski
|
0b86a41634 |
Apply multi-model review feedback (round 3)
Round-3 review by claude-opus-5, claude-opus-4.8, gpt-5.6-sol, gpt-5.5: 3 SHIP, 1 FIX. Both of gpt-5.6-sol's blocking items are fixed here, along with the non-blocking findings from opus-5 and gpt-5.5. gpt-5.6-sol (blocking): - The Shell rules table used an undeclared `pages:` prefix, and its "Not this" cell claimed `<ShellContent Content="..."/>` constructs pages eagerly. `ShellContent` is `[ContentProperty(nameof(Content))]` with `Content` typed `object` (ShellContent.cs:15,45), so the eager form is nested markup, not a string attribute. Row rewritten with a declared `xmlns:views` and the real eager form `<ShellContent><views:MyPage /></ShellContent>`. - QueryProperty prose said "ViewModel" while the example was still a ContentPage. Also flagged independently by gpt-5.5 (consensus). opus-5 (non-blocking, both real): - The rules-table cell I added in round 2 contained `</...>` — not valid XML. A model lifting that cell verbatim emits an unparseable .csproj. Reworded to prose so there is no truncated tag to copy. - The QueryProperty ViewModel example used a plain auto-property. On a page that was harmless (code-behind reads it); on a ViewModel it is the binding target, and Shell applies query attributes *after* BindingContext is set, so a non-notifying property leaves the binding on its initial value. Switched to [ObservableProperty] and explained why. - The "complete, copy-pasteable page" never set BindingContext, so pasting it renders blank — the exact failure maui-data-binding lists first. Added it. Both changed snippets were compile-verified in a net11.0-maccatalyst app (flagship CollectionView page with root x:DataType + BindingContext, and the [QueryProperty] + [ObservableProperty] ViewModel with CommunityToolkit.Mvvm). Measured (maui-collectionview, paired base + current run per round, same session, executor claude-opus-4.6, judge gpt-5.5, n=3/stimulus, 12 trials/arm): round 3 base +0.00 this PR +0.23 delta +0.23 round 4 base -0.07 this PR +0.17 delta +0.24 round 5 base +0.00 this PR +0.13 delta +0.13 Round 5 is this commit. The delta is smaller than rounds 3-4 but the 95% CIs overlap heavily at n=12 (base CI +/-0.24), so this is not a demonstrated regression. The changes in this commit are correctness fixes - invalid XML, a silently-broken binding, and an inaccurate Shell claim - which are worth making regardless of a benchmark that cannot resolve a 0.1 difference. A higher-n run follows to tighten the estimate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469 |
||
|
Jakub Florkowski
|
746679a716 |
Apply multi-model review feedback (round 2) + Copilot comment
Round-2 review by claude-opus-5, claude-opus-4.8, gpt-5.6-sol, gpt-5.5:
2 SHIP, 2 FIX. All blocking and consensus findings addressed.
Consensus finding (opus-5 + gpt-5.6-sol) — the flagship "complete,
copy-pasteable page" declared xmlns:models but had no root x:DataType, so its
outer ItemsSource binding was an uncompiled reflection binding. That undercut
the skill's own compiled-bindings guidance and left rubric credit unclaimed on
collectionview stimulus 1. Added xmlns:vm + root x:DataType.
opus-5 blocking claim — <WarningsAsErrors>XC0022;XC0025</WarningsAsErrors>
(pre-existing, line 110 on main) was said to break any build using Source= or
RelativeSource bindings, because XC0025 means "binding not compiled due to an
explicit Source" and MauiEnableXamlCBindingWithSourceCompilation defaults true
only under AOT/full-trim.
Tested empirically: a net11.0-maccatalyst app with that exact
WarningsAsErrors value plus both a Source={x:Reference} binding and a
RelativeSource binding BUILDS CLEAN. The XC diagnostics are not emitted on
the current SourceGen path (Descriptors.cs has XC0022/0023/0025 commented
out), so the claim does not reproduce on current MAUI.
It can still bite on the older XamlC path, and the reasoning is sound, so
the guidance now pairs the switch with the setting and states the caveat
rather than changing the rubric-rewarded value.
Other round-2 fixes:
- "A binding falls back to reflection (XC0022/XC0023) | Fix the path" still
carried pre-correction semantics; neither code concerns the binding path
(that is XC0045). Reworded. (opus-5)
- "Everything below is a suggestion the agent may decline" literally demoted
the Key Rules and the Validation checklist. Scoped it to the API reference
sections and promoted x:DataType to a fourth non-negotiable. (opus-5)
- AddScoped lifetime table row and checklist still implied a manual scope is
the only scope; both now reflect window lifetime. (gpt-5.6-sol)
- Shell "Option 2: QueryProperty — apply directly on the page class"
contradicted the new rule table and the eval rubric ("on the ViewModel, not
the page"). (opus-5)
Copilot review comment — "Always use ObservableCollection<T>, not List<T>.
Swapping a List forces a full re-render" contradicted this PR's own
"use ObservableCollection when the list mutates" rule, and the claim was
imprecise: replacing ItemsSource re-renders regardless of collection type.
Rewritten around INotifyCollectionChanged and in-place mutation.
Not changed: gpt-5.6-sol and opus-5 both suggested editing
tests/dotnet-maui/*/eval.yaml rubrics (safe-area "replaced by", DI "behaves
like Singleton") to match the corrected facts. Changing the benchmark in the
same PR that changes the skill would invalidate the comparison, so these are
left for maintainers and called out in the PR instead.
Measured (maui-collectionview, executor claude-opus-4.6, judge gpt-5.5,
n=3/stimulus, 12 trials/arm, base re-run in the same session each time):
round 3 base +0.00 this PR +0.23 delta +0.23
round 4 base -0.07 this PR +0.17 delta +0.24
Absolute numbers move with run-to-run noise (three independent base runs gave
-0.13, +0.00, -0.07), but the advantage over base is stable at ~+0.24 mean and
~+25pp win rate across every paired run.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469
|
||
|
Jakub Florkowski
|
ad057a07db |
Apply multi-model review feedback (4-model consensus round 1)
Reviewed by claude-opus-5, claude-opus-4.8, gpt-5.6-sol and gpt-5.5.
Verdicts were split (2 SHIP / 1 FIX / 1 NEEDS_CHANGES), so the blocking and
consensus findings are addressed here. Every disputed factual claim was
re-verified against the dotnet/maui source before changing anything.
Factual corrections (reviewers were right, I was wrong):
- ListView IS marked [Obsolete] as of .NET 10 ("ListView is deprecated. Please
use CollectionView instead.", ListView.cs:19). Verified it is present on
main, net10.0 and release/10.0.1xx-sr7, and absent on release/9.0.1xx. The
previous "it is not removed or obsolete" wording was wrong. (gpt-5.6-sol)
- MAUI DOES create one IServiceScope per window
(MauiContextExtensions.MakeWindowScope, called from the iOS/Android/Windows/
Tizen ApplicationExtensions), so "a Scoped registration resolves from the
root scope and behaves like a Singleton" was over-broad. Reworded to keep the
rubric-rewarded "no built-in request scope like ASP.NET Core" phrasing while
stating the window-scope reality. (opus-5, gpt-5.6-sol)
- ItemTapped -> SelectionChanged is not behaviour-preserving (SelectionChanged
only fires when the selection changes), and RowHeight -> MeasureFirstItem is
not an explicit row height. Split into accurate rows. (gpt-5.6-sol)
Rules that could suppress rubric-rewarded answers (the main eval risk):
- The ListView stop-condition ("do not migrate unless the user asked to
migrate") fired on eval stimulus 4, which asks "Should I keep using ListView
in MAUI?" - the user asks *whether*, not *to*. Reworded to recommend
CollectionView when asked which control to use, while still forbidding
silent rewrites. Flagged independently by opus-5 and gpt-5.5.
- data-binding's blanket "do not convert a working binding" could talk the
agent out of the rubric-rewarded x:DataType recommendation. Scoped it. (opus-5)
- "Point out a concrete defect or say nothing" -> answer the question asked;
"say nothing" invited non-answers, which judges penalise. (opus-5)
Internal contradictions introduced by this PR:
- data-binding Common Pitfalls still said "enable XC0025 as error" for a
missing x:DataType; XC0022 is the missing-x:DataType code. (opus-4.8)
- The rules table said x:DataType "root element only" two rows above a row
requiring one per DataTemplate. Reworded to "wherever a binding scope
starts". (opus-5, gpt-5.6-sol)
- The Validation checklist re-armed items the Scope Control section had just
demoted; scoped it to markup the agent actually wrote. (opus-5)
- Dropped the "Using ListView instead of CollectionView" pitfall row, which
contradicted the new migration section. (opus-5)
Other:
- Removed the escaped pipe (\|) from the theming rules table - a model reading
raw Markdown could copy it into C#. (Copilot reviewer, opus-5)
- Softened "Apply these three rules to every theming answer" so narrow
questions don't attract unrelated UiMode/UserAppTheme guidance. (gpt-5.5)
- De-duplicated the AddScoped guidance that appeared 4x. (opus-5)
Measured effect (maui-collectionview, executor claude-opus-4.6, judge gpt-5.5,
n=3/stimulus, 12 trials/arm, base re-run in the same session):
base (main, no fix) 33% win 4 wins / 4 ties / 4 losses mean +0.00 [-0.22,+0.22]
previous commit 42% win 5 wins / 6 ties / 1 loss mean +0.13 [-0.03,+0.30]
this commit 58% win 7 wins / 5 ties / 0 losses mean +0.23 [+0.10,+0.36]
Zero losses, and the confidence interval is now entirely above zero - the
first arm in this series with a statistically significant positive impact.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469
|
||
|
Jakub Florkowski
|
8a200fa49d |
Fix markdownlint MD012 (duplicate blank line)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469 |
||
|
Jakub Florkowski
|
b07628c9a5 |
maui-collectionview: fix eval-identified loss causes
Driven by a local skilled-vs-baseline run (vally 0.10, executor claude-opus-4.6, judge gpt-5.5, n=3 per stimulus). The judge rationales named two concrete, fixable causes for the remaining losses: 1. "Response B introduces an unresolved x:DataType prefix unless additional namespace declarations are added" (penalised on 2 trials). The skill used x:DataType="models:..." in six snippets but never declared xmlns:models, so the examples were not copy-pasteable. Basic Setup is now a complete ContentPage including the xmlns:models declaration, with an explicit instruction to carry the matching xmlns when handing over a snippet. 2. Baseline beat the skill on the ListView/ViewCell migration stimulus by supplying nuance the skill lacked - that ListView still exists, plus event/API replacements. Added a "Migrating from ListView" section with a verified mapping table (ViewCell, ItemSelected/ItemTapped -> SelectionChanged, IsPullToRefreshEnabled -> RefreshView, IsGroupingEnabled -> IsGrouped, HasUnevenRows -> ItemSizingStrategy, and the missing separator APIs). Every member was confirmed against Controls/PublicAPI.Shipped.txt. Measured effect on the same eval (12 trials per arm, identical settings): original skill 17% win rate 2 wins / 4 ties / 6 losses mean -0.13 this PR 42% win rate 5 wins / 6 ties / 1 loss mean +0.13 Losses drop 6 -> 1 and the mean impact flips from negative (the FIX-REGRESSION signal in #895) to positive. n=12 on a single executor, so directional rather than significant, but the loss collapse is the intended effect of the regression fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469 |
||
|
Jakub Florkowski
|
84050721e0 |
Fix dotnet-maui skill regressions and sharpen decisiveness (#895)
Addresses the cross-family evaluation findings in dotnet/skills#895 for the
dotnet-maui plugin (8 skills, 48% pass, 1 P0).
Every claim below was validated empirically by building a real .NET MAUI app
against the current SDK, not by code reading alone.
FIX-REGRESSION - maui-collectionview (P0):
- ItemSizingStrategy was documented on LinearItemsLayout in both SKILL.md and
references/collectionview-api.md. It is declared on StructuredItemsView, so
the documented markup is a hard build break:
"error MAUIX2002: No accessible property, BindableProperty, or event found
for ItemSizingStrategy". Corrected to set it on CollectionView, with the
invalid form kept as an explicit counter-example.
- Added a "Scope Control" section with stop-conditions (do NOT act when...),
narrowed the frontmatter trigger, demoted the non-essential guidance to
suggestions the agent may decline, and reduced the three non-negotiable
rules to those that actually break code. This targets the ~30% of trials
where skilled output was worse than baseline through over-application.
- Added a validation checklist.
HIGH severity, non-compiling - maui-app-lifecycle:
- The iOS ConfigureLifecycleEvents examples called .DidBecomeActive() and
.WillResignActive(), which do not exist:
"error CS1061: IiOSLifecycleBuilder does not contain a definition for
DidBecomeActive". Corrected to .OnActivated() / .OnResignActivation() and
documented that the builder method names deliberately differ from the UIKit
selector names (the mapping tables now show both columns).
Correctness - maui-data-binding:
- The XC0022-XC0025 meanings table was wrong on all four rows. Corrected
against Build.Tasks/BuildException.cs and ErrorMessages.resx (XC0022 =
binding without x:DataType, XC0023 = x:DataType explicitly null, XC0024 =
x:DataType from outer scope, XC0025 = explicit Source).
Correctness - maui-safe-area:
- Claimed WindowSoftInputModeAdjust.Resize was "removed". It still compiles
and is not obsolete. Reframed as Android-only and superseded, preserving
the skill's brevity (EFFICIENT-WIN - deliberately not grown).
STRENGTHEN - decisiveness (ties dominated these skills):
- Added a compact "Rules That Change the Answer" table to maui-shell-navigation,
maui-data-binding, maui-dependency-injection and maui-theming, converting
reference prose into when-A-do-B guidance, each with an explicit "do not
churn working code" stop-condition.
Notably NOT changed:
- maui-theming's {AppThemeBinding Light={DynamicResource ...}} guidance. This
looked invalid from source reading, but a runtime probe proved it is fully
dynamic (value tracked a dictionary swap: #FF0000 -> #0000FF, while the
StaticResource control stayed #FF0000). The original guidance is correct.
- maui-shell-navigation fixture/SDK pinning. Per the #909 correction the
errored trial was a judge-side disabled-PAT failure, not fixture
nondeterminism, so the skill was treated as STRENGTHEN.
- dotnet-maui-doctor (EXEMPLAR) is unchanged.
Validation:
- skill-validator check --plugin ./plugins/dotnet-maui -> all checks passed
(8 skills, 0 errors, 26 reference files scanned).
- All corrected snippets compile in a net11.0-maccatalyst MAUI app; both
original buggy forms reproduce the exact errors quoted above.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 58513e0c-4086-4112-9ea1-95dbd8e8e469
|
||
|
Jan Krivanek
|
9b2706630d |
Rewrite system-text-json-net11 skill as imperative, decisive guidance (#926)
The dotnet11/system-text-json-net11 skill scored 0%% pass across all five model families (issue #902, ADD-DECISIVENESS): it read as reference prose and the judge saw no behavior change vs baseline (7 ties, 67%% invocation). Rewrite it into imperative when-A-do-B-verify-C guidance: - Sharper frontmatter USE FOR / DO NOT USE FOR with trigger keywords to improve discovery. - Decision table mapping each request to the exact API and the anti-pattern to avoid. - Explicit DO/DON'T that target the eval's failure modes: no custom JsonNamingPolicy subclass, no cast of non-generic JsonTypeInfo, no try/catch to probe metadata. - net11.0 run instructions (file-based app + project) so the program is actually executed. - Verification checklist and common-pitfalls table. Validated with skill-validator check (all checks passed). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> |
||
|
Abhitej John
|
f09fd590c4 |
Merge pull request #932 from dotnet/abhitejjohn-bridge-eval-report-gap
Enrich Vally evaluation PR comment (legend, reason, overfit, activation, per-scenario detail) |
||
|
leslierichardson95
|
10331dbf09 |
Cut mcp-csharp-debug skill (#938)
Remove the mcp-csharp-debug skill and its eval tests. Per the dotnet-ai skills-eval (#889) and telemetry review, the skill had weak eval impact (+0.27) and thin real-world adoption (3 users, 0 with >=5 invocations), making it a cut candidate rather than a strengthen target. - Delete plugins/dotnet-ai/skills/mcp-csharp-debug/ - Delete tests/dotnet-ai/mcp-csharp-debug/ - Remove CODEOWNERS entries for the skill and its tests - Remove dangling references (frontmatter DO NOT USE FOR, Stop Signals, Related Skills) from mcp-csharp-create, mcp-csharp-test, mcp-csharp-publish - Update skill-validator doc example to a surviving skill Refs dotnet/skills#889 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6a8ce111-58e5-48a7-9f7f-8de972bdc693 |
||
|
Amaury Levé
|
71414ce000 |
Improve dotnet-test eval coverage and efficiency (#917)
* Improve dotnet-test eval coverage and efficiency Address remaining high-confidence items from #899 by bounding the code-testing pipeline and adding eval coverage for grade-tests and find-untested-sources. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e430fee9-d3df-4ef5-85a4-745ae4b17046 * Fix dotnet-test eval activation and quality Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9c5c1a52-4f99-49d6-b503-1bec713a6e98 * Strengthen dotnet-test skill activation Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9c5c1a52-4f99-49d6-b503-1bec713a6e98 |
||
|
Abhitej John
|
081a817700 |
Merge pull request #881 from dotnet/dependabot/nuget/dot-config/all-other-nuget-b26771954b
Bump the all-other-nuget group with 7 updates |
||
|
Abhitej John
|
9f7d1576bd |
Address PR review: rank-order high-priority details; explicit non-success fallback
- consolidate.mjs: sort the high-priority detail set by rank so failing (❌) blocks are budgeted before inconclusive (⚠️) ones; previously skill-name order could let a ⚠️ block consume budget a later ❌ block needed. - evaluation.yml: add an explicit branch for a non-success evaluate result (failure/cancelled) so the 'produced no results' guidance is reserved for a successful job that emitted zero verdicts. (Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Amaury Levé
|
5fff8f1022 |
Improve dotnet-test-migration skill signal (#929)
* Improve migration skill eval signal Trim unnecessary xUnit-to-MSTest workflow turns and make MSTest v1/v2 migration preserve the detected test platform. Add focused cost and activation-boundary eval coverage for issue #900. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 860bd5ec-3bee-44ec-8fad-1bdcc745b9e1 * Strengthen migration evaluation signal Use the first evaluation's per-trial evidence to remove an inert scenario, cover semantic assertion migration, broaden MSTest risk guidance, and make testsettings migration objective. Also fix the MSTest 4.1/Test SDK compatibility rule and add a fast path to reduce retry-heavy migrations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 860bd5ec-3bee-44ec-8fad-1bdcc745b9e1 * Fix migration eval review findings Move the sequence grader into its matching stimulus, positively require element-wise comparison, and make runsettings XML checks tolerant of casing and whitespace. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 860bd5ec-3bee-44ec-8fad-1bdcc745b9e1 * Strengthen migration eval execution signal Convert weak advice-only scenarios into verified migrations, require test discovery and complete framework state, and correct misleading migration mappings found in eval replay. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06f10e04-4c49-40c3-b663-69be4525e4cb --------- Copilot-Session: 860bd5ec-3bee-44ec-8fad-1bdcc745b9e1 Copilot-Session: 06f10e04-4c49-40c3-b663-69be4525e4cb |
||
|
Abhitej John
|
2630435776 |
Address PR review: two-phase detail budgeting respects triage priority
The greedy budget loop could include a passing (✅) detail after skipping an oversized failing/inconclusive one. Select in two phases instead: fit as many ❌/⚠️ blocks as possible, and only surface ✅ blocks when every high-priority block fit. (Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Abhitej John
|
32c13da92e |
Address PR review: count activation coverage over expected scenarios only
activationCell() now computes activated/total over scenarios where activation is expected (expectActivation !== false), matching the missingExpected logic. Scenarios meant to stay dormant no longer drag down coverage (e.g. a correct dormant scenario previously showed 1/2 instead of 1/1). Legend and InvestigatingResults.md wording updated to match. (Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Jan Krivanek
|
15c336709e |
Fold Close Stale PRs into deterministic pr-triage stale sweep (#928)
Replace the agentic close-stale-prs.agent.md with a deterministic stale-PR sweep hosted in pr-triage-batch.yml. The new stale-sweep job runs .github/scripts/pr-stale-sweep.sh weekly (cron 17 4 * * 1) and on manual dispatch (stale_sweep=true), warning about and closing PRs with no non-bot activity for 30/37 days. Preserves the original policy (no-stale and maestro exemptions, drafts included, non-bot activity timer) without model calls or token cost. Deletes close-stale-prs.agent.md and its compiled lock file, and updates docs/design/pr-triage-workflows.md. Fixes #915 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> |
||
|
Abhitej John
|
15dae68800 |
Address review: drop dead MessagePack pins, fix stale comments
SDK 1.0.7's net10.0 nuspec only depends on Microsoft.Extensions.AI.Abstractions and Microsoft.Extensions.Logging.Abstractions -- it no longer pulls in StreamJsonRpc/MessagePack/Nerdbank.MessagePack. Verified empirically: with the Nerdbank.MessagePack and MessagePack pins removed, restore succeeds, those packages (and StreamJsonRpc) vanish from the transitive graph entirely, and 'dotnet list package --include-transitive --vulnerable' reports no vulnerable packages. Removed both dead pins and their stale comments. Kept the SQLitePCLRaw.lib.e_sqlite3 pin: it comes via Microsoft.Data.Sqlite (not the Copilot SDK) and removing it reintroduces the vulnerable native lib 2.1.11 (NU1903 / GHSA-2m69-gcr7-jv3q). Refreshed its comment's stale Microsoft.Data.Sqlite version (10.0.7 -> 10.0.10). Fixed the OnPermissionRequest comment: in SDK 1.x PermissionRequest is a discriminated union with per-kind path data (Read.Path, Write.FileName, Shell.FullCommandText/PossiblePaths); the old 'no path data' note was stale. Enforcement still lives in Hooks.OnPreToolUse (unchanged). src + tests build 0 warnings/0 errors; all 604 tests pass; 0 vulnerable pkgs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 849ee596-027f-48c8-afde-9930ecc12719 |
||
|
Abhitej John
|
6565d794ef |
Address PR review: escape details text, exclude errored trials, budget comment size
- Escape skillName/reason in per-skill <details> via td() (Copilot) - Exclude errored trials from per-scenario W/T/L so scenario rows match the verdict summary (y87feng) - Budget per-skill details under GitHub's 65,536-char comment limit, keeping failing/inconclusive details first and noting how many were omitted (y87feng) - Restore legacy '⚠️ NOT ACTIVATED' wording in the skill-validator doc and clarify the current Vally '⚠️ N/total' format alongside it (Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Abhitej John
|
76b4c74a31 |
docs: add before (pre-change) Vally PR comment screenshot
Baseline render of the same real artifacts using the pre-change consolidate.mjs, for before/after comparison in the PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |