Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 124 additions & 0 deletions scripts/checks/no-unit-blocks-in-live-e2e.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0
//
// Guard: pure unit blocks must not hide inside test/e2e/live/** files.
//
// vitest.config.ts only collects test/e2e/live/**/*.test.ts when live E2E is
// enabled (NEMOCLAW_RUN_LIVE_E2E=1). On PR CI that flag is false, so the entire
// file is uncollected — including any `describe(...)` unit block embedded in it.
// Such blocks are dead weight on PR CI: they read like coverage but never run
// where they could. This is exactly how two mockable regressions stayed
// unguarded (the skill-agent classifiers and the openclaw TUI-correlation
// logic, the latter saved only by a lucky root-level duplicate).
//
// Convention this guard enforces: inside test/e2e/live/**, the vitest unit
// primitive `it(` is banned. Live cases are declared with `test` — directly, or
// (more often) through a gate wrapper assigned from `shouldRunLiveE2E() ? test
// : test.skip` / `test.skipIf(!shouldRunLiveE2E())`, sometimes grouped under
// `describe.sequential(...)`. A live case never needs `it(`; when `it(` appears
// in a live file it is invariably a pure-unit block someone parked there (as
// happened with the skill-agent and messaging classifier blocks). Such a block
// is dead on PR CI and belongs in an importable module + a PR-collected test
// (root test/**, a co-located src/**/*.test.ts, or test/e2e/support/**).
//
// We deliberately do NOT try to flag bare `test(` unit cases: a live test that
// uses module-level helpers legitimately reads as `test("...", async () => …)`
// with no fixture, and is syntactically indistinguishable from a unit case. The
// `it(` ban is the reliable, zero-false-positive line.

import { existsSync, readdirSync, readFileSync, statSync } from "node:fs";
import path from "node:path";
import { fileURLToPath } from "node:url";

