Close the id-keying audit: make the corpus-identity gate a tested unit - #214
Merged
Conversation
…audit Discharges the family-wide audit: every cache, baseline, join and tool of ours that could key on question_id/corpus_id/revision across corpus versions. RESULT: one gap, in one surface, already closed in #213 -- the --regrade cross-artifact join, whose position-and-text keying catches a reworded redraw and catches nothing when the text is stable and only the gold moves. Everything else is clean, and two of those are worth recording rather than merely ticking: - PREPARED-CORPUS REUSE already compares datasetSha256 alongside eleven identity fields, and its own header states the principle this audit is about -- "treating unknown as equal is how a check stops being able to fail". It reports unrecorded fields on older manifests as DRIFT rather than agreement. That is the standard the re-grade has now been brought up to. - No other verb reads two artifacts at all, so --regrade was the only place the hazard could live. WHAT THIS COMMIT ADDS: the gate's decision was a code path only exercisable by running the tool against a hand-mutated artifact. It is now CorpusIdentity.Verify with three verdicts and unit tests for each, including hex-casing (artifacts and manifests disagree about it, and that is not a redraw). Unverifiable stays its own verdict deliberately. Folded into Match it passes silently -- the constant-column failure this project has been bitten by three times. Folded into Mismatch it blocks every artifact older than provenance capture, making the gate obstructive enough to get bypassed. So it warns and refuses to be read as agreement. Release 0-warn; LongMemEval 742/742. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9gMsvo3SpYqEBENbaefo
There was a problem hiding this comment.
🟡 Changes recommended
The mismatch abort message can throw at runtime when slicing short/corrupt SHA strings ([..16]), which undermines the “fail safely” goal of the gate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extracts the corpus-identity gate used by typedmemeval --regrade into a testable unit (CorpusIdentity.Verify) so re-grading can reliably detect corpus redraws (where question_id and even question text may remain stable while gold changes), preventing meaningless agreement results.
Changes:
- Replaces the inline corpus SHA gate in
TypedMemEvalRegradeProgramwith aCorpusIdentity.Verifyhelper that returns a 3-way verdict (Match / Mismatch / Unverifiable). - Updates re-grade flow to switch on the verdict, aborting on mismatch and warning (without failing) when identity cannot be verified.
- Adds unit tests covering all three verdicts, including case-insensitive SHA comparisons.
File summaries
| File | Description |
|---|---|
| tools/AgentMemory.LongMemEval/TypedMemEvalRegradeProgram.cs | Extracts corpus-identity comparison into CorpusIdentity.Verify and switches re-grade gating logic to use a 3-way verdict. |
| tests/AgentMemory.Tests.Unit.LongMemEval/CorpusIdentityTests.cs | Adds unit tests verifying Match/Mismatch/Unverifiable behavior, including hex-casing tolerance. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot's review of #214: the mismatch message sliced both shas at a fixed [..16], which throws ArgumentOutOfRangeException on a truncated or corrupt value -- in the one branch whose entire purpose is to fail SAFELY with an exit code rather than an exception. Second time on this same gate. #213 hardened the sha's TYPE (a non-string CorpusSha256 would have thrown out of GetString()) and left its LENGTH assumed. Same defensive question, same guard, asked about one property and not the next. A guard that can throw is not a guard. Verified end to end with a truncated sha: it now aborts cleanly and prints the short value as-is rather than crashing. Release 0-warn; LongMemEval 744/744. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RM9gMsvo3SpYqEBENbaefo
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.
Discharges the family-wide audit of anything that could key on
question_id/corpus_id/revisionacross corpus versions.Why it was needed: AgentEval redraws corpora keeping the
question_idset 100% identical withzero byte-identical items, and neither
corpus_idnorrevisionmoves —corpus_sha256is theonly distinguishing field. For bitemporal, 27 of 60 items keep the same question text with a
different gold. Anything joining across corpus versions on an id is silently wrong, in the
direction that yields a confident number rather than an error.
Result: one gap, one surface, already closed in #213
--regradecross-artifact joindatasetSha256+ 11 identity fieldsTwo results worth more than a tick: prepared-corpus reuse already compares
datasetSha256and itsown header states the principle this audit is about — "treating unknown as equal is how a check stops
being able to fail" — reporting unrecorded fields on older manifests as drift, not agreement.
That is the standard the re-grade has now been raised to. And no other verb reads two artifacts,
so
--regradewas the only place the hazard could live.What this PR adds
The gate's decision was a code path only exercisable by running the tool against a hand-mutated
artifact. It is now
CorpusIdentity.Verifywith three verdicts and unit tests for each — includinghex casing, since artifacts and manifests disagree about it and that is not a redraw.
Unverifiablestays its own verdict deliberately. Folded intoMatchit passes silently — theconstant-column failure this project has hit three times. Folded into
Mismatchit blocks everyartifact older than provenance capture, making the gate obstructive enough to get bypassed. So it
warns at the call site and refuses to be read as agreement.
Release 0-warn; LongMemEval 742/742.