Make the coverage-analysis plateau fixture self-consistent - #964
Conversation
`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
Skill Coverage Report
|
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
This PR fixes a self-inconsistency in the coverage-analysis plateau Cobertura fixture (declared rates/totals vs enumerated <line> payload) so baseline vs skilled runs can’t read different “truths” from the same file. With the last known offender resolved, it also promotes the eval-quality gate’s aggregate-vs-payload Cobertura check from warning to error and adds a self-test covering that failure mode.
Changes:
- Make
coverage-analysis/fixtures/plateau/coverage.cobertura.xmlself-consistent by aligning declared line/branch rates and totals to the enumerated<line>/condition-coveragepayload. - Update the
coverage-analysisplateau scenario prompt to match the new (correct) stated percentage. - Promote “aggregate declared line-rate contradicts payload” from warning to error in
check_eval_quality.py, add a targeted self-test, and update the eval-quality README accordingly.
Show a summary per file
| File | Description |
|---|---|
| tests/dotnet-test/coverage-analysis/fixtures/plateau/coverage.cobertura.xml | Align declared coverage rates/totals with the actual <line> / branch payload to eliminate split-brain parsing. |
| tests/dotnet-test/coverage-analysis/eval.yaml | Update plateau prompt percentage to match the corrected fixture. |
| eng/eval-quality/check_eval_quality.py | Treat aggregate-vs-payload Cobertura line-rate contradictions as errors (not warnings) and update messaging. |
| eng/eval-quality/selftest_eval_quality.py | Add a self-test case that fails only on aggregate-vs-payload contradiction. |
| eng/eval-quality/README.md | Move the aggregate-vs-payload check into the “Failing checks” section and document the plateau history/fix. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
📊 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);
ℹ️ Column legend
❌ coverage-analysis — detailsReason: Mean preference +60.0% [95% CI -26.1%, 146.1%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — not credible (95% CI includes 0)
🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
Two documentation inconsistencies surfaced in review: - The module docstring's "FAILS on unambiguous bugs" list named four checks while the gate fails on seven. It now mirrors the README's numbered "Failing checks" one-for-one, keeping the explanation of why each defect matters rather than just what it detects. - Both the docstring and the README asserted that warnings never fail, which contradicts the documented `--strict` flag. They now say warnings fail only under `--strict`, note that CI runs without it, and the runtime header reads "WARNINGS (reported; failing only with --strict)". Documentation only; no behavior change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 57b65d53-dc7a-4d0a-b5c6-d17687018f01
|
/evaluate eba38c3 |
📊 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);
ℹ️ Column legend
❌ coverage-analysis — detailsReason: Mean preference +60.0% [95% CI -26.1%, 146.1%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — not credible (95% CI includes 0)
🔍 Full Results - additional metrics and failure investigation steps
▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
`report_power` looked the 95% t critical value up as `T95[n]`, but `T95` is keyed by degrees of freedom, and the pass gate's confidence interval uses `t(n-1)`. Every reported threshold was therefore too lenient. Verified against this PR's own evaluation artifacts: `coverage-analysis` scored 0.4/1.0/0.4, and vally reported a 95% CI of [-0.2606, 1.4606]. That interval reproduces exactly with t(df=2) = 4.303 and not with t(3) = 3.182, confirming which value the real gate uses. The practical effect is that a thin eval read as a near miss when it was not: n=3 appeared to need mean/sd > 1.84 against a true 2.48, and n=2 appeared to need 3.04 against a true 8.98. `coverage-analysis` sits at 1.73, which looks 6% short under the old number and 30% short under the correct one - the difference between "re-run it" and "raise n or runs". - `T95` gains its missing df=1 entry (12.706) and a comment naming the key as degrees of freedom, since n=2 would otherwise fall through to the 1.96 normal-approximation default. - The README power table is recomputed. Its surrounding prose already quoted the correct 0.77 for 9 trials, so only the table was ever wrong. - A self-test asserts the n=3 warning quotes 2.48; it fails against the old lookup, which produces 1.84. No failing check changes behavior, so the gate's exit code is unaffected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 57b65d53-dc7a-4d0a-b5c6-d17687018f01
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
eng/eval-quality/selftest_eval_quality.py:81
output_caseignores the gate’s return code, so the self-test can report OK even if the gate fails for an unexpected reason (as long as the expected substring appears in the output). Since this helper is meant to validate warning-only behavior, it should also assert the gate exits successfully.
_, out = run_gate(d)
ok = expect_substring in out
print(f" [{'OK ' if ok else 'BAD'}] {label:<52} expected={expect_substring!r}")
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
`output_case` checked only that the expected substring appeared in the gate's output. Warnings are printed before errors, so a scratch tree that failed for an unrelated reason would still emit the substring and the case would report OK while the gate was broken. The helper exists to validate warning-only behavior, which means a clean exit is part of what it is asserting. Raised by review on the previous commit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 57b65d53-dc7a-4d0a-b5c6-d17687018f01
|
/evaluate e90a28b |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
eng/eval-quality/check_eval_quality.py:18
- The docstring’s failing-check #4 says only
line-rate, but the implementation also validatesbranch-rateagainstbranches-covered/branches-valid. This makes the module documentation slightly misleading about what causes a hard failure.
4. Whole-file Cobertura totals contradict the file `line-rate`. Summary
attributes are another parse path, so mismatched totals split readers on
the same fixture.
eng/eval-quality/selftest_eval_quality.py:84
output_caserunsgit add -Awithoutcheck=True, so if staging fails (e.g., git not available or repo corruption) the self-test can produce misleading results rather than failing fast.
mutate(d)
subprocess.run(["git", "add", "-A"], cwd=d, capture_output=True)
code, out = run_gate(d)
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
📊 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);
ℹ️ Column legend
❌ coverage-analysis — detailsReason: Mean preference +60.0% [95% CI -26.1%, 146.1%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — not credible (95% CI includes 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 |
Failing check #4 validates the declared file rate against the summary attributes for both lines and branches, but the docstring listed only `line-rate`. This PR's own fixture fix corrected exactly a branch-rate mismatch (declared 0.60 against a 7/16 payload), so the omission hid a case the gate really does fail on. The README already documented both. Raised by review on the previous commit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 57b65d53-dc7a-4d0a-b5c6-d17687018f01
|
/evaluate 7f5556c |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
eng/eval-quality/selftest_eval_quality.py:85
output_case()runsgit add -Awithoutcheck=True. Ifgit addfails for an environmental/unexpected reason, this helper can silently proceed and potentially produce a false-positive result (e.g., missing staging changes what the gate sees). It’s better to fail fast here so the self-test can’t pass for the wrong reason.
mutate(d)
subprocess.run(["git", "add", "-A"], cwd=d, capture_output=True)
code, out = run_gate(d)
ok = code == 0 and expect_substring in out
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
📊 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);
ℹ️ Column legend
✅ coverage-analysis — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — credibly better
🔍 Full Results - additional metrics and failure investigation steps ▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
Both `case` and `output_case` ran `git add -A` without checking it. A silent staging failure would change what the gate sees for any mutation that adds a new file, letting a case pass for the wrong reason - the same class of false negative the untracked-fixture case exists to catch. Applied to both helpers rather than only the new one, so the harness stays consistent. Verified against every existing case, including `untracked_fixture`, which deliberately manipulates the index. Raised twice by review; I declined it the first time on the grounds that `scratch()` already proves git works and that patching one helper would leave the other inconsistent. Fixing both removes that objection, and the cost is two lines. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 57b65d53-dc7a-4d0a-b5c6-d17687018f01
|
/evaluate 0ab01cf |
📊 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);
ℹ️ Column legend
✅ coverage-analysis — detailsReason: Mean preference +40.0% [95% CI 40.0%, 40.0%], win rate 100.0% (3W/0T/0L over 3 trial(s)) — credibly better
🔍 Full Results - additional metrics and failure investigation steps ▶ Sessions Visualisation -- interactive replay of all evaluation sessions |
Fixes #958.
tests/dotnet-test/coverage-analysis/fixtures/plateau/coverage.cobertura.xmldeclared a 75% line-rate while the<line>elements beneath it enumerate 22 of 47 lines (47%). Thecrap-scoreskill documents both parse paths — readline-rate, or recompute from<lines>when it is absent — so the baseline and skilled arms could read different coverage inputs from the same file and the eval would measure that disagreement instead of the skill.What changed
75% is arithmetically unreachable from this payload:
CalculateGpacontributes 24 of the 47 lines athits="0"and the rubric requires it to remain the 0% blocker, capping the achievable rate at 23/47 = 48.9%. So this takes option 1 from the issue and restates the plateau at 47% — the scenario depends on one method dominating the shortfall, not on the specific number.0.75 → 0.47at file, package and class level, withlines-covered="22" lines-valid="47".0.60 → 0.44withbranches-covered="7" branches-valid="16", which is what thecondition-coveragedata actually sums to. The old12/20agreed with the declaredbranch-ratebut not with the payload, so it was the same split-brain one attribute over.line-ratewas already self-consistent and is untouched.Every level of the fixture now agrees with the lines beneath it:
Gate promoted from warning to error
The aggregate-vs-payload check was a warning only because of this fixture — the code comment and
eng/eval-quality/README.mdboth said so explicitly. With the plateau fixture repaired there are no remaining offenders, so it now fails, per the issue's closing note.check_eval_quality.py:warnings.append→errors.append, with the fix instruction in the message; module docstring updated.selftest_eval_quality.py: new case whose methods and file summary attributes are self-consistent so checks 3 and 4 both pass, and only the file/package/class rates contradict the enumerated lines — verified to fail for exactly that reason and no other.README.md: section moved from Warnings to Failing checks as Skill or agent for handling .NET interop #5 (later ones renumbered), keeping the plateau history as the worked example.Verification