fix(scoring): test the arithmetic this benchmark's numbers are made of - #42
Merged
Merged
Conversation
Phase 6 of the plan calls for mutation-testing the scoring logic, on the
grounds that a measurement tool's measurement is its product. Doing it put
tooltrace/scoring/builtin.py at a 57.4% mutation score: 23 of 54 mutants
survived, meaning 23 behaviours in the scorers that no test constrained.
Three findings, in order of severity.
**A scorer that could be inverted silently.** In `file_not_contains`, mutating
`in` to `not in` survived. That scorer would have scored 1.0 for a file
*containing* the forbidden text and 0.0 for a clean one -- doing exactly the
opposite of its name -- with the whole suite green.
**A constraint that has never worked.** Writing a boundary test for
`git_diff`'s `max_changed_files` crashed it:
changed = {line.split()[2] for line in ... if line.startswith("+++ b/")}
A git diff header is `+++ b/path`, two whitespace-separated fields, so `[2]`
raises IndexError on every real diff. The constraint could not return a score,
only crash. No shipped task uses it, which is why nothing noticed. Verified the
format against real `git diff HEAD` output rather than assuming.
**The pytest scorer's core calculation was entirely unconstrained.** Every
operator in
total = passed + failed + errors
ratio = passed / total if total else 0.0
score = 1.0 if ratio >= min_ratio and errors == 0 else round(ratio, 4)
survived mutation: the total could be computed by subtraction, the ratio by
multiplication, the threshold inverted, and the errors clause flipped from
`and` to `or`. It was untested because it was unreachable -- welded inside
`_tests_pass` behind a `subprocess.run` of a real pytest in a temp workspace.
Extracting `score_pytest_output` is what made the assertions possible.
Mutation score now 77.8% (42/54). Every comparison and arithmetic mutant is
killed. The 12 survivors are boolean short-circuits and bool constants, mostly
`isinstance(...) and ...` guards whose second operand is unreachable without
the first -- equivalent mutants rather than gaps. Stating that rather than
chasing the number: a mutation score is a diagnostic, not a target.
One of my own mistakes worth recording: my first attempt to kill the JSON-path
`Lt -> LtE` mutant used index 9 on a three-element array, where `9 < 3` and
`9 <= 3` are both false, so the mutant survived a test written specifically to
kill it. Only `idx == len` distinguishes them.
Verified: ruff, ruff format, mypy (73 files), 340 tests pass, coverage 85.48%
against the 80% floor.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
webdevsamran
added a commit
that referenced
this pull request
Sep 12, 2026
`docs/feature-status.md` grades 122 capabilities and calls itself this
project's verification artifact. Its path check inspected only backtick-quoted
tokens containing `/` or ending `.py` -- and 97 of the 122 rows cited bare prose
("clustering module", "calibration sets module", "conformance fixtures module").
So roughly three quarters of the table was never verified by anything, which is
how four rows survived as `I` with no implementation behind them:
- #45 multi-judge adapters and #46 judge calibration datasets -> `D` (declared
only). No file matching `judge*.py` or `calibrat*.py` has ever existed; the
only mentions of the word are a reserved `judge_config` field that is never
assigned, a `ScoringContract.judge_required` declaration, and a lint warning.
- #42 abstention/calibration tasks and #121 backup/restore tooling -> `N` (not
implemented). Nothing in the package mentions abstention or clarification, and
neither backup nor restore appears in the server or in the
`docs/self-hosting.md` that #121 cited as its evidence.
Seventy further rows now cite a path and the symbols inside it, replacing prose.
Every citation was verified against the file before being written -- a
fabricated citation would be the same defect in a new coat.
Three mechanisms stop this recurring:
- an `I`/`E`/`P` row must cite something inspectable (`N` and `D` are exempt by
definition -- their content is that nothing ships);
- a claim probe rejects any row claiming a capability whose implementation is
absent from disk, with a non-vacuity test asserting the probe still matches
rows 45 and 46 and still does *not* match row 44, which legitimately claims
independence *from* a judge;
- the path resolver moved to `scripts/check_doc_code_refs.py` and runs in CI
across all 25 documents. The test imports it rather than keeping a private
copy, which is how the two drifted in the first place.
That checker immediately found `docs/differentiators.md` citing four modules
that do not exist, one of them `scoring/judges.py`. Its "judge-independent"
section claimed multi-judge disagreement reporting and calibration-drift
datasets; it now states what is true, which is the stronger claim -- scoring is
judge-*free*, so a third party can recompute a score from the bundle alone.
The resolver also had a real bug: `lstrip("./")` strips *characters*, so a
dotfile citation silently lost its leading dot. Now `removeprefix`.
The checker is deliberately narrow -- a path needs a separator and a known
extension -- and skips fenced blocks and blockquotes, because a dated correction
note exists precisely to say "this row used to cite X". A checker that cries
wolf is one people learn to ignore.
Adds one step to the existing `python` job. No job names changed.
webdevsamran
added a commit
that referenced
this pull request
Sep 12, 2026
Two more shipped false claims, both on the settings page, both the same shape as the demo-row leak: the console described a feature, and the feature did not exist. It told operators retention was "configurable with deletion of expired records". `apply_retention` was written, tested, and called by nothing outside its own tests. And it told them "self-hosted metadata and artifact references support backup/restore", linking to `docs/self-hosting.md#backup-and-restore` -- an anchor that does not exist, for a feature `docs/feature-status.md` graded **N: no backup or restore code ships**. Two shipped artifacts of this project contradicted each other in writing, and the console was the one lying. Both resolved by writing the code rather than deleting the sentence. `GET /api/v1/export` returns everything this process holds that exists only in memory. `POST /api/v1/import` restores it, and the three decisions in it are all the uncomfortable ones: the restore **replaces** rather than merges, because one that left yesterday's deleted user in place is not a restore and the operator would have no way to tell; it refuses a snapshot from a version it does not understand **whole**, rather than restoring the half it recognises; and it **re-verifies the audit chain** rather than trusting it, because a backup that laundered a tampered chain would defeat the only reason to keep one. Bundles are excluded on purpose -- they are checksummed files on a disk, and inlining gigabytes of them into JSON would be a slower `cp` with no extra safety. `POST /api/v1/retention` defaults `dry_run` to true. A deletion endpoint whose default is to delete is one somebody triggers while exploring the API. Legal holds are honoured, the deletion is audited, and the response says in words that administrative retention is not a legal-compliance determination -- which this project cannot determine and will not imply in a console an auditor reads. Two more found on the way: - the policies page advertised `max_concurrency`, `monthly_token_budget` and `monetary_budget_usd`. None appears in `WorkspacePolicy` or in any code path. The table is now derived from the payload, so it can only show settings that exist; - `POLICY_DEMO` walked past the demo-leak gate by not being spelled `DEMO_*` -- the same bug under a different variable name. The pattern matches either now: a check that only catches the naming convention catches only the developers who follow it. Row 121 moves from **N** to **I**, leaving one **N** in the matrix (#42, abstention/calibration, which genuinely has no code). 1908 Python tests at 88.4% coverage; 199 vitest; 59 Playwright.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 6 calls for mutation-testing the scoring logic, on the grounds that a measurement tool's measurement is its product. Doing it put
tooltrace/scoring/builtin.pyat a 57.4% mutation score — 23 of 54 mutants survived, meaning 23 behaviours no test constrained.Three findings, in order of severity.
1. A scorer that could be inverted silently
In
file_not_contains, mutatingin→not insurvived. That scorer would have returned 1.0 for a file containing the forbidden text and 0.0 for a clean one — doing exactly the opposite of its name, with the whole suite green.2. A constraint that has never worked
Writing a boundary test for
git_diff'smax_changed_filescrashed it:A git diff header is
+++ b/path— two whitespace-separated fields — so[2]raisesIndexErroron every real diff. The constraint could not return a score, only crash. No shipped task uses it, which is why nothing noticed. I verified the format against realgit diff HEADoutput rather than reasoning about it.3. The pytest scorer's core calculation was entirely unconstrained
Every operator survived:
The total could have been computed by subtraction, the ratio by multiplication, the threshold inverted. It was untested because it was unreachable — welded inside
_tests_passbehind asubprocess.runof a real pytest in a temp workspace. Extractingscore_pytest_outputis what made the assertions possible.Result
Mutation score 57.4% → 77.8% (42/54). Every comparison and arithmetic mutant is killed.
The 12 survivors are boolean short-circuits and bool constants, mostly
isinstance(...) and ...guards whose second operand is unreachable without the first — equivalent mutants rather than gaps. Saying so rather than chasing the number: a mutation score is a diagnostic, not a target.One of my own mistakes
My first attempt to kill the JSON-path
Lt → LtEmutant used index 9 on a three-element array, where9 < 3and9 <= 3are both false — so the mutant survived a test written specifically to kill it. Onlyidx == lendistinguishes them.Verification
ruff/ruff format --checkmypypytest