chore(blueprint): add pyright strict type checking by cv · Pull Request #523 · NVIDIA/NemoClaw

cv · 2026-03-20T22:35:00Z

Summary

Adds pyright in strict mode for nemoclaw-blueprint/ Python code
Integrated into make check via uv run --with pyright pyright .
No new project dependencies — pyright runs ephemerally via uv

Motivation

AI agents contributing to this repo can introduce subtle type errors in Python code. Unlike TypeScript (which already has tsc --strict), the Python side had no type checker. This closes that gap.

Test plan

cd nemoclaw-blueprint && make check passes (ruff + pyright)
make check from root still works
CI lint job catches type errors in PRs

🤖 Generated with Claude Code

Summary by CodeRabbit

Chores
- Added a CI lint job on PRs that provisions Node & Python and runs repository-wide linting.
- Enhanced verification to include Python type-checking (pyright) alongside existing linters.
- Relaxed TypeScript linting rules for test files and adjusted project fallback behavior.
- Applied non-behavioral code formatting and minor style/format refinements across the codebase.

Adds a dedicated lint job that runs `make check` on every PR, covering ESLint, Prettier, tsc --noEmit (TypeScript) and ruff (Python). Previously, lint failures were only caught locally — now they block merge. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Adds pyright in strict mode to the Python nemoclaw-blueprint package, integrated into `make check`. This catches type errors locally and in CI, which is especially important for AI-agent-authored contributions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-03-20T22:35:17Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a repository-wide lint job to the PR GitHub Actions workflow, integrates Pyright into Makefile checks with strict Python 3.11 settings, tightens ESLint test-file exceptions, and applies non-functional TypeScript formatting/rewrites.

Changes

Cohort / File(s)	Summary
CI/CD workflow `.github/workflows/pr.yaml`	Added a new `jobs.lint` job that provisions Node.js (v22) and Python (3.11), installs `uv` and `ruff`, runs `npm install` in `nemoclaw`, and executes `make check`.
Makefile & Pyright config `nemoclaw-blueprint/Makefile`, `nemoclaw-blueprint/pyproject.toml`	Appended `uv run --with pyright pyright .` to the `check` target; added `[tool.pyright]` with `pythonVersion = "3.11"`, `typeCheckingMode = "strict"`, and `include = ["orchestrator", "migrations"]`.
ESLint adjustments `nemoclaw/eslint.config.mjs`	Changed `parserOptions.projectService` from `true` to an object allowing a default project for test files and added a config block disabling `@typescript-eslint.no-unsafe-assignment` and `@typescript-eslint/no-unsafe-member-access` for `src/*/.test.ts`.
Non-functional TypeScript edits `nemoclaw/src/commands/migration-state.ts`, `nemoclaw/src/index.ts`	Reformatted expressions and function signatures into multi-line layouts; no API/behavior changes.

Sequence Diagram(s)

sequenceDiagram
  participant GH as GitHub Actions Runner
  participant Node as Node.js v22
  participant Py as Python 3.11
  participant UV as uv
  participant NPM as npm (nemoclaw)
  participant Make as make check

  GH->>Node: Provision Node.js v22
  GH->>Py: Provision Python 3.11
  GH->>UV: Install uv
  UV->>Py: uv installs ruff
  GH->>NPM: Run `npm install` in `nemoclaw`
  GH->>Make: Run `make check` (ruff, pyright, format checks)
  Make-->>GH: Linting results

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hopped into CI with a whiskered cheer,

Linted the paths both far and near,
Types checked strict, formats stood tall,
Tests kept cozy within their hall,
A little rabbit applauds the clear gear.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'chore(blueprint): add pyright strict type checking' clearly and concisely describes the main change: adding Pyright with strict type checking to the blueprint package.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

coderabbitai

🧹 Nitpick comments (1)

.github/workflows/pr.yaml (1)

28-29: npm cache may require a lockfile.

The cache: npm option typically requires package-lock.json to exist in the repository root (or the directory specified by cache-dependency-path). If the lockfile is in nemoclaw/ instead, consider adding cache-dependency-path: nemoclaw/package-lock.json.

Proposed fix

       - name: Setup Node.js
         uses: actions/setup-node@v4
         with:
           node-version: "22"
           cache: npm
