diff --git a/eng/eval-quality/README.md b/eng/eval-quality/README.md index 2eb0af056d..9fb662b1fd 100644 --- a/eng/eval-quality/README.md +++ b/eng/eval-quality/README.md @@ -15,7 +15,7 @@ python eng/eval-quality/selftest_eval_quality.py # prove the gate still fi ## Failing checks -All six are **structural** — they inspect file existence, git state, declared +All seven are **structural** — they inspect file existence, git state, declared numbers, or YAML keys. None of them interprets prose, so they cannot fire spuriously on a well-written eval. @@ -86,7 +86,27 @@ Fix it by making the totals agree with both the declared rate and the summed the rate — that leaves one number for every reader. The same applies to `branches-covered`/`branches-valid` against `branch-rate`. -### 5. Grader with a missing or empty required config +### 5. Aggregate `line-rate` contradicts the `` beneath it + +A file, package or class whose declared `line-rate` disagrees with the `` +elements underneath it — the same split-brain as checks 3 and 4, at the level +the prompt usually quotes. + +This shipped for a while as a warning because of one fixture: +`coverage-analysis/fixtures/plateau` declared 75% while its `` implied +47%, and the scenario prompt said *"my coverage is stuck at 75%"*. It could not +be repaired by recomputing — `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% — so the fix reached into the scenario itself. It was resolved by +restating the plateau at 47% (declared rates and totals aligned to 22/47, prompt +reworded); the plateau story depends on one method dominating the shortfall, not +on the specific number. With that fixture repaired there are no offenders left, +so the check now fails instead of warning. + +Fix an occurrence the same way: make the declared rate match the payload, and if +a prompt or rubric quotes the old figure, update it in the same change. + +### 6. Grader with a missing or empty required config A grader whose `config` is absent, null, or missing its required key (`pattern`, `substring`, `command`, `path`) parses as valid YAML and **enforces @@ -109,7 +129,7 @@ bespoke regex validator caught it — the validator did `(g.get("config") or {}).get("pattern")` and silently skipped the entry, so the pattern count was identical before and after the fix. Only review caught it. -### 6. Dormancy guard that also sets `reject_skills` +### 7. Dormancy guard that also sets `reject_skills` A dormancy guard is a stimulus with `expect_activation: false`: an off-target request where the skill should stay dormant rather than hijack the task. @@ -124,22 +144,10 @@ The repo convention is `expect_activation: false` **alone** (see `system-text-json-net11`), so the skill is actually loaded and the guard measures the real property. -## Warnings (reported, never failing) - -### Aggregate `line-rate` vs the lines beneath it +## Warnings (reported; failing only under `--strict`) -A file, package or class whose declared `line-rate` disagrees with the `` -elements underneath it. This is a warning rather than an error because a real -coverage report may legitimately summarise more than it enumerates, and because -the correct fix sometimes reaches into the scenario itself. - -Live example: `coverage-analysis/fixtures/plateau` declares 75% while its -`` imply 47%, and the scenario prompt says *"my coverage is stuck at -75%"*. It cannot simply be recomputed — `CalculateGpa` contributes 24 lines at -0% coverage and the rubric requires it to stay the 0% blocker, which caps the -achievable rate at 23/47 = 48.9%. Making the payload true would mean rewriting -the fixture and the prompt together, so the gate reports it and leaves the -judgement to a human. +CI runs the gate without `--strict`, so these are informational there. Passing +`--strict` returns exit code 1 when any warning is present. ### Statistical power @@ -154,11 +162,20 @@ sqrt(n) × (mean / sd) > t(n-1) | n | required mean/sd | | ---: | ---: | | 1 | undefined — a single trial decides | -| 2 | 3.04 | -| 3 | 1.84 | -| 4 | 1.39 | -| 6 | 1.00 | -| 8 | 0.82 | +| 2 | 8.98 | +| 3 | 2.48 | +| 4 | 1.59 | +| 6 | 1.05 | +| 8 | 0.84 | + +These are `t(n-1)/sqrt(n)`. An earlier revision of this table read the critical +value at `t(n)` instead, understating every row — n=3 appeared to need 1.84 when +it really needs 2.48, and n=2 appeared to need 3.04 against a true 8.98. That +made a thin eval look one good trial away from credible when it was not. The +worked example below is the check: `coverage-analysis` scoring 0.4/1.0/0.4 has +mean/sd = 1.73, which reads as a near miss against the old 1.84 but is 30% short +of the real 2.48 — a difference that changes the remedy from "re-run it" to +"raise n or runs". Consequences seen in practice: `coverage-analysis` **won 100% of its trials in four consecutive runs and failed all four**; `migrate-static-to-wrapper` missed diff --git a/eng/eval-quality/check_eval_quality.py b/eng/eval-quality/check_eval_quality.py index 031542a7f9..4c5e9d1795 100644 --- a/eng/eval-quality/check_eval_quality.py +++ b/eng/eval-quality/check_eval_quality.py @@ -5,27 +5,39 @@ cannot silently recur in any plugin. FAILS on unambiguous bugs: - 1. A stimulus references a fixture that is missing on disk. - 2. A stimulus references a fixture that exists but is NOT tracked by git. - `.gitignore` once silently swallowed a Cobertura fixture: the scenarios - passed locally and would have failed at setup in CI. - 3. A Cobertura fixture whose declared `line-rate` contradicts its own - `` data. The crap-score skill documents both parse paths, so the - two arms of a comparison can legitimately read different inputs and the - eval measures the disagreement instead of the skill. - 4. A dormancy guard (`expect_activation: false`) that also sets - `reject_skills`. That forces the skilled arm skill-free, making it - identical to the baseline arm, so the score is judge noise. + 1. Referenced fixture missing on disk. The scenario fails at setup, which + reads as a skill failure. + 2. Referenced fixture not tracked by git. `.gitignore` once silently swallowed + a Cobertura fixture: the scenarios passed locally and would have failed in + CI. + 3. Cobertura `line-rate` contradicts its own ``. The crap-score skill + documents both parse paths, so the two arms can read different inputs and + the eval measures that disagreement instead of the skill. + 4. Whole-file Cobertura totals contradict the declared file rate, for lines + (`lines-covered`/`lines-valid` vs `line-rate`) or branches + (`branches-covered`/`branches-valid` vs `branch-rate`). Summary attributes + are another parse path, so mismatched totals split readers on the same + fixture. + 5. Aggregate `line-rate` contradicts the `` beneath it. File, package, + and class rates are often the prompt-level coverage number, so disagreement + there changes what the scenario is asking about. + 6. Grader with a missing or empty required config. The YAML parses, but the + grader silently enforces nothing and the scenario has one fewer assertion + than it appears to. + 7. Dormancy guard that also sets `reject_skills`. That forces the skilled arm + skill-free, making it identical to the baseline arm, so the score is judge + noise. Every failing check above is structural — it inspects file existence, git -state, or YAML keys — so it cannot fire spuriously on well-written content. +state, declared numbers, or YAML shape/keys — so it cannot fire spuriously on +well-written content. -REPORTS (does not fail) pre-existing debt and judgement calls: statistical -power, orphaned fixtures, skills with no eval, and dormancy guards that appear -to lack an anti-hijack rubric item. That last one is deliberately a warning: -detecting "the rubric says the skill should stay dormant" needs phrase -matching, which will always have false positives, and a gate that blocks a PR -spuriously is a gate the team turns off. +REPORTS warnings for pre-existing debt and judgement calls: statistical power, +orphaned fixtures, skills with no eval, and dormancy guards that appear to lack +an anti-hijack rubric item. Warnings do not fail unless `--strict` is passed. +That last one is deliberately a warning: detecting "the rubric says the skill +should stay dormant" needs phrase matching, which will always have false +positives, and a gate that blocks a PR spuriously is a gate the team turns off. Usage: python eng/eval-quality/check_eval_quality.py [--strict] """ @@ -45,8 +57,12 @@ print("PyYAML is required: pip install pyyaml", file=sys.stderr) raise SystemExit(2) -T95 = {2: 4.303, 3: 3.182, 4: 2.776, 5: 2.571, 6: 2.447, 7: 2.365, 8: 2.306, - 9: 2.262, 10: 2.228, 11: 2.201, 12: 2.179, 13: 2.160, 14: 2.145, 15: 2.131} +# 95% two-sided t critical values, keyed by DEGREES OF FREEDOM (n - 1), which is +# what the pass gate's confidence interval uses. Keying this by n instead is an +# easy and costly slip: it makes every reported threshold too lenient. +T95 = {1: 12.706, 2: 4.303, 3: 3.182, 4: 2.776, 5: 2.571, 6: 2.447, 7: 2.365, + 8: 2.306, 9: 2.262, 10: 2.228, 11: 2.201, 12: 2.179, 13: 2.160, + 14: 2.145, 15: 2.131} ANTI_HIJACK = ("derail", "did not attempt", "outside the scope", "out of scope", "did not perform", "declined", "does not load", "does not reference", @@ -207,10 +223,13 @@ def check_cobertura() -> None: f"different whole-file {unit} coverage numbers, so the arms disagree " f"depending on which attribute a skill happens to read") - # Aggregates vs the underlying payload. Reported rather than failed: - # a real report may legitimately summarise more than it enumerates, - # and forcing a rewrite of a scenario whose prompt quotes the declared - # figure is a bigger change than this check should compel. + # Aggregates vs the underlying payload. A file, package or class that + # declares one rate while the elements beneath it imply another + # is the same split-brain bug one level up: a skill that trusts the + # attribute and one that recomputes read different inputs. Held as a + # warning only while coverage-analysis/fixtures/plateau declared 75% + # against a 47% payload; that fixture is now self-consistent, so the + # check fails instead of warning. for el, label in ( [(tree.getroot(), "file")] + [(p, f"package '{p.get('name')}'") for p in tree.iter("package")] @@ -221,9 +240,11 @@ def check_cobertura() -> None: if not total or declared is None: continue if abs(covered / total - float(declared)) >= 0.011: - warnings.append( + errors.append( f"{path}: {label} declares line-rate={float(declared):.2f} but the " - f" beneath it imply {covered / total:.2f} ({covered}/{total})") + f" beneath it imply {covered / total:.2f} ({covered}/{total}); " + f"make the declared rate match the payload, and if a scenario prompt " + f"or rubric quotes the old figure, update it too") def report_power(specs: list[str]) -> None: @@ -233,7 +254,7 @@ def report_power(specs: list[str]) -> None: doc = yaml.safe_load(fh) or {} n = len(doc.get("stimuli") or []) if n <= 3: - need = T95.get(n, 1.96) / math.sqrt(n) if n >= 2 else float("inf") + need = T95.get(n - 1, 1.96) / math.sqrt(n) if n >= 2 else float("inf") thin.append((n, need, spec)) if not thin: return @@ -305,7 +326,7 @@ def main() -> int: print(f"Eval quality gate — checked {len(specs)} eval spec(s).\n") if warnings: - print("WARNINGS (reported, not failing):") + print("WARNINGS (reported; failing only with --strict):") for w in warnings: print(f" {w}") print() diff --git a/eng/eval-quality/selftest_eval_quality.py b/eng/eval-quality/selftest_eval_quality.py index 48e296a558..1d5a26157c 100644 --- a/eng/eval-quality/selftest_eval_quality.py +++ b/eng/eval-quality/selftest_eval_quality.py @@ -56,7 +56,7 @@ def case(label, mutate, expect_fail): d = scratch() try: mutate(d) - subprocess.run(["git", "add", "-A"], cwd=d, capture_output=True) + subprocess.run(["git", "add", "-A"], cwd=d, capture_output=True, check=True) code, out = run_gate(d) failed = code != 0 ok = failed == expect_fail @@ -70,6 +70,31 @@ def case(label, mutate, expect_fail): shutil.rmtree(d, ignore_errors=True) +def output_case(label, mutate, expect_substring): + """Assert on what the gate *reports*, for checks that warn rather than fail. + + The exit code is asserted too: warnings are printed before errors, so a + scratch tree that failed for an unrelated reason would still emit the + expected substring and this case would pass while the gate was broken. + + Staging is checked for the same reason: a silent `git add` failure would + change what the gate sees for any mutation that adds a new file. + """ + d = scratch() + try: + mutate(d) + subprocess.run(["git", "add", "-A"], cwd=d, capture_output=True, check=True) + code, out = run_gate(d) + ok = code == 0 and expect_substring in out + print(f" [{'OK ' if ok else 'BAD'}] {label:<52} expected={expect_substring!r}") + if not ok: + print(f" exit={code}") + print(" " + out.strip().replace("\n", "\n ")[:900]) + return ok + finally: + shutil.rmtree(d, ignore_errors=True) + + EV = lambda d: os.path.join(d, "tests", "demo", "widget", "eval.yaml") @@ -119,6 +144,28 @@ def inconsistent_file_totals(d): ) +def aggregate_contradicts_payload(d): + # Every method agrees with its own , and the file summary attributes + # agree with the declared file line-rate — so checks 3 and 4 both pass. Only + # the file/package/class rates contradict the lines actually enumerated + # (1/4 = 0.25, not 0.75). This is the coverage-analysis/plateau shape. + p = os.path.join(d, "tests", "demo", "widget", "fixtures", "sample", "coverage.cobertura.xml") + with open(p, "w") as f: + f.write( + '' + '' + '' + '' + '' + '' + "" + '' + '' + '' + "" + ) + + def empty_grader_config(d): # An edit that leaves `- type: output-matches` / `config:` with the pattern # attached to the NEXT list item. The document still parses; the grader @@ -134,6 +181,20 @@ def empty_grader_config(d): ) +def three_scenarios(d): + # n=3, so the power warning must quote t(n-1)/sqrt(n) = 4.303/sqrt(3) = 2.48. + # Reading the critical value at t(n) instead yields 1.84 — the off-by-one + # that made thin evals look close to credible when they were not. + with open(EV(d), "a") as f: + for i in (2, 3): + f.write( + f" - name: Does the thing {i}\n" + f" prompt: do it {i}\n" + f" rubric:\n" + f" - Did the thing {i}\n" + ) + + def guard_with_reject_skills(d): with open(EV(d), "a") as f: f.write( @@ -166,9 +227,11 @@ def guard_ok(d): case("fixture present but NOT tracked by git", untracked_fixture, expect_fail=True), case("Cobertura line-rate contradicts its ", bad_cobertura, expect_fail=True), case("Cobertura file totals contradict file line-rate", inconsistent_file_totals, expect_fail=True), + case("Cobertura aggregate rate contradicts its payload", aggregate_contradicts_payload, expect_fail=True), case("grader with an empty config enforces nothing", empty_grader_config, expect_fail=True), case("dormancy guard also sets reject_skills", guard_with_reject_skills, expect_fail=True), case("well-formed dormancy guard", guard_ok, expect_fail=False), + output_case("power threshold uses t(n-1), not t(n)", three_scenarios, "> 2.48"), ] print() if all(results): diff --git a/tests/dotnet-test/coverage-analysis/eval.yaml b/tests/dotnet-test/coverage-analysis/eval.yaml index e198c36374..4ae19be6ad 100644 --- a/tests/dotnet-test/coverage-analysis/eval.yaml +++ b/tests/dotnet-test/coverage-analysis/eval.yaml @@ -71,7 +71,7 @@ stimuli: - Identifies CalculateGpa as untested or highest risk due to complexity with no coverage - Provides prioritized recommendations for where to add tests - name: Coverage plateau diagnosis - prompt: My coverage is stuck at 75% and I can't get it higher. What's blocking me? Coverage data is in + prompt: My coverage is stuck at 47% and I can't get it higher. What's blocking me? Coverage data is in TestResults/coverage.cobertura.xml. environment: files: diff --git a/tests/dotnet-test/coverage-analysis/fixtures/plateau/coverage.cobertura.xml b/tests/dotnet-test/coverage-analysis/fixtures/plateau/coverage.cobertura.xml index de7b43c656..833f616423 100644 --- a/tests/dotnet-test/coverage-analysis/fixtures/plateau/coverage.cobertura.xml +++ b/tests/dotnet-test/coverage-analysis/fixtures/plateau/coverage.cobertura.xml @@ -1,9 +1,9 @@ - + - + - +