feat: emit automatic skill-load marks [1/3]#391
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:XL
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughAdds skill-load detection from tool inputs and metadata, emits ChangesSkill-load detection and marking
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ToolCaller
participant ToolCall
participant SkillLoadDetector
participant EventSubscriber
ToolCaller->>ToolCall: invoke tool
ToolCall->>SkillLoadDetector: detect or parse precomputed loads
SkillLoadDetector-->>ToolCall: skill-load entries
ToolCall->>EventSubscriber: emit tool-start event
ToolCall->>EventSubscriber: emit skill.load marks
sequenceDiagram
participant ClaudeCode
participant Adapter
participant Session
participant ATIFExporter
ClaudeCode->>Adapter: UserPromptExpansion
Adapter->>Adapter: infer slash-command skill load
Adapter->>Session: HookMark with source metadata
Session->>ATIFExporter: emit skill.load.inferred
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/session.rs`:
- Around line 953-965: Extract the "prompt_expansion" and "skill_load_source"
string literals into shared constants and use them in both
NormalizedEvent::HookMark handling in session.rs and inferred_skill_load_event
in adapters/mod.rs. Keep the comparison and metadata lookup wired through those
constants so both call sites stay in sync and future typos can’t silently fall
back to "hook_mark".
In `@crates/cli/tests/coverage/adapters_tests.rs`:
- Around line 132-156: The negative-path check in
does_not_infer_non_slash_or_empty_prompt_expansions is too weak because indexing
skill_load_source can silently return Null. Update the assertions on
claude_code::adapt’s HookMark branch to verify the metadata key is absent or not
equal to prompt_expansion, and also confirm the fallback plain hook_mark shape
by checking the event name/payload for the rejected UserPromptExpansion cases.
In `@crates/core/src/api/skill_load.rs`:
- Around line 236-345: The standalone command parsing in tokenize_simple_command
is hand-rolling shell quoting and escaping, which can miss edge cases. Replace
this custom tokenizer with a maintained shell-lexing approach or crate, then
keep complete_reader_paths, positional_paths, and powershell_content_paths using
the parsed tokens for the reader-safety heuristic. Make sure the new parsing
still preserves the current safe-rejection behavior for pipelines, subshells,
redirects, and other unsupported constructs.
- Around line 131-141: The structured-reader detection in is_structured_reader
is too permissive because it uses a bare suffix match after
normalize_identifier, which can misclassify unrelated tool names as readers.
Update the matching logic in is_structured_reader to follow the project’s real
naming boundary rules by checking token/segment boundaries on the original,
separator-preserved tool name rather than the fully concatenated identifier.
Keep the existing reader name set, but ensure only exact names or valid
namespaced forms are recognized, so tool names like thread or spread do not
trigger a skill.load mark.
- Around line 160-217: The recursive JSON walkers in has_partial_read_controls,
collect_path_skill_names, collect_named_strings, and collect_named_values should
not recurse indefinitely on deeply nested ToolCallParams.args. Replace the
recursive descent with an explicit stack/queue or add a bounded depth check so
nested Value::Object and Value::Array inputs cannot cause stack overflow. Keep
the existing normalization and traversal behavior in skill_load.rs while making
the traversal iterative and safe for maliciously nested tool args.
In `@crates/core/tests/unit/atif_tests.rs`:
- Around line 2628-2656: The test in AtifExporter is too weak because it only
proves no step is emitted when the tool scope is incomplete, not that
Event::Mark filtering works. Update
test_exporter_omits_skill_load_mark_from_atif_steps to drive a full tool
lifecycle with matching Start and End events, place the skill.load mark between
them, and use AtifExporter::subscriber() or the normal export path that
exercises the mark-filtering logic. Then assert exactly one step is produced and
verify the resulting step contains the tool scope data only, with no skill.load
mark data or metadata preserved.
In `@crates/core/tests/unit/skill_load_tests.rs`:
- Around line 1-294: The test suite in SkillLoad coverage is missing the
regression case for the reader-name false positive mentioned in skill_load.rs.
Add a negative test near the existing structured-reader assertions in
skill_load_tests.rs that exercises detect() with a tool name like "thread" and a
SKILL.md path, and assert it is rejected so the ends_with-based matching bug
stays fixed. Refer to detect and
assert_rejected/structured_readers_reject_non_skill_paths_missing_parents_and_non_read_tools
when adding the case.
🪄 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: 3bb3aa60-3d1a-4f47-bf71-9bd0016704d7
📒 Files selected for processing (14)
crates/cli/src/adapters/mod.rscrates/cli/src/installer.rscrates/cli/src/session.rscrates/cli/tests/coverage/adapters_tests.rscrates/cli/tests/coverage/installer_tests.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/api/mod.rscrates/core/src/api/skill_load.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/atif_tests.rscrates/core/tests/unit/skill_load_tests.rsintegrations/coding-agents/claude-code/hooks/hooks.jsonintegrations/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 (16)
**/*
📄 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/core/src/api/mod.rscrates/cli/tests/coverage/installer_tests.rscrates/core/tests/unit/atif_tests.rscrates/cli/src/session.rscrates/cli/src/adapters/mod.rsintegrations/coding-agents/claude-code/hooks/hooks.jsoncrates/cli/src/installer.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/cli/tests/coverage/adapters_tests.rscrates/core/src/api/skill_load.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/core/src/api/mod.rscrates/cli/tests/coverage/installer_tests.rscrates/core/tests/unit/atif_tests.rscrates/cli/src/session.rscrates/cli/src/adapters/mod.rsintegrations/coding-agents/claude-code/hooks/hooks.jsoncrates/cli/src/installer.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/cli/tests/coverage/adapters_tests.rscrates/core/src/api/skill_load.rs
**/*.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/src/api/mod.rscrates/cli/tests/coverage/installer_tests.rscrates/core/tests/unit/atif_tests.rscrates/cli/src/session.rscrates/cli/src/adapters/mod.rscrates/cli/src/installer.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/cli/tests/coverage/adapters_tests.rscrates/core/src/api/skill_load.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/src/api/mod.rscrates/core/tests/unit/atif_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/core/src/api/skill_load.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/src/api/mod.rscrates/core/tests/unit/atif_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/core/src/api/skill_load.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Follow binding naming conventions in Rust and Python: use
snake_case.
Files:
crates/core/src/api/mod.rscrates/cli/tests/coverage/installer_tests.rscrates/core/tests/unit/atif_tests.rscrates/cli/src/session.rscrates/cli/src/adapters/mod.rscrates/cli/src/installer.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/cli/tests/coverage/adapters_tests.rscrates/core/src/api/skill_load.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/src/api/mod.rscrates/cli/tests/coverage/installer_tests.rscrates/core/tests/unit/atif_tests.rscrates/cli/src/session.rscrates/cli/src/adapters/mod.rscrates/cli/src/installer.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/cli/tests/coverage/adapters_tests.rscrates/core/src/api/skill_load.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/src/api/mod.rscrates/cli/tests/coverage/installer_tests.rscrates/core/tests/unit/atif_tests.rscrates/cli/src/session.rscrates/cli/src/adapters/mod.rscrates/cli/src/installer.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/cli/tests/coverage/adapters_tests.rscrates/core/src/api/skill_load.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/src/api/mod.rscrates/cli/tests/coverage/installer_tests.rscrates/core/tests/unit/atif_tests.rscrates/cli/src/session.rscrates/cli/src/adapters/mod.rscrates/cli/src/installer.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/cli/tests/coverage/adapters_tests.rscrates/core/src/api/skill_load.rs
crates/core/src/{api/**/*.rs,api/runtime/**/*.rs,codec/**/*.rs,json.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Implement the new or changed public runtime behavior first in the Rust core, especially under
crates/core/src/api/and related core modules such ascrates/core/src/api/runtime/,crates/core/src/codec/, andcrates/core/src/json.rs.
Files:
crates/core/src/api/mod.rscrates/core/src/api/tool.rscrates/core/src/api/skill_load.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/core/src/api/mod.rscrates/cli/src/session.rscrates/cli/src/adapters/mod.rscrates/cli/src/installer.rscrates/core/src/api/tool.rscrates/core/src/api/skill_load.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/src/api/mod.rscrates/core/tests/unit/atif_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/core/src/api/skill_load.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/src/api/mod.rscrates/cli/tests/coverage/installer_tests.rscrates/core/tests/unit/atif_tests.rscrates/cli/src/session.rscrates/cli/src/adapters/mod.rscrates/cli/src/installer.rscrates/cli/tests/coverage/session_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/cli/tests/coverage/adapters_tests.rscrates/core/src/api/skill_load.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/src/api/mod.rscrates/core/tests/unit/atif_tests.rscrates/core/src/api/tool.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/core/src/api/skill_load.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/installer_tests.rscrates/core/tests/unit/atif_tests.rscrates/cli/tests/coverage/session_tests.rscrates/core/tests/integration/api_surface_tests.rscrates/core/tests/unit/skill_load_tests.rscrates/cli/tests/coverage/adapters_tests.rs
crates/core/src/api/{tool,llm}.rs
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Wire the new middleware chain into the execute path in
crates/core/src/api/tool.rsorcrates/core/src/api/llm.rsat the appropriate pipeline stage
Files:
crates/core/src/api/tool.rs
🔇 Additional comments (12)
crates/core/src/api/tool.rs (2)
17-17: LGTM!Also applies to: 233-252
268-298: 🗄️ Data Integrity & IntegrationNo change needed: orphaned
skill.loadmarks are tolerated
skill.loadmarks can remain when the tool start event is sanitized away, and the observability exporters already treat missing parents as orphan marks rather than invalid data.> Likely an incorrect or invalid review comment.crates/core/tests/integration/api_surface_tests.rs (1)
341-571: LGTM!crates/core/src/api/mod.rs (1)
22-22: LGTM!crates/cli/src/adapters/mod.rs (1)
784-794: 🎯 Functional CorrectnessVerify Codex emits matching
expansion_type/command_namefields.
installer.rsnow wiresUserPromptExpansioninto both Claude Code and Codex hook configs, butinferred_skill_load_eventonly demonstrates coverage against Claude-shaped payloads (expansion_type: "slash_command",command_name). If Codex'sUserPromptExpansionhook payload uses different field names/values, this inference silently falls through to a generichook_markfor Codex without any test coverage confirming that path is exercised or acceptable.As per PR objectives, this PR intends to normalize "hook behavior for Claude Code, Codex, and Hermes, including inferred slash-command expansion marks" — please confirm Codex payload shape parity.
Also applies to: 811-840
crates/cli/src/installer.rs (1)
18-33: LGTM!crates/cli/tests/coverage/adapters_tests.rs (1)
51-130: LGTM!crates/cli/tests/coverage/installer_tests.rs (1)
81-87: LGTM!crates/cli/tests/coverage/session_tests.rs (1)
3388-3453: LGTM!integrations/coding-agents/claude-code/hooks/hooks.json (1)
25-35: LGTM!integrations/coding-agents/codex/hooks/hooks.json (1)
26-36: LGTM!crates/cli/src/session.rs (1)
953-965: 🎯 Functional CorrectnessATIF already skips all
Markevents
skill.load.inferredwon't surface as an ATIF step; the exporter filters marks generically, not by the literalskill.loadname.> Likely an incorrect or invalid review comment.
zhongxuanwang-nv
left a comment
zhongxuanwang-nv
left a comment
There was a problem hiding this comment.
Nitty things, thanks Will!
UserPromptExpansion is only present in Claude Code's plugin hook whitelist from 2.1.116. Older hosts reject the entire plugin hooks file on the unknown event name and silently load no relay hooks, for both the transparent-run temp plugin and the marketplace plugin. Document the verified floor on HOOK_EVENTS, make doctor warn when the probed Claude Code predates it, and state the requirement in the plugin README. Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com> (cherry picked from commit d3b8959)
Signed-off-by: Will Killian <wkillian@nvidia.com>
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
3 participants
Overview
Implements the shared RELAY-443 runtime behavior for automatic skill-load marks. This is the first commit in a three-PR cumulative stack; all three PRs target upstream
mainas requested.Details
SKILL.mdreads, and safe standalone shell readers in the Rust tool lifecycle.skill.loadmarks with minimal payloads, per-call deduplication, and failure-path preservation.srctree.Validation:
just test-rust,just test-python,just test-go,just test-node,cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings, anduv run pre-commit run --all-files.Breaking changes: none.
Where should the reviewer start?
Start with
crates/core/src/api/skill_load.rs, then review its lifecycle integration incrates/core/src/api/tool.rsand the external test matrix incrates/core/tests/unit/skill_load_tests.rs.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation