Rewrite system-text-json-net11 skill as imperative, decisive guidance - #926
Conversation
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>
|
/evaluate |
📊 Skill Evaluation Results1 skill(s) evaluated — 1 improved, 0 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0).
🔍 Full Results - additional metrics and failure investigation steps ▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
There was a problem hiding this comment.
Pull request overview
This PR rewrites the dotnet11/system-text-json-net11 skill to be more imperative and behavior-shaping (vs. reference prose), aiming to improve cross-family eval outcomes by increasing decisiveness and verifiable execution.
Changes:
- Replaces descriptive prose with an imperative “decision table” + explicit DO/DON’T rules for the three .NET 11 System.Text.Json APIs.
- Adds runnable
net11.0execution instructions (file-based and project-based) plus a worked example. - Adds a validation checklist and “common pitfalls” table to enforce verification and avoid known anti-patterns.
Show a summary per file
| File | Description |
|---|---|
| plugins/dotnet11/skills/system-text-json-net11/SKILL.md | Rewrites the skill into decisive, imperative guidance with runnable instructions, a decision table, and validation/pitfalls sections. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
plugins/dotnet11/skills/system-text-json-net11/SKILL.md:153
- The worked example claims to demonstrate “typed metadata + PascalCase”, but
Person’s properties are already PascalCase (Name,Age), so the naming policy doesn’t visibly change the output. Use non-PascalCase member names so the example actually provesJsonNamingPolicy.PascalCaseis applied.
string json = JsonSerializer.Serialize(new Person("Jane", 30), typeInfo);
Console.WriteLine(json);
// {"Name":"Jane","Age":30}
record Person(string Name, int Age);
plugins/dotnet11/skills/system-text-json-net11/SKILL.md:107
- In the
TryGetTypeInfoexample,infois declared as nullable (JsonTypeInfo<Person>?) but the sample uses the null-forgiving operator (info!). This teaches an unsafe pattern; prefer a guard that makes the non-null assumption explicit and use braces for readability.
if (options.TryGetTypeInfo<Person>(out JsonTypeInfo<Person>? info))
Console.WriteLine($"Resolved: {info!.Type.Name}");
else
Console.WriteLine("Type info not available");
- Files reviewed: 1/1 changed files
- Comments generated: 1
|
👋 @JanKrivanek — this PR has 1 unresolved review thread(s). When you're ready, please address the feedback and push an update; the triage bot will pick up the next state automatically. (Add the |
Address PR review feedback and fix runtime gotchas found by actually running every snippet on the net11.0 SDK: - Make the Rule 2, Rule 3, and worked-example snippets self-contained (imports, options, and Person defined) so they compile and run if copy/pasted. - The worked example now uses lowercase record members (name/age) so JsonNamingPolicy.PascalCase visibly rewrites them to Name/Age in the output. - Replace the null-forgiving info! with an explicit 'is not null' guard and braces in the TryGetTypeInfo example. - Document that GetTypeInfo<T>()/TryGetTypeInfo<T>() require a TypeInfoResolver (they throw NoMetadataForType otherwise) — verified against the .NET 11 SDK. - Recommend a console project as the primary runnable host and warn that file-based apps (dotnet run app.cs) disable System.Text.Json reflection, so plain serialization throws unless you set DefaultJsonTypeInfoResolver. - Add matching decision-table note, checklist items, and pitfalls. Every snippet was executed on 11.0.100-preview.5 and produces the documented output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Skill Coverage Report
Uncovered:
|
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
plugins/dotnet11/skills/system-text-json-net11/SKILL.md:122
- Same issue as above: this snippet includes the file-based-app-only
#:propertydirective, but the surrounding section reads like general guidance. Add a short note so readers don’t paste it into a.csprojand get a compile error.
#:property TargetFramework=net11.0
plugins/dotnet11/skills/system-text-json-net11/SKILL.md:199
- The worked example starts with the file-based-app
#:propertydirective, but this section isn’t explicitly limited to file-based apps. Add a note so the snippet can be safely copied into a.csprojproject (where#:directives won’t compile).
#:property TargetFramework=net11.0
- Files reviewed: 1/1 changed files
- Comments generated: 1
|
/evaluate |
Add an inline note above each file-based #:property directive explaining that it only applies to file-based apps (dotnet run app.cs) and must be replaced with <TargetFramework>net11.0</TargetFramework> in a .csproj project, addressing PR review feedback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/evaluate |
📊 Skill Evaluation Results1 skill(s) evaluated — 0 improved, 1 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0).
🔍 Full Results - additional metrics and failure investigation steps
|
- Wrap generic API signatures (GetTypeInfo<T>(), JsonTypeInfo<T>, etc.) in backticks in the frontmatter description so Markdown/HTML renderers don't drop the <T> as a stray tag in skill catalogs. - Add 'using System.Text.Json;' and 'Console.WriteLine(json);' to the Rule 1 snippet so it is self-contained and prints its output. Verified it emits the documented PascalCase JSON on a net11.0 project host. Addresses PR review feedback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/evaluate |
|
❌ Evaluation failed. View workflow run |
Match the canonical form used in the API-replacement table (JsonSerializerOptions.TryGetTypeInfo<T>(out JsonTypeInfo<T>? info)) so the frontmatter is consistent and keyword matching is clearer. Addresses PR review feedback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
📊 Skill Evaluation Results1 skill(s) evaluated — 0 improved, 1 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0).
🔍 Full Results - additional metrics and failure investigation steps
|
The decision-table verify cell and the three PascalCase snippets (Rule 1,
Option A, Option B) used FirstName/LastName output, while the canonical
worked example uses Person(string name, int age) => "Name"/"Age". Switch
them to serialize { name, age } so every PascalCase example consistently
produces {"Name":"Jane","Age":30}. Verified both the project-host and
file-based snippets emit that output on net11.0.
Addresses PR review feedback.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/evaluate |
📊 Skill Evaluation Results1 skill(s) evaluated — 0 improved, 1 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0).
🔍 Full Results - additional metrics and failure investigation steps
|
📊 Skill Evaluation Results1 skill(s) evaluated — 0 improved, 1 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0).
🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
github-actions
Bot
added
waiting-on-review
|
✅ Evaluation passed for |
|
@JanKrivanek Thanks for the rewrite — turning the old prose into a decisive "symptom → do this → never do this → verify" table makes this much more actionable, and the frontmatter triggers now line up cleanly with what the evaluation checks. One reviewer verified the .NET 11 APIs you lean on ( I ran the change through several independent AI code reviews plus an automated skill-quality check, and a cross-family evaluation (five models doing the task, scored by a separate model). Sharing the feedback. The evaluation came back mixed — worth knowing before merge. The rewrite helped three models, was roughly neutral on Opus, and regressed on GPT, where the current version was clearly preferred. Full run: https://github.com/dotnet/skills/actions/runs/29988426362
Findings, grouped by severity. 🔴 Blocking — evaluation environment (not a skill-quality problem)
🟡 Non-blocking
⚪ Minor
Verdicts ranged from "approve as-is" to "small changes first." Net: the mixed evaluation result is an environment/rubric artifact rather than a regression in the skill, and the one change I'd genuinely make is the small grader-pattern cleanup above. Thanks again! |
The evaluation's negative checks fail a run whose answer matches
'class <name> : JsonNamingPolicy' or 'catch {'. The decision table and
Common pitfalls spelled those out literally (lines 51, 253, 256). Follow the
approach already used on line 49: use an ellipsis for the class name and
break the 'catch {' adjacency ('catch (…) { … }') so a model that echoes a
"never do this" row can't trip the very test the skill helps pass. Wording
only; guidance is unchanged.
Addresses maintainer review feedback.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Done in On the 🔴 environment finding — thanks for digging through the run logs. Agreed it's an eval-runner/rubric artifact (no .NET 11 SDK in the environment + a "must build and run" rubric), not a skill regression, so there's nothing to change in the skill file for it. Flagging for whoever owns the eval runner: installing the .NET 11 SDK and/or relaxing the must-execute requirement for scenarios that legitimately can't run would remove the asymmetry. On the ⚪ minor items — I've deliberately kept this diff minimal and scoped to the grader cleanup you called out, rather than folding in unrelated edits unreviewed. Happy to add any of these to this PR if you'd like them here:
Just say the word. Thanks for the thorough review! |
|
/evaluate |
The Rule 1 PascalCase snippet relies on reflection-based serialization, which is enabled in a console project but disabled in a file-based app (dotnet run app.cs), where it would throw NoMetadataForType. Add a short leading comment marking it as a console-project snippet and pointing readers to set TypeInfoResolver = new DefaultJsonTypeInfoResolver() (per the existing "Producing runnable output" section) if they run it file-based. Comment only; code unchanged. Addresses PR review feedback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/evaluate |
📊 Skill Evaluation Results1 skill(s) evaluated — 0 improved, 1 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0).
🔍 Full Results - additional metrics and failure investigation steps
|
📊 Skill Evaluation Results1 skill(s) evaluated — 0 improved, 1 no credible improvement. A skill passes only on a credible improvement over baseline (mean preference > 0 with its 95% CI above 0).
🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
|
@jaredpar, @jasonmalinowski - please have a look (you are the codeowners of the area) |
Summary
Fixes #902. The
dotnet11/system-text-json-net11skill scored 0% pass across all five model families in the cross-family evaluation (action bucket ADD-DECISIVENESS, 🟠 P1). The judge saw no behavior change vs. baseline (7 tie-trials, ~67% invocation) because the skill read as reference prose — API signatures the model already reproduces on its own — rather than imperative behavioral guidance.What changed
Rewrote
plugins/dotnet11/skills/system-text-json-net11/SKILL.mdfollowing the CTA guidance (convert "here's what X is" into "when you see A, do B, verify with C", add a worked example, add a verification step):USE FOR/DO NOT USE FORand trigger keywords (PascalCase property names,GetTypeInfo<T>,TryGetTypeInfo<T>) to improve discovery — invocation was only ~67%.JsonNamingPolicy.PascalCase; never a customJsonNamingPolicysubclass or per-member[JsonPropertyName].GetTypeInfo<T>(); never cast the non-generic overload.TryGetTypeInfo<T>(out …); nevertry/catcharoundGetTypeInfo.net11.0run instructions (file-based app viadotnet run app.cs+#:property TargetFramework=net11.0, and a project variant) so the program is actually executed and prints output — the graders requireexit-successand matching JSON.The three .NET 11 APIs were confirmed against Microsoft Learn. The existing
tests/dotnet11/system-text-json-net11/eval.yamlis unchanged and still aligns with the rewritten guidance.Validation
skill-validator check --plugin ./plugins/dotnet11→ ✅ all checks passed (frontmatter, references, links)./evaluateon this PR.