Repository navigation
fix(review): never return a verdict that claims more coverage than the run had - #28
Merged
Merged
Conversation
…e run had Three ways the engine over-claimed: - A diff no language profile matched took an early return that logged `Approve (NO LANGUAGE)` and reported a green verdict with zero lenses run — a pass for a diff nobody looked at. It is now `INCOMPLETE (no language profile)`, in the report and in the persisted record, with a message that says nothing was reviewed and names the languages the engine supports. - An `args.languages` pin naming an id that does not exist was dropped by `filter(Boolean)` and fell into that same green path. It is now a stop: `INCOMPLETE (unknown language pin)`, naming the unknown id and the available ones, before any profile work happens. - Files that matched no active profile were listed in a report section but never touched the verdict. The material ones — everything that is not a doc, an asset or a lockfile, judged from the path alone — now join `notRun`, the existing INCOMPLETE mechanism, so a partially covered run can no longer come back as a bare Approve.
…ource code shares The lockfile alternative in INERT_UNCOVERED matched any basename shaped like `lock.<ext>` / `<x>-lock.<ext>`, so `db/lock.sql`, `src/lock.rs`, `internal/spin-lock.go` and `charts/x/templates/lock.yaml` were all dropped from the material remainder — the silent overclaim this branch exists to remove, exempting exactly the SQL migration and the RBAC manifest the framing names. Lockfiles are now matched by the names they actually have, and the classifier is split into three narrow exemptions: prose/asset extensions, that lockfile name set, and generated artifacts whose path is unambiguous (`*.snap`, `*.pb.go`, `*_pb2.py`, `*.min.js`, `__generated__/`, `generated/`, `vendor/`, `node_modules/`). Exempting regenerated artifacts keeps the new INCOMPLETE marker from firing on noise nobody needs reviewed. `.csv` is no longer blanket inert — seed and fixture data is data a change can break. `resolveProfilePin` no longer calls `.filter` on whatever it is handed: `languages: 'rust'` (a known argument-transport shape here) threw a TypeError and aborted the whole review, where the code it replaced tolerated a string by accident. The pin is normalised first, and an unusable shape reads as "no pin", degrading toward running the review rather than crashing. The INCOMPLETE advice line no longer tells the caller to "fix the cause and re-run" when the cause is that no profile exists for those files — re-running cannot review them; it now says so and points at reviewing them another way. Tests cover the lock-shaped source paths, migrations, generated artifacts and the scalar/garbage pin inputs that let these through, and the remaining source-text tripwires carry a comment stating what they do and do not catch.
Three places turned "we do not know" back into the most permissive answer, which rebuilds one layer up the overclaim the leaf verdicts were just fixed to avoid: - `worstVerdict` (lib/run-record.mjs and its verbatim copy in rust-audit.js) returned `Approve` for anything that was not Block/Warning, so a persisted `INCOMPLETE (no language profile)` aggregated back into a green audit verdict. INCOMPLETE — and any verdict from no vocabulary we can read as green — now aggregates to Warning. - `analyze-runs` counted `Approve (INCOMPLETE)` in both the incomplete and the approve column. Now that uncovered files make that suffix common, the double count would inflate the approve figure into a coverage claim. The four buckets are mutually exclusive. - rust-audit's `reviewResult` mapped a nested `⛔ INCOMPLETE — unknown language pin` onto a Block dimension, rendering an operator error as a code-quality block. INCOMPLETE is now a Warning dimension that says the review did not run, and an unrecognisable report no longer falls through to Approve.
The comment claimed the four buckets were mutually exclusive, but only approve had been de-duplicated: Block (INCOMPLETE) and Warning (INCOMPLETE) incremented two buckets each, so the columns summed to more than the run count. Rule now implemented and documented: severity first, then coverage — a suffixed Block/Warning stays a block/warning (partial coverage cannot un-find a finding), a suffixed Approve counts only as incomplete (an approve is a claim about what was not found, and only holds over what was looked at). incompleteRuns stays the honest total.
worstVerdict([]) returned 'Approve': with zero dimension verdicts — every sub-agent dead, or nothing run at all — the aggregate reported green. That is a total outage rendered as a pass, the same overclaim the surrounding clauses were added to prevent. An empty set now aggregates to INCOMPLETE, which no consumer reads as green.
…rose Three defects in one classifier: - worstVerdict([]) returned Approve here too (the file inlines a verbatim copy of the lib/ helpers because the sandbox cannot import). Same fix, kept identical, now guarded by a byte-identity test. - reviewResult tested /INCOMPLETE/i FIRST, so a nested review returning '⛔ Block (INCOMPLETE)' was downgraded to Warning — the opposite of the severity-first rule lib/analyze-runs.mjs states and implements for the run store. Partial coverage cannot un-find a finding already made. The shared rule is now spelled out at both sites. - the verdict was decided by matching /⚠️ |Warning/ over the WHOLE report, so an Approve whose body carries a⚠️ line — the 'Not reviewed' section review.js now emits — scored Warning, and any body merely containing the word INCOMPLETE was reported as uncovered. Reports have a stable shape: a '## Verdict' heading followed by the verdict line. Classify on that line alone; a report with no readable verdict line stays Warning. Also stop stacking a second '(INCOMPLETE)' marker onto an aggregate that is already INCOMPLETE.
…INCOMPLETE The no-profile early return classified every changed file as unreviewed, so a README-, asset- or lockfile-only change came back "NOTHING WAS REVIEWED". A marker that fires on ordinary prose changes stops being read, which destroys its value on the diffs that really do hide unreviewed code. Both paths now use the same notion of material. With no material file at all the verdict says nothing needed reviewing (a green Approve, recorded as such); a diff carrying unsupported source — Python, Go — is still INCOMPLETE, and now names how many material files went unreviewed.
…e of coverage reviewResult classified severity first, but the summary line was still set from a bare INCOMPLETE test — so a nested `⛔ Block (INCOMPLETE)` was a Block and was simultaneously described as "uncovered, not clean". The summary now follows the same classification as the verdict: a Block reads as a block with partial coverage as a clause, an unreadable verdict reads as unverified.
A second normalizer sat upstream of resolveProfilePin and made its hardening
unreachable: an Array.isArray guard turned a scalar `languages: 'rust'` into
null (pin silently dropped, the review auto-detected instead) and a .map(String)
turned `[null]` into the id `'null'`, hard-aborting the run. Hand the raw arg
through; resolveProfilePin is the single place that decides what a pin means,
and the tests now run args -> requestedLangs -> resolveProfilePin, the real
chain, which is how this escaped.
`.txt` as an inert extension was the lockfile shape-match bug in another
costume: CMakeLists.txt, requirements.txt and conanfile.txt are build and
dependency source, and a diff of nothing but those took the green "nothing
needed reviewing" return. Match genuinely inert .txt files by name instead.
The logger ran as `cd <reviewed repo> && node "${CLAUDE_PLUGIN_ROOT:-.}/..."`,
so the `.` fallback resolved against the REVIEWED repo, where craft's script is
not. Every checkpoint, the finalize record and the prior-round chain died on
"Cannot find module", the first two silently. Capture an absolute path in a
variable before the cd.
An empty diff fell through to noLanguageMessage(0, 0) and asserted a coverage
hole of size zero. It stays INCOMPLETE — a half-failed detection looks the same
— but it now says what actually happened.
And "incomplete" named two different quantities in one report: ten
Block (INCOMPLETE) runs rendered as "10 incomplete" in the header and
B/W/A 10/0/0 in the row with no incomplete clause at all. The overlapping total
is now "partial coverage", carried per workflow too; the exclusive bucket is
"incomplete-only".
…ng wolf The empty-diff / nothing-to-review / no-profile guards ran AFTER the language-pin fallback, and every internal caller pins — so on rust-review, nix-review and both rust-audit dispatches they were dead code, and an empty diff got the full lens pipeline over nothing and a bare "Approve — no findings across rust". The decision now lives in a pure resolveCoverage() taken from the change set, tested through the pinned path; a pin only chooses WHICH profile reviews real material. Also: project configuration (CI workflows, deny/rustfmt.toml, Dockerfile, justfile) is reported as not reviewed but no longer fires the INCOMPLETE marker — it is not the reviewed program's source, and a marker firing on every PR stops being read. Real source in any other language still fires it. And the coverage note no longer pollutes notRun, whose exact-string ranking it was filling with count-1 rows; it is merged into incompleteNotes for the verdict only. Duplicate pins are deduped.
…ulary greens
verdictLine sliced from the END of the `## Verdict` heading, so a report writing
the verdict inline (`## Verdict: ⛔ Block — 2 High`) landed on the next heading,
matched nothing, and had its Block downgraded to Warning as "could not be read".
When the heading carries text after `Verdict`, that text is the verdict.
And the non-permissive aggregate was being applied to free text: dimension verdicts
came from a bare {type:'string'} while the miri prompt asks for "Clean / UB-found",
so "No UB detected" or "OK" flipped an all-green audit to Warning. Pin the
vocabulary with an enum where it is produced, and normalise once where results are
collected. Unrecognisable text is still returned unchanged — the default stands.
The uncovered-files note embedded a count and up to five file names, so every run contributed a unique key to a ranking whose whole point is repeated failures — the NOT RUN table filled with count-1 rows and the real repeats sank. It was also the opposite claim from every other entry there: a notRun item is fixable by re-running, an uncovered file never will be. review.js no longer pushes it; the aggregate now reads the record's own uncoveredFiles and reports them in their own section.
…-files tests appended The previous commit replaced lib/analyze-runs.test.mjs wholesale instead of adding to it, dropping 18 existing tests. They are back, unchanged apart from the empty- aggregate shape assertion, which now names the two new keys.
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.
What this changes
A review must never return a verdict that claims more coverage than the run actually
had. Today it does, in two grades, and this closes both — plus the aggregation layer
where the first grade quietly came back.
A diff that matched no language profile returned
Approve (NO LANGUAGE)— a greenverdict with zero lenses run. The engine declares exactly two profiles,
rustandnix; nothing matches.js,.ts,.py,.go, SQL or YAML. The run store holds fourrecords with that verdict, one of them against this repository, which is written in
JavaScript and therefore cannot be reviewed by its own engine at all. It is now
INCOMPLETE (no language profile), and the report says plainly that nothing wasreviewed and that this is not an approval.
An unknown
languagespin was dropped silently bymap(...).filter(Boolean)andlanded in the same green path. It is now an explicit error naming the unknown id and the
available ones.
Uncovered files did not lower the claim. In a mixed diff, files matching no profile
were listed in a report section but the verdict stayed
Approve. A change whose SQLmigration or RBAC manifest nobody read came back green. The material remainder now
forces
Approve (INCOMPLETE).An unrecognised verdict defaulted to
Approve.worstVerdictandrust-audit'sreviewResultboth fell through to the most permissive outcome, so a persistedINCOMPLETE (no language profile)aggregated back into green — the same overclaim, onelayer up. Unrecognised now reads as Warning.
What two review rounds caught
migrations/002-lock.sql,db/lock.sql,src/lock.rs,internal/spin-lock.go,charts/x/templates/lock.yamlall read as inert and never reached the uncovered list —exempting precisely the migration and manifest cases this change exists to catch. Both
reviewers found it independently. Lockfiles are now matched by their real names.
resolveProfilePincalled.filter()on any truthyvalue, so
languages: 'rust'(or a JSON-decoded string, a documented transport failuremode in this file) threw a
TypeErrorand aborted the whole review, where the previouscode tolerated it. Input is normalised; an unusable shape degrades to "no pin".
Approve (INCOMPLETE)for regenerated snapshots. A marker that fires constantly stops beingread, which would have destroyed the value being added. Conservative path-only
exemptions were added.
when the cause is that no profile exists. It now says what the reader can actually do.
analyze-runscountedApprove (INCOMPLETE)in both the approve and incompletebuckets; the new, far more frequent suffix would have inflated the approve column.
The classification defaults to material when in doubt: a false "material" costs one
honest INCOMPLETE, a false "inert" costs a silent overclaim.
Scope — what this does NOT do
It adds no lens and no language profile.
craftstill cannot review a JavaScript diff —it will now say so instead of returning green, which is the point, but self-review of
this engine by this engine remains impossible and is a separate decision.
Verification
node --test182 pass / 0 fail; lint clean;check-workflows6/6 parse withCRAFT_VERSION 0.16.0matching the manifest;check-skillsandcheck-evalsclean.The paths that escaped the first attempt are now asserted explicitly in
lib/review-coverage.test.mjs, on both sides of the classification.