fix(test): satisfy strict typecheck on Slack token validation tests by cjagwani · Pull Request #2458 · NVIDIA/NemoClaw

cjagwani · 2026-04-24T21:51:55Z

Summary

Tip of main currently fails npm run typecheck:cli with 8 errors in test/onboard.test.ts — patterns introduced by #2130 (Slack token validation tests) don't satisfy the strict: true setting introduced by #2422. Every PR opened against main inherits these errors, which makes the checks CI job red on unrelated PRs (e.g. #2454).

Changes

test/onboard.test.ts — 8 annotations, zero behavior change:

4× .pop()! — Array.pop() returns string | undefined. Each site is immediately preceded by an assert.equal(result.status, 0) that guarantees non-empty stdout, so the non-null assertion is safe.
3× (c: { key: string }) => c.key === … — saveCalls entries only read .key.
1× (c: { name: string }) => c.name === … — MESSAGING_CHANNELS entry lookup only reads .name.

The tests themselves still exercise the same cases; these changes are purely to make the TypeScript compiler happy.

Type of Change

Code change (feature, bug fix, or refactor)

Verification

npx tsc -p tsconfig.cli.json — exits 0 (was 2 with 8 errors)
npx prek run --all-files passes
npm test passes
Tests added or updated for new or changed behavior
No secrets, API keys, or credentials committed

AI Disclosure

AI-assisted — tool: Claude Code

Signed-off-by: Charan Jagwani cjagwani@nvidia.com

Summary by CodeRabbit

Tests

Improved reliability of JSON payload extraction in onboarding tests with defensive null-checks before parsing.
Enhanced type safety in test callbacks and iterators with explicit TypeScript type annotations.

#2130's tests on Slack bot/app token validation use patterns that fail tsc --strict introduced by #2422: Array.pop() returns string | undefined (TS2345 when passed to JSON.parse) and find/some callbacks needed explicit parameter types (TS7006). These errors are present on tip of main and cascade into every subsequent PR's CI typecheck. Fixes applied: - Add non-null assertion on .pop() after lines that already assert result.status === 0 with non-empty stdout (4 sites). - Annotate callback parameter types: saveCalls entries use { key: string }, MESSAGING_CHANNELS entries use { name: string } (4 sites). Zero behavior change; tests still cover the same cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>

coderabbitai · 2026-04-24T21:52:07Z

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: 1af79138-a5a6-4d29-b159-477b219a37d6

📥 Commits

Reviewing files that changed from the base of the PR and between 260b237 and 0e562e6.

📒 Files selected for processing (1)

test/onboard.test.ts

📝 Walkthrough

Walkthrough

Hardened JSON payload extraction in Slack onboarding tests by asserting non-null results before parsing. Added explicit TypeScript types for callback elements in test assertions to improve type safety and prevent potential null reference errors.

Changes

Cohort / File(s)	Summary
Test Safety Improvements `test/onboard.test.ts`	Added null assertions before `JSON.parse` operations on `split("\n").pop()` results and explicit TypeScript types for callback parameters in `saveCalls.some()` and `MESSAGING_CHANNELS.find()` predicates to strengthen type safety.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 With null assertions firm and true,
And types explicit through and through,
This rabbit's made your tests so sound,
Where safety's in each assertion found! 🛡️
JSON parsing, guarded well indeed!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing TypeScript strict-mode type-checking errors in the Slack token validation tests, which aligns with the PR's primary objective of satisfying strict typecheck requirements.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 fix/onboard-test-ts-strict

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

ericksoa

Approve — clean, minimal fix for a real CI blocker.

All 8 changes are correct:

.pop()! is safe at all 4 sites — each is guarded by assert.equal(result.status, 0) on the preceding line
{ key: string } / { name: string } inline annotations are the minimal shape needed for the callbacks

One note: PR #2227 added a // @ts-nocheck workaround for the same errors. Once this merges, #2227 should rebase and drop that workaround — the proper type annotations here are the better fix.

@ts-nocheck

PR #2458 properly annotated the callback parameters and .pop() calls that caused CI failures. Remove the blanket @ts-nocheck directive that was added as a temporary workaround.

