Update CLI implementation with modern tooling and conventions #1456

relu91 · 2025-11-28T15:15:46Z

I know that this PR will be long to review, but it has been sitting on my pc for a while, and it is time to push the changes. The primary goal was to get rid of the VM2 module, which is now deprecated and audited as a critical security risk. I used the opportunity to completely revise the implementation and make it:

Hopefully easier to maintain
More useful and self-explanatory

In summary, this PR delivers a significant refactor and several quality-of-life improvements to the cli module, primarily focusing on simplifying configuration, execution, and improving code organization.

Key Changes and Improvements

Configuration Unification: Environment variables are now consistently treated as configuration parameters, simplifying how settings are managed and loaded for the CLI execution.
- ⚠️ Breaking Change: Now you should prefix every env with NODE_WOT_ and the cli automatically map that variable to a property in the configuration schema (e.g., WOT_SERVIENT_SERVIENT_CLIENTONLY -> servient.clientOnly)
Execution Simplification: Simplified the CLI script execution flow; now we simply require the files, avoiding dependence from virtual machines or complicated compilation steps. Virtualization can always be achieved with other means.
- ⚠️ Breaking Change: This drops support for running remote scripts from the Default Servient Thing, a feature that was rarely used and controversial due to security concerns.
Major Refactoring and Testing:
- Completed a major refactor of configuration building, configuration sync with the JSON schema, and logging organization.
- Improved test coverage for the new configuration and parsing functionalities.
MQTT Fix : Allows for optional URI in the MQTT Server configuration.
Audit fix: Now, when you install node-wot, you should not see any audit warnings.

Open Point for Discussion

Given the extensive changes and the focus on the primary user experience:

Should we rename the CLI tool? The current name, wot-servient, is potentially surprising for regular users as it differs from the package name. I propose renaming it to node-wot to align with the project name and make it feel less surprising.

cli.ts log now enables by default error and warning messages.

As a result the responsibility of managing the initial configuration for logging has been moved outside the DefaultServient.

…son schema

WARNING: Drops support for running remote scripts from the Default Servient Thing. This feature was rarely used and controversial due to security implications. Users wanting to run remote scripts can easily implement this feature in their own Servient by extending the CLI Servient.

now envs are treated as config parameters

relu91 · 2025-12-01T14:52:02Z

There is a small problem in the CLI tests due to some file shared across different unit tests. I'll fix them asap. They should not require changes to the business logic of the new CLI.

danielpeintner · 2025-12-02T10:07:30Z

