Validate discovery and rubric attribution - #1248
burtenshaw wants to merge 10 commits into
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
Alignment Review Report
Automated Checks
- Lint: PASS (this PR's files) —
ruff format --check,ruff check, andusort checkare all clean on the 15 changed.pyfiles. The lint hook's repo-wide failures (56ruff formatfiles, 2usortfiles) are pre-existing drift confined toenvs/*andtests/envs/{test_grid_world,test_julia_env}.py— none are touched by this PR. - Debug code: CLEAN — no
print/breakpoint/pdb/TODO/FIXMEin the changed files. Thecheck-debug.shfindings are all in unrelated, pre-existing files (cli/commands/build.py,import_env.py,serve.py,core/harness/__init__.py, ...). - Tests: 578 passed, 34 skipped in
tests/test_validation/(skips are Docker/integration). This includes the new discovery/rubric/attribution graders, the collector opt-in, and the artifact bundle.
Open RFCs Context
- RFC 008 — Environment Auto-Validation (In Review): the RFC this PR implements ("PR6"). The doc is updated in lockstep with a new "Declaration and rubric evidence (PR6)" section — implementation and spec move together.
- RFC 004 — Rubric System (In Review): defines the rubric-introspection schema (
aggregationLiteral,config_available, per-nodescore/evaluated) that the new graders consume. - RFC 011 — ARD-backed catalog discovery (Draft): a downstream consumer of RFC 008's check contracts (see RFC Conflicts).
Tier 1: Fixes Required
None.
Tier 2: Alignment Discussion
Principle Conflicts
None identified. Notes on the areas most at risk:
- Rewards inside environment (INVARIANTS / RFC 002):
RubricIntrospectableGrader/RewardAttributionGraderonly verify the subject's own attribution — they recompute expected scores from the environment's self-reported rubric tree and compare against the emitted step reward. No external/augmenting reward computation is introduced. - Rubric vocabulary alignment: the grader's aggregation set
{weighted_sum, sequential, gate, leaf, unknown}matches RFC 004'saggregationLiteral andcore/env_server/session_telemetry.pyexactly, and the recomputation mirrorscore/rubrics/containers.py(Sequentialfail-fast on 0,Gatethreshold,WeightedSumweights sum to 1, tol 1e-6). Unknown/custom aggregation is marked incomplete (SKIP) rather than mis-scored. - No credential exposure (INVARIANTS): discovery rejects credential echoes at collection time (
contains_credential, incl. unicode-escaped), anddiscovery.jsonis redacted; the_TOKENregex gains a negative lookbehind so identifiers liketask_declaration_accuracyare preserved while standalone tokens still redact. Covered by tests. - Dual API boundary (INVARIANTS):
tools/listis issued by the infra validator over the authenticated validation replay socket (before reset) solely to check declaration accuracy; it does not exposereset/step/stateto an agent.
RFC Conflicts
One low-severity coordination note (non-blocking):
ALIGNMENT FLAG: This PR concretizes the runtime.tool_declaration_accuracy / runtime.task_declaration_accuracy / runtime.rubric_introspectable / runtime.reward_attribution check contracts and the new discovery.json evidence format.
- RFC at stake: RFC 011 (ARD-backed catalog discovery, Draft)
- The concern: RFC 011 explicitly consumes
runtime.tool_declaration_accuracyand treats RFC 008 check/report contracts as immutable inputs for its "validated interface" claim, and carries an open question — "Which immutable RFC 008 report/check contracts support validated interface claims" (RFC 011 open question 8). This PR is directly relevant: it moves these checks from PENDING/NOT_APPLICABLE to IMPLEMENTED and fixes theircheck_ids and evidence shape. There is no behavioral conflict — this is the RFC 008 side delivering what RFC 011 wants — but the RFC 011 consumer and its open question should be reconciled against the now-finalized contract names and thediscovery.jsonshape. - Suggested reviewers: @thegovind (RFC 011 author), @zkwentz (RFC 008 author)
Summary
- 0 mechanical issues to fix
- 1 alignment point for human review (RFC 011 <-> RFC 008 contract coordination; informational)
- 0 blocking RFC conflicts
Overall this is a clean, tightly-scoped, and unusually well-tested change (22 new parametrized fault cases wired into the loopback/Docker acceptance manifests). Nice touches: bounded byte + shared-deadline budgets across discovery, trust_env=False + no-redirect + identity-encoding task sampling, and the invariant that unavailable/malformed discovery can never collapse into an empty passing set.
Sent by Cursor Automation: Pre-review
There was a problem hiding this comment.
Two issues need fixing before merge.
- Preserve the difference between an omitted declaration and an explicit empty declaration in
src/openenv/validation/graders/runtime/discovery.py:83and:131.
Both declared_tools and declared_task_count are optional in manifest-v2.schema.json, but their default factories turn omission into [] and {}. The grader then treats those defaults as claims of zero tools or tasks. At this head, omitting declared_tools from an otherwise valid manifest while discovering echo fails with “1 discovered tools are undeclared.” Setting task_api=true, omitting declared_task_count, and exposing a valid train split also fails.
model_fields_set retains the omitted-versus-explicit distinction. Please use it, or make these declarations required by the schema. An explicit empty collection should still mean zero; omission should remain unknown or not applicable as RFC 008 specifies.
- Resolve the Task API namespace from the server contract instead of using
manifest.nameinsrc/openenv/validation/runner.py:181andsrc/openenv/validation/runtime/discovery.py:30.
The documented route namespace comes from create_app(env_name=...) and is exposed by GET /list_environments. No manifest rule requires it to match the name in openenv.yaml. Existing environments already use different pairs, including grid_world with grid_world_env and textarena with textarena_env. This implementation queries a valid provider at the wrong path, receives a 404, and fails task_declaration_accuracy.
Please collect and bound /list_environments, use the advertised single environment name, and add a regression where the manifest and route names differ. An explicit schema field that binds and validates the two names would also work.
At 4a916533, the focused discovery, task, runtime, artifact, process, and CLI suite otherwise passed with 159 tests passing and 22 skipped. Ruff, Ruff format, usort, the debug scan, and git diff --check passed for all 17 changed Python files. CI is green at this head, including Linux Docker.
RFCs 004 and 008 are still in review, and draft RFC 011 consumes these contracts. Their owners should confirm the final declaration, rubric, and discovery evidence semantics.
|
Addressed both review findings in 8645817, now at 6f5ea57 after synchronizing PR5. Omitted tool/count declarations stay unknown through report serialization; explicit empty declarations still assert zero. Task discovery bounds All 12 CI checks pass at this head. Fresh HF Jobs and Linux Docker CI evidence was independently verified: 610 fast, 36 protocol and 26 Docker tests, zero failures or skips. The first HF attempt timed out downloading Gradio before tests; the same-commit retry completed successfully. Ready for re-review; RFC/core-owner signoff remains outstanding. |
|
Synchronized PR5's cleanup-verdict fix in 2327a60. Fresh HF Jobs evidence is independently verified: 625 fast and 36 protocol tests, zero failures or skips. Linux Docker CI passed all 26 Docker cases; its artifacts and exact source hashes are independently verified. All 12 CI checks are green at this head. The formal stack remains synchronized. |


This PR checks tool and task declarations, rubric configuration and per-step reward attribution. Part 6 of #1177; builds on #1247.