fix(results): hard-remove legacy grading.json compat reads in manifest.ts - #1711
Merged
Conversation
…t.ts Found while auditing av-kfik.46.4/46.5. manifest.ts still tolerantly read a pre-major-version grading.json shape (assertion_results, verdict-as-pass, graders/evaluators as aliases for component_results) — while validate.ts already treats those same field names as hard errors. One code path rejected the legacy shape, another silently accepted it. No production users exist yet for this still-unreleased major version, so there's no on-disk bundle to preserve compatibility for. Removed rather than tested: - readGradingAssertionResults()/mapGradingAssertions() (assertion_results fallback) — dead, call site now derives directly from the component itself. - graders/evaluators branches in readNestedGradingScores() and hydrateManifestRecord()'s type augmentation — the `evaluators` fallback was explicitly commented as a v4.13-era TODO to remove once old run directories are gone. - verdict/score-threshold fallback for deriving `pass` — `pass` is always present per the current grading.json contract. Verified: 252 tests across apps/cli/test/commands/results pass; Dashboard's EvalDetail.test.ts already asserts these exact legacy fields are ignored (unaffected, no changes needed there — it was already written correctly). Live check: real eval run -> `results summary`/`results show` against the resulting bundle render correctly through the simplified code path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deploying agentv with
|
| Latest commit: |
b33c2d3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://900ed169.agentv.pages.dev |
| Branch Preview URL: | https://fix-hard-remove-legacy-gradi.agentv.pages.dev |
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.
Summary
Found while auditing
av-kfik.46.4/av-kfik.46.5(result-artifact contract cleanup).apps/cli/src/commands/results/manifest.tsstill tolerantly read a pre-major-versiongrading.jsonshape:assertion_resultsas a fallback for assertion detailverdict === 'pass'/ score-threshold guessing as a fallback for derivingpassgraders/evaluatorsas legacy aliases forcomponent_results— theevaluatorsbranch was explicitly commented"evaluators was renamed to graders in v4.13 — read both for backwards compat... TODO: remove... once old run directories are no longer in use."Meanwhile
apps/cli/src/commands/results/validate.tsalready treats all of these same field names (assertion_results,assertions,passed,evidence,verdict,graders,checks) as hard validation errors. One code path rejected the legacy shape as invalid; another silently read it. Since this major version has no production users yet, there's no on-disk bundle to preserve compatibility for — hard-removing is correct rather than adding test coverage for a fallback that will never be exercised.Changes
readGradingAssertionResults()/mapGradingAssertions()— theassertion_resultsfallback. Call site now derives directly from the grading component itself (mapComponentAssertion), matching what already happens whencomponent_resultsis present.graders/evaluatorsbranches inreadNestedGradingScores()and the ad hoc type augmentation inhydrateManifestRecord()— both now read onlycomponent_results(and the separate, still-currentscoresfield).mapGradingEvaluator()'spassderivation toevaluator.pass === true—passis always present per the currentgrading.jsoncontract, so theverdict/score-threshold fallback was dead weight.Dashboard's
EvalDetail.test.tsalready has a test asserting these exact legacy fields (assertion_results,evidence,graders,checks) are ignored when parsing a grading artifact — that code path (parseGradingArtifact/parseComponentResult) was already written correctly with no legacy tolerance, so no changes needed there. No test anywhere exercisedmanifest.ts's now-removed fallback branches (confirmed via search) — nothing to update/remove on the test side beyond the source itself.Test plan
bun run typecheck(workspace) — clean.bunx biome checkon the touched file — clean.bun test apps/cli/test/commands/results— 252/252 pass.bun test apps/dashboard/src/components/EvalDetail.test.ts— 5/5 pass (unaffected, confirms Dashboard-side parsing was already correct).examples/features/rubric/evals/operators.eval.yaml, live Azure target + grader), thenresults summary/results showagainst the resulting bundle — both render correctly through the simplified code path (assertions with text/passed/evidence, correct scores).🤖 Generated with Claude Code