From 7e7291d3395a0ac24805216b506f15514c43e347 Mon Sep 17 00:00:00 2001 From: Will Killian <2007799+willkill07@users.noreply.github.com> Date: Tue, 4 Aug 2026 14:15:55 -0400 Subject: [PATCH 1/9] chore: remove npm CLI packaging (#690) #### Overview Remove the Node-launcher-based npm distribution of the NeMo Relay CLI. Native binaries, shell and PowerShell installers, Cargo installation, and PyPI `nemo-relay-cli-bin` wheels remain supported; Node.js bindings and OpenClaw npm packages are unchanged. - [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. #### Details - Simplify `scripts/package-cli-bin.py` and its tests to produce only PyPI wheels. - Remove npm CLI workspaces, lockfile entries, version-bump logic, CI artifacts, publication jobs, and path filters. - Remove npm CLI installation guidance and update release documentation to describe the retired npm packages. - Preserve native CLI release binaries and wheels, plus the existing Node.js binding and OpenClaw npm release paths. This intentionally removes the supported npm installation path for the CLI. Deprecating the already-published npm packages remains a post-merge registry operation so package history is retained. Validation: - Focused CLI wheel packaging unit tests: 3 passed. - `npm ci --ignore-scripts --audit=false --fund=false`. - `just test-python` in isolated user configuration: 614 passed, 45 skipped. - `just test-node` in isolated user configuration: 349 passed. - `just docs` and `just docs-linkcheck` (zero errors; existing external redirect check returned HTTP 403 warning). - `uv run pre-commit run --all-files`. #### Where should the reviewer start? Start with `scripts/package-cli-bin.py` and `.github/workflows/ci_rust.yml`, which show the packaging and release-automation boundary after npm CLI removal. Then review `RELEASING.md` for the maintainer-facing retirement guidance. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes RELAY-665 ## Summary by CodeRabbit * **Chores** * Discontinued npm CLI package distribution. CI/CD workflows updated to remove npm artifact validation, publishing, and uploads; now publish only Python wheel and Node.js packages. * Refactored CLI packaging script and configuration to support PyPI wheel distribution exclusively. * Simplified package workspace by removing npm CLI package module. * **Documentation** * Updated installation documentation, release guides, and README to remove npm CLI installation instructions. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Maryam Najafian (https://github.com/mnajafian-nv) URL: https://github.com/NVIDIA/NeMo-Relay/pull/690 --- .github/ci-path-filters.yml | 4 - .github/workflows/ci.yaml | 28 ----- .github/workflows/ci_rust.yml | 22 +--- .gitlab-ci.yml | 14 +-- README.md | 6 - RELEASING.md | 41 +++---- crates/cli/README.md | 6 - docs/getting-started/installation.mdx | 14 +-- justfile | 8 +- package-lock.json | 12 +- package.json | 3 +- packages/cli-bin/package.json | 7 -- scripts/package-cli-bin.py | 154 +------------------------- scripts/tests/test_package_cli_bin.py | 60 +--------- 14 files changed, 36 insertions(+), 343 deletions(-) delete mode 100644 packages/cli-bin/package.json diff --git a/.github/ci-path-filters.yml b/.github/ci-path-filters.yml index 812b9af1b..d0efde810 100644 --- a/.github/ci-path-filters.yml +++ b/.github/ci-path-filters.yml @@ -59,7 +59,6 @@ rust_package: - 'crates/ffi/nemo_relay.h' - 'crates/ffi/src/**' - 'justfile' - - 'packages/cli-bin/**' - 'python/cli-bin/**' - 'rust-toolchain.toml' - 'scripts/package-cli-bin.py' @@ -82,10 +81,8 @@ node_package: - 'justfile' - 'package.json' - 'package-lock.json' - - 'packages/cli-bin/**' - 'pyproject.toml' - 'rust-toolchain.toml' - - 'scripts/package-cli-bin.py' - 'scripts/package-node-bin.py' - 'uv.lock' @@ -132,7 +129,6 @@ dependencies: - 'integrations/**/package.json' - 'package.json' - 'package-lock.json' - - 'packages/cli-bin/**' - 'pyproject.toml' - 'python/cli-bin/**' - 'scripts/licensing/**' diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 6bd1e232b..c45f35cb5 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -310,12 +310,10 @@ jobs: cli_binaries=(nemo-relay-cli-*) cli_wheels=(nemo_relay_cli_bin-*.whl) python_api_wheels=(nemo_relay-*.whl) - cli_npm=(nemo-relay-bin-npm-*.tgz) node_npm=(nemo-relay-node-npm-*.tgz) expect_count 7 "CLI binary" "${cli_binaries[@]}" expect_count 7 "CLI wheel" "${cli_wheels[@]}" expect_count 7 "Python API wheel" "${python_api_wheels[@]}" - expect_count 8 "CLI npm" "${cli_npm[@]}" expect_count 8 "Node npm" "${node_npm[@]}" mapfile -t distribution_assets < <( @@ -523,13 +521,6 @@ jobs: name: openclaw-npm path: openclaw-package/ - - name: Download CLI npm artifacts - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 - with: - pattern: cli-npm-package-* - merge-multiple: true - path: cli-packages/ - - name: Select npm dist-tag run: | set -e @@ -558,25 +549,6 @@ jobs: npm publish "./$metapackage" --access public --tag "${NEMO_RELAY_NPM_DIST_TAG}" fi - - name: Publish CLI packages to npm - run: | - set -euo pipefail - version="${{ github.ref_name }}" - for pkg in cli-packages/nemo-relay-bin-npm-{linux-x64,linux-arm64,linux-x64-musl,linux-arm64-musl,darwin-arm64,win32-x64,win32-arm64}-${version}.tgz; do - name="$(tar xOf "$pkg" package/package.json | node -p 'JSON.parse(require("fs").readFileSync(0, "utf8")).name')" - if npm view "${name}@${version}" version --registry https://registry.npmjs.org >/dev/null 2>&1; then - echo "${name} ${version} already exists on npm; skipping" - continue - fi - npm publish "./$pkg" --access public --tag "${NEMO_RELAY_NPM_DIST_TAG}" - done - launcher="cli-packages/nemo-relay-bin-npm-${version}.tgz" - if npm view "nemo-relay-cli-bin@${version}" version --registry https://registry.npmjs.org >/dev/null 2>&1; then - echo "nemo-relay-cli-bin ${version} already exists on npm; skipping" - else - npm publish "./$launcher" --access public --tag "${NEMO_RELAY_NPM_DIST_TAG}" - fi - - name: Publish OpenClaw plugin package to npm run: | set -euo pipefail diff --git a/.github/workflows/ci_rust.yml b/.github/workflows/ci_rust.yml index c76ee9d30..85e0e43ff 100644 --- a/.github/workflows/ci_rust.yml +++ b/.github/workflows/ci_rust.yml @@ -327,7 +327,7 @@ jobs: mkdir -p "${NEMO_RELAY_CI_WORKSPACE_TMP}/cli" cp "$source" "${NEMO_RELAY_CI_WORKSPACE_TMP}/cli/${asset}" - - name: Package CLI binary for PyPI and npm + - name: Package CLI binary for PyPI working-directory: ${{ env.NEMO_RELAY_CI_WORKSPACE }} run: | set -euo pipefail @@ -343,16 +343,11 @@ jobs: fi rm -rf "${NEMO_RELAY_CI_WORKSPACE_TMP}/cli-packages" mkdir -p "${NEMO_RELAY_CI_WORKSPACE_TMP}/cli-packages" - args=( - --binary "$source" - --target "$target" - --version "$version" + python scripts/package-cli-bin.py \ + --binary "$source" \ + --target "$target" \ + --version "$version" \ --output-dir "${NEMO_RELAY_CI_WORKSPACE_TMP}/cli-packages" - ) - if [ "${{ matrix.platform }}" = "linux-amd64" ]; then - args+=(--npm-launcher) - fi - python scripts/package-cli-bin.py "${args[@]}" - name: Install and run CLI wheel if: ${{ !endsWith(matrix.target, '-musl') }} @@ -402,10 +397,3 @@ jobs: name: cli-python-wheel-${{ matrix.platform }} path: ${{ env.NEMO_RELAY_CI_WORKSPACE_TMP }}/cli-packages/*.whl if-no-files-found: error - - - name: Upload CLI npm artifact - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 - with: - name: cli-npm-package-${{ matrix.platform }} - path: ${{ env.NEMO_RELAY_CI_WORKSPACE_TMP }}/cli-packages/*.tgz - if-no-files-found: error diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 92153c744..1e89de2c4 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -68,7 +68,7 @@ collect:github-artifacts: fi export GH_TOKEN="${NEMO_RELAY_CI_GITHUB_TOKEN}" - mkdir -p collected/wheels collected/sdists collected/node collected/cli-npm downloaded + mkdir -p collected/wheels collected/sdists collected/node downloaded tag="${CI_COMMIT_TAG:-}" if [ -z "$tag" ]; then @@ -133,7 +133,6 @@ collect:github-artifacts: gh run download "$run_id" --repo "$NEMO_RELAY_CI_GITHUB_REPOSITORY" --pattern 'wheel-*' --dir downloaded/wheels gh run download "$run_id" --repo "$NEMO_RELAY_CI_GITHUB_REPOSITORY" --pattern 'cli-python-wheel-*' --dir downloaded/cli-wheels - gh run download "$run_id" --repo "$NEMO_RELAY_CI_GITHUB_REPOSITORY" --pattern 'cli-npm-package-*' --dir downloaded/cli-npm gh run download "$run_id" --repo "$NEMO_RELAY_CI_GITHUB_REPOSITORY" --name 'plugin-wheel' --dir downloaded/wheels gh run download "$run_id" --repo "$NEMO_RELAY_CI_GITHUB_REPOSITORY" --name 'python-sdist' --dir downloaded/sdists gh run download "$run_id" --repo "$NEMO_RELAY_CI_GITHUB_REPOSITORY" --pattern 'node-npm-package-*' --dir downloaded/node @@ -141,7 +140,6 @@ collect:github-artifacts: find downloaded/wheels -type f -name '*.whl' -exec cp {} collected/wheels/ \; find downloaded/cli-wheels -type f -name '*.whl' -exec cp {} collected/wheels/ \; find downloaded/sdists -type f -name 'nemo_relay-*.tar.gz' -exec cp {} collected/sdists/ \; - find downloaded/cli-npm -type f -name '*.tgz' -exec cp {} collected/cli-npm/ \; find downloaded/node -type f -name '*.tgz' -exec cp {} collected/node/ \; if ! ls collected/wheels/*.whl >/dev/null 2>&1; then @@ -156,10 +154,6 @@ collect:github-artifacts: echo "Error: collected GitHub Node artifacts did not contain any .tgz files." >&2 exit 1 fi - if ! ls collected/cli-npm/*.tgz >/dev/null 2>&1; then - echo "Error: collected GitHub CLI npm artifacts did not contain any .tgz files." >&2 - exit 1 - fi { printf '{\n' @@ -177,7 +171,6 @@ collect:github-artifacts: - collected/wheels/*.whl - collected/sdists/nemo_relay-*.tar.gz - collected/node/*.tgz - - collected/cli-npm/*.tgz - collected/github-run.json publish:artifactory:wheels: @@ -337,8 +330,3 @@ publish:artifactory:npm: npm publish --tag dev "$package" done npm publish --tag dev "collected/node/nemo-relay-node-npm-${CI_COMMIT_TAG}.tgz" - for platform in linux-x64 linux-arm64 linux-x64-musl linux-arm64-musl darwin-arm64 win32-x64 win32-arm64; do - package="collected/cli-npm/nemo-relay-bin-npm-${platform}-${CI_COMMIT_TAG}.tgz" - npm publish --tag dev "$package" - done - npm publish --tag dev "collected/cli-npm/nemo-relay-bin-npm-${CI_COMMIT_TAG}.tgz" diff --git a/README.md b/README.md index e064be81e..94b223fe9 100644 --- a/README.md +++ b/README.md @@ -58,12 +58,6 @@ Install the prebuilt CLI from PyPI: pip install nemo-relay-cli-bin ``` -Install the prebuilt CLI from npm: - -```bash -npm install --global nemo-relay-cli-bin -``` - Python API users can install the matching CLI through the optional extra: ```bash diff --git a/RELEASING.md b/RELEASING.md index f2ce9ba00..cf6d058cb 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -32,13 +32,16 @@ The release pipeline publishes these package surfaces from a tag push: |---|---| | crates.io | `nemo-relay-types`, `nemo-relay-plugin`, `nemo-relay-worker-proto`, `nemo-relay-worker`, `nemo-relay`, `nemo-relay-adaptive`, `nemo-relay-pii-redaction`, `nemo-relay-switchyard`, `nemo-relay-ffi`, `nemo-relay-cli` | | PyPI | `nemo-relay` wheels and source distribution, `nemo-relay-plugin` and `nemo-relay-cli-bin` wheels | -| npm | `nemo-relay-node` and its seven platform packages, `nemo-relay-openclaw`, `nemo-relay-cli-bin`, and its seven platform packages | -| GitHub Releases | CLI binaries, `nemo-relay` and `nemo-relay-cli-bin` wheels, CLI and Node npm tarballs, and checksums | +| npm | `nemo-relay-node` and its seven platform packages, and `nemo-relay-openclaw` | +| GitHub Releases | CLI binaries, `nemo-relay` and `nemo-relay-cli-bin` wheels, Node npm tarballs, and checksums | | Fern | The documentation site | Go remains source-first. There is no separate Go package-manager publication step in the repository release workflow. +The former npm `nemo-relay-cli-bin` metapackage and its seven platform +packages are retired and must not be republished. + The mirrored GitLab pipeline also publishes the same tag's collected package artifacts to NVIDIA Artifactory. It is driven by the tag push, not by a GitLab pipeline schedule. @@ -61,9 +64,8 @@ NeMo Relay versions are anchored on the workspace SemVer in the repository root lock entries and must be updated with it. - `integrations/openclaw/package.json` carries the base npm version for the OpenClaw plugin package and must stay aligned with the same release version. -- `packages/cli-bin/package.json` and `python/cli-bin/pyproject.toml` carry the - base CLI package versions in their registry-specific SemVer and PEP 440 - spellings. The `nemo-relay[cli]` extra must pin the exact PyPI version. +- `python/cli-bin/pyproject.toml` carries the base CLI package version in its + PEP 440 spelling. The `nemo-relay[cli]` extra must pin the exact PyPI version. - The Python package version is derived at packaging time. `pyproject.toml` stays `dynamic = ["version"]` in the repository, and the packaging recipe writes a concrete version into `pyproject.toml` and `crates/python/Cargo.toml` @@ -151,11 +153,6 @@ Before you create a release tag, confirm the following: `nemo-relay-node-linux-x64-musl`, `nemo-relay-node-linux-arm64-musl`, `nemo-relay-node-darwin-arm64`, `nemo-relay-node-win32-x64-msvc`, and `nemo-relay-node-win32-arm64-msvc` - - npm trusted publishers are configured for `nemo-relay-cli-bin` and - `nemo-relay-cli-bin-linux-x64`, `nemo-relay-cli-bin-linux-arm64`, - `nemo-relay-cli-bin-linux-x64-musl`, `nemo-relay-cli-bin-linux-arm64-musl`, - `nemo-relay-cli-bin-darwin-arm64`, `nemo-relay-cli-bin-win32-x64`, and - `nemo-relay-cli-bin-win32-arm64` 5. The GitHub Release entry is ready to become the only canonical release-notes surface. @@ -182,7 +179,7 @@ The helper updates: 4. [`integrations/openclaw/package.json`](integrations/openclaw/package.json) and the `integrations/openclaw` entry in the root [`package-lock.json`](package-lock.json) to the same release version. -5. The Python and npm `nemo-relay-cli-bin` metadata, including the exact +5. The Python `nemo-relay-cli-bin` metadata, including the exact `nemo-relay[cli]` dependency version. Review docs and snippets that mention explicit versions, including: @@ -268,18 +265,15 @@ The release pipeline then: - `package-python-plugin` builds the `nemo-relay-plugin` wheel. - The Rust CLI matrix builds GNU Linux binaries in manylinux containers and musl Linux binaries natively, validates each in its matching container, and - packages each prebuilt binary as a - `nemo-relay-cli-bin` wheel and npm platform package, and creates the npm - launcher package once. + packages each prebuilt binary as a `nemo-relay-cli-bin` wheel. - The distribution release-asset job uploads the CLI binaries, `nemo-relay` - API wheels, CLI wheels, CLI npm packages, and split Node npm packages. + API wheels, CLI wheels, and split Node npm packages. `SHA256SUMS` covers every attached distribution artifact, and raw CLI binaries also receive individual `.sha256` files for installer compatibility. GitHub-facing npm artifacts use - `nemo-relay-bin-npm[--]-.tgz` for the CLI and - `nemo-relay-node-npm[--]-.tgz` for Node.js; their - registry package names remain in the tarball manifests. + `nemo-relay-node-npm[--]-.tgz`; their registry package + names remain in the tarball manifests. 4. Publishes packages from the top-level workflow after the reusable packaging jobs complete: - `publish-rust` stamps Cargo workspace versions from the release tag, then @@ -293,8 +287,8 @@ The release pipeline then: artifacts plus the `nemo-relay-plugin` and `nemo-relay-cli-bin` wheels, then uploads them to PyPI with trusted publishing from the top-level workflow - - `publish-npm` publishes the Node.js and CLI native packages before their - metapackages, then publishes the OpenClaw package, through npm trusted + - `publish-npm` publishes the Node.js native packages before their + metapackage, then publishes the OpenClaw package, through npm trusted publishing from the top-level workflow - Stable tags publish to the npm `latest` dist-tag - Prerelease tags such as `0.1.0-rc.1` publish to the npm `next` @@ -329,8 +323,7 @@ npm trusted publishing has its own registry-side constraints: - Each npm package can only have one trusted publisher configured at a time. - Configure trusted publishers for `nemo-relay-node`, all seven Node platform - packages, `nemo-relay-openclaw`, `nemo-relay-cli-bin`, and all seven CLI - platform packages before pushing a release tag. + packages, and `nemo-relay-openclaw` before pushing a release tag. - npm trusted publishing currently supports GitHub-hosted runners, not self-hosted runners. @@ -363,8 +356,8 @@ After the release is live, verify: are visible on crates.io. 2. The `nemo-relay` and `nemo-relay-cli-bin` wheels are visible on PyPI, and `pip install "nemo-relay[cli]"` exposes `nemo-relay`. -3. The `nemo-relay-node`, its seven platform packages, `nemo-relay-openclaw`, - `nemo-relay-cli-bin`, and its seven platform packages are visible on npm. +3. The `nemo-relay-node`, its seven platform packages, and + `nemo-relay-openclaw` are visible on npm. 4. The Unix and Windows installers resolve the new stable tag and verify matching CLI release asset checksums on their supported platforms. 5. The Fern documentation site shows the expected version and release notes. diff --git a/crates/cli/README.md b/crates/cli/README.md index b2d39ea10..19badc512 100644 --- a/crates/cli/README.md +++ b/crates/cli/README.md @@ -68,12 +68,6 @@ Install the prebuilt CLI from PyPI: pip install nemo-relay-cli-bin ``` -Install the prebuilt CLI from npm: - -```bash -npm install --global nemo-relay-cli-bin -``` - Install the Python API and matching CLI with the optional extra: ```bash diff --git a/docs/getting-started/installation.mdx b/docs/getting-started/installation.mdx index eb6830f3c..254ccc442 100644 --- a/docs/getting-started/installation.mdx +++ b/docs/getting-started/installation.mdx @@ -45,12 +45,6 @@ pip install "nemo-relay[cli]" ``` - -```bash -npm install --global nemo-relay-cli-bin -``` - - ```bash curl -fsSL https://raw.githubusercontent.com/NVIDIA/NeMo-Relay/main/install.sh | sh @@ -114,9 +108,9 @@ user `PATH`; the Unix installer does not edit shell configuration. Use the GNU target on glibc-based distributions such as Ubuntu, Debian, and RHEL. Use the musl target on Alpine Linux and other musl-based systems. The Unix shell installer intentionally selects the portable musl target on Linux. -For package versions that publish the corresponding artifacts, `pip`, `uv`, and -`npm` automatically select a target compatible with the host CPU and C library -on supported platforms. +For package versions that publish the corresponding artifacts, `pip` and `uv` +automatically select a target compatible with the host CPU and C library on +supported platforms. The installer reports the detected operating system and architecture when no matching asset exists. Intel macOS is not supported because no matching release @@ -186,7 +180,7 @@ new download before replacing the binary in the selected installation directory. ### Alternative Installation Methods -The PyPI, npm, and installer methods install the same CLI release with a binary +The PyPI and installer methods install the same CLI release with a binary compatible with the host platform. Use Cargo on unsupported platforms or when you prefer to build from source: diff --git a/justfile b/justfile index 5f058e28c..fd6f78737 100644 --- a/justfile +++ b/justfile @@ -564,7 +564,6 @@ set_node_package_versions() { local version="$1" set_npm_package_version crates/node/package.json package-lock.json "$version" crates/node set_npm_package_version integrations/openclaw/package.json package-lock.json "$version" integrations/openclaw - set_npm_package_version packages/cli-bin/package.json package-lock.json "$version" packages/cli-bin set_npm_package_dependency_version integrations/openclaw/package.json package-lock.json integrations/openclaw nemo-relay-node "$version" } @@ -1769,8 +1768,8 @@ package-python-plugin: exit 1 fi -# Package a prebuilt CLI binary for PyPI and npm. -package-cli-bin binary target version package_dir npm_launcher="false": +# Package a prebuilt CLI binary for PyPI. +package-cli-bin binary target version package_dir: #!/usr/bin/env bash set -euo pipefail cd "$NEMO_RELAY_REPO_ROOT" @@ -1780,7 +1779,4 @@ package-cli-bin binary target version package_dir npm_launcher="false": --version "{{ version }}" --output-dir "{{ package_dir }}" ) - if [[ "{{ npm_launcher }}" == "true" ]]; then - args+=(--npm-launcher) - fi uv run --no-project python scripts/package-cli-bin.py "${args[@]}" diff --git a/package-lock.json b/package-lock.json index ead50344b..475fe716e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7,8 +7,7 @@ "name": "nemo-relay-workspace", "workspaces": [ "crates/node", - "integrations/openclaw", - "packages/cli-bin" + "integrations/openclaw" ], "devDependencies": { "fern-api": "5.57.0", @@ -929,10 +928,6 @@ "node": ">=16 || 14 >=14.17" } }, - "node_modules/nemo-relay-cli-bin": { - "resolved": "packages/cli-bin", - "link": true - }, "node_modules/nemo-relay-node": { "resolved": "crates/node", "link": true @@ -5134,11 +5129,6 @@ "funding": { "url": "https://github.com/sponsors/eemeli" } - }, - "packages/cli-bin": { - "name": "nemo-relay-cli-bin", - "version": "0.7.0", - "license": "Apache-2.0" } } } diff --git a/package.json b/package.json index 9c8d01d4e..cf8803a04 100644 --- a/package.json +++ b/package.json @@ -7,8 +7,7 @@ }, "workspaces": [ "crates/node", - "integrations/openclaw", - "packages/cli-bin" + "integrations/openclaw" ], "devDependencies": { "fern-api": "5.57.0", diff --git a/packages/cli-bin/package.json b/packages/cli-bin/package.json deleted file mode 100644 index 43cc3afba..000000000 --- a/packages/cli-bin/package.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "name": "nemo-relay-cli-bin", - "version": "0.7.0", - "description": "Prebuilt NeMo Relay command-line interface.", - "private": true, - "license": "Apache-2.0" -} diff --git a/scripts/package-cli-bin.py b/scripts/package-cli-bin.py index 15d5eefaa..690452950 100755 --- a/scripts/package-cli-bin.py +++ b/scripts/package-cli-bin.py @@ -2,18 +2,15 @@ # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 -"""Package a prebuilt NeMo Relay CLI binary for PyPI and npm.""" +"""Package a prebuilt NeMo Relay CLI binary for PyPI.""" from __future__ import annotations import argparse import base64 import hashlib -import io -import json import os import stat -import tarfile import zipfile from dataclasses import dataclass from pathlib import Path @@ -30,17 +27,8 @@ class Platform: """Describe one supported CLI distribution platform.""" target: str - npm_suffix: str - npm_os: str - npm_cpu: str wheel_platforms: tuple[str, ...] executable: str - libc: str | None = None - - @property - def npm_package(self) -> str: - """Return the npm package name for this platform.""" - return f"{PACKAGE_NAME}-{self.npm_suffix}" PLATFORMS = { @@ -48,61 +36,36 @@ def npm_package(self) -> str: for platform in ( Platform( "x86_64-unknown-linux-gnu", - "linux-x64", - "linux", - "x64", ("manylinux_2_17_x86_64",), "nemo-relay", - "glibc", ), Platform( "aarch64-unknown-linux-gnu", - "linux-arm64", - "linux", - "arm64", ("manylinux_2_17_aarch64",), "nemo-relay", - "glibc", ), Platform( "x86_64-unknown-linux-musl", - "linux-x64-musl", - "linux", - "x64", ("musllinux_1_2_x86_64",), "nemo-relay", - "musl", ), Platform( "aarch64-unknown-linux-musl", - "linux-arm64-musl", - "linux", - "arm64", ("musllinux_1_2_aarch64",), "nemo-relay", - "musl", ), Platform( "aarch64-apple-darwin", - "darwin-arm64", - "darwin", - "arm64", ("macosx_11_0_arm64",), "nemo-relay", ), Platform( "x86_64-pc-windows-msvc", - "win32-x64", - "win32", - "x64", ("win_amd64",), "nemo-relay.exe", ), Platform( "aarch64-pc-windows-msvc", - "win32-arm64", - "win32", - "arm64", ("win_arm64",), "nemo-relay.exe", ), @@ -190,109 +153,6 @@ def build_wheel(binary: Path, platform: Platform, version: str, output: Path) -> return destination -def add_tar_bytes(archive: tarfile.TarFile, path: str, content: bytes, mode: int = 0o644) -> None: - """Add one regular file to an npm tarball.""" - info = tarfile.TarInfo(path) - info.size = len(content) - info.mode = mode - archive.addfile(info, io.BytesIO(content)) - - -def build_npm_platform(binary: Path, platform: Platform, version: str, output: Path) -> Path: - """Build an OS- and CPU-constrained npm package containing the CLI binary.""" - filename = f"nemo-relay-bin-npm-{platform.npm_suffix}-{version}.tgz" - destination = output / filename - manifest = { - "name": platform.npm_package, - "version": version, - "description": f"{SUMMARY} ({platform.npm_os} {platform.npm_cpu})", - "os": [platform.npm_os], - "cpu": [platform.npm_cpu], - "files": [f"bin/{platform.executable}"], - "license": LICENSE, - "repository": {"type": "git", "url": f"git+{REPOSITORY}.git"}, - } - if platform.libc is not None: - manifest["libc"] = [platform.libc] - with tarfile.open(destination, "w:gz") as archive: - add_tar_bytes(archive, "package/package.json", json.dumps(manifest, indent=2).encode() + b"\n") - add_tar_bytes( - archive, - f"package/bin/{platform.executable}", - binary.read_bytes(), - mode=0o755, - ) - add_tar_bytes(archive, "package/LICENSE", (ROOT / "LICENSE").read_bytes()) - return destination - - -def launcher_source() -> bytes: - """Return the Node.js launcher that selects the installed native package.""" - mapping: dict[str, dict[str, dict[str, str]]] = {} - for platform in PLATFORMS.values(): - key = f"{platform.npm_os}-{platform.npm_cpu}" - libc = platform.libc or "default" - mapping.setdefault(key, {})[libc] = { - "package": platform.npm_package, - "executable": platform.executable, - } - return ( - "#!/usr/bin/env node\n" - "// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved.\n" - "// SPDX-License-Identifier: Apache-2.0\n\n" - "const { spawnSync } = require('node:child_process');\n" - "const path = require('node:path');\n\n" - f"const platforms = {json.dumps(mapping, indent=2)};\n" - "const key = `${process.platform}-${process.arch}`;\n" - "const libc = process.platform === 'linux'\n" - " ? (process.report?.getReport?.().header.glibcVersionRuntime ? 'glibc' : 'musl')\n" - " : 'default';\n" - "const selected = platforms[key]?.[libc] ?? platforms[key]?.default;\n" - "if (!selected) {\n" - " console.error(`nemo-relay-cli-bin does not support ${process.platform}/${process.arch}`);\n" - " process.exit(1);\n" - "}\n" - "let manifest;\n" - "try {\n" - " manifest = require.resolve(`${selected.package}/package.json`);\n" - "} catch (error) {\n" - " console.error(\n" - " `The native package ${selected.package} is missing. ` +\n" - " 'Reinstall nemo-relay-cli-bin without omitting optional dependencies.',\n" - " );\n" - " process.exit(1);\n" - "}\n" - "const executable = path.join(path.dirname(manifest), 'bin', selected.executable);\n" - "const result = spawnSync(executable, process.argv.slice(2), { stdio: 'inherit' });\n" - "if (result.error) {\n" - " console.error(`Failed to start ${executable}: ${result.error.message}`);\n" - " process.exit(1);\n" - "}\n" - "process.exit(result.status === null ? 1 : result.status);\n" - ).encode() - - -def build_npm_launcher(version: str, output: Path) -> Path: - """Build the portable npm launcher package.""" - destination = output / f"nemo-relay-bin-npm-{version}.tgz" - manifest = { - "name": PACKAGE_NAME, - "version": version, - "description": SUMMARY, - "bin": {"nemo-relay": "bin/nemo-relay.js"}, - "files": ["bin/nemo-relay.js"], - "engines": {"node": ">=24.0.0"}, - "optionalDependencies": {platform.npm_package: version for platform in PLATFORMS.values()}, - "license": LICENSE, - "repository": {"type": "git", "url": f"git+{REPOSITORY}.git"}, - } - with tarfile.open(destination, "w:gz") as archive: - add_tar_bytes(archive, "package/package.json", json.dumps(manifest, indent=2).encode() + b"\n") - add_tar_bytes(archive, "package/bin/nemo-relay.js", launcher_source(), mode=0o755) - add_tar_bytes(archive, "package/LICENSE", (ROOT / "LICENSE").read_bytes()) - return destination - - def parse_args() -> argparse.Namespace: """Parse CLI package assembly arguments.""" parser = argparse.ArgumentParser() @@ -300,25 +160,17 @@ def parse_args() -> argparse.Namespace: parser.add_argument("--target", choices=sorted(PLATFORMS), required=True) parser.add_argument("--version", required=True) parser.add_argument("--output-dir", type=Path, required=True) - parser.add_argument("--npm-launcher", action="store_true") return parser.parse_args() def main() -> None: - """Build the wheel and npm artifacts requested on the command line.""" + """Build the wheel requested on the command line.""" args = parse_args() if not args.binary.is_file(): raise SystemExit(f"CLI binary does not exist: {args.binary}") args.output_dir.mkdir(parents=True, exist_ok=True) platform = PLATFORMS[args.target] - artifacts = [ - build_wheel(args.binary, platform, args.version, args.output_dir), - build_npm_platform(args.binary, platform, args.version, args.output_dir), - ] - if args.npm_launcher: - artifacts.append(build_npm_launcher(args.version, args.output_dir)) - for artifact in artifacts: - print(artifact) + print(build_wheel(args.binary, platform, args.version, args.output_dir)) if __name__ == "__main__": diff --git a/scripts/tests/test_package_cli_bin.py b/scripts/tests/test_package_cli_bin.py index 6d352cd46..90e1a78da 100644 --- a/scripts/tests/test_package_cli_bin.py +++ b/scripts/tests/test_package_cli_bin.py @@ -1,20 +1,16 @@ # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 -"""Tests for CLI wheel and npm package assembly.""" +"""Tests for CLI wheel assembly.""" import importlib.util -import json import os import stat -import subprocess import sys -import tarfile import tempfile import unittest import zipfile from pathlib import Path -from typing import IO ROOT = Path(__file__).resolve().parents[2] SPEC = importlib.util.spec_from_file_location("package_cli_bin", ROOT / "scripts" / "package-cli-bin.py") @@ -24,15 +20,8 @@ SPEC.loader.exec_module(PACKAGE_CLI_BIN) -def required_member(archive: tarfile.TarFile, name: str) -> IO[bytes]: - member = archive.extractfile(name) - if member is None: - raise AssertionError(f"archive is missing {name}") - return member - - class PackageCliBinTests(unittest.TestCase): - def test_packages_linux_binaries_for_python_and_npm(self) -> None: + def test_packages_linux_binaries_for_python(self) -> None: with tempfile.TemporaryDirectory() as temporary: output = Path(temporary) binary = output / "nemo-relay" @@ -44,10 +33,7 @@ def test_packages_linux_binaries_for_python_and_npm(self) -> None: try: os.chdir(output) wheel = PACKAGE_CLI_BIN.build_wheel(binary, gnu_platform, "0.7.0-rc.1", output) - native = PACKAGE_CLI_BIN.build_npm_platform(binary, gnu_platform, "0.7.0-rc.1", output) musl_wheel = PACKAGE_CLI_BIN.build_wheel(binary, musl_platform, "0.7.0-rc.1", output) - musl_native = PACKAGE_CLI_BIN.build_npm_platform(binary, musl_platform, "0.7.0-rc.1", output) - launcher = PACKAGE_CLI_BIN.build_npm_launcher("0.7.0-rc.1", output) finally: os.chdir(previous_directory) @@ -63,48 +49,6 @@ def test_packages_linux_binaries_for_python_and_npm(self) -> None: self.assertIn("0.7.0rc1-py3-none-musllinux_1_2_x86_64", musl_wheel.name) - self.assertEqual(native.name, "nemo-relay-bin-npm-linux-x64-0.7.0-rc.1.tgz") - with tarfile.open(native) as archive: - manifest = json.load(required_member(archive, "package/package.json")) - self.assertEqual(manifest["os"], ["linux"]) - self.assertEqual(manifest["cpu"], ["x64"]) - self.assertEqual(manifest["libc"], ["glibc"]) - self.assertEqual( - required_member(archive, "package/bin/nemo-relay").read(), - b"test-binary", - ) - - self.assertEqual(musl_native.name, "nemo-relay-bin-npm-linux-x64-musl-0.7.0-rc.1.tgz") - with tarfile.open(musl_native) as archive: - manifest = json.load(required_member(archive, "package/package.json")) - self.assertEqual(manifest["libc"], ["musl"]) - - self.assertEqual(launcher.name, "nemo-relay-bin-npm-0.7.0-rc.1.tgz") - with tarfile.open(launcher) as archive: - manifest = json.load(required_member(archive, "package/package.json")) - self.assertEqual(manifest["bin"]["nemo-relay"], "bin/nemo-relay.js") - self.assertEqual( - manifest["optionalDependencies"]["nemo-relay-cli-bin-linux-x64"], - "0.7.0-rc.1", - ) - self.assertEqual( - manifest["optionalDependencies"]["nemo-relay-cli-bin-linux-x64-musl"], - "0.7.0-rc.1", - ) - launcher_path = output / "launcher/package/bin/nemo-relay.js" - launcher_path.parent.mkdir(parents=True) - launcher_path.write_bytes(required_member(archive, "package/bin/nemo-relay.js").read()) - - result = subprocess.run( - ["node", output / "launcher/package/bin/nemo-relay.js", "--version"], - check=False, - capture_output=True, - text=True, - ) - self.assertEqual(result.returncode, 1) - self.assertIn("The native package nemo-relay-cli-bin-", result.stderr) - self.assertIn("is missing", result.stderr) - def test_rejects_unsupported_version(self) -> None: with self.assertRaisesRegex(ValueError, "unsupported package version"): PACKAGE_CLI_BIN.wheel_version("dev-deadbeef") From d96291dda8e981938e2298c19e944810b1102f64 Mon Sep 17 00:00:00 2001 From: Will Killian <2007799+willkill07@users.noreply.github.com> Date: Tue, 4 Aug 2026 14:16:26 -0400 Subject: [PATCH 2/9] fix: canonicalize OpenTelemetry destinations (#693) #### Overview Canonicalize OpenTelemetry destinations for duplicate detection while leaving exporter request URLs and public configuration unchanged. - [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. #### Details - Compare structured destinations after existing bare OTLP/HTTP trace-path resolution. - Realize default HTTP and HTTPS ports, normalize repeated and trailing path slashes, and retain query ordering and values. - Canonicalize standardized localhost names, IPv4 `127.0.0.0/8` forms, IPv4 shorthand, and IPv6 `::1` without DNS lookups. - Preserve transport and scheme distinctions, repeated projection support, parse-failure fallback behavior, and exporter routing. - Add validation and activation-time regression coverage and document endpoint equivalence rules. Validation included the Rust, Python, Node.js, and Go suites; documentation and link checks; and `uv run pre-commit run --all-files`. #### Where should the reviewer start? Start with `canonicalize_opentelemetry_destination` in `crates/core/src/observability/plugin_component.rs`, then review the table-driven collision cases in `crates/core/tests/unit/observability/plugin_component_tests.rs`. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes RELAY-630 ## Summary by CodeRabbit * **Bug Fixes** * Improved OpenTelemetry endpoint collision detection by recognizing equivalent URL formats, including default ports, normalized paths, host casing, and loopback addresses. * Preserved meaningful differences such as schemes, ports, hosts, paths, query strings, and transport types. * Updated collision messages to display normalized destinations more clearly. * **Documentation** * Expanded observability configuration guidance with details about endpoint normalization and duplicate detection behavior. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Maryam Najafian (https://github.com/mnajafian-nv) URL: https://github.com/NVIDIA/NeMo-Relay/pull/693 --- .../src/observability/plugin_component.rs | 107 ++++++++++++- .../observability/plugin_component_tests.rs | 145 +++++++++++++++++- .../observability/configuration.mdx | 8 +- .../observability/opentelemetry.mdx | 6 +- 4 files changed, 253 insertions(+), 13 deletions(-) diff --git a/crates/core/src/observability/plugin_component.rs b/crates/core/src/observability/plugin_component.rs index b622f3016..9929589ec 100644 --- a/crates/core/src/observability/plugin_component.rs +++ b/crates/core/src/observability/plugin_component.rs @@ -21,6 +21,7 @@ use std::borrow::Cow; use std::collections::{HashMap, HashSet}; use std::future::Future; +use std::net::IpAddr; use std::panic::{AssertUnwindSafe, catch_unwind}; use std::path::{Component, Path, PathBuf}; use std::pin::Pin; @@ -2377,6 +2378,23 @@ struct OpenTelemetryDestinationCollision { message: String, } +#[derive(Debug, PartialEq, Eq)] +enum OpenTelemetryDestinationKey { + Url { + scheme: String, + host: String, + port: Option, + path: String, + query: Option, + }, + Raw(String), +} + +struct OpenTelemetryDestination { + key: OpenTelemetryDestinationKey, + display: String, +} + fn validate_distinct_opentelemetry_destinations( endpoints: &[OpenTelemetryEndpointConfig], ) -> PluginResult<()> { @@ -2398,7 +2416,7 @@ fn opentelemetry_destination_collision_errors( let endpoint_destination = opentelemetry_destination(endpoint); let other_destination = opentelemetry_destination(other); if endpoint.transport == other.transport - && endpoint_destination == other_destination + && endpoint_destination.key == other_destination.key && endpoint.otel_type != other.otel_type { errors.push(OpenTelemetryDestinationCollision { @@ -2408,7 +2426,7 @@ fn opentelemetry_destination_collision_errors( opentelemetry_type_name(other.otel_type), opentelemetry_type_name(endpoint.otel_type), endpoint.transport, - endpoint_destination, + endpoint_destination.display, ), }); } @@ -2417,13 +2435,94 @@ fn opentelemetry_destination_collision_errors( errors } -fn opentelemetry_destination(endpoint: &OpenTelemetryEndpointConfig) -> Cow<'_, str> { +fn opentelemetry_destination(endpoint: &OpenTelemetryEndpointConfig) -> OpenTelemetryDestination { let configured_endpoint = endpoint.endpoint.trim(); - if endpoint.transport == "http_binary" { + let effective_endpoint = if endpoint.transport == "http_binary" { resolve_http_trace_endpoint(configured_endpoint) } else { Cow::Borrowed(configured_endpoint) + }; + canonicalize_opentelemetry_destination(&effective_endpoint) +} + +fn canonicalize_opentelemetry_destination(endpoint: &str) -> OpenTelemetryDestination { + let Ok(url) = reqwest::Url::parse(endpoint) else { + return raw_opentelemetry_destination(endpoint); + }; + if !matches!(url.scheme(), "http" | "https") { + return raw_opentelemetry_destination(endpoint); + } + let Some(url_host) = url.host_str() else { + return raw_opentelemetry_destination(endpoint); + }; + + let scheme = url.scheme().to_string(); + let host = canonical_opentelemetry_host(url_host); + let port = url.port_or_known_default(); + let path = normalize_opentelemetry_path(url.path()); + let query = url.query().map(str::to_string); + let display = format!( + "{scheme}://{host}{}{path}{}", + port.map(|port| format!(":{port}")).unwrap_or_default(), + query + .as_deref() + .map(|query| format!("?{query}")) + .unwrap_or_default(), + ); + OpenTelemetryDestination { + key: OpenTelemetryDestinationKey::Url { + scheme, + host, + port, + path, + query, + }, + display, + } +} + +fn raw_opentelemetry_destination(endpoint: &str) -> OpenTelemetryDestination { + OpenTelemetryDestination { + key: OpenTelemetryDestinationKey::Raw(endpoint.to_string()), + display: endpoint.to_string(), + } +} + +fn canonical_opentelemetry_host(host: &str) -> String { + let domain = host.strip_suffix('.').unwrap_or(host); + let unbracketed = host + .strip_prefix('[') + .and_then(|host| host.strip_suffix(']')) + .unwrap_or(host); + let is_loopback_domain = domain == "localhost" || domain.ends_with(".localhost"); + let is_loopback_address = unbracketed + .parse::() + .is_ok_and(|address| address.is_loopback()); + if is_loopback_domain || is_loopback_address { + "".to_string() + } else { + host.to_string() + } +} + +fn normalize_opentelemetry_path(path: &str) -> String { + let mut normalized = String::with_capacity(path.len()); + let mut previous_was_slash = false; + for character in path.chars() { + if character == '/' { + if !previous_was_slash { + normalized.push(character); + } + previous_was_slash = true; + } else { + normalized.push(character); + previous_was_slash = false; + } + } + while normalized.len() > 1 && normalized.ends_with('/') { + normalized.pop(); } + normalized } const fn opentelemetry_type_name(otel_type: OpenTelemetryType) -> &'static str { diff --git a/crates/core/tests/unit/observability/plugin_component_tests.rs b/crates/core/tests/unit/observability/plugin_component_tests.rs index f6e4eb362..48f3f6775 100644 --- a/crates/core/tests/unit/observability/plugin_component_tests.rs +++ b/crates/core/tests/unit/observability/plugin_component_tests.rs @@ -2732,7 +2732,140 @@ fn opentelemetry_endpoints_fan_out_to_heterogeneous_and_repeated_types() { } #[test] -fn opentelemetry_rejects_different_projection_types_at_the_same_effective_destination() { +fn opentelemetry_rejects_canonical_equivalent_destinations() { + for (first, second) in [ + ( + "http://collector.example/v1/traces", + "http://collector.example:80/v1/traces", + ), + ( + "https://collector.example/v1/traces", + "https://collector.example:443/v1/traces", + ), + ( + "HTTP://COLLECTOR.EXAMPLE/v1/traces", + "http://collector.example/v1/traces", + ), + ( + "http://collector.example//v1///traces", + "http://collector.example/v1/traces/", + ), + ("http://localhost/v1/traces", "http://LOCALHOST/v1/traces"), + ("http://localhost/v1/traces", "http://localhost./v1/traces"), + ( + "http://localhost/v1/traces", + "http://agent.localhost/v1/traces", + ), + ("http://localhost/v1/traces", "http://127.0.0.2/v1/traces"), + ("http://localhost/v1/traces", "http://127.1/v1/traces"), + ("http://localhost/v1/traces", "http://[::1]/v1/traces"), + ] { + let config = plugin_config(json!({ + "opentelemetry": { + "enabled": true, + "endpoints": [ + {"type": "full", "endpoint": first}, + {"type": "gen_ai", "endpoint": second} + ] + } + })); + + let report = validate_plugin_config(&config); + assert!( + report.diagnostics.iter().any(|diagnostic| { + diagnostic.code == "observability.unsafe_otel_destination_collision" + }), + "expected equivalent destinations {first:?} and {second:?} to collide" + ); + } + + let grpc = plugin_config(json!({ + "opentelemetry": { + "enabled": true, + "endpoints": [ + { + "type": "full", + "transport": "grpc", + "endpoint": "https://collector.example" + }, + { + "type": "gen_ai", + "transport": "grpc", + "endpoint": "https://collector.example:443/" + } + ] + } + })); + assert!(validate_plugin_config(&grpc).has_errors()); +} + +#[test] +fn opentelemetry_allows_distinct_canonical_destinations() { + for (first, second) in [ + ( + "http://collector.example:4318/v1/traces", + "http://collector.example:4319/v1/traces", + ), + ( + "http://collector.example:443/v1/traces", + "https://collector.example/v1/traces", + ), + ( + "http://collector.example/v1/traces", + "http://collector.example/custom/traces", + ), + ( + "http://collector.example/v1/traces", + "http://collector.example/v1%2Ftraces", + ), + ( + "http://collector.example/v1/traces?tenant=one", + "http://collector.example/v1/traces?tenant=two", + ), + ( + "http://localhost.example/v1/traces", + "http://localhost/v1/traces", + ), + ("http://[::2]/v1/traces", "http://localhost/v1/traces"), + ] { + let config = plugin_config(json!({ + "opentelemetry": { + "enabled": true, + "endpoints": [ + {"type": "full", "endpoint": first}, + {"type": "gen_ai", "endpoint": second} + ] + } + })); + + assert!( + !validate_plugin_config(&config).has_errors(), + "expected distinct destinations {first:?} and {second:?} to remain valid" + ); + } + + let different_transports = plugin_config(json!({ + "opentelemetry": { + "enabled": true, + "endpoints": [ + { + "type": "full", + "transport": "http_binary", + "endpoint": "http://collector.example/v1/traces" + }, + { + "type": "gen_ai", + "transport": "grpc", + "endpoint": "http://collector.example/v1/traces" + } + ] + } + })); + assert!(!validate_plugin_config(&different_transports).has_errors()); +} + +#[test] +fn opentelemetry_rejects_canonical_collision_during_validation_and_activation() { let _guard = crate::observability::test_mutex().lock().unwrap(); reset_runtime(); let config = plugin_config(json!({ @@ -2740,8 +2873,8 @@ fn opentelemetry_rejects_different_projection_types_at_the_same_effective_destin "opentelemetry": { "enabled": true, "endpoints": [ - {"type": "full", "endpoint": " http://127.0.0.1:4318 "}, - {"type": "gen_ai", "endpoint": "http://127.0.0.1:4318/v1/traces"} + {"type": "full", "endpoint": " http://LOCALHOST:80//v1///traces/ "}, + {"type": "gen_ai", "endpoint": "http://127.1/v1/traces"} ] } })); @@ -2755,7 +2888,7 @@ fn opentelemetry_rejects_different_projection_types_at_the_same_effective_destin && diagnostic.message.contains("endpoints[1] (gen_ai)") && diagnostic .message - .contains("http://127.0.0.1:4318/v1/traces") + .contains("http://:80/v1/traces") })); assert!(futures::executor::block_on(initialize_plugins_exact(config)).is_err()); assert!( @@ -2773,8 +2906,8 @@ fn opentelemetry_allows_repeated_projection_types_at_the_same_destination() { "opentelemetry": { "enabled": true, "endpoints": [ - {"type": "full", "endpoint": "http://127.0.0.1:4318/v1/traces"}, - {"type": "full", "endpoint": "http://127.0.0.1:4318/v1/traces"} + {"type": "full", "endpoint": "http://LOCALHOST:80//v1///traces/"}, + {"type": "full", "endpoint": "http://127.1/v1/traces"} ] } })); diff --git a/docs/configure-plugins/observability/configuration.mdx b/docs/configure-plugins/observability/configuration.mdx index 2cf596424..88b8b61ca 100644 --- a/docs/configure-plugins/observability/configuration.mdx +++ b/docs/configure-plugins/observability/configuration.mdx @@ -71,8 +71,12 @@ endpoint and transport: Relay rejects that configuration because their deterministic trace and span IDs would collide at the receiver. For `http_binary`, this comparison uses the effective trace destination, so a bare URL and the same URL with `/v1/traces` also -collide. All endpoints are constructed before the plugin -registers its fan-out subscriber. +collide. The comparison realizes HTTP port `80` and HTTPS port `443`, collapses +repeated path slashes, ignores a non-root trailing slash, and treats standardized +loopback forms (`localhost`, names under `.localhost`, `127.0.0.0/8`, and `::1`) +as the same host without resolving DNS. Query strings remain part of the +destination. All endpoints are constructed before the plugin registers its +fan-out subscriber. ## Multi-Endpoint Lifecycle diff --git a/docs/configure-plugins/observability/opentelemetry.mdx b/docs/configure-plugins/observability/opentelemetry.mdx index 7b21893f2..e04e4c98b 100644 --- a/docs/configure-plugins/observability/opentelemetry.mdx +++ b/docs/configure-plugins/observability/opentelemetry.mdx @@ -31,7 +31,11 @@ compliant trace and span IDs from Relay lifecycle UUIDs, so endpoints that receive the same event stream use the same identifiers and parentage. Different endpoint types must therefore use independent OTLP destinations; configuring them with the same endpoint and transport is rejected to prevent identifier -collisions at the receiver. +collisions at the receiver. Duplicate detection compares canonical destinations: +HTTP and HTTPS default ports are realized, repeated and trailing path slashes +are normalized, and standardized loopback hosts such as `localhost`, names +under `.localhost`, `127.0.0.0/8`, and `::1` are equivalent. Relay does not use +DNS resolution for this comparison, and query strings remain significant. Rooted Relay propagation continues the Relay-derived trace across the import boundary. Rootless propagation retains Relay event parentage but starts a new OpenTelemetry trace from the first local event. Carry W3C `traceparent` and From f312b1049c36f46264fdd35ac6fd1fa30026f190 Mon Sep 17 00:00:00 2001 From: Will Killian <2007799+willkill07@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:26:54 -0400 Subject: [PATCH 3/9] fix: stamp CLI release version before packaging (#689) #### Overview Stamp the intended package version into every prebuilt CLI binary before compilation and prevent corrected 0.7.0 binaries from reusing gateways started by incorrectly identified RC artifacts. - [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. #### Details - Add a Cargo-only version recipe with SemVer build metadata support. - Derive one CLI package version in CI and reuse it for Cargo, wheel, npm, and binary verification. - Assert exact `nemo-relay ` output across native, manylinux, musllinux, macOS, and Windows packaging paths. - Increment the bootstrap protocol from 2 to 3 and reject protocol-2 gateways. - Update bootstrap and CLI compatibility fixtures for protocol 3. - No public API changes. Existing protocol-2 gateways are intentionally incompatible and must stop or idle-shutdown before replacement. Validation: - Cargo version recipe with `0.7.0-rc.6` and `0.7.0+deadbeef` - Exact prerelease CLI `--version` assertion - CLI packaging-script tests - Focused bootstrap compatibility test - All 98 CLI integration tests - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets -- -D warnings` - `uv run pre-commit run --all-files` - `just test-rust` passed the affected workspace and CLI suites; its final FFI unit target encountered an ambient `/Users/wkillian/.nemo-relay/plugins.toml`, causing one unexpected discovery warning and cascading poisoned-lock failures unrelated to this change. #### Where should the reviewer start? Start with `.github/workflows/ci_rust.yml` for the single-source version derivation and exact verification, then `crates/cli/src/bootstrap/mod.rs` and the bootstrap compatibility test for the protocol bump. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes RELAY-664 ## Summary by CodeRabbit * **New Features** * Added support for the updated bootstrap protocol, improving compatibility with current gateway connections. * Added a dedicated command to update the Cargo package version independently. * **Bug Fixes** * Improved version validation for SemVer build metadata and invalid leading-zero identifiers. * Added checks to ensure CLI binaries and packages report the expected version. * **Tests** * Expanded compatibility coverage for unsupported protocol and package-version combinations. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Maryam Najafian (https://github.com/mnajafian-nv) URL: https://github.com/NVIDIA/NeMo-Relay/pull/689 --- .github/workflows/ci_rust.yml | 53 ++++++++++++++++--- crates/cli/src/bootstrap/mod.rs | 2 +- crates/cli/tests/cli_tests.rs | 23 ++++---- .../tests/coverage/shared/bootstrap_tests.rs | 18 ++++--- justfile | 29 +++++++++- 5 files changed, 99 insertions(+), 26 deletions(-) diff --git a/.github/workflows/ci_rust.yml b/.github/workflows/ci_rust.yml index 85e0e43ff..5cdd1c361 100644 --- a/.github/workflows/ci_rust.yml +++ b/.github/workflows/ci_rust.yml @@ -254,6 +254,28 @@ jobs: cache-bin: false save-if: false + - uses: astral-sh/setup-uv@cec208311dfd045dd5311c1add060b2062131d57 # v8 + with: + version: ${{ steps.ci-config.outputs.uv_version }} + + - uses: taiki-e/install-action@c070f87102a1c75b3183910f391c1cb887fe13c8 # v2.77.6 + with: + tool: just@${{ steps.ci-config.outputs.just_version }} + + - name: Set CLI binary version + working-directory: ${{ env.NEMO_RELAY_CI_WORKSPACE }} + env: + REF_NAME: ${{ github.ref_name }} + REF_TYPE: ${{ github.ref_type }} + run: | + set -euo pipefail + version="$REF_NAME" + if [[ "$REF_TYPE" != "tag" ]]; then + version="$(sed -n 's/^version = "\(.*\)"$/\1/p' Cargo.toml | head -n1)+${GIT_COMMIT::8}" + fi + just set-cargo-version "$version" + printf 'NEMO_RELAY_CLI_PACKAGE_VERSION=%s\n' "$version" >> "$GITHUB_ENV" + - name: Install musl tools if: ${{ startsWith(matrix.target, 'x86_64-unknown-linux-musl') || startsWith(matrix.target, 'aarch64-unknown-linux-musl') }} run: | @@ -300,8 +322,17 @@ jobs: docker run --rm \ --volume "${{ env.NEMO_RELAY_CI_WORKSPACE }}:${{ env.NEMO_RELAY_CI_WORKSPACE }}" \ --workdir "${{ env.NEMO_RELAY_CI_WORKSPACE }}" \ + --env NEMO_RELAY_CI_WORKSPACE \ + --env NEMO_RELAY_CLI_PACKAGE_VERSION \ "${{ matrix.runtime_image }}" \ - "${{ env.NEMO_RELAY_CI_WORKSPACE }}/target/${{ matrix.target }}/release/nemo-relay" --version + /bin/sh -ec ' + actual="$("$NEMO_RELAY_CI_WORKSPACE/target/${{ matrix.target }}/release/nemo-relay" --version)" + expected="nemo-relay $NEMO_RELAY_CLI_PACKAGE_VERSION" + if [ "$actual" != "$expected" ]; then + echo "Error: expected CLI version \"$expected\", got \"$actual\"" >&2 + exit 1 + fi + ' - name: Stage CLI binary artifact working-directory: ${{ env.NEMO_RELAY_CI_WORKSPACE }} @@ -337,10 +368,7 @@ jobs: binary="${binary}.exe" fi source="${NEMO_RELAY_CI_WORKSPACE}/target/${target}/release/${binary}" - version="${{ github.ref_name }}" - if [ "${{ github.ref_type }}" != "tag" ]; then - version="$(sed -n 's/^version = "\(.*\)"$/\1/p' Cargo.toml | head -n1)+${GIT_COMMIT::8}" - fi + version="$NEMO_RELAY_CLI_PACKAGE_VERSION" rm -rf "${NEMO_RELAY_CI_WORKSPACE_TMP}/cli-packages" mkdir -p "${NEMO_RELAY_CI_WORKSPACE_TMP}/cli-packages" python scripts/package-cli-bin.py \ @@ -364,7 +392,12 @@ jobs: venv_cli="${NEMO_RELAY_CI_WORKSPACE_TMP}/cli-wheel-venv/bin/nemo-relay" fi "$venv_python" -m pip install --force-reinstall --no-deps "${wheels[0]}" - "$venv_cli" --version + actual="$("$venv_cli" --version)" + expected="nemo-relay $NEMO_RELAY_CLI_PACKAGE_VERSION" + if [[ "$actual" != "$expected" ]]; then + echo "Error: expected CLI version '$expected', got '$actual'" >&2 + exit 1 + fi - name: Install and run CLI wheel on musllinux if: ${{ endsWith(matrix.target, '-musl') }} @@ -375,13 +408,19 @@ jobs: --volume "${{ env.NEMO_RELAY_CI_WORKSPACE }}:${{ env.NEMO_RELAY_CI_WORKSPACE }}" \ --workdir "${{ env.NEMO_RELAY_CI_WORKSPACE }}" \ --env NEMO_RELAY_CI_WORKSPACE_TMP \ + --env NEMO_RELAY_CLI_PACKAGE_VERSION \ "${{ matrix.runtime_image }}" \ /bin/sh -ec ' wheel="$(find "$NEMO_RELAY_CI_WORKSPACE_TMP/cli-packages" -maxdepth 1 -name "*.whl" -print -quit)" test -n "$wheel" /opt/python/cp311-cp311/bin/python -m venv /tmp/nemo-relay-cli-wheel-venv /tmp/nemo-relay-cli-wheel-venv/bin/python -m pip install --force-reinstall --no-deps "$wheel" - /tmp/nemo-relay-cli-wheel-venv/bin/nemo-relay --version + actual="$(/tmp/nemo-relay-cli-wheel-venv/bin/nemo-relay --version)" + expected="nemo-relay $NEMO_RELAY_CLI_PACKAGE_VERSION" + if [ "$actual" != "$expected" ]; then + echo "Error: expected CLI version \"$expected\", got \"$actual\"" >&2 + exit 1 + fi ' - name: Upload CLI binary artifact diff --git a/crates/cli/src/bootstrap/mod.rs b/crates/cli/src/bootstrap/mod.rs index fbd9142d4..79e8aad02 100644 --- a/crates/cli/src/bootstrap/mod.rs +++ b/crates/cli/src/bootstrap/mod.rs @@ -38,7 +38,7 @@ use state::{BOOTSTRAP_STATE_DIR_ENV, state_dir as bootstrap_state_dir}; pub(crate) const DEFAULT_BIND: &str = "127.0.0.1:47632"; pub(crate) const DEFAULT_URL: &str = "http://127.0.0.1:47632"; pub(crate) const HEALTHZ_TIMEOUT: Duration = Duration::from_millis(500); -pub(crate) const BOOTSTRAP_PROTOCOL_VERSION: u64 = 2; +pub(crate) const BOOTSTRAP_PROTOCOL_VERSION: u64 = 3; pub(super) const BOOTSTRAP_LOCK_TIMEOUT: Duration = Duration::from_secs(20); const BOOTSTRAP_START_TIMEOUT: Duration = Duration::from_secs(10); diff --git a/crates/cli/tests/cli_tests.rs b/crates/cli/tests/cli_tests.rs index 4fce1eea1..6cc3e6bc3 100644 --- a/crates/cli/tests/cli_tests.rs +++ b/crates/cli/tests/cli_tests.rs @@ -24,6 +24,7 @@ fn gateway_bin() -> &'static str { } const ACTIVE_GENERATION_TOKEN: &str = "active-generation"; +const BOOTSTRAP_PROTOCOL_VERSION: u64 = 3; const SIDECAR_PUBLICATION_TIMEOUT: Duration = Duration::from_secs(30); fn write_active_generation(temp: &std::path::Path) -> std::path::PathBuf { @@ -639,8 +640,9 @@ fn cli_mcp_starts_gateway_even_when_stdio_closes_before_request() { fn cli_mcp_rejects_an_unauthenticated_transparent_gateway() { let temp = tempfile::tempdir().unwrap(); let body = format!( - r#"{{"status":"ok","service":"nemo-relay","version":"{}","bootstrap_protocol":2,"instance_id":"transparent"}}"#, - env!("CARGO_PKG_VERSION") + r#"{{"status":"ok","service":"nemo-relay","version":"{}","bootstrap_protocol":{},"instance_id":"transparent"}}"#, + env!("CARGO_PKG_VERSION"), + BOOTSTRAP_PROTOCOL_VERSION, ); let (gateway_url, received) = spawn_single_request_server(200, &body); let mut child = Command::new(gateway_bin()) @@ -1127,8 +1129,9 @@ fn write_fake_bootstrap_health( let nonce = bootstrap_request_header(request, "x-nemo-relay-bootstrap-nonce").unwrap(); let proof_header = fake_bootstrap_proof_header(proof, key_path, fingerprint, nonce); let body = format!( - r#"{{"status":"ok","service":"nemo-relay","version":"{}","bootstrap_protocol":2,"instance_id":"test-instance"}}"#, - env!("CARGO_PKG_VERSION") + r#"{{"status":"ok","service":"nemo-relay","version":"{}","bootstrap_protocol":{},"instance_id":"test-instance"}}"#, + env!("CARGO_PKG_VERSION"), + BOOTSTRAP_PROTOCOL_VERSION, ); stream .write_all( @@ -1171,8 +1174,9 @@ fn handle_fake_bootstrap_tunnel( let health = read_http_request(&mut stream); requests.lock().unwrap().push(health); let body = format!( - r#"{{"status":"ok","service":"nemo-relay","version":"{}","bootstrap_protocol":2,"instance_id":"test-instance"}}"#, - env!("CARGO_PKG_VERSION") + r#"{{"status":"ok","service":"nemo-relay","version":"{}","bootstrap_protocol":{},"instance_id":"test-instance"}}"#, + env!("CARGO_PKG_VERSION"), + BOOTSTRAP_PROTOCOL_VERSION, ); stream .write_all( @@ -1635,7 +1639,7 @@ fn cli_mcp_clients_share_gateway_until_final_idle_shutdown() { let health = relay_health(address); assert_eq!(health["service"], "nemo-relay"); assert_eq!(health["version"], env!("CARGO_PKG_VERSION")); - assert_eq!(health["bootstrap_protocol"], 2); + assert_eq!(health["bootstrap_protocol"], BOOTSTRAP_PROTOCOL_VERSION); assert!( health["instance_id"] .as_str() @@ -4500,8 +4504,9 @@ fn write_phase_health_response( .ok_or_else(|| "health probe omitted its nonce".to_string())?; let proof = fake_bootstrap_proof(key, fingerprint, nonce); let body = format!( - r#"{{"status":"ok","service":"nemo-relay","version":"{}","bootstrap_protocol":2,"instance_id":"phase-health"}}"#, - env!("CARGO_PKG_VERSION") + r#"{{"status":"ok","service":"nemo-relay","version":"{}","bootstrap_protocol":{},"instance_id":"phase-health"}}"#, + env!("CARGO_PKG_VERSION"), + BOOTSTRAP_PROTOCOL_VERSION, ); stream .write_all( diff --git a/crates/cli/tests/coverage/shared/bootstrap_tests.rs b/crates/cli/tests/coverage/shared/bootstrap_tests.rs index 24f8feaf4..15ab05263 100644 --- a/crates/cli/tests/coverage/shared/bootstrap_tests.rs +++ b/crates/cli/tests/coverage/shared/bootstrap_tests.rs @@ -88,17 +88,21 @@ fn compatible_gateway_is_reused_without_starting_another_process() { #[test] fn foreign_and_incompatible_listeners_are_never_adopted() { crate::test_support::enable_operational_logs(); + let incompatible = format!( + "{{\"status\":\"incompatible\",\"service\":\"nemo-relay\",\"version\":\"other\",\"bootstrap_protocol\":{BOOTSTRAP_PROTOCOL_VERSION},\"instance_id\":\"other\"}}" + ); + let previous_protocol = format!( + "{{\"status\":\"ok\",\"service\":\"nemo-relay\",\"version\":\"{}\",\"bootstrap_protocol\":{},\"instance_id\":\"previous\"}}", + env!("CARGO_PKG_VERSION"), + BOOTSTRAP_PROTOCOL_VERSION - 1, + ); for (status, body, expected) in [ - ("200 OK", "{}", "not a compatible"), - ( - "409 Conflict", - "{\"status\":\"incompatible\",\"service\":\"nemo-relay\",\"version\":\"other\",\"bootstrap_protocol\":2,\"instance_id\":\"other\"}", - "different version", - ), + ("200 OK", "{}".to_string(), "not a compatible"), + ("409 Conflict", incompatible, "different version"), + ("200 OK", previous_protocol, "not a compatible"), ] { let listener = TcpListener::bind("127.0.0.1:0").unwrap(); let address = listener.local_addr().unwrap(); - let body = body.to_string(); listener.set_nonblocking(true).unwrap(); let (stop_tx, stop_rx) = std::sync::mpsc::channel(); let server = std::thread::spawn(move || { diff --git a/justfile b/justfile index fd6f78737..4d331e8c3 100644 --- a/justfile +++ b/justfile @@ -453,8 +453,17 @@ import sys version = sys.argv[1] if version.startswith("v"): raise SystemExit("Release tags must not start with 'v'; use raw SemVer such as 0.1.0") -if not re.fullmatch(r"\d+\.\d+\.\d+(?:-(?:alpha|beta|rc)\.\d+)?", version): - raise SystemExit(f"Unsupported release tag '{version}'; use 0.1.0 or prereleases like 0.1.0-rc.1") +numeric_identifier = r"(?:0|[1-9][0-9]*)" +if not re.fullmatch( + rf"{numeric_identifier}\.{numeric_identifier}\.{numeric_identifier}" + rf"(?:-(?:alpha|beta|rc)\.{numeric_identifier})?" + r"(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?", + version, +): + raise SystemExit( + f"Unsupported Cargo version '{version}'; use 0.1.0, prereleases like " + "0.1.0-rc.1, or build metadata like 0.1.0+deadbeef" + ) path = Path("Cargo.toml") text = path.read_text() @@ -1484,6 +1493,22 @@ set-version version="": cd "$NEMO_RELAY_REPO_ROOT" set_project_version "$version" +# Set only the Cargo workspace version for release artifact builds. +# [version] or --set ref_name= +set-cargo-version version="": + #!/usr/bin/env bash + {{ bash_helpers }} + version="{{ version }}" + if [[ -z "$version" ]]; then + version="{{ ref_name }}" + fi + if [[ -z "$version" ]]; then + echo "Error: version is required for set-cargo-version" >&2 + exit 1 + fi + cd "$NEMO_RELAY_REPO_ROOT" + set_cargo_workspace_version "$version" + # --set [output_dir=] [ref_name=] package-rust: #!/usr/bin/env bash From 81dd3f03f7ce2904e4eedb84a6f5db87178c563b Mon Sep 17 00:00:00 2001 From: Bryan Bednarski Date: Tue, 4 Aug 2026 13:44:09 -0600 Subject: [PATCH 4/9] docs: deprecate Switchyard plugin (#700) #### Overview Deprecates the experimental `nemo-relay-switchyard` plugin ahead of its removal in NeMo Relay 0.8 and replacement by a Switchyard-owned native plugin. Clarifies that the existing integration is supported by NeMo Relay 0.6.0 and 0.7.0. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Mark the Switchyard docs landing page and navigation entry as deprecated. - Render the deprecation message as a native warning callout on the MDX documentation pages. - Add removal and replacement notices to the configuration guide, crate README, and example README. - Document support for the existing integration in NeMo Relay 0.6.0 and 0.7.0 across the landing page, architecture description, prerequisite table, configuration guide, crate README, and examples. - Tell readers that the NeMo Relay 0.8 documentation will provide updated configuration guidance and a migration plan when the replacement is available. - Keep the notice independent of any specific upstream pull request or provisional migration commands. Validation: - Targeted pre-commit checks passed for all six changed files, including YAML validation and docs link checking. - `git diff --check` passed. - The repository-wide pre-commit run completed all applicable formatting, lint, type, Cargo, Go, Node, attribution, and docs-link checks successfully. The generated Python worker-protocol check could not run because `just` is not installed. - `just docs` could not run because `just` is not installed in the environment. #### Where should the reviewer start? Start with `docs/configure-plugins/switchyard/about.mdx`, which contains the canonical deprecation warning and the supported-version language reused by the other entry points. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to: none ## Summary by CodeRabbit * **Documentation** * Marked the Switchyard integration as deprecated across guides, navigation, and examples. * Documented compatibility with NeMo Relay 0.6.0 and 0.7.0. * Added notice that the integration will be removed in Relay 0.8 and replaced by a native Switchyard plugin. * Updated setup guidance and example instructions for supported Relay versions. Authors: - Bryan Bednarski (https://github.com/bbednarski9) Approvers: - Will Killian (https://github.com/willkill07) URL: https://github.com/NVIDIA/NeMo-Relay/pull/700 --- crates/switchyard/README.md | 9 ++++++-- docs/configure-plugins/about.mdx | 7 +++--- docs/configure-plugins/switchyard/about.mdx | 22 +++++++++++++------ .../switchyard/configuration.mdx | 15 +++++++++---- docs/index.yml | 2 +- examples/switchyard/README.md | 15 +++++++++---- 6 files changed, 49 insertions(+), 21 deletions(-) diff --git a/crates/switchyard/README.md b/crates/switchyard/README.md index ba50b355f..0d8dece3b 100644 --- a/crates/switchyard/README.md +++ b/crates/switchyard/README.md @@ -9,14 +9,19 @@ SPDX-License-Identifier: Apache-2.0 # NeMo Relay Switchyard Plugin +> **Deprecated:** The experimental `nemo-relay-switchyard` plugin will be +> removed in NeMo Relay 0.8 and replaced by a Switchyard-owned native plugin. +> The NeMo Relay 0.8 documentation will include an updated configuration guide +> and migration plan when the replacement is available. + `nemo-relay-switchyard` is NeMo Relay's experimental integration with the [NVIDIA NeMo Switchyard](https://github.com/NVIDIA-NeMo/Switchyard) Decision API. It adds routing-aware LLM execution intercepts to the Relay runtime while preserving Relay ownership of provider credentials, target bindings, dispatch, retries, fallbacks, and observability. -NeMo Relay 0.6.0 uses a separately running Switchyard Decision API from the -`topic/nemo-relay-integration` branch. +NeMo Relay 0.6.0 and 0.7.0 use a separately running Switchyard Decision API +from the `topic/nemo-relay-integration` branch. Install it from crates.io, or build it from the NeMo Relay source checkout with the optional CLI feature while the Switchyard Decision API contract and diff --git a/docs/configure-plugins/about.mdx b/docs/configure-plugins/about.mdx index b96315a2f..6e1de0772 100644 --- a/docs/configure-plugins/about.mdx +++ b/docs/configure-plugins/about.mdx @@ -33,9 +33,10 @@ entries in `plugins.toml`. Guardrails-backed policy checks. - [PII Redaction](/configure-plugins/pii-redaction/about) sanitizes sensitive data in observability payloads. -- [Switchyard (Experimental)](/configure-plugins/switchyard/about) routes - requests using decisions from a separately running Switchyard Decision API - service. +- [Switchyard (Deprecated)](/configure-plugins/switchyard/about) routes requests + using decisions from a separately running Switchyard Decision API service. + The experimental plugin will be removed in NeMo Relay 0.8 and replaced by a + Switchyard-owned native plugin. - [Model Pricing](/configure-plugins/model-pricing) configures catalog sources for cost estimates on managed LLM responses. diff --git a/docs/configure-plugins/switchyard/about.mdx b/docs/configure-plugins/switchyard/about.mdx index 321a4d43d..b6806b1a2 100644 --- a/docs/configure-plugins/switchyard/about.mdx +++ b/docs/configure-plugins/switchyard/about.mdx @@ -1,7 +1,7 @@ --- -title: "Switchyard (Experimental)" -sidebar-title: "Switchyard (Experimental)" -description: "Set up and validate the experimental Switchyard Decision API integration for NeMo Relay 0.6.0." +title: "Switchyard (Deprecated)" +sidebar-title: "Switchyard (Deprecated)" +description: "Set up and validate the deprecated Switchyard Decision API integration for NeMo Relay 0.6.0 and 0.7.0." position: 5 --- import { MermaidStyles } from "@/components/MermaidStyles"; @@ -9,13 +9,21 @@ import { MermaidStyles } from "@/components/MermaidStyles"; {/* SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. SPDX-License-Identifier: Apache-2.0 */} + +The experimental `nemo-relay-switchyard` plugin will be removed in NeMo Relay +0.8 and replaced by a Switchyard-owned native plugin. Continue using this page +for the existing integration. The NeMo Relay 0.8 documentation will include an +updated configuration guide and migration plan when the replacement is +available. + + > **Experimental:** The Switchyard integration is an early-access feature. It > is not enabled in default Relay builds, its configuration and contracts can > change, and the current deployment requires a separately running Switchyard > Decision API service. -> **NeMo Relay 0.6.0 architecture:** This release uses the external Switchyard -> Decision API from the +> **NeMo Relay 0.6.0 and 0.7.0 architecture:** These releases use the external +> Switchyard Decision API from the > [`topic/nemo-relay-integration`](https://github.com/NVIDIA-NeMo/Switchyard/tree/topic/nemo-relay-integration) > branch. @@ -27,7 +35,7 @@ and a complex prompt to a more capable model. ## Architecture -The following diagram shows the NeMo Relay 0.6.0 service boundary: +The following diagram shows the NeMo Relay 0.6.0 and 0.7.0 service boundary: @@ -65,7 +73,7 @@ Install the following prerequisites before you start: | Requirement | Version or Value | | --- | --- | -| NeMo Relay | Tag `0.6.0` | +| NeMo Relay | Tags `0.6.0` and `0.7.0` | | Rust | `1.96.1` | | Switchyard branch | `topic/nemo-relay-integration` | | Switchyard commit | `8f9db9a6a47f848cdff1d262276ba25a8ae9cbc8` | diff --git a/docs/configure-plugins/switchyard/configuration.mdx b/docs/configure-plugins/switchyard/configuration.mdx index 2b09b555e..0c406af58 100644 --- a/docs/configure-plugins/switchyard/configuration.mdx +++ b/docs/configure-plugins/switchyard/configuration.mdx @@ -1,17 +1,24 @@ --- title: "Switchyard Configuration" sidebar-title: "Configuration" -description: "Configure the experimental Relay-native Switchyard Decision API plugin." +description: "Configure the deprecated Relay-native Switchyard Decision API plugin." position: 2 --- {/* SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. SPDX-License-Identifier: Apache-2.0 */} -> **Experimental NeMo Relay 0.6.0 integration:** This release calls a separately -> running Switchyard Decision API. + +The experimental `nemo-relay-switchyard` plugin will be removed in NeMo Relay +0.8 and replaced by a Switchyard-owned native plugin. The NeMo Relay 0.8 +documentation will include an updated configuration guide and migration plan +when the replacement is available. + + +> **Experimental NeMo Relay 0.6.0 and 0.7.0 integration:** These releases call a +> separately running Switchyard Decision API. Start with the -[Switchyard 0.6.0 setup and validation guide](./about.mdx) +[Switchyard setup and validation guide](./about.mdx) before using this page as the complete configuration reference. The Switchyard component connects Relay's CLI gateway to a separately running diff --git a/docs/index.yml b/docs/index.yml index fca80db2d..ff4dc3a70 100644 --- a/docs/index.yml +++ b/docs/index.yml @@ -41,7 +41,7 @@ navigation: title: "PII Redaction" title-source: frontmatter - folder: ./configure-plugins/switchyard - title: "Switchyard (Experimental)" + title: "Switchyard (Deprecated)" title-source: frontmatter - page: "Model Pricing" path: ./configure-plugins/model-pricing.mdx diff --git a/examples/switchyard/README.md b/examples/switchyard/README.md index 87a2cff68..b5fedb23e 100644 --- a/examples/switchyard/README.md +++ b/examples/switchyard/README.md @@ -5,9 +5,16 @@ SPDX-License-Identifier: Apache-2.0 # Switchyard Integration Examples -These examples exercise the experimental NeMo Relay 0.6.0 integration with a separately running -Switchyard Decision API service and the in-process Switchyard translation library. They are -manual, local validation workflows rather than production startup orchestration. +> **Deprecated:** These examples exercise the experimental +> `nemo-relay-switchyard` plugin, which will be removed in NeMo Relay 0.8 and +> replaced by a Switchyard-owned native plugin. The NeMo Relay 0.8 documentation +> will include updated examples and a migration plan when the replacement is +> available. + +These examples exercise the experimental NeMo Relay 0.6.0 and 0.7.0 integration with a +separately running Switchyard Decision API service and the in-process Switchyard translation +library. They are manual, local validation workflows rather than production startup +orchestration. For the canonical architecture, setup, configuration, validation, and troubleshooting workflow, refer to the @@ -15,7 +22,7 @@ refer to the ## Required Switchyard Revision -The NeMo Relay 0.6.0 scripts require the following public topic branch and commit: +The scripts for NeMo Relay 0.6.0 and 0.7.0 require the following public topic branch and commit: ```text https://github.com/NVIDIA-NeMo/Switchyard/tree/topic/nemo-relay-integration From dc779abc59b6d1f3eaadf1fbe3f9f98a7d721984 Mon Sep 17 00:00:00 2001 From: Will Killian <2007799+willkill07@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:45:19 -0400 Subject: [PATCH 5/9] fix: fail closed generated enforcement hooks (#692) #### Overview Make generated coding-agent enforcement hooks fail closed while keeping observational and after-the-fact hooks explicitly fail open. - [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. #### Details - Add mutually exclusive `hook-forward --fail-open` and `--fail-closed` modes while preserving the existing unqualified command and `NEMO_RELAY_FAIL_CLOSED=1` behavior. - Generate event-specific POSIX and PowerShell commands for transparent runs, persistent installs, and packaged Claude Code and Codex hooks. - Fail closed for Claude/Codex `PreToolUse` and `PermissionRequest` plus Hermes `pre_tool_call`; explicitly fail open for all other generated events. - Update Codex and Hermes hook validation, trust, uninstall, and migration handling to recognize both policy-specific commands and legacy single-command installations without removing user-owned entries. - Add regression coverage for policy precedence, conflicting flags, generated event mappings, Windows envelopes, packaged hooks, trust state, and legacy migration. - Document the policy split and required reinstall in the CLI guide, coding-agent integration guide, and v0.7 release notes. Validation: - `uv run pre-commit run --all-files` - `just docs` - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets -- -D warnings` - Workspace and CLI portions of `just test-rust`, including all 100 CLI integration tests - Complete 88-test FFI binary from an isolated working directory. The normal Cargo FFI invocation inherited `/Users/wkillian/.nemo-relay/plugins.toml` and violated an existing empty-diagnostics test assumption. This change does not introduce a breaking API change. Existing unqualified manual `hook-forward` commands retain their current compatibility behavior. #### Where should the reviewer start? Start with `crates/cli/src/hooks/encoding.rs`, which defines the per-event policy mapping and generates the paired commands. Then review `crates/cli/src/agents/codex/host.rs` and `crates/cli/src/agents/hermes/integration.rs` for trust and legacy migration behavior. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes RELAY-645 ## Summary by CodeRabbit * **New Features** * Hook forwarding now supports explicit fail-open and fail-closed policies. * Permission-related hooks fail closed, while lifecycle and reporting hooks fail open. * Hook behavior is selected per event across supported coding-agent integrations. * Added options to override the default failure policy. * **Bug Fixes** * Improved hook installation, validation, trust reporting, and cleanup for event-specific policies. * **Documentation** * Added upgrade guidance, policy details, and instructions for reinstalling generated hooks. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Maryam Najafian (https://github.com/mnajafian-nv) URL: https://github.com/NVIDIA/NeMo-Relay/pull/692 --- crates/cli/src/agents/claude/launch.rs | 6 +- crates/cli/src/agents/codex/host.rs | 77 ++++--- crates/cli/src/agents/codex/launch.rs | 6 +- crates/cli/src/agents/hermes/config.rs | 45 ++-- crates/cli/src/agents/hermes/integration.rs | 85 ++++--- crates/cli/src/agents/hermes/trust.rs | 28 ++- crates/cli/src/agents/mod.rs | 4 +- crates/cli/src/commands/hook_forward.rs | 13 +- crates/cli/src/hooks/delivery.rs | 3 +- crates/cli/src/hooks/encoding.rs | 127 +++++++++-- crates/cli/src/hooks/mod.rs | 11 +- crates/cli/src/hooks/types.rs | 19 +- crates/cli/tests/cli_tests.rs | 70 +++++- .../cli/tests/coverage/agents/hermes_tests.rs | 213 +++++++++++++----- .../coverage/agents/plugin_host_tests.rs | 89 ++++++-- .../coverage/agents/plugin_install_tests.rs | 9 +- .../tests/coverage/shared/installer_tests.rs | 108 ++++++--- .../cli/tests/fixtures/windows_hook_relay.rs | 1 + docs/about-nemo-relay/release-notes/index.mdx | 4 + docs/nemo-relay-cli/basic-usage.mdx | 20 +- integrations/coding-agents/README.md | 20 +- .../claude-code/hooks/hooks.json | 28 +-- .../coding-agents/codex/hooks/hooks.json | 20 +- 23 files changed, 726 insertions(+), 280 deletions(-) diff --git a/crates/cli/src/agents/claude/launch.rs b/crates/cli/src/agents/claude/launch.rs index 4147c7ccb..89ab79e32 100644 --- a/crates/cli/src/agents/claude/launch.rs +++ b/crates/cli/src/agents/claude/launch.rs @@ -7,7 +7,7 @@ use serde_json::{Value, json}; use crate::agents::CodingAgent; use crate::error::CliError; -use crate::hooks::{generated_hooks, transparent_hook_forward_command}; +use crate::hooks::{generated_policy_hooks, transparent_hook_forward_commands}; use crate::process::{PreparedAgentLaunch, insert_after_host}; pub(crate) fn prepare( @@ -61,7 +61,7 @@ pub(crate) fn prepare( })) .map_err(|error| CliError::Launch(error.to_string()))?, )?; - let hook_command = transparent_hook_forward_command( + let hook_commands = transparent_hook_forward_commands( &transparent_hook_executable(), CodingAgent::ClaudeCode, gateway_url, @@ -69,7 +69,7 @@ pub(crate) fn prepare( .map_err(CliError::Launch)?; write_hooks( &root.join("hooks/hooks.json"), - generated_hooks(CodingAgent::ClaudeCode, &hook_command), + generated_policy_hooks(CodingAgent::ClaudeCode, &hook_commands), )?; let settings_path = root.join("settings.json"); let settings = settings_overlay(&launch.argv, launch.host_index, gateway_url)?; diff --git a/crates/cli/src/agents/codex/host.rs b/crates/cli/src/agents/codex/host.rs index 185121ffb..ffaeb4b51 100644 --- a/crates/cli/src/agents/codex/host.rs +++ b/crates/cli/src/agents/codex/host.rs @@ -14,6 +14,7 @@ use toml_edit::{DocumentMut, InlineTable, Item, Table, Value as TomlValue, value use crate::agents::CodingAgent; use crate::configuration::{BOOTSTRAP_CLIENT_TOKEN_HEADER, BootstrapChallengeKey, RELAY_PLUGIN_ID}; +#[cfg(test)] use crate::hooks::generated_hooks; #[cfg(test)] use crate::hooks::merge_hooks; @@ -109,11 +110,11 @@ pub(crate) fn install_codex_with_generation( pub(crate) fn install_codex_with_trust( gateway_url: &str, - expected_command: &str, + expected_commands: &crate::hooks::GeneratedHookCommands, trust_hooks: F, ) -> Result where - F: FnOnce(&Path, &Path, &str) -> Result<(), String>, + F: FnOnce(&Path, &Path, &crate::hooks::GeneratedHookCommands) -> Result<(), String>, { let home = home_dir()?; let codex_dir = codex_home_dir()?; @@ -125,7 +126,7 @@ where let snapshots = codex_install_snapshots(&config_path, &hooks_path)?; let install_result = remove_legacy_codex_hooks(&hooks_path) .and_then(|()| install_codex_config(&config_path, gateway_url)) - .and_then(|()| trust_hooks(&home, &config_path, expected_command)); + .and_then(|()| trust_hooks(&home, &config_path, expected_commands)); if let Err(error) = install_result { return match restore_codex_install_snapshots(&snapshots) { Ok(()) => Err(error), @@ -257,9 +258,9 @@ pub(crate) fn codex_hook_trust_report_with_generation( pub(crate) fn codex_hook_trust_report_with_client( client: &mut dyn CodexHooksClient, cwd: &Path, - expected_command: &str, + expected_commands: &crate::hooks::GeneratedHookCommands, ) -> Result { - let hooks = relay_codex_hooks(client, cwd, expected_command)?; + let hooks = relay_codex_hooks(client, cwd, expected_commands)?; Ok(codex_hook_trust_report_for(&hooks)) } @@ -267,9 +268,9 @@ pub(crate) fn auto_trust_codex_hooks( client: &mut dyn CodexHooksClient, cwd: &Path, config_path: &Path, - expected_command: &str, + expected_commands: &crate::hooks::GeneratedHookCommands, ) -> Result<(), String> { - let hooks = relay_codex_hooks(client, cwd, expected_command)?; + let hooks = relay_codex_hooks(client, cwd, expected_commands)?; let before = codex_hook_trust_report_for(&hooks); if !before.missing_required.is_empty() || !before.duplicate_required.is_empty() { return Err(format!( @@ -280,7 +281,7 @@ pub(crate) fn auto_trust_codex_hooks( } let state = snapshot_hook_trust_state(config_path, &hooks)?; let trust_result = client.trust_hooks(&hooks).and_then(|()| { - let verified_hooks = relay_codex_hooks(client, cwd, expected_command)?; + let verified_hooks = relay_codex_hooks(client, cwd, expected_commands)?; let verified = codex_hook_trust_report_for(&verified_hooks); let unverified_targets = hooks .iter() @@ -308,7 +309,7 @@ pub(crate) fn auto_trust_codex_hooks( return restore_hook_trust_after_failure( client, cwd, - expected_command, + expected_commands, &hooks, &state, error, @@ -320,21 +321,28 @@ pub(crate) fn auto_trust_codex_hooks( fn relay_codex_hooks( client: &mut dyn CodexHooksClient, cwd: &Path, - expected_command: &str, + expected_commands: &crate::hooks::GeneratedHookCommands, ) -> Result, String> { let hooks = relay_codex_plugin_hooks(client, cwd)? .into_iter() - .filter(|hook| hook.command.as_deref() == Some(expected_command)) + .filter(|hook| { + expected_codex_hook_command(expected_commands, &hook.event_name).is_some_and( + |expected| { + hook.command.as_deref() == Some(expected) + || hook.command.as_deref() == expected_commands.legacy() + }, + ) + }) .collect::>(); - validate_loaded_hook_sources(&hooks, expected_command)?; + validate_loaded_hook_sources(&hooks, expected_commands)?; Ok(hooks) } fn validate_loaded_hook_sources( hooks: &[CodexHookMetadata], - expected_command: &str, + expected_commands: &crate::hooks::GeneratedHookCommands, ) -> Result<(), String> { - let expected = generated_hooks(CodingAgent::Codex, expected_command); + let expected = crate::hooks::generated_policy_hooks(CodingAgent::Codex, expected_commands); let sources = hooks .iter() .map(|hook| hook.source_path.as_str()) @@ -538,7 +546,7 @@ fn verify_restored_hook_trust( fn restore_hook_trust_after_failure( client: &mut dyn CodexHooksClient, cwd: &Path, - expected_command: &str, + expected_commands: &crate::hooks::GeneratedHookCommands, before: &[CodexHookMetadata], state: &[(String, Option)], original_error: String, @@ -548,7 +556,7 @@ fn restore_hook_trust_after_failure( "{original_error}; additionally failed to restore Codex hook trust: {rollback_error}" )); } - let restored = relay_codex_hooks(client, cwd, expected_command).map_err(|rollback_error| { + let restored = relay_codex_hooks(client, cwd, expected_commands).map_err(|rollback_error| { format!( "{original_error}; additionally failed to verify restored Codex hook trust: {rollback_error}" ) @@ -569,14 +577,16 @@ fn restore_hook_trust_after_failure( } #[cfg(test)] -pub(crate) fn expected_plugin_hook_command(plugin_hooks_path: &Path) -> Result { +pub(crate) fn expected_plugin_hook_command( + plugin_hooks_path: &Path, +) -> Result { expected_plugin_hook_command_with_token(plugin_hooks_path, None) } fn expected_plugin_hook_command_with_token( plugin_hooks_path: &Path, generation_token: Option<&str>, -) -> Result { +) -> Result { let relay = current_exe()?; let relay = relay.canonicalize().unwrap_or(relay); let relay = portable_executable_path(relay); @@ -614,9 +624,12 @@ fn plugin_generation_file(plugin_hooks_path: &Path) -> Result { } } -fn validate_plugin_hooks(path: &Path, expected_command: &str) -> Result<(), String> { +fn validate_plugin_hooks( + path: &Path, + expected_commands: &crate::hooks::GeneratedHookCommands, +) -> Result<(), String> { let actual = read_json_object(path)?; - let expected = generated_hooks(CodingAgent::Codex, expected_command); + let expected = crate::hooks::generated_policy_hooks(CodingAgent::Codex, expected_commands); if actual == expected { Ok(()) } else { @@ -674,7 +687,7 @@ fn is_generated_codex_hook_event(event: &str) -> bool { .any(|expected| normalize_hook_event(expected) == normalized) } -fn normalize_hook_event(event: &str) -> String { +pub(crate) fn normalize_hook_event(event: &str) -> String { event .chars() .filter(|character| character.is_ascii_alphanumeric()) @@ -682,6 +695,18 @@ fn normalize_hook_event(event: &str) -> String { .collect() } +pub(crate) fn expected_codex_hook_command<'a>( + commands: &'a crate::hooks::GeneratedHookCommands, + event: &str, +) -> Option<&'a str> { + let normalized = normalize_hook_event(event); + CodingAgent::Codex + .hook_events() + .iter() + .find(|expected| normalize_hook_event(expected) == normalized) + .map(|event| commands.for_event(event)) +} + fn codex_install_snapshots( config_path: &Path, hooks_path: &Path, @@ -1635,7 +1660,7 @@ pub(crate) fn codex_hooks_installed_with_generation( generation_token: Option<&str>, ) -> Result { let value = read_json_object(path)?; - let generated = generated_hooks( + let generated = crate::hooks::generated_policy_hooks( CodingAgent::Codex, &expected_plugin_hook_command_with_token(path, generation_token)?, ); @@ -1660,8 +1685,8 @@ pub(crate) fn codex_plugin_hook_command( relay: &Path, generation: &Path, generation_token: &str, -) -> Result { - crate::hooks::persistent_hook_forward_command( +) -> Result { + crate::hooks::persistent_hook_forward_commands( relay, CodingAgent::Codex, generation, @@ -1675,8 +1700,8 @@ pub(crate) fn codex_plugin_hook_command_for_platform( generation: &Path, generation_token: &str, windows: bool, -) -> String { - crate::hooks::persistent_hook_forward_command_for_platform( +) -> crate::hooks::GeneratedHookCommands { + crate::hooks::persistent_hook_forward_commands_for_platform( relay, CodingAgent::Codex, generation, diff --git a/crates/cli/src/agents/codex/launch.rs b/crates/cli/src/agents/codex/launch.rs index ad20c5cc0..41cfeafe6 100644 --- a/crates/cli/src/agents/codex/launch.rs +++ b/crates/cli/src/agents/codex/launch.rs @@ -8,7 +8,7 @@ use serde_json::Value; use crate::agents::CodingAgent; use crate::configuration::{RELAY_PLUGIN_ID, RELAY_SOURCE_PLUGIN_ID}; use crate::error::CliError; -use crate::hooks::{generated_hooks, transparent_hook_forward_command}; +use crate::hooks::{generated_policy_hooks, transparent_hook_forward_commands}; use crate::process::{PreparedAgentLaunch, insert_after_host}; pub(crate) fn prepare(launch: &mut PreparedAgentLaunch, gateway_url: &str) -> Result<(), CliError> { @@ -26,13 +26,13 @@ pub(crate) fn prepare(launch: &mut PreparedAgentLaunch, gateway_url: &str) -> Re or pass `--openai-base-url` to an upstream that needs no key." ); } - let hook_command = transparent_hook_forward_command( + let hook_commands = transparent_hook_forward_commands( &transparent_hook_executable(), CodingAgent::Codex, gateway_url, ) .map_err(CliError::Launch)?; - let hook_groups = generated_hooks(CodingAgent::Codex, &hook_command); + let hook_groups = generated_policy_hooks(CodingAgent::Codex, &hook_commands); let mut args = vec![ "--config".to_string(), "features.hooks=true".to_string(), diff --git a/crates/cli/src/agents/hermes/config.rs b/crates/cli/src/agents/hermes/config.rs index 95d305fb3..06c3d413b 100644 --- a/crates/cli/src/agents/hermes/config.rs +++ b/crates/cli/src/agents/hermes/config.rs @@ -8,7 +8,7 @@ use std::path::{Path, PathBuf}; use serde_json::{Map, Value, json}; use crate::error::CliError; -use crate::hooks::{generated_hooks, merge_hooks}; +use crate::hooks::{GeneratedHookCommands, generated_policy_hooks, merge_hooks}; pub(super) use crate::mcp::SERVER_NAME as MCP_SERVER_NAME; @@ -32,9 +32,9 @@ pub(crate) fn transparent_config( ) -> Result { let mut root = parse_yaml_object(Some(existing), "Hermes config")?; let owned = owned_install_command(&root, relay, None)?; - strip_owned_hooks(&mut root, owned.as_deref())?; + strip_owned_hooks(&mut root, owned.as_ref())?; remove_owned_mcp(&mut root, owned.is_some())?; - let command = crate::hooks::transparent_hook_forward_command( + let commands = crate::hooks::transparent_hook_forward_commands( relay, crate::agents::CodingAgent::Hermes, gateway_url, @@ -42,7 +42,7 @@ pub(crate) fn transparent_config( .map_err(CliError::Install)?; let mut root = merge_hooks( root, - generated_hooks(crate::agents::CodingAgent::Hermes, &command), + generated_policy_hooks(crate::agents::CodingAgent::Hermes, &commands), )?; let object = root .as_object_mut() @@ -75,12 +75,12 @@ pub(crate) fn transparent_config( serde_yaml::to_string(&root).map_err(|error| CliError::Install(error.to_string())) } -pub(crate) fn persistent_hook_command( +pub(crate) fn persistent_hook_commands( relay: &Path, generation: &Path, generation_token: &str, -) -> Result { - crate::hooks::persistent_hook_forward_command( +) -> Result { + crate::hooks::persistent_hook_forward_commands( relay, crate::agents::CodingAgent::Hermes, generation, @@ -89,13 +89,13 @@ pub(crate) fn persistent_hook_command( } #[cfg(test)] -pub(super) fn persistent_hook_command_for_platform( +pub(super) fn persistent_hook_commands_for_platform( relay: &Path, generation: &Path, generation_token: &str, windows: bool, -) -> String { - crate::hooks::persistent_hook_forward_command_for_platform( +) -> GeneratedHookCommands { + crate::hooks::persistent_hook_forward_commands_for_platform( relay, crate::agents::CodingAgent::Hermes, generation, @@ -107,7 +107,7 @@ pub(super) fn persistent_hook_command_for_platform( pub(super) fn persistent_config( existing: Option<&str>, relay: &Path, - command: &str, + commands: &GeneratedHookCommands, generation: &Path, generation_token: &str, environment: &[String], @@ -123,10 +123,10 @@ pub(super) fn persistent_config( "Hermes MCP server `{MCP_SERVER_NAME}` already exists and is not managed by Relay; rename or remove it before installing the Relay integration" ))); } - strip_owned_hooks(&mut root, owned.as_deref())?; + strip_owned_hooks(&mut root, owned.as_ref())?; root = merge_hooks( root, - generated_hooks(crate::agents::CodingAgent::Hermes, command), + generated_policy_hooks(crate::agents::CodingAgent::Hermes, commands), )?; let servers = object_field_mut(&mut root, "mcp_servers", "mcp_servers")?; servers.insert( @@ -162,7 +162,7 @@ pub(super) fn forwarded_environment_names( pub(super) fn strip_owned_hooks( root: &mut Value, - owned_command: Option<&str>, + owned_commands: Option<&GeneratedHookCommands>, ) -> Result<(), CliError> { let Some(hooks) = root.get_mut("hooks") else { return Ok(()); @@ -180,7 +180,9 @@ pub(super) fn strip_owned_hooks( group .get("command") .and_then(Value::as_str) - .is_none_or(|command| Some(command) != owned_command) + .is_none_or(|command| { + owned_commands.is_none_or(|commands| !commands.contains(command)) + }) }); if groups.is_empty() { empty.push(event.clone()); @@ -221,7 +223,7 @@ pub(super) fn owned_install_command( root: &Value, relay: &Path, expected_generation: Option<&Path>, -) -> Result, CliError> { +) -> Result, CliError> { let Some(server) = root.pointer(&format!("/mcp_servers/{MCP_SERVER_NAME}")) else { return Ok(None); }; @@ -243,15 +245,18 @@ pub(super) fn owned_install_command( && !token.is_empty() && expected_generation.is_none_or(|expected| Path::new(generation) == expected) { - let command = persistent_hook_command(relay, Path::new(generation), token) + let commands = persistent_hook_commands(relay, Path::new(generation), token) .map_err(CliError::Install)?; - return Ok(Some(command)); + return Ok(Some(commands)); } } legacy_owned_command(root, relay) } -fn legacy_owned_command(root: &Value, relay: &Path) -> Result, CliError> { +fn legacy_owned_command( + root: &Value, + relay: &Path, +) -> Result, CliError> { let server = &root["mcp_servers"][MCP_SERVER_NAME]; if server.get("args") != Some(&json!(["mcp", "--agent", "hermes"])) { return Ok(None); @@ -274,7 +279,7 @@ fn legacy_owned_command(root: &Value, relay: &Path) -> Result, Cl } common = Some(commands[0]); } - Ok(common.map(str::to_owned)) + Ok(common.map(GeneratedHookCommands::uniform)) } fn legacy_command_uses_relay(command: &str, relay: &Path) -> bool { diff --git a/crates/cli/src/agents/hermes/integration.rs b/crates/cli/src/agents/hermes/integration.rs index f14457a3f..e1ea8c2b7 100644 --- a/crates/cli/src/agents/hermes/integration.rs +++ b/crates/cli/src/agents/hermes/integration.rs @@ -11,13 +11,13 @@ use std::time::SystemTime; use serde_json::{Map, Value, json}; #[cfg(test)] -use super::config::persistent_hook_command_for_platform; +use super::config::persistent_hook_commands_for_platform; use super::config::{ MCP_SERVER_NAME, expected_mcp_server, forwarded_environment_names, owned_install_command, parse_yaml_object, persistent_config, relay_is_executable, remove_owned_mcp, strip_owned_hooks, user_config_path_with_override, yaml_bytes, }; -pub(crate) use super::config::{persistent_hook_command, transparent_config}; +pub(crate) use super::config::{persistent_hook_commands, transparent_config}; use super::files::{ FileSnapshot, INSTALL_LOCK_TIMEOUT, PersistentPaths, acquire_allowlist_lock, acquire_install_lock, read_optional_utf8, remove_optional_file, replace_optional_file, @@ -27,6 +27,7 @@ use crate::agents::CodingAgent; use crate::bootstrap::DEFAULT_BIND; use crate::error::CliError; use crate::filesystem::atomic_write; +use crate::hooks::GeneratedHookCommands; #[cfg(test)] use crate::installation::generation::GENERATION_FILE_NAME; use crate::installation::generation::{ @@ -166,7 +167,10 @@ fn config_has_managed_state(config: &Value) -> bool { owned_command_from_config(config, None).is_some() } -fn allowlist_has_owned_command(allowlist: &Value, command: Option<&str>) -> bool { +fn allowlist_has_owned_command( + allowlist: &Value, + commands: Option<&GeneratedHookCommands>, +) -> bool { allowlist .get("approvals") .and_then(Value::as_array) @@ -174,16 +178,23 @@ fn allowlist_has_owned_command(allowlist: &Value, command: Option<&str>) -> bool .flatten() .filter_map(|entry| entry.get("command").and_then(Value::as_str)) .any(|candidate| { - command == Some(candidate) - || (command.is_none() && is_persistent_relay_hook_command(candidate)) + commands.is_some_and(|commands| commands.contains(candidate)) + || (commands.is_none() && is_persistent_relay_hook_command(candidate)) }) } fn is_persistent_relay_hook_command(command: &str) -> bool { #[cfg(any(windows, test))] if let Some(arguments) = crate::hooks::decode_windows_hook_command(command) { + let arguments = arguments.as_slice(); + let base = match arguments { + [base @ .., policy] if matches!(policy.as_str(), "--fail-open" | "--fail-closed") => { + base + } + base => base, + }; return matches!( - arguments.as_slice(), + base, [ _, hook_forward, @@ -211,7 +222,10 @@ fn is_persistent_relay_hook_command(command: &str) -> bool { && command.contains("--generation-token") } -fn owned_command_from_config(config: &Value, generation: Option<&Path>) -> Option { +fn owned_command_from_config( + config: &Value, + generation: Option<&Path>, +) -> Option { let relay = config .pointer(&format!("/mcp_servers/{MCP_SERVER_NAME}/command")) .and_then(Value::as_str) @@ -235,9 +249,9 @@ pub(crate) fn diagnose_persistent(config_path: &Path) -> Result )); } let generation = InstallGeneration::capture(paths.generation.clone())?; - let command = persistent_hook_command(&relay, &paths.generation, generation.token())?; - verify_hook_definitions(&config, &command)?; - verify_trust(&paths.allowlist, &command)?; + let commands = persistent_hook_commands(&relay, &paths.generation, generation.token())?; + verify_hook_definitions(&config, &commands)?; + verify_trust(&paths.allowlist, &commands)?; let mcp_env = config["mcp_servers"][MCP_SERVER_NAME] .get("env") @@ -374,20 +388,20 @@ where }; let environment = forwarded_environment_names(environment, plugin_config); let token = uuid::Uuid::now_v7().to_string(); - let command = - persistent_hook_command(relay, &paths.generation, &token).map_err(CliError::Install)?; + let commands = + persistent_hook_commands(relay, &paths.generation, &token).map_err(CliError::Install)?; let config = persistent_config( existing_config.as_deref(), relay, - &command, + &commands, &paths.generation, &token, &environment, )?; let allowlist = trusted_hooks( existing_allowlist.as_deref(), - previous_command.as_deref(), - &command, + previous_command.as_ref(), + &commands, relay, now, )?; @@ -404,7 +418,7 @@ where verify_install( &paths, relay, - &command, + &commands, &environment, &token, generation_transaction, @@ -437,7 +451,7 @@ where .map(|raw| { let mut root = parse_yaml_object(Some(&raw), "Hermes config")?; let owned = owned_command_from_config(&root, Some(&paths.generation)); - strip_owned_hooks(&mut root, owned.as_deref())?; + strip_owned_hooks(&mut root, owned.as_ref())?; remove_owned_mcp(&mut root, owned.is_some())?; if root.as_object().is_some_and(Map::is_empty) { Ok(None) @@ -466,7 +480,12 @@ where entry .get("command") .and_then(Value::as_str) - .is_none_or(|command| Some(command) != owned.as_deref()) + .is_none_or(|command| { + owned.as_ref().map_or_else( + || !is_persistent_relay_hook_command(command), + |commands| !commands.contains(command), + ) + }) }); if approvals.is_empty() { object.remove("approvals"); @@ -485,7 +504,7 @@ where remove_optional_file(&paths.generation)?; replace_optional_file(&paths.allowlist, allowlist.as_deref(), &mut write)?; replace_optional_file(&paths.config, config.as_deref(), &mut write)?; - verify_uninstall(&paths, owned.as_deref()) + verify_uninstall(&paths, owned.as_ref()) })(); if let Err(error) = result { return rollback_error("uninstall", error, &snapshots, &mut write); @@ -520,7 +539,7 @@ where fn verify_install( paths: &PersistentPaths, relay: &Path, - command: &str, + commands: &GeneratedHookCommands, environment: &[String], token: &str, generation_transaction: Option<&GenerationRetirement>, @@ -532,8 +551,8 @@ fn verify_install( if config.pointer("/mcp_servers/nemo-relay") != Some(&expected) { return Err("Hermes MCP server did not persist exactly".into()); } - verify_hook_definitions(&config, command)?; - verify_trust(&paths.allowlist, command)?; + verify_hook_definitions(&config, commands)?; + verify_trust(&paths.allowlist, commands)?; let actual_token = match generation_transaction { Some(transaction) => transaction.active_visible_token()?, @@ -547,7 +566,7 @@ fn verify_install( Ok(()) } -fn verify_hook_definitions(config: &Value, command: &str) -> Result<(), String> { +fn verify_hook_definitions(config: &Value, commands: &GeneratedHookCommands) -> Result<(), String> { for event in CodingAgent::Hermes.hook_events() { let groups = config .pointer(&format!("/hooks/{event}")) @@ -555,7 +574,9 @@ fn verify_hook_definitions(config: &Value, command: &str) -> Result<(), String> .ok_or_else(|| format!("Hermes hook {event} is missing"))?; let matching = groups .iter() - .filter(|group| group.get("command").and_then(Value::as_str) == Some(command)) + .filter(|group| { + group.get("command").and_then(Value::as_str) == Some(commands.for_event(event)) + }) .count(); if matching != 1 { return Err(format!( @@ -573,9 +594,12 @@ fn verify_hook_definitions(config: &Value, command: &str) -> Result<(), String> .as_array() .ok_or_else(|| format!("Hermes {event} hooks must be an array"))?; if !CodingAgent::Hermes.hook_events().contains(&event.as_str()) - && groups - .iter() - .any(|group| group.get("command").and_then(Value::as_str) == Some(command)) + && groups.iter().any(|group| { + group + .get("command") + .and_then(Value::as_str) + .is_some_and(|command| commands.contains(command)) + }) { return Err("Hermes config contains an unexpected Relay hook handler".into()); } @@ -583,7 +607,10 @@ fn verify_hook_definitions(config: &Value, command: &str) -> Result<(), String> Ok(()) } -fn verify_uninstall(paths: &PersistentPaths, owned_command: Option<&str>) -> Result<(), String> { +fn verify_uninstall( + paths: &PersistentPaths, + owned_commands: Option<&GeneratedHookCommands>, +) -> Result<(), String> { if paths.generation.exists() { return Err("Hermes MCP generation fence still exists".into()); } @@ -596,7 +623,7 @@ fn verify_uninstall(paths: &PersistentPaths, owned_command: Option<&str>) -> Res if let Some(raw) = read_optional_utf8(&paths.allowlist).map_err(|error| error.to_string())? { let allowlist = parse_json_object(Some(&raw), "Hermes shell-hook allowlist") .map_err(|e| e.to_string())?; - if allowlist_has_owned_command(&allowlist, owned_command) { + if allowlist_has_owned_command(&allowlist, owned_commands) { return Err("managed Hermes Relay trust approval still exists".into()); } } diff --git a/crates/cli/src/agents/hermes/trust.rs b/crates/cli/src/agents/hermes/trust.rs index 494c26c53..4dd529dff 100644 --- a/crates/cli/src/agents/hermes/trust.rs +++ b/crates/cli/src/agents/hermes/trust.rs @@ -12,11 +12,12 @@ use serde_json::{Value, json}; use crate::agents::CodingAgent; use crate::error::CliError; +use crate::hooks::GeneratedHookCommands; pub(super) fn trusted_hooks( existing: Option<&str>, - previous_command: Option<&str>, - command: &str, + previous_commands: Option<&GeneratedHookCommands>, + commands: &GeneratedHookCommands, relay: &Path, now: SystemTime, ) -> Result { @@ -34,7 +35,9 @@ pub(super) fn trusted_hooks( entry .get("command") .and_then(Value::as_str) - .is_none_or(|candidate| Some(candidate) != previous_command) + .is_none_or(|candidate| { + previous_commands.is_none_or(|commands| !commands.contains(candidate)) + }) }); let approved_at = timestamp(now); let script_mtime_at_approval = fs::metadata(relay) @@ -44,7 +47,7 @@ pub(super) fn trusted_hooks( approvals.extend(CodingAgent::Hermes.hook_events().iter().map(|event| { json!({ "event": event, - "command": command, + "command": commands.for_event(event), "approved_at": approved_at, "script_mtime_at_approval": script_mtime_at_approval, }) @@ -56,7 +59,10 @@ fn timestamp(time: SystemTime) -> String { DateTime::::from(time).to_rfc3339_opts(SecondsFormat::Micros, true) } -pub(super) fn verify_trust(allowlist_path: &Path, command: &str) -> Result<(), String> { +pub(super) fn verify_trust( + allowlist_path: &Path, + commands: &GeneratedHookCommands, +) -> Result<(), String> { let raw = fs::read_to_string(allowlist_path) .map_err(|error| format!("failed to read {}: {error}", allowlist_path.display()))?; let allowlist = @@ -70,7 +76,8 @@ pub(super) fn verify_trust(allowlist_path: &Path, command: &str) -> Result<(), S .iter() .filter(|entry| { entry.get("event").and_then(Value::as_str) == Some(event) - && entry.get("command").and_then(Value::as_str) == Some(command) + && entry.get("command").and_then(Value::as_str) + == Some(commands.for_event(event)) }) .count(); if matching != 1 { @@ -80,14 +87,19 @@ pub(super) fn verify_trust(allowlist_path: &Path, command: &str) -> Result<(), S } } for entry in approvals { - if entry.get("command").and_then(Value::as_str) != Some(command) { + let Some(command) = entry.get("command").and_then(Value::as_str) else { + continue; + }; + if !commands.contains(command) { continue; } let event = entry .get("event") .and_then(Value::as_str) .ok_or_else(|| "Hermes Relay hook approval is missing its event".to_string())?; - if !CodingAgent::Hermes.hook_events().contains(&event) { + if !CodingAgent::Hermes.hook_events().contains(&event) + || command != commands.for_event(event) + { return Err("Hermes allowlist contains an unexpected Relay hook approval".into()); } } diff --git a/crates/cli/src/agents/mod.rs b/crates/cli/src/agents/mod.rs index 41cd5f72a..376387797 100644 --- a/crates/cli/src/agents/mod.rs +++ b/crates/cli/src/agents/mod.rs @@ -200,13 +200,13 @@ impl crate::installation::marketplace::MarketplaceHost for CodingAgent { generation_fence: &std::path::Path, generation_token: &str, ) -> Result { - let command = crate::hooks::persistent_hook_forward_command( + let commands = crate::hooks::persistent_hook_forward_commands( relay, self, generation_fence, generation_token, )?; - Ok(crate::hooks::generated_hooks(self, &command)) + Ok(crate::hooks::generated_policy_hooks(self, &commands)) } fn plugin_registration_args(self, plugin_id: &str) -> Vec { diff --git a/crates/cli/src/commands/hook_forward.rs b/crates/cli/src/commands/hook_forward.rs index 9b767570f..14fdf5770 100644 --- a/crates/cli/src/commands/hook_forward.rs +++ b/crates/cli/src/commands/hook_forward.rs @@ -45,8 +45,11 @@ pub(crate) struct HookForwardCommand { #[arg(long, value_enum)] pub(crate) gateway_mode: Option, /// Return a failure when the payload cannot be delivered or Relay rejects it. - #[arg(long)] + #[arg(long, conflicts_with = "fail_open")] pub(crate) fail_closed: bool, + /// Allow the coding agent to continue when the payload cannot be delivered. + #[arg(long, conflicts_with = "fail_closed")] + pub(crate) fail_open: bool, } impl HookForwardCommand { @@ -61,7 +64,13 @@ impl HookForwardCommand { profile: self.profile, session_metadata: self.session_metadata, gateway_mode: self.gateway_mode.map(Into::into), - fail_closed: self.fail_closed, + failure_policy: if self.fail_closed { + crate::hooks::HookFailurePolicy::FailClosed + } else if self.fail_open { + crate::hooks::HookFailurePolicy::FailOpen + } else { + crate::hooks::HookFailurePolicy::Default + }, } } } diff --git a/crates/cli/src/hooks/delivery.rs b/crates/cli/src/hooks/delivery.rs index b8db65620..cb9cf566f 100644 --- a/crates/cli/src/hooks/delivery.rs +++ b/crates/cli/src/hooks/delivery.rs @@ -29,8 +29,7 @@ pub(crate) async fn hook_forward(command: HookForwardRequest) -> Result<(), CliE return Ok(()); } validate_optional_json("session metadata", command.session_metadata.as_deref())?; - let fail_closed = - command.fail_closed || std::env::var("NEMO_RELAY_FAIL_CLOSED").ok().as_deref() == Some("1"); + let fail_closed = command.failure_policy.fail_closed(); let destination = hook_destination(&command); let persistent = match persistent_gateway(&destination) { Ok(persistent) => persistent, diff --git a/crates/cli/src/hooks/encoding.rs b/crates/cli/src/hooks/encoding.rs index 1ac762c65..f9eaf5113 100644 --- a/crates/cli/src/hooks/encoding.rs +++ b/crates/cli/src/hooks/encoding.rs @@ -12,22 +12,70 @@ use crate::agents::CodingAgent; #[cfg(any(windows, test))] use base64::Engine; +#[cfg(test)] pub(crate) fn generated_hooks(agent: CodingAgent, command: &str) -> Value { + generated_policy_hooks(agent, &GeneratedHookCommands::uniform(command)) +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct GeneratedHookCommands { + fail_open: String, + fail_closed: String, + legacy: Option, +} + +impl GeneratedHookCommands { + pub(crate) fn new(fail_open: impl Into, fail_closed: impl Into) -> Self { + Self { + fail_open: fail_open.into(), + fail_closed: fail_closed.into(), + legacy: None, + } + } + + pub(crate) fn uniform(command: impl Into) -> Self { + let command = command.into(); + Self::new(command.clone(), command) + } + + pub(crate) fn for_event(&self, event: &str) -> &str { + if event_requires_fail_closed(event) { + &self.fail_closed + } else { + &self.fail_open + } + } + + pub(crate) fn contains(&self, command: &str) -> bool { + command == self.fail_open + || command == self.fail_closed + || self.legacy.as_deref() == Some(command) + } + + pub(crate) fn legacy(&self) -> Option<&str> { + self.legacy.as_deref() + } +} + +pub(crate) fn generated_policy_hooks( + agent: CodingAgent, + commands: &GeneratedHookCommands, +) -> Value { if agent.uses_direct_hook_entries() { - direct_hooks(agent.hook_events(), command) + direct_hooks(agent.hook_events(), commands) } else { - grouped_hooks(agent.hook_events(), command) + grouped_hooks(agent.hook_events(), commands) } } /// Canonical persistent hook command used by every supported host. -pub(crate) fn persistent_hook_forward_command( +pub(crate) fn persistent_hook_forward_commands( relay: &Path, agent: CodingAgent, generation_file: &Path, generation_token: &str, -) -> Result { - hook_command( +) -> Result { + hook_commands( relay, &persistent_hook_arguments(agent, generation_file, generation_token), ) @@ -35,22 +83,22 @@ pub(crate) fn persistent_hook_forward_command( /// Canonical transparent hook command. It embeds the process-private dynamic gateway so hook hosts /// that filter inherited environment variables cannot redirect delivery to the fixed endpoint. -pub(crate) fn transparent_hook_forward_command( +pub(crate) fn transparent_hook_forward_commands( relay: &Path, agent: CodingAgent, gateway_url: &str, -) -> Result { - hook_command(relay, &transparent_hook_arguments(agent, gateway_url)) +) -> Result { + hook_commands(relay, &transparent_hook_arguments(agent, gateway_url)) } #[cfg(test)] -pub(crate) fn transparent_hook_forward_command_for_platform( +pub(crate) fn transparent_hook_forward_commands_for_platform( relay: &Path, agent: CodingAgent, gateway_url: &str, windows: bool, -) -> String { - hook_command_for_platform( +) -> GeneratedHookCommands { + hook_commands_for_platform( relay, &transparent_hook_arguments(agent, gateway_url), windows, @@ -58,14 +106,14 @@ pub(crate) fn transparent_hook_forward_command_for_platform( } #[cfg(test)] -pub(crate) fn persistent_hook_forward_command_for_platform( +pub(crate) fn persistent_hook_forward_commands_for_platform( relay: &Path, agent: CodingAgent, generation_file: &Path, generation_token: &str, windows: bool, -) -> String { - hook_command_for_platform( +) -> GeneratedHookCommands { + hook_commands_for_platform( relay, &persistent_hook_arguments(agent, generation_file, generation_token), windows, @@ -99,6 +147,45 @@ pub(super) fn persistent_hook_arguments( ] } +fn hook_commands(relay: &Path, arguments: &[String]) -> Result { + let mut commands = GeneratedHookCommands::new( + hook_command(relay, &with_failure_policy(arguments, "--fail-open"))?, + hook_command(relay, &with_failure_policy(arguments, "--fail-closed"))?, + ); + commands.legacy = Some(hook_command(relay, arguments)?); + Ok(commands) +} + +#[cfg(test)] +fn hook_commands_for_platform( + relay: &Path, + arguments: &[String], + windows: bool, +) -> GeneratedHookCommands { + let mut commands = GeneratedHookCommands::new( + hook_command_for_platform( + relay, + &with_failure_policy(arguments, "--fail-open"), + windows, + ), + hook_command_for_platform( + relay, + &with_failure_policy(arguments, "--fail-closed"), + windows, + ), + ); + commands.legacy = Some(hook_command_for_platform(relay, arguments, windows)); + commands +} + +fn with_failure_policy(arguments: &[String], policy: &str) -> Vec { + arguments + .iter() + .cloned() + .chain(std::iter::once(policy.to_string())) + .collect() +} + pub(super) fn hook_command(relay: &Path, arguments: &[String]) -> Result { #[cfg(windows)] { @@ -302,14 +389,14 @@ pub(super) fn parse_powershell_single_quoted_arguments(mut raw: &str) -> Option< (!arguments.is_empty()).then_some(arguments) } -pub(super) fn direct_hooks(events: &[&str], command: &str) -> Value { +fn direct_hooks(events: &[&str], commands: &GeneratedHookCommands) -> Value { let hooks: serde_json::Map = events .iter() .map(|event| { ( (*event).to_string(), json!([{ - "command": command, + "command": commands.for_event(event), "timeout": 30 }]), ) @@ -321,7 +408,7 @@ pub(super) fn direct_hooks(events: &[&str], command: &str) -> Value { // Generates hook groups for Claude/Codex events and adds a wildcard matcher to tool events when // the target agent requires matcher-scoped tool hooks. Non-tool events omit matchers so they fire // for the full lifecycle. -pub(super) fn grouped_hooks(events: &[&str], command: &str) -> Value { +fn grouped_hooks(events: &[&str], commands: &GeneratedHookCommands) -> Value { let hooks: serde_json::Map = events .iter() .map(|event| { @@ -333,7 +420,7 @@ pub(super) fn grouped_hooks(events: &[&str], command: &str) -> Value { "hooks".into(), json!([{ "type": "command", - "command": command, + "command": commands.for_event(event), "timeout": 30 }]), ); @@ -354,3 +441,7 @@ pub(crate) fn event_matches_tools(event: &str) -> bool { "PreToolUse" | "PostToolUse" | "PostToolUseFailure" | "PermissionRequest" ) } + +pub(crate) fn event_requires_fail_closed(event: &str) -> bool { + matches!(event, "PreToolUse" | "PermissionRequest" | "pre_tool_call") +} diff --git a/crates/cli/src/hooks/mod.rs b/crates/cli/src/hooks/mod.rs index 6389f3c2e..d21efe041 100644 --- a/crates/cli/src/hooks/mod.rs +++ b/crates/cli/src/hooks/mod.rs @@ -23,18 +23,19 @@ pub(crate) use destination::{ pub(crate) use encoding::decode_windows_hook_command; #[cfg(all(test, windows))] pub(crate) use encoding::windows_powershell_path; -#[cfg(test)] pub(crate) use encoding::{ - encoded_windows_hook_command, event_matches_tools, - persistent_hook_forward_command_for_platform, transparent_hook_forward_command_for_platform, + GeneratedHookCommands, generated_policy_hooks, persistent_hook_forward_commands, + transparent_hook_forward_commands, }; +#[cfg(test)] pub(crate) use encoding::{ - generated_hooks, persistent_hook_forward_command, transparent_hook_forward_command, + encoded_windows_hook_command, event_matches_tools, event_requires_fail_closed, generated_hooks, + persistent_hook_forward_commands_for_platform, transparent_hook_forward_commands_for_platform, }; pub(crate) use merging::merge_hooks; #[cfg(test)] pub(crate) use response::{handle_hook_forward_status, handle_verified_hook_forward_response}; -pub(crate) use types::{GatewayMode, HookForwardRequest}; +pub(crate) use types::{GatewayMode, HookFailurePolicy, HookForwardRequest}; #[cfg(test)] use serde_json::json; diff --git a/crates/cli/src/hooks/types.rs b/crates/cli/src/hooks/types.rs index 9959b309d..b11713112 100644 --- a/crates/cli/src/hooks/types.rs +++ b/crates/cli/src/hooks/types.rs @@ -16,7 +16,24 @@ pub(crate) struct HookForwardRequest { pub(crate) profile: Option, pub(crate) session_metadata: Option, pub(crate) gateway_mode: Option, - pub(crate) fail_closed: bool, + pub(crate) failure_policy: HookFailurePolicy, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum HookFailurePolicy { + Default, + FailOpen, + FailClosed, +} + +impl HookFailurePolicy { + pub(crate) fn fail_closed(self) -> bool { + match self { + Self::Default => std::env::var("NEMO_RELAY_FAIL_CLOSED").ok().as_deref() == Some("1"), + Self::FailOpen => false, + Self::FailClosed => true, + } + } } #[derive(Debug, Clone, Copy, PartialEq, Eq)] diff --git a/crates/cli/tests/cli_tests.rs b/crates/cli/tests/cli_tests.rs index 6cc3e6bc3..7a59b9ced 100644 --- a/crates/cli/tests/cli_tests.rs +++ b/crates/cli/tests/cli_tests.rs @@ -788,17 +788,23 @@ fn assert_hermes_install_config(config_path: &std::path::Path, hermes_home: &std .unwrap() .contains("not-written-to-config") ); - let command = config["hooks"]["on_session_start"][0]["command"] - .as_str() - .unwrap(); - assert!(command.contains("hook-forward hermes")); let approvals: serde_json::Value = serde_json::from_str( &std::fs::read_to_string(hermes_home.join("shell-hooks-allowlist.json")).unwrap(), ) .unwrap(); let approvals = approvals["approvals"].as_array().unwrap(); assert_eq!(approvals.len(), 13); - assert!(approvals.iter().all(|entry| entry["command"] == command)); + for approval in approvals { + let event = approval["event"].as_str().unwrap(); + let command = approval["command"].as_str().unwrap(); + assert!(command.contains("hook-forward hermes")); + assert_eq!(approval["command"], config["hooks"][event][0]["command"]); + if event == "pre_tool_call" { + assert!(command.ends_with(" --fail-closed")); + } else { + assert!(command.ends_with(" --fail-open")); + } + } } #[cfg(unix)] @@ -1318,6 +1324,60 @@ fn cli_codex_hook_launch_resolution_error_respects_forwarding_policy() { } } +#[test] +fn cli_hook_forward_explicit_policy_overrides_the_environment() { + let temp = tempfile::tempdir().unwrap(); + let generation = write_active_generation(temp.path()); + for (policy, environment, succeeds) in [ + ("--fail-open", Some("1"), true), + ("--fail-closed", None, false), + ] { + let mut command = Command::new(gateway_bin()); + command + .args([ + "hook-forward", + "codex", + "--gateway-url", + "http://127.0.0.1:1", + "--generation-file", + ]) + .arg(&generation) + .arg("--generation-token") + .arg(ACTIVE_GENERATION_TOKEN) + .arg(policy) + .env("HOME", temp.path()) + .env("XDG_CONFIG_HOME", temp.path().join("xdg")) + .env("XDG_RUNTIME_DIR", temp.path().join("runtime")) + .env("TMPDIR", temp.path()) + .env("NEMO_RELAY_PLUGIN_IDLE_TIMEOUT_SECS", "not-a-number") + .env_remove("NEMO_RELAY_FAIL_CLOSED") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + if let Some(value) = environment { + command.env("NEMO_RELAY_FAIL_CLOSED", value); + } + let mut child = command.spawn().unwrap(); + child.stdin.take().unwrap().write_all(b"{}").unwrap(); + let output = wait_child_with_output(child); + assert_eq!(output.status.success(), succeeds, "{policy}"); + assert!( + String::from_utf8_lossy(&output.stderr).contains("NEMO_RELAY_PLUGIN_IDLE_TIMEOUT_SECS") + ); + } +} + +#[test] +fn cli_hook_forward_rejects_conflicting_failure_policies() { + let output = Command::new(gateway_bin()) + .args(["hook-forward", "codex", "--fail-open", "--fail-closed"]) + .output() + .unwrap(); + + assert!(!output.status.success()); + assert!(String::from_utf8_lossy(&output.stderr).contains("cannot be used with")); +} + #[test] fn cli_codex_hook_launch_resolution_error_retains_default_payload_cap() { const DEFAULT_HOOK_PAYLOAD_BYTES: usize = 20 * 1024 * 1024; diff --git a/crates/cli/tests/coverage/agents/hermes_tests.rs b/crates/cli/tests/coverage/agents/hermes_tests.rs index eae5ee906..ede96d7fc 100644 --- a/crates/cli/tests/coverage/agents/hermes_tests.rs +++ b/crates/cli/tests/coverage/agents/hermes_tests.rs @@ -118,16 +118,20 @@ fn hook_command_round_trips_paths_and_platform_metacharacters() { let relay = Path::new("/tmp/NeMo $Relay`test'/bin/nemo-relay"); let generation = Path::new("/tmp/generation"); assert_eq!( - persistent_hook_command_for_platform(relay, generation, TEST_GENERATION_TOKEN, false), - "'/tmp/NeMo $Relay`test'\\''/bin/nemo-relay' hook-forward hermes --gateway-url http://127.0.0.1:47632 --generation-file /tmp/generation --generation-token test-generation" + persistent_hook_commands_for_platform(relay, generation, TEST_GENERATION_TOKEN, false) + .for_event("on_session_start"), + "'/tmp/NeMo $Relay`test'\\''/bin/nemo-relay' hook-forward hermes --gateway-url http://127.0.0.1:47632 --generation-file /tmp/generation --generation-token test-generation --fail-open" ); assert_eq!( - crate::hooks::decode_windows_hook_command(&persistent_hook_command_for_platform( - Path::new(r"C:\Program Files\NeMo 100%\bin\nemo-relay.exe"), - Path::new(r"C:\Temp\generation"), - TEST_GENERATION_TOKEN, - true, - )) + crate::hooks::decode_windows_hook_command( + persistent_hook_commands_for_platform( + Path::new(r"C:\Program Files\NeMo 100%\bin\nemo-relay.exe"), + Path::new(r"C:\Temp\generation"), + TEST_GENERATION_TOKEN, + true, + ) + .for_event("pre_tool_call") + ) .unwrap(), vec![ r"C:\Program Files\NeMo 100%\bin\nemo-relay.exe", @@ -139,25 +143,29 @@ fn hook_command_round_trips_paths_and_platform_metacharacters() { r"C:\Temp\generation", "--generation-token", TEST_GENERATION_TOKEN, + "--fail-closed", ] ); assert_eq!( - crate::hooks::transparent_hook_forward_command_for_platform( + crate::hooks::transparent_hook_forward_commands_for_platform( relay, CodingAgent::Hermes, "http://127.0.0.1:1234", false, - ), - "'/tmp/NeMo $Relay`test'\\''/bin/nemo-relay' hook-forward hermes --gateway-url http://127.0.0.1:1234 --transparent-run" + ) + .for_event("on_session_start"), + "'/tmp/NeMo $Relay`test'\\''/bin/nemo-relay' hook-forward hermes --gateway-url http://127.0.0.1:1234 --transparent-run --fail-open" ); - let encoded = persistent_hook_command_for_platform( + let encoded = persistent_hook_commands_for_platform( Path::new(r"C:\Program Files\NeMo 100%\bin\nemo-relay.exe"), Path::new(r"C:\Temp\generation"), TEST_GENERATION_TOKEN, true, ); - assert!(is_persistent_relay_hook_command(&encoded)); - let encoded_codex = crate::hooks::persistent_hook_forward_command_for_platform( + assert!(is_persistent_relay_hook_command( + encoded.for_event("pre_tool_call") + )); + let encoded_codex = crate::hooks::persistent_hook_forward_commands_for_platform( Path::new(r"C:\Program Files\NeMo 100%\bin\nemo-relay.exe"), CodingAgent::Codex, Path::new(r"C:\Temp\generation"), @@ -165,7 +173,9 @@ fn hook_command_round_trips_paths_and_platform_metacharacters() { true, ); assert_ne!(encoded, encoded_codex); - assert!(!is_persistent_relay_hook_command(&encoded_codex)); + assert!(!is_persistent_relay_hook_command( + encoded_codex.for_event("PreToolUse") + )); } #[test] @@ -200,7 +210,7 @@ fn persistent_config_migrates_owned_state_and_preserves_unrelated_config() { let temp = tempfile::tempdir().unwrap(); let relay = relay_binary(temp.path()); let generation = temp.path().join(GENERATION_FILE_NAME); - let command = persistent_hook_command(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); + let command = persistent_hook_commands(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); let legacy_command = format!("{} hook-forward hermes", relay.display()); let mut legacy_hooks = serde_json::Map::new(); for event in CodingAgent::Hermes.hook_events() { @@ -264,7 +274,7 @@ fn persistent_config_migrates_owned_state_and_preserves_unrelated_config() { ); assert_eq!( merged["hooks"]["on_session_start"][1]["command"], - json!(command) + json!(command.for_event("on_session_start")) ); assert_eq!( merged["hooks"]["custom_event"][0]["command"], @@ -275,7 +285,7 @@ fn persistent_config_migrates_owned_state_and_preserves_unrelated_config() { assert_eq!( groups .iter() - .filter(|group| group["command"] == json!(command)) + .filter(|group| group["command"] == json!(command.for_event(event))) .count(), 1, "event {event}" @@ -288,7 +298,7 @@ fn persistent_config_rejects_a_foreign_server_with_the_reserved_name() { let temp = tempfile::tempdir().unwrap(); let relay = relay_binary(temp.path()); let generation = temp.path().join(GENERATION_FILE_NAME); - let command = persistent_hook_command(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); + let command = persistent_hook_commands(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); let existing = r#" model: keep-me mcp_servers: @@ -317,7 +327,7 @@ fn manual_same_named_mcp_and_hooks_are_never_claimed() { let temp = tempfile::tempdir().unwrap(); let relay = relay_binary(temp.path()); let generation = temp.path().join(GENERATION_FILE_NAME); - let command = persistent_hook_command(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); + let command = persistent_hook_commands(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); let manual = serde_yaml::to_string(&json!({ "mcp_servers": { MCP_SERVER_NAME: {"command": relay, "args": ["mcp"], "env": {"CUSTOM": "keep"}} @@ -373,21 +383,15 @@ fn modern_mcp_generation_proves_ownership_independently_of_hook_completeness() { let mut root = persistent_config( None, &relay, - &persistent_hook_command(&relay, &generation, "hook-token").unwrap(), + &persistent_hook_commands(&relay, &generation, "hook-token").unwrap(), &generation, "mcp-token", &[], ) .unwrap(); assert_eq!( - owned_install_command(&root, &relay, Some(&generation)) - .unwrap() - .as_deref(), - Some( - persistent_hook_command(&relay, &generation, "mcp-token") - .unwrap() - .as_str() - ) + owned_install_command(&root, &relay, Some(&generation)).unwrap(), + Some(persistent_hook_commands(&relay, &generation, "mcp-token").unwrap()) ); root["mcp_servers"][MCP_SERVER_NAME]["command"] = json!(temp.path().join("other/nemo-relay")); @@ -398,6 +402,43 @@ fn modern_mcp_generation_proves_ownership_independently_of_hook_completeness() { ); } +#[test] +fn persistent_config_migrates_modern_single_command_hooks_to_explicit_policies() { + let temp = tempfile::tempdir().unwrap(); + let relay = relay_binary(temp.path()); + let generation = temp.path().join(GENERATION_FILE_NAME); + let commands = persistent_hook_commands(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); + let mut installed = persistent_config( + None, + &relay, + &commands, + &generation, + TEST_GENERATION_TOKEN, + &[], + ) + .unwrap(); + let legacy = commands.legacy().unwrap(); + for event in CodingAgent::Hermes.hook_events() { + installed["hooks"][event][0]["command"] = json!(legacy); + } + + let migrated = persistent_config( + Some(&serde_yaml::to_string(&installed).unwrap()), + &relay, + &commands, + &generation, + TEST_GENERATION_TOKEN, + &[], + ) + .unwrap(); + + for event in CodingAgent::Hermes.hook_events() { + let hooks = migrated["hooks"][event].as_array().unwrap(); + assert_eq!(hooks.len(), 1, "event {event}"); + assert_eq!(hooks[0]["command"], json!(commands.for_event(event))); + } +} + #[test] fn foreign_reserved_server_aborts_install_before_any_file_changes() { let temp = tempfile::tempdir().unwrap(); @@ -425,7 +466,7 @@ fn trusted_hooks_migrates_only_relay_approvals_and_records_every_event() { let temp = tempfile::tempdir().unwrap(); let relay = relay_binary(temp.path()); let generation = temp.path().join(GENERATION_FILE_NAME); - let command = persistent_hook_command(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); + let command = persistent_hook_commands(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); let existing = json!({ "schema": 7, "approvals": [ @@ -435,9 +476,10 @@ fn trusted_hooks_migrates_only_relay_approvals_and_records_every_event() { ] }); let now = UNIX_EPOCH + Duration::from_secs(1_700_000_000); + let legacy = crate::hooks::GeneratedHookCommands::uniform("nemo-relay hook-forward hermes"); let merged = trusted_hooks( Some(&serde_json::to_string(&existing).unwrap()), - Some("nemo-relay hook-forward hermes"), + Some(&legacy), &command, &relay, now, @@ -455,7 +497,10 @@ fn trusted_hooks_migrates_only_relay_approvals_and_records_every_event() { for event in CodingAgent::Hermes.hook_events() { let entries = approvals .iter() - .filter(|entry| entry["event"] == json!(event) && entry["command"] == json!(command)) + .filter(|entry| { + entry["event"] == json!(event) + && entry["command"] == json!(command.for_event(event)) + }) .collect::>(); assert_eq!(entries.len(), 1, "event {event}"); assert_eq!( @@ -471,7 +516,7 @@ fn verification_rejects_relay_handlers_and_approvals_on_unexpected_events() { let temp = tempfile::tempdir().unwrap(); let relay = relay_binary(temp.path()); let generation = temp.path().join(GENERATION_FILE_NAME); - let command = persistent_hook_command(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); + let command = persistent_hook_commands(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); let mut config = persistent_config( None, &relay, @@ -481,7 +526,8 @@ fn verification_rejects_relay_handlers_and_approvals_on_unexpected_events() { &[], ) .unwrap(); - config["hooks"]["unexpected_event"] = json!([{"command": command, "timeout": 30}]); + config["hooks"]["unexpected_event"] = + json!([{"command": command.for_event("on_session_start"), "timeout": 30}]); let error = verify_hook_definitions(&config, &command).unwrap_err(); assert!(error.contains("unexpected Relay hook")); let mut malformed = persistent_config( @@ -493,14 +539,15 @@ fn verification_rejects_relay_handlers_and_approvals_on_unexpected_events() { &[], ) .unwrap(); - malformed["hooks"]["unexpected_event"] = json!({"command": command}); + malformed["hooks"]["unexpected_event"] = + json!({"command": command.for_event("on_session_start")}); let error = verify_hook_definitions(&malformed, &command).unwrap_err(); assert!(error.contains("must be an array")); let mut allowlist = trusted_hooks(None, None, &command, &relay, UNIX_EPOCH).unwrap(); allowlist["approvals"].as_array_mut().unwrap().push(json!({ "event": "unexpected_event", - "command": command, + "command": command.for_event("on_session_start"), "approved_at": "1970-01-01T00:00:00.000000Z" })); let path = temp.path().join("shell-hooks-allowlist.json"); @@ -513,12 +560,25 @@ fn verification_rejects_relay_handlers_and_approvals_on_unexpected_events() { .as_array_mut() .unwrap() .push(json!({ - "command": command, + "command": command.for_event("on_session_start"), "approved_at": "1970-01-01T00:00:00.000000Z" })); std::fs::write(&path, serde_json::to_vec(&missing_event).unwrap()).unwrap(); let error = verify_trust(&path, &command).unwrap_err(); assert!(error.contains("missing its event")); + + let mut wrong_policy = trusted_hooks(None, None, &command, &relay, UNIX_EPOCH).unwrap(); + wrong_policy["approvals"] + .as_array_mut() + .unwrap() + .push(json!({ + "event": "pre_tool_call", + "command": command.for_event("on_session_start"), + "approved_at": "1970-01-01T00:00:00.000000Z" + })); + std::fs::write(&path, serde_json::to_vec(&wrong_policy).unwrap()).unwrap(); + let error = verify_trust(&path, &command).unwrap_err(); + assert!(error.contains("unexpected Relay hook approval")); } #[test] @@ -526,7 +586,7 @@ fn hermes_structure_and_trust_validation_cover_exact_failure_shapes() { let temp = tempfile::tempdir().unwrap(); let relay = relay_binary(temp.path()); let generation = temp.path().join(GENERATION_FILE_NAME); - let command = persistent_hook_command(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); + let command = persistent_hook_commands(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); let error = trusted_hooks( Some(r#"{"approvals": {}}"#), @@ -617,13 +677,15 @@ fn install_is_verified_idempotent_and_rotates_the_generation() { let config = yaml(&paths.config); let second_command = - persistent_hook_command(&relay, &paths.generation, &second_generation).unwrap(); + persistent_hook_commands(&relay, &paths.generation, &second_generation).unwrap(); assert_eq!( config["hooks"]["on_session_start"] .as_array() .unwrap() .iter() - .filter(|group| group["command"] == json!(second_command)) + .filter(|group| { + group["command"] == json!(second_command.for_event("on_session_start")) + }) .count(), 1 ); @@ -954,6 +1016,41 @@ fn uninstall_removes_only_relay_owned_hermes_state() { assert_eq!(allowlist["approvals"][0]["command"], json!("custom-hook")); } +#[test] +fn uninstall_removes_orphaned_generated_approval_without_config() { + let temp = tempfile::tempdir().unwrap(); + let relay = relay_binary(temp.path()); + let paths = paths(&temp.path().join("hermes")); + let commands = + persistent_hook_commands(&relay, &paths.generation, TEST_GENERATION_TOKEN).unwrap(); + std::fs::create_dir_all(paths.allowlist.parent().unwrap()).unwrap(); + std::fs::write( + &paths.allowlist, + serde_json::to_vec(&json!({ + "approvals": [ + { + "event": "pre_tool_call", + "command": commands.for_event("pre_tool_call") + }, + { + "event": "custom_event", + "command": "custom-hook" + } + ] + })) + .unwrap(), + ) + .unwrap(); + + let removed = uninstall_persistent_with(paths.clone(), atomic_write).unwrap(); + + assert_eq!(removed, vec![paths.allowlist.clone()]); + assert_eq!( + json_file(&paths.allowlist)["approvals"], + json!([{"event": "custom_event", "command": "custom-hook"}]) + ); +} + #[test] fn uninstall_rolls_back_every_file_when_commit_fails() { let temp = tempfile::tempdir().unwrap(); @@ -1084,11 +1181,11 @@ fn persistent_state_detection_recognizes_each_relay_owned_surface() { &roots[2].config, serde_yaml::to_string(&json!({ "hooks": { - "on_session_start": [{"command": persistent_hook_command( + "on_session_start": [{"command": persistent_hook_commands( &relay, &roots[2].generation, TEST_GENERATION_TOKEN - ).unwrap()}] + ).unwrap().for_event("on_session_start")}] } })) .unwrap(), @@ -1099,11 +1196,11 @@ fn persistent_state_detection_recognizes_each_relay_owned_surface() { serde_json::to_vec(&json!({ "approvals": [{ "event": "on_session_start", - "command": persistent_hook_command( + "command": persistent_hook_commands( &relay, &roots[3].generation, TEST_GENERATION_TOKEN - ).unwrap() + ).unwrap().for_event("on_session_start") }] })) .unwrap(), @@ -1124,7 +1221,7 @@ fn persistent_state_detection_recognizes_each_relay_owned_surface() { fn transparent_config_suppresses_only_the_managed_mcp_and_uses_one_relay_hook() { let temp = tempfile::tempdir().unwrap(); let relay = relay_binary(temp.path()); - let command = crate::hooks::transparent_hook_forward_command( + let command = crate::hooks::transparent_hook_forward_commands( &relay, CodingAgent::Hermes, "http://127.0.0.1:1234", @@ -1132,7 +1229,7 @@ fn transparent_config_suppresses_only_the_managed_mcp_and_uses_one_relay_hook() .unwrap(); let generation = temp.path().join(GENERATION_FILE_NAME); let persistent_command = - persistent_hook_command(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); + persistent_hook_commands(&relay, &generation, TEST_GENERATION_TOKEN).unwrap(); let mut existing = persistent_config( None, &relay, @@ -1172,7 +1269,7 @@ fn transparent_config_suppresses_only_the_managed_mcp_and_uses_one_relay_hook() groups .iter() .filter_map(|group| group.get("command").and_then(Value::as_str)) - .filter(|candidate| **candidate == command) + .filter(|candidate| *candidate == command.for_event(event)) .count(), 1, "event {event}" @@ -1180,7 +1277,7 @@ fn transparent_config_suppresses_only_the_managed_mcp_and_uses_one_relay_hook() assert!( groups .iter() - .any(|group| group["command"] == json!(command)) + .any(|group| group["command"] == json!(command.for_event(event))) ); } assert!( @@ -1345,21 +1442,18 @@ fn hermes_uninstall_and_verification_reject_malformed_or_residual_state() { .unwrap(); let config = yaml(&hermes_paths.config); - let command = config["hooks"]["on_session_start"][0]["command"] - .as_str() - .unwrap() - .to_string(); let token = InstallGeneration::capture(hermes_paths.generation.clone()) .unwrap() .token() .to_owned(); + let command = persistent_hook_commands(&relay, &hermes_paths.generation, &token).unwrap(); let expected_environment = forwarded_environment_names(&[], None); let mut duplicate_hook = config.clone(); duplicate_hook["hooks"]["on_session_start"] .as_array_mut() .unwrap() - .push(json!({"command": command})); + .push(json!({"command": command.for_event("on_session_start")})); let error = verify_hook_definitions(&duplicate_hook, &command).unwrap_err(); assert!( error.contains("exactly one trusted Relay handler"), @@ -1406,15 +1500,12 @@ fn hermes_uninstall_and_verification_reject_malformed_or_residual_state() { atomic_write, ) .unwrap(); - let config = yaml(&hermes_paths.config); - let command = config["hooks"]["on_session_start"][0]["command"] - .as_str() - .unwrap() - .to_string(); let expected_token = InstallGeneration::capture(hermes_paths.generation.clone()) .unwrap() .token() .to_owned(); + let command = + persistent_hook_commands(&relay, &hermes_paths.generation, &expected_token).unwrap(); crate::installation::generation::write_new_generation(&hermes_paths.generation).unwrap(); let error = verify_install( &hermes_paths, @@ -1447,18 +1538,18 @@ fn hermes_uninstall_verifier_identifies_each_residual_owned_surface() { install_persistent_with(paths.clone(), &relay, &[], None, UNIX_EPOCH, atomic_write).unwrap(); let command = owned_command_from_config(&yaml(&paths.config), Some(&paths.generation)); - let error = verify_uninstall(&paths, command.as_deref()).unwrap_err(); + let error = verify_uninstall(&paths, command.as_ref()).unwrap_err(); assert!(error.contains("generation fence still exists"), "{error}"); std::fs::remove_file(&paths.generation).unwrap(); - let error = verify_uninstall(&paths, command.as_deref()).unwrap_err(); + let error = verify_uninstall(&paths, command.as_ref()).unwrap_err(); assert!( error.contains("managed Hermes Relay config still exists"), "{error}" ); std::fs::remove_file(&paths.config).unwrap(); - let error = verify_uninstall(&paths, command.as_deref()).unwrap_err(); + let error = verify_uninstall(&paths, command.as_ref()).unwrap_err(); assert!( error.contains("managed Hermes Relay trust approval still exists"), "{error}" diff --git a/crates/cli/tests/coverage/agents/plugin_host_tests.rs b/crates/cli/tests/coverage/agents/plugin_host_tests.rs index c44f13e99..9d6b15447 100644 --- a/crates/cli/tests/coverage/agents/plugin_host_tests.rs +++ b/crates/cli/tests/coverage/agents/plugin_host_tests.rs @@ -86,7 +86,7 @@ impl CodexHooksClient for FakeCodexHooksClient { } } -fn expected_plugin_command() -> String { +fn expected_plugin_command() -> crate::hooks::GeneratedHookCommands { let relay = current_exe().unwrap(); let relay = relay.canonicalize().unwrap_or(relay); let relay = portable_executable_path(relay); @@ -108,7 +108,7 @@ fn write_plugin_generation_for_hooks(path: &Path) { .unwrap(); } -fn expected_plugin_command_for_hooks(path: &Path) -> String { +fn expected_plugin_command_for_hooks(path: &Path, event: &str) -> String { fs::read_to_string(path) .ok() .and_then(|raw| serde_json::from_str::(&raw).ok()) @@ -116,8 +116,9 @@ fn expected_plugin_command_for_hooks(path: &Path) -> String { value .get("hooks")? .as_object()? - .values() - .next()? + .iter() + .find(|(name, _)| normalize_hook_event(name) == normalize_hook_event(event))? + .1 .as_array()? .first()? .get("hooks")? @@ -127,7 +128,7 @@ fn expected_plugin_command_for_hooks(path: &Path) -> String { .as_str() .map(str::to_owned) }) - .unwrap_or_else(expected_plugin_command) + .unwrap_or_else(|| expected_plugin_command().for_event(event).to_owned()) } fn empty_codex_hooks_client() -> FakeCodexHooksClient { @@ -143,7 +144,7 @@ fn write_plugin_hooks(plugin_root: &Path) -> PathBuf { write_plugin_generation_for_hooks(&path); fs::write( &path, - serde_json::to_vec_pretty(&generated_hooks( + serde_json::to_vec_pretty(&crate::hooks::generated_policy_hooks( CodingAgent::Codex, &expected_plugin_hook_command(&path).unwrap(), )) @@ -170,9 +171,9 @@ fn codex_hook_metadata( fs::create_dir_all(hooks_path.parent().unwrap()).unwrap(); fs::write( &hooks_path, - serde_json::to_vec_pretty(&generated_hooks( + serde_json::to_vec_pretty(&crate::hooks::generated_policy_hooks( CodingAgent::Codex, - &expected_plugin_command_for_hooks(&hooks_path), + &expected_plugin_command(), )) .unwrap(), ) @@ -182,7 +183,7 @@ fn codex_hook_metadata( key: key.into(), event_name: event_name.into(), handler_type: "command".into(), - command: Some(expected_plugin_command_for_hooks(&hooks_path)), + command: Some(expected_plugin_command_for_hooks(&hooks_path, event_name)), source_path: hooks_path.display().to_string(), source: "plugin".into(), plugin_id: Some(CODEX_PLUGIN_ID.into()), @@ -949,8 +950,13 @@ fn codex_auto_trust_rejects_modified_loaded_plugin_hook_file() { let config_path = dir.path().join("config.toml"); fs::write(&config_path, "").unwrap(); let mut hooks = required_codex_hook_metadata(&reported_hooks_path, "untrusted", true); + let expected = expected_plugin_command(); for hook in &mut hooks { - hook.command = Some(expected_plugin_command()); + hook.command = Some( + expected_codex_hook_command(&expected, &hook.event_name) + .unwrap() + .to_owned(), + ); } let mut client = FakeCodexHooksClient { hook_lists: VecDeque::from([Ok(hooks)]), @@ -1018,6 +1024,33 @@ fn codex_hook_trust_report_distinguishes_modified_disabled_and_missing_hooks() { ); } +#[test] +fn codex_hook_trust_report_recognizes_legacy_generated_hooks_as_stale() { + let dir = tempdir().unwrap(); + let hooks_path = dir.path().join(".codex").join("hooks.json"); + let expected = expected_plugin_command(); + let legacy = expected.legacy().unwrap().to_owned(); + let mut hooks = required_codex_hook_metadata(&hooks_path, "trusted", true); + for hook in &mut hooks { + hook.command = Some(legacy.clone()); + } + fs::write( + &hooks_path, + serde_json::to_vec_pretty(&generated_hooks(CodingAgent::Codex, &legacy)).unwrap(), + ) + .unwrap(); + let mut client = FakeCodexHooksClient { + hook_lists: VecDeque::from([Ok(hooks)]), + ..FakeCodexHooksClient::default() + }; + + let error = codex_hook_trust_report_with_client(&mut client, dir.path(), &expected) + .expect_err("legacy generated hooks must require reinstall"); + + assert!(error.contains("loaded modified Relay hooks"), "{error}"); + assert!(error.contains("install codex --force"), "{error}"); +} + #[test] fn codex_hook_state_key_path_quotes_arbitrary_hook_identity() { assert_eq!( @@ -1896,7 +1929,11 @@ fn codex_setup_can_validate_hooks_while_installer_holds_the_generation_lock() { fs::create_dir_all(hooks_path.parent().unwrap()).unwrap(); fs::write( &hooks_path, - serde_json::to_vec_pretty(&generated_hooks(CodingAgent::Codex, &command)).unwrap(), + serde_json::to_vec_pretty(&crate::hooks::generated_policy_hooks( + CodingAgent::Codex, + &command, + )) + .unwrap(), ) .unwrap(); let _transaction = @@ -2000,7 +2037,7 @@ fn codex_setup_uses_plugin_hooks_without_writing_user_hooks() { DEFAULT_URL, &expected_plugin_command(), |_home, _config, command| { - assert_eq!(command, expected_plugin_command()); + assert_eq!(command, &expected_plugin_command()); Ok(()) }, ) @@ -2876,12 +2913,10 @@ fn windows_shell_argument_quoting_and_hook_encoding_preserve_paths() { r#""C:\Program Files\NeMo 100%%cd:~,%\bin\nemo-relay.exe""# ); assert_eq!( - crate::hooks::decode_windows_hook_command(&codex_plugin_hook_command_for_platform( - &relay, - &generation, - "test-generation", - true, - )) + crate::hooks::decode_windows_hook_command( + codex_plugin_hook_command_for_platform(&relay, &generation, "test-generation", true,) + .for_event("PreToolUse") + ) .unwrap(), vec![ relay.display().to_string(), @@ -2893,6 +2928,7 @@ fn windows_shell_argument_quoting_and_hook_encoding_preserve_paths() { generation.display().to_string(), "--generation-token".into(), "test-generation".into(), + "--fail-closed".into(), ] ); assert_eq!( @@ -2913,7 +2949,10 @@ fn generated_windows_hook_command_executes_exact_arguments() { let marker = temp.path().join("hook-ran.txt"); let input_marker = temp.path().join("hook-input.txt"); let generation = temp.path().join("Generation & %USERPROFILE%"); - let command = codex_plugin_hook_command(&relay, &generation, "test-generation").unwrap(); + let command = codex_plugin_hook_command(&relay, &generation, "test-generation") + .unwrap() + .for_event("PreToolUse") + .to_owned(); let mut child = std::process::Command::new("cmd.exe") .arg("/C") .arg(&command) @@ -2950,7 +2989,10 @@ fn generated_windows_hook_command_propagates_the_relay_exit_code() { let relay = temp.path().join("relay failure.exe"); compile_windows_hook_test_relay(&relay); let generation = temp.path().join("generation"); - let command = codex_plugin_hook_command(&relay, &generation, "test-generation").unwrap(); + let command = codex_plugin_hook_command(&relay, &generation, "test-generation") + .unwrap() + .for_event("PreToolUse") + .to_owned(); let status = std::process::Command::new("cmd.exe") .arg("/C") @@ -2990,8 +3032,9 @@ fn posix_shell_argument_quoting_and_hook_encoding_preserve_paths() { "'/tmp/NeMo $Relay`test'\\''/bin/nemo-relay'" ); assert_eq!( - codex_plugin_hook_command_for_platform(&relay, &generation, "test-generation", false), - "'/tmp/NeMo $Relay`test'\\''/bin/nemo-relay' hook-forward codex --gateway-url http://127.0.0.1:47632 --generation-file '/tmp/NeMo $Relay`test'\\''/plugin/.nemo-relay-generation' --generation-token test-generation" + codex_plugin_hook_command_for_platform(&relay, &generation, "test-generation", false) + .for_event("SessionStart"), + "'/tmp/NeMo $Relay`test'\\''/bin/nemo-relay' hook-forward codex --gateway-url http://127.0.0.1:47632 --generation-file '/tmp/NeMo $Relay`test'\\''/plugin/.nemo-relay-generation' --generation-token test-generation --fail-open" ); assert_eq!(shell_quote_arg_for_platform("", false), "''"); assert_eq!( @@ -3780,7 +3823,7 @@ fn plugin_host_entrypoints_reject_unsupported_agents_and_report_json() { "SessionStart": [{ "hooks": [{ "type": "command", - "command": expected_plugin_command(), + "command": expected_plugin_command().for_event("SessionStart"), "timeout": 30 }] }] diff --git a/crates/cli/tests/coverage/agents/plugin_install_tests.rs b/crates/cli/tests/coverage/agents/plugin_install_tests.rs index 8af743d26..d20daa95b 100644 --- a/crates/cli/tests/coverage/agents/plugin_install_tests.rs +++ b/crates/cli/tests/coverage/agents/plugin_install_tests.rs @@ -1420,13 +1420,14 @@ fn plugin_manifests_and_hooks_use_path_based_relay_command() { ) .unwrap()["hooks"]["SessionStart"][0]["hooks"][0]["command"], json!( - crate::hooks::persistent_hook_forward_command( + crate::hooks::persistent_hook_forward_commands( Path::new("/bin/nemo-relay"), CodingAgent::Codex, &generation_fence, TEST_GENERATION_TOKEN, ) .unwrap() + .for_event("SessionStart") ) ); assert_eq!( @@ -1438,13 +1439,14 @@ fn plugin_manifests_and_hooks_use_path_based_relay_command() { ) .unwrap()["hooks"]["SessionStart"][0]["hooks"][0]["command"], json!( - crate::hooks::persistent_hook_forward_command( + crate::hooks::persistent_hook_forward_commands( Path::new("/bin/nemo-relay"), CodingAgent::ClaudeCode, &generation_fence, TEST_GENERATION_TOKEN, ) .unwrap() + .for_event("SessionStart") ) ); } @@ -1471,13 +1473,14 @@ fn relay_identity_prefers_the_path_resolved_executable() { ) .unwrap()["hooks"]["SessionStart"][0]["hooks"][0]["command"], json!( - crate::hooks::persistent_hook_forward_command( + crate::hooks::persistent_hook_forward_commands( &relay, CodingAgent::Codex, &generation, TEST_GENERATION_TOKEN, ) .unwrap() + .for_event("SessionStart") ) ); assert_eq!( diff --git a/crates/cli/tests/coverage/shared/installer_tests.rs b/crates/cli/tests/coverage/shared/installer_tests.rs index f77f7d74d..40d82f12d 100644 --- a/crates/cli/tests/coverage/shared/installer_tests.rs +++ b/crates/cli/tests/coverage/shared/installer_tests.rs @@ -92,7 +92,7 @@ async fn transparent_hook_delivery_authenticates_the_wrapper_gateway() { profile: None, session_metadata: None, gateway_mode: None, - fail_closed: true, + failure_policy: HookFailurePolicy::FailClosed, }; let gateway = transparent_gateway_spec(&gateway_url).unwrap(); @@ -329,42 +329,39 @@ fn helper_formatting_and_headers_cover_optional_paths() { #[test] fn generated_hook_dispatch_covers_all_agents() { - for agent in [ - CodingAgent::ClaudeCode, - CodingAgent::Codex, - CodingAgent::Hermes, - ] { - assert!(generated_hooks(agent, "cmd")["hooks"].is_object()); - } + assert_generated_hook_policies(); assert_eq!( - transparent_hook_forward_command_for_platform( + transparent_hook_forward_commands_for_platform( Path::new("nemo-relay"), CodingAgent::Hermes, "http://127.0.0.1:1234", false, - ), - "nemo-relay hook-forward hermes --gateway-url http://127.0.0.1:1234 --transparent-run" + ) + .for_event("on_session_start"), + "nemo-relay hook-forward hermes --gateway-url http://127.0.0.1:1234 --transparent-run --fail-open" ); assert_eq!( - transparent_hook_forward_command_for_platform( + transparent_hook_forward_commands_for_platform( Path::new("/abs/path/to/nemo-relay"), CodingAgent::Codex, "http://127.0.0.1:1234", false, - ), - "/abs/path/to/nemo-relay hook-forward codex --gateway-url http://127.0.0.1:1234 --transparent-run" + ) + .for_event("PreToolUse"), + "/abs/path/to/nemo-relay hook-forward codex --gateway-url http://127.0.0.1:1234 --transparent-run --fail-closed" ); let relay = Path::new("/opt/NeMo Relay's & tools/nemo-relay"); assert_eq!( - transparent_hook_forward_command_for_platform( + transparent_hook_forward_commands_for_platform( relay, CodingAgent::Codex, "http://127.0.0.1:1234", false - ), - r#"'/opt/NeMo Relay'\''s & tools/nemo-relay' hook-forward codex --gateway-url http://127.0.0.1:1234 --transparent-run"# + ) + .for_event("SessionStart"), + r#"'/opt/NeMo Relay'\''s & tools/nemo-relay' hook-forward codex --gateway-url http://127.0.0.1:1234 --transparent-run --fail-open"# ); - let native = transparent_hook_forward_command( + let native = transparent_hook_forward_commands( Path::new("nemo-relay"), CodingAgent::Hermes, "http://127.0.0.1:1234", @@ -372,7 +369,7 @@ fn generated_hook_dispatch_covers_all_agents() { .unwrap(); if cfg!(windows) { assert_eq!( - decode_windows_hook_command(&native).unwrap(), + decode_windows_hook_command(native.for_event("on_session_start")).unwrap(), vec![ String::from("nemo-relay"), String::from("hook-forward"), @@ -380,12 +377,13 @@ fn generated_hook_dispatch_covers_all_agents() { String::from("--gateway-url"), String::from("http://127.0.0.1:1234"), String::from("--transparent-run"), + String::from("--fail-open"), ] ); } else { assert_eq!( native, - transparent_hook_forward_command_for_platform( + transparent_hook_forward_commands_for_platform( Path::new("nemo-relay"), CodingAgent::Hermes, "http://127.0.0.1:1234", @@ -393,12 +391,13 @@ fn generated_hook_dispatch_covers_all_agents() { ) ); } - let windows = transparent_hook_forward_command_for_platform( + let windows = transparent_hook_forward_commands_for_platform( relay, CodingAgent::ClaudeCode, "http://127.0.0.1:1234", true, ); + let windows = windows.for_event("PreToolUse"); let (launcher, encoded) = windows.rsplit_once(' ').unwrap(); assert_eq!( launcher, @@ -412,7 +411,7 @@ fn generated_hook_dispatch_covers_all_agents() { || matches!(character, '+' | '/' | '=')) ); assert_eq!( - decode_windows_hook_command(&windows).unwrap(), + decode_windows_hook_command(windows).unwrap(), vec![ relay.display().to_string(), "hook-forward".into(), @@ -420,6 +419,7 @@ fn generated_hook_dispatch_covers_all_agents() { "--gateway-url".into(), "http://127.0.0.1:1234".into(), "--transparent-run".into(), + "--fail-closed".into(), ] ); assert!(decode_windows_hook_command("powershell.exe -EncodedCommand invalid").is_none()); @@ -446,6 +446,38 @@ fn generated_hook_dispatch_covers_all_agents() { assert!(error.contains("shorten the Relay or plugin installation path")); } +fn assert_generated_hook_policies() { + for agent in [ + CodingAgent::ClaudeCode, + CodingAgent::Codex, + CodingAgent::Hermes, + ] { + assert!(generated_hooks(agent, "cmd")["hooks"].is_object()); + let commands = GeneratedHookCommands::new("cmd --fail-open", "cmd --fail-closed"); + let generated = generated_policy_hooks(agent, &commands); + for event in agent.hook_events() { + let command = if agent.uses_direct_hook_entries() { + generated["hooks"][event][0]["command"].as_str() + } else { + generated["hooks"][event][0]["hooks"][0]["command"].as_str() + } + .unwrap(); + assert_eq!( + command, + commands.for_event(event), + "unexpected policy for {} {event}", + agent.label() + ); + assert_eq!( + command.ends_with("--fail-closed"), + event_requires_fail_closed(event), + "unexpected enforcement classification for {} {event}", + agent.label() + ); + } + } +} + #[test] fn codex_generation_uses_exactly_the_supported_hook_schema() { let generated = generated_hooks(CodingAgent::Codex, "cmd"); @@ -521,34 +553,46 @@ fn packaged_plugin_hooks_use_expected_forwarding_commands() { assert_eq!( claude["hooks"]["SessionStart"][0]["hooks"][0]["command"], json!(format!( - "nemo-relay hook-forward claude --gateway-url {} --forward-only", + "nemo-relay hook-forward claude --gateway-url {} --forward-only --fail-open", crate::bootstrap::DEFAULT_URL )) ); assert_eq!( codex["hooks"]["SessionStart"][0]["hooks"][0]["command"], json!(format!( - "nemo-relay hook-forward codex --gateway-url {} --forward-only", + "nemo-relay hook-forward codex --gateway-url {} --forward-only --fail-open", crate::bootstrap::DEFAULT_URL )) ); assert_eq!( claude["hooks"], - generated_hooks( + generated_policy_hooks( CodingAgent::ClaudeCode, - &format!( - "nemo-relay hook-forward claude --gateway-url {} --forward-only", - crate::bootstrap::DEFAULT_URL + &GeneratedHookCommands::new( + format!( + "nemo-relay hook-forward claude --gateway-url {} --forward-only --fail-open", + crate::bootstrap::DEFAULT_URL + ), + format!( + "nemo-relay hook-forward claude --gateway-url {} --forward-only --fail-closed", + crate::bootstrap::DEFAULT_URL + ), ), )["hooks"] ); assert_eq!( codex["hooks"], - generated_hooks( + generated_policy_hooks( CodingAgent::Codex, - &format!( - "nemo-relay hook-forward codex --gateway-url {} --forward-only", - crate::bootstrap::DEFAULT_URL + &GeneratedHookCommands::new( + format!( + "nemo-relay hook-forward codex --gateway-url {} --forward-only --fail-open", + crate::bootstrap::DEFAULT_URL + ), + format!( + "nemo-relay hook-forward codex --gateway-url {} --forward-only --fail-closed", + crate::bootstrap::DEFAULT_URL + ), ), )["hooks"] ); diff --git a/crates/cli/tests/fixtures/windows_hook_relay.rs b/crates/cli/tests/fixtures/windows_hook_relay.rs index e8a03b5d0..cedd3c885 100644 --- a/crates/cli/tests/fixtures/windows_hook_relay.rs +++ b/crates/cli/tests/fixtures/windows_hook_relay.rs @@ -16,6 +16,7 @@ fn main() { generation, OsString::from("--generation-token"), OsString::from("test-generation"), + OsString::from("--fail-closed"), ]; let actual = std::env::args_os().skip(1).collect::>(); if actual != expected { diff --git a/docs/about-nemo-relay/release-notes/index.mdx b/docs/about-nemo-relay/release-notes/index.mdx index 408d20151..a839cb127 100644 --- a/docs/about-nemo-relay/release-notes/index.mdx +++ b/docs/about-nemo-relay/release-notes/index.mdx @@ -32,6 +32,10 @@ compatibility information applies to the current 0.7 prerelease. ### Highlights +- Generated coding-agent enforcement hooks now fail closed when Relay cannot + start, authenticate, evaluate, or deliver a response. Lifecycle and + after-the-fact hooks explicitly fail open. Reinstall each coding-agent + integration with `nemo-relay install --force` after upgrading. - LLM observability sanitizers now receive the active request or response codec for each managed call. Sanitizers can normalize built-in, runtime-registered, and opaque codec payloads without changing the diff --git a/docs/nemo-relay-cli/basic-usage.mdx b/docs/nemo-relay-cli/basic-usage.mdx index 13417c964..55d9b4e8a 100644 --- a/docs/nemo-relay-cli/basic-usage.mdx +++ b/docs/nemo-relay-cli/basic-usage.mdx @@ -516,9 +516,17 @@ stores the canonical absolute command and trusts only its exact event pairs; it does not enable global hook auto-acceptance. `hook-forward` reads the canonical hook payload from standard input, sends it -to the matching endpoint, and prints the endpoint response. It fails open by -default so observability outages do not block the coding agent. Add -`--fail-closed` only when policy requires hook delivery to block the agent. +to the matching endpoint, and prints the endpoint response. Generated +`PreToolUse`, `PermissionRequest`, and Hermes `pre_tool_call` hooks use +`--fail-closed`; generated lifecycle and after-the-fact hooks use +`--fail-open`. This blocks permission-bearing operations when Relay cannot +evaluate them without making observability-only hooks a runtime dependency. +Rerun `nemo-relay install --force` after upgrading to replace legacy +generated hooks. + +Manually authored commands fail open when neither policy flag is present. +`NEMO_RELAY_FAIL_CLOSED=1` changes that default for compatibility, while an +explicit flag takes precedence over the environment. These flags control delivery and metadata: @@ -530,9 +538,11 @@ These flags control delivery and metadata: - `--session-metadata` sets `x-nemo-relay-session-metadata`. - `--profile` sets `x-nemo-relay-config-profile`. - `--gateway-mode` sets `x-nemo-relay-gateway-mode`. +- `--fail-open` allows the agent to continue after a delivery failure, even + when `NEMO_RELAY_FAIL_CLOSED=1` is set. Structured guardrail rejections still + return a failure. - `--fail-closed` returns a failure when delivery fails or Relay rejects the - hook. Without it, forwarding fails open so an observability outage does not - block the coding agent. + hook. ## Agent Guides diff --git a/integrations/coding-agents/README.md b/integrations/coding-agents/README.md index 20ab6d57f..0ef4d79e9 100644 --- a/integrations/coding-agents/README.md +++ b/integrations/coding-agents/README.md @@ -262,14 +262,16 @@ URL. For Codex, the installed plugin file is the sole persistent Relay hook source; installation does not add Relay groups to `~/.codex/hooks.json`. -Since hook forwarding fails open by default, gateway or sidecar outages do not -block the coding agent. The hook command exits successfully after logging the -forwarding problem, so the host agent can continue even though that hook -payload can be missing from telemetry. For wrapper-generated `hook-forward` -commands, add `--fail-closed` when policy requires hook delivery to block the -agent. For generated persistent hooks, set `NEMO_RELAY_FAIL_CLOSED=1` in the hook -execution environment. In that mode, forwarding failures return a non-zero -hook command status to the host. +Generated hooks select an explicit failure policy by event. `PreToolUse`, +`PermissionRequest`, and Hermes `pre_tool_call` hooks use `--fail-closed`, so +Relay startup, authentication, delivery, and response failures block the +permission-bearing operation. Lifecycle and after-the-fact hooks use +`--fail-open`, so observability outages do not block unrelated agent work. + +After upgrading, rerun `nemo-relay install --force` to replace legacy +generated hooks that did not carry an explicit policy. Manually authored +`hook-forward` commands still fail open by default; set +`NEMO_RELAY_FAIL_CLOSED=1` or add `--fail-closed` when they enforce policy. These `hook-forward` options control delivery and metadata: @@ -284,6 +286,8 @@ These `hook-forward` options control delivery and metadata: - `--profile ` records a configuration profile in session metadata. - `--gateway-mode hook-only|passthrough|required` records the expected gateway behavior in session metadata. +- `--fail-open` allows the coding agent to continue after a delivery failure, + even when `NEMO_RELAY_FAIL_CLOSED=1` is set. - `--fail-closed` returns a failure when delivery fails or Relay rejects the hook instead of allowing the coding agent to continue. diff --git a/integrations/coding-agents/claude-code/hooks/hooks.json b/integrations/coding-agents/claude-code/hooks/hooks.json index 73cb06ef4..7524fa193 100644 --- a/integrations/coding-agents/claude-code/hooks/hooks.json +++ b/integrations/coding-agents/claude-code/hooks/hooks.json @@ -5,7 +5,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -16,7 +16,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -27,7 +27,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -39,7 +39,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-closed", "timeout": 30 } ] @@ -51,7 +51,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -63,7 +63,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -75,7 +75,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-closed", "timeout": 30 } ] @@ -86,7 +86,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -97,7 +97,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -108,7 +108,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -119,7 +119,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -130,7 +130,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -141,7 +141,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -152,7 +152,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward claude --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] diff --git a/integrations/coding-agents/codex/hooks/hooks.json b/integrations/coding-agents/codex/hooks/hooks.json index 550a462bc..a20626ad0 100644 --- a/integrations/coding-agents/codex/hooks/hooks.json +++ b/integrations/coding-agents/codex/hooks/hooks.json @@ -6,7 +6,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -17,7 +17,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -29,7 +29,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only --fail-closed", "timeout": 30 } ] @@ -41,7 +41,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -53,7 +53,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only --fail-closed", "timeout": 30 } ] @@ -64,7 +64,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -75,7 +75,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -86,7 +86,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -97,7 +97,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] @@ -108,7 +108,7 @@ "hooks": [ { "type": "command", - "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only", + "command": "nemo-relay hook-forward codex --gateway-url http://127.0.0.1:47632 --forward-only --fail-open", "timeout": 30 } ] From 3cd5052f55b6ce8fb6ac0789cdc046941cff66c3 Mon Sep 17 00:00:00 2001 From: Will Killian <2007799+willkill07@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:17:28 -0400 Subject: [PATCH 6/9] fix: preserve callback error details in traces (#688) #### Overview Preserve structured callback failure details through OpenTelemetry projection so leaf and turn-level spans report the same error classification and expose the originating binding exception type without parsing status descriptions. - [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. #### Details - Add a structured `CallbackException` flow error that retains Python and JavaScript exception class names while preserving the existing `internal_error` classification. - Emit standard `exception` span events with `exception.type` for the `full` and `gen_ai` projections. - Propagate descendant `error.type` and exception type to errored parent spans, including across scopes suppressed by the GenAI projection. - Match suppressed parent contexts by both trace ID and span ID, with a regression covering colliding span IDs across concurrent traces. - Preserve caller-provided `exception.type` metadata and capture JavaScript exception names even when the exception's message getter throws. - Add Rust projection regressions and Python and Node callback metadata coverage. - Document callback exception projection, parent-span inheritance, and the Rust migration requirement. Breaking change for Rust source compatibility: `FlowError` is exhaustive, so downstream exhaustive matches must handle the new `CallbackException` variant (or add a wildcard) when upgrading. The variant maps to the existing internal failure classification while retaining the binding exception type for observability. Validation: - `cargo fmt --all` - `cargo clippy --workspace --all-targets -- -D warnings` - focused Rust and Node regression tests - `just test-go` - `just docs` - `uv run pre-commit run --all-files` (all hooks passed; the docs link hook was rerun successfully after an initial race with concurrent API reference generation) - `just test-rust`: 1,164/1,165 passed; the single worker-reader timeout passed on isolated rerun - `just test-python`: 603 passed; 11 unrelated plugin tests discovered `/Users/wkillian/.nemo-relay/plugins.toml` - `just test-node`: 339 passed; 10 unrelated plugin tests discovered the same ancestor configuration #### Where should the reviewer start? Start with `crates/core/src/observability/otel.rs`, particularly end-span projection and propagation to the nearest exported parent, then review `failed_descendant_classification_and_exception_propagate_to_agent_span` and `suppressed_parent_error_propagation_isolated_by_trace_id` in the corresponding test module. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes RELAY-603 ## Summary by CodeRabbit * **New Features** * Callback failures now preserve their original exception type and message across JavaScript and Python integrations. * OpenTelemetry traces include exception types, exception events, and propagated descendant error metadata. * Failed operations consistently report internal errors while retaining detailed exception classifications. * **Documentation** * Expanded OpenTelemetry documentation covering callback exceptions, error inheritance, and metadata behavior. * **Tests** * Added coverage for exception metadata in callback, LLM, tool, streaming, and span error scenarios. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Maryam Najafian (https://github.com/mnajafian-nv) URL: https://github.com/NVIDIA/NeMo-Relay/pull/688 --- crates/core/src/api/shared.rs | 5 + crates/core/src/error.rs | 19 +- crates/core/src/observability/otel.rs | 80 ++++- crates/core/src/plugin/dynamic/native.rs | 4 +- crates/core/tests/coverage/error_tests.rs | 6 + .../tests/unit/observability/otel_tests.rs | 310 ++++++++++++++++++ crates/core/tests/unit/shared_tests.rs | 24 ++ crates/ffi/src/error.rs | 4 +- crates/ffi/tests/coverage/error_tests.rs | 7 + crates/node/src/callable.rs | 9 +- crates/node/src/callback_factory.rs | 39 ++- crates/node/src/promise_call.rs | 6 +- crates/node/tests/llm_tests.mjs | 14 +- crates/node/tests/tools_tests.mjs | 7 +- crates/python/src/py_callable.rs | 34 +- .../observability/opentelemetry.mdx | 21 +- python/tests/test_llm.py | 97 +++++- python/tests/test_tools.py | 4 +- 18 files changed, 641 insertions(+), 49 deletions(-) diff --git a/crates/core/src/api/shared.rs b/crates/core/src/api/shared.rs index 2f823a27d..dadd33c69 100644 --- a/crates/core/src/api/shared.rs +++ b/crates/core/src/api/shared.rs @@ -220,6 +220,11 @@ pub(crate) fn metadata_with_otel_error(metadata: Option, error: &FlowError metadata .entry("error.type".to_string()) .or_insert_with(|| Json::String(error.otel_error_type().to_string())); + if let Some(exception_type) = error.exception_type() { + metadata + .entry("exception.type".to_string()) + .or_insert_with(|| Json::String(exception_type.to_string())); + } } metadata } diff --git a/crates/core/src/error.rs b/crates/core/src/error.rs index 270f9737a..23ed1c01a 100644 --- a/crates/core/src/error.rs +++ b/crates/core/src/error.rs @@ -129,6 +129,15 @@ pub enum FlowError { /// An internal runtime error (e.g., lock poisoning). #[error("internal error: {0}")] Internal(String), + + /// An exception raised by a language-binding callback. + #[error("internal error: {message}")] + CallbackException { + /// Original binding-rendered exception message. + message: String, + /// Original language exception class name. + exception_type: String, + }, } /// A specialized [`Result`](std::result::Result) type for NeMo Relay operations. @@ -158,7 +167,15 @@ impl FlowError { UpstreamFailureClass::InvalidRequest => "invalid_request", UpstreamFailureClass::Other => "upstream_error", }, - Self::Internal(_) => "internal_error", + Self::Internal(_) | Self::CallbackException { .. } => "internal_error", + } + } + + /// Returns the originating language exception class, when available. + pub(crate) fn exception_type(&self) -> Option<&str> { + match self { + Self::CallbackException { exception_type, .. } => Some(exception_type), + _ => None, } } } diff --git a/crates/core/src/observability/otel.rs b/crates/core/src/observability/otel.rs index 362ea5ca3..5a80992de 100644 --- a/crates/core/src/observability/otel.rs +++ b/crates/core/src/observability/otel.rs @@ -948,6 +948,8 @@ pub(super) struct ActiveSpan { span_context: SpanContext, start_model_name: Option, projected_attributes: Vec, + descendant_error_type: Option, + descendant_exception_type: Option, } pub(super) struct OtelEventProcessor { @@ -1151,12 +1153,15 @@ impl OtelEventProcessor { span_context, start_model_name, projected_attributes, + descendant_error_type: None, + descendant_exception_type: None, }, ); } fn process_end(&mut self, event: &Event) { let Some(mut active_span) = self.active_spans.remove(&event.uuid()) else { + self.propagate_suppressed_error_metadata(event); return; }; self.record_completed_span_context(event.uuid(), active_span.span_context.clone()); @@ -1167,6 +1172,36 @@ impl OtelEventProcessor { OpenTelemetryType::GenAi => super::otel_genai::end_attributes(event), OpenTelemetryType::OpenInference => super::openinference::end_attributes(event), }; + let is_error = metadata_string(event, "otel.status_code") == Some("ERROR"); + let explicit_error_type = metadata_string(event, "error.type"); + let error_type = is_error.then(|| { + explicit_error_type + .map(ToOwned::to_owned) + .or(active_span.descendant_error_type.take()) + .unwrap_or_else(|| "_OTHER".to_string()) + }); + let exception_type = is_error + .then(|| { + metadata_string(event, "exception.type") + .map(ToOwned::to_owned) + .or(active_span.descendant_exception_type.take()) + }) + .flatten(); + if matches!( + self.otel_type, + OpenTelemetryType::Full | OpenTelemetryType::GenAi + ) && let Some(error_type) = error_type.as_ref() + { + attributes.retain(|attribute| attribute.key.as_str() != "error.type"); + attributes.push(KeyValue::new("error.type", error_type.clone())); + } + if let Some(exception_type) = exception_type.as_ref() { + active_span.span.add_event_with_timestamp( + "exception", + to_system_time(*event.timestamp()), + vec![KeyValue::new("exception.type", exception_type.clone())], + ); + } let end_model_name = model_name_for_llm_event(event).or_else(|| active_span.start_model_name.take()); if self.otel_type == OpenTelemetryType::Full @@ -1187,12 +1222,40 @@ impl OtelEventProcessor { &self.attribute_mappings, )); } + if is_error && let Some(parent_span) = self.find_parent_span_mut(event) { + if parent_span.descendant_error_type.is_none() { + parent_span.descendant_error_type = error_type; + } + if parent_span.descendant_exception_type.is_none() { + parent_span.descendant_exception_type = exception_type; + } + } active_span.span.set_attributes(attributes); active_span .span .end_with_timestamp(to_system_time(*event.timestamp())); } + fn propagate_suppressed_error_metadata(&mut self, event: &Event) { + if self.otel_type != OpenTelemetryType::GenAi + || !self.suppressed_parent_contexts.contains_key(&event.uuid()) + || metadata_string(event, "otel.status_code") != Some("ERROR") + { + return; + } + let error_type = metadata_string(event, "error.type").map(ToOwned::to_owned); + let exception_type = metadata_string(event, "exception.type").map(ToOwned::to_owned); + let Some(parent_span) = self.find_parent_span_mut(event) else { + return; + }; + if parent_span.descendant_error_type.is_none() { + parent_span.descendant_error_type = error_type; + } + if parent_span.descendant_exception_type.is_none() { + parent_span.descendant_exception_type = exception_type; + } + } + fn process_mark(&mut self, event: &Event) { if self.otel_type == OpenTelemetryType::GenAi { return; @@ -1310,9 +1373,16 @@ impl OtelEventProcessor { } fn parent_span_uuid(&self, event: &Event) -> Option { - event - .parent_uuid() - .filter(|uuid| self.active_spans.contains_key(uuid)) + let parent_uuid = event.parent_uuid()?; + if self.active_spans.contains_key(&parent_uuid) { + return Some(parent_uuid); + } + let suppressed_parent = self.suppressed_parent_contexts.get(&parent_uuid)?; + self.active_spans.iter().find_map(|(uuid, active_span)| { + (active_span.span_context.trace_id() == suppressed_parent.trace_id() + && active_span.span_context.span_id() == suppressed_parent.span_id()) + .then_some(*uuid) + }) } fn find_parent_span(&self, event: &Event) -> Option<&ActiveSpan> { @@ -1368,6 +1438,10 @@ impl OtelEventProcessor { } } +fn metadata_string<'a>(event: &'a Event, key: &str) -> Option<&'a str> { + event.metadata()?.get(key)?.as_str() +} + fn span_kind(event: &Event) -> SpanKind { match semantic_scope_type(event) { Some(ScopeType::Llm) => SpanKind::Client, diff --git a/crates/core/src/plugin/dynamic/native.rs b/crates/core/src/plugin/dynamic/native.rs index 3b0124b5b..7fe3d1998 100644 --- a/crates/core/src/plugin/dynamic/native.rs +++ b/crates/core/src/plugin/dynamic/native.rs @@ -1293,7 +1293,9 @@ fn status_from_flow_error(err: FlowError) -> NemoRelayStatus { FlowError::InvalidArgument(_) => NemoRelayStatus::InvalidArg, FlowError::ScopeStackEmpty => NemoRelayStatus::ScopeStackEmpty, FlowError::GuardrailRejected(_) => NemoRelayStatus::GuardrailRejected, - FlowError::Upstream(_) | FlowError::Internal(_) => NemoRelayStatus::Internal, + FlowError::Upstream(_) | FlowError::Internal(_) | FlowError::CallbackException { .. } => { + NemoRelayStatus::Internal + } } } diff --git a/crates/core/tests/coverage/error_tests.rs b/crates/core/tests/coverage/error_tests.rs index 55e823a72..5c4f02356 100644 --- a/crates/core/tests/coverage/error_tests.rs +++ b/crates/core/tests/coverage/error_tests.rs @@ -104,6 +104,12 @@ fn otel_error_type_maps_internal_failures_to_generic_code() { FlowError::Internal("application callback failed".into()).otel_error_type(), "internal_error" ); + let external = FlowError::CallbackException { + message: "ValueError: boom".into(), + exception_type: "ValueError".into(), + }; + assert_eq!(external.otel_error_type(), "internal_error"); + assert_eq!(external.exception_type(), Some("ValueError")); } #[test] diff --git a/crates/core/tests/unit/observability/otel_tests.rs b/crates/core/tests/unit/observability/otel_tests.rs index 88bf5ecea..1d8172947 100644 --- a/crates/core/tests/unit/observability/otel_tests.rs +++ b/crates/core/tests/unit/observability/otel_tests.rs @@ -528,6 +528,27 @@ fn make_end_event( ) } +fn make_end_event_with_metadata( + uuid: Uuid, + parent_uuid: Option, + name: &str, + scope_type: ScopeType, + metadata: Json, +) -> Event { + Event::Scope(ScopeEvent::new( + BaseEvent::builder() + .parent_uuid_opt(parent_uuid) + .uuid(uuid) + .name(name) + .metadata(metadata) + .build(), + ScopeCategory::End, + Vec::new(), + EventCategory::from(scope_type), + None, + )) +} + fn make_scope_event( scope_category: ScopeCategory, uuid: Uuid, @@ -1581,6 +1602,295 @@ fn gen_ai_end_projection_preserves_explicit_error_type() { ); } +#[test] +fn failed_descendant_classification_and_exception_propagate_to_agent_span() { + for otel_type in [OpenTelemetryType::Full, OpenTelemetryType::GenAi] { + let (provider, exporter) = make_provider(); + let mut processor = + OtelEventProcessor::new_with_mark_projection_and_exclusions_and_mappings( + provider, + "error-propagation-test".to_string(), + otel_type, + MarkProjection::default(), + default_mark_exclude_names(), + Vec::new(), + ); + let agent_uuid = Uuid::now_v7(); + let function_uuid = Uuid::now_v7(); + let llm_uuid = Uuid::now_v7(); + processor.process(&make_start_event( + agent_uuid, + None, + "agent", + ScopeType::Agent, + None, + )); + let llm_parent_uuid = if otel_type == OpenTelemetryType::GenAi { + processor.process(&make_start_event( + function_uuid, + Some(agent_uuid), + "function", + ScopeType::Function, + None, + )); + function_uuid + } else { + agent_uuid + }; + processor.process(&make_start_event( + llm_uuid, + Some(llm_parent_uuid), + "chat", + ScopeType::Llm, + None, + )); + processor.process(&make_end_event_with_metadata( + llm_uuid, + Some(llm_parent_uuid), + "chat", + ScopeType::Llm, + json!({ + "otel.status_code": "ERROR", + "otel.status_description": "internal error: ValueError: boom", + "error.type": "internal_error", + "exception.type": "ValueError", + }), + )); + if otel_type == OpenTelemetryType::GenAi { + processor.process(&make_end_event_with_metadata( + function_uuid, + Some(agent_uuid), + "function", + ScopeType::Function, + json!({ + "otel.status_code": "ERROR", + "otel.status_description": "internal error: ValueError: boom", + }), + )); + } + processor.process(&make_end_event_with_metadata( + agent_uuid, + None, + "agent", + ScopeType::Agent, + json!({ + "otel.status_code": "ERROR", + "otel.status_description": "internal error: ValueError: boom", + }), + )); + processor.force_flush().unwrap(); + + let spans = exporter.get_finished_spans().unwrap(); + assert_eq!(spans.len(), 2); + for span in &spans { + assert_eq!( + attr_map(&span.attributes).get("error.type"), + Some(&"internal_error".to_string()) + ); + let exception = span + .events + .events + .iter() + .find(|event| event.name.as_ref() == "exception") + .expect("expected exception event"); + assert_eq!( + attr_map(&exception.attributes).get("exception.type"), + Some(&"ValueError".to_string()) + ); + } + } +} + +#[test] +fn suppressed_function_error_propagates_to_agent_span() { + let (provider, exporter) = make_provider(); + let mut processor = OtelEventProcessor::new_with_mark_projection_and_exclusions_and_mappings( + provider, + "suppressed-error-propagation-test".to_string(), + OpenTelemetryType::GenAi, + MarkProjection::default(), + default_mark_exclude_names(), + Vec::new(), + ); + let agent_uuid = Uuid::now_v7(); + let function_uuid = Uuid::now_v7(); + processor.process(&make_start_event( + agent_uuid, + None, + "agent", + ScopeType::Agent, + None, + )); + processor.process(&make_start_event( + function_uuid, + Some(agent_uuid), + "function", + ScopeType::Function, + None, + )); + processor.process(&make_end_event_with_metadata( + function_uuid, + Some(agent_uuid), + "function", + ScopeType::Function, + json!({ + "otel.status_code": "ERROR", + "error.type": "internal_error", + "exception.type": "ValueError", + }), + )); + processor.process(&make_end_event_with_metadata( + agent_uuid, + None, + "agent", + ScopeType::Agent, + json!({"otel.status_code": "ERROR"}), + )); + processor.force_flush().unwrap(); + + let spans = exporter.get_finished_spans().unwrap(); + assert_eq!(spans.len(), 1); + let agent_span = &spans[0]; + assert_eq!( + attr_map(&agent_span.attributes).get("error.type"), + Some(&"internal_error".to_string()) + ); + let exception = agent_span + .events + .events + .iter() + .find(|event| event.name.as_ref() == "exception") + .expect("expected propagated exception event"); + assert_eq!( + attr_map(&exception.attributes).get("exception.type"), + Some(&"ValueError".to_string()) + ); +} + +#[test] +fn suppressed_parent_error_propagation_isolated_by_trace_id() { + let (provider, exporter) = make_provider(); + let mut processor = OtelEventProcessor::new_with_mark_projection_and_exclusions_and_mappings( + provider, + "error-trace-isolation-test".to_string(), + OpenTelemetryType::GenAi, + MarkProjection::default(), + default_mark_exclude_names(), + Vec::new(), + ); + let shared_span_id = [0xAB; 8]; + let mut first_agent_bytes = [0x11; 16]; + first_agent_bytes[8..].copy_from_slice(&shared_span_id); + let mut second_agent_bytes = [0x22; 16]; + second_agent_bytes[8..].copy_from_slice(&shared_span_id); + let cases = [ + ( + Uuid::from_bytes(first_agent_bytes), + Uuid::now_v7(), + Uuid::now_v7(), + "first_error", + "FirstException", + ), + ( + Uuid::from_bytes(second_agent_bytes), + Uuid::now_v7(), + Uuid::now_v7(), + "second_error", + "SecondException", + ), + ]; + assert_eq!( + relay_span_id(cases[0].0), + relay_span_id(cases[1].0), + "fixture agents must share a span ID" + ); + assert_ne!( + relay_trace_id(cases[0].0), + relay_trace_id(cases[1].0), + "fixture agents must belong to different traces" + ); + + for (agent_uuid, function_uuid, llm_uuid, _, _) in cases { + processor.process(&make_start_event( + agent_uuid, + None, + "agent", + ScopeType::Agent, + None, + )); + processor.process(&make_start_event( + function_uuid, + Some(agent_uuid), + "function", + ScopeType::Function, + None, + )); + processor.process(&make_start_event( + llm_uuid, + Some(function_uuid), + "chat", + ScopeType::Llm, + None, + )); + } + for (agent_uuid, function_uuid, llm_uuid, error_type, exception_type) in cases { + processor.process(&make_end_event_with_metadata( + llm_uuid, + Some(function_uuid), + "chat", + ScopeType::Llm, + json!({ + "otel.status_code": "ERROR", + "error.type": error_type, + "exception.type": exception_type, + }), + )); + processor.process(&make_end_event_with_metadata( + function_uuid, + Some(agent_uuid), + "function", + ScopeType::Function, + json!({"otel.status_code": "ERROR"}), + )); + } + for (agent_uuid, _, _, _, _) in cases { + processor.process(&make_end_event_with_metadata( + agent_uuid, + None, + "agent", + ScopeType::Agent, + json!({"otel.status_code": "ERROR"}), + )); + } + processor.force_flush().unwrap(); + + let spans = exporter.get_finished_spans().unwrap(); + assert_eq!(spans.len(), 4); + for (agent_uuid, _, _, error_type, exception_type) in cases { + let agent_span = spans + .iter() + .find(|span| { + span.span_context.trace_id() == relay_trace_id(agent_uuid) + && span.parent_span_id == SpanId::INVALID + }) + .expect("expected agent span for trace"); + assert_eq!( + attr_map(&agent_span.attributes).get("error.type"), + Some(&error_type.to_string()) + ); + let exception = agent_span + .events + .events + .iter() + .find(|event| event.name.as_ref() == "exception") + .expect("expected propagated exception event"); + assert_eq!( + attr_map(&exception.attributes).get("exception.type"), + Some(&exception_type.to_string()) + ); + } +} + #[test] fn gen_ai_projection_prefers_standard_names_and_normalized_provider_details() { let agent = make_start_event( diff --git a/crates/core/tests/unit/shared_tests.rs b/crates/core/tests/unit/shared_tests.rs index 2249ca9f7..69793a38f 100644 --- a/crates/core/tests/unit/shared_tests.rs +++ b/crates/core/tests/unit/shared_tests.rs @@ -142,6 +142,30 @@ fn test_metadata_with_otel_error_adds_structured_error_type() { .unwrap(); assert_eq!(explicit_metadata["error.type"], json!("provider_timeout")); + + let external_metadata = metadata_with_otel_error( + None, + &FlowError::CallbackException { + message: "ValueError: boom".into(), + exception_type: "ValueError".into(), + }, + ) + .unwrap(); + assert_eq!(external_metadata["error.type"], json!("internal_error")); + assert_eq!(external_metadata["exception.type"], json!("ValueError")); + + let explicit_exception_metadata = metadata_with_otel_error( + Some(json!({"exception.type": "CallerException"})), + &FlowError::CallbackException { + message: "ValueError: boom".into(), + exception_type: "ValueError".into(), + }, + ) + .unwrap(); + assert_eq!( + explicit_exception_metadata["exception.type"], + json!("CallerException") + ); } #[test] diff --git a/crates/ffi/src/error.rs b/crates/ffi/src/error.rs index 207197123..3bfa7a4f2 100644 --- a/crates/ffi/src/error.rs +++ b/crates/ffi/src/error.rs @@ -120,7 +120,9 @@ impl From<&FlowError> for NemoRelayStatus { FlowError::InvalidArgument(_) => NemoRelayStatus::InvalidArg, FlowError::ScopeStackEmpty => NemoRelayStatus::ScopeStackEmpty, FlowError::GuardrailRejected(_) => NemoRelayStatus::GuardrailRejected, - FlowError::Upstream(_) | FlowError::Internal(_) => NemoRelayStatus::Internal, + FlowError::Upstream(_) + | FlowError::Internal(_) + | FlowError::CallbackException { .. } => NemoRelayStatus::Internal, } } } diff --git a/crates/ffi/tests/coverage/error_tests.rs b/crates/ffi/tests/coverage/error_tests.rs index fe2792666..cc5587d47 100644 --- a/crates/ffi/tests/coverage/error_tests.rs +++ b/crates/ffi/tests/coverage/error_tests.rs @@ -70,6 +70,13 @@ fn test_status_from_error_maps_variants_and_sets_message() { FlowError::Internal("boom".into()), NemoRelayStatus::Internal, ), + ( + FlowError::CallbackException { + message: "callback boom".into(), + exception_type: "ValueError".into(), + }, + NemoRelayStatus::Internal, + ), (FlowError::ScopeStackEmpty, NemoRelayStatus::ScopeStackEmpty), ]; diff --git a/crates/node/src/callable.rs b/crates/node/src/callable.rs index b6052059a..e807979be 100644 --- a/crates/node/src/callable.rs +++ b/crates/node/src/callable.rs @@ -163,6 +163,8 @@ struct MiddlewareCallbackResult { value: Json, #[serde(default)] error: String, + #[serde(default, rename = "exceptionType")] + exception_type: String, } /// Wrap a middleware callback so exceptions cross the N-API boundary as data. @@ -207,11 +209,16 @@ pub(crate) fn unwrap_middleware_result(value: Json, error_prefix: &str) -> Resul })?; if result.ok { Ok(result.value) - } else { + } else if result.exception_type.is_empty() { Err(FlowError::Internal(format!( "{error_prefix}: {}", result.error ))) + } else { + Err(FlowError::CallbackException { + message: format!("{error_prefix}: {}", result.error), + exception_type: result.exception_type, + }) } } diff --git a/crates/node/src/callback_factory.rs b/crates/node/src/callback_factory.rs index cb7988849..0990d343e 100644 --- a/crates/node/src/callback_factory.rs +++ b/crates/node/src/callback_factory.rs @@ -165,16 +165,26 @@ const CALLBACK_FACTORIES_SOURCE: &str = r#"(() => { }, (error) => { settlePublication(); let message = 'unknown error'; + let exceptionType = 'Error'; try { if (typeof error === 'string') { message = error; } else if (error === null || (typeof error !== 'object' && typeof error !== 'function')) { message = String(error); - } else if (error != null && typeof error.message === 'string') { - message = error.message; + } else if (error != null) { + const errorMessage = error.message; + if (typeof errorMessage === 'string') { + message = errorMessage; + } } } catch {} - reject(message); + try { + const errorName = error?.name; + if (typeof errorName === 'string' && errorName.length > 0) { + exceptionType = errorName; + } + } catch {} + reject(message, exceptionType); }); }; eventSanitizerContext.run(token, invoke); @@ -188,10 +198,20 @@ const CALLBACK_FACTORIES_SOURCE: &str = r#"(() => { return { ok: true, value: jsonValue(value === undefined ? null : value) }; } catch (error) { let message = 'JavaScript callback failed'; + let exceptionType = 'Error'; try { - message = String(error?.message ?? error); + const errorMessage = error?.message; + if (typeof errorMessage === 'string') { + message = errorMessage; + } } catch {} - return { ok: false, error: message }; + try { + const errorName = error?.name; + if (typeof errorName === 'string' && errorName.length > 0) { + exceptionType = errorName; + } + } catch {} + return { ok: false, error: message, exceptionType }; } }; }, @@ -212,11 +232,18 @@ const CALLBACK_FACTORIES_SOURCE: &str = r#"(() => { ) { if (error != null) { let message = 'unknown error'; + let exceptionType = 'Error'; try { message = String(error?.message ?? error); } catch {} + try { + const errorName = error?.name; + if (typeof errorName === 'string' && errorName.length > 0) { + exceptionType = errorName; + } + } catch {} if (typeof reject === 'function') { - reject(message); + reject(message, exceptionType); } return; } diff --git a/crates/node/src/promise_call.rs b/crates/node/src/promise_call.rs index 892bd6627..7832468d5 100644 --- a/crates/node/src/promise_call.rs +++ b/crates/node/src/promise_call.rs @@ -334,7 +334,11 @@ fn build_completion_unknowns( let message = ctx .get::(0) .unwrap_or_else(|_| "unknown error".to_string()); - completion.send(Err(FlowError::Internal(message))); + let exception_type = ctx.get::(1).unwrap_or_else(|_| "Error".to_string()); + completion.send(Err(FlowError::CallbackException { + message, + exception_type, + })); ctx.env.get_undefined() })?; diff --git a/crates/node/tests/llm_tests.mjs b/crates/node/tests/llm_tests.mjs index a2254af50..a65e3f702 100644 --- a/crates/node/tests/llm_tests.mjs +++ b/crates/node/tests/llm_tests.mjs @@ -90,6 +90,9 @@ function sparseArray() { function unprintableError() { const error = new Error('sanitize request guardrail failed'); Object.defineProperties(error, { + name: { + value: 'GetterError', + }, message: { get() { throw new Error('message getter boom'); @@ -285,11 +288,11 @@ describe('LLM execute', () => { await assert.rejects( () => - llmCallExecuteAsync( + llmCallExecute( 'exec_status_error_llm', makeNative(), - async () => { - throw new Error('llm status failure'); + () => { + throw unprintableError(); }, null, null, @@ -299,7 +302,7 @@ describe('LLM execute', () => { }, null, ), - /llm status failure/, + /JavaScript callback failed/, ); await flushSubscribers(); @@ -320,8 +323,9 @@ describe('LLM execute', () => { assert.ok(errorEnd, 'expected failed llm end event'); assert.equal(errorEnd.metadata.caller, 'node-llm-error'); assert.equal(errorEnd.metadata['otel.status_code'], 'ERROR'); - assert.match(errorEnd.metadata['otel.status_description'], /llm status failure/); + assert.match(errorEnd.metadata['otel.status_description'], /JavaScript callback failed/); assert.equal(errorEnd.metadata['error.type'], 'internal_error'); + assert.equal(errorEnd.metadata['exception.type'], 'GetterError'); } finally { deregisterSubscriber('node_llm_status_metadata_sub'); } diff --git a/crates/node/tests/tools_tests.mjs b/crates/node/tests/tools_tests.mjs index 29a262e91..e1cc7c6a1 100644 --- a/crates/node/tests/tools_tests.mjs +++ b/crates/node/tests/tools_tests.mjs @@ -380,11 +380,11 @@ describe('Tool execute', () => { await assert.rejects( () => - toolCallExecuteAsync( + toolCallExecute( 'exec_status_error_tool', {}, - async () => { - throw new Error('tool status failure'); + () => { + throw new TypeError('tool status failure'); }, null, null, @@ -416,6 +416,7 @@ describe('Tool execute', () => { assert.equal(errorEnd.metadata['otel.status_code'], 'ERROR'); assert.match(errorEnd.metadata['otel.status_description'], /tool status failure/); assert.equal(errorEnd.metadata['error.type'], 'internal_error'); + assert.equal(errorEnd.metadata['exception.type'], 'TypeError'); } finally { deregisterSubscriber('node_tool_status_metadata_sub'); } diff --git a/crates/python/src/py_callable.rs b/crates/python/src/py_callable.rs index 50772a3e6..b73a1dacf 100644 --- a/crates/python/src/py_callable.rs +++ b/crates/python/src/py_callable.rs @@ -59,6 +59,20 @@ use crate::py_types::{ type PyValueFuture = Pin>> + Send>>; +fn python_callback_error(error: PyErr) -> FlowError { + let exception_type = Python::attach(|py| { + error + .get_type(py) + .getattr("__name__") + .and_then(|name| name.extract::()) + .unwrap_or_else(|_| "Exception".to_string()) + }); + FlowError::CallbackException { + message: error.to_string(), + exception_type, + } +} + struct CancellablePyFuture { inner: PyValueFuture, scheduled: Arc>, @@ -296,9 +310,7 @@ async fn resolve_json_or_future( match outcome? { Ok(json) => Ok(json), Err(future) => { - let py_result = future - .await - .map_err(|e| FlowError::Internal(e.to_string()))?; + let py_result = future.await.map_err(python_callback_error)?; Python::attach(|py| { py_to_json(py_result.bind(py)) .map_err(|e: PyErr| FlowError::Internal(e.to_string())) @@ -505,7 +517,7 @@ async fn resolve_py_object_or_future( ) -> FlowResult> { match outcome? { Ok(value) => Ok(value), - Err(future) => future.await.map_err(|e| FlowError::Internal(e.to_string())), + Err(future) => future.await.map_err(python_callback_error), } } @@ -573,7 +585,7 @@ async fn await_async_iter_task_result(task: Py) -> FlowResult context.call_method1("run", (callback.bind(py), py_args)), None => callback.bind(py).call1((py_args,)), } - .map_err(|error| FlowError::Internal(error.to_string()))?; + .map_err(python_callback_error)?; split_json_or_future_with_locals(py, result.unbind(), task_locals.as_ref()) })) .await @@ -1264,7 +1276,7 @@ pub fn wrap_py_llm_stream_exec_intercept_fn( } None => callback.bind(py).call1((py_req, py_next)), } - .map_err(|e: PyErr| FlowError::Internal(e.to_string()))?; + .map_err(python_callback_error)?; let outcome = split_py_object_or_future_with_locals( py, result.unbind(), @@ -1508,7 +1520,7 @@ pub fn wrap_py_llm_exec_fn( Some(context) => context.call_method1("run", (callback.bind(py), py_req)), None => callback.bind(py).call1((py_req,)), } - .map_err(|error| FlowError::Internal(error.to_string()))?; + .map_err(python_callback_error)?; split_json_or_future_with_locals(py, result.unbind(), task_locals.as_ref()) })) .await @@ -1546,7 +1558,7 @@ pub fn wrap_py_llm_stream_exec_fn( Some(context) => context.call_method1("run", (callback.bind(py), py_req)), None => callback.bind(py).call1((py_req,)), } - .map_err(|error| FlowError::Internal(error.to_string()))?; + .map_err(python_callback_error)?; let outcome = split_py_object_or_future_with_locals( py, result.unbind(), @@ -1566,7 +1578,7 @@ pub fn wrap_py_llm_stream_exec_fn( /// The collector is invoked with each intercepted chunk (after stream response /// intercepts have been applied). It receives a single JSON-converted Python /// object argument. If the Python callable raises an exception, it is converted -/// to a `FlowError::Internal` and returned as `Err`, which terminates the +/// to a `FlowError::CallbackException` and returned as `Err`, which terminates the /// stream. If the callable returns normally (including `None`), the collector /// returns `Ok(())`. pub fn wrap_py_collector_fn( @@ -1578,7 +1590,7 @@ pub fn wrap_py_collector_fn( .map_err(|e| FlowError::Internal(format!("collector json_to_py failed: {e}")))?; py_fn .call1(py, (py_chunk,)) - .map_err(|e| FlowError::Internal(format!("Python collector error: {e}")))?; + .map_err(python_callback_error)?; Ok(()) }) }) diff --git a/docs/configure-plugins/observability/opentelemetry.mdx b/docs/configure-plugins/observability/opentelemetry.mdx index e04e4c98b..edfc432d7 100644 --- a/docs/configure-plugins/observability/opentelemetry.mdx +++ b/docs/configure-plugins/observability/opentelemetry.mdx @@ -210,14 +210,23 @@ For managed LLM, tool, and stream failures, NeMo Relay maps structured | Upstream invalid request | `invalid_request` | | Other upstream failure | `upstream_error` | | `Internal` | `internal_error` | +| Binding callback exception | `internal_error` | External application and callback exceptions that do not have a more specific -`FlowError` classification are represented as `Internal` and emit -`internal_error`. NeMo Relay does not inspect error messages to recover Python, -JavaScript, or application-defined exception class names. When no structured -`FlowError` is available, such as a cancellation or dropped execution, the -GenAI projection emits `_OTHER`. Caller-provided `error.type` metadata takes -precedence over the derived mapping. +`FlowError` classification emit `internal_error`. Python and JavaScript callback +boundaries also preserve the exception class separately, and both the `full` +and `gen_ai` projections emit an `exception` span event with `exception.type`. +NeMo Relay does not inspect error messages to recover exception class names. +When an errored parent span has no useful classification of its own, it +inherits the failed descendant's `error.type` and exception type. When no +structured `FlowError` is available, such as a cancellation or dropped +execution, the projection emits `_OTHER`. Caller-provided `error.type` and +`exception.type` metadata take precedence over values derived from `FlowError`. + +`FlowError` is an exhaustive Rust enum. Rust callers upgrading to this release +must handle the new `CallbackException` variant in exhaustive matches. It maps +to the same internal status as `Internal`, while retaining `exception_type` for +observability projection. ## Direct Subscriber diff --git a/python/tests/test_llm.py b/python/tests/test_llm.py index 0065ed786..ba441981b 100644 --- a/python/tests/test_llm.py +++ b/python/tests/test_llm.py @@ -1338,22 +1338,101 @@ async def test_stream_execution_intercept_propagates_direct___anext__error(self) finally: intercepts.deregister_llm_stream_execution("py_llm_stream_direct_error") + async def test_stream_execution_intercept_failure_emits_exception_type(self): + events = [] + subscribers.register("py_llm_stream_intercept_failure_sub", events.append) + + def failing_middleware(request, next): + raise ValueError("stream intercept boom") + + intercepts.register_llm_stream_execution( + "py_llm_stream_failure", + 1, + failing_middleware, + ) + try: + with pytest.raises(RuntimeError, match="stream intercept boom"): + await llm.stream_execute( + "stream_intercept_failure_llm", + make_request(), + lambda request: _single_chunk_stream(), + lambda chunk: None, + lambda: {}, + ) + finally: + intercepts.deregister_llm_stream_execution("py_llm_stream_failure") + await subscribers.flush_async() + subscribers.deregister("py_llm_stream_intercept_failure_sub") + + metadata = _llm_event(events, "stream_intercept_failure_llm", "end").metadata + assert isinstance(metadata, dict) + assert metadata["exception.type"] == "ValueError" + async def test_stream_execute_collector_failure_raises(self): + events = [] + subscribers.register("py_llm_stream_collector_failure_sub", events.append) + def stream_func(request): async def gen(): yield {"token": "hello"} return gen() - stream = await llm.stream_execute( - "stream_collector_fail_llm", - make_request(), - stream_func, - lambda chunk: raise_runtime_error("collector boom"), - lambda: {}, - ) - with pytest.raises(RuntimeError, match="collector boom"): - await anext(stream) + try: + stream = await llm.stream_execute( + "stream_collector_fail_llm", + make_request(), + stream_func, + lambda chunk: raise_runtime_error("collector boom"), + lambda: {}, + ) + with pytest.raises(RuntimeError, match="collector boom"): + await anext(stream) + await subscribers.flush_async() + finally: + subscribers.deregister("py_llm_stream_collector_failure_sub") + + metadata = _llm_event(events, "stream_collector_fail_llm", "end").metadata + assert isinstance(metadata, dict) + assert metadata["exception.type"] == "RuntimeError" + + async def test_stream_execute_callback_failure_emits_exception_type(self): + events = [] + subscribers.register("py_llm_stream_callback_failure_sub", events.append) + + def stream_func(request): + raise ValueError("stream callback boom") + + async def async_stream_func(request): + raise TypeError("async stream callback boom") + + try: + with pytest.raises(RuntimeError, match="stream callback boom"): + await llm.stream_execute( + "stream_callback_fail_llm", + make_request(), + stream_func, + lambda chunk: None, + lambda: {}, + ) + with pytest.raises(RuntimeError, match="async stream callback boom"): + await llm.stream_execute( + "async_stream_callback_fail_llm", + make_request(), + async_stream_func, + lambda chunk: None, + lambda: {}, + ) + await subscribers.flush_async() + finally: + subscribers.deregister("py_llm_stream_callback_failure_sub") + + metadata = _llm_event(events, "stream_callback_fail_llm", "end").metadata + assert isinstance(metadata, dict) + assert metadata["exception.type"] == "ValueError" + metadata = _llm_event(events, "async_stream_callback_fail_llm", "end").metadata + assert isinstance(metadata, dict) + assert metadata["exception.type"] == "TypeError" async def test_stream_execute_finalizer_failure_records_null_output(self): events = [] diff --git a/python/tests/test_tools.py b/python/tests/test_tools.py index 51de3c645..96b72affb 100644 --- a/python/tests/test_tools.py +++ b/python/tests/test_tools.py @@ -208,7 +208,7 @@ async def test_execute_failure_emits_end_event(self): subscribers.register("py_tool_exec_failure_sub", lambda e: events.append(e)) def failing(args): - raise RuntimeError("boom") + raise ValueError("boom") with pytest.raises(RuntimeError, match="boom"): await tools.execute("failing_tool", {"x": 1}, failing) @@ -224,6 +224,8 @@ def failing(args): assert all(e.category == "tool" for e in events) assert events[0].uuid == events[1].uuid assert events[1].data is None + assert events[1].metadata["error.type"] == "internal_error" + assert events[1].metadata["exception.type"] == "ValueError" class TestToolGuardrails: From 842fa38244e446377492384ff5c2a2c678449749 Mon Sep 17 00:00:00 2001 From: Will Killian <2007799+willkill07@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:45:16 -0400 Subject: [PATCH 7/9] feat: initialize binding logging from environment (#691) #### Overview Install a default process-lifetime `LoggingRuntime` when the Python, Node.js, and Go bindings load, with environment control matching the CLI. - [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. #### Details - Add an idempotent core initializer that retains the default logging runtime for the process lifetime. - Initialize operational logging from `NEMO_RELAY_LOG`, `NEMO_RELAY_LOG_STDERR_FORMAT`, and `NEMO_RELAY_LOG_CONFIG_PATH` in Python, Node.js, and Go. - Fail binding startup when logging environment configuration is invalid. - Add subprocess coverage for successful and invalid environment configuration in every binding. - Document automatic binding initialization and environment precedence. - Breaking changes: none. Validation: - Python: 616 passed, 45 skipped. - Node.js: 351 passed. - Go: full suite passed. - FFI: 88 passed in an isolated configuration environment. - `cargo clippy --workspace --all-targets -- -D warnings` passed. - `uv run pre-commit run --all-files` passed. - `just docs` passed. #### Where should the reviewer start? Start with `initialize_default_logging` in `crates/core/src/logging/mod.rs`, then review the Python, Node.js, and Go binding initialization entry points and their subprocess tests. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes RELAY-602 ## Summary by CodeRabbit * **New Features** * Python, Node.js, and Go bindings now automatically initialize operational logging using environment variables or built-in defaults. * Added graceful shutdown APIs that flush pending file-sink records. * Node.js logging remains active across worker environments and shuts down after the final environment closes. * Python logging now shuts down automatically when the interpreter exits. * Existing application loggers are preserved when no explicit logging configuration is provided. * **Bug Fixes** * Invalid logging configuration now reports clear initialization errors. * **Tests** * Added coverage for initialization, shutdown, validation, and file-sink flushing. * **Documentation** * Clarified logging configuration and shutdown requirements for language bindings. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Maryam Najafian (https://github.com/mnajafian-nv) URL: https://github.com/NVIDIA/NeMo-Relay/pull/691 --- crates/core/src/logging/mod.rs | 91 ++++++++++++++- crates/core/tests/coverage/logging_tests.rs | 115 ++++++++++++++++++- crates/ffi/nemo_relay.h | 17 +++ crates/ffi/src/api/mod.rs | 29 +++++ crates/ffi/tests/unit/api_tests.rs | 13 +++ crates/node/src/api/mod.rs | 32 +++++- crates/node/tests/logging_tests.mjs | 121 ++++++++++++++++++++ crates/python/src/lib.rs | 5 + crates/python/src/py_api/mod.rs | 8 ++ docs/reference/operational-logging.mdx | 29 ++++- go/nemo_relay/README.md | 6 + go/nemo_relay/logging_test.go | 111 ++++++++++++++++++ go/nemo_relay/nemo_relay.go | 14 +++ python/nemo_relay/__init__.py | 4 + python/nemo_relay/_native.pyi | 2 + python/tests/test_logging.py | 67 +++++++++++ 16 files changed, 651 insertions(+), 13 deletions(-) create mode 100644 crates/node/tests/logging_tests.mjs create mode 100644 go/nemo_relay/logging_test.go create mode 100644 python/tests/test_logging.py diff --git a/crates/core/src/logging/mod.rs b/crates/core/src/logging/mod.rs index 9b3c7528c..4d06f5806 100644 --- a/crates/core/src/logging/mod.rs +++ b/crates/core/src/logging/mod.rs @@ -13,7 +13,7 @@ mod sink; use std::io::{self, Write}; use std::path::Path; -use std::sync::{Arc, Mutex, MutexGuard}; +use std::sync::{Arc, Mutex, MutexGuard, Weak}; use spdlog::sink::Sink; use spdlog::{Logger, ThreadPool}; @@ -33,6 +33,8 @@ use sink::log_level_filter; pub(crate) use format::format_event_for_test; static LOGGER_LIFECYCLE_LOCK: Mutex<()> = Mutex::new(()); +static DEFAULT_LOGGING_RUNTIME: Mutex> = Mutex::new(None); +static ACTIVE_RELAY_LOGGER: Mutex>> = Mutex::new(None); fn lock_logger_lifecycle() -> MutexGuard<'static, ()> { LOGGER_LIFECYCLE_LOCK @@ -44,6 +46,32 @@ fn log_crate_proxy_is_installed() -> bool { std::ptr::addr_eq(log::logger(), spdlog::log_crate_proxy() as &dyn log::Log) } +fn active_relay_logger_exists() -> bool { + ACTIVE_RELAY_LOGGER + .lock() + .unwrap_or_else(|error| error.into_inner()) + .as_ref() + .is_some_and(|logger| logger.upgrade().is_some()) +} + +fn set_active_relay_logger(logger: &Arc) { + *ACTIVE_RELAY_LOGGER + .lock() + .unwrap_or_else(|error| error.into_inner()) = Some(Arc::downgrade(logger)); +} + +fn clear_active_relay_logger(logger: &Arc) { + let mut active = ACTIVE_RELAY_LOGGER + .lock() + .unwrap_or_else(|error| error.into_inner()); + if active + .as_ref() + .is_some_and(|current| Weak::ptr_eq(current, &Arc::downgrade(logger))) + { + *active = None; + } +} + fn install_log_crate_proxy() -> Result<()> { match spdlog::init_log_crate_proxy() { Ok(()) => Ok(()), @@ -75,17 +103,22 @@ impl LoggingRuntime { /// opened. Dropping the returned runtime flushes sinks and detaches its logger from the /// process-global `log` proxy when it is still installed. pub fn configure(config: LoggingConfig) -> Result { - let root_relay_id = Uuid::now_v7().to_string(); - let (logger, thread_pools) = build_logger(&config, root_relay_id.clone())?; - // Install once per process. Subsequent calls (tests / re-entry) reuse the proxy and swap // the receiver logger. A different global logger would prevent Relay sinks from receiving // `log` facade records, so fail instead of returning a nonfunctional runtime. let _lifecycle = lock_logger_lifecycle(); + Self::configure_with_lifecycle_lock(config) + } + + fn configure_with_lifecycle_lock(config: LoggingConfig) -> Result { + let root_relay_id = Uuid::now_v7().to_string(); + let (logger, thread_pools) = build_logger(&config, root_relay_id.clone())?; + install_log_crate_proxy()?; spdlog::log_crate_proxy().set_logger(Some(Arc::clone(&logger))); spdlog::log_crate_proxy().set_filter(None); log::set_max_level(log_level_filter(config.level)); + set_active_relay_logger(&logger); log::info!( target: "nemo_relay.logging", @@ -154,7 +187,10 @@ impl Drop for LoggingRuntime { if let Some(logger) = detached && !Arc::ptr_eq(&logger, &self.logger) { - spdlog::log_crate_proxy().set_logger(Some(logger)); + spdlog::log_crate_proxy().set_logger(Some(Arc::clone(&logger))); + set_active_relay_logger(&logger); + } else { + clear_active_relay_logger(&self.logger); } } } @@ -170,6 +206,51 @@ pub fn init_logging(config: &LoggingConfig) -> Result { LoggingRuntime::configure(config.clone()) } +/// Installs and retains the default process-wide logging runtime for a language binding. +/// +/// Configuration is resolved from the supported logging environment variables, with built-in +/// defaults when none are present. Repeated initialization in the same linked runtime is a no-op. +#[doc(hidden)] +pub fn initialize_default_logging() -> Result<()> { + let mut runtime = DEFAULT_LOGGING_RUNTIME.lock().map_err(|error| { + FlowError::Internal(format!("default logging runtime lock poisoned: {error}")) + })?; + if runtime.is_none() { + let config = LoggingConfig::from_environment()?; + let uses_default_config = config.is_none(); + let _lifecycle = lock_logger_lifecycle(); + if uses_default_config && active_relay_logger_exists() { + return Ok(()); + } + match LoggingRuntime::configure_with_lifecycle_lock(config.unwrap_or_default()) { + Ok(configured) => *runtime = Some(configured), + // Language bindings initialize logging automatically. When Relay was not explicitly + // configured, defer to an application logger that already owns the process facade. + Err(FlowError::AlreadyExists(_)) if uses_default_config => {} + Err(error) => return Err(error), + } + } + Ok(()) +} + +/// Shuts down and releases the default process-wide logging runtime for a language binding. +/// +/// Repeated shutdown in the same linked runtime is a no-op. The runtime is removed from shared +/// state before its sinks are drained so shutdown does not hold the default-runtime lock. +#[doc(hidden)] +pub fn shutdown_default_logging() -> Result<()> { + let runtime = DEFAULT_LOGGING_RUNTIME + .lock() + .map_err(|error| { + FlowError::Internal(format!("default logging runtime lock poisoned: {error}")) + })? + .take(); + if let Some(runtime) = runtime { + runtime.shutdown(); + } + Ok(()) +} + #[cfg(test)] #[path = "../../tests/coverage/logging_tests.rs"] mod tests; diff --git a/crates/core/tests/coverage/logging_tests.rs b/crates/core/tests/coverage/logging_tests.rs index 430c4f3b6..6ecf30e6a 100644 --- a/crates/core/tests/coverage/logging_tests.rs +++ b/crates/core/tests/coverage/logging_tests.rs @@ -4,7 +4,7 @@ use crate::logging::{ FileLogRotationConfig, FileLogSinkConfig, LogFormat, LogLevel, LogSinkConfig, LoggingConfig, LoggingRuntime, MAX_FILE_SINK_QUEUE_ENTRIES, MAX_FILE_SINK_RETAINED_FILES, build_logger, - format_event_for_test, init_logging, + format_event_for_test, init_logging, initialize_default_logging, shutdown_default_logging, }; use opentelemetry::trace::{Span as _, Tracer as _, TracerProvider as _}; use opentelemetry_sdk::error::OTelSdkResult; @@ -192,6 +192,105 @@ queue_capacity = 16 assert_eq!(record["fields"]["source"], "toml"); } +#[test] +fn default_logging_runtime_initializes_once_and_shuts_down_idempotently() { + let temp = tempfile::tempdir().unwrap(); + let config_path = temp.path().join("logging.toml"); + let log_path = temp.path().join("relay.log.jsonl"); + std::fs::write( + &config_path, + format!( + r#" +[logging] +level = "info" +stderr_format = "human" +flush_interval_millis = 0 + +[[logging.sinks]] +path = {} +level = "info" +format = "jsonl" +queue_capacity = 16 +"#, + toml_basic_string(log_path.to_string_lossy().as_ref()) + ), + ) + .unwrap(); + let _environment = LoggingEnvScope::set(&[ + ("NEMO_RELAY_LOG", None), + ("NEMO_RELAY_LOG_STDERR_FORMAT", None), + ("NEMO_RELAY_LOG_CONFIG_PATH", Some(config_path.as_os_str())), + ]); + + shutdown_default_logging().unwrap(); + initialize_default_logging().unwrap(); + initialize_default_logging().unwrap(); + shutdown_default_logging().unwrap(); + shutdown_default_logging().unwrap(); + + let records = std::fs::read_to_string(log_path) + .unwrap() + .lines() + .map(|line| serde_json::from_str::(line).expect("valid JSONL lifecycle record")) + .collect::>(); + assert_eq!( + records + .iter() + .filter(|record| record["event"] == "logging_initialized") + .count(), + 1 + ); + assert_eq!( + records + .iter() + .filter(|record| record["event"] == "logging_shutdown_started") + .count(), + 1 + ); +} + +#[test] +fn implicit_default_logging_preserves_an_existing_relay_logger() { + let _environment = LoggingEnvScope::set(&[ + ("NEMO_RELAY_LOG", None), + ("NEMO_RELAY_LOG_STDERR_FORMAT", None), + ("NEMO_RELAY_LOG_CONFIG_PATH", None), + ]); + shutdown_default_logging().unwrap(); + let host_runtime = init_logging(&default_config()).unwrap(); + + initialize_default_logging().unwrap(); + shutdown_default_logging().unwrap(); + + let receiver = spdlog::log_crate_proxy().swap_logger(None); + let preserves_host_logger = receiver + .as_ref() + .is_some_and(|receiver| Arc::ptr_eq(receiver, &host_runtime.logger)); + spdlog::log_crate_proxy().set_logger(receiver); + assert!( + preserves_host_logger, + "implicit binding startup must preserve the host Relay logger" + ); + drop(host_runtime); +} + +#[test] +fn default_logging_runtime_rejects_invalid_environment() { + let _environment = LoggingEnvScope::set(&[ + ("NEMO_RELAY_LOG", Some(OsStr::new(""))), + ("NEMO_RELAY_LOG_STDERR_FORMAT", None), + ("NEMO_RELAY_LOG_CONFIG_PATH", None), + ]); + + shutdown_default_logging().unwrap(); + let error = initialize_default_logging().unwrap_err().to_string(); + + assert!( + error.contains("NEMO_RELAY_LOG must not be empty"), + "{error}" + ); +} + #[test] fn logging_config_from_environment_resolves_direct_settings() { let _environment = LoggingEnvScope::set(&[ @@ -1581,6 +1680,17 @@ fn configure_rejects_preinstalled_foreign_logger() { .contains("process-global log facade is already initialized by another logger"), "{error}" ); + initialize_default_logging() + .expect("unconfigured default logging should defer to the foreign logger"); + unsafe { std::env::set_var("NEMO_RELAY_LOG", "info") }; + let error = initialize_default_logging() + .expect_err("explicit default logging should reject the foreign logger"); + assert!( + error + .to_string() + .contains("process-global log facade is already initialized by another logger"), + "{error}" + ); return; } @@ -1589,6 +1699,9 @@ fn configure_rejects_preinstalled_foreign_logger() { let output = std::process::Command::new(std::env::current_exe().unwrap()) .args(["--exact", test_name, "--nocapture"]) .env(FOREIGN_LOGGER_CHILD_ENV, "1") + .env_remove("NEMO_RELAY_LOG") + .env_remove("NEMO_RELAY_LOG_STDERR_FORMAT") + .env_remove("NEMO_RELAY_LOG_CONFIG_PATH") .output() .expect("foreign logger child test should start"); diff --git a/crates/ffi/nemo_relay.h b/crates/ffi/nemo_relay.h index 9a17fe485..112286a8b 100644 --- a/crates/ffi/nemo_relay.h +++ b/crates/ffi/nemo_relay.h @@ -445,6 +445,23 @@ typedef char *(*NemoRelayToolExecInterceptCb)(void *user_data, */ typedef char *(*NemoRelayToolExecCb)(void *user_data, const char *args_json); +/** + * Initializes the Go binding runtime and installs default operational logging. + * + * Logging configuration is resolved from `NEMO_RELAY_LOG`, + * `NEMO_RELAY_LOG_STDERR_FORMAT`, or `NEMO_RELAY_LOG_CONFIG_PATH`, with built-in defaults when + * none are set. Repeated initialization is a no-op. + */ +NemoRelayStatus nemo_relay_initialize_default_logging(void); + +/** + * Shuts down and releases the default operational logging runtime. + * + * Pending file-sink records are drained before this function returns. Repeated shutdown is a + * no-op. + */ +NemoRelayStatus nemo_relay_shutdown_default_logging(void); + /** * Run the registered tool request intercept chain on the given arguments. * diff --git a/crates/ffi/src/api/mod.rs b/crates/ffi/src/api/mod.rs index cd08d8d03..848ad7243 100644 --- a/crates/ffi/src/api/mod.rs +++ b/crates/ffi/src/api/mod.rs @@ -99,6 +99,35 @@ fn tokio_runtime() -> &'static Runtime { }) } +/// Initializes the Go binding runtime and installs default operational logging. +/// +/// Logging configuration is resolved from `NEMO_RELAY_LOG`, +/// `NEMO_RELAY_LOG_STDERR_FORMAT`, or `NEMO_RELAY_LOG_CONFIG_PATH`, with built-in defaults when +/// none are set. Repeated initialization is a no-op. +#[unsafe(no_mangle)] +pub extern "C" fn nemo_relay_initialize_default_logging() -> NemoRelayStatus { + clear_last_error(); + let result = nemo_relay::shared_runtime::initialize_shared_runtime_binding("go") + .and_then(|()| nemo_relay::logging::initialize_default_logging()); + match result { + Ok(()) => NemoRelayStatus::Ok, + Err(error) => status_from_error(&error), + } +} + +/// Shuts down and releases the default operational logging runtime. +/// +/// Pending file-sink records are drained before this function returns. Repeated shutdown is a +/// no-op. +#[unsafe(no_mangle)] +pub extern "C" fn nemo_relay_shutdown_default_logging() -> NemoRelayStatus { + clear_last_error(); + match nemo_relay::logging::shutdown_default_logging() { + Ok(()) => NemoRelayStatus::Ok, + Err(error) => status_from_error(&error), + } +} + fn block_on_sync_ffi(future: F) -> FlowResult where T: Send, diff --git a/crates/ffi/tests/unit/api_tests.rs b/crates/ffi/tests/unit/api_tests.rs index 6f9eac085..e99c1b5ca 100644 --- a/crates/ffi/tests/unit/api_tests.rs +++ b/crates/ffi/tests/unit/api_tests.rs @@ -243,6 +243,19 @@ unsafe fn fresh_scope_stack() -> *mut FfiScopeStack { stack } +#[test] +fn default_logging_shutdown_is_idempotent() { + let _guard = lock_unpoisoned(&TEST_MUTEX); + assert_status!( + api::nemo_relay_shutdown_default_logging(), + NemoRelayStatus::Ok + ); + assert_status!( + api::nemo_relay_shutdown_default_logging(), + NemoRelayStatus::Ok + ); +} + #[test] fn propagation_context_json_round_trips_through_the_ffi() { let _guard = lock_unpoisoned(&TEST_MUTEX); diff --git a/crates/node/src/api/mod.rs b/crates/node/src/api/mod.rs index 9daec5dad..c81958ef8 100644 --- a/crates/node/src/api/mod.rs +++ b/crates/node/src/api/mod.rs @@ -16,7 +16,7 @@ use std::pin::Pin; use std::ptr; use std::sync::Arc; use std::sync::Mutex as StdMutex; -use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering}; use std::task::{Context, Poll}; use chrono::{DateTime, Utc}; @@ -88,6 +88,29 @@ use crate::promise_call::with_publication_callback_context; use crate::stream::LlmStream; use crate::types::{LlmHandle, ScopeHandle, ScopeStack, ScopeType, ToolHandle}; +static NODE_ENVIRONMENT_COUNT: AtomicUsize = AtomicUsize::new(0); +static NODE_ENVIRONMENT_LIFECYCLE_LOCK: StdMutex<()> = StdMutex::new(()); + +fn register_node_environment() -> FlowResult<()> { + let _guard = NODE_ENVIRONMENT_LIFECYCLE_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + nemo_relay::logging::initialize_default_logging()?; + NODE_ENVIRONMENT_COUNT.fetch_add(1, Ordering::AcqRel); + Ok(()) +} + +fn cleanup_node_environment() { + let _guard = NODE_ENVIRONMENT_LIFECYCLE_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + if NODE_ENVIRONMENT_COUNT.fetch_sub(1, Ordering::AcqRel) == 1 + && let Err(error) = nemo_relay::logging::shutdown_default_logging() + { + eprintln!("nemo-relay: operational logging shutdown failed: {error}"); + } +} + fn effective_scope_context( env: &Env, ) -> napi::Result<( @@ -127,7 +150,12 @@ fn init() { #[cfg(not(test))] #[napi_derive::module_exports] -fn install_well_known_symbol_methods(exports: JsObject, env: Env) -> napi::Result<()> { +fn install_well_known_symbol_methods(exports: JsObject, mut env: Env) -> napi::Result<()> { + register_node_environment().map_err(to_napi_err)?; + if let Err(error) = env.add_env_cleanup_hook((), |_| cleanup_node_environment()) { + cleanup_node_environment(); + return Err(error); + } let activation: JsFunction = exports.get_named_property("DynamicPluginActivation")?; let activation = activation.coerce_to_object()?; let mut prototype: JsObject = activation.get_named_property("prototype")?; diff --git a/crates/node/tests/logging_tests.mjs b/crates/node/tests/logging_tests.mjs new file mode 100644 index 000000000..2e4c8170b --- /dev/null +++ b/crates/node/tests/logging_tests.mjs @@ -0,0 +1,121 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const packageDirectory = fileURLToPath(new URL('..', import.meta.url)); +const loggingEnvironmentNames = ['NEMO_RELAY_LOG', 'NEMO_RELAY_LOG_STDERR_FORMAT', 'NEMO_RELAY_LOG_CONFIG_PATH']; + +function requireBinding(loggingEnvironment, source = "require('./index.js')") { + const environment = { ...process.env }; + for (const name of loggingEnvironmentNames) { + delete environment[name]; + } + Object.assign(environment, loggingEnvironment); + return spawnSync(process.execPath, ['-e', source], { + cwd: packageDirectory, + encoding: 'utf8', + env: environment, + }); +} + +describe('operational logging', () => { + it('initializes from the logging environment', () => { + const result = requireBinding({ + NEMO_RELAY_LOG: 'info', + NEMO_RELAY_LOG_STDERR_FORMAT: 'jsonl', + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stderr, /"event":"logging_initialized"/); + }); + + it('rejects an invalid logging environment', () => { + const result = requireBinding({ NEMO_RELAY_LOG: '' }); + + assert.notEqual(result.status, 0); + assert.match(result.stderr, /NEMO_RELAY_LOG must not be empty/); + }); + + it('flushes file sinks during environment cleanup', () => { + const directory = mkdtempSync(join(tmpdir(), 'nemo-relay-node-logging-')); + try { + const configPath = join(directory, 'logging.toml'); + const logPath = join(directory, 'operational.jsonl'); + writeFileSync( + configPath, + `[logging] +level = "info" +stderr_format = "human" +flush_interval_millis = 0 + +[[logging.sinks]] +path = ${JSON.stringify(logPath)} +level = "info" +format = "jsonl" +queue_capacity = 16 +`, + ); + + const result = requireBinding({ NEMO_RELAY_LOG_CONFIG_PATH: configPath }); + + assert.equal(result.status, 0, result.stderr); + assert.match(readFileSync(logPath, 'utf8'), /"event":"logging_shutdown_started"/); + } finally { + rmSync(directory, { recursive: true, force: true }); + } + }); + + it('keeps logging active while another Node environment remains', () => { + const directory = mkdtempSync(join(tmpdir(), 'nemo-relay-node-worker-logging-')); + try { + const configPath = join(directory, 'logging.toml'); + const logPath = join(directory, 'operational.jsonl'); + writeFileSync( + configPath, + `[logging] +level = "info" +stderr_format = "human" +flush_interval_millis = 0 + +[[logging.sinks]] +path = ${JSON.stringify(logPath)} +level = "info" +format = "jsonl" +queue_capacity = 16 +`, + ); + const workerSource = `require(${JSON.stringify(join(packageDirectory, 'index.js'))})`; + const source = ` +const { Worker } = require('node:worker_threads'); +const relay = require('./index.js'); +const worker = new Worker(${JSON.stringify(workerSource)}, { + eval: true, +}); +worker.once('error', (error) => { + console.error(error); + process.exitCode = 1; +}); +worker.once('exit', (code) => { + if (code !== 0) process.exitCode = code; + relay.deregisterPlugin('adaptive'); +}); +`; + + const result = requireBinding({ NEMO_RELAY_LOG_CONFIG_PATH: configPath }, source); + + assert.equal(result.status, 0, result.stderr); + const output = readFileSync(logPath, 'utf8'); + assert.match(output, /"event":"plugin_deregistered"/); + assert.match(output, /"event":"logging_shutdown_started"/); + } finally { + rmSync(directory, { recursive: true, force: true }); + } + }); +}); diff --git a/crates/python/src/lib.rs b/crates/python/src/lib.rs index 84ebb2295..a5f7ed04b 100644 --- a/crates/python/src/lib.rs +++ b/crates/python/src/lib.rs @@ -50,6 +50,11 @@ fn _native(m: &Bound<'_, PyModule>) -> PyResult<()> { "failed to initialize NeMo Relay runtime ownership: {e}" )) })?; + nemo_relay::logging::initialize_default_logging().map_err(|e| { + pyo3::exceptions::PyRuntimeError::new_err(format!( + "failed to initialize NeMo Relay operational logging: {e}" + )) + })?; register_adaptive_component().map_err(|e| { pyo3::exceptions::PyRuntimeError::new_err(format!( "failed to register adaptive plugin component: {e}" diff --git a/crates/python/src/py_api/mod.rs b/crates/python/src/py_api/mod.rs index d09933a0f..54f801702 100644 --- a/crates/python/src/py_api/mod.rs +++ b/crates/python/src/py_api/mod.rs @@ -62,6 +62,12 @@ fn to_py_err(e: FlowError) -> PyErr { PyErr::new::(e.to_string()) } +#[pyfunction(name = "_shutdown_default_logging")] +fn py_shutdown_default_logging(py: Python<'_>) -> PyResult<()> { + py.detach(nemo_relay::logging::shutdown_default_logging) + .map_err(to_py_err) +} + fn python_event_loop_running(py: Python<'_>) -> PyResult { match py.import("asyncio")?.call_method0("get_running_loop") { Ok(_) => Ok(true), @@ -2083,6 +2089,8 @@ fn scope_deregister_subscriber(scope_uuid: &str, name: &str) -> PyResult { /// Register all API functions into the given `PyModule`. pub fn register(m: &Bound<'_, PyModule>) -> PyResult<()> { + m.add_function(wrap_pyfunction!(py_shutdown_default_logging, m)?)?; + // Scope stack creation / binding / query m.add_function(wrap_pyfunction!(create_scope_stack, m)?)?; m.add_function(wrap_pyfunction!(capture_propagation_context, m)?)?; diff --git a/docs/reference/operational-logging.mdx b/docs/reference/operational-logging.mdx index 942a75149..c3912f29f 100644 --- a/docs/reference/operational-logging.mdx +++ b/docs/reference/operational-logging.mdx @@ -29,7 +29,7 @@ Use the source that matches how Relay is launched: | Use Case | Configuration Source | | --- | --- | | Run the Relay CLI with temporary settings | `--log-*` options | -| Configure a process without a Relay config file | `NEMO_RELAY_LOG*` environment variables | +| Configure a language binding or CLI process | `NEMO_RELAY_LOG*` environment variables | | Reuse logging settings across runs | `[logging]` in TOML | | Embed Relay in a Rust application | `LoggingConfig` and `LoggingRuntime` | @@ -40,8 +40,10 @@ For CLI processes, Relay selects one source in this order: 3. `[logging]` in the resolved Relay `config.toml` 4. Built-in defaults -Sources are selected rather than merged. Rust applications explicitly choose -which `LoggingRuntime` initialization method to use and do not apply the CLI +Sources are selected rather than merged. Python, Node.js, and Go install a +process-lifetime `LoggingRuntime` when the binding loads, using environment +configuration or built-in defaults. Rust applications explicitly choose which +`LoggingRuntime` initialization method to use and do not apply the CLI precedence rules. ## CLI Options @@ -63,8 +65,9 @@ Do not combine `--log-config-path` with `--log-level` or ## Environment Variables -Set these variables for the CLI or a Rust application that initializes logging -with `LoggingRuntime::configure_from_environment()`: +Set these variables for a Python, Node.js, or Go process, the CLI, or a Rust +application that initializes logging with +`LoggingRuntime::configure_from_environment()`: ```bash export NEMO_RELAY_LOG=debug @@ -85,6 +88,22 @@ export NEMO_RELAY_LOG_CONFIG_PATH=/absolute/path/to/logging.toml `NEMO_RELAY_LOG_CONFIG_PATH` cannot be combined with the other logging environment variables. +When none of these variables are set, Python, Node.js, and Go install Relay's +built-in default logger unless the host already owns Rust's process-global +`log` facade. They also preserve an existing Relay logger rather than replacing +it. Records emitted before any logger is installed are discarded, and a host +cannot install its own logger after Relay has claimed the facade. Set one of +these variables when Relay must configure its own logging. + +This binding behavior differs from +`LoggingRuntime::configure_from_environment()`, which always attempts to +install Relay's built-in defaults when no variables are set. + +Python and Node.js drain pending file-sink records during normal runtime +teardown. Go applications that configure file sinks must call +`nemo_relay.ShutdownLogging` before `main` returns; defer it near the start of +`main` so it runs after other Relay cleanup. + ## TOML Configuration Logging settings use a `[logging]` table: diff --git a/go/nemo_relay/README.md b/go/nemo_relay/README.md index 57bbca94c..5162c1bf9 100644 --- a/go/nemo_relay/README.md +++ b/go/nemo_relay/README.md @@ -110,6 +110,12 @@ import ( ) func main() { + defer func() { + if err := nemo.ShutdownLogging(); err != nil { + log.Printf("shut down NeMo Relay logging: %v", err) + } + }() + if err := nemo.RegisterSubscriber("printer", func(event nemo.Event) { fmt.Printf("%s %s\n", event.Kind(), event.Name()) fmt.Println(string(event.JSON())) diff --git a/go/nemo_relay/logging_test.go b/go/nemo_relay/logging_test.go new file mode 100644 index 000000000..09af8e799 --- /dev/null +++ b/go/nemo_relay/logging_test.go @@ -0,0 +1,111 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package nemo_relay + +import ( + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" +) + +const loggingHelperEnvironment = "NEMO_RELAY_TEST_LOGGING_HELPER" + +var loggingEnvironmentNames = map[string]struct{}{ + "NEMO_RELAY_LOG": {}, + "NEMO_RELAY_LOG_STDERR_FORMAT": {}, + "NEMO_RELAY_LOG_CONFIG_PATH": {}, +} + +func loggingTestEnvironment(values ...string) []string { + environment := make([]string, 0, len(os.Environ())+len(values)) + for _, value := range os.Environ() { + name, _, _ := strings.Cut(value, "=") + if _, isLoggingEnvironment := loggingEnvironmentNames[name]; !isLoggingEnvironment { + environment = append(environment, value) + } + } + return append(environment, values...) +} + +func TestBindingLoggingEnvironment(t *testing.T) { + if helper := os.Getenv(loggingHelperEnvironment); helper != "" { + if helper == "shutdown" { + if err := ShutdownLogging(); err != nil { + t.Fatalf("logging shutdown failed: %v", err) + } + } + return + } + + t.Run("initializes from environment", func(t *testing.T) { + command := exec.Command(os.Args[0], "-test.run=TestBindingLoggingEnvironment") + command.Env = loggingTestEnvironment( + loggingHelperEnvironment+"=shutdown", + "NEMO_RELAY_LOG=info", + "NEMO_RELAY_LOG_STDERR_FORMAT=jsonl", + ) + output, err := command.CombinedOutput() + if err != nil { + t.Fatalf("binding import failed: %v\n%s", err, output) + } + if !strings.Contains(string(output), `"event":"logging_initialized"`) { + t.Fatalf("logging initialization event missing from output:\n%s", output) + } + }) + + t.Run("rejects invalid environment", func(t *testing.T) { + command := exec.Command(os.Args[0], "-test.run=TestBindingLoggingEnvironment") + command.Env = loggingTestEnvironment( + loggingHelperEnvironment+"=1", + "NEMO_RELAY_LOG=", + ) + output, err := command.CombinedOutput() + if err == nil { + t.Fatalf("binding initialization unexpectedly succeeded:\n%s", output) + } + if !strings.Contains(string(output), "NEMO_RELAY_LOG must not be empty") { + t.Fatalf("logging initialization error missing from output:\n%s", output) + } + }) + + t.Run("flushes file sink during shutdown", func(t *testing.T) { + directory := t.TempDir() + configPath := filepath.Join(directory, "logging.toml") + logPath := filepath.Join(directory, "operational.jsonl") + config := `[logging] +level = "info" +stderr_format = "human" +flush_interval_millis = 0 + +[[logging.sinks]] +path = ` + strconv.Quote(logPath) + ` +level = "info" +format = "jsonl" +queue_capacity = 16 +` + if err := os.WriteFile(configPath, []byte(config), 0o600); err != nil { + t.Fatalf("write logging config: %v", err) + } + + command := exec.Command(os.Args[0], "-test.run=TestBindingLoggingEnvironment") + command.Env = loggingTestEnvironment( + loggingHelperEnvironment+"=shutdown", + "NEMO_RELAY_LOG_CONFIG_PATH="+configPath, + ) + output, err := command.CombinedOutput() + if err != nil { + t.Fatalf("binding logging shutdown failed: %v\n%s", err, output) + } + contents, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read operational log: %v", err) + } + if !strings.Contains(string(contents), `"event":"logging_shutdown_started"`) { + t.Fatalf("logging shutdown event missing from file:\n%s", contents) + } + }) +} diff --git a/go/nemo_relay/nemo_relay.go b/go/nemo_relay/nemo_relay.go index 3046120f5..8646c122a 100644 --- a/go/nemo_relay/nemo_relay.go +++ b/go/nemo_relay/nemo_relay.go @@ -46,6 +46,8 @@ typedef struct NemoRelayLlmSanitizeResponseContext { uint32_t codec_kind; const typedef void (*NemoRelayFreeFn)(void* user_data); // Core API +extern int32_t nemo_relay_initialize_default_logging(void); +extern int32_t nemo_relay_shutdown_default_logging(void); extern int32_t nemo_relay_get_handle(FfiScopeHandle** out); extern int32_t nemo_relay_push_scope(const char* name, int32_t scope_type, const FfiScopeHandle* parent, uint32_t attributes, const char* data_json, const char* metadata_json, const char* input_json, const int64_t* timestamp_unix_micros, FfiScopeHandle** out); extern int32_t nemo_relay_pop_scope(const FfiScopeHandle* handle, const char* output_json, const char* metadata_json, const int64_t* timestamp_unix_micros); @@ -298,6 +300,18 @@ import ( const defaultServiceName = "nemo-relay" +func init() { + if err := checkStatus(C.nemo_relay_initialize_default_logging()); err != nil { + panic(fmt.Sprintf("failed to initialize NeMo Relay operational logging: %v", err)) + } +} + +// ShutdownLogging drains pending operational log records and releases the default logging runtime. +// Callers that configure file sinks should defer ShutdownLogging from main. +func ShutdownLogging() error { + return checkStatus(C.nemo_relay_shutdown_default_logging()) +} + func checkedValue[T any](status int32, value T) (T, error) { if err := checkStatus(C.int32_t(status)); err != nil { var zero T diff --git a/python/nemo_relay/__init__.py b/python/nemo_relay/__init__.py index 585fbe683..1fa187220 100644 --- a/python/nemo_relay/__init__.py +++ b/python/nemo_relay/__init__.py @@ -77,6 +77,7 @@ async def main(): from __future__ import annotations +import atexit import contextvars import typing from collections.abc import Callable as AbcCallable @@ -119,6 +120,7 @@ async def main(): ToolAttributes, ToolExecutionInterceptOutcome, ToolHandle, + _shutdown_default_logging, ) from nemo_relay._native import ( capture_propagation_context as _capture_propagation_context, @@ -136,6 +138,8 @@ async def main(): from nemo_relay._native import set_thread_scope_stack as _set_thread_scope_stack from nemo_relay._native import sync_thread_scope_stack as _sync_thread_scope_stack +atexit.register(_shutdown_default_logging) + #: Scalar JSON leaf values accepted in NeMo Relay payloads. This alias has no #: runtime behavior; it exists to document and type JSON-compatible public API #: arguments and return values. diff --git a/python/nemo_relay/_native.pyi b/python/nemo_relay/_native.pyi index 43f2fa5f1..cc025751c 100644 --- a/python/nemo_relay/_native.pyi +++ b/python/nemo_relay/_native.pyi @@ -32,6 +32,8 @@ _JsonObject: TypeAlias = dict[str, _JsonValue] _Json: TypeAlias = _JsonValue _MessageContent: TypeAlias = str | Sequence[Mapping[str, _JsonValue]] +def _shutdown_default_logging() -> None: ... + class _EventSanitizeFields(TypedDict): data: _Json | None category_profile: _JsonObject | None diff --git a/python/tests/test_logging.py b/python/tests/test_logging.py new file mode 100644 index 000000000..9312a7ac9 --- /dev/null +++ b/python/tests/test_logging.py @@ -0,0 +1,67 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +import json +import os +import subprocess +import sys + +_LOG_ENVIRONMENT = ( + "NEMO_RELAY_LOG", + "NEMO_RELAY_LOG_STDERR_FORMAT", + "NEMO_RELAY_LOG_CONFIG_PATH", +) + + +def _import_nemo_relay(**logging_environment: str) -> subprocess.CompletedProcess[str]: + environment = os.environ.copy() + for name in _LOG_ENVIRONMENT: + environment.pop(name, None) + environment.update(logging_environment) + return subprocess.run( + [sys.executable, "-c", "import nemo_relay"], + check=False, + capture_output=True, + text=True, + env=environment, + ) + + +def test_binding_initializes_logging_from_environment(): + completed = _import_nemo_relay( + NEMO_RELAY_LOG="info", + NEMO_RELAY_LOG_STDERR_FORMAT="jsonl", + ) + + assert completed.returncode == 0, completed.stderr + assert '"event":"logging_initialized"' in completed.stderr + + +def test_binding_rejects_invalid_logging_environment(): + completed = _import_nemo_relay(NEMO_RELAY_LOG="") + + assert completed.returncode != 0 + assert "NEMO_RELAY_LOG must not be empty" in completed.stderr + + +def test_binding_flushes_file_sink_during_shutdown(tmp_path): + config_path = tmp_path / "logging.toml" + log_path = tmp_path / "operational.jsonl" + config_path.write_text( + f"""[logging] +level = "info" +stderr_format = "human" +flush_interval_millis = 0 + +[[logging.sinks]] +path = {json.dumps(str(log_path))} +level = "info" +format = "jsonl" +queue_capacity = 16 +""" + ) + + completed = _import_nemo_relay(NEMO_RELAY_LOG_CONFIG_PATH=str(config_path)) + + assert completed.returncode == 0, completed.stderr + assert '"event":"logging_shutdown_started"' in log_path.read_text() From 05700e140deee8a27ed057dbe23943f8236a4104 Mon Sep 17 00:00:00 2001 From: Will Killian <2007799+willkill07@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:52:46 -0400 Subject: [PATCH 8/9] fix: surface ATIF delivery failures consistently (#687) #### Overview Reject non-string ATIF filename metadata instead of silently routing trajectories through a fallback, and make remote-delivery teardown failures consistently observable and drainable. - [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. #### Details - Reject present non-string metadata values even when a filename fallback is configured, while retaining fallback behavior for missing and null values. - Preserve remote-upload diagnostics in `plugin.report()` after subscriber delivery is flushed. - Remove the misleading registration-removal wrapper from ATIF delivery errors. - Reset completed Python asynchronous clear state so a failed clear is drainable and a later clear or initialization succeeds. - Add Rust and Python regression coverage and document the resulting filename, reporting, and teardown contract. Validation: - Focused Rust filename-rendering, runtime-diagnostic, HTTP retry, and Python-binding teardown tests passed. - The Python end-to-end non-string metadata and drainable-clear regression passed against `release/0.7`. - `cargo fmt`, workspace clippy, cargo check, changed-file pre-commit checks, Go tests, and documentation checks passed. - Full Rust, Python, and Node runs were also exercised; local failures were limited to assertions receiving the inherited-configuration warning from `/Users/wkillian/.nemo-relay/plugins.toml`, followed by global-state cascades where those tests aborted before cleanup. #### Where should the reviewer start? Start with `render_atif_filename` in `crates/core/src/observability/plugin_component.rs`, then the end-to-end regression in `python/tests/test_observability_plugin.py`. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes: RELAY-638 ## Summary by CodeRabbit - **Bug Fixes** - Improved observability plugin error reporting for invalid, non-string, or nested metadata in destination filenames. - Preserved delivery diagnostics through teardown, recovery, and failed reconfiguration. - Prevented overlapping asynchronous cleanup operations from overwriting newer plugin state. - Distinguished delivery errors from registration leaks, incomplete removal, and fatal shutdown failures. - **Documentation** - Clarified filename fallback behavior, diagnostic persistence, teardown reporting, and recovery outcomes. - **Tests** - Expanded coverage for invalid metadata, failed delivery cleanup, runtime diagnostics, and asynchronous configuration clearing. Authors: - Will Killian (https://github.com/willkill07) Approvers: - Maryam Najafian (https://github.com/mnajafian-nv) - Eric Evans II (https://github.com/ericevans-nv) URL: https://github.com/NVIDIA/NeMo-Relay/pull/687 --- .../src/observability/plugin_component.rs | 139 +++++++++----- crates/core/src/plugin.rs | 154 +++++++++++----- .../tests/integration/atif_storage_tests.rs | 32 +++- .../observability/plugin_component_tests.rs | 39 +++- crates/core/tests/unit/plugin_tests.rs | 173 ++++++++++++++++-- crates/python/src/py_plugin.rs | 23 ++- .../coverage/py_plugin_coverage_tests.rs | 30 +++ docs/configure-plugins/observability/atif.mdx | 29 +-- python/tests/test_observability_plugin.py | 47 ++++- 9 files changed, 541 insertions(+), 125 deletions(-) diff --git a/crates/core/src/observability/plugin_component.rs b/crates/core/src/observability/plugin_component.rs index 9929589ec..32101a29c 100644 --- a/crates/core/src/observability/plugin_component.rs +++ b/crates/core/src/observability/plugin_component.rs @@ -63,8 +63,9 @@ use crate::observability::{ use crate::plugin::{ ATIF_RUNTIME_DELIVERY_FAILURE_MARKER, ConfigDiagnostic, ConfigPolicy, DiagnosticLevel, OTEL_RUNTIME_DELIVERY_FAILURE_MARKER, Plugin, PluginComponentSpec, PluginError, - PluginRegistration, PluginRegistrationContext, Result as PluginResult, UnsupportedBehavior, - apply_global_config_policy, deregister_plugin, register_builtin_plugin, + PluginRegistration, PluginRegistrationCleanupOutcome, PluginRegistrationContext, + Result as PluginResult, UnsupportedBehavior, apply_global_config_policy, deregister_plugin, + register_builtin_plugin, }; use crate::plugin::{RuntimeDiagnostic, record_active_plugin_runtime_diagnostic}; @@ -892,41 +893,62 @@ fn register_atif_dispatcher( ); ctx.register_subscriber("atif", dispatcher)?; let shutdown_storage = Arc::clone(&storage); - ctx.add_registration(PluginRegistration::new( + ctx.add_registration(PluginRegistration::new_with_outcome( "observability", ctx.qualify_name("atif.shutdown"), Box::new(move || { - let work = { - let mut guard = manager.lock().map_err(|err| { - PluginError::Internal(format!("ATIF dispatcher lock poisoned: {err}")) - })?; - guard.flush_open_agents() - }; - for (scope_uuid, name) in work.scope_subscribers { - deregister_atif_shutdown_subscriber(&scope_uuid, &name)?; - } - for export in work.exports { - let write = prepare_atif_shutdown_file(&export, Arc::clone(&manager)) - .map_err(observability_registration_error)?; - let agent_uuid = write.agent_uuid; - let targets = { - let guard = manager.lock().map_err(|err| { + let work = match (|| -> PluginResult<_> { + let work = { + let mut guard = manager.lock().map_err(|err| { PluginError::Internal(format!("ATIF dispatcher lock poisoned: {err}")) })?; - guard.sink_targets() + guard.flush_open_agents() }; - let results = write_atif(&write, shutdown_storage.as_slice(), &targets); - let mut guard = manager.lock().map_err(|err| { - PluginError::Internal(format!("ATIF dispatcher lock poisoned: {err}")) - })?; - let _ = guard.complete_scope_write(agent_uuid, results); + for (scope_uuid, name) in &work.scope_subscribers { + deregister_atif_shutdown_subscriber(scope_uuid, name)?; + } + Ok(work) + })() { + Ok(work) => work, + Err(error) => return PluginRegistrationCleanupOutcome::NotRemoved(error), + }; + + let delivery = (|| -> PluginResult<()> { + for export in work.exports { + let write = prepare_atif_shutdown_file(&export, Arc::clone(&manager)) + .map_err(observability_registration_error)?; + let agent_uuid = write.agent_uuid; + let targets = { + let guard = manager.lock().map_err(|err| { + PluginError::Internal(format!("ATIF dispatcher lock poisoned: {err}")) + })?; + guard.sink_targets() + }; + let results = write_atif(&write, shutdown_storage.as_slice(), &targets); + let mut guard = manager.lock().map_err(|err| { + PluginError::Internal(format!("ATIF dispatcher lock poisoned: {err}")) + })?; + let _ = guard.complete_scope_write(agent_uuid, results); + } + Ok(()) + })(); + if let Err(error) = delivery { + return PluginRegistrationCleanupOutcome::RemovedWithError(error); + } + let guard = match manager.lock() { + Ok(guard) => guard, + Err(error) => { + return PluginRegistrationCleanupOutcome::RemovedWithError( + PluginError::Internal(format!("ATIF dispatcher lock poisoned: {error}")), + ); + } + }; + match guard.last_error_result() { + Ok(()) => PluginRegistrationCleanupOutcome::Removed, + Err(error) => PluginRegistrationCleanupOutcome::RemovedWithError( + observability_registration_error(error), + ), } - let guard = manager.lock().map_err(|err| { - PluginError::Internal(format!("ATIF dispatcher lock poisoned: {err}")) - })?; - guard - .last_error_result() - .map_err(observability_registration_error) }), )); Ok(()) @@ -999,10 +1021,20 @@ fn register_opentelemetry( // Retain the subscribers as long as the registered fan-out callback exists. // Their tracer providers and exporter runtimes must outlive event delivery. let delivery_subscribers = subscribers.clone(); - ctx.add_registration(PluginRegistration::new( + ctx.add_registration(PluginRegistration::new_with_outcome( "observability", ctx.qualify_name("opentelemetry.shutdown"), - Box::new(move || shutdown_opentelemetry_subscribers(&subscribers).map_or(Ok(()), Err)), + Box::new( + move || match shutdown_opentelemetry_subscribers(&subscribers) { + None => PluginRegistrationCleanupOutcome::Removed, + Some(OpenTelemetryShutdownFailure::Delivery(error)) => { + PluginRegistrationCleanupOutcome::RemovedWithError(error) + } + Some(OpenTelemetryShutdownFailure::Other(error)) => { + PluginRegistrationCleanupOutcome::NotRemoved(error) + } + }, + ), )); ctx.register_subscriber( "opentelemetry", @@ -1059,9 +1091,14 @@ fn build_opentelemetry_subscribers( Ok(subscribers) } +enum OpenTelemetryShutdownFailure { + Delivery(PluginError), + Other(PluginError), +} + fn shutdown_opentelemetry_subscribers( subscribers: &[Arc], -) -> Option { +) -> Option { let mut errors = Vec::new(); if let Err(error) = flush_subscribers() { errors.push(crate::observability::otel::OpenTelemetryError::Core(error)); @@ -1086,7 +1123,12 @@ fn shutdown_opentelemetry_subscribers( } else { format!("OpenTelemetry shutdown failures: {summary}") }; - Some(PluginError::RegistrationFailed(message)) + let error = PluginError::RegistrationFailed(message); + Some(if all_delivery_failures { + OpenTelemetryShutdownFailure::Delivery(error) + } else { + OpenTelemetryShutdownFailure::Other(error) + }) } fn shutdown_opentelemetry_providers( @@ -1624,16 +1666,31 @@ fn render_atif_filename( })?; let expression = rendered[selector_start..end].to_string(); let (selector, fallback) = parse_atif_metadata_expression(&expression)?; - let value = selector - .split('.') - .fold(metadata, |value, segment| value?.get(segment)) - .and_then(Json::as_str) - .or(fallback) - .ok_or_else(|| { + let mut resolved = metadata; + for segment in selector.split('.') { + resolved = match resolved { + Some(Json::Object(object)) => object.get(segment), + None | Some(Json::Null) => break, + Some(_) => { + return Err(format!( + "filename_template placeholder '{{metadata.{selector}}}' traversed a non-object value" + )); + } + }; + } + let value = match resolved { + Some(Json::String(value)) => value.as_str(), + None | Some(Json::Null) => fallback.ok_or_else(|| { format!( "filename_template placeholder '{{metadata.{selector}}}' must resolve to a string" ) - })?; + })?, + Some(_) => { + return Err(format!( + "filename_template placeholder '{{metadata.{selector}}}' resolved to a non-string value" + )); + } + }; if !is_safe_atif_metadata_path(value) { return Err(format!( "metadata path '{selector}' must be a path-safe relative fragment" diff --git a/crates/core/src/plugin.rs b/crates/core/src/plugin.rs index 9fbce2c81..335ec7afd 100644 --- a/crates/core/src/plugin.rs +++ b/crates/core/src/plugin.rs @@ -337,7 +337,13 @@ pub struct PluginRegistration { pub kind: String, /// Runtime-qualified registration name. pub name: String, - deregister: Box Result<()> + Send>, + deregister: Box PluginRegistrationCleanupOutcome + Send>, +} + +pub(crate) enum PluginRegistrationCleanupOutcome { + Removed, + RemovedWithError(PluginError), + NotRemoved(PluginError), } impl fmt::Debug for PluginRegistration { @@ -354,7 +360,22 @@ impl PluginRegistration { pub fn new( kind: impl Into, name: impl Into, - deregister: Box Result<()> + Send>, + mut deregister: Box Result<()> + Send>, + ) -> Self { + Self { + kind: kind.into(), + name: name.into(), + deregister: Box::new(move || match deregister() { + Ok(()) => PluginRegistrationCleanupOutcome::Removed, + Err(error) => PluginRegistrationCleanupOutcome::NotRemoved(error), + }), + } + } + + pub(crate) fn new_with_outcome( + kind: impl Into, + name: impl Into, + deregister: Box PluginRegistrationCleanupOutcome + Send>, ) -> Self { Self { kind: kind.into(), @@ -1558,12 +1579,39 @@ async fn initialize_plugins_exact_inner( }; if let Some(mut previous_state) = previous { - let teardown_errors = rollback_registrations_checked(&mut previous_state.registrations); - if !teardown_errors.is_empty() { - record_rollback_failures(rollback_failures.as_ref(), teardown_errors.clone()); + // Keep the previous report installed while teardown callbacks run so + // runtime diagnostics emitted by teardown remain observable. + { + let mut guard = ACTIVE_PLUGIN_CONFIGURATION.lock().map_err(|err| { + PluginError::Internal(format!("active plugin configuration lock poisoned: {err}")) + })?; + *guard = Some(ActivePluginConfiguration { + config: previous_state.config.clone(), + report: previous_state.report.clone(), + registrations: Vec::new(), + }); + } + let teardown = rollback_registrations_checked(&mut previous_state.registrations); + let teardown_report = ACTIVE_PLUGIN_CONFIGURATION + .lock() + .map_err(|err| { + PluginError::Internal(format!("active plugin configuration lock poisoned: {err}")) + })? + .take() + .map(|state| state.report); + if !teardown.errors.is_empty() { + if let Some(report) = + teardown_report.filter(|report| !report.runtime_diagnostics.is_empty()) + && let Ok(mut guard) = LAST_FAILED_RUNTIME_DIAGNOSTICS_REPORT.lock() + { + *guard = Some(report); + } + if !teardown.callbacks_cleared { + record_rollback_failures(rollback_failures.as_ref(), teardown.errors.clone()); + } return Err(PluginError::RegistrationFailed(format!( "previous plugin configuration could not be cleared: {}", - teardown_errors.join("; ") + teardown.errors.join("; ") ))); } match initialize_plugin_components_catching_panics( @@ -2123,7 +2171,7 @@ fn clear_plugin_configuration_inner() -> PluginHostClearOutcome { }; // Keep the report installed while callbacks run so runtime diagnostics // emitted by teardown work can be recorded against it. - let deregistration_errors = registrations + let deregistration = registrations .as_mut() .map(rollback_registrations_checked) .unwrap_or_default(); @@ -2138,16 +2186,10 @@ fn clear_plugin_configuration_inner() -> PluginHostClearOutcome { }; } }; - // Runtime delivery failures are reported by an otherwise successful - // deregistration callback. They must propagate without treating callback - // removal itself as unsafe. - let callbacks_cleared = deregistration_errors - .iter() - .all(|error| is_runtime_delivery_failure(error)); - let deregistration_error = (!deregistration_errors.is_empty()).then(|| { + let deregistration_error = (!deregistration.errors.is_empty()).then(|| { PluginError::RegistrationFailed(format!( "plugin teardown failed: {}", - deregistration_errors.join("; ") + deregistration.errors.join("; ") )) }); let result = match (flush_error, deregistration_error) { @@ -2170,19 +2212,10 @@ fn clear_plugin_configuration_inner() -> PluginHostClearOutcome { } PluginHostClearOutcome { result, - callbacks_cleared, + callbacks_cleared: deregistration.callbacks_cleared, } } -fn is_runtime_delivery_failure(error: &str) -> bool { - [ - ATIF_RUNTIME_DELIVERY_FAILURE_MARKER, - OTEL_RUNTIME_DELIVERY_FAILURE_MARKER, - ] - .iter() - .any(|marker| error.contains(&format!("registration failed: {marker}:"))) -} - pub(crate) fn plugin_configuration_is_active() -> Result { ACTIVE_PLUGIN_CONFIGURATION .lock() @@ -2342,26 +2375,53 @@ pub fn rollback_registrations(registrations: &mut Vec) { let _ = rollback_registrations_checked(registrations); } -fn rollback_registrations_checked(registrations: &mut Vec) -> Vec { - let mut errors = Vec::new(); +struct PluginRollbackOutcome { + errors: Vec, + callbacks_cleared: bool, +} + +impl Default for PluginRollbackOutcome { + fn default() -> Self { + Self { + errors: Vec::new(), + callbacks_cleared: true, + } + } +} + +fn rollback_registrations_checked( + registrations: &mut Vec, +) -> PluginRollbackOutcome { + let mut outcome = PluginRollbackOutcome::default(); for registration in registrations.iter_mut().rev() { - let failure = match catch_unwind(AssertUnwindSafe(|| (registration.deregister)())) { - Ok(Ok(())) => None, - Ok(Err(error)) => Some(error.to_string()), - Err(payload) => Some(format!( - "deregistration panicked: {}", - panic_payload_message(payload) - )), - }; - if let Some(error) = failure { - errors.push(format!( - "{} registration '{}' could not be removed: {error}", - registration.kind, registration.name - )); + match catch_unwind(AssertUnwindSafe(|| (registration.deregister)())) { + Ok(PluginRegistrationCleanupOutcome::Removed) => {} + Ok(PluginRegistrationCleanupOutcome::RemovedWithError(error)) => { + outcome.errors.push(format!( + "{} registration '{}' reported a delivery failure: {error}", + registration.kind, registration.name + )); + } + Ok(PluginRegistrationCleanupOutcome::NotRemoved(error)) => { + outcome.callbacks_cleared = false; + outcome.errors.push(format!( + "{} registration '{}' could not be removed: {error}", + registration.kind, registration.name + )); + } + Err(payload) => { + outcome.callbacks_cleared = false; + outcome.errors.push(format!( + "{} registration '{}' could not be removed: deregistration panicked: {}", + registration.kind, + registration.name, + panic_payload_message(payload) + )); + } } } registrations.clear(); - errors + outcome } fn panic_payload_message(payload: Box) -> String { @@ -2444,8 +2504,10 @@ impl PendingPluginRegistrations { impl Drop for PendingPluginRegistrations { fn drop(&mut self) { - let errors = rollback_registrations_checked(&mut self.registrations); - record_rollback_failures(self.rollback_failures.as_ref(), errors); + let outcome = rollback_registrations_checked(&mut self.registrations); + if !outcome.callbacks_cleared { + record_rollback_failures(self.rollback_failures.as_ref(), outcome.errors); + } } } @@ -2469,8 +2531,10 @@ impl PendingPluginRegistrationContext { impl Drop for PendingPluginRegistrationContext { fn drop(&mut self) { - let errors = rollback_registrations_checked(&mut self.context.registrations); - record_rollback_failures(self.rollback_failures.as_ref(), errors); + let outcome = rollback_registrations_checked(&mut self.context.registrations); + if !outcome.callbacks_cleared { + record_rollback_failures(self.rollback_failures.as_ref(), outcome.errors); + } } } diff --git a/crates/core/tests/integration/atif_storage_tests.rs b/crates/core/tests/integration/atif_storage_tests.rs index 09d3d9f04..84a1083b0 100644 --- a/crates/core/tests/integration/atif_storage_tests.rs +++ b/crates/core/tests/integration/atif_storage_tests.rs @@ -23,7 +23,8 @@ use nemo_relay::api::scope::{PopScopeParams, PushScopeParams, ScopeType, pop_sco use nemo_relay::api::subscriber::flush_subscribers; use nemo_relay::observability::plugin_component::OBSERVABILITY_PLUGIN_KIND; use nemo_relay::plugin::{ - PluginComponentSpec, PluginConfig, clear_plugin_configuration, initialize_plugins, + PluginComponentSpec, PluginConfig, active_plugin_report, clear_plugin_configuration, + initialize_plugins, }; use object_store::{ObjectStore, ObjectStoreExt as _}; use serde_json::{Value as Json, json}; @@ -497,13 +498,18 @@ fn atif_storage_posts_trajectory_to_http_endpoints() { fn atif_storage_http_non_2xx_retries_on_the_next_trajectory() { let _guard = PLUGIN_TEST_LOCK.lock().unwrap(); reset_runtime(); + let recovery_directory = tempfile::tempdir().expect("create recovery directory"); let mut server = start_http_server(2, vec![("/fail", 500)]); // SAFETY: this uniquely named env var is only touched by this test. unsafe { std::env::set_var("NEMO_RELAY_ATIF_HTTP_TEST_TOKEN", "Bearer test-token"); } - let config = build_http_observability_config(&[format!("{}/fail", server.base_url)]); + let mut config = build_http_observability_config(&[format!("{}/fail", server.base_url)]); + config.components[0].config["atif"] + .as_object_mut() + .expect("ATIF config should be an object") + .insert("output_directory".into(), json!(recovery_directory.path())); futures::executor::block_on(initialize_plugins(config)) .expect("observability plugin should initialize with HTTP storage"); @@ -529,6 +535,15 @@ fn atif_storage_http_non_2xx_retries_on_the_next_trajectory() { .expect("pop second agent scope"); flush_subscribers().expect("HTTP upload subscriber should flush after failure"); + let report = active_plugin_report().expect("active plugin report should remain readable"); + let diagnostic = report + .runtime_diagnostics + .iter() + .find(|diagnostic| diagnostic.code == "atif.remote_delivery_failed") + .expect("failed uploads should be visible before teardown"); + assert_eq!(diagnostic.field.as_deref(), Some("storage[0]")); + assert_eq!(diagnostic.count, 2); + server.stop(); { let requests = server.received.lock().unwrap(); @@ -557,6 +572,19 @@ fn atif_storage_http_non_2xx_retries_on_the_next_trajectory() { teardown.to_string().contains("atif.remote_delivery_failed"), "teardown should identify the failed remote destination: {teardown}" ); + assert!( + !teardown.to_string().contains("could not be removed"), + "delivery failure should not imply a leaked registration: {teardown}" + ); + let retained_report = + active_plugin_report().expect("failed teardown should retain the plugin report"); + let retained_diagnostic = retained_report + .runtime_diagnostics + .iter() + .find(|diagnostic| diagnostic.code == "atif.remote_delivery_failed") + .expect("failed upload diagnostic should remain readable after teardown"); + assert_eq!(retained_diagnostic.field.as_deref(), Some("storage[0]")); + assert_eq!(retained_diagnostic.count, 2); // SAFETY: cleanup of test-only env var. unsafe { std::env::remove_var("NEMO_RELAY_ATIF_HTTP_TEST_TOKEN"); diff --git a/crates/core/tests/unit/observability/plugin_component_tests.rs b/crates/core/tests/unit/observability/plugin_component_tests.rs index 48f3f6775..abddf29be 100644 --- a/crates/core/tests/unit/observability/plugin_component_tests.rs +++ b/crates/core/tests/unit/observability/plugin_component_tests.rs @@ -2447,6 +2447,35 @@ fn atif_metadata_template_values_must_be_safe_path_fragments() { .prepare_destination("session-1", Some(&non_string)) .is_err() ); + let dispatcher_with_fallback = AtifDispatcher::new(AtifSectionConfig { + filename_template: "{metadata.artifact_path:-unassigned}/trajectory-{session_id}.json" + .to_string(), + ..AtifSectionConfig::default() + }); + let error = dispatcher_with_fallback + .prepare_destination("session-1", Some(&non_string)) + .unwrap_err(); + assert!(error.contains("resolved to a non-string value"), "{error}"); + let nested_non_string = json!({"artifact": 123}); + let nested_dispatcher = AtifDispatcher::new(AtifSectionConfig { + filename_template: "{metadata.artifact.path:-unassigned}/trajectory-{session_id}.json" + .to_string(), + ..AtifSectionConfig::default() + }); + let error = nested_dispatcher + .prepare_destination("session-1", Some(&nested_non_string)) + .unwrap_err(); + assert!(error.contains("traversed a non-object value"), "{error}"); + let nested_null = json!({"artifact": null}); + let destination = nested_dispatcher + .prepare_destination("session-1", Some(&nested_null)) + .unwrap(); + assert_eq!(destination.0, "unassigned/trajectory-session-1.json"); + let nested_string = json!({"artifact": {"path": "tenant-a/team_1"}}); + let destination = nested_dispatcher + .prepare_destination("session-1", Some(&nested_string)) + .unwrap(); + assert_eq!(destination.0, "tenant-a/team_1/trajectory-session-1.json"); for template in [ "/tmp/trajectory-{session_id}.json", @@ -3037,9 +3066,13 @@ fn opentelemetry_shutdown_helper_retains_every_endpoint_failure() { }) .collect::>(); - let error = shutdown_opentelemetry_subscribers(&subscribers) - .expect("mixed endpoint shutdown failures should be reported") - .to_string(); + let OpenTelemetryShutdownFailure::Other(error) = + shutdown_opentelemetry_subscribers(&subscribers) + .expect("mixed endpoint shutdown failures should be reported") + else { + panic!("mixed endpoint shutdown failures must retain the registration failure outcome"); + }; + let error = error.to_string(); assert_eq!(dropped_calls.load(Ordering::SeqCst), 1); assert_eq!(timeout_calls.load(Ordering::SeqCst), 1); diff --git a/crates/core/tests/unit/plugin_tests.rs b/crates/core/tests/unit/plugin_tests.rs index 816f2ab8d..815239734 100644 --- a/crates/core/tests/unit/plugin_tests.rs +++ b/crates/core/tests/unit/plugin_tests.rs @@ -1477,6 +1477,37 @@ fn test_pending_registration_records_rollback_failures() { assert!(failures[0].contains("rollback remained registered")); } +#[test] +fn test_pending_rollbacks_ignore_delivery_only_errors() { + let failures = Arc::new(Mutex::new(Vec::new())); + let delivery_error = || { + PluginRegistrationCleanupOutcome::RemovedWithError(PluginError::RegistrationFailed( + "delivery failed".into(), + )) + }; + { + let mut pending = PendingPluginRegistrations::new(Some(Arc::clone(&failures))); + pending.extend(vec![PluginRegistration::new_with_outcome( + "fixture", + "delivery-only-registration", + Box::new(delivery_error), + )]); + } + { + let mut pending = + PendingPluginRegistrationContext::new("fixture.".into(), Some(Arc::clone(&failures))); + pending + .context + .add_registration(PluginRegistration::new_with_outcome( + "fixture", + "delivery-only-context-registration", + Box::new(delivery_error), + )); + } + + assert!(failures.lock().unwrap().is_empty()); +} + #[test] fn test_checked_teardown_reports_unremoved_registrations() { let _guard = lock_runtime_owner(); @@ -1506,13 +1537,41 @@ fn test_checked_teardown_reports_unremoved_registrations() { } #[test] -fn test_teardown_runtime_diagnostics_remain_in_the_plugin_report() { +fn test_teardown_marker_text_does_not_imply_successful_removal() { let _guard = lock_runtime_owner(); reset_global(); store_active_plugin_configuration( PluginConfig::default(), ConfigReport::default(), vec![PluginRegistration::new( + "fixture", + "stale-marker-callback", + Box::new(|| { + Err(PluginError::RegistrationFailed(format!( + "unrelated failure mentioning {}", + crate::plugin::ATIF_RUNTIME_DELIVERY_FAILURE_MARKER + ))) + }), + )], + ) + .unwrap(); + + let outcome = clear_plugin_configuration_inner(); + assert!(!outcome.callbacks_cleared); + let error = outcome.result.unwrap_err().to_string(); + assert!(error.contains("stale-marker-callback"), "{error}"); + assert!(error.contains("could not be removed"), "{error}"); + reset_global(); +} + +#[test] +fn test_teardown_runtime_diagnostics_remain_in_the_plugin_report() { + let _guard = lock_runtime_owner(); + reset_global(); + store_active_plugin_configuration( + PluginConfig::default(), + ConfigReport::default(), + vec![PluginRegistration::new_with_outcome( "fixture", "atif-shutdown", Box::new(|| { @@ -1524,10 +1583,11 @@ fn test_teardown_runtime_diagnostics_remain_in_the_plugin_report() { session_id: Some("session-123".into()), count: 1, }); - Err(PluginError::RegistrationFailed(format!( + let error = PluginError::RegistrationFailed(format!( "{}: atif.remote_delivery_failed (1)", crate::plugin::ATIF_RUNTIME_DELIVERY_FAILURE_MARKER - ))) + )); + PluginRegistrationCleanupOutcome::RemovedWithError(error) }), )], ) @@ -1535,7 +1595,9 @@ fn test_teardown_runtime_diagnostics_remain_in_the_plugin_report() { let outcome = clear_plugin_configuration_inner(); assert!(outcome.callbacks_cleared); - assert!(outcome.result.is_err()); + let error = outcome.result.unwrap_err().to_string(); + assert!(error.contains("atif.remote_delivery_failed"), "{error}"); + assert!(!error.contains("could not be removed"), "{error}"); let report = active_plugin_report().expect("failed teardown should retain its report"); assert_eq!(report.runtime_diagnostics.len(), 1); let diagnostic = &report.runtime_diagnostics[0]; @@ -1556,14 +1618,16 @@ fn test_opentelemetry_delivery_failure_allows_later_plugin_configuration() { store_active_plugin_configuration( PluginConfig::default(), ConfigReport::default(), - vec![PluginRegistration::new( + vec![PluginRegistration::new_with_outcome( "fixture", "opentelemetry-shutdown", Box::new(|| { - Err(PluginError::RegistrationFailed(format!( - "{}: otel.spans_dropped (2)", - crate::plugin::OTEL_RUNTIME_DELIVERY_FAILURE_MARKER - ))) + PluginRegistrationCleanupOutcome::RemovedWithError(PluginError::RegistrationFailed( + format!( + "{}: otel.spans_dropped (2)", + crate::plugin::OTEL_RUNTIME_DELIVERY_FAILURE_MARKER + ), + )) }), )], ) @@ -1582,14 +1646,16 @@ fn test_mixed_opentelemetry_shutdown_failure_blocks_later_configuration() { store_active_plugin_configuration( PluginConfig::default(), ConfigReport::default(), - vec![PluginRegistration::new( + vec![PluginRegistration::new_with_outcome( "fixture", "opentelemetry-shutdown", Box::new(|| { - Err(PluginError::RegistrationFailed(format!( - "OpenTelemetry shutdown failures: provider error: {}: otel.spans_dropped (2); endpoint shutdown timed out", - crate::plugin::OTEL_RUNTIME_DELIVERY_FAILURE_MARKER - ))) + PluginRegistrationCleanupOutcome::NotRemoved(PluginError::RegistrationFailed( + format!( + "OpenTelemetry shutdown failures: provider error: {}: otel.spans_dropped (2); endpoint shutdown timed out", + crate::plugin::OTEL_RUNTIME_DELIVERY_FAILURE_MARKER + ), + )) }), )], ) @@ -1602,6 +1668,72 @@ fn test_mixed_opentelemetry_shutdown_failure_blocks_later_configuration() { reset_global(); } +#[test] +fn test_replacement_teardown_runtime_diagnostics_remain_in_the_plugin_report() { + let _guard = lock_runtime_owner(); + reset_global(); + store_active_plugin_configuration( + PluginConfig::default(), + ConfigReport::default(), + vec![PluginRegistration::new_with_outcome( + "fixture", + "atif-shutdown", + Box::new(|| { + record_active_plugin_runtime_diagnostic(RuntimeDiagnostic { + code: "atif.remote_delivery_failed".into(), + component: "observability".into(), + field: Some("storage[0]".into()), + message: "HTTP 500".into(), + session_id: Some("session-123".into()), + count: 1, + }); + PluginRegistrationCleanupOutcome::RemovedWithError(PluginError::RegistrationFailed( + "ATIF delivery failed".into(), + )) + }), + )], + ) + .unwrap(); + + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let error = runtime + .block_on(initialize_plugins_exact(PluginConfig::default())) + .unwrap_err(); + assert!( + error.to_string().contains("ATIF delivery failed"), + "{error}" + ); + assert!( + error + .to_string() + .contains("fixture registration 'atif-shutdown' reported a delivery failure"), + "{error}" + ); + assert!( + !plugin_configuration_is_active().unwrap(), + "a replacement aborted by delivery failure must not leave a configuration active" + ); + let report = active_plugin_report().expect("failed replacement should retain its report"); + assert_eq!(report.runtime_diagnostics.len(), 1); + assert_eq!( + report.runtime_diagnostics[0].code, + "atif.remote_delivery_failed" + ); + assert_eq!( + report.runtime_diagnostics[0].field.as_deref(), + Some("storage[0]") + ); + assert_eq!(report.runtime_diagnostics[0].count, 1); + + runtime + .block_on(initialize_plugins_exact(PluginConfig::default())) + .expect("delivery-only teardown errors must not block a later initialization"); + reset_global(); +} + #[test] fn test_legacy_clear_retains_mutation_owner_after_incomplete_teardown() { let _guard = lock_runtime_owner(); @@ -2207,11 +2339,18 @@ fn test_plugin_registration_context_maps_deregistration_errors() { set_conflicting_runtime_owner_for_tests(); for (registration, expected) in registrations.iter_mut().zip(expected_messages) { match (registration.deregister)() { - Err(PluginError::RegistrationFailed(message)) => { + PluginRegistrationCleanupOutcome::NotRemoved(PluginError::RegistrationFailed( + message, + )) => { assert!(message.contains(expected), "{message}"); } - Err(other) => panic!("unexpected deregistration failure: {other}"), - Ok(()) => panic!("expected deregistration to fail"), + PluginRegistrationCleanupOutcome::NotRemoved(other) => { + panic!("unexpected deregistration failure: {other}") + } + PluginRegistrationCleanupOutcome::Removed + | PluginRegistrationCleanupOutcome::RemovedWithError(_) => { + panic!("expected deregistration to fail") + } } } diff --git a/crates/python/src/py_plugin.rs b/crates/python/src/py_plugin.rs index 281ae0eca..ec11ce32e 100644 --- a/crates/python/src/py_plugin.rs +++ b/crates/python/src/py_plugin.rs @@ -933,16 +933,20 @@ impl PluginConfigurationClearState { let result = std::panic::catch_unwind(clear_plugin_configuration) .map_err(|_| PluginTeardownError::runtime("plugin teardown task panicked")) .and_then(|result| result.map_err(PluginTeardownError::from_plugin_error)); - clear_state.completion.finish(result); + clear_state.finish(result); }); if let Err(error) = spawn { - self.completion - .finish(Err(PluginTeardownError::runtime(format!( - "failed to start plugin teardown task: {error}" - )))); + self.finish(Err(PluginTeardownError::runtime(format!( + "failed to start plugin teardown task: {error}" + )))); } } + fn finish(self: &Arc, result: PluginTeardownResult) { + reset_plugin_configuration_clear_state_if(self); + self.completion.finish(result); + } + async fn wait_for_clear(&self) -> PluginTeardownResult { self.completion.wait("plugin teardown").await } @@ -965,6 +969,15 @@ fn reset_plugin_configuration_clear_state() { Arc::new(PluginConfigurationClearState::new()); } +fn reset_plugin_configuration_clear_state_if(completed: &Arc) { + let mut current = PLUGIN_CONFIGURATION_CLEAR_STATE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + if Arc::ptr_eq(¤t, completed) { + *current = Arc::new(PluginConfigurationClearState::new()); + } +} + #[pymethods] impl PyPluginHostActivation { /// Return the activation report captured during initialization. diff --git a/crates/python/tests/coverage/py_plugin_coverage_tests.rs b/crates/python/tests/coverage/py_plugin_coverage_tests.rs index 88127cdea..b8818295b 100644 --- a/crates/python/tests/coverage/py_plugin_coverage_tests.rs +++ b/crates/python/tests/coverage/py_plugin_coverage_tests.rs @@ -165,6 +165,7 @@ fn async_clear_binding_completes_on_python_event_loop() { let _python = crate::test_support::init_python_test(); let _plugin_test_state = lock_plugin_test_state_for_tests(); Python::attach(|py| { + let first_clear_state = plugin_configuration_clear_state(); let module = PyModule::new(py, "_plugin_async_clear").unwrap(); register(&module).unwrap(); let helpers = load_module( @@ -174,6 +175,19 @@ async def clear(module): await module.clear_plugin_configuration_async() "#, ); + with_event_loop(py, |event_loop| { + let clear = helpers + .getattr("clear") + .unwrap() + .call1((module.clone(),)) + .unwrap(); + event_loop + .call_method1("run_until_complete", (clear,)) + .unwrap(); + }); + let second_clear_state = plugin_configuration_clear_state(); + assert!(!Arc::ptr_eq(&first_clear_state, &second_clear_state)); + with_event_loop(py, |event_loop| { let clear = helpers.getattr("clear").unwrap().call1((module,)).unwrap(); event_loop @@ -184,6 +198,22 @@ async def clear(module): }); } +#[test] +fn stale_async_clear_completion_keeps_the_newer_state() { + let _plugin_test_state = lock_plugin_test_state_for_tests(); + let older = plugin_configuration_clear_state(); + let newer = Arc::new(PluginConfigurationClearState::new()); + *PLUGIN_CONFIGURATION_CLEAR_STATE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) = Arc::clone(&newer); + + reset_plugin_configuration_clear_state_if(&older); + + let newer_state_remained_current = Arc::ptr_eq(&plugin_configuration_clear_state(), &newer); + reset_plugin_configuration_clear_state(); + assert!(newer_state_remained_current); +} + #[test] fn plugin_context_registers_all_runtime_hooks_and_drains_registrations() { let _python = crate::test_support::init_python_test(); diff --git a/docs/configure-plugins/observability/atif.mdx b/docs/configure-plugins/observability/atif.mdx index dec1ad05c..077d11c2d 100644 --- a/docs/configure-plugins/observability/atif.mdx +++ b/docs/configure-plugins/observability/atif.mdx @@ -89,18 +89,20 @@ With scope metadata `{"atif_prefix":"tenant-a/session-123"}`, Relay writes still contain `{session_id}`. Use `:-` to provide a literal fallback when metadata can be absent, for example -`{metadata.atif_prefix:-unassigned}`. Relay also uses the fallback when the -metadata value is not a string. +`{metadata.atif_prefix:-unassigned}`. Relay uses the fallback when the selected +metadata field is absent or `null`. A present non-string value is rejected +instead of being routed through the fallback. Each metadata placeholder must resolve to a string containing a non-empty, relative path fragment. Slash-separated segments can contain ASCII letters, digits, `-`, `_`, `.`, and `~`. Relay rejects empty segments, `.` and `..` segments, absolute paths, backslashes, spaces, and other characters. If a -placeholder has no fallback and is missing or non-string, or if its resolved -value is unsafe, Relay skips that trajectory. It records a runtime diagnostic -in `plugin.report()` and causes plugin teardown to fail. Literal template text -must also be a relative, traversal-free path. The rendered filename applies to -local, S3, and HTTP storage in the same way as a static filename. +placeholder is missing without a fallback, is present but non-string, or +resolves to an unsafe value, Relay skips that trajectory. It records a runtime +diagnostic in `plugin.report()` and causes plugin teardown to fail. Literal +template text must also be a relative, traversal-free path. The rendered +filename applies to local, S3, and HTTP storage in the same way as a static +filename. The CLI gateway parses `x-nemo-relay-session-metadata` as JSON and merges it into the top-level scope metadata: @@ -245,10 +247,15 @@ delivery diagnostic and retries that destination for each later trajectory. Other destinations continue to receive writes. When every remote destination fails for one trajectory, Relay writes a local recovery copy under `output_directory`; a successful remote destination does not create that copy. -The diagnostic remains visible in `plugin.report()` and is retained there after -a failed teardown. Teardown also reports the degraded delivery, even when the -local recovery write succeeds. Fatal dispatcher failures, such as trajectory -serialization failures, are also reported during teardown. +The diagnostic becomes visible in `plugin.report()` after subscriber delivery +is flushed and is retained there after a failed teardown. Teardown also reports +the degraded delivery, even when the local recovery write succeeds. This error +reports delivery degradation, not a registration leak; callbacks have already +been removed. If the failure is still pending when initialization replaces the +configuration, that replacement returns the delivery error and leaves no +configuration active; a subsequent clear or initialization is safe. Fatal +dispatcher failures, such as trajectory serialization failures, are also +reported during teardown. ## Expected Output diff --git a/python/tests/test_observability_plugin.py b/python/tests/test_observability_plugin.py index 68b7dd3b6..b457f703a 100644 --- a/python/tests/test_observability_plugin.py +++ b/python/tests/test_observability_plugin.py @@ -14,7 +14,7 @@ import pytest -from nemo_relay import ScopeType, plugin, scope +from nemo_relay import ScopeType, plugin, scope, subscribers from nemo_relay.observability import ( OBSERVABILITY_PLUGIN_KIND, AtifConfig, @@ -439,6 +439,51 @@ async def test_atif_flushes_open_agent_on_clear(self, tmp_path): finally: scope.pop(handle) + async def test_atif_non_string_metadata_is_reported_and_failed_clear_is_drainable(self, tmp_path): + await plugin.initialize( + plugin.PluginConfig( + components=[ + ComponentSpec( + ObservabilityConfig( + atif=AtifConfig( + enabled=True, + output_directory=str(tmp_path), + filename_template="{metadata.atif_prefix:-unassigned}/trajectory-{session_id}.json", + ) + ) + ) + ] + ) + ) + + try: + with scope.scope("python-invalid-metadata-agent", ScopeType.Agent, metadata={"atif_prefix": 123}): + pass + await subscribers.flush_async() + + report = plugin.report() + assert report is not None + assert any( + diagnostic["code"] == "atif.destination_render_failed" and "non-string" in diagnostic["message"] + for diagnostic in report["runtime_diagnostics"] + ) + assert not (tmp_path / "unassigned").exists() + + with pytest.raises(RuntimeError, match=r"atif\.destination_render_failed") as teardown: + await plugin.clear_async() + assert "could not be removed" not in str(teardown.value) + retained = plugin.report() + assert retained is not None + assert any( + diagnostic["code"] == "atif.destination_render_failed" for diagnostic in retained["runtime_diagnostics"] + ) + finally: + try: + await plugin.clear_async() + except RuntimeError: + await plugin.clear_async() + assert plugin.report() is None + async def test_atif_splits_multiple_top_level_agent_scopes(self, tmp_path): await plugin.initialize( plugin.PluginConfig( From e5876f1189faeabeba3b8690509fe1644d32a3bb Mon Sep 17 00:00:00 2001 From: Bryan Bednarski Date: Tue, 4 Aug 2026 16:12:58 -0600 Subject: [PATCH 9/9] fix(python): load dynamic plugin specs from TOML (#694) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### Overview Adds a minimal Python compatibility API that converts standard `[[plugins.dynamic]]` records from one explicit `plugins.toml` into the existing `DynamicPluginActivationSpec` objects accepted by `initialize_with_dynamic_plugins()`. This unblocks Python applications that embed Relay without introducing the larger file-backed activation, lifecycle reconciliation, dynamic layering, or initialization redesign proposed for a later release. The new API is intentionally a temporary 0.7 surface: ```python plugin_config_path = os.environ["NEMO_RELAY_PLUGINS_TOML"] dynamic_plugins = plugin.load_dynamic_plugin_activation_specs(plugin_config_path) activation = await plugin.initialize_with_dynamic_plugins({}, dynamic_plugins) ``` `NEMO_RELAY_PLUGINS_TOML` is an optional host-side convention in this example. Relay does not read the environment variable automatically; the embedding application resolves a path through its environment, command-line, or configuration system and passes that path to the helper. - [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. A broader implementation exists in NVIDIA/NeMo-Relay#684. This PR is a deliberately scoped 0.7 alternative that reuses the existing activation owner instead of introducing shared lifecycle and host-configuration infrastructure. #### Details ##### Public API Adds: ```python def load_dynamic_plugin_activation_specs( plugin_config_path: str | os.PathLike[str], ) -> list[DynamicPluginActivationSpec]: ... ``` The helper: * Reads one explicitly selected `plugins.toml`. * Parses every `[[plugins.dynamic]]` record in declaration order. * Resolves relative manifest paths against the selected file. * Reads `plugin.id` and `plugin.kind` from each manifest. * Preserves the record's JSON-compatible `config`. * Rejects malformed TOML, invalid record shapes, unsupported fields, invalid plugin identities, duplicate plugin IDs, and non-JSON configuration. * Returns the existing activation-spec type without loading code. The existing dynamic initializer now accepts a `Sequence` rather than only a `list`. This reflects its existing behavior and allows parser results, lists, and tuples to compose without casts. ##### Developer flow ```mermaid flowchart LR User["User selects a plugins.toml"] --> Host["Embedding host resolves the path"] Env["Optional NEMO_RELAY_PLUGINS_TOML"] --> Host Host --> Helper["load_dynamic_plugin_activation_specs(path)"] Helper --> Config["Read one explicit plugins.toml"] Config --> Records["Parse [[plugins.dynamic]] records"] Records --> Manifests["Resolve and read relay-plugin.toml manifests"] Manifests --> Specs["Build DynamicPluginActivationSpec list"] Specs --> Initialize["initialize_with_dynamic_plugins(config, specs)"] Initialize --> Activation["Owned PluginHostActivation"] Activation --> Runtime["Host retains activation while work is admitted"] Runtime --> Close["await activation.close() during shutdown"] ``` ##### Configuration behavior The temporary dynamic path and existing static configuration path remain separate: ```mermaid flowchart TB subgraph Static["Existing static component resolution"] UserConfig["User plugins.toml"] --> StaticLayering["User → project → system → programmatic overlay"] ProjectConfig["Project .nemo-relay/plugins.toml"] --> StaticLayering SystemConfig["System /etc/nemo-relay/plugins.toml"] --> StaticLayering end subgraph Dynamic["New 0.7 compatibility path"] ExplicitPath["One explicit plugins.toml path"] --> DynamicParser["Parse [[plugins.dynamic]] only"] DynamicParser --> DynamicSpecs["Explicit activation specs"] end StaticLayering --> HostInitializer["Existing dynamic host initializer"] DynamicSpecs --> HostInitializer HostInitializer --> OwnedHost["PluginHostActivation"] ``` The helper does not perform dynamic-plugin layering. It reads only the explicitly supplied file. Static `[[components]]` from that file are inherited only when the same file is also selected by Relay's normal static discovery. Every dynamic declaration in the selected file becomes an activation spec. Passing those specs to `initialize_with_dynamic_plugins()` is explicit consent to load the referenced trusted native libraries or worker processes. Python workers that require a lifecycle-managed `environment_ref` still require the existing explicit activation or CLI lifecycle path. ##### Intentional non-goals This PR does not: * Consolidate `initialize()` and `initialize_with_dynamic_plugins()`. * Add a unified `initialize_from_plugins_toml()` API. * Discover or merge dynamic records across user, project, and system layers. * Read or reconcile `.dynamic-plugins.json`. * Consult CLI enablement or tombstone state. * Provision or attest Python worker environments. * Change plugin enablement, install plugins, or execute package managers. * Change Rust, Node.js, Go, FFI, or CLI behavior. The helper is documented as a 0.7 compatibility surface and is expected to be deprecated after the unified file-backed initializer lands. Keeping the conversion behind one Relay API lets embedded hosts remove their TOML and manifest parsing now while keeping the future migration localized to one call site. ##### Documentation and validation Updates the Python type stub, plugin-configuration guide, and 0.7 release notes. Tests cover relative and absolute manifest resolution, native and worker spec construction, config preservation, malformed records and TOML, missing manifests, duplicate IDs, and end-to-end native activation from a real `[[plugins.dynamic]]` record. Validation completed: * Focused parser and native-activation tests: `16 passed`. * Ruff formatting and linting. * `ty` type checking. * Changed-file and repository-wide pre-commit suites. * Cargo formatting, clippy, check, and dependency-policy checks. * Python worker protobuf compatibility. * Go formatting and vet. * Node formatting and public docstring checks. * Fern structure and strict broken-link validation. The complete dynamic-host Python module was also attempted locally. Pre-existing tests inherited an invalid machine-level `/etc/nemo-relay/plugins.toml`, and sandboxed worker tests could not bind Unix sockets. The tests directly covering this change passed independently. Breaking changes: none. #### Where should the reviewer start? Start with `python/nemo_relay/plugin.py`, specifically `load_dynamic_plugin_activation_specs()`. The central design decision is that this helper performs only the missing file-to-activation-spec conversion. It deliberately reuses the existing dynamic initializer and owned activation lifetime rather than introducing another activation owner or pulling CLI lifecycle behavior into the Python binding. Then review `python/tests/test_dynamic_plugin_host.py` for the standard TOML parsing, failure behavior, and end-to-end native activation coverage. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) * Relates to NVIDIA/NeMo-Relay#673 * Relates to NVIDIA/NeMo-Relay#684 * Relates to [NousResearch/hermes-agent#77915]() ## Summary by CodeRabbit * **New Features** * Added a Python compatibility helper for loading dynamic plugin activation specifications from a selected `plugins.toml` file. * Supports manifest path resolution, ordered activation specifications, nested configuration, duplicate detection, and validation of plugin records and JSON values. * Dynamic plugin initialization now accepts any ordered collection of activation specifications. * **Documentation** * Added guidance covering configuration resolution, explicit loading consent, supported behavior, limitations, and planned deprecation. * **Tests** * Expanded coverage for valid configurations, absolute and nested manifest paths, malformed files, duplicate IDs, and missing manifests. ## Summary by CodeRabbit * **New Features** * Added support for loading dynamic plugin activation settings from a selected `plugins.toml` file. * Added validation for manifests, duplicate identifiers, malformed configuration, and invalid JSON values. * Dynamic plugin initialization now accepts any ordered collection of activation specifications. * **Documentation** * Added configuration guidance, behavior details, limitations, compatibility notes, and planned deprecation information. * **Tests** * Added coverage for valid configurations, path resolution, nested settings, and common loading errors. Authors: - Bryan Bednarski (https://github.com/bbednarski9) Approvers: - Will Killian (https://github.com/willkill07) - Maryam Najafian (https://github.com/mnajafian-nv) URL: https://github.com/NVIDIA/NeMo-Relay/pull/694 --- docs/about-nemo-relay/release-notes/index.mdx | 6 + .../plugin-configuration-files.mdx | 44 +++++ python/nemo_relay/plugin.py | 116 +++++++++++- python/nemo_relay/plugin.pyi | 8 +- python/tests/test_dynamic_plugin_host.py | 175 +++++++++++++++++- 5 files changed, 345 insertions(+), 4 deletions(-) diff --git a/docs/about-nemo-relay/release-notes/index.mdx b/docs/about-nemo-relay/release-notes/index.mdx index a839cb127..6d9ed0ae7 100644 --- a/docs/about-nemo-relay/release-notes/index.mdx +++ b/docs/about-nemo-relay/release-notes/index.mdx @@ -102,6 +102,12 @@ their values cannot be isolated between endpoints. ### Fixed Known Issues in 0.7 +- Embedded Python hosts can use + `plugin.load_dynamic_plugin_activation_specs(path)` to convert the standard + `[[plugins.dynamic]]` records in one explicitly selected `plugins.toml` into + the activation specs accepted by `initialize_with_dynamic_plugins()`. This + scoped 0.7 compatibility helper removes host-side TOML and manifest parsing; + a future unified file-backed initializer is expected to replace it. - Programmatically declared plugin components now apply their `enabled` value over discovered file configuration. When code re-enables a component that a discovered file disabled, initialization reports a warning that names the diff --git a/docs/configure-plugins/plugin-configuration-files.mdx b/docs/configure-plugins/plugin-configuration-files.mdx index e5a486c9f..203360961 100644 --- a/docs/configure-plugins/plugin-configuration-files.mdx +++ b/docs/configure-plugins/plugin-configuration-files.mdx @@ -163,6 +163,50 @@ manifest’s optional static JSON Schema before you enable or run the plugin. Us dynamic-plugin lifecycle. Refer to [Configure Discoverable Plugins](/configure-plugins/discoverable-plugins) for manifest, trust, and policy requirements. +### Embedded Python Compatibility Helper + +Python hosts that already own plugin activation can convert the dynamic records +from one explicitly selected file into the activation specs accepted by the +0.7 host API: + +```python +import asyncio + +from nemo_relay import plugin + + +async def main() -> None: + dynamic_plugins = plugin.load_dynamic_plugin_activation_specs( + "path/to/plugins.toml" + ) + activation = await plugin.initialize_with_dynamic_plugins({}, dynamic_plugins) + async with activation: + # Run your host application while dynamic plugins are active. + ... + + +asyncio.run(main()) +``` + +The helper resolves each manifest relative to `plugins.toml` and reads the +plugin ID and execution lane from the manifest. It does not perform discovery, +consult CLI lifecycle state, provision a Python worker environment, or change +enablement. + +On success, the helper returns every `[[plugins.dynamic]]` declaration in file +order; it does not skip invalid entries. A missing `plugins.toml` or referenced +manifest raises `FileNotFoundError`. Malformed TOML, invalid records or required +manifest fields, duplicate plugin IDs, and non-JSON configuration raise +`ValueError`. The helper does not apply an optional manifest-declared static +JSON Schema. + +Passing the result to `initialize_with_dynamic_plugins()` is explicit consent +to load those trusted native libraries or worker processes. + +This helper is a 0.7 compatibility surface for embedded integrations and is +planned for deprecation after Relay provides a unified file-backed +initializer. Keep its use localized so migration is straightforward. + The runtime reads only files named `plugins.toml` during default discovery. ## Runtime Discovery diff --git a/python/nemo_relay/plugin.py b/python/nemo_relay/plugin.py index dbd807083..478c30bce 100644 --- a/python/nemo_relay/plugin.py +++ b/python/nemo_relay/plugin.py @@ -11,8 +11,13 @@ from __future__ import annotations import asyncio +import json +import os +import tomllib +from collections.abc import Sequence from contextlib import asynccontextmanager from dataclasses import dataclass, field, fields, is_dataclass +from pathlib import Path from typing import TYPE_CHECKING, AsyncIterator, Callable, Literal, Protocol, Self, TypedDict, cast from nemo_relay import ( @@ -417,6 +422,114 @@ async def __aexit__( await self.close() +def load_dynamic_plugin_activation_specs( + plugin_config_path: str | os.PathLike[str], +) -> list[DynamicPluginActivationSpec]: + """Load dynamic activation specs from one standard ``plugins.toml``. + + Args: + plugin_config_path: Explicit path to the ``plugins.toml`` file. + + Returns: + Activation specs for every ``[[plugins.dynamic]]`` record, in file + order. Manifest paths are resolved relative to ``plugins.toml`` and + each plugin's identifier and execution lane come from its manifest. + + Behavior: + This 0.7 compatibility helper parses one explicit file only. It does + not perform standard discovery, inspect CLI lifecycle state, provision + worker environments, change enablement, or activate plugins. It is + planned for deprecation after a unified file-backed initializer is + available. Keep its use localized and pass the result to + :func:`initialize_with_dynamic_plugins`. + """ + source = Path(os.fspath(plugin_config_path)).resolve() + document = _load_plugin_toml(source, "plugin TOML") + version = document.get("version", 1) + if not isinstance(version, int) or isinstance(version, bool) or version != 1: + raise ValueError(f"plugin config version {version!r} in {source} is unsupported; expected 1") + plugins = document.get("plugins", {}) + if not isinstance(plugins, dict): + raise ValueError(f"invalid dynamic plugin config in {source}: 'plugins' must be a table") + plugins = cast(dict[str, object], plugins) + dynamic_plugins = plugins.get("dynamic", []) + if not isinstance(dynamic_plugins, list): + raise ValueError(f"invalid dynamic plugin config in {source}: 'plugins.dynamic' must be an array of tables") + + specs: list[DynamicPluginActivationSpec] = [] + seen_plugin_ids: set[str] = set() + for index, entry in enumerate(dynamic_plugins): + if not isinstance(entry, dict): + raise ValueError(f"invalid dynamic plugin config in {source}: plugins.dynamic[{index}] must be a table") + entry = cast(dict[str, object], entry) + unknown_fields = sorted(set(entry) - {"manifest", "config"}) + if unknown_fields: + raise ValueError( + f"invalid dynamic plugin config in {source}: plugins.dynamic[{index}] has unknown fields: " + + ", ".join(unknown_fields) + ) + manifest_ref = entry.get("manifest") + if not isinstance(manifest_ref, str) or not manifest_ref.strip(): + raise ValueError( + f"invalid dynamic plugin config in {source}: " + f"plugins.dynamic[{index}].manifest must be a non-empty string" + ) + manifest_path = Path(manifest_ref) + if not manifest_path.is_absolute(): + manifest_path = source.parent / manifest_path + manifest_path = manifest_path.resolve() + + manifest = _load_plugin_toml(manifest_path, "dynamic plugin manifest") + identity = manifest.get("plugin") + if not isinstance(identity, dict): + raise ValueError(f"invalid dynamic plugin manifest in {manifest_path}: 'plugin' must be a table") + identity = cast(dict[str, object], identity) + plugin_id = identity.get("id") + if not isinstance(plugin_id, str) or not plugin_id.strip(): + raise ValueError( + f"invalid dynamic plugin manifest in {manifest_path}: 'plugin.id' must be a non-empty string" + ) + plugin_id = plugin_id.strip() + kind = identity.get("kind") + if kind not in ("rust_dynamic", "worker"): + raise ValueError( + f"invalid dynamic plugin manifest in {manifest_path}: 'plugin.kind' must be 'rust_dynamic' or 'worker'" + ) + if plugin_id in seen_plugin_ids: + raise ValueError(f"duplicate dynamic plugin id {plugin_id!r} in {source}") + seen_plugin_ids.add(plugin_id) + + config = entry.get("config", {}) + if not isinstance(config, dict): + raise ValueError( + f"invalid dynamic plugin config in {source}: plugins.dynamic[{index}].config must be a table" + ) + try: + normalized_config = cast(JsonObject, json.loads(json.dumps(config, allow_nan=False))) + except (TypeError, ValueError) as error: + raise ValueError( + f"invalid dynamic plugin config in {source}: " + f"plugins.dynamic[{index}].config must contain JSON values: {error}" + ) from error + specs.append( + DynamicPluginActivationSpec( + plugin_id=plugin_id, + kind=cast(DynamicPluginKind, kind), + manifest_ref=str(manifest_path), + config=normalized_config, + ) + ) + return specs + + +def _load_plugin_toml(path: Path, description: str) -> dict[str, object]: + try: + with path.open("rb") as file: + return cast(dict[str, object], tomllib.load(file)) + except tomllib.TOMLDecodeError as error: + raise ValueError(f"invalid {description} in {path}: {error}") from error + + def validate(config: PluginConfig | JsonObject) -> ConfigReport: """Validate a plugin configuration without changing runtime state. @@ -452,7 +565,7 @@ async def initialize(config: PluginConfig | JsonObject) -> ConfigReport: async def initialize_with_dynamic_plugins( config: PluginConfig | JsonObject, - dynamic_plugins: list[DynamicPluginActivationSpec | JsonObject], + dynamic_plugins: Sequence[DynamicPluginActivationSpec | JsonObject], ) -> PluginHostActivation: """Initialize registered components with dynamic plugins as one owned host. @@ -607,6 +720,7 @@ def deregister(plugin_kind: str) -> bool: "PluginContext", "PluginHostActivation", "Plugin", + "load_dynamic_plugin_activation_specs", "initialize_with_dynamic_plugins", "clear", "clear_async", diff --git a/python/nemo_relay/plugin.pyi b/python/nemo_relay/plugin.pyi index 39bc464d5..ee8f165dd 100644 --- a/python/nemo_relay/plugin.pyi +++ b/python/nemo_relay/plugin.pyi @@ -1,7 +1,8 @@ # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 -from collections.abc import Callable +import os +from collections.abc import Callable, Sequence from types import TracebackType from typing import AsyncContextManager, Literal, Protocol, Self, TypedDict @@ -160,11 +161,14 @@ class PluginHostActivation: traceback: TracebackType | None, ) -> None: ... +def load_dynamic_plugin_activation_specs( + plugin_config_path: str | os.PathLike[str], +) -> list[DynamicPluginActivationSpec]: ... def validate(config: PluginConfig | JsonObject) -> ConfigReport: ... async def initialize(config: PluginConfig | JsonObject) -> ConfigReport: ... async def initialize_with_dynamic_plugins( config: PluginConfig | JsonObject, - dynamic_plugins: list[DynamicPluginActivationSpec | JsonObject], + dynamic_plugins: Sequence[DynamicPluginActivationSpec | JsonObject], ) -> PluginHostActivation: ... def clear() -> None: ... async def clear_async() -> None: ... diff --git a/python/tests/test_dynamic_plugin_host.py b/python/tests/test_dynamic_plugin_host.py index 97340cec5..f06673032 100644 --- a/python/tests/test_dynamic_plugin_host.py +++ b/python/tests/test_dynamic_plugin_host.py @@ -197,6 +197,175 @@ def test_dynamic_plugin_activation_spec_preserves_nested_json_nulls(): } +def test_load_dynamic_plugin_activation_specs_resolves_standard_toml(tmp_path: Path): + manifests = tmp_path / "plugins" + manifests.mkdir() + native_manifest = manifests / "native.toml" + native_manifest.write_text( + textwrap.dedent( + """ + manifest_version = 1 + + [plugin] + id = "fixture.native" + kind = "rust_dynamic" + """ + ) + ) + worker_manifest = manifests / "worker.toml" + worker_manifest.write_text( + textwrap.dedent( + """ + manifest_version = 1 + + [plugin] + id = "fixture.worker" + kind = "worker" + """ + ) + ) + plugins_toml = tmp_path / "plugins.toml" + plugins_toml.write_text( + textwrap.dedent( + f""" + version = 1 + + [[plugins.dynamic]] + manifest = "plugins/native.toml" + + [plugins.dynamic.config] + mode = "strict" + nested = {{ enabled = true }} + + [[plugins.dynamic]] + manifest = {str(worker_manifest)!r} + """ + ) + ) + + specs = plugin.load_dynamic_plugin_activation_specs(plugins_toml) + + assert [spec.to_dict() for spec in specs] == [ + { + "plugin_id": "fixture.native", + "kind": "rust_dynamic", + "manifest_ref": str(native_manifest.resolve()), + "config": {"mode": "strict", "nested": {"enabled": True}}, + }, + { + "plugin_id": "fixture.worker", + "kind": "worker", + "manifest_ref": str(worker_manifest.resolve()), + "config": {}, + }, + ] + + +@pytest.mark.parametrize( + ("toml_version", "version"), + [("true", True), ("0", 0), ("2", 2), ('"1"', "1")], +) +def test_load_dynamic_plugin_activation_specs_rejects_unsupported_version( + tmp_path: Path, toml_version: str, version: object +): + plugins_toml = tmp_path / "plugins.toml" + plugins_toml.write_text(f"version = {toml_version}\n") + + with pytest.raises(ValueError) as error: + plugin.load_dynamic_plugin_activation_specs(plugins_toml) + assert str(error.value) == ( + f"plugin config version {version!r} in {plugins_toml.resolve()} is unsupported; expected 1" + ) + + +def test_load_dynamic_plugin_activation_specs_rejects_duplicate_ids(tmp_path: Path): + manifest = tmp_path / "relay-plugin.toml" + manifest.write_text("[plugin]\nid = 'duplicate'\nkind = 'rust_dynamic'\n") + plugins_toml = tmp_path / "plugins.toml" + plugins_toml.write_text( + "[[plugins.dynamic]]\nmanifest = 'relay-plugin.toml'\n[[plugins.dynamic]]\nmanifest = 'relay-plugin.toml'\n" + ) + + with pytest.raises(ValueError, match="duplicate dynamic plugin id 'duplicate'"): + plugin.load_dynamic_plugin_activation_specs(plugins_toml) + + +@pytest.mark.parametrize( + ("plugins_toml", "manifest", "message"), + [ + ("plugins = []\n", None, "'plugins' must be a table"), + ("[plugins]\ndynamic = 'invalid'\n", None, "'plugins.dynamic' must be an array of tables"), + ("[plugins]\ndynamic = ['invalid']\n", None, r"plugins.dynamic\[0\] must be a table"), + ( + "[[plugins.dynamic]]\nmanifest = 'relay-plugin.toml'\nunsupported = true\n", + "[plugin]\nid = 'fixture'\nkind = 'rust_dynamic'\n", + "has unknown fields: unsupported", + ), + ("[[plugins.dynamic]]\nmanifest = ''\n", None, "manifest must be a non-empty string"), + ( + "[[plugins.dynamic]]\nmanifest = 'relay-plugin.toml'\n", + "plugin = 'invalid'\n", + "'plugin' must be a table", + ), + ( + "[[plugins.dynamic]]\nmanifest = 'relay-plugin.toml'\n", + "[plugin]\nkind = 'rust_dynamic'\n", + "'plugin.id' must be a non-empty string", + ), + ( + "[[plugins.dynamic]]\nmanifest = 'relay-plugin.toml'\n", + "[plugin]\nid = 'fixture'\nkind = 'invalid'\n", + "'plugin.kind' must be 'rust_dynamic' or 'worker'", + ), + ( + "[[plugins.dynamic]]\nmanifest = 'relay-plugin.toml'\nconfig = 'invalid'\n", + "[plugin]\nid = 'fixture'\nkind = 'rust_dynamic'\n", + "config must be a table", + ), + ( + "[[plugins.dynamic]]\nmanifest = 'relay-plugin.toml'\n" + "[plugins.dynamic.config]\nwhen = 1979-05-27T07:32:00Z\n", + "[plugin]\nid = 'fixture'\nkind = 'rust_dynamic'\n", + "config must contain JSON values", + ), + ], +) +def test_load_dynamic_plugin_activation_specs_rejects_invalid_records( + tmp_path: Path, + plugins_toml: str, + manifest: str | None, + message: str, +): + config_path = tmp_path / "plugins.toml" + config_path.write_text(plugins_toml) + if manifest is not None: + (tmp_path / "relay-plugin.toml").write_text(manifest) + + with pytest.raises(ValueError, match=message): + plugin.load_dynamic_plugin_activation_specs(config_path) + + +@pytest.mark.parametrize("filename", ["plugins.toml", "relay-plugin.toml"]) +def test_load_dynamic_plugin_activation_specs_rejects_invalid_toml(tmp_path: Path, filename: str): + plugins_toml = tmp_path / "plugins.toml" + plugins_toml.write_text("[[plugins.dynamic]]\nmanifest = 'relay-plugin.toml'\n") + (tmp_path / "relay-plugin.toml").write_text("[plugin]\nid = 'fixture'\nkind = 'rust_dynamic'\n") + (tmp_path / filename).write_text("invalid = [\n") + + with pytest.raises(ValueError, match="invalid .* in"): + plugin.load_dynamic_plugin_activation_specs(plugins_toml) + + +def test_load_dynamic_plugin_activation_specs_reports_missing_manifest(tmp_path: Path): + plugins_toml = tmp_path / "plugins.toml" + plugins_toml.write_text("[[plugins.dynamic]]\nmanifest = 'missing/relay-plugin.toml'\n") + missing_manifest = (tmp_path / "missing" / "relay-plugin.toml").resolve() + + with pytest.raises(FileNotFoundError) as error: + plugin.load_dynamic_plugin_activation_specs(plugins_toml) + assert Path(error.value.filename) == missing_manifest + + def test_validate_omits_raw_plugin_config_nulls_but_preserves_component_config_nulls( monkeypatch: pytest.MonkeyPatch, ): @@ -332,6 +501,9 @@ def register(self, _plugin_config, context): [[components]] kind = {static_kind!r} enabled = true + + [[plugins.dynamic]] + manifest = {str(native_dynamic_plugin.manifest)!r} """ ) ) @@ -343,7 +515,8 @@ def register(self, _plugin_config, context): plugin.register(static_kind, cast(plugin.Plugin, FileStaticPlugin())) activation = None try: - activation = await plugin.initialize_with_dynamic_plugins(plugin.PluginConfig(), [native_dynamic_plugin.spec()]) + dynamic_plugins = plugin.load_dynamic_plugin_activation_specs(plugins_toml) + activation = await plugin.initialize_with_dynamic_plugins(plugin.PluginConfig(), tuple(dynamic_plugins)) assert activation.report == { "diagnostics": [ {