feat(eval-author): implement the discover command - #988
Open
aleckhoury wants to merge 12 commits into
Open
Conversation
Base automatically changed from
ase-685-eval-author-cli-scaffolding/akhoury
to
main
July 30, 2026 14:57
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the ChangesEval Author discovery
Sequence Diagram(s)sequenceDiagram
participant User
participant EvalAuthorCLI
participant discover
participant HarborValidation
participant HarborFileset
User->>EvalAuthorCLI: Run discover with repository options
EvalAuthorCLI->>discover: Submit DiscoverOptions
discover->>HarborFileset: Load prior discovery report
discover->>HarborValidation: Assemble and validate candidate config
HarborValidation-->>discover: Return findings and validation outcome
discover->>HarborFileset: Persist discovery.md and optional harbor-job.yaml
HarborFileset-->>discover: Return persistence findings
discover-->>EvalAuthorCLI: Return DiscoverResult
EvalAuthorCLI-->>User: Print verdict, findings, and run instructions
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/sources.py (1)
425-439: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
_find_wrapperreturns whicheverharbor_wrapper.pywalk_dirsyields first.Elsewhere in this module ordering is pinned deliberately (
sorted(directory.iterdir()),sorted(repo_root.rglob(...))invalidate._module_search_path). A repo with two wrappers would get a run-to-run-unstable agent import path and a persisted artifact that changes without the repo changing. Sorting the candidates makes the choice reproducible.♻️ Deterministic wrapper selection
- for directory in walk_dirs(root): - path = directory / _WRAPPER_FILENAME - if not path.is_file(): - continue - class_name = _agent_class_name(path) + for directory in sorted(walk_dirs(root)): + path = directory / _WRAPPER_FILENAME + if not path.is_file(): + continue + class_name = _agent_class_name(path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/sources.py` around lines 425 - 439, Update _find_wrapper to collect all valid harbor_wrapper.py candidates discovered by walk_dirs, order them deterministically by path, and return the first sorted candidate with its parsed class name. Preserve the existing behavior of ignoring files without an agent class and returning None when no valid wrapper exists.plugins/nemo-eval-author/tests/discover/test_validate.py (1)
281-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two round-trip tests are green whether or not they tested anything.
With no
harboron PATH both collapse to awarnbranch that asserts nothing about the round trip. Askipifmakes the gap visible in CI output instead of counting as coverage.♻️ Skip instead of passing vacuously
+import shutil + +import pytest + +requires_harbor_cli = pytest.mark.skipif(shutil.which("harbor") is None, reason="no harbor executable on PATH") + + +@requires_harbor_cli async def test_the_persisted_config_is_checked_through_the_harbor_cli(tmp_path): ... - assert finding.status in {"pass", "warn"} - if finding.status == "pass": - assert finding.harbor_call == "harbor job start --print-config" + assert finding.status == "pass" + assert finding.harbor_call == "harbor job start --print-config"Same for the rejection test.
🤖 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-eval-author/tests/discover/test_validate.py` around lines 281 - 301, Update both round-trip tests, test_the_persisted_config_is_checked_through_the_harbor_cli and test_a_config_the_cli_rejects_fails_the_round_trip, to skip explicitly when the harbor executable is unavailable instead of accepting the warn result. Detect the executable using the project’s existing subprocess/path lookup convention, apply skipif or an equivalent test skip before assertions, and keep the remaining assertions focused on successful validation or rejection when harbor is available.plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/memory.py (1)
125-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
restampparses the front matter twice.
_split_front_matter(text)andparse_front_matter(text)both internally re-split/re-parse the same text. Minor duplication; could reusesplit[0]to derivefrontinstead of a second full parse.🤖 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-eval-author/src/nemo_eval_author_plugin/discovery/memory.py` around lines 125 - 145, Update restamp so it splits and parses the front matter only once: reuse the successful result from _split_front_matter(text) and derive front from split[0] instead of calling parse_front_matter(text) separately. Preserve the existing None fallback when splitting or front-matter parsing fails, along with the current timestamp, Harbor version, and rendering behavior.plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/run.py (1)
267-271: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnfiltered
rglobacross the whole repo tree.
repo_root.rglob(f"{module}.py")walks every directory (including vendor/dependency trees likenode_modules,.venv,.gitif present) to fingerprint agent-module inputs. On larger repos this can be a slow, repeated I/O scan on every full (non-cached) discovery run.♻️ Suggested fix
- paths.extend(sorted(repo_root.rglob(f"{module}.py"))) + paths.extend( + sorted( + p for p in repo_root.rglob(f"{module}.py") + if not any(part in {".git", ".venv", "node_modules", "__pycache__"} for part in p.parts) + ) + )🤖 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-eval-author/src/nemo_eval_author_plugin/discovery/run.py` around lines 267 - 271, Restrict the module-file search in the outcome.config agent loop to repository source paths rather than scanning all of repo_root with rglob. Update the paths collection around agent_config.import_path to use the existing repository ignore/exclusion mechanism, or otherwise prune vendor, dependency, VCS, and virtual-environment directories while preserving discovery of valid agent module files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/cli.py`:
- Around line 45-58: Update _report_discovery so the result.reused branch also
iterates over and prints result.memory_findings using the existing _GROUP_ORDER
and _echo_finding flow, while preserving the revalidation message and existing
non-reused behavior.
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/agent.py`:
- Around line 46-54: Move the ProposeFn type alias below the ScoutProposal class
definition, then change Awaitable["ScoutProposal"] to use the concrete
ScoutProposal reference without quotes while preserving the existing callable
signature.
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/memory.py`:
- Around line 106-122: Update the prior-record construction after
parse_front_matter in load_previous to safely handle YAML-coerced values: coerce
timestamp-like and other optional string fields to str or None, and parse
runnable so quoted "false" remains False rather than using bool() directly.
Ensure any invalid record or construction error follows the existing fallback
behavior and load_previous returns None instead of raising.
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/run.py`:
- Around line 177-191: Update the reused-report reconstruction in the discovery
flow around DiscoveryReport.model_validate to extract and preserve
required_env_vars from the persisted front matter, along with the existing
report fields. Ensure cached or dry-run results retain the original required
environment variables and findings so cli.py’s _report_discovery continues to
display them.
- Around line 193-208: Distinguish the harmless withheld-config finding from
genuine upload warnings in memory.persist by assigning the former a unique name
such as config_withheld while retaining upload for actual failures. Update the
memory_findings filter in DiscoverResult within the reuse path to remove only
the config_withheld finding, preserving real upload warnings.
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/validate.py`:
- Around line 264-306: Update _rung_coverage to resolve each dataset.path
against repo_root before checking is_dir, preserving absolute paths while
anchoring repo-relative paths correctly. Store the result as dataset_path and
use it consistently for is_dir, iterdir, the coverage message, and the finding’s
path= value.
- Around line 204-218: The _resolved_local_paths function should distinguish a
missing job._task_configs attribute from an empty task list. Detect the absent
attribute and return or propagate the established compatibility error before
iterating; continue treating an explicitly present empty _task_configs
collection as empty.
---
Nitpick comments:
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/memory.py`:
- Around line 125-145: Update restamp so it splits and parses the front matter
only once: reuse the successful result from _split_front_matter(text) and derive
front from split[0] instead of calling parse_front_matter(text) separately.
Preserve the existing None fallback when splitting or front-matter parsing
fails, along with the current timestamp, Harbor version, and rendering behavior.
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/run.py`:
- Around line 267-271: Restrict the module-file search in the outcome.config
agent loop to repository source paths rather than scanning all of repo_root with
rglob. Update the paths collection around agent_config.import_path to use the
existing repository ignore/exclusion mechanism, or otherwise prune vendor,
dependency, VCS, and virtual-environment directories while preserving discovery
of valid agent module files.
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/sources.py`:
- Around line 425-439: Update _find_wrapper to collect all valid
harbor_wrapper.py candidates discovered by walk_dirs, order them
deterministically by path, and return the first sorted candidate with its parsed
class name. Preserve the existing behavior of ignoring files without an agent
class and returning None when no valid wrapper exists.
In `@plugins/nemo-eval-author/tests/discover/test_validate.py`:
- Around line 281-301: Update both round-trip tests,
test_the_persisted_config_is_checked_through_the_harbor_cli and
test_a_config_the_cli_rejects_fails_the_round_trip, to skip explicitly when the
harbor executable is unavailable instead of accepting the warn result. Detect
the executable using the project’s existing subprocess/path lookup convention,
apply skipif or an equivalent test skip before assertions, and keep the
remaining assertions focused on successful validation or rejection when harbor
is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f7b4f6c2-ef49-412b-ac05-aeadea0548df
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
plugins/nemo-eval-author/pyproject.tomlplugins/nemo-eval-author/src/nemo_eval_author_plugin/cli.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/agent.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/memory.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/models.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/report.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/run.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/scan.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/sources.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/validate.pyplugins/nemo-eval-author/tests/discover/test_command.pyplugins/nemo-eval-author/tests/discover/test_memory.pyplugins/nemo-eval-author/tests/discover/test_report.pyplugins/nemo-eval-author/tests/discover/test_scan.pyplugins/nemo-eval-author/tests/discover/test_scout.pyplugins/nemo-eval-author/tests/discover/test_sources.pyplugins/nemo-eval-author/tests/discover/test_validate.pyplugins/nemo-eval-author/tests/harbor_fixtures.pyplugins/nemo-eval-author/tests/test_cli.py
Comment on lines
+193
to
+208
| persisted, memory_findings = await memory.persist( | ||
| client, | ||
| agent=agent, | ||
| workspace=options.workspace, | ||
| markdown=restamped, | ||
| # The stored config is already correct and already uploaded; rewriting it would | ||
| # churn the fileset to produce identical bytes. | ||
| job_config=None, | ||
| ) | ||
| return DiscoverResult( | ||
| report=record, | ||
| markdown=restamped, | ||
| persisted=persisted, | ||
| reused=True, | ||
| memory_findings=[item for item in memory_findings if item.name != "upload" or item.status != "warn"], | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Overbroad filter can silently drop real upload failures during reuse.
The filter item.name != "upload" or item.status != "warn" is meant to strip the harmless "config withheld" info message that persist() always emits when job_config=None — but real upload failures from persist() (lines 188-198 in memory.py) use the exact same name="upload", status="warn" pair. This filter can't distinguish "nothing to upload" from "the discovery.md re-upload failed," so a genuine fileset outage during a cached/reused run is silently discarded from memory_findings.
Give the two Findings distinct names (e.g. "upload" vs "config_withheld") so the filter can target only the intended one.
🔧 Suggested fix
- findings.append(
- Finding(
- name="upload",
- group=_GROUP,
- status="warn",
- message=f"Withheld {JOB_CONFIG_FILENAME}: no config passed Harbor's schema check",
- hint="A config in this fileset is always one Harbor could load, so none was written.",
- )
- )
+ findings.append(
+ Finding(
+ name="config_withheld",
+ group=_GROUP,
+ status="warn",
+ message=f"Withheld {JOB_CONFIG_FILENAME}: no config passed Harbor's schema check",
+ hint="A config in this fileset is always one Harbor could load, so none was written.",
+ )
+ )(in memory.py, persist()), then in run.py:
- memory_findings=[item for item in memory_findings if item.name != "upload" or item.status != "warn"],
+ memory_findings=[item for item in memory_findings if item.name != "config_withheld"],🤖 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-eval-author/src/nemo_eval_author_plugin/discovery/run.py` around
lines 193 - 208, Distinguish the harmless withheld-config finding from genuine
upload warnings in memory.persist by assigning the former a unique name such as
config_withheld while retaining upload for actual failures. Update the
memory_findings filter in DiscoverResult within the reuse path to remove only
the config_withheld finding, preserving real upload warnings.
Comment on lines
+264
to
+306
| def _rung_coverage(config: JobConfig, resolved: list[Path], outcome: ValidationOutcome) -> None: | ||
| """Did Harbor pick up every task the repo appears to contain? | ||
|
|
||
| Harbor drops a directory it cannot parse as a task without saying so: given ten task | ||
| dirs where three are half-written, ``Job.create`` resolves seven and raises nothing. | ||
| The run then succeeds and reports a score over seven tasks, which is the most expensive | ||
| kind of wrong, because nothing about the output looks incomplete. | ||
|
|
||
| A dataset that narrows itself with ``task_names``, ``exclude_task_names`` or ``n_tasks`` | ||
| is deliberately running a subset, so its unresolved directories are reported as | ||
| information rather than as a problem. | ||
| """ | ||
| resolved_set = {path.resolve() for path in resolved} | ||
| for dataset in config.datasets: | ||
| if dataset.path is None or not dataset.path.is_dir(): | ||
| continue | ||
| on_disk = [ | ||
| child | ||
| for child in sorted(dataset.path.iterdir()) | ||
| if child.is_dir() and (child / _TASK_CONFIG_FILENAME).is_file() and child.name != _TEMPLATE_DIR_NAME | ||
| ] | ||
| dropped = [child for child in on_disk if child.resolve() not in resolved_set] | ||
| if not dropped: | ||
| continue | ||
|
|
||
| selective = dataset.task_names or dataset.exclude_task_names or dataset.n_tasks | ||
| listed = ", ".join(f"{child.name} ({_task_failure_reason(child)})" for child in dropped[:_MAX_LISTED_DROPPED]) | ||
| remainder = len(dropped) - _MAX_LISTED_DROPPED | ||
| message = f"{len(dropped)} of {len(on_disk)} task dir(s) under {dataset.path.name} were not picked up: {listed}" | ||
| outcome.findings.append( | ||
| _harbor( | ||
| "coverage", | ||
| "warn" if selective else "fail", | ||
| message + (f", and {remainder} more" if remainder > 0 else ""), | ||
| "Job.create", | ||
| path=dataset.path, | ||
| hint=( | ||
| "The dataset selects a subset, so this is expected." | ||
| if selective | ||
| else "Harbor skips these silently, so a run would score fewer tasks than the repo contains." | ||
| ), | ||
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The coverage rung silently no-ops for a config with repo-relative dataset paths.
dataset.path.is_dir() resolves against the process cwd. Assembled candidates carry absolute paths, but a user-written configs/eval.yaml (the highest-priority source) typically says path: evals/validation, and discover does not run from repo_root. The rung whose whole purpose is catching tasks Harbor drops then quietly checks nothing. Resolve against repo_root before the is_dir test.
🐛 Anchor the dataset path to the repo root
-def _rung_coverage(config: JobConfig, resolved: list[Path], outcome: ValidationOutcome) -> None:
+def _rung_coverage(config: JobConfig, resolved: list[Path], repo_root: Path, outcome: ValidationOutcome) -> None:
...
resolved_set = {path.resolve() for path in resolved}
for dataset in config.datasets:
- if dataset.path is None or not dataset.path.is_dir():
+ if dataset.path is None:
+ continue
+ dataset_path = dataset.path if dataset.path.is_absolute() else repo_root / dataset.path
+ if not dataset_path.is_dir():
continueand use dataset_path for iterdir(), the message, and 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-eval-author/src/nemo_eval_author_plugin/discovery/validate.py`
around lines 264 - 306, Update _rung_coverage to resolve each dataset.path
against repo_root before checking is_dir, preserving absolute paths while
anchoring repo-relative paths correctly. Store the result as dataset_path and
use it consistently for is_dir, iterdir, the coverage message, and the finding’s
path= value.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/report.py`:
- Around line 188-197: Update _verdict_section so the runnable-report message
says Harbor accepted the validation checks, rather than claiming every check was
answered by Harbor. Preserve the existing config provenance detail and
report.harbor_version information.
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/validate.py`:
- Around line 99-111: Remove the process-wide contextlib.chdir around _ladder in
run_ladder; do not hold a global working-directory change across awaits.
Preserve repository-root-relative resolution by isolating the path-dependent
Harbor validation in a subprocess or by passing explicit repo_root-resolved
paths into the Harbor calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5f8dcde0-9234-4ff9-b175-ce7fc7ea25ff
📒 Files selected for processing (14)
plugins/nemo-eval-author/src/nemo_eval_author_plugin/cli.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/agent.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/memory.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/models.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/report.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/run.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/sources.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/validate.pyplugins/nemo-eval-author/tests/discover/test_command.pyplugins/nemo-eval-author/tests/discover/test_memory.pyplugins/nemo-eval-author/tests/discover/test_report.pyplugins/nemo-eval-author/tests/discover/test_sources.pyplugins/nemo-eval-author/tests/discover/test_validate.pyplugins/nemo-eval-author/tests/harbor_fixtures.py
🚧 Files skipped from review as they are similar to previous changes (6)
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/sources.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/run.py
- plugins/nemo-eval-author/tests/discover/test_memory.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/cli.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/agent.py
- plugins/nemo-eval-author/tests/discover/test_command.py
Discovery exists so a later, cheaper model can run a repo's Harbor evals without re-deriving how. That only works if every fact it records was proved rather than observed, so the command assembles a candidate JobConfig from the strongest source the repo offers and then makes Harbor's own validators judge it: schema, Job.create resolution, per-task validity, the reward-file contract, required host vars, the agent import path, backend preflight, and a round trip of the persisted bytes through the Harbor CLI. The config and the report are therefore two separate claims. discovery.md is uploaded either way so a failure is documented instead of lost, while harbor-job.yaml is withheld unless every rung passed, which keeps the fileset from ever holding a config that reads as ready to run. Exit 0 likewise means validated *and* recorded, so the command works as a gate. Harbor drops task directories it cannot parse without raising, so a per-task check alone would report every surviving task valid while one silently vanished. A coverage rung diffs the resolved tasks against what is on disk to surface that, staying quiet when the dataset filters tasks on purpose. The LLM scout is bounded by the same ladder: its proposal is revalidated from the schema rung and adopted only if Harbor then accepts it, so a wrong guess costs a round trip rather than replacing failures the user could have acted on. Anything it influenced is marked as inference in the report. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Each file was named for the module it covers with a test_discovery_ prefix repeating what the directory can say instead, so the prefix is now the directory. Two names could not follow the modules they test. The CLI file is test_command.py rather than test_cli.py because the plugin already has a tests/test_cli.py, and running the plugin's suite on its own leaves pytest in prepend import mode, where two test files sharing a basename abort collection. The fixtures module has to stay at the tests/ root. The repo-root conftest forces importmode = importlib, under which pytest puts no test directory on sys.path; tests/ is there only because pytest loads that directory's conftest during startup, while prepend mode still applies. A helper inside discover/ imports fine from within the plugin and then fails the moment CI runs from the repo root, so both configs are now part of what gets checked. It keeps the harbor_ prefix for the same class of reason: every plugin's tests/ shares sys.path in that run, so a bare fixtures module would bind to whichever copy loaded first. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
A repo that maintains its own job config had that file copied into the fileset, leaving two configs describing one run and no owner for the second. Discovery now publishes a config only when it authored one; when the repo owns the file, the report points at it and validates that path directly. Front matter carries a run_config block so a consumer reads one field instead of guessing which case it is in, and the report says why a config file is involved at all: -c is Harbor's only interface for local task directories. Ownership ends the moment we change the payload, so --env-backend injection and scout proposals mark the source adjusted and fall back to publishing our copy. Two bugs surfaced while testing the repo-owned path. The in-process ladder judged relative dataset paths against the process directory, so a repo config saying `path: evals/validation` failed resolution while `harbor job start -c` from the repo root would have run it; the ladder now runs with cwd at the repo root, like the subprocess round trip already did. That exposed Job.create opening a job.log under a relative jobs/ dir, which resolution now redirects to scratch space so discovery leaves the repo untouched. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
…on ladder `validate` wrapped run_ladder in asyncio.run for a CLI that turned out to be async, and `temp_logs_dir` was exposed for tests that never used it. Nothing imports either. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
aleckhoury
force-pushed
the
ase-677-eval-author-discover-command/akhoury
branch
from
July 31, 2026 16:28
b7719f1 to
72549f7
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/validate.py (1)
301-331: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStore an absolute dataset path in the coverage finding.
dataset.pathcan be repo-relative. Insiderun_ladderthe chdir makesis_dir()anditerdir()correct, butpath=dataset.pathoutlives that scope._front_matterand_verdict_sectionlater calldisplay_path(...)on it after the chdir is undone, so the finding can point at a path that does not exist from the caller's directory. Resolve once and reuse.🐛 Proposed fix
resolved_set = {path.resolve() for path in resolved} for dataset in config.datasets: - if dataset.path is None or not dataset.path.is_dir(): + if dataset.path is None: + continue + dataset_path = dataset.path.resolve() + if not dataset_path.is_dir(): continue on_disk = [ child - for child in sorted(dataset.path.iterdir()) + for child in sorted(dataset_path.iterdir()) if child.is_dir() and (child / _TASK_CONFIG_FILENAME).is_file() and child.name != _TEMPLATE_DIR_NAME ] @@ - message = f"{len(dropped)} of {len(on_disk)} task dir(s) under {dataset.path.name} were not picked up: {listed}" + message = f"{len(dropped)} of {len(on_disk)} task dir(s) under {dataset_path.name} were not picked up: {listed}" outcome.findings.append( _harbor( "coverage", "warn" if selective else "fail", message + (f", and {remainder} more" if remainder > 0 else ""), "Job.create", - path=dataset.path, + path=dataset_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-eval-author/src/nemo_eval_author_plugin/discovery/validate.py` around lines 301 - 331, Resolve dataset.path to an absolute path before constructing the coverage finding, then reuse that resolved value for the directory checks, iteration, message, and _harbor path argument within this loop. Update the relevant dataset-path handling around the coverage finding so later _front_matter and _verdict_section calls receive a stable absolute path after the working directory is restored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/validate.py`:
- Around line 301-331: Resolve dataset.path to an absolute path before
constructing the coverage finding, then reuse that resolved value for the
directory checks, iteration, message, and _harbor path argument within this
loop. Update the relevant dataset-path handling around the coverage finding so
later _front_matter and _verdict_section calls receive a stable absolute path
after the working directory is restored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b672265a-dbbc-4ea1-bd7a-d4674629333c
📒 Files selected for processing (19)
plugins/nemo-eval-author/pyproject.tomlplugins/nemo-eval-author/src/nemo_eval_author_plugin/cli.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/agent.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/memory.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/models.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/report.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/run.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/scan.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/sources.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/validate.pyplugins/nemo-eval-author/tests/discover/test_command.pyplugins/nemo-eval-author/tests/discover/test_memory.pyplugins/nemo-eval-author/tests/discover/test_report.pyplugins/nemo-eval-author/tests/discover/test_scan.pyplugins/nemo-eval-author/tests/discover/test_scout.pyplugins/nemo-eval-author/tests/discover/test_sources.pyplugins/nemo-eval-author/tests/discover/test_validate.pyplugins/nemo-eval-author/tests/harbor_fixtures.pyplugins/nemo-eval-author/tests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (11)
- plugins/nemo-eval-author/tests/discover/test_validate.py
- plugins/nemo-eval-author/tests/test_cli.py
- plugins/nemo-eval-author/tests/discover/test_scout.py
- plugins/nemo-eval-author/tests/discover/test_report.py
- plugins/nemo-eval-author/tests/discover/test_memory.py
- plugins/nemo-eval-author/tests/discover/test_command.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/scan.py
- plugins/nemo-eval-author/tests/discover/test_scan.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/memory.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/sources.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/discovery/run.py
Contributor
|
--deep said how hard the command tries; --fix says what it does, which is edit the config until Harbor accepts it. Now off by default, so the plain command reads a repo and reports, and reaching a model is something the caller asks for. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
--workspace, --base-url and --env-backend let one invocation contradict the configuration every other nemo command obeys. The cluster, credentials and workspace now come from the active context, and the Harbor environment backend is left to the repo's own config or Harbor's default. The report still records which workspace it wrote to, since a reader needs to name one to download the artifact. Dropping the backend injection also leaves the scout as the only thing that can mark a config source adjusted, which is the case that genuinely changes what a later run should pass to -c. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Addresses review feedback on #988. The freshness gate that lets a repeat run skip the Harbor validation ladder was the weak point. load_previous could raise on a foreign report instead of declining it, contradicting its own docstring, and any truthy runnable value counted as permission to skip, so a run could exit zero having validated nothing. The input digest also hashed module copies found under .venv, tying a valid report's lifetime to a dependency reinstall. Beyond the gate: a reused report keeps the host variables a run needs, the reuse path explains an upload failure instead of only reporting one, a missing Job._task_configs is reported as our Harbor incompatibility rather than as the user's empty repo, and the verdict no longer claims Harbor answered checks that came from the filesystem or the scout. Also pins NMP_CONFIG_FILE in the discover fixture so the suite stops reading whichever platform context the developer happens to have, names the withheld-config finding for what it is rather than reusing "upload", records that discover takes one call per process because the ladder chdirs and mutates sys.path, points the doctor placeholder at ASE-769, and brings both READMEs in line with the command as it now stands. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
…the README The scout this flag enables runs shell commands in the repo under inspection with no sandbox, and it chooses those commands by reading files from that same repo, so hostile content there can steer it. A flag called --fix reads as routine maintenance. The name should carry the risk. The negative form is dropped rather than renamed, since --no-dangerously-fix is noise when the default is already off; test_the_scout_is_opt_in still pins that default. The README now leads with active development and not for production use, and names the hazard rather than leaving a reader to assume it means bugs: validating a config imports the target repo's agent module into the running process, so pointing Eval Author at a repository is equivalent to running that repository's code. Sandboxing is not implemented yet. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Review found the command over-scoped. What comes out here was either untrustworthy, never read, or big enough to deserve a change of its own. The --dangerously-fix scout is gone. Handing a language model an unsandboxed shell in the repo under inspection is a decision that needs its own argument and its own review, not a flag riding along with discovery. ConfigSource .adjusted and the "inference" provenance go with it, since only the scout could ever set either. The report freshness layer is gone. It fingerprinted an explicit list of files, so adding a new eval task was invisible to it: a second run printed that the previous verdict still holds and exited zero without ever validating the new task. It also gave DiscoveryReport a second constructor, assembled from front matter, free to drift from build_report. memory.py is now write-only, every run revalidates from scratch, and build_report is the only way a report comes into being. Finding.provenance is gone. Every producer set it and nothing read it: it reached no artifact and no output. harbor_call already carries the same signal and is rendered by both the CLI and the report, so the absence of a call is now what marks a finding we observed rather than one Harbor judged. One behavior change, and the reason a repo's exit code can move: the reward rung is now advisory. It was a substring search over test scripts wearing a harbor_call that only locates the script path, and it was wrong in both directions. A script writing the reward through a shell variable was reported as a hard failure, blocking discovery and withholding the config for a repo whose evals are fine; a script that wrote nothing but echoed the string "reward.json" passed. It now warns, carries no harbor_call, and says the script never names a reward file. Two tests pin that, including the false negative. It is not a commit of its own because the hunks that update its tests are the same hunks that delete the scout's. _rung_coverage stays, after being cut and put back: writing a replacement test showed Harbor silently drops a task dir with a malformed task.toml while the ladder reports one of one valid and exits zero. Nothing else catches it. Also: the markdown body loses the essays on what -c means and how to run the oracle, leaving verdict, run command, findings and host variables, while the front matter stands as the machine-readable contract apart from the dropped inputs keys. sources.source_priority() and report.canonical_digest() were never called. scan.find_doctrine and find_skills returned tuples whose first element the only call site discarded. The discover help now says that validating a config imports the inspected repo's agent module into this process, which with the scout gone is the last code-execution hazard and was named nowhere in the CLI. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
…dirs Experimentalist writes config.json and lock.json into its job directory, which is exactly the pair _from_prior_job matches on, and prior_job outranks the profile. So running the optimizer and then discover in the same repo selected a config whose agent import path is rooted at the package Experimentalist synthesizes in sys.modules for the duration of its own run. No separate harbor process can import that, so discover reported "Harbor cannot run this repo's evals" about a repo that had just been evaluated successfully. Two guards, neither of which reshuffles SOURCE_PRIORITY. Pruning eval-and-optimize and .nemo-optimizer keeps every probe that walks the tree out of Experimentalist's output, including find_skills and _from_convention, which were picking up candidate agent copies. Discarding a config whose agent import path starts at _AGENT_IMPORT_ROOT covers the same config found anywhere else: a package synthesized in one process is by construction not runnable from a file. The constant is imported rather than copied so a rename in Experimentalist breaks the build, not the verdict. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
shutil.which("harbor") finds whatever PATH resolves first, which on a machine with a
uv tool or pipx install is not the environment's own console script: 0.6.4 was
observed on PATH against 0.18.0 imported in process. Every other rung is an
in-process Harbor call and the report stamps only the in-process version, so the one
rung that shells out could return a verdict from a different Harbor and say nothing
about it.
Prefer the script beside sys.executable, which is the one that imports the Harbor
these verdicts came from, and keep PATH as the fallback for an interpreter whose
scripts live elsewhere.
Signed-off-by: Alec Khoury <akhoury@nvidia.com>
The persisted harbor-job.yaml named a bare `harbor_wrapper:WrappedAgent`, and the command discover printed alongside it failed on the first trial with `No module named 'harbor_wrapper'`. Harbor imports agents through importlib.import_module and never adds the working directory to sys.path, so a bare module name resolves only if something already put its directory there. Worse for a nested wrapper: _find_wrapper locates src/myagent/harbor_wrapper.py and the import path dropped src/myagent entirely, while the finding's own hint pointed at the file. Nothing in the ladder could catch it. --print-config returns before any agent import, and _check_agent_import rglobbed the repo for a matching module and inserted its directory into sys.path itself — so the rung passed for a reason `harbor job start -c` would not have. That is the part worth naming: the config was not merely wrong, it was vouched for by a check that had quietly arranged its own success. _agent_entry now returns the wrapper's directory beside the import path, repo-relative like every other path in the artifact and `.` for a wrapper at the root. A Harbor JobConfig has nowhere to carry it, so it travels as run_config.pythonpath in discovery.md's front matter and as a PYTHONPATH prefix on the command both the report and the CLI print, built in one place so the two cannot drift. _check_agent_import imports with exactly that directory and nothing else: if the recorded path does not make the module importable, the rung fails and names a directory that would. The sys.modules eviction stays and now carries more weight, since with nothing added to the path a stale module is the only way an unimportable config could still pass. Three related fixes ride along, each touching the same functions closely enough that their hunks do not separate: _from_profile resolved every declared dataset as a path, so a profile pairing `validation: tau2-bench-live-validation@1.0` with a registry_url yielded a directory under the profile dir that does not exist and a Job.create FileNotFoundError — a false "cannot run" about a profile the optimizer runs daily. registry_url was not even visible before, being swallowed by extra="ignore". A ref now becomes a DatasetConfig naming name, version and registry_url, and warns that resolving it needs the registry to be reachable. Dropping base=BaseAgent matches harbor/agents/factory.py, which calls import_class(import_path, label="agent") with no base — Harbor is strict for verifiers and deliberately not for agents. A rung stricter than the gate it stands for fails repos Harbor runs. A class that is not a BaseAgent now warns instead, carrying no harbor_call because Harbor does not make that judgement. A harbor_wrapper.py whose class did not match _AGENT_BASE_CLASSES reported "No harbor_wrapper.py found" and fell back to the oracle, which replays solution/solve.sh and evaluates nothing. The file is named now, along with the bases that were looked for, so the reader is not sent hunting for something that is right there. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes ASE-677.
Warning
Active development. Not intended for production use.
discoverexecutes code from the repository it inspects: validating a config imports that repository's agent module into the running process, and--dangerously-fixadditionally hands a language model an unsandboxed shell in that directory. Point it only at repositories whose code you would already run yourself. Sandboxing is not implemented; see the follow-ups at the end.What
discoverdoesnemo eval-author discoverpoints at an agent repository and answers one question: can Harbor run this repo's evals, and if so, exactly how. It records the answer as a durable artifact in a fileset.It exists to separate discovery from execution. Working out how a repo runs its evals takes a capable model and a lot of reading; running them afterwards should not. This command does the reading once, so a later step —
propose,run, or a person — can act on the result without re-deriving it.That only works if the record can be trusted, which rules out a report of impressions. Two rules follow from it:
passis the same code path a real run takes, and afailis the error that run would have hit.Nothing here builds a container, which is what keeps it cheap enough to run on every change. The one check that needs containers — Harbor's oracle, which replays each task's own solution and should score 1.0 — is recommended in the report rather than run.
Using it
--repo.--agentoptimizer.yaml, else the directory name--dangerously-fix--refresh--dry-runThere is deliberately no flag for the platform or the Harbor environment backend. The cluster, credentials and workspace come from the active
nemocontext, and the backend comes from the repo's own config or Harbor's default, so one invocation cannot quietly disagree with the configuration everything else obeys.Exit 0 means validated and recorded, so this works as a gate. On a repo that maintains its own config:
A repo that cannot run its evals gets the same treatment in reverse: the blocking rung, the Harbor call that reported it, and a hint, on stderr, with exit 1.
What it produces
Everything lands in the
nemo-eval-authorfileset under{agent}/. Nothing is written into the repo being inspected.discovery.md— YAML front matter for machines, prose for people. The front matter carriesrunnable, the config source and its provenance,run_config, per-rung validation results,harbor_version, the host variables the config requires, and aninputs_digestover every file a verdict was derived from. The prose says what Harbor checked, how to run it, what the repo offers for authoring evals, and what would invalidate the report.harbor-job.yaml— only when discovery authored the config.-cis the only interface Harbor offers for local task directories, since--datasetand--taskread a filesystem path as a registry package reference. A repo with task directories and no config file therefore cannot be run as-is, so discovery writes one. A repo that already maintains its own config keeps it: the report points at that path and validates it directly, rather than publishing a copy that drifts the moment the original is edited.run_configin the front matter names the target either way, so a consumer reads one field instead of inferring which case it is in.Secrets are safe to persist: Harbor's own
AgentConfig.envserializer rewrites sensitive values as${VAR}templates, so the config carries the variable names a run needs and none of their values.How it's structured
sources.pyJobConfigfrom the strongest source the repo offersvalidate.pyagent.pyscan.pyreport.pydiscovery.mdand, when discovery owns it,harbor-job.yamlmemory.pyrun.pycli.pymodels.pyFinding,ConfigSource,CandidateConfig,RunTarget,DiscoveryReportSources, ranked by how much trust they earn
config_fileprior_jobconfig.jsonfrom a completed job directory, which is a config Harbor already ranprofileoptimizer.yaml, reading the validation split so a run cannot silently score the set used to optimizeconventionEvery source is attempted even after one wins, because "we also found a prior job directory" tells a reader which evidence was passed over.
The ladder
Each rung is a Harbor call and a
Finding. Rungs run in dependency order and stop only where a later rung would have nothing left to judge.JobConfig.model_validateJob.create— expands datasets, resolves skills, builds metrics, checks resource policiesimport_class/AgentFactoryEnvironmentFactory.run_preflightTask.is_valid_dirTaskPaths.discovered_test_path_forget_required_host_varsharbor job start --print-configThree of these do more than relay a Harbor answer, each for a documented reason. Coverage exists because
Job.createdrops task directories it cannot parse without raising: ten task dirs where three are half-written resolve to seven, and the run then reports a score over seven tasks, which is the most expensive kind of wrong. Reward checks statically what Harbor only enforces at trial time, because a task whose test script never writes a reward file fails after containers are already built — and Harbor's own task template mentions the reward in a comment while writing nothing. Round trip is the only rung that judges bytes rather than an in-memory object, because bytes are what a later agent hands to Harbor.The ladder runs with
cwdat the repo root, since Harbor resolves a config's relative paths against the process directory and a repo's own config means them relative to itself.The scout, and its containment
The scout is a bounded LLM agent, opt-in behind
--dangerously-fixand reached only for rungs a look at the repo could settle. Its output is a proposal, not a result: it is revalidated from the schema rung up and adopted only if Harbor then accepts it. A partial fix is not adopted, because runnable is the bar. Anything it influenced is markedinferencein the report, and the config's provenance says it was adjusted by the scout, so a reader can tell inference from proof.That containment is epistemic, not a security boundary, and the distinction matters for review. Revalidation stops a model from talking its way into a passing verdict. It does nothing about what the model does while working: the scout holds
GuardedShellTools, which refuses commands naming a held-out dataset split and nothing else. That is a tripwire against overfitting — its own docstring says so — not a sandbox. Any other command runs, in the repo directory, with theAUTHOR_*credentials in the environment, and the scout chooses those commands by reading files from the repository under inspection, so hostile content there can steer it. Hence the flag name, and hence sandboxing gating any recommendation to turn it on.Worth a reviewer's eye: an adopted proposal carries the model's
rationalestring intodiscovery.md. It is prose a human reads rather than something re-executed, but it is the one place model output reaches the artifact.Freshness
The
inputs_digestis a hash over every file a verdict was derived from. On re-run, an unchanged digest means the previous verdicts still hold, so the report is restamped and the ladder is skipped entirely. A prior failure is always re-derived, because the reason it failed may have been the machine rather than the repo — Docker not running, a cloud extra missing.Test plan
uv run pytest plugins/nemo-eval-author/tests— 152 passed, 2 skipped (both pre-existing credential-gated canaries), under both the root and plugin-local pytest configssteps/<name>/instruction.md), Windows (test.bat), separate verifier image, anddocker_imagewith no Dockerfilediscovery.mdalone, an authored or adjusted config publishes the copy, andrun_configsurvives the front-matter round trip the reuse path depends ontest_plugin_boundary.pygreen — no Experimentalist import beyond the already-allowlistedclientandcomponents.toolsuv run ruff check,ruff format --check,uv run --frozen ty check,uv lock --check,uv run pre-commit rundiscover --dry-runagainst both a repo with its own config and a repo with only task directoriesFollow-ups this leaves open
Sandboxing is the one that blocks recommending this to anyone. Neither the agent-import rung nor the scout is isolated from the host, and both are described above rather than glossed. Until that lands, the plugin is marked active development and stays out of the default install, reachable only through the optional
experimentalistdependency group.ASE-769 (
doctor) owns the machine half of the split assumed here: discover records what the repo demands, doctor checks whether this machine satisfies it. ASE-678 covers prerequisite gating for the other verbs. ASE-675 (propose) inherits a validated config rather than a description of one.schema_version: 1in the front matter is the seam for the later long-term-memory ticket.Also open, from review: a config whose datasets resolve only to a remote Harbor registry is currently reported as "zero tasks," which contradicts what the resolution rung intends; and the freshness digest walks with a depth cap that the agent rung's module import does not share, so a module deeper than six directories could be imported without being fingerprinted.
Summary by CodeRabbit
nemo eval-author discovercommand to inspect repositories and identify runnable evaluation configurations.