Feed the runner's evidence options, and add the caller-feeds standing check - #212
Merged
Conversation
…er-feeds standing check --evidence-detail was doing half its job on the typedmemeval verb. It reached the ADAPTER, which builds our own evidence view, and never reached the RUNNER: TypedMemEvalOptionMapping faithfully copies facade.EvidenceCaptureMode and facade.EvidenceTopK through to AgentEval, and BuildFacade set neither. The flag looked wired because half of it was. That is the fourth codepath in a week where reachable was not the same as fed -- after an extraction lever no harness could set, a renderer no harness could set, and a public write verb no pipeline calls. The mapping is now shared rather than duplicated (CaptureModeFor, one definition, two callers), because it was already duplicated by OMISSION once. THE STANDING CHECK is the more useful half. FacadeCallerFeedsTests reflects over every settable option the runner accepts and requires each one to be classified: either FED by BuildFacade, or DELIBERATELY DEFAULTED with the reason recorded next to it. An option nobody has classified fails the suite. It is a declared map rather than a clever diff on purpose. Comparing a built facade against a default one cannot distinguish "fed with a value that happens to equal the default" from "never fed" -- and that exact ambiguity is the bug. Hand classification costs one line when the runner gains an option; the alternative is discovering it during a paid run, which is what happened here. Nine options are recorded as deliberately defaulted, most of them judge configuration: choosing our own retry count or temperature would make our scores incomparable with anyone else's. Release 0-warn; 5192 + 732 unit tests green. 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 new regression test currently validates only the mapping function, not that BuildFacade actually wires the runner options, so it wouldn’t catch the same “reachable but not fed” regression if reintroduced.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes the TypedMemEval harness so --evidence-detail configures both consumers (the adapter’s evidence view and the AgentEval runner’s evidence capture options) and adds a unit-test “caller-feeds” standing check to prevent future runner options from being left unclassified/unwired.
Changes:
- Feed the runner’s
EvidenceCaptureModeandEvidenceTopKoptions when building the TypedMemEval facade. - Centralize evidence-detail → runner capture-mode mapping in
LongMemEvalBenchmarkProtocol.CaptureModeFor(...)for reuse across callers. - Add
FacadeCallerFeedsTeststo require every settable runner option to be either explicitly fed or intentionally defaulted (with a recorded reason).
File summaries
| File | Description |
|---|---|
| tools/AgentMemory.LongMemEval/TypedMemEvalProgram.cs | Ensures TypedMemEval’s facade passes evidence capture settings through to the runner. |
| tools/AgentMemory.LongMemEval/LongMemEvalBenchmarkProtocol.cs | Extracts shared evidence-detail → capture-mode mapping for consistent wiring. |
| tests/AgentMemory.Tests.Unit.LongMemEval/FacadeCallerFeedsTests.cs | Adds a standing test to force classification of all runner options and a regression check for evidence wiring. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- 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 #212 caught that the evidence test verified the MAPPING FUNCTION and never the assignment: delete the line in BuildFacade and it stayed green. A guard that cannot come out the other way -- inside the test written to enforce exactly that rule. It now calls BuildFacade itself, and that is verified by sabotage: with the assignment removed the test FAILS, with it restored the suite is 733/733. That check is the difference between a guard and a decoration, and I had shipped the decoration. BuildFacade is internal rather than private so the guard can assert the real thing. Written as a [Fact] with a loop rather than a [Theory]: xUnit requires a public test class and the detail enum is internal, so it cannot appear in a public method signature. Option names in the classification tables are now nameof(...) rather than string literals, so a renamed option breaks the build instead of silently falling out of the "classified" set -- the same failure mode one layer up. Release 0-warn; 5192 + 733 unit tests green. 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.
--evidence-detailwas doing half its job on the typedmemeval verb: it reached the adapter andnever the runner.
TypedMemEvalOptionMappingfaithfully copiesfacade.EvidenceCaptureModeandfacade.EvidenceTopKthrough to AgentEval, andBuildFacadeset neither. The flag looked wiredbecause half of it was.
That is the fourth codepath in a week where reachable was not the same as fed — after an
extraction lever no harness could set, a renderer no harness could set, and a public write verb no
pipeline calls.
The mapping is now shared (
CaptureModeFor, one definition, two callers) rather than duplicated —it had already been duplicated by omission once.
The standing check is the more useful half
FacadeCallerFeedsTestsreflects over every settable option the runner accepts and requires each tobe classified: fed by
BuildFacade, or deliberately defaulted with the reason recorded. Anoption nobody has classified fails the suite.
It is a declared map rather than a clever diff on purpose: comparing a built facade against a default
one cannot distinguish "fed with a value that happens to equal the default" from "never fed" —
and that ambiguity is precisely the bug. Hand classification costs one line when the runner gains an
option; the alternative is finding out during a paid run, which is what happened here.
Nine options are recorded as deliberately defaulted, mostly judge configuration — choosing our own
retry count or temperature would make our scores incomparable with anyone else's.
Release 0-warn; 5192 + 732 green.