Skip to content

feat(relay): add Switchyard-owned HTTP dynamic plugin - #270

Open
bbednarski9 wants to merge 13 commits into
NVIDIA-NeMo:mainfrom
bbednarski9:feat/nemo-relay-plugin-owned-http-client
Open

feat(relay): add Switchyard-owned HTTP dynamic plugin#270
bbednarski9 wants to merge 13 commits into
NVIDIA-NeMo:mainfrom
bbednarski9:feat/nemo-relay-plugin-owned-http-client

Conversation

@bbednarski9

@bbednarski9 bbednarski9 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What

Adds the external nvidia.switchyard NeMo Relay native plugin. The plugin embeds
switchyard-libsy, drives Algorithm::run_stream, and uses
switchyard-llm-client for provider HTTP dispatch rather than depending on a
Switchyard service or a Relay targeted-provider continuation.

The initial integration supports seeded weighted-random and LLM-classifier
routing for buffered and streaming OpenAI Chat, OpenAI Responses, and Anthropic
Messages traffic. It consumes the stream-preservation contract landed in #192,
and sends buffered final-response failures through the existing routing error
and exactly-once trusted-fallback path.

Why

Switchyard algorithms need to own the complete run_stream lifecycle so future
policies can inspect intermediate responses and make multiple provider calls.
Owning provider HTTP dispatch inside the plugin keeps that lifecycle intact
while using NeMo Relay's released native API v1 Rust SDK. It removes the need
for the targeted LLM continuation contract proposed in NVIDIA/NeMo-Relay#594.

Related: #192, #220, #271, #274, NVIDIA/NeMo-Relay#594

How tested

  • uv run ruff check . clean (N/A: Rust-only plugin)
  • uv run mypy switchyard clean (N/A: Rust-only plugin)
  • uv run pytest tests/ green (N/A: Rust-only plugin)
  • cargo test -p switchyard-nemo-relay-plugin — 29 focused unit tests
  • cargo test --workspace --all-targets
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo fmt --all -- --check
  • git diff --check

Checklist

  • One class per file; filename = snake_case of the primary class. (N/A: no Python classes added.)
  • New public symbols exported from switchyard/__init__.py.__all__ if intended for downstream use. (N/A: Rust plugin surface.)
  • Unit tests added for new components / bug fixes.
  • README / --help updated if customer-facing surface changed.
  • Commits signed off (Signed-off-by: Your Name <email>) per the DCO.

Notes for reviewers

  • This PR intentionally pins the published Relay 0.7.0-rc.5 crate and
    declares >=0.7.0-rc.5,<1.0. These move to stable 0.7.0 before merge. RC5
    also removes the platform-specific timezone dependency expansion present in
    the RC4 lockfile.
  • Managed calls bypass Relay middleware registered after the Switchyard
    intercept and Relay's provider callback. Their HTTP subcalls therefore do not
    create nested Relay LLM lifecycle spans; the outer LLM span and genuine
    Switchyard routing marks remain in the same exported trace.
  • With feat(translation): preserve raw stream events #192 landed, same-protocol streams replay each preserved provider JSON
    event, including provider-specific fields. This preserves parsed JSON, not raw
    SSE bytes or framing. Cross-protocol streams still use normalized chunks
    without reject-lossy diagnostics; replacing normalized content or aggregating
    the stream drops its per-event preservation envelope.
  • Target URLs with query parameters, including Azure-style api-version, are
    rejected by the initial client contract.

Review context

This PR is the core Switchyard -> NeMo Relay integration PR.

  • I reduced this down to just the core plugin code.
  • I removed the E2E tests.
  • I upstreamed the Relay Cargo.lock indirect dependencies that were pointed out earlier.
  • The PR currently provides a source-built cdylib and package_bundle.py to produce an installable bundle containing the shared library, manifest, schema, and integrity digest; future work can publish the supporting crates.
  • I know it's XL, but outside of Cargo.toml, the changes are isolated to the new crates/switchyard-nemo-relay-plugin crate.
  • It's currently pinned to NeMo Relay v0.7.0-rc.5, which should be promoted to the official release on Wednesday, and I'll update it then.

Adjacent hardening

