feat(evaluator-sdk): agent-skill injection for FabricContainerRuntime - #841
Merged
Merged
Conversation
Contributor
|
SandyChapman
force-pushed
the
fabric-container-skill-injection/schapman
branch
from
July 22, 2026 12:22
ce8121f to
0ed94f6
Compare
SandyChapman
changed the base branch from
main
to
fabric-multi-skill-injection/schapman
July 22, 2026 12:22
SandyChapman
force-pushed
the
fabric-container-skill-injection/schapman
branch
from
July 22, 2026 12:33
0ed94f6 to
83e14d8
Compare
SandyChapman
marked this pull request as ready for review
July 22, 2026 12:41
ngoncharenko
approved these changes
Jul 23, 2026
SandyChapman
force-pushed
the
fabric-multi-skill-injection/schapman
branch
2 times, most recently
from
July 28, 2026 15:24
ed1a8a0 to
32f367e
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFabric runtimes now support additive multi-skill injection, typed routing modes, sandbox staging, rollback, provenance metadata, Codex cleanup, collision validation, and expanded tests. ChangesFabric multi-skill injection
Sequence Diagram(s)sequenceDiagram
participant FabricAgentRuntime
participant FabricContainerRuntime
participant FabricPlanner
participant Sandbox
participant TrialEvidence
FabricAgentRuntime->>FabricContainerRuntime: run_tasks with skills
FabricContainerRuntime->>FabricPlanner: resolve skill injection mode
FabricPlanner-->>FabricContainerRuntime: native or codex_skills_dir
FabricContainerRuntime->>Sandbox: stage skill seed and profiles
Sandbox-->>FabricContainerRuntime: execution result and workspace
FabricContainerRuntime->>TrialEvidence: clean bundles and record provenance
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (2)
packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.py (1)
347-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFake Fabric planner is duplicated from
test_fabric_runtime.py.
_FakeHarness,_FakeConfig,_FakeProfile,_FakeAdapterInfo,_FakePlan,_harness_nameare near-identical in both test modules. Move to a sharedconftest.pyhelper so routing semantics can't drift between the host and container suites.🤖 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/tests/agent_eval/test_fabric_container_runtime.py` around lines 347 - 417, Move the duplicated fake Fabric support symbols—_FakeHarness, _FakeConfig, _FakeProfile, _FakeAdapterInfo, _FakePlan, and _harness_name—from both test modules into a shared conftest.py helper. Update the host and container suites to import and reuse those shared definitions, preserving the existing planning and routing behavior while removing the local duplicates.packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py (1)
347-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRuntime-agnostic skill helpers are copied into both runtimes.
_skill_metadatais byte-identical across the two modules, as are_existing_skill_pathsand the body of_resolve_skill_mode. Same root cause as theSkillSetextraction already done: these belong inskills.py, not duplicated per runtime. Two copies of the provenance-metadata contract will drift.
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py#L347-L355: drop the static method and import a sharedskill_metadata(provenances)fromskills.py.packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py#L498-L506: delete the mirrored copy and import the same shared helper; do the same for_existing_skill_paths(shareddeclared_skill_paths(config, profiles)).🤖 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/runtimes/fabric/runtime.py` around lines 347 - 355, Move the duplicated skill helpers into skills.py: add shared skill_metadata(provenances) and declared_skill_paths(config, profiles), then remove _skill_metadata and _existing_skill_paths from packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py at lines 347-355 and the mirrored methods from packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py at lines 498-506. Import and use the shared helpers in both runtimes, preserving the existing provenance metadata and path-resolution behavior.
🤖 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/runtimes/fabric/skills.py`:
- Around line 317-334: Update the skill installation loop around install_skill
to compute each skill’s target root and whether it pre-existed before staging.
Register the target in staged_roots before calling install_skill only when it
was not pre-existing, so partial bundles are removed on failure without deleting
reserved task-seed paths; retain provenance handling for successful
installations.
---
Nitpick comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py`:
- Around line 347-355: Move the duplicated skill helpers into skills.py: add
shared skill_metadata(provenances) and declared_skill_paths(config, profiles),
then remove _skill_metadata and _existing_skill_paths from
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py
at lines 347-355 and the mirrored methods from
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py
at lines 498-506. Import and use the shared helpers in both runtimes, preserving
the existing provenance metadata and path-resolution behavior.
In
`@packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.py`:
- Around line 347-417: Move the duplicated fake Fabric support
symbols—_FakeHarness, _FakeConfig, _FakeProfile, _FakeAdapterInfo, _FakePlan,
and _harness_name—from both test modules into a shared conftest.py helper.
Update the host and container suites to import and reuse those shared
definitions, preserving the existing planning and routing behavior while
removing the local duplicates.
🪄 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: 2daa902f-d0df-4e71-be0a-4f0c1c071cd3
⛔ Files ignored due to path filters (3)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/container_runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/skills.pyis excluded by!sdk/**
📒 Files selected for processing (6)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/skills.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_skills.py
SandyChapman
force-pushed
the
fabric-container-skill-injection/schapman
branch
from
July 28, 2026 16:29
8bc8139 to
ee5e787
Compare
SandyChapman
enabled auto-merge
July 28, 2026 16:32
Extends agent-skill injection to the containerized runtime so a skills A/B eval runs in a sandbox, not just on the host. Each bundle is rendered into the sandbox seed set at the harness's in-sandbox discovery path (native: /in/skills/<name>; codex: <workspace>/.agents/skills/<name>), with at most ONE merged native skills overlay — Fabric applies profile skills.paths last-wins, so per-skill overlays would drop all but the last. Codex bundles are pruned from the downloaded evidence so injected files don't read as agent output to workspace-reading metrics. Review follow-ups folded in: - SkillSet: a frozen dataclass owning the skill list, its uniqueness check, and the clone-on-mutation helpers, replacing the copy of that logic each runtime carried. - Same-bundle conflict detection: .agents/skills/ is populated by two independent, equally valid sources — the runtime skills parameter (A/B, staged pre-start) and the task's own files inputs (skills a task always ships, uploaded post-start). Tasks may freely seed skills there; only both writing the same <name>/ is rejected, since the task upload lands second and would leave the stamped provenance hash describing content the agent never saw. - Rollback fix (CodeRabbit): install_skills registered a rollback target only after install_skill returned, so a skill failing AFTER writing files left its partial bundle on disk. The target is now registered before staging, skipping paths that already exist so a pre-existing codex task-seed is still never deleted. Vendor: SDK mirror synced. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
fabric-container-skill-injection/schapman
branch
from
July 28, 2026 16:41
ee5e787 to
b5df96e
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py (1)
224-257: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCodex bundle cleanup can turn a successful trial into a failure.
_remove_injected_bundleis documented as best-effort ("any filesystem error here must not fail an otherwise-successful trial"), but neither the call site nor the function itself guards against unexpectedOSErrors from.resolve()/.exists(). If one is raised, it propagates to the outerexcept Exception, discarding an already-successful run and reporting the trial as failed — contradicting the stated intent.🛡️ Proposed fix
if skill_mode == SKILL_MODE_CODEX_SKILLS_DIR: for provenance in skill_provenances: - await asyncio.to_thread(_remove_injected_bundle, out_dir / "workspace", provenance["location"]) + try: + await asyncio.to_thread( + _remove_injected_bundle, out_dir / "workspace", provenance["location"] + ) + except OSError: + logger.warning( + "failed to remove injected skill bundle from evidence", + extra={"location": provenance["location"]}, + )🤖 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/runtimes/fabric/container_runtime.py` around lines 224 - 257, Guard the Codex cleanup loop in _run_task so filesystem errors from _remove_injected_bundle, including failures during path resolution or existence checks, are treated as best-effort cleanup failures and do not enter the outer task-failure handler. Preserve the successful result flow by continuing to _to_trial after cleanup attempts, while keeping unrelated task errors handled by the existing outer except block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py`:
- Around line 224-257: Guard the Codex cleanup loop in _run_task so filesystem
errors from _remove_injected_bundle, including failures during path resolution
or existence checks, are treated as best-effort cleanup failures and do not
enter the outer task-failure handler. Preserve the successful result flow by
continuing to _to_trial after cleanup attempts, while keeping unrelated task
errors handled by the existing outer except block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 59ef3cfa-9565-4471-af3f-9c8ed5d8447d
⛔ Files ignored due to path filters (3)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/container_runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/skills.pyis excluded by!sdk/**
📒 Files selected for processing (6)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/skills.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_skills.py
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_skills.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/skills.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py
soluwalana
pushed a commit
that referenced
this pull request
Jul 28, 2026
…#841) Extends agent-skill injection to the containerized runtime so a skills A/B eval runs in a sandbox, not just on the host. Each bundle is rendered into the sandbox seed set at the harness's in-sandbox discovery path (native: /in/skills/<name>; codex: <workspace>/.agents/skills/<name>), with at most ONE merged native skills overlay — Fabric applies profile skills.paths last-wins, so per-skill overlays would drop all but the last. Codex bundles are pruned from the downloaded evidence so injected files don't read as agent output to workspace-reading metrics. Review follow-ups folded in: - SkillSet: a frozen dataclass owning the skill list, its uniqueness check, and the clone-on-mutation helpers, replacing the copy of that logic each runtime carried. - Same-bundle conflict detection: .agents/skills/ is populated by two independent, equally valid sources — the runtime skills parameter (A/B, staged pre-start) and the task's own files inputs (skills a task always ships, uploaded post-start). Tasks may freely seed skills there; only both writing the same <name>/ is rejected, since the task upload lands second and would leave the stamped provenance hash describing content the agent never saw. - Rollback fix (CodeRabbit): install_skills registered a rollback target only after install_skill returned, so a skill failing AFTER writing files left its partial bundle on disk. The target is now registered before staging, skipping paths that already exist so a pre-existing codex task-seed is still never deleted. Vendor: SDK mirror synced. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds agent-skill injection to the sandboxed
FabricContainerRuntime, so an isolated (Docker) Fabric eval can run WITH an agentskills.io skill — previously only the hostFabricAgentRuntimesupported skills (limitation L1 from the LAB-examples gap analysis). It mirrors the host runtime's skill design and extends the same two-mode contract to the container.How
__init__gains askill: AgentSkill | None; newwith_skill()clone method so an A/B eval derives baseline (with_skill(None)) and treated (with_skill(skill)) runtimes from one instance._resolve_skill_mode).nemo_fabricis imported lazily on the host only when a skill is set, so the no-skill container path stays dependency-free.stage_skill_seed()inskills.py— the sandboxed sibling ofinstall_skill. Instead of a hostcopytree(the container has no host workspace), it renders the bundle into the sandbox seed set (SandboxSpec.files) at the harness's in-sandbox discovery path:accepts: ["skills"]) →/in/skills/<name>/+ askills.pathsprofile overlay (preserving any pre-configured skill paths, last-wins);<workspace>/.agents/skills/<name>/(self-discovery), no overlay./out/workspacebefore it's exposed as evidence (so injected files don't skew workspace-reading metrics). Native staging lives under/inand is never downloaded, so it needs no cleanup.SkillMode = Literal["native", "codex_skills_dir"], threaded through both runtimes andSkillProvenance.mode.Text bundles only: the seed set is
dict[str, str], so a binary file raisesSkillInjectionError(the hostinstall_skillcopytree path still handles binary bundles).Tests
test_fabric_container_runtime.pymirroring the host skill-injection suite: native seed-set + overlay, preconfigured-path preservation, runtime-discovered adapter, codex seed + evidence exclusion, fail-fast on an unsupported adapter, no-skill-no-probe, andwith_skillcopy. Full fabric suite (container + host + skills) = 76 passing; ruff + format clean;tyclean; SDK mirror re-vendored (make vendor).mode=native, staged path, content hash) stamped correctly; no leaked containers; native skill correctly absent from workspace evidence.Notes
fabric-adapter.jsondeclaresaccepts: ["models", "telemetry"](noskills), so Fabric routes codex skillsunsupported, while the codex CLI itself self-discovers.agents/skills/. When the codex adapter addsskills, this branch can be deleted and codex flows through the same native path.Summary by CodeRabbit
New Features
with_skill/with_skills, and enhanced trials to record multi-skill provenance.Bug Fixes
Tests