refactor(types): consolidate duplicated errno and JSON type helpers by jyaunches · Pull Request #2447 · NVIDIA/NemoClaw

jyaunches · 2026-04-24T19:43:58Z

Summary

Follow-up to PR #2422 (refactor(types): tighten repo-wide type boundaries). Addresses the review warnings and suggestions from triage.

Changes

1. Shared `isErrnoException` / `isPermissionError` — `src/lib/errno.ts`

PR #2422 removed all @ts-nocheck markers, which required each module to define its own isErrnoException + ErrnoLike type locally. This resulted in 6 slightly different copies across config-io, credentials, http-probe, onboard, onboard-session, and registry.

This PR extracts a single canonical version into src/lib/errno.ts that:

Accepts unknown so callers never need an instanceof Error pre-cast
Checks for both code and errno properties (covering all Node.js error shapes)
Includes a convenience isPermissionError for the common EACCES/EPERM guard

This also simplifies many catch blocks that previously did error instanceof Error && isErrnoException(error) — the redundant instanceof Error check is now handled inside the shared function.

2. Shared JSON recursive types — `src/lib/json-types.ts`

PR #2422 introduced identical Scalar/Value/Object type triples in multiple modules:

LooseScalar / LooseValue / LooseObject in onboard.ts and agent-onboard.ts
PolicyScalar / PolicyValue / PolicyObject in policies.ts
SessionJsonPrimitive / SessionJsonValue / UnknownRecord in onboard-session.ts

This PR extracts canonical JsonScalar / JsonValue / JsonObject types into src/lib/json-types.ts. Each module re-aliases them under its domain names for readability.

Note: The plugin types (PluginScalar/PluginValue/PluginRecord in nemoclaw/src/index.ts) are intentionally kept in-place because the plugin compiles separately from the CLI and cannot share imports.

3. `Reflect.get` / `Reflect.set` convention comment

Adds a clarifying comment at the first usage site (usage-notice.ts) explaining why Reflect.get is preferred over as Record<…> casts throughout the codebase — it avoids widening the target type and satisfies no-unsafe-member-access lint rules.

4. `ws-proxy-fix.ts` behavioral guard documentation

Documents the !host null guard in the Discord WebSocket tunnel path, explaining that the previous code would have attempted a tunnel with an undefined host (which would fail downstream anyway). The guard now explicitly falls through to the original https.request unchanged.

Type of Change

Code change (feature, bug fix, or refactor)
Code change with doc updates
Doc only (prose changes, no code sample modifications)
Doc only (includes code sample changes)

Verification

tsc -p tsconfig.cli.json --noEmit passes
npm test passes (only pre-existing failures in install-preflight and ssrf-parity)
Tests added for new errno.ts module (15 test cases)
No secrets, API keys, or credentials committed
Docs updated for user-facing behavior changes
make docs builds without warnings (doc changes only)

AI Disclosure

AI-assisted — tool: pi coding agent

Signed-off-by: Jess Yaunches jyaunches@nvidia.com

Summary by CodeRabbit

Bug Fixes
- Improved WebSocket proxy handling to avoid creating tunnels when the request target host is undefined.
Tests
- Added tests validating errno/permission detection utilities.
Documentation
- Added explanatory comment clarifying property-access rationale.
Chores
- Centralized JSON-type aliases and errno helpers for consistent error handling across modules.

Follow-up to PR #2422. Addresses review warnings and suggestions: 1. Extract shared isErrnoException/isPermissionError into src/lib/errno.ts - Unifies 6 duplicate definitions (config-io, credentials, http-probe, onboard, onboard-session, registry) into a single source of truth - Accepts `unknown` so callers never need instanceof Error pre-casts - Simplifies catch blocks across onboard-session.ts and registry.ts 2. Extract shared JSON recursive types into src/lib/json-types.ts - Unifies LooseScalar/LooseValue/LooseObject (onboard, agent-onboard), PolicyScalar/PolicyValue/PolicyObject (policies), and SessionJsonPrimitive/SessionJsonValue (onboard-session) - Plugin types (PluginScalar/PluginValue/PluginRecord) kept in-place since nemoclaw/src/ compiles separately from the CLI 3. Add Reflect.get/Reflect.set convention comment in usage-notice.ts explaining why the pattern is preferred over `as Record<…>` casts 4. Document the ws-proxy-fix.ts !host guard behavioral change explaining why it falls through to the original request instead of attempting a tunnel with an undefined host Signed-off-by: Jess Yaunches <jyaunches@nvidia.com>