## Summary Prevents the class of regression that required #2458. Adds an unconditional `typecheck:cli` step to the shared CI action so future hook-filter drift can't hide a tsc error. ## Related Follow-up to #2458 (which fixed the symptom — this addresses the root cause). ## Root cause recap - `.pre-commit-config.yaml` had `files: ^(bin|scripts)/` on the `tsc-cli` hook. - PRs that touched only `test/` or `src/` never triggered the check. - #2130 (test-only Slack validation) slipped through that gap. - #2422 later turned on `strict: true`, and the latent errors surfaced — but for every downstream PR, not the PR that introduced them. - Six open PRs ended up blocked on the same error cluster before anyone noticed. ## Changes `.github/actions/basic-checks/action.yaml` — new **Typecheck CLI + tests (strict)** step running `npm run typecheck:cli` unconditionally. Independent of prek hook configuration, so future filter drift can't hide a tsc error from CI. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Verification - [x] YAML parses - [x] `npm run typecheck:cli` exits 0 on this branch (requires #2458 merged to main first) - [x] Tests added or updated for new or changed behavior (N/A — infra change, existing suite covers) - [x] No secrets, API keys, or credentials committed ## AI Disclosure - [x] AI-assisted — tool: Claude Code --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>  ## Summary by CodeRabbit * **Chores** * Enhanced continuous integration pipeline with additional TypeScript typechecking to improve code quality assurance and detect type-related issues earlier in the development process.  Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…VIDIA#2458) ## Summary Tip of `main` currently fails `npm run typecheck:cli` with 8 errors in `test/onboard.test.ts` — patterns introduced by NVIDIA#2130 (Slack token validation tests) don't satisfy the `strict: true` setting introduced by NVIDIA#2422. Every PR opened against main inherits these errors, which makes the `checks` CI job red on unrelated PRs (e.g. NVIDIA#2454). ## Related Follows from NVIDIA#2130 + NVIDIA#2422 interaction. No linked issue — local-only breakage caught by CI on downstream PRs. ## Changes `test/onboard.test.ts` — 8 annotations, zero behavior change: - **4× `.pop()!`** — `Array.pop()` returns `string | undefined`. Each site is immediately preceded by an `assert.equal(result.status, 0)` that guarantees non-empty stdout, so the non-null assertion is safe. - **3× `(c: { key: string }) => c.key === …`** — `saveCalls` entries only read `.key`. - **1× `(c: { name: string }) => c.name === …`** — `MESSAGING_CHANNELS` entry lookup only reads `.name`. The tests themselves still exercise the same cases; these changes are purely to make the TypeScript compiler happy. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Verification - [x] `npx tsc -p tsconfig.cli.json` — exits 0 (was 2 with 8 errors) - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed ## AI Disclosure - [x] AI-assisted — tool: Claude Code --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>  ## Summary by CodeRabbit ## Tests * Improved reliability of JSON payload extraction in onboarding tests with defensive null-checks before parsing. * Enhanced type safety in test callbacks and iterators with explicit TypeScript type annotations.  Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

