feat(evaluator-sdk): add read_trials bundle loader + self-contained bundle refs - #808
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesPortable trial persistence
Sequence Diagram(s)sequenceDiagram
participant AgentEvalResult
participant persist_run
participant trials.jsonl
participant read_trials
participant run_dir
AgentEvalResult->>persist_run: provide trials
persist_run->>trials.jsonl: write bundle-relative evidence refs
read_trials->>trials.jsonl: stream persisted trial records
read_trials->>run_dir: resolve evidence refs
read_trials-->>AgentEvalResult: return hydrated trials
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/persistence.py`:
- Around line 110-131: Update _resolve_evidence_ref so both rebuilt paths are
resolved and accepted only when they remain contained within run_dir; reject
traversal or legacy-tail results that escape the bundle even if the target
exists. Preserve valid in-bundle relative and legacy evidence resolution, and
add a regression test covering a crafted .. reference targeting an existing
out-of-bundle file.
🪄 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: a89b569b-a015-452b-9870-365110d4ba5c
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/persistence.pyis excluded by!sdk/**
📒 Files selected for processing (2)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/persistence.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_persistence.py
Contributor
|
SandyChapman
force-pushed
the
read-trials-bundle-loader/schapman
branch
from
July 21, 2026 14:47
d254173 to
b9a75e1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/persistence.py (1)
93-145: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winContainment check is fixed, but returned ref still uses the unresolved path — reopens a TOCTOU gap.
_resolves_withinvalidatespath.resolve()stays underbase, but_resolve_evidence_refreturnsstr(rebuilt)/the unresolved joined path, not the canonical resolved one. Any symlink component inrebuiltis re-followed whenever the returned ref is later opened for re-scoring — potentially much later, perread_trials' own docstring — so a symlink swapped after this check still escapes the bundle despite passing containment.Return the resolved canonical path so the validated location is what actually gets used later.
🔒 Suggested fix
- if not candidate.is_absolute(): - rebuilt = run_dir / candidate - if _resolves_within(base, rebuilt): - return str(rebuilt) + if not candidate.is_absolute(): + resolved = _resolve_within(base, run_dir / candidate) + if resolved is not None: + return str(resolved) elif candidate.exists(): return ref parts = candidate.parts if "evidence" in parts: - rebuilt = run_dir / Path(*parts[parts.index("evidence") :]) - if _resolves_within(base, rebuilt): - return str(rebuilt) + resolved = _resolve_within(base, run_dir / Path(*parts[parts.index("evidence") :])) + if resolved is not None: + return str(resolved) return ref -def _resolves_within(base: Path, path: Path) -> bool: +def _resolve_within(base: Path, path: Path) -> Path | None: resolved = path.resolve() if not resolved.exists(): - return False - return resolved == base or base in resolved.parents + return None + if resolved == base or base in resolved.parents: + return resolved + return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/persistence.py` around lines 93 - 145, Update _resolve_evidence_ref to return the canonical resolved path produced after containment validation, rather than str(rebuilt), for both relative refs and rebuilt evidence refs. Reuse the resolved path from _resolves_within (or adjust that helper to expose it) so read_trials stores the exact validated location and later rescoring cannot reopen the unresolved symlink path.
🤖 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 `@packages/nemo_evaluator_sdk/tests/agent_eval/test_persistence.py`:
- Around line 59-84: Extend
test_persist_run_writes_bundle_relative_refs_that_survive_a_move with a
workspace located outside the bundle, then verify persist_run writes its
absolute evidence reference unchanged and read_trials returns that same absolute
reference. Keep the existing in-bundle relative-reference and moved-bundle
assertions intact.
---
Duplicate comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/persistence.py`:
- Around line 93-145: Update _resolve_evidence_ref to return the canonical
resolved path produced after containment validation, rather than str(rebuilt),
for both relative refs and rebuilt evidence refs. Reuse the resolved path from
_resolves_within (or adjust that helper to expose it) so read_trials stores the
exact validated location and later rescoring cannot reopen the unresolved
symlink path.
🪄 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: dcc259bf-e2ba-4765-ad63-f7af65a41e1f
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/persistence.pyis excluded by!sdk/**
📒 Files selected for processing (2)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/persistence.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_persistence.py
…undle refs persist_run had no inverse — a persisted run bundle could not be loaded back to re-score its stored trials with a different metric/judge without re-running the agent. Add read_trials(run_dir) to hydrate persisted trials with their evidence, and make persist_run write bundle-relative evidence refs so a moved or copied bundle re-scores with no path fixups. read_trials stays backward-compatible with legacy absolute refs (and rebuilds moved-bundle paths from the evidence/ tail). Covered by round-trip tests. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
read-trials-bundle-loader/schapman
branch
from
July 21, 2026 14:58
b9a75e1 to
6806214
Compare
arpitsardhana
approved these changes
Jul 22, 2026
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.
What
Adds the inverse of
persist_run, so a persisted agent-eval run bundle can be loaded back and re-scored with a different metric/judge without re-running the agent:read_trials(run_dir)— hydratestrials.jsonlintoAgentEvalTrialobjects with their evidence, resolving each evidencerefagainst the bundle dir. Feed the result toAgentEvaluator().run(tasks=…, trials=…)to re-score.persist_runnow writes bundle-relative evidence refs (_write_trials) — a trial's evidence lives under the bundle, so storing the ref relative to the bundle (not the launch CWD) makes a moved or copied bundle re-score with no path fixups. Refs pointing outside the bundle are left verbatim.Why
persist_runhad no loader, so re-scoring a stored run (e.g. with a cheaper/faster or corrected judge) meant re-running the agent — expensive and often impossible after the fact. This makes "run once, re-grade later / elsewhere" a first-class, generic capability (any runner's bundle, not tied to a specific benchmark).Compatibility
read_trialsis backward-compatible: it resolves bundle-relative refs (new), still-valid absolute refs, and legacy/moved bundles (rebuilt underrun_dirfrom theevidence/tail). Existing bundles keep working.Tests
test_persistence.py: hydrate + valid ref, moved-bundle ref rebuild, and apersist_run→ assert-relative-ref →copytree→read_trialsround-trip proving self-containment.pytestgreen; existing evaluator persist/imported-trials tests unaffected. Includes the re-vendored SDK mirror.Summary by CodeRabbit
trials.jsonlwithin run bundles.