This PR is self-contained. Two independent follow-ups deliberately keep broader
library changes out of its review scope:

@bbednarski9
bbednarski9 force-pushed the feat/nemo-relay-plugin-owned-http-client branch from 0e3eb27 to 58b3186 Compare August 3, 2026 23:35
@bbednarski9
bbednarski9 marked this pull request as ready for review August 4, 2026 01:49
@bbednarski9
bbednarski9 requested a review from a team as a code owner August 4, 2026 01:49
@bbednarski9
bbednarski9 force-pushed the feat/nemo-relay-plugin-owned-http-client branch from 58b3186 to 3dcee4d Compare August 4, 2026 01:53
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The pull request adds a native Switchyard NeMo Relay plugin with configuration, routing, translation, asynchronous host integration, streaming support, packaging, and documentation. It also adds HTTP transport limits, redirect rejection, timeout defaults, header redaction, and safer error reporting.

Changes

Transport safety

Layer / File(s) Summary
Provider transport safeguards
crates/libsy-llm-client/*, crates/protocol/src/client.rs, crates/switchyard-translation/src/helpers.rs
Provider requests reject redirects, use connection and read timeouts, limit response bodies and SSE frames, redact header values, and hide raw error content from display messages. Tests cover these limits and error variants.

NeMo Relay plugin

Layer / File(s) Summary
Configuration and provider targets
crates/switchyard-nemo-relay-plugin/config.schema.json, crates/switchyard-nemo-relay-plugin/src/config.rs, crates/switchyard-nemo-relay-plugin/src/client.rs, crates/switchyard-nemo-relay-plugin/src/translation.rs
The plugin validates versioned configuration, secure URLs and headers, prepares routed provider clients, supports random and LLM-classifier routing, and translates requests and responses.
Executor and native host interop
crates/switchyard-nemo-relay-plugin/src/executor.rs, crates/switchyard-nemo-relay-plugin/src/ffi.rs
A dedicated Tokio executor runs plugin tasks. FFI helpers manage host resources, buffered and streaming calls, cancellation, backpressure, completion, and cleanup.
Routing and response runtime
crates/switchyard-nemo-relay-plugin/src/runtime.rs
The runtime decodes requests, performs routed calls and retries, emits routing metadata, handles fallback targets, propagates context, and encodes buffered or streaming responses.
Native registration and packaging
crates/switchyard-nemo-relay-plugin/src/lib.rs, crates/switchyard-nemo-relay-plugin/Cargo.toml, crates/switchyard-nemo-relay-plugin/relay-plugin.toml, crates/switchyard-nemo-relay-plugin/config.schema.json, crates/switchyard-nemo-relay-plugin/scripts/package_bundle.py
The crate registers native middleware, validates host ABI and configuration, forwards unmanaged protocols, and packages the dynamic library with its schema and manifest.
Documentation and workspace wiring
Cargo.toml, README.md, docs/index.md, crates/switchyard-nemo-relay-plugin/README.md, CHANGELOG.md
Workspace, project, package, and changelog documentation now reference the NeMo Relay plugin and its transport behavior.

Estimated code review effort: 5 (Critical) | ~120 minutes

Poem

A rabbit checks the routes at night,
With tiny paws, the headers right.
Streams stay bounded, errors tame,
Relay hops through every frame.
“Ship the plugin!” the rabbit sings,
While Tokio hums on careful wings.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 45.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the addition of the Switchyard-owned HTTP dynamic Relay plugin.
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch feat/nemo-relay-plugin-owned-http-client

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (12)
crates/switchyard-nemo-relay-plugin/src/config.rs (2)

411-421: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Extend the sensitive-header check beyond the exact-match list.

is_sensitive_target_header uses a fixed denylist. A provider credential header outside that list, for example x-provider-token or openai-api-key, still passes validate_headers and gets stored as plaintext in Relay configuration. The guard at Line 92 exists to prevent exactly that.

Add a substring heuristic so unlisted credential headers also route through header_env.

🔒 Proposed change
 fn is_sensitive_target_header(name: &str) -> bool {
     matches!(
         name,
         "authorization"
             | "cookie"
             | "x-api-key"
             | "api-key"
             | "anthropic-api-key"
             | "x-goog-api-key"
     ) || name.contains("api-key")
         || name.contains("api_key")
         || name.contains("token")
         || name.contains("secret")
         || name.contains("password")
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/src/config.rs` around lines 411 - 421,
Update is_sensitive_target_header to retain the existing exact-match denylist
and also return true when the header name contains a credential-related
substring, such as “api-key” or “token,” so unlisted provider credential headers
are routed through header_env by validate_headers instead of stored as
plaintext.

700-726: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add a regression test for an invalid environment-supplied header value.

The tests cover an unset variable and invalid variable names. They do not cover validate_header(name, &value) at Line 129, which rejects a malformed value read from the environment, for example a value that contains a newline or a control character. That path blocks header injection into the provider request.

Add a case that sets a variable to an invalid value and asserts that prepare() fails.

Based on learnings from the coding guidelines: "Define verifiable success criteria, write regression tests for bugs and invalid inputs, and verify each implementation step."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/src/config.rs` around lines 700 - 726,
Add a regression test alongside
validation_does_not_resolve_environment_backed_headers and
invalid_environment_variable_names_are_rejected_before_resolution that sets the
referenced environment variable to a malformed header value, such as one
containing a newline or control character, then calls config.prepare() and
asserts it returns an error. Keep the existing variable-name validation coverage
unchanged and verify the failure identifies the invalid header value.

Source: Coding guidelines

crates/switchyard-nemo-relay-plugin/src/translation.rs (2)

60-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a test for the Anthropic JSON-schema rejection.

request_policy is the runtime backstop that stops a JSON-schema response format from reaching an anthropic_messages target. config.rs at Line 311 rejects only an Anthropic classifier target at configuration time, so this policy is the sole guard for a routed Anthropic target. No test covers it.

Add a case that builds an LlmRequest with a JSON-schema response format, then asserts validate_target_request fails for WireFormat::AnthropicMessages and succeeds for WireFormat::OpenAiChat.

As per coding guidelines: "Define verifiable success criteria, write regression tests for bugs and invalid inputs, and verify each implementation step."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/src/translation.rs` around lines 60 - 68,
Add a regression test covering request_policy’s JSON-schema capability
restriction: construct an LlmRequest using a JSON-schema response format, assert
validate_target_request rejects it for WireFormat::AnthropicMessages, and assert
validation succeeds for WireFormat::OpenAiChat.

Source: Coding guidelines


1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Two new modules in this crate have no //! module comment. client.rs states its intent at Line 4, but translation.rs and runtime.rs do not. The shared root cause is one missing convention pass over the new modules.

  • crates/switchyard-nemo-relay-plugin/src/translation.rs#L1-L11: add a //! comment stating that the module adapts Relay request and response bodies to Switchyard protocol types and applies the plugin translation policies.
  • crates/switchyard-nemo-relay-plugin/src/runtime.rs#L1-L19: add a //! comment stating that the module decodes inbound requests, drives the libsy algorithm with retries and a trusted fallback, and encodes buffered or streaming responses back to the host.

As per coding guidelines: "For Rust changes, document public items with /// comments and add concise comments for module intent, private helpers with non-obvious behavior, important tests, and complex validation, routing, configuration, async, lifecycle, or concurrency logic."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/src/translation.rs` around lines 1 - 11,
Add concise //! module documentation to
crates/switchyard-nemo-relay-plugin/src/translation.rs#L1-L11 describing
adaptation between Relay request/response bodies and Switchyard protocol types,
including application of plugin translation policies. Also document
crates/switchyard-nemo-relay-plugin/src/runtime.rs#L1-L19 with its role in
decoding inbound requests, driving libsy with retries and a trusted fallback,
and encoding buffered or streaming responses; no other changes are needed.

Source: Coding guidelines

crates/switchyard-nemo-relay-plugin/src/runtime.rs (2)

365-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use tracing instead of eprintln!.

This code runs inside a plugin loaded by the Relay host. eprintln! writes to raw stderr, so the message bypasses the host log pipeline and carries no level or structured fields. The workspace already uses tracing, for example the spans in crates/libsy-llm-client/src/client.rs.

Replace the call with tracing::warn!.

♻️ Proposed change
         if let Err(error) = parent.emit_mark(name, &data, metadata) {
-            eprintln!("Switchyard could not emit routing mark {name:?}: {error}");
+            tracing::warn!(mark = name, %error, "Switchyard could not emit routing mark");
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/src/runtime.rs` around lines 365 - 370,
Update the error handling in mark to replace the raw eprintln! call with
tracing::warn!, preserving the existing routing-mark error message and including
the name and error fields in the structured log.

565-601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add unit tests for the retry and stream helpers.

This module holds the retry, fallback, and streaming state machine, and has one test that covers a pure helper. libsy_error_retryable, failure_mark_data, and returned_events need no Relay host and are testable now.

Cover at minimum:

  • libsy_error_retryable returns true for each listed status and false for 400, 401, and 404.
  • returned_events rejects an empty LlmResponse::Stream and preserves the first chunk otherwise.
  • failure_mark_data sets failure_kind to http, non_http, and algorithm for the three branches.

These tests would have caught the two defects flagged at Lines 134-197 and Lines 259-266.

As per coding guidelines: "Define verifiable success criteria, write regression tests for bugs and invalid inputs, and verify each implementation step."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/src/runtime.rs` around lines 565 - 601,
Extend the existing tests module with unit tests for the pure helpers
libsy_error_retryable, returned_events, and failure_mark_data. Verify
retryability for every listed status plus false for 400, 401, and 404; ensure
returned_events rejects an empty LlmResponse::Stream and retains the first chunk
for a non-empty stream; and assert failure_mark_data produces http, non_http,
and algorithm for its three branches without requiring a Relay host.

Source: Coding guidelines

crates/switchyard-nemo-relay-plugin/scripts/package_bundle.py (2)

16-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider hashlib.file_digest instead of the manual chunk loop.

The coding guidelines target Python 3.12+. hashlib.file_digest is available from 3.11 and removes the read loop.

♻️ Proposed change
 def digest(path: Path) -> str:
     """Return the lowercase SHA-256 digest for a file."""
-    value = hashlib.sha256()
-    with path.open("rb") as stream:
-        for chunk in iter(lambda: stream.read(1024 * 1024), b""):
-            value.update(chunk)
-    return value.hexdigest()
+    with path.open("rb") as stream:
+        return hashlib.file_digest(stream, "sha256").hexdigest()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/scripts/package_bundle.py` around lines
16 - 22, Update the digest function to use hashlib.file_digest with the opened
file stream and SHA-256, replacing the manual chunk-reading loop while
preserving the lowercase hexadecimal digest returned by hexdigest().

Source: Coding guidelines


36-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Validate config.schema.json before the script copies files.

The script validates the library and the manifest placeholders before it mutates the output directory. It does not validate config.schema.json. If that file is missing, shutil.copy2 raises FileNotFoundError after the library copy already ran. The output directory is then partially populated and no longer empty, so a rerun fails the emptiness check.

♻️ Proposed change
     manifest = (CRATE_ROOT / "relay-plugin.toml").read_text(encoding="utf-8")
     placeholders = ("<platform-library-file>", "<artifact-sha256>")
     missing = [placeholder for placeholder in placeholders if placeholder not in manifest]
     if missing:
         parser.error(f"plugin manifest is missing placeholders: {', '.join(missing)}")
 
+    schema = CRATE_ROOT / "config.schema.json"
+    if not schema.is_file():
+        parser.error(f"plugin configuration schema does not exist: {schema}")
+
     output = args.output.resolve()
@@
     shutil.copy2(library, artifact)
-    shutil.copy2(CRATE_ROOT / "config.schema.json", output / "config.schema.json")
+    shutil.copy2(schema, output / "config.schema.json")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/scripts/package_bundle.py` around lines
36 - 51, Validate the existence and required file condition for
config.schema.json before creating or mutating the output directory in the
packaging flow. Update the logic around the existing manifest and library
validation, using the config.schema.json source path, so missing-file errors are
reported through parser.error before shutil.copy2 performs either copy; preserve
the existing output-directory checks and copy behavior otherwise.
crates/switchyard-nemo-relay-plugin/src/executor.rs (1)

113-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a bounded wait in this test.

receiver.recv() blocks forever if the spawned task never runs. The test then hangs CI instead of failing. The second test already uses recv_timeout. Apply the same pattern here.

♻️ Proposed change
-        assert_eq!(receiver.recv().unwrap(), "done");
+        assert_eq!(
+            receiver
+                .recv_timeout(std::time::Duration::from_secs(5))
+                .expect("spawned work must complete"),
+            "done"
+        );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/src/executor.rs` around lines 113 - 121,
Update executor_runs_buffered_and_spawned_work to replace the unbounded
receiver.recv() call with receiver.recv_timeout(), using the same bounded-wait
pattern and timeout established by the neighboring test.
crates/switchyard-nemo-relay-plugin/src/lib.rs (2)

365-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a test for an unsupported future version.

The tests cover version 1 and a non-integer version. The Some(version) branch for any other integer is untested. Add a case for version = 3 and assert the "unsupported Switchyard config version" message. The coding guidelines require regression tests for invalid inputs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/src/lib.rs` around lines 365 - 401, Add a
regression test alongside
version_one_service_config_gets_a_migration_error_before_v2_deserialization and
version_must_be_an_integer that passes {"version": 3} to parse_config, asserts
parsing fails, and verifies the error contains the “unsupported Switchyard
config version” message.

Source: Coding guidelines


4-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add module intent comments to the new modules. The new crate omits module-level comments in two files. ffi.rs includes a //! comment; these two do not.

  • crates/switchyard-nemo-relay-plugin/src/lib.rs#L4-L9: add a crate-level //! comment that states the plugin's purpose and the host ABI it targets.
  • crates/switchyard-nemo-relay-plugin/src/executor.rs#L4-L11: add a //! comment that states why the plugin owns a dedicated Tokio runtime thread.

The coding guidelines require concise comments for module intent in crates/**/*.rs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/src/lib.rs` around lines 4 - 9, Add
concise module-level intent comments at both affected sites: in
crates/switchyard-nemo-relay-plugin/src/lib.rs lines 4-9, add a crate-level //!
comment describing the plugin’s purpose and targeted host ABI; in
crates/switchyard-nemo-relay-plugin/src/executor.rs lines 4-11, add a //!
comment explaining why the plugin owns a dedicated Tokio runtime thread.

Source: Coding guidelines

crates/switchyard-nemo-relay-plugin/src/ffi.rs (1)

323-337: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider the wakeup cost of cancellation polling at high concurrency.

Each in-flight call adds one timer wakeup every 10 ms on the two-worker runtime. With thousands of concurrent calls this becomes a constant background load. The host table exposes no cancellation notification, so polling is reasonable now. Consider a backoff that starts short and grows to a longer interval, or make the interval configurable.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/switchyard-nemo-relay-plugin/src/ffi.rs` around lines 323 - 337,
Reduce cancellation polling overhead in wait_for_completion_cancellation and
wait_for_stream_cancellation by replacing the fixed CANCELLATION_POLL delay with
a short initial interval that backs off to a configurable or bounded maximum.
Preserve prompt cancellation detection while preventing thousands of in-flight
calls from waking every 10 ms indefinitely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/libsy-llm-client/src/client.rs`:
- Around line 285-306: The response handling tests need coverage for an
oversized successful response. Add a regression test in the existing client test
suite that returns a 2xx response whose body exceeds
MAX_BUFFERED_RESPONSE_BYTES, then assert the request fails with
LlmClientError::InvalidResponse while preserving the existing oversized
error-body tests.

In `@crates/switchyard-nemo-relay-plugin/src/ffi.rs`:
- Around line 368-423: Bound the Internal-status retry loops in push_stream and
reject_stream using the existing timing imports and a shared
MAX_BACKPRESSURE_WAIT duration near the other limits. Stop retrying once the
deadline is reached and return an error/status that lets the callback settle the
stream, while preserving cancellation handling and normal successful or
non-Internal responses.

In `@crates/switchyard-nemo-relay-plugin/src/runtime.rs`:
- Around line 372-392: The emit_decision method currently sends prompt-derived
decision.reasoning through ParentScope::emit_mark; constrain this field before
including it in the routing mark. Prefer truncating reasoning to a fixed maximum
length (or gate it behind an off-by-default configuration flag), while
preserving identifier-only metadata and existing decision fields.
- Around line 92-100: Add exponential backoff before retry iterations in the
routing retry loops, including both non-streaming and streaming paths such as
the visible retry branch and execute_stream. When the upstream failure includes
a Retry-After value, use that delay instead of the calculated backoff; otherwise
apply the existing retry-attempt count to compute an exponentially increasing
sleep before re-driving the request.
- Around line 259-266: Bound the outer event-processing loop in the runtime flow
around committed and retry handling so a pass that ends with committed == false
and no retry arm cannot restart indefinitely. Track whether a retry occurred
during the pass, or otherwise detect that no progress was made, and return an
appropriate error before re-entering the outer loop; preserve normal retry and
successful commitment behavior.
- Around line 134-197: Make the fallback_used binding mutable in the surrounding
request loop, and set it to true in the Err(failure) if !fallback_used arm
immediately before switching to the trusted fallback stream via
fallback_response. Preserve the existing retry and error handling for failures
that occur before fallback activation.

---

Nitpick comments:
In `@crates/switchyard-nemo-relay-plugin/scripts/package_bundle.py`:
- Around line 16-22: Update the digest function to use hashlib.file_digest with
the opened file stream and SHA-256, replacing the manual chunk-reading loop
while preserving the lowercase hexadecimal digest returned by hexdigest().
- Around line 36-51: Validate the existence and required file condition for
config.schema.json before creating or mutating the output directory in the
packaging flow. Update the logic around the existing manifest and library
validation, using the config.schema.json source path, so missing-file errors are
reported through parser.error before shutil.copy2 performs either copy; preserve
the existing output-directory checks and copy behavior otherwise.

In `@crates/switchyard-nemo-relay-plugin/src/config.rs`:
- Around line 411-421: Update is_sensitive_target_header to retain the existing
exact-match denylist and also return true when the header name contains a
credential-related substring, such as “api-key” or “token,” so unlisted provider
credential headers are routed through header_env by validate_headers instead of
stored as plaintext.
- Around line 700-726: Add a regression test alongside
validation_does_not_resolve_environment_backed_headers and
invalid_environment_variable_names_are_rejected_before_resolution that sets the
referenced environment variable to a malformed header value, such as one
containing a newline or control character, then calls config.prepare() and
asserts it returns an error. Keep the existing variable-name validation coverage
unchanged and verify the failure identifies the invalid header value.

In `@crates/switchyard-nemo-relay-plugin/src/executor.rs`:
- Around line 113-121: Update executor_runs_buffered_and_spawned_work to replace
the unbounded receiver.recv() call with receiver.recv_timeout(), using the same
bounded-wait pattern and timeout established by the neighboring test.

In `@crates/switchyard-nemo-relay-plugin/src/ffi.rs`:
- Around line 323-337: Reduce cancellation polling overhead in
wait_for_completion_cancellation and wait_for_stream_cancellation by replacing
the fixed CANCELLATION_POLL delay with a short initial interval that backs off
to a configurable or bounded maximum. Preserve prompt cancellation detection
while preventing thousands of in-flight calls from waking every 10 ms
indefinitely.

In `@crates/switchyard-nemo-relay-plugin/src/lib.rs`:
- Around line 365-401: Add a regression test alongside
version_one_service_config_gets_a_migration_error_before_v2_deserialization and
version_must_be_an_integer that passes {"version": 3} to parse_config, asserts
parsing fails, and verifies the error contains the “unsupported Switchyard
config version” message.
- Around line 4-9: Add concise module-level intent comments at both affected
sites: in crates/switchyard-nemo-relay-plugin/src/lib.rs lines 4-9, add a
crate-level //! comment describing the plugin’s purpose and targeted host ABI;
in crates/switchyard-nemo-relay-plugin/src/executor.rs lines 4-11, add a //!
comment explaining why the plugin owns a dedicated Tokio runtime thread.

In `@crates/switchyard-nemo-relay-plugin/src/runtime.rs`:
- Around line 365-370: Update the error handling in mark to replace the raw
eprintln! call with tracing::warn!, preserving the existing routing-mark error
message and including the name and error fields in the structured log.
- Around line 565-601: Extend the existing tests module with unit tests for the
pure helpers libsy_error_retryable, returned_events, and failure_mark_data.
Verify retryability for every listed status plus false for 400, 401, and 404;
ensure returned_events rejects an empty LlmResponse::Stream and retains the
first chunk for a non-empty stream; and assert failure_mark_data produces http,
non_http, and algorithm for its three branches without requiring a Relay host.

In `@crates/switchyard-nemo-relay-plugin/src/translation.rs`:
- Around line 60-68: Add a regression test covering request_policy’s JSON-schema
capability restriction: construct an LlmRequest using a JSON-schema response
format, assert validate_target_request rejects it for
WireFormat::AnthropicMessages, and assert validation succeeds for
WireFormat::OpenAiChat.
- Around line 1-11: Add concise //! module documentation to
crates/switchyard-nemo-relay-plugin/src/translation.rs#L1-L11 describing
adaptation between Relay request/response bodies and Switchyard protocol types,
including application of plugin translation policies. Also document
crates/switchyard-nemo-relay-plugin/src/runtime.rs#L1-L19 with its role in
decoding inbound requests, driving libsy with retries and a trusted fallback,
and encoding buffered or streaming responses; no other changes are needed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3570ba9b-fc47-4a61-b0ff-3d3a77c017b1

📥 Commits

Reviewing files that changed from the base of the PR and between 091bc89 and 58b3186.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock, !Cargo.lock
📒 Files selected for processing (22)
  • CHANGELOG.md
  • Cargo.toml
  • README.md
  • crates/libsy-llm-client/README.md
  • crates/libsy-llm-client/src/backend.rs
  • crates/libsy-llm-client/src/client.rs
  • crates/libsy/src/core/algorithm.rs
  • crates/protocol/src/client.rs
  • crates/switchyard-nemo-relay-plugin/Cargo.toml
  • crates/switchyard-nemo-relay-plugin/README.md
  • crates/switchyard-nemo-relay-plugin/config.schema.json
  • crates/switchyard-nemo-relay-plugin/relay-plugin.toml
  • crates/switchyard-nemo-relay-plugin/scripts/package_bundle.py
  • crates/switchyard-nemo-relay-plugin/src/client.rs
  • crates/switchyard-nemo-relay-plugin/src/config.rs
  • crates/switchyard-nemo-relay-plugin/src/executor.rs
  • crates/switchyard-nemo-relay-plugin/src/ffi.rs
  • crates/switchyard-nemo-relay-plugin/src/lib.rs
  • crates/switchyard-nemo-relay-plugin/src/runtime.rs
  • crates/switchyard-nemo-relay-plugin/src/translation.rs
  • crates/switchyard-translation/src/helpers.rs
  • docs/index.md

Comment thread crates/libsy-llm-client/src/client.rs Outdated
Comment thread crates/switchyard-nemo-relay-plugin/src/ffi.rs Outdated
Comment thread crates/switchyard-nemo-relay-plugin/src/runtime.rs
Comment thread crates/switchyard-nemo-relay-plugin/src/runtime.rs Outdated
Comment thread crates/switchyard-nemo-relay-plugin/src/runtime.rs
Comment thread crates/switchyard-nemo-relay-plugin/src/runtime.rs
@bbednarski9
bbednarski9 force-pushed the feat/nemo-relay-plugin-owned-http-client branch from 3dcee4d to 1815a45 Compare August 4, 2026 02:06
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
@bbednarski9

Copy link
Copy Markdown
Contributor Author

switchyard-rust-review evidence

I reviewed PR #270 at 3dcee4d6 against main at a9c04b31, following the repository's switchyard-rust-review skill. I completed separate passes for correctness, async/cancellation behavior, streaming and fallback state, protocol/HTTP boundaries, security and credential handling, allocation/dependency choices, comments/naming, tests, and then a second focused pass over every changed hunk.

Verdict: changes requested

High

  1. Managed calls can exhaust Relay's normal Tokio worker pool. PluginExecutor::run synchronously waits on an mpsc receiver, and SwitchyardStream::next blocks in recv_blocking. Relay 0.7 invokes these safe native callbacks on its async runtime workers, not its blocking pool. Enough slow buffered calls or idle streams can therefore occupy every worker and stall unrelated gateway work and disconnect processing. The README accurately documents the limitation and TOKIO_WORKER_THREADS mitigation, but pool sizing does not restore cancellation or non-blocking behavior. This needs explicit maintainer acceptance as a production limitation, or an async/yielding SDK boundary before broad deployment.

  2. A fallback stream can invoke trusted fallback twice. When returned_events rejects the initially selected response, the branch at runtime.rs:175-205 replaces it with a fallback response but leaves fallback_used as false. If that fallback stream then fails before its first event, the !fallback_used guards at runtime.rs:219-256 call the trusted fallback a second time. Model fallback as mutable/explicit state and add a regression test with an invalid selected stream followed by a pre-commit fallback-stream failure.

  3. Buffered final-response translation failures bypass trusted fallback. The success branch returns translation::encode_response(...) directly at runtime.rs:95-100; only errors from drive reach the fallback branch at runtime.rs:102-120. A selected cross-protocol response that cannot be represented losslessly therefore fails the outer call even though no caller response has been committed and a same-protocol trusted fallback is configured. Feed final encode failures through the same error/fallback state machine and test an unsupported cross-protocol response feature.

Medium

  1. Streaming error/fallback marks are lost if the fallback HTTP call itself fails. The code records routing.error, and fallback_response records routing.fallback, but both are only flushed after the awaited fallback returns successfully at runtime.rs:159-173. The ? path sends only StreamMessage::Error, so the resulting trajectory omits the real routing failure and fallback attempt. Ensure accumulated marks are flushed on every terminal path, including fallback setup/HTTP failure.

  2. Plugin transport policy silently changes every existing TranslatingLlmClient consumer. The PR adds fixed global connect/read timeouts in client.rs:54-56 and applies them in the only public constructor at client.rs:98-113, alongside new global response-size and redirect behavior. switchyard-server and other library users call this same constructor, so a plugin-specific safety policy can now terminate an existing stream after 120 seconds without any configuration or opt-out. Preserve prior shared-client behavior and inject/configure the stricter policy for the plugin, or explicitly approve and document this as a crate-wide contract change.

  3. The routing state machine has no checked-in behavioral regression coverage. The 641-line runtime's only test is the metadata-copy test at runtime.rs:605-640; there is no crate tests/ suite that loads the produced cdylib through Relay. Config/helper unit tests and manual E2E evidence do not protect retry reselection, exactly-once fallback, stream commitment, late errors, routing marks, or the SDK/manifest loading boundary in future changes. Add focused runtime tests for the branches above and an automated dynamic-load smoke test using the published Relay 0.7 surface.

Low

  1. Mark-emission failures bypass structured tracing. emit_mark writes directly with eprintln!, which cannot be filtered or correlated and may interleave under concurrency. Use a structured tracing event with the mark name and error as fields.

Positive checks

  • Production code contains no unwrap/expect calls and no custom raw C/FFI adapter; the plugin uses Relay's typed Rust SDK.
  • The provider client clears inbound transport headers before target dispatch, validates target URLs/headers, rejects redirects, bounds response/error data, and redacts configured header values from Debug.
  • Streaming uses a bounded 32-message channel and aborts unfinished producer work when the iterator can be dropped.
  • The source changes are clean under git diff --check.

Validation performed at this head

  • cargo test -p switchyard-translation -p switchyard-llm-client -p switchyard-nemo-relay-plugin — passed.
  • cargo fmt --all -- --check — passed.
  • cargo clippy --workspace --all-targets -- -D warnings — passed.
  • GitHub's aggregate CI Success check is green at 3dcee4d6, including the Ubuntu workspace test job.

Release condition already documented by the PR: replace the development nemo-relay-plugin = 0.7.0-rc.4 dependency and manifest lower bound with stable 0.7.0 before publication.

Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
@bbednarski9

Copy link
Copy Markdown
Contributor Author

comment#1: We documented the limitation in f28309b and clarified it in f035e0d

The durable fix requires an async/yielding Relay SDK surface. This is out of scope for Relay 0.7. If it proves to be an issue after initial integration, we can upstream to Relay

Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants