Repository navigation
ci: validate Relay session-root skills for #358 - #377
Closed
AnuradhaKaruppiah wants to merge 16 commits into
Closed
AnuradhaKaruppiah wants to merge 16 commits into
AnuradhaKaruppiah wants to merge 16 commits into
Conversation
Relay derives ATIF session identity from the propagated root, so a root that changes per request makes every invocation its own session. A caller whose work spans several requests - a chat turn at a time, say - therefore had no way to land those turns in one session. An adapter now reads `relay_session_root` (exported as SESSION_ROOT_CONTEXT_KEY) from the run request context and uses it as the Relay propagation root, keeping the request as the parent. Each turn still gets its own trajectory; they share one session. Without a session root the root falls back to the request, which is the behaviour from #260. The key is deliberately not `session_id`: adapters already surface harness session ids of their own, and a caller sending one for unrelated reasons would have had its traces silently regrouped. A test pins that a plain `session_id` roots nothing. Validation lives in relay_request_context, the one function that talks to Relay, so a non-UUID root never reaches PropagationContext. An empty or malformed root falls back to the request root and is left out of the scope metadata instead of being recorded as if used. A Deep Agents test covers the adapter path end to end for both a UUID and a non-UUID root. This requires Relay 0.9, the first release where ATIF session identity comes from the propagation root (NVIDIA/NeMo-Relay#959); on 0.7 and 0.8 the session is the Agent scope's own event and a supplied root is ignored. Squashed from b91590f, b73f421, 2048446, 5792902 on relay-session-propagation-root/mschwab. Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit b8456be)
Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit c8ef378)
Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit 43fc527)
Relay derives the OTel span id from the low eight bytes of a UUID and rejects a PropagationContext whose parent or root has them all zero, nil included. Such a relay_session_root reached Relay and failed the run instead of falling back to the request root. Apply Relay's rule in the shared guard and pin it against the real binding. Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit de22ad6)
The custom-agent example is the reference the adapter-authoring skill points at, and it still rooted propagation at the request alone. Forward the run request's relay_session_root to relay_request_context, matching the Deep Agents and mini-SWE-agent adapters. Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit 1389eb1)
Callers had no way to discover the key, its UUID requirement, or the fallback rule. Describe it in the Python SDK guide, the common, Deep Agents, and mini-SWE-agent READMEs, and keep both integration skills and the adapter-authoring eval in parity. Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit 168f554)
The shared UUID guard now also rejects identifiers Relay cannot use, so a request id with all-zero final eight bytes takes the non-UUID path instead of raising inside Relay. Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit 835d2d5)
Fabric drops an unusable session root without an error so telemetry never fails an invocation. Say so, and say the value must be a UUID string, so a caller knows grouping can quietly not happen. Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit ef1f5a8)
Reuse _stub_relay in test_relay_request_context instead of repeating its body, require session_root on the two private telemetry entry points that each have one production caller, and fold the context-key rationale into the session_root_id docstring. Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit 52b822d)
Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit 30614c5)
With a non-UUID request ID, which is the SDK default, the session root is both parent and root. The docs, skill, and eval said the request always stayed the parent. Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit be7b4fe)
Drop the Relay 0.9 sentence, which every Relay-enabled package already pins. Keep the identifier rules in the maintainer-facing common README and say only 'unusable value' in user docs. Reflow the skill paragraphs and name the helpers the same way in the skill and its eval. Signed-off-by: mschwab <mschwab@nvidia.com> (cherry picked from commit c01c9d4)
Project the optional northbound field into adapter context with precedence over the legacy key. Keep Rust, Python, schemas, references, and integration guidance in parity. Signed-off-by: Ajay Thorve <athorve@nvidia.com> Signed-off-by: Anuradha Karuppiah <26330987+AnuradhaKaruppiah@users.noreply.github.com>
Exercise per-request and shared-root Relay export through the persistent native runtime. Assert that distinct invocation files retain their own trajectories when a later turn uses the same session ID. Signed-off-by: Anuradha Karuppiah <26330987+AnuradhaKaruppiah@users.noreply.github.com>
Collaborator
Author
|
/nvskills-ci |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (6)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (28)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (58)
🧰 Additional context used📚 Code guidelines (8)📓 Path-based instructions (23)Review the Rust core for runtime lifecycle correctness, handle validation, capability routing accuracy, schema stability, and error semantics.⚙️ CodeRabbit configuration file Files:
Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.⚙️ CodeRabbit configuration file Files:
Enforce the product name in user-facing prose: use "NVIDIA NeMo Fabric" on first use and "NeMo Fabric" thereafter.⚙️ CodeRabbit configuration file Files:
Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public NeMo Fabric contracts.⚙️ CodeRabbit configuration file Files:
Do not flag SKILL.md files for missing SPDX headers.⚙️ CodeRabbit configuration file Files:
Schemas are generated public contract snapshots.⚙️ CodeRabbit configuration file Files:
Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.⚙️ CodeRabbit configuration file Files:
Source excerpt: `FabricConfig` is the northbound source of consumer intent.📄 CodeRabbit inference engine (schemas/SCHEMA.md) Files:
Source excerpt: Follow these repository-specific requirements after applying the public skill: Place a Python adapter under `adapters/python//` with `LICENSE -> ../../../LICENSE`, `README.md`, `.fabric-adapter.json`, Python pack...📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md) Files:
Source excerpt: Treat all files under `docs/reference/api/` as generated output.📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md) Files:
Source excerpt: In MDX files, top-of-file comments must use JSX comment delimiters: `{/*` to open and `*/}` to close.📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md) Files:
Source excerpt: MDX top-of-file SPDX comments use HTML comment delimiters instead of `{/* ...📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md) Files:
Source excerpt: For links between files under `docs/`, use paths relative to the source file and include the target file's `.mdx` extension.📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md) Files:
Source excerpt: If Python code or a Python-facing adapter changed, run `just test-python`.📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md) Files:
Source excerpt: **Schema or public contract changed** Run the Rust, Python, and TypeScript suites and review changes under `schemas/`, the checked-in Python adapter-contract representations, generated TypeScript sources, and generated API r...📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md) Files:
Source excerpt: [ ] Any Rust change ran `just test-rust` Source excerpt: [ ] Any Rust change ran `cargo fmt --all -- --check` Source excerpt: [ ] `crates/fabric-core` changes ran both the Rust and Python suites📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md) Files:
Source excerpt: If Rust code changed, run `cargo fmt --all -- --check` and `just test-rust`.📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md) Files:
Source excerpt: [ ] Relevant adapter or example `README.md` files updated when examples or adapters have changed.📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md) Files:
Source excerpt: This directory is the maintainer skill set for developing NeMo Fabric itself.📄 CodeRabbit inference engine (.agents/skills/README.md) Files:
Source excerpt: Verify README and docs entry points still match current package names and paths.📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md) Files:
Source excerpt: Follow these repository-specific requirements after applying the public skill: Give each Python leaf adapter a small base installation, a `harness` extra for package-installable target packages, and a `full` extra for packag...📄 CodeRabbit inference engine (.agents/skills/contribute-adapter/SKILL.md) Files:
Source excerpt: **Frontmatter:** each `SKILL.md` begins with YAML frontmatter containing at least `name` and `description`.📄 CodeRabbit inference engine (skills/README.md) Files:
Source excerpt: Copy an individual skill directory, such as `nemo-fabric-integrate/` or `nemo-fabric-build-adapter/`, into the place your coding agent discovers skills **in your own project**.📄 CodeRabbit inference engine (skills/README.md) Files:
🧠 Learnings (1)📓 Common learnings🔇 Additional comments (27)
Priority: ➖ Normal Change: Feature Merge Risk: ⚪ Minimal · up to The session-root changes have no identified blocker in this review. This draft is designated for validation only and should be closed rather than merged. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 21.05% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 57 functions across 19 files. (14 skipped: 14 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Fern docs preview: https://nvidia-preview-pull-request-377.docs.buildwithfern.com/nemo/fabric |
Signed-off-by: Anuradha Karuppiah <26330987+AnuradhaKaruppiah@users.noreply.github.com>
Collaborator
Author
|
/nvskills-ci |
2 tasks done
Signed-off-by: nvskills-svc-account <svc-nvskills-signing@nvidia.com>
rapids-bot Bot
pushed a commit
that referenced
this pull request
Oct 8, 2026
#### Overview Group conversation turns under one NeMo Relay session. Callers reuse a UUID in `RunRequest.relay_session_root` across invocations; each invocation exports its own ATIF trajectory under the shared session ID. Rebased onto `main` at `e1f630ea`. The original feature, test, and documentation commits are preserved. The 0.4.1 release-version changes, manifest changes, lockfile changes, and release-example edits have been removed. Package versions remain those of main (0.5.0), with no dependency changes in this PR. #### Details - Carry optional `relay_session_root` directly from Rust/Python `RunRequest` to the typed Rust/Python/TypeScript `AgentRunRequest`, with schemas and generated references in parity. Adapters read the typed field directly; the context-key constant, lookup helper, and legacy context fallback are removed. Regression tests prove conflicting context keys do not affect propagation. - Deep Agents, mini-SWE-agent, and the reference LangGraph adapter use the shared root. A usable UUID request remains the parent; otherwise the session root is also the parent. Other adapters keep existing behavior. - Missing or unusable roots preserve per-request behavior. Nil UUIDs and UUIDs with a zero low-eight-byte span ID do not reach Relay's constructor. A bare `session_id` does not regroup traces. - Extend the persistent-runtime end-to-end test to cover both UUID and SDK-generated request IDs. Assert that each invocation has a distinct trajectory file and that later turns do not overwrite earlier trajectories. - Compatibility: Python/JSON callers can omit the field. Rust callers constructing `RunRequest` or `AgentRunRequest` via struct literals must initialize `relay_session_root` (or use a supported constructor/default). Match runtime and adapter-contract versions when sending the new field. #### Validation - Fresh isolated environment built from main's frozen lockfile, including the rebased native extension and real Relay 0.9.3. - Deep Agents E2E: **12 passed, 5 skipped**, including all **3 shared-session cases**. Uses a real native runtime, persistent Deep Agents host, local mock model endpoint, and real Relay ATIF exporter. Covers per-request roots, shared roots with UUID requests, and shared roots with SDK-generated requests. - Export evidence: two control invocations have different session IDs; four shared-root invocations across two runtimes produce four distinct ATIF files with session ID `018f47a4-3af7-7d94-8e61-9f0f89b5d314`. - Uploaded the four newly generated typed-field trajectories to local Phoenix project `fabric-pr358-typed-session` (**4 traces, 12 spans**). Phoenix indexes the reused conversation ID under the original `fabric-pr358-shared-session` session, which now contains **8 distinct traces** across the two validation runs. This is local validation; no live Helix/Intake ingestion was exercised. - `just test-rust`: **161 passed**; `cargo check -p fabric-python --locked` and Rust formatting pass. Rust tests use the isolated Python interpreter and its library path. - Complete `just test-python`: **1,867 passed, 96 skipped**, with no failures. TypeScript builds completed before Python tests to keep adapter entrypoints stable. - `just test-typescript`: passes, including adapter tests, package contents, consumer-install checks, and the audit gate (**0 high, 0 critical**). - `just docs`, Ruff checks, and diff hygiene pass. Generated API references are current. - Centralized NVSkills CI passed for the typed-field fix `ac263cc2`. Its generated benchmark reports, skill cards, and cryptographic signatures are included in service commit `8d145865`; both consumer skill reports recommend publication. All checks are green on final head `8d145865`, including Rust, the Python 3.11–3.14/platform matrix, wheel builds, TypeScript, pre-commit, NVSkills signature verification, DCO, and CodeRabbit. Validation used the temporary same-commit upstream draft #377 because the service rejects fork requests; that draft is now closed without merging. The original status gates timed out waiting for the long skills run and passed after retrying; no validation was bypassed. #### Where should the reviewer start? Start with `crates/fabric-core/src/agent_execution.rs` for the typed adapter request and `crates/fabric-core/src/runtime.rs` for direct field projection, then `adapters/python/common/src/nemo_fabric_adapters/common/utils.py` for UUID/root/parent selection. `tests/e2e/test_deepagents.py::test_deepagents_persistent_host_with_relay_and_mock_model` proves separate exported trajectories share the caller's session ID, including SDK-generated request IDs. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) Relates to #260, #263, #313. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. Authors: - Marcus (https://github.com/marcusds) - Ajay Thorve (https://github.com/AjayThorve) - Anuradha Karuppiah (https://github.com/AnuradhaKaruppiah) - https://github.com/svc-nvskills-signing Approvers: - Anuradha Karuppiah (https://github.com/AnuradhaKaruppiah) URL: #358
This branch was successfully deployed
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
4 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.
Overview
Temporary validation mirror of #358. The upstream branch starts at exactly the same commit,
4e243ee1c912efe27bf57aecdc2f4bcd8822958a, so the NVSkills service can validate the consumer-skill changes through its supported upstream-branch workflow. The service rejects fork-origin pull requests.This draft is for validation only. Keep #358 as the product pull request and close this mirror after validation. Do not merge this draft.
Details
The code, tests, documentation, and skills are identical to #358. No additional product changes are introduced by this mirror.
Validation
Rust, Python, TypeScript, and Check CI have passed on #358 at this commit. Request the missing NVSkills validation here and verify its status before requesting merge on #358.
Where should the reviewer start?
See #358 for the functional review and end-to-end evidence. This draft exists to run the supported NVSkills validation service on the identical commit.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to #358.
Summary by CodeRabbit