fix: isolate per-pair failures in evaluateItems instead of discarding the batch

[adnanrhussain]

Third of four foundational SDK PRs. Independent of #148/#149 — based on main.

The bug: evaluateItems collected per-pair outcomes with Promise.allSettled and then did throw errors[0], at both the pair and item level. So one typo'd code, one 429, or one malformed model response discarded every other pair's completed LLM work — one bad row in a 50-row batch threw away the other 49. The unreachable {statementCode: '', ...} fallback it left behind shows per-pair isolation was the original intent. evaluateByGrade inherits all of it, and is the mode most likely to hit one dud standard out of ~30.

What changed:

• Failing pairs return a result carrying error and the correct statementCode (available by index — the dead fallback used '' because it had lost the mapping).
• Input validation is per item, not a precondition for the whole call.
• evaluate() still throws — one pair wants an exception, 200 want a report.
• byStandard gains evaluatedCount/errorCount/filteredCount/noComponentsCount, which sum to the question count so no outcome is unaccounted for. coverageCount: 0 previously conflated "every pair errored", "every pair was filtered", and "genuinely didn't align" — presenting an unmeasured standard as an uncovered one.
• The LLM limiter moved to the instance; it was per call, so N concurrent calls each got their own pLimit(10).

Not marked breaking, deliberately. It is one in the strict sense, but these methods have no external consumers — the only in-repo caller is the demo server, pinned to the published ^0.8.0. And release-please-config.json sets no bump-minor-pre-major, so a ! would cut 1.0.0 and declare the SDK stable as a side effect of an error-handling fix. Add that setting first if a future change needs the marker. Callers wanting the old semantics: standards.some(s => s.error).

Two known downstream consequences, both for the demo server (pinned to the published ^0.8.0, so neither bites until we bump it):

• Sharing the instance limiter means its concurrent requests share one queue, so a 30-standard request can delay the next interactive one. It can pass options.concurrency to opt out.
• Its /api/evaluate route relies on evaluateItems rejecting to return a 500. Failures now resolve with per-standard error fields, so the handler must inspect them instead. Follow-up, not part of this PR.

A caller's onProgress cannot affect evaluation. Progress is counted once outside the try, and the callback runs through a guard that logs and continues — otherwise a throwing callback would discard a finished result, double-count, and reject the whole call, which is the failure this PR removes.

error carries name, statusCode and retryable as well as message and code, so a report can group failures by kind and separate transient ones. name is needed because subclasses share a code.

npm run lint (0 errors), typecheck, test:unit — 332 passing.

Mutation tested with Stryker: 70.6% on covered code (216 killed, 90 survivors), with every changed region fully killed. The new tests were verified load-bearing rather than assumed — skipping them raises the survivor count.

[Copilot]

Pull request overview

Improves the TypeScript SDK’s Math Standards Alignment evaluator to preserve partial results in batch modes (evaluateItems, evaluateByGrade) by isolating per-pair and per-item failures, while keeping evaluate() as a throwing primitive. This prevents one bad (question, standard) pair (or one invalid item) from discarding the rest of the batch’s completed work.

Changes:

• Update evaluateItems to return per-pair error results (with correct statementCode) instead of rejecting the batch on the first failure.
• Make question validation per-item and adjust progress reporting denominator to exclude invalid items / filtered pairs.
• Extend evaluateByGrade aggregation to track evaluatedCount, errorCount, and filteredCount, and share an instance-wide LLM concurrency limiter across concurrent calls.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
sdks/typescript/src/evaluators/math/standards-alignment.ts	Implements per-pair error isolation, per-item validation behavior, by-standard metrics, and shared instance concurrency limiting.
sdks/typescript/tests/unit/evaluators/math/standards-alignment.test.ts	Adds unit tests covering isolated failures, correct attribution, progress reconciliation, by-standard accounting, and shared concurrency behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 97.91667% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...escript/src/evaluators/math/standards-alignment.ts	97.91%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[adnanrhussain]

Fixed the P0 and finding 4 in 99799a6c; findings 2, 5 and 6 declined or deferred with reasoning.

P0 (progress inside try/catch) — confirmed and fixed. completed++/onProgress now run once, outside the try, and the callback is invoked through a guard that logs and continues. A throwing callback can no longer discard a finished result, double-count, or reject the batch. Your read was right that this reintroduced both halves of the bug the PR exists to remove.

Finding 4 (error channel dropped detail) — error now carries name, statusCode and retryable alongside message and code. name matters because subclasses share a code, so a report grouping by kind needs the narrower field.

Finding 3 — fixed, and it was the same defect as my own error fix: a zero-learning-component pair was landing in evaluatedCount, making "0 aligned of 1 evaluated" read as a judgement rather than absent data. evaluatedCount now requires totalCount > 0.

Finding 2 (item with empty statementCodes loses its validation error) — real, deferred. Surfacing it needs an item-level error channel on the return type, which is a wider API change than this PR should carry, and the input is doubly degenerate (invalid question and no codes). Worth its own issue.

Finding 5 (options.concurrency builds a fresh limiter) — working as documented, not a defect. It is the deliberate escape hatch; the doc comment states that N concurrent calls each supplying it get N times the budget, and the demo server needs exactly that to avoid queueing interactive requests behind a large batch.

Finding 6 (demo route) — correct, and a genuine follow-up: the demo pins the published ^0.8.0, so it is unaffected until we bump it, at which point the handler must inspect per-standard error instead of relying on a rejection. Noted in the PR body.

Mutation tested: all four changed regions fully killed; the two new tests kill 9 mutants (89 → 95 survivors without them). File is at 69.8% on covered code, up from 64.2%.

[adnanrhussain]

Three of four addressed in 369b51f1; the concurrency one is a deliberate design choice and I'll stop relitigating it.

Empty statementCodes (your finding 4) — you were right to push back, and I was wrong to defer it. The old code threw; the new code dropped the failure entirely, which is a hole in the contract this PR introduces, not an edge case. QuestionResult now carries an item-level error, set whenever the question itself fails, so it surfaces regardless of how many codes the item has. Exported as a named type.

Validation error bypassing describeFailure — correct and my inconsistency: I wrote a doc saying name is carried so reports can group by kind, then left the one path that skipped it. Now goes through describeFailure like every other failure.

Zero-LC pair in no counter — added noComponentsCount. The four denominators now sum to the number of questions, so a zero coverageCount is always attributable rather than reading as all-zeros with no signal. Doc updated to say that instead of citing only errorCount.

llmConcurrency dead field — removed; it was read only to build the limiter on the next line.

Concurrency escape hatch — declining, final answer. It is doing what it documents: options.concurrency is a per-call budget, and N callers supplying it get N times that. Your measurement (1 → 8 in flight) is the documented behaviour, not a deviation from it. The instance limit is a default, not a ceiling, because the demo server needs to opt out — its interactive requests must not queue behind a 30-standard batch. Making it a hard ceiling would remove that, and nesting the limiters would too. If we want a real ceiling later, that is a design change with its own trade-off, not a fix here.

Mutation: all four changed regions fully killed; file at 71.2% covered, up from 69.8%.