coderabbitai · 2026-04-24T19:44:10Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5fc2dba4-ecd5-4057-ae4d-f9f2e6e8959c

📥 Commits

Reviewing files that changed from the base of the PR and between d36a6a3 and 3a9f55b.

📒 Files selected for processing (2)

src/lib/config-io.ts
src/lib/policies.ts

✅ Files skipped from review due to trivial changes (1)

src/lib/policies.ts

📝 Walkthrough

Walkthrough

Consolidates errno and JSON type utilities into new shared modules (src/lib/errno.ts, src/lib/json-types.ts), updates many modules to import those utilities instead of local definitions, adds a defensive fallback in the Discord WebSocket upgrade detection to avoid tunnel construction when host is undefined, and adds tests for the errno helpers.

Changes

Cohort / File(s)	Summary
WebSocket Proxy Defensive Fix `nemoclaw-blueprint/scripts/ws-proxy-fix.js`, `nemoclaw-blueprint/scripts/ws-proxy-fix.ts`	Detects Discord WebSocket upgrades where normalized `host` is `undefined` and returns early by calling the original `https.request` instead of injecting/constructing a tunnel agent.
New Error Handling Utilities `src/lib/errno.ts`	Adds shared type-guard functions `isErrnoException(error: unknown): error is NodeJS.ErrnoException` and `isPermissionError(error: unknown): error is NodeJS.ErrnoException`.
New JSON Type Definitions `src/lib/json-types.ts`	Adds shared type aliases `JsonScalar`, `JsonValue`, and `JsonObject` for loosely-typed JSON shapes.
Errno Utility Migration `src/lib/config-io.ts`, `src/lib/credentials.ts`, `src/lib/http-probe.ts`, `src/lib/registry.ts`, `src/lib/onboard-session.ts`, `src/lib/onboard.ts`	Replace local errno-like type guards with imports from `./errno`; update catch branches to use `isErrnoException`/`isPermissionError` and keep existing `.code` checks.
JSON Type Aliases Migration `src/lib/agent-onboard.ts`, `src/lib/policies.ts`, `src/lib/onboard-session.ts`, `src/lib/onboard.ts`	Replace file-local loose JSON scalar/object types with imports from `./json-types`; update aliases and function/type signatures to reference centralized types.
Documentation Addition `src/lib/usage-notice.ts`	Adds a multiline comment explaining `Reflect.get` usage for `readStringProperty` and related eslint rationale.
Error Utilities Test Suite `test/errno.test.ts`	Adds tests verifying `isErrnoException` and `isPermissionError` behavior, including narrowing and expected true/false cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A hop, a fix, a tidy type,
Errno and JSON now take flight,
When hosts are missing, requests stay sane,
Tests hop in to guard the gain,
Code nibbles neat — a rabbit’s delight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main objective: consolidating duplicated errno and JSON type helper implementations across multiple modules into shared utility modules.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch refactor/type-precision-followups

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

…on-followups

… top Address self-review nits: - Add clarifying comment on config-io.ts local JSON types explaining why they differ from json-types.ts (no undefined in strict JSON) - Move import statement in policies.ts above CJS require() block for consistent style with other modules Signed-off-by: Jess Yaunches <jyaunches@nvidia.com>

