Skip to content

fix: harden Knowledge Graph client pagination, code normalization, and ambiguity detection - #148

Open
adnanrhussain wants to merge 2 commits into
mainfrom
ahussain/sdk-kg-hardening
Open

fix: harden Knowledge Graph client pagination, code normalization, and ambiguity detection#148
adnanrhussain wants to merge 2 commits into
mainfrom
ahussain/sdk-kg-hardening

Conversation

@adnanrhussain

@adnanrhussain adnanrhussain commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

First of four foundational SDK PRs preparing the batch CLI to run Math Standards Alignment over CSV input. Confined to the KG client: nothing is added to or removed from the package's named exports.

One caveat on compatibility. KnowledgeGraphClient is not exported, but it is structurally reachable through the exported MathStandardsAlignmentEvaluatorConfig._kgClient (an @internal test seam). A consumer with a typed mock of that seam would break here — StandardInfo gained required statementCode/normalizedCode, getStandardCandidates/getLearningComponentSet are new, and StandardInfoOptions.limit is gone. Mocks that cast (as unknown as KnowledgeGraphClient, as our tests do) are unaffected.

Three fixes

  1. getStandardsByGrade threw on paginated results — any grade with >500 standards was a hard failure. Now walks cursors.
  2. Statement codes weren't normalized. The KG matches codes case-insensitively, so 3.md.c.7.d and 3.MD.C.7.d are the same standard but cost two cache entries and two round trips.
  3. Ambiguous matches resolved silently. One code can match several standards; the client picked the first with no signal to the caller.

Validated against the Knowledge Graph, not assumed

I scanned the KG (read-only Neo4j) to check whether each fix addresses something real. All three do:

Change Evidence (Mathematics)
Pagination 16 (jurisdiction, grade) buckets exceed 500 standards; max is 1,127. The old code hard-threw on every one.
Canonical code 25% of statement codes (11,702 / 46,359) contain lowercase characters. Echoing the uppercased lookup key would have corrupted a quarter of all reported codes.
Ambiguity 1,950 of 43,019 (jurisdiction, code) pairs match multiple standards — worst case 40. After discarding zero-LC candidates and identical duplicates, 82 are still genuinely ambiguous.

Concrete example: Utah F.IF.7.b resolves to five different standards — Secondary Math I/II/III, Sec III Honors, and Precalculus — with 3/5/0/0/10 learning components and different expectations each. All five share isCurrent=true, adoptionStatus=Adopted, normalizedStatementType=Standard, and identical gradeLevel. The only disambiguator is the course, which lives on an ancestor node and is not queryable via search. So detection is the only lever available.

Notes for review

  • statementCode vs normalizedCode. StandardInfo carries both: the KG's own spelling (CCSS sub-standards are lowercase, so the uppercased form is not a valid code) and the canonical dedupe key.
  • limit removed from StandardInfoOptions, and the request now states limit: 50 explicitly. The search endpoint's default is 5, not the cap — omitting it would silently truncate a candidate list that reaches 40 in practice. The cache key doesn't include limit, so leaving it caller-configurable let two callers poison each other's results.
  • The cache holds every candidate, not just the chosen one. getStandardCandidates() exposes the list; getStandardInfo() is unchanged externally (first match, sets ambiguous). This means the resolver in the follow-up PR reuses one request — asserted by a test.
  • Latent bug fixed in passing: the getStandardInfo cache key omitted academicSubject, so the same code under Mathematics and ELA returned whichever landed first.
  • _paginate is shared by both paginated endpoints and adds a repeated-cursor guard the old LC loop lacked; the standards query is typed against the generated spec rather than Record<string, unknown>.
  • Three of the evaluator's call sites passed limit: 1, which made ambiguity detection unreachable on the only path that runs today. Those are updated here.
  • ambiguous is observable, not just recorded. The evaluator warns with the chosen uuid and description so a surprising result is traceable to the choice rather than the model, and an ambiguous code is treated as relevant during coarse filtering — that filter sees one arbitrary candidate's description and could otherwise produce a false negative.
  • Not-found is raised per caller, not inside the shared request, so concurrent callers with different spellings of one code share a lookup but each see their own spelling.
  • _paginate bounds itself with MAX_PAGES against a server minting a fresh cursor per page.
  • Undescribed learning components are counted, not just dropped. The KG documents description as optional, and alignment is judged from that text, so an undescribed component is unevaluable — but discarding it silently makes a populated standard look unauthored. getLearningComponentSet returns { components, undescribedCount } so a caller can tell the two apart; getLearningComponents is unchanged and still returns evaluable components only.

Consequence of fixing pagination: evaluateByGrade fan-out

The old >500 throw was the only de-facto cap on evaluateByGrade. Removing it means a grade with 1,127 standards now succeeds where it previously errored — 10 questions against it is ~11,270 LLM calls.

Rather than reintroduce a cap (which would re-break exactly the grades this PR fixes), evaluateByGrade now warns above 500 projected pairs, reporting question count, standard count and total, so an expensive run is an informed one. Hard limits stay in the batch layer, where maxInputRows already lives and where the CLI can prompt.

It also dedupes the code list, which fixes a real bug the pagination change made more visible: a jurisdiction reusing a code across courses returns one item per course, so byStandard previously emitted duplicate rows for every such code. evaluateItems deduped internally, so LLM cost was unaffected — only the report was wrong.

The evaluator still evaluates the first candidate; pruning by learning components lands in #149, and this PR's ambiguity warning covers the interval.

Verification

npm run lint (0 errors), typecheck, test:unit — 343 passing; KG suite 19 → 40 tests.

