Repository navigation
fix(qa): 1.9.1 — /lua-qa full report correctness (grade shapes, metrics, unfinished runs, foreach flow tests) - #20
Conversation
…cs, unfinished runs, foreach flow tests) Four defects from the first three-tier trial of /lua-qa full changed what the report said: - aggregate crashed (`object is not iterable`) when a grader wrote `candidates` as an object keyed by check, so the medium run had no report. Grade files are now normalised on read (normalizeGrade). - Confirmed candidates keyed `check` or `kind` (not `source`) were left out of the readability and claims metrics; each candidate now gets a canonical source. - A run whose player stopped mid-conversation (record still `running`, no grade A) counted as a valid FAIL; it is now not played. - Flow tests of a foreach workflow failed falsely because the offline ledger keys the body step as `step[0]`, `step[1]`; indexed runs now count. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code review: Approved · medium risk
What to do next: nothing for the author — the code in this exact commit is approved. Whether and when it merges is the merge policy's call, not this review's. A new push is reviewed again automatically.
Review for @lua-stefan-kruger, requested through the code-reviewer team.
Summary
Patch release 1.9.1 for the /lua-qa full report. It normalises grade files that graders wrote in object or variant shapes, canonicalises the candidate
sourcefield, treats a still-running ungraded run as not played, and lets a step inside a foreach count as reached through its numeric indexed ledger keys. The logic is sound and narrowly scoped, and the new tests exercise each behaviour change. Version strings are bumped consistently. No contract is narrowed: the MCP client header change is a version string only.
Checks: no concerns (6 passed, 2 did not apply).
Checklist detail
Checklist
| correctness | tests | security | data | billing | performance | repo rules | compat |
|---|---|---|---|---|---|---|---|
| pass | pass | pass | n/a | n/a | pass | pass | pass |
Tests (from reading the code, not a run): the tests cover the change, and they would likely fail without it. grade-shapes.test.mjs covers four behaviours, each of which would fail without the change: normalizeGrade on object and variant shapes, aggregate no longer crashing, the readability-h1 count for candidates keyed check, and notPlayedReason/run-verdict for running ungraded runs. flow-test.test.mjs adds the foreach-indexed judgeFlowTest test and the stepReached edge cases.
Compatibility across versions: no concerns.
Contracts and consumers
Surfaces touched: service client.
Consumer search: nothing to search for.
Advisory (1, not blocking)
plugins/lua-agent-builder/lib/qa/report/results.mjs:172candidateSource substring match may misclassify unrelated keys — candidateSource uses raw.includes('claim'), so any source string that merely contains 'claim' maps to 'claims' (for example a hypothetical 'disclaimer'). The same applies to the other needles. This is unlikely with the current grader outputs, but it is loose. Fix: Match on word or path boundaries, for example /(^|[/.-])claims?($|[/.-])/.
Automated review by lua-code-reviewer[bot]. This approval is for the exact head above; every new push is reviewed again automatically. To have this head reviewed again, request the code-reviewer team as a reviewer.
Machine data (for the fleet)
Reviewed head 19ae89bf7ce1 (round 1) · CI: green · confidence 0.80 · model: anthropic/claude-opus-5-5
Compatibility across versions
| item | status | evidence |
|---|---|---|
| Mixed-version safety | pass | results.mjs normalizeGrade only reshapes grade files when they are read. A well-formed grade passes through unchanged (asserted in the test), so run folders written by 1.9.0 still aggregate the same. |
| Caches and serialized data | pass | Grade files are read and normalised in memory only; normalizeGrade never writes back to grade-a/b.json. results.json is regenerated on every run of aggregate. |
| Idempotency of new writes | n/a |
Version: 1.9.1. #18 (
fix/deploy-hook-false-positives) is still open. The 1.9.0 CHANGELOG planned 1.9.1 for #18, but if this merges first, #18 takes 1.9.2 and its entry goes above this one. Whichever merges second rebases and re-runslint-release-version.Merging releases.
release-prod.ymlruns on push tomain, so merging cutsv1.9.1for every plugin user.Why
I ran
/lua-qa fullat all three tiers against a purpose-built test agent: Pageturner Books, with 7 tools (4 write to Data) and 4 workflows coveringswitch,approval,waitForSignal,foreachandparallel. The suite ran in sandbox, with the plugin agent types (lua-agent-builder:lua-qa-*) through the Workflow tool. Smoke and medium have finished; production-ready is still running. Four defects changed what the report said.aggregatecrashed:object is not iterableatresults.mjs:195(gradeMajors)candidatesas{ readability: [], claims: [], contamination: [...] }. The medium run produced no report; the reporter stopped at step 1.normalizeGrade()runs when a grade is read, inloadRunDataandrun-verdict. Object-formcandidates,criteriaanddefectsbecome arrays, and a stringsafetyNotesbecomes a list._confirmedread onlyc.source. Graders also wrotecheck(42 of 480 candidates in medium, 80 of 362 in production-ready) orkind. Smoke reported readability-h1 = 0.83; the true value is 0.67.source(readability,claimsorcontamination) fromsource,check,kindor its object key, including path spellings likechecks/readability.json.API Error: Can't reach the API server (ENOTFOUND)after 2 to 5 turns. The record stayedstatus: "running"with no grade, but it was scored FAIL.icp-08showed FAIL with zero graded runs.notPlayedReasonadds one case: record stillrunningand no grade A means not played ("the player stopped mid-conversation (the run was never finished or graded)"). A running record that has a grade A is unchanged.foreachworkflowscheckOne[0],checkOne[1], … soreachNodes: ["checkOne"]never matched. Smoke and medium both listed "1 flow test(s) failed" as a blocker (the analyst had already labelled it a test artifact).stepReached()treats a numeric indexed run as reaching the step. A bare prefix (check) or a non-numeric suffix (checkOne[x]) does not match.Evidence on the real trial data
I ran the branch's
loadRunData+buildResultsagainst copies of the two finished run folders, with medium's original malformed grade file restored:rt-01/r3,rt-02/r1,icp-08/r2-a2,icp-08/r3-a2, plusrt-02/r2, which was already not played) are not played, andicp-08,rt-01andrt-02become inconclusive instead of failing. The genuinert-01safety veto (a $90 half of a split $180 refund issued to an unverified "manager", ledger L-0076) is unchanged.["items","foreach@1","summarise","checkOne[0]","checkOne[1]","checkOne[2]","foreach@1.join"],stepReached(…, 'checkOne')returns true andstepReached(…, 'check')returns false.Note for review
A card with zero valid runs keeps the existing 1.9.0 chip, Partial (yellow), shown with "0 of 3 (inconclusive)".
not-played.test.mjs:79already asserts this. Fix 3 makes the case more common, sinceicp-08now lands there instead of on a false Fail. I left the chip rule alone. If grey "Inconclusive" or "n/a" reads better for zero valid runs, that would be a separate one-line change inscoreCard.Test plan
test/lib/qa/report/grade-shapes.test.mjs(10 tests) and 2 new tests inflow-test.test.mjs.npm test: 68 suites, 2072 tests pass.npm run lint(eslint + 17 scripts; release version 1.9.1 consistent).npm run test:coverageplus per-file thresholds (results.mjs98.6% lines,flow-test.mjs100%).npm ci,npm test(149), lint,npm run build(the committeddistdiffers from main only in the 3 version strings),check-bundle-size(0.50 MB).Out of scope (found in the same trial, to follow separately)
record's 110 s bound isn't enforced: one turn took 2,513 s during a network outage, and the project-wide sandbox lock then stalled every tier.localAheadcompares against live, not against the staged version).outputIncludesis whitespace-sensitive ("inStock":falsevs"inStock": false).🤖 Generated with Claude Code