test(agentic-use): add evaluator agent benchmark matrix - #3
Merged
Conversation
Contributor
|
All contributors have signed the DCO ✍️ ✅ |
Contributor
Author
|
I have read the DCO document, and I hereby sign the DCO. |
Contributor
Author
|
recheck |
Contributor
Author
|
I have read the DCO document, and I hereby sign the DCO. |
Contributor
|
SandyChapman
enabled auto-merge
May 21, 2026 13:08
ngoncharenko
approved these changes
May 21, 2026
5 tasks
github-merge-queue Bot
pushed a commit
that referenced
this pull request
May 28, 2026
* fix: address open CodeQL alerts in TypeScript code Close 21 open CodeQL alerts on main: Security - LargeFileWorker: remove dead `download` (untrusted-URL fetch) and `upload` actions; only `downloadAsFile` (SDK path-based) is used by callers. Closes #4 (client-side-request-forgery) and #17 (missing-origin-check). - orval/generate.ts: use `fs.mkdtempSync` for the OpenAPI spec temp file instead of a predictable `os.tmpdir()` path. Closes #5 (insecure-temporary-file). Code-quality - Drop redundant `this.page = page` / `this.request = request` in 11 e2e-tests classes — TS parameter properties (`public readonly page: Page`, `private request: APIRequestContext`) already assign the field. Closes #22-#32 (useless-assignment-to-property). - Drop redundant null/undefined checks after narrowing in ReportTraceModal/utils, BenchmarkDetailsPanel, api/intake/utils, ActionMenu, useSubmitICLsFile. Closes #33-#37. - SafeSynthesizerJobReportRoute/util: drop unreachable `else if (score >= 8)` branches and the dead `UNAVAILABLE` fallback; add explicit `Number.isNaN` guard at the top of each grading helper. Closes #20, #21. - WorkspaceDashboardRoute: drop inner `MODEL_COMPARE_ENABLED ? a : b` ternary that always picked `a` (lives inside an outer `MODEL_COMPARE_ENABLED &&` guard); drop now-unused `getWorkspaceBaseModelsRoute` import. Closes #19. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: refactor remaining CodeQL-flagged build scripts to argv form Drop shell interpolation in dev/build scripts so user-supplied branch names, commit hashes, paths, and env values cannot be parsed as shell syntax. Also plug a TOCTOU and add origin allowlists for the two http-to-file fetches. - scripts/cherry-pick.ts: route every git call through execFileSync('git', [...]). Closes #6-#10 (indirect-cmd-line-injection). - scripts/git-utils.ts: openBrowser uses execFile + argv array; status/branch helpers use execFileSync with argv. Removes the brittle " → \" escape and the shell-interpolated browser command. Closes #1 (incomplete-sanitization) and #11 (indirect-cmd-line-injection). - sdk/orval/format-generated.ts: prettier runs via execFileSync. Closes #2 (shell-cmd-injection-from-env) and #13 (indirect-cmd-line-injection). - sdk/orval/generate.ts: orval runs via execFileSync, with its parameters passed in env instead of interpolated into a shell string; remote spec fetches are restricted to an allowlist of github/gitlab hosts; the existsSync+readFileSync TOCTOU in postProcessZodFiles is collapsed into a single try/catch on ENOENT. Closes #3 (file-system-race), #12 (indirect-cmd-line-injection), and #14 (http-to-file-access). - studio/scripts/fetch-styles.ts: validate that the fetch URL hostname matches the configured Kaizen CDN before fetching. Closes #15 (http-to-file-access). Signed-off-by: mschwab <mschwab@nvidia.com> * fix: close remaining CodeQL alerts re-emitted on PR scan - scripts/git-utils.ts openBrowser: parse URL with `new URL()` and require http/https before spawning. Replace the Windows `cmd /c start` shell invocation with `rundll32 url.dll,FileProtocolHandler` so no branch goes through a shell. Pass `--` separator on darwin/linux so a URL starting with `-` cannot be parsed as an option. Closes #3951. - sdk/orval/generate.ts: delete the unused HTTP-fetch branch from `getFile()`. All current `serviceConfigs` reference local YAML paths, so the network->file write CodeQL flagged on line 131 (#14) no longer exists. Throws a clear error if a remote URL is configured. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: drop -- separator for xdg-open xdg-open does not honor -- as an option terminator; passing it as an arg caused openBrowser to fail on Linux. URL is already validated to http(s), so the separator wasn't load-bearing — just drop it on the Linux branch. Codex review on PR #75. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: address CodeRabbit findings on PR #75 - scripts/git-utils.ts: drop `--` from macOS `open` argv too. `open`'s man page does not document `--` as an end-of-options separator. URL is already validated to http(s), so the separator wasn't load-bearing. - sdk/orval/format-generated.ts: on Windows, run prettier through `cmd.exe /c` so the `prettier.cmd` shim resolves. `execFileSync` on Windows cannot launch .cmd shims directly. - sdk/orval/generate.ts: same Windows wrap for `pnpm exec orval`. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: validate format-generated.ts servicePath argv The Windows cmd.exe /c wrap added in ec7aa93 re-opened a CodeQL data-flow finding (#3961, #3962) because generatedPath traces back to process.argv[2]. Validate the argv against a safe-char regex at entry so CodeQL sees it as sanitized before it flows into argv or paths. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: replace regex with hardcoded Set allowlist for servicePath CodeQL did not recognize the regex check as a sanitizer; switching to a hardcoded Set lookup against known serviceConfigs paths so the data flow is reducible to a finite set of literal values. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: use prettier Node API instead of subprocess Replace the prettier CLI invocation with prettier's programmatic format/resolveConfig/getFileInfo API. No subprocess means no cmd.exe wrap, no command-line argument flow, and the CodeQL indirect-command-line-injection / shell-cmd-injection-from-env alerts on format-generated.ts can resolve. Also fixes the Windows .cmd shim resolution problem CR raised, since prettier now runs in-process. The servicePath argv is still validated against a hardcoded Set of known serviceConfigs paths to prevent directory traversal via path.join. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: use readdirSync withFileTypes to avoid statSync TOCTOU CodeQL flagged the statSync -> readFileSync / writeFileSync pair in formatWithPrettier as a file-system-race. Getting Dirent entries from readdirSync(dir, { withFileTypes: true }) lets us check isDirectory / isFile inline without a separate stat round-trip, closing the alert. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: drop remaining statSync usages in format-generated.ts Codex flagged that getTsFiles and splitZodTagFilesIn still used the readdir-string + statSync pattern, leaving two more file-system-race sinks even after formatWithPrettier was converted. Switch both to readdirSync(dir, { withFileTypes: true }) and use Dirent.isFile() / isDirectory() inline. Removes the last statSync from this script. Signed-off-by: mschwab <mschwab@nvidia.com> --------- Signed-off-by: mschwab <mschwab@nvidia.com>
aray12
pushed a commit
that referenced
this pull request
May 28, 2026
* fix: address open CodeQL alerts in TypeScript code Close 21 open CodeQL alerts on main: Security - LargeFileWorker: remove dead `download` (untrusted-URL fetch) and `upload` actions; only `downloadAsFile` (SDK path-based) is used by callers. Closes #4 (client-side-request-forgery) and #17 (missing-origin-check). - orval/generate.ts: use `fs.mkdtempSync` for the OpenAPI spec temp file instead of a predictable `os.tmpdir()` path. Closes #5 (insecure-temporary-file). Code-quality - Drop redundant `this.page = page` / `this.request = request` in 11 e2e-tests classes — TS parameter properties (`public readonly page: Page`, `private request: APIRequestContext`) already assign the field. Closes #22-#32 (useless-assignment-to-property). - Drop redundant null/undefined checks after narrowing in ReportTraceModal/utils, BenchmarkDetailsPanel, api/intake/utils, ActionMenu, useSubmitICLsFile. Closes #33-#37. - SafeSynthesizerJobReportRoute/util: drop unreachable `else if (score >= 8)` branches and the dead `UNAVAILABLE` fallback; add explicit `Number.isNaN` guard at the top of each grading helper. Closes #20, #21. - WorkspaceDashboardRoute: drop inner `MODEL_COMPARE_ENABLED ? a : b` ternary that always picked `a` (lives inside an outer `MODEL_COMPARE_ENABLED &&` guard); drop now-unused `getWorkspaceBaseModelsRoute` import. Closes #19. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: refactor remaining CodeQL-flagged build scripts to argv form Drop shell interpolation in dev/build scripts so user-supplied branch names, commit hashes, paths, and env values cannot be parsed as shell syntax. Also plug a TOCTOU and add origin allowlists for the two http-to-file fetches. - scripts/cherry-pick.ts: route every git call through execFileSync('git', [...]). Closes #6-#10 (indirect-cmd-line-injection). - scripts/git-utils.ts: openBrowser uses execFile + argv array; status/branch helpers use execFileSync with argv. Removes the brittle " → \" escape and the shell-interpolated browser command. Closes #1 (incomplete-sanitization) and #11 (indirect-cmd-line-injection). - sdk/orval/format-generated.ts: prettier runs via execFileSync. Closes #2 (shell-cmd-injection-from-env) and #13 (indirect-cmd-line-injection). - sdk/orval/generate.ts: orval runs via execFileSync, with its parameters passed in env instead of interpolated into a shell string; remote spec fetches are restricted to an allowlist of github/gitlab hosts; the existsSync+readFileSync TOCTOU in postProcessZodFiles is collapsed into a single try/catch on ENOENT. Closes #3 (file-system-race), #12 (indirect-cmd-line-injection), and #14 (http-to-file-access). - studio/scripts/fetch-styles.ts: validate that the fetch URL hostname matches the configured Kaizen CDN before fetching. Closes #15 (http-to-file-access). Signed-off-by: mschwab <mschwab@nvidia.com> * fix: close remaining CodeQL alerts re-emitted on PR scan - scripts/git-utils.ts openBrowser: parse URL with `new URL()` and require http/https before spawning. Replace the Windows `cmd /c start` shell invocation with `rundll32 url.dll,FileProtocolHandler` so no branch goes through a shell. Pass `--` separator on darwin/linux so a URL starting with `-` cannot be parsed as an option. Closes #3951. - sdk/orval/generate.ts: delete the unused HTTP-fetch branch from `getFile()`. All current `serviceConfigs` reference local YAML paths, so the network->file write CodeQL flagged on line 131 (#14) no longer exists. Throws a clear error if a remote URL is configured. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: drop -- separator for xdg-open xdg-open does not honor -- as an option terminator; passing it as an arg caused openBrowser to fail on Linux. URL is already validated to http(s), so the separator wasn't load-bearing — just drop it on the Linux branch. Codex review on PR #75. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: address CodeRabbit findings on PR #75 - scripts/git-utils.ts: drop `--` from macOS `open` argv too. `open`'s man page does not document `--` as an end-of-options separator. URL is already validated to http(s), so the separator wasn't load-bearing. - sdk/orval/format-generated.ts: on Windows, run prettier through `cmd.exe /c` so the `prettier.cmd` shim resolves. `execFileSync` on Windows cannot launch .cmd shims directly. - sdk/orval/generate.ts: same Windows wrap for `pnpm exec orval`. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: validate format-generated.ts servicePath argv The Windows cmd.exe /c wrap added in ec7aa93 re-opened a CodeQL data-flow finding (#3961, #3962) because generatedPath traces back to process.argv[2]. Validate the argv against a safe-char regex at entry so CodeQL sees it as sanitized before it flows into argv or paths. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: replace regex with hardcoded Set allowlist for servicePath CodeQL did not recognize the regex check as a sanitizer; switching to a hardcoded Set lookup against known serviceConfigs paths so the data flow is reducible to a finite set of literal values. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: use prettier Node API instead of subprocess Replace the prettier CLI invocation with prettier's programmatic format/resolveConfig/getFileInfo API. No subprocess means no cmd.exe wrap, no command-line argument flow, and the CodeQL indirect-command-line-injection / shell-cmd-injection-from-env alerts on format-generated.ts can resolve. Also fixes the Windows .cmd shim resolution problem CR raised, since prettier now runs in-process. The servicePath argv is still validated against a hardcoded Set of known serviceConfigs paths to prevent directory traversal via path.join. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: use readdirSync withFileTypes to avoid statSync TOCTOU CodeQL flagged the statSync -> readFileSync / writeFileSync pair in formatWithPrettier as a file-system-race. Getting Dirent entries from readdirSync(dir, { withFileTypes: true }) lets us check isDirectory / isFile inline without a separate stat round-trip, closing the alert. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: drop remaining statSync usages in format-generated.ts Codex flagged that getTsFiles and splitZodTagFilesIn still used the readdir-string + statSync pattern, leaving two more file-system-race sinks even after formatWithPrettier was converted. Switch both to readdirSync(dir, { withFileTypes: true }) and use Dirent.isFile() / isDirectory() inline. Removes the last statSync from this script. Signed-off-by: mschwab <mschwab@nvidia.com> --------- Signed-off-by: mschwab <mschwab@nvidia.com> Signed-off-by: Alex Ray <alray@nvidia.com>
benmccown
added a commit
that referenced
this pull request
Jun 23, 2026
Composition over inheritance for the k8s reconcilers (review #2/#3): * Extract StatusProjector (pod-status projection, crash-loop/pending-timeout error builders, host URL) and ResourceDeleter (idempotent 404-tolerant delete) as standalone collaborators. * Reconciler is now a pure interface (the 5 verbs); NimOperatorReconciler and K8sReconciler compose the projector + deleter instead of inheriting them. The backend builds both collaborators in init() and injects them. Thread the reconcile context through the backend interface (review #19): * create/update/get_model_deployment_status now take a single ctx: ModelContext instead of (deployment, config, model_entity); applied across the ServiceBackend ABC and the docker / none / k8s backends, the deployment reconciler call sites, and the test mocks. delete stays (workspace, name). Fixes + nits: * Harden NIMService status read against a null status/state (review #15): (nim_status.get("state") or "").lower() can no longer raise. * Convert nim_operator logging to structured extra={} (review #13); avoid the reserved LogRecord 'name' key (use resource_name / deployment_name). * Flatten the Files-service create/update branches into a guard-clause helper (review #14). * compile_puller_job: rename args -> container_args (review #17). * Reconciler nits: import the vllm_k8s_compiler module under its full name (review #7), reflow the P3 (a)/(b) comment (review #8), quote values in the model-source error (review #9), drop the _ = image_pull_secrets dance (review #12), name the event-message cap MAX_EVENT_MESSAGE_CHARS (review #6), and document the _select_reconciler None contract (review #16). Signed-off-by: Ben McCown <bmccown@nvidia.com>
benmccown
added a commit
that referenced
this pull request
Jun 24, 2026
Composition over inheritance for the k8s reconcilers (review #2/#3): * Extract StatusProjector (pod-status projection, crash-loop/pending-timeout error builders, host URL) and ResourceDeleter (idempotent 404-tolerant delete) as standalone collaborators. * Reconciler is now a pure interface (the 5 verbs); NimOperatorReconciler and K8sReconciler compose the projector + deleter instead of inheriting them. The backend builds both collaborators in init() and injects them. Thread the reconcile context through the backend interface (review #19): * create/update/get_model_deployment_status now take a single ctx: ModelContext instead of (deployment, config, model_entity); applied across the ServiceBackend ABC and the docker / none / k8s backends, the deployment reconciler call sites, and the test mocks. delete stays (workspace, name). Fixes + nits: * Harden NIMService status read against a null status/state (review #15): (nim_status.get("state") or "").lower() can no longer raise. * Convert nim_operator logging to structured extra={} (review #13); avoid the reserved LogRecord 'name' key (use resource_name / deployment_name). * Flatten the Files-service create/update branches into a guard-clause helper (review #14). * compile_puller_job: rename args -> container_args (review #17). * Reconciler nits: import the vllm_k8s_compiler module under its full name (review #7), reflow the P3 (a)/(b) comment (review #8), quote values in the model-source error (review #9), drop the _ = image_pull_secrets dance (review #12), name the event-message cap MAX_EVENT_MESSAGE_CHARS (review #6), and document the _select_reconciler None contract (review #16). Signed-off-by: Ben McCown <bmccown@nvidia.com>
8 tasks
marcusds
added a commit
that referenced
this pull request
Jul 17, 2026
#3: add is_external_agent(agent) (backend, mirrors is_container_deployment_mode) and isExternalAgent() (frontend helper); replace the 4 backend + 6 frontend inline `source == "external"` / `source === 'external'` checks so the next AgentSource value only touches one predicate. #6: extract the external-agent logs notice into a reusable ExternalAgentNotice component + a shared EXTERNAL_AGENT_HEADING constant, so the "runs outside NeMo Platform" phrasing lives in one place. Signed-off-by: mschwab <mschwab@nvidia.com>
sklinglernv
added a commit
that referenced
this pull request
Aug 4, 2026
Nine of sixteen review comments were valid. The two that were real defects: `log_model_config` guarded the tiers but resolved the endpoint and key unguarded, so the banner raised on exactly the unconfigured install it exists to diagnose. Every field is optional there now; callers that need a value still fail in `api_base()` / `api_key()`. `asyncio.gather(..., return_exceptions=True)` returns `CancelledError`, which derives from BaseException, so the `isinstance(r, Exception)` filter classified a cancelled Coder as neither failed nor succeeded and let the candidate through to evaluation and ranking as though its source had been written. Cancellation now re-raises and unwinds the round. Pre-existing on main; fixed here because this branch already rewrote that block. Both have regression tests, each verified to fail with its fix reverted. Also: `candidate_metric_keys` coerced non-strings, so malformed cached metadata (`[1]`) passed as a valid measurement and skipped a fresh Insight evaluation -- it now reads as "not recorded"; the mirror test asserted only on the entity and would have passed if `project_candidate` reverted to a fixed channel allowlist, so it projects through a real mirror now; Eval Author's autouse fixture clears `Configuration` alongside the client caches, since bridged endpoints outlive `monkeypatch`; the README example imports `Dataset` from `entities`; a duplicated env prefix in AGENTS.md; a prompt referring to `insight_reward` where its own code block defines `insight_rewards`; the nooa rev in `third_party/requirements-main.txt` and the framework skill; and the api_base doc row, which claimed "required" while the CLI defaults it to the gateway. Declined: requiring HTTPS for the API base (#3, #8) would break local endpoints, and the actual credential-forwarding risk is already scheme-guarded in `_is_gateway_base`. The dotenv-linter ordering finding (#7) cites a hook this repo does not configure. The conftest credential-namespace overlap (#4) is real but pre-existing and identical on main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com>
ryana
pushed a commit
to ryana/nemo-platform
that referenced
this pull request
Aug 12, 2026
…erimentalist (NVIDIA-NeMo#1038) * doc: fix readme steps Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): delete the remote backend and dead ABC surface M1 groundwork for the microkernel refactor: remove seams that never had an implementation, so the real ones are easier to see. - Delete `RemoteExperimentalistBackend`. It delegated 100% to the local backend, raised `NotImplementedError` for five methods, and was unreachable — the CLI rejected `--mode remote` before ever constructing it. - Drop `--mode` and its plumbing through `cli.py`, `run.py`, and `make_experimentalist_backend`, which now always builds the local backend. - Delete `list_traces`, `get_trace`, `list_scores` and `get_agent` from `ExperimentalistBackend`. All four were abstract with no implementation anywhere and no caller; ~22% of the ABC was fiction. - Replace the `LocalExperimentalistBackend.__new__` hack in `WorkspaceTool` with a module-level `load_candidate()`. The hack existed only to reach an 8-line private deserializer without triggering `__init__`'s directory creation. No behavior change: every deleted path was unreachable or unimplemented. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): resolve model tiers at construction, not import Nine agent classes bound an LLM in the class body (``class X(Agent, llm=get_smart_model())``), and the getters call ``_required_env``. Importing any component module therefore required live credentials — which blocks registry discovery (it must import component modules to find them), forced `cli.py` to lazy-import so `doctor` could run, and is the sole reason Eval Author ships an `_env_bridge` side-effect module. - Agent classes take ``Agent`` plainly and resolve their tier in ``__init__``, via ``kwargs.pop("llm", None) or get_<tier>_model()``. The tier stays the default; an explicit ``llm=`` still wins, so components are now injectable. - Three ``@strategy(..., llm=...)`` method overrides are decorator arguments and evaluate at import regardless. They take a `LazyModel` proxy that builds its client on first attribute access. nooa accepts the override as ``Any``, stores it with ``setattr`` and reads it back with ``getattr`` — there is no ``isinstance`` check — so the proxy is transparent; `lazy_model()` carries the one documented cast. - Cache on ``(name, api_base, api_key)`` rather than per-tier with no key, so two tiers naming the same model share a client and a changed key is not ignored. - Same change for `EvalAuthor`, whose class-body binding blocked `loop.py`, which imports it at module scope. Tiers and defaults are unchanged. Covered by a subprocess test asserting that importing loop, coder and EvalAuthor builds no client — a subprocess because the client cache is process-global and an earlier test would mask a regression. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(eval-author): delete the _env_bridge side-effect module `_env_bridge.py` existed for one reason: the Experimentalist agents Eval Author borrows (TraceAnalyzer, TraceExplorer) built their LLM in the class body, so they read `EXPERIMENTALIST_*` the moment their module was imported. Bridging therefore had to happen before an import, which no function call inside the module can do — hence a module imported purely for its side effect, plus an isort-ordering constraint documented in three places. Those agents now resolve their tier when constructed, so `EvalAuthor.__init__` calls `bridge_author_env_to_experimentalist()` directly and the module is gone. The `AUTHOR_*` → `EXPERIMENTALIST_*` credential fallback stays. It is a separate mechanism serving Eval Author's own clients, and per the boundary ratchet it is removable only once the allowlist is empty — not by this change. The ordering test becomes a construction test, and clears `EXPERIMENTALIST_*` inside the subprocess before constructing: importing nooa loads a `.env`, which would otherwise pre-fill the slots and mask a broken bridge. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): one config tree, and a models: block `resolve.py` carried a second declaration of six component configs — `CoderConfig`, `AnalyzerConfig`, `ProposerConfig`, `GoalTreeConfig`, `RationalizerConfig`, `TraceAnalyzerConfig` — byte-equivalent to the components' own, plus four `model_validate(x.model_dump())` round trips to convert between them at each use site. They existed because importing a component module used to require credentials, so `resolve.py` could not import the real classes. Lazy binding removed that constraint. - New `config.py` owns the run-config tree and imports each component slice from the component that consumes it. `resolve.py` keeps input resolution only, 131 lines lighter. - The four round trips are gone: the tree already holds the right classes. - Every importer points at `config.py`; no re-export shim. - Add a `models:` block (`smart`/`mid`/`fast`), mirroring the key the benchmark configs already use. `run.py` applies it before any agent is constructed, so a config-file choice takes effect; unset tiers keep the environment's value. Credentials stay environment-only and are not accepted in config. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): move Layer A entities out of components/evaluator/ `Dataset`, `Task`, `TrialResult`, `EvaluationResult`, `DatasetRef`, `ResourceRef` and the metric/dependency models lived in `experimentalist/components/evaluator/models.py` — Layer A filed inside a Layer B component. They are the contract every plugin speaks, and the evaluator is one consumer among several, so they now sit in `entities.py` beside `ExperimentRun` and `Candidate`. Pure relocation: no field, validator or behaviour changes. 43 files repointed, no re-export shim. This also reclassifies the Eval Author boundary ratchet, which previously counted imports without distinguishing what they were for. It now pins two lists: - `_SHARED_LAYER_A` — the entity contract. Permanent by design. Duplicating it would fork the contract and break Studio comparability, which is the one thing the design cannot trade away. - `_BORROWED_BEHAVIOUR` — Harbor, tools, trace analysis, the backend factory. Still debt, still shrink-only, still "duplicate rather than add a row". Five of the twelve original rows were Layer A and collapse into one shared entry; the reverse assertion now guards only the debt list, so removing a borrowed module still fails loudly while sharing entities does not. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): reward channels as an open map on Candidate `Candidate` carried four reward channels across ten fields — `train_reward`, `validation_reward`, `insight_reward`, `validation_trajectory_reward`, their `_details` twins, and two insight-only sidecars — and the channel list was re-hardcoded in five places: `slim()`, `__repr__`, `ExperimentMirror.SPLITS`, `_split_reward`, and `insight_promotion`. A strategy with a new reward channel could not reach Studio without editing our entity, which contradicts the microkernel's universal-`record_reward` claim. rewards: dict[str, RewardRecord] # channel -> {metrics, summary, trials, metadata} Keyed by *channel*, not by split: trajectory scoring is a second measurement of the validation split, so a split-keyed map cannot hold both. Channels today are `train`, `validation`, `insight` and `validation-trajectory`; adding one now costs no entity change, and `ExperimentMirror` iterates what was measured instead of an allowlist. `RewardRecord.summary` separates a scalar rollup from the dimensions. The trajectory aggregate previously shared a namespace with goal-tree node ids, so a node named `aggregate` silently overwrote it — and any selector doing Pareto over those metrics would have treated the rollup as a dimension dominating every real one. Also here: `optimization_params` (the genotype HPO breeds from, absent until now), the insight sidecars move to the channel's `metadata`, `trajectory_detail` keeps its own field because its shape is per-node-per-task rather than per-trial, and the untyped `artifacts` dict is gone. This changes the on-disk `metadata.json` shape. Alpha software, no migration: existing experiment directories will not load. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): model names are required, with no built-in default `_DEFAULTS` named `openai/openai/openai/gpt-5.5` and friends. Those are NVIDIA Inference Gateway routing paths, not portable model ids, so against any other endpoint they name nothing — and the failure surfaced at the first LLM call as an opaque provider error rather than a configuration problem. Gating them on the gateway hostname was the first fix I tried. It worked, but it added a third copy of `_is_gateway_base` to library code to defend a default that serves almost nobody: the getting-started guide exports all three tiers explicitly, `benchmarks/run.py` sets them from its config, and the README table lists them. Deleting the defaults removes both the wrong values and the branch. `model_name(tier)` now reads its variable and raises when unset. A model name is only meaningful against a specific endpoint, so there is no portable value to fall back to — name them as your endpoint does (`openai/openai/openai/gpt-5-mini` on the gateway, `gpt-5-mini` against OpenAI directly). The error arrives when the agent is constructed, before any Docker work. `log_model_config` shows "(unset)" rather than raising; a display helper should not be the thing that fails. The `INFERENCE_API_KEY` → `EXPERIMENTALIST_API_KEY` copy in `cli.py` is untouched. That one is a scoping check on a secret, not a convenience, and removing it is a separate decision — tracked as an open question in the plan. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): EvolutionNode is not a Candidate `7493877ff` replaced the `Candidate` reward fields with accessors via a bulk rename, which also rewrote three call sites in `models.py` whose receiver is an `EvolutionNode`. The node wraps a candidate and exposes `train_reward` / `val_reward` / `trajectory_reward` properties that delegate; it has no `metrics()`. `EvolutionTree.to_markdown_table` and `EvolutionNode.reward_str` therefore raised `AttributeError` at runtime. Nothing caught it: neither method had a test, so the whole suite stayed green and a live run crashed while writing the round report. Restores the property access and adds coverage for both rendering paths. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): one reward accessor, and channel-agnostic rendering Two follow-ups to the reward-channel map. `Candidate.metrics(channel)` / `Candidate.trials(channel)` made `metrics` name two different levels of the structure — the accessor keyed by *channel*, and `RewardRecord.metrics` keyed by *dimension* — so `candidate.metrics("train")` read like an index into a dict that does not exist. One accessor now returns the record itself: candidate.reward("train").metrics # dimensions candidate.reward("train").trials "train" in candidate.rewards # was it measured at all `EvolutionNode` was the sixth place hardcoding the channel list, and the one the previous commit's message wrongly claimed to have fixed. It exposed exactly three named reward properties and `reward_str` / `to_markdown_table` iterated them, so a new channel reached Studio but stayed invisible in `OPTIMIZATION.md`. The tell was already there: `insight` has existed for weeks and had no property. Both now derive from `candidate.rewards`: `reward_str` renders whatever was measured, and the table's columns are the union of (channel, dimension) across nodes. The three named properties stay — the terminator and winner selection mean `validation` specifically, and should say so. Covered by a test that adds an unknown channel and asserts it appears in both. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): drop two dead Candidate fields Neither has a reader, and the plan removes both. `optimization_params` was added by an earlier commit in this branch as HPO's genotype slot. §3.1 now puts HPO parameters in `parameters.json` inside the candidate artifact instead, so Layer A never interprets them and `Candidate` gains no component-specific field. `artifacts: dict[str, Any]` was the untyped `exclude=True` dict used to pass directory paths around at runtime. It has no readers left; the three `model_dump(exclude={"artifacts"})` call sites go with it. Corrects the record: the commit that introduced the reward map claimed the `artifacts` dict was "deleted outright". It was not — it survived until now. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): delete _with_reward, use Candidate.set_reward `_with_reward()` was a second implementation of the channel merge that `Candidate.set_reward` already performs — same "take the existing record, apply the parts that were supplied, put it back" logic, written twice. It existed only because `_update_candidate(candidate, updates={...})` wants a *value* for the field, while `set_reward` mutates. But `_update_candidate` merely `setattr`s the updates onto the candidate before persisting, so mutating first is equivalent: candidate.set_reward("validation", metrics=..., trials=...) await self._update_candidate(candidate, workspace=..., backend=..., run_id=...) One merge implementation, on the entity that owns the field, and the five call sites say what they do instead of assembling a dict to hand to a setattr loop. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): one home for the insight-suite readers `_suite_identity` and `_metric_keys` were defined identically in both `loop.py` and `insight_promotion.py` — a duplication I introduced when the insight sidecars moved from dedicated `Candidate` fields into the `insight` channel's `metadata`. They now live once, in `insight_promotion.py`, as `candidate_suite_identity` and `candidate_metric_keys`, and `loop.py` imports them alongside the other insight-suite helpers it already takes from there. Deliberately not methods on `Candidate`. `suite_identity` is the content hash of the Eval Author's generated suite, used to decide whether a cached insight reward was measured against the suite currently in play; `metric_keys` are that suite's validated metric names. Both are insight-suite semantics, and Layer A stores a channel's `metadata` without interpreting it — the same rule §3.1 applies to HPO's payload. Putting them on the entity would re-add the component-specific coupling the channel map removed. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * review: address MR feedback on docstrings, config fields, and a duplicate - `entities.py` docstring drops the "Layer A"/"Layer B" vocabulary and the note about where these models used to live. Both only make sense with the design doc open, and the file should read on its own. - `EvolutionaryOptimizerConfig` and `ModelsConfig` fields carry descriptions, so the run config is self-documenting. - `_evaluate_insight_candidates` had `"insight" not in candidate.rewards` twice. It came from translating `insight_reward is None or insight_reward_details is None` — two fields that became one channel. Removed the duplicate and left a note on why one check is now correct: a RewardRecord holds metrics and trials together, and empty trials are valid cached state rather than a missing measurement. - The evolution-tree table test asserts the whole rendered table instead of scattered substrings. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): adopt NemoConfig for deployment settings The Experimentalist was the only plugin in the repo configuring itself with a bare BaseModel and an ad-hoc EXPERIMENTALIST_* prefix; ten others subclass NemoConfig. Split the configuration into the two kinds it always was, and give the deployment half the platform's treatment. ExperimentalistConfig (new, settings.py) holds the endpoint, its credential and the model tiers. As a NemoConfig it gets the NEMO_EXPERIMENTALIST_ env prefix, an `experimentalist:` section in the platform config file, and the platform's precedence: env, then file, then defaults. It lives in its own module rather than config.py because config.py imports each component's config slice, and those components resolve models through components/model_config.py -- so the settings have to sit below the components, not above them. EvolutionaryOptimizerConfig keeps holding run parameters and stays a plain BaseModel with no env binding at all. A stale NEMO_EXPERIMENTALIST_MAX_ROUNDS truncating a run whose --config says 15 would be a bad failure, and it would make config_snapshot a dishonest record of what ran. Its `models:` key is now rejected outright rather than silently ignored. This deletes ModelsConfig.apply_to_env(), which wrote config values into os.environ so module-level getters could see them -- inverting precedence so that config silently beat anything the operator had exported. EXPERIMENTALIST_* becomes NEMO_EXPERIMENTALIST_*, and the tier names become NEMO_EXPERIMENTALIST_MODELS_{SMART,MID,FAST}. No compatibility shim: this is pre-1.0, and two accepted names for one variable is worse than one rename. Eval Author's bridge retargets to the new destinations and now clears the settings cache after writing, so it is no longer order-dependent. The test conftest gains placeholder tier names; the suite had been relying on a developer .env being loaded by the nooa import to supply them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): rename the env vars the onboarding path actually uses The .env.example the getting-started guide tells you to copy still exported the old names, so a fresh run of docs/get-started/example-agent.mdx would have found no model tiers. Dotfiles were missed by the first sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * test(eval-author): pin that bridging survives an already-resolved settings cache Verified to fail without the Configuration.clear_cache() in the bridge: this is the ordering that used to silently no-op in a real run, where something resolves a model before EvalAuthor.__init__ gets to bridge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * docs(experimentalist): add the plugin config to the config reference Registers ExperimentalistConfig in the generator and regenerates docs/set-up/config-reference.mdx, so the endpoint and model tiers are discoverable alongside every other service's settings rather than only in the plugin README. Field descriptions are written for that audience: which models these are (the ones doing the optimizing, not the agent under test) and a pointer to set the credential through the environment rather than the config file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * docs(experimentalist): give the tiers different models in the README example The example exported the same model for smart, mid and fast, which makes the tiering look pointless — the reviewer reasonably asked whether smart was meant to be smarter. Use the same spread the example agent's .env.example already uses, and say what the tiers are buying. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): log why a candidate build failed, not just that it did `return_exceptions=True` turns a build failure into a value rather than a raise, so the exception was discarded and the log said only "Impl failed: agent-1". That is not diagnosable after the fact, and a killed candidate is the one thing a run cannot cheaply reproduce. Keep the exception and log it with a traceback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(eval-author): drop the mode argument left dangling by the backend removal Deleting the remote backend removed `mode` from `make_experimentalist_backend`, but `run_eval_author` still passed `mode=mode` — a TypeError on every insight-mode run. `mode` had no other use, so the parameter goes too. The unit test hid this: its fake backend factory declared `mode` and accepted the argument, so 646 tests stayed green over a call that could not work. The fake now mirrors the real signature. CI's `lint-python-types` caught it because `unknown-argument` is not in its suppression list, while the pre-commit `ty` hook runs without the insights/experimentalist dependency groups and never resolved it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * fix(experimentalist): repoint the example trace recorder at the moved entities Moving Layer A out of components/evaluator/ orphaned the import in record_tau_airline_traces.py, which is step 3 of the getting-started guide — a new user following the guide hit ModuleNotFoundError before recording anything. The same stale path appeared in Eval Author's README. Nothing caught this: plugins/nemo-experimentalist/examples/ is in ty's exclude list and no test imports the script, so a green test suite and a green `Lint all` said nothing about it. Found by actually running the guide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): a method's tier reads off the instance, not the env Three `@strategy` methods pick a non-default tier. They did it with `LazyModel`, a proxy resolving from the environment at first use, because a decorator argument is evaluated at import time and there is no instance to read config from then. That is the same import-time binding this milestone set out to remove -- deferred rather than eliminated -- and it is why the environment is still a transport. nooa now accepts a callable for `@strategy(llm=...)`, resolved on each generation call with the agent in scope, so the tier reads off the instance like every other model these components use. The dependency bump is not separable from the removal. nooa duck-type-probes the `llm=` value for `acall`; on a forwarding proxy that fires `__getattr__` and builds a client at import, breaking the guarantee that importing a component needs no credentials -- which registry discovery and `doctor` depend on, and which this milestone's own test asserts. That test catches it immediately. The pin moves to the current nooa main. It stays a commit rather than a tag because v0.0.8 predates the support; the comment above it now says so instead of citing the MCP fix it was originally written for. Both lock files move with it. Also pins what the same upstream change fixed in passing: `llm=` was stripped from call kwargs only for `_session_locals`, so the per-call precedence `actor.py` documents was unreachable for a direct call. The tier choice does not use it, but a regression there would be silent. The ty override that exempts nooa's ellipsis-bodied methods gains this one test module, which defines throwaway agents for the same reason. Signed-off-by: Severin Klingler <sklingler@nvidia.com> * review: address bot findings on the banner, cancellation, and stale pins Nine of sixteen review comments were valid. The two that were real defects: `log_model_config` guarded the tiers but resolved the endpoint and key unguarded, so the banner raised on exactly the unconfigured install it exists to diagnose. Every field is optional there now; callers that need a value still fail in `api_base()` / `api_key()`. `asyncio.gather(..., return_exceptions=True)` returns `CancelledError`, which derives from BaseException, so the `isinstance(r, Exception)` filter classified a cancelled Coder as neither failed nor succeeded and let the candidate through to evaluation and ranking as though its source had been written. Cancellation now re-raises and unwinds the round. Pre-existing on main; fixed here because this branch already rewrote that block. Both have regression tests, each verified to fail with its fix reverted. Also: `candidate_metric_keys` coerced non-strings, so malformed cached metadata (`[1]`) passed as a valid measurement and skipped a fresh Insight evaluation -- it now reads as "not recorded"; the mirror test asserted only on the entity and would have passed if `project_candidate` reverted to a fixed channel allowlist, so it projects through a real mirror now; Eval Author's autouse fixture clears `Configuration` alongside the client caches, since bridged endpoints outlive `monkeypatch`; the README example imports `Dataset` from `entities`; a duplicated env prefix in AGENTS.md; a prompt referring to `insight_reward` where its own code block defines `insight_rewards`; the nooa rev in `third_party/requirements-main.txt` and the framework skill; and the api_base doc row, which claimed "required" while the CLI defaults it to the gateway. Declined: requiring HTTPS for the API base (NVIDIA-NeMo#3, NVIDIA-NeMo#8) would break local endpoints, and the actual credential-forwarding risk is already scheme-guarded in `_is_gateway_base`. The dotenv-linter ordering finding (NVIDIA-NeMo#7) cites a hook this repo does not configure. The conftest credential-namespace overlap (NVIDIA-NeMo#4) is real but pre-existing and identical on main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * docs(experimentalist): drop the stale import-time credential claim The comment said importing experimentalist.run needs NEMO_EXPERIMENTALIST_API_* at import time. That stopped being true in this branch -- components resolve their tier when constructed, not imported, which is what test_importing_components_resolves_no_model asserts. Verified: with every credential and tier variable unset, the module imports and builds zero clients. It survived because the rename rewrote the variable names in it instead of noticing the sentence was obsolete. The lazy import stays, but for import cost rather than credentials, and it only defers ~85 ms since cli.py already pulls in most of the same chain. One line is enough; the mechanism goes away in M1 when the runner collapses the _flow closure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * refactor(experimentalist): one way to read a channel, and an honest name for the writer Two spellings of the same read existed: `reward(ch)` and, twice in insight_promotion.py, `rewards.get(ch, RewardRecord())`. Collapsed to the former; its `RewardRecord` import is now dead and goes too. `set_reward` merges — an argument left as None keeps what the channel already holds — so `set_` was the wrong verb. It is `record_reward` now. Keeping the merge rather than dropping it to a replace: every caller writes a channel exactly once today, so nothing exercises it, but the channel set is open by design and a second writer adding metadata to a channel another path measured would silently lose its metrics. That is the kind of loss no test catches, so the defensive default is worth the unused branch. Two tests pin it, plus the unmeasured-versus- measured-empty distinction that eight evaluation gates depend on. `rewards` stays public: it is the serialized key in metadata.json and the entity store, `channel in rewards` is load-bearing at those eight sites and cannot be expressed through the accessor, and there is no cross-channel invariant to protect. Collapsing the two readers into one mapping whose `__missing__` returns without storing is the better shape, but it needs a Pydantic core schema (a bare dict subclass is rejected) and touches ~55 sites, so it is noted in M1 alongside the lifecycle change that removes the merge's reason to exist. Explicitly not a defaultdict, whose `__missing__` inserts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * test(experimentalist): drop the nooa contract tests and their ty override test_method_llm_injection.py defined throwaway agents to re-derive nooa's own `@strategy(llm=...)` resolution. Three of its four cases pinned behaviour this plugin does not use -- the default fallback, the baked-on decorator client the callable replaced, and a call-level override its own docstring called "not what the tier choice will use". Only the callable case matched a production site, and pinning a dependency's API surface is not this repo's job. The docstrings also cited an external PR number and another repo's `actor.py`, which rot on the next pin bump. Its pyproject ty override goes with it: the `empty-body` exemption is back to the two source trees, since the entry existed only for that module's ellipsis-bodied throwaways. The gap it half-covered is real -- test_experimentalist_analyzer.py builds its analyzer with object.__new__ and assigns over the strategy methods, so nothing proves a method-level tier reaches the model it names. Recorded in M1, where config injection makes it testable against a real component instead of a stand-in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> * test(experimentalist): inject settings instead of driving them through env Most of test_model_config.py set NEMO_EXPERIMENTALIST_* to get a value in place. Only two tests are actually about *where* a value comes from -- the config-file one and the env-overrides-file one -- so the rest now install an ExperimentalistConfig with Configuration.set_override and stop caring about the environment entirely. That also settles the isolation concern raised on the config-file test: an override bypasses file loading, so those tests no longer depend on whether /etc/nmp/config.yaml happens to exist on the machine. The helper clears the plugin's env first, and that is load-bearing rather than tidiness: these settings are env-first by design, so constructing ExperimentalistConfig(models=...) would otherwise read conftest's import-time placeholders over the values the test passes. The autouse fixture snapshots and restores os.environ, and now clears Configuration overrides too, since an override outlives the test that set it exactly as a cached config does. Also drops _CHANNEL_ABBREV. It shortened two of three channel names -- "train" mapped to itself -- and was a hardcoded closed list of channels in a change whose point is that the channel set is open. Since it fell back to the raw name for anything it did not know, tables already mixed abbreviated and full names as soon as a third party added a channel. Rendering every channel by its own name is wider and consistent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Severin Klingler <sklingler@nvidia.com> --------- Signed-off-by: Severin Klingler <sklingler@nvidia.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation