RFC 008: local and remote validation architecture - #1
Conversation
📝 WalkthroughWalkthroughThe change adds RFC 008 shared validation contracts, Harbor task support, local and remote execution, dedicated sandbox security, strict publish gating, portable validation reports, CLI updates, documentation, templates, and extensive tests. ChangesValidation and publish workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant ValidateCLI
participant ValidationPlanner
participant HFSandboxProvider
participant ValidationReport
Developer->>ValidateCLI: run publish validation
ValidateCLI->>ValidationPlanner: build shared validation plan
ValidateCLI->>HFSandboxProvider: upload revision and validator archives
HFSandboxProvider-->>ValidateCLI: return validation report
ValidateCLI->>ValidationReport: verify report and source digest
ValidationReport-->>Developer: render publish gate result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
c645b99 to
e160992
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openenv/cli/commands/push.py`:
- Around line 355-362: Update _validate_copy_source to apply
validation_source_path_allowed before rejecting symlinks, so candidates under
excluded trees such as .venv or node_modules are skipped. Only raise
typer.BadParameter for symlinks on paths allowed for staging or hashing,
preserving the existing error message and relative-path handling.
In `@src/openenv/validation/local.py`:
- Around line 507-531: Narrow the try/except around _launch_environment so only
environment startup failures are caught and converted to runtime_error. Move the
nested _run invocation outside that exception handler while preserving the
launched runtime_url path, ensuring exceptions from build_validation_plan or
execute_validation_plan propagate without triggering a second _run execution.
- Around line 571-583: Update the remediation rendering loop to use the redacted
safe_remediation values for the command and documentation branches: read the
command from safe_remediation when calling shlex.join and the URL from
safe_remediation when rendering Docs. Keep the existing path handling and output
structure unchanged.
In `@src/openenv/validation/specs/harbor.py`:
- Around line 64-70: Update _valid_name to use re.fullmatch with the existing
task-name pattern instead of re.match, preserving the existing ".." rejection
and error behavior so trailing-newline or multiline values cannot pass
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a0e41b6-47f1-4b7f-9a07-350b8b0d1dab
📒 Files selected for processing (51)
.claude/docs/REPO_WALKTHROUGH.md.claude/skills/generate-openenv-env/SKILL.md.claude/skills/generate-openenv-env/assets/openenv_env_template/README.md.claude/skills/generate-openenv-env/assets/openenv_env_template/task.toml.claude/skills/generate-openenv-env/references/env-generation-checklist.mdREADME.mddocs/source/getting_started/contributing-envs.mddocs/source/getting_started/environment-builder.mddocs/source/guides/runtime-providers.mddocs/source/reference/cli.mddocs/source/reference/core.mdenvs/echo_env/task.tomlpyproject.tomlrfcs/008-local-remote-validation.mdrfcs/010-echo-env-token-world-model.mdrfcs/README.mdsrc/openenv/cli/_validation.pysrc/openenv/cli/commands/push.pysrc/openenv/cli/commands/validate.pysrc/openenv/cli/templates/openenv_env/README.mdsrc/openenv/cli/templates/openenv_env/task.tomlsrc/openenv/core/containers/runtime/hf_sandbox_provider.pysrc/openenv/validation/__init__.pysrc/openenv/validation/checks.pysrc/openenv/validation/executor.pysrc/openenv/validation/local.pysrc/openenv/validation/models.pysrc/openenv/validation/planner.pysrc/openenv/validation/remote.pysrc/openenv/validation/runtime_probe.pysrc/openenv/validation/security.pysrc/openenv/validation/serialization.pysrc/openenv/validation/source_inspection.pysrc/openenv/validation/specs/__init__.pysrc/openenv/validation/specs/base.pysrc/openenv/validation/specs/harbor.pysrc/openenv/validation/specs/openenv.pytests/test_cli/test_init.pytests/test_cli/test_push.pytests/test_cli/test_validate.pytests/test_core/test_hf_sandbox_provider.pytests/test_validation/__init__.pytests/test_validation/_helpers.pytests/test_validation/test_executor.pytests/test_validation/test_guidance.pytests/test_validation/test_harbor_spec.pytests/test_validation/test_local.pytests/test_validation/test_planner.pytests/test_validation/test_remote.pytests/test_validation/test_security.pytests/test_validation/test_spec_registry.py
| try: | ||
| with _launch_environment(source_path, timeout_s) as launched_url: | ||
| return _run( | ||
| target, | ||
| profile=selected_profile, | ||
| source_path=source_path, | ||
| spec_load=spec_load, | ||
| policy=policy, | ||
| runtime_url=launched_url, | ||
| runtime_error=None, | ||
| runtime_unavailable_reason=None, | ||
| timeout_s=timeout_s, | ||
| ) | ||
| except Exception as exc: | ||
| return _run( | ||
| target, | ||
| profile=selected_profile, | ||
| source_path=source_path, | ||
| spec_load=spec_load, | ||
| policy=policy, | ||
| runtime_url=None, | ||
| runtime_error=type(exc).__name__, | ||
| runtime_unavailable_reason=None, | ||
| timeout_s=timeout_s, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Narrow the try/except to cover only environment launch, not the nested validation run.
The current except Exception wraps both _launch_environment and the nested _run(...) call. Any exception from build_validation_plan/execute_validation_plan inside the with block (unrelated to server startup) gets silently reclassified as a "runtime probe error" and triggers a second full re-execution of _run without a running server — masking the real cause and doubling work.
♻️ Proposed refactor to separate launch failures from execution failures
- try:
- with _launch_environment(source_path, timeout_s) as launched_url:
- return _run(
- target,
- profile=selected_profile,
- source_path=source_path,
- spec_load=spec_load,
- policy=policy,
- runtime_url=launched_url,
- runtime_error=None,
- runtime_unavailable_reason=None,
- timeout_s=timeout_s,
- )
- except Exception as exc:
- return _run(
- target,
- profile=selected_profile,
- source_path=source_path,
- spec_load=spec_load,
- policy=policy,
- runtime_url=None,
- runtime_error=type(exc).__name__,
- runtime_unavailable_reason=None,
- timeout_s=timeout_s,
- )
+ launch_cm = _launch_environment(source_path, timeout_s)
+ try:
+ launched_url = launch_cm.__enter__()
+ except Exception as exc:
+ return _run(
+ target,
+ profile=selected_profile,
+ source_path=source_path,
+ spec_load=spec_load,
+ policy=policy,
+ runtime_url=None,
+ runtime_error=type(exc).__name__,
+ runtime_unavailable_reason=None,
+ timeout_s=timeout_s,
+ )
+ try:
+ return _run(
+ target,
+ profile=selected_profile,
+ source_path=source_path,
+ spec_load=spec_load,
+ policy=policy,
+ runtime_url=launched_url,
+ runtime_error=None,
+ runtime_unavailable_reason=None,
+ timeout_s=timeout_s,
+ )
+ finally:
+ launch_cm.__exit__(None, None, None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openenv/validation/local.py` around lines 507 - 531, Narrow the
try/except around _launch_environment so only environment startup failures are
caught and converted to runtime_error. Move the nested _run invocation outside
that exception handler while preserving the launched runtime_url path, ensuring
exceptions from build_validation_plan or execute_validation_plan propagate
without triggering a second _run execution.
| for remediation in result.remediation: | ||
| safe_remediation = remediation.to_dict() | ||
| lines.append(f" Fix: {safe_remediation['message']}") | ||
| if remediation.argv: | ||
| lines.append(f" $ {shlex.join(remediation.argv)}") | ||
| elif remediation.path is not None: | ||
| destination = remediation.path | ||
| if remediation.pointer is not None: | ||
| destination += remediation.pointer | ||
| lines.append(f" Edit: {destination}") | ||
| elif remediation.url is not None: | ||
| lines.append(f" Docs: {remediation.url}") | ||
| if verbose and result.evidence: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## local.py around format_shared_validation_report\n'
sed -n '540,620p' src/openenv/validation/local.py
printf '\n## models.py around ValidationRemediation\n'
sed -n '1,260p' src/openenv/validation/models.pyRepository: zkwentz/OpenEnv
Length of output: 11746
Use safe_remediation for the command and docs branches. to_dict() already redacts argv and url, but this renderer still prints the raw values, which can leak sensitive remediation data into CLI output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openenv/validation/local.py` around lines 571 - 583, Update the
remediation rendering loop to use the redacted safe_remediation values for the
command and documentation branches: read the command from safe_remediation when
calling shlex.join and the URL from safe_remediation when rendering Docs. Keep
the existing path handling and output structure unchanged.
| @field_validator("name") | ||
| @classmethod | ||
| def _valid_name(cls, value: str) -> str: | ||
| pattern = r"^[A-Za-z0-9_-][A-Za-z0-9._-]*/[A-Za-z0-9_-][A-Za-z0-9._-]*$" | ||
| if not re.match(pattern, value) or ".." in value: | ||
| raise ValueError("task name must use the org/name format") | ||
| return value |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Anchor the Harbor task-name check with fullmatch. re.match(..., "$") still accepts "org/name\n" because $ can match before a trailing newline. Use re.fullmatch(...) here so malformed multiline TOML values can’t pass the identity check.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 67-67: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.match(pattern, value)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openenv/validation/specs/harbor.py` around lines 64 - 70, Update
_valid_name to use re.fullmatch with the existing task-name pattern instead of
re.match, preserving the existing ".." rejection and error behavior so
trailing-newline or multiline values cannot pass validation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openenv/cli/commands/push.py (1)
273-277: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire
nameto be a non-empty string.Line 273 accepts truthy non-strings; e.g. a mapping can reach
env_name.replace(...)during staging and crash before the publish gate.Proposed fix
env_name = manifest.get("name") - if not env_name: - raise typer.BadParameter("openenv.yaml must contain a 'name' field") + if not isinstance(env_name, str) or not env_name.strip(): + raise typer.BadParameter( + "openenv.yaml must contain a non-empty string 'name' field" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openenv/cli/commands/push.py` around lines 273 - 277, Update the manifest validation around env_name in the push command to require a non-empty string, rejecting truthy non-string values before returning or staging. Preserve the existing BadParameter message and return behavior for valid names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/openenv/cli/commands/push.py`:
- Around line 273-277: Update the manifest validation around env_name in the
push command to require a non-empty string, rejecting truthy non-string values
before returning or staging. Preserve the existing BadParameter message and
return behavior for valid names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7491f9e6-469d-4a22-b56a-0fdfb3586ead
📒 Files selected for processing (11)
.claude/skills/generate-openenv-env/assets/openenv_env_template/README.mdREADME.mddocs/source/getting_started/contributing-envs.mddocs/source/getting_started/environment-builder.mddocs/source/reference/cli.mdsrc/openenv/cli/commands/push.pysrc/openenv/cli/commands/validate.pysrc/openenv/cli/templates/openenv_env/README.mdsrc/openenv/validation/__init__.pysrc/openenv/validation/remote.pytests/test_cli/test_push.py
🚧 Files skipped from review as they are similar to previous changes (9)
- src/openenv/validation/init.py
- README.md
- docs/source/getting_started/contributing-envs.md
- docs/source/reference/cli.md
- docs/source/getting_started/environment-builder.md
- src/openenv/cli/templates/openenv_env/README.md
- src/openenv/cli/commands/validate.py
- .claude/skills/generate-openenv-env/assets/openenv_env_template/README.md
- src/openenv/validation/remote.py
| ) | ||
|
|
||
| provenance = _provenance(root_path, path, model.schema_version, document_digest) | ||
| if model.schema_version != "1.1": |
There was a problem hiding this comment.
The exact schema_version == "1.1" pin here couples the adapter to one snapshot of the Harbor schema. Suggest validating by major version and treating unknown minors/fields as advisory rather than a hard fail.
| raise ValueError("report criterion metadata does not match the policy") | ||
|
|
||
|
|
||
| def run_remote_validation( |
There was a problem hiding this comment.
run_local_validation accepts spec_registry and policy, but run_remote_validation doesn't, so remote validation is pinned to the built-in openenv spec. Can the remote path accept a custom registry + policy too? Otherwise we can only validate a custom task.md adapter locally, not in an HF Sandbox.
| effective_runtime_url = _normalize_runtime_url(effective_runtime_url) | ||
|
|
||
| if source_path is None: | ||
| if spec_id not in {None, "openenv"} or spec_registry is not None: |
There was a problem hiding this comment.
The URL/runtime entrypoint rejects a custom registry. We can work around it by exposing our task as a standard OpenEnv server, so it's not blocking. But we want to confirm that the split (custom adapter for the static path, stock spec for the runtime path) is acceptable, or whether you intend to unify them.
| process.wait(timeout=5.0) | ||
|
|
||
|
|
||
| def _server_command(env_path: Path, app: str, port: int) -> list[str]: |
There was a problem hiding this comment.
_server_command launches a single uvicorn app on one port, and _launch_environment yields one launched_url. Our served environments aren't a single app (the manifest declares multiple long-running services (auth/gmail/slack/gcal/... each on its own port) that the framework starts and gates the agent on via each service's /health.)
Is multi-service orchestration something the SERVED launch path could support, or is the expectation that an adapter collapses its environment behind a single OpenEnv-protocol endpoint? For us the latter is workable via our adaptor layer, just confirming the intended contract.
2 participants
What & Why
Environment authors need a fast local loop, an isolated remote option before publishing, actionable failure guidance, and a clear separation from richer independent Hub certification. This PR contains the complete RFC 008 implementation: spec-neutral planning/reporting, local and dedicated-Sandbox execution, strict staged push gating, a versioned unofficial author report, and the configuration/documentation needed to use it; future task-package dispatch remains tracked in huggingface/OpenEnv#898.
Risk & Review Guidance
src/openenv/validation/remote.py:242and:551for Sandbox/report trust boundaries; (2)src/openenv/cli/commands/push.py:342,:394, and:622for exact-snapshot validation and upload ordering; (3)src/openenv/validation/specs/base.py:490andplanner.py:227for future spec independenceThis fork PR is intentionally large because it is the requested single-PR integration view. Human review should normally use the 17 smaller upstream draft PRs from #942 through #958.
Decisions & Alternatives
ValidationSubjectbehind trusted, pinned adapters — rejected coupling the planner directly to Harbor; served OpenEnv is initial, while one-shot/external specs can be added for Spec-agnostic task validation: openenv validate-task for external task.md packages (PostTrain Arena / BenchFlow) huggingface/OpenEnv#898.SandboxPoolfor uploaded cross-user code.Verification
PYTHONPATH=src:envs .venv/bin/python -m pytest tests/ -q -m 'not integration'— 1,559 passed, 114 skipped, 31 external integration tests deselected locally.PYTHONPATH=src:envs .venv/bin/python -m pytest tests/test_cli/test_push.py tests/test_validation/test_remote.py -q— 70 passed after the final excluded-symlink refinement.echo_envstrict publish report — 20/22 criteria passed, 2 advisory skips, 16/16 blocking criteria passed, zero blocking skips,certified: false.Scope & Non-Goals
task.tomlvalues.tests/envs/test_grid_world.pyandtests/envs/test_julia_env.pyremain untouched.Breaking Changes / Migration Notes
openenv pushnow requires HF authentication capable of creating a dedicated Sandbox and a fully passing strict publish profile.task.toml; older environments need a truthful requirements envelope before publishing..openenv/and are explicitly non-certified.Review Structure
The complete implementation is available as review-sized upstream drafts #942–#958. This PR remains the requested one-piece integration view on the fork.