+          cache-dependency-path: nemoclaw/package-lock.json

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/pr.yaml around lines 28 - 29, The GitHub Actions workflow
currently uses "cache: npm" but doesn't point to the repository's lockfile;
update the workflow to add the cache-dependency-path key referencing the
lockfile location (e.g. set cache-dependency-path: "nemoclaw/package-lock.json")
or move the package-lock.json to the repo root so the existing cache: npm works;
modify the workflow's node-version/cache block to include the
cache-dependency-path entry so the npm cache is keyed correctly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @.github/workflows/pr.yaml:
- Around line 28-29: The GitHub Actions workflow currently uses "cache: npm" but
doesn't point to the repository's lockfile; update the workflow to add the
cache-dependency-path key referencing the lockfile location (e.g. set
cache-dependency-path: "nemoclaw/package-lock.json") or move the
package-lock.json to the repo root so the existing cache: npm works; modify the
workflow's node-version/cache block to include the cache-dependency-path entry
so the npm cache is keyed correctly.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 704e03f2-e288-4c0f-8b4d-15a66e2d2c58

📥 Commits

Reviewing files that changed from the base of the PR and between ab5048a and 0db938a.

📒 Files selected for processing (3)

.github/workflows/pr.yaml
nemoclaw-blueprint/Makefile
nemoclaw-blueprint/pyproject.toml

- Remove async from functions without await in fetch.ts, use Promise.reject/resolve instead - Add braces to void arrow expression in logs.ts - Configure allowDefaultProject for test files excluded from tsconfig - Relax no-unsafe-assignment/member-access rules for test files - Fix unused variable in status.test.ts - Apply prettier formatting to all source files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

nemoclaw/src/blueprint/fetch.ts (1)

49-60: ⚠️ Potential issue | 🟠 Major

Synchronous throw breaks the Promise contract in non-async function.

The downloadAndCache function at line 58 throws synchronously despite returning a Promise. Callers using promise chaining (.catch(), .then()) will not catch this error. Change to return Promise.reject(...) to match the Promise return type and align with the pattern used elsewhere in the file (fetchBlueprint and resolveLatestVersion).

