Skip to content

fix(cli): remove unreachable plugin modules and associated tests part 1#1030

Open
ksapru wants to merge 1 commit intoNVIDIA:mainfrom
ksapru:refactor/remove-unused-exports
Open

fix(cli): remove unreachable plugin modules and associated tests part 1#1030
ksapru wants to merge 1 commit intoNVIDIA:mainfrom
ksapru:refactor/remove-unused-exports

Conversation

@ksapru
Copy link
Copy Markdown

@ksapru ksapru commented Mar 27, 2026

Summary

Removes unused plugin modules that are compiled into dist/ but are not reachable from the plugin entry point, reducing package size and maintenance surface.

Related Issue

Fixes #977 (part 1)

Changes

  • Remove unused modules:
    • blueprint/runner.ts
    • blueprint/ssrf.ts
  • Remove associated test files and unused imports in the credential file
  • Verified no remaining static or dynamic import paths to removed modules

Verification

  • Confirmed no static imports from the plugin entry point (index.ts) or its dependency graph
  • Searched for dynamic imports (import(), require) — none reference these modules
  • Verified plugin manifest exposes a single entry point (dist/index.js)
  • tsc build succeeds with no missing references
  • Full test suite passes with no behavioral regressions

Rationale

These modules are not currently wired into any CLI command or plugin entrypoint.
Keeping them increases package size and maintenance surface without providing runtime functionality.

If future work requires these features, they can be reintroduced behind explicit entry points.

Risk Assessment

Low risk:

  • Modules are not imported anywhere in the runtime path
  • No CLI commands depend on these modules
  • No dynamic loading mechanisms are present

Rollback:

  • Changes are isolated and can be reverted by restoring the removed files

Type of Change

  • Code change for a new feature, bug fix, or refactor.
  • Code change with doc updates.
  • Doc only. Prose changes without code sample modifications.
  • Doc only. Includes code sample changes.

Testing

  • npx prek run --all-files passes (or equivalently make check).
  • npm test passes.
  • make docs builds without warnings. (for doc-only changes)

Checklist

General

Code Changes

  • Formatters applied — npx prek run --all-files auto-fixes formatting (or make format for targeted runs).
  • Tests added or updated for new or changed behavior.
  • No secrets, API keys, or credentials committed.
  • Doc pages updated for any user-facing behavior changes (new commands, changed defaults, new features, bug fixes that contradict existing docs).

Doc Changes

  • Follows the style guide.
  • New pages include SPDX license header and frontmatter, if creating a new page.
  • Cross-references and links verified.

Summary by CodeRabbit

  • Refactor
    • Removed the standalone Blueprint runner CLI and its sandbox orchestration commands (those CLI actions are no longer available).
  • Chores
    • Removed endpoint validation utilities that enforced private/internal IP checks for provided endpoints.
  • Tests
    • Deleted multiple test suites covering runner behavior, SSRF/endpoint validation, and a runner-related credential-exposure check; remaining credential tests focus on onboarding.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4cfc07fb-613f-4ee0-b233-34d852a86bc5

📥 Commits

Reviewing files that changed from the base of the PR and between d5a807f and a8587b6.

📒 Files selected for processing (5)
  • nemoclaw/src/blueprint/runner.test.ts
  • nemoclaw/src/blueprint/runner.ts
  • nemoclaw/src/blueprint/ssrf.test.ts
  • nemoclaw/src/blueprint/ssrf.ts
  • test/credential-exposure.test.js
💤 Files with no reviewable changes (5)
  • test/credential-exposure.test.js
  • nemoclaw/src/blueprint/ssrf.ts
  • nemoclaw/src/blueprint/ssrf.test.ts
  • nemoclaw/src/blueprint/runner.test.ts
  • nemoclaw/src/blueprint/runner.ts

📝 Walkthrough

Walkthrough

Removed the unreachable Blueprint runner and SSRF modules plus their associated Vitest tests, and deleted a runner-specific credential-exposure test case; remaining code and tests unaffected.

Changes

Cohort / File(s) Summary
Blueprint Runner
nemoclaw/src/blueprint/runner.ts, nemoclaw/src/blueprint/runner.test.ts
Deleted the CLI runner implementation and its entire test suite — removed plan/apply/status/rollback handlers, run-id/progress stdout contracts, blueprint loading and endpoint override/SSRF checks, and CLI parsing tests.
SSRF Validation
nemoclaw/src/blueprint/ssrf.ts, nemoclaw/src/blueprint/ssrf.test.ts
Deleted IP classification and endpoint URL validation logic plus tests that covered DNS resolution, CIDR/private-IP checks, and scheme/hostname validation.
Credential-exposure test cleanup
test/credential-exposure.test.js
Removed the runner.ts–targeted credential-exposure assertion and related regex/template checks; retained onboard-related credential exposure tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nudged the tree and watched code fall,

Old branches cleared to tidy the hall.
Fewer footprints on the build and test,
A lighter hop and well-earned rest.

🚥 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 accurately describes the main change: removing unreachable plugin modules (blueprint/runner.ts, blueprint/ssrf.ts) and associated tests as part of a larger refactoring effort.
Linked Issues check ✅ Passed The PR fully addresses the coding requirements from issue #977: removes blueprint/runner.ts, blueprint/ssrf.ts, and their test files; updates credential-exposure.test.js to remove runner.ts references; verifies no remaining imports reference removed modules; TypeScript build succeeds.
Out of Scope Changes check ✅ Passed All changes are directly scoped to removing unreachable modules identified in issue #977 and their associated tests; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

@ksapru ksapru force-pushed the refactor/remove-unused-exports branch from fdcd53b to d5a807f Compare March 27, 2026 21:18
@ksapru ksapru force-pushed the refactor/remove-unused-exports branch from d5a807f to a8587b6 Compare March 27, 2026 21:29
@ksapru
Copy link
Copy Markdown
Author

ksapru commented Mar 27, 2026

Hi! Thanks again for reviewing and approving the PR.

It looks like there are 3 workflows awaiting approval — just wanted to check if anything is needed from my side to unblock them.

Happy to make any additional changes if required!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dead plugin modules: 4 TypeScript files ship in dist/ but are unreachable at runtime

2 participants