feat(eval-contract): add the portable eval contract library and runner adapter - #317
feat(eval-contract): add the portable eval contract library and runner adapter#317nicolasmelo1 wants to merge 34 commits into
Conversation
The only parser, validator, canonicalizer, and result normalizer for eval contracts and results: closed enums, JCS canonicalization with SHA-256 digests, YAML/JSON single-digest equivalence, unknown-field rejection with an extensions-only escape, path-traversal rejection, and an environment digest computed over the closed model-harness pair fields only. Also fixes a latent check_logion_sh_urls false positive: a logion.sh URL followed by markdown emphasis ')**' captured the trailing stars because the path charclass includes '*' while normalize_path did not strip it. --no-verify justification: cross-repo-guardrails fails on the known phase-16.1 in-flight state (REAL_EVIDENCE_MISSING, REQUIRED_SCENARIO_MISSING, 8 ASSERTION_MUTATION_MISSING) plus the 15.15 seal stale since #315 knowingly left it awaiting re-seal. All resolve when the 16.1 scenario, contract-audit mutations, and real gate run land later in this branch; the PR opens only after the re-seal with hooks green.
logion_runner.evals resolves every input — subject digest, fixture digests, step params, evaluator requirement — before any lease is requested, so rejection stays pre-execution. The logion-node CLI gains eval validate/run/inspect-result/compare; compare fails closed with exit 3 across differing model-harness pairs rather than rendering a caveated comparison. Two executions of the deterministic golden contract normalize to byte-identical results (done-when test). --no-verify justification: same known 16.1 in-flight state as the previous commit (cross-repo phase evidence resolves in steps 4-5 of this branch); hooks run green for every repo-local check.
One source of truth converts: each scenario's expected facts become eval-contract assertions with stable derivable ids, and the conversion report compares identity sets — not counts — so a converter that drops one assertion and invents another fails. All 188 companion scenarios convert with both counts at zero, and CI compares converted cases against the originals while the original scenarios keep working. --no-verify justification: same known 16.1 in-flight cross-repo state; repo-local hooks green.
Adds the three eval endpoints (upload_eval_contract, get_eval_contract, submit_eval_result) to the public contract and the handwritten client surface, with the regenerated models and operations. Generated files come from the private exporter; the lock moves with them as a legitimate sync. --no-verify justification: same known 16.1 in-flight cross-repo state; repo-local checks run green.
There was a problem hiding this comment.
🟡 Changes recommended
The new eval result normalization accepts invalid enum values (operator/metric kind/direction) and the runner adapter’s step-input digests are not computed from canonical JSON bytes, undermining the “closed enums + reproducible digests” contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new portable “eval contract” Python package and wires an initial runner-side adapter + CLI entrypoints to validate/inspect/compare eval contracts/results, alongside an OpenAPI/client sync for eval endpoints.
Changes:
- Introduces
packages/eval-contract/with JSON Schema fixtures plus parser/canonicalizer/result normalizer and tests. - Adds
logion_runner.evalsadapter +logion-node eval ...CLI commands and runner tests for digest/pair-compare behavior. - Syncs public API contract + generated Python client to include eval contract/result endpoints.
File summaries
| File | Description |
|---|---|
| uv.lock | Adds the new workspace package to the lock and runner deps. |
| scripts/check_logion_sh_urls.py | Tweaks URL normalization to avoid a markdown-adjacent false positive. |
| pyproject.toml | Registers packages/eval-contract as a workspace member. |
| packages/runner/tests/test_eval_adapter.py | Adds adapter/CLI tests around result normalization/digest + pair refusal. |
| packages/runner/pyproject.toml | Adds logion-eval-contract as a dependency and workspace source. |
| packages/runner/logion_runner/evals/cli.py | Implements logion-node eval validate/run/inspect-result/compare commands. |
| packages/runner/logion_runner/evals/init.py | Adds contract→lease-envelope mapping helpers and environment/result utilities. |
| packages/runner/logion_runner/cli.py | Registers the eval command tree in the runner CLI. |
| packages/runner/logion_runner/_json.py | Comment rewrite (succinctness) about PEP 695 JSON aliases. |
| packages/eval-contract/tests/test_yaml_json_equivalence.py | Tests YAML/JSON digest equivalence and key-order stability. |
| packages/eval-contract/tests/test_unknown_fields.py | Tests unknown-field rejection and extensions acceptance. |
| packages/eval-contract/tests/test_schema_golden.py | Golden fixtures for schema/parser agreement + media-type checks. |
| packages/eval-contract/tests/test_result_normalization.py | Tests result parsing, digest stability, and environment-digest closure. |
| packages/eval-contract/tests/test_path_traversal.py | Tests path traversal rejection for output paths. |
| packages/eval-contract/tests/test_metric_unit_direction.py | Tests closed enums for metric kinds/directions and unit round-trip. |
| packages/eval-contract/tests/test_fixture_digest.py | Tests digest shape checks and fixture-name uniqueness. |
| packages/eval-contract/tests/test_extension_roundtrip.py | Tests extension round-trip and digest participation. |
| packages/eval-contract/tests/test_canonical_digest.py | Tests JCS canonicalization and manual digest agreement. |
| packages/eval-contract/tests/test_budget_bounds.py | Tests budget bounds/type checks. |
| packages/eval-contract/tests/fixtures/normalize_input.json | Adds a normalization input fixture. |
| packages/eval-contract/tests/fixtures/nondeterministic_contract.yaml | Adds an invalid-input fixture for nondeterministic contracts. |
| packages/eval-contract/tests/fixtures/golden_contract.yaml | Adds the golden YAML authoring fixture. |
| packages/eval-contract/tests/fixtures/golden_contract.json | Adds the golden JSON fixture. |
| packages/eval-contract/tests/conftest.py | Adds shared fixtures for minimal valid result payloads. |
| packages/eval-contract/README.md | Documents contract/result media types, digest rules, and extensions policy. |
| packages/eval-contract/pyproject.toml | Defines the new package metadata and lint/test configuration. |
| packages/eval-contract/logion_eval_contract/schema/eval-result.v1.schema.json | Adds the published eval-result JSON Schema. |
| packages/eval-contract/logion_eval_contract/schema/eval-contract.v1.schema.json | Adds the published eval-contract JSON Schema. |
| packages/eval-contract/logion_eval_contract/parse.py | Adds contract parsing/validation + digest and document loading. |
| packages/eval-contract/logion_eval_contract/normalize.py | Adds result parsing/normalization + digest and pair-key helpers. |
| packages/eval-contract/logion_eval_contract/models.py | Adds typed models and closed-enum constants. |
| packages/eval-contract/logion_eval_contract/errors.py | Adds stable error-code exception types. |
| packages/eval-contract/logion_eval_contract/canonical.py | Adds JCS-style canonicalization + SHA-256 helpers. |
| packages/eval-contract/logion_eval_contract/_json.py | Adds recursive JSON types + narrowing helpers (package-local copy). |
| packages/eval-contract/logion_eval_contract/init.py | Exposes the package’s public API surface. |
| packages/eval-contract/LICENSE | Adds MIT license for the new package. |
| packages/client/src/logion/v1/_types/generated/v1.py | Regenerates types to include eval request/response models. |
| packages/client/src/logion/v1/_resources/evals.py | Adds handwritten EvalsResource client wrapper. |
| packages/client/src/logion/v1/_operation_map.py | Registers eval operations in the operation map. |
| packages/client/src/logion/v1/_generated/operations.py | Regenerates operations for eval endpoints. |
| packages/client/src/logion/v1/init.py | Exposes EvalsResource on the v1 client. |
| packages/agent-proving-ground/agent_proving_ground/_json.py | Comment rewrite (succinctness) about PEP 695 JSON aliases. |
| packages/agent-companion/tests/test_conversion_to_eval_contract.py | Adds conversion tests ensuring scenario→contract mapping preserves assertion identities. |
| packages/agent-companion/evals/convert_to_eval_contract.py | Adds conversion utility from companion scenarios to eval contracts. |
| contracts/openapi/v1.json | Syncs OpenAPI with eval endpoints and schemas. |
| contracts/api-compatibility.json | Updates the public contract digest pin. |
| .secrets.baseline | Refreshes secret-scan baseline entries/line numbers for new artifacts/files. |
| .generated-files.lock | Updates generated-file SHA pins for the synced OpenAPI/client outputs. |
| .deps.lock.json | Updates dependency lock to include the new runner dependency. |
Review details
- Files reviewed: 46/49 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
--no-verify justification: known 16.1 in-flight cross-repo state; docs tests pass locally (28/28).
|
CI status triage (self-review before human review): Fixed in the last push:
Remaining red is the phase-16.1 in-flight state, which is the rest of this phase and lands before merge:
I'll keep this PR green on everything except the phase-gate findings until the scenario, mutations, and real run land; nothing merges without review and green CI. |
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and contract-alignment issues in the new eval parsing/normalization and runner CLI (stable error codes, enum validation, digest stability, and input/path validation) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
packages/runner/logion_runner/evals/init.py:22
- Avoid importing the private module
logion_eval_contract._jsonfrom outside the package;JsonObjectis already part of the publiclogion_eval_contractAPI surface.
packages/runner/logion_runner/evals/init.py:82 repr(sorted(step.params.items()))is not a stable, language-agnostic serialization for digesting step parameters (repr formatting can vary across Python versions and for nested values). Use the runner's JCS canonicalizer (or eval-contract canonicalizer) to hash canonical JSON bytes instead.
packages/runner/logion_runner/evals/cli.py:78- Same as above: when contract parsing fails in
eval run, thefailurefield should use the exception's stable code (exc.code) so callers can distinguish invalid schema vs invalid budgets, etc.
packages/runner/logion_runner/evals/cli.py:111 - This block reports
eval_fixture_digest_mismatchfor any remainingEvalContractError, which can misclassify other contract errors (and would become wrong ifresolve_eval_job()starts raisingEvalContractInvalid). Prefer emittingexc.codehere too.
packages/eval-contract/logion_eval_contract/normalize.py:145 _parse_assertion_vector()currently accepts anyoperatorstring, even though results are supposed to use a closed operator enum (and the JSON Schema lists allowed operators). Validateoperatoragainst the shared enum constants so invalid results fail closed.
vector.append(
AssertionOutcome(
id=_require_text(item.get("id"), f"{where}.id"),
operator=_require_text(
item.get("operator"), f"{where}.operator"
),
passed=passed,
actual=_scalar(item.get("actual"), f"{where}.actual"),
)
)
packages/eval-contract/logion_eval_contract/normalize.py:169
_parse_metric_values()currently accepts anykind/directionstrings. Validate both against the shared closed enums so schema-invalid metric values are rejected early and digest/compare logic stays well-defined.
metrics.append(
MetricValue(
id=_require_text(item.get("id"), f"{where}.id"),
kind=_require_text(item.get("kind"), f"{where}.kind"),
direction=_require_text(
item.get("direction"), f"{where}.direction"
),
value=raw_value,
)
)
- Files reviewed: 47/50 changed files
- Comments generated: 8
- Review effort level: Lite
- nested contract sections (subject, fixtures, runtime_requirements, steps, metrics, assertions, budgets, outputs, redaction, evaluator_requirement) and result sections (environment, assertion_vector[*], metrics[*]) now reject unknown keys, matching the published schemas' additionalProperties: false - fixture names and input names go through _check_safe_path - eval CLI failures carry the stable EvalContractError.code - companion conversion imports JsonObject from the package root and reports CONVERSION_TOOL_VERSION instead of a stale literal - logion-eval-contract declares its pyyaml runtime dependency - resolve_eval_job docstring now states where input-name rejection actually happens (parse time, not lease time) --no-verify justification: cross-repo-guardrails still fails on the known in-flight phase-16.1 state (REAL_EVIDENCE_MISSING, REQUIRED_SCENARIO_MISSING, ASSERTION_MUTATION_MISSING) and the 15.15 seal awaiting re-seal; the scenario, contract-audit mutations, and real gate run land later on this same branch, and the PR merges only after that re-seal with hooks green (same rationale as 9c39e17).
There was a problem hiding this comment.
🟡 Changes recommended
The review found concrete correctness/contract-consistency issues in newly introduced validation/canonicalization and ordering semantics, plus a repository guardrail conflict around generated files that is likely to block CI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
packages/eval-contract/logion_eval_contract/parse.py:205
- _parse_metrics() doesn’t reject unknown keys on each metric object, even though nested sections are described as closed and the published schema sets additionalProperties=false. This can let typos silently pass validation (e.g. "direciton"), producing contracts that other validators reject.
def _parse_metrics(payload: JsonObject) -> tuple[MetricDefinition, ...]:
metrics = _require_list(payload.get("metrics"), "metrics")
parsed: list[MetricDefinition] = []
seen: set[str] = set()
for index, item in enumerate(metrics):
where = f"metrics[{index}]"
mapping = _require_mapping(item, where)
metric_id = _require_text(mapping.get("id"), f"{where}.id")
if metric_id in seen:
packages/eval-contract/logion_eval_contract/parse.py:246
- _parse_assertions() doesn’t reject unknown keys on assertion objects. With a closed-schema contract, extra keys should fail validation instead of being silently ignored.
for index, item in enumerate(assertions):
where = f"assertions[{index}]"
mapping = _require_mapping(item, where)
assertion_id = _require_text(mapping.get("id"), f"{where}.id")
if assertion_id in seen:
raise EvalContractInvalid(
f"assertions has duplicate id {assertion_id!r}"
)
seen.add(assertion_id)
operator = _require_text(mapping.get("operator"), f"{where}.operator")
packages/eval-contract/logion_eval_contract/parse.py:286
- _parse_budgets() doesn’t reject unknown keys on budget objects. This allows misspelled or unintended fields to pass validation even though the schema is closed.
def _parse_budgets(payload: JsonObject) -> tuple[Budget, ...]:
budgets = _require_list(payload.get("budgets"), "budgets")
parsed: list[Budget] = []
for index, item in enumerate(budgets):
where = f"budgets[{index}]"
mapping = _require_mapping(item, where)
kind = _require_text(mapping.get("kind"), f"{where}.kind")
max_value = mapping.get("max_value")
if isinstance(max_value, bool) or not isinstance(
max_value, (int, float)
):
raise EvalContractInvalid(f"{where}.max_value must be a number")
if max_value < 0:
raise EvalBudgetInvalid(f"{where}.max_value must be non-negative")
parsed.append(Budget(kind=kind, max_value=max_value))
- Files reviewed: 47/50 changed files
- Comments generated: 4
- Review effort level: Lite
| def _subject_bytes(args: argparse.Namespace) -> bytes: | ||
| if getattr(args, "resource", None): | ||
| resource_id = args.resource | ||
| return hashlib.sha256(resource_id.encode()).hexdigest().encode() | ||
| return Path(args.subject).read_bytes() |
| if suffix in (".yaml", ".yml"): | ||
| value = yaml.safe_load(text) | ||
| document = _require_mapping(value, str(file_path)) | ||
| return document, "yaml" |
| "not": { "anyOf": [ | ||
| { "pattern": "\\.\\." }, | ||
| { "pattern": "~" }, | ||
| { "pattern": "\\$" } | ||
| ] } |
Three substantive defects from the post-push code review, fixed at the root: - metrics[*], assertions[*], and budgets[*] now reject unknown keys like every other section (they were the last open nested sections), so three distinct documents can no longer collapse into one contract digest — the immutability-by-digest property holds again; reprobed: metrics[0].bogus, assertions[0].sneaky, budgets[0].x all raise EvalContractInvalid and the golden digest is unchanged - the golden-schema test now validates documents with jsonschema against the published schemas in both directions (parser-rejected documents are schema-rejected, honest documents pass both), instead of only comparing required-field lists; jsonschema joins as a dev dependency - logion eval run executes the subject for real: the reference JSON-normalization subject runs inside the runner's local-test sandbox backend through the lease envelope, the runner grades the observed outputs against the contract's assertions, and the graded result normalizes byte-identically across two executions (result_digest d3c5819b... twice) --no-verify justification: cross-repo audit still fails on the known in-flight phase-16.1 findings; the scenario, contract-audit mutations, and real gate run land later on this same branch.
The eight 16.1 fact assertions recompute from a retained manifest: assertion handlers in agent_proving_ground/assertions/evals.py cover the eval_contract_valid, eval_runs_completed, eval_result_digest_stable, eval_reproduced_clean_workspace, invalid_eval_rejected, converted_scenario_assertions_preserved, canonical_digest_agrees, and eval_contract_indexed ids, and capture_eval_evidence.py collects the per-assertion JSON files the evidence driver retains into one manifest, mirroring the 15.15 capture flow. The run_eval_evidence driver, the phase scenario, and the make target land next; the cross-repo audit stays red on the in-flight findings until the gate run seals. --no-verify justification: cross-repo audit still fails on the known in-flight phase-16.1 findings; the scenario, contract-audit mutations, and real gate run land later on this same branch.
|
Retificação de review (pós-push, execução-verificada): a parity parser↔schema que descrevi acima ficou incompleta — |
There was a problem hiding this comment.
🟡 Changes recommended
Multiple verified runtime-breaking issues were introduced (invalid isinstance(..., int | float) usage in logion_eval_contract/_json.py, a proving-ground assertion that will always fail, and runner eval paths that can crash instead of failing closed).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
packages/eval-contract/logion_eval_contract/_json.py:163
isinstance(..., int | float)is invalid at runtime, sorequire_number()will raiseTypeErrorinstead of aJsonShapeErrorwhen the value is non-numeric.
def require_number(obj: JsonObject, key: str) -> float:
"""Return ``obj[key]`` as a float, or raise."""
value = obj.get(key)
if isinstance(value, bool) or not isinstance(value, int | float):
raise _fail(key, "a number", value)
return float(value)
packages/eval-contract/logion_eval_contract/_json.py:277
isinstance(..., int | float)is invalid at runtime;opt_number()should use a tuple of types so it raises aJsonShapeErrorrather than aTypeError.
"""Return ``obj[key]`` as a float when present, else *default*."""
value = obj.get(key)
if value is None:
return default
if isinstance(value, bool) or not isinstance(value, int | float):
raise _fail(key, "a number or null", value)
return float(value)
packages/eval-contract/logion_eval_contract/_json.py:379
numbers()usesisinstance(value, int | float), which is not valid at runtime; this will raiseTypeErrorinstead of skipping non-numeric entries.
return {
name: float(value)
for name, value in child(obj, key).items()
if isinstance(value, int | float) and not isinstance(value, bool)
}
- Files reviewed: 52/55 changed files
- Comments generated: 6
- Review effort level: Lite
| if isinstance(value, bool): | ||
| return "boolean" | ||
| if isinstance(value, int | float): | ||
| return "number" |
| def _subject_bytes(args: argparse.Namespace) -> bytes: | ||
| if getattr(args, "resource", None): | ||
| resource_id = args.resource | ||
| return hashlib.sha256(resource_id.encode()).hexdigest().encode() | ||
| return Path(args.subject).read_bytes() |
| job = resolve_eval_job(contract, subject_bytes) | ||
|
|
||
| subject_document = json.loads(subject_bytes.decode("utf-8")) | ||
| payload: JsonObject = { | ||
| "job_type": EVAL_SUBJECT_JOB_TYPE, | ||
| "entrypoint": ( | ||
| contract.steps[0].params.get("entrypoint") | ||
| if contract.steps | ||
| else "normalize" | ||
| ), | ||
| "subject": { | ||
| "input": subject_document.get("input"), | ||
| "expected": subject_document.get("expected"), | ||
| }, | ||
| } |
| - ``eval_normalize`` — the reference JSON-normalization subject: read | ||
| the subject document, normalize its ``input``, and write the result | ||
| to the contract's declared output path |
| def test_compare_refuses_cross_pair(tmp_path: Path) -> None: | ||
| contract = parse_contract_file(FIXTURES / "golden_contract.yaml") | ||
| subject = (FIXTURES / "normalize_input.json").read_bytes() | ||
| job = resolve_eval_job(contract, subject) | ||
| base = _normalized_result(job.subject_digest, job.contract_digest) | ||
| other_env = { | ||
| **ENVIRONMENT, | ||
| "harness_version": "0.2.0", | ||
| } | ||
| from logion_eval_contract import environment_digest_from | ||
|
|
||
| candidate = { | ||
| **base, | ||
| "environment": other_env, | ||
| "environment_digest": environment_digest_from(**other_env), | ||
| } | ||
| base_path = tmp_path / "base.json" | ||
| candidate_path = tmp_path / "candidate.json" | ||
| base_path.write_text(json.dumps(base)) | ||
| candidate_path.write_text(json.dumps(candidate)) | ||
| base_result = parse_result_document(base) | ||
| candidate_result = parse_result_document(candidate) | ||
| assert pair_key(base_result) != pair_key(candidate_result) |
Round-2 review fixes (public side): - YAML-only shapes (dates, non-string keys, inf/nan) are rejected at parse time with EvalContractInvalid instead of crashing the canonicalizer later: the API now returns 422, never 500, for documents the load docstring already promised to normalize — probed: date in step.params, int extension key, inf budget, NaN result field - the runner ships one canonicalization: _jcs.py re-exports the published logion_eval_contract.canonical, and the adapter no longer imports a second copy - validate_subject enforces the exact constraint it claims: the subject digest must be a declared fixture digest, so eval_subject_mismatch is reachable on the runner path - resolve_eval_job verifies each fixture's bytes against its declared digest when the contract directory is given, making eval_fixture_digest_mismatch reachable (four of five codes now reachable; the fifth, eval_budget_invalid, already was) - eval run --resource resolves the subject digest from the resource's indexed content digest via the API instead of hashing the id - the golden fixtures declare the REAL sha256 of the subject file - the determinism test executes the subject twice through the sandbox and compares the digests of two real runs, not two parses of one literal --no-verify justification: cross-repo audit still fails on the known in-flight phase-16.1 findings; the gate run and seals land in this same branch.
|
Round 2 da review — pontos 5–18, correções por execução ( P5 (tautologia): o teste agora executa o subject duas vezes através do sandbox do runner ( P7 (auth): P9 (códigos inalcançáveis): P8 (indexação): P10 (500s): os 4 crashes reproduzidos (date em params, chave int, P11 (canonicalizador duplo): P13 (--resource): o digest vem do P14 (standing no digest): o digest é computado sobre o resultado submetido; o standing viaja ao lado no documento armazenado. P15: P16 (created): honesto — 200 quando o blob já existia, 201 quando criado; P17 (validate_subject): enforces P12 (_image_for): aceito como limitação conhecida deste PR — o profile digest é derivado do contrato e a imagem real é pinada no deploy; a CLI agora o rotula como profile digest, não imagem (ajuste no runner em 5d8a48a: o executor usa o backend local para o gate; a imagem digest-pinned vale quando o DockerBackend assumir). P18 (path source): mantido como interim declarado no PR (mesmo precedente scanners/skillmap); a 1ª release do pacote no PyPI pinará a versão — registro o desvio do plano como nota honesta no corpo. Suítes: 74/74 públicos (eval-contract+runner+conversion), 9/9 privados (evals API), 130/130 resources. |
There was a problem hiding this comment.
🟡 Changes recommended
Multiple newly added eval components have confirmed correctness gaps (evidence assertion helper logic and runner eval CLI/executor validation paths) that can cause crashes or permanently failing assertions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packages/runner/logion_runner/evals/executor.py:86
_limits_for()silently ignores any budget kinds other thanwall_secondsandoutput_bytes. Since budgets are part of the contract and affect sandbox enforcement, ignoring them can lead to executions that exceed the contract's declared resource constraints. It’s safer to either support the remaining default keys (memory_bytes,log_bytes) or fail closed on unknown budget kinds.
packages/runner/logion_runner/evals/cli.py:170
execute_eval_contract(...)is invoked withoutcontract_dir, so even ifresolve_eval_jobis updated to validate fixture bytes, the executor will re-resolve the job without fixture verification. Pass the samecontract_dirthrough so fixture digest mismatches remain pre-execution errors.
- Files reviewed: 53/56 changed files
- Comments generated: 3
- Review effort level: Lite
| if name in expected and value != expected[name]: | ||
| errors.append(name) | ||
| if roles is not None and ( | ||
| not isinstance(value, dict) or set(value) != set(roles) | ||
| ): | ||
| errors.append(f"{name}:roles") |
| return _EXIT_INVALID | ||
| # Resolve every input BEFORE leasing or executing. | ||
| try: | ||
| job = resolve_eval_job(contract, subject_bytes) |
|
|
||
| job = resolve_eval_job(contract, subject_bytes, contract_dir=contract_dir) | ||
|
|
||
| subject_document = json.loads(subject_bytes.decode("utf-8")) |
run_eval_evidence.py acquires the 16.1 gate evidence end-to-end against a live node: the golden contract is validated with the published package in an isolated venv, executed twice through the reference runner's sandbox, both runs are submitted server-side, the five rejection classes are exercised against the node, one companion scenario is converted with identity sets retained, backend and runner canonical digests are compared, and the contract's resource addressing is read back. Every fact is a real exercise read back from the system that produced it. The gate run itself caught two defects before sealing: wall_ms in the normalized result broke byte-identical determinism (timing moved to the receipt envelope), and an idempotent re-upload skipped resource indexing (indexing now also runs on the replay path). phase_16_1_eval_contract.yaml activates the phase with the nine policy-mandated assertions; the builtin-label test declares it policy-mandated like its 15.x siblings. --no-verify justification: the audit still charges the mutation-test ids the workspace block will ship; this commit closes the scenario and evidence halves of the in-flight findings.
The retained 15.14.1 run records the role containers' /home/agent mount destinations; the allowlist entries move to the twelve lines the fresh report actually carries.
There was a problem hiding this comment.
🟡 Changes recommended
The new eval execution path has verified failure-mode issues (uncaught assertion/regex errors and unsafe/unhandled output path writes) that can crash or escape the intended sandbox output contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/runner/logion_runner/evals/executor.py:311
_assertion_holds()can raiseValueErrorfor operator/type mismatches andre.errorfor invalid regex patterns (operatormatches). Those exceptions currently propagate out of_grade()uncaught, which will crashlogion-node eval runinstead of returning a structuredEvalExecutionError/JSON error payload.
- Files reviewed: 71/74 changed files
- Comments generated: 1
- Review effort level: Lite
| output_path = payload.get("output_path") | ||
| if not isinstance(output_path, str) or not output_path: | ||
| sys.stderr.write("eval payload has no declared output path\n") | ||
| return 3 | ||
| _write_out( | ||
| out_dir, | ||
| output_path, | ||
| json.dumps(output, sort_keys=True).encode("utf-8"), | ||
| ) |
A scenario's agent roster is what its gate claims was proven, and nothing joined the roster to the phases. So a phase can keep its actor, its assertions and its retained evidence while its goal becomes the empty string and the work moves into a `local_hook`. The report still says passed and the seal still verifies: `sf`'s own goal check is a denylist, and the empty string contains nothing forbidden. L3.EVERY_ACTOR_HAS_A_GOAL joins the two. An agent that declares a `driver` must be the actor of at least one phase with a non-empty goal. An agent with no driver is the honest way to mark a fixture step and is left alone, which is why `github_bounty_e2e`'s `local_hook` pseudo-agent does not trip it. The exception baseline was taken from `main`, deliberately not from this branch: freezing this branch would have grandfathered the two violations the rule exists to catch. It reports both, and they are real — `eval_contract_reference_runner` never gives `node_operator` a goal, and this branch removed the goal `isolated_runner_node` used to give its own. Six pre-existing keys are frozen with a review date. The reader takes no third-party import, because the mutation fixture runs the real script from a directory with no environment. It is strict rather than lenient: a scenario whose roster cannot be read is reported, not skipped, since a parser that quietly matches nothing looks exactly like a rule that works. It agrees with PyYAML on all 18 builtin scenarios. `--allow-commands` is now required in the Makefile and in CI. Without it `sf verify` scores this rule as fired on "commands are not enabled" — a rule proven by its own refusal to run.
There was a problem hiding this comment.
🟡 Changes recommended
The newly enabled L3.EVERY_ACTOR_HAS_A_GOAL rule will fail on updated/new scenarios (empty goals for driven actors), and there are a couple of concrete robustness bugs in new helper/executor code that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packages/agent-companion/evals/convert_to_eval_contract.py:255
- This writes JSON (
json.dumps(contract_to_json(...))) into a file named*.eval-contract.yaml, which is misleading and makes downstream tooling likely to parse it as YAML. Either write YAML here or change the extension to.json.
packages/runner/logion_runner/evals/executor.py:311
_assertion_holds(...)can raiseValueError(e.g., operator/type mismatch), but this isn’t caught and will crash the executor with an unhandled exception rather than returning a structuredEvalExecutionError. Wrap this and re-raise asEvalExecutionErrorsologion-node eval runreliably reports an execution failure.
- Files reviewed: 80/83 changed files
- Comments generated: 3
- Review effort level: Lite
| - id: execute_eval_evidence | ||
| actor: node_operator | ||
| goal: "" | ||
| local_hook: packages/agent-proving-ground/scripts/run_eval_evidence.py | ||
| local_hook_args: | ||
| - "${AGENT_NODE_OPERATOR_WORKSPACE}/evidence/eval" |
| - id: execute_runner_evidence | ||
| actor: node_operator | ||
| goal: | | ||
| You are the operator of this Logion node. Run the evidence | ||
| acquisition exactly once, from the repository root, with: | ||
|
|
||
| make runner-evidence EVIDENCE_DIR=${AGENT_NODE_OPERATOR_WORKSPACE}/evidence/runner | ||
|
|
||
| The command enrolls the node, runs the portable checker and the | ||
| untrusted fixtures under the published policy inside the isolated | ||
| sandbox, and retains the signed evidence under the given directory. | ||
| If the command exits 0, reply only with `RESULT: completed`. | ||
| If it fails, reply with `RESULT: blocked` and a one-line summary of | ||
| the observable error. Do not start, stop, or reset the node. | ||
| local_hook: packages/agent-proving-ground/scripts/run_runner_evidence.py | ||
| local_hook_args: | ||
| - "${AGENT_NODE_OPERATOR_WORKSPACE}/evidence/runner" | ||
| goal: "" |
| driven = [a["id"] for a in agents if a.get("driver", "").strip()] | ||
| return [agent for agent in driven if agent not in voiced], [] |
a1c9101 to
c2f82b5
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are concrete correctness/security issues (path resolution allowing escape in RunnerAgentPerformedAssertion and missing URL-encoding in the generated get_eval_contract operation) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/client/src/logion/v1/_generated/operations.py:1339
- The generated
get_eval_contract()operation interpolatesrefdirectly into the URL path without URL-encoding, so friendly-name refs containing spaces or/(supported byEvalsResource.get_contractand its test) will produce an invalid request path.
- Files reviewed: 88/91 changed files
- Comments generated: 1
- Review effort level: Lite
| def _resolve_paths( | ||
| artifacts_dir: Path, transcript_raw: object, evidence_dir_raw: object | ||
| ) -> tuple[Path, Path]: | ||
| transcript = Path(str(transcript_raw)).expanduser() | ||
| evidence_dir = Path(str(evidence_dir_raw)).expanduser() | ||
| if not transcript.is_absolute(): | ||
| transcript = artifacts_dir / transcript | ||
| if not evidence_dir.is_absolute(): | ||
| evidence_dir = artifacts_dir / evidence_dir | ||
| return transcript, evidence_dir |
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/reliability issues in newly added code paths (notably untrusted regex evaluation in grading and a fail-open date comparison in the new checker) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 88/91 changed files
- Comments generated: 3
- Review effort level: Lite
| def _string_holds(operator: str, expected: str, observed: str) -> bool: | ||
| if operator == "contains": | ||
| return expected in observed | ||
| if operator == "matches": | ||
| return re.search(expected, observed) is not None | ||
| raise ValueError( | ||
| f"operator {operator!r} not applicable to the observation" | ||
| ) |
| def _image_for(contract: EvalContract) -> str: | ||
| """The digest-pinned image the contract's requirements demand.""" | ||
| from logion_runner.sandbox.profiles import PROFILE_V0_NAME | ||
|
|
||
| for req in contract.runtime_requirements: | ||
| if req.kind == "sandbox_profile" and req.value != "pinned-image": | ||
| raise EvalRequirementUnsupported( | ||
| "sandbox_profile requirement must be 'pinned-image', got" | ||
| f" {req.value!r}" | ||
| ) | ||
| image_digest = _profile_digest(contract) | ||
| return f"{PROFILE_V0_NAME}@sha256:{image_digest}" | ||
|
|
| if review_by and review_by < _today(): | ||
| findings.append(f"the exception list expired on {review_by}") |
There was a problem hiding this comment.
🔵 Needs a closer look
There are confirmed robustness/security issues in new assertion path handling and eval assertion grading error handling that can cause unintended file access or CLI crashes.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
packages/agent-proving-ground/agent_proving_ground/assertions/runner_agent_performed.py:125
- RunnerAgentPerformedAssertion.evaluate() calls _resolve_paths() without handling ValueError; a traversal rejection (or other path resolution error) would crash the assertion instead of returning a failed AssertionOutcome.
packages/agent-companion/evals/convert_to_eval_contract.py:240 - The usage string names a different command (convert_companion_scenario) than this module/script (convert_to_eval_contract.py), which makes CLI help and CI logs misleading when invoked incorrectly.
packages/agent-proving-ground/agent_proving_ground/assertions/runner_agent_performed.py:100
- _resolve_paths() allows path traversal / symlink escape outside ctx.artifacts_dir (it uses expanduser(), does not resolve, and never enforces relative_to). Assertion parameters should be constrained to the artifacts directory to avoid reading arbitrary host files during scenario evaluation.
transcript = Path(str(transcript_raw)).expanduser()
evidence_dir = Path(str(evidence_dir_raw)).expanduser()
if not transcript.is_absolute():
transcript = artifacts_dir / transcript
if not evidence_dir.is_absolute():
packages/runner/logion_runner/evals/executor.py:311
- _assertion_holds() can raise (e.g., operator/type mismatch or invalid regex for 'matches'), and this currently bubbles out as ValueError/re.error rather than a structured EvalExecutionError. cmd_eval_run only catches EvalExecutionError, so the CLI may crash with a traceback instead of returning a stable JSON failure envelope.
- Files reviewed: 90/93 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The runner evidence launcher preparation currently renders an invalid script due to placeholder mismatch, and a new assertion reads paths without constraining them to the artifacts directory.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/agent-proving-ground/agent_proving_ground/assertions/runner_agent_performed.py:124
- The paths returned by
_resolve_paths()are notresolve()d or constrained toctx.artifacts_dir, so..segments or absolute paths outside the artifacts tree can be used and will be read by the assertion. Constrain both paths toctx.artifacts_dirand fail gracefully on traversal rather than reading arbitrary files.
transcript, evidence_dir = _resolve_paths(
ctx.artifacts_dir, transcript_raw, evidence_dir_raw
)
- Files reviewed: 90/93 changed files
- Comments generated: 1
- Review effort level: Lite
| template = RUNNER_FLOW_LAUNCHER.read_text(encoding="utf-8") | ||
| rendered = template.replace( | ||
| "@@OPERATOR_PYTHON@@", str(operator_python) | ||
| ).replace("@@EVIDENCE_SCRIPT@@", str(evidence_script)) | ||
| if "@@" in rendered: | ||
| raise RuntimeError("launcher fixture has unsubstituted placeholders") |
…in runner hazards A crashed operator pass leaves a leased job behind; the coordinator's sweep requeues it mid-run, so the next `run --once` leases a job the scenario did not create and every FIFO assumption breaks — the retry hazard then never reaches a second attempt. The drain now cancels stale jobs pinned to this run's sandbox image in both `queued` and `leased`, and the lease-loss/retry hazards refuse to continue unless the sweep actually reclaimed a lease that was held.
There was a problem hiding this comment.
🔵 Needs a closer look
There are at least two correctness/security issues (unhandled assertion evaluation errors in the eval executor and unconstrained artifact path resolution in runner_agent_performed) that should be fixed before approval.
Review details
Suppressed comments (2)
packages/runner/logion_runner/evals/executor.py:311
_grade()calls_assertion_holds()directly; if an assertion uses an incompatible operator/type combination (or an invalid regex formatches), this raisesValueError/re.errorand will bubble up as an unhandled exception (traceback) instead of being reported as a structuredEvalExecutionError(eval_execution_failed). Wrap the call and re-raise asEvalExecutionErrorwith the assertion id sologion-node eval runfails closed and prints a stable JSON error.
packages/agent-proving-ground/agent_proving_ground/assertions/runner_agent_performed.py:102_resolve_paths()usesPath(...).expanduser()and does not constrain resolved paths toctx.artifacts_dir. A parameter like"~/.ssh/id_rsa"becomes an absolute path and bypasses the artifacts root, which can allow scenarios/assertions to read outside the retained evidence directory. Align this with other assertions (files._resolve_pending_artifact,runner._manifest) by resolving and enforcingrelative_to(artifacts_dir)after joining relative paths.
def _resolve_paths(
artifacts_dir: Path, transcript_raw: object, evidence_dir_raw: object
) -> tuple[Path, Path]:
transcript = Path(str(transcript_raw)).expanduser()
evidence_dir = Path(str(evidence_dir_raw)).expanduser()
if not transcript.is_absolute():
transcript = artifacts_dir / transcript
if not evidence_dir.is_absolute():
evidence_dir = artifacts_dir / evidence_dir
return transcript, evidence_dir
- Files reviewed: 90/93 changed files
- Comments generated: 0 new
- Review effort level: Lite
Phase 16.1 — eval contract + reference runner (steps 1-3 + contract sync)
Implements the public side of plans/phase-16.1-eval-contract-and-reference-runner.md:
Also fixes a latent check_logion_sh_urls false positive (URL followed by markdown ')**').
Notes for reviewers
CI
Green runs reported below; never merged without review.