feat(cli)!: add MCP-managed shared gateway for coding agents#395
feat(cli)!: add MCP-managed shared gateway for coding agents#395willkill07 wants to merge 102 commits into
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (4)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (4)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR restructures the CLI into an ChangesCodex plugin_shim trust flow and MCP packaging
Estimated code review effort: 5 (Critical) | ~180 minutes Persistent agent integration platform
Test coverage for restructured CLI
Documentation and packaging assets
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:XL
willkill07
force-pushed
the
wkk_relay-447-codex-bootstrap
branch
from
37d6608 to
d8740a1
Compare
willkill07
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cli/src/plugin_shim/shared.rs`:
- Around line 41-60: The sidecar lock acquisition loop in shared.rs can time out
before stale-lock recovery becomes eligible, so adjust the timing constants used
by ensure_sidecar and related lock handling: either increase
SIDECAR_LOCK_TIMEOUT to be at least as long as STALE_LOCK_AFTER, or reduce the
stale threshold so repair_stale_lock can reclaim an orphaned lock within the
same call. Keep the behavior around fs::create_dir, healthz, repair_stale_lock,
and sidecar_start_error consistent so retryable hook calls do not fail early
with “sidecar lock timed out”.
In `@README.md`:
- Around line 123-126: The README warning uses trust-state wording too broadly
and should match CodexHookTrustReport terminology. Update the text around the
install and doctor guidance so it distinguishes modified hooks from untrusted
hooks instead of collapsing them into one state, and make sure the wording
matches what nemo-relay doctor actually reports for generated, modified, and
manual/source-marketplace hooks.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: f2e7bcd6-9e1a-48bf-93b6-e41075a2cd5d
📒 Files selected for processing (11)
README.mdcrates/cli/src/plugin_shim/codex.rscrates/cli/src/plugin_shim/codex_app_server.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/tests/coverage/plugin_shim_tests.rscrates/cli/tests/coverage/server_tests.rsdocs/nemo-relay-cli/plugin-installation.mdxintegrations/coding-agents/codex/README.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (25)
**/*.{md,rst,html,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
**/*.{md,rst,html,txt}: Always spellNVIDIAin all caps. Do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun because the name starts with an 'en' sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company.
Use trademark symbols with product names only when the document type or legal guidance requires them.
Verify official capitalization, spacing, and hyphenation for product names.
Precede NVIDIA product names withNVIDIAon first mention when it is natural and accurate.
Do not rewrite product names for grammar or title-case rules.
Preserve third-party product names according to the owner's spelling.
Include the company name and full model qualifier on first use when it helps identify the model.
Preserve the official capitalization and punctuation of model names.
Use shorter family names only after the full name is established.
Spell out a term on first use and put the acronym in parentheses unless the acronym is widely understood by the intended audience.
Use the acronym on later mentions after it has been defined.
For long documents, reintroduce the full term if readers might lose context.
Form plurals of acronyms withs, not an apostrophe, such asGPUs.
In headings, common acronyms can remain abbreviated. Spell out the term in the first or second sentence of the body.
Common terms such asCPU,GPU,PC,API, andUIusually do not need to be spelled out for developer audiences.
Files:
README.mdintegrations/coding-agents/codex/README.md
**/*.{md,rst,html}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Link the first mention of a product name when the destination helps the reader.
Files:
README.mdintegrations/coding-agents/codex/README.md
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Spell
NVIDIAin all caps. Do not useNvidia,nvidia, orNV.
Files:
README.mdintegrations/coding-agents/codex/README.md
**/*.{md,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
**/*.{md,rst}: Format commands, code elements, expressions, package names, file names, and paths as inline code.
Use descriptive link text. Avoid raw URLs and weak anchors such as "here" or "read more."
Use title case consistently for technical documentation headings.
Introduce code blocks, lists, tables, and images with complete sentences.
Write procedures as imperative steps. Keep steps parallel and split long procedures into smaller tasks.
Prefer active voice, present tense, short sentences, contractions, and plain English.
Usecanfor possibility and reservemayfor permission.
Useafterfor temporal relationships instead ofonce.
Preferrefer tooverseewhen the wording points readers to another resource.
Avoid culture-specific idioms, unnecessary Latinisms, jokes, and marketing exaggeration in technical docs.
Spell out months in body text, avoid ordinal dates, and use clear time zones.
Spell out whole numbers from zero through nine unless they are technical values, parameters, versions, or UI values.
Use numerals for 10 or greater and include commas in thousands.
Do not add trademark symbols to learning-oriented docs unless the source, platform, or legal guidance explicitly requires them.
Files:
README.mdintegrations/coding-agents/codex/README.md
**/*.md
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-technical-docs.md)
**/*.md: Use title case consistently in technical documentation headings
Avoid quotation marks, ampersands, and exclamation marks in headings
Keep product, event, research, and whitepaper names in their official title case
Use title case for table headers
Do not force social-media sentence case into technical docs
Format code elements, commands, parameters, package names, and expressions in monospace
Format directories, file names, and paths in monospace using backticks
Use angle brackets inside monospace for variables inside paths, such as/home/<username>/.login
Format error messages and strings in quotation marks, keeping literal code strings in code formatting when clearer
Format UI buttons, menus, fields, and labels in bold
Use angle brackets between UI labels for menu paths, such as File > Save As
Use italics for new terms on first use, sparingly and only when introducing the term
Use italics for publication titles
Format keyboard shortcuts in plain text, such as Press Ctrl+Alt+Delete
Use owner/repo link text for GitHub repositories, preferring[NVIDIA/NeMo](link)over prose references like 'the GitHub repo'
Introduce every code block with a complete sentence
Do not make a code block complete the grammar of the previous sentence
Do not continue a sentence after a code block
Use syntax highlighting when the format supports it for code blocks
Avoid the word 'snippet' unless the surrounding docs already use it as a term of art
Keep inline method, function, and class references consistent with nearby docs, omitting empty parentheses for prose readability when no call is shown
Use descriptive anchor text that matches the destination title when possible for links
Avoid raw URLs in running text
Avoid generic anchor text such as 'here,' 'this page,' and 'read more'
Include acronyms in link text when a linked term includes an acronym
Do not link long sentences or multiple sentences
Avoid links that pull readers away from a procedure unless the link is a p...
Files:
README.mdintegrations/coding-agents/codex/README.md
{README.md,docs/**/*.{md,rst,txt},fern/**/*}
📄 CodeRabbit inference engine (.agents/skills/prepare-code-freeze/SKILL.md)
Search and update documentation source for references to the old version in
README.md,docs, andferndirectories, updating current-version install commands, package examples, and configuration examples to<next-version>
Files:
README.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Update
README.md,fern/, package READMEs, and binding-support notes when public behavior, package names, examples, or supported bindings change.
**/*.{md,mdx}: Prefer the documented public API, not internal shortcuts
Keep package names, repo references, and build commands current
Keep release-process and release-notes guidance in repo-maintainer docs such asRELEASING.md, not as user-facing docs pages orCHANGELOG.md
Keep stable user-facing wrappers atscripts/root in docs and examples; only point at namespaced helper paths when documenting internal maintenance work
When detailed dynamic plugin guides exist, keep Rust native plugin examples, Python worker plugin examples, andgrpc-v1protocol details on separate pagesIf links in documentation change, run
just docs-linkcheck.
Files:
README.mddocs/nemo-relay-cli/plugin-installation.mdxintegrations/coding-agents/codex/README.md
**/*.{md,markdown,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Markdown/MDX documentation files using the HTML comment block form.
Files:
README.mddocs/nemo-relay-cli/plugin-installation.mdxintegrations/coding-agents/codex/README.md
{docs/**/*.md,README.md}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update reference docs, language-binding docs, READMEs, and example documentation when the public surface or expected usage changes.
Files:
README.md
{README.md,docs/index.md}
📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Update entry-point docs when examples or reading paths change
Files:
README.md
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
README.mdcrates/cli/tests/coverage/server_tests.rscrates/cli/src/server.rsdocs/nemo-relay-cli/plugin-installation.mdxintegrations/coding-agents/codex/README.mdcrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugin_shim/codex_app_server.rscrates/cli/src/plugin_shim/codex.rscrates/cli/tests/coverage/plugin_shim_tests.rs
README.md
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If documentation examples or commands in
README.mdchange, run the targeted docs checks appropriate to the change.
Files:
README.md
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
This file provides guidance to agents, including Claude Code and OpenAI Codex, when working in this repository.
Project Overview
NeMo Relay is a multi-language agent runtime framework for execution scopes, lifecycle events, middleware, plugins, and observability around tool and LLM calls. The core runtime is Rust. Primary supported bindings are Rust, Python, and Node.js. Go and the raw C FFI are experimental and source-first.
The shared runtime model is:
- Scope stacks decide where work belongs and which scope-local behavior is visible.
- Middleware registries decide what guardrails and intercepts run around managed calls.
- Plugins install reusable runtime behavior from configuration.
- Events record runtime behavior in ATOF form.
- Subscribers and exporters consume events in-process or export them to ATIF, OpenTelemetry, OpenInference, or other backends.
Repository Structure
The repository layout separates the Rust runtime, language bindings,
documentation, integrations, and agent-facing skills.crates/ core/ # Rust core runtime crate, published as nemo-relay adaptive/ # Adaptive runtime primitives and plugin components python/ # PyO3 native extension for the Python package ffi/ # Raw C ABI layer used by downstream bindings such as Go node/ # NAPI Node.js binding and JavaScript/TypeScript entry points python/ nemo_relay/ # Python wrapper package: scopes, tools, LLM, middleware, typed helpers, plugins, adaptive helpers tests/ # Python tests go/ nemo_relay/ # Experimental Go CGo binding and tests fern/ # Fern documentation site scripts/ # Stable wrappers and helper scripts; build/test/docs entry points live in justfile skills/ # Published Codex/agent skills for NeMo Relay usage patternsPrerequisites
Insta...
Files:
README.mdcrates/cli/tests/coverage/server_tests.rscrates/cli/src/server.rsdocs/nemo-relay-cli/plugin-installation.mdxintegrations/coding-agents/codex/README.mdcrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugin_shim/codex_app_server.rscrates/cli/src/plugin_shim/codex.rscrates/cli/tests/coverage/plugin_shim_tests.rs
{docs/**,README.md,CONTRIBUTING.md,RELEASING.md,SECURITY.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,RELEASING.md,SECURITY.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency across language bindings.
Flag stale examples, missing SPDX headers where required, and instructions that no longer match CI or pre-commit behavior.
Files:
README.mddocs/nemo-relay-cli/plugin-installation.mdx
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/cli/tests/coverage/server_tests.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugin_shim/codex_app_server.rscrates/cli/src/plugin_shim/codex.rscrates/cli/tests/coverage/plugin_shim_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/tests/coverage/server_tests.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugin_shim/codex_app_server.rscrates/cli/src/plugin_shim/codex.rscrates/cli/tests/coverage/plugin_shim_tests.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/cli/tests/coverage/server_tests.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugin_shim/codex_app_server.rscrates/cli/src/plugin_shim/codex.rscrates/cli/tests/coverage/plugin_shim_tests.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/cli/tests/coverage/server_tests.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugin_shim/codex_app_server.rscrates/cli/src/plugin_shim/codex.rscrates/cli/tests/coverage/plugin_shim_tests.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/cli/tests/coverage/server_tests.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugin_shim/codex_app_server.rscrates/cli/src/plugin_shim/codex.rscrates/cli/tests/coverage/plugin_shim_tests.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/cli/tests/coverage/server_tests.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugin_shim/codex_app_server.rscrates/cli/src/plugin_shim/codex.rscrates/cli/tests/coverage/plugin_shim_tests.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/server_tests.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/tests/coverage/plugin_shim_tests.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/cli/src/server.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugin_shim/codex_app_server.rscrates/cli/src/plugin_shim/codex.rs
**/*.mdx
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
MDX top-of-file SPDX comments must use {/* ... */} delimiters instead of HTML comment delimiters (Must-Fix)
In MDX files, top-of-file comments must use JSX comment delimiters (
{/*to open and*/}to close); do not use HTML comments for MDX SPDX headers
Files:
docs/nemo-relay-cli/plugin-installation.mdx
{docs,examples}/**/*
📄 CodeRabbit inference engine (.agents/skills/rename-surfaces/SKILL.md)
Update docs and examples.
Files:
docs/nemo-relay-cli/plugin-installation.mdx
docs/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If documentation examples or commands under
docs/change, run the targeted docs checks appropriate to the change.
Files:
docs/nemo-relay-cli/plugin-installation.mdx
🔇 Additional comments (18)
crates/cli/src/server.rs (2)
300-314: LGTM!
316-335: 🩺 Stability & Availability
idle_shutdown_readyis reachable fromserver_tests.rs#[cfg(test)] #[path = "../tests/coverage/server_tests.rs"] mod tests;makes that file a child module, so the private helper is visible there.> Likely an incorrect or invalid review comment.crates/cli/tests/coverage/server_tests.rs (1)
540-568: LGTM!docs/nemo-relay-cli/plugin-installation.mdx (1)
81-99: LGTM!Also applies to: 127-132, 165-167
integrations/coding-agents/codex/README.md (1)
46-63: LGTM!Also applies to: 192-194, 292-304
crates/cli/src/plugin_shim/codex.rs (4)
117-122: Same let-chain / MSRV concern flagged incodex_app_server.rsLines 158-160 applies here (&& let Some(client) = ...); resolve once by confirming the pinned toolchain is>= 1.88.
236-240: 🎯 Functional Correctness | ⚡ Quick winLexical path equality may reject legitimately-matching hooks.
Path::new(&hook.source_path) == hooks_pathcompares components lexically. If the app-server reports a canonicalized/symlink-resolved path (common on macOS, e.g./var→/private/var) whilehome_dir().join(".codex/hooks.json")is not resolved, this filter yields no hooks, andauto_trust_codex_hooksthen fails with "missing required hooks". Consider comparing canonicalized paths (or file identity) when both exist.
22-59: LGTM!Also applies to: 61-116, 123-135, 144-234, 245-302
42-43: 🗄️ Data Integrity & IntegrationNo rollback gap here
prepare_codex_configonly parses and validatesconfig.toml; it doesn’t mutate files, so taking snapshots afterward doesn’t miss any install-time changes.> Likely an incorrect or invalid review comment.crates/cli/src/plugin_shim/codex_app_server.rs (3)
45-94: LGTM!Also applies to: 96-122, 124-139, 141-147, 167-182, 184-197, 199-209, 211-225
158-160: 🎯 Functional CorrectnessLet chains are supported by the pinned toolchain
rust-toolchain.tomlpins Rust1.93.0, soif let Some(errors) = ... && !errors.is_empty()is valid. The MSRV concern does not apply here. The same applies tocrates/cli/src/plugin_shim/codex.rs.> Likely an incorrect or invalid review comment.
18-30: 🗄️ Data Integrity & IntegrationNo change needed The
CodexHookMetadatafield names are already mapped to the app-server’s camelCase schema by#[serde(rename_all = "camelCase")]; the snake_case Rust fields are correct here.> Likely an incorrect or invalid review comment.crates/cli/src/plugin_shim/mod.rs (2)
101-110: LGTM!
202-223: LGTM!Also applies to: 233-245
crates/cli/src/plugin_shim/shared.rs (1)
382-476: LGTM!crates/cli/tests/coverage/plugin_shim_tests.rs (2)
17-140: LGTM!
311-403: LGTM!Also applies to: 468-645, 985-1058
crates/cli/tests/coverage/plugin_install_tests.rs (1)
385-408: LGTM!
willkill07
changed the title
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cli/src/plugin_install/host.rs`:
- Around line 293-296: The relay executable path handling in resolve_executable
should keep canonicalization for matching, but avoid serializing a Windows \\?\
path into the MCP config command. Update the host.rs flow around RELAY_COMMAND
so the value written to .mcp.json is normalized to a Windows-friendly path while
preserving the canonicalized form only for comparison/lookup. Use the existing
executable resolution and config-writing logic in this area to ensure the
serialized MCP command remains executable on Windows.
In `@crates/cli/src/plugin_install/mod.rs`:
- Around line 594-605: The Generated MCP server check builds the MCP config path
inline with a literal ".mcp.json", which is inconsistent with the other manifest
path helpers and may drift from PluginLayout::new. Add a dedicated
mcp_config_path helper near the existing path helpers and use it in the
readiness.plugin branch instead of calling plugin.join directly, keeping the
path construction centralized alongside marketplace_manifest_path and
plugin_manifest_path.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 088df63e-c8eb-465a-9b69-608258e79bb2
📒 Files selected for processing (21)
crates/cli/Cargo.tomlcrates/cli/src/config.rscrates/cli/src/main.rscrates/cli/src/mcp.rscrates/cli/src/plugin_install/host.rscrates/cli/src/plugin_install/marketplace.rscrates/cli/src/plugin_install/mod.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/plugin_install/state.rscrates/cli/src/plugin_shim/codex.rscrates/cli/src/server.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/main_tests.rscrates/cli/tests/coverage/mcp_tests.rscrates/cli/tests/coverage/plugin_install_tests.rsintegrations/coding-agents/README.mdintegrations/coding-agents/codex/.codex-plugin/plugin.jsonintegrations/coding-agents/codex/.mcp.jsonintegrations/coding-agents/codex/README.mdintegrations/coding-agents/codex/hooks/hooks.json
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (19)
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
integrations/coding-agents/codex/hooks/hooks.jsoncrates/cli/Cargo.tomlcrates/cli/tests/coverage/main_tests.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/main.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/mcp_tests.rscrates/cli/src/config.rscrates/cli/src/plugin_install/host.rscrates/cli/src/plugin_install/state.rscrates/cli/tests/coverage/installer_tests.rsintegrations/coding-agents/README.mdcrates/cli/src/plugin_install/marketplace.rsintegrations/coding-agents/codex/README.mdcrates/cli/src/plugin_install/mod.rscrates/cli/src/mcp.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/codex.rs
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
This file provides guidance to agents, including Claude Code and OpenAI Codex, when working in this repository.
Project Overview
NeMo Relay is a multi-language agent runtime framework for execution scopes, lifecycle events, middleware, plugins, and observability around tool and LLM calls. The core runtime is Rust. Primary supported bindings are Rust, Python, and Node.js. Go and the raw C FFI are experimental and source-first.
The shared runtime model is:
- Scope stacks decide where work belongs and which scope-local behavior is visible.
- Middleware registries decide what guardrails and intercepts run around managed calls.
- Plugins install reusable runtime behavior from configuration.
- Events record runtime behavior in ATOF form.
- Subscribers and exporters consume events in-process or export them to ATIF, OpenTelemetry, OpenInference, or other backends.
Repository Structure
The repository layout separates the Rust runtime, language bindings,
documentation, integrations, and agent-facing skills.crates/ core/ # Rust core runtime crate, published as nemo-relay adaptive/ # Adaptive runtime primitives and plugin components python/ # PyO3 native extension for the Python package ffi/ # Raw C ABI layer used by downstream bindings such as Go node/ # NAPI Node.js binding and JavaScript/TypeScript entry points python/ nemo_relay/ # Python wrapper package: scopes, tools, LLM, middleware, typed helpers, plugins, adaptive helpers tests/ # Python tests go/ nemo_relay/ # Experimental Go CGo binding and tests fern/ # Fern documentation site scripts/ # Stable wrappers and helper scripts; build/test/docs entry points live in justfile skills/ # Published Codex/agent skills for NeMo Relay usage patternsPrerequisites
Insta...
Files:
integrations/coding-agents/codex/hooks/hooks.jsoncrates/cli/Cargo.tomlcrates/cli/tests/coverage/main_tests.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/main.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/mcp_tests.rscrates/cli/src/config.rscrates/cli/src/plugin_install/host.rscrates/cli/src/plugin_install/state.rscrates/cli/tests/coverage/installer_tests.rsintegrations/coding-agents/README.mdcrates/cli/src/plugin_install/marketplace.rsintegrations/coding-agents/codex/README.mdcrates/cli/src/plugin_install/mod.rscrates/cli/src/mcp.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/codex.rs
**/Cargo.toml
📄 CodeRabbit inference engine (.agents/skills/prepare-code-freeze/SKILL.md)
Confirm or infer the target release version from
upstream/main:Cargo.toml. Derive the release branch asrelease/<major>.<minor>.Keep Rust package names and workspace metadata in
Cargo.tomlinternally consistent across the project.
Files:
crates/cli/Cargo.toml
**/*.toml
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all TOML files using the
#comment form.
Files:
crates/cli/Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/cli/tests/coverage/main_tests.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/main.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/mcp_tests.rscrates/cli/src/config.rscrates/cli/src/plugin_install/host.rscrates/cli/src/plugin_install/state.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/src/plugin_install/marketplace.rscrates/cli/src/plugin_install/mod.rscrates/cli/src/mcp.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/codex.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/tests/coverage/main_tests.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/main.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/mcp_tests.rscrates/cli/src/config.rscrates/cli/src/plugin_install/host.rscrates/cli/src/plugin_install/state.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/src/plugin_install/marketplace.rscrates/cli/src/plugin_install/mod.rscrates/cli/src/mcp.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/codex.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/cli/tests/coverage/main_tests.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/main.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/mcp_tests.rscrates/cli/src/config.rscrates/cli/src/plugin_install/host.rscrates/cli/src/plugin_install/state.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/src/plugin_install/marketplace.rscrates/cli/src/plugin_install/mod.rscrates/cli/src/mcp.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/codex.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/cli/tests/coverage/main_tests.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/main.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/mcp_tests.rscrates/cli/src/config.rscrates/cli/src/plugin_install/host.rscrates/cli/src/plugin_install/state.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/src/plugin_install/marketplace.rscrates/cli/src/plugin_install/mod.rscrates/cli/src/mcp.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/codex.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/cli/tests/coverage/main_tests.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/main.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/mcp_tests.rscrates/cli/src/config.rscrates/cli/src/plugin_install/host.rscrates/cli/src/plugin_install/state.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/src/plugin_install/marketplace.rscrates/cli/src/plugin_install/mod.rscrates/cli/src/mcp.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/codex.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/cli/tests/coverage/main_tests.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/main.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/mcp_tests.rscrates/cli/src/config.rscrates/cli/src/plugin_install/host.rscrates/cli/src/plugin_install/state.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/src/plugin_install/marketplace.rscrates/cli/src/plugin_install/mod.rscrates/cli/src/mcp.rscrates/cli/src/server.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/src/plugin_shim/codex.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/main_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/mcp_tests.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/plugin_install_tests.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/cli/src/plugin_install/setup.rscrates/cli/src/main.rscrates/cli/src/config.rscrates/cli/src/plugin_install/host.rscrates/cli/src/plugin_install/state.rscrates/cli/src/plugin_install/marketplace.rscrates/cli/src/plugin_install/mod.rscrates/cli/src/mcp.rscrates/cli/src/server.rscrates/cli/src/plugin_shim/codex.rs
**/*.{md,rst,html,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
**/*.{md,rst,html,txt}: Always spellNVIDIAin all caps. Do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun because the name starts with an 'en' sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company.
Use trademark symbols with product names only when the document type or legal guidance requires them.
Verify official capitalization, spacing, and hyphenation for product names.
Precede NVIDIA product names withNVIDIAon first mention when it is natural and accurate.
Do not rewrite product names for grammar or title-case rules.
Preserve third-party product names according to the owner's spelling.
Include the company name and full model qualifier on first use when it helps identify the model.
Preserve the official capitalization and punctuation of model names.
Use shorter family names only after the full name is established.
Spell out a term on first use and put the acronym in parentheses unless the acronym is widely understood by the intended audience.
Use the acronym on later mentions after it has been defined.
For long documents, reintroduce the full term if readers might lose context.
Form plurals of acronyms withs, not an apostrophe, such asGPUs.
In headings, common acronyms can remain abbreviated. Spell out the term in the first or second sentence of the body.
Common terms such asCPU,GPU,PC,API, andUIusually do not need to be spelled out for developer audiences.
Files:
integrations/coding-agents/README.mdintegrations/coding-agents/codex/README.md
**/*.{md,rst,html}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Link the first mention of a product name when the destination helps the reader.
Files:
integrations/coding-agents/README.mdintegrations/coding-agents/codex/README.md
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Spell
NVIDIAin all caps. Do not useNvidia,nvidia, orNV.
Files:
integrations/coding-agents/README.mdintegrations/coding-agents/codex/README.md
**/*.{md,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
**/*.{md,rst}: Format commands, code elements, expressions, package names, file names, and paths as inline code.
Use descriptive link text. Avoid raw URLs and weak anchors such as "here" or "read more."
Use title case consistently for technical documentation headings.
Introduce code blocks, lists, tables, and images with complete sentences.
Write procedures as imperative steps. Keep steps parallel and split long procedures into smaller tasks.
Prefer active voice, present tense, short sentences, contractions, and plain English.
Usecanfor possibility and reservemayfor permission.
Useafterfor temporal relationships instead ofonce.
Preferrefer tooverseewhen the wording points readers to another resource.
Avoid culture-specific idioms, unnecessary Latinisms, jokes, and marketing exaggeration in technical docs.
Spell out months in body text, avoid ordinal dates, and use clear time zones.
Spell out whole numbers from zero through nine unless they are technical values, parameters, versions, or UI values.
Use numerals for 10 or greater and include commas in thousands.
Do not add trademark symbols to learning-oriented docs unless the source, platform, or legal guidance explicitly requires them.
Files:
integrations/coding-agents/README.mdintegrations/coding-agents/codex/README.md
**/*.md
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-technical-docs.md)
**/*.md: Use title case consistently in technical documentation headings
Avoid quotation marks, ampersands, and exclamation marks in headings
Keep product, event, research, and whitepaper names in their official title case
Use title case for table headers
Do not force social-media sentence case into technical docs
Format code elements, commands, parameters, package names, and expressions in monospace
Format directories, file names, and paths in monospace using backticks
Use angle brackets inside monospace for variables inside paths, such as/home/<username>/.login
Format error messages and strings in quotation marks, keeping literal code strings in code formatting when clearer
Format UI buttons, menus, fields, and labels in bold
Use angle brackets between UI labels for menu paths, such as File > Save As
Use italics for new terms on first use, sparingly and only when introducing the term
Use italics for publication titles
Format keyboard shortcuts in plain text, such as Press Ctrl+Alt+Delete
Use owner/repo link text for GitHub repositories, preferring[NVIDIA/NeMo](link)over prose references like 'the GitHub repo'
Introduce every code block with a complete sentence
Do not make a code block complete the grammar of the previous sentence
Do not continue a sentence after a code block
Use syntax highlighting when the format supports it for code blocks
Avoid the word 'snippet' unless the surrounding docs already use it as a term of art
Keep inline method, function, and class references consistent with nearby docs, omitting empty parentheses for prose readability when no call is shown
Use descriptive anchor text that matches the destination title when possible for links
Avoid raw URLs in running text
Avoid generic anchor text such as 'here,' 'this page,' and 'read more'
Include acronyms in link text when a linked term includes an acronym
Do not link long sentences or multiple sentences
Avoid links that pull readers away from a procedure unless the link is a p...
Files:
integrations/coding-agents/README.mdintegrations/coding-agents/codex/README.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Update
README.md,fern/, package READMEs, and binding-support notes when public behavior, package names, examples, or supported bindings change.
**/*.{md,mdx}: Prefer the documented public API, not internal shortcuts
Keep package names, repo references, and build commands current
Keep release-process and release-notes guidance in repo-maintainer docs such asRELEASING.md, not as user-facing docs pages orCHANGELOG.md
Keep stable user-facing wrappers atscripts/root in docs and examples; only point at namespaced helper paths when documenting internal maintenance work
When detailed dynamic plugin guides exist, keep Rust native plugin examples, Python worker plugin examples, andgrpc-v1protocol details on separate pagesIf links in documentation change, run
just docs-linkcheck.
Files:
integrations/coding-agents/README.mdintegrations/coding-agents/codex/README.md
**/*.{md,markdown,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Markdown/MDX documentation files using the HTML comment block form.
Files:
integrations/coding-agents/README.mdintegrations/coding-agents/codex/README.md
🔇 Additional comments (26)
crates/cli/src/server.rs (2)
53-63: LGTM!Also applies to: 65-85, 87-91, 132-146, 178-188
148-164: 🩺 Stability & AvailabilityReadiness signal is correctly ordered. It fires after plugin/router setup and before
axum::serve, and the MCP launcher only waits on it before starting stdio handling.crates/cli/Cargo.toml (1)
53-53: LGTM!crates/cli/src/config.rs (1)
72-83: LGTM!crates/cli/src/main.rs (1)
17-17: LGTM!Also applies to: 84-84
crates/cli/src/mcp.rs (2)
1-104: LGTM!Also applies to: 131-187
105-129: 🗄️ Data Integrity & IntegrationMCP stdio uses newline-delimited JSON, so
read_lineis the right framing here.> Likely an incorrect or invalid review comment.integrations/coding-agents/codex/.mcp.json (1)
1-15: LGTM!integrations/coding-agents/codex/README.md (1)
22-23: LGTM!Also applies to: 48-68, 196-200, 227-227, 299-317, 338-342
integrations/coding-agents/codex/hooks/hooks.json (1)
2-151: LGTM!crates/cli/tests/cli_tests.rs (1)
164-217: LGTM!Also applies to: 1329-1329
crates/cli/tests/coverage/main_tests.rs (1)
86-96: LGTM!crates/cli/tests/coverage/mcp_tests.rs (1)
1-201: LGTM!Good coverage of readiness gating, EOF shutdown, and the plugin-activation error path — satisfies the path instruction to prefer lifecycle/error-path assertions over shallow smoke tests.
crates/cli/src/plugin_shim/codex.rs (3)
146-314: LGTM!Trust classification, canonicalized path matching, missing-required computation, and the snapshot/rollback aggregation helpers all look correct and consistent with the PR's trust-hardening goals.
117-122: 🩺 Stability & AvailabilityNo issue — the workspace targets Rust 1.93.0 and edition 2024, so this
letchain is supported.
36-53: 🗄️ Data Integrity & IntegrationSnapshot order is fine
prepare_codex_configonly parsesconfig.tomland returns an error on invalid TOML; it does not write to disk, so taking the snapshot afterward still captures the pre-install state.> Likely an incorrect or invalid review comment.integrations/coding-agents/README.md (1)
25-36: LGTM!Also applies to: 69-85
integrations/coding-agents/codex/.codex-plugin/plugin.json (1)
4-22: LGTM!crates/cli/src/plugin_install/state.rs (1)
50-50: LGTM!Also applies to: 74-83
crates/cli/src/plugin_install/host.rs (1)
319-338: LGTM! Mirrors the existingvalidate_relay_plugin_shimpattern and matches the mocked-failure test expectations.crates/cli/src/plugin_install/marketplace.rs (2)
143-160: 🎯 Functional CorrectnessConfirm the hardcoded MCP bind address doesn't drift from a single source of truth, and how port conflicts are handled.
NEMO_RELAY_GATEWAY_BINDis hardcoded here to a fixed port (127.0.0.1:47632), while docs describe the transparent-wrapper flow using a dynamic port. Since this config is marked"required": true, ifconfig.rs/mcp.rs(default bind handling, not in this file set) ever changes the default bind or if the port is already in use (e.g., two installs, a leftover process), Codex startup could fail hard. Worth confirming this literal is sourced from (or kept in sync with) the same constant used by themcpsubcommand's default bind, and whether a bind failure is retried/surfaced gracefully.
20-28: LGTM! Consistent with updated tests (installer_tests.rs, plugin_install_tests.rs) confirming the generated.mcp.jsoncontent, manifest description/interface text, and the new Codex hooks-template behavior.Also applies to: 49-51, 100-131, 176-176
crates/cli/src/plugin_install/mod.rs (1)
361-369: LGTM! Correctly gates Codex plugin generation behindvalidate_relay_mcp, matchingrelay_without_native_mcp_fails_codex_install_before_generating_plugin.crates/cli/src/plugin_install/setup.rs (1)
58-58: LGTM!crates/cli/tests/coverage/plugin_install_tests.rs (1)
287-293: LGTM! Good coverage of the new MCP validation gate, generated config content, and readiness failure modes (missing config, mismatched binary), consistent with the source changes.Also applies to: 346-362, 474-474, 529-534, 847-870, 1045-1069, 1444-1487
crates/cli/tests/coverage/installer_tests.rs (1)
171-179: LGTM!Also applies to: 228-245, 283-283
willkill07
force-pushed
the
wkk_relay-447-codex-bootstrap
branch
from
027d8ed to
f7779fc
Compare
willkill07
changed the title
github-actions
Bot
added
size:XXL
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (47)
crates/cli/src/agents/claude/host.rs (1)
164-170: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve an originally absent settings file.
Writing
{}as the backup loses the fact thatsettings.jsondid not exist.restore_claude_providerthen leaves a new empty file after uninstall. Store absence explicitly and remove the settings file during restore when Relay created it.🤖 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/cli/src/agents/claude/host.rs` around lines 164 - 170, Update the backup and restore flow around restore_claude_provider so an originally absent settings.json is represented explicitly rather than by writing an empty JSON object. Record the absence in the backup state, and during restore remove the settings file when that marker indicates Relay created it; preserve existing file contents and restoration behavior otherwise.crates/cli/src/agents/codex/host.rs (1)
727-728: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Read the client token from both valid TOML table forms.
codex_provider_headersupports inline and regular tables, butcodex_provider_client_tokenonly accepts inline tables. Reformattinghttp_headersas a regular table makes installation and ownership detection lose a valid token, potentially preserving generated Relay state in the uninstall backup.Proposed fix
pub(crate) fn codex_provider_client_token(doc: &DocumentMut) -> Option<&str> { - doc.get("model_providers") - .and_then(Item::as_table) - .and_then(|providers| providers.get("nemo-relay-openai")) - .and_then(Item::as_table) - .and_then(|provider| provider.get("http_headers")) - .and_then(Item::as_inline_table) - .and_then(|headers| headers.get(BOOTSTRAP_CLIENT_TOKEN_HEADER)) + codex_provider_header(doc, BOOTSTRAP_CLIENT_TOKEN_HEADER) .and_then(TomlValue::as_str) }Also applies to: 1528-1554
🤖 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/cli/src/agents/codex/host.rs` around lines 727 - 728, Update codex_provider_client_token and its callers, including the installation/ownership detection path around has_managed_proof, to read client tokens from both inline and regular TOML table forms, matching codex_provider_header behavior. Preserve existing token validation while ensuring regular-table http_headers still detects the valid token and handles Relay-state backup correctly.crates/cli/src/agents/hermes/config.rs (1)
192-220: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use durable ownership evidence for partial Hermes state.
Ownership currently depends on complete configuration or a previously known exact command, so damaged or orphaned Relay state can survive repair and uninstall.
crates/cli/src/agents/hermes/config.rs#L192-L220: Recognize modern ownership from the Relay MCP command, arguments, and generation fence; validate hook completeness separately.crates/cli/src/agents/hermes/integration.rs#L146-L177: Add constrained recognition for orphaned Relay approvals or persist an explicit ownership marker.📍 Affects 2 files
crates/cli/src/agents/hermes/config.rs#L192-L220(this comment)crates/cli/src/agents/hermes/integration.rs#L146-L177🤖 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/cli/src/agents/hermes/config.rs` around lines 192 - 220, Update crates/cli/src/agents/hermes/config.rs lines 192-220 so modern ownership is recognized from the Relay MCP command, expected arguments, and generation fence without requiring a complete hook set; keep hook completeness as a separate validation result. In crates/cli/src/agents/hermes/integration.rs lines 146-177, add constrained recognition for orphaned Relay approvals or persist an explicit ownership marker so damaged or orphaned state can be repaired or uninstalled.crates/cli/src/agents/hermes/integration.rs (1)
232-245: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate forwarded environment values, not only names.
Line 239 accepts any value for a required environment key. Doctor therefore reports success when
${NAME}has been replaced by a stale or fixed value, even though MCP forwarding is broken. Compare each entry with the exact placeholder generated byexpected_mcp_server.Proposed Fix
- let missing = forwarded_environment_names(&environment, plugin_config.as_ref()) + let invalid = forwarded_environment_names(&environment, plugin_config.as_ref()) .into_iter() - .filter(|name| !mcp_env.contains_key(name)) + .filter(|name| { + let expected = format!("${{{name}}}"); + mcp_env.get(name.as_str()).and_then(Value::as_str) != Some(expected.as_str()) + }) .collect::<Vec<_>>(); - if !missing.is_empty() { + if !invalid.is_empty() { return Err(format!( - "Hermes Relay MCP is missing environment names {}; run `nemo-relay install hermes --force`", - missing.join(", ") + "Hermes Relay MCP has missing or invalid environment entries {}; run `nemo-relay install hermes --force`", + invalid.join(", ") ));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let plugin_config = crate::configuration::user_plugin_runtime_config().map_err(|e| e.to_string())?; let environment = env::vars_os() .filter_map(|(name, _)| name.into_string().ok()) .collect::<Vec<_>>(); let invalid = forwarded_environment_names(&environment, plugin_config.as_ref()) .into_iter() .filter(|name| { let expected = format!("${{{name}}}"); mcp_env.get(name.as_str()).and_then(Value::as_str) != Some(expected.as_str()) }) .collect::<Vec<_>>(); if !invalid.is_empty() { return Err(format!( "Hermes Relay MCP has missing or invalid environment entries {}; run `nemo-relay install hermes --force`", invalid.join(", ") ));🤖 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/cli/src/agents/hermes/integration.rs` around lines 232 - 245, Update the validation around forwarded_environment_names to verify values as well as keys: for each required environment name, compare mcp_env’s entry with the exact placeholder generated by expected_mcp_server, and include entries that are missing or mismatched in the existing error path. Preserve the current success behavior only when every required key has the expected placeholder value.crates/cli/src/bootstrap/state.rs (1)
55-60: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Accept authenticated owners from compatible CLI versions.
The exact package-version check makes gateway refresh fail during upgrades before the authenticated probe and shutdown run. Use
bootstrap_protocolfor compatibility; retainversionas informational metadata.Proposed fix
fn valid_for(&self, url: &str) -> bool { self.service == "nemo-relay" - && self.version == env!("CARGO_PKG_VERSION") && self.bootstrap_protocol == BOOTSTRAP_PROTOCOL_VERSION && self.url == url && !self.shutdown_token.is_empty() }The marketplace
refresh_gatewaypath callsstop_owned_and_reset, making this reachable during installation and upgrades.Also applies to: 214-229
🤖 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/cli/src/bootstrap/state.rs` around lines 55 - 60, Update State::valid_for to remove the exact CARGO_PKG_VERSION comparison and rely on bootstrap_protocol compatibility, while retaining the version field as informational metadata. Preserve the service, URL, and non-empty shutdown_token checks so authenticated owners from compatible CLI versions are accepted during refresh.crates/cli/src/commands/configure/mod.rs (1)
20-29: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Support resetting global configuration.
The wizard can save global or both scopes, but
model::resetonly examines./.nemo-relay/config.toml. A globally configured user therefore gets “No project config” and remains configured. Add an explicit scope option or clear the applicable project and global files, with tests for each scope.🤖 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/cli/src/commands/configure/mod.rs` around lines 20 - 29, Update the reset flow in execute and model::reset to support the global scope as well as the project scope, using an explicit scope option or clearing both applicable configuration files when requested. Preserve agent filtering and existing project-reset behavior, and add tests covering project, global, and both-scope resets.crates/cli/src/commands/install.rs (1)
99-101: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate Agent-Specific Exit Codes
Both loops discard a successful non-zero
ExitCode, causingnemo-relay install alloruninstall allto report success after an agent-specific operation reports failure. Preserve or aggregate non-success statuses.
crates/cli/src/commands/install.rs#L99-L101: capture and return or aggregate the result frominstall_integration.crates/cli/src/commands/install.rs#L119-L121: capture and return or aggregate the result fromuninstall_integration.📍 Affects 1 file
crates/cli/src/commands/install.rs#L99-L101(this comment)crates/cli/src/commands/install.rs#L119-L121🤖 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/cli/src/commands/install.rs` around lines 99 - 101, Preserve non-success exit statuses in both agent loops: update install_integration at crates/cli/src/commands/install.rs:99-101 and uninstall_integration at crates/cli/src/commands/install.rs:119-121 to capture and return or aggregate each ExitCode instead of discarding it, so install all and uninstall all report agent-specific failures.crates/cli/src/commands/run.rs (1)
76-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not run default setup for a missing explicit config path.
configure::runcannot receive or createexplicit_config, yet the launcher still receives that missing path afterward. A typo in--configtherefore launches an unrelated setup flow and then fails anyway.Proposed fix
- let needs_setup = explicit_config.map_or_else( - || !crate::configuration::any_config_file_exists(), - |path| !path.exists(), - ); + let needs_setup = + explicit_config.is_none() && !crate::configuration::any_config_file_exists();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let explicit_config = inherited.config.as_deref(); let needs_setup = explicit_config.is_none() && !crate::configuration::any_config_file_exists(); if needs_setup { super::configure::run(Some(agent)).await?; } let runtime = crate::process::RunOverrides { agent: Some(agent), config: explicit_config.map(PathBuf::from),🤖 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/cli/src/commands/run.rs` around lines 76 - 86, Update the setup decision in the run command to invoke configure::run only when no explicit config path was supplied and no default config exists; when explicit_config points to a missing path, skip default setup and preserve that path for the subsequent RunOverrides construction.crates/cli/src/configuration/mod.rs (1)
151-159: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require the bootstrap fingerprint for every managed child.
A supplied
ready_filemarks this as managed bootstrap, but a missing fingerprint silently returnsNoneand bypasses identity verification. Fail closed instead.Proposed fix
- let Some(expected) = env::var(BOOTSTRAP_FINGERPRINT_ENV) + let expected = env::var(BOOTSTRAP_FINGERPRINT_ENV) .ok() .filter(|fingerprint| !fingerprint.is_empty()) - else { - return Ok(None); - }; + .ok_or_else(|| { + CliError::Config( + "managed bootstrap requires a non-empty bootstrap fingerprint".into(), + ) + })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if args.ready_file.is_none() { return Ok(None); } let expected = env::var(BOOTSTRAP_FINGERPRINT_ENV) .ok() .filter(|fingerprint| !fingerprint.is_empty()) .ok_or_else(|| { CliError::Config( "managed bootstrap requires a non-empty bootstrap fingerprint".into(), ) })?;🤖 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/cli/src/configuration/mod.rs` around lines 151 - 159, Update the managed-bootstrap handling around args.ready_file and BOOTSTRAP_FINGERPRINT_ENV so a supplied ready_file requires a non-empty bootstrap fingerprint; return an error instead of Ok(None) when the fingerprint is missing or empty, while preserving Ok(None) for unmanaged children without a ready_file.crates/cli/src/diagnostics/environment.rs (1)
8-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle Windows diagnostics without Unix-only assumptions.
Line 10 renders
windowsbecauseos_versionis always empty there, while Lines 12-16 only inspectSHELL. Omit the separator when no version is available and useCOMSPECon Windows so doctor reports remain accurate.🤖 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/cli/src/diagnostics/environment.rs` around lines 8 - 27, Update collect_environment and os_version so the OS string omits the separator when no version is available instead of producing “windows ”. Select the shell variable by platform, using COMSPEC on Windows and SHELL elsewhere, while preserving basename extraction and the existing Unix version lookup.crates/cli/src/diagnostics/probes.rs (1)
29-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -nP -C5 '\bcheck_dir_writable\s*\(|\bcheck_directory\s*\(' \ crates/cli/src crates/cli/testsRepository: NVIDIA/NeMo-Relay
Length of output: 5133
🏁 Script executed:
sed -n '1,120p' crates/cli/src/diagnostics/probes.rs printf '\n--- tests ---\n' sed -n '400,430p' crates/cli/tests/coverage/shared/doctor_tests.rs printf '\n--- more tests ---\n' sed -n '960,980p' crates/cli/tests/coverage/shared/doctor_tests.rsRepository: NVIDIA/NeMo-Relay
Length of output: 4290
Probe actual directory writability.
permissions().readonly()only inspects metadata, so it can pass even when this process cannot create entries there (ACLs, ownership, mount options, platform attributes). Use a temporarycreate_new(true)file and clean it up afterward.🤖 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/cli/src/diagnostics/probes.rs` around lines 29 - 43, Update check_dir_writable to verify actual write access by creating a uniquely named temporary file in the validated directory with create_new(true), then remove that file before returning success. Preserve the existing directory validation and propagate creation or cleanup I/O errors appropriately; do not rely on metadata.permissions().readonly().crates/cli/src/diagnostics/render.rs (1)
214-218: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render each check's status marker.
These checks affect the exit code and conclusion, but the human output discards
check.status, leaving users unable to identify failures or warnings.Proposed fix
- out.push_str(&format!(" {:<22} {}\n", check.name, check.details)); + out.push_str(&format!( + " {} {:<22} {}\n", + format_status(check.status), + check.name, + check.details + )); ... - out.push_str(&format!(" {}\n", check.details)); + out.push_str(&format!( + " {} {}\n", + format_status(check.status), + check.details + ));Also applies to: 222-226
🤖 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/cli/src/diagnostics/render.rs` around lines 214 - 218, Update format_human_checks to include each Check’s status marker in the rendered line, using the existing status representation and preserving the current name/details alignment. Ensure both human-check output paths covered by this function expose whether each check passed, warned, or failed.crates/cli/src/filesystem/bounded.rs (1)
22-89: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the gateway-specific suffix from this generic reader.
Every error says
for persistent gateway identity, including plugin manifest, artifact, signature, and runtime-closure reads. Build the message solely fromdescriptionso trust failures report the actual operation.🤖 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/cli/src/filesystem/bounded.rs` around lines 22 - 89, Update the generic bounded-file reader’s error messages to remove the hardcoded “for persistent gateway identity” suffix and construct them solely from the provided description. Apply this consistently to the metadata, open, and read error paths while preserving the existing path and error details.crates/cli/src/filesystem/temp.rs (1)
8-25: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files crates/cli/src/filesystem printf '\n--- outline temp.rs ---\n' ast-grep outline crates/cli/src/filesystem/temp.rs --view expanded printf '\n--- search protect_private_windows_path ---\n' rg -n "protect_private_windows_path|private_system_temp_dir|private_temp_dir|DirBuilderExt|mode\\(" crates/cli/src -S printf '\n--- maybe related filesystem helpers ---\n' rg -n "windows.*private|private.*windows|DACL|ACL" crates/cli/src -SRepository: NVIDIA/NeMo-Relay
Length of output: 6503
🏁 Script executed:
sed -n '1,140p' crates/cli/src/agents/hermes/launch.rs printf '\n--- atomic.rs relevant section ---\n' sed -n '173,250p' crates/cli/src/filesystem/atomic.rs printf '\n--- configuration usage ---\n' sed -n '660,725p' crates/cli/src/configuration/mod.rs printf '\n--- temp.rs ---\n' sed -n '1,120p' crates/cli/src/filesystem/temp.rsRepository: NVIDIA/NeMo-Relay
Length of output: 10937
Harden Windows temp directories before returning them.
private_temp_dir()only sets0o700on Unix; on Windows it returns a newly created directory with the inherited ACL. This can expose Hermes overlay contents if the parent ACL is broad. Applyprotect_private_windows_path()aftercreate(), and remove the directory if hardening fails.🤖 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/cli/src/filesystem/temp.rs` around lines 8 - 25, Update private_temp_dir to call protect_private_windows_path after builder.create(&path) succeeds, applying it only on Windows as appropriate. If hardening fails, remove the newly created directory before returning the error; otherwise preserve the existing successful path return behavior.crates/cli/src/gateway/client.rs (2)
77-90: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate resolved addresses instead of trusting the
localhosthostname.
parse_loopback_urlacceptslocalhost, but both callers use the first resolver result without verifyingaddress.ip().is_loopback(). A modified resolver entry can send the fingerprint—and the shutdown token—to a remote service. It also causes avoidable dual-stack failures.Filter all results to loopback addresses and attempt each until one connects.
Also applies to: 291-309, 472-497
🤖 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/cli/src/gateway/client.rs` around lines 77 - 90, Update the address resolution and connection logic in parse_loopback_url and the corresponding callers around the shutdown-token and fingerprint connections: filter resolved addresses with address.ip().is_loopback(), then attempt TcpStream::connect_timeout for each remaining address until one succeeds. Do not use only the first resolver result; return the existing connection error when no loopback address connects, while preserving the current rejection behavior when resolution yields no valid loopback addresses.
120-180: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
The bootstrap proof does not authenticate this TCP peer.
The proof covers only the fingerprint and nonce. A foreign listener can forward that challenge to another genuine Relay using the same per-user identity, replay its proof, and then receive the plaintext POST. Reusing the connection does not prevent challenge relaying.
Establish an authenticated encrypted channel, such as TLS with a pinned per-user identity, before transmitting hook payloads.
🤖 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/cli/src/gateway/client.rs` around lines 120 - 180, Update the gateway connection flow surrounding the bootstrap challenge and verified POST so it establishes an authenticated encrypted channel before sending any hook payload. Use TLS with the appropriate pinned per-user Relay identity, perform certificate/peer validation before transmitting the request, and preserve the existing bootstrap health verification as an additional check rather than relying on it to authenticate the TCP peer.crates/cli/src/gateway/response.rs (1)
56-67: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Strip every header nominated by
Connection.The current fixed-name filter removes
Connectionitself but forwards extension headers it names, allowing hop-specific or sensitive metadata to cross the proxy boundary.
crates/cli/src/gateway/response.rs#L56-L67: derive request exclusions from the complete header map and itsConnectiontokens.crates/cli/src/gateway/response.rs#L29-L35: apply the same map-aware filtering to upstream responses.crates/cli/src/gateway/mod.rs#L649-L659: use the map-aware request filter for general forwarding.crates/cli/src/gateway/mod.rs#L857-L867: use it for/modelsforwarding as well.📍 Affects 2 files
crates/cli/src/gateway/response.rs#L56-L67(this comment)crates/cli/src/gateway/response.rs#L29-L35crates/cli/src/gateway/mod.rs#L649-L659crates/cli/src/gateway/mod.rs#L857-L867🤖 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/cli/src/gateway/response.rs` around lines 56 - 67, Make header filtering map-aware so every header named by the Connection header’s tokens is excluded, in addition to the existing hop-by-hop and sensitive-header rules. Update should_forward_request_header and the response filtering logic in crates/cli/src/gateway/response.rs lines 56-67 and 29-35 to receive the complete header map, then update both forwarding call sites in crates/cli/src/gateway/mod.rs lines 649-659 and 857-867 to use the new request filter, preserving the existing exclusions including Accept-Encoding.crates/cli/src/hooks/delivery.rs (1)
102-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve fail-open behavior for transport setup failures.
Lines 102 and 114 use
?, so header construction, client construction, or task failures abort the hook even whenfail_closedis false. Route these outer errors throughhandle_hook_error.Proposed fix
- let response = - send_verified_hook_forward_request(&command, gateway, &destination.gateway_url, input) - .await?; + let response = match send_verified_hook_forward_request( + &command, + gateway, + &destination.gateway_url, + input, + ) + .await + { + Ok(response) => response, + Err(error) => return handle_hook_error(error, fail_closed), + }; return handle_verified_hook_forward_response(response, fail_closed); @@ - let response = send_hook_forward_request(&command, &url, input).await?; + let response = match send_hook_forward_request(&command, &url, input).await { + Ok(response) => response, + Err(error) => return handle_hook_error(error, fail_closed), + };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if let Some(gateway) = verified_gateway { let response = match send_verified_hook_forward_request( &command, gateway, &destination.gateway_url, input, ) .await { Ok(response) => response, Err(error) => return handle_hook_error(error, fail_closed), }; return handle_verified_hook_forward_response(response, fail_closed); } let url = format!( "{}{}", destination.gateway_url.trim_end_matches('/'), command.agent.hook_path() ); let response = match send_hook_forward_request(&command, &url, input).await { Ok(response) => response, Err(error) => return handle_hook_error(error, fail_closed), }; handle_hook_forward_response(response, fail_closed).await🤖 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/cli/src/hooks/delivery.rs` around lines 102 - 115, Update both forwarding branches in the hook delivery flow to handle transport setup or task errors through handle_hook_error instead of propagating them with ?. Preserve the existing fail_closed behavior by routing errors from send_verified_hook_forward_request and send_hook_forward_request through handle_hook_error, while leaving successful response handling unchanged.crates/cli/src/installation/marketplace/assets.rs (1)
100-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Propagate failure instead of emitting a relative lifecycle path.
If
current_dirfails, the generated MCP and hook configurations retain a relative generation-fence path. The host can later launch them from another directory, making a successful installation unusable. ReturnResult<PathBuf, String>and propagate the error.🤖 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/cli/src/installation/marketplace/assets.rs` around lines 100 - 107, Update absolute_or_self to return Result<PathBuf, String> instead of falling back to the relative path when env::current_dir fails. Propagate the current-directory error to callers generating MCP and hook configurations, and adjust those call sites to handle or return the failure.crates/cli/src/installation/marketplace/host.rs (1)
261-272: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve a stable Relay executable path for generated integrations.
current_exefollowed bycanonicalizepins hooks and MCP configuration to the physical binary. Versioned package-manager targets can disappear after an upgrade, breaking existing integrations. Prefer the PATH-resolvednemo-relaypath, withcurrent_executableas fallback; the subsequent capability checks still reject an incompatible binary.Proposed fix
if options.dry_run { return Ok(PathBuf::from(RELAY_COMMAND)); } + if let Some(path) = runner.resolve_executable(RELAY_COMMAND)? { + return Ok(crate::process::portable_executable_path(path)); + } runner .current_executable() .map(|path| path.canonicalize().unwrap_or(path)) .map(crate::process::portable_executable_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pub(crate) fn require_relay( options: &PluginInstallOptions, runner: &dyn CommandRunner, ) -> Result<PathBuf, String> { if options.dry_run { return Ok(PathBuf::from(RELAY_COMMAND)); } if let Some(path) = runner.resolve_executable(RELAY_COMMAND)? { return Ok(crate::process::portable_executable_path(path)); } runner .current_executable() .map(|path| path.canonicalize().unwrap_or(path)) .map(crate::process::portable_executable_path) }🤖 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/cli/src/installation/marketplace/host.rs` around lines 261 - 272, Update require_relay to prefer the PATH-resolved nemo-relay executable so generated integrations retain a stable command path, using current_executable only when PATH resolution fails. Preserve the existing dry-run behavior and portable_executable_path processing, while leaving subsequent capability validation unchanged.crates/cli/src/mcp/gateway.rs (1)
175-178: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Do Tokio 1.x docs state that an already-running spawn_blocking task cannot be aborted?💡 Result:
Yes, the official Tokio 1.x documentation explicitly states that tasks spawned using spawn_blocking cannot be aborted once they have started running [1][2][3]. Because spawn_blocking tasks are not asynchronous, they cannot be cancelled via the standard abort mechanism [1][4][3]. If you call abort on a spawn_blocking task that is already running, the call will have no effect, and the task will continue to execute normally [1][5][6]. The only exception is if the task has not yet started running; in that case, calling abort may prevent it from starting, though this is not guaranteed [1][7][6].
Citations:
- 1: https://docs.rs/tokio/1/tokio/task/fn.spawn_blocking.html
- 2: https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html
- 3: https://docs.rs/tokio/latest/tokio/task/fn.spawn%5Fblocking.html
- 4: https://docs.rs/tokio/latest/tokio/task/struct.AbortHandle.html
- 5: https://docs.rs/tokio/latest/tokio/task/struct.JoinHandle.html
- 6: https://docs.rs/tokio/latest/src/tokio/runtime/task/join.rs.html
- 7: https://docs.rs/tokio/1.48.0/tokio/task/fn.spawn_blocking.html
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant file and surrounding lines. FILE="crates/cli/src/mcp/gateway.rs" wc -l "$FILE" sed -n '1,260p' "$FILE"Repository: NVIDIA/NeMo-Relay
Length of output: 9041
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the maintenance loop and recovery call site. FILE="crates/cli/src/mcp/gateway.rs" sed -n '160,260p' "$FILE"Repository: NVIDIA/NeMo-Relay
Length of output: 3082
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the maintenance loop implementation and how the restart callback is awaited. FILE="crates/cli/src/mcp/gateway.rs" sed -n '260,393p' "$FILE"Repository: NVIDIA/NeMo-Relay
Length of output: 4550
Cancel blocking recovery on lease drop
crates/cli/src/mcp/gateway.rs:175-205—abort()only stops the outer monitor task; ifrecover_gateway()has already enteredspawn_blocking, that blocking work keeps running and can still recover/adopt the gateway after MCP stdio is gone. Add cooperative shutdown around recovery so teardown waits for in-flight maintenance to finish.🤖 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/cli/src/mcp/gateway.rs` around lines 175 - 178, Update GatewayLease cleanup around Drop::drop and the recovery flow so dropping the lease cooperatively signals shutdown, prevents further gateway adoption, and waits for any in-flight recover_gateway work—including spawn_blocking operations—to finish before teardown completes. Ensure the monitor and blocking recovery share the shutdown state and preserve normal recovery behavior while the lease is active.crates/cli/src/mcp/protocol.rs (1)
27-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What ID types and notification fields does MCP protocol revision 2025-06-18 require?💡 Result:
The Model Context Protocol (MCP) revision 2025-06-18 defines specific requirements for notification structures and data types [1][2]. Notification Fields In the MCP protocol, notifications are one-way messages and, according to the JSON-RPC standard used by the protocol, MUST NOT include an ID field [3][4]. All notifications share a common structure that allows for optional metadata via the
_metafield [5][6]. This_metaobject is reserved by MCP for additional metadata [5][6]. While not explicitly mandated for every notification type, the protocol supports the use of_metafor cross-cutting concerns [5]. For notifications delivered via specific streams (such assubscriptions/listen), the server is required to includeio.modelcontextprotocol/subscriptionIdwithin the_metafield, allowing the client to correlate the notification with the original request [3][7][4]. ID Types The protocol utilizes JSON-RPC 2.0 for messaging [8][9]. Within this framework, a request ID—when used for standard requests (not notifications)—is defined as either astringor anumber[10][7][11]. While notifications specifically must not include an ID, any request associated with a process that later triggers a notification (such as a subscription request) uses these string or number types for its identifier [9][7]. Summary of Key Requirements - Notifications: MUST NOT include an ID field [3][4]. - Notification Structure: May include an optional_metafield for metadata [5][6]. - Subscription Notifications: MUST includeio.modelcontextprotocol/subscriptionIdin_metaif delivered via a subscription stream [3][4]. - ID Data Type: Where IDs are required (e.g., in standard requests), the type is astringornumber[10][7][11].Citations:
- 1: https://modelcontextprotocol.io/specification/2025-06-18/changelog
- 2: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/ff960c9e/docs/specification/2025-06-18/index.mdx
- 3: https://modelcontextprotocol.io/specification/draft/basic
- 4: https://mcp.mintlify.app/specification/draft/basic
- 5: https://github.com/modelcontextprotocol/specification/blob/main/schema/2025-06-18/schema.ts
- 6: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-06-18/schema.ts
- 7: https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/main/schema/draft/schema.ts
- 8: https://mcp-staging.mintlify.app/specification/2025-06-18/basic/lifecycle
- 9: https://modelcontextprotocol.io/specification/2025-06-18/basic/utilities/cancellation
- 10: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/draft/schema.ts
- 11: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-03-26/schema.ts
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '== protocol.rs ==\n' sed -n '1,220p' crates/cli/src/mcp/protocol.rs printf '\n== search for response_for usages/tests ==\n' rg -n "response_for\\(|jsonrpc_error\\(|Invalid Request|method" crates/cli/src -g '!target' printf '\n== candidate tests in cli/mcp ==\n' fd -a . crates/cli/src/mcpRepository: NVIDIA/NeMo-Relay
Length of output: 6642
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '== mcp/session.rs ==\n' sed -n '1,240p' crates/cli/src/mcp/session.rs printf '\n== mcp/transport.rs ==\n' sed -n '1,220p' crates/cli/src/mcp/transport.rs printf '\n== mcp/mod.rs ==\n' sed -n '1,220p' crates/cli/src/mcp/mod.rsRepository: NVIDIA/NeMo-Relay
Length of output: 8913
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '== shared/mcp_tests.rs (relevant parts) ==\n' sed -n '1,260p' crates/cli/tests/coverage/shared/mcp_tests.rs printf '\n== search for MCP notification/request shape docs in repo ==\n' rg -n "notification|jsonrpc|protocolVersion|id.*method|method.*id" crates/cli docs README.md -g '!target'Repository: NVIDIA/NeMo-Relay
Length of output: 15731
🌐 Web query:
MCP 2025-06-18 request id must be string or integer and notifications must not include id💡 Result:
According to the Model Context Protocol (MCP) specification version 2025-06-18, the requirements for request IDs and notifications are as follows: Requests Requests must include a unique ID to initiate an operation [1][2]. This ID must be either a string or an integer [1][2][3]. Unlike the base JSON-RPC 2.0 specification, the ID must not be null [1][2]. Furthermore, the request ID must not have been previously used by the requestor within the same session [1][2]. Notifications Notifications are defined as one-way messages that do not expect a response [1][4]. Under the MCP specification, notifications must not include an ID [5][1][4]. It is important to note that while notifications themselves must not contain an ID field in their JSON-RPC structure [1], certain specific notification types, such as the cancellation notification (notifications/cancelled), include a request ID within their params object to reference a specific pending request [6][7].
Citations:
- 1: https://mcp-staging.mintlify.app/specification/2025-06-18/basic
- 2: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/ff960c9e/docs/specification/2025-06-18/basic/index.mdx
- 3: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-06-18/schema.ts
- 4: https://mcp.mintlify.app/specification/2025-11-25/basic
- 5: https://modelcontextprotocol.io/specification/2025-06-18/basic
- 6: https://mcp.mintlify.app/specification/2025-06-18/basic/utilities/cancellation
- 7: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-03-26/schema.ts
Check
methodbefore treating an object as a notification. ID-less frames like{"jsonrpc":"2.0"}or{"jsonrpc":"2.0","method":123}are currently dropped instead of returning-32600, andidis echoed without enforcing MCP’s string/integer request-ID shape. Only suppress responses for valid notifications.🤖 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/cli/src/mcp/protocol.rs` around lines 27 - 38, Update response_for to validate the method and request ID before applying notification suppression: reject missing or non-string method values and IDs that are neither strings nor integers with a -32600 Invalid Request response, while preserving null IDs for malformed requests as appropriate. Only return None for valid JSON-RPC notifications with a valid method and no ID.crates/cli/src/plugins/mod.rs (1)
1185-1194: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prevent clear from removing required fields.
This path ignores
field.optional, despite the UI promising that clear applies only to optional fields. Required nested fields can consequently be removed with Backspace/Delete.Proposed fix
let Some(field) = schema.fields.get(selected) else { return false; }; + if !field.optional { + return false; + } remove_value_field(value, field.name);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn clear_value_field( value: &mut Value, schema: &nemo_relay::config_editor::EditorSchema, selected: usize, ) -> bool { let Some(field) = schema.fields.get(selected) else { return false; }; if !field.optional { return false; } remove_value_field(value, field.name); true🤖 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/cli/src/plugins/mod.rs` around lines 1185 - 1194, The clear_value_field function must refuse to remove required fields. After retrieving the selected schema field, check field.optional and return false without calling remove_value_field when it is not optional; preserve the existing removal and true return for optional fields.crates/cli/src/plugins/schema/mod.rs (1)
1231-1242: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect coverage for resource-local references and duplicate anchors. fd -a -t f 'plugins_schema_tests.rs' . \ --exec rg -n -C4 '\$id|\$anchor|resource-local|nested resource' {}Repository: NVIDIA/NeMo-Relay
Length of output: 435
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the resolver implementation and related tests. for f in crates/cli/src/plugins/schema/mod.rs crates/cli/tests/plugins_schema_tests.rs; do echo "=== $f ===" wc -l "$f" done echo "=== resolver symbols ===" rg -n 'resolve_fragment|resolve_schema_chain|decode_reference_fragment|\$id|\$anchor' crates/cli/src/plugins/schema/mod.rs crates/cli/tests/plugins_schema_tests.rs echo "=== source slice mod.rs 1180-1325 ===" sed -n '1180,1325p' crates/cli/src/plugins/schema/mod.rs echo "=== tests slice plugins_schema_tests.rs 1-260 ===" sed -n '1,260p' crates/cli/tests/plugins_schema_tests.rsRepository: NVIDIA/NeMo-Relay
Length of output: 363
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== schema-related test files ===" fd -a -t f '.*schema.*test.*\.rs$' crates/cli crates -i echo "=== resolver symbols in mod.rs ===" rg -n 'resolve_fragment|resolve_schema_chain|decode_reference_fragment|\$id|\$anchor|writeOnly|secret' crates/cli/src/plugins/schema/mod.rs echo "=== mod.rs slice 1180-1314 ===" sed -n '1180,1314p' crates/cli/src/plugins/schema/mod.rs echo "=== likely test file candidates with $id/$anchor ===" fd -a -t f '.+\.rs$' crates/cli -i --exec rg -n -C3 '\$id|\$anchor|writeOnly|secret' {}Repository: NVIDIA/NeMo-Relay
Length of output: 9787
🏁 Script executed:
#!/bin/bash set -euo pipefail test_file=crates/cli/tests/coverage/shared/plugins_schema_tests.rs echo "=== anchor/writeOnly references ===" rg -n -C4 '\$anchor|\$id|writeOnly|secret|redact|unwrap' "$test_file" echo "=== focused slices ===" sed -n '120,240p' "$test_file" sed -n '240,420p' "$test_file"Repository: NVIDIA/NeMo-Relay
Length of output: 28370
Resolve fragments within the active schema resource.
resolve_fragmentwalks the whole document, so nested$idboundaries are ignored and duplicate anchors can bind to the wrong schema. That can desync validation from secret discovery and leave awriteOnlyfield unredacted. (crates/cli/src/plugins/schema/mod.rs:1231-1242, 1287-1305)🤖 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/cli/src/plugins/schema/mod.rs` around lines 1231 - 1242, Update resolve_schema_chain and its resolve_fragment call to resolve fragments against the currently active schema resource, honoring nested $id boundaries rather than searching the entire root document. Ensure duplicate anchors are scoped to that resource while preserving cycle and missing-reference errors, and apply the same resource-aware resolution in the related reference path around the downstream resolve_fragment usage.crates/cli/src/process/launcher.rs (1)
635-654: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact credentials from displayed exporter endpoints.
ATIF HTTP and OTLP endpoints are rendered verbatim in both the automatic TTY banner and
Also applies to: 662-665
🤖 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/cli/src/process/launcher.rs` around lines 635 - 654, Redact credentials before formatting exporter destinations in the destination-building logic around the OpenTelemetry and OpenInference branches. Reuse or add a URL-sanitization helper that removes userinfo and sensitive query values from ATIF HTTP and OTLP endpoints, then use the sanitized value in both the automatic TTY banner and --print output while preserving the existing fallback text.crates/cli/src/server/mod.rs (2)
98-107: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the broad
pkill -f nemo-relayrecovery command.This pattern kills every process whose command line contains
nemo-relay, including active agents and unrelated test runs. Direct users to managed shutdown or owner-PID identification instead.🤖 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/cli/src/server/mod.rs` around lines 98 - 107, Update the AddrInUse error message in the server launch error path to remove the broad `pkill -f nemo-relay` command. Replace it with guidance to use managed shutdown or identify and terminate only the owning daemon PID, while preserving the existing Windows and free-port alternatives.
239-241: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -nP -C5 \ '\b(?:pub(?:\([^)]*\))?\s+)?fn\s+load_worker_plugins\b|\bCommand::new\s*\(|\.clear_env\s*\(|\.env_remove\s*\(|NEMO_RELAY_BOOTSTRAP_SHUTDOWN_TOKEN' \ cratesRepository: NVIDIA/NeMo-Relay
Length of output: 50374
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the server and the cited surrounding code. sed -n '220,260p' crates/cli/src/server/mod.rs printf '\n---\n' sed -n '430,470p' crates/cli/src/server/mod.rs printf '\n---\n' sed -n '700,740p' crates/cli/src/server/mod.rs printf '\n=== plugin worker / plugin activation surfaces ===\n' rg -n -C 3 'PluginActivation::initialize|bootstrap_shutdown_channel|NEMO_RELAY_BOOTSTRAP_SHUTDOWN_TOKEN|env_remove|clear_env|Command::new\(' crates/cli/src crates/adaptive crates/core crates/node crates/python crates/ffiRepository: NVIDIA/NeMo-Relay
Length of output: 18572
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the activation and worker-launch code paths. ast-grep outline crates/core/src/plugin/dynamic/worker.rs --view expanded printf '\n---\n' ast-grep outline crates/cli/src/server/mod.rs --view expanded printf '\n---\n' rg -n -C 4 'struct PluginActivation|impl PluginActivation|async fn initialize|fn initialize\(' crates/cli/src crates/core/src crates/adaptive/srcRepository: NVIDIA/NeMo-Relay
Length of output: 17280
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '146,240p' crates/core/src/plugin/dynamic/worker.rs printf '\n---\n' sed -n '677,790p' crates/core/src/plugin/dynamic/worker.rs printf '\n---\n' sed -n '439,470p' crates/cli/src/server/mod.rs printf '\n---\n' sed -n '630,760p' crates/cli/src/server/mod.rsRepository: NVIDIA/NeMo-Relay
Length of output: 13858
Move the bootstrap shutdown token out of the ambient environment before plugin activation.
serve_listener_with_dynamic_inner()initializes worker plugins beforebootstrap_shutdown_channel()reads the token, and worker processes inherit the parent environment by default. Read and removeNEMO_RELAY_BOOTSTRAP_SHUTDOWN_TOKENbefore any plugin workers spawn, then pass it only to the bootstrap shutdown path.🤖 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/cli/src/server/mod.rs` around lines 239 - 241, Update serve_listener_with_dynamic_inner() to read and remove NEMO_RELAY_BOOTSTRAP_SHUTDOWN_TOKEN from the environment before PluginActivation::initialize() can spawn worker plugins, then pass the captured token exclusively into bootstrap_shutdown_channel() or its bootstrap shutdown handling path. Preserve the existing plugin activation flow while ensuring workers cannot inherit the token.crates/cli/src/sessions/correlation.rs (2)
201-210: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not persist the weak subagent-start ownership guess.
subagent_startis explicitly a weak newest-worker heuristic inSession::set_last_subagent_start_owner, but this list lets it teach request affinity. The first unhinted call can therefore permanently route matching request payloads to the guessed worker for the rest of the turn.Proposed fix
| "matched_hint" | "active_subagent" - | "subagent_start" | "request_affinity"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pub(super) fn owner_status_teaches_request_affinity(status: &str) -> bool { matches!( status, "explicit" | "single_hint" | "matched_hint" | "active_subagent" | "request_affinity" )🤖 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/cli/src/sessions/correlation.rs` around lines 201 - 210, Remove "subagent_start" from the statuses accepted by owner_status_teaches_request_affinity. Preserve affinity teaching for explicit, hint-based, active-subagent, and request-affinity statuses so the weak guess from Session::set_last_subagent_start_owner is not persisted.
320-352: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not treat one retained session as one active session.
single_active_session_idchecks onlyHashMapcardinality. Idle closure deliberately retains dormant agent roots, so a later headerless request can join a stale session and inherit its agent identity, metadata, and configuration. Select an actually active or recent session, or isolate the request when the sole entry is dormant.🤖 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/cli/src/sessions/correlation.rs` around lines 320 - 352, The single_active_session_id fallback must not equate one retained session with one active session. Update single_active_session_id to select the sole session only when it is currently active or recent according to the existing Session state/timestamps; otherwise return None so gateway_session_for_call isolates headerless requests through its unique-root path. Preserve explicit session_id handling and the empty-session behavior.crates/cli/src/sessions/idle.rs (1)
43-95: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reserve session IDs while idle closure runs.
After Line 55 removes an idle session, another hook or gateway request can recreate the same ID. Line 88 then overwrites that new live session with the old retained state, losing handles and lifecycle events; the subsequent alignment cleanup can also clear newly created ownership state. Use a tombstone or generation check, or keep the session exclusively owned until closure and reinsertion complete.
🤖 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/cli/src/sessions/idle.rs` around lines 43 - 95, Prevent idle-session cleanup from overwriting or clearing a newer session created with the same ID while close_turn_for_reason runs. Update the idle-session removal and reinsertion flow around the sessions map and retained_sessions so each removed session remains exclusively owned, or use a tombstone/generation check before reinserting and applying clear_for_ended_subagent. Preserve cleanup for the original session while skipping stale retention and alignment cleanup when a newer live session exists.crates/cli/tests/architecture_tests.rs (1)
47-124: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Inspect Rust syntax instead of matching raw source text.
Legal forms such as
use crate::{commands::run};,use clap::{Parser};, and grouped agent imports bypass these checks, while comments can trigger false failures. Parse or tokenize imports and attributes so these tests actually enforce the stated boundaries.As per path instructions, tests should cover the behavior promised by the changed surface.
Also applies to: 141-202
🤖 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/cli/tests/architecture_tests.rs` around lines 47 - 124, The architecture tests currently rely on raw source-text matching, allowing grouped imports through and producing false positives from comments. Update shared-services, clap-syntax, and agent-dependency checks to parse or tokenize Rust imports and attributes, correctly detecting grouped forms such as crate and agent imports while ignoring comments. Extend the tests to cover these legal grouped-import cases and the promised boundary behavior.Source: Path instructions
crates/cli/tests/cli_tests.rs (1)
3199-3207: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Terminate Relay through its signal handler on startup timeout.
Line 3202 sends SIGKILL only to Relay. If the agent spawned between lines 3152-3157 but has not published the PID file, the infinite-loop agent and its
sleepdescendant survive and can pollute subsequent tests.Proposed fix
- let _ = relay.kill(); - let output = relay.wait_with_output().unwrap(); + // SAFETY: Relay is owned by this test; SIGTERM permits supervised cleanup. + let _ = unsafe { libc::kill(relay.id() as i32, libc::SIGTERM) }; + let output = wait_child_with_output(relay);As per path instructions, review automation changes for reproducibility.
🤖 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/cli/tests/cli_tests.rs` around lines 3199 - 3207, Update the startup-timeout cleanup in the PID-file wait loop to terminate Relay through its registered signal handler instead of calling relay.kill(), which sends SIGKILL. Send the graceful termination signal to Relay, then wait for it to exit so its agent and sleep descendants are cleaned up before the test panics.Source: Path instructions
crates/cli/tests/coverage/agents/hermes_tests.rs (1)
603-642: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that reinstallation removes the previous hook command.
The test confirms one new command exists but would still pass if every event retained the old generation command. Assert that
first_commandoccurs zero times after reinstall to fully validate idempotency.As per path instructions, tests should cover lifecycle behavior promised by the changed API surface.
🤖 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/cli/tests/coverage/agents/hermes_tests.rs` around lines 603 - 642, Extend the reinstallation assertions around first_command to verify that the old hook command occurs zero times in the updated hooks["on_session_start"] array. Keep the existing assertion for exactly one second_command, ensuring the lifecycle test validates removal of the previous generation command.Source: Path instructions
crates/cli/tests/coverage/agents/launcher_tests.rs (2)
850-883: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Create both Hermes launches concurrently.
Both overlays are currently constructed serially, so this cannot catch races in temporary-path allocation or overlay population. Start the calls in separate threads behind a barrier before asserting isolation.
As per path instructions, tests should cover cross-request isolation where relevant.
🤖 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/cli/tests/coverage/agents/launcher_tests.rs` around lines 850 - 883, Update concurrent_hermes_runs_use_independent_overlays_without_mutating_user_config to construct both PreparedAgentLaunch instances concurrently, using separate threads synchronized by a barrier so both calls begin together. Join the threads, collect their results, then assert the generated overlays are distinct and the original user configuration remains unchanged.Source: Path instructions
1512-1524: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the nonexistent path the inferred executable.
With
agent: Some(CodingAgent::Codex), this path is appended to the defaultcodexcommand. A dry-run regression could therefore launch a real Codex installation instead of failing deterministically.Proposed fix
- agent: Some(CodingAgent::Codex), + agent: None, ... - command: vec!["/path/that/does/not/exist".into()], + command: vec!["/path/that/does/not/exist/codex".into()],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.#[tokio::test] async fn dry_run_does_not_spawn_agent() { let command = RunOverrides { agent: None, config: None, openai_base_url: None, anthropic_base_url: None, session_metadata: None, plugin_config_path: None, dry_run: true, print: false, command: vec!["/path/that/does/not/exist/codex".into()], };🤖 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/cli/tests/coverage/agents/launcher_tests.rs` around lines 1512 - 1524, Update dry_run_does_not_spawn_agent so the nonexistent path is used as the inferred executable rather than being appended to the default Codex command. Configure the command inputs accordingly while preserving dry_run: true, ensuring the test fails deterministically without launching a real Codex installation.crates/cli/tests/coverage/agents/plugin_install_tests.rs (1)
3663-3697: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the preexisting symlink dangling after rollback.
The target is absent before staging, but Line 3696 requires it to exist afterward. That permits staging to leave external filesystem residue instead of preserving the original state.
Proposed fix
- assert!(symlink_target.exists()); + assert!(!symlink_target.exists());If this fails, fix staging cleanup so it does not follow or create the symlink target.
As per path instructions, tests should cover error paths and the promised rollback behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.#[cfg(unix)] #[test] fn failed_staging_preserves_a_preexisting_dangling_generation_lock_symlink() { use std::os::unix::fs::symlink; let dir = tempdir().unwrap(); let target = PluginLayout::new(CodingAgent::Codex, dir.path()); let symlink_target = dir.path().join("generation-lock-target"); symlink(&symlink_target, &target.generation_lock).unwrap(); let stage_parent = dir.path().join("deterministic-symlink-stage"); let staged = PluginLayout::new(CodingAgent::Codex, &stage_parent); crate::filesystem::fail_next_atomic_write(&staged.mcp_config); let error = match stage_plugin_marketplace_at( CodingAgent::Codex, Path::new("/bin/nemo-relay"), &target, true, &options(dir.path()), stage_parent.clone(), ) { Ok(_) => panic!("staging unexpectedly succeeded"), Err(error) => error, }; assert!(error.contains("injected test failure"), "{error}"); assert!(!stage_parent.exists()); assert!( std::fs::symlink_metadata(&target.generation_lock) .unwrap() .file_type() .is_symlink() ); assert!(!symlink_target.exists()); }🤖 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/cli/tests/coverage/agents/plugin_install_tests.rs` around lines 3663 - 3697, Correct failed staging rollback so the preexisting generation-lock symlink remains dangling and its external target is neither followed nor created. Update the cleanup logic used by stage_plugin_marketplace_at, then adjust failed_staging_preserves_a_preexisting_dangling_generation_lock_symlink to assert the symlink remains while symlink_target stays absent, preserving the existing staging-error and temporary-directory cleanup assertions.Source: Path instructions
crates/cli/tests/coverage/shared/bootstrap_state_tests.rs (1)
225-266: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound mock network-server operations across the test suite.
Blocking listeners and reads convert client regressions into indefinite CI hangs rather than test failures.
crates/cli/tests/coverage/shared/bootstrap_state_tests.rs#L225-L266: Add deadlines for both expected connections and request reads.crates/cli/tests/coverage/shared/bootstrap_tests.rs#L118-L181: Replace assumed connection counts with bounded accepts.crates/cli/tests/coverage/shared/doctor_tests.rs#L13-L45: Add accept/read timeouts to the shared capture server.crates/cli/tests/coverage/shared/doctor_tests.rs#L1294-L1454: Apply the same bounded helper to standalone probe servers.📍 Affects 3 files
crates/cli/tests/coverage/shared/bootstrap_state_tests.rs#L225-L266(this comment)crates/cli/tests/coverage/shared/bootstrap_tests.rs#L118-L181crates/cli/tests/coverage/shared/doctor_tests.rs#L13-L45crates/cli/tests/coverage/shared/doctor_tests.rs#L1294-L1454🤖 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/cli/tests/coverage/shared/bootstrap_state_tests.rs` around lines 225 - 266, Bound all mock network-server operations to prevent indefinite test hangs: in crates/cli/tests/coverage/shared/bootstrap_state_tests.rs:225-266, add deadlines to both expected listener accepts and request reads; in crates/cli/tests/coverage/shared/bootstrap_tests.rs:118-181, replace assumed connection counts with bounded accepts; in crates/cli/tests/coverage/shared/doctor_tests.rs:13-45, add accept and read timeouts to the shared capture server; and in crates/cli/tests/coverage/shared/doctor_tests.rs:1294-1454, apply the same bounded-server helper to the standalone probe servers.crates/cli/tests/coverage/shared/config_tests.rs (1)
480-522: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove root-dependent unreadability fixtures.
Unix mode
000does not prevent root from reading a file, so these tests fail in privileged containers.
crates/cli/tests/coverage/shared/config_tests.rs#L480-L522: Use deterministic configuration-read failures or handle privileged execution.crates/cli/tests/coverage/shared/config_tests.rs#L1765-L1787: Inject the bounded-reader failure or skip the permission-only assertion for root.📍 Affects 1 file
crates/cli/tests/coverage/shared/config_tests.rs#L480-L522(this comment)crates/cli/tests/coverage/shared/config_tests.rs#L1765-L1787🤖 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/cli/tests/coverage/shared/config_tests.rs` around lines 480 - 522, Replace the permission-mode-based unreadable-file setup in crates/cli/tests/coverage/shared/config_tests.rs:480-522 with a deterministic configuration-read failure, or explicitly skip the permission-only assertions when running as root. Apply the same bounded-reader failure injection or root skip to the permission assertion at crates/cli/tests/coverage/shared/config_tests.rs:1765-1787, preserving checks that errors include the relevant source path.crates/cli/tests/coverage/shared/install_generation_tests.rs (1)
393-423: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,260p' crates/cli/tests/coverage/shared/install_generation_tests.rsRepository: NVIDIA/NeMo-Relay
Length of output: 9753
🏁 Script executed:
sed -n '1,260p' crates/cli/src/commands/mod.rsRepository: NVIDIA/NeMo-Relay
Length of output: 7578
🏁 Script executed:
sed -n '340,460p' crates/cli/tests/coverage/shared/install_generation_tests.rsRepository: NVIDIA/NeMo-Relay
Length of output: 5042
🏁 Script executed:
rg -n "CwdTestScope|CWD_TEST_LOCK|lock_cwd|current_dir|set_current_dir" crates/cli/tests/coverage/shared/install_generation_tests.rs crates/cli/src/commands/mod.rsRepository: NVIDIA/NeMo-Relay
Length of output: 1841
🏁 Script executed:
rg -n "CwdTestScope::(locked|enter)\(" crates/cli tests crates/cli/testsRepository: NVIDIA/NeMo-Relay
Length of output: 1415
🏁 Script executed:
rg -n "pub\(crate\) mod test_support|use crate::commands::test_support|use super::test_support" crates/cli/src crates/cli/testsRepository: NVIDIA/NeMo-Relay
Length of output: 220
Serialize
relative_legacy_marker_reencodes_an_absolute_lock_for_rollbackwith the shared CWD guard
crates/cli/tests/coverage/shared/install_generation_tests.rs:393-423reads and later resolves multiple relative paths against the process CWD, but it doesn’t acquirecrate::test_support::CwdTestScope::locked()like the other CWD-dependent CLI tests. A parallel test that callsset_current_dircan redirecttempdir_in,rename, orInstallGeneration::capturemid-flight. Add the shared guard for the full test.🤖 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/cli/tests/coverage/shared/install_generation_tests.rs` around lines 393 - 423, Add crate::test_support::CwdTestScope::locked() at the start of relative_legacy_marker_reencodes_an_absolute_lock_for_rollback and retain the guard for the entire test, covering all relative-path operations and InstallGeneration::capture.crates/cli/tests/coverage/shared/mcp_gateway_tests.rs (1)
27-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files crates/cli/tests/coverage/shared/mcp_gateway_tests.rs crates/cli/src | sed -n '1,120p' echo '--- mcp_gateway_tests.rs ---' cat -n crates/cli/tests/coverage/shared/mcp_gateway_tests.rs | sed -n '1,220p' echo '--- search maintain_gateway_with ---' rg -n "maintain_gateway_with|production_heartbeat_recovers_after_one_thirty_second_interval|advance\\(Duration::from_secs\\(30\\)\\)" crates/cli -S echo '--- relevant implementation ---' fd -a "mcp_gateway" crates/cli crates -t f 2>/dev/null | sed -n '1,120p'Repository: NVIDIA/NeMo-Relay
Length of output: 16482
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- maintain_gateway_with implementation ---' sed -n '235,360p' crates/cli/src/mcp/gateway.rs echo '--- similar paused-time tests in mcp_tests.rs ---' sed -n '300,700p' crates/cli/tests/coverage/shared/mcp_tests.rsRepository: NVIDIA/NeMo-Relay
Length of output: 19435
Yield once before advancing the clock.
tokio::spawnmay not pollmaintain_gateway_withbeforeadvance(30s), so this can auto-advance into the next timer and pass without proving the restart happened after the first interval. Add a startup barrier ortokio::task::yield_now().awaitbefore advancing.🤖 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/cli/tests/coverage/shared/mcp_gateway_tests.rs` around lines 27 - 55, Ensure maintain_gateway_with is polled once before advancing simulated time in production_heartbeat_recovers_after_one_thirty_second_interval. Add a startup synchronization barrier or await tokio::task::yield_now() immediately after spawning the monitor and before advance(Duration::from_secs(30)), preserving the test’s verification that recovery occurs after the first interval.Source: Path instructions
crates/cli/src/filesystem/snapshots.rs (1)
13-26: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Publish backup files atomically before accepting them.
Line 19 copies directly to the final path, while Line 15 treats any existing path as complete. An interrupted copy leaves a partial backup that future calls accept and rollback can restore over Claude or Codex configuration. Write and sync a temporary file, preserve permissions/DACLs, then atomically rename it.
🤖 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/cli/src/filesystem/snapshots.rs` around lines 13 - 26, Update backup to create a temporary file beside the destination, copy the source into it, preserve the source permissions and platform-specific DACLs, and sync it before publication. Atomically rename the completed temporary file to the path returned by backup_path, ensuring existing destination checks never accept an interrupted partial copy and cleaning up temporary files on failure.crates/cli/src/plugins/lifecycle/environment.rs (2)
96-106: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject entrypoints containing additional separators.
Line 96 accepts
pkg.module:factory:extrabecausesplit_onceleaves the second colon incallable. This passes provisioning validation but is not the promisedmodule:functionform.Proposed fix
if callable.is_empty() + || callable.contains(':') || module.is_empty()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let (module, callable) = entrypoint.split_once(':').ok_or_else(|| { format!( "Python worker load.entrypoint '{entrypoint}' must use the unambiguous module:function form" ) })?; if callable.is_empty() || callable.contains(':') || module.is_empty() || module .split('.') .any(|segment| segment.is_empty() || segment.contains(['/', '\\', ':'])) {🤖 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/cli/src/plugins/lifecycle/environment.rs` around lines 96 - 106, Update the entrypoint validation around the module/callable parsing to reject callable values containing additional ':' separators, while preserving valid module:function entries and the existing module-segment checks. Ensure inputs such as pkg.module:factory:extra fail provisioning validation.
478-480: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use an unambiguous tree-digest encoding.
Concatenating
lossy_path + NUL + contentspermits different file trees to produce the same digest, and non-UTF-8 paths can collapse underto_string_lossy. The HMAC authenticates that ambiguous digest, so it does not prevent this attestation bypass. Frame entry type, raw path, and payload lengths as the snapshot digest already does.Proposed fix
- digest.update(relative.to_string_lossy().as_bytes()); - digest.update([0]); + let relative = relative.as_os_str().as_encoded_bytes(); + digest.update([0]); // Regular file. + digest.update((relative.len() as u64).to_le_bytes()); + digest.update(relative); + digest.update((bytes.len() as u64).to_le_bytes()); digest.update(&bytes);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.digest.update([0]); // Regular file. let relative = relative.as_os_str().as_encoded_bytes(); digest.update((relative.len() as u64).to_le_bytes()); digest.update(relative); digest.update((bytes.len() as u64).to_le_bytes()); digest.update(&bytes);🤖 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/cli/src/plugins/lifecycle/environment.rs` around lines 478 - 480, Update the tree-digest construction around the digest update calls to use unambiguous framing like the snapshot digest: encode the entry type, raw path bytes, and payload length before the payload, preserving non-UTF-8 path bytes without lossy conversion. Ensure distinct file trees cannot produce the same digest before HMAC authentication.crates/cli/src/plugins/lifecycle/mod.rs (1)
271-300: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply Python entrypoint validation in both
plugins validatebranches.These paths report success—and the ID path records valid status—without calling
validate_python_entrypoint_artifact. A manifest rejected byplugins addor activation can therefore be reported as valid here.Proposed fix
let (manifest, manifest_ref) = load_manifest_for_action("validate", &path)?; + validate_python_entrypoint_artifact(&manifest, &manifest_ref).map_err(|message| { + plugin_failed_with_code( + "plugins validate", + Some(command.target.clone()), + "manifest_invalid", + message, + ) + })?; load_config_schema_for_manifest(&manifest, &manifest_ref)?;Apply the equivalent check after loading the manifest in the plugin-ID branch.
Also applies to: 310-327
🤖 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/cli/src/plugins/lifecycle/mod.rs` around lines 271 - 300, Call validate_python_entrypoint_artifact for the loaded manifest in both plugins validate branches, including the path branch shown and the plugin-ID branch near the corresponding success handling. Perform the check after loading the manifest and before reporting success or recording valid status, propagating any validation error consistently with the existing flow.crates/cli/src/plugins/pricing.rs (1)
143-156: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound pricing catalog reads before allocation.
read_to_stringaccepts an arbitrarily large configured file and can exhaust process memory before parsing. Use the repository’s bounded regular-file reader, then validate UTF-8.Proposed fix
- let raw = std::fs::read_to_string(path).map_err(|source| { - CliError::Config(format!( - "could not read model pricing catalog '{}': {source}", - path.display() - )) - })?; + let bytes = crate::filesystem::bounded::read_bounded_regular_file( + path, + "model pricing catalog", + ) + .map_err(CliError::Config)?; + let raw = std::str::from_utf8(&bytes).map_err(|source| { + CliError::Config(format!( + "model pricing catalog '{}' is not valid UTF-8: {source}", + path.display() + )) + })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn read_pricing_catalog(path: &Path) -> Result<PricingCatalog, CliError> { let bytes = crate::filesystem::bounded::read_bounded_regular_file( path, "model pricing catalog", ) .map_err(CliError::Config)?; let raw = std::str::from_utf8(&bytes).map_err(|source| { CliError::Config(format!( "model pricing catalog '{}' is not valid UTF-8: {source}", path.display() )) })?; PricingCatalog::from_json_str(raw).map_err(|error| { CliError::Config(format!( "invalid model pricing catalog '{}': {error}", path.display() )) }) }🤖 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/cli/src/plugins/pricing.rs` around lines 143 - 156, Update read_pricing_catalog to replace std::fs::read_to_string with the repository’s bounded regular-file reader, applying the configured size limit before allocation and preserving the existing read-error context. Convert the bounded reader’s bytes to UTF-8 explicitly, mapping invalid UTF-8 to CliError::Config, then pass the resulting string to PricingCatalog::from_json_str with the existing parse-error context.crates/cli/src/process/detached.rs (2)
80-106: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Declared Rust toolchain:" fd -a '^rust-toolchain(\.toml)?$' . \ --exec sh -c 'echo "--- $1"; sed -n "1,120p" "$1"' sh {} echo "Process-spawn sites requiring inheritance coordination:" rg -n -C4 '\bspawn_detached\s*\(|\bCommand::new\s*\(|\.spawn\s*\(' crates/cli/srcRepository: NVIDIA/NeMo-Relay
Length of output: 14711
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== crates/cli/src/process/detached.rs ===" wc -l crates/cli/src/process/detached.rs sed -n '1,260p' crates/cli/src/process/detached.rs echo echo "=== HandleInheritanceGuard definition/usages ===" rg -n -C4 'struct HandleInheritanceGuard|impl HandleInheritanceGuard|suppress\(|restore\(' crates/cli/src echo echo "=== terminate_tree / kill fallback sites ===" rg -n -C5 'terminate_tree|child\.kill\(\)|taskkill|kill_on_drop|ProcessTree' crates/cli/src/process crates/cli/src/bootstrap crates/cli/src/gatewayRepository: NVIDIA/NeMo-Relay
Length of output: 36220
🏁 Script executed:
#!/bin/bash set -euo pipefail sysroot="$(rustc --print sysroot)" echo "SYSROOT=$sysroot" echo echo "=== Windows process implementation in std ===" fd -a 'process.rs$' "$sysroot/lib/rustlib/src/rust/library/std/src/os/windows" || true if [ -f "$sysroot/lib/rustlib/src/rust/library/std/src/os/windows/process.rs" ]; then sed -n '1,260p' "$sysroot/lib/rustlib/src/rust/library/std/src/os/windows/process.rs" fi echo echo "=== Search for handle list / inheritance in std sources ===" rg -n -C3 'PROC_THREAD_ATTRIBUTE_HANDLE_LIST|HANDLE_LIST|bInheritHandles|SetHandleInformation|handle_list|inherit' \ "$sysroot/lib/rustlib/src/rust/library/std/src" | head -n 200Repository: NVIDIA/NeMo-Relay
Length of output: 198
🌐 Web query:
Rust std::process::Command Windows PROC_THREAD_ATTRIBUTE_HANDLE_LIST handle inheritance source💡 Result:
In Rust, you can control handle inheritance when spawning a child process on Windows using the
std::os::windows::process::CommandExttrait [1][2]. To usePROC_THREAD_ATTRIBUTE_HANDLE_LISTfor explicit handle inheritance, you must interact with theProcThreadAttributeListAPI provided by the standard library [3][4]. Key Mechanisms: 1. CommandExt::spawn_with_attributes: This method allows you to pass aProcThreadAttributeListto theCreateProcesscall [1][3]. You can use this to include thePROC_THREAD_ATTRIBUTE_HANDLE_LISTattribute, which contains a specific whitelist of handles that the child process is permitted to inherit [5][4]. 2. Requirements for the Attribute: When using this attribute, you must set thebInheritHandlesparameter of the underlyingCreateProcessfunction toTRUE[4]. This is done via theCommandExt::inherit_handles(true)method [1][2]. 3. Handle Setup: The handles added to this list must have been created as inheritable handles (e.g., usingSECURITY_ATTRIBUTESwithbInheritHandleset to true) and cannot include pseudo-handles like those fromGetCurrentProcessorGetCurrentThread[5][4]. For more granular control, theCommandExttrait also providesinherit_handles(false), which disables the default behavior where all inheritable handles in the calling process are passed to the new process [1][2]. If you require a strict whitelist, the recommended approach is to setinherit_handles(true)and then supply aProcThreadAttributeListcontaining exactly the handles you wish to expose to the child [6][4][7]. Note that some methods related to these extensions (such asinherit_handlesor specificProcThreadAttributeListbuilders) may be behind unstable or nightly feature flags in certain Rust versions [1][2], so you should check the official documentation for the version of the compiler you are using [1].Citations:
- 1: https://doc.rust-lang.org/stable/std/os/windows/process/trait.CommandExt.html
- 2: https://doc.rust-lang.org/std/os/windows/process/trait.CommandExt.html
- 3: https://doc.rust-lang.org/std/os/windows/process/struct.ProcThreadAttributeListBuilder.html
- 4: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute
- 5: https://learn.microsoft.com/en-us/windows/win32/procthread/inheritance
- 6: rust-lang/rust#115501
- 7: https://docs.rs/running-process-core/latest/src/running_process_core/spawn.rs.html
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Search the repo for explicit handle-list or Windows process spawn notes:" rg -n -C3 'PROC_THREAD_ATTRIBUTE_HANDLE_LIST|handle list|inherit handles|bInheritHandles|STARTUPINFOEX|SetHandleInformation|HandleInheritanceGuard' .Repository: NVIDIA/NeMo-Relay
Length of output: 4089
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,230p' crates/cli/src/process/detached.rs | rg -n -C2 'test|#[[:space:]]*cfg\(test\)|handle|inherit|taskkill|terminate_tree|spawn_detached'Repository: NVIDIA/NeMo-Relay
Length of output: 4269
🌐 Web query:
site:doc.rust-lang.org std::process::Command Windows handle_list bInheritHandles PROC_THREAD_ATTRIBUTE_HANDLE_LIST💡 Result:
In Rust's standard library, support for managing the inheritance of handles and specific process attributes on Windows is provided through the std::os::windows::process::CommandExt trait [1][2]. The specific Windows attribute PROC_THREAD_ATTRIBUTE_HANDLE_LIST is used with the UpdateProcThreadAttribute function to restrict handle inheritance to a specific subset of handles, rather than inheriting all inheritable handles [1][2]. While Rust provides a mechanism to use arbitrary attributes via spawn_with_attributes, handling handle lists requires manual construction of the attribute list [1][3]. 1. InheritHandles: The CommandExt trait includes an inherit_handles(bool) method (currently nightly-only, behind feature flag windows_process_extensions_inherit_handles) [4][2]. When set to true (the default), all inheritable handles in the calling process are inherited by the child [1][2]. Setting this to false prevents the default inheritance of all handles [1][4]. 2. PROC_THREAD_ATTRIBUTE_HANDLE_LIST: To achieve more granular control—such as passing only a specific list of handles—you can use the spawn_with_attributes method (nightly-only, behind feature flag windows_process_extensions_raw_attribute) [1][3][5]. You would construct a ProcThreadAttributeList and populate it using the raw_attribute method, providing the handle list as the value [3][6]. Note that the attribute value for PROC_THREAD_ATTRIBUTE_HANDLE_LIST is a pointer to an array of HANDLE values, and its size must be correctly specified [3]. Since both spawn_with_attributes and inherit_handles are currently experimental/nightly-only features, they require the use of #![feature(...)] attributes in your Rust code and are subject to change [4][2][7]. Accessing these functions requires importing the CommandExt trait [1][2].
Citations:
- 1: https://doc.rust-lang.org/stable/std/os/windows/process/trait.CommandExt.html
- 2: https://doc.rust-lang.org/beta/std/os/windows/process/trait.CommandExt.html
- 3: https://doc.rust-lang.org/std/os/windows/process/struct.ProcThreadAttributeListBuilder.html
- 4: https://doc.rust-lang.org/stable/std/process/struct.Command.html
- 5: https://doc.rust-lang.org/std/process/struct.Command.html?mut=
- 6: https://doc.rust-lang.org/stable/src/std/os/windows/process.rs.html
- 7: https://doc.rust-lang.org/nightly/std/process/struct.Command.html
Use an explicit Windows handle allowlist here. Clearing inherit bits on only stdin/stdout/stderr still leaves every other inheritable handle eligible for the detached gateway, and the temporary flag flip is only serialized for this helper. Switch to
PROC_THREAD_ATTRIBUTE_HANDLE_LIST/spawn_with_attributes(or equivalentCreateProcessWsetup) so only the intended handles can cross into the child.🤖 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/cli/src/process/detached.rs` around lines 80 - 106, Update spawn_detached to use an explicit Windows handle allowlist through PROC_THREAD_ATTRIBUTE_HANDLE_LIST with spawn_with_attributes or equivalent CreateProcessW setup. Pass only the intended standard handles to the child, replacing the current HandleInheritanceGuard::suppress approach while preserving the existing spawn, restoration, and cleanup error handling.
216-224: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C5 '\bterminate_tree\s*\(|taskkill|child\.kill\s*\(' \ crates/cli/src crates/cli/testsRepository: NVIDIA/NeMo-Relay
Length of output: 9473
🏁 Script executed:
#!/bin/bash set -euo pipefail ast-grep outline crates/cli/src/process/detached.rs --view expanded sed -n '1,260p' crates/cli/src/process/detached.rs printf '\n--- windows spawn usage ---\n' rg -n -C3 'configure_detached\(|terminate_tree\(|creation_flags|DETACHED_PROCESS|CREATE_NEW_PROCESS_GROUP|job object|taskkill|kill\(\)' crates/cli/srcRepository: NVIDIA/NeMo-Relay
Length of output: 21421
Avoid the
child.kill()fallback on Windows.
Iftaskkill /T /Ffails,child.kill()only stops the direct process and can leave descendants running, which breaks tree cleanup during reaper shutdown. Switch this path to a Windows Job Object kill or surface the cleanup failure instead.🤖 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/cli/src/process/detached.rs` around lines 216 - 224, Update the Windows cleanup branch in the detached-process termination flow to remove the child.kill() fallback after taskkill fails. Use the existing Windows Job Object termination mechanism if available, or propagate/report the cleanup failure instead of attempting direct-child termination, preserving complete process-tree cleanup.
Signed-off-by: Will Killian <wkillian@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cli/src/gateway/mod.rs (1)
640-668: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract shared header-forwarding/auth-injection logic to avoid drift.
forward_upstream_requestandmodelsindependently reimplement the same sequence — strip replaceable auth headers, loop withshould_forward_request_header, theninject_provider_auth— differing only in how the request builder/body is constructed. Since this is the exact code path that decides which credentials leave the process, having it duplicated risks the two copies silently diverging on a future header-policy change.♻️ Proposed refactor sketch
+fn apply_forwarding_headers( + mut builder: reqwest::RequestBuilder, + headers: &HeaderMap, + forwarding: ProviderForwarding, +) -> reqwest::RequestBuilder { + let sanitized = strip_replaceable_agent_auth_headers( + headers, + forwarding.route, + forwarding.allow_environment_provider_auth, + ); + for (name, value) in &sanitized { + if should_forward_request_header(name, &sanitized) { + builder = builder.header(name, value); + } + } + inject_provider_auth(builder, forwarding.route, &sanitized, forwarding.allow_environment_provider_auth) +}Then both
forward_upstream_requestandmodelscallapply_forwarding_headers(upstream, headers, forwarding)instead of repeating the loop.Also applies to: 858-874
🤖 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/cli/src/gateway/mod.rs` around lines 640 - 668, Extract the shared header policy from forward_upstream_request and the corresponding models path into an apply_forwarding_headers helper that accepts a request builder, headers, and ProviderForwarding. Keep strip_replaceable_agent_auth_headers, should_forward_request_header, and inject_provider_auth in that helper, then have both callers construct their request-specific body/builder and delegate header forwarding and auth injection to it.
🤖 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/cli/src/agents/claude/host.rs`:
- Around line 51-53: Update the backup flow around managed_provider and
backup_claude_settings to preserve the original pre-Relay settings across
gateway URL changes. Before overwriting the backup, detect whether the existing
backup already represents the user’s original configuration and retain it;
ensure uninstall restoration never replaces the original ANTHROPIC_BASE_URL with
a prior Relay gateway value.
In `@crates/cli/src/commands/configure/model.rs`:
- Around line 77-84: Update the comment above the CodingAgent::ALL iteration to
describe filtering agents whose executable resolves on PATH, removing the
outdated “Pairs” wording. Keep the existing resolve_executable_in_path
presence-check logic unchanged.
In `@crates/cli/src/commands/diagnostics.rs`:
- Around line 14-24: Handle the combination of DoctorCommand.agent and
DoctorCommand.plugin before entering the plugin execution path. When both are
provided, reject the command with a clear validation error (or explicitly scope
the plugin operation to the selected agent) instead of silently discarding
agent; preserve existing behavior when only one option is set.
In `@crates/cli/src/commands/install.rs`:
- Around line 85-131: Update the agent-processing loops in install and uninstall
so an Err from one integration is recorded as a non-success result while
processing continues for every remaining agent. Preserve successful processing
and existing status propagation, and ensure both install_integration and
uninstall_integration failures are handled consistently without using ? to abort
the batch.
In `@crates/cli/src/diagnostics/render.rs`:
- Around line 268-273: Update agents_report to preserve and surface
resolve_server_config errors instead of silently applying
GatewayConfig::default. Match collect_report’s existing error-reporting behavior
for config resolution while retaining the current collect_agents flow when
resolution succeeds.
In `@crates/cli/src/gateway/client.rs`:
- Around line 98-115: Cache the loaded bootstrap challenge key and Relay TLS
identity instead of reloading them on each post_verified invocation. Add
OnceLock/OnceCell-backed initialization for BootstrapChallengeKey::load and
RelayTlsIdentity::load, reuse those cached values in post_verified and
probe_with_instance, and preserve the existing error mapping during first
initialization.
- Around line 323-362: The request_shutdown flow must authenticate and issue the
shutdown request on the same TCP connection established by the gateway probe.
Reuse the existing post_verified pattern used by bootstrap/state.rs, updating
request_shutdown and its caller as needed so the verified connection is retained
and the token is never sent over a newly connected socket; preserve the existing
timeout, response validation, and error behavior.
In `@crates/cli/src/hooks/response.rs`:
- Around line 41-60: Update handle_verified_hook_forward_response so
StatusCode::from_u16 conversion failures honor fail_closed instead of
propagating unconditionally via ?. Preserve the existing CliError::Install error
for fail-closed mode, while returning the successful fail-open result for
invalid statuses.
In `@crates/cli/src/installation/operation_lock.rs`:
- Around line 28-35: Update PluginOperationLock::acquire to detect when
install_dir and global_lock_dir resolve to the same path, then avoid acquiring
the install-root lock a second time while preserving the existing global lock
and returned guard behavior. Use the existing acquire_lock_file flow and
represent the skipped root lock consistently with the PluginOperationLock
fields.
In `@crates/cli/src/server/mod.rs`:
- Around line 237-243: Move NEMO_RELAY_BOOTSTRAP_SHUTDOWN_TOKEN capture and
removal into run_cli before the Tokio runtime is created, then pass the captured
optional token through the existing configuration/state path to the server
logic. Remove the unsafe remove_var call from the async startup block while
preserving the current empty-token filtering behavior.
In `@crates/cli/tests/coverage/shared/bootstrap_state_tests.rs`:
- Around line 53-97: Extract read_headers, accept_bounded, and header from
crates/cli/tests/coverage/shared/bootstrap_state_tests.rs#L53-L97 into a shared
test helper module, preserving their existing timeout, nonblocking polling, and
case-insensitive parsing behavior. Update
crates/cli/tests/coverage/shared/bootstrap_tests.rs#L78-L122 to reuse all three
helpers, and crates/cli/tests/coverage/shared/doctor_tests.rs#L13-L35 to reuse
accept_bounded; remove the duplicated local definitions.
- Around line 9-51: Move the shared scoped environment override implementation
into crate::test_support, retaining ENV_TEST_LOCK acquisition, variable
snapshotting, restoration on Drop, and the common set API. In
crates/cli/tests/coverage/shared/bootstrap_state_tests.rs#L9-L51,
bootstrap_tests.rs#L10-L76, completions_install_tests.rs#L107-L147, and
doctor_tests.rs#L71-L111, remove the local EnvScope declarations and use the
shared helper; in gateway_client_tests.rs#L12-L50, replace TestEnvironment with
the shared helper configured for HOME and XDG_CONFIG_HOME.
In `@crates/cli/tests/coverage/shared/bootstrap_tests.rs`:
- Around line 175-208: The test helper in
foreign_and_incompatible_listeners_are_never_adopted should not hard-code an
expected accept count based on the response body. Make the mock server accept
connections until the client’s interaction completes, using a bounded or
otherwise clean termination mechanism that cannot block server.join when acquire
changes its retry sequence. Preserve the existing response handling and error
assertions.
---
Outside diff comments:
In `@crates/cli/src/gateway/mod.rs`:
- Around line 640-668: Extract the shared header policy from
forward_upstream_request and the corresponding models path into an
apply_forwarding_headers helper that accepts a request builder, headers, and
ProviderForwarding. Keep strip_replaceable_agent_auth_headers,
should_forward_request_header, and inject_provider_auth in that helper, then
have both callers construct their request-specific body/builder and delegate
header forwarding and auth injection to it.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 4e52ad3b-dad0-46f1-86ec-f73b45f672ae
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (192)
.claude-plugin/marketplace.json.config/nextest.tomlATTRIBUTIONS-Rust.mdcodecov.ymlcrates/cli/Cargo.tomlcrates/cli/src/agents/claude/adapter.rscrates/cli/src/agents/claude/alignment.rscrates/cli/src/agents/claude/assets.rscrates/cli/src/agents/claude/doctor.rscrates/cli/src/agents/claude/host.rscrates/cli/src/agents/claude/install.rscrates/cli/src/agents/claude/launch.rscrates/cli/src/agents/claude/mod.rscrates/cli/src/agents/codex/adapter.rscrates/cli/src/agents/codex/alignment.rscrates/cli/src/agents/codex/app_server.rscrates/cli/src/agents/codex/assets.rscrates/cli/src/agents/codex/doctor.rscrates/cli/src/agents/codex/host.rscrates/cli/src/agents/codex/install.rscrates/cli/src/agents/codex/launch.rscrates/cli/src/agents/codex/mod.rscrates/cli/src/agents/hermes/adapter.rscrates/cli/src/agents/hermes/alignment.rscrates/cli/src/agents/hermes/config.rscrates/cli/src/agents/hermes/doctor.rscrates/cli/src/agents/hermes/files.rscrates/cli/src/agents/hermes/install.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/agents/hermes/mod.rscrates/cli/src/agents/hermes/trust.rscrates/cli/src/agents/mod.rscrates/cli/src/agents/shared/adapters.rscrates/cli/src/agents/shared/alignment.rscrates/cli/src/agents/shared/host.rscrates/cli/src/agents/shared/mod.rscrates/cli/src/banner.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/bootstrap/state.rscrates/cli/src/commands/completions/install.rscrates/cli/src/commands/completions/mod.rscrates/cli/src/commands/configure/mod.rscrates/cli/src/commands/configure/model.rscrates/cli/src/commands/configure/wizard.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/commands/hook_forward.rscrates/cli/src/commands/install.rscrates/cli/src/commands/mcp.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/model_pricing/mod.rscrates/cli/src/commands/plugins/mod.rscrates/cli/src/commands/plugins/subcommands.rscrates/cli/src/commands/root.rscrates/cli/src/commands/run.rscrates/cli/src/commands/serve.rscrates/cli/src/config.rscrates/cli/src/configuration/mod.rscrates/cli/src/configuration/types.rscrates/cli/src/diagnostics/environment.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/src/diagnostics/probes.rscrates/cli/src/diagnostics/render.rscrates/cli/src/events/json_path.rscrates/cli/src/events/mod.rscrates/cli/src/filesystem/atomic.rscrates/cli/src/filesystem/bounded.rscrates/cli/src/filesystem/locks.rscrates/cli/src/filesystem/mod.rscrates/cli/src/filesystem/snapshots.rscrates/cli/src/filesystem/temp.rscrates/cli/src/gateway/client.rscrates/cli/src/gateway/mod.rscrates/cli/src/gateway/request.rscrates/cli/src/gateway/response.rscrates/cli/src/gateway/routes.rscrates/cli/src/gateway/tls.rscrates/cli/src/hooks/delivery.rscrates/cli/src/hooks/destination.rscrates/cli/src/hooks/encoding.rscrates/cli/src/hooks/merging.rscrates/cli/src/hooks/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/hooks/types.rscrates/cli/src/installation/generation.rscrates/cli/src/installation/marketplace/assets.rscrates/cli/src/installation/marketplace/host.rscrates/cli/src/installation/marketplace/mod.rscrates/cli/src/installation/marketplace/setup.rscrates/cli/src/installation/marketplace/spec.rscrates/cli/src/installation/marketplace/state.rscrates/cli/src/installation/mod.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/installer.rscrates/cli/src/lib.rscrates/cli/src/main.rscrates/cli/src/mcp/gateway.rscrates/cli/src/mcp/mod.rscrates/cli/src/mcp/protocol.rscrates/cli/src/mcp/session.rscrates/cli/src/mcp/transport.rscrates/cli/src/mcp_environment.rscrates/cli/src/plugin_install/marketplace.rscrates/cli/src/plugin_install/mod.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/plugin_shim/claude.rscrates/cli/src/plugin_shim/codex.rscrates/cli/src/plugin_shim/command.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/lifecycle.rscrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/src/plugins/lifecycle/trust.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/pricing.rscrates/cli/src/plugins/schema/mod.rscrates/cli/src/plugins/schema/secrets.rscrates/cli/src/plugins/types.rscrates/cli/src/process/detached.rscrates/cli/src/process/launcher.rscrates/cli/src/process/mod.rscrates/cli/src/process/prepared.rscrates/cli/src/process/supervision.rscrates/cli/src/process/supervision/fallback.rscrates/cli/src/process/supervision/unix.rscrates/cli/src/process/supervision/windows.rscrates/cli/src/process/types.rscrates/cli/src/server.rscrates/cli/src/server/mod.rscrates/cli/src/server/types.rscrates/cli/src/sessions/correlation.rscrates/cli/src/sessions/idle.rscrates/cli/src/sessions/mod.rscrates/cli/src/sessions/routing.rscrates/cli/src/sessions/types.rscrates/cli/tests/architecture_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/agents/adapters_tests.rscrates/cli/tests/coverage/agents/alignment_claude_code_tests.rscrates/cli/tests/coverage/agents/alignment_codex_tests.rscrates/cli/tests/coverage/agents/alignment_tests.rscrates/cli/tests/coverage/agents/coding_agent_tests.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rscrates/cli/tests/coverage/agents/plugin_install_setup_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/commands/model_pricing_tests.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/tests/coverage/plugin_shim_tests.rscrates/cli/tests/coverage/shared/agent_process_tests.rscrates/cli/tests/coverage/shared/banner_tests.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/tests/coverage/shared/config_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/shared/file_io_tests.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/gateway_tls_tests.rscrates/cli/tests/coverage/shared/hook_assertions.rscrates/cli/tests/coverage/shared/install_generation_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/shared/mcp_gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/plugins_lifecycle_target_tests.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/tests/coverage/shared/setup_tests.rscrates/cli/tests/fixtures/windows_hook_relay.rsintegrations/coding-agents/claude-code/.claude-plugin/plugin.jsonintegrations/coding-agents/claude-code/.mcp.jsonintegrations/coding-agents/claude-code/hooks/hooks.jsonintegrations/coding-agents/codex/.codex-plugin/plugin.jsonintegrations/coding-agents/codex/.mcp.jsonintegrations/coding-agents/codex/hooks/hooks.jsonjustfile
💤 Files with no reviewable changes (1)
- crates/cli/src/config.rs
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cli/src/gateway/mod.rs (1)
830-880: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate header-forward/auth-injection logic in
modelsinstead of reusingforward_upstream_request.Lines 858-874 hand-roll the same "strip → forward filtered headers → inject provider auth" sequence already implemented in
forward_upstream_request(Lines 655-666). Sinceshould_forward_request_header/inject_provider_authare already generic over the request builder pattern, consider extracting a small helper that both call sites share (e.g., a function that takes aRequestBuilderand applies sanitize+forward+inject) so future changes to the security-sensitive forwarding rules don't need to be kept in sync across two copies.♻️ Sketch of a shared helper
fn apply_forwarded_headers( mut builder: reqwest::RequestBuilder, headers: &HeaderMap, route: ProviderRoute, allow_environment_provider_auth: bool, ) -> reqwest::RequestBuilder { let sanitized = strip_replaceable_agent_auth_headers(headers, route, allow_environment_provider_auth); for (name, value) in &sanitized { if should_forward_request_header(name, &sanitized) { builder = builder.header(name, value); } } inject_provider_auth(builder, route, &sanitized, allow_environment_provider_auth) }Both
forward_upstream_requestandmodelscan then call this helper.🤖 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/cli/src/gateway/mod.rs` around lines 830 - 880, Extract the shared sanitize, filtered-header forwarding, and provider-auth injection sequence into a helper near the existing forwarding utilities, using the reqwest RequestBuilder and the existing symbols strip_replaceable_agent_auth_headers, should_forward_request_header, and inject_provider_auth. Update both forward_upstream_request and models to call this helper, preserving their current route and allow_environment_provider_auth values while removing the duplicated logic from models.
🤖 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 `@ATTRIBUTIONS-Rust.md`:
- Around line 9498-9502: Update the generated attribution sections, including
the deranged block and all listed occurrences, to include blank lines around
headings and label each verbatim license fence as text. Modify the attribution
generator that produces these sections so regeneration preserves the
Markdown-lint-compliant formatting.
In `@crates/cli/Cargo.toml`:
- Around line 35-67: Replace the fs2 dependency with the maintained fs4 crate in
the dependency declaration, then update the lock-sensitive filesystem code to
use fs4’s equivalent locking API and imports while preserving existing
synchronous behavior.
In `@crates/cli/src/agents/claude/doctor.rs`:
- Around line 4-6: Extract the duplicated hook status implementation into a
shared helper in crates/cli/src/agents/shared/mod.rs, then update hook_status in
crates/cli/src/agents/claude/doctor.rs#L4-L6 and
crates/cli/src/agents/codex/doctor.rs#L4-L6 to call that helper while preserving
the existing Result<String, String> behavior and message.
In `@crates/cli/src/agents/claude/mod.rs`:
- Around line 14-43: Update parse_version to derive the parenthesized suffix
from DESCRIPTOR.version_product instead of hardcoding "Claude Code", while
preserving the existing Version parsing behavior and failure handling.
In `@crates/cli/src/agents/codex/assets.rs`:
- Around line 46-53: Update mcp_config to replace the expect on
server.as_object_mut() with an error return when server is not a JSON object,
while preserving the existing field insertion and successful Result path for
valid objects.
In `@crates/cli/src/filesystem/atomic.rs`:
- Around line 39-47: Update the unsupported-platform branch of
atomic_write_private to fail loudly instead of calling atomic_write_impl with
AtomicWritePrivacy::Standard and no permissions. Preserve the existing private
behavior on Unix and Windows, and ensure the fallback cannot silently write
without the owner-only privacy guarantee.
In `@crates/cli/src/hooks/response.rs`:
- Around line 112-124: Update guardrail_rejection_reason so a matching
error.type of "nemo_relay_guardrail_rejected" always returns Some: preserve the
reason-then-message selection when present, but provide a non-empty fallback
when both fields are absent. Keep non-matching types and invalid payloads
returning None so handle_hook_forward_status reaches the unconditional
GuardrailRejected path for every matched guardrail rejection.
In `@crates/cli/src/process/launcher.rs`:
- Around line 140-200: Preserve the primary startup or supervision error while
performing cleanup in execute_live_run_with_dynamic and supervise_prepared_run.
At each cleanup site, capture failures from prepared.restore(),
running_server.stop(), or child.terminate() and combine them with the original
wait_for_health, prepared.spawn, or gateway_result error using the established
filesystem cleanup-error pattern; do not let cleanup failures replace the
original diagnosis.
In `@crates/cli/src/process/mod.rs`:
- Around line 197-212: Extract the shared executable-resolution and
argument-slicing logic from std_command and tokio_command into a helper, then
have both functions reuse it while constructing their respective Command types.
Preserve the existing empty-argv assertion, resolve_executable fallback
behavior, and argv ordering.
In `@crates/cli/src/server/mod.rs`:
- Around line 554-560: Update the bootstrap shutdown token validation in the
handler around shutdown.token to compare the supplied token using the existing
constant-time ct_eq convention, matching the fingerprint checks near the other
authentication paths, while preserving the current FORBIDDEN response for
invalid or missing tokens.
In `@crates/cli/tests/coverage/shared/plugins_tests.rs`:
- Around line 2308-2356: Make
target_path_resolves_user_scope_from_xdg_and_reports_missing_home restore HOME,
XDG_CONFIG_HOME, and USERPROFILE during unwinding as well as normal completion.
Introduce a local Drop guard immediately after saving the previous environment
values, move the existing restoration logic into its Drop implementation, and
retain the lock until restoration has completed.
In `@crates/cli/tests/coverage/shared/setup_tests.rs`:
- Around line 42-82: Move the duplicated EnvScope implementation into
crate::test_support, retaining its serialized environment set/restore behavior
and centralized unsafe operations. In
crates/cli/tests/coverage/shared/setup_tests.rs lines 42-82, remove the local
EnvScope and adapt XdgScope as a thin wrapper around the shared helper if
needed; in crates/cli/tests/coverage/shared/bootstrap_tests.rs lines 39-76,
remove the local EnvScope copy and use the shared test_support helper.
---
Outside diff comments:
In `@crates/cli/src/gateway/mod.rs`:
- Around line 830-880: Extract the shared sanitize, filtered-header forwarding,
and provider-auth injection sequence into a helper near the existing forwarding
utilities, using the reqwest RequestBuilder and the existing symbols
strip_replaceable_agent_auth_headers, should_forward_request_header, and
inject_provider_auth. Update both forward_upstream_request and models to call
this helper, preserving their current route and allow_environment_provider_auth
values while removing the duplicated logic from models.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 4e52ad3b-dad0-46f1-86ec-f73b45f672ae
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (192)
.claude-plugin/marketplace.json.config/nextest.tomlATTRIBUTIONS-Rust.mdcodecov.ymlcrates/cli/Cargo.tomlcrates/cli/src/agents/claude/adapter.rscrates/cli/src/agents/claude/alignment.rscrates/cli/src/agents/claude/assets.rscrates/cli/src/agents/claude/doctor.rscrates/cli/src/agents/claude/host.rscrates/cli/src/agents/claude/install.rscrates/cli/src/agents/claude/launch.rscrates/cli/src/agents/claude/mod.rscrates/cli/src/agents/codex/adapter.rscrates/cli/src/agents/codex/alignment.rscrates/cli/src/agents/codex/app_server.rscrates/cli/src/agents/codex/assets.rscrates/cli/src/agents/codex/doctor.rscrates/cli/src/agents/codex/host.rscrates/cli/src/agents/codex/install.rscrates/cli/src/agents/codex/launch.rscrates/cli/src/agents/codex/mod.rscrates/cli/src/agents/hermes/adapter.rscrates/cli/src/agents/hermes/alignment.rscrates/cli/src/agents/hermes/config.rscrates/cli/src/agents/hermes/doctor.rscrates/cli/src/agents/hermes/files.rscrates/cli/src/agents/hermes/install.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/agents/hermes/mod.rscrates/cli/src/agents/hermes/trust.rscrates/cli/src/agents/mod.rscrates/cli/src/agents/shared/adapters.rscrates/cli/src/agents/shared/alignment.rscrates/cli/src/agents/shared/host.rscrates/cli/src/agents/shared/mod.rscrates/cli/src/banner.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/bootstrap/state.rscrates/cli/src/commands/completions/install.rscrates/cli/src/commands/completions/mod.rscrates/cli/src/commands/configure/mod.rscrates/cli/src/commands/configure/model.rscrates/cli/src/commands/configure/wizard.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/commands/hook_forward.rscrates/cli/src/commands/install.rscrates/cli/src/commands/mcp.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/model_pricing/mod.rscrates/cli/src/commands/plugins/mod.rscrates/cli/src/commands/plugins/subcommands.rscrates/cli/src/commands/root.rscrates/cli/src/commands/run.rscrates/cli/src/commands/serve.rscrates/cli/src/config.rscrates/cli/src/configuration/mod.rscrates/cli/src/configuration/types.rscrates/cli/src/diagnostics/environment.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/model.rscrates/cli/src/diagnostics/probes.rscrates/cli/src/diagnostics/render.rscrates/cli/src/events/json_path.rscrates/cli/src/events/mod.rscrates/cli/src/filesystem/atomic.rscrates/cli/src/filesystem/bounded.rscrates/cli/src/filesystem/locks.rscrates/cli/src/filesystem/mod.rscrates/cli/src/filesystem/snapshots.rscrates/cli/src/filesystem/temp.rscrates/cli/src/gateway/client.rscrates/cli/src/gateway/mod.rscrates/cli/src/gateway/request.rscrates/cli/src/gateway/response.rscrates/cli/src/gateway/routes.rscrates/cli/src/gateway/tls.rscrates/cli/src/hooks/delivery.rscrates/cli/src/hooks/destination.rscrates/cli/src/hooks/encoding.rscrates/cli/src/hooks/merging.rscrates/cli/src/hooks/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/hooks/types.rscrates/cli/src/installation/generation.rscrates/cli/src/installation/marketplace/assets.rscrates/cli/src/installation/marketplace/host.rscrates/cli/src/installation/marketplace/mod.rscrates/cli/src/installation/marketplace/setup.rscrates/cli/src/installation/marketplace/spec.rscrates/cli/src/installation/marketplace/state.rscrates/cli/src/installation/mod.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/installer.rscrates/cli/src/lib.rscrates/cli/src/main.rscrates/cli/src/mcp/gateway.rscrates/cli/src/mcp/mod.rscrates/cli/src/mcp/protocol.rscrates/cli/src/mcp/session.rscrates/cli/src/mcp/transport.rscrates/cli/src/mcp_environment.rscrates/cli/src/plugin_install/marketplace.rscrates/cli/src/plugin_install/mod.rscrates/cli/src/plugin_install/setup.rscrates/cli/src/plugin_shim/claude.rscrates/cli/src/plugin_shim/codex.rscrates/cli/src/plugin_shim/command.rscrates/cli/src/plugin_shim/mod.rscrates/cli/src/plugin_shim/shared.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/dynamic_editor.rscrates/cli/src/plugins/lifecycle.rscrates/cli/src/plugins/lifecycle/environment.rscrates/cli/src/plugins/lifecycle/mod.rscrates/cli/src/plugins/lifecycle/render.rscrates/cli/src/plugins/lifecycle/responses.rscrates/cli/src/plugins/lifecycle/state.rscrates/cli/src/plugins/lifecycle/target.rscrates/cli/src/plugins/lifecycle/trust.rscrates/cli/src/plugins/mod.rscrates/cli/src/plugins/pricing.rscrates/cli/src/plugins/schema/mod.rscrates/cli/src/plugins/schema/secrets.rscrates/cli/src/plugins/types.rscrates/cli/src/process/detached.rscrates/cli/src/process/launcher.rscrates/cli/src/process/mod.rscrates/cli/src/process/prepared.rscrates/cli/src/process/supervision.rscrates/cli/src/process/supervision/fallback.rscrates/cli/src/process/supervision/unix.rscrates/cli/src/process/supervision/windows.rscrates/cli/src/process/types.rscrates/cli/src/server.rscrates/cli/src/server/mod.rscrates/cli/src/server/types.rscrates/cli/src/sessions/correlation.rscrates/cli/src/sessions/idle.rscrates/cli/src/sessions/mod.rscrates/cli/src/sessions/routing.rscrates/cli/src/sessions/types.rscrates/cli/tests/architecture_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/agents/adapters_tests.rscrates/cli/tests/coverage/agents/alignment_claude_code_tests.rscrates/cli/tests/coverage/agents/alignment_codex_tests.rscrates/cli/tests/coverage/agents/alignment_tests.rscrates/cli/tests/coverage/agents/coding_agent_tests.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rscrates/cli/tests/coverage/agents/plugin_install_setup_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/commands/model_pricing_tests.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/launcher_tests.rscrates/cli/tests/coverage/plugin_install_tests.rscrates/cli/tests/coverage/plugin_shim_tests.rscrates/cli/tests/coverage/shared/agent_process_tests.rscrates/cli/tests/coverage/shared/banner_tests.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/tests/coverage/shared/config_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/shared/file_io_tests.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/gateway_tls_tests.rscrates/cli/tests/coverage/shared/hook_assertions.rscrates/cli/tests/coverage/shared/install_generation_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/shared/mcp_gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/plugins_lifecycle_target_tests.rscrates/cli/tests/coverage/shared/plugins_lifecycle_tests.rscrates/cli/tests/coverage/shared/plugins_schema_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/tests/coverage/shared/setup_tests.rscrates/cli/tests/fixtures/windows_hook_relay.rsintegrations/coding-agents/claude-code/.claude-plugin/plugin.jsonintegrations/coding-agents/claude-code/.mcp.jsonintegrations/coding-agents/claude-code/hooks/hooks.jsonintegrations/coding-agents/codex/.codex-plugin/plugin.jsonintegrations/coding-agents/codex/.mcp.jsonintegrations/coding-agents/codex/hooks/hooks.jsonjustfile
💤 Files with no reviewable changes (1)
- crates/cli/src/config.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (12)
ATTRIBUTIONS-Rust.md (1)
9498-9502: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the generated attribution blocks Markdown-lint compliant.
Each added section lacks blank lines around headings and uses an untyped license fence. Add the required spacing and use
textfor the verbatim license blocks; update the generator if this file is regenerated.As per coding guidelines, use syntax highlighting when the format supports it for code blocks.
Proposed formatting
## deranged - 0.5.8 + **Repository URL**: https://github.com/jhpratt/deranged **License Type(s)**: Apache-2.0 + ### License: https://spdx.org/licenses/Apache-2.0.html -``` +```textAlso applies to: 13337-13341, 24478-24482, 27409-27413, 28234-28238, 32319-32323, 39454-39458, 39535-39539, 47043-47047, 47252-47256, 47333-47337, 52920-52924
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 9498-9498: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 9501-9501: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above(MD022, blanks-around-headings)
[warning] 9501-9501: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 9502-9502: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 9502-9502: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@ATTRIBUTIONS-Rust.md` around lines 9498 - 9502, Update the generated attribution sections, including the deranged block and all listed occurrences, to include blank lines around headings and label each verbatim license fence as text. Modify the attribution generator that produces these sections so regeneration preserves the Markdown-lint-compliant formatting.Sources: Coding guidelines, Linters/SAST tools
crates/cli/Cargo.toml (1)
35-67: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Consider
fs4instead offs2for file locking.
fs2appears unmaintained; its maintained forkfs4is a drop-in replacement that also replaces thelibcdependency withrustixand adds async support. Since this PR is adding new lock-sensitive filesystem code (atomic writes, operation locks), pulling in a maintained fork now avoids inheriting stale/unmaintained lock-handling code.🤖 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/cli/Cargo.toml` around lines 35 - 67, Replace the fs2 dependency with the maintained fs4 crate in the dependency declaration, then update the lock-sensitive filesystem code to use fs4’s equivalent locking API and imports while preserving existing synchronous behavior.crates/cli/src/agents/claude/doctor.rs (1)
4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate
hook_status()implementation across agent doctor modules. Both files define the exact same function body; a shared helper (e.g., incrates/cli/src/agents/shared/mod.rs) would remove the copy-paste and keep future wording changes in one place.
crates/cli/src/agents/claude/doctor.rs#L4-L6: replace with a call to a sharedhook_status()helper.crates/cli/src/agents/codex/doctor.rs#L4-L6: replace with a call to the same shared helper.📍 Affects 2 files
crates/cli/src/agents/claude/doctor.rs#L4-L6(this comment)crates/cli/src/agents/codex/doctor.rs#L4-L6🤖 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/cli/src/agents/claude/doctor.rs` around lines 4 - 6, Extract the duplicated hook status implementation into a shared helper in crates/cli/src/agents/shared/mod.rs, then update hook_status in crates/cli/src/agents/claude/doctor.rs#L4-L6 and crates/cli/src/agents/codex/doctor.rs#L4-L6 to call that helper while preserving the existing Result<String, String> behavior and message.crates/cli/src/agents/claude/mod.rs (1)
14-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Suffix literal duplicates
DESCRIPTOR.version_product.
parse_versionhardcodes" (Claude Code)"separately fromDESCRIPTOR.version_product("Claude Code"). If one is updated without the other, version parsing silently fails.♻️ Derive suffix from DESCRIPTOR
pub(super) fn parse_version(raw: &str) -> Option<Version> { - Version::parse(raw.strip_suffix(" (Claude Code)")?).ok() + let suffix = format!(" ({})", DESCRIPTOR.version_product); + Version::parse(raw.strip_suffix(suffix.as_str())?).ok() }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pub(super) const DESCRIPTOR: AgentDescriptor = AgentDescriptor { argument: "claude", install_argument: "claude-code", label: "Claude Code", executable: "claude", hook_path: "/hooks/claude-code", version_product: "Claude Code", minimum_version: (2, 1, 121), hook_events: &[ "SessionStart", "UserPromptSubmit", "UserPromptExpansion", "PreToolUse", "PostToolUse", "PostToolUseFailure", "PermissionRequest", "SubagentStart", "SubagentStop", "Notification", "Stop", "PreCompact", "PostCompact", "SessionEnd", ], direct_hook_entries: false, }; pub(super) fn parse_version(raw: &str) -> Option<Version> { let suffix = format!(" ({})", DESCRIPTOR.version_product); Version::parse(raw.strip_suffix(suffix.as_str())?).ok() }🤖 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/cli/src/agents/claude/mod.rs` around lines 14 - 43, Update parse_version to derive the parenthesized suffix from DESCRIPTOR.version_product instead of hardcoding "Claude Code", while preserving the existing Version parsing behavior and failure handling.crates/cli/src/agents/codex/assets.rs (1)
46-53: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Prefer returning an error over
expectin a fallible function.
mcp_configalready returnsResult<Value, String>, but a non-objectservertriggers a panic instead of anErr. Practically the input is Relay-constructed, so this is defensive only.♻️ Convert the panic into an error
- let fields = server - .as_object_mut() - .expect("persistent MCP server is a JSON object"); + let fields = server + .as_object_mut() + .ok_or_else(|| "persistent MCP server must be a JSON object".to_string())?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pub(crate) fn mcp_config(mut server: Value) -> Result<Value, String> { let fields = server .as_object_mut() .ok_or_else(|| "persistent MCP server must be a JSON object".to_string())?; fields.insert("env_vars".into(), json!(mcp_env_vars()?)); fields.insert("required".into(), json!(true)); fields.insert("startup_timeout_sec".into(), json!(20)); Ok(json!({ (SERVER_NAME): server }))🤖 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/cli/src/agents/codex/assets.rs` around lines 46 - 53, Update mcp_config to replace the expect on server.as_object_mut() with an error return when server is not a JSON object, while preserving the existing field insertion and successful Result path for valid objects.crates/cli/src/filesystem/atomic.rs (1)
39-47: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Silent privacy downgrade on unsupported platforms.
On the
#[cfg(not(any(unix, windows)))]branch,atomic_write_privatecallsatomic_write_implwithpermissions = NoneandAtomicWritePrivacy::Standard. Sincefs::set_permissionsis only invoked whenpermissions.is_some()(Line 104), no restriction is applied at all — the function silently writes the secret with default OS permissions instead of the owner-only guarantee its own doc comment promises. Prefer failing loudly if this platform combination is ever built.🔒 Proposed fix
#[cfg(not(any(unix, windows)))] { - atomic_write_impl(path, bytes, None, AtomicWritePrivacy::Standard, None) + Err(format!( + "cannot create private file {}: unsupported platform", + path.display() + )) }🤖 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/cli/src/filesystem/atomic.rs` around lines 39 - 47, Update the unsupported-platform branch of atomic_write_private to fail loudly instead of calling atomic_write_impl with AtomicWritePrivacy::Standard and no permissions. Preserve the existing private behavior on Unix and Windows, and ensure the fallback cannot silently write without the owner-only privacy guarantee.crates/cli/src/hooks/response.rs (1)
112-124: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Guardrail rejection can silently pass through if
reason/messageare absent.
guardrail_rejection_reasonreturnsNonewhen the matched-typeerror object lacks bothreasonandmessage, even though the type discriminator confirms a guardrail block. ThatNonethen routes through thefail_closed-conditional branch inhandle_hook_forward_statusinstead of the unconditionalGuardrailRejectederror, so a minimal guardrail-rejection payload can be treated as a normal failure and silently succeed (Ok(())) whenfail_closedis false — bypassing the guardrail.🛡️ Proposed fix: always surface a matched guardrail type as a rejection
pub(super) fn guardrail_rejection_reason(body: &str) -> Option<String> { let value: Value = serde_json::from_str(body).ok()?; let error = value.get("error")?; - (error.get("type").and_then(Value::as_str) == Some("nemo_relay_guardrail_rejected")) - .then(|| { - error - .get("reason") - .and_then(Value::as_str) - .or_else(|| error.get("message").and_then(Value::as_str)) - .map(ToOwned::to_owned) - }) - .flatten() + if error.get("type").and_then(Value::as_str) != Some("nemo_relay_guardrail_rejected") { + return None; + } + Some( + error + .get("reason") + .and_then(Value::as_str) + .or_else(|| error.get("message").and_then(Value::as_str)) + .unwrap_or("guardrail rejected the request") + .to_owned(), + ) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pub(super) fn guardrail_rejection_reason(body: &str) -> Option<String> { let value: Value = serde_json::from_str(body).ok()?; let error = value.get("error")?; if error.get("type").and_then(Value::as_str) != Some("nemo_relay_guardrail_rejected") { return None; } Some( error .get("reason") .and_then(Value::as_str) .or_else(|| error.get("message").and_then(Value::as_str)) .unwrap_or("guardrail rejected the request") .to_owned(), ) }🤖 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/cli/src/hooks/response.rs` around lines 112 - 124, Update guardrail_rejection_reason so a matching error.type of "nemo_relay_guardrail_rejected" always returns Some: preserve the reason-then-message selection when present, but provide a non-empty fallback when both fields are absent. Keep non-matching types and invalid payloads returning None so handle_hook_forward_status reaches the unconditional GuardrailRejected path for every matched guardrail rejection.crates/cli/src/process/launcher.rs (1)
140-200: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cleanup errors can mask the real startup/supervision failure.
Three sites in this block discard the primary error when cleanup also fails:
- Lines 154-160:
wait_for_healthfailure is lost ifprepared.restore()orrunning_server.stop()also errors.- Lines 168-177:
prepared.spawn()failure is lost ifrestore/stopalso error.- Lines 187-198: the gateway result (including the "gateway stopped before agent exited" diagnosis) is lost if
restore/child.terminate()also error.This masks the most useful diagnostic exactly when things are already going wrong (a failure cascade).
crates/cli/src/filesystem/temp.rs(lines 21-33) already shows the better pattern in this same PR — combine the primary error with the cleanup error into one message instead of letting the later fallible step overwrite the earlier one.🛠️ Proposed fix for the health-check failure path (apply the same pattern to the other two sites)
if let Err(error) = wait_for_health(gateway_url, &bootstrap_fingerprint).await { - let restore = prepared.restore(); - let server_result = running_server.stop().await; - restore?; - server_result?; - return Err(error); + let restore_result = prepared.restore(); + let server_result = running_server.stop().await; + let mut message = error.to_string(); + if let Err(restore_error) = restore_result { + message.push_str(&format!("; additionally failed to restore state: {restore_error}")); + } + if let Err(server_error) = server_result { + message.push_str(&format!("; additionally failed to stop gateway: {server_error}")); + } + return Err(CliError::Launch(message)); }🤖 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/cli/src/process/launcher.rs` around lines 140 - 200, Preserve the primary startup or supervision error while performing cleanup in execute_live_run_with_dynamic and supervise_prepared_run. At each cleanup site, capture failures from prepared.restore(), running_server.stop(), or child.terminate() and combine them with the original wait_for_health, prepared.spawn, or gateway_result error using the established filesystem cleanup-error pattern; do not let cleanup failures replace the original diagnosis.crates/cli/src/process/mod.rs (1)
197-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Deduplicate executable resolution between
std_commandandtokio_command.Both functions repeat the same
resolve_executable(&argv[0]).unwrap_or_else(...)+args(&argv[1..])logic. Extracting a shared helper avoids the two implementations silently drifting apart later.♻️ Proposed refactor
+fn resolve_program(argv: &[String]) -> PathBuf { + debug_assert!(!argv.is_empty()); + resolve_executable(&argv[0]).unwrap_or_else(|| PathBuf::from(&argv[0])) +} + pub(crate) fn std_command(argv: &[String]) -> Command { - debug_assert!(!argv.is_empty()); - let program = resolve_executable(&argv[0]).unwrap_or_else(|| PathBuf::from(&argv[0])); - let mut command = Command::new(program); + let mut command = Command::new(resolve_program(argv)); command.args(&argv[1..]); command } pub(crate) fn tokio_command(argv: &[String]) -> tokio::process::Command { - debug_assert!(!argv.is_empty()); - let program = resolve_executable(&argv[0]).unwrap_or_else(|| PathBuf::from(&argv[0])); - let mut command = tokio::process::Command::new(program); + let mut command = tokio::process::Command::new(resolve_program(argv)); command.args(&argv[1..]); command }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn resolve_program(argv: &[String]) -> PathBuf { debug_assert!(!argv.is_empty()); resolve_executable(&argv[0]).unwrap_or_else(|| PathBuf::from(&argv[0])) } pub(crate) fn std_command(argv: &[String]) -> Command { let mut command = Command::new(resolve_program(argv)); command.args(&argv[1..]); command } /// Creates an asynchronous command with the same argv behavior as [`std_command`]. pub(crate) fn tokio_command(argv: &[String]) -> tokio::process::Command { let mut command = tokio::process::Command::new(resolve_program(argv)); command.args(&argv[1..]); command }🤖 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/cli/src/process/mod.rs` around lines 197 - 212, Extract the shared executable-resolution and argument-slicing logic from std_command and tokio_command into a helper, then have both functions reuse it while constructing their respective Command types. Preserve the existing empty-argv assertion, resolve_executable fallback behavior, and argv ordering.crates/cli/src/server/mod.rs (1)
554-560: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Compare the bootstrap shutdown token in constant time. This uses plain
!=, unlike the fingerprint checks at lines 487 and 582 which usect_eq. Match that convention to avoid a timing side channel and keep the auth paths consistent.🔒 Suggested fix
- if headers - .get("x-nemo-relay-bootstrap-token") - .and_then(|value| value.to_str().ok()) - != Some(shutdown.token.as_str()) - { - return StatusCode::FORBIDDEN; - } + let token_matches = headers + .get("x-nemo-relay-bootstrap-token") + .and_then(|value| value.to_str().ok()) + .is_some_and(|token| bool::from(token.as_bytes().ct_eq(shutdown.token.as_bytes()))); + if !token_matches { + return StatusCode::FORBIDDEN; + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let token_matches = headers .get("x-nemo-relay-bootstrap-token") .and_then(|value| value.to_str().ok()) .is_some_and(|token| bool::from(token.as_bytes().ct_eq(shutdown.token.as_bytes()))); if !token_matches { return StatusCode::FORBIDDEN; }🤖 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/cli/src/server/mod.rs` around lines 554 - 560, Update the bootstrap shutdown token validation in the handler around shutdown.token to compare the supplied token using the existing constant-time ct_eq convention, matching the fingerprint checks near the other authentication paths, while preserving the current FORBIDDEN response for invalid or missing tokens.crates/cli/tests/coverage/shared/plugins_tests.rs (1)
2308-2356: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Optional: make env restoration panic-safe.
If any assertion between L2324 and L2342 panics, the
unsafe { set_var/remove_var }restore block never runs, leavingHOME/XDG_CONFIG_HOME/USERPROFILEclobbered for the rest of the process. SinceENV_TEST_LOCKis recovered viainto_inner()on poison, subsequent tests under that lock would observe the mutated environment. Wrap the saved values in aDropguard that restores on unwind.🤖 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/cli/tests/coverage/shared/plugins_tests.rs` around lines 2308 - 2356, Make target_path_resolves_user_scope_from_xdg_and_reports_missing_home restore HOME, XDG_CONFIG_HOME, and USERPROFILE during unwinding as well as normal completion. Introduce a local Drop guard immediately after saving the previous environment values, move the existing restoration logic into its Drop implementation, and retain the lock until restoration has completed.crates/cli/tests/coverage/shared/setup_tests.rs (1)
42-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hoist the duplicated
EnvScopeintotest_support. Both modules define a near-identical serialized env set/restore guard wrappingunsafeset_var/remove_var, and both already sharecrate::test_support::ENV_TEST_LOCK. Centralizing it removes the duplicatedunsafeblocks and keeps restore semantics in one place.
crates/cli/tests/coverage/shared/setup_tests.rs#L42-L82: replace the localEnvScopewith the shared helper (foldXdgScopein as a thin wrapper if still needed).crates/cli/tests/coverage/shared/bootstrap_tests.rs#L39-L76: remove the localEnvScopecopy and use the shared helper.📍 Affects 2 files
crates/cli/tests/coverage/shared/setup_tests.rs#L42-L82(this comment)crates/cli/tests/coverage/shared/bootstrap_tests.rs#L39-L76🤖 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/cli/tests/coverage/shared/setup_tests.rs` around lines 42 - 82, Move the duplicated EnvScope implementation into crate::test_support, retaining its serialized environment set/restore behavior and centralized unsafe operations. In crates/cli/tests/coverage/shared/setup_tests.rs lines 42-82, remove the local EnvScope and adapt XdgScope as a thin wrapper around the shared helper if needed; in crates/cli/tests/coverage/shared/bootstrap_tests.rs lines 39-76, remove the local EnvScope copy and use the shared test_support helper.
Signed-off-by: Will Killian <wkillian@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cli/tests/coverage/shared/bootstrap_state_tests.rs (1)
60-96: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConsider adding a test for the fingerprint-less
stop_owned_and_resetpath.Given
valid_fornow mandates a non-emptybootstrap_fingerprint(seebootstrap/state.rs), andpublish_owner_from_envstill permits writing a record without one, this suite doesn't cover the resulting behavior: an owner record written without a fingerprint can never be validated bystop_owned_and_reset. Adding a test that asserts this behavior (or, once fixed, assertspublish_owner_from_envrejects the fingerprint-less case) would lock in the intended contract per the changed API surface.As per path instructions, "Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant."
🤖 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/cli/tests/coverage/shared/bootstrap_state_tests.rs` around lines 60 - 96, Extend the bootstrap state tests around publish_owner_from_env and stop_owned_and_reset to cover records without a bootstrap_fingerprint. Assert the intended contract: either publish_owner_from_env rejects fingerprint-less ownership records, or a successfully written fingerprint-less record cannot be validated and stopped by stop_owned_and_reset; align the test with the behavior implemented by valid_for.Source: Path instructions
🤖 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/cli/src/installation/operation_lock.rs`:
- Around line 30-38: Update the root-lock setup around directories_alias to
create or normalize install_dir before comparing it with global_lock_dir,
ensuring paths such as <global>/new/.. are recognized as aliases even when new
does not exist. Preserve the existing root lock acquisition behavior for
non-alias directories, and add a regression test covering this path form.
In `@crates/cli/tests/coverage/agents/plugin_install_tests.rs`:
- Around line 1219-1234: Strengthen
plugin_operation_lock_acquires_an_aliased_global_and_install_root_once by
explicitly verifying mutual exclusion after the first
PluginOperationLock::acquire: attempt a second acquire using the aliased paths
and assert it times out or otherwise fails while the first _lock remains held.
Keep the existing lock-path assertion, and ensure the test demonstrates that
global and install-root aliasing produces one effective lock rather than
silently acquiring twice.
---
Outside diff comments:
In `@crates/cli/tests/coverage/shared/bootstrap_state_tests.rs`:
- Around line 60-96: Extend the bootstrap state tests around
publish_owner_from_env and stop_owned_and_reset to cover records without a
bootstrap_fingerprint. Assert the intended contract: either
publish_owner_from_env rejects fingerprint-less ownership records, or a
successfully written fingerprint-less record cannot be validated and stopped by
stop_owned_and_reset; align the test with the behavior implemented by valid_for.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: af93eb75-cf56-4f4e-8653-5665be6531c7
📒 Files selected for processing (28)
crates/cli/src/agents/claude/host.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/bootstrap/state.rscrates/cli/src/commands/configure/model.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/commands/install.rscrates/cli/src/commands/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/diagnostics/render.rscrates/cli/src/gateway/client.rscrates/cli/src/hooks/encoding.rscrates/cli/src/hooks/mod.rscrates/cli/src/hooks/response.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/lib.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/shared/test_support.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/cli/src/lib.rscrates/cli/src/hooks/mod.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/hooks/response.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/install.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/src/diagnostics/render.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/hooks/encoding.rscrates/cli/src/agents/claude/host.rscrates/cli/src/gateway/client.rscrates/cli/src/bootstrap/state.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/commands/configure/model.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/src/lib.rscrates/cli/src/hooks/mod.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/hooks/response.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/install.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/src/diagnostics/render.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/hooks/encoding.rscrates/cli/src/agents/claude/host.rscrates/cli/src/gateway/client.rscrates/cli/src/bootstrap/state.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/commands/configure/model.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/cli/src/lib.rscrates/cli/src/hooks/mod.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/hooks/response.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/install.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/src/diagnostics/render.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/hooks/encoding.rscrates/cli/src/agents/claude/host.rscrates/cli/src/gateway/client.rscrates/cli/src/bootstrap/state.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/commands/configure/model.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/cli/src/lib.rscrates/cli/src/hooks/mod.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/hooks/response.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/install.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/src/diagnostics/render.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/hooks/encoding.rscrates/cli/src/agents/claude/host.rscrates/cli/src/gateway/client.rscrates/cli/src/bootstrap/state.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/commands/configure/model.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/cli/src/lib.rscrates/cli/src/hooks/mod.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/hooks/response.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/install.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/src/diagnostics/render.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/hooks/encoding.rscrates/cli/src/agents/claude/host.rscrates/cli/src/gateway/client.rscrates/cli/src/bootstrap/state.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/commands/configure/model.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/cli/src/lib.rscrates/cli/src/hooks/mod.rscrates/cli/src/commands/diagnostics.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/hooks/response.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/install.rscrates/cli/src/diagnostics/render.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/hooks/encoding.rscrates/cli/src/agents/claude/host.rscrates/cli/src/gateway/client.rscrates/cli/src/bootstrap/state.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/commands/configure/model.rscrates/cli/src/server/mod.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/cli/src/lib.rscrates/cli/src/hooks/mod.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/hooks/response.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/install.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/src/diagnostics/render.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/hooks/encoding.rscrates/cli/src/agents/claude/host.rscrates/cli/src/gateway/client.rscrates/cli/src/bootstrap/state.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/commands/configure/model.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/cli/src/lib.rscrates/cli/src/hooks/mod.rscrates/cli/src/commands/diagnostics.rscrates/cli/tests/coverage/shared/test_support.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/hooks/response.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/src/commands/mod.rscrates/cli/src/commands/install.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/src/diagnostics/render.rscrates/cli/src/bootstrap/mod.rscrates/cli/src/hooks/encoding.rscrates/cli/src/agents/claude/host.rscrates/cli/src/gateway/client.rscrates/cli/src/bootstrap/state.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/src/agents/hermes/integration.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/commands/configure/model.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/shared/test_support.rscrates/cli/tests/coverage/shared/gateway_client_tests.rscrates/cli/tests/coverage/shared/completions_install_tests.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/installer_tests.rscrates/cli/tests/coverage/commands/main_tests.rscrates/cli/tests/coverage/shared/bootstrap_tests.rscrates/cli/tests/coverage/agents/hermes_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/agents/plugin_host_tests.rs
🔇 Additional comments (33)
crates/cli/src/agents/claude/host.rs (1)
52-74: LGTM!Also applies to: 90-97
crates/cli/src/agents/hermes/integration.rs (1)
182-212: LGTM!crates/cli/src/hooks/encoding.rs (1)
237-237: LGTM!Also applies to: 276-276
crates/cli/src/hooks/mod.rs (1)
22-28: LGTM!crates/cli/src/hooks/response.rs (1)
50-61: LGTM!crates/cli/tests/coverage/agents/hermes_tests.rs (1)
159-159: LGTM!Also applies to: 168-168
crates/cli/tests/coverage/agents/plugin_host_tests.rs (1)
2764-2790: LGTM!crates/cli/src/bootstrap/mod.rs (1)
282-282: LGTM!crates/cli/src/gateway/client.rs (1)
6-25: LGTM!Also applies to: 107-112, 178-178, 197-197, 214-230, 292-292, 328-332, 349-380, 410-436
crates/cli/tests/coverage/shared/completions_install_tests.rs (1)
8-8: LGTM!crates/cli/tests/coverage/shared/test_support.rs (1)
1-153: LGTM!crates/cli/tests/coverage/commands/main_tests.rs (1)
16-28: LGTM!Also applies to: 115-138
crates/cli/src/bootstrap/state.rs (3)
250-257: 🩺 Stability & Availability | 💤 Low value
.expect()here is only safe because of thevalid_forgate above it.This is fine given the current call ordering (
valid_foralready returned early on a missing fingerprint), but it's a latent panic risk if that ordering orvalid_for's invariant ever changes. Worth a short comment tying the.expect()to the precondition enforced above, so future refactors don't silently break the invariant.
68-342: LGTM!
56-65: 🩺 Stability & AvailabilityNo issue: managed owner records already require a fingerprint The only managed bootstrap path that writes
NEMO_RELAY_BOOTSTRAP_STATE_DIRandNEMO_RELAY_BOOTSTRAP_SHUTDOWN_TOKENalso setsNEMO_RELAY_BOOTSTRAP_FINGERPRINT, andmanaged_bootstrap_identityrejects empty fingerprints when a ready file is used.publish_owner_from_envdoes not create a fingerprint-less managed record here.> Likely an incorrect or invalid review comment.crates/cli/src/server/mod.rs (3)
71-231:BootstrapServeOptionsrefactor resolves the prior env-var race concern.Threading
fingerprint/identity/ready_file/shutdown_tokenthrough this struct instead of the previous inlinestd::env::remove_var(...)removes the unsafe process-env mutation race flagged in an earlier review round. LGTM.
292-355: LGTM!
245-291: 🩺 Stability & AvailabilityDrop this concern:
bootstrap/mod.rssetsNEMO_RELAY_BOOTSTRAP_FINGERPRINTfrom the samespec.bootstrap_fingerprintthat feedsbootstrap_fingerprint, sopublish_owner_from_envsees the same value and the owner record does not become fingerprint-less on this path.> Likely an incorrect or invalid review comment.crates/cli/tests/coverage/shared/bootstrap_state_tests.rs (1)
10-59: LGTM!Also applies to: 98-218
crates/cli/tests/coverage/shared/bootstrap_tests.rs (1)
86-134: Nonblocking accept-loop with stop channel resolves the prior hard-coded connection-count concern.The loop now accepts until explicitly signalled to stop, rather than assuming a fixed number of
acquire()retries — addresses the previously flagged brittleness. LGTM.crates/cli/tests/coverage/shared/doctor_tests.rs (1)
316-335: LGTM!Also applies to: 671-689
crates/cli/tests/coverage/shared/gateway_client_tests.rs (1)
46-238: LGTM!crates/cli/tests/coverage/shared/installer_tests.rs (1)
194-216: LGTM!crates/cli/src/commands/configure/model.rs (1)
73-84: LGTM!crates/cli/src/commands/diagnostics.rs (2)
15-24: LGTM!
32-79: LGTM!crates/cli/src/commands/mod.rs (2)
93-132: LGTM!
55-61: 🎯 Functional Correctness
bootstrap_shutdown_tokenis only used for daemon startup. The explicit subcommand paths inrun_command()go throughprocess::launcher::run(...), and there is noservesubcommand here to forward it to.> Likely an incorrect or invalid review comment.crates/cli/src/commands/install.rs (1)
85-145: LGTM!crates/cli/src/diagnostics/mod.rs (2)
295-378: LGTM!Also applies to: 401-418
1100-1109: LGTM!crates/cli/src/diagnostics/render.rs (1)
270-273: LGTM!crates/cli/src/lib.rs (1)
39-64: LGTM!
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/cli/src/gateway/mod.rs (2)
862-927: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDispatch-route override contract is incomplete and fails open instead of closed.
from_dispatch_overridedoesn't cover allProviderRoutevariants, andeffective_dispatch_requestsilently falls back to the original route on any parse failure while still applying an independently-suppliedoverride_url— the combination can forward a request to a new upstream URL under the wrong provider's auth-header semantics.
crates/cli/src/gateway/mod.rs#L862-L927: whenINTERNAL_DISPATCH_ROUTE_HEADERis present but fails to parse viafrom_dispatch_override, dropoverride_urltoo (fail closed) instead of pairing the new URL with the stale route.crates/cli/src/gateway/routes.rs#L49-L63: addfrom_dispatch_overridematch arms forOpenAiModelsandAnthropicCountTokens(or document why they're intentionally excluded from override targeting).🤖 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/cli/src/gateway/mod.rs` around lines 862 - 927, Update effective_dispatch_request in crates/cli/src/gateway/mod.rs (lines 862-927) to fail closed when INTERNAL_DISPATCH_ROUTE_HEADER is present but from_dispatch_override cannot parse it: discard override_url rather than applying it with the original route. Update from_dispatch_override in crates/cli/src/gateway/routes.rs (lines 49-63) to handle OpenAiModels and AnthropicCountTokens, unless those variants are explicitly documented as unsupported override targets.
752-776: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
GatewayCallGuard::dropcan skip cleanup without a Tokio handle. If this destructor runs outside a Tokio runtime, bothrecord_gateway_response_hintsandfinish_gateway_callare skipped, leavingactive_gateway_callsstale and blocking idle shutdown.🤖 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/cli/src/gateway/mod.rs` around lines 752 - 776, Update GatewayCallGuard::drop to always execute the response-hint recording and finish_gateway_call cleanup, including when Handle::try_current() finds no Tokio runtime. Preserve the existing async cleanup sequence and ensure it is run using an appropriate runtime fallback rather than silently skipping cleanup.
🤖 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 `@ATTRIBUTIONS-Rust.md`:
- Around line 38435-38439: Update the switchyard-translation attribution entry
by adding blank lines around its headings and changing the license code fence to
include an explicit text language, while preserving the existing license
content.
In `@crates/cli/src/gateway/mod.rs`:
- Around line 81-205: Remove the commented-out pre-refactor code blocks
containing PreparedGatewayRequest and the former gateway route/response helpers,
including the block around prepare_gateway_request and its related functions and
the additional block noted in the review. Keep the active implementations in
gateway::request, gateway::routes, and gateway::response unchanged.
In `@crates/cli/src/gateway/routes.rs`:
- Around line 49-63: The from_dispatch_override method does not support the
OpenAiModels and AnthropicCountTokens ProviderRoute variants. Add the
appropriate dispatch-override aliases for both variants so valid overrides
resolve to their corresponding route classifications, and ensure
effective_dispatch_request does not silently apply an unparseable override to a
different route.
In `@crates/cli/src/server/mod.rs`:
- Around line 740-776: Extract the shared adaptive, PII, and feature-gated
switchyard registration plus switchyard ATOF validation into a
register_and_validate_plugin_components(&PluginConfig) helper. In
crates/cli/src/server/mod.rs lines 740-776, call it from
initialize_plugin_host’s static path; in crates/cli/src/server/mod.rs lines
785-917, replace PluginActivation::initialize’s inline registration and
validation; and in crates/cli/src/diagnostics/mod.rs lines 482-515, replace
collect_observability’s duplicated block with the helper.
In `@crates/cli/tests/coverage/shared/plugins_tests.rs`:
- Around line 2417-2430: Extend
tagged_unions_support_list_items_and_top_level_fields to construct a list schema
field whose list_item is Some(&TAGGED_LIST_ITEM), then invoke the relevant
editor item-label or variant-handling path through that field and assert the
expected tagged-union behavior. Retain the existing top-level field assertions
while ensuring the test no longer passes TAGGED_LIST_ITEM only as a direct
helper argument.
---
Outside diff comments:
In `@crates/cli/src/gateway/mod.rs`:
- Around line 862-927: Update effective_dispatch_request in
crates/cli/src/gateway/mod.rs (lines 862-927) to fail closed when
INTERNAL_DISPATCH_ROUTE_HEADER is present but from_dispatch_override cannot
parse it: discard override_url rather than applying it with the original route.
Update from_dispatch_override in crates/cli/src/gateway/routes.rs (lines 49-63)
to handle OpenAiModels and AnthropicCountTokens, unless those variants are
explicitly documented as unsupported override targets.
- Around line 752-776: Update GatewayCallGuard::drop to always execute the
response-hint recording and finish_gateway_call cleanup, including when
Handle::try_current() finds no Tokio runtime. Preserve the existing async
cleanup sequence and ensure it is run using an appropriate runtime fallback
rather than silently skipping cleanup.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 07434d67-647c-4734-8c4b-acf4ab15d595
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
ATTRIBUTIONS-Rust.mdcrates/cli/Cargo.tomlcrates/cli/src/agents/hermes/config.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/gateway/mod.rscrates/cli/src/gateway/routes.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/mcp/protocol.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/mod.rscrates/cli/src/server/mod.rscrates/cli/src/sessions/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rsscripts/README.md
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Node.js / Test (windows-arm64)
- GitHub Check: Python / Test (windows-arm64)
- GitHub Check: Rust / Test (macos-arm64)
- GitHub Check: Python / Test (macos-arm64)
- GitHub Check: Rust / Test (windows-arm64)
- GitHub Check: Rust / Test (windows-amd64)
🧰 Additional context used
📓 Path-based instructions (19)
**/*.{md,rst,html,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
**/*.{md,rst,html,txt}: Always spellNVIDIAin all caps. Do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun because the name starts with an 'en' sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company.
Use trademark symbols with product names only when the document type or legal guidance requires them.
Verify official capitalization, spacing, and hyphenation for product names.
Precede NVIDIA product names withNVIDIAon first mention when it is natural and accurate.
Do not rewrite product names for grammar or title-case rules.
Preserve third-party product names according to the owner's spelling.
Include the company name and full model qualifier on first use when it helps identify the model.
Preserve the official capitalization and punctuation of model names.
Use shorter family names only after the full name is established.
Spell out a term on first use and put the acronym in parentheses unless the acronym is widely understood by the intended audience.
Use the acronym on later mentions after it has been defined.
For long documents, reintroduce the full term if readers might lose context.
Form plurals of acronyms withs, not an apostrophe, such asGPUs.
In headings, common acronyms can remain abbreviated. Spell out the term in the first or second sentence of the body.
Common terms such asCPU,GPU,PC,API, andUIusually do not need to be spelled out for developer audiences.
Files:
scripts/README.mdATTRIBUTIONS-Rust.md
**/*.{md,rst,html}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Link the first mention of a product name when the destination helps the reader.
Files:
scripts/README.mdATTRIBUTIONS-Rust.md
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Spell
NVIDIAin all caps. Do not useNvidia,nvidia, orNV.
Files:
scripts/README.mdATTRIBUTIONS-Rust.md
**/*.{md,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
**/*.{md,rst}: Format commands, code elements, expressions, package names, file names, and paths as inline code.
Use descriptive link text. Avoid raw URLs and weak anchors such as "here" or "read more."
Use title case consistently for technical documentation headings.
Introduce code blocks, lists, tables, and images with complete sentences.
Write procedures as imperative steps. Keep steps parallel and split long procedures into smaller tasks.
Prefer active voice, present tense, short sentences, contractions, and plain English.
Usecanfor possibility and reservemayfor permission.
Useafterfor temporal relationships instead ofonce.
Preferrefer tooverseewhen the wording points readers to another resource.
Avoid culture-specific idioms, unnecessary Latinisms, jokes, and marketing exaggeration in technical docs.
Spell out months in body text, avoid ordinal dates, and use clear time zones.
Spell out whole numbers from zero through nine unless they are technical values, parameters, versions, or UI values.
Use numerals for 10 or greater and include commas in thousands.
Do not add trademark symbols to learning-oriented docs unless the source, platform, or legal guidance explicitly requires them.
Files:
scripts/README.mdATTRIBUTIONS-Rust.md
**/*.md
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-technical-docs.md)
**/*.md: Use title case consistently in technical documentation headings
Avoid quotation marks, ampersands, and exclamation marks in headings
Keep product, event, research, and whitepaper names in their official title case
Use title case for table headers
Do not force social-media sentence case into technical docs
Format code elements, commands, parameters, package names, and expressions in monospace
Format directories, file names, and paths in monospace using backticks
Use angle brackets inside monospace for variables inside paths, such as/home/<username>/.login
Format error messages and strings in quotation marks, keeping literal code strings in code formatting when clearer
Format UI buttons, menus, fields, and labels in bold
Use angle brackets between UI labels for menu paths, such as File > Save As
Use italics for new terms on first use, sparingly and only when introducing the term
Use italics for publication titles
Format keyboard shortcuts in plain text, such as Press Ctrl+Alt+Delete
Use owner/repo link text for GitHub repositories, preferring[NVIDIA/NeMo](link)over prose references like 'the GitHub repo'
Introduce every code block with a complete sentence
Do not make a code block complete the grammar of the previous sentence
Do not continue a sentence after a code block
Use syntax highlighting when the format supports it for code blocks
Avoid the word 'snippet' unless the surrounding docs already use it as a term of art
Keep inline method, function, and class references consistent with nearby docs, omitting empty parentheses for prose readability when no call is shown
Use descriptive anchor text that matches the destination title when possible for links
Avoid raw URLs in running text
Avoid generic anchor text such as 'here,' 'this page,' and 'read more'
Include acronyms in link text when a linked term includes an acronym
Do not link long sentences or multiple sentences
Avoid links that pull readers away from a procedure unless the link is a p...
Files:
scripts/README.mdATTRIBUTIONS-Rust.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Update
README.md,fern/, package READMEs, and binding-support notes when public behavior, package names, examples, or supported bindings change.
**/*.{md,mdx}: Prefer the documented public API, not internal shortcuts
Keep package names, repo references, and build commands current
Keep release-process and release-notes guidance in repo-maintainer docs such asRELEASING.md, not as user-facing docs pages orCHANGELOG.md
Keep stable user-facing wrappers atscripts/root in docs and examples; only point at namespaced helper paths when documenting internal maintenance work
When detailed dynamic plugin guides exist, keep Rust native plugin examples, Python worker plugin examples, andgrpc-v1protocol details on separate pagesIf links in documentation change, run
just docs-linkcheck.
Files:
scripts/README.mdATTRIBUTIONS-Rust.md
**/*.{md,markdown,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Markdown/MDX documentation files using the HTML comment block form.
Files:
scripts/README.mdATTRIBUTIONS-Rust.md
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
scripts/README.mdcrates/cli/Cargo.tomlATTRIBUTIONS-Rust.mdcrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/mcp/protocol.rscrates/cli/src/plugins/config_io.rscrates/cli/src/gateway/routes.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/agents/hermes/config.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/plugins/mod.rscrates/cli/tests/cli_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/sessions/mod.rs
{.github/**,.gitlab-ci.yml,.pre-commit-config.yaml,justfile,scripts/**}
⚙️ CodeRabbit configuration file
{.github/**,.gitlab-ci.yml,.pre-commit-config.yaml,justfile,scripts/**}: Review automation changes for reproducibility, pinned versions where appropriate, secret handling, and consistency with the documented validation matrix.
Pay attention to commands that need generated native artifacts, FFI libraries, or platform-specific environment variables.
Files:
scripts/README.md
**/Cargo.toml
📄 CodeRabbit inference engine (.agents/skills/prepare-code-freeze/SKILL.md)
Confirm or infer the target release version from
upstream/main:Cargo.toml. Derive the release branch asrelease/<major>.<minor>.Keep Rust package names and workspace metadata in
Cargo.tomlinternally consistent across the project.
Files:
crates/cli/Cargo.toml
**/*.toml
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all TOML files using the
#comment form.
Files:
crates/cli/Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/mcp/protocol.rscrates/cli/src/plugins/config_io.rscrates/cli/src/gateway/routes.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/agents/hermes/config.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/plugins/mod.rscrates/cli/tests/cli_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/sessions/mod.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/mcp/protocol.rscrates/cli/src/plugins/config_io.rscrates/cli/src/gateway/routes.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/agents/hermes/config.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/plugins/mod.rscrates/cli/tests/cli_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/sessions/mod.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/mcp/protocol.rscrates/cli/src/plugins/config_io.rscrates/cli/src/gateway/routes.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/agents/hermes/config.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/plugins/mod.rscrates/cli/tests/cli_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/sessions/mod.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/mcp/protocol.rscrates/cli/src/plugins/config_io.rscrates/cli/src/gateway/routes.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/agents/hermes/config.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/plugins/mod.rscrates/cli/tests/cli_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/sessions/mod.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/mcp/protocol.rscrates/cli/src/plugins/config_io.rscrates/cli/src/gateway/routes.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/agents/hermes/config.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/plugins/mod.rscrates/cli/tests/cli_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/sessions/mod.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/mcp/protocol.rscrates/cli/src/plugins/config_io.rscrates/cli/src/gateway/routes.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/agents/hermes/config.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/server/mod.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/src/plugins/mod.rscrates/cli/tests/cli_tests.rscrates/cli/src/gateway/mod.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/src/sessions/mod.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rs
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/cli/src/installation/operation_lock.rscrates/cli/src/mcp/protocol.rscrates/cli/src/plugins/config_io.rscrates/cli/src/gateway/routes.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/agents/hermes/config.rscrates/cli/src/server/mod.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/plugins/mod.rscrates/cli/src/gateway/mod.rscrates/cli/src/sessions/mod.rs
🪛 markdownlint-cli2 (0.23.0)
ATTRIBUTIONS-Rust.md
[warning] 38435-38435: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 38438-38438: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 38438-38438: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 38439-38439: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 38439-38439: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (25)
crates/cli/Cargo.toml (1)
37-82: 📐 Maintainability & Code QualityNo dependency-boundary issues here.
libcandwindows-sysare target-gated, andtokio’stest-util,shell-words, andsynstay in test-only usage.scripts/README.md (1)
21-28: 📐 Maintainability & Code QualityNo Change Needed For These Opt-In E2E Recipes. The
justfileentries exist, stay out of the default test flow and CI, and call out the required local client.crates/cli/src/mcp/protocol.rs (1)
8-9: LGTM! Version negotiation correctly supports both2025-11-25and2025-06-18, resolving the prior review thread's request.Also applies to: 53-58
crates/cli/src/agents/hermes/launch.rs (2)
23-27: LGTM!OPENAI_BASE_URLderivation and overlay creation wiring look correct.Also applies to: 41-41
15-22: 🩺 Stability & AvailabilityNo issue: Hermes launch already receives
GATEWAY_URL_ENV.process/launcher.rsinjectscrate::configuration::GATEWAY_URL_ENVbeforecrate::agents::prepare_launch, andagents/mod.rsonly routes Hermes through that launch path.> Likely an incorrect or invalid review comment.crates/cli/src/gateway/mod.rs (1)
393-443: LGTM! Retry-aware upstream failure classification (transport_failure/http_failure),ProviderForwardingthreading throughforward_upstream_request, and the newFlowError::Upstream→CliError::ProviderFailuremapping look correct and consistently applied across the buffered and streaming paths, with sensitive headers properly filtered infailure_headers.Also applies to: 553-616, 800-836, 1067-1159
crates/cli/tests/coverage/agents/plugin_install_tests.rs (1)
1220-1242: LGTM! Matches the mutual-exclusion fix already confirmed resolved in the prior review thread (commit 4d6b464).crates/cli/tests/coverage/shared/bootstrap_state_tests.rs (1)
210-215: LGTM!crates/cli/src/plugins/config_io.rs (2)
12-15: LGTM!Also applies to: 716-722, 422-422
276-284: 🎯 Functional CorrectnessNo issue here.
PluginsScopeArgsandPricingScopeArgsmap every multi-flag combination toConfigurationScope::Invalid, sotarget_scopestill rejects the same inputs.> Likely an incorrect or invalid review comment.crates/cli/tests/coverage/shared/gateway_tests.rs (1)
15-15: LGTM! Solid coverage: internal dispatch-control stripping, upstream failure classification (retryability + sensitive-header scrubbing), and the retry-aware buffered-body structured-error regression are all correctly exercised.Also applies to: 340-529
crates/cli/tests/coverage/shared/doctor_tests.rs (1)
1264-1290: LGTM!crates/cli/src/plugins/mod.rs (4)
335-364: LGTM!
823-1050: LGTM! "Only write back if changed" is applied consistently, and Clear always sets an empty collection rather than removing the key, so required List/StringMap fields stay deserializable.
1052-1372: LGTM! Tagged-union Reset/Clear collapsing to a singleResetvariant matches the existing scalar-field pattern in this file, and out-of-range variant/discriminator lookups fail gracefully rather than panicking.
1374-1479: LGTM! Struct-based and raw-Value-based editors mirror the section-based dispatch faithfully, and the newprompt_valueguard cleanly rejects structured kinds instead of falling through.Also applies to: 1644-1772, 1873-1963
crates/cli/src/installation/operation_lock.rs (1)
21-109: LGTM!crates/cli/tests/coverage/shared/mcp_tests.rs (1)
16-16: LGTM!Also applies to: 142-168
crates/cli/tests/coverage/shared/session_tests.rs (2)
24-96: LGTM!
5436-5444: LGTM!crates/cli/tests/coverage/shared/server_tests.rs (1)
1976-2005: LGTM!crates/cli/src/sessions/mod.rs (1)
50-152: LGTM!Also applies to: 887-900, 1067-1081, 1493-1493, 1525-1568
crates/cli/src/diagnostics/mod.rs (1)
819-861: 🔒 Security & PrivacyNo leak into
Check.details.header_envvalues are only applied to outbound requests; theCheck.detailsstrings in this path include only the URL, transport, or static error text, not header contents.> Likely an incorrect or invalid review comment.crates/cli/src/agents/hermes/config.rs (1)
43-67: 🎯 Functional CorrectnessNo issue: this Hermes model rewrite matches the config format.
model.defaultis the expected key for the string shorthand, andprovider/base_urlare the right overrides here.> Likely an incorrect or invalid review comment.crates/cli/tests/cli_tests.rs (1)
122-168: 📐 Maintainability & Code QualityRun the required Rust validation before handoff.
crates/cli/tests/cli_tests.rs#L122-L168: Runcargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, andjust test-rust.crates/cli/tests/coverage/shared/plugins_tests.rs#L9-L11: Include this changed test module in the same validation run.As per coding guidelines, any Rust change must run
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, andjust test-rust.Source: Coding guidelines
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/cli/src/gateway/mod.rs (2)
862-927: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDispatch-route override contract is incomplete and fails open instead of closed.
from_dispatch_overridedoesn't cover allProviderRoutevariants, andeffective_dispatch_requestsilently falls back to the original route on any parse failure while still applying an independently-suppliedoverride_url— the combination can forward a request to a new upstream URL under the wrong provider's auth-header semantics.
crates/cli/src/gateway/mod.rs#L862-L927: whenINTERNAL_DISPATCH_ROUTE_HEADERis present but fails to parse viafrom_dispatch_override, dropoverride_urltoo (fail closed) instead of pairing the new URL with the stale route.crates/cli/src/gateway/routes.rs#L49-L63: addfrom_dispatch_overridematch arms forOpenAiModelsandAnthropicCountTokens(or document why they're intentionally excluded from override targeting).🤖 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/cli/src/gateway/mod.rs` around lines 862 - 927, Update effective_dispatch_request in crates/cli/src/gateway/mod.rs (lines 862-927) to fail closed when INTERNAL_DISPATCH_ROUTE_HEADER is present but from_dispatch_override cannot parse it: discard override_url rather than applying it with the original route. Update from_dispatch_override in crates/cli/src/gateway/routes.rs (lines 49-63) to handle OpenAiModels and AnthropicCountTokens, unless those variants are explicitly documented as unsupported override targets.
752-776: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
GatewayCallGuard::dropcan skip cleanup without a Tokio handle. If this destructor runs outside a Tokio runtime, bothrecord_gateway_response_hintsandfinish_gateway_callare skipped, leavingactive_gateway_callsstale and blocking idle shutdown.🤖 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/cli/src/gateway/mod.rs` around lines 752 - 776, Update GatewayCallGuard::drop to always execute the response-hint recording and finish_gateway_call cleanup, including when Handle::try_current() finds no Tokio runtime. Preserve the existing async cleanup sequence and ensure it is run using an appropriate runtime fallback rather than silently skipping cleanup.
🤖 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 `@ATTRIBUTIONS-Rust.md`:
- Around line 38435-38439: Update the switchyard-translation attribution entry
by adding blank lines around its headings and changing the license code fence to
include an explicit text language, while preserving the existing license
content.
In `@crates/cli/src/gateway/mod.rs`:
- Around line 81-205: Remove the commented-out pre-refactor code blocks
containing PreparedGatewayRequest and the former gateway route/response helpers,
including the block around prepare_gateway_request and its related functions and
the additional block noted in the review. Keep the active implementations in
gateway::request, gateway::routes, and gateway::response unchanged.
In `@crates/cli/src/gateway/routes.rs`:
- Around line 49-63: The from_dispatch_override method does not support the
OpenAiModels and AnthropicCountTokens ProviderRoute variants. Add the
appropriate dispatch-override aliases for both variants so valid overrides
resolve to their corresponding route classifications, and ensure
effective_dispatch_request does not silently apply an unparseable override to a
different route.
In `@crates/cli/src/server/mod.rs`:
- Around line 740-776: Extract the shared adaptive, PII, and feature-gated
switchyard registration plus switchyard ATOF validation into a
register_and_validate_plugin_components(&PluginConfig) helper. In
crates/cli/src/server/mod.rs lines 740-776, call it from
initialize_plugin_host’s static path; in crates/cli/src/server/mod.rs lines
785-917, replace PluginActivation::initialize’s inline registration and
validation; and in crates/cli/src/diagnostics/mod.rs lines 482-515, replace
collect_observability’s duplicated block with the helper.
In `@crates/cli/tests/coverage/shared/plugins_tests.rs`:
- Around line 2417-2430: Extend
tagged_unions_support_list_items_and_top_level_fields to construct a list schema
field whose list_item is Some(&TAGGED_LIST_ITEM), then invoke the relevant
editor item-label or variant-handling path through that field and assert the
expected tagged-union behavior. Retain the existing top-level field assertions
while ensuring the test no longer passes TAGGED_LIST_ITEM only as a direct
helper argument.
---
Outside diff comments:
In `@crates/cli/src/gateway/mod.rs`:
- Around line 862-927: Update effective_dispatch_request in
crates/cli/src/gateway/mod.rs (lines 862-927) to fail closed when
INTERNAL_DISPATCH_ROUTE_HEADER is present but from_dispatch_override cannot
parse it: discard override_url rather than applying it with the original route.
Update from_dispatch_override in crates/cli/src/gateway/routes.rs (lines 49-63)
to handle OpenAiModels and AnthropicCountTokens, unless those variants are
explicitly documented as unsupported override targets.
- Around line 752-776: Update GatewayCallGuard::drop to always execute the
response-hint recording and finish_gateway_call cleanup, including when
Handle::try_current() finds no Tokio runtime. Preserve the existing async
cleanup sequence and ensure it is run using an appropriate runtime fallback
rather than silently skipping cleanup.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 07434d67-647c-4734-8c4b-acf4ab15d595
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
ATTRIBUTIONS-Rust.mdcrates/cli/Cargo.tomlcrates/cli/src/agents/hermes/config.rscrates/cli/src/agents/hermes/launch.rscrates/cli/src/diagnostics/mod.rscrates/cli/src/gateway/mod.rscrates/cli/src/gateway/routes.rscrates/cli/src/installation/operation_lock.rscrates/cli/src/mcp/protocol.rscrates/cli/src/plugins/config_io.rscrates/cli/src/plugins/mod.rscrates/cli/src/server/mod.rscrates/cli/src/sessions/mod.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/agents/plugin_install_tests.rscrates/cli/tests/coverage/shared/bootstrap_state_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/shared/gateway_tests.rscrates/cli/tests/coverage/shared/mcp_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rsscripts/README.md
📜 Review details
🔇 Additional comments (25)
crates/cli/Cargo.toml (1)
37-82: 📐 Maintainability & Code QualityNo dependency-boundary issues here.
libcandwindows-sysare target-gated, andtokio’stest-util,shell-words, andsynstay in test-only usage.scripts/README.md (1)
21-28: 📐 Maintainability & Code QualityNo Change Needed For These Opt-In E2E Recipes. The
justfileentries exist, stay out of the default test flow and CI, and call out the required local client.crates/cli/src/mcp/protocol.rs (1)
8-9: LGTM! Version negotiation correctly supports both2025-11-25and2025-06-18, resolving the prior review thread's request.Also applies to: 53-58
crates/cli/src/agents/hermes/launch.rs (2)
23-27: LGTM!OPENAI_BASE_URLderivation and overlay creation wiring look correct.Also applies to: 41-41
15-22: 🩺 Stability & AvailabilityNo issue: Hermes launch already receives
GATEWAY_URL_ENV.process/launcher.rsinjectscrate::configuration::GATEWAY_URL_ENVbeforecrate::agents::prepare_launch, andagents/mod.rsonly routes Hermes through that launch path.> Likely an incorrect or invalid review comment.crates/cli/src/gateway/mod.rs (1)
393-443: LGTM! Retry-aware upstream failure classification (transport_failure/http_failure),ProviderForwardingthreading throughforward_upstream_request, and the newFlowError::Upstream→CliError::ProviderFailuremapping look correct and consistently applied across the buffered and streaming paths, with sensitive headers properly filtered infailure_headers.Also applies to: 553-616, 800-836, 1067-1159
crates/cli/tests/coverage/agents/plugin_install_tests.rs (1)
1220-1242: LGTM! Matches the mutual-exclusion fix already confirmed resolved in the prior review thread (commit 4d6b464).crates/cli/tests/coverage/shared/bootstrap_state_tests.rs (1)
210-215: LGTM!crates/cli/src/plugins/config_io.rs (2)
12-15: LGTM!Also applies to: 716-722, 422-422
276-284: 🎯 Functional CorrectnessNo issue here.
PluginsScopeArgsandPricingScopeArgsmap every multi-flag combination toConfigurationScope::Invalid, sotarget_scopestill rejects the same inputs.> Likely an incorrect or invalid review comment.crates/cli/tests/coverage/shared/gateway_tests.rs (1)
15-15: LGTM! Solid coverage: internal dispatch-control stripping, upstream failure classification (retryability + sensitive-header scrubbing), and the retry-aware buffered-body structured-error regression are all correctly exercised.Also applies to: 340-529
crates/cli/tests/coverage/shared/doctor_tests.rs (1)
1264-1290: LGTM!crates/cli/src/plugins/mod.rs (4)
335-364: LGTM!
823-1050: LGTM! "Only write back if changed" is applied consistently, and Clear always sets an empty collection rather than removing the key, so required List/StringMap fields stay deserializable.
1052-1372: LGTM! Tagged-union Reset/Clear collapsing to a singleResetvariant matches the existing scalar-field pattern in this file, and out-of-range variant/discriminator lookups fail gracefully rather than panicking.
1374-1479: LGTM! Struct-based and raw-Value-based editors mirror the section-based dispatch faithfully, and the newprompt_valueguard cleanly rejects structured kinds instead of falling through.Also applies to: 1644-1772, 1873-1963
crates/cli/src/installation/operation_lock.rs (1)
21-109: LGTM!crates/cli/tests/coverage/shared/mcp_tests.rs (1)
16-16: LGTM!Also applies to: 142-168
crates/cli/tests/coverage/shared/session_tests.rs (2)
24-96: LGTM!
5436-5444: LGTM!crates/cli/tests/coverage/shared/server_tests.rs (1)
1976-2005: LGTM!crates/cli/src/sessions/mod.rs (1)
50-152: LGTM!Also applies to: 887-900, 1067-1081, 1493-1493, 1525-1568
crates/cli/src/diagnostics/mod.rs (1)
819-861: 🔒 Security & PrivacyNo leak into
Check.details.header_envvalues are only applied to outbound requests; theCheck.detailsstrings in this path include only the URL, transport, or static error text, not header contents.> Likely an incorrect or invalid review comment.crates/cli/src/agents/hermes/config.rs (1)
43-67: 🎯 Functional CorrectnessNo issue: this Hermes model rewrite matches the config format.
model.defaultis the expected key for the string shorthand, andprovider/base_urlare the right overrides here.> Likely an incorrect or invalid review comment.crates/cli/tests/cli_tests.rs (1)
122-168: 📐 Maintainability & Code QualityRun the required Rust validation before handoff.
crates/cli/tests/cli_tests.rs#L122-L168: Runcargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, andjust test-rust.crates/cli/tests/coverage/shared/plugins_tests.rs#L9-L11: Include this changed test module in the same validation run.As per coding guidelines, any Rust change must run
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, andjust test-rust.Source: Coding guidelines
🛑 Comments failed to post (5)
ATTRIBUTIONS-Rust.md (1)
38435-38439: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix Markdown spacing and fence metadata.
Add blank lines around the headings and specify a language for the license fence, such as
text, to satisfy the reported Markdown lint rules.Proposed fix
## switchyard-translation - 0.1.0 + **Repository URL**: https://github.com/NVIDIA-NeMo/Switchyard **License Type(s)**: Apache-2.0 + ### License: https://spdx.org/licenses/Apache-2.0.html -``` +```text🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 38435-38435: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 38438-38438: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above(MD022, blanks-around-headings)
[warning] 38438-38438: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
[warning] 38439-38439: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 38439-38439: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@ATTRIBUTIONS-Rust.md` around lines 38435 - 38439, Update the switchyard-translation attribution entry by adding blank lines around its headings and changing the license code fence to include an explicit text language, while preserving the existing license content.Source: Linters/SAST tools
crates/cli/src/gateway/mod.rs (1)
81-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the large "pre-refactor" commented-out code blocks.
Two multi-hundred-line dead code blocks (former
PreparedGatewayRequest/route/response helpers now moved togateway::request/gateway::routes/gateway::response) are left commented out in this diff. They add significant noise and risk drifting out of sync with the real implementations.Also applies to: 1215-1497
🤖 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/cli/src/gateway/mod.rs` around lines 81 - 205, Remove the commented-out pre-refactor code blocks containing PreparedGatewayRequest and the former gateway route/response helpers, including the block around prepare_gateway_request and its related functions and the additional block noted in the review. Keep the active implementations in gateway::request, gateway::routes, and gateway::response unchanged.crates/cli/src/gateway/routes.rs (1)
49-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
from_dispatch_overridedoesn't cover allProviderRoutevariants.
OpenAiModelsandAnthropicCountTokenshave no override aliases, so any dispatch-override attempt targeting them returnsNone. Combined with the silent fallback ineffective_dispatch_request(gateway/mod.rs), an unparseable/incomplete route override can leaveoverride_urlapplied against the wrong route classification. See the linked consolidated comment for the full-picture fix.🤖 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/cli/src/gateway/routes.rs` around lines 49 - 63, The from_dispatch_override method does not support the OpenAiModels and AnthropicCountTokens ProviderRoute variants. Add the appropriate dispatch-override aliases for both variants so valid overrides resolve to their corresponding route classifications, and ensure effective_dispatch_request does not silently apply an unparseable override to a different route.crates/cli/src/server/mod.rs (1)
740-776: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Plugin component registration/validation is duplicated across three sites. The adaptive/PII/switchyard registration and switchyard ATOF validation sequence is copy-pasted rather than shared, creating drift risk if a future component is added to only one path.
crates/cli/src/server/mod.rs#L740-L776: extract a sharedregister_and_validate_plugin_components(&PluginConfig)helper and call it from the static fast path here.crates/cli/src/server/mod.rs#L785-L917: replace the inline registration (798-807) and validation (904-907) calls inPluginActivation::initializewith the same shared helper.crates/cli/src/diagnostics/mod.rs#L482-L515: replace the inline registration/validation block incollect_observabilitywith the same shared helper.📍 Affects 2 files
crates/cli/src/server/mod.rs#L740-L776(this comment)crates/cli/src/server/mod.rs#L785-L917crates/cli/src/diagnostics/mod.rs#L482-L515🤖 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/cli/src/server/mod.rs` around lines 740 - 776, Extract the shared adaptive, PII, and feature-gated switchyard registration plus switchyard ATOF validation into a register_and_validate_plugin_components(&PluginConfig) helper. In crates/cli/src/server/mod.rs lines 740-776, call it from initialize_plugin_host’s static path; in crates/cli/src/server/mod.rs lines 785-917, replace PluginActivation::initialize’s inline registration and validation; and in crates/cli/src/diagnostics/mod.rs lines 482-515, replace collect_observability’s duplicated block with the helper.crates/cli/tests/coverage/shared/plugins_tests.rs (1)
2417-2430: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the list-field metadata path.
This directly passes
TAGGED_LIST_ITEMtoeditor_item_label, but noListschema field references it. Add coverage that constructs a list field withlist_item: Some(&TAGGED_LIST_ITEM)and verifies rendering or variant handling through that schema path.As per path instructions, tests should cover the behavior promised by the changed API surface.
🤖 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/cli/tests/coverage/shared/plugins_tests.rs` around lines 2417 - 2430, Extend tagged_unions_support_list_items_and_top_level_fields to construct a list schema field whose list_item is Some(&TAGGED_LIST_ITEM), then invoke the relevant editor item-label or variant-handling path through that field and assert the expected tagged-union behavior. Retain the existing top-level field assertions while ensuring the test no longer passes TAGGED_LIST_ITEM only as a direct helper argument.Source: Path instructions
Salonijain27
left a comment
Salonijain27
left a comment
There was a problem hiding this comment.
Approved from a dependency point of view
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com> # Conflicts: # crates/cli/src/launcher.rs # crates/cli/tests/coverage/launcher_tests.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cli/src/process/launcher.rs`:
- Around line 596-618: Update the AtofSinkSectionConfig::Stream branch in
observability_exporter_destinations to pass stream.url through the existing
sanitized_url helper before adding it to destinations, matching the credential
and query-parameter redaction used by other exporter destinations while
preserving the “ATOF ” label.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 746a7889-4df6-40df-9bb5-96978f0e4285
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/cli/src/diagnostics/mod.rscrates/cli/src/process/launcher.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/core/tests/unit/plugin_tests.rsjustfile
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (16)
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
**/*.rs: Any Rust change must runjust test-rust
Any Rust change must runcargo fmt --all
Any Rust change must runcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allfor all FFI work since it is Rust work
Runjust test-rustto validate FFI changes
Runcargo clippy --workspace --all-targets -- -D warningsto enforce strict linting on FFI workWhen Rust files changed as part of Go work, also run
cargo fmt --all,just test-rust, andcargo clippy --workspace --all-targets -- -D warnings
**/*.rs: Runcargo fmt --allwhen Rust files are changed as part of Node work
Runcargo clippy --workspace --all-targets -- -D warningswhen Rust files are changed as part of Node work
Runjust test-rustwhen Rust files are changed as part of Node workWhen changing the core Rust runtime or Rust-facing API surface, format Rust code with
cargo fmt(rustfmt defaults), keepcargo clippy -- -D warningsclean, and satisfycargo deny checkperdeny.toml.
**/*.rs: If any Rust code changed, always runjust test-rust.
If any Rust code changed, also runcargo fmt --all.
If any Rust code changed, also runcargo clippy --workspace --all-targets -- -D warnings.
For Rust changes headed for review, runcargo fmt --allandcargo clippy --workspace --all-targets -- -D warningseven if relying on pre-commit.
Files:
crates/core/tests/unit/plugin_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rs
{crates/core,crates/adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes to
crates/coreorcrates/adaptivemust run the full language matrix
Files:
crates/core/tests/unit/plugin_tests.rs
crates/core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/test-go-binding/SKILL.md)
If the change touched
crates/coreor shared runtime semantics, also usevalidate-changefor broader validation
Files:
crates/core/tests/unit/plugin_tests.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/tests/unit/plugin_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rs
**/*.{rs,py,js,mjs,cjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,py,js,mjs,cjs,ts,tsx}: UseJson = serde_json::Valuein Rust-facing runtime APIs where the existing code expects JSON payloads.
UseResult<T>withFlowErrorin core runtime paths, and keep errors explicit and binding-appropriate at the wrapper layer.
Keep async behavior on the existing tokio-based model; bindings should preserve callback and future lifetimes rather than blocking or hiding async work unexpectedly.
Files:
crates/core/tests/unit/plugin_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rs
**/*.{rs,py,go,js,ts,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use language-appropriate naming conventions: Rust
snake_case, C FFI exports prefixednemo_relay_, GoPascalCase, Node.jscamelCase, and Pythonsnake_case.
Files:
crates/core/tests/unit/plugin_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rs
**/*.{rs,go,js,ts}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add the SPDX license header to all Rust, Go, JavaScript, and TypeScript source files using the corresponding
//comment form.
Files:
crates/core/tests/unit/plugin_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rs
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
Files:
crates/core/tests/unit/plugin_tests.rsjustfilecrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rs
crates/{core,adaptive}/**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If
crates/coreorcrates/adaptivechanged, run the full validation matrix across Rust, Python, Go, and Node.js.
Files:
crates/core/tests/unit/plugin_tests.rs
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
Files:
crates/core/tests/unit/plugin_tests.rscrates/cli/src/process/launcher.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/src/diagnostics/mod.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rs
crates/{core,adaptive}/**/*.rs
⚙️ CodeRabbit configuration file
crates/{core,adaptive}/**/*.rs: Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions.
Pay close attention to task-local/thread-local scope propagation, callback lifetimes, stream finalization, and root_uuid isolation.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
Files:
crates/core/tests/unit/plugin_tests.rs
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
crates/core/tests/unit/plugin_tests.rscrates/cli/tests/coverage/shared/session_tests.rscrates/cli/tests/coverage/shared/doctor_tests.rscrates/cli/tests/cli_tests.rscrates/cli/tests/coverage/shared/server_tests.rscrates/cli/tests/coverage/agents/launcher_tests.rscrates/cli/tests/coverage/shared/plugins_tests.rs
{justfile,codecov.yml,codecov.yaml,.github/workflows/**/*.yml,.github/workflows/**/*.yaml}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
justfile, Codecov, and CI package/test workflows must include new plugin crates and packages.
Files:
justfile
justfile
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Keep
justfilebuild, test, clean, version, and package recipes for plugin crates and packages aligned with the current packaging layout.
Files:
justfile
{.github/**,.gitlab-ci.yml,.pre-commit-config.yaml,justfile,scripts/**}
⚙️ CodeRabbit configuration file
{.github/**,.gitlab-ci.yml,.pre-commit-config.yaml,justfile,scripts/**}: Review automation changes for reproducibility, pinned versions where appropriate, secret handling, and consistency with the documented validation matrix.
Pay attention to commands that need generated native artifacts, FFI libraries, or platform-specific environment variables.
Files:
justfile
{crates/**/src/**/*.rs,python/**/*.py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Do not add tests under
src; Rust tests belong in cratetests/trees, and Python SDK tests belong underpython/tests.
Files:
crates/cli/src/process/launcher.rscrates/cli/src/diagnostics/mod.rs
🔇 Additional comments (11)
crates/cli/tests/cli_tests.rs (1)
3000-3009: LGTM!crates/cli/tests/coverage/agents/launcher_tests.rs (1)
467-480: 🗄️ Data Integrity & IntegrationConfirm the ATOF file-sink mode is intentionally omitted.
This v2 fixture’s file sink lacks
mode, whilecrates/cli/tests/cli_tests.rsexplicitly setsmode = "append". Verify that omission is valid and equivalent; otherwise this test may fail validation or exercise different behavior than intended. Addmode = "append"if it is required.justfile (3)
1059-1070: LGTM!
1126-1127: 🩺 Stability & AvailabilityVerify that CI preserves FFI test serialization.
The local path now runs
nemo-relay-ffiwith--test-threads=1, but CI still runs the entire workspace throughcargo nextest run --workspaceat Line 1117. If this split prevents shared-state or cleanup races, confirm that.config/nextest.tomlgives the FFI tests equivalent serialization; otherwise CI can reintroduce the failure this change is intended to prevent.As per path instructions, review automation changes for reproducibility, pinned versions where appropriate, secret handling, and consistency with the documented validation matrix.
Source: Path instructions
1140-1142: 🩺 Stability & AvailabilityReview native Windows config isolation in the Python and Node test recipes.
XDG_CONFIG_HOMEalone may not isolate Windows config directories; align these recipes withtest-rustby settingAPPDATAandLOCALAPPDATAif they run on Windows. Same applies to the later recipe.crates/cli/src/diagnostics/mod.rs (1)
552-612: LGTM!Also applies to: 773-990, 944-961, 1098-1166
crates/cli/tests/coverage/shared/doctor_tests.rs (1)
788-815: LGTM!Also applies to: 944-959, 1120-1168, 1206-1238
crates/cli/tests/coverage/shared/plugins_tests.rs (1)
176-208: LGTM!Also applies to: 647-667, 763-790, 812-878, 1508-1536, 2015-2044, 2314-2327, 2606-2622
crates/cli/tests/coverage/shared/server_tests.rs (1)
842-933: LGTM!Also applies to: 953-1039, 1042-1242, 1245-1391, 1394-1510, 1513-1805, 1808-1943, 2110-2142
crates/cli/tests/coverage/shared/session_tests.rs (1)
98-120: LGTM!Also applies to: 3163-3251, 3299-3413, 3634-3697
crates/core/tests/unit/plugin_tests.rs (1)
1143-1169: LGTM! Good addition — waiting onPLUGIN_MUTATION_OWNERreturning toIdlecloses a race between diagnostic visibility and lease release. As per path instructions, "Review the Rust runtime for async correctness, scope isolation, middleware ordering, and event lifecycle regressions."Source: Path instructions
Signed-off-by: Will Killian <wkillian@nvidia.com>
Signed-off-by: Will Killian <wkillian@nvidia.com>
Salonijain27
left a comment
Salonijain27
left a comment
There was a problem hiding this comment.
Approved from a dependency point of view
5 participants
Warning
BREAKING CHANGE: [Plugin configuration and installation] Existing
Codex, Claude Code, and Hermes Relay integrations must be reinstalled. Generated
MCP entries now invoke
nemo-relay mcp, persistent hooks are agent-owned andgeneration-fenced, and the shared gateway uses user-scoped configuration.
Run
nemo-relay install <agent> --forcefor each installed integration, ornemo-relay install all --force. Previously generated integration state isnot compatible with the new lifecycle and may fail readiness or doctor checks.
Overview
Add an MCP-managed shared Relay gateway for Codex, Claude Code, and Hermes. The new lifecycle guarantees that Relay is ready before installed coding agents emit hooks or routed provider traffic, fixing RELAY-447.
nemo-relay mcpeagerly starts or adopts the existing Rust gateway before reading MCP input, heartbeats it while stdio is open, and coordinates one recovery. Codex, Claude Code, and Hermes use the same lifecycle model.Details
Problems addressed
New behavior
nemo-relay mcpacquires the gateway before MCP initialization and advertises no tools.alwaysLoad; Hermes uses the same MCP lifecycle and canonical hooks.Component breakdown
crates/cli/src/commands/crates/cli/src/agents/crates/cli/src/mcp/gateway.rscrates/cli/src/server.rscrates/cli/src/bootstrap/crates/cli/src/gateway/client.rscrates/cli/src/process/detached.rssrc/crates/cli/tests/The CLI source now has two explicit ownership axes.
commands/owns command execution;agents/owns agent vertical slices and shared installation transactions. Gateway, MCP transport, sessions, bootstrap lifecycle, and runtime configuration remain agent-neutral. Architecture tests prevent retired top-level agent trees from returning and prohibit shared-service-to-command and cross-agent dependencies.Compatibility and limitations
/modelsbefore launching required MCP servers; captured turns and routed provider requests still wait for readiness.Validation
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningsjust test-rust(992 CLI unit tests and 80 external CLI process tests, plus the complete Rust workspace)uv run pre-commit run --all-filescargo test -p nemo-relay-cli --test architecture_testsWhere should the reviewer start?
Review in four bounded passes:
crates/cli/src/commands/mod.rsand its thin executor modules to verify unchanged dispatch and exit behavior.crates/cli/src/agents/mod.rs, then one host directory at a time. Verify agent-specific adapters, alignment, hooks, trust, and setup are colocated.crates/cli/src/mcp/gateway.rs,bootstrap/mod.rs,gateway/client.rs,process/detached.rs, andserver/mod.rsfor eager acquisition, authenticated adoption, recovery, and idle shutdown.crates/cli/tests/architecture_tests.rsand the grouped coverage directories as the acceptance specification.The key runtime boundary remains: hosts own MCP stdio, MCP coordinates liveness, and the existing Relay gateway owns server lifecycle.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Bugs addressed
Gateway bootstrap and lifecycle
nemo-relay mcpstarts, before waiting for MCP initialization.Hooks and event delivery
nemo-relay runsessions from also invoking persistent hooks and duplicating events.Codex correlation
client_metadata.session_idfor Codex root-session correlation.prompt_cache_keyas a compatibility fallback.client_metadata.thread_idas subagent ownership forcollab_spawn.Installation, trust, and rollback
alwaysLoadconfiguration and validates it during diagnosis.Filesystem and ownership safety
marketplaceRootorpluginRootvalues from granting recursive deletion authority over arbitrary directories.Process management
Environment propagation
NEMO_RELAY_,OTEL_, andAWS_.header_env, AWS secret-key, and session-token variables.New features
Adds the Rust-native
nemo-relay mcpcommand.Makes
nemo-relay mcpa lifecycle coordinator rather than a tool-serving MCP implementation.Advertises no MCP tools while keeping the MCP stdio session alive.
Supports the architecture:
Starts or adopts a detached Rust
nemo-relay --bind 127.0.0.1:47632gateway.Allows multiple Codex, Claude Code, and Hermes MCP processes to share one gateway.
Starts gateway acquisition immediately when MCP stdio opens.
Heartbeats the gateway while MCP remains connected.
Performs one coordinated gateway recovery after failure.
Adds authenticated gateway health, shutdown, hook-delivery, and ownership operations.
Extends
/healthzwith Relay identity, version, protocol, and compatibility information.Adds readiness-file reporting for automatically assigned ports.
Uses system and user Relay configuration for persistent plugin mode.
Keeps project-specific configuration available through transparent
nemo-relay run.Adds a consistent persistent MCP lifecycle for Codex, Claude Code, and Hermes.
Adds generated MCP environment-forwarding declarations for all three agents.
Adds plugin-owned Codex hooks with automatic, verified trust management.
Adds Claude Code plugin activation through
alwaysLoad.Adds Hermes MCP, hook, trust, generation, install, uninstall, and doctor integration.
Adds shared gateway idle shutdown after the final MCP client disconnects.
Adds generation-fenced hook and MCP identities to prevent stale installations from controlling a replacement gateway.
Adds opt-in cold-start and concurrency E2E suites for Codex, Claude Code, and Hermes.
Improvements
Architecture and readability
Reorganizes the CLI around two explicit ownership axes:
commands/owns CLI syntax, validation, dispatch, rendering, and exit codes.agents/owns Codex, Claude Code, and Hermes behavior.Adds an internal library root with a minimal process entrypoint.
Reduces
main.rsto the application entrypoint.Moves all Clap-derived types under
commands/.Makes
alla command-only install target instead of a runtime agent identity.Replaces command-shaped configuration DTOs with subsystem-owned runtime inputs.
Gives Codex, Claude, and Hermes complete vertical slices for launch, hooks, assets, trust, setup, installation, and diagnosis.
Removes obsolete horizontal adapter, alignment, host, installer, and plugin-shim facades.
Removes the production Node bootstrap path; gateway bootstrap is entirely Rust-native.
Separates launch preparation from shared process supervision.
Separates gateway startup coordination from gateway transport and server execution.
Separates detached process handling from bootstrap ownership.
Splits gateway routing, request preparation, response handling, and authenticated client transport.
Splits session routing, correlation, ownership, and idle shutdown.
Splits configuration, diagnostics, filesystem operations, hooks, installation transactions, and plugin services by responsibility.
Keeps agent-neutral installation primitives free of host selection.
Centralizes concrete agent dispatch in
agents/mod.rs.Makes relative persistent output paths deterministic by starting the gateway from the user configuration directory.
Security and transactional integrity
Diagnostics and operator experience
Expands doctor checks for:
alwaysLoad;Provides actionable reinstall remediation when generated state is stale.
Distinguishes foreign listener conflicts from compatible Relay reuse.
Preserves existing CLI output, JSON schemas, aliases, paths, and exit behavior.
Documents operational
hook-forwardflags directly in CLI help without brittle prose-pinning tests.Testing and quality
Keeps all tests outside the
src/tree.Increases the CLI suite to 995 unit tests plus 80 process-level tests.
Adds architecture tests that enforce command, agent, and shared-service dependency boundaries.
Adds coverage for:
Adds real-agent opt-in E2Es without making Codex, Claude Code, or Hermes mandatory Rust CI dependencies.
Adds ten-cold-run and concurrent-session coverage for Codex and Claude Code.
Adds a Hermes MCP E2E target for environments where Hermes is installed.
Enforces CLI coverage through Codecov while avoiding artificial help-text or environment-name tests.
Adds test serialization for process-global environment and working-directory mutations.
Restores cross-platform compilation and coverage-module visibility after the CLI reorganization.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation