chore(tooling): enforce honest coverage reporting#3154
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (10)
📝 WalkthroughWalkthroughThis PR consolidates build/lint/check tooling from scattered command invocations into centralized npm scripts, introduces automated coverage-ignore directive enforcement, removes v8 ignore comments from source files, and implements sandbox gateway probing, policy/channel management, session detection, and snapshot state reconciliation. ChangesBuild Infrastructure Consolidation
Coverage Visibility Enforcement
Sandbox Gateway & Policy Management Features
Command Dependencies & CLI Adapters
Test Infrastructure Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
package.json (1)
28-28: ⚡ Quick winAdd
--checkflag to enforce format validation in CI.The current
format:checkcommand runs in write mode (exits 0 even when formatting issues exist), so it won't fail CI builds when code doesn't match format expectations. The--checkflag reports violations as failures (exit code 1), making it suitable for blocking non-compliant commits.Suggested patch
- "format:check": "npx `@biomejs/biome` format .", + "format:check": "npx `@biomejs/biome` format --check .",🤖 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 `@package.json` at line 28, The "format:check" npm script currently runs Biome in write/format mode and won't fail CI; update the "format:check" script entry so it invokes Biome with the --check flag (i.e., run npx `@biomejs/biome` --check .) so format violations return a non-zero exit code; modify the package.json "format:check" script string accordingly (refer to the "format:check" script name to locate the entry).
🤖 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 `@scripts/checks/no-coverage-ignore.ts`:
- Around line 20-21: The current check uses FORBIDDEN_DIRECTIVE ("v8 ignore")
and flags any occurrence even inside string literals; update the scan so it only
matches actual comment tokens by detecting comment prefixes (// and /* ... */)
before searching for FORBIDDEN_DIRECTIVE. Modify the logic that reads lines (the
code referencing FORBIDDEN_DIRECTIVE) to first extract comment text (handle
single-line '//' and block '/*...*/') and then check that extracted comment
contains FORBIDDEN_DIRECTIVE, preserving existing reporting behavior when found.
In `@src/lib/commands/gateway-token.ts`:
- Around line 53-55: The stdout error handler currently uses
process.stdout.on("error", ...) and only exits on EPIPE, which suppresses
non-EPIPE errors and leaks listeners across repeated run() calls; change this to
add a one-time listener (use once instead of on) on process.stdout and for
non-EPIPE errors re-emit or propagate the error rather than silently swallowing
it (e.g., call process.emit('uncaughtException', err) or log and exit with
non-zero), and ensure any listener is removed after handling so repeated
invocations of the run/command do not accumulate listeners; key symbols to
modify: process.stdout.on("error", ...), the err.code === "EPIPE" branch, and
the process.exit(0) behavior.
---
Nitpick comments:
In `@package.json`:
- Line 28: The "format:check" npm script currently runs Biome in write/format
mode and won't fail CI; update the "format:check" script entry so it invokes
Biome with the --check flag (i.e., run npx `@biomejs/biome` --check .) so format
violations return a non-zero exit code; modify the package.json "format:check"
script string accordingly (refer to the "format:check" script name to locate the
entry).
🪄 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: d12d63ff-00d4-4676-829d-412f524bde92
📒 Files selected for processing (41)
.pre-commit-config.yamlMakefileci/coverage-threshold-cli.jsonpackage.jsonscripts/check-coverage-ratchet.tsscripts/checks/direct-credential-env.tsscripts/checks/layer-import-boundaries.tsscripts/checks/no-coverage-ignore.tsscripts/checks/run.tssrc/lib/actions/maintenance.tssrc/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/destroy.tssrc/lib/actions/sandbox/doctor.tssrc/lib/actions/sandbox/gateway-state.tssrc/lib/actions/sandbox/logs.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/actions/sandbox/runtime.tssrc/lib/actions/sandbox/skill-install.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/actions/sandbox/status.tssrc/lib/actions/uninstall-plan.tssrc/lib/actions/uninstall-run-plan.tssrc/lib/actions/upgrade-sandboxes.tssrc/lib/adapters/openshell/runtime.tssrc/lib/commands/gateway-token.tssrc/lib/commands/uninstall.tssrc/lib/domain/policy-channel.tssrc/lib/domain/sandbox/destroy.tssrc/lib/domain/sandbox/logs.tssrc/lib/domain/uninstall/paths.tssrc/lib/domain/uninstall/plan.tssrc/lib/domain/uninstall/shims.tssrc/lib/list-command-deps.tssrc/lib/recover-cli-command.tssrc/lib/sandbox-config-set-cli-command.tssrc/lib/status-command-deps.tstest/layer-import-boundaries.test.tstest/no-direct-credential-env.test.tstest/preinstall-node-version.test.ts
💤 Files with no reviewable changes (27)
- src/lib/domain/uninstall/shims.ts
- src/lib/domain/policy-channel.ts
- src/lib/actions/sandbox/status.ts
- src/lib/actions/maintenance.ts
- src/lib/domain/uninstall/paths.ts
- src/lib/sandbox-config-set-cli-command.ts
- src/lib/actions/sandbox/runtime.ts
- src/lib/actions/sandbox/skill-install.ts
- src/lib/domain/sandbox/destroy.ts
- src/lib/domain/sandbox/logs.ts
- src/lib/domain/uninstall/plan.ts
- src/lib/recover-cli-command.ts
- src/lib/actions/sandbox/connect.ts
- src/lib/list-command-deps.ts
- src/lib/actions/upgrade-sandboxes.ts
- src/lib/actions/uninstall-plan.ts
- src/lib/actions/sandbox/snapshot.ts
- src/lib/status-command-deps.ts
- src/lib/actions/sandbox/destroy.ts
- src/lib/actions/sandbox/logs.ts
- src/lib/actions/sandbox/doctor.ts
- src/lib/adapters/openshell/runtime.ts
- src/lib/actions/sandbox/process-recovery.ts
- src/lib/actions/uninstall-run-plan.ts
- src/lib/actions/sandbox/policy-channel.ts
- src/lib/actions/sandbox/gateway-state.ts
- src/lib/actions/sandbox/rebuild.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
…-and-script-shims
wscurran
added
CI/CD
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/commands/gateway-token.ts (1)
69-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the stdout error listener after each successful run.
Line 69 uses
once, but if no stdout"error"happens, the listener stays registered. Repeated in-processrun()calls can accumulate pending listeners and later trigger multiple handlers on one error.Suggested patch
- process.stdout.once("error", (err: NodeJS.ErrnoException) => { + const onStdoutError = (err: NodeJS.ErrnoException): never | void => { if (err.code === "EPIPE") { process.exit(0); return; } throw err; - }); + }; + process.stdout.once("error", onStdoutError); - const runtime = getRuntimeBridge(); - const exitCode = runGatewayTokenCommand( - args.sandboxName, - { quiet: flags.quiet === true }, - { - fetchToken: runtime.fetchGatewayAuthTokenFromSandbox, - getSandboxAgent: runtime.getSandboxAgent, - }, - ); - // NCQ `#3180`: avoid this.exit(code), which throws `@oclif/core` ExitError. - // The legacy `nemoclaw <name> gateway-token` dispatch did not catch the - // throw, leaking a raw JS stack trace to the user. Always assigning - // process.exitCode keeps the diagnostic output clean and prevents a - // stale non-zero code from a prior run() in the same process from - // bleeding through on a successful invocation. - process.exitCode = exitCode; + try { + const runtime = getRuntimeBridge(); + const exitCode = runGatewayTokenCommand( + args.sandboxName, + { quiet: flags.quiet === true }, + { + fetchToken: runtime.fetchGatewayAuthTokenFromSandbox, + getSandboxAgent: runtime.getSandboxAgent, + }, + ); + // NCQ `#3180`: avoid this.exit(code), which throws `@oclif/core` ExitError. + // The legacy `nemoclaw <name> gateway-token` dispatch did not catch the + // throw, leaking a raw JS stack trace to the user. Always assigning + // process.exitCode keeps the diagnostic output clean and prevents a + // stale non-zero code from a prior run() in the same process from + // bleeding through on a successful invocation. + process.exitCode = exitCode; + } finally { + process.stdout.off("error", onStdoutError); + }#!/bin/bash # Verify registration exists and cleanup is (or is not) present. rg -nP --type=ts -C2 'process\.stdout\.once\("error"\s*,\s*' src/lib/commands/gateway-token.ts rg -nP --type=ts -C2 'process\.stdout\.(off|removeListener)\("error"\s*,' src/lib/commands/gateway-token.ts🤖 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/commands/gateway-token.ts` around lines 69 - 75, The temporary stdout error listener added with process.stdout.once("error", ...) can remain registered across repeated in-process run() calls if no error occurs; fix by extracting the listener into a named function (e.g. const onStdoutError = (err: NodeJS.ErrnoException) => { ... }) and register it with process.stdout.once("error", onStdoutError), then always remove it after a successful run (process.stdout.removeListener("error", onStdoutError) or process.stdout.off(...)) in the normal completion/cleanup path of the function that invokes this listener (the run() / gateway-token command flow) so no stale listeners accumulate.
🤖 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.
Duplicate comments:
In `@src/lib/commands/gateway-token.ts`:
- Around line 69-75: The temporary stdout error listener added with
process.stdout.once("error", ...) can remain registered across repeated
in-process run() calls if no error occurs; fix by extracting the listener into a
named function (e.g. const onStdoutError = (err: NodeJS.ErrnoException) => { ...
}) and register it with process.stdout.once("error", onStdoutError), then always
remove it after a successful run (process.stdout.removeListener("error",
onStdoutError) or process.stdout.off(...)) in the normal completion/cleanup path
of the function that invokes this listener (the run() / gateway-token command
flow) so no stale listeners accumulate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 67659282-6bb5-4a49-923c-399adc78b295
📒 Files selected for processing (20)
package.jsonscripts/checks/no-coverage-ignore.tssrc/lib/actions/maintenance.tssrc/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/destroy.tssrc/lib/actions/sandbox/doctor.tssrc/lib/actions/sandbox/gateway-state.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/actions/sandbox/skill-install.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/actions/sandbox/status.tssrc/lib/actions/uninstall/plan.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/actions/upgrade-sandboxes.tssrc/lib/commands/gateway-token.tssrc/lib/commands/uninstall.tssrc/lib/nim.test.tstest/no-coverage-ignore.test.ts
💤 Files with no reviewable changes (14)
- src/lib/actions/maintenance.ts
- src/lib/actions/upgrade-sandboxes.ts
- src/lib/actions/sandbox/status.ts
- src/lib/actions/uninstall/run-plan.ts
- src/lib/actions/sandbox/rebuild.ts
- src/lib/actions/uninstall/plan.ts
- src/lib/actions/sandbox/skill-install.ts
- src/lib/actions/sandbox/connect.ts
- src/lib/actions/sandbox/doctor.ts
- src/lib/actions/sandbox/snapshot.ts
- src/lib/actions/sandbox/process-recovery.ts
- src/lib/actions/sandbox/destroy.ts
- src/lib/actions/sandbox/policy-channel.ts
- src/lib/actions/sandbox/gateway-state.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/commands/uninstall.ts
- test/no-coverage-ignore.test.ts
Contributor
Author
|
Maintainer update: synced this branch with current main and resolved the CodeRabbit follow-ups. Validation:
Current merge state is MERGEABLE/BLOCKED only because review is still required. |
Contributor
Author
|
Maintainer update: resynced with latest main again and resolved the new conflict from the inference/onboard module move.\n\nValidation:\n- |
cjagwani
approved these changes
May 8, 2026
Contributor
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
LGTM, approving. Coverage rebaseline is the right call, v8 ignore directives were hiding real gaps, and once this lands the next coverage run on main will be the honest baseline.
cjagwani
approved these changes
May 8, 2026
Contributor
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
LGTM, approving. Coverage rebaseline is the right call — v8 ignore directives were hiding real gaps, and once this lands the next coverage run on main will be the honest baseline.
One small thing for future readers: the process.stdout.on(...) → once(...) + rethrow change in src/lib/commands/gateway-token.ts:69-75 is a real behavior shift (was silently swallowing non-EPIPE stdout errors, now surfaces them). It's almost certainly the right call — EBADF/EIO shouldn't be swallowed — but it's the only semantic change in an otherwise pure-tooling PR. Worth a one-line mention in the body or a follow-up commit message so anyone bisecting later doesn't trip on it. Not a blocker.
Thanks for the cleanup.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remove source-level V8 coverage ignore directives so CLI coverage reports include previously hidden orchestration code. Add repository checks to keep coverage directives out of source and convert Makefile targets into npm-script-backed compatibility shims.
Changes
v8 ignoredirectives from CLI action, domain, adapter, and command modules.scripts/checks/no-coverage-ignore.tsandscripts/checks/run.ts, and wired the generic repository checks into prek andnpm run lint.scripts/checks/while keeping specialized lifecycle checks at their existing top-levelscripts/check-*paths.ci/coverage-threshold-cli.json.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests