fix(sdk): surface a swallowed KG failure, repoint the report grade filter, honour VC's declared conditions - #247
Merged
Merged
Conversation
…lter, honour VC's declared conditions
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR tightens SDK conformance to evaluator contracts and fixes a few correctness gaps: it surfaces Knowledge Graph (KG) prefetch failures instead of silently reporting 0/0, corrects the standards report grade filter to use the formatter’s emitted field, and ensures vocabulary-complexity routing / preprocessing behavior is derived from the contract rather than hardcoded.
Changes:
- Preserve and report KG learning-component prefetch failures on coarse-filtered math standards alignment results (with logging + unit test coverage).
- Fix the standards report template to filter/render
gradeLevel(and assert template/formatter agreement via unit test). - Make
vocabulary-complexitybranch selection and FK preprocessing conditional followconfig.json(prompt layer + evaluator), plus update related tests and stale readability JSDoc.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdks/typescript/src/evaluators/academic-standards-alignment/mathematics/math-standards-alignment.ts | Record KG prefetch failures and surface them on coarse-filtered results. |
| sdks/typescript/tests/unit/evaluators/math-standards-alignment.test.ts | Add regression test ensuring KG prefetch failure is observable on coarse-filtered path. |
| sdks/typescript/src/batch/families/standards-report.html | Use gradeLevel for grade filtering and display in the standards report template. |
| sdks/typescript/tests/unit/batch/standards-family.test.ts | Assert the report template references gradeLevel and formatter still emits it. |
| sdks/typescript/src/evaluators/student-facing-text/ela-reading/vocabulary-complexity.ts | Honor contract-declared grade conditions and FK preprocessing condition (skip FK for 5–12). |
| sdks/typescript/src/prompts/vocabulary-complexity/system.ts | Route system prompt by contract condition rather than hardcoded grades. |
| sdks/typescript/src/prompts/vocabulary-complexity/user.ts | Route user template + FK substitution by contract conditions and guard FK substitution. |
| sdks/typescript/tests/unit/prompts/vocabulary-complexity.test.ts | Add contract-conformance tests for branch selection and FK binding behavior. |
| sdks/typescript/tests/unit/evaluators/vocabulary-complexity.test.ts | Loosen mocked timing assertion to allow 0ms processing time. |
| sdks/typescript/src/evaluators/multi-step.ts | Add requirePreprocessing helper for contract-derived preprocessing lookup. |
| sdks/typescript/src/features/readability.ts | Update stale JSDoc about remaining PREPROCESSING_GAPS caller(s). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Merged
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.
Four independent findings from the code review. Items 2 and 7 follow in their own PRs; 6 and 8 are deferred.
1. A swallowed Knowledge Graph failure reported as real data
math-standards-alignment.tsprefetched learning components with.catch(() => undefined). A miss then fell through tototalCount: cached?.components.length ?? 0, so a failed KG call reported0/0— indistinguishable from a standard that genuinely has no learning components.The failure now travels on the result's
error, which already means exactly this: "alignedCount is 0 because nothing was measured, not because nothing aligned." A warning is logged too.totalCountstill reads 0, but a caller can now tell why.3. The standards report's grade filter emptied the table
standards-report.htmlfiltered and displayedit.grade; the formatter emitsgradeLevel. Sinceit.gradewas alwaysundefined, selecting a grade filtered out everything rather than filtering. Three uses repointed.Nothing caught it because the filter is browser JS in a template that no test executes. It is now asserted on the template text — no bare
it.grade,it.gradeLevelpresent, and the formatter still emits that field — the same approach the evaluator-ordering check uses.4. JSDoc that miscounted by 3×
readability.tssaid "see PREPROCESSING_GAPS … for the three evaluators that call this instead." #237 migrated two of them: there is one caller and one gap entry. A stale pointer is worse than none, because it is the one people follow.5.
vocabulary-complexityignored its own declared conditions — in three placesThe contract declares
fk_scoreconditional on grades 3-4, and the two complexity steps mutually exclusive on the same grades. The SDK restated that rule rather than reading it:prompts/…/user.tsstudentGradeLevel === '3' || === '4'chose the templateprompts/…/system.tsAll three now read the contract.
grepfor=== '3'in the evaluator or prompt layers returns nothing.Also fixed:
getUserPromptcalledfkLevel.toString()unconditionally, so an absent score would have put the string"undefined"into any template carrying the token.Two tests I had to rewrite because they proved nothing
Worth recording, since both passed on the first attempt and neither could fail.
The condition tests. I first asserted the rendered prompt contained no
{fk_score}and no"undefined". Both hold whether or not the condition is honoured — the other-grades template never carried the token. Reverting all three fixes left 14/14 passing. They now assert what is observable: that the branch selection differs by grade, and that the grades 5-12 prompt renders identically with or without a score. Each fix reverted fails one.The prefetch test. It asserted the coarse-filtered result carried the error — and passed with the fix reverted, because the coarse mock marked the standard relevant, so it reached the evaluation error path instead. The mock's verdict is now controllable and the test forces
relevant: false, plus assertscoarseFiltered === trueso it cannot silently drift back to the other branch.Validation
typecheck,lint,test:unit(1129, up from 1121),build,scripts/check.pyfk_score→ 1; report grade field → 1; prefetch error → 1generate-schema.test.tsspawns a subprocess and timed out at 6959ms under load, passing 38/38 in isolation. Not touched here.One test assertion loosened
processingTimeMs > 0in the vocabulary-complexity test began failing: with both providers mocked and the 5-12 branch no longer computing a readability score, the evaluation finishes inside one clock tick. It now asserts>= 0, with the reason recorded — a positive wall-clock on a fully mocked path was measuring that FK happened to take a millisecond, not anything about the product.