test/e2e/live/full-e2e.test.ts (1)

194-222: 🚀 Performance & Scalability | 🔵 Trivial

Consider validating the cold-path budget before the expensive onboarding run.

readFullE2eColdPathBudget() is only invoked here, after input.install and the first agent turn have already completed on the hosted runner. If ci/onboard-performance-budget.json is malformed, the failure surfaces only after the full (expensive, credential-gated) cold-onboard cycle finishes. This is largely mitigated by the separate schema/unit tests catching malformed configs earlier in CI, but hoisting the budget read to the start of the test would fail fast and avoid burning a live cold-onboard run on a config typo that slipped past those checks.

🤖 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/e2e/live/full-e2e.test.ts` around lines 194 - 222, Move the
readFullE2eColdPathBudget() call to the beginning of the cold-path test, before
input.install and the first agent turn execute, and reuse that budget for
evaluateColdOnboardPerformance and the artifact output. Preserve the existing
budget validation and reporting behavior while ensuring malformed configuration
fails before the expensive onboarding run.

test/e2e/live/full-e2e.test.ts (1)

142-155: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Load the budget before the expensive turn, not after.

readFullE2eColdPathBudget() is only called at line 194, after the network-bound execShell first-agent-turn call (lines 179-191, bounded by FIRST_TURN_TIMEOUT_MS) has already completed. If ci/onboard-performance-budget.json is missing or invalid, the test still pays for that round trip before discovering the config error. Since the budget read/validation is cheap and independent of the timed cold-onboard window, hoist it to the top of assertColdOnboardPerformance (or even before readAndDeleteTraceWindow) to fail fast on bad config.

♻️ Proposed reordering

 ): Promise<void> {
+  const budget = readFullE2eColdPathBudget();
   const traceWindow = readAndDeleteTraceWindow(input.traceFile, input.traceDirectory);
   ...
   const totalMs = Date.now() - traceWindow.startedAtMs;
   const totalSecs = Math.ceil(totalMs / 1_000);
-  const budget = readFullE2eColdPathBudget();
   const performance = evaluateColdOnboardPerformance(traceWindow, totalMs, budget);

Also applies to: 192-195

🤖 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/e2e/live/full-e2e.test.ts` around lines 142 - 155, Move the
readFullE2eColdPathBudget() call to the beginning of
assertColdOnboardPerformance, before readAndDeleteTraceWindow and the
network-bound execShell first-agent-turn operation. Preserve the existing budget
validation and later use, ensuring missing or invalid configuration fails before
the timed cold-onboard work starts.

test/e2e/live/full-e2e.test.ts (1)

200-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Local performance variable shadows the global performance object.

const performance = evaluateColdOnboardPerformance(...) shadows Node's built-in perf_hooks.performance global for the rest of the function. It's harmless today since Date.now() is used for timing elsewhere in this scope, but it's a footgun if someone later reaches for performance.now() inside this function and instead silently gets the evaluation result object.

♻️ Suggested rename

-  const performance = evaluateColdOnboardPerformance(
+  const performanceEvaluation = evaluateColdOnboardPerformance(
     traceWindow,
     firstTurnCompletedAtMs,
     input.budget,
   );
   const rootStartToFirstTurnCompletionSecs = Math.ceil(
-    performance.rootStartToFirstTurnCompletionMs / 1_000,
+    performanceEvaluation.rootStartToFirstTurnCompletionMs / 1_000,
   );

(and update the remaining performance.* references at lines 222, 225, 232-234, 258-260 accordingly)

🤖 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/e2e/live/full-e2e.test.ts` around lines 200 - 234, Rename the local
result from evaluateColdOnboardPerformance in the current test function to avoid
shadowing the global performance object, and update all references to its
rootStartToFirstTurnCompletionMs, rootEndToFirstTurnCompletionMs, passed, and
violations properties accordingly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/e2e/support/onboard-performance.test.ts (1)

1-12: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
sed -n '1,15p' test/e2e/support/onboard-performance.test.ts

Repository: NVIDIA/NemoClaw

Length of output: 743

---

🏁 Script executed:

sed -n '1,120p' test/e2e/support/onboard-performance.test.ts && printf '\n--- SEARCH ---\n' && rg -n "`@module-tag`|credential-free" test/e2e/support/onboard-performance.test.ts

Repository: NVIDIA/NemoClaw

Length of output: 4271

---

Add the credential-free module tag
test/e2e/support/onboard-performance.test.ts should include @module-tag e2e/credential-free at the top so it’s treated as a credential-free E2E test.

🤖 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/e2e/support/onboard-performance.test.ts` around lines 1 - 12, Add the
`@module-tag` e2e/credential-free marker at the top of the onboard-performance
test module, before the existing imports, so the test is classified as
credential-free.

Source: Path instructions

src/lib/onboard/machine/handlers/gateway.test.ts (1)

103-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Ternary-as-statement for side effects risks a no-unused-expressions lint failure.

The restore/delete logic correctly fixes the previously flagged issue (env var is now captured and restored instead of unconditionally deleted). However, using a bare ternary expression as a statement (lines 104-106) is flagged as incorrect by ESLint's no-unused-expressions rule under its default options (allowTernary: false), even when both branches are function calls. If this repo's ESLint config doesn't set allowTernary: true, this pattern will fail lint. An if/else is more idiomatic here and avoids the risk entirely.

♻️ Suggested fix

   } finally {
-    previousTraceFile === undefined
-      ? Reflect.deleteProperty(process.env, TRACE_FILE_ENV)
-      : Reflect.set(process.env, TRACE_FILE_ENV, previousTraceFile);
+    if (previousTraceFile === undefined) {
+      delete process.env[TRACE_FILE_ENV];
+    } else {
+      process.env[TRACE_FILE_ENV] = previousTraceFile;
+    }
     resetTraceForTests();
     fs.rmSync(directory, { recursive: true, force: true });
   }

Based on learnings, this mirrors the previously suggested fix for restoring TRACE_FILE_ENV, which used plain delete/assignment rather than Reflect. Confirm whether no-unused-expressions/allowTernary is configured for this repo before deciding whether to adjust.

🤖 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/machine/handlers/gateway.test.ts` around lines 103 - 106,
Replace the bare ternary side-effect statement in the finally cleanup with an
explicit if/else: delete TRACE_FILE_ENV when previousTraceFile is undefined,
otherwise restore it to the captured value. Keep the existing previousTraceFile
handling and environment restoration behavior unchanged.

Source: Learnings