Repository navigation
fix: forward Relay logging variables to the Claude child - #385
Open
mnajafian-nv wants to merge 1 commit into
Open
mnajafian-nv wants to merge 1 commit into
mnajafian-nv wants to merge 1 commit into
Conversation
The Claude adapter hides every inherited variable outside its allowlist by overriding it with an empty string, because ClaudeAgentOptions.env can only override os.environ, not remove from it. NeMo Relay rejects an empty NEMO_RELAY_LOG, NEMO_RELAY_LOG_CONFIG_PATH, NEMO_RELAY_LOG_STDERR, or NEMO_RELAY_LOG_STDERR_FORMAT, so when Fabric starts from a shell that sets any of them, every Relay hook-forward call exits with a configuration error. No lifecycle hook reaches the gateway, and a run whose model call succeeded fails with claude_relay_atif_timeout. Keep these four logging settings from the parent environment. They hold log levels and a log config path, not credentials. Other variables, including NEMO_RELAY_CLIENT_TOKEN, stay blanked; Relay treats an empty client token as unset. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
WalkthroughThe Claude adapter now forwards four NeMo Relay logging variables from the parent environment to Claude. Documentation lists the variables, and a parameterized test checks that their values are preserved while the Relay client token and an unrelated secret are blanked. ChangesClaude Relay logging environment
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The adapter forwards Relay logging settings while continuing to exclude the client token. The reported test-environment leak is prevented by automatic cleanup, leaving no concrete merge-blocking risk. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title uses valid Conventional Commits syntax with the allowed lowercase type fix, stays under 72 characters, has no trailing period, and accurately summarizes forwarding Relay logging variables …Description check The description includes the required overview, reviewer starting point, related-issues section, contribution confirmation, and duplicate-check confirmation. It also provides relevant implementation a…
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
-
-
🧪 Generate unit tests (beta)
-
-
-
- Create a new PR
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Fern docs preview: https://nvidia-preview-pull-request-385.docs.buildwithfern.com/nemo/fabric |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Preserve the parent's NeMo Relay logging configuration in Claude Code child processes. The adapter currently blanks inherited variables outside its allowlist, and Relay rejects empty logging values. This causes every Relay hook to fail and the run to end with
claude_relay_atif_timeout.Other inherited variables, including
NEMO_RELAY_CLIENT_TOKEN, remain filtered. No breaking changes or dependency changes.Details
NEMO_RELAY_LOG,NEMO_RELAY_LOG_CONFIG_PATH,NEMO_RELAY_LOG_STDERR, andNEMO_RELAY_LOG_STDERR_FORMAT.Validation
main.claude_relay_atif_timeout. The full Python suite was not run locally; CI covers it.Where should the reviewer start?
Start with
INHERITED_ENV_NAMESin the Claude adapter, thentest_build_options_relay_logging_environment_set_forwards_parent_values.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to: none.
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.