feat(cli): add structured logging with --debug and NEMOCLAW_LOG_LEVEL - #6268
Closed
sauravdev wants to merge 2 commits into
Closed
feat(cli): add structured logging with --debug and NEMOCLAW_LOG_LEVEL#6268sauravdev wants to merge 2 commits into
sauravdev wants to merge 2 commits into
Conversation
- docs/inference: custom-llm-provider, switch-to-brev-nemotron-120b, switch-to-nemotron-super-120b - skill/nat: SKILL.md and reference docs - nemoclaw-sandbox-policy.yaml Rebased onto NVIDIA/NemoClaw main (at d9aced4). Prior edits to files that upstream refactored or deleted were dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces src/lib/cli/logger.ts — a singleton Logger class with four levels (error < warn < info < debug) written exclusively to stderr so stdout remains clean for --json consumers. - `NEMOCLAW_LOG_LEVEL=debug` env var sets level at process start - `--debug` flag (inherited by all commands via NemoClawCommand.baseFlags) activates debug level and ISO-8601 timestamp prefixes - `-q / --quiet` flag suppresses info; shows only warn+error - `NEMOCLAW_DEBUG=1` and `DEBUG=*nemoclaw*` also activate debug level NemoClawCommand.init() reads both flags and configures the singleton before any command's run() method executes, so subcommands see the correct level without needing to parse flags themselves. Adds unit tests covering all level transitions and env-var pickup. Gradual migration of existing console.log/console.error call sites to log.info/log.error can be done incrementally on separate branches. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds documentation for custom LLM providers and switching to Brev-hosted Nemotron models, a new sandbox policy YAML file, NAT skill/reference documentation, and a CLI logger module wired into NemoClawCommand with new debug/quiet flags. ChangesDocumentation additions
Sandbox policy
CLI logger
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as NemoClaw CLI
participant Command as NemoClawCommand.init
participant Logger as log singleton
CLI->>Command: parse flags (--debug/--quiet)
Command->>Logger: setDebug(true) / setQuiet(true)
Logger-->>Command: updated level/quiet state
Command-->>CLI: proceed with command execution
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
docs/inference/custom-llm-provider.md (2)
334-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwitch these cross-page links to route-style paths.
As per path instructions, use route-style links without file extensions for docs-to-docs navigation.
♻️ Proposed fix
- [Switch Inference Models at Runtime](./switch-inference-providers.md) - [Inference Profiles Reference](../reference/inference-profiles.md) - [Network Policy — Approve Network Requests](../network-policy/approve-network-requests.md) + [Switch Inference Models at Runtime](./switch-inference-providers) + [Inference Profiles Reference](../reference/inference-profiles) + [Network Policy — Approve Network Requests](../network-policy/approve-network-requests)🤖 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 `@docs/inference/custom-llm-provider.md` around lines 334 - 338, The Related Topics links in the custom LLM provider docs still use relative file paths with .md targets; update those cross-page links to route-style docs paths without file extensions. Use the existing link entries in the Related Topics section as the place to fix, and convert each docs-to-docs reference to the route-style form used elsewhere in the docs.Source: Path instructions
57-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the literal
nemoclawcommand here.As per path instructions, use literal command names on pages that have only one agent variant.
♻️ Proposed fix
-$ openclaw nemoclaw onboard \ +$ nemoclaw onboard \ ... -openclaw nemoclaw status +nemoclaw statusAlso applies to: 229-230
🤖 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 `@docs/inference/custom-llm-provider.md` around lines 57 - 61, The onboarding command example in the custom LLM provider docs uses the wrong literal CLI name; replace the command in the shown example with the exact `nemoclaw` invocation. Update the `onboard` example so it matches the product’s literal command naming, and make the same change anywhere else in this doc where the command example uses the alternate name.Source: Path instructions
src/lib/cli/logger.ts (2)
36-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winQuiet mode isn't durable against later
setLevel/setDebugcalls.
setQuietclamps_levelonce, butshouldLogonly consults_level—_quietis otherwise inert. If any code callssetLevel/setDebugaftersetQuiet(true), the level rises again and quiet mode is silently defeated even thoughisQuiet()still reportstrue. Current wiring innemoclaw-oclif-command.tscallssetDebugbeforesetQuiet, so today's usage is safe, but the class doesn't defend against future callers doing the reverse.♻️ Suggested refactor: compute effective level from base level + quiet
class Logger { private _level: LogLevel; private _quiet: boolean; private _timestamps: boolean; private shouldLog(level: LogLevel): boolean { - return LEVEL_RANK[level] <= LEVEL_RANK[this._level]; + const effectiveLevel = this._quiet && LEVEL_RANK[this._level] > LEVEL_RANK["warn"] + ? "warn" + : this._level; + return LEVEL_RANK[level] <= LEVEL_RANK[effectiveLevel]; } }🤖 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 `@src/lib/cli/logger.ts` around lines 36 - 77, Quiet mode in Logger is only applied once in setQuiet, but shouldLog still reads the mutable _level directly, so later setLevel or setDebug calls can override quiet unexpectedly. Update the Logger class so the effective log decision always incorporates _quiet alongside _level, and make setLevel/setDebug preserve quiet behavior rather than bypass it; use the Logger methods and shouldLog as the main places to adjust.
79-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate timestamp-prefix logic between
prefix()anddebugObject().
debugObjectreimplements the same[timestamp] [LEVEL]formatting asprefix()instead of reusing it.♻️ Suggested refactor
debugObject(label: string, obj: unknown): void { if (!this.shouldLog("debug")) return; - const ts = this._timestamps ? `[${new Date().toISOString()}] [DEBUG] ` : ""; - process.stderr.write(`${ts}${label}: ${JSON.stringify(obj, null, 2)}\n`); + process.stderr.write(`${this.prefix("debug")}${label}: ${JSON.stringify(obj, null, 2)}\n`); }🤖 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 `@src/lib/cli/logger.ts` around lines 79 - 114, The logger formatting logic is duplicated between prefix() and debugObject(), so refactor debugObject() to reuse prefix() instead of building its own timestamp/level string. Keep the existing debugObject() behavior and JSON output, but centralize the shared `[timestamp] [LEVEL]` prefix formatting in Logger.prefix so future changes stay consistent.skill/nat/references/custom-tools.md (1)
11-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTag the directory tree fence.
The unlabelled fence at Line 11 will trip markdownlint MD040. Mark it as
textso the page stays lint-clean.♻️ Proposed fix
-``` +```text🤖 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 `@skill/nat/references/custom-tools.md` around lines 11 - 19, The directory tree fence in custom-tools.md is unlabeled and will trigger markdownlint MD040; update the fenced block to use the text language tag so the tree example in the my_custom_tool layout renders as plain text and stays lint-clean.Source: Linters/SAST tools
skill/nat/references/function-groups.md (1)
37-57: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid blocking S3 calls inside the async builder.
put_objectandget_objectrun on the event loop here, so any real storage call will block other agent work. Please verify the example against NAT's async pattern and switch to an async client orasyncio.to_thread(...)ifboto3must stay.🔧 Proposed fix
+import asyncio + `@register_function_group`(config_type=ObjectStoreConfig) async def build_object_store(config: ObjectStoreConfig, builder: Builder): @@ async def save_fn(filename: str, content: bytes) -> str: - s3_client.put_object(Bucket=config.bucket, Key=filename, Body=content) + await asyncio.to_thread( + s3_client.put_object, + Bucket=config.bucket, + Key=filename, + Body=content, + ) return f"Saved {filename}" async def load_fn(filename: str) -> bytes: - response = s3_client.get_object(Bucket=config.bucket, Key=filename) + response = await asyncio.to_thread( + s3_client.get_object, + Bucket=config.bucket, + Key=filename, + ) return response['Body'].read()🤖 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 `@skill/nat/references/function-groups.md` around lines 37 - 57, The async builder in build_object_store is using blocking boto3 calls inside save_fn and load_fn, which can stall the event loop. Update the FunctionGroup-backed functions to follow NAT’s async pattern by either switching to an async S3 client or wrapping s3_client.put_object and s3_client.get_object with asyncio.to_thread so the storage operations do not block. Keep the existing build_object_store, save_fn, and load_fn structure, but ensure the S3 I/O is executed off the event loop.skill/nat/references/examples.md (1)
15-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClean up the markdownlint failures in this page.
Add a language tag to the tree fence and restore the blank lines around the category headings so the file matches the repo's docs rules.
Also applies to: 28-73
🤖 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 `@skill/nat/references/examples.md` around lines 15 - 24, The markdown in examples.md has lint issues: the tree diagram fence is missing a language tag and the category headings lose required surrounding blank lines. Update the fenced example under the example structure so it uses an appropriate language identifier, and adjust the heading spacing in the referenced section so headings are surrounded by blank lines to match the repo’s docs rules.Source: Linters/SAST tools
🤖 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 `@docs/inference/custom-llm-provider.md`:
- Around line 267-268: The guidance for local providers is inaccurate because
`host.openshell.internal` is not automatically reachable unless the policy
explicitly allowlists it. Update the wording in the custom-llm-provider doc
section for local providers (vLLM, Ollama) to tell users they must add
`host.openshell.internal` to the gateway/network policy, and verify the
surrounding network-policy example and any related references in the same
section match that requirement.
In `@docs/inference/switch-to-brev-nemotron-120b.md`:
- Around line 196-199: Add the missing blank line before the fenced bash example
in the markdown section so the “Check with:” text is separated from the code
block. Update the nearby prose and fenced block formatting in the affected
markdown snippet to satisfy markdownlint-cli2 requirements for fenced blocks.
- Around line 136-139: The plain-text example in this markdown snippet is
missing a language tag on its fence, which triggers markdownlint. Update the
fenced block around the model table in switch-to-brev-nemotron-120b.md so the
opening fence is marked as text, keeping the example content unchanged.
In `@docs/inference/switch-to-nemotron-super-120b.md`:
- Around line 418-421: The expected-output fenced blocks in the
switch-to-nemotron-super-120b doc are missing a language tag, which triggers
markdownlint failures. Update the plain-text output fences referenced by the
document’s expected-output sections to use the text fence label consistently,
including the blocks associated with Provider/Model examples and the other noted
occurrences. Use the existing fenced output sections in this markdown file as
the places to fix.
In `@nemoclaw-sandbox-policy.yaml`:
- Around line 26-34: The openai networkPolicyEntry is missing the required
binaries field, causing schema validation to fail. Update the openai entry in
the policy so it includes binaries alongside name and endpoints, matching the
structure used by other valid policy entries and the expected networkPolicyEntry
schema.
In `@skill/nat/SKILL.md`:
- Around line 81-84: The workflow type list contains a misspelled literal in the
SKILL.md agent examples. Update the `rewwo_agent` entry in the agent list to the
correct `rewoo_agent` spelling so readers copy the valid workflow name; the fix
is localized to the agent literals section alongside `react_agent`,
`reasoning_agent`, and `responses_api_agent`.
---
Nitpick comments:
In `@docs/inference/custom-llm-provider.md`:
- Around line 334-338: The Related Topics links in the custom LLM provider docs
still use relative file paths with .md targets; update those cross-page links to
route-style docs paths without file extensions. Use the existing link entries in
the Related Topics section as the place to fix, and convert each docs-to-docs
reference to the route-style form used elsewhere in the docs.
- Around line 57-61: The onboarding command example in the custom LLM provider
docs uses the wrong literal CLI name; replace the command in the shown example
with the exact `nemoclaw` invocation. Update the `onboard` example so it matches
the product’s literal command naming, and make the same change anywhere else in
this doc where the command example uses the alternate name.
In `@skill/nat/references/custom-tools.md`:
- Around line 11-19: The directory tree fence in custom-tools.md is unlabeled
and will trigger markdownlint MD040; update the fenced block to use the text
language tag so the tree example in the my_custom_tool layout renders as plain
text and stays lint-clean.
In `@skill/nat/references/examples.md`:
- Around line 15-24: The markdown in examples.md has lint issues: the tree
diagram fence is missing a language tag and the category headings lose required
surrounding blank lines. Update the fenced example under the example structure
so it uses an appropriate language identifier, and adjust the heading spacing in
the referenced section so headings are surrounded by blank lines to match the
repo’s docs rules.
In `@skill/nat/references/function-groups.md`:
- Around line 37-57: The async builder in build_object_store is using blocking
boto3 calls inside save_fn and load_fn, which can stall the event loop. Update
the FunctionGroup-backed functions to follow NAT’s async pattern by either
switching to an async S3 client or wrapping s3_client.put_object and
s3_client.get_object with asyncio.to_thread so the storage operations do not
block. Keep the existing build_object_store, save_fn, and load_fn structure, but
ensure the S3 I/O is executed off the event loop.
In `@src/lib/cli/logger.ts`:
- Around line 36-77: Quiet mode in Logger is only applied once in setQuiet, but
shouldLog still reads the mutable _level directly, so later setLevel or setDebug
calls can override quiet unexpectedly. Update the Logger class so the effective
log decision always incorporates _quiet alongside _level, and make
setLevel/setDebug preserve quiet behavior rather than bypass it; use the Logger
methods and shouldLog as the main places to adjust.
- Around line 79-114: The logger formatting logic is duplicated between prefix()
and debugObject(), so refactor debugObject() to reuse prefix() instead of
building its own timestamp/level string. Keep the existing debugObject()
behavior and JSON output, but centralize the shared `[timestamp] [LEVEL]` prefix
formatting in Logger.prefix so future changes stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c519cb49-3a8d-417a-9be6-f56ee1e7f6b0
📒 Files selected for processing (13)
docs/inference/custom-llm-provider.mddocs/inference/switch-to-brev-nemotron-120b.mddocs/inference/switch-to-nemotron-super-120b.mdnemoclaw-sandbox-policy.yamlskill/nat/SKILL.mdskill/nat/references/a2a-server.mdskill/nat/references/custom-tools.mdskill/nat/references/examples.mdskill/nat/references/function-groups.mdskill/nat/references/install-from-source.mdsrc/lib/cli/logger.test.tssrc/lib/cli/logger.tssrc/lib/cli/nemoclaw-oclif-command.ts
Comment on lines
+267
to
+268
| For local providers (vLLM, Ollama), traffic goes through `host.openshell.internal` | ||
| which is already allowed by the gateway. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the local-provider network policy guidance.
The provided policy check snippet shows host.openshell.internal is only reachable when the policy explicitly allowlists it, so saying it is already allowed will send users to a setup that still fails.
♻️ Proposed fix
-For local providers (vLLM, Ollama), traffic goes through `host.openshell.internal`
-which is already allowed by the gateway.
+For local providers (vLLM, Ollama), traffic goes through `host.openshell.internal`.
+Add `host.openshell.internal` to the sandbox policy before you use those providers.📝 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.
Suggested change
| For local providers (vLLM, Ollama), traffic goes through `host.openshell.internal` | |
| which is already allowed by the gateway. | |
| For local providers (vLLM, Ollama), traffic goes through `host.openshell.internal`. | |
| Add `host.openshell.internal` to the sandbox policy before you use those providers. |
🤖 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 `@docs/inference/custom-llm-provider.md` around lines 267 - 268, The guidance
for local providers is inaccurate because `host.openshell.internal` is not
automatically reachable unless the policy explicitly allowlists it. Update the
wording in the custom-llm-provider doc section for local providers (vLLM,
Ollama) to tell users they must add `host.openshell.internal` to the
gateway/network policy, and verify the surrounding network-policy example and
any related references in the same section match that requirement.
Comment on lines
+136
to
+139
| ``` | ||
| Model Input Ctx Local Auth Tags | ||
| openai/nvidia/nemotron-3-super-120b-a12b text 128k no yes default,configured | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark the output fence as text.
This plain-text example needs a language tag, or markdownlint-cli2 will flag it.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 136-136: 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 `@docs/inference/switch-to-brev-nemotron-120b.md` around lines 136 - 139, The
plain-text example in this markdown snippet is missing a language tag on its
fence, which triggers markdownlint. Update the fenced block around the model
table in switch-to-brev-nemotron-120b.md so the opening fence is marked as text,
keeping the example content unchanged.
Source: Linters/SAST tools
Comment on lines
+196
to
+199
| Check with: | ||
| ```bash | ||
| openclaw models list | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line before the fenced example.
markdownlint-cli2 requires blank lines around fenced blocks, and this one starts immediately after “Check with:”.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 197-197: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 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 `@docs/inference/switch-to-brev-nemotron-120b.md` around lines 196 - 199, Add
the missing blank line before the fenced bash example in the markdown section so
the “Check with:” text is separated from the code block. Update the nearby prose
and fenced block formatting in the affected markdown snippet to satisfy
markdownlint-cli2 requirements for fenced blocks.
Source: Linters/SAST tools
Comment on lines
+418
to
+421
| ``` | ||
| Provider: brev-nemotron | ||
| Model: nvidia/nemotron-3-super-120b-a12b | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark the expected-output fences as text.
These plain-text output blocks need a language tag to satisfy markdownlint-cli2.
Also applies to: 435-438, 483-487
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 418-418: 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 `@docs/inference/switch-to-nemotron-super-120b.md` around lines 418 - 421, The
expected-output fenced blocks in the switch-to-nemotron-super-120b doc are
missing a language tag, which triggers markdownlint failures. Update the
plain-text output fences referenced by the document’s expected-output sections
to use the text fence label consistently, including the blocks associated with
Provider/Model examples and the other noted occurrences. Use the existing fenced
output sections in this markdown file as the places to fix.
Source: Linters/SAST tools
Comment on lines
+26
to
+34
| openai: | ||
| name: openai | ||
| endpoints: | ||
| - host: api.openai.com | ||
| port: 443 | ||
| access: full | ||
| - host: inference.local | ||
| port: 80 | ||
| access: full |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify schema requires binaries and how policy is validated/loaded
fd -t f 'sandbox-policy.schema.json' --exec cat {}
rg -nP -C3 '"required"\s*:\s*\[[^]]*binaries' schemas/
rg -nP -C3 'network_policies|networkPolicyEntry|binaries' --type ts -g '!*.test.ts'Repository: NVIDIA/NemoClaw
Length of output: 7388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant file excerpt =="
cat -n nemoclaw-sandbox-policy.yaml | sed -n '1,120p'
echo
echo "== references to inference.local =="
rg -n "inference\.local|api\.openai\.com|openai:" -S .
echo
echo "== other network policy entries with binaries =="
python3 - <<'PY'
from pathlib import Path
import re
text = Path("nemoclaw-sandbox-policy.yaml").read_text()
for name in ["clawhub", "openclaw_api", "openclaw_docs", "npm_registry", "github", "openai"]:
m = re.search(rf'^\s*{re.escape(name)}:\n(.*?)(?=^\s*[A-Za-z0-9_]+:|\Z)', text, re.S | re.M)
if not m:
continue
block = m.group(0)
print(f"\n--- {name} ---")
print(block)
PYRepository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema excerpt around networkPolicyEntry =="
sed -n '50,95p' schemas/sandbox-policy.schema.json
echo
echo "== policy preset schema excerpt around networkPolicyEntry =="
sed -n '25,70p' schemas/policy-preset.schema.jsonRepository: NVIDIA/NemoClaw
Length of output: 3358
Add binaries to openai. nemoclaw-sandbox-policy.yaml:26-34
Every networkPolicyEntry requires name, endpoints, and binaries; openai is missing binaries, so this policy fails schema validation.
🤖 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-sandbox-policy.yaml` around lines 26 - 34, The openai
networkPolicyEntry is missing the required binaries field, causing schema
validation to fail. Update the openai entry in the policy so it includes
binaries alongside name and endpoints, matching the structure used by other
valid policy entries and the expected networkPolicyEntry schema.
Comment on lines
+81
to
+84
| - `react_agent` — Reasoning and acting | ||
| - `reasoning_agent` — Advanced reasoning | ||
| - `rewwo_agent` — Reasoning Without Observation | ||
| - `responses_api_agent` — OpenAI Responses API |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the rewoo_agent literal.
rewwo_agent is misspelled, so readers will copy a non-existent workflow type.
🐛 Proposed fix
-`rewwo_agent` — Reasoning Without Observation
+`rewoo_agent` — Reasoning Without Observation📝 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.
Suggested change
| - `react_agent` — Reasoning and acting | |
| - `reasoning_agent` — Advanced reasoning | |
| - `rewwo_agent` — Reasoning Without Observation | |
| - `responses_api_agent` — OpenAI Responses API | |
| - `react_agent` — Reasoning and acting | |
| - `reasoning_agent` — Advanced reasoning | |
| - `rewoo_agent` — Reasoning Without Observation | |
| - `responses_api_agent` — OpenAI Responses API |
🤖 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 `@skill/nat/SKILL.md` around lines 81 - 84, The workflow type list contains a
misspelled literal in the SKILL.md agent examples. Update the `rewwo_agent`
entry in the agent list to the correct `rewoo_agent` spelling so readers copy
the valid workflow name; the fix is localized to the agent literals section
alongside `react_agent`, `reasoning_agent`, and `responses_api_agent`.
cv
requested changes
Jul 4, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the contribution. I’m requesting changes because this PR is not scoped to structured logging.
The PR adds a large set of unrelated files that have nothing to do with the logging feature described in the title/body:
docs/inference/custom-llm-provider.mddocs/inference/switch-to-brev-nemotron-120b.mddocs/inference/switch-to-nemotron-super-120b.mdnemoclaw-sandbox-policy.yamlskill/nat/**
Please remove those from this PR or split them into separate, focused PRs with their own summaries, tests, and docs review. Keeping unrelated docs, policy, and skill additions bundled with a CLI logging change makes the review unsafe and makes it impossible to reason about the actual intended change set.
There is also at least one logging-scope concern to address before this can be reconsidered: adding quiet as a global base flag with -q conflicts with existing command-specific meanings/usages, especially nemoclaw debug --quick|-q, and overlaps with existing quiet flags on commands such as gateway token/restart and dashboard URL. A repo-wide base flag needs an explicit compatibility audit and tests proving existing public CLI grammar is not broken.
Please rescope the PR to logging-only changes, then we can review the logger design and flag behavior separately.
21 tasks
cv
added a commit
that referenced
this pull request
Jul 9, 2026
…#6272) <!-- markdownlint-disable MD041 --> ## Summary Introduces a centralized CLI logging facility with `error`, `warn`, `info`, and `debug` levels written exclusively to `stderr`. Logging can be configured with `NEMOCLAW_LOG_LEVEL`, `NEMOCLAW_DEBUG`, or hidden long-form `--debug` and `--quiet` flags on commands whose oclif parser owns those options. The logger now uses NemoClaw's shared credential taxonomy as a non-throwing redaction boundary for messages, split arguments, structured values, and argv arrays. Parser-owned flags do not acquire host meaning after `--`, and raw passthrough commands preserve downstream arguments unchanged. The generic `DEBUG` variable is intentionally not a NemoClaw logger control because oclif can emit raw argv before NemoClaw's redaction boundary. The command reference directs users to the NemoClaw-specific controls instead. Replaces #6268, which was closed while it still carried unrelated changes. Existing `console.*` call sites remain unchanged so migration to leveled logging can proceed incrementally. ## Changes - Add `src/lib/cli/logger.ts` with leveled output, deterministic environment precedence, reversible configuration, and non-throwing serialization for circular values, BigInt, Error, Map, and Set. - Add hidden, mutually exclusive `--debug` and `--quiet` base flags without claiming a global `-q` shorthand. - Configure host logging from oclif parser output while preserving strict-false passthrough and `--` argument ownership. - Expand shared log redaction for canonical credential fields, uppercase environment keys, private/session keys, Basic/Digest/proxy authorization, cookies, split logger arguments, and inline or positional argv credentials. Public-key, author, and OAuth labels remain visible. - Document logging controls, precedence, passthrough behavior, and the generic `DEBUG` risk in the canonical command reference and synchronized Hermes variant. - Add unit and integration regressions for credential disclosure paths, serializer and sink failures, environment precedence, singleton reset, passthrough ownership, uninstall forwarding, existing quiet flags, and `debug -q`. ## 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 all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [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: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: maintainer security review and requested changes in #6272 (review); fixes applied through `a3ab263da` and independently re-audited. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [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 (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Verification detail: - Pre-commit and pre-push hooks pass, including repository checks, environment-variable documentation, gitleaks, commitlint, and CLI TypeScript. - `npm run typecheck:cli` and `npm run build:cli` pass. - Focused logger, redaction, parser, passthrough, and command-adapter tests pass (104/104). - Broader redaction consumers pass (94/94), and compiled diagnostic redaction passes (41/41). - Both observed CI-shard regressions were reproduced locally and fixed: provenance now preserves line boundaries during redaction, while valid Basic/Bearer headers retain later same-line diagnostics. Exact split-label matching also preserves ordinary diagnostic prose while still redacting credential labels and flags. The final focused redaction/logger/provenance/inference set passes (66/66), broader redaction consumers pass (100/100), and compiled OpenClaw integration passes (13/13) after the latest merge from `main`. - Adversarial probes cover uppercase/private/session fields, Basic/Digest/proxy authorization, cookies, split arguments, inline and positional argv, dash-prefixed opaque values, and safe public-key/author/OAuth exceptions. - Debug CLI integration passes (11 passed, 1 platform-dependent test skipped). - Test-size and test-title gates pass. - `npm run docs` passes with 0 Fern errors and 2 pre-existing Fern warnings; documentation review found no additional pages requiring changes. --- Signed-off-by: sauravdev <saurava@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Introduced centralized CLI stderr logging with `NEMOCLAW_LOG_LEVEL` / `NEMOCLAW_DEBUG`, quiet/debug modes, and timestamped debug output. * Added structured `debugObject` output plus sequence-aware redaction for log-like values. * **Bug Fixes** * Improved `--debug`/`--quiet` interactions with environment precedence and `--` option boundaries; ensured stderr write failures don’t break commands. * **Security** * Strengthened credential redaction for headers/cookies and sensitive CLI flag values, including “fails closed” handling. * **Documentation** * Documented logging environment variables, precedence, and hidden flag behavior. * **Tests** * Expanded coverage for parsing, robustness (complex serialization), redaction correctness, and mock isolation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…NVIDIA#6272) <!-- markdownlint-disable MD041 --> ## Summary Introduces a centralized CLI logging facility with `error`, `warn`, `info`, and `debug` levels written exclusively to `stderr`. Logging can be configured with `NEMOCLAW_LOG_LEVEL`, `NEMOCLAW_DEBUG`, or hidden long-form `--debug` and `--quiet` flags on commands whose oclif parser owns those options. The logger now uses NemoClaw's shared credential taxonomy as a non-throwing redaction boundary for messages, split arguments, structured values, and argv arrays. Parser-owned flags do not acquire host meaning after `--`, and raw passthrough commands preserve downstream arguments unchanged. The generic `DEBUG` variable is intentionally not a NemoClaw logger control because oclif can emit raw argv before NemoClaw's redaction boundary. The command reference directs users to the NemoClaw-specific controls instead. Replaces NVIDIA#6268, which was closed while it still carried unrelated changes. Existing `console.*` call sites remain unchanged so migration to leveled logging can proceed incrementally. ## Changes - Add `src/lib/cli/logger.ts` with leveled output, deterministic environment precedence, reversible configuration, and non-throwing serialization for circular values, BigInt, Error, Map, and Set. - Add hidden, mutually exclusive `--debug` and `--quiet` base flags without claiming a global `-q` shorthand. - Configure host logging from oclif parser output while preserving strict-false passthrough and `--` argument ownership. - Expand shared log redaction for canonical credential fields, uppercase environment keys, private/session keys, Basic/Digest/proxy authorization, cookies, split logger arguments, and inline or positional argv credentials. Public-key, author, and OAuth labels remain visible. - Document logging controls, precedence, passthrough behavior, and the generic `DEBUG` risk in the canonical command reference and synchronized Hermes variant. - Add unit and integration regressions for credential disclosure paths, serializer and sink failures, environment precedence, singleton reset, passthrough ownership, uninstall forwarding, existing quiet flags, and `debug -q`. ## 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 all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [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: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: maintainer security review and requested changes in NVIDIA#6272 (review); fixes applied through `a3ab263da` and independently re-audited. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [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 (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Verification detail: - Pre-commit and pre-push hooks pass, including repository checks, environment-variable documentation, gitleaks, commitlint, and CLI TypeScript. - `npm run typecheck:cli` and `npm run build:cli` pass. - Focused logger, redaction, parser, passthrough, and command-adapter tests pass (104/104). - Broader redaction consumers pass (94/94), and compiled diagnostic redaction passes (41/41). - Both observed CI-shard regressions were reproduced locally and fixed: provenance now preserves line boundaries during redaction, while valid Basic/Bearer headers retain later same-line diagnostics. Exact split-label matching also preserves ordinary diagnostic prose while still redacting credential labels and flags. The final focused redaction/logger/provenance/inference set passes (66/66), broader redaction consumers pass (100/100), and compiled OpenClaw integration passes (13/13) after the latest merge from `main`. - Adversarial probes cover uppercase/private/session fields, Basic/Digest/proxy authorization, cookies, split arguments, inline and positional argv, dash-prefixed opaque values, and safe public-key/author/OAuth exceptions. - Debug CLI integration passes (11 passed, 1 platform-dependent test skipped). - Test-size and test-title gates pass. - `npm run docs` passes with 0 Fern errors and 2 pre-existing Fern warnings; documentation review found no additional pages requiring changes. --- Signed-off-by: sauravdev <saurava@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Introduced centralized CLI stderr logging with `NEMOCLAW_LOG_LEVEL` / `NEMOCLAW_DEBUG`, quiet/debug modes, and timestamped debug output. * Added structured `debugObject` output plus sequence-aware redaction for log-like values. * **Bug Fixes** * Improved `--debug`/`--quiet` interactions with environment precedence and `--` option boundaries; ensured stderr write failures don’t break commands. * **Security** * Strengthened credential redaction for headers/cookies and sensitive CLI flag values, including “fails closed” handling. * **Documentation** * Documented logging environment variables, precedence, and hidden flag behavior. * **Tests** * Expanded coverage for parsing, robustness (complex serialization), redaction correctness, and mock isolation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
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
3 participants
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.
Summary
Introduces a centralized logging facility (
src/lib/cli/logger.ts) with four levels (error < warn < info < debug) written exclusively to stderr, and wires--debugand--quiet/-qflags intoNemoClawCommand.baseFlagsso every command inherits log-level control. This establishes the infrastructure for incrementally migrating the CLI's scatteredconsole.log/console.errorcall sites to leveled logging.Changes
src/lib/cli/logger.ts: singletonLoggerwitherror/warn/info/debugmethods and adebugObjecthelper; all output goes to stderr so stdout stays clean for--jsonconsumers; debug level adds ISO-8601 timestamps.NEMOCLAW_LOG_LEVEL=<level>,NEMOCLAW_DEBUG=1, orDEBUG=*nemoclaw*.src/lib/cli/nemoclaw-oclif-command.ts:baseFlagsgains--debugand--quiet/-q;init()configures the singleton before any command'srun()executes.src/lib/cli/logger.test.ts: 10 unit tests covering level transitions, env-var pickup, quiet mode, anddebugObjectgating.console.*call sites are untouched; migration tolog.*is intended as incremental follow-up work per module.Type of Change
Quality Gates
--help; a docs page makes sense once call-site migration begins and behavior becomes visible.Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Verification detail:
npm run build:clipasses;npx vitest run src/lib/cli/logger.test.ts(10/10) andsrc/lib/cli/nemoclaw-oclif-command.test.ts(4/4) pass.Signed-off-by: sauravdev saurava@nvidia.com
Summary by CodeRabbit
New Features
--debugand--quietoptions for more control over command output.Bug Fixes
Documentation