eval: add first TradeEvaluation LLM judge slice - #705
Conversation
Review finding — park before merge: the owning contract has not authorized implementationThis PR is bounded and maintainer-only, but it is currently sequenced ahead of its own evidence gate.
Disposition: do not merge this PR in the current state. Park it or convert it to draft. The smallest valid resume condition is:
This is a sequencing/ownership block, not a finding that the code architecture is necessarily wrong. I have not treated CI or the synthetic bank as acceptance evidence because the owning issue deliberately requires observed product evidence first. |
Owner sequencing update — human and LLM lanes proceed separatelyThe former parking reason on this PR is superseded by the owner's explicit direction in this work session:
This changes sequencing only; it does not turn the synthetic judge into product acceptance or import any private #610 material. |
atomchung
left a comment
There was a problem hiding this comment.
Review result: changes required before merge
CI is green and several boundaries are implemented carefully: deterministic eligibility precedes model calls, the judge is blind to sibling/reference answers, per-axis ambiguity fails closed, calibration remains explicit, and receipts are covered by local data controls. I do not see a privacy or canonical-state defect in this diff.
Two findings block merging this as the first automated FOMO QA slice.
[P1] The bank grades committed prose, not the current product/agent output
TA-001 stores every answer's prose directly, and run_judge() sends that frozen prose to the judge. Nothing in this PR runs the current consider route through a supported host/model, captures the answer actually generated under the current SKILL.md / docs/output-voice.md / route prompt, or substitutes that output into the fixture.
Consequence: a later prompt/model regression can make the real product bury the lead, contradict itself, or become disclaimer-heavy while this suite remains byte-for-byte green, because it continues judging the same hand-authored passing witnesses. This is useful rubric calibration, but it is not yet automated workflow QA or a prompt/model regression harness.
Smallest viable correction:
- Keep the frozen witnesses as judge mutation/calibration assets.
- Add a candidate-generation input path that accepts or produces the current answer for the same frozen
TradeEvaluationpayload, then runs deterministic delivery/provenance checks and the semantic judge on that candidate. - At minimum, support
--answer-file(or a structured run artifact) and clearly reportfixture_witnessversuscandidate_output. Host execution can remain a later slice, but frozen-witness grading must not be presented as product regression coverage. - Add a regression proving that changing the candidate answer changes the result without editing
TA-001.
Without this, the harness proves the judge can reproduce labels selected by the author; it does not test whether FOMO Kernel currently produces a good answer.
[P2] Shared agy discovery regresses from PATH lookup to one machine-specific location
evals/judge_episodes.py previously used shutil.which("agy"), so any valid PATH installation worked. This PR changes the default to ~/.local/bin/agy and considers agy unavailable everywhere else unless the maintainer discovers and sets TR_JUDGE_AGY_PATH.
That is a backward-compatibility regression in the pre-existing question-episode judge, outside the new TradeEvaluation adapter. Homebrew, pipx, uv-tool, and other PATH installations can stop resolving.
Smallest correction: preserve TR_JUDGE_AGY_PATH as an explicit override, otherwise fall back to shutil.which("agy"); use the resolved absolute path for subprocess execution. Add tests for override and PATH-discovered cases.
Non-blocking assessment
- A dedicated answer-level adapter is defensible, but it is already becoming a second fixture/loader/coverage/report architecture beside
evals/episodes. Keep the first bank bounded and do not expand routes before the candidate-output path proves real regression value. - The durable receipt design is stronger than required for synthetic-only calibration, but reasonable because future candidate outputs may contain private decision text and it is correctly governed by
data-status/export/reset. - Owner ratification must remain separate from #610 owner-live acceptance, as the PR states.
Please address P1 and P2, then rerun the full suite and the opt-in --plan path. A real billable judge pass is not required for these code corrections.
Review feedback addressed in
|
atomchung
left a comment
There was a problem hiding this comment.
Re-review result: one remaining blocker
The previous P2 is correctly fixed: an explicit TR_JUDGE_AGY_PATH remains authoritative, otherwise the shared judge falls back to shutil.which("agy"), resolves an absolute executable path, and CI is green on the latest head.
The previous P1 is only partially fixed.
[P1] candidate_output still cannot faithfully ingest a normal product answer
--answer-file now exists, but _claim_bound_presentation() requires:
- every visible byte to be one of
agent_case.for[]oragent_case.against[]claims; - every claim to appear exactly once;
presented_textto equal the newline join of those claims exactly.
That is not the normal TradeEvaluation response contract. A real answer also contains material that is neither a case-for nor a case-against claim, for example:
- the attached basis limitation;
- a grouped unchecked boundary;
- the user-owned resolution sentence (
keep open / modify / decline); - connective synthesis between grounded claims.
To feed such an answer into this lane, the capture step must either delete that text or misclassify it as a for/against claim. In both cases the artifact is no longer the exact answer the user saw. The current tests therefore prove that changing a specially authored claim-only candidate changes the judge result; they do not yet prove that an ordinary current FOMO Kernel answer can be captured and replayed without rewriting it.
This also weakens the four selected axes: caveat_discipline is being judged only after caveats have been forced into the claim ontology, and the final resolution surface cannot be represented honestly at all.
Smallest correction
Keep production validation on the structured agent_case, but bind it to the visible answer through spans/segments rather than making the whole answer equal the case claims. For example:
{
"presented_text": "...exact full answer...",
"segments": [
{"kind":"claim_ref","side":"against","index":0,"start":0,"end":42},
{"kind":"limitation","start":43,"end":91},
{"kind":"resolution","start":92,"end":130}
]
}Then require every structured claim exactly once, reject overlapping/out-of-range spans, run number/date provenance and challenge coverage over the complete presented_text, and constrain non-claim segments to the existing limitation/resolution obligations. Another equivalent design is fine; the key acceptance test is:
Take an ordinary two-paragraph-plus-resolution TradeEvaluation answer produced under the current route contract, store it byte-identically, and pass it through
--answer-filewithout rewriting any sentence into a special claim-only form.
Until that works, describe this lane as a claim-only candidate witness, not a current product-output regression path. Because the PR body currently claims it grades an “exact newly captured answer,” I still do not recommend merge in the current state.
Remaining assessment
- CI on
5b88f02is green. - Receipt durability, ambiguity accounting, calibration disclosure, privacy controls, and PATH compatibility look sound.
- The implementation has grown to ~2.7k added lines for one first-cut evaluator. Do not add another persistence or backend abstraction while fixing this; narrow the candidate contract rather than expanding the framework further.
Final re-review update — remaining P1 and adversarial follow-ups closedLatest head: The claim-only candidate restriction is replaced by schema-v3 full-answer segmentation. An ordinary two-paragraph-plus-resolution TradeEvaluation answer now round-trips character-for-character. The complete surface is partitioned into exact The final independent reviews also found and verified fixes for four direct-call/evidence counterexamples:
Resolution option markers are mechanically checked, while the docs explicitly avoid claiming those markers prove arbitrary prose honestly offers the choices. That remaining semantic boundary stays with the relevant judge axes and owner review. Verification on the integrated head:
#610 remains the separate private human lane and is not accepted by this result. |
Refs #590. Refs #579. Refs #676. Related but independent: #610.
User before / after
Before, maintainers could replay deterministic TradeEvaluation facts and question-level judge axes, but could not collect auditable answer-level evidence for a final answer that buried the decision, contradicted itself, merely restated fields, or padded caveats. A frozen witness bank alone also could not detect a regression in a newly captured product/agent answer.
After, maintainers have two explicit opt-in paths over one fictional TradeEvaluation:
fixture_witnesscalibrates the judge against orthogonal committed mutations.candidate_output --answer-file ...grades the exact complete answer captured for the same frozen evaluation without editing the witness bank.Both paths run production provenance, challenge-delivery fidelity, and shared number/date provenance checks before any model call. The semantic judge then emits repeated per-axis evidence into a durable local receipt. The result remains advisory and uncalibrated until the owner ratifies the labels.
Two independent lanes
Neither lane consumes the other's verdict, and no #610/private trade material enters this public bank or the external judge route.
What changed
evals/judge_episodes.pyfor backend resolution, blind single-answer prompting, repeated votes, fail-closed parsing, and ambiguity behavior; preserve explicitTR_JUDGE_AGY_PATHoverride with portable PATH discovery otherwise.decision_focus,internal_consistency,decision_synthesis, andcaveat_discipline.--answer-fileinput for a byte-for-answer-equivalent normal two-paragraph-plus-resolution candidate. Ordered Unicode-character segments partition the completepresented_text: each production-validated case claim is reproduced exactly once; limitations may reference only existing challenge obligations; connective prose is declared agent judgment; the single final resolution declares and visibly names open/declined/modified; separators are whitespace only.answer_provenancevalidator, shared challenge-delivery fidelity, and number/date provenance against the complete answer. Negative engine deltas may be displayed as positive magnitudes with directional prose; direction remains structured-case/internal-consistency work.coach.pystatus/export/reset governance. Model calls remain opt-in and outside CI.Synthetic semantic evidence
The earlier live calibration run used the unchanged witness bank, prompt, and four-axis contract:
agy/gemini-3.1-pro-high, three samples per eligible answer.6 agreement / 0 disagreement / 0 ambiguousfor each axis.declared_by: agent; this proves reproduction of the bank, not owner accuracy or product acceptance.The final corrections changed candidate ingestion and deterministic receipt interlocks, not those witness labels or semantic prompts, so no additional billable run was used as a merge gate.
Verification
python3.12 tests/run_all.py: 53/53 suites passed after integratingorigin/main@cc67f93.python3.12 evals/judge_trade_answers.py --plan: 18 model calls planned, with six eligible witnesses and one deterministic skip.Python 3.11 and 3.12 remain covered by the normal PR CI matrix. A billable/nondeterministic semantic call is intentionally not part of CI.
Review / calibration decision
No product or merge-blocking choice is delegated to the LLM. Owner label ratification remains an optional next calibration step and is separate from #610's private human walk.