Mutation tested with Stryker: 82% on covered code (149 killed, 33 survivors). Several tests here exist specifically because a mutant survived without them, and two that earned nothing measurable were deleted. One earlier attempt asserted the wrong thing entirely — a URL assertion that passed while its mutant lived, because the line under test builds the cache key, not the request.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens the internal Knowledge Graph client for upcoming batch evaluation workflows.

Changes:

  • Adds cursor pagination with malformed/repeated-cursor protection.
  • Normalizes statement-code lookups and cache keys.
  • Detects ambiguous matches and fixes subject-aware caching.

Reviewed changes

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

Show a summary per file
File Description
client.ts Implements pagination, normalization, and ambiguity detection.
types.ts Extends resolved standard metadata.
index.ts Re-exports normalization helper.
standards-alignment.ts Removes obsolete lookup limits.
client.test.ts Adds KG hardening regressions.
standards-alignment.test.ts Updates evaluator expectations.
spec.md Documents requirements and acceptance criteria.

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

Comment thread sdks/typescript/src/knowledge-graph/client.ts
Comment thread sdks/typescript/specs/001-kg-client-hardening/spec.md Outdated
@adnanrhussain
adnanrhussain force-pushed the ahussain/sdk-kg-hardening branch 2 times, most recently from 7a60394 to 40a2a1b Compare August 6, 2026 23:05
@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.59091% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
sdks/typescript/src/knowledge-graph/client.ts 97.22% 2 Missing ⚠️
...escript/src/evaluators/math/standards-alignment.ts 93.75% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comment thread sdks/typescript/src/knowledge-graph/types.ts
@adnanrhussain
adnanrhussain force-pushed the ahussain/sdk-kg-hardening branch 2 times, most recently from 009d334 to 0192bb5 Compare August 7, 2026 03:58
@adnanrhussain
adnanrhussain marked this pull request as ready for review August 7, 2026 04:14
@adnanrhussain
adnanrhussain requested a balanced review from Copilot August 7, 2026 04:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (2)

sdks/typescript/src/evaluators/math/standards-alignment.ts:455

  • When coarse filtering is enabled, this still uses only the first candidate's description and discards ambiguous. If that description is irrelevant but another standard sharing the code is relevant, the model can filter the code out; _evaluateCore then never runs, so the new ambiguity warning is also skipped. Until candidate resolution is available, ambiguous codes should bypass coarse filtering (or all candidate descriptions should be supplied) so an arbitrary first match cannot create a false negative.
          this.kgClient.getStandardInfo(code, { jurisdiction, academicSubject: KG_SUBJECT })

sdks/typescript/src/knowledge-graph/client.ts:121

  • This cached request closes over the first caller's raw spelling. Concurrent normalized variants share the promise, so if the lookup is empty, later callers receive Standard not found with the first caller's code rather than their own, contrary to the new error-message contract. Cache an empty candidate result and construct the not-found error after each caller awaits it, preserving both request deduplication and caller-specific messages.
      p = this.limit(() => this._fetchStandardCandidates(statementCode, normalized, opts));

@adnanrhussain
adnanrhussain force-pushed the ahussain/sdk-kg-hardening branch from 0192bb5 to bb2154e Compare August 7, 2026 04:47
@adnanrhussain

Copy link
Copy Markdown
Collaborator Author

Both suppressed comments were valid and are fixed in bb2154e5.

Cached not-found message — real bug, and my own test missed it by only exercising a single caller. _fetchStandardCandidates now returns [] and getStandardCandidates raises after each caller awaits, so concurrent callers with different spellings each see their own. Test asserts that with one shared request.

Coarse filter — an ambiguous code is now treated as relevant rather than filtered on one arbitrary candidate's description, matching the filter's existing fail-open behaviour for omitted codes. Test covers it.

Both verified killed under mutation testing.

@adnanrhussain
adnanrhussain force-pushed the ahussain/sdk-kg-hardening branch from bb2154e to 3a3777f Compare August 7, 2026 04:55
@adnanrhussain

Copy link
Copy Markdown
Collaborator Author

Good catch, and correct — that was a regression I introduced. Moving the not-found throw out of _fetchStandardCandidates (to give concurrent callers their own spelling) meant the fetch fulfilled with [], so p.catch never fired and the empty entry stayed cached for the client's lifetime. On main the rejection evicted it.

Fixed in 3a3777fc: getStandardCandidates evicts the entry when the resolved array is empty, guarded on promise identity so a concurrent retry isn't discarded. That keeps both properties — per-caller error message and retryable not-found.

Test added asserting a second call re-issues the request after an empty response; verified it fails without the eviction.

@adnanrhussain
adnanrhussain force-pushed the ahussain/sdk-kg-hardening branch from 3a3777f to 8ffea5b Compare August 7, 2026 05:09
@adnanrhussain

Copy link
Copy Markdown
Collaborator Author

Both fixed in 8ffea5b0.

Fan-outevaluateByGrade now warns above 500 projected pairs with question/standard/pair counts. Deliberately not a hard cap: a throw would re-break the >500-standard grades this PR exists to fix, and limits belong in the batch layer where maxInputRows already lives.

Dedupe — the code list is now deduped, which fixes a bug the review didn't flag: a jurisdiction reusing a code across courses returned one item per course, so byStandard emitted duplicate rows per such code. evaluateItems deduped internally so LLM cost was unaffected; only the report was wrong.

Two numbers in the finding were off, for the record: the grade listing is ~3 paginated list requests (1,127÷500), not 1,127 searches; and the ~1,950 ambiguous pairs are a global Mathematics figure, not per-run — within one (jurisdiction, grade) bucket it is a fraction of the per-jurisdiction counts (Arkansas 57 down to 1), so the coarse-filter fail-open adds a few percent rather than defeating filtering.

Tests updated rather than added; every changed line verified killed under mutation testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants