[FEAT]: Add durable terminal evaluation contract - #146
Spencer Schoenberg (spencrr) wants to merge 4 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
3cc7cc8 to
fb90c72
Compare
c3b5f80 to
9b7c6a6
Compare
9b7c6a6 to
ef98095
Compare
There was a problem hiding this comment.
🟡 Changes recommended
A huge terminal confidence value can raise an uncaught overflow and abort controller-side xdist merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds durable terminal-evaluation and population provenance across core results, reporting, and xdist transport.
Changes:
- Adds terminal evaluation, trace-end reason, evaluation-purpose, and resolver APIs.
- Centralizes population validation.
- Extends persistence, truncation handling, tests, and documentation.
File summaries
| File | Description |
|---|---|
rampart/core/_population.py |
Adds shared population validation. |
rampart/core/types.py |
Adds provenance enums and turn purpose. |
rampart/core/result.py |
Extends results and verdict resolution. |
rampart/core/execution.py |
Validates trial parameters early. |
rampart/core/__init__.py |
Exports new core APIs. |
rampart/pytest_plugin/_xdist.py |
Transports and validates provenance. |
rampart/reporting/json_file.py |
Serializes terminal metadata. |
rampart/probes/_single_turn.py |
Clarifies turn-limit behavior. |
rampart/probes/_factory.py |
Updates probe API documentation. |
rampart/attacks/_xpia.py |
Clarifies XPIA turn limits. |
rampart/attacks/_factory.py |
Updates XPIA API documentation. |
tests/unit/core/test_types.py |
Tests enums and turn validation. |
tests/unit/core/test_result.py |
Tests result contracts and resolvers. |
tests/unit/core/test_execution.py |
Tests early trial validation. |
tests/unit/pytest_plugin/test_xdist.py |
Tests transport and truncation. |
tests/unit/pytest_plugin/test_xdist_aggregation.py |
Tests worker/controller persistence. |
tests/unit/reporting/test_json_file.py |
Tests JSON provenance output. |
docs/api/core-types.md |
Documents new core APIs. |
docs/usage/results-and-reporting.md |
Explains result provenance. |
docs/usage/xdist.md |
Documents envelope behavior. |
docs/probes/behavioral.md |
Updates probe turn-limit semantics. |
docs/attacks/xpia.md |
Updates XPIA turn-limit semantics. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ef98095 to
acbf019
Compare
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Extreme malformed integers can escape the xdist fail-closed error boundary.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
rampart/pytest_plugin/_xdist.py:998
- A sufficiently large integer overflows
float(), then its!rconversion can itself raiseValueErrorunder Python's integer-string digit limit. That exception escapes instead of becomingWorkerOutputError, so malformed worker data can abort the pytest hook rather than marking the run incomplete. Render the value through the existing safe string helper.
msg = f"Confidence could not be converted to float: {raw_confidence!r}."
- Files reviewed: 23/23 changed files
- Comments generated: 3
- Review effort level: Balanced
Keep numeric overflow and evaluation text conversion failures inside the worker-output error boundary so controllers mark runs incomplete and preserve earlier results.
Require explicit response scopes and distinguish online evidence from terminal verdict input without an alias. Apply canonical policies consistently to shared evaluation schemas and version the tightened population contract. Migrate callers and document direct pre-1.0 API replacement while preserving fail-closed records.
acbf019 to
226cf4e
Compare
Use final_trace_evaluation consistently in Result, canonical records, JSON reports, and xdist transport. Update examples and round-trip tests without retaining an alias for the earlier spelling.
Nina Chikanov (nina-msft)
left a comment
There was a problem hiding this comment.
Thanks for addressing my previous comments! This round of review focused on the major bump to the schema and associated changes. I don't think the deprecation change needs a lot of discussion but is worth aligning on with Behnam at the very least (since he helped push through #185) in a quick call or in PL.
| "previous_contract_sha256": "da1a36daa27b5fc2cdaf4305ba3aa743f52789643de94ed97916ebbaa5fc2b47", | ||
| "decision": "new-major", | ||
| "rationale": "Compared with the actual PR target upstream/main at 6c8a8b74f729d6768014793031610493ebe7f74f, v2 narrows PopulationRef.id from any string to a nonempty string. Live population constructors and canonical boundary revalidation enforce the provenance invariants; the fingerprint now also tracks their shared _population.py policy. Optional final_trace_evaluation, trace_end_reason, and turn eval_purpose fields have defined absence semantics and do not conflate final verdict input with online evidence. The unmerged terminal_evaluation spelling is replaced by final_trace_evaluation across the API, canonical schema, xdist transport, and JSON reports without an alias. Shared EvalResult definitions consistently apply canonical strict-type, finite-number, and Unicode policies without changing independent adapters. The published v1 schema is preserved unchanged, but this codec reads and writes only v2 and rejects unsupported versions. No aliases, deprecation window, dual reader, or upcaster is promised. Canonical codec adoption by xdist and reporting remains out of scope.", | ||
| "migration_note": "docs/concepts/trace-schema.md" |
There was a problem hiding this comment.
this file should contain the migration instructions directly instead of referencing the trace-schema markdown file. e.g.:
"migration_note": "This reader accepts only v2; no v1 decoder or migration utility is provided. Convert v1 records explicitly into separate v2 records and validate them with deserialize_record(). Empty population IDs require legitimate producer provenance or regeneration. Leave unrecorded `final_trace_evaluation`, `trace_end_reason`, and `eval_purpose` fields absent or null, and preserve the original records.
I missed this in my change initially - in order for us to enable this we need to update check_trace_compatibility.py and its tests: they currently require migration_note to be a file path, rather than nonempty inline text.
imo it makes more sense to have this note inline than to create another file simply for migration notes. If you'd like I can put together a quick PR for this on Monday and then you can absorb it in this PR.
| - migrating persisted data is an explicit operation; reading never rewrites an | ||
| artifact in place; and | ||
| - encountering an unsupported major fails closed. | ||
| ## v1 to v2 migration note |
There was a problem hiding this comment.
This migration note should not live here, see my other comment on trace-compatibility.json. Please restore the Migration mechanics section with the exception that the first line being outdated since v2 exists.
|
|
||
| This is a release-support commitment; the current reader supports only | ||
| `rampart.trace.v1`. | ||
| ## Pre-1.0 support policy |
There was a problem hiding this comment.
Fair - but this is an intentional choice that wasn't covered in the design discussion for this. I'm okay with having this as our policy for as long as we don't support deprecation throughout the project, but that won't necessarily be until version 1.0.
I can imagine us being pre-1.0 for a while haha, so we may also just want to name this section ## Support Policy without any commitment to a release number.
Behnam (@behnam-o) - I'd like to hear your thoughts on our deprecation policy extending to the schema vs having migration support as previously discussed
| the migration policy requires: | ||
|
|
||
| - writers emit the latest supported major; | ||
| - each major bump ships an adjacent upcaster (`vN-1 → vN`) and an explicit |
There was a problem hiding this comment.
We originally envisioned adding migration support for each major bump. I agree that this isn't necessarily required right now since we don't have many folks relying on this schema - but would like to double check with Behnam (@behnam-o) or Bashir Partovi (@bashirpartovi) before we commit.
There was a problem hiding this comment.
This comment is kind a dup of the above btw :)
| - `pytest-xdist` itself does not support interactive debugging (`--pdb`, `--trace`); | ||
| use single-process mode for debugging. | ||
|
|
||
| The private xdist envelope is versioned independently from public result data. |
There was a problem hiding this comment.
I don't think this doc addition is necessary in its current form, please remove.
You could retain 2 user-visible details in the existing Incomplete Runs and size-cap sections:
"Malformed worker results mark the run incomplete without discarding previously received results. Truncation markers preserve population provenance when it fits; otherwise, _rampart_population_ref_omitted records its omission."
| "contract_sha256": "828c2783d81d06185caeeef1a7ff83e025d9470d3c9dc894a6a555e555c47ed9", | ||
| "previous_contract_sha256": "da1a36daa27b5fc2cdaf4305ba3aa743f52789643de94ed97916ebbaa5fc2b47", | ||
| "decision": "new-major", | ||
| "rationale": "Compared with the actual PR target upstream/main at 6c8a8b74f729d6768014793031610493ebe7f74f, v2 narrows PopulationRef.id from any string to a nonempty string. Live population constructors and canonical boundary revalidation enforce the provenance invariants; the fingerprint now also tracks their shared _population.py policy. Optional final_trace_evaluation, trace_end_reason, and turn eval_purpose fields have defined absence semantics and do not conflate final verdict input with online evidence. The unmerged terminal_evaluation spelling is replaced by final_trace_evaluation across the API, canonical schema, xdist transport, and JSON reports without an alias. Shared EvalResult definitions consistently apply canonical strict-type, finite-number, and Unicode policies without changing independent adapters. The published v1 schema is preserved unchanged, but this codec reads and writes only v2 and rejects unsupported versions. No aliases, deprecation window, dual reader, or upcaster is promised. Canonical codec adoption by xdist and reporting remains out of scope.", |
There was a problem hiding this comment.
Recommend shortening the rationale to be more clear and explain why this contract change is breaking:
"V2 requires nonempty PopulationRef.id values, rejecting records accepted by v1 and therefore requiring a major version bump. The optional final_trace_evaluation, trace_end_reason, and Turn.eval_purpose fields define absence as “not recorded”; those additions alone are compatible. Repeated EvalResult definitions now consistently receive canonical validation policies. The published v1 schema is preserved unchanged."
Description
Adds the per-execution provenance needed before final-trace verdict cadence changes.
Result.terminal_evaluationstores the evaluator output for the terminal trace,Result.trace_end_reasonrecords why trace production ended, andTurn.eval_purposeidentifies online stop checks.Result.turn_evaluationsmakes the online evidence boundary explicit whileeval_resultsremains a compatibility view.The direct attack and probe resolvers require one evaluation and reject unknown runtime outcomes instead of falling through. Population provenance now shares validation across
PopulationRef,PopulationResult, andexecute_trials_async, with invalid thresholds rejected before an execution factory runs.This PR also includes the persistence work previously split into #147 so the contract cannot land without transport support. The current xdist v2 envelope and JSON report carry terminal evaluation, trace-end reason, turn purpose, and population provenance together. Malformed worker data fails closed, including overflowing confidence values, and oversized results produce bounded incomplete markers while retaining population provenance when it fits.
Breaking changes
None for valid callers. Invalid population provenance and malformed evaluator outcomes now fail early instead of being accepted or falling through.
Checklist
pre-commit run --all-filespassesValidation: 1,122 unit tests pass. Strict documentation build and all pre-commit checks pass.