Add non-null assertions for Array.pop() returns and explicit parameter types for .some()/.find() callbacks that lost type inference after the errno/JSON type consolidation in #2447. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…VIDIA#2447) ## Summary Follow-up to PR NVIDIA#2422 (`refactor(types): tighten repo-wide type boundaries`). Addresses the review warnings and suggestions from triage. ## Changes ### 1. Shared `isErrnoException` / `isPermissionError` — `src/lib/errno.ts` PR NVIDIA#2422 removed all `@ts-nocheck` markers, which required each module to define its own `isErrnoException` + `ErrnoLike` type locally. This resulted in 6 slightly different copies across `config-io`, `credentials`, `http-probe`, `onboard`, `onboard-session`, and `registry`. This PR extracts a single canonical version into `src/lib/errno.ts` that: - Accepts `unknown` so callers never need an `instanceof Error` pre-cast - Checks for both `code` and `errno` properties (covering all Node.js error shapes) - Includes a convenience `isPermissionError` for the common EACCES/EPERM guard This also simplifies many catch blocks that previously did `error instanceof Error && isErrnoException(error)` — the redundant `instanceof Error` check is now handled inside the shared function. ### 2. Shared JSON recursive types — `src/lib/json-types.ts` PR NVIDIA#2422 introduced identical Scalar/Value/Object type triples in multiple modules: - `LooseScalar` / `LooseValue` / `LooseObject` in `onboard.ts` and `agent-onboard.ts` - `PolicyScalar` / `PolicyValue` / `PolicyObject` in `policies.ts` - `SessionJsonPrimitive` / `SessionJsonValue` / `UnknownRecord` in `onboard-session.ts` This PR extracts canonical `JsonScalar` / `JsonValue` / `JsonObject` types into `src/lib/json-types.ts`. Each module re-aliases them under its domain names for readability. > **Note:** The plugin types (`PluginScalar`/`PluginValue`/`PluginRecord` in `nemoclaw/src/index.ts`) are intentionally kept in-place because the plugin compiles separately from the CLI and cannot share imports. ### 3. `Reflect.get` / `Reflect.set` convention comment Adds a clarifying comment at the first usage site (`usage-notice.ts`) explaining why `Reflect.get` is preferred over `as Record<…>` casts throughout the codebase — it avoids widening the target type and satisfies `no-unsafe-member-access` lint rules. ### 4. `ws-proxy-fix.ts` behavioral guard documentation Documents the `!host` null guard in the Discord WebSocket tunnel path, explaining that the previous code would have attempted a tunnel with an undefined host (which would fail downstream anyway). The guard now explicitly falls through to the original `https.request` unchanged. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `tsc -p tsconfig.cli.json --noEmit` passes - [x] `npm test` passes (only pre-existing failures in install-preflight and ssrf-parity) - [x] Tests added for new `errno.ts` module (15 test cases) - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) ## AI Disclosure - [x] AI-assisted — tool: pi coding agent --- Signed-off-by: Jess Yaunches <jyaunches@nvidia.com>  ## Summary by CodeRabbit * **Bug Fixes** * Improved WebSocket proxy handling to avoid creating tunnels when the request target host is undefined. * **Tests** * Added tests validating errno/permission detection utilities. * **Documentation** * Added explanatory comment clarifying property-access rationale. * **Chores** * Centralized JSON-type aliases and errno helpers for consistent error handling across modules.  --------- Signed-off-by: Jess Yaunches <jyaunches@nvidia.com>

jyaunches self-assigned this Apr 24, 2026

jyaunches added 2 commits April 24, 2026 15:52

Merge remote-tracking branch 'origin/main' into refactor/type-precisi…

6619f88

…on-followups

cv approved these changes Apr 24, 2026

View reviewed changes

jyaunches merged commit 260b237 into main Apr 24, 2026
18 checks passed

wscurran added the refactor PR restructures code without intended behavior change label Jun 8, 2026

jyaunches deleted the refactor/type-precision-followups branch June 12, 2026 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(types): consolidate duplicated errno and JSON type helpers#2447

refactor(types): consolidate duplicated errno and JSON type helpers#2447
jyaunches merged 3 commits into
mainfrom
refactor/type-precision-followups

jyaunches commented Apr 24, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 24, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jyaunches commented Apr 24, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

1. Shared isErrnoException / isPermissionError — src/lib/errno.ts

2. Shared JSON recursive types — src/lib/json-types.ts

3. Reflect.get / Reflect.set convention comment

4. ws-proxy-fix.ts behavioral guard documentation

Type of Change

Verification

AI Disclosure

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jyaunches commented Apr 24, 2026 •

edited by coderabbitai Bot

Loading

1. Shared `isErrnoException` / `isPermissionError` — `src/lib/errno.ts`

2. Shared JSON recursive types — `src/lib/json-types.ts`

3. `Reflect.get` / `Reflect.set` convention comment

4. `ws-proxy-fix.ts` behavioral guard documentation

coderabbitai Bot commented Apr 24, 2026 •

edited

Loading