feat(experimentalist): isolate optimization with OpenShell and Harbor - #1008
Open
ryana wants to merge 16 commits into
Open
feat(experimentalist): isolate optimization with OpenShell and Harbor#1008ryana wants to merge 16 commits into
ryana wants to merge 16 commits into
Conversation
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
ryana
force-pushed
the
aire-823-experimentalist-openshell-harbor/rangilly
branch
from
July 31, 2026 02:21
cfbfa29 to
2b6c594
Compare
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
ryana
force-pushed
the
aire-823-experimentalist-openshell-harbor/rangilly
branch
from
July 31, 2026 02:26
3c76fd5 to
ff05339
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe plugin now prepares credential-free inputs on the trusted host, runs the Experimentalist in OpenShell, and evaluates candidates through an authenticated Harbor bridge with content-addressed datasets, bounded dependency sessions, trusted candidate execution, and secure artifact handling. ChangesOpenShell execution
Trusted Harbor bridge
Trusted candidate execution
Remote evaluator integration
Sequence Diagram(s)sequenceDiagram
participant CLI
participant OpenShellLauncher
participant OpenShellSandbox
participant HarborBridge
participant RemoteHarborEvaluator
CLI->>OpenShellLauncher: prepare and launch run
OpenShellLauncher->>HarborBridge: start authenticated bridge
OpenShellLauncher->>OpenShellSandbox: create sandbox and upload manifest
OpenShellSandbox->>RemoteHarborEvaluator: execute Experimentalist workflow
RemoteHarborEvaluator->>HarborBridge: submit evaluation and dependency commands
HarborBridge-->>RemoteHarborEvaluator: evaluation status and artifacts
RemoteHarborEvaluator-->>OpenShellSandbox: evaluation result
OpenShellSandbox-->>CLI: run summary
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py (2)
1-1: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winExpose Harbor command execution through the returned runtime
HarborDependencyContext.__aenter__returnsHarborDependencyRuntime, which does not implementexecute().use_dependency_runtime()therefore selects no executor, andGuardedShellTools.run()executes commands on the host instead of inside Harbor. Make the active returned runtime implementDependencyCommandExecutorwhile preserving theDependencyRuntimeAPI.🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py` at line 1, Update HarborDependencyRuntime and HarborDependencyContext.__aenter__ so the returned active runtime implements DependencyCommandExecutor while retaining the existing DependencyRuntime API. Ensure use_dependency_runtime() recognizes it as the command executor, allowing GuardedShellTools.run() to execute commands inside Harbor rather than on the host.
219-346: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winExpose
execute()fromHarborDependencyContext.__aenter__.
__aenter__returnsself._runtime, butexecute()exists only onHarborDependencyContext. Local Harbor commands therefore fail theDependencyCommandExecutorcheck and fall back to host execution instead of running in the Harbor container. Make the returned object exposeexecute().🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py` around lines 219 - 346, Update HarborDependencyContext.__aenter__ to return an object exposing the context’s execute() method, rather than returning self._runtime alone. Preserve the existing runtime startup and readiness checks, and ensure the returned object satisfies DependencyCommandExecutor so local commands execute inside the Harbor environment.
🧹 Nitpick comments (9)
plugins/nemo-experimentalist/tests/test_harbor_bridge_service.py (2)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the missing type hint and import
jsonnormally.
registeredhas no annotation;RegisteredEnvelopeis already imported.__import__("json")hides a plain module import.♻️ Proposed refactor
-def _request_parts(tmp_path: Path, registered) -> tuple[dict[str, str], dict[str, tuple[str, bytes, str]]]: +def _request_parts( + tmp_path: Path, registered: RegisteredEnvelope +) -> tuple[dict[str, str], dict[str, tuple[str, bytes, str]]]:+import json- data={"metadata": __import__("json").dumps(metadata)}, + data={"metadata": json.dumps(metadata)},As per coding guidelines: "In Python code, prefer concrete type hints over string-based type hints".
Also applies to: 228-228
🤖 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 `@plugins/nemo-experimentalist/tests/test_harbor_bridge_service.py` at line 55, Update _request_parts to annotate registered with the imported RegisteredEnvelope type, and replace any __import__("json") usage in the affected code with a normal top-level json import. Keep the existing behavior and return annotations unchanged.Source: Coding guidelines
283-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDependency-session and cancellation endpoints have no test coverage.
The tests cover evaluation submission, polling, and artifacts. They omit
POST /v1/dependencies,POST /v1/dependencies/{session_id}/exec,DELETE /v1/dependencies/{session_id}, andDELETE /v1/evaluations/{job_id}. The capability-token check atservice.pyLines 553-558 is untested, including the 403 path. Add cases for those routes.🤖 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 `@plugins/nemo-experimentalist/tests/test_harbor_bridge_service.py` around lines 283 - 294, Add tests alongside test_job_api_requires_authentication covering POST /v1/dependencies, POST /v1/dependencies/{session_id}/exec, DELETE /v1/dependencies/{session_id}, and DELETE /v1/evaluations/{job_id}. Verify successful behavior and authentication failures, including the capability-token validation and its 403 response, using the existing app factory, TestClient, and RecordingRunner fixtures.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.py (2)
197-201: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOne
mkdirexec per uploaded file adds avoidable container round trips.Each file triggers a separate
exec_as_rootcall. Compute the unique parent directories first, then create them in one command.♻️ Proposed refactor
- for source in candidate_files: - relative = source.relative_to(candidate_root).as_posix() - target = f"{REMOTE_PROJECT}/{relative}" - await self.exec_as_root(environment, command=f"mkdir -p {shlex.quote(str(Path(target).parent))}") - await environment.upload_file(source, target) + relatives = [source.relative_to(candidate_root).as_posix() for source in candidate_files] + parents = sorted({f"{REMOTE_PROJECT}/{relative}".rsplit("/", 1)[0] for relative in relatives}) + if parents: + quoted = " ".join(shlex.quote(parent) for parent in parents) + await self.exec_as_root(environment, command=f"mkdir -p {quoted}") + for source, relative in zip(candidate_files, relatives, strict=True): + await environment.upload_file(source, f"{REMOTE_PROJECT}/{relative}")🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.py` around lines 197 - 201, Update the upload flow around the candidate_files loop to collect unique parent directories for all target paths, then invoke exec_as_root once with a command that creates every directory. Keep each environment.upload_file call unchanged and ensure directory paths remain safely shell-quoted.
189-195: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
chmod -R a+rwx {REMOTE_ROOT}makes the venv and the uv binary world-writable.The later
chmod 0755 {REMOTE_UV}andchmod -R a+rX {REMOTE_PROJECT}restore only two paths.{REMOTE_VENV},{REMOTE_PYTHON_INSTALLS}, and{REMOTE_UV_CACHE}stay mode 0777, so candidate code can replace the interpreter that the adapter executes. Grant write access to the agent user only, for example withchown, or restrict the mode toa+rXplus targeted writable directories.🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.py` around lines 189 - 195, The setup command in the trusted-agent environment initialization currently makes REMOTE_ROOT recursively world-writable, including REMOTE_VENV, REMOTE_PYTHON_INSTALLS, and REMOTE_UV_CACHE. Update the command passed to exec_as_root so agent-required writes use ownership or targeted directory permissions, while preserving read/execute access and ensuring the interpreter and uv binary cannot be replaced by candidate code.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.py (1)
451-461: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShield the cancellation cleanup request.
After
asyncio.CancelledError, the task is marked cancelled. The nextawaiton the new client can raiseCancelledErroragain, so the job cancellation DELETE may never reach the bridge. This leaks a running remote evaluation. Wrap the cleanup inasyncio.shieldwith a bounded timeout.♻️ Proposed change
except asyncio.CancelledError: if job_id is not None: - async with httpx.AsyncClient( - timeout=options.request_timeout_sec, - transport=self._transport, - ) as client: - await client.delete( - f"{str(options.bridge_url).rstrip('/')}/v1/evaluations/{job_id}", - headers=headers, - ) + async def _cancel_remote_job() -> None: + async with httpx.AsyncClient( + timeout=options.request_timeout_sec, + transport=self._transport, + ) as client: + await client.delete( + f"{str(options.bridge_url).rstrip('/')}/v1/evaluations/{job_id}", + headers=headers, + ) + + with contextlib.suppress(Exception): + await asyncio.shield(asyncio.ensure_future(_cancel_remote_job())) raise🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.py` around lines 451 - 461, Update the asyncio.CancelledError cleanup in the evaluation flow to shield the job-cancellation DELETE request from task cancellation and enforce a bounded timeout using the existing request-timeout configuration. Preserve the conditional job_id check and re-raise the original cancellation after cleanup is attempted.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/dependencies.py (1)
95-101: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
close()can raise on a benign race.
stop()raisesKeyErrorwhen a session id is already removed.close()reads the id list, releases the lock, then callsstop()for each id. A concurrentstop()turns shutdown into anExceptionGroup. IgnoreKeyErrorinclose().🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/dependencies.py` around lines 95 - 101, Update Harbor dependency manager close() to filter out benign KeyError results from the asyncio.gather() calls to stop(), while preserving propagation of all other exceptions through ExceptionGroup. Keep stop() behavior unchanged and continue raising only when non-KeyError failures remain.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/envelopes.py (1)
397-410: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard against a non-table
[task]value.If
task.tomlsetstaskto a scalar,task_table.get("name")raisesAttributeErrorinstead of the intendedValueError. Check the type first.Proposed fix
- task_table = document.get("task") - if task_table is None or not isinstance(task_table.get("name"), str): + task_table = document.get("task") + if not isinstance(task_table, dict) or not isinstance(task_table.get("name"), str): raise ValueError(f"Trusted Harbor task has no [task].name: {path}")🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/envelopes.py` around lines 397 - 410, Update _set_materialized_identity to validate that the parsed task value is a table or mapping before accessing task_table.get("name"). Raise the existing ValueError for both missing and non-table [task] values, while preserving the current name validation and rewrite behavior for valid tables.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/run.sh (1)
95-101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDownload
/sandbox/outputeven when the inner run fails.
set -eaborts at line 95 on a non-zero exit. The trap then deletes the sandbox, so all partial artifacts and logs are lost for exactly the runs that need inspection.Proposed fix
-openshell sandbox exec --name "$sandbox_name" --workdir "$remote_input" -- \ - /app/.venv/bin/python -m nemo_experimentalist_plugin.openshell.inner \ - --manifest "$remote_input/run.json" \ - --output /sandbox/output - -mkdir -p "$output_dir" -openshell sandbox download "$sandbox_name" /sandbox/output "$output_dir" +run_status=0 +openshell sandbox exec --name "$sandbox_name" --workdir "$remote_input" -- \ + /app/.venv/bin/python -m nemo_experimentalist_plugin.openshell.inner \ + --manifest "$remote_input/run.json" \ + --output /sandbox/output || run_status=$? + +mkdir -p "$output_dir" +openshell sandbox download "$sandbox_name" /sandbox/output "$output_dir" || true +exit "$run_status"🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/run.sh` around lines 95 - 101, Update the openshell execution flow around the inner Python command so a non-zero exit does not immediately terminate the script before downloading artifacts. Preserve the inner command’s exit status, run the existing mkdir and sandbox download steps regardless of success or failure, then return the original failure status after cleanup/download completes.plugins/nemo-experimentalist/tests/test_harbor_bridge_contracts.py (1)
113-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the extraction size cap.
No test exercises
max_bytesormax_filesonextract_directory_archive. A test that declares small header sizes but ships a large payload would catch the header-trust gap flagged inarchives.pylines 96-113.🤖 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 `@plugins/nemo-experimentalist/tests/test_harbor_bridge_contracts.py` around lines 113 - 127, Add a test alongside test_archive_rejects_duplicate_paths that calls extract_directory_archive with small max_bytes and/or max_files limits, using an archive whose header declares a small size but whose payload exceeds that size. Assert extraction raises ValueError and matches the size-limit error, covering enforcement against actual payload bytes rather than trusting tar headers.
🤖 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 `@plugins/nemo-experimentalist/README.md`:
- Around line 70-77: Update the documented EXPERIMENTALIST_SMART_MODEL_NAME and
EXPERIMENTALIST_FAST_MODEL_NAME values to remove the repeated openai/ prefixes,
leaving each model name with the format expected after configure-providers.sh
removes one prefix.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.py`:
- Around line 239-256: Update __aexit__ to log shutdown failures instead of
raising DependencyRuntimeError during cleanup. Preserve any active body
exception by returning False without raising when exc is present, and only
re-raise the shutdown error when exc is None; continue clearing the runtime
session and capability state.
- Around line 214-219: Propagate the configured max_archive_bytes value into
RemoteHarborDependencyRuntime and use it when creating the dependency overlay
archive in the digest handling path around create_directory_archive, instead of
relying on the fixed 512 MiB default. Preserve the existing archive opening and
files mapping behavior.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/dependencies.py`:
- Around line 62-68: Update session ID construction in the session-start flow to
ensure the value stored in _sessions never exceeds the
DependencySession.session_id Identifier limit, accounting for the request_id and
generated suffix. Preserve uniqueness while truncating or otherwise bounding
request_id before composing session_id, and ensure any validation failure cannot
leave the already-started Harbor environment running.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/service.py`:
- Around line 318-321: Update require_auth and require_capability to compare
authorization/token values as encoded bytes rather than strings, preventing
hmac.compare_digest from raising TypeError for non-ASCII header input. Preserve
the existing 401 Unauthorized behavior for missing or mismatched credentials.
- Around line 276-281: Update the trial sanitization loop around trial.metadata
and trial.metrics in the export flow to also redact every value in trial.outputs
using the existing _redact_value helper and sensitive configuration. Preserve
the outputs structure while replacing each output value with its redacted form,
ensuring host paths and sensitive data cannot pass through unredacted.
- Around line 412-445: Update the submission validation try/except around
extract_directory_archive to handle tarfile.TarError, either by adding it to the
caught exceptions or converting it to ValueError at the extraction boundary.
Ensure malformed candidate or overlay archives follow the existing 422 rejection
path and work_dir cleanup via the logger.exception, shutil.rmtree, and
HTTPException flow.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/configure-providers.sh`:
- Around line 36-50: Make the bridge-provider setup in the bridge creation flow
idempotent by deleting any existing "$bridge_provider" before invoking openshell
provider create, matching the inference path behavior. Move the
cleanup_failed_setup definition and its EXIT trap before provider creation so
failures during openshell provider create trigger deletion of a partially
configured provider.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/launcher.py`:
- Around line 187-205: Update the bridge launch arguments in the
subprocess.Popen call to use an explicit, configurable bind-host value instead
of hardcoded 0.0.0.0, defaulting to 127.0.0.1 and allowing the Docker/OpenShell
gateway address to be supplied when required; keep the existing localhost probe
and port behavior aligned with the selected bind address.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/preparation.py`:
- Around line 71-77: Update _copy_file and its caller so symlink validation
occurs before path resolution: stop resolving inputs.agent_spec before passing
it to _copy_file, and resolve only within _copy_file after checking
source.is_symlink() (or otherwise preserve the original path for that check).
Keep regular-file and hard-link rejection behavior unchanged.
In `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py`:
- Around line 145-160: Update the openshell-inference-credential check in the
preflight flow so it provides an independent signal: remove INFERENCE_API_KEY
from the optimizer_credential candidate set, or remove the redundant check
entirely. If retaining the check, align its hint with the credentials it
actually evaluates.
In
`@plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.py`:
- Around line 64-71: Move the EvaluatorFactory monkeypatch out of the
MutatingEvalAuthor class body and into the enclosing test function alongside the
other monkeypatch.setattr calls. Keep the patch behavior unchanged and ensure it
is applied before the class or test logic that depends on it executes.
---
Outside diff comments:
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py`:
- Line 1: Update HarborDependencyRuntime and HarborDependencyContext.__aenter__
so the returned active runtime implements DependencyCommandExecutor while
retaining the existing DependencyRuntime API. Ensure use_dependency_runtime()
recognizes it as the command executor, allowing GuardedShellTools.run() to
execute commands inside Harbor rather than on the host.
- Around line 219-346: Update HarborDependencyContext.__aenter__ to return an
object exposing the context’s execute() method, rather than returning
self._runtime alone. Preserve the existing runtime startup and readiness checks,
and ensure the returned object satisfies DependencyCommandExecutor so local
commands execute inside the Harbor environment.
---
Nitpick comments:
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.py`:
- Around line 451-461: Update the asyncio.CancelledError cleanup in the
evaluation flow to shield the job-cancellation DELETE request from task
cancellation and enforce a bounded timeout using the existing request-timeout
configuration. Preserve the conditional job_id check and re-raise the original
cancellation after cleanup is attempted.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/dependencies.py`:
- Around line 95-101: Update Harbor dependency manager close() to filter out
benign KeyError results from the asyncio.gather() calls to stop(), while
preserving propagation of all other exceptions through ExceptionGroup. Keep
stop() behavior unchanged and continue raising only when non-KeyError failures
remain.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/envelopes.py`:
- Around line 397-410: Update _set_materialized_identity to validate that the
parsed task value is a table or mapping before accessing task_table.get("name").
Raise the existing ValueError for both missing and non-table [task] values,
while preserving the current name validation and rewrite behavior for valid
tables.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.py`:
- Around line 197-201: Update the upload flow around the candidate_files loop to
collect unique parent directories for all target paths, then invoke exec_as_root
once with a command that creates every directory. Keep each
environment.upload_file call unchanged and ensure directory paths remain safely
shell-quoted.
- Around line 189-195: The setup command in the trusted-agent environment
initialization currently makes REMOTE_ROOT recursively world-writable, including
REMOTE_VENV, REMOTE_PYTHON_INSTALLS, and REMOTE_UV_CACHE. Update the command
passed to exec_as_root so agent-required writes use ownership or targeted
directory permissions, while preserving read/execute access and ensuring the
interpreter and uv binary cannot be replaced by candidate code.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/run.sh`:
- Around line 95-101: Update the openshell execution flow around the inner
Python command so a non-zero exit does not immediately terminate the script
before downloading artifacts. Preserve the inner command’s exit status, run the
existing mkdir and sandbox download steps regardless of success or failure, then
return the original failure status after cleanup/download completes.
In `@plugins/nemo-experimentalist/tests/test_harbor_bridge_contracts.py`:
- Around line 113-127: Add a test alongside test_archive_rejects_duplicate_paths
that calls extract_directory_archive with small max_bytes and/or max_files
limits, using an archive whose header declares a small size but whose payload
exceeds that size. Assert extraction raises ValueError and matches the
size-limit error, covering enforcement against actual payload bytes rather than
trusting tar headers.
In `@plugins/nemo-experimentalist/tests/test_harbor_bridge_service.py`:
- Line 55: Update _request_parts to annotate registered with the imported
RegisteredEnvelope type, and replace any __import__("json") usage in the
affected code with a normal top-level json import. Keep the existing behavior
and return annotations unchanged.
- Around line 283-294: Add tests alongside test_job_api_requires_authentication
covering POST /v1/dependencies, POST /v1/dependencies/{session_id}/exec, DELETE
/v1/dependencies/{session_id}, and DELETE /v1/evaluations/{job_id}. Verify
successful behavior and authentication failures, including the capability-token
validation and its 403 response, using the existing app factory, TestClient, and
RecordingRunner fixtures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8b3293ae-48d3-4d1c-85e1-62b2da06e982
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
docker-bake.hclplugins/nemo-experimentalist/Dockerfileplugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/pyproject.tomlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/analyzer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/models.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/rationalizer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/tools.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_analyzer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/archives.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/contracts.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/dependencies.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/envelopes.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/runner.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/service.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/configure-providers.shplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/inner.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/launcher.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/policy.docker-desktop.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/policy.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/preparation.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/provider-profiles/nemo-experimentalist-harbor-bridge.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/run.shplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.pyplugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.pyplugins/nemo-experimentalist/tests/test_cli_profile.pyplugins/nemo-experimentalist/tests/test_experiment_cli.pyplugins/nemo-experimentalist/tests/test_harbor_bridge_contracts.pyplugins/nemo-experimentalist/tests/test_harbor_bridge_runner.pyplugins/nemo-experimentalist/tests/test_harbor_bridge_service.pyplugins/nemo-experimentalist/tests/test_openshell_runtime.pyplugins/nemo-experimentalist/tests/test_preflight.pyplugins/nemo-experimentalist/tests/test_remote_harbor.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
plugins/nemo-experimentalist/tests/test_harbor_bridge_service.py (1)
220-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
__import__("json")with a module-level import.Line 228 uses
__import__("json").dumps(...). Addimport jsonat the top of the file and calljson.dumps(metadata).♻️ Proposed change
import time +import json from pathlib import Path- data={"metadata": __import__("json").dumps(metadata)}, + data={"metadata": json.dumps(metadata)},🤖 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 `@plugins/nemo-experimentalist/tests/test_harbor_bridge_service.py` around lines 220 - 232, Add a module-level json import in test_harbor_bridge_service.py and update test_job_api_returns_422_for_unknown_authority to call json.dumps(metadata) instead of dynamically importing json.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.py (1)
451-463: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel the remote job on every failure path, and do not let cleanup mask the original error.
The DELETE runs only for
asyncio.CancelledError. ATimeoutErrorat line 429, a pollingRuntimeError, or an artifact failure leaves the bridge job running until application shutdown. The bareawait client.deletecan also raise and replace the original exception.♻️ Proposed change
- except asyncio.CancelledError: + except BaseException: if job_id is not None: - async with httpx.AsyncClient( - timeout=options.request_timeout_sec, - transport=self._transport, - ) as client: - await client.delete( - f"{str(options.bridge_url).rstrip('/')}/v1/evaluations/{job_id}", - headers=headers, - ) + try: + async with httpx.AsyncClient( + timeout=options.request_timeout_sec, + transport=self._transport, + ) as client: + await client.delete( + f"{str(options.bridge_url).rstrip('/')}/v1/evaluations/{job_id}", + headers=headers, + ) + except Exception: + logger.warning("Failed to cancel Harbor bridge job %s", job_id, exc_info=True) raise🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.py` around lines 451 - 463, Update the remote evaluation cleanup around the job execution flow and its existing asyncio.CancelledError handler so any failure after job creation, including timeouts, polling RuntimeError, artifact errors, and cancellation, attempts to DELETE the remote job. Make the deletion best-effort by catching cleanup failures and preserving the original exception, while retaining staging removal in finally via shutil.rmtree.plugins/nemo-experimentalist/tests/test_remote_harbor.py (1)
89-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the trace directory creation idempotent.
_RecordingRunner.runexecutes twice.mkdir()withoutexist_ok=Trueraises if the bridge ever reuses awork_dir. Usetrace.parent.mkdir(parents=True, exist_ok=True).🤖 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 `@plugins/nemo-experimentalist/tests/test_remote_harbor.py` around lines 89 - 91, Update the trace directory creation in _RecordingRunner.run to call trace.parent.mkdir with parents=True and exist_ok=True, so repeated executions safely reuse an existing work_dir without raising.
🤖 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
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.py`:
- Around line 345-353: Resolve artifact_root before iterating over result
resources, then use that resolved path for both local artifact resolution and
the is_relative_to containment check in the resource-rewriting flow. Preserve
the existing missing/unsafe artifact validation and URI conversion behavior.
- Around line 141-167: Update execute so the _request call uses an HTTP client
timeout derived from the timeout argument, with sufficient allowance for the
requested command duration instead of always using self.request_timeout_sec;
keep the existing timeout_sec conversion and response handling unchanged.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.py`:
- Around line 277-283: Update the candidate execution call in trusted_agent’s
execution flow to pass a bounded, non-None timeout_sec to exec_as_agent. Use the
existing agent timeout configuration when available, with a safe finite fallback
for tasks that omit [agent].timeout_sec, so environment.exec cannot run
indefinitely.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/launcher.py`:
- Around line 296-314: Update the cleanup state in the launch flow around
_configure_providers so _delete_bridge_provider is attempted whenever provider
configuration was invoked, including when it fails partway through. Set the
relevant state before calling _configure_providers and preserve the existing
cleanup behavior for cases where configuration is not attempted.
---
Nitpick comments:
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.py`:
- Around line 451-463: Update the remote evaluation cleanup around the job
execution flow and its existing asyncio.CancelledError handler so any failure
after job creation, including timeouts, polling RuntimeError, artifact errors,
and cancellation, attempts to DELETE the remote job. Make the deletion
best-effort by catching cleanup failures and preserving the original exception,
while retaining staging removal in finally via shutil.rmtree.
In `@plugins/nemo-experimentalist/tests/test_harbor_bridge_service.py`:
- Around line 220-232: Add a module-level json import in
test_harbor_bridge_service.py and update
test_job_api_returns_422_for_unknown_authority to call json.dumps(metadata)
instead of dynamically importing json.
In `@plugins/nemo-experimentalist/tests/test_remote_harbor.py`:
- Around line 89-91: Update the trace directory creation in _RecordingRunner.run
to call trace.parent.mkdir with parents=True and exist_ok=True, so repeated
executions safely reuse an existing work_dir without raising.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 51fd0d95-11d3-4d3c-bfac-f47c90ab7cd4
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
docker-bake.hclplugins/nemo-experimentalist/Dockerfileplugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/pyproject.tomlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/analyzer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/models.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/rationalizer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/tools.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_analyzer.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/archives.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/contracts.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/dependencies.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/envelopes.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/runner.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/service.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/configure-providers.shplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/inner.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/launcher.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/policy.docker-desktop.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/policy.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/preparation.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/provider-profiles/nemo-experimentalist-harbor-bridge.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/run.shplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.pyplugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.pyplugins/nemo-experimentalist/tests/test_cli_profile.pyplugins/nemo-experimentalist/tests/test_experiment_cli.pyplugins/nemo-experimentalist/tests/test_harbor_bridge_contracts.pyplugins/nemo-experimentalist/tests/test_harbor_bridge_runner.pyplugins/nemo-experimentalist/tests/test_harbor_bridge_service.pyplugins/nemo-experimentalist/tests/test_openshell_runtime.pyplugins/nemo-experimentalist/tests/test_preflight.pyplugins/nemo-experimentalist/tests/test_remote_harbor.py
🚧 Files skipped from review as they are similar to previous changes (30)
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/analyzer.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/rationalizer.py
- plugins/nemo-experimentalist/pyproject.toml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/policy.docker-desktop.yaml
- plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py
- plugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.py
- docker-bake.hcl
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_analyzer.py
- plugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/policy.yaml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/provider-profiles/nemo-experimentalist-harbor-bridge.yaml
- plugins/nemo-experimentalist/README.md
- plugins/nemo-experimentalist/Dockerfile
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/runner.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/configure-providers.sh
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.py
- plugins/nemo-experimentalist/tests/test_cli_profile.py
- plugins/nemo-experimentalist/tests/test_harbor_bridge_runner.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/contracts.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/dependencies.py
- plugins/nemo-experimentalist/tests/test_harbor_bridge_contracts.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/tools.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/models.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/archives.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/service.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Contributor
|
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Contributor
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 (2)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.py (2)
219-223: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the candidate-controlled summary read.
The candidate writes
/logs/agent/summary.jsonat Lines 270-271.read_text()loads the complete file before parsing. A candidate can write an oversized JSON document and exhaust bridge memory. Read at most a fixed byte limit and reject oversized input before callingjson.loads.🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.py` around lines 219 - 223, Update the summary read in the trusted-agent method to enforce a fixed maximum byte limit before parsing: inspect the summary file size or bounded-read result, reject oversized input, and only pass accepted content to json.loads. Preserve the existing silent handling for OSError and JSON decoding failures, using the surrounding summary-reading symbols and summary_path flow.
198-202: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse POSIX semantics for remote paths. On Windows,
Path(target).parentproduces backslashes. Nested candidate uploads then target a directory thatmkdirdid not create. UsePurePosixPath(target).parentorposixpath.dirname.🤖 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.py` around lines 198 - 202, Update the candidate upload loop around the relative and target path construction to use POSIX path semantics for the remote destination, replacing the platform-dependent Path handling in the mkdir command with PurePosixPath or posixpath. Preserve the existing quoting and upload behavior while ensuring nested remote directories are created correctly on Windows.
🤖 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
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.py`:
- Around line 219-223: Update the summary read in the trusted-agent method to
enforce a fixed maximum byte limit before parsing: inspect the summary file size
or bounded-read result, reject oversized input, and only pass accepted content
to json.loads. Preserve the existing silent handling for OSError and JSON
decoding failures, using the surrounding summary-reading symbols and
summary_path flow.
- Around line 198-202: Update the candidate upload loop around the relative and
target path construction to use POSIX path semantics for the remote destination,
replacing the platform-dependent Path handling in the mkdir command with
PurePosixPath or posixpath. Preserve the existing quoting and upload behavior
while ensuring nested remote directories are created correctly on Windows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7874e48e-07ad-4662-8c71-05f8dda8f559
📒 Files selected for processing (6)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/harbor_bridge/trusted_agent.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/launcher.pyplugins/nemo-experimentalist/tests/test_harbor_bridge_runner.pyplugins/nemo-experimentalist/tests/test_openshell_runtime.pyplugins/nemo-experimentalist/tests/test_remote_harbor.py
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/nemo-experimentalist/tests/test_harbor_bridge_runner.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/launcher.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.py
ryana
marked this pull request as draft
July 31, 2026 04:21
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
ryana
marked this pull request as ready for review
July 31, 2026 14:40
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plugins/nemo-experimentalist/docker-bake.hcl`:
- Line 1: Add the standard NVIDIA copyright and Apache-2.0 SPDX license headers
at the beginning of docker-bake.hcl, before the existing “Experimentalist
control plane” comment, following the repository’s established header format.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 590355de-18a4-4abe-91a7-9f571ba23a02
📒 Files selected for processing (10)
plugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/docker-bake.hclplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/launcher.pyplugins/nemo-experimentalist/tests/test_cli_profile.pyplugins/nemo-experimentalist/tests/test_experiment_cli.pyplugins/nemo-experimentalist/tests/test_openshell_runtime.py
🚧 Files skipped from review as they are similar to previous changes (8)
- plugins/nemo-experimentalist/tests/test_cli_profile.py
- plugins/nemo-experimentalist/tests/test_experiment_cli.py
- plugins/nemo-experimentalist/README.md
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/remote_harbor.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py
- plugins/nemo-experimentalist/tests/test_openshell_runtime.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/openshell/launcher.py
| @@ -0,0 +1,15 @@ | |||
| # Experimentalist control plane. Harbor and Docker remain host-side. | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required SPDX header.
Line 1 starts with a project comment, but the file has no SPDX copyright or license identifiers. Add both headers before the existing comment.
Proposed fix
+# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
# Experimentalist control plane. Harbor and Docker remain host-side.As per coding guidelines, every file under plugins/nemo-experimentalist/**/* must include the NVIDIA copyright header and Apache-2.0 license.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Experimentalist control plane. Harbor and Docker remain host-side. | |
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |
| # SPDX-License-Identifier: Apache-2.0 | |
| # Experimentalist control plane. Harbor and Docker remain host-side. |
🤖 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 `@plugins/nemo-experimentalist/docker-bake.hcl` at line 1, Add the standard
NVIDIA copyright and Apache-2.0 SPDX license headers at the beginning of
docker-bake.hcl, before the existing “Experimentalist control plane” comment,
following the repository’s established header format.
Source: Coding guidelines
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com> Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com> Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
Signed-off-by: Ryan Angilly <rangilly@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nemo experimentalist runresolve inputs on the trusted host and execute the optimization loop inside OpenShell, with no host-side fallbackpython -m mainentrypointTrust boundary
The trusted host owns input resolution, the task catalog, OpenShell lifecycle, the bridge, Harbor orchestration, and Docker. The OpenShell sandbox owns the LLM-driven optimization loop and receives only a run-specific input snapshot. Candidate Python is inert data on the host and executes only after the trusted adapter uploads it into a Harbor task container.
The bridge does not accept a Harbor job config, image, mount, environment map, Compose document, import path, entrypoint, Docker option, or verifier mode. Unknown fields fail validation. Task overlays are restricted to host-declared mutable paths, and catalog/archive digests are reverified before execution.
The bridge credential is stored in an OpenShell provider. The sandbox sees an opaque placeholder; OpenShell replaces it with the real bearer token only on policy-approved bridge requests.
Validation
uv run --frozen pytest plugins/nemo-experimentalist/tests -q— 636 passedtychecks — passedlocal/nmp-experimentalist:localforlinux/arm64ReadyScope
This prototype does not add an inference relay, change Platform Inference Gateway behavior, force a separate verifier container, add source-control publishing, or redesign Harbor's trial mounts.
Related
mainSummary by CodeRabbit
New Features
Bug Fixes
Documentation