packages/binding-mqtt/src/mqtt-broker-server.ts

    private async startBroker() {
        return new Promise<void>((resolve, reject) => {
+            if (this.brokerURI == null) {
+                throw new Error("Unexpected configuration state broker was started even if brokerURI is null");


Suggested change

throw new Error("Unexpected configuration state broker was started even if brokerURI is null");

throw new Error("Unexpected configuration state. Broker was started but brokerURI is null");

danielpeintner · 2025-12-02T10:12:01Z

packages/cli/src/parsers/config-file-parser.ts

+        if (err instanceof InvalidArgumentError) {
+            throw err;
+        }
+        throw new InvalidArgumentError(`\nError reading config file: ${err}`);


Suggested change

throw new InvalidArgumentError(`\nError reading config file: ${err}`);

throw new InvalidArgumentError(`Error reading config file: ${err}`);

I don't think we should start with a new line... any reason for that?

danielpeintner · 2025-12-02T10:15:39Z

packages/cli/src/parsers/ip-parser.ts

+export function parseIp(value: string, previous: string) {
+    if (!/^([a-z]*|[\d.]*)(:[0-9]{2,5})?$/.test(value)) {
+        throw new InvalidArgumentError("Invalid host:port combo");
+    }
+
+    return value;
+}


Several comments:

Parameter previous is never used

Does it work with ipv6

do we need the code at all? To me, if a wrong IP is given it should/will fail somewhere else and it is not our job to test valid IPs..

We can remove it, but this code is mainly used to parse and validate the inputs for --inspect and --inspect-brk and it might be useful to fail early rather than brak the nodejs debugger. Nonetheless, I agree with you that the code is not that great as it doesn't cover ipv6... we can remove it for now.

danielpeintner

Note: I did not fully check nor run it locally, but I would like to make some comments:

THANK YOU for the effort. I think it is very helpful to get rid of VM2
I am surprised that no file has been removed (can it be?)
some more comments, see below

danielpeintner · 2025-12-02T10:20:43Z

packages/cli/.gitignore

@@ -0,0 +1,2 @@
+src/generated
+test/resources


Shouldn't we use the global .gitignore?

Moreover, we add test/resources which in turn says .gitkeep with no files? Is the empty folder needed!?

I think it is better to ignore them in this subfolder as they are specific files for this package. About .gitkeep I used another (more solid) approach we can remove it once the tests pass.

danielpeintner · 2025-12-02T10:23:35Z

packages/cli/package.json

    },
    "scripts": {
-        "build": "tsc -b",
+        "build:json": "node import-json.js",


May I suggest renaming the file to something like
import-json-configuration.js ... or even leave json out.

Initially, I wasn't sure what kind of json gets imported!?

"build:json" --> I don't think the name conveys what is done...

The script has two purposes:

import package.json so that we can use the version field safely

import configuration schema and generate its corresponding ts file

any better naming suggestions? build:transform ? and the file name import-json-to-ts ?

relu91 added 12 commits November 28, 2025 11:15

refactor(cli): drop vm2 and use NodeJS vm module

e78447b

refactor(cli/default-serivent): use async await in run method

abb813c

refactor(cli): better code organization and minor improvements logging

ee16e04

cli.ts log now enables by default error and warning messages.

refactor(cli): organize logging configuration

e493e36

As a result the responsibility of managing the initial configuration for logging has been moved outside the DefaultServient.

refactor(cli/parsers/config-file-parser): better error messages

6f5953c

refactor(cli): improve configuration building and keep it sync with j…

861bcad

…son schema

fix(mqtt): allow optional uri in MQTT Server configuration

7bb51f3

feat(cli): unify configuration parameters

7269236

now envs are treated as config parameters

chore: remove testing files

1f1d88e

test(cli): cover new functionalities with basic tests

28c8a6f

chore: fix critical audit warnings

93cd428

relu91 requested review from danielpeintner, egekorkan and sebastiankb as code owners November 28, 2025 15:15

relu91 added 2 commits November 28, 2025 16:37

chore: fix eslint warnings

f116cbc

style: run format

5718765

relu91 mentioned this pull request Nov 29, 2025

refactor: eslint no-process-exit, turn off warning in CLI #1433

Open

danielpeintner reviewed Dec 2, 2025

View reviewed changes

test(cli): use tmp library for script files

e5ddb9b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update CLI implementation with modern tooling and conventions #1456

Update CLI implementation with modern tooling and conventions #1456

Uh oh!

relu91 commented Nov 28, 2025 •

edited

Loading

Uh oh!

relu91 commented Dec 1, 2025

Uh oh!

danielpeintner Dec 2, 2025 •

edited

Loading

Uh oh!

danielpeintner Dec 2, 2025

Uh oh!

danielpeintner Dec 2, 2025

Uh oh!

relu91 Dec 18, 2025

Uh oh!

danielpeintner left a comment

Uh oh!

danielpeintner Dec 2, 2025

Uh oh!

relu91 Dec 18, 2025

Uh oh!

danielpeintner Dec 2, 2025

Uh oh!

relu91 Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	throw new Error("Unexpected configuration state broker was started even if brokerURI is null");
	throw new Error("Unexpected configuration state. Broker was started but brokerURI is null");

	throw new InvalidArgumentError(`\nError reading config file: ${err}`);
	throw new InvalidArgumentError(`Error reading config file: ${err}`);

Update CLI implementation with modern tooling and conventions #1456

Are you sure you want to change the base?

Update CLI implementation with modern tooling and conventions #1456

Uh oh!

Conversation

relu91 commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key Changes and Improvements

Open Point for Discussion

Uh oh!

relu91 commented Dec 1, 2025

Uh oh!

danielpeintner Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielpeintner Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

danielpeintner Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

relu91 Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

danielpeintner left a comment

Choose a reason for hiding this comment

Uh oh!

danielpeintner Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

relu91 Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

danielpeintner Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

relu91 Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

relu91 commented Nov 28, 2025 •

edited

Loading

danielpeintner Dec 2, 2025 •

edited

Loading