Repository navigation
chore(tooling): switch linting to biome - #2862
Merged
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMigrate repository tooling from Prettier/ESLint to Biome, remove ESLint flat configs and custom rule, add a TypeScript AST checker for direct credential env reads, update pre-commit hooks/Makefile/package scripts, remove many inline ESLint suppressions, and adjust/add tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Git as Git / Commit
participant Pre as pre-commit hooks
participant Biome as Biome (format & lint)
participant Guard as TS Checker (check-direct-credential-env)
participant FS as File System
Dev->>Git: git commit (staged files)
Git->>Pre: run pre-commit hooks
Pre->>Biome: npx biome format --write (formatter)
Pre->>Biome: npx biome lint (linter)
Pre->>Guard: run local guard if matched files changed
Guard->>FS: read matched files (e.g. src/lib/onboard.ts)
Guard->>Guard: parse TS/JS AST and locate direct env reads
Guard-->>Pre: return violations or success
Pre->>Dev: allow commit or abort with errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 7/10 reviews remaining, refill in 12 minutes and 33 seconds. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
AGENTS.md (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required SPDX header to this Markdown file.
This modified Markdown file is missing the required SPDX HTML comment header.
Suggested patch
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> +<!-- SPDX-License-Identifier: Apache-2.0 --> + # Agent InstructionsAs per coding guidelines:
**/*.{js,ts,tsx,sh,md}must include SPDX headers, and “Markdown uses HTML comments.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 1, Add the required SPDX HTML comment header to the top of AGENTS.md: open the file and insert an HTML comment containing the SPDX license identifier (e.g., the project’s SPDX short-tag) as the very first line so the Markdown file complies with the `**/*.{js,ts,tsx,sh,md}` header rule; ensure it uses an HTML comment format (not plain text) so the Markdown parser ignores it and commit the change.src/lib/onboard.ts (1)
4995-5000:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix credential “missing” detection for dynamic env keys to treat whitespace-only env vars as unset.
Same issue as above:
!process.env[credentialEnv]won’t treat whitespace-only strings as missing. Since this block is responsible for applyingNEMOCLAW_PROVIDER_KEYto provider-specific credential env vars, whitespace-only values should be normalized and treated as unset.🔧 Proposed change
const _providerKeyHint = (process.env.NEMOCLAW_PROVIDER_KEY || "").trim(); // check-direct-credential-env-ignore -- intentional: checking if env is already set before applying NEMOCLAW_PROVIDER_KEY override - if (_providerKeyHint && credentialEnv && !process.env[credentialEnv]) { - process.env[credentialEnv] = _providerKeyHint; - } + if (_providerKeyHint && credentialEnv) { + const existingCredentialKey = normalizeCredentialValue( + process.env[credentialEnv] ?? "", + ); + if (!existingCredentialKey) { + process.env[credentialEnv] = _providerKeyHint; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 4995 - 5000, The check that decides whether to apply NEMOCLAW_PROVIDER_KEY uses !process.env[credentialEnv], which treats whitespace-only values as present; change the condition to treat whitespace-only env vars as unset by trimming the existing env value before testing (e.g. compute existing = (process.env[credentialEnv] || "").trim() and use !existing in the if). Update the if that references _providerKeyHint, credentialEnv and process.env[credentialEnv] so it checks trimmed existing value instead of raw process.env to ensure whitespace-only values are overridden by _providerKeyHint.
🧹 Nitpick comments (3)
src/lib/usage-notice.ts (2)
214-226: ⚡ Quick winType
require("./credentials").promptexplicitly to make the CJS boundary safe.Right now
askis inferred frompromptFn || require("./credentials").prompt, which can be “any-ish” across the CJS/TS boundary. Even if it works today, this makes the contract fragile. Consider explicitly casting the required prompt toPromptFnand using??to express the null-default intent.Suggested change
// credentials is still CJS - const ask = promptFn || require("./credentials").prompt; + const ask: PromptFn = + promptFn ?? + (require("./credentials") as { prompt: PromptFn }).prompt; const answer = String(await ask(` ${config.interactivePrompt}`)) .trim() .toLowerCase();This keeps the runtime behavior identical while making the type contract explicit at compile time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/usage-notice.ts` around lines 214 - 226, The binding for ask is currently inferred from promptFn || require("./credentials").prompt which crosses a CJS/TS boundary; explicitly cast the required prompt to the expected PromptFn type and use the nullish-coalescing operator to express the default intent. Locate the ask assignment in usage-notice.ts (the ask variable, promptFn, and require("./credentials").prompt) and change it so the CJS import is asserted/typed as PromptFn and use "promptFn ?? (require(...).prompt as PromptFn)" so the type contract is explicit while keeping runtime behavior identical.
214-222: ⚡ Quick winPreserve UX on prompt failures (add try/catch around
ask).If
require("./credentials").prompt(or the providedpromptFn) rejects/throws,ensureUsageNoticeConsentwill throw and skip the normal “Installation cancelled” messaging + boolean return path. Wrapping the prompt call lets you fail closed with a consistentfalseoutcome.Suggested change
- const answer = String(await ask(` ${config.interactivePrompt}`)) - .trim() - .toLowerCase(); + let answer: string; + try { + answer = String(await ask(` ${config.interactivePrompt}`)) + .trim() + .toLowerCase(); + } catch { + writeLine(" Installation cancelled"); + return false; + } if (answer !== "yes") { writeLine(" Installation cancelled"); return false; }This is especially relevant since this is an onboarding/consent flow where reliability matters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/usage-notice.ts` around lines 214 - 222, The prompt call (ask) can throw/reject and currently would bubble out, skipping the normal cancellation flow; wrap the await ask(...) invocation in a try/catch inside ensureUsageNoticeConsent (or the function containing this snippet) so any thrown error is caught, call writeLine(" Installation cancelled") and return false on catch, preserving the same UX path for failures; reference the promptFn / require("./credentials").prompt and the local variable ask and ensure you still trim().toLowerCase() only when the call succeeds.test/no-direct-credential-env.test.ts (1)
56-80: ⚡ Quick winAdd a regression case where suppression token appears in a string literal.
Right now there’s no test proving that only actual comments can suppress. Adding this prevents future bypass regressions in the guard logic.
✅ Suggested test addition
it.each([ @@ // Dynamic read with credential-containing variable name ["if (!process.env[credentialEnv]) {}", "[credentialEnv]"], ["const x = process.env[resolvedCredentialEnv];", "[resolvedCredentialEnv]"], + // Suppression token inside non-comment text must NOT suppress + [ + "const marker = 'no-direct-credential-env';\nconst key = process.env.NVIDIA_API_KEY;", + "NVIDIA_API_KEY", + ], ])("flags %s", (code, key) => { expect(findDirectCredentialEnvReads(code)).toMatchObject([{ key }]); });Based on learnings: Security-sensitive code paths (credential sanitization, SSRF validation, Docker capability drops, process limits) require extra test coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/no-direct-credential-env.test.ts` around lines 56 - 80, Add a regression test in test/no-direct-credential-env.test.ts within the existing it.each cases for findDirectCredentialEnvReads that ensures a suppression token appearing inside a string literal does NOT suppress detection; specifically add a case where a string containing the suppression marker is present alongside a direct process.env read and assert findDirectCredentialEnvReads still reports the credential key (e.g., OPENAI_API_KEY), referencing the existing test harness (the it.each block and the findDirectCredentialEnvReads helper) so only actual comment tokens (not string literals) suppress the rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 99-115: The Biome pre-commit hooks (ids: biome-format and
biome-lint-fix) currently only match TypeScript under nemoclaw/src and skip
root-level CLI TypeScript files; update the hooks' files regex to also include
root CLI TS (e.g., add ^src/.*\.ts$ or include src/.*\.ts in the existing
alternations) so that root-level src/**/*.ts files are covered by both
biome-format and biome-lint-fix; ensure both the "files" entries for
biome-format and biome-lint-fix are changed consistently to include the root src
pattern.
In `@biome.json`:
- Around line 10-23: The Biome include list currently misses root CLI TypeScript
files (e.g., src/lib/onboard.ts), so files under src/**/*.ts are not
linted/formatted; update the "includes" array in biome.json to add the pattern
"src/**/*.ts" (or a similar root-src glob) alongside existing entries so Biome
will pick up and process those CLI TypeScript files such as src/lib/onboard.ts.
In `@scripts/check-direct-credential-env.ts`:
- Around line 197-203: The suppression detection in hasSuppressionComment
currently tests raw line text (using sourceFile.getFullText().split...) which
allows suppression tokens inside strings; change it to only inspect actual
comment text by using TypeScript's comment APIs (e.g.,
ts.forEachLeadingCommentRange or ts.getLeadingCommentRanges) on the node (and
optionally the previous token/node) and test SUPPRESSION_PATTERN against the
comment text; update hasSuppressionComment to obtain the node start
(node.getStart(sourceFile)) and scan leading comment ranges for that position
(and the preceding token's trailing comments if you want to support
previous-line comments), returning true only if a comment's text matches
SUPPRESSION_PATTERN.
In `@src/lib/onboard.ts`:
- Around line 4956-4960: The guard currently checks !process.env.NVIDIA_API_KEY
which treats an all-whitespace value as present; change the condition to treat
whitespace-only as unset by trimming the existing env var before checking (e.g.,
use !( (process.env.NVIDIA_API_KEY || "").trim() ) ). Keep using the
already-trimmed _nvProviderKey and only assign it when the trimmed
NVIDIA_API_KEY is empty, updating the block that sets process.env.NVIDIA_API_KEY
from _nvProviderKey.
---
Outside diff comments:
In `@AGENTS.md`:
- Line 1: Add the required SPDX HTML comment header to the top of AGENTS.md:
open the file and insert an HTML comment containing the SPDX license identifier
(e.g., the project’s SPDX short-tag) as the very first line so the Markdown file
complies with the `**/*.{js,ts,tsx,sh,md}` header rule; ensure it uses an HTML
comment format (not plain text) so the Markdown parser ignores it and commit the
change.
In `@src/lib/onboard.ts`:
- Around line 4995-5000: The check that decides whether to apply
NEMOCLAW_PROVIDER_KEY uses !process.env[credentialEnv], which treats
whitespace-only values as present; change the condition to treat whitespace-only
env vars as unset by trimming the existing env value before testing (e.g.
compute existing = (process.env[credentialEnv] || "").trim() and use !existing
in the if). Update the if that references _providerKeyHint, credentialEnv and
process.env[credentialEnv] so it checks trimmed existing value instead of raw
process.env to ensure whitespace-only values are overridden by _providerKeyHint.
---
Nitpick comments:
In `@src/lib/usage-notice.ts`:
- Around line 214-226: The binding for ask is currently inferred from promptFn
|| require("./credentials").prompt which crosses a CJS/TS boundary; explicitly
cast the required prompt to the expected PromptFn type and use the
nullish-coalescing operator to express the default intent. Locate the ask
assignment in usage-notice.ts (the ask variable, promptFn, and
require("./credentials").prompt) and change it so the CJS import is
asserted/typed as PromptFn and use "promptFn ?? (require(...).prompt as
PromptFn)" so the type contract is explicit while keeping runtime behavior
identical.
- Around line 214-222: The prompt call (ask) can throw/reject and currently
would bubble out, skipping the normal cancellation flow; wrap the await ask(...)
invocation in a try/catch inside ensureUsageNoticeConsent (or the function
containing this snippet) so any thrown error is caught, call writeLine("
Installation cancelled") and return false on catch, preserving the same UX path
for failures; reference the promptFn / require("./credentials").prompt and the
local variable ask and ensure you still trim().toLowerCase() only when the call
succeeds.
In `@test/no-direct-credential-env.test.ts`:
- Around line 56-80: Add a regression test in
test/no-direct-credential-env.test.ts within the existing it.each cases for
findDirectCredentialEnvReads that ensures a suppression token appearing inside a
string literal does NOT suppress detection; specifically add a case where a
string containing the suppression marker is present alongside a direct
process.env read and assert findDirectCredentialEnvReads still reports the
credential key (e.g., OPENAI_API_KEY), referencing the existing test harness
(the it.each block and the findDirectCredentialEnvReads helper) so only actual
comment tokens (not string literals) suppress the rule.
🪄 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: c94c99ed-5033-4f18-b3ef-36804943aa53
⛔ Files ignored due to path filters (2)
nemoclaw/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
.pre-commit-config.yaml.prettierrcAGENTS.mdMakefilebiome.jsoncommitlint.config.jseslint-rules/no-direct-credential-env.jseslint.config.mjsnemoclaw/.prettierrcnemoclaw/eslint.config.mjsnemoclaw/package.jsonnemoclaw/src/blueprint/private-networks.test.tsnemoclaw/src/blueprint/runner.test.tsnemoclaw/src/commands/migration-state.test.tsnemoclaw/src/lib/ports.test.tsnemoclaw/src/register.test.tspackage.jsonscripts/check-direct-credential-env.tssrc/lib/agent-defs.tssrc/lib/credentials.tssrc/lib/gateway-state.tssrc/lib/http-probe.tssrc/lib/inference-config.tssrc/lib/local-inference.tssrc/lib/model-prompts.tssrc/lib/nim.tssrc/lib/onboard-inference-probes.tssrc/lib/onboard-session.tssrc/lib/onboard.tssrc/lib/openshell.tssrc/lib/preflight.tssrc/lib/provider-models.tssrc/lib/runtime-recovery.tssrc/lib/skill-install.tssrc/lib/usage-notice.tssrc/nemoclaw.tstest/http-proxy-fix-e2e.test.tstest/http-proxy-fix-rewrite.test.tstest/no-direct-credential-env.test.tstest/onboard.test.tstest/validate-configs-dangerous-hosts.test.ts
💤 Files with no reviewable changes (27)
- src/lib/onboard-session.ts
- .prettierrc
- src/lib/gateway-state.ts
- src/lib/onboard-inference-probes.ts
- nemoclaw/.prettierrc
- src/lib/credentials.ts
- src/lib/runtime-recovery.ts
- nemoclaw/src/register.test.ts
- src/lib/agent-defs.ts
- src/lib/model-prompts.ts
- src/lib/provider-models.ts
- nemoclaw/src/lib/ports.test.ts
- src/lib/http-probe.ts
- src/lib/local-inference.ts
- src/lib/preflight.ts
- test/validate-configs-dangerous-hosts.test.ts
- test/http-proxy-fix-e2e.test.ts
- src/lib/inference-config.ts
- src/nemoclaw.ts
- nemoclaw/eslint.config.mjs
- test/onboard.test.ts
- eslint.config.mjs
- eslint-rules/no-direct-credential-env.js
- test/http-proxy-fix-rewrite.test.ts
- src/lib/openshell.ts
- src/lib/skill-install.ts
- src/lib/nim.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
AGENTS.md (2)
108-115: ⚡ Quick winClarify Biome config scope in the JavaScript style bullets.
The TS section says “root Biome config”, but the JS bullet just says “Biome config in
biome.json”. Consider making it explicit that this is the repo-root config (and that it applies to formatting/linting as described elsewhere), e.g.:🛠️ Suggested tweak
- - Biome config in `biome.json` + - Biome config at repo root (`./biome.json`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 108 - 115, Update the JavaScript style bullet about Biome to explicitly state it's the repo-root Biome config and that it governs formatting/linting as referenced elsewhere; specifically change the "Biome config in `biome.json`" bullet to something like "Repo-root Biome config in `biome.json` (applies to formatting and linting as described elsewhere)" so it matches the TS section's "root Biome config" wording and clarifies scope for readers.
169-175: 💤 Low valueUse inline-code formatting consistently for
node_modules.In the “Gotchas” bullet,
node_modulesis currently not wrapped in backticks; wrapping it improves scanability and matches the rest of the conventions in this doc:🛠️ Suggested tweak
-- - The `nemoclaw/` subdirectory has its own `package.json` and `node_modules`, while sharing the root Biome config — it's a separate npm project +- - The `nemoclaw/` subdirectory has its own `package.json` and `node_modules`, while sharing the root Biome config — it's a separate npm project(If the intent is that both
package.jsonandnode_modulesshould be backticked, ensure both are consistently formatted in the actual file.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 169 - 175, The "Gotchas" bullet is inconsistent: `node_modules` is not inline-code formatted like the other terms; update the text in the bullet about the `nemoclaw/` subdirectory so both `package.json` and `node_modules` are wrapped in backticks (e.g., `package.json` and `node_modules`) to match the document's convention and improve scanability—edit the line containing "nemoclaw/ subdirectory has its own package.json and node_modules" accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@AGENTS.md`:
- Around line 108-115: Update the JavaScript style bullet about Biome to
explicitly state it's the repo-root Biome config and that it governs
formatting/linting as referenced elsewhere; specifically change the "Biome
config in `biome.json`" bullet to something like "Repo-root Biome config in
`biome.json` (applies to formatting and linting as described elsewhere)" so it
matches the TS section's "root Biome config" wording and clarifies scope for
readers.
- Around line 169-175: The "Gotchas" bullet is inconsistent: `node_modules` is
not inline-code formatted like the other terms; update the text in the bullet
about the `nemoclaw/` subdirectory so both `package.json` and `node_modules` are
wrapped in backticks (e.g., `package.json` and `node_modules`) to match the
document's convention and improve scanability—edit the line containing
"nemoclaw/ subdirectory has its own package.json and node_modules" accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ba1f18b8-f794-48af-b307-e7abb6649174
📒 Files selected for processing (8)
.pre-commit-config.yamlAGENTS.mdbiome.jsonscripts/check-direct-credential-env.tssrc/lib/onboard.tssrc/lib/usage-notice.tstest/no-direct-credential-env.test.tstest/usage-notice.test.ts
✅ Files skipped from review due to trivial changes (2)
- biome.json
- scripts/check-direct-credential-env.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/usage-notice.ts
- test/no-direct-credential-env.test.ts
- .pre-commit-config.yaml
- src/lib/onboard.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard.ts (1)
5199-5209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply
NEMOCLAW_PROVIDER_KEYbefore hydrating saved credentials.Line 5199 hydrates the provider-specific credential into
process.envbefore the new fallback checks on Lines 5207 and 5248 run. So when a key is already saved locally,existingNvidiaKey/existingCredentialKeyis never empty and the explicitNEMOCLAW_PROVIDER_KEYoverride is ignored. That still lets stored credentials win over an explicit env override.🔧 Suggested fix
- hydrateCredentialEnv(credentialEnv); + const providerKeyOverride = normalizeCredentialValue( + process.env.NEMOCLAW_PROVIDER_KEY ?? "", + ); + if (providerKeyOverride && credentialEnv) { + // check-direct-credential-env-ignore -- intentional: explicit alias should win + // unless the provider-specific env var was already exported. + const existingCredentialKey = normalizeCredentialValue(process.env[credentialEnv] ?? ""); + if (!existingCredentialKey) { + process.env[credentialEnv] = providerKeyOverride; + } + } + hydrateCredentialEnv(credentialEnv); if (selected.key === "build") { - // Allow NEMOCLAW_PROVIDER_KEY as a fallback for NVIDIA_API_KEY. - // Check raw process.env first — NEMOCLAW_PROVIDER_KEY is a user-facing - // override that should take precedence before resolving from credentials.json. - const _nvProviderKey = (process.env.NEMOCLAW_PROVIDER_KEY || "").trim(); - // check-direct-credential-env-ignore -- intentional: checking if env is already set before applying NEMOCLAW_PROVIDER_KEY override - const existingNvidiaKey = normalizeCredentialValue(process.env.NVIDIA_API_KEY ?? ""); - if (_nvProviderKey && !existingNvidiaKey) { - process.env.NVIDIA_API_KEY = _nvProviderKey; - } if (isNonInteractive()) { const resolvedNvidiaKey = resolveProviderCredential("NVIDIA_API_KEY"); if (!resolvedNvidiaKey) { console.error( " NVIDIA_API_KEY (or NEMOCLAW_PROVIDER_KEY) is required for NVIDIA Endpoints in non-interactive mode.", @@ } else { - // NEMOCLAW_PROVIDER_KEY is a universal alias: if the specific credential env - // isn't already set, use NEMOCLAW_PROVIDER_KEY as the API key for this provider. - // Check raw process.env — the override must apply before resolving from credentials.json. - const _providerKeyHint = (process.env.NEMOCLAW_PROVIDER_KEY || "").trim(); - if (_providerKeyHint && credentialEnv) { - // check-direct-credential-env-ignore -- intentional: checking if env is already set before applying NEMOCLAW_PROVIDER_KEY override - const existingCredentialKey = normalizeCredentialValue(process.env[credentialEnv] ?? ""); - if (!existingCredentialKey) { - process.env[credentialEnv] = _providerKeyHint; - } - } - if (isNonInteractive()) { if (!resolveProviderCredential(credentialEnv)) { console.error( ` ${credentialEnv} (or NEMOCLAW_PROVIDER_KEY) is required for ${remoteConfig.label} in non-interactive mode.`,Based on learnings: credential hydration here must preserve fill-only-if-missing semantics so explicitly provided environment credentials are not shadowed by saved credentials.
Also applies to: 5246-5250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 5199 - 5209, The saved-credentials hydration currently runs before honoring the explicit NEMOCLAW_PROVIDER_KEY override, so hydrateCredentialEnv(...) can shadow an explicit env value; fix by applying the NEMOCLAW_PROVIDER_KEY check and setting process.env.NVIDIA_API_KEY (using normalizeCredentialValue logic) before calling hydrateCredentialEnv, or alternatively change hydrateCredentialEnv to be strictly fill-only-if-missing so it never overwrites an already-set process.env variable; apply the same ordering/guard for the other provider check that uses existingCredentialKey (the block around lines referencing existingCredentialKey / normalizeCredentialValue).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 5199-5209: The saved-credentials hydration currently runs before
honoring the explicit NEMOCLAW_PROVIDER_KEY override, so
hydrateCredentialEnv(...) can shadow an explicit env value; fix by applying the
NEMOCLAW_PROVIDER_KEY check and setting process.env.NVIDIA_API_KEY (using
normalizeCredentialValue logic) before calling hydrateCredentialEnv, or
alternatively change hydrateCredentialEnv to be strictly fill-only-if-missing so
it never overwrites an already-set process.env variable; apply the same
ordering/guard for the other provider check that uses existingCredentialKey (the
block around lines referencing existingCredentialKey /
normalizeCredentialValue).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e7964960-22e2-4022-b534-78f1a8319603
📒 Files selected for processing (6)
.pre-commit-config.yamlnemoclaw/src/blueprint/runner.test.tspackage.jsonsrc/lib/onboard.tssrc/nemoclaw.tstest/onboard.test.ts
💤 Files with no reviewable changes (2)
- test/onboard.test.ts
- src/nemoclaw.ts
✅ Files skipped from review due to trivial changes (1)
- nemoclaw/src/blueprint/runner.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- .pre-commit-config.yaml
brandonpelfrey
approved these changes
May 1, 2026
5 of 12 tasks
This was referenced Jul 4, 2026
9 of 21 tasks
cjagwani
added a commit
that referenced
this pull request
Aug 21, 2026
<!-- markdownlint-disable MD041 --> ## Summary Restore the PR exact OpenClaw MCP qualification path end to end. The job now onboards the exact managed image, preserves a classified MCP credential environment alongside gateway-only inference credentials, and performs authenticated discovery with the live revision-scoped OpenShell placeholder. ## Root cause The same two five-phase qualification jobs were genuinely green before the dependency upgrade. The last clean pre-upgrade pair, [run 32332905722](https://github.com/NVIDIA/NemoClaw/actions/runs/32332905722) at `ab5db717b`, installed OpenShell 0.0.101 and passed without a waiver. [NemoClaw PR #9192](#9192) then merged at 2026-08-19 22:29 PDT and upgraded OpenShell directly from 0.0.101 to 0.0.106. The first exact pair based on 0.0.106, [run 32338412376](https://github.com/NVIDIA/NemoClaw/actions/runs/32338412376), failed phase 3 in both passes with no successful MCP request. The causal OpenShell change is [OpenShell PR #2510](NVIDIA/OpenShell#2510), merged as `0120535ef`, which introduced endpoint-bound static credential snapshots. A profileless inference provider contributes a static environment key without binding or non-secret classification; the binding-capable supervisor rejects that snapshot as `provider environment contains an unclassified credential key` and revokes the otherwise correctly bound MCP credential too. The failures occurred at five successive boundaries: 1. The PR MCP child environment dropped the managed-image catalog and activation inputs. Onboarding therefore built a Dockerfile image instead of qualifying the candidate image. 2. With the exact image active, OpenShell 0.0.106 emitted the legacy `openai` provider credential without selected-profile binding metadata. The supervisor rejected the provider environment as containing an unclassified credential and atomically withheld the MCP static credential too. 3. After classifying the inference provider with an endpointless profile, the discovery runtime still synthesized `openshell:resolve:env:<KEY>`. OpenShell's bound resolver requires the current live revision-scoped value (`openshell:resolve:env:v<revision>_<KEY>`), so discovery received HTTP 500 before any request reached the fake MCP server. 4. Once diagnostic discovery used the live revision and successfully listed `fake_echo` and `fake_status`, the managed mcporter config still persisted the canonical unversioned placeholder. The direct agent-adapter proof therefore received HTTP 500 even though the diagnostic path was green. 5. After both diagnostic discovery and direct mcporter discovery passed, the test entered a separate trusted-private DNS-rebinding fixture. That fixture rewrote `/etc/hosts` on the runner and sandbox, but OpenShell resolves egress in the Docker supervisor namespace. The supervisor never observed the fake hostname mapping, rejected the connection before it reached the server, and the negative-only raw probe had previously passed for the same wrong reason. 6. Once both passes reached the rebuild lifecycle, the 0.0.106 migration's pre-delete `removeGeneratedPolicy()` correctly removed `mcp-bridge-fake`, but the captured policy selection still handed that generated name to inner onboarding and generic policy replay. The first correction normalized the rebuild session, but resumed sandbox creation then overwrote it from the intentionally preserved crash-recovery registry row. Recreate therefore still failed deterministically with `Preset not found: mcp-bridge-fake` before the dedicated MCP restore phase could reattach the provider, generated policy, and adapter. This is the normal host-gateway / one-container-per-sandbox topology. No custom MCP sidecar is involved. ## Changes - Preserve the workflow-owned managed-image catalog, candidate SHA, live qualification flag, and supervisor image across the MCP child-process boundary. - Activate onboarding through `--temp-managed-runtime` and `--temp-managed-runtime-catalog`, then require the sandbox receipt to identify the exact candidate revision. - Import an endpointless, inference-capable `openai` profile before the endpointless MCP profile so OpenShell can classify gateway-only inference credentials without injecting them into workloads. - When `openai` already exists, export it and require the exact gateway-only boundary: `id: openai`, empty credentials/endpoints/binaries, and `inference_capable: true`. Fail closed before MCP policy or provider mutation on export failure, malformed output, or a mismatch. - Make MCP discovery read the fresh process environment and accept only a canonical or revision-scoped OpenShell placeholder for the declared key. Raw, wrong-key, malformed, missing, and injected values fail closed and never enter argv, output, or a request. - Return the bounded live credential revision from the attachment-readiness proof and project that exact revision into managed mcporter configuration. Post-write registration inspection now requires the same readiness-proven revision (`v12` cannot verify as `v11`); canonical status/removal matching remains available only when readiness was canonical. - Qualify every `mcp-bridge-*.ts` change through the PR and main managed-image workflow boundaries so adapter projection changes cannot bypass this live proof. - Scope exact managed-image CI to the topology it actually owns: exact-image onboarding, authenticated public MCP discovery, direct adapter use, endpoint boundaries, credential rotation, restart, and removal. The evidence records `managed-image-discovery`; the job no longer claims trusted-private DNS-rebinding coverage from a runner/sandbox hosts fixture that cannot control the supervisor resolver. Full MCP E2E retains that proof for supervisor-authoritative DNS topologies. - Exclude only the generated policy names already preserved by the MCP rebuild transaction from inner-onboard and generic policy replay. The outer rebuild now carries that normalized selection through an explicit authoritative create intent, so sandbox recreation cannot replace it from the stale source registry row or ambient policy variables. Matching-journal recovery remains a fallback, a journal for another sandbox cannot supply policy state, the crash-recovery registry remains untouched, built-in and operator policy selections remain unchanged, and the dedicated post-rebuild MCP phase remains the sole owner of restoring the provider-bound generated policy and adapter. - Rebuild and pin the reviewed MCP discovery runtime bundle. The `openai` profile is a provisional compatibility path for the pinned 0.0.106 binary, not the intended ownership model. The ownership-free fix is [OpenShell PR #2862](NVIDIA/OpenShell#2862): at the gateway response boundary, remove each static key that lacks binding metadata before sending the snapshot to a binding-capable supervisor. Bound static credentials and valid dynamic credentials remain active, provider resolution stays unchanged, and legacy supervisors retain their existing strip-all behavior. The full 1,415-test server suite passes (1,408 passed, 7 ignored), as do formatting and warning-as-error clippy. After that fix is released and NemoClaw updates its pin, this PR should remove the provisional shared profile and its lifecycle code. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — Senthil explicitly accepted the provisional `openai` profile ownership boundary and approved at `52b2db132`; Ryan's rebuild and exact-revision findings on `75aefaf3b` are addressed by signed commits `a51adb149` and `6c05be2d9`, and the current head awaits re-review. OpenShell PR #2862 remains the ownership-free follow-up. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every pushed commit is signed and DCO-compliant - [x] Normal pre-commit, commit-msg, and pre-push hooks passed - [x] Targeted behavior tests pass for the current change set — the 371-test MCP bridge suite, 217 publication/risk-boundary tests, the current 179-test affected workflow/scope suite, the earlier 149 focused provider, discovery, onboarding, image, build-context, and publication tests, the current 126-test policy/rebuild suite, and the current 98-test adapter/status/crash/restart suite pass; the isolated discovery runtime wire test and typecheck also pass - [x] `npm run typecheck:cli`, `npm run build:cli`, focused Oxlint, formatting, `npm run checks:repository`, and the 32-test growth guard pass - [ ] Applicable broad gate passed — current replacement managed-image run [32468003695](https://github.com/NVIDIA/NemoClaw/actions/runs/32468003695) is pending for signed commit `6c05be2d9`; run [32463784345](https://github.com/NVIDIA/NemoClaw/actions/runs/32463784345) passed the exact-image build and phases 1–3 in both discovery passes, including authenticated `fake_echo`/`fake_status` discovery with `credentialRewriteMatched: true`, then proved that inner sandbox creation still reloaded the stale generated-policy name from the preserved registry; attempt 3 of run [32457422244](https://github.com/NVIDIA/NemoClaw/actions/runs/32457422244) first reproduced that same phase-4 boundary in both passes, run [32454193518](https://github.com/NVIDIA/NemoClaw/actions/runs/32454193518) first exposed rebuild failure, and run [32452343170](https://github.com/NVIDIA/NemoClaw/actions/runs/32452343170) reached live discovery in both passes but was cancelled by a newer push - [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) - [ ] 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) --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Co-authored-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com>
prekshivyas
added a commit
that referenced
this pull request
Sep 11, 2026
## Outcome NemoClaw now performs a focused, full cutover from OpenShell `0.0.106` to exactly `0.0.116`. Active selectors, installers, runtime images, the TypeScript SDK, credential-boundary evidence, and directly affected tests agree on `0.0.116`; static credential consumers accept its stable `s<64 lowercase hex>` handles. The obsolete NemoClaw OpenAI endpointless-profile workaround is removed because NVIDIA/OpenShell#2862 is an ancestor of the exact `0.0.116` tag, while ordinary OpenAI provider support remains intact. ## Reason The earlier dependency upgrade pretrusted OpenShell `0.0.116` artifacts but did not activate them across NemoClaw. OpenShell `0.0.116` introduces stable credential handles and a main-process spec and already includes the upstream static-credential classification repair from NVIDIA/OpenShell#2862. Retaining `0.0.106` selectors or NemoClaw's pre-#2862 OpenAI profile shim would leave the cutover internally inconsistent. ### Related issues Fixes #11229 ## Changes - Pin the supported OpenShell window, installers, workflows, managed runtimes, and SDK dependency to exactly `0.0.116`, using the reviewed release hashes and manifests. - Replace the `0.0.106` child-visible credential evidence with the `0.0.116` manifest and bind it to the current NemoClaw credential consumers. - Accept legacy revision handles where backward parsing remains required, while requiring stable `s[a-f0-9]{64}` handles in exact-`0.0.116` static-credential runtime proofs. - Preserve a stable handle across same-authorization token refreshes and require a new handle after key removal, provider detach/re-add, or another authorization-epoch change. The credential-generation-window and MCP provider readiness tests protect this behavior. - Remove the obsolete automatic OpenAI compatibility-profile import, validation, checked-in profile, and profile-specific fixtures now that `0.0.116` contains NVIDIA/OpenShell#2862. Keep the ordinary OpenAI provider type and the separate endpointless `nemoclaw-mcp-v1` profile. - Fail closed on any recovery or omitted-blueprint path that would select a release other than exactly `0.0.116`; legacy `0.0.106` supervisor selection is removed. - Honor the `0.0.116` main-process spec and perform managed Docker replacement while the OpenShell sandbox is deliberately stopped, then restart through OpenShell authority only after protected bootstrap completion and durable shared-state commit. - Require a post-update wire-level authorization proof before registering an MCP adapter when OpenShell retains the same stable credential handle. - Compare the external gateway's complete release token exactly, keep child supervision below the enclosing sandbox timeout, and preserve credential redaction in messaging diagnostics. - Rebuild the reviewed MCP tool-discovery bundles and update directly affected documentation and test fixtures. ## Verification - Post-review OpenAI-removal and merged-main focused matrix — 360 tests across 20 files passed. - `npm run typecheck:cli` — passed. - Live E2E assertion census regenerated from the merged tree; `npm run e2e:assertions:check` passed with 1,795 direct expect calls across 86 files. - Managed-bootstrap and provider-profile suite — 314 tests across 18 files passed. - Earlier full `npm run validate:pr` — passed, including the strict source build, plugin/JS/CLI typechecks, formatting, lint, schemas, repository checks, E2E semantic phases, source-shape budget, growth guardrails, and commit validation. Final exact-head validation will run after the required Pi receipt refresh. - Focused OpenShell/MCP/stable-credential Vitest suite — 206 tests passed. - Review-focused activation suite — 97 tests across 7 files passed. - Base-trust transition and installer suite — 111 tests across 4 files passed. - `npm run docs` — passed with 0 errors. - `npm --prefix tools/mcp-tool-discovery-runtime run bundle:reviewed:check` — passed. - `npm --prefix tools/mcp-tool-discovery-runtime test` — passed. - `npm --prefix tools/mcp-tool-discovery-runtime run typecheck` — passed. - `npx vitest run --project installer-integration test/installer-integration/install-hermes-portable-active.test.ts --testTimeout=120000` — 3 tests passed against the committed manifest. - Base-controlled SDK and installer authorization landed in #11253. The final exact supervisor template authority landed separately in #11292 after independent maintainer approval and exact candidate verification. - The diff contains no secrets, API keys, or raw credentials. ## Review notes This PR changes contributor-sensitive workflow, installer, agent-runtime, sandbox, credential, messaging, and E2E enforcement paths. Review covered the full cutover diff, current-main conflict resolution, base-trust separation, and the focused checks listed above. Removing the obsolete checked-in OpenAI profile changes a Pi managed-image input. Exact-head CI must publish the AMD64 and ARM64 Pi candidates from one cohort; the checked-in qualification receipts will then be refreshed from those authoritative contracts before merge. No receipt is synthesized locally. --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for stable credential handles that persist across refreshes and provider updates. * Improved sandbox lifecycle recovery, restart, rollback, and process-command handling. * Managed images now default to the non-root `sandbox` user. * **Security & Reliability** * OpenShell installations now require the exact stable `0.0.116` release with verified artifacts. * Development builds and channels are rejected. * Credential authorization is revalidated before preserving unchanged handles. * **Documentation** * Updated setup, troubleshooting, security, and lifecycle guidance for OpenShell `0.0.116`. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.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
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
Switches JavaScript and TypeScript linting/formatting from ESLint and Prettier to Biome. Keeps the credential environment-variable security check by replacing the custom ESLint rule with a standalone TypeScript guard wired into npm scripts and prek.
Changes
biome.jsonand update root/plugin npm scripts plusMakefiletargets to use Biome.scripts/check-direct-credential-env.tsand update tests to preserve the direct credential env guard.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit