fix(plugin): route registration banner to stderr - #5674
Conversation
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
📝 WalkthroughWalkthroughThe plugin registration startup banner in ChangesBanner output routed to stderr
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw/src/register.test.ts (1)
81-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore
process.stderr.writeafter each test to prevent cross-test bleed.
vi.clearAllMocks()resets call history but does not restore spied implementations. AddingafterEach(() => vi.restoreAllMocks())(or restoringstderrWritedirectly) avoids leakage into other suites.Suggested patch
describe("plugin registration", () => { beforeEach(() => { vi.clearAllMocks(); mockStderrWrite(); }); + afterEach(() => { + vi.restoreAllMocks(); + }); @@ describe("before_tool_call secret scanner hook (`#1233`)", () => { beforeEach(() => { vi.clearAllMocks(); mockStderrWrite(); }); + afterEach(() => { + vi.restoreAllMocks(); + });Also applies to: 209-210
🤖 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 `@nemoclaw/src/register.test.ts` at line 81, The mockStderrWrite function call spies on process.stderr.write but the spy is not being restored between tests, which can cause state to leak from one test to another. Add an afterEach hook in the test suite that calls vi.restoreAllMocks() to properly restore the original implementations of spied functions after each test completes, ensuring no cross-test contamination occurs.
🤖 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.
Nitpick comments:
In `@nemoclaw/src/register.test.ts`:
- Line 81: The mockStderrWrite function call spies on process.stderr.write but
the spy is not being restored between tests, which can cause state to leak from
one test to another. Add an afterEach hook in the test suite that calls
vi.restoreAllMocks() to properly restore the original implementations of spied
functions after each test completes, ensuring no cross-test contamination
occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e1e9707-82ef-4363-87d5-7e7005ad92e2
📒 Files selected for processing (2)
nemoclaw/src/index.tsnemoclaw/src/register.test.ts
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
|
✨ Thanks for routing the plugin registration banner to stderr to keep non-JSON agent passthrough stdout clean. This proposes a way to write the banner via process.stderr.write and updates plugin registration tests to capture stderr. Related open issues: |
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed the exact head after the shared-test rerun: the registration banner is redirected to stderr, stdout remains machine-readable, and the focused regression coverage plus full CI are green.
<!-- markdownlint-disable MD041 --> ## Summary Routes the in-sandbox NemoClaw registration banner to `stderr` so non-JSON agent output on `stdout` remains machine-readable. This is a current-main, GitHub-verified replacement for #5674 that preserves the contributor's original authorship. ## Related Issue Fixes #5654. ## Changes - Write the plugin registration banner directly to `stderr` instead of plugin info logs. - Cover banner routing, live model rendering, mock restoration, and the non-JSON passthrough stream boundary. - Document the agent command's stdout/stderr contract. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npm --prefix nemoclaw test -- src/register.test.ts` passed 24/24; `npx vitest run --project cli src/lib/actions/sandbox/agent/passthrough.test.ts` passed 38/38; plugin and CLI builds passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) `npm run docs` passed with zero errors and two pre-existing Fern warnings. --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated agent startup so the NemoClaw registration banner is emitted to standard error, avoiding interference with non-JSON agent replies on standard output. * **Documentation** * Refreshed the command reference to clarify where the banner appears during non-JSON runs. * **Tests** * Revised banner-related tests to capture and assert standard error output, and added coverage to ensure standard output remains clean for non-JSON replies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
<!-- markdownlint-disable MD041 --> ## Summary Routes the in-sandbox NemoClaw registration banner to `stderr` so non-JSON agent output on `stdout` remains machine-readable. This is a current-main, GitHub-verified replacement for NVIDIA#5674 that preserves the contributor's original authorship. ## Related Issue Fixes NVIDIA#5654. ## Changes - Write the plugin registration banner directly to `stderr` instead of plugin info logs. - Cover banner routing, live model rendering, mock restoration, and the non-JSON passthrough stream boundary. - Document the agent command's stdout/stderr contract. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npm --prefix nemoclaw test -- src/register.test.ts` passed 24/24; `npx vitest run --project cli src/lib/actions/sandbox/agent/passthrough.test.ts` passed 38/38; plugin and CLI builds passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) `npm run docs` passed with zero errors and two pre-existing Fern warnings. --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated agent startup so the NemoClaw registration banner is emitted to standard error, avoiding interference with non-JSON agent replies on standard output. * **Documentation** * Refreshed the command reference to clarify where the banner appears during non-JSON runs. * **Tests** * Revised banner-related tests to capture and assert standard error output, and added coverage to ensure standard output remains clean for non-JSON replies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
Summary
Routes the NemoClaw plugin registration banner directly to stderr instead of
api.logger.info, so non-JSONnemoclaw <sandbox> agentpassthrough stdout remains reserved for the agent reply.Related Issue
Fixes #5654
Changes
process.stderr.write.api.logger.info.Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Targeted checks run:
cd nemoclaw && npm test -- src/register.test.tscd nemoclaw && npm run lint -- src/index.ts src/register.test.tscd nemoclaw && npm run buildChecklist
General
Code Changes
Signed-off-by: WilliamK112 164879897+WilliamK112@users.noreply.github.com
Summary by CodeRabbit
Refactor
Tests