feat(experimentalist): migrate Experimentalist plugin into the monorepo - #896
Conversation
40c56d4 to
ad46896
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds a new ChangesNeMo Experimentalist Plugin
Sequence Diagram(s)sequenceDiagram
participant CLI as ExperimentalistCLI
participant Resolver as resolve.py
participant Backend as ExperimentalistBackend
participant Optimizer as EvolutionaryOptimizer
participant EvalAuthor as EvalAuthor
participant Evaluator as HarborEvaluator
CLI->>Resolver: resolve_experiment_inputs(profile, flags)
Resolver-->>CLI: ResolvedExperimentInputs (datasets, agent, task_template)
CLI->>Backend: make_experimentalist_backend(mode, client)
CLI->>Optimizer: run_experimentalist(deps)
Optimizer->>EvalAuthor: run(insight, traces) [if insight-driven]
EvalAuthor-->>Optimizer: EvalAuthorResult (train/validation datasets)
loop each round
Optimizer->>Evaluator: run(candidate, dataset)
Evaluator-->>Optimizer: EvaluationResult
Optimizer->>Backend: persist_evaluation / create_candidate
end
Optimizer->>Backend: persist_result(winner)
Backend-->>CLI: ExperimentalistResult
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Moves the Experimentalist agent-optimization plugin over from the standalone nemo-optimizer repo: plugin source, its unit tests, the Terminal-Bench benchmark harness, the LangChain agent-under-test example, and the framework skills the harness depends on. Adds `nemo experimentalist run|doctor`. nooa and harbor are both 3.12-only, so the plugin's dependencies on them are marked and the whole plugin is gated behind `python_full_version >= '3.12'` in the experimentalist dependency group. Without that a workspace member with a higher floor drags the entire monorepo's requires-python from 3.11 up to 3.12. nooa is a git dependency rather than a PyPI release, so osv cannot read its license metadata and it needs an entry in the license overrides. The plugin redacts userinfo from git clone URLs so tokens never reach logs. Testing that requires credential-shaped URLs, which TruffleHog's URI detector flags on structure regardless of the value, so those fixtures are annotated inline with trufflehog:ignore rather than excluding the files from scanning. nemo-optimizer is left untouched; removing the migrated files there is a follow-up once this copy is proven. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
ad46896 to
043637b
Compare
The lint and unit-test jobs resolve on 3.11, where uv correctly skips the plugin and its 3.12-only nooa/harbor dependencies. ty still walked the plugin's files and pytest still collected its tests, so every import of those two packages came back unresolved and every test module failed to import. Adds the plugin to the existing find_spec-guarded discovery exclusions alongside nemo-insights, and turns off unresolved-import for the plugin so the rest of its type checking still runs. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
b22138b to
a3a92ec
Compare
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/tools.py (1)
154-223: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winPath traversal in
WorkspaceToolbypasses the held-out-split guard.
read_agent_file/read_analysis_filejoinagent_id/relative_path/path_or_roundonto_agents_root/_analysis_rootwith no..-containment check before_read_filereads the path. SinceWorkspaceToolis exposed to CodeAct-driven agents (Proposer,TraceAnalyzer) that can run arbitrary Python, a call likeself.workspace.read_agent_file("agent-1", "../../../dataset/validation/task-1/tests/test.sh")escapes_agents_rootand reads held-out data — exactly the leakageGuardedShellTools/DatasetToolwere built to prevent on the shell channel. This is a third, unguardedPath.read_textchannel.🔒 Suggested fix — enforce containment in `_read_file`
def _read_file(self, path: Path, limit: int | None = 4000) -> str: - if not path.exists(): + resolved = path.resolve() + if not resolved.is_relative_to(self.workspace): + logger.warning(f"WorkspaceTool blocked out-of-workspace read: {path}") + return "" + if not resolved.exists(): return "" try: - text = path.read_text().strip() + text = resolved.read_text().strip()🤖 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/tools.py` around lines 154 - 223, Prevent path traversal in WorkspaceTool by enforcing that paths passed to _read_file remain within the intended workspace roots before reading. Update read_agent_file and read_analysis_file (and their callers) to validate resolved paths using containment checks, returning the existing empty-string result for escapes or otherwise rejecting them consistently; preserve valid relative-path and round-based reads.
🟡 Minor comments (16)
pyproject.toml-648-652 (1)
648-652: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
unresolved-importscoped tighter
include = ["plugins/nemo-experimentalist/**"]disables import checking for the whole plugin on 3.12 too. Limit it to the 3.11-only files, or switch toallowed-unresolved-importsfornooa/harboronly.🤖 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 `@pyproject.toml` around lines 648 - 652, Tighten the tool.ty override for unresolved-import instead of disabling import checking across all of plugins/nemo-experimentalist. Scope the override to the Python 3.11-only files, or replace it with allowed-unresolved-imports limited to the nooa and harbor imports, while preserving import checking for the rest of the plugin.plugins/nemo-experimentalist/pyproject.toml-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing SPDX header.
As per coding guidelines: "Include the required NVIDIA SPDX copyright and Apache-2.0 license header in every file."
🔧 Proposed fix
+# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + [project]🤖 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/pyproject.toml` at line 1, Add the required NVIDIA SPDX copyright and Apache-2.0 license header at the top of pyproject.toml, before the [project] section, following the repository’s standard header format.Source: Coding guidelines
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py-458-458 (1)
458-458: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the credential hint path to
examples/terminal-bench-agent/.env.example._ENV_EXAMPLE_POINTERstill points atexamples/tau2-nemo-oo-agent, so the missing-credential hint sends users to the wrong place.🤖 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/preflight.py` at line 458, Update the _ENV_EXAMPLE_POINTER constant to reference examples/terminal-bench-agent/.env.example so missing-credential hints direct users to the correct environment example.plugins/nemo-experimentalist/benchmarks/README.md-58-75 (1)
58-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the benchmark paths.
The documented commands use
benchmarks/experimentalist/..., but the runner and configs are underbenchmarks/run.pyandbenchmarks/configs/. These commands will fail as written.Proposed fix
-uv run python benchmarks/experimentalist/run.py --validate-only +uv run python benchmarks/run.py --validate-only -uv run python benchmarks/experimentalist/run.py \ - --config benchmarks/experimentalist/configs/smoke.yaml +uv run python benchmarks/run.py \ + --config benchmarks/configs/smoke.yaml -uv run python benchmarks/experimentalist/run.py \ - --config benchmarks/experimentalist/configs/quality.yaml +uv run python benchmarks/run.py \ + --config benchmarks/configs/quality.yaml🤖 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/benchmarks/README.md` around lines 58 - 75, Update all benchmark commands in the README to reference the actual runner at benchmarks/run.py and configuration files under benchmarks/configs/, including the validate-only, smoke, and quality examples. Preserve the existing command options and benchmark behavior while correcting only the paths.plugins/nemo-experimentalist/framework-skills/langchain-framework/references/ecosystem-primer.md-112-120 (1)
112-120: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope the warning.
LANGSMITH_API_KEY,LANGCHAIN_API_KEY, andLANGGRAPH_HOST_API_KEYare all accepted by the LangGraph CLI for auth, so “OLDER NAMES NO LONGER WORK” is too broad. Separate LangSmith tracing vars from LangGraph deployment auth.🤖 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/framework-skills/langchain-framework/references/ecosystem-primer.md` around lines 112 - 120, Update the “Set Environment Variables” guidance to scope the naming warning specifically to LangSmith tracing variables, while documenting that LANGSMITH_API_KEY, LANGCHAIN_API_KEY, and LANGGRAPH_HOST_API_KEY remain accepted for LangGraph CLI deployment authentication. Keep the existing LangSmith observability variables unchanged.plugins/nemo-experimentalist/framework-skills/langchain-framework/references/langchain-rag.md-42-45 (1)
42-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
langchain_core.vectorstoresforInMemoryVectorStore.
langchain_community.vectorstoresis the outdated path here; update the import to match the current LangChain API.🤖 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/framework-skills/langchain-framework/references/langchain-rag.md` around lines 42 - 45, Update the InMemoryVectorStore import in the LangChain RAG reference to use langchain_core.vectorstores instead of langchain_community.vectorstores, while leaving the other imports unchanged.plugins/nemo-experimentalist/tests/experimentalist/test_tools.py-13-22 (1)
13-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
pythonmay not exist on PATH.Many CI images ship only
python3, making this test fail for environmental reasons. Use the running interpreter.♻️ Proposed fix
+import sys + async def test_guarded_shell_tools_runs_allowed_commands(tmp_path): shell = GuardedShellTools(cwd=tmp_path) try: - result = await shell.run("python -", stdin="print('allowed')") + result = await shell.run(f"{sys.executable} -", stdin="print('allowed')") finally: await shell.close()🤖 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/experimentalist/test_tools.py` around lines 13 - 22, Update test_guarded_shell_tools_runs_allowed_commands to invoke the currently running Python interpreter rather than the literal "python" command, while preserving the existing stdin, output, return-code, and success assertions.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/otlp.py-55-67 (1)
55-67: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNaive ISO timestamps get local-time semantics.
datetime.fromisoformat("2026-01-01T00:00:00").timestamp()uses the host timezone, shifting span times by the UTC offset. Default to UTC whentzinfois absent.🛠️ Proposed fix
- from datetime import datetime + from datetime import UTC, datetime try: dt = datetime.fromisoformat(str(value).replace("Z", "+00:00")) except ValueError: return None + if dt.tzinfo is None: + dt = dt.replace(tzinfo=UTC) return str(int(dt.timestamp() * 1_000_000_000))🤖 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/otlp.py` around lines 55 - 67, Update _to_unix_nano so parsed ISO datetimes without tzinfo are assigned UTC before calling timestamp(). Preserve existing timezone-aware handling and return behavior for invalid or numeric values.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py-42-81 (1)
42-81: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocstring contradicts the implementation.
Doc says missing metrics count as 0 and the denominator is always
len(results)so failures penalize the score. Code drops failed trials, divides bylen(completed), and raises on inconsistent metric keys — failures do not affect the aggregate at all.Either fix the doc or the math; as written, callers will misread aggregate scores.
📝 Docstring aligned with current behavior
- Defaults to averaging each metric across all trials, treating trials that - did not emit a metric (e.g. failed trials) as contributing 0. The denominator - is always ``len(results)``, not the number of trials that reported each metric, - so failure counts against the aggregate score. + Defaults to averaging each metric across trials whose status is not + ``"failed"``. Failed trials are excluded from both numerator and + denominator, so they do not lower the aggregate. All completed trials + must report the same metric names.🤖 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/base.py` around lines 42 - 81, Update the docstring for aggregate_results to describe the current implementation: exclude failed trials, return an empty mapping when no trials complete, require consistent metric keys among completed trials, and average each metric over len(completed). Remove claims that missing metrics contribute zero or that len(results) is the denominator.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py-102-106 (1)
102-106: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA base
EvaluatorConfiginstance bypasses coercion.Only
dictis validated intotype(self.options). Passing a plainEvaluatorConfigforwards it as-is to_run, whereHarborEvaluatorreadsoptions.jobs_dir/options.job_name→AttributeError. Coerce any non-matching model too.🛡️ Proposed coercion
if options is None: options = self.options elif isinstance(options, dict): options = type(self.options).model_validate({**self.options.model_dump(), **options}) + elif not isinstance(options, type(self.options)): + options = type(self.options).model_validate( + {**self.options.model_dump(), **options.model_dump()} + )🤖 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/base.py` around lines 102 - 106, Update the options handling in the evaluator method containing the _run call so any provided configuration that is not already an instance of type(self.options) is coerced through type(self.options).model_validate, including base EvaluatorConfig instances; preserve the existing self.options default and dictionary-merge behavior, and pass the resulting typed configuration to _run.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/tools.py-108-121 (1)
108-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
list_agents()crashes on an agent id without a hyphen.
s.split("-")[1]raisesIndexErrorfor any qualifying directory name that has no-(e.g. a manually-created agent dir). Guard the split length.🐛 Suggested fix
- return sorted( - ids, - key=lambda s: int(s.split("-")[1]) if s.split("-")[1].isdigit() else 0, - ) + def _sort_key(agent_id: str) -> int: + parts = agent_id.split("-", 1) + return int(parts[1]) if len(parts) > 1 and parts[1].isdigit() else 0 + + return sorted(ids, key=_sort_key)🤖 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/tools.py` around lines 108 - 121, Update the sorting key in list_agents so agent IDs without a hyphen do not raise IndexError; guard the split before accessing its second element, while preserving numeric ordering for IDs with a numeric suffix and the existing fallback for nonnumeric values.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/coder.py-682-696 (1)
682-696: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
run_pyright()output is ignored
run()callsself.run_pyright(candidate.name)twice and discards the diagnostics. Plain method returns are not auto-fed back into the agent loop here, so pyright errors fromapply_change/wire_up_changenever surface. Log or act on non-empty output before continuing.🤖 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/coder.py` around lines 682 - 696, Update the two run_pyright calls in the candidate workflow to capture their diagnostics and log or otherwise propagate any non-empty output before continuing. Ensure pyright errors from apply_change and wire_up_change are surfaced to the agent loop rather than discarded, while preserving the existing sequencing around optimize_subproblem and integration_check.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/experiment_mirror.py-7-8 (1)
7-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring names the wrong client surface. The module claims it touches
client.experiment_groups/client.experiments, but_upsert_experiment(Lines 205, 219) and the retrieves at Lines 250, 264 all useclient.evaluations. Fix the docstring so readers can find the real API surface.🤖 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/experiment_mirror.py` around lines 7 - 8, Update the module docstring in experiment_mirror.py to reference client.evaluations instead of client.experiment_groups and client.experiments, matching the API used by _upsert_experiment and the retrieval logic.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/experimentalist_backend.py-821-842 (1)
821-842: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRetry budget docstring is wrong.
retries=5sleeps only 4 times (1+2+4+8 = 15s), not "1s, 2s, 4s, 8s, 16s (up to 31s)". Either fix the text or sleep after the final failed attempt.📝 Proposed fix
- Intake indexing after OTLP upload can take several seconds. Uses exponential - backoff: 1s, 2s, 4s, 8s, 16s (up to 31s total) by default. + Intake indexing after OTLP upload can take several seconds. With the default + ``retries=5`` there are four backoff sleeps: 1s, 2s, 4s, 8s (15s total).🤖 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/experimentalist_backend.py` around lines 821 - 842, Correct the `_retrieve_trace_with_retry` docstring to match the current loop behavior: with the default `retries=5`, it performs four sleeps of 1, 2, 4, and 8 seconds, totaling 15 seconds, and does not sleep after the final attempt. Keep the retry implementation unchanged.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/experimentalist_backend.py-864-865 (1)
864-865: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
winner_agentgets an entity id here but a label everywhere else.loop._finalizewritesrun_entity.winner_agent = best_id(a candidate label), andexperiment_mirror.group_metadatapublishes it aswinner_candidate. Locally id == label so the divergence hides; with store-assigned ids the two writers disagree.🐛 Proposed fix
if result.winner is not None: - run.winner_agent = result.winner.id + run.winner_agent = result.winner.label🤖 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/experimentalist_backend.py` around lines 864 - 865, Update the winner assignment in the result-handling flow to store the winning candidate’s label rather than result.winner.id, matching loop._finalize and experiment_mirror.group_metadata. Reuse the winner label available from result.winner and preserve the existing None guard.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/repository.py-63-76 (1)
63-76: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winHost detection is a bare substring match.
"github" in urlmatcheshttps://attacker.example.com/github/repo.git, selectingghfor an unrelated host. Match against the parsed hostname instead.🛡️ Proposed fix
- if "github" in url: - cli = "gh" - elif "gitlab" in url: - cli = "glab" - else: - return None - hostname = urlsplit(url).hostname if "://" in url else url.partition("@")[2].partition(":")[0] - return cli, hostname or "" + hostname = urlsplit(url).hostname if "://" in url else url.partition("@")[2].partition(":")[0] + host = (hostname or "").lower() + if "github" in host: + cli = "gh" + elif "gitlab" in host: + cli = "glab" + else: + return None + return cli, host🤖 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/repository.py` around lines 63 - 76, Update pr_cli_for_repo_url to detect GitHub or GitLab from the parsed remote hostname rather than the full URL string. Preserve support for github.com and self-hosted GitLab hosts while ensuring path segments or unrelated hostnames containing “github” or “gitlab” do not select a CLI.
🧹 Nitpick comments (19)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/entities.py (1)
62-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
typing.Selfinstead of quoted self-references.As per coding guidelines: "Use concrete type hints rather than string-based annotations, and do not hide imports under
TYPE_CHECKING."♻️ Proposed fix
-from typing import Any, Literal, Sequence +from typing import Any, Literal, Self, Sequence- def _restore_id_from_json(cls, data: Any, handler: Any) -> "ExperimentRun": + def _restore_id_from_json(cls, data: Any, handler: Any) -> Self:- def slim(self) -> "Candidate": + def slim(self) -> Self:Also applies to: 188-188
🤖 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/entities.py` at line 62, Update the return annotations of _restore_id_from_json and the other referenced class method to use typing.Self instead of the quoted "ExperimentRun" reference. Import Self directly from typing and keep the annotations concrete without TYPE_CHECKING-only imports.Source: Coding guidelines
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py (1)
138-166: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConfig models ignore unknown keys.
AgentProfileforbids extras, but a typo in--config(max_rouds: 30) validates cleanly and the run silently uses defaults. Forbid extras on the config models too.♻️ Proposed fix
class EvolutionaryOptimizerConfig(BaseModel): """Complete import-safe schema for one optimizer run.""" + model_config = ConfigDict(extra="forbid") + `@model_validator`(mode="before")-from pydantic import BaseModel, Field, ValidationError, model_validator +from pydantic import BaseModel, ConfigDict, Field, ValidationError, model_validatorSame treatment is worth applying to the nested
*Configmodels.🤖 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/resolve.py` around lines 138 - 166, Configure EvolutionaryOptimizerConfig and the nested *Config models to reject unknown fields, including typos such as max_rouds, instead of silently ignoring them. Apply the same extra-field-forbidden policy used by AgentProfile across the relevant Pydantic config classes while preserving all declared defaults and validation behavior.plugins/nemo-experimentalist/framework-skills/langchain-framework/SKILL.md (1)
9-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep the top-level skill concise.
Move the detailed API, middleware, persistence, and integration material into the linked reference files; retain framework selection, installation, and a short quickstart here. This file is too large for progressive disclosure.
Based on learnings, SKILL.md files should remain concise execution/configuration files and load detailed reference material separately.
Also applies to: 537-557
🤖 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/framework-skills/langchain-framework/SKILL.md` around lines 9 - 16, Condense the top-level SKILL.md to framework selection guidance, installation, and a brief quickstart. Move detailed API, middleware, persistence, and integration content—including the material around lines 537-557—into linked reference files, and ensure this skill links to those references for progressive disclosure.Source: Learnings
plugins/nemo-experimentalist/tests/test_experimentalist_benchmark.py (1)
56-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
>= 0is vacuous.max_validation_repair_attemptsis already bounded0..10byEvalAuthorConfig, so this asserts nothing about the shipped configs. Pin the expected values per config instead.🤖 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_experimentalist_benchmark.py` around lines 56 - 57, Update the assertions in the benchmark test to verify each shipped config’s exact max_validation_repair_attempts value rather than merely asserting it is nonnegative. Use the config-specific expected values while preserving the existing n_attempts assertion.plugins/nemo-experimentalist/tests/test_eval_author_agent.py (1)
187-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFakes never exercise the suite identity contract.
record_analysisis a no-op andpromote_localreturns tasks with ids (task-{ref}) that are not slugs, so the real coupling — agent keys statuses bytask.id, manifest keys by slug, and trials are paired positionally — is invisible to these tests. Assert the received status keys against the staged slugs.🤖 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_eval_author_agent.py` around lines 187 - 195, Update the fake suite methods promote_local and record_analysis to enforce the identity contract: return promoted tasks using the staged task slugs rather than synthetic task-{ref} ids, and assert that record_analysis receives exactly those staged slugs as status keys. Preserve positional pairing with staged_tasks when constructing the promoted Dataset.plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py (1)
917-935: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard this real-
bashtest.
shutil.which("bash", path=os.defpath)returningNoneon a minimal or non-POSIX runner turns an environment gap into a hard failure. Skip instead.♻️ Skip when bash is unavailable
`@pytest.mark.asyncio` async def test_shell_noexec_environment_prevents_execution_without_n_flag() -> None: bash_path = shutil.which("bash", path=os.defpath) - assert bash_path is not None + if bash_path is None: + pytest.skip("bash not available on the default 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/tests/experimentalist/test_evaluator_harbor.py` around lines 917 - 935, Update test_shell_noexec_environment_prevents_execution_without_n_flag to skip the test when shutil.which("bash", path=os.defpath) returns None, instead of asserting bash_path is not None; retain the existing subprocess assertions when bash is available.plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_base.py (1)
228-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name claims a merge it never asserts.
result.idis identical with or without the dict override, so this passes even if merging breaks. Capture the effective options in_runand assertforce_rerun is True.♻️ Assert the merged option
`@pytest.mark.asyncio` async def test_run_with_dict_options_merges(): class ConcreteDataset(Dataset): `@classmethod` def from_ref(cls, ref): return cls(id="test") - evaluator = ConcreteEvaluator(options=EvaluatorConfig(force_rerun=False)) + seen: list[EvaluatorConfig] = [] + + class RecordingEvaluator(ConcreteEvaluator): + async def _run(self, agent: Path, dataset: Dataset, options: EvaluatorConfig) -> Sequence[TrialResult]: + seen.append(options) + return await super()._run(agent, dataset, options) + + evaluator = RecordingEvaluator(options=EvaluatorConfig(force_rerun=False)) dataset = ConcreteDataset(id="ds") result = await evaluator.run( agent=Path("/tmp/agent"), dataset=dataset, options={"force_rerun": True}, ) assert result.id == "agent-ds" + assert seen[0].force_rerun is 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/experimentalist/test_evaluator_base.py` around lines 228 - 241, Update test_run_with_dict_options_merges to capture the effective options received by the evaluator’s _run method, then assert that force_rerun is True after passing the dictionary override. Keep the existing result.id assertion, but ensure the test directly verifies that run merges the override into EvaluatorConfig.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/models.py (1)
13-20: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd bounds to the other two int fields.
max_validation_repair_attemptsis constrained butmax_summary_tokens/max_tracesaccept 0 and negatives, which silently disables summarization/trace analysis.♻️ Proposed constraints
max_summary_tokens: int = Field( default=80_000, + gt=0, description="Max tokens the token-budget summarizer may use.", ) max_traces: int = Field( default=10, + ge=0, description="Max trace refs from the insight to analyze in depth.", )🤖 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/eval_author/models.py` around lines 13 - 20, Update the int fields max_summary_tokens and max_traces in the relevant model to enforce positive-value bounds consistent with max_validation_repair_attempts. Ensure zero and negative values are rejected while preserving their existing defaults and descriptions.plugins/nemo-experimentalist/tests/test_cli_profile.py (1)
229-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFinite-iterator patches of stdlib
uuid.uuid4can drain unexpectedly.cli.uuidis the stdlib module, so both tests replaceuuid.uuid4process-wide; any unrelated caller during the test exhausts the scripted list and fails withStopIterationinstead of the intended assertion.
plugins/nemo-experimentalist/tests/test_cli_profile.py#L229-L244: back the collision script with a fallback (e.g.itertools.chain(scripted, iter(uuid.UUID(hex="f"*32), None))or a callable that returns a fresh UUID once the script is exhausted).plugins/nemo-experimentalist/tests/test_cli_profile.py#L662-L676: apply the same fallback to thegenerated_uuidsiterator here.🤖 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_cli_profile.py` around lines 229 - 244, The finite uuid.uuid4 patches can raise StopIteration if unrelated calls consume the scripted values. In plugins/nemo-experimentalist/tests/test_cli_profile.py lines 229-244 and 662-676, update each generated_uuids setup and its monkeypatch of cli.uuid.uuid4 to retain the scripted sequence while returning a fallback UUID after exhaustion, preserving the intended collision values first.plugins/nemo-experimentalist/tests/test_skills_entry_point.py (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop
from __future__ import annotations.It turns every annotation into a string, and nothing here needs it (Python floor is 3.11+).
♻️ Proposed fix
-from __future__ import annotations - from pathlib import PathAs per coding guidelines, "In Python code, prefer concrete type hints over string-based type hints".
🤖 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_skills_entry_point.py` around lines 6 - 8, Remove the unnecessary `from __future__ import annotations` directive from `test_skills_entry_point.py`, preserving the existing concrete type annotations and imports.Source: Coding guidelines
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_explorer.py (2)
2223-2237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate closure logic; reuse
_session_span_ids.Same fixed-point walk as lines 1857-1868, minus turn span ids — so
get_raw_spanssilently omits turn spans that aren't parented to the session span.♻️ Proposed refactor
session = self._find_session(session_id) if not session: return f"Session not found: {session_id}" - span_ids = {session.span_id} - changed = True - while changed: - changed = False - for span in self.raw_spans: - if span.parent_span_id in span_ids and span.span_id not in span_ids: - span_ids.add(span.span_id) - changed = True + span_ids = self._session_span_ids(session) spans = [span.model_dump(mode="json") for span in self.raw_spans if span.span_id in span_ids]🤖 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/trace_explorer.py` around lines 2223 - 2237, Update get_raw_spans to reuse _session_span_ids for collecting the session subtree span IDs instead of duplicating the fixed-point traversal. Ensure the result includes all raw spans associated with the session while excluding turn span IDs as required by that helper, then preserve the existing JSON serialization.
1339-1349: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
local_path_from_uriinstead of prefix stripping.
ref.uri[len("file://"):]mishandles percent-encoded paths (%20) and plain (schemeless) paths that the rest of the evaluator layer accepts.models.local_path_from_urialready normalizes both.♻️ Proposed refactor
-from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import ResourceRef +from nemo_experimentalist_plugin.experimentalist.components.evaluator.models import ResourceRef, local_path_from_uri- if ref.uri.startswith("file://"): - return await cls.from_file(ref.uri[len("file://") :]) + if not ref.uri.startswith("intake://"): + return await cls.from_file(local_path_from_uri(ref.uri, context="Trace reference"))🤖 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/trace_explorer.py` around lines 1339 - 1349, Update the file-resource branch in the trace-loading method to use models.local_path_from_uri(ref.uri) instead of manually removing the file:// prefix, then pass the normalized path to cls.from_file. Preserve the existing Intake URI handling and unsupported-resource validation.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/cache.py (1)
48-63: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBlocking I/O inside concurrent async pipeline.
trace_hashsynchronously reads/hashes the whole trace file. It's invoked fromTraceAnalyzer.run()(async) via_trace_cache_key, andAgentAnalyzer.run()fans out manyTraceAnalyzer.run()calls concurrently withasyncio.gather. Each hash call blocks the event loop, serializing what should be parallel I/O.Offload to a thread at the call site (or here):
♻️ Suggested fix
-def trace_hash(trace_path: str | Path) -> str: - h = hashlib.sha256() - with open(trace_path, "rb") as f: - for chunk in iter(lambda: f.read(1 << 20), b""): - h.update(chunk) - digest = h.hexdigest() - return f"trace-{digest}" +def _trace_hash_sync(trace_path: str | Path) -> str: + h = hashlib.sha256() + with open(trace_path, "rb") as f: + for chunk in iter(lambda: f.read(1 << 20), b""): + h.update(chunk) + return f"trace-{h.hexdigest()}" + + +async def trace_hash_async(trace_path: str | Path) -> str: + return await asyncio.to_thread(_trace_hash_sync, trace_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/experimentalist/components/cache.py` around lines 48 - 63, Offload the synchronous trace hashing performed by trace_hash when called from the async _trace_cache_key path, using the project’s async thread offloading mechanism and awaiting its result before constructing the cache key. Keep trace_hash’s file-reading behavior unchanged, and ensure concurrent TraceAnalyzer.run calls from AgentAnalyzer.run do not block the event loop.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/tools.py (1)
187-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
get_metadata()reaches intoLocalExperimentalistBackendinternals, and its docstring doesn't match behavior.Building a throwaway instance via
__new__and poking_backend._eo/calling_backend._load_candidatecouples this to another class's private implementation. Separately, the docstring promises "a minimal candidate with default values" on a missing file, but the code raisesFileNotFoundErrorinstead — callers currently mask this by wrapping the call in try/except (e.g. proposer.py), but the contract as documented is wrong.Consider exposing a small public/shared helper (e.g. a module-level
load_candidate(path)inexperimentalist_backend.py) instead of reaching into_eo/_load_candidate.🤖 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/tools.py` around lines 187 - 209, Refactor get_metadata() to use a public shared candidate-loading helper, such as load_candidate(path) in experimentalist_backend, instead of constructing LocalExperimentalistBackend with __new__ or accessing _eo and _load_candidate. Align get_metadata()’s docstring and behavior by returning the documented minimal default Candidate when metadata.json is missing or unreadable, while preserving normal deserialization for valid files.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/util.py (1)
10-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent skips make a mistyped skills dir undiagnosable. A path that isn't a directory or lacks
SKILL.mdis dropped without a trace. Log it.♻️ Proposed fix
+import logging from pathlib import Path from nooa import TextSkill from nooa.skill_registry import SkillRegistry +logger = logging.getLogger(__name__) + def load_framework_skills(registry: SkillRegistry, dirs: list[Path]) -> None: """Register TextSkills from user-provided framework skill directories.""" for skill_dir in dirs: if not skill_dir.is_dir(): + logger.warning("Framework skill dir not found, skipping: %s", skill_dir) continue if (skill_dir / "SKILL.md").exists() or (skill_dir / "skill.md").exists(): skill = TextSkill(path=skill_dir) registry.register(f"ext.{skill.id}", skill) + else: + logger.warning("No SKILL.md in %s, skipping", skill_dir)🤖 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/util.py` around lines 10 - 19, Update load_framework_skills to log each user-provided skill path that is not a directory or lacks either recognized SKILL.md filename before continuing; preserve registration and loading behavior for valid skill directories.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py (2)
1252-1275: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead guard.
pendingis already proven non-empty by the early return at Line 1253, so theif pending:at Line 1255 is redundant and only obscures thatcandidate_resultsis always bound.♻️ Proposed cleanup
pending = [c for c in candidates if c.validation_reward is None] if not pending: return {} - if pending: - splits = frozenset({"validation"}) - restore_heldout_splits(self.working_dir, splits=splits) - try: - candidate_results = await asyncio.gather( - *[ - self._evaluate_agent( - c, - dataset, - evaluator, - ) - for c in pending - ] - ) - finally: - ensure_heldout_hidden(self.working_dir, splits=splits) + splits = frozenset({"validation"}) + restore_heldout_splits(self.working_dir, splits=splits) + try: + candidate_results = await asyncio.gather( + *[self._evaluate_agent(c, dataset, evaluator) for c in pending] + ) + finally: + ensure_heldout_hidden(self.working_dir, splits=splits)🤖 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/loop.py` around lines 1252 - 1275, Remove the redundant `if pending:` guard in the candidate evaluation flow, keeping `restore_heldout_splits`, the `try/finally` around `_evaluate_agent` gathering, and the subsequent result mapping unconditionally after the existing empty-`pending` early return.
923-987: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo near-identical scan loops. Lines 936-959 and 962-977 repeat the same agent-dir filter +
metadata.jsonparse. Extract an iterator (_iter_agent_metadata(agents_dir) -> Iterator[tuple[Path, dict]]) and drive both passes from it.🤖 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/loop.py` around lines 923 - 987, The _delete_all_artifacts method duplicates agent-directory filtering and metadata parsing across two scan loops. Add an _iter_agent_metadata helper yielding each valid agent Path and parsed metadata, then use it for both artifact deletion and killed_round cleanup while preserving each pass’s existing behavior.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/models.py (1)
264-267: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSort key guards the wrong component. It tests
split("-")[-1].isdigit()but parsessplit("-")[1], so any label with an extra segment (agent-foo-1) raisesValueErrorinstead of falling back to0.♻️ Proposed fix
- for label in sorted( - self.nodes.keys(), - key=lambda x: int(x.split("-")[1]) if x.split("-")[-1].isdigit() else 0, - ): + def _order(label: str) -> int: + parts = label.split("-") + return int(parts[1]) if len(parts) > 1 and parts[1].isdigit() else 0 + + for label in sorted(self.nodes.keys(), key=_order):🤖 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/models.py` around lines 264 - 267, Update the sorting key in the node-label iteration to validate the same component it parses: guard the second segment used by int(), rather than the final segment. Ensure labels such as “agent-foo-1” fall back to 0 instead of raising ValueError, while preserving numeric sorting for valid labels.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py (1)
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the concrete config type here
config: Anydrops type safety. ImportEvolutionaryOptimizerConfigdirectly and useEvolutionaryOptimizerConfig | Nonehere.🤖 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/deps.py` at line 59, Update the config annotation in the relevant dependency definition to import and use EvolutionaryOptimizerConfig directly, replacing Any with EvolutionaryOptimizerConfig | None while preserving the existing default of None.Source: Coding guidelines
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (1)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/run.py (1)
91-109: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEval Author staging contract diverges between the CLI entrypoint and the optimizer loop.
run_eval_authorstages only the task template, so train/validation refs reach the agent pointing at caller-owned directories, and the test locks that behavior in.
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/run.py#L91-L109: replacestage_task_templatewithstage_eval_author_inputsand build the datasets from the staged refs.plugins/nemo-experimentalist/tests/test_eval_author_run.py#L190-L190: update thedataset_refsexpectation to the stagedexperiment_dir/dataset/{train,validation}URIs instead oftrain_ref/validation_ref.🤖 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/eval_author/run.py` around lines 91 - 109, Update run_eval_author to call stage_eval_author_inputs instead of stage_task_template, then build the task template and train/validation datasets from the returned staged references. In plugins/nemo-experimentalist/tests/test_eval_author_run.py:190, update dataset_refs to expect experiment_dir/dataset/{train,validation} URIs rather than train_ref and validation_ref.
🟡 Minor comments (22)
plugins/nemo-experimentalist/framework-skills/langchain-framework/references/ecosystem-primer.md-173-178 (1)
173-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPoint structured-output questions to
langchain-fundamentals.The
langchain-middlewareentry is labeled for structured output here, but the reference table maps structured output tolangchain-fundamentalsand middleware to HITL. This sends agents to the wrong skill.🤖 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/framework-skills/langchain-framework/references/ecosystem-primer.md` around lines 173 - 178, Update the LangChain skill list near langchain-fundamentals so structured-output questions point to langchain-fundamentals, and revise langchain-middleware to represent middleware/HITL topics consistent with the reference table.plugins/nemo-experimentalist/framework-skills/langchain-framework/SKILL.md-158-167 (1)
158-167: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass typed context via
context=.
context_schemaexpects the runtime object incontext=, not underconfigurable.context; keepconfigforthread_idor other run-scoped settings.🤖 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/framework-skills/langchain-framework/SKILL.md` around lines 158 - 167, Update the agent.invoke call to pass the UserContext instance through the context= argument, while retaining config only for thread_id or other run-scoped settings; keep the create_agent context_schema declaration unchanged.plugins/nemo-experimentalist/framework-skills/langchain-framework/references/deep-agents-core.md-227-236 (1)
227-236: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't make the checkpointer look mandatory for basic agent creation. It’s only needed for persistence across turns and HITL resume flows, so this example should present it as optional.
🤖 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/framework-skills/langchain-framework/references/deep-agents-core.md` around lines 227 - 236, Update the createDeepAgent example so the checkpointer option is omitted from basic agent creation, while preserving the existing backend, skills, and invocation flow. Present MemorySaver only as an optional configuration for persistence across turns or HITL resume scenarios.plugins/nemo-experimentalist/framework-skills/langchain-framework/references/deep-agents-core.md-267-271 (1)
267-271: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace the backend factories with direct backend instances.
plugins/nemo-experimentalist/framework-skills/langchain-framework/references/deep-agents-core.md#L267-L271plugins/nemo-experimentalist/framework-skills/langchain-framework/references/deep-agents-memory.md#L89-L95Both examples still use the callable
backendform; update them to the current backend API.🤖 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/framework-skills/langchain-framework/references/deep-agents-core.md` around lines 267 - 271, The create_deep_agent examples still pass backend factories instead of direct backend instances. Update the backend arguments in plugins/nemo-experimentalist/framework-skills/langchain-framework/references/deep-agents-core.md lines 267-271 and plugins/nemo-experimentalist/framework-skills/langchain-framework/references/deep-agents-memory.md lines 89-95 to instantiate and pass the appropriate backend objects directly, preserving each example’s existing configuration.plugins/nemo-experimentalist/framework-skills/langchain-framework/references/langgraph-fundamentals.md-611-615 (1)
611-615: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid redeclaring
result.Lines 612 and 614 declare
const resultin the same scope, so the TypeScript example does not compile. Use distinct names or show the configurations as separate snippets.As per coding guidelines: “Ensure all code snippets are tested and actually work before publishing.”
🤖 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/framework-skills/langchain-framework/references/langgraph-fundamentals.md` around lines 611 - 615, Update the LangGraph invoke examples so they do not redeclare the const result variable in the same scope: either assign distinct names to the two graph.invoke calls or separate them into independently scoped snippets, while preserving both invocation examples.Source: Coding guidelines
plugins/nemo-experimentalist/framework-skills/langchain-framework/references/langchain-rag.md-262-267 (1)
262-267: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRestrict FAISS deserialization to trusted indexes.
allow_dangerous_deserialization=Trueenables pickle loading; note that it should only be used for locally generated, integrity-protected indexes here and in the later FAISS example.🤖 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/framework-skills/langchain-framework/references/langchain-rag.md` around lines 262 - 267, Update the FAISS.load_local examples to document that allow_dangerous_deserialization=True is only appropriate for locally generated, integrity-protected, trusted indexes; add the same restriction to the later FAISS example while preserving the existing loading behavior.plugins/nemo-experimentalist/examples/terminal-bench-agent/agent.py-42-46 (1)
42-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the exit code when truncating output.
Long output drops the leading
exit_code=...field, so the agent cannot distinguish failed commands from successful ones. Keep the status header and truncate only stdout/stderr content.🤖 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/examples/terminal-bench-agent/agent.py` around lines 42 - 46, Update the output assembly around rendered and MAX_COMMAND_OUTPUT_CHARS so truncation preserves the leading exit_code status header. Separate the status header from stdout/stderr content, truncate only the content while retaining the header, and keep the existing omitted-character indicator and full-output behavior.plugins/nemo-experimentalist/framework-skills/langchain-framework/references/langgraph-persistence.md-321-362 (1)
321-362: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the parallel-agent snippet runnable.
Both tabs call undefined
create_agent/createAgent,fruit_info/fruitInfo, andveggie_info/veggieInfo. Define/import them or label this as a partial fragment and link a complete example. As per coding guidelines, “Ensure all code snippets are tested and actually work before publishing.”🤖 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/framework-skills/langchain-framework/references/langgraph-persistence.md` around lines 321 - 362, Make the parallel-agent examples runnable by defining or importing create_agent/createAgent, fruit_info/fruitInfo, and veggie_info/veggieInfo before they are used in create_sub_agent/createSubAgent. If the surrounding document cannot provide valid implementations, label both snippets as partial fragments and link to a complete tested example instead.Source: Coding guidelines
plugins/nemo-experimentalist/framework-skills/langchain-framework/references/openinference-tracing.md-24-89 (1)
24-89: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the shipped OTLP JSONL schema.
This example emits one
ReadableSpan.to_json()per line, butexamples/terminal-bench-agent/tracing.pyserializesExportTraceServiceRequestbatches withencode_spans()andMessageToDict(). Update the exporter and output-format section together so consumers parse the actual trace file. Based on the providedplugins/nemo-experimentalist/examples/terminal-bench-agent/tracing.py:28-33contract and coding guideline “Ensure all code snippets are tested and actually work before publishing.”🤖 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/framework-skills/langchain-framework/references/openinference-tracing.md` around lines 24 - 89, Align _FileSpanExporter and the “Output format” section with the shipped tracing implementation: serialize ExportTraceServiceRequest batches using encode_spans() and MessageToDict(), rather than writing individual ReadableSpan.to_json() records. Document the resulting OTLP JSONL structure and ensure the shown code matches the tested contract in tracing.py.Source: Coding guidelines
plugins/nemo-experimentalist/tests/experimentalist/test_tools.py-13-22 (1)
13-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
sys.executableinstead of barepython.
pythonis absent on many CI images (python3 only), making this test environment-dependent.🔧 Proposed fix
+import sys + async def test_guarded_shell_tools_runs_allowed_commands(tmp_path): shell = GuardedShellTools(cwd=tmp_path) try: - result = await shell.run("python -", stdin="print('allowed')") + result = await shell.run(f"{sys.executable} -", stdin="print('allowed')") finally: await shell.close()🤖 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/experimentalist/test_tools.py` around lines 13 - 22, Update test_guarded_shell_tools_runs_allowed_commands to invoke the interpreter via sys.executable instead of the bare "python" command, adding the necessary sys import while preserving the existing stdin, output, return-code, and success assertions.plugins/nemo-experimentalist/tests/test_eval_author_run.py-190-190 (1)
190-190: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThis assertion locks in the unstaged-dataset behavior. Once
run_eval_authorstages train/validation, expect the stagedexperiment_dir/dataset/{train,validation}URIs here instead oftrain_ref/validation_ref.🤖 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_eval_author_run.py` at line 190, Update the assertion in the run_eval_author test to expect staged train and validation dataset URIs under experiment_dir/dataset/{train,validation} after staging, while preserving the existing ("harbor", ...) dataset reference structure.plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.py-25-25 (1)
25-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
/tmp/nonexistent.jsonlis a shared-filesystem assumption. If that path ever exists (another run, another user), theFileNotFoundErrortest silently stops testing what it claims. Derive it fromtmp_path.🛡️ Use tmp_path
-def test_concrete_dataset_raises_on_nonexistent_file(): - ref = DatasetRef(uri=_NONEXISTENT_URI, description="test") +def test_concrete_dataset_raises_on_nonexistent_file(tmp_path): + ref = DatasetRef(uri=str(tmp_path / "nonexistent.jsonl"), description="test") with pytest.raises(FileNotFoundError): ConcreteDataset.from_ref(ref)Also applies to: 82-85
🤖 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/experimentalist/test_evaluator_factory.py` at line 25, Update the FileNotFoundError test and its _NONEXISTENT_URI usage to derive the missing JSONL path from pytest’s tmp_path fixture instead of the shared /tmp/nonexistent.jsonl constant, ensuring the target is unique and does not exist before invoking the evaluator factory.Source: Linters/SAST tools
plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py-917-934 (1)
917-934: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHard-fails where
bashisn't onos.defpath.assert bash_path is not Noneturns an environment gap into a test failure; alsoSHELLOPTS=noexecis bash-specific. Skip instead.🛡️ Skip when bash is unavailable
`@pytest.mark.asyncio` -async def test_shell_noexec_environment_prevents_execution_without_n_flag() -> None: - bash_path = shutil.which("bash", path=os.defpath) - assert bash_path is not None +@pytest.mark.skipif( + shutil.which("bash", path=os.defpath) is None, + reason="bash not available on os.defpath", +) +async def test_shell_noexec_environment_prevents_execution_without_n_flag() -> None: + bash_path = shutil.which("bash", path=os.defpath) process = await asyncio.create_subprocess_exec(🤖 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/experimentalist/test_evaluator_harbor.py` around lines 917 - 934, Update test_shell_noexec_environment_prevents_execution_without_n_flag to skip rather than fail when shutil.which cannot find bash on os.defpath. Since the test depends on bash-specific SHELLOPTS=noexec behavior, use the test framework’s skip mechanism before subprocess creation when bash_path is unavailable.plugins/nemo-experimentalist/pyproject.toml-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing NVIDIA copyright / Apache-2.0 SPDX header.
Every file under
plugins/nemo-experimentalist/must carry it; the other new files in this PR do.As per coding guidelines: "Include the required NVIDIA copyright and Apache-2.0 SPDX header in every file."
🧹 Proposed fix
+# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + [project]🤖 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/pyproject.toml` at line 1, Add the required NVIDIA copyright notice and Apache-2.0 SPDX license header at the beginning of the pyproject.toml file, before the existing [project] section, matching the header format used by other files in this plugin.Source: Coding guidelines
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/models.py-264-267 (1)
264-267: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSort key guard checks the wrong segment.
The guard tests
split("-")[-1]but castssplit("-")[1]. A label likeagent-fix-2raisesValueErrorand kills table rendering.🐛 Proposed fix
for label in sorted( self.nodes.keys(), - key=lambda x: int(x.split("-")[1]) if x.split("-")[-1].isdigit() else 0, + key=lambda x: (int(tail) if (tail := x.rpartition("-")[2]).isdigit() else 0, x), ):🤖 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/models.py` around lines 264 - 267, Update the sorting key in the node-label loop of the model table rendering logic so the segment validated for numeric content is the same segment passed to int(). Ensure labels with additional hyphen-separated segments, such as agent-fix-2, do not raise during sorting.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/tools.py-169-169 (1)
169-169: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a concrete annotation instead of the string literal.
"str | int"should bestr | int.As per coding guidelines: "In Python code, prefer concrete type hints over string-based type hints".
📝 Proposed fix
- def read_analysis_file(self, path_or_round: "str | int", limit: int | None = 8000) -> str: + def read_analysis_file(self, path_or_round: str | int, limit: int | None = 8000) -> str:🤖 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/tools.py` at line 169, Update the read_analysis_file method signature to use the concrete str | int type annotation instead of the quoted string literal, leaving the rest of the signature unchanged.Source: Coding guidelines
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py-43-62 (1)
43-62: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring contradicts the implementation.
The docstring claims failed trials contribute 0 and the denominator is always
len(results). The code excludes failed trials entirely and divides bylen(completed), so failures do not count against the aggregate — and an all-failed run returns{}(confirmed bytests/experimentalist/test_evaluator_base.py). Misleading for anyone reasoning about selection scores.📝 Proposed docstring fix
- Defaults to averaging each metric across all trials, treating trials that - did not emit a metric (e.g. failed trials) as contributing 0. The denominator - is always ``len(results)``, not the number of trials that reported each metric, - so failure counts against the aggregate score. + Defaults to averaging each metric across non-failed trials only; failed + trials are excluded from both numerator and denominator, so failures do + not drag the aggregate down. Returns ``{}`` when every trial failed. + All non-failed trials must report the same metric keys.🤖 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/base.py` around lines 43 - 62, The aggregation docstring for the evaluator method should match the implementation: state that failed trials are excluded from aggregation and metrics are averaged over completed trials only, with an all-failed input returning an empty dictionary. Update the related parameter and return descriptions if needed, without changing the aggregation logic.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py-38-41 (1)
38-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring documents a nonexistent
datasetfield.Fields are
train_dataset,validation_dataset,task_template;evaluator_typeandagent_specare undocumented.🤖 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/deps.py` around lines 38 - 41, Update the relevant class docstring to document the actual fields train_dataset, validation_dataset, task_template, evaluator_type, and agent_spec; remove the nonexistent dataset entry and accurately describe each field’s purpose and default where applicable.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/result.py-32-35 (1)
32-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
min_length=1can turn a missing run id into a late ValidationError.
loop._runbuilds this withrun_id = run_entity.id or ""(Line 548 there). If a backend ever returns a run without an id, the whole optimization completes and then dies constructing the result. Either drop the constraint or fail fast right aftercreate_run.🤖 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/result.py` around lines 32 - 35, Address the missing run ID in the result construction flow: after create_run returns, validate that the run entity has a non-empty id and fail immediately before optimization proceeds, while preserving the existing run_id field constraint. Update the code around the run creation logic in loop._run rather than allowing run_id = run_entity.id or "" to defer failure to Result validation.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/dataset_staging.py-31-35 (1)
31-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_stagesilently rejects Fileset-backed train/validation refs.
stage_task_templatehandlesfileset://URIs, but_stageroutes everything through_local_directory, which raises for a non-local scheme. SinceDatasetReffor train/validation can legitimately carry a fileset URI (same type astask_template), insight-mode runs with fileset datasets fail at staging with a confusing "not a directory" error. Either route both through the download path or raise an explicit unsupported-scheme error.🤖 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/dataset_staging.py` around lines 31 - 35, Update _stage to handle fileset:// DatasetRef URIs consistently with stage_task_template instead of always calling _local_directory; route fileset-backed refs through the existing download/staging path, or explicitly reject unsupported schemes with a clear error before local-directory resolution. Preserve the current local-directory copy behavior and returned DatasetRef update for supported local refs.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py-1497-1502 (1)
1497-1502: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
isinstance(r, Exception)missesBaseExceptionresults.
asyncio.gather(return_exceptions=True)also returnsBaseExceptioninstances (e.g.asyncio.CancelledError). A cancelled Coder task is then treated as a successful implementation, so the candidate survives with unmodified code and is evaluated as a real variant.🐛 Proposed fix
- failed = {c.name for c, r in zip(candidates, results, strict=True) if isinstance(r, Exception)} + failed = {c.name for c, r in zip(candidates, results, strict=True) if isinstance(r, BaseException)}🤖 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/loop.py` around lines 1497 - 1502, Update the failed-candidate comprehension after the gather call to recognize all BaseException results, including asyncio.CancelledError, rather than only Exception instances. Ensure cancelled or otherwise base-level failures are added to failed so their candidates are not evaluated as successful variants.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/otlp.py-55-67 (1)
55-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNaive ISO timestamps are interpreted as local time.
datetime.fromisoformat("2026-01-01T00:00:00").timestamp()uses the host timezone, so tz-less Intake timestamps get shifted by the local UTC offset before being emitted as unix nanos — spans land at the wrong wall-clock time depending on where the run executes.🐛 Proposed fix
+from datetime import UTC, datetime ... try: dt = datetime.fromisoformat(str(value).replace("Z", "+00:00")) except ValueError: return None + if dt.tzinfo is None: + dt = dt.replace(tzinfo=UTC) return str(int(dt.timestamp() * 1_000_000_000))🤖 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/otlp.py` around lines 55 - 67, Update _to_unix_nano so ISO timestamps without timezone information are interpreted as UTC before calling timestamp(), while preserving explicit timezone offsets and existing handling for epoch values and invalid inputs.
🧹 Nitpick comments (24)
plugins/nemo-experimentalist/framework-skills/langchain-framework/references/langchain-rag.md (1)
42-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the core import here.
langchain_community.vectorstores.InMemoryVectorStoreis legacy/back-compat; the current LangChain docs point tolangchain_core.vectorstores.InMemoryVectorStore.Proposed fix
-from langchain_community.vectorstores import InMemoryVectorStore +from langchain_core.vectorstores import InMemoryVectorStore🤖 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/framework-skills/langchain-framework/references/langchain-rag.md` around lines 42 - 45, Update the InMemoryVectorStore import in the LangChain RAG reference to use langchain_core.vectorstores instead of the legacy langchain_community.vectorstores module; leave the other imports unchanged.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/materialization.py (1)
93-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOpaque
IndexErrorwhen the template dir isn't a valid Harbor task.list(...)[0]on an empty listing crashes with no context. Raise a descriptive error namingtask_dir.♻️ Proposed guard
- task = list(HarborDataset.from_path(task_dir).list_tasks())[0] + staged_tasks_found = list(HarborDataset.from_path(task_dir).list_tasks()) + if not staged_tasks_found: + raise ValueError(f"Staged task template contains no Harbor task: {task_dir}") + task = staged_tasks_found[0]🤖 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/eval_author/materialization.py` around lines 93 - 94, Replace the direct first-element access in the materialization flow with an explicit check of the tasks returned by HarborDataset.from_path(task_dir).list_tasks(). If no tasks are found, raise a descriptive error that includes task_dir; otherwise preserve the existing task selection and StagedInsightTask construction.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/agent.py (1)
361-393: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPublished Fileset leaks when augmentation or validation fails.
publish_filesetsucceeds at Line 361, thenaugment_dataset/_validate_eval_author_resultcan raise for the remainder of_run— the remote Fileset stays behind with no owner and no cleanup path. Either publish after augmentation succeeds, or wrap Lines 363-395 so the Fileset is deleted on failure.🤖 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/eval_author/agent.py` around lines 361 - 393, Ensure the Fileset published by insight_suite.publish_fileset is cleaned up whenever augment_dataset or _validate_eval_author_result fails before _run completes. Either move publishing until after successful augmentation and validation, or wrap the existing post-publication workflow in failure cleanup that deletes the published Fileset before propagating the exception.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/models.py (1)
13-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBound
max_summary_tokensandmax_tracestoo.
max_validation_repair_attemptsis constrained but the other two accept0/negative, silently disabling summarization or trace analysis.♻️ Add lower bounds
max_summary_tokens: int = Field( default=80_000, + gt=0, description="Max tokens the token-budget summarizer may use.", ) max_traces: int = Field( default=10, + ge=1, description="Max trace refs from the insight to analyze in depth.", )🤖 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/eval_author/models.py` around lines 13 - 26, Add lower-bound validation to the max_summary_tokens and max_traces fields in the relevant model, ensuring both values are at least 1 so summarization and trace analysis cannot be disabled by zero or negative inputs. Keep their existing defaults, descriptions, and max_validation_repair_attempts constraints unchanged.plugins/nemo-experimentalist/tests/test_experiment_mirror.py (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the leftover
# VERIFY-2marker.🧹 Cleanup
-from nemo_platform import ConflictError, NotFoundError, omit # VERIFY-2 +from nemo_platform import ConflictError, NotFoundError, omit🤖 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_experiment_mirror.py` at line 10, Remove the leftover “# VERIFY-2” comment from the import statement in test_experiment_mirror.py while preserving the existing ConflictError, NotFoundError, and omit imports.plugins/nemo-experimentalist/tests/test_resolve.py (1)
424-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win1s
wait_fortimeouts will flake on loaded CI runners. Applies to Lines 436, 439, 469, and 619 too. Bump to ~5-10s; these guard against hangs, not latency.🤖 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_resolve.py` around lines 424 - 455, Increase the asyncio.wait_for timeouts in test_concurrent_identical_refs_publish_one_complete_winner and the other affected tests at the referenced wait_for calls to approximately 5–10 seconds. Keep these timeouts as hang guards while allowing normal execution on loaded CI runners.plugins/nemo-experimentalist/tests/test_otlp.py (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnneeded
from __future__ import annotations— no forward references here, and it makes every annotation lazily string-evaluated. Drop it.As per coding guidelines: "In Python code, prefer concrete type hints over string-based type hints".
🤖 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_otlp.py` at line 10, Remove the unnecessary from __future__ import annotations statement from test_otlp.py, leaving the existing concrete type annotations unchanged.Source: Coding guidelines
plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_base.py (2)
47-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth entries use
id="trial1". Second should be"trial2"— duplicate trial ids make the fixture misleading.🤖 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/experimentalist/test_evaluator_base.py` around lines 47 - 63, Update the second TrialResult in _FAILED_TRIAL_RESULTS to use id="trial2" instead of duplicating "trial1"; leave the first fixture entry unchanged.
215-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two tests only assert
result.id, so neither actually covers options resolution.
test_run_with_dict_options_mergespasses if the dict were silently ignored. Capture the resolved options in_runand assert them.♻️ Assert the resolved options
`@pytest.mark.asyncio` async def test_run_with_dict_options_merges(): class ConcreteDataset(Dataset): `@classmethod` def from_ref(cls, ref): return cls(id="test") - evaluator = ConcreteEvaluator(options=EvaluatorConfig(force_rerun=False)) + seen: list[EvaluatorConfig] = [] + + class RecordingEvaluator(ConcreteEvaluator): + async def _run(self, agent: Path, dataset: Dataset, options: EvaluatorConfig) -> Sequence[TrialResult]: + seen.append(options) + return await super()._run(agent, dataset, options) + + evaluator = RecordingEvaluator(options=EvaluatorConfig(force_rerun=False)) dataset = ConcreteDataset(id="ds") result = await evaluator.run( agent=Path("/tmp/agent"), dataset=dataset, options={"force_rerun": True}, ) assert result.id == "agent-ds" + assert seen[0].force_rerun is 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/experimentalist/test_evaluator_base.py` around lines 215 - 241, Update both tests around ConcreteEvaluator.run to capture the options received by the evaluator’s _run method, then assert that None uses the evaluator’s default EvaluatorConfig and the dictionary input merges with the defaults, including force_rerun=True. Keep the existing result.id assertions while adding assertions that verify the resolved options rather than only successful execution.plugins/nemo-experimentalist/tests/test_profile.py (1)
104-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion reduces to
profile_dir == tmp_path.resolve(), already covered at Line 67 — the.nemo-optimizer/insights.yamljoin is purepathlib. Also re-implementswrite_profilewith a literal filename. Either drop it or assert against the shared insights-path helper.🤖 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_profile.py` around lines 104 - 111, Remove the redundant path-joining assertion in test_profile_directory_anchors_shared_insights_path, or replace it with an assertion that exercises the shared insights-path helper directly. Avoid duplicating write_profile’s literal filename/path construction while preserving coverage of the helper’s behavior.plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.py (1)
56-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSingle-case
parametrizeadds indirection for no coverage. Inline it.🤖 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/experimentalist/test_evaluator_factory.py` around lines 56 - 66, Remove the single-case pytest.mark.parametrize wrapper from test_build_dataset_raises_on_invalid_type and inline its evaluator type, DatasetRef, and expected ValueError directly in the test. Preserve the existing DatasetFactory configuration and assertion behavior.plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py (2)
1276-1279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertion-free smoke tests.
test_chmod_path_chainonly checks "doesn't raise"; addassert (deep.stat().st_mode & 0o777) == 0o777and the same fortmp_path / "a"(and confirmtmp_pathitself was untouched). Same gap intest_harbor_dependency_context_stop_started_runtime_no_environment(Line 1115),test_harbor_dependency_context_stop_runtime_with_stop_command(Line 1714), andtest_run_dependency_command_success(Line 1481).🤖 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/experimentalist/test_evaluator_harbor.py` around lines 1276 - 1279, Strengthen the assertion-free smoke tests, including test_chmod_path_chain, test_harbor_dependency_context_stop_started_runtime_no_environment, test_harbor_dependency_context_stop_runtime_with_stop_command, and test_run_dependency_command_success, by asserting the expected filesystem or command outcome rather than only checking that no exception is raised. In test_chmod_path_chain, verify deep and tmp_path / "a" have mode 0o777 and confirm tmp_path itself remains unchanged.
1136-1273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win~200 lines of copy-pasted Harbor runtime scaffolding across three tests.
FakeEnv/FakeEnvPaths/FakeTrialPaths/FakeHarborTaskplus the fourmonkeypatch.setattrcalls are identical except forcapabilities.mountedandhealthcheck. Extract a fixture/helper parameterized on those two knobs.Also applies to: 1766-1842
🤖 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/experimentalist/test_evaluator_harbor.py` around lines 1136 - 1273, Extract the duplicated Harbor runtime scaffolding from the three affected tests, including FakeEnv, FakeEnvPaths, FakeTrialPaths, FakeHarborTask, and the four Harbor monkeypatches, into a reusable fixture or helper. Parameterize it with capabilities.mounted and healthcheck, then update test_harbor_dependency_context_start_runtime, its mounted variant, and the additional test around the later duplicate block to use it while preserving each test’s existing configuration.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_explorer.py (2)
1857-1868: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated span-subtree walk.
_session_span_idsandget_raw_spansimplement the same O(n²) closure loop. Haveget_raw_spanscall_session_span_ids.Also applies to: 2223-2237
🤖 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/trace_explorer.py` around lines 1857 - 1868, Update get_raw_spans to reuse _session_span_ids for collecting the session span subtree, removing its duplicated closure loop while preserving the existing filtering and return behavior.
863-889: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSerial per-session pagination, and it runs even when the trace has no spans.
_fetch_intake_eval_contextsawaits one paginator per session id sequentially — latency scales linearly with session count. Also,from_intakefetches eval contexts before the empty-spans check, so a missing trace pays the full round trip.♻️ Proposed changes
async for item in paginator: @@ + if not spans: + raise ValueError(f"No spans found in Intake for trace: {trace_id}") + eval_contexts = await _fetch_intake_eval_contexts( client=client, workspace=workspace, session_ids=session_ids, page_size=page_size, ) - - if not spans: - raise ValueError(f"No spans found in Intake for trace: {trace_id}")Then fan the per-session queries out with
asyncio.gatherinside_fetch_intake_eval_contexts, deduplicating after the gather to keep results stable.Also applies to: 1379-1387
🤖 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/trace_explorer.py` around lines 863 - 889, Update _fetch_intake_eval_contexts to run each session’s evaluator-results pagination concurrently via asyncio.gather, then merge the per-session results in sorted session order and perform deduplication afterward so output remains stable. In from_intake, move the empty-spans early return before fetching intake evaluation contexts, avoiding the request when the trace contains no spans.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py (1)
78-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReaching into
tree._iter_nodes()breaks the abstraction this class exists to avoid.
validate_treedeliberately stays import-free of the agent module, then depends on a private method of the runtimeGoalTree. Expose a public iterator onGoalTreeinstead.🤖 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/resolve.py` around lines 78 - 98, Update validate_tree to stop calling the private tree._iter_nodes() method; add or use a public iterator on GoalTree for traversing all nodes, and use that public API when collecting initial_nodes while preserving the existing validation rules.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.py (1)
73-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
EvaluatorFactoryignores the injection seamDatasetFactoryprovides.
DatasetFactoryacceptssupported_evaluator_types;EvaluatorFactoryhardcodes the module dict. Mirror the constructor so both resolve through one registry.♻️ Proposed refactor
class EvaluatorFactory: """Build concrete evaluators from evaluator type.""" + def __init__( + self, + supported_evaluator_types: dict[EvaluatorType, tuple[type[Dataset], type[Evaluator], type[EvaluatorConfig]]] + | None = None, + ) -> None: + self.supported_evaluator_types = supported_evaluator_types or _SUPPORTED_EVALUATOR_TYPES + def build_evaluator( @@ - if evaluator_type in _SUPPORTED_EVALUATOR_TYPES: + if evaluator_type in self.supported_evaluator_types: if isinstance(config, EvaluatorConfig): config = config.model_dump() elif not isinstance(config, dict): raise TypeError(f"{evaluator_type.capitalize()} evaluator config must be an EvaluatorConfig or dict") - evaluator_config = _SUPPORTED_EVALUATOR_TYPES[evaluator_type][2].model_validate(config) - return _SUPPORTED_EVALUATOR_TYPES[evaluator_type][1]( - options=evaluator_config, experiment_dir=experiment_dir - ) + _, evaluator_cls, config_cls = self.supported_evaluator_types[evaluator_type] + return evaluator_cls(options=config_cls.model_validate(config), experiment_dir=experiment_dir)🤖 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/factory.py` around lines 73 - 107, Update EvaluatorFactory to accept a supported_evaluator_types registry through its constructor, matching DatasetFactory’s injection seam, and store it on the instance. Replace build_evaluator’s direct references to the module-level _SUPPORTED_EVALUATOR_TYPES with the injected registry while preserving validation, lookup, and error behavior.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/models.py (1)
196-204: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA single incompatible
metadata.jsonaborts tree rebuild.Corrupt JSON and I/O errors are tolerated, but
ValidationErrorfrommodel_validatepropagates and breaks resume. Skip and log instead.🤖 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/models.py` around lines 196 - 204, Update the tree rebuild flow around Candidate.model_validate to catch ValidationError for malformed metadata, log the skipped metadata file, and continue processing remaining candidates. Preserve the existing handling of JSON and I/O errors and only add this tolerance around model validation.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.py (1)
160-194: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
ancestoragainst survivor labels.Every other field is checked, but an invented
ancestorslips through and only fails later in the coder as a missing agent directory.♻️ Proposed fix
self._validate_improvements( improvements=improvements, max_candidates=max_candidates, allowed_types=set(available_types) or all_types, + allowed_ancestors={s.label for s in proposal_survivors}, )`@staticmethod` def _validate_improvements( *, improvements: list[Improvement], max_candidates: int, allowed_types: set[str], + allowed_ancestors: set[str], ) -> None: @@ for improvement in improvements: + if improvement.ancestor not in allowed_ancestors: + raise ValueError( + f"Proposer returned unknown ancestor {improvement.ancestor!r}; " + f"survivors: {sorted(allowed_ancestors)}" + ) optimization_type = improvement.optimization_type🤖 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/proposer.py` around lines 160 - 194, Update _validate_improvements to validate each improvement’s ancestor against the available survivor labels before accepting it. Reuse the existing survivor-label source or pass the allowed labels into the validator, and raise a clear ValueError for an unknown ancestor while preserving the current optimization type and description checks.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/cards.py (1)
1322-1331: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin
encoding="utf-8"on the catalog reads.Default locale decoding can break the YAML on non-UTF-8 systems.
♻️ Proposed fix
if self._model_catalog_path is not None: - raw_catalog = self._model_catalog_path.read_text() + raw_catalog = self._model_catalog_path.read_text(encoding="utf-8") else: catalog_ref = resources.files("nemo_experimentalist_plugin").joinpath("assets/models.yaml") try: - raw_catalog = catalog_ref.read_text() + raw_catalog = catalog_ref.read_text(encoding="utf-8") except FileNotFoundError: catalog_path = Path(__file__).resolve().parents[2] / "assets" / "models.yaml" - raw_catalog = catalog_path.read_text() + raw_catalog = catalog_path.read_text(encoding="utf-8")🤖 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/cards.py` around lines 1322 - 1331, Update the catalog reads in the model-catalog loading logic to pass encoding="utf-8" for both self._model_catalog_path.read_text() and catalog_ref.read_text(), and also for the fallback catalog_path.read_text() call. Keep the existing fallback and validation flow unchanged.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/model_config.py (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefault model ids duplicated between the factories and the log banner.
Six literals, three pairs. Change one and the banner silently reports the wrong model. Extract constants.
♻️ Proposed refactor
+DEFAULT_SMART_MODEL = "openai/openai/openai/gpt-5.5" +DEFAULT_MID_MODEL = "openai/gcp/google/gemini-3.5-flash" +DEFAULT_FAST_MODEL = "openai/openai/openai/gpt-5-mini" + + def _required_env(name: str) -> str:- name = _optional_env("EXPERIMENTALIST_SMART_MODEL_NAME", "openai/openai/openai/gpt-5.5") + name = _optional_env("EXPERIMENTALIST_SMART_MODEL_NAME", DEFAULT_SMART_MODEL)Apply the same substitution to the mid/fast factories and to all three reads in
log_model_config.Also applies to: 51-51, 68-68, 93-95
🤖 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/model_config.py` at line 34, Extract shared constants for the default smart, mid, and fast model IDs in model_config.py, then use those constants in the corresponding environment lookups and all three model reads in log_model_config. Remove the duplicated string literals while preserving the existing configuration behavior.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/tools.py (1)
198-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared candidate deserializer here
This only needs JSON→
Candidateparsing; avoid a dummyLocalExperimentalistBackendinstance and call the shared_load_entity(Candidate, path)helper instead.🤖 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/tools.py` around lines 198 - 209, Update the metadata-loading flow to use the shared _load_entity helper with Candidate and path directly, removing the dummy LocalExperimentalistBackend construction, _eo assignment, and _load_candidate call. Preserve the existing missing-file validation and return the deserialized Candidate.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py (1)
1586-1592: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded fan-out:
len(nodes) × len(tasks)LLM scorer calls in onegather.
max_trajectory_taskscaps tasks but not leaves, so a 4-leaf tree × 50 tasks issues 200 concurrent scoring agents, each loading traces. Consider a semaphore bounded by a config value.🤖 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/loop.py` around lines 1586 - 1592, Bound the concurrent GroupLeafScorer calls in the scoring flow around GroupLeafScorer.run and asyncio.gather using a semaphore configured by the appropriate max-concurrency setting, while preserving the existing node/task combinations and scoring results. Ensure each scoring task acquires and releases the semaphore so fan-out is limited rather than launching all combinations concurrently.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py (1)
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
EvolutionaryOptimizerConfiginstead ofAnyforExperimentalistDeps.config.
resolve.pydefines the type, and there’s noresolve -> depsimport, so this can be concrete without a cycle.🤖 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/deps.py` at line 59, Update ExperimentalistDeps.config to use EvolutionaryOptimizerConfig instead of Any, importing the type from resolve.py now that no resolve-to-deps cycle exists. Preserve the existing optional None default and remove the workaround comment.Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (9)
plugins/nemo-experimentalist/framework-skills/langchain-framework/references/deep-agents-memory.md (1)
69-74: 🎯 Functional Correctness | 🟠 MajorRemove deprecated backend factories from every example.
Use preconstructed
StateBackend()/StoreBackend()instances and pass them directly tocreate_deep_agent; update the corresponding explanatory snippets too. The factory form is deprecated and may break after an upgrade. (docs.langchain.com)Also applies to: 224-227, 269-271, 319-320
🤖 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/framework-skills/langchain-framework/references/deep-agents-memory.md` around lines 69 - 74, Replace deprecated backend factory lambdas in all affected examples, including the composite backend example and the referenced sections, with preconstructed StateBackend() and StoreBackend() instances passed directly to create_deep_agent. Update the accompanying explanatory snippets to describe the direct-instance pattern and preserve each example’s existing routing behavior.plugins/nemo-experimentalist/benchmarks/README.md (1)
58-75: 🎯 Functional Correctness | 🟠 MajorFix the benchmark runner paths.
Use the provided
benchmarks/run.pyentry point andbenchmarks/configs/{smoke,quality}.yamlpaths; the current commands reference nonexistent paths.Proposed fix
-uv run python benchmarks/experimentalist/run.py --validate-only +uv run python benchmarks/run.py --validate-only - --config benchmarks/experimentalist/configs/smoke.yaml + --config benchmarks/configs/smoke.yaml - --config benchmarks/experimentalist/configs/quality.yaml + --config benchmarks/configs/quality.yaml🤖 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/benchmarks/README.md` around lines 58 - 75, Update all benchmark commands in the README to invoke the provided benchmarks/run.py entry point and reference benchmarks/configs/smoke.yaml or benchmarks/configs/quality.yaml, including the validate-only command without a configuration path.plugins/nemo-experimentalist/framework-skills/langchain-framework/references/deep-agents-orchestration.md (1)
9-15: 🎯 Functional Correctness | 🟠 MajorDo not claim HITL is enabled by default.
HumanInTheLoopMiddlewareis configured throughinterrupt_on; approval workflows also require a checkpointer. The current wording can make readers believe sensitive tools are gated automatically. (docs.langchain.com)🤖 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/framework-skills/langchain-framework/references/deep-agents-orchestration.md` around lines 9 - 15, Update the Deep Agents orchestration description to remove the claim that HumanInTheLoopMiddleware is automatically included or enabled by default in create_deep_agent(). State that HITL approval requires configuring interrupt_on and a checkpointer, while preserving the descriptions of SubAgentMiddleware and TodoListMiddleware.plugins/nemo-experimentalist/framework-skills/langchain-framework/SKILL.md (1)
370-388: 🎯 Functional Correctness | 🟠 MajorUse a skills directory, not
Skill(...).This example uses the obsolete object-based API. Deep Agents expects
skillsto contain directories withSKILL.mdfiles, so copied code will not load the skill. (docs.langchain.com)🤖 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/framework-skills/langchain-framework/SKILL.md` around lines 370 - 388, Update the “Skills” example to use a skills directory containing SKILL.md files instead of importing or instantiating the obsolete Skill object. Adjust the create_deep_agent configuration to pass the directory through skills while preserving the example’s intended policy-loading behavior, and remove the read_policy and Skill-based setup.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_scorer.py (2)
117-121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
explorer.get_spans()does not exist.
TraceExplorerexposesget_span_id(session_id, turn_index),get_raw_span(span_id),get_raw_spans(session_id)— noget_spans(). This docstring is the CodeAct prompt, so the agent will call a missing method and fall back to guessed span ids.🤖 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/trace_scorer.py` around lines 117 - 121, Update the CodeAct prompt in the trace-scoring component to reference TraceExplorer’s available span APIs, especially get_raw_spans(session_id) or get_span_id(session_id, turn_index), instead of the nonexistent explorer.get_spans(). Require span_ids to be copied verbatim from retrieved span objects and include only spans directly supporting the score.
126-135: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOne unloadable trace aborts the whole scoring group; contradicts Line 84.
from_refraisesValueErrorfor anintake://ref when_client/_nmp_workspaceisNone(trace_explorer.pyLine 1349). That propagates out ofrunand, viaasyncio.gatherin the loop, kills all trajectory scoring. Wrap per-trial loading, log via the modulelogger(defined Line 19, currently unused) instead of🤖 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/trace_scorer.py` around lines 126 - 135, Update the per-trial loading loop in the scorer’s run flow to catch failures from TraceExplorer.from_ref, log the agent and exception with the module logger, and continue scoring the remaining trials. Replace the diagnostic print calls with logger usage while preserving successful explorer processing and the skip for trials without traces.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/otlp.py (1)
83-111: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winO(n²) serialization — the whole batch is re-encoded per span.
Each iteration re-serializes the accumulated batch, so a 5k-span trace does millions of span encodings. Encode each
resourceSpansentry once and accumulate its byte size.🤖 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/otlp.py` around lines 83 - 111, Update _serialize_chunks to serialize each resourceSpans entry once, then accumulate encoded sizes while building payload batches instead of re-encoding the entire batch on every iteration. Preserve max_bytes splitting, single-entry oversized warnings, and final payload generation using the existing ExportTraceServiceRequest format.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/experimentalist_backend.py (1)
297-298: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemote mode still can't run insight-driven optimization.
_filesis aLocalExperimentalistBackendbuilt with the platform client (Line 295) and itsget_insight(Line 455) already resolves platform ids. Raising here makes remote strictly weaker than local+client and hard-fails the loop wheneverdeps.insightis set. Delegate like every other method.🤖 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/experimentalist_backend.py` around lines 297 - 298, Update RemoteExperimentalistBackend.get_insight to delegate to its _files LocalExperimentalistBackend instance, passing workspace and insight_id, instead of raising NotImplementedError. Preserve the async Insight return behavior and match the delegation pattern used by the other backend methods.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/repository.py (1)
178-209: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGit option injection via
url/refstill unaddressed.
agent_pathis validated buturl(Line 181),ref(Line 204), andbranch/base_refinpush_branch/publish(Lines 358-359) reach git as positional args. A leading-turns them into options (--upload-pack=,--config=core.sshCommand=) → command execution. Add--where git accepts it and reject leading-dash values.🤖 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/repository.py` around lines 178 - 209, Prevent Git option injection in the repository operations: reject leading-dash values for url, ref, branch, and base_ref before constructing commands. Update run_git callers such as clone_cmd, push_branch, and publish to pass the Git argument separator where supported, including before repository refs and branch names, while preserving normal Git behavior.Source: Linters/SAST tools
🟡 Minor comments (12)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/skills/README.md-6-10 (1)
6-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd prerequisites and Next Steps.
State the required plugin/CLI installation before
nemo skills install, then add a final link to the plugin README or Terminator skill. As per coding guidelines, “Always list prerequisites at the top” and “Include 'Next Steps' section at the end.”🤖 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/skills/README.md` around lines 6 - 10, Update the skills README introduction to list the required plugin and CLI installation prerequisites before describing or invoking nemo skills install. Add a final “Next Steps” section containing a link to the plugin README or the terminator skill, while preserving the existing skill-discovery guidance.Source: Coding guidelines
plugins/nemo-experimentalist/framework-skills/nooa/SKILL.md-20-22 (1)
20-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace obsolete
Optimizerterminology.Use “Experimentalist” here; plugin files must not restore Optimizer names. As per coding guidelines, “Use the Experimentalist product name and paths … Do not restore Optimizer names or compatibility aliases.”
🤖 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/framework-skills/nooa/SKILL.md` around lines 20 - 22, Replace the obsolete “Optimizer” terminology in the framework behavior guidance with “Experimentalist,” including the heading and any related references in this section. Preserve the instruction to inspect the pinned implementation or matching upstream skill before changing behavior, and do not add compatibility aliases or Optimizer paths.Source: Coding guidelines
plugins/nemo-experimentalist/examples/terminal-bench-agent/agent.py-63-66 (1)
63-66: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle the timeout-kill race.
The child can exit after
TimeoutExpiredbut beforekillpg, causingProcessLookupErrorand failing the agent instead of returning bounded timeout output.Proposed fix
except subprocess.TimeoutExpired: - os.killpg(process.pid, signal.SIGKILL) + try: + os.killpg(process.pid, signal.SIGKILL) + except ProcessLookupError: + pass stdout, stderr = process.communicate()🤖 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/examples/terminal-bench-agent/agent.py` around lines 63 - 66, Update the TimeoutExpired handling around process.killpg and process.communicate so a child that exits before termination does not raise ProcessLookupError. Catch and ignore that specific race, then still collect the bounded output and append the timeout message before returning the agent result.plugins/nemo-experimentalist/pyproject.toml-1-4 (1)
1-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing SPDX header.
As per coding guidelines, "Every file must contain the required NVIDIA SPDX copyright header and Apache-2.0 license identifier."
📄 Proposed fix
+# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + [project]🤖 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/pyproject.toml` around lines 1 - 4, Add the required NVIDIA SPDX copyright header and Apache-2.0 license identifier at the top of the project metadata file, before the existing [project] section. Preserve all current project fields and values unchanged.Source: Coding guidelines
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py-784-786 (1)
784-786: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocstring contradicts
_trial_metrics: booleans are not dropped.
_trial_metricscallsfloat(value), sotruebecomes1.0. This text is the contract shown to the agent authoringreward.json, so the claim is actively misleading.📝 Proposed fix
- - ``/logs/verifier/reward.json`` — flat JSON object; **every value must be a - plain number** (int or float). Harbor calls ``float(value)`` on each entry - and silently drops non-numeric values (nested objects, booleans, strings). + - ``/logs/verifier/reward.json`` — flat JSON object; **every value must be a + plain number** (int or float). Each entry is passed to ``float(value)``; + values that cannot be converted (nested objects, non-numeric strings) are + silently dropped, and booleans convert to ``1.0``/``0.0``.🤖 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 784 - 786, Update the reward.json docstring near _trial_metrics to accurately state that values convertible by float(value), including booleans, are accepted and converted to numeric values; retain the description of nested objects and other non-convertible values being dropped.plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py-917-934 (1)
917-934: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGuard the real-bash test with
skipif.assert bash_path is not Noneturns a missing/bin/bash(slim images, non-glibc CI) into a failure rather than a skip.💚 Proposed fix
+@pytest.mark.skipif(shutil.which("bash", path=os.defpath) is None, reason="bash unavailable on system path") `@pytest.mark.asyncio` async def test_shell_noexec_environment_prevents_execution_without_n_flag() -> None: bash_path = shutil.which("bash", path=os.defpath) - assert bash_path is not 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 `@plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py` around lines 917 - 934, Guard test_shell_noexec_environment_prevents_execution_without_n_flag with pytest.mark.skipif based on bash availability, and remove the unconditional bash_path assertion. The test should skip when shutil.which("bash", path=os.defpath) returns None while continuing to execute normally when bash is available.plugins/nemo-experimentalist/tests/experimentalist/test_tools.py-13-22 (1)
13-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBare
pythonmay not exist on the test host. Many Linux images ship onlypython3, so this asserts on shell availability rather than the guard logic.💚 Proposed fix
+import sys + ... - result = await shell.run("python -", stdin="print('allowed')") + result = await shell.run(f"{sys.executable} -", stdin="print('allowed')")🤖 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/experimentalist/test_tools.py` around lines 13 - 22, Update test_guarded_shell_tools_runs_allowed_commands to invoke the interpreter through the repository’s portable Python executable reference, such as sys.executable, instead of relying on the bare “python” command; keep the allowed-command assertions and cleanup behavior unchanged.plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.py-36-39 (1)
36-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the string annotation.
♻️ Proposed fix
+from __future__ import annotations`@classmethod` - def from_ref(cls, ref: DatasetRef) -> "ConcreteDataset": + def from_ref(cls, ref: DatasetRef) -> ConcreteDataset:As per coding guidelines: "In Python code, prefer concrete type hints over string-based type hints".
🤖 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/experimentalist/test_evaluator_factory.py` around lines 36 - 39, Replace the string-based return annotation in ConcreteDataset.from_ref with the concrete ConcreteDataset type annotation, preserving the method’s existing behavior and classmethod signature.Source: Coding guidelines
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/proposer.py-187-189 (1)
187-189: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidation contradicts the prompt. Lines 272-274 tell the model duplicate axes are permitted "when no other viable direction exists", but this raises unconditionally. Align one side or the round fails on a compliant response.
🤖 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/proposer.py` around lines 187 - 189, Align the duplicate optimization_type validation in the proposer response handling with the prompt’s allowance for duplicates when no other viable direction exists. Update the logic around seen_types so duplicate axes are accepted only under that stated fallback condition, while preserving rejection of duplicates when another viable direction is available.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/goal_tree.py-437-441 (1)
437-441: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrompt references a nonexistent
task_idvariable.
_generatetakes onlydatasetandagent_spec;task_idis not in scope, so the CodeAct cell will eitherNameErroror the model will invent a value. Point it at the dataset instead.🐛 Proposed fix
- Return a GoalTree(task_id=task_id, root=GoalNode(...)). The framework validates + Return a GoalTree(task_id=dataset.id, root=GoalNode(...)). The framework validates🤖 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/goal_tree.py` around lines 437 - 441, Update the output instructions in _generate to reference the available dataset rather than the nonexistent task_id variable. Ensure the requested GoalTree uses the dataset-derived identifier expected by the surrounding implementation, without introducing or asking the model to invent a task_id.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/experimentalist_backend.py-760-762 (1)
760-762: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBare
intake://<id>refs yield a malformed trace id.
removeprefix("intake://traces/")leavesintake://abcuntouched for the bare form thatTraceExplorer.from_refexplicitly supports (trace_explorer.pyLines 1342-1346), soretrievegets a URI instead of an id. Chain both prefixes asfrom_refdoes.🐛 Proposed fix
- trace_id = uri.removeprefix("intake://traces/") + trace_id = uri.removeprefix("intake://").removeprefix("traces/")🤖 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/experimentalist_backend.py` around lines 760 - 762, Update the URI handling in the retrieve flow around _retrieve_trace_with_retry so intake://traces/<id> and bare intake://<id> references both produce only the trace ID. Chain the same prefix-removal behavior used by TraceExplorer.from_ref, preserving the existing workspace argument and retry call.plugins/nemo-experimentalist/tests/test_cli_profile.py-575-583 (1)
575-583: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the Optimizer-prefixed test env var.
NEMO_OPT_TEST_DOTENVkeeps the obsolete prefix.♻️ Proposed rename
- monkeypatch.delenv("NEMO_OPT_TEST_DOTENV", raising=False) - (profile_tree / ".env").write_text("NEMO_OPT_TEST_DOTENV=from-env-file\n", encoding="utf-8") + monkeypatch.delenv("NEMO_EXPERIMENTALIST_TEST_DOTENV", raising=False) + (profile_tree / ".env").write_text("NEMO_EXPERIMENTALIST_TEST_DOTENV=from-env-file\n", encoding="utf-8") monkeypatch.chdir(profile_tree) try: result = runner.invoke(app, ["run", "-o", str(profile_tree / "out")]) assert result.exit_code == 0, result.output assert "Loaded .env" in result.output - assert os.environ["NEMO_OPT_TEST_DOTENV"] == "from-env-file" + assert os.environ["NEMO_EXPERIMENTALIST_TEST_DOTENV"] == "from-env-file" finally: - os.environ.pop("NEMO_OPT_TEST_DOTENV", None) + os.environ.pop("NEMO_EXPERIMENTALIST_TEST_DOTENV", None)As per coding guidelines: "Use
EXPERIMENTALIST_*andNEMO_EXPERIMENTALIST_*environment-variable names instead of the obsolete Optimizer-prefixed variables."🤖 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_cli_profile.py` around lines 575 - 583, Rename the test environment variable from NEMO_OPT_TEST_DOTENV to the appropriate NEMO_EXPERIMENTALIST_* name throughout this test, including monkeypatch.delenv, the generated .env content, and the os.environ assertion; preserve the existing dotenv-loading behavior and expected value.Source: Coding guidelines
🧹 Nitpick comments (19)
plugins/nemo-experimentalist/framework-skills/langchain-framework/references/langchain-dependencies.md (1)
366-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace pip hints with uv commands.
The import guidance uses
# pip:labels, conflicting with this plugin’s uv-only dependency workflow. Use explicituv addexamples instead.As per coding guidelines, use
uvexclusively for dependency and environment management; do not use pip.🤖 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/framework-skills/langchain-framework/references/langchain-dependencies.md` around lines 366 - 381, Replace the inline “pip:” dependency hints in the dedicated-package import examples with explicit “uv add” commands, using the correct package names for langchain-tavily, langchain-chroma, and langchain-pinecone. Keep the import guidance and the WikipediaQueryRun note unchanged, and remove all pip references from this example.Source: Coding guidelines
plugins/nemo-experimentalist/framework-skills/langchain-framework/references/langgraph-persistence.md (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a top-level heading.
<overview>is not an H1, so this fails MD041. Add# LangGraph persistenceafter the front matter.🤖 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/framework-skills/langchain-framework/references/langgraph-persistence.md` at line 8, Add a top-level H1 heading, “LangGraph persistence,” immediately after the front matter and before the existing <overview> content in the documentation.Source: Linters/SAST tools
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py (1)
91-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReaching into
tree._iter_nodes()couples this module to GoalTree internals.Counting initial nodes via a private member defeats the import-safety boundary this module is built around. Walk the already-visited nodes instead.
♻️ Proposed refactor
def validate_tree(self, tree: Any) -> Any: """Validate a runtime GoalTree without importing the agent module.""" + initial_nodes: list[Any] = [] def visit(node: Any, depth: int) -> None: if depth > self.max_depth: raise ValueError(f"goal tree exceeds max depth {self.max_depth} at node {node.id!r}") if node.added_at_generation is None and depth > self.max_initial_depth: raise ValueError( f"initial goal tree nodes exceed max depth {self.max_initial_depth} at node {node.id!r}" ) + if node.added_at_generation is None: + initial_nodes.append(node) for child in node.children: visit(child, depth + 1) visit(tree.root, 1) - initial_nodes = [node for node in tree._iter_nodes() if node.added_at_generation is 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 `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py` around lines 91 - 98, Replace the private tree._iter_nodes() usage in the resolve flow after visit(tree.root, 1) with counting the nodes already collected during traversal. Preserve the existing added_at_generation filtering and min_initial_nodes/max_initial_nodes validation behavior while avoiding GoalTree internals.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/client.py (1)
28-28: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse
ipaddressfor loopback detection.String matching misses
127.0.0.2,::ffff:127.0.0.1, and other loopback forms, which then take the OIDC bootstrap path this module explicitly avoids for local platforms.♻️ Proposed refactor
+import ipaddress from urllib.parse import urlparse @@ -LOOPBACK_HOSTS = frozenset({"localhost", "127.0.0.1", "::1", "0.0.0.0"}) +LOOPBACK_HOST_NAMES = frozenset({"localhost", "0.0.0.0"}) + + +def _is_local_host(host: str) -> bool: + if host in LOOPBACK_HOST_NAMES: + return True + try: + return ipaddress.ip_address(host).is_loopback + except ValueError: + return False @@ - if host in LOOPBACK_HOSTS or not config_path.exists(): + if _is_local_host(host) or not config_path.exists():Also applies to: 44-47
🤖 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/client.py` at line 28, Replace the exact-string LOOPBACK_HOSTS check with ipaddress-based loopback detection in the client host-validation logic, covering IPv4, IPv6, mapped IPv4, and other loopback forms. Update both the definition near LOOPBACK_HOSTS and the related checks so local hosts continue to bypass the OIDC bootstrap path.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/entities.py (1)
60-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueString-based forward-ref return types violate guideline.
-> "ExperimentRun"(line 62) and-> "Candidate"(line 188) are string type hints. Addfrom __future__ import annotationsand drop the quotes for concrete self-referencing hints.As per coding guidelines, "In Python code, prefer concrete type hints over string-based type hints, and do not import those types only under
TYPE_CHECKING; import them normally when possible."♻️ Proposed fix
+from __future__ import annotations + from typing import Any, Literal, Sequence @@ - def _restore_id_from_json(cls, data: Any, handler: Any) -> "ExperimentRun": + def _restore_id_from_json(cls, data: Any, handler: Any) -> ExperimentRun: @@ - def slim(self) -> "Candidate": + def slim(self) -> Candidate:Also applies to: 186-196
🤖 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/entities.py` around lines 60 - 75, Add `from __future__ import annotations` at the top of the module, then replace the quoted self-referencing return annotations in `_restore_id_from_json` and the method returning `Candidate` with concrete `ExperimentRun` and `Candidate` annotations. Preserve the existing imports and method behavior.Source: Coding guidelines
pyproject.toml (1)
645-652: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
unresolved-importignore is broader than the stated 3.12-only rationale.This override silences unresolved-import for all of
plugins/nemo-experimentalist/**, not just nooa/harbor imports. Sinceextra-paths(line 564) already lets ty resolve the plugin's own local imports, this also masks genuine internal import typos/bugs across the whole plugin in the 3.11 lint environment.Consider scoping the ignore to files that actually import nooa/harbor (or add targeted inline suppressions there) instead of the whole tree.
🤖 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 `@pyproject.toml` around lines 645 - 652, Scope the ty unresolved-import suppression in the tool.ty.overrides configuration to only the nemo-experimentalist files that import the 3.12-only nooa or harbor packages, or replace it with targeted inline suppressions at those imports. Preserve resolution and reporting for the plugin’s local imports via the existing extra-paths configuration.plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_base.py (1)
228-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest asserts nothing about the merge.
result.id == "agent-ds"holds regardless of options. The dict-coercion branch inEvaluator.run(base.pyLine 105) is the thing under test — capture theoptionsthat reach_run.💚 Proposed assertion
evaluator = ConcreteEvaluator(options=EvaluatorConfig(force_rerun=False)) + received: list[EvaluatorConfig] = [] + + async def _run(agent, dataset, options): + received.append(options) + return [] + + evaluator._run = _run # type: ignore[method-assign] dataset = ConcreteDataset(id="ds") result = await evaluator.run( agent=Path("/tmp/agent"), dataset=dataset, options={"force_rerun": True}, ) assert result.id == "agent-ds" + assert received[0].force_rerun is True + assert evaluator.options.force_rerun is False🤖 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/experimentalist/test_evaluator_base.py` around lines 228 - 241, Update test_run_with_dict_options_merges to observe the options passed into ConcreteEvaluator._run, such as capturing them in the test subclass or a stub, and assert that the dict input preserves the configured values while overriding force_rerun with True. Keep the existing run invocation and result assertion as appropriate, but make the test directly verify the options reaching _run.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/models.py (1)
13-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd bounds like the sibling field has.
max_validation_repair_attemptsis bounded but these two aren't. A negativemax_tracesmakesinsight.trace_refs[: self._config.max_traces]inagent.pysilently drop traces from the tail instead of failing.♻️ Proposed bounds
max_summary_tokens: int = Field( default=80_000, + gt=0, description="Max tokens the token-budget summarizer may use.", ) max_traces: int = Field( default=10, + ge=0, description="Max trace refs from the insight to analyze in depth.", )🤖 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/eval_author/models.py` around lines 13 - 20, Update the max_summary_tokens and max_traces fields in the configuration model to use the same non-negative bounds as the sibling max_validation_repair_attempts field. Ensure negative values are rejected so max_traces cannot cause tail slicing in agent.py, while preserving their existing defaults and descriptions.plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.py (1)
4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
collections.abc.Sequence.typing.Sequenceis deprecated.♻️ Proposed fix
import json +from collections.abc import Sequence from pathlib import Path -from typing import Sequence🤖 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/experimentalist/test_evaluator_factory.py` around lines 4 - 6, Update the import in test_evaluator_factory.py to use Sequence from collections.abc instead of typing, and preserve all existing type annotations and behavior.plugins/nemo-experimentalist/tests/test_profile.py (1)
104-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis assertion is tautological. Both sides are constructed from
tmp_path, so it reduces toprofile_dir == tmp_path.resolve()— already covered at line 67. Assert against the Insights contract constants instead (and usePROFILE_FILENAMEon line 105 rather than the literal), otherwise a rename of the state directory would not be caught here.🤖 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_profile.py` around lines 104 - 111, Update test_profile_directory_anchors_shared_insights_path to build the profile path with the existing PROFILE_FILENAME constant and assert the Insights path against the established Insights contract/state-directory constants rather than reconstructing it from tmp_path. Preserve the test’s verification that profile.profile_dir resolves to the configured profile location while ensuring a state-directory rename would fail this assertion.plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.py (1)
1136-1273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the Harbor runtime fakes into a fixture.
FakeEnv/FakeEnvPaths/FakeTrialPaths/FakeHarborTaskplus the fourmonkeypatch.setattrcalls are duplicated verbatim here and again at lines 1766-1842, differing only incapabilities.mountedandhealthcheck. One parametrizable fixture removes ~120 lines and prevents drift when_start_harbor_runtimechanges.🤖 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/experimentalist/test_evaluator_harbor.py` around lines 1136 - 1273, Extract the duplicated Harbor runtime setup from test_harbor_dependency_context_start_runtime and test_harbor_dependency_context_start_runtime_mounted into a parametrizable pytest fixture. Have the fixture configure capabilities.mounted and healthcheck as parameters, provide FakeEnv/FakeEnvPaths/FakeTrialPaths/FakeHarborTask, and apply all four monkeypatches; update both tests and the corresponding tests near the later duplicated block to consume it while preserving their existing parameter-specific behavior.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.py (1)
76-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
EvaluatorFactorybypasses the injection patternDatasetFactoryestablishes.
DatasetFactorytakes an injectable registry;EvaluatorFactoryhardcodes the module global, so custom evaluator types registered in one factory are invisible to the other. Also,DatasetFactoryaliases the shared module dict — a caller mutating it changes global state.♻️ Suggested symmetry
class EvaluatorFactory: """Build concrete evaluators from evaluator type.""" + def __init__( + self, + supported_evaluator_types: dict[EvaluatorType, tuple[type[Dataset], type[Evaluator], type[EvaluatorConfig]]] + | None = None, + ) -> None: + self.supported_evaluator_types = dict(supported_evaluator_types or _SUPPORTED_EVALUATOR_TYPES) + def build_evaluator(Then reference
self.supported_evaluator_typesin the body, and usedict(...)inDatasetFactory.__init__too.🤖 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/factory.py` around lines 76 - 107, Update EvaluatorFactory to accept an injectable evaluator registry like DatasetFactory, store it on self.supported_evaluator_types, and use that instance attribute throughout build_evaluator instead of the module-level _SUPPORTED_EVALUATOR_TYPES. In DatasetFactory.__init__, copy the provided registry with dict(...) so factory-local mutations cannot alter shared global state.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/__init__.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew empty
__init__.pyfiles conflict with this plugin's namespace-package guideline.Both files only add an SPDX header, no re-exports. The plugin-specific guideline says not to add
__init__.pyhere; a repo-wide learning says not to flag__init__.pygenerally, but its stated exception (leaf packages doing re-exports) doesn't apply since neither file re-exports anything.
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/__init__.py#L1-3: remove, rely on implicit namespace package.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/__init__.py#L1-3: remove, rely on implicit namespace package.As per coding guidelines ("Do not add
__init__.pyfiles; use implicit namespace packages") and based on a retrieved learning noting__init__.pyshouldn't be flagged unless it contradicts explicit-packages/re-export conventions — flagged here because no re-export exists.🤖 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/__init__.py` at line 1, Remove the empty __init__.py files from the experimentalist and experimentalist/components packages, including their SPDX-only contents, so both use implicit namespace packaging as required. Do not add replacements or re-exports.Sources: Coding guidelines, Learnings
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/tools.py (2)
169-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a concrete type hint instead of a quoted string.
"str | int"should be unquoted; the plugin targets Python 3.11 wherestr | intis valid at runtime withoutfrom __future__ import annotations.As per coding guidelines: "In Python code, prefer concrete type hints over string-based type hints, and do not import those types only under `TYPE_CHECKING`; import them normally when possible."♻️ Fix
- def read_analysis_file(self, path_or_round: "str | int", limit: int | None = 8000) -> str: + def read_analysis_file(self, path_or_round: str | int, limit: int | None = 8000) -> str:🤖 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/tools.py` at line 169, Update the read_analysis_file method annotation to use the concrete str | int union directly instead of a quoted string, while preserving the existing return type and limit annotation.Source: Coding guidelines
187-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile: bypasses
LocalExperimentalistBackend.__init__to reuse_load_candidate.
__new__+ manually setting the private_eoattribute couplesWorkspaceTooltoLocalExperimentalistBackend's internal state. If__init__later sets up more state_load_candidatedepends on, this throwaway instance silently breaks.Expose a proper classmethod/static helper on
LocalExperimentalistBackend(e.g.load_candidate(path)) forWorkspaceToolto call instead of poking private state.🤖 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/tools.py` around lines 187 - 209, Replace the __new__-based backend construction in WorkspaceTool.get_metadata with a public class-level or static helper on LocalExperimentalistBackend, such as load_candidate(path), that performs candidate deserialization without requiring instance state. Implement the helper by reusing the existing _load_candidate logic, then call it after the metadata existence check; remove the manual _eo assignment and private-state coupling.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/model_config.py (1)
86-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefault model names duplicated between factories and the banner. Lines 93-95 restate the defaults from Lines 34, 51, 68; they will drift and the banner will misreport. Hoist to module constants.
🤖 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/model_config.py` around lines 86 - 105, Define shared module-level constants for the smart, mid, and fast default model names, then update both the model factories and log_model_config to reference those constants instead of duplicating string literals. Preserve the existing defaults and banner output.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/loop.py (1)
936-977: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated agent-dir scan. Two identical filter+parse loops. Extract an iterator yielding
(agent_dir, meta).🤖 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/loop.py` around lines 936 - 977, Extract the repeated agent directory filtering and metadata parsing from the rollback logic into a shared iterator yielding (agent_dir, meta). Update both cleanup loops to consume this iterator while preserving their existing deletion and killed_round handling.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_explorer.py (1)
1857-1868: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDescendant-span closure is implemented twice with a quadratic rescan. Both sites repeatedly sweep
self.raw_spansuntil no change; a singleparent_span_id → childrenindex built once in__init__fixes cost and duplication.
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_explorer.py#L1857-L1868: rewrite_session_span_idsto walk the parent→children index.plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_explorer.py#L2228-L2236: call_session_span_idsinstead of re-implementing the loop.🤖 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/trace_explorer.py` around lines 1857 - 1868, Replace the repeated raw_spans rescans with a parent_span_id-to-children index built once in __init__. In plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/trace_explorer.py#L1857-L1868, update _session_span_ids to traverse that index; in the same file#L2228-L2236, call _session_span_ids instead of duplicating the descendant loop.plugins/nemo-experimentalist/tests/test_experiment_mirror.py (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the leftover
# VERIFY-2marker.♻️ Proposed cleanup
-from nemo_platform import ConflictError, NotFoundError, omit # VERIFY-2 +from nemo_platform import ConflictError, NotFoundError, omit🤖 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_experiment_mirror.py` at line 10, Remove the leftover “# VERIFY-2” marker from the import statement while preserving the existing ConflictError, NotFoundError, and omit imports.
- RemoteExperimentalistBackend.get_insight delegated to the local backend it already wraps instead of raising NotImplementedError, which made remote mode strictly weaker than local and hard-failed any insight-driven run. - _serialize_chunks encoded the whole accumulated batch once per span, so a 5k-span trace did millions of span encodings. Each entry is now encoded once and its size accumulated; chunk boundaries are unchanged. - The trace-scoring prompt told the agent to call explorer.get_spans(), which does not exist, so it would fall back to guessing span ids. Points at get_span_id/get_turn_data instead. - terminator and deps no longer hide EvolutionaryOptimizerConfig behind TYPE_CHECKING or Any. It lives in resolve, which does not import either module, so there is no cycle to avoid. - The benchmark README pointed at benchmarks/experimentalist/, a path that predates the move into the monorepo. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
2 participants
Moves the Experimentalist agent-optimization plugin over from the standalone
nemo-optimizerrepo. Purely additive —nemo-optimizeris untouched, and removing the migrated files there is a follow-up once this copy is proven.What moved
benchmarks/)langchain-frameworkandnooaframework skills the harness depends onThis adds
nemo experimentalist run|doctoras a top-level CLI group.Two things worth a closer look
The plugin silently raised the monorepo's Python floor. A uv workspace resolves to the highest
requires-pythonamong its members, so declaring the plugin at>=3.12rewrote the root lock's floor from 3.11 to 3.12 and dropped everypython_full_version < '3.12'conditional package. CI type-checks against 3.11, so this would have broken in a way that looked unrelated to the plugin.The plugin is now declared at
>=3.11, its two genuinely 3.12-only dependencies (nooa,harbor) carry markers, and the whole plugin is gated behindpython_full_version >= '3.12'in theexperimentalistdependency group so it can't install half-resolved. The lock diff is purely additive: 82 lines, 4 new packages, zero version changes, norequires-pythonchange.In a future change, we will either bump the python version or adjust
nooato allow Python 3.11.nooais a git dependency rather than a PyPI release, so osv cannot read its license metadata and it needs an entry in the license overrides.Validation
nemo experimentalist doctorreaches the live inference endpoint, finds Docker, confirms harbor importsbenchmarks/run.py --validate-onlyresolves all paths and validates 89 Terminal-Bench tasks (this is what proves theREPO_ROOT→PLUGIN_ROOTrewrites are right)ruff checkclean on everything addedmake test-unitadds zero failures versus the baselineOpen source review
No secrets or keys, no internal-only hostnames (
inference-api.nvidia.comis already referenced across the public SDK skills), no vendored Terminal-Bench task content — tasks download to a cache at runtime. Thelangchain-frameworkskill carries a provenance note stating it summarizes LangChain's MIT-licensed public docs, with original structure and comparisons.Summary by CodeRabbit
nemo experimentalist runandnemo experimentalist doctorworkflows for optimizer execution and environment validation.