const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../..");
const LIVE_DIR = path.join(REPO_ROOT, "test", "e2e", "live");
const TEST_FILE_PATTERN = /\.(?:test|spec)\.(?:[cm]?[jt]s)$/;
// Match the vitest unit primitive `it(` — including `it.each(`, `it.only(`,
// `it.skip(`, etc. — as a call at a statement boundary. The leading boundary
// (line start or whitespace) prevents matching inside a custom identifier, and
// requiring a call paren after the optional member keeps non-call references
// from matching.
const IT_PRIMITIVE_PATTERN =
/(?:^|[\s;{(])it(?:\.(?:each|only|skip|todo|fails|concurrent|sequential))?\s*\(/;

export type LiveUnitBlockViolation = {
readonly file: string;
readonly line: number;
readonly text: string;
};

function toRepoPath(absPath: string): string {
return path.relative(REPO_ROOT, absPath).split(path.sep).join("/");
}

function* walkFiles(dir: string): Generator<string> {
if (!existsSync(dir)) return;
for (const entry of readdirSync(dir)) {
const absPath = path.join(dir, entry);
const stats = statSync(absPath);
if (stats.isDirectory()) {
yield* walkFiles(absPath);
} else if (stats.isFile() && TEST_FILE_PATTERN.test(entry)) {
yield absPath;
}
}
}

export function findLiveUnitBlocks(source: string, file: string): LiveUnitBlockViolation[] {
const violations: LiveUnitBlockViolation[] = [];
const lines = source.split(/\r\n|\r|\n/);
for (let i = 0; i < lines.length; i += 1) {
const text = lines[i] ?? "";
const trimmed = text.trimStart();
// Skip import lines (`import { it, test } from "vitest"`) and comments.
if (trimmed.startsWith("import ") || trimmed.startsWith("//") || trimmed.startsWith("*")) {
continue;
}
if (IT_PRIMITIVE_PATTERN.test(text)) {
violations.push({ file, line: i + 1, text: trimmed });
}
}
return violations;
}

export function collectLiveUnitBlocks(dir = LIVE_DIR): LiveUnitBlockViolation[] {
return [...walkFiles(dir)]
.flatMap((absPath) => findLiveUnitBlocks(readFileSync(absPath, "utf-8"), toRepoPath(absPath)))
.sort((a, b) => a.file.localeCompare(b.file) || a.line - b.line);
}

export function formatViolations(violations: readonly LiveUnitBlockViolation[]): string {
const out = [
"Live E2E unit-block guard failed.",
"",
"These test/e2e/live/** files use the vitest unit primitive it(...). That glob",
"is only collected when NEMOCLAW_RUN_LIVE_E2E=1, so an it(...) block never runs",
"on PR CI — it looks like coverage but guards nothing. Live cases use test(...)",
"(directly or via a gate wrapper); it(...) in a live file is always a pure-unit",
"block parked in the wrong place.",
"",
"Fix: extract the helper under test into an importable module (src/** or",
"test/e2e/support/**) and move the it(...) block to a PR-collected project",
"(root test/**/*.test.ts, a co-located src/**/*.test.ts, or test/e2e/support/**).",
"Keep the live test importing the shared helper.",
"",
];
for (const v of violations) {
out.push(`- ${v.file}:${v.line} ${v.text}`);
}
return out.join("\n");
}

function main(): void {
const violations = collectLiveUnitBlocks();
if (violations.length > 0) {
console.error(formatViolations(violations));
process.exitCode = 1;
return;
}
console.log("Live E2E unit-block guard passed: no it(...) blocks in test/e2e/live/**.");
}

if (fileURLToPath(import.meta.url) === path.resolve(process.argv[1] ?? "")) {
main();
}
5 changes: 5 additions & 0 deletions scripts/checks/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ const CHECKS: readonly CheckCommand[] = [
command: TSX,
args: ["scripts/checks/test-title-style.ts"],
},
{
name: "no-unit-blocks-in-live-e2e",
command: TSX,
args: ["scripts/checks/no-unit-blocks-in-live-e2e.ts"],
},
];

function main(): void {
Expand Down
7 changes: 5 additions & 2 deletions scripts/nemoclaw-start.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1893,6 +1893,9 @@ node_options_has_require() {
local token
local tokens=()
IFS=$' \t\n' read -r -a tokens <<<"${NODE_OPTIONS:-}"
# Iterating "${tokens[@]}" on an empty array trips `set -u` on bash 3.2
# (macOS default); guard so the local unit harnesses run there too.
[ "${#tokens[@]}" -gt 0 ] || return 1
for token in "${tokens[@]}"; do
if [ "$previous" = "--require" ] && [ "$token" = "$wanted" ]; then
return 0
Expand Down Expand Up @@ -2000,7 +2003,7 @@ validate_nemoclaw_tmp_permissions() {
[ -n "$_target" ] && _dynamic_targets+=("$_target")
done < <(messaging_runtime_preload_targets)

validate_tmp_permissions "$_SANDBOX_SAFETY_NET" "$_PROXY_FIX_SCRIPT" "$_NEMOTRON_FIX_SCRIPT" "$_WS_FIX_SCRIPT" "$_SECCOMP_GUARD_SCRIPT" "$_CIAO_GUARD_SCRIPT" "${_dynamic_targets[@]}"
validate_tmp_permissions "$_SANDBOX_SAFETY_NET" "$_PROXY_FIX_SCRIPT" "$_NEMOTRON_FIX_SCRIPT" "$_WS_FIX_SCRIPT" "$_SECCOMP_GUARD_SCRIPT" "$_CIAO_GUARD_SCRIPT" "${_dynamic_targets[@]+"${_dynamic_targets[@]}"}"
}

verify_messaging_runtime_secret_scans() {
Expand Down Expand Up @@ -2360,7 +2363,7 @@ start_auto_pair() {
if [ "$(id -u)" -eq 0 ]; then
run_prefix=("${STEP_DOWN_PREFIX_SANDBOX[@]}")
fi
OPENCLAW_BIN="$OPENCLAW" nohup "${run_prefix[@]}" python3 -u - <<'PYAUTOPAIR' >>/tmp/auto-pair.log 2>&1 &
OPENCLAW_BIN="$OPENCLAW" nohup "${run_prefix[@]+"${run_prefix[@]}"}" python3 -u - <<'PYAUTOPAIR' >>/tmp/auto-pair.log 2>&1 &
import json
import importlib.util
import os
Expand Down
13 changes: 13 additions & 0 deletions src/lib/actions/sandbox/snapshot.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,19 @@ describe("runSandboxSnapshot", () => {
expect(output).toContain("2 snapshot(s). Restore with:");
});

it("prints create, list, and restore usage for the bare help branch", async () => {
const consoleLog = vi.spyOn(console, "log").mockImplementation(() => {});
const { runSandboxSnapshot } = await import("./snapshot");

await runSandboxSnapshot("alpha", { kind: "help" });

const output = consoleLog.mock.calls.flat().join("\n");
expect(output).toContain("Usage:");
expect(output).toContain("alpha snapshot create");
expect(output).toContain("alpha snapshot list");
expect(output).toContain("alpha snapshot restore");
});

it("restores the latest snapshot into the source sandbox", async () => {
const consoleLog = vi.spyOn(console, "log").mockImplementation(() => {});
getLatestBackupMock.mockReturnValue({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

// Unit coverage for the WhatsApp compact-QR preload's pure shape-detect and
// patch helpers (NemoClaw#4522 wrong-package-patch regression class). The live
// whatsapp-qr-compact E2E only asserts terminal row counts against the real
// upstream renderer; these tests pin the load-hook contract hermetically with
// fake module objects so no real qrcode / qrcode-terminal dependency is needed.

import { describe, expect, it, vi } from "vitest";

import {
isQrcodePackage,
isQrcodeTerminalPackage,
patchQrcode,
patchQrcodeTerminal,
} from "./whatsapp-qr-compact";

// A fake of the `qrcode` package main: has its OWN toString + create().
function makeQrcodeFake() {
const calls: Array<{ text: unknown; opts: unknown; cb: unknown }> = [];
const mod = {
calls,
create() {
return {};
},
toString(text: unknown, opts?: unknown, cb?: unknown) {
calls.push({ text, opts, cb });
return "QR";
},
};
return mod;
}

// A fake of the `qrcode-terminal` package: has generate(), no create().
function makeQrcodeTerminalFake() {
const calls: Array<{ text: unknown; opts: unknown; cb: unknown }> = [];
const mod = {
calls,
generate(text: unknown, opts?: unknown, cb?: unknown) {
calls.push({ text, opts, cb });
},
};
return mod;
}

describe("isQrcodePackage (#4522)", () => {
it("detects the qrcode package main by own toString + create", () => {
expect(isQrcodePackage(makeQrcodeFake())).toBe(true);
});

it("does not match a lookalike submodule that only has create()", () => {
// qrcode's internal lib/core/qrcode.js exposes create() but only the
// inherited Object.prototype.toString — it must NOT be patched.
const submodule = {
create() {
return {};
},
};
expect(isQrcodePackage(submodule)).toBe(false);
});

it("does not match qrcode-terminal (has generate, no create)", () => {
expect(isQrcodePackage(makeQrcodeTerminalFake())).toBe(false);
});
});

describe("isQrcodeTerminalPackage (#4522)", () => {
it("detects qrcode-terminal by own generate and absent create", () => {
expect(isQrcodeTerminalPackage(makeQrcodeTerminalFake())).toBe(true);
});

it("does not match the qrcode package (has create)", () => {
expect(isQrcodeTerminalPackage(makeQrcodeFake())).toBe(false);
});
});

describe("patchQrcode (#4522)", () => {
it("forces small:true only for terminal renders", () => {
const mod = makeQrcodeFake();
patchQrcode(mod);
mod.toString("payload", { type: "terminal" });
expect(mod.calls[0].opts).toEqual({ type: "terminal", small: true });
});

it.each(["svg", "png", "utf8"])("leaves type=%s options untouched", (type) => {
const mod = makeQrcodeFake();
patchQrcode(mod);
mod.toString("payload", { type });
expect(mod.calls[0].opts).toEqual({ type });
expect((mod.calls[0].opts as Record<string, unknown>).small).toBeUndefined();
});

it("does not mutate the caller-supplied options object", () => {
const mod = makeQrcodeFake();
patchQrcode(mod);
const opts = { type: "terminal" };
mod.toString("payload", opts);
expect(opts).toEqual({ type: "terminal" });
});

it("preserves the toString(text, cb) signature", () => {
const mod = makeQrcodeFake();
patchQrcode(mod);
const cb = vi.fn();
mod.toString("payload", cb);
expect(mod.calls[0].cb).toBe(cb);
// No opts object was supplied, so nothing is forced.
expect(mod.calls[0].opts).toEqual({});
});

it("is idempotent: double-patch does not re-wrap", () => {
const mod = makeQrcodeFake();
patchQrcode(mod);
const wrappedOnce = mod.toString;
patchQrcode(mod);
expect(mod.toString).toBe(wrappedOnce);
// And forcing still works exactly once.
mod.toString("payload", { type: "terminal" });
expect(mod.calls[0].opts).toEqual({ type: "terminal", small: true });
});
});

describe("patchQrcodeTerminal (#4522)", () => {
it("forces small:true on generate", () => {
const mod = makeQrcodeTerminalFake();
patchQrcodeTerminal(mod);
mod.generate("payload", {});
expect(mod.calls[0].opts).toEqual({ small: true });
});

it("is idempotent: double-patch does not re-wrap", () => {
const mod = makeQrcodeTerminalFake();
patchQrcodeTerminal(mod);
const wrappedOnce = mod.generate;
patchQrcodeTerminal(mod);
expect(mod.generate).toBe(wrappedOnce);
});
});

describe("Module._load hook path-segment matching (#4522)", () => {
it('patches import("qrcode")\'s resolved absolute path', async () => {
// Simulate the real load hook: install a Module._load wrapper identical to
// the runtime's, then require by an ABSOLUTE resolved path (as import()
// bottoms out at) and confirm the returned module got the compact patch.
const Module = (await import("node:module")).default as unknown as {
_load: (...args: unknown[]) => unknown;
};
const qrcodeFake = makeQrcodeFake();
const absolutePath = "/tmp/app/node_modules/qrcode/lib/index.js";
const origLoad = Module._load;
Module._load = function (request: unknown, ..._rest: unknown[]) {
const loaded = request === absolutePath ? qrcodeFake : {};
if (typeof request === "string" && request.indexOf("qrcode") !== -1) {
if (isQrcodePackage(loaded)) return patchQrcode(loaded);
if (isQrcodeTerminalPackage(loaded)) return patchQrcodeTerminal(loaded);
}
return loaded;
};
try {
const loaded = Module._load(absolutePath) as ReturnType<typeof makeQrcodeFake>;
expect(loaded).toBe(qrcodeFake);
loaded.toString("payload", { type: "terminal" });
expect(loaded.calls[0].opts).toEqual({ type: "terminal", small: true });
} finally {
Module._load = origLoad;
}
});
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Loading
Loading