fix: let the turn that poisoned the runtime publish its own artifacts - #204
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (19)
🧰 Additional context used📓 Path-based instructions (11)**/*.{rs,py,pyi,json,yaml,yml}📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Files:
**/*📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
Files:
**/*.{rs,py}📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Files:
**/*.{py,pyi}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{rs,py,pyi}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{py,pyi,rs}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{rs,py,toml}📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Files:
**/*.{toml,rs,py}📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Files:
**/*.{md,mdx,yml,py,rs,sh}📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
Files:
adapters/*/src/**/*.py📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
Files:
{adapters/**,examples/**}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2026-07-09T22:28:51.689ZApplied to files:
🔇 Additional comments (3)
WalkthroughThe adapter now records the pre-invocation quarantine state. The poisoning invocation can publish its artifacts. Later quarantined invocations skip artifact collection. Tests verify both behaviors. ChangesTelemetry artifact ownership
Estimated code review effort: 3 (Moderate) | ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.py`:
- Around line 598-600: Update the _telemetry_output method signature to make
inherited_quarantine keyword-only by placing it after *, then update its call
site in the surrounding telemetry flow to pass
inherited_quarantine=inherited_quarantine explicitly.
🪄 Autofix
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: ASSERTIVE
Plan: Enterprise
Run ID: d3fcd4ee-711a-42e0-99e8-09373e7c7693
📒 Files selected for processing (2)
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_deepagents.py
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Pre-commit
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.11, linux-amd64)
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_deepagents.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_deepagents.py
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.Use
snake_casefor functions and variables; usePascalCasefor Rust types and Python classes.
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_deepagents.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.In Python SDK, adapters, examples, and tests, follow the existing style, use type annotations for public APIs, and keep native binding declarations synchronized with their Rust implementations.
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_deepagents.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_deepagents.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.Public contract changes must keep native Python binding declarations synchronized with their Rust implementations.
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_deepagents.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_deepagents.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_deepagents.py
**/*.{md,mdx,yml,py,rs,sh}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
Keep documentation aligned with current NeMo Fabric behavior, repository layout, entry points, commands, package names, APIs, bindings, and support claims.
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.pytests/adapters/test_deepagents.py
adapters/*/src/**/*.py
📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md)
adapters/*/src/**/*.py: Implement adapters using the existing Fabricpythonorprocessrunner and normalized request/result contracts; do not add a runner or one-off abstraction for a single adapter.
Treat normalizedconfig, Fabric-resolved plans, andruntime_contextas authoritative; reserveharness.settingsfor adapter-wide behavior and apply precedence in the order: normalized config, plans/context, harness settings, descriptor/default values.
Reject conflicting duplicate declarations and unsupported behavior with actionable errors naming the field and supported alternatives; never silently drop configuration.
Validate dependency versions, hooks, and credentials before harness invocation, and never expose credential values in outputs, errors, events, logs, or fixtures.
Forward only required system, selected credential, telemetry, and documented harness-specific environment variables; never forward or log unrelated environment values.
Maintain one local adapter host per Fabric runtime across orderedstart→invoke*→stopoperations. Emit one JSON lifecycle response per request on stdout and diagnostics on stderr.
Return harness-level invocation failures as successful lifecycle responses containingresponse: null,failed: true, and structurederrorfields (code,message,retryable, and optionalmetadata).
Do not emit NeMo Relay stream records on adapter stdout; return exactly one terminal lifecycle response while streaming occurs through the SDK-owned out-of-band endpoint.
Scope workspace, generated configuration, state, sessions, and artifacts to the resolved runtime context, and isolate stateful adapter instances by Fabric runtime ID.
Usestartto initialize adapter-owned harness state, retain it for continuation across repeatedinvokecalls on the same runtime, and release it instop.
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.py
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public NeMo Fabric contracts.
Files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.py
tests/adapters/**/*.py
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
tests/adapters/**/*.py: If an adapter or integration changes, run its focused tests.
For adapter behavior changes, run focused adapter tests undertests/adapters, then runjust test-python.
Files:
tests/adapters/test_deepagents.py
tests/**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the corresponding Rust crate or the relevant area under
tests/.
Files:
tests/adapters/test_deepagents.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorAsyncMock, using thespecargument when necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
If a fixture is needed in multiple test files, define it once inconftest.pyrather than repeating it.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its returned value is unused or it returns no value.
Avoid defensive programming in tests; access expected values directly so missing data raises a clear failure, such as usingresults["data"]instead ofresults.get("data").
When adapter installation metadata changes, packaging metadata tests must directly assert that the root project depends unconditionally on the exact-versionnemo-fabric-runtimedistribution.
Packaging metadata tests must verify that each root harness extra delegates to the matching version of the leaf adapter'sharnessextra.
Packaging metadata tests must verify that bare leaf dependencies remain adapter-owned and that the rootadapter-testsdependency group installs each leaf through itsharnessextra.
Packaging metadata tests must verify that every leaf providesfull; only adapters importing NeMo Relay Python APIs providerelay, while adapters using an external Relay executable havefullequal toharness.
Files:
tests/adapters/test_deepagents.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/adapters/test_deepagents.py
🧠 Learnings (1)
📚 Learning: 2026-07-09T22:28:51.689Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 43
File: adapters/claude-sdk/src/nemo_fabric_adapters/claude_sdk/adapter.py:164-168
Timestamp: 2026-07-09T22:28:51.689Z
Learning: In the NeMo-Fabric adapters, treat path values used in Fabric adapter configuration (including logic like `_resolve_path` in adapter.py) as config-root-relative. Do not apply `Path.expanduser()` (or otherwise apply `~`/home or shell-style expansion), because it will make the resolved paths normalize inconsistently across adapters. Also, do not rely on or add any resolution behavior that uses `harness.settings.cwd` as an override point for these adapter paths—`harness.settings.cwd` is explicitly unsupported in this adapter context.
Applied to files:
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.py
🪛 Ruff (0.16.1)
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.py
[warning] 697-697: Boolean-typed positional argument in function definition
(FBT001)
🔇 Additional comments (2)
adapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.py (1)
719-720: LGTM!tests/adapters/test_deepagents.py (1)
542-548: LGTM!Also applies to: 564-566
`invoke()` reads the quarantine state from before the turn, but `_telemetry_output` consulted `self._telemetry_quarantine` directly. By the time it ran, `_invoke_with_telemetry` had already set that flag on the turn whose fault caused the quarantine, so that turn suppressed its own `relay_artifacts` as well. That contradicts the documented contract. A turn that opened a request scope and faulted during teardown produced its own partial trajectory, so it publishes it flagged as degraded; only turns that inherit the quarantine have none of their own and must not claim an earlier turn's. Pass the pre-invocation state in rather than re-reading the field afterwards. The existing regression test asserted only that later turns withhold artifacts, which is why this went unnoticed. It now stubs a recognisable artifact and asserts by value, because real collection over an empty evidence directory returns an empty list — a presence-only assertion passed either way. Reported by @AjayThorve in review of NVIDIA#191. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
814ad59 to
60609d4
Compare
|
/merge |
2 participants
Overview
Follow-up to #191, addressing the P2 @AjayThorve raised in review after it had merged.
invoke()captures the quarantine state from before the turn, but_telemetry_outputconsultedself._telemetry_quarantinedirectly. By the time it runs,_invoke_with_telemetryhas already set that flag on the turn whose fault caused the quarantine — so that turn suppressed its ownrelay_artifactstoo, reproducing asfaulting_turn_artifacts=None.That contradicts the contract #191 documented: a scope- or flush-degraded turn still references its partial artifacts, and only turns that inherit the quarantine omit them.
Where should the reviewer start?
_telemetry_outputinadapters/deepagents/src/nemo_fabric_adapters/deepagents/adapter.py, which now takesinherited_quarantineas a parameter rather than re-reading the field afterwards. The distinction it encodes: a turn that opened a request scope and faulted during teardown produced its own partial trajectory, so it publishes it flagged as degraded; a turn that inherits the quarantine opened no scope and has nothing of its own, so it must not claim an earlier turn's artifacts.Then the assertion change in
test_a_dirty_scope_stack_quarantines_telemetry_for_later_turns. The original test asserted only that later turns withhold artifacts and said nothing about the faulting turn, which is why this went unnoticed. Strengthening it exposed a second weakness worth flagging: that test does not stubcollect_relay_artifacts, so real collection over an empty evidence directory returns[]— a presence-only check like"relay_artifacts" in firstpasses against an implementation that publishes an empty list forever. It now stubs a recognisable artifact and asserts by value.Both directions are mutation-checked: restoring the original
self._telemetry_quarantineread fails the test withKeyError: 'relay_artifacts', and removing the suppression entirely fails it because the inherited turn would publish the earlier turn's artifacts as its own.Testing
just test-python(full suite on this base) — 702 passed, 53 skipped.tests/adapters/test_deepagents.py— 56 passed.ruff check/ruff format --check(v0.15.21, matching.pre-commit-config.yaml) and the copyright-header hook pass on both changed files.No behaviour changes beyond the artifact policy, and no API changes.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to fix: keep a completed Deep Agents turn completed when telemetry fails #191
Relates to NVBug 6562846
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
Summary by CodeRabbit