Skip to content

Validate discovery and rubric attribution - #1248

Open
burtenshaw wants to merge 10 commits into
ben/rfc008-l2-05-repeatabilityfrom
ben/rfc008-l2-06-discovery
Open

burtenshaw wants to merge 10 commits into
ben/rfc008-l2-05-repeatabilityfrom
ben/rfc008-l2-06-discovery

Conversation

@burtenshaw

Copy link
Copy Markdown
Collaborator

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

@burtenshaw
burtenshaw added this pull request to stack #1183 September 24, 2026 10:27
@burtenshaw burtenshaw added feature size: extra-large Extra-large pull request labels Sep 24, 2026 — with Cursor
@bot-ci-comment

Copy link
Copy Markdown

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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alignment Review Report

Automated Checks

  • Lint: PASS (this PR's files) — ruff format --check, ruff check, and usort check are all clean on the 15 changed .py files. The lint hook's repo-wide failures (56 ruff format files, 2 usort files) are pre-existing drift confined to envs/* and tests/envs/{test_grid_world,test_julia_env}.py — none are touched by this PR.
  • Debug code: CLEAN — no print / breakpoint / pdb / TODO / FIXME in the changed files. The check-debug.sh findings 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 (aggregation Literal, config_available, per-node score/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 / RewardAttributionGrader only 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's aggregation Literal and core/env_server/session_telemetry.py exactly, and the recomputation mirrors core/rubrics/containers.py (Sequential fail-fast on 0, Gate threshold, WeightedSum weights 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), and discovery.json is redacted; the _TOKEN regex gains a negative lookbehind so identifiers like task_declaration_accuracy are preserved while standalone tokens still redact. Covered by tests.
  • Dual API boundary (INVARIANTS): tools/list is issued by the infra validator over the authenticated validation replay socket (before reset) solely to check declaration accuracy; it does not expose reset/step/state to 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_accuracy and 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 their check_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 the discovery.json shape.
  • 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.

Open in Web View Automation 

Sent by Cursor Automation: Pre-review

Comment thread src/openenv/validation/graders/runtime/discovery.py

@k21993 k21993 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues need fixing before merge.

  1. Preserve the difference between an omitted declaration and an explicit empty declaration in src/openenv/validation/graders/runtime/discovery.py:83 and :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.

  1. Resolve the Task API namespace from the server contract instead of using manifest.name in src/openenv/validation/runner.py:181 and src/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.

@cursor cursor Bot mentioned this pull request Sep 25, 2026
16 tasks
@burtenshaw

burtenshaw commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator Author

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 /list_environments and uses its advertised namespace, with an installed-process regression for differing package and server names.

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.

@burtenshaw

Copy link
Copy Markdown
Collaborator Author

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 branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature size: extra-large Extra-large pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants