test: backfill mockable coverage for live-only behavior + guard against it - #6086
Merged
Conversation
The two regressions #6065 fixed (NEMOCLAW_MODEL_OVERRIDE overwritten by gateway reconcile; guard-chain recovery warning not reaching the gateway log) were only caught by live E2E targets (runtime-overrides, issue-2478-crash-loop-recovery) that do not run on PR CI. Both behaviors are cheaply verifiable with mocked shell-units, so pin them in the PR gate: - reconcile: an explicit override survives a divergent gateway model and the stale in-file fallback; normal drift-correction still runs when the override is unset (fences the early return itself). - guard recovery: the restore warning is mirrored into _NEMOCLAW_GATEWAY_LOG as well as stderr, and stays silent when the chain is already complete. Test-only; no production code change. SKIP=test-cli: the cli+integration vitest hook trips on pre-existing macOS bash 3.2 `set -u` empty-array failures in nemoclaw-start.sh unit harnesses (green on CI bash 5.x); the new tests pass and stub those code paths. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR expands test coverage, extracts reusable E2E helpers, hardens shell/runtime handling for empty-array cases, adds a live E2E unit-block guard, and excludes runtime preload tests from TypeScript compilation. ChangesNemoclaw start and sandbox coverage
Live E2E unit-block check
WhatsApp QR compact preload
Shared live E2E classifiers
Hermes secret-boundary hardening
Ollama auth proxy coverage
OpenClaw policy behavior
Runtime preload compilation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 4 items to resolve/justify, 0 in-scope improvements
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
node_options_has_require iterated "${tokens[@]}" on an empty array, which
trips `set -u` on bash 3.2 (macOS default) with "unbound variable" — this
made the nemoclaw-start.sh unit harnesses fail locally even though CI
(bash 5.x) was green. Guard the empty case, and apply the codebase's
existing "${arr[@]+...}" idiom (already used for RESPAWN_TIMES/_PRUNED) to
the two other reachable-empty iterations (_dynamic_targets, run_prefix).
Behavior-preserving; lets the shell-unit suite run on stock macOS bash.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…st it An audit of the live E2E suite (which does not run on PR CI) found behavior-critical assertions that were only guarded by live targets, the same gap that let the #6065 regressions ship. Backfill the high-priority, security/recovery-class ones as fast mocked units that run on every PR: - ollama-auth-proxy: Bearer enforcement, no /api/tags bypass (#3338), header stripping, non-ASCII auth no-crash (#4820), backend 502. - config get: credential redaction + gateway-key omission (the nvapi- class). - device approval policy: scope-upgrade allowlist gate, gateway-env stripping, and recover-failed rejection paths (#4462). - shields audit JSONL: credentials redacted before persistence. - hermes env secret boundary: value-shape (not key-name) discriminator accepts openshell:resolve refs and rejects raw secrets without echoing (added to the dedicated hardening suite). - dashboard bind: NEMOCLAW_DASHBOARD_BIND opt-in incl. negative cases (#3259). - whatsapp compact QR: package shape-detection + terminal-only small (#4522). Guard the recurrence: scripts/checks/no-unit-blocks-in-live-e2e.ts bans the vitest it(...) primitive inside test/e2e/live/** (those blocks never run on PR CI). Relocate the two existing offenders — the skill-agent and messaging-compatible-endpoint classifier blocks — into importable test/e2e/support modules with PR-collected unit tests; the live tests import them unchanged. whatsapp-qr-compact.ts: minimal behavior-preserving refactor to export the pure shape-detection/patch helpers (the preload still auto-installs on require); tsconfig.runtime-preloads.json excludes the new co-located test from the shipped preload build. SKIP=test-cli: the full cli+integration vitest hook trips on pre-existing macOS bash 3.2 failures in untouched shell-harness suites (select/set -u); CI runs bash 5.x green. Every new/changed file here was verified green individually, and the checks registry + budget + gitleaks + typecheck pass. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
![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 (1)
src/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.ts (1)
133-169: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExtract the
Module._loadrouting decision into an exported pure function for testability.Only the shape-detect/patch helpers are exported; the actual request→loaded routing logic inside
installWhatsappQrCompactHook(lines 159-166) is neither exported nor independently testable. As a result, the companion test file has to hand-copy this exactifchain to exercise "path-segment matching" behavior instead of calling the real code — see the corresponding comment inwhatsapp-qr-compact.test.ts(this is also what triggered the CI codebase-growth guardrail failure on that test file). Extracting the branch as a named export lets tests call the real routing logic directly and removes the duplicated conditionals from the test.♻️ Suggested extraction
+// Pure routing decision extracted so callers (and tests) can exercise the +// exact same logic the installed hook uses, without re-implementing it. +export function resolvePatchedModule(request: unknown, loaded: unknown) { + if (typeof request === "string" && request.indexOf("qrcode") !== -1) { + try { + if (isQrcodePackage(loaded)) return patchQrcode(loaded); + if (isQrcodeTerminalPackage(loaded)) return patchQrcodeTerminal(loaded); + } catch (_e) { + return loaded; + } + } + return loaded; +} + -export { hasOwn, isQrcodePackage, isQrcodeTerminalPackage, patchQrcode, patchQrcodeTerminal }; +export { + hasOwn, + isQrcodePackage, + isQrcodeTerminalPackage, + patchQrcode, + patchQrcodeTerminal, + resolvePatchedModule, +};Module._load = function (request, _parent, _isMain) { var loaded = origLoad.apply(this, arguments); - // Cheap path filter: only inspect modules whose request mentions qrcode. - // `import("qrcode")` arrives here as the resolved absolute path - // (…/qrcode/lib/index.js), so match on the path segment too, not just the - // bare specifier. - if (typeof request === "string" && request.indexOf("qrcode") !== -1) { - try { - if (isQrcodePackage(loaded)) return patchQrcode(loaded); - if (isQrcodeTerminalPackage(loaded)) return patchQrcodeTerminal(loaded); - } catch (_e) { - return loaded; - } - } - return loaded; + return resolvePatchedModule(request, loaded); };🤖 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/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.ts` around lines 133 - 169, The Module._load request-to-loaded routing inside installWhatsappQrCompactHook is duplicated in tests and not independently testable. Extract that request matching and patch-selection branch into a new exported pure function, then have installWhatsappQrCompactHook call it from the Module._load wrapper. Update the companion test to invoke the exported routing function directly instead of hand-copying the if chain, while keeping isQrcodePackage, isQrcodeTerminalPackage, patchQrcode, and patchQrcodeTerminal as the underlying helpers.Source: Pipeline failures
🧹 Nitpick comments (5)
test/ollama-auth-proxy-handler.test.ts (1)
59-68: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePort-release/rebind window can flake under CI port churn.
freePort()closes the probe socket beforestartProxy()binds to the same port; another process could grab it in between under parallel CI load, causing an intermittent proxy-start failure or wrong process binding the port.🤖 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 `@test/ollama-auth-proxy-handler.test.ts` around lines 59 - 68, The current freePort() helper in the test can race because it closes the probe server before startProxy() binds, leaving a window where another process may claim the port. Update the test setup so the proxy binds to the discovered port without a release/rebind gap, or otherwise keep the probe reserved until the proxy is ready. Use the freePort() and startProxy() helpers in test/ollama-auth-proxy-handler.test.ts to locate and adjust the port allocation flow.src/lib/shields/audit-format.test.ts (1)
131-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame env save/restore-via-if pattern as
dashboard-access.test.ts.This is the identical
if (saved === undefined) delete ... else process.env.X = savedpattern that trips thetest-conditionals:scangrowth guardrail in the siblingdashboard-access.test.tsfile in this cohort. Consider switching tovi.stubEnv("HOME", homeDir)/vi.unstubAllEnvs()here too, both to avoid the same conditional-growth risk and to de-duplicate this boilerplate across the two files.🤖 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/shields/audit-format.test.ts` around lines 131 - 144, The HOME env setup/restore in the test hooks repeats the same save/restore conditional pattern that should be removed. Update the `beforeEach`/`afterEach` logic in `audit-format.test.ts` to use `vi.stubEnv("HOME", homeDir)` and `vi.unstubAllEnvs()` instead of manually saving `savedHome` and branching on restore, matching the approach used in `dashboard-access.test.ts` and avoiding the conditional boilerplate in these hooks.test/openclaw-device-approval-policy.test.ts (1)
49-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated Python bootstrap boilerplate across three helper functions.
callDecision,callGatewayEnv, andrunRecovery(line 26 onward, not shown in this segment) each embed the sameimportlib.util.spec_from_file_location/module_from_spec/exec_modulebootstrap. Consider factoring the common module-load prologue into a shared template string interpolated with the call-specific tail, to avoid drift if the loading mechanism ever changes.🤖 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 `@test/openclaw-device-approval-policy.test.ts` around lines 49 - 90, The Python bootstrap logic is duplicated across callDecision, callGatewayEnv, and runRecovery, so factor the shared importlib.util module-loading prologue into one reusable template or helper string. Keep the call-specific tail separate for approval_request_decision, gateway_approval_env, and the recovery path, and have all three helpers build on the same shared loader so future changes to module loading stay consistent.test/hermes-env-secret-boundary-hardening.test.ts (1)
76-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew helper duplicates the runtime-boundary harness already in
test/hermes-start.test.ts.
runRuntimeEnvValidationis nearly identical torunHermesRuntimeEnvSecretBoundaryintest/hermes-start.test.ts(temp dir setup, script generation,_HERMES_BOUNDARY_TIMEOUTno-op, spawnSync options). This PR already needed to apply the sameset -u/bash-3.2 fix in both places — a sign the duplication is starting to drift. Consider extracting a shared harness helper (e.g. undertest/support/) that both files import, so future fixes only need to land once.🤖 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 `@test/hermes-env-secret-boundary-hardening.test.ts` around lines 76 - 111, The runtime-boundary harness logic in runRuntimeEnvValidation is duplicated from runHermesRuntimeEnvSecretBoundary and will drift again; extract the shared temp-script/spawnSync setup into a common helper under test/support/ and have both tests call it. Keep the existing set -u, _HERMES_BOUNDARY_TIMEOUT no-op, and validator wiring behavior intact when moving the shared logic.src/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.ts (1)
84-131: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueStatic-analysis prototype-pollution flag is likely a false positive here.
for...in+hasOwnPropertycopies attacker-controllable keys, butoptshere is always supplied by internal WhatsApp render call sites (never untrusted external input), andmergedis a fresh local object per call — a__proto__key would only affect that one local object, not the sharedObject.prototype. Given the internal-only threat model, a guard is optional defense-in-depth rather than a real fix for an exploitable path.🛡️ Optional defensive guard (applies to both merge loops, lines 94-96 and 121-123)
for (var key in opts) { - if (Object.prototype.hasOwnProperty.call(opts, key)) merged[key] = opts[key]; + if ( + Object.prototype.hasOwnProperty.call(opts, key) && + key !== "__proto__" && + key !== "constructor" && + key !== "prototype" + ) { + merged[key] = opts[key]; + } }🤖 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/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.ts` around lines 84 - 131, The prototype-pollution warning comes from the option-copy loops inside patchQrcode and patchQrcodeTerminal, where merged is built from opts via for...in plus hasOwnProperty. Add a small defensive guard in both merge loops to skip dangerous keys like __proto__, constructor, and prototype while keeping the existing internal behavior unchanged, so the local merged object cannot be poisoned even if opts is ever unexpected.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 `@src/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.test.ts`:
- Around line 141-169: This test is duplicating the production Module._load
routing logic instead of exercising the real behavior, so replace the
hand-copied path-segment and package-shape conditionals in
whatsapp-qr-compact.test.ts with a call to the exported resolvePatchedModule
helper from whatsapp-qr-compact.ts once it is available. Keep the test focused
on asserting that installWhatsappQrCompactHook routes an absolute qrcode path
through the actual production logic and returns the patched module, rather than
re-implementing the branching inside the test.
In `@src/lib/onboard/dashboard-access.test.ts`:
- Around line 90-98: Replace the manual process.env snapshot/restore in
dashboard-access.test.ts with Vitest env stubbing. Update the setup around the
NEMOCLAW_DASHBOARD_BIND cases to use vi.stubEnv for each test value, and remove
the savedEnv if/else cleanup in afterEach in favor of vi.unstubAllEnvs. Keep the
changes localized to the dashboard access test helpers so the existing test
cases continue to use the same NEMOCLAW_DASHBOARD_BIND symbol.
In `@test/hermes-env-secret-boundary-hardening.test.ts`:
- Around line 54-58: The no-op prefix used in the hermes boundary harness is too
GNU-specific because the `env` invocation in the `_HERMES_BOUNDARY_TIMEOUT`
setup uses `--`, which breaks on macOS/BSD before the validator runs. Update the
prefix in `hermes-env-secret-boundary-hardening.test.ts` to use `env` without
`--`, keeping the existing boundary harness behavior intact while making it
portable across platforms.
In `@test/openclaw-device-approval-policy.test.ts`:
- Around line 45-47: The tests gated by hasPython3() are currently returning
early and showing as passed when python3 is missing, which hides skipped
coverage. Update the affected Vitest suite in
openclaw-device-approval-policy.test.ts to use skip semantics such as
describe.skipIf or it.skipIf around the python-dependent cases instead of
returning from each test. Use hasPython3() as the condition, and apply it
consistently to the suite or each affected test so the reports clearly show
skipped tests rather than false positives.
---
Outside diff comments:
In `@src/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.ts`:
- Around line 133-169: The Module._load request-to-loaded routing inside
installWhatsappQrCompactHook is duplicated in tests and not independently
testable. Extract that request matching and patch-selection branch into a new
exported pure function, then have installWhatsappQrCompactHook call it from the
Module._load wrapper. Update the companion test to invoke the exported routing
function directly instead of hand-copying the if chain, while keeping
isQrcodePackage, isQrcodeTerminalPackage, patchQrcode, and patchQrcodeTerminal
as the underlying helpers.
---
Nitpick comments:
In `@src/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.ts`:
- Around line 84-131: The prototype-pollution warning comes from the option-copy
loops inside patchQrcode and patchQrcodeTerminal, where merged is built from
opts via for...in plus hasOwnProperty. Add a small defensive guard in both merge
loops to skip dangerous keys like __proto__, constructor, and prototype while
keeping the existing internal behavior unchanged, so the local merged object
cannot be poisoned even if opts is ever unexpected.
In `@src/lib/shields/audit-format.test.ts`:
- Around line 131-144: The HOME env setup/restore in the test hooks repeats the
same save/restore conditional pattern that should be removed. Update the
`beforeEach`/`afterEach` logic in `audit-format.test.ts` to use
`vi.stubEnv("HOME", homeDir)` and `vi.unstubAllEnvs()` instead of manually
saving `savedHome` and branching on restore, matching the approach used in
`dashboard-access.test.ts` and avoiding the conditional boilerplate in these
hooks.
In `@test/hermes-env-secret-boundary-hardening.test.ts`:
- Around line 76-111: The runtime-boundary harness logic in
runRuntimeEnvValidation is duplicated from runHermesRuntimeEnvSecretBoundary and
will drift again; extract the shared temp-script/spawnSync setup into a common
helper under test/support/ and have both tests call it. Keep the existing set
-u, _HERMES_BOUNDARY_TIMEOUT no-op, and validator wiring behavior intact when
moving the shared logic.
In `@test/ollama-auth-proxy-handler.test.ts`:
- Around line 59-68: The current freePort() helper in the test can race because
it closes the probe server before startProxy() binds, leaving a window where
another process may claim the port. Update the test setup so the proxy binds to
the discovered port without a release/rebind gap, or otherwise keep the probe
reserved until the proxy is ready. Use the freePort() and startProxy() helpers
in test/ollama-auth-proxy-handler.test.ts to locate and adjust the port
allocation flow.
In `@test/openclaw-device-approval-policy.test.ts`:
- Around line 49-90: The Python bootstrap logic is duplicated across
callDecision, callGatewayEnv, and runRecovery, so factor the shared
importlib.util module-loading prologue into one reusable template or helper
string. Keep the call-specific tail separate for approval_request_decision,
gateway_approval_env, and the recovery path, and have all three helpers build on
the same shared loader so future changes to module loading 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: 4a2394ba-fc26-43d5-8633-4a9daadcdf66
📒 Files selected for processing (20)
scripts/checks/no-unit-blocks-in-live-e2e.tsscripts/checks/run.tsscripts/nemoclaw-start.shsrc/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.test.tssrc/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.tssrc/lib/onboard/dashboard-access.test.tssrc/lib/sandbox/config-get.test.tssrc/lib/shields/audit-format.test.tstest/e2e/live/messaging-compatible-endpoint.test.tstest/e2e/live/skill-agent.test.tstest/e2e/support/messaging-endpoint-classifiers.test.tstest/e2e/support/messaging-endpoint-classifiers.tstest/e2e/support/skill-agent-classifiers.test.tstest/e2e/support/skill-agent-classifiers.tstest/hermes-env-secret-boundary-hardening.test.tstest/hermes-start.test.tstest/no-unit-blocks-in-live-e2e.test.tstest/ollama-auth-proxy-handler.test.tstest/openclaw-device-approval-policy.test.tstsconfig.runtime-preloads.json
✅ Files skipped from review due to trivial changes (3)
- test/no-unit-blocks-in-live-e2e.test.ts
- test/e2e/support/messaging-endpoint-classifiers.ts
- scripts/checks/no-unit-blocks-in-live-e2e.ts
Comment on lines
+90
to
+98
| const savedEnv = process.env.NEMOCLAW_DASHBOARD_BIND; | ||
|
|
||
| afterEach(() => { | ||
| if (savedEnv === undefined) { | ||
| delete process.env.NEMOCLAW_DASHBOARD_BIND; | ||
| } else { | ||
| process.env.NEMOCLAW_DASHBOARD_BIND = savedEnv; | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== package vitest version ==\n'
if [ -f package.json ]; then
python3 - <<'PY'
import json
from pathlib import Path
p = Path("package.json")
data = json.loads(p.read_text())
for section in ("dependencies", "devDependencies", "peerDependencies", "optionalDependencies"):
deps = data.get(section, {})
if "vitest" in deps:
print(section, deps["vitest"])
PY
fi
printf '\n== lockfile vitest references ==\n'
rg -n '"vitest"|vitest@' package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null | head -n 80 || true
printf '\n== dashboard-access.test.ts excerpt ==\n'
sed -n '1,220p' src/lib/onboard/dashboard-access.test.tsRepository: NVIDIA/NemoClaw
Length of output: 6238
🌐 Web query:
Vitest 4.1.0 vi.stubEnv vi.unstubAllEnvs API
💡 Result:
In Vitest, vi.stubEnv and vi.unstubAllEnvs are utilities designed to safely modify and manage environment variables within your tests [1][2]. vi.stubEnv(name, value) This function changes the value of an environment variable on both process.env and import.meta.env [1][3]. - It tracks the original value of the variable, allowing you to restore it later [1][2]. - You should use this method instead of direct assignment if you intend to use vi.unstubAllEnvs to reset the state, as direct assignment will not be tracked by Vitest [3]. vi.unstubAllEnvs This function restores all environment variables changed via vi.stubEnv to their original values [1][3]. - When called for the first time, Vitest records the original values of the modified variables and maintains them until this function is called again [1][2]. Automatic Reset You can configure Vitest to automatically call vi.unstubAllEnvs before every test by enabling the unstubEnvs option in your Vitest configuration file [4][5]: // vitest.config.ts export default defineConfig({ test: { unstubEnvs: true, }, }) Warning: Enabling unstubEnvs may cause issues with concurrent, asynchronous tests, as one test finishing and resetting variables could impact other tests running simultaneously [5]. Alternatively, you can manually call vi.unstubAllEnvs in a beforeEach hook [4][6].
Citations:
- 1: https://vitest.dev/api/vi
- 2: https://github.com/vitest-dev/vitest/blob/v4.1.4/docs/api/vi.md
- 3: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/vi.md
- 4: https://vitest.dev/guide/mocking
- 5: https://vitest.dev/config/unstubenvs.html
- 6: https://github.com/vitest-dev/vitest/blob/main/docs/guide/mocking.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== vitest API docs in repo references ==\n'
rg -n "stubEnv|unstubAllEnvs|restoreAllMocks|unstubAllEnvs" src test . 2>/dev/null | head -n 80 || trueRepository: NVIDIA/NemoClaw
Length of output: 6122
Replace the manual env restore with Vitest env stubs in src/lib/onboard/dashboard-access.test.ts:90-146
vi.stubEnv/vi.unstubAllEnvs are available in Vitest 4.1.0, so the if/else snapshot can go. Update the two process.env.NEMOCLAW_DASHBOARD_BIND cases to vi.stubEnv(...) as well, since direct assignment won’t be cleaned up by unstubAllEnvs.
🤖 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/onboard/dashboard-access.test.ts` around lines 90 - 98, Replace the
manual process.env snapshot/restore in dashboard-access.test.ts with Vitest env
stubbing. Update the setup around the NEMOCLAW_DASHBOARD_BIND cases to use
vi.stubEnv for each test value, and remove the savedEnv if/else cleanup in
afterEach in favor of vi.unstubAllEnvs. Keep the changes localized to the
dashboard access test helpers so the existing test cases continue to use the
same NEMOCLAW_DASHBOARD_BIND symbol.
Source: Pipeline failures
Comment on lines
+54
to
+58
| // A harmless no-op prefix (not an empty array): macOS bash 3.2 treats | ||
| // "${empty[@]}" as an unbound variable under `set -u`, which would abort | ||
| // the harness before the validator ever runs. `env --` just execs the | ||
| // validator unchanged. | ||
| "_HERMES_BOUNDARY_TIMEOUT=(env --)", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the macOS/BSD env(1) command support "--" to signal end of options before the command to run?
💡 Result:
No, the macOS env(1) command does not support the "--" argument to signal the end of options [1][2]. The macOS implementation of env (which is derived from FreeBSD [3]) follows its own specific syntax [2] and does not adhere to the common POSIX utility convention of using "--" to terminate option parsing [4][5]. According to the macOS manual page [2], options must be specified before any name=value pairs and the utility to be executed. If you attempt to use "--", env will typically treat it as an invalid option or as part of the arguments rather than a delimiter [2]. This behavior differs from GNU env (found on most Linux distributions), which does support standard POSIX-style argument parsing [6][7]. If you require a way to safely pass arguments that might look like options to a command executed via env on macOS, you may need to use a wrapper, a shell function, or sh -c to invoke the target command instead.
Citations:
- 1: https://ss64.com/mac/env.html
- 2: https://keith.github.io/xcode-man-pages/env.1.html
- 3: https://unix.stackexchange.com/questions/774127/why-does-env-s-with-quoted-strings-in-the-shebang-line-work-fine-in-ubuntu-but
- 4: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/env.html
- 5: https://manpages.ubuntu.com/manpages/focal/man1/env.1posix.html
- 6: https://www.gnu.org/software/coreutils/manual/html_node/env-invocation.html
- 7: https://man7.org/linux/man-pages/man1/env.1.html
🏁 Script executed:
sed -n '1,140p' test/hermes-env-secret-boundary-hardening.test.tsRepository: NVIDIA/NemoClaw
Length of output: 5177
🏁 Script executed:
rg -n "_HERMES_BOUNDARY_TIMEOUT|validate_hermes_env_secret_boundary|validate_hermes_runtime_env_secret_boundary" agents/hermes/start.sh test/hermes-env-secret-boundary-hardening.test.tsRepository: NVIDIA/NemoClaw
Length of output: 2101
🏁 Script executed:
sed -n '1704,1720p' agents/hermes/start.shRepository: NVIDIA/NemoClaw
Length of output: 610
Drop the -- from this env prefix
env -- is not supported by macOS/BSD env, so this harness will fail on the target platform before the validator runs. env alone keeps the prefix portable.
🤖 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 `@test/hermes-env-secret-boundary-hardening.test.ts` around lines 54 - 58, The
no-op prefix used in the hermes boundary harness is too GNU-specific because the
`env` invocation in the `_HERMES_BOUNDARY_TIMEOUT` setup uses `--`, which breaks
on macOS/BSD before the validator runs. Update the prefix in
`hermes-env-secret-boundary-hardening.test.ts` to use `env` without `--`,
keeping the existing boundary harness behavior intact while making it portable
across platforms.
Second batch from the live-E2E coverage audit — the medium/low-priority seams, as fast mocked units that run on PR CI: - ollama-auth-proxy token file lifecycle: 0600 mode, persisted-token match, and divergent-token repair on restart (host runner). - extra-placeholder-keys: distinct accepted keys map to distinct canonical openshell:resolve:env: placeholders, and the accepted-keys breadcrumb names accepted keys while omitting a co-submitted refused GITHUB_TOKEN. - hermes remove_stale_gateway_file: a symlink or stale file at the gateway PID path is removed without following the link (regular file, never symlink). - token-rotation: selective-rebuild names only the changed provider(s). - _validate_port: out-of-range/non-numeric ports fail closed with the exact "Invalid <NAME>=<value> (expected 1024-65535)" message. - snapshot: the bare `snapshot` help branch prints create/list/restore usage. Also relocate the pure-unit cases that lived as bare test(...) inside two live files (common-egress parsers + openclaw-inference-switch reply matcher) into importable test/e2e/support helper modules with PR-collected unit tests; the live tests import the helpers unchanged. Two audit items are intentionally deferred: the install.sh "Resolved install ref:" log assertion and the OpenClaw anthropic plain-baseUrl assertion both land in legacy-budget-capped files where the growth guardrail forbids bumping the budget; both are low value (a log string; a contract already covered on the Hermes side and enforced live on the OpenClaw side). SKIP=test-cli: the full cli+integration vitest hook trips on pre-existing macOS bash 3.2 failures in untouched shell-harness suites; CI runs bash 5.x green. Every new/changed file here was verified green individually, and the checks registry + budget + gitleaks + CLI typecheck pass. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
test/nemoclaw-start-extra-placeholder-breadcrumb.test.ts (2)
64-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemp directory not cleaned up if an assertion or parse throws before line 97.
fs.rmSync(root, ...)at line 97 only runs ifJSON.parseon line 96 succeeds; ifspawnSyncresult is unexpected or the config file is corrupted, the temp dir underos.tmpdir()leaks. Low risk in CI but worth wrapping intry/finallyfor hygiene.♻️ Suggested cleanup guarantee
- const updated = JSON.parse(fs.readFileSync(configPath, "utf-8")); - fs.rmSync(root, { recursive: true, force: true }); - return { result, config: updated }; + try { + const updated = JSON.parse(fs.readFileSync(configPath, "utf-8")); + return { result, config: updated }; + } finally { + fs.rmSync(root, { recursive: true, force: true }); + }🤖 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 `@test/nemoclaw-start-extra-placeholder-breadcrumb.test.ts` around lines 64 - 99, The temporary directory cleanup in runRefresh is not guaranteed because fs.rmSync(root, ...) only executes after JSON.parse and other assertions succeed. Wrap the body of runRefresh around the temp-dir setup, spawnSync execution, and config read in a try/finally so the root directory is always removed even if parsing or an assertion fails.
38-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHeredoc terminator matching doesn't strip leading whitespace for
<<-style heredocs.The opener regex accepts
<<-?(both<<and<<-forms), but the terminator-line checkline === heredocTerminatorrequires an exact match. With<<-'TERM', bash permits the closing terminator line to be tab-indented, which this check would miss, causing the loop to run to EOF and throw "Expected a top-level close...". Not currently triggered (the script apparently only uses<<'PY'), but worth hardening or documenting the assumption since this is a shared test helper.🔧 Suggested hardening
if (heredocTerminator !== null) { - if (line === heredocTerminator) heredocTerminator = null; + if (line.trimStart() === heredocTerminator) heredocTerminator = null; continue; }🤖 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 `@test/nemoclaw-start-extra-placeholder-breadcrumb.test.ts` around lines 38 - 57, The heredoc parsing in extractShellFunction currently treats all terminators as exact string matches, which breaks `<<-` heredocs where the closing token may be tab-indented. Update the helper to distinguish `<<-` from `<<` when setting heredocTerminator, and allow leading tab whitespace when matching the closing line for `<<-` forms while keeping exact matching for normal heredocs. Use the existing `extractShellFunction` logic and its opener regex as the place to harden this behavior.test/runtime-shell.test.ts (1)
168-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
it.eachdataset across both blocks.The same six
{ name, value }cases are repeated verbatim forget_local_provider_base_urlandcheck_local_provider_health. Extracting into a sharedconst invalidPortCases = [...]would remove the duplication.Also applies to: 186-192
🤖 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 `@test/runtime-shell.test.ts` around lines 168 - 174, The `it.each` invalid port cases are duplicated in both the `get_local_provider_base_url` and `check_local_provider_health` test blocks. Extract the repeated `{ name, value }` dataset into a shared `const invalidPortCases` near the top of the test file and reuse it in both `it.each` calls to keep the cases defined once.test/credential-rotation.test.ts (2)
260-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating near-duplicate cases with
it.each.The five tests share identical structure (build a
threeProviderPlan, calldetectMessagingCredentialRotation, assertchangedProviders), differing only in which hashes rotate. A table-drivenit.eachwould cut duplication while preserving the same behavioral coverage.🤖 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 `@test/credential-rotation.test.ts` around lines 260 - 379, The five `detectMessagingCredentialRotation` tests are near-duplicates and should be consolidated into a table-driven `it.each` block. Keep the shared setup around `threeProviderPlan`, `registry.getSandbox`, and the `changedProviders` assertions, and parameterize only the differing hash/token scenarios plus the expected provider list and `changed` value. Preserve the existing coverage for A-only, middle-only, multiple, all, and none cases while reducing repetition in `test/credential-rotation.test.ts`.
272-379: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove
vi.restoreAllMocks()intoafterEachfor guaranteed cleanup.Each test calls
vi.restoreAllMocks()as its last statement. If anexpect(...)earlier in the test throws, that line never runs and theregistry.getSandboxspy leaks into subsequent tests, risking cross-test pollution.🔧 Proposed fix
+ afterEach(() => { + vi.restoreAllMocks(); + }); + describe("selective-rebuild provider naming", () => { ... it("names ONLY provider A and excludes unchanged siblings B and C", () => { ... expect(result.changedProviders.join(", ")).toBe(A); - vi.restoreAllMocks(); });(repeat removal for the other four
itblocks)🤖 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 `@test/credential-rotation.test.ts` around lines 272 - 379, Move the repeated vi.restoreAllMocks() cleanup out of each detectMessagingCredentialRotation test and into a shared afterEach so it always runs even if an assertion fails. Update the surrounding test suite that spies on registry.getSandbox to rely on this global teardown instead of per-test cleanup, and remove the trailing restore call from each it block.test/hermes-gateway-pid-cleanup.test.ts (1)
58-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated try/finally cleanup.
Each of the four
itblocks repeats the sametry { ... } finally { fs.rmSync(tmp, { recursive: true, force: true }); }pattern. A small helper (e.g.withTmpCleanup(tmp, fn)) or aafterEachhook tracking the last created tmp dir would remove this duplication and reduce the chance of a future test forgetting the cleanup.♻️ Example refactor sketch
+let currentTmp: string | undefined; + +afterEach(() => { + if (currentTmp) { + fs.rmSync(currentTmp, { recursive: true, force: true }); + currentTmp = undefined; + } +}); + function runRemoveStale( seed: (tmp: string, pidPath: string) => void, label = "legacy PID file", ): { status: number | null; stderr: string; tmp: string; pidPath: string } { ... const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "hermes-gw-pid-cleanup-")); + currentTmp = tmp; ... }🤖 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 `@test/hermes-gateway-pid-cleanup.test.ts` around lines 58 - 132, The four tests in hermes-gateway-pid-cleanup.test.ts repeat the same tmp cleanup try/finally pattern. Extract that cleanup into a shared helper or an afterEach-based cleanup tied to runRemoveStale so each it block only focuses on assertions, and the tmp directory is always removed consistently.test/ollama-proxy-recovery.test.ts (2)
622-624: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid pinning the repair test to the exact
killinvocation.Line 623 asserts one specific shell command and call order instead of the recovery behavior. This will fail on harmless cleanup refactors (
process.kill, helper indirection, extra preflight commands) even if the proxy still repairs correctly. As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."🤖 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 `@test/ollama-proxy-recovery.test.ts` around lines 622 - 624, The repair test is too tightly coupled to the exact `kill` command and mock call order in the recovery flow. Update the assertions in `ollama-proxy-recovery.test.ts` to verify the observable recovery outcome through the public boundary instead of `payload.runCommands[0]`, while still checking the stale proxy is reclaimed and `spawnedToken` is the FILE token via the recovery scenario helpers.Source: Path instructions
395-398: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueLoosen the cleanup assertion in the divergent-token test. The test already proves the repair path by checking the spawned token and on-disk token; asserting
runCommands[0]is exactly["kill", "4242"]locks it to one cleanup implementation and adds avoidable churn on refactors.🤖 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 `@test/ollama-proxy-recovery.test.ts` around lines 395 - 398, Loosen the cleanup assertion in the divergent-token test so it validates the repair behavior without requiring a specific cleanup command shape. In the test around the proxy recovery flow, keep the checks that confirm the spawned token and on-disk token match, but replace the exact runCommands[0] equality assertion with a more flexible assertion tied to the cleanup path in the recovery logic so refactors in the cleanup implementation do not break the test.Source: Learnings
🤖 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 `@test/nemoclaw-start-extra-placeholder-breadcrumb.test.ts`:
- Around line 1-198: The new conditionals in extractShellFunction are tripping
the Codebase Growth Guardrails count for this test file. Simplify the
heredoc-aware parsing logic in the
test/nemoclaw-start-extra-placeholder-breadcrumb suite to avoid adding counted
if statements, or move the helper logic out of the *.test.ts file into shared
non-test support so the guardrail no longer sees it. Keep the existing behavior
of extractShellFunction and runRefresh intact while reducing the conditional
footprint.
---
Nitpick comments:
In `@test/credential-rotation.test.ts`:
- Around line 260-379: The five `detectMessagingCredentialRotation` tests are
near-duplicates and should be consolidated into a table-driven `it.each` block.
Keep the shared setup around `threeProviderPlan`, `registry.getSandbox`, and the
`changedProviders` assertions, and parameterize only the differing hash/token
scenarios plus the expected provider list and `changed` value. Preserve the
existing coverage for A-only, middle-only, multiple, all, and none cases while
reducing repetition in `test/credential-rotation.test.ts`.
- Around line 272-379: Move the repeated vi.restoreAllMocks() cleanup out of
each detectMessagingCredentialRotation test and into a shared afterEach so it
always runs even if an assertion fails. Update the surrounding test suite that
spies on registry.getSandbox to rely on this global teardown instead of per-test
cleanup, and remove the trailing restore call from each it block.
In `@test/hermes-gateway-pid-cleanup.test.ts`:
- Around line 58-132: The four tests in hermes-gateway-pid-cleanup.test.ts
repeat the same tmp cleanup try/finally pattern. Extract that cleanup into a
shared helper or an afterEach-based cleanup tied to runRemoveStale so each it
block only focuses on assertions, and the tmp directory is always removed
consistently.
In `@test/nemoclaw-start-extra-placeholder-breadcrumb.test.ts`:
- Around line 64-99: The temporary directory cleanup in runRefresh is not
guaranteed because fs.rmSync(root, ...) only executes after JSON.parse and other
assertions succeed. Wrap the body of runRefresh around the temp-dir setup,
spawnSync execution, and config read in a try/finally so the root directory is
always removed even if parsing or an assertion fails.
- Around line 38-57: The heredoc parsing in extractShellFunction currently
treats all terminators as exact string matches, which breaks `<<-` heredocs
where the closing token may be tab-indented. Update the helper to distinguish
`<<-` from `<<` when setting heredocTerminator, and allow leading tab whitespace
when matching the closing line for `<<-` forms while keeping exact matching for
normal heredocs. Use the existing `extractShellFunction` logic and its opener
regex as the place to harden this behavior.
In `@test/ollama-proxy-recovery.test.ts`:
- Around line 622-624: The repair test is too tightly coupled to the exact
`kill` command and mock call order in the recovery flow. Update the assertions
in `ollama-proxy-recovery.test.ts` to verify the observable recovery outcome
through the public boundary instead of `payload.runCommands[0]`, while still
checking the stale proxy is reclaimed and `spawnedToken` is the FILE token via
the recovery scenario helpers.
- Around line 395-398: Loosen the cleanup assertion in the divergent-token test
so it validates the repair behavior without requiring a specific cleanup command
shape. In the test around the proxy recovery flow, keep the checks that confirm
the spawned token and on-disk token match, but replace the exact runCommands[0]
equality assertion with a more flexible assertion tied to the cleanup path in
the recovery logic so refactors in the cleanup implementation do not break the
test.
In `@test/runtime-shell.test.ts`:
- Around line 168-174: The `it.each` invalid port cases are duplicated in both
the `get_local_provider_base_url` and `check_local_provider_health` test blocks.
Extract the repeated `{ name, value }` dataset into a shared `const
invalidPortCases` near the top of the test file and reuse it in both `it.each`
calls to keep the cases defined once.
🪄 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: 36212af4-bb80-4d15-9a7f-6098a0736154
📒 Files selected for processing (12)
src/lib/actions/sandbox/snapshot.test.tstest/credential-rotation.test.tstest/e2e/live/common-egress-agent-helpers.tstest/e2e/live/common-egress-agent.test.tstest/e2e/live/openclaw-inference-switch-helpers.tstest/e2e/live/openclaw-inference-switch.test.tstest/e2e/support/common-egress-agent-helpers.test.tstest/e2e/support/openclaw-inference-switch-helpers.test.tstest/hermes-gateway-pid-cleanup.test.tstest/nemoclaw-start-extra-placeholder-breadcrumb.test.tstest/ollama-proxy-recovery.test.tstest/runtime-shell.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/e2e/support/common-egress-agent-helpers.test.ts
The codebase-growth-guardrails check requires changed *.test.ts files not to add `if` statements — test bodies must stay linear. Move the if-bearing harness/stub code (ollama HTTP stub + driver, device-approval python invokers, hermes/placeholder shell-fn extractors, whatsapp fake-module builder) out of the counted test files into co-located non-test helper modules, and replace the device-approval per-test `if (!hasPython3()) return;` gates with a module-level it.skipIf. Env-restore teardown branches become branchless Object.assign. No test behavior changes; every affected suite still passes. tsconfig.runtime-preloads.json also excludes *-test-helpers.ts so the new whatsapp test helper is not compiled into the shipped runtime preloads. SKIP=test-cli: same pre-existing macOS bash 3.2 shell-harness failures as the prior commits; CI runs bash 5.x green. All touched suites verified green individually; checks registry + budget + typecheck pass. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…hell-units' into test/mock-recovery-reconcile-shell-units
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
test/hermes-gateway-pid-cleanup.test.ts (1)
17-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid observable-outcome coverage for the cleanup contract.
Each test asserts on exit status, stderr text, and real filesystem state (symlink/file existence) rather than internal implementation details, which aligns well with behavioral-confidence testing for this contract.
One minor, purely optional improvement: the
try { ... } finally { fs.rmSync(tmp, { recursive: true, force: true }); }cleanup boilerplate is duplicated across all four tests. Could be consolidated with anafterEachthat tracks the currenttmpdir, but this is a nitpick and not required.🤖 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 `@test/hermes-gateway-pid-cleanup.test.ts` around lines 17 - 92, The tests already cover the cleanup behavior well, but the repeated try/finally tmp directory teardown is duplicated across all four cases. Refactor the `Hermes remove_stale_gateway_file cleanup (legacy gateway.pid)` suite to centralize the temporary directory cleanup, ideally via a shared `afterEach` or helper used by `runRemoveStale`-based tests, while keeping the existing assertions on `stderr`, `status`, and filesystem state unchanged.test/ollama-auth-proxy-handler-helpers.ts (1)
121-150: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
request()has no timeout; a hung proxy hangs the test indefinitely.Every request relies solely on Vitest's own test timeout as a backstop. A short request timeout here would fail faster with a clearer error message instead of surfacing as a generic suite timeout.
🤖 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 `@test/ollama-auth-proxy-handler-helpers.ts` around lines 121 - 150, The request helper in request() lacks an explicit network timeout, so a stalled proxy can leave tests hanging until the suite timeout. Add a short timeout to the http.request call in request(), and make sure the timeout handler aborts the request and rejects with a clear error so failures surface quickly and clearly. Keep the change localized to the request() helper and preserve the existing resolve/reject behavior for successful responses and other request errors.
🤖 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
`@src/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact-test-helpers.ts`:
- Around line 24-33: The non-qrcode branch is still triggering the qrcode
patching side effects because `patchQrcode` and `patchQrcodeTerminal` are
evaluated before `isQrcodeRequest` is checked. Update the module loader function
in `whatsapp-qr-compact-test-helpers` so the `isQrcodeRequest` guard wraps the
patching logic itself, and only compute `patched` when the request string
actually contains "qrcode"; otherwise return the unmodified `loaded` value.
In `@test/nemoclaw-start-extra-placeholder-breadcrumb-helpers.ts`:
- Around line 49-85: The temp directory cleanup in runRefresh should always run
even when spawnSync fails or the config file cannot be parsed. Move the
fs.rmSync(root, ...) cleanup into a finally-style path around the
JSON.parse/readback logic, so root is removed regardless of failures. Use
runRefresh, spawnSync, and the config read/parse block as the anchor points when
updating the helper.
In `@test/ollama-auth-proxy-handler-helpers.ts`:
- Around line 80-96: The retry loop in startProxy keeps rescheduling tryConnect
after the promise has already settled, so add a cancellation/settled flag in the
helper around the http.request retry logic. Update the startProxy Promise flow
to set that flag on resolve, reject, and the child.once("exit") path, and guard
both the req.on("error") retry scheduling and the success callback so no further
reconnect attempts or late resolve/clearTimeout calls happen once settled.
---
Nitpick comments:
In `@test/hermes-gateway-pid-cleanup.test.ts`:
- Around line 17-92: The tests already cover the cleanup behavior well, but the
repeated try/finally tmp directory teardown is duplicated across all four cases.
Refactor the `Hermes remove_stale_gateway_file cleanup (legacy gateway.pid)`
suite to centralize the temporary directory cleanup, ideally via a shared
`afterEach` or helper used by `runRemoveStale`-based tests, while keeping the
existing assertions on `stderr`, `status`, and filesystem state unchanged.
In `@test/ollama-auth-proxy-handler-helpers.ts`:
- Around line 121-150: The request helper in request() lacks an explicit network
timeout, so a stalled proxy can leave tests hanging until the suite timeout. Add
a short timeout to the http.request call in request(), and make sure the timeout
handler aborts the request and rejects with a clear error so failures surface
quickly and clearly. Keep the change localized to the request() helper and
preserve the existing resolve/reject behavior for successful responses and other
request errors.
🪄 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: f9a7c8a5-3f37-44bb-a931-7c5a1a3f9688
📒 Files selected for processing (13)
src/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact-test-helpers.tssrc/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.test.tssrc/lib/onboard/dashboard-access.test.tssrc/lib/shields/audit-format.test.tstest/e2e/support/common-egress-agent-helpers.test.tstest/hermes-gateway-pid-cleanup-helpers.tstest/hermes-gateway-pid-cleanup.test.tstest/nemoclaw-start-extra-placeholder-breadcrumb-helpers.tstest/nemoclaw-start-extra-placeholder-breadcrumb.test.tstest/ollama-auth-proxy-handler-helpers.tstest/ollama-auth-proxy-handler.test.tstest/openclaw-device-approval-policy.test.tstsconfig.runtime-preloads.json
✅ Files skipped from review due to trivial changes (1)
- tsconfig.runtime-preloads.json
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/support/common-egress-agent-helpers.test.ts
- src/lib/shields/audit-format.test.ts
- src/lib/onboard/dashboard-access.test.ts
- src/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.test.ts
Comment on lines
+24
to
+33
| return function (request: unknown, ..._rest: unknown[]) { | ||
| const loaded = request === absolutePath ? patchedModule : {}; | ||
| const isQrcodeRequest = typeof request === "string" && request.indexOf("qrcode") !== -1; | ||
| const patched = isQrcodePackage(loaded) | ||
| ? patchQrcode(loaded) | ||
| : isQrcodeTerminalPackage(loaded) | ||
| ? patchQrcodeTerminal(loaded) | ||
| : loaded; | ||
| return isQrcodeRequest ? patched : loaded; | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Patch side effects leak through the "non-qrcode-request" branch, weakening the test guarantee.
patchQrcode/patchQrcodeTerminal mutate mod in place (setting __nemoclawCompactPatched and wrapping toString/generate) and return the same reference. Here, patched is computed unconditionally from loaded's shape (Lines 27-31) before checking isQrcodeRequest (Line 32). When request === absolutePath but the request string doesn't contain "qrcode", loaded (= patchedModule) still gets mutated as a side effect of computing patched, even though the function returns loaded (which is now the same mutated object). The isQrcodeRequest guard therefore doesn't actually prevent patching — it only decides which variable name is returned, not whether the mutation happened.
This defeats the doc comment's claim ("applies the compact patch to any request whose string contains 'qrcode', and passes everything else through") and could let tests pass without truly exercising the "should NOT patch a non-qrcode request" case, since the object is patched regardless.
🐛 Proposed fix: guard the patch calls behind `isQrcodeRequest`
return function (request: unknown, ..._rest: unknown[]) {
const loaded = request === absolutePath ? patchedModule : {};
const isQrcodeRequest = typeof request === "string" && request.indexOf("qrcode") !== -1;
- const patched = isQrcodePackage(loaded)
- ? patchQrcode(loaded)
- : isQrcodeTerminalPackage(loaded)
- ? patchQrcodeTerminal(loaded)
- : loaded;
- return isQrcodeRequest ? patched : loaded;
+ if (!isQrcodeRequest) return loaded;
+ if (isQrcodePackage(loaded)) return patchQrcode(loaded);
+ if (isQrcodeTerminalPackage(loaded)) return patchQrcodeTerminal(loaded);
+ return loaded;
};As per path instructions for test files, "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
📝 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
| return function (request: unknown, ..._rest: unknown[]) { | |
| const loaded = request === absolutePath ? patchedModule : {}; | |
| const isQrcodeRequest = typeof request === "string" && request.indexOf("qrcode") !== -1; | |
| const patched = isQrcodePackage(loaded) | |
| ? patchQrcode(loaded) | |
| : isQrcodeTerminalPackage(loaded) | |
| ? patchQrcodeTerminal(loaded) | |
| : loaded; | |
| return isQrcodeRequest ? patched : loaded; | |
| }; | |
| return function (request: unknown, ..._rest: unknown[]) { | |
| const loaded = request === absolutePath ? patchedModule : {}; | |
| const isQrcodeRequest = typeof request === "string" && request.indexOf("qrcode") !== -1; | |
| if (!isQrcodeRequest) return loaded; | |
| if (isQrcodePackage(loaded)) return patchQrcode(loaded); | |
| if (isQrcodeTerminalPackage(loaded)) return patchQrcodeTerminal(loaded); | |
| return loaded; | |
| }; |
🤖 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/messaging/channels/whatsapp/runtime/whatsapp-qr-compact-test-helpers.ts`
around lines 24 - 33, The non-qrcode branch is still triggering the qrcode
patching side effects because `patchQrcode` and `patchQrcodeTerminal` are
evaluated before `isQrcodeRequest` is checked. Update the module loader function
in `whatsapp-qr-compact-test-helpers` so the `isQrcodeRequest` guard wraps the
patching logic itself, and only compute `patched` when the request string
actually contains "qrcode"; otherwise return the unmodified `loaded` value.
Source: Path instructions
Comment on lines
+49
to
+85
| export function runRefresh(config: unknown, env: Record<string, string> = {}): RunResult { | ||
| const src = fs.readFileSync(START_SCRIPT, "utf-8"); | ||
| const root = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-extra-placeholder-")); | ||
| const openclawDir = path.join(root, ".openclaw"); | ||
| fs.mkdirSync(openclawDir, { recursive: true }); | ||
| const configPath = path.join(openclawDir, "openclaw.json"); | ||
| const hashPath = path.join(openclawDir, ".config-hash"); | ||
| fs.writeFileSync(configPath, `${JSON.stringify(config, null, 2)}\n`); | ||
| fs.writeFileSync(hashPath, "oldhash\n"); | ||
|
|
||
| const fn = extractShellFunction(src, "refresh_openclaw_provider_placeholders").replaceAll( | ||
| "/sandbox/.openclaw", | ||
| openclawDir, | ||
| ); | ||
| // Stub the config-mutability guards and the dir-owner probe so the helper | ||
| // runs on a mutable temp dir without touching real sandbox ownership. This | ||
| // isolates the extras-validation + placeholder-rewrite path under test. | ||
| const wrapper = [ | ||
| "#!/usr/bin/env bash", | ||
| "set -eu", | ||
| "openclaw_config_dir_owner() { echo sandbox; }", | ||
| "prepare_openclaw_config_for_write() { :; }", | ||
| "restore_openclaw_config_after_write() { :; }", | ||
| fn, | ||
| "refresh_openclaw_provider_placeholders", | ||
| ].join("\n"); | ||
| const script = path.join(root, "run.sh"); | ||
| fs.writeFileSync(script, wrapper, { mode: 0o700 }); | ||
| const result = spawnSync("bash", [script], { | ||
| encoding: "utf-8", | ||
| env: { PATH: process.env.PATH || "", ...env }, | ||
| timeout: 5000, | ||
| }); | ||
| const updated = JSON.parse(fs.readFileSync(configPath, "utf-8")); | ||
| fs.rmSync(root, { recursive: true, force: true }); | ||
| return { result, config: updated }; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Temp dir leaks if spawnSync fails or output is malformed.
fs.rmSync(root, ...) at Line 83 only runs after JSON.parse(fs.readFileSync(configPath, "utf-8")) succeeds (Line 82). If the wrapped script errors out or writes invalid JSON, JSON.parse throws and root is never cleaned up, leaking temp directories on every such failure (accumulating in CI over repeated runs).
🧹 Proposed fix
const script = path.join(root, "run.sh");
fs.writeFileSync(script, wrapper, { mode: 0o700 });
- const result = spawnSync("bash", [script], {
- encoding: "utf-8",
- env: { PATH: process.env.PATH || "", ...env },
- timeout: 5000,
- });
- const updated = JSON.parse(fs.readFileSync(configPath, "utf-8"));
- fs.rmSync(root, { recursive: true, force: true });
- return { result, config: updated };
+ try {
+ const result = spawnSync("bash", [script], {
+ encoding: "utf-8",
+ env: { PATH: process.env.PATH || "", ...env },
+ timeout: 5000,
+ });
+ const updated = JSON.parse(fs.readFileSync(configPath, "utf-8"));
+ return { result, config: updated };
+ } finally {
+ fs.rmSync(root, { recursive: true, force: true });
+ }
}📝 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
| export function runRefresh(config: unknown, env: Record<string, string> = {}): RunResult { | |
| const src = fs.readFileSync(START_SCRIPT, "utf-8"); | |
| const root = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-extra-placeholder-")); | |
| const openclawDir = path.join(root, ".openclaw"); | |
| fs.mkdirSync(openclawDir, { recursive: true }); | |
| const configPath = path.join(openclawDir, "openclaw.json"); | |
| const hashPath = path.join(openclawDir, ".config-hash"); | |
| fs.writeFileSync(configPath, `${JSON.stringify(config, null, 2)}\n`); | |
| fs.writeFileSync(hashPath, "oldhash\n"); | |
| const fn = extractShellFunction(src, "refresh_openclaw_provider_placeholders").replaceAll( | |
| "/sandbox/.openclaw", | |
| openclawDir, | |
| ); | |
| // Stub the config-mutability guards and the dir-owner probe so the helper | |
| // runs on a mutable temp dir without touching real sandbox ownership. This | |
| // isolates the extras-validation + placeholder-rewrite path under test. | |
| const wrapper = [ | |
| "#!/usr/bin/env bash", | |
| "set -eu", | |
| "openclaw_config_dir_owner() { echo sandbox; }", | |
| "prepare_openclaw_config_for_write() { :; }", | |
| "restore_openclaw_config_after_write() { :; }", | |
| fn, | |
| "refresh_openclaw_provider_placeholders", | |
| ].join("\n"); | |
| const script = path.join(root, "run.sh"); | |
| fs.writeFileSync(script, wrapper, { mode: 0o700 }); | |
| const result = spawnSync("bash", [script], { | |
| encoding: "utf-8", | |
| env: { PATH: process.env.PATH || "", ...env }, | |
| timeout: 5000, | |
| }); | |
| const updated = JSON.parse(fs.readFileSync(configPath, "utf-8")); | |
| fs.rmSync(root, { recursive: true, force: true }); | |
| return { result, config: updated }; | |
| } | |
| export function runRefresh(config: unknown, env: Record<string, string> = {}): RunResult { | |
| const src = fs.readFileSync(START_SCRIPT, "utf-8"); | |
| const root = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-extra-placeholder-")); | |
| const openclawDir = path.join(root, ".openclaw"); | |
| fs.mkdirSync(openclawDir, { recursive: true }); | |
| const configPath = path.join(openclawDir, "openclaw.json"); | |
| const hashPath = path.join(openclawDir, ".config-hash"); | |
| fs.writeFileSync(configPath, `${JSON.stringify(config, null, 2)}\n`); | |
| fs.writeFileSync(hashPath, "oldhash\n"); | |
| const fn = extractShellFunction(src, "refresh_openclaw_provider_placeholders").replaceAll( | |
| "/sandbox/.openclaw", | |
| openclawDir, | |
| ); | |
| // Stub the config-mutability guards and the dir-owner probe so the helper | |
| // runs on a mutable temp dir without touching real sandbox ownership. This | |
| // isolates the extras-validation + placeholder-rewrite path under test. | |
| const wrapper = [ | |
| "#!/usr/bin/env bash", | |
| "set -eu", | |
| "openclaw_config_dir_owner() { echo sandbox; }", | |
| "prepare_openclaw_config_for_write() { :; }", | |
| "restore_openclaw_config_after_write() { :; }", | |
| fn, | |
| "refresh_openclaw_provider_placeholders", | |
| ].join("\n"); | |
| const script = path.join(root, "run.sh"); | |
| fs.writeFileSync(script, wrapper, { mode: 0o700 }); | |
| try { | |
| const result = spawnSync("bash", [script], { | |
| encoding: "utf-8", | |
| env: { PATH: process.env.PATH || "", ...env }, | |
| timeout: 5000, | |
| }); | |
| const updated = JSON.parse(fs.readFileSync(configPath, "utf-8")); | |
| return { result, config: updated }; | |
| } finally { | |
| fs.rmSync(root, { recursive: true, force: true }); | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 49-49: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(START_SCRIPT, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 55-55: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(configPath, ${JSON.stringify(config, null, 2)}\n)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 56-56: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(hashPath, "oldhash\n")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 75-75: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(script, wrapper, { mode: 0o700 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 81-81: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(configPath, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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 `@test/nemoclaw-start-extra-placeholder-breadcrumb-helpers.ts` around lines 49
- 85, The temp directory cleanup in runRefresh should always run even when
spawnSync fails or the config file cannot be parsed. Move the fs.rmSync(root,
...) cleanup into a finally-style path around the JSON.parse/readback logic, so
root is removed regardless of failures. Use runRefresh, spawnSync, and the
config read/parse block as the anchor points when updating the helper.
Comment on lines
+80
to
+96
| await new Promise<void>((resolve, reject) => { | ||
| const timer = setTimeout(() => reject(new Error("proxy did not start in time")), 5_000); | ||
| const tryConnect = (): void => { | ||
| const req = http.request( | ||
| { host: "127.0.0.1", port: proxyPort, path: "/", method: "GET" }, | ||
| (res) => { | ||
| res.resume(); | ||
| clearTimeout(timer); | ||
| resolve(); | ||
| }, | ||
| ); | ||
| req.on("error", () => setTimeout(tryConnect, 100)); | ||
| req.end(); | ||
| }; | ||
| child.once("exit", (code) => reject(new Error(`proxy exited early with code ${code}`))); | ||
| tryConnect(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Retry loop in startProxy keeps running after the promise settles.
When the outer 5s timer fires and rejects, nothing stops req.on("error", () => setTimeout(tryConnect, 100)) from continuing to schedule reconnect attempts. This is a harmless-but-wasteful leak in the happy path, but if the child later starts responding, a stray tryConnect could still fire and call resolve()/clearTimeout() on an already-settled promise (no-op, but confusing to debug) and keep polling a proxy from a test that already failed/torn down.
🔧 Proposed fix: add a cancellation flag
await new Promise<void>((resolve, reject) => {
+ let settled = false;
const timer = setTimeout(() => reject(new Error("proxy did not start in time")), 5_000);
const tryConnect = (): void => {
+ if (settled) return;
const req = http.request(
{ host: "127.0.0.1", port: proxyPort, path: "/", method: "GET" },
(res) => {
res.resume();
+ settled = true;
clearTimeout(timer);
resolve();
},
);
- req.on("error", () => setTimeout(tryConnect, 100));
+ req.on("error", () => {
+ if (!settled) setTimeout(tryConnect, 100);
+ });
req.end();
};
- child.once("exit", (code) => reject(new Error(`proxy exited early with code ${code}`)));
+ child.once("exit", (code) => {
+ settled = true;
+ reject(new Error(`proxy exited early with code ${code}`));
+ });
tryConnect();
});🤖 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 `@test/ollama-auth-proxy-handler-helpers.ts` around lines 80 - 96, The retry
loop in startProxy keeps rescheduling tryConnect after the promise has already
settled, so add a cancellation/settled flag in the helper around the
http.request retry logic. Update the startProxy Promise flow to set that flag on
resolve, reject, and the child.once("exit") path, and guard both the
req.on("error") retry scheduling and the success callback so no further
reconnect attempts or late resolve/clearTimeout calls happen once settled.
wscurran
added
area: ci
Contributor
This was referenced Jul 2, 2026
…concile-shell-units # Conflicts: # test/e2e/live/openclaw-inference-switch.test.ts # test/hermes-env-secret-boundary-hardening.test.ts # test/hermes-start.test.ts # test/openclaw-device-approval-policy.test.ts
- whatsapp-qr-compact: extract pure resolvePatchedModule so the runtime hook and its test share one routing decision instead of a re-implemented copy, and so a non-qrcode request never mutates the loaded module as a side effect - dashboard-access: use vi.stubEnv/vi.unstubAllEnvs instead of a manual process.env snapshot/restore - nemoclaw-start placeholder breadcrumb helper: rm the temp dir in finally so a spawn/JSON.parse failure cannot leak it - ollama auth proxy handler helper: add a settled flag so the startup retry loop stops once the promise resolves/rejects Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Collaborator
Author
|
Rebased onto latest Merge conflicts (4 test files):
CodeRabbit findings:
Verification: affected suites pass locally (device-approval, whatsapp-qr-compact, dashboard-access, hermes-start, ollama-proxy-recovery, ollama-auth-proxy-handler). The test-conditional guardrail shows no net-new |
mockBaselineInference and its baseline constants lived in the live openclaw-inference-switch target and were only asserted inside test(...) blocks there, so that pure config wiring ran solely under the opt-in live lane (the it-block guard does not catch test(...) blocks). Extract them into openclaw-inference-switch-helpers.ts and assert them from the e2e-support project, matching the agentReplyContainsToken backfill. The redundant live test(...) assertion blocks are removed; the live target imports the helpers for its runtime flow. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Collaborator
Author
|
Closed the one live-only coverage gap found while auditing this branch.
Not touched: the guard still only flags |
The second harness in this file (runRuntimeEnvValidation) was not part of the main-merge conflict, so it kept the pre-merge shape: it ran validate_hermes_runtime_env_secret_boundary — which main's #5595 changed to invoke $_HERMES_PYTHON — without defining _HERMES_PYTHON, and still used the non-portable env -- no-op. Under set -u this aborted with '_HERMES_PYTHON: unbound variable', failing cli-test-shards (3). Align it with the start-env harness: command builtin no-op + _HERMES_PYTHON from command -v python3. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
agentReplyContainsToken in messaging-endpoint-classifiers.ts was exported but never called: both the live target and the e2e-support unit test assert the route token via parseOpenClawAgentText(...).toContain(COMPAT_AGENT_REPLY) directly, and switching them to the boolean predicate would lose the toContain diagnostic. Keep only the shared COMPAT_AGENT_* constants; the underlying parseOpenClawAgentText behavior stays covered by the support test. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Collaborator
Author
PR Review Advisor (GPT-5.5) responsePRA-5 — resolved in a7ee8b2: The remaining four are resolve-or-justify (0 required). Justifications below. PRA-1 — Live E2E unit-block guard (architecture) — justifiedThis is a preventive repository lint, not a runtime workaround masking an invalid state. The convention it enforces is real and intentional:
PRA-2 — Ciao
|
Contributor
E2E Target Results — ❌ Some jobs failedRun: 28815809174
|
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28816134534
|
jyaunches
approved these changes
Jul 6, 2026
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…st it (NVIDIA#6086) ## Summary Two regressions from NVIDIA#5874 (fixed in NVIDIA#6065) only surfaced in **live E2E targets that don't run on PR CI**. This PR closes that class of gap: it audits the live suite for behavior-critical assertions that are cheaply mockable, backfills them as fast units that run on **every** PR, and adds a guard so pure-unit blocks can't hide in live files again. ## What's here **1. The two direct NVIDIA#6065 regression fences** (mocked shell-units) - `reconcile`: an explicit `NEMOCLAW_MODEL_OVERRIDE` survives a divergent gateway model and the stale in-file fallback; normal drift-correction still runs when unset. - `guard recovery`: the restore warning is mirrored into `_NEMOCLAW_GATEWAY_LOG` (the marker the crash-loop E2E polls), and stays silent when the chain is healthy. **2. High-priority mockable backfill** (security/recovery class) - ollama-auth-proxy: Bearer enforcement, no `/api/tags` bypass (NVIDIA#3338), header stripping, non-ASCII auth no-crash (NVIDIA#4820), backend 502. - `config get`: credential redaction + `gateway`-key omission (the `nvapi-` regression class). - device approval policy: scope-upgrade allowlist gate, gateway-env stripping, recover-failed rejection paths (NVIDIA#4462). - shields audit JSONL: credentials redacted before persistence. - hermes env secret boundary: value-shape (not key-name) discriminator; raw secrets rejected without echoing. - dashboard bind: `NEMOCLAW_DASHBOARD_BIND` opt-in incl. negative cases (NVIDIA#3259). - whatsapp compact QR: package shape-detection + terminal-only `small` (NVIDIA#4522). **3. Medium/low backfill** - ollama token-file lifecycle (0600 / persisted / divergent-repair); extra-placeholder-keys canonical placeholder + accepted-keys breadcrumb; hermes `remove_stale_gateway_file` symlink-safety; token-rotation selective-rebuild naming; `_validate_port` fail-closed; snapshot `help` branch. **4. Regression guard** - `scripts/checks/no-unit-blocks-in-live-e2e.ts` bans the vitest `it(...)` primitive inside `test/e2e/live/**` (that glob is uncollected on PR CI, so such blocks never run). Wired into the checks registry with its own unit test. - Relocated the existing offenders (skill-agent + messaging-compatible-endpoint classifier blocks, plus the bare-`test(` unit cases in common-egress + openclaw-inference-switch) into importable `test/e2e/support` modules with PR-collected tests; the live tests import them unchanged. ## Notes - Minimal behavior-preserving refactor to `whatsapp-qr-compact.ts` to export its pure helpers (the preload still auto-installs on require); `tsconfig.runtime-preloads.json` excludes the new co-located test from the shipped preload build. - `nemoclaw-start.sh`: made two possibly-empty-array iterations bash-3.2-safe via the existing `"${arr[@]+...}"` idiom so the shell-unit harnesses run on stock macOS bash. - **Deferred (2 low-value items):** the install.sh "Resolved install ref:" log assertion and the OpenClaw anthropic plain-baseUrl assertion both land in legacy-budget-capped test files where the growth guardrail forbids bumping the budget; the anthropic contract is already covered on the Hermes side and enforced live on the OpenClaw side. ## Verification - Every new/changed test verified green individually across the `cli`, `integration`, and `e2e-support` projects. - `npm run checks` (incl. the new live-unit-block guard), test-file-size budget, gitleaks, and CLI typecheck all pass. - The full `test-cli` pre-commit hook was skipped locally only because it trips on **pre-existing** macOS bash 3.2 failures in untouched shell-harness suites (`select`/`set -u`); CI runs bash 5.x, where they are green. --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved startup robustness for environment parsing and background launch behavior. * WhatsApp compact-QR rendering more consistently uses the compact “terminal” style. * Dashboard remote bind activates only when explicitly opted in via `NEMOCLAW_DASHBOARD_BIND=0.0.0.0`. * Tightened audit/config redaction to prevent secret leakage and omit gateway details. * **Tests** * Expanded coverage for guard-chain recovery warnings, model override precedence, Hermes env-boundary hardening, and proxy/policy correctness. * **Chores** * Added a CI safeguard to prevent unit-test primitives from being included in live E2E tests. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@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
5 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
Two regressions from #5874 (fixed in #6065) only surfaced in live E2E targets that don't run on PR CI. This PR closes that class of gap: it audits the live suite for behavior-critical assertions that are cheaply mockable, backfills them as fast units that run on every PR, and adds a guard so pure-unit blocks can't hide in live files again.
What's here
1. The two direct #6065 regression fences (mocked shell-units)
reconcile: an explicitNEMOCLAW_MODEL_OVERRIDEsurvives a divergent gateway model and the stale in-file fallback; normal drift-correction still runs when unset.guard recovery: the restore warning is mirrored into_NEMOCLAW_GATEWAY_LOG(the marker the crash-loop E2E polls), and stays silent when the chain is healthy.2. High-priority mockable backfill (security/recovery class)
/api/tagsbypass ([Brev][Security] Ollama auth proxy on port 11435 leaves Ollama-native /api/* endpoints unauthenticated #3338), header stripping, non-ASCII auth no-crash ([Ubuntu 24.04][Onboard] local Ollama onboard fails auth proxy failed to start on :11435 #4820), backend 502.config get: credential redaction +gateway-key omission (thenvapi-regression class).NEMOCLAW_DASHBOARD_BINDopt-in incl. negative cases ([macOS][Brev][CLI&UX] Dashboard port 18789 hard-bound to 127.0.0.1 — no flag/env to bind 0.0.0.0 for remote-SSH-deployed hosts #3259).small([DGX Spark][CLI&UX] WhatsApp QR code renders too large in terminal — fills entire screen, impossible to scan #4522).3. Medium/low backfill
remove_stale_gateway_filesymlink-safety; token-rotation selective-rebuild naming;_validate_portfail-closed; snapshothelpbranch.4. Regression guard
scripts/checks/no-unit-blocks-in-live-e2e.tsbans the vitestit(...)primitive insidetest/e2e/live/**(that glob is uncollected on PR CI, so such blocks never run). Wired into the checks registry with its own unit test.test(unit cases in common-egress + openclaw-inference-switch) into importabletest/e2e/supportmodules with PR-collected tests; the live tests import them unchanged.Notes
whatsapp-qr-compact.tsto export its pure helpers (the preload still auto-installs on require);tsconfig.runtime-preloads.jsonexcludes the new co-located test from the shipped preload build.nemoclaw-start.sh: made two possibly-empty-array iterations bash-3.2-safe via the existing"${arr[@]+...}"idiom so the shell-unit harnesses run on stock macOS bash.Verification
cli,integration, ande2e-supportprojects.npm run checks(incl. the new live-unit-block guard), test-file-size budget, gitleaks, and CLI typecheck all pass.test-clipre-commit hook was skipped locally only because it trips on pre-existing macOS bash 3.2 failures in untouched shell-harness suites (select/set -u); CI runs bash 5.x, where they are green.Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
NEMOCLAW_DASHBOARD_BIND=0.0.0.0.