## Summary Prevents the class of regression that required NVIDIA#2458. Adds an unconditional `typecheck:cli` step to the shared CI action so future hook-filter drift can't hide a tsc error. ## Related Follow-up to NVIDIA#2458 (which fixed the symptom — this addresses the root cause). ## Root cause recap - `.pre-commit-config.yaml` had `files: ^(bin|scripts)/` on the `tsc-cli` hook. - PRs that touched only `test/` or `src/` never triggered the check. - NVIDIA#2130 (test-only Slack validation) slipped through that gap. - NVIDIA#2422 later turned on `strict: true`, and the latent errors surfaced — but for every downstream PR, not the PR that introduced them. - Six open PRs ended up blocked on the same error cluster before anyone noticed. ## Changes `.github/actions/basic-checks/action.yaml` — new **Typecheck CLI + tests (strict)** step running `npm run typecheck:cli` unconditionally. Independent of prek hook configuration, so future filter drift can't hide a tsc error from CI. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Verification - [x] YAML parses - [x] `npm run typecheck:cli` exits 0 on this branch (requires NVIDIA#2458 merged to main first) - [x] Tests added or updated for new or changed behavior (N/A — infra change, existing suite covers) - [x] No secrets, API keys, or credentials committed ## AI Disclosure - [x] AI-assisted — tool: Claude Code --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>  ## Summary by CodeRabbit * **Chores** * Enhanced continuous integration pipeline with additional TypeScript typechecking to improve code quality assurance and detect type-related issues earlier in the development process.  Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…VIDIA#2458) ## Summary Tip of `main` currently fails `npm run typecheck:cli` with 8 errors in `test/onboard.test.ts` — patterns introduced by NVIDIA#2130 (Slack token validation tests) don't satisfy the `strict: true` setting introduced by NVIDIA#2422. Every PR opened against main inherits these errors, which makes the `checks` CI job red on unrelated PRs (e.g. NVIDIA#2454). ## Related Follows from NVIDIA#2130 + NVIDIA#2422 interaction. No linked issue — local-only breakage caught by CI on downstream PRs. ## Changes `test/onboard.test.ts` — 8 annotations, zero behavior change: - **4× `.pop()!`** — `Array.pop()` returns `string | undefined`. Each site is immediately preceded by an `assert.equal(result.status, 0)` that guarantees non-empty stdout, so the non-null assertion is safe. - **3× `(c: { key: string }) => c.key === …`** — `saveCalls` entries only read `.key`. - **1× `(c: { name: string }) => c.name === …`** — `MESSAGING_CHANNELS` entry lookup only reads `.name`. The tests themselves still exercise the same cases; these changes are purely to make the TypeScript compiler happy. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Verification - [x] `npx tsc -p tsconfig.cli.json` — exits 0 (was 2 with 8 errors) - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed ## AI Disclosure - [x] AI-assisted — tool: Claude Code --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>  ## Summary by CodeRabbit ## Tests * Improved reliability of JSON payload extraction in onboarding tests with defensive null-checks before parsing. * Enhanced type safety in test callbacks and iterators with explicit TypeScript type annotations.  Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

## Summary Prevents the class of regression that required NVIDIA#2458. Adds an unconditional `typecheck:cli` step to the shared CI action so future hook-filter drift can't hide a tsc error. ## Related Follow-up to NVIDIA#2458 (which fixed the symptom — this addresses the root cause). ## Root cause recap - `.pre-commit-config.yaml` had `files: ^(bin|scripts)/` on the `tsc-cli` hook. - PRs that touched only `test/` or `src/` never triggered the check. - NVIDIA#2130 (test-only Slack validation) slipped through that gap. - NVIDIA#2422 later turned on `strict: true`, and the latent errors surfaced — but for every downstream PR, not the PR that introduced them. - Six open PRs ended up blocked on the same error cluster before anyone noticed. ## Changes `.github/actions/basic-checks/action.yaml` — new **Typecheck CLI + tests (strict)** step running `npm run typecheck:cli` unconditionally. Independent of prek hook configuration, so future filter drift can't hide a tsc error from CI. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Verification - [x] YAML parses - [x] `npm run typecheck:cli` exits 0 on this branch (requires NVIDIA#2458 merged to main first) - [x] Tests added or updated for new or changed behavior (N/A — infra change, existing suite covers) - [x] No secrets, API keys, or credentials committed ## AI Disclosure - [x] AI-assisted — tool: Claude Code --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>  ## Summary by CodeRabbit * **Chores** * Enhanced continuous integration pipeline with additional TypeScript typechecking to improve code quality assurance and detect type-related issues earlier in the development process.  Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

cjagwani mentioned this pull request Apr 24, 2026

ci: gate every PR on tsc-cli regardless of touched paths #2460

Closed

7 tasks

ericksoa approved these changes Apr 24, 2026

View reviewed changes

ericksoa merged commit f41f5ec into main Apr 24, 2026
19 checks passed

cjagwani mentioned this pull request Apr 24, 2026

ci: gate every PR on tsc-cli regardless of touched paths #2461

Merged

6 tasks

ericksoa added a commit that referenced this pull request Apr 24, 2026

merge(main): pick up strict typecheck fixes from #2458

05415af

cjagwani self-assigned this May 5, 2026

wscurran added the bug-fix PR fixes a bug or regression label Jun 8, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(test): satisfy strict typecheck on Slack token validation tests#2458

fix(test): satisfy strict typecheck on Slack token validation tests#2458
ericksoa merged 1 commit into
mainfrom
fix/onboard-test-ts-strict

cjagwani 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

Uh oh!

ericksoa left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Summary

Related

Changes

Type of Change

Verification

AI Disclosure

Summary by CodeRabbit

Tests

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

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

cjagwani commented Apr 24, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 24, 2026 •

edited

Loading