Proposed fix

 export function downloadAndCache(
   registry: string,
   version: string,
 ): Promise<{ localPath: string; manifest: BlueprintManifest }> {
   // Future: HTTP fetch + tar extract + manifest parse
   void registry;
   const localPath = getCachedBlueprintPath(version);
   const manifest = readCachedManifest(version);
   if (!manifest) {
-    throw new Error(`Failed to read manifest after download for version ${version}`);
+    return Promise.reject(
+      new Error(`Failed to read manifest after download for version ${version}`),
+    );
   }
   return Promise.resolve({ localPath, manifest });
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/src/blueprint/fetch.ts` around lines 49 - 60, downloadAndCache
currently throws synchronously when readCachedManifest(version) returns falsy,
breaking the Promise contract; instead return a rejected Promise so callers can
catch it. In the downloadAndCache function (which uses getCachedBlueprintPath
and readCachedManifest), replace the synchronous throw with returning
Promise.reject(new Error(...)) and keep the same error message so the function
always returns a Promise that rejects on failure.

🧹 Nitpick comments (1)

nemoclaw/eslint.config.mjs (1)
11-13: Consider aligning allowDefaultProject pattern with the test override scope for consistency.

Line 12 uses src/*/*.test.ts while line 33 uses src/**/*.test.ts. While current test files at src/commands/status.test.ts are covered by both patterns, aligning them would prevent inconsistency if tests are added at deeper nesting levels.
Suggested change
         projectService: {
-          allowDefaultProject: ["src/**/*.test.ts"],
+          allowDefaultProject: ["src/**/*.test.ts"],
         },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/eslint.config.mjs` around lines 11 - 13, The projectService
configuration uses allowDefaultProject: ["src/*/*.test.ts"] which is narrower
than the test override pattern; update allowDefaultProject in the projectService
block to the recursive pattern "src/**/*.test.ts" to match the test override
scope so deeper-nested tests (and files like src/commands/status.test.ts) remain
consistently covered; modify the allowDefaultProject value within the
projectService object accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@nemoclaw/src/blueprint/fetch.ts`:
- Around line 49-60: downloadAndCache currently throws synchronously when
readCachedManifest(version) returns falsy, breaking the Promise contract;
instead return a rejected Promise so callers can catch it. In the
downloadAndCache function (which uses getCachedBlueprintPath and
readCachedManifest), replace the synchronous throw with returning
Promise.reject(new Error(...)) and keep the same error message so the function
always returns a Promise that rejects on failure.

---

Nitpick comments:
In `@nemoclaw/eslint.config.mjs`:
- Around line 11-13: The projectService configuration uses allowDefaultProject:
["src/*/*.test.ts"] which is narrower than the test override pattern; update
allowDefaultProject in the projectService block to the recursive pattern
"src/**/*.test.ts" to match the test override scope so deeper-nested tests (and
files like src/commands/status.test.ts) remain consistently covered; modify the
allowDefaultProject value within the projectService object accordingly.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8ff0f99a-d5f2-4353-9a40-2b5b0d6fc4e3

📥 Commits

Reviewing files that changed from the base of the PR and between 0db938a and b7ed27b.

📒 Files selected for processing (10)

nemoclaw/eslint.config.mjs
nemoclaw/src/blueprint/fetch.ts
nemoclaw/src/cli.ts
nemoclaw/src/commands/logs.ts
nemoclaw/src/commands/migrate.ts
nemoclaw/src/commands/migration-state.ts
nemoclaw/src/commands/onboard.ts
nemoclaw/src/commands/status.test.ts
nemoclaw/src/commands/status.ts
nemoclaw/src/index.ts

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

nemoclaw/src/commands/status.ts
nemoclaw/src/commands/logs.ts
nemoclaw/src/index.ts
nemoclaw/src/commands/onboard.ts
nemoclaw/src/commands/migration-state.ts
nemoclaw/src/commands/status.test.ts
nemoclaw/src/cli.ts
nemoclaw/src/commands/migrate.ts

# Conflicts: # nemoclaw/src/blueprint/fetch.ts # nemoclaw/src/cli.ts # nemoclaw/src/commands/logs.ts # nemoclaw/src/commands/migrate.ts # nemoclaw/src/commands/onboard.ts # nemoclaw/src/commands/status.test.ts # nemoclaw/src/commands/status.ts

…iles The glob `src/*/*.test.ts` missed files like `src/register.test.ts`. Changed to `src/**/*.test.ts` to cover test files at any depth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

typescript-eslint disallows ** in allowDefaultProject globs for performance reasons. Use explicit single-level patterns instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ericksoa

Clean setup — pyright strict mode via ephemeral uv run is elegant (no permanent dep), and the new CI lint job closes the gap between local and CI linting. ESLint test-file relaxations are reasonable.

* ci: add lint job to PR workflow Adds a dedicated lint job that runs `make check` on every PR, covering ESLint, Prettier, tsc --noEmit (TypeScript) and ruff (Python). Previously, lint failures were only caught locally — now they block merge. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore(blueprint): add pyright strict type checking Adds pyright in strict mode to the Python nemoclaw-blueprint package, integrated into `make check`. This catches type errors locally and in CI, which is especially important for AI-agent-authored contributions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(lint): resolve ESLint errors and format issues - Remove async from functions without await in fetch.ts, use Promise.reject/resolve instead - Add braces to void arrow expression in logs.ts - Configure allowDefaultProject for test files excluded from tsconfig - Relax no-unsafe-assignment/member-access rules for test files - Fix unused variable in status.test.ts - Apply prettier formatting to all source files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * ci: trigger workflow re-run * fix(lint): update allowDefaultProject glob to match root-level test files The glob `src/*/*.test.ts` missed files like `src/register.test.ts`. Changed to `src/**/*.test.ts` to cover test files at any depth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(lint): use single-level globs in allowDefaultProject typescript-eslint disallows ** in allowDefaultProject globs for performance reasons. Use explicit single-level patterns instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: format register.test.ts with prettier Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

cv and others added 2 commits March 20, 2026 15:32

coderabbitai Bot reviewed Mar 20, 2026

View reviewed changes

cv self-assigned this Mar 21, 2026

coderabbitai Bot reviewed Mar 21, 2026

View reviewed changes

cv and others added 5 commits March 21, 2026 01:15

ci: trigger workflow re-run

3d9db64

fix(lint): update allowDefaultProject glob to match root-level test f…

cfeb898

…iles The glob `src/*/*.test.ts` missed files like `src/register.test.ts`. Changed to `src/**/*.test.ts` to cover test files at any depth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fix(lint): use single-level globs in allowDefaultProject

4e88791

typescript-eslint disallows ** in allowDefaultProject globs for performance reasons. Use explicit single-level patterns instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fix: format register.test.ts with prettier

622382e

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

ericksoa approved these changes Mar 21, 2026

View reviewed changes

ericksoa merged commit e52c2f0 into NVIDIA:main Mar 21, 2026
4 checks passed

cv deleted the chore/add-pyright branch March 21, 2026 17:04

ericksoa mentioned this pull request Mar 21, 2026

ci: add lint job to PR workflow #524

Closed

3 tasks

gn00295120 mentioned this pull request Mar 21, 2026

security: strip credentials from migration snapshots and enforce blueprint digest #156

Closed

3 tasks

wscurran added the chore Build, CI, dependency, or tooling maintenance label Jun 8, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(blueprint): add pyright strict type checking#523

chore(blueprint): add pyright strict type checking#523
ericksoa merged 8 commits into
NVIDIA:mainfrom
cv:chore/add-pyright

cv commented Mar 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 20, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

ericksoa left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cv commented Mar 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Test plan

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ericksoa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cv commented Mar 20, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 20, 2026